| Message ID | 20211101232144.134590-5-djrscally@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for OV5693 sensor | expand |
Hi, On 11/2/21 00:21, Daniel Scally wrote: > From: Hans de Goede <hdegoede@redhat.com> > > ov5693_sw_standby() starts/stops streaming rename it so that it is actually > named after what it does. > > This also removes the weird enable inverting going on before. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/media/i2c/ov5693.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c > index 8ad51f8f88cf..2613bad49f78 100644 > --- a/drivers/media/i2c/ov5693.c > +++ b/drivers/media/i2c/ov5693.c > @@ -742,13 +742,13 @@ static int ov5693_mode_configure(struct ov5693_device *ov5693) > return ret; > } > > -static int ov5693_sw_standby(struct ov5693_device *ov5693, bool standby) > +static int ov5693_enable_streaming(struct ov5693_device *ov5693, bool enable) > { > int ret = 0; > > ov5693_write_reg(ov5693, OV5693_SW_STREAM_REG, > - standby ? OV5693_STOP_STREAMING : > - OV5693_START_STREAMING, &ret); > + enable ? OV5693_STOP_STREAMING : > + OV5693_START_STREAMING, &ret); In this version of this patch the function still behaves as if it is setting the sensor in a standby mode, my original version had this: ov5693_write_reg(ov5693, OV5693_SW_STREAM_REG, enable ? OV5693_START_STREAMING : OV5693_STOP_STREAMING, &ret); Which makes the function behave more logical. > > return ret; > } > @@ -784,9 +784,9 @@ static int ov5693_sensor_init(struct ov5693_device *ov5693) > return ret; > } > > - ret = ov5693_sw_standby(ov5693, true); > + ret = ov5693_enable_streaming(ov5693, true); And my original version changes the true to false here, stopping streaming on init (as before when setting standby to true, including the error messages being different: ret = ov5693_enable_streaming(ov5693, false); if (ret) dev_err(ov5693->dev, "%s stop streaming error\n", __func__); > if (ret) > - dev_err(ov5693->dev, "software standby error\n"); > + dev_err(ov5693->dev, "enable streaming error\n"); > > return ret; > } > @@ -1119,7 +1119,7 @@ static int ov5693_s_stream(struct v4l2_subdev *sd, int enable) > } > > mutex_lock(&ov5693->lock); > - ret = ov5693_sw_standby(ov5693, enable); > + ret = ov5693_enable_streaming(ov5693, enable); And this used to be a change from !enable -> enable. Note that the current version cannot work, you pass enable (instead of !enable) but ov5693_enable_streaming() still sends OV5693_STOP_STREAMING if enable is true, so you are now stopping streaming when called to enable it ? > mutex_unlock(&ov5693->lock); > > if (ret) > Besides this needing some work, it is fine with me, and I believe that it is best, to just squash this and patch 5/5 into patch 2 (since your introducing this driver in this series it is a bit to then apply fixes to it in the same series). Regards, Hans
Hi Hans On 02/11/2021 09:24, Hans de Goede wrote: > Hi, > > On 11/2/21 00:21, Daniel Scally wrote: >> From: Hans de Goede <hdegoede@redhat.com> >> >> ov5693_sw_standby() starts/stops streaming rename it so that it is actually >> named after what it does. >> >> This also removes the weird enable inverting going on before. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/media/i2c/ov5693.c | 12 ++++++------ >> 1 file changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c >> index 8ad51f8f88cf..2613bad49f78 100644 >> --- a/drivers/media/i2c/ov5693.c >> +++ b/drivers/media/i2c/ov5693.c >> @@ -742,13 +742,13 @@ static int ov5693_mode_configure(struct ov5693_device *ov5693) >> return ret; >> } >> >> -static int ov5693_sw_standby(struct ov5693_device *ov5693, bool standby) >> +static int ov5693_enable_streaming(struct ov5693_device *ov5693, bool enable) >> { >> int ret = 0; >> >> ov5693_write_reg(ov5693, OV5693_SW_STREAM_REG, >> - standby ? OV5693_STOP_STREAMING : >> - OV5693_START_STREAMING, &ret); >> + enable ? OV5693_STOP_STREAMING : >> + OV5693_START_STREAMING, &ret); > In this version of this patch the function still behaves as if it is > setting the sensor in a standby mode, my original version had this: > > ov5693_write_reg(ov5693, OV5693_SW_STREAM_REG, > enable ? OV5693_START_STREAMING : OV5693_STOP_STREAMING, > &ret); > > Which makes the function behave more logical. > > >> >> return ret; >> } >> @@ -784,9 +784,9 @@ static int ov5693_sensor_init(struct ov5693_device *ov5693) >> return ret; >> } >> >> - ret = ov5693_sw_standby(ov5693, true); >> + ret = ov5693_enable_streaming(ov5693, true); > And my original version changes the true to false here, stopping > streaming on init (as before when setting standby to true, > including the error messages being different: > > ret = ov5693_enable_streaming(ov5693, false); > if (ret) > dev_err(ov5693->dev, "%s stop streaming error\n", __func__); > > > >> if (ret) >> - dev_err(ov5693->dev, "software standby error\n"); >> + dev_err(ov5693->dev, "enable streaming error\n"); >> >> return ret; >> } >> @@ -1119,7 +1119,7 @@ static int ov5693_s_stream(struct v4l2_subdev *sd, int enable) >> } >> >> mutex_lock(&ov5693->lock); >> - ret = ov5693_sw_standby(ov5693, enable); >> + ret = ov5693_enable_streaming(ov5693, enable); > And this used to be a change from !enable -> enable. > > Note that the current version cannot work, you pass enable > (instead of !enable) but ov5693_enable_streaming() still > sends OV5693_STOP_STREAMING if enable is true, so you are > now stopping streaming when called to enable it ? > >> mutex_unlock(&ov5693->lock); >> >> if (ret) >> > Besides this needing some work, it is fine with me, and > I believe that it is best, to just squash this and > patch 5/5 into patch 2 (since your introducing this > driver in this series it is a bit to then apply fixes > to it in the same series). OK; I'll squash them then. Sorry to have messed up the application too! The patch wouldn't apply cleanly due to other changes I'd made, and apparently I wasn't nearly as careful sorting it as I thought I had been. > > Regards, > > Hans > >
diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c index 8ad51f8f88cf..2613bad49f78 100644 --- a/drivers/media/i2c/ov5693.c +++ b/drivers/media/i2c/ov5693.c @@ -742,13 +742,13 @@ static int ov5693_mode_configure(struct ov5693_device *ov5693) return ret; } -static int ov5693_sw_standby(struct ov5693_device *ov5693, bool standby) +static int ov5693_enable_streaming(struct ov5693_device *ov5693, bool enable) { int ret = 0; ov5693_write_reg(ov5693, OV5693_SW_STREAM_REG, - standby ? OV5693_STOP_STREAMING : - OV5693_START_STREAMING, &ret); + enable ? OV5693_STOP_STREAMING : + OV5693_START_STREAMING, &ret); return ret; } @@ -784,9 +784,9 @@ static int ov5693_sensor_init(struct ov5693_device *ov5693) return ret; } - ret = ov5693_sw_standby(ov5693, true); + ret = ov5693_enable_streaming(ov5693, true); if (ret) - dev_err(ov5693->dev, "software standby error\n"); + dev_err(ov5693->dev, "enable streaming error\n"); return ret; } @@ -1119,7 +1119,7 @@ static int ov5693_s_stream(struct v4l2_subdev *sd, int enable) } mutex_lock(&ov5693->lock); - ret = ov5693_sw_standby(ov5693, enable); + ret = ov5693_enable_streaming(ov5693, enable); mutex_unlock(&ov5693->lock); if (ret)