| Message ID | 20230627131830.54601-4-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ov2680: Bugfixes + ACPI + selection(crop-tgt) API support | expand |
Hi Hans On Tue, Jun 27, 2023 at 03:18:04PM +0200, Hans de Goede wrote: > ov2680_vflip_disable() / ov2680_hflip_disable() pass BIT(0) instead of > 0 as value to ov2680_mod_reg(). > > While fixing this also: > > 1. Stop having separate enable/disable functions for hflip / vflip > 2. Move the is_streaming check, which is unique to hflip / vflip > into the ov2680_set_?flip() functions. The patch looks good, but one little question on the controls update procedure. Usually s_ctrl() handlers checks for the sensor power state, like the driver here is already doing by testing the is_enabled[*] flag, but they usually call __v4l2_ctrl_handler_setup() unconditionally at s_stream(1) time, not only if a new mode has been applied by calling s_fmt(). Controls could be updated by userspace while the sensor is powered off, and new values should be applied regardless if a new mode, has been applied or not. Does it make sense to you ? [*] or better, if the sensor is ported to use pm_runtime first (by dropping support for the deprecated .s_power(), or do you need s_power() on your platform ?) you can use pm_runtime_get_if_in_use() instead of keeping track manually of the is_enabled flag. > > for a nice code cleanup. > > Fixes: 3ee47cad3e69 ("media: ov2680: Add Omnivision OV2680 sensor driver") > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> > Acked-by: Rui Miguel Silva <rmfrfs@gmail.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/media/i2c/ov2680.c | 50 ++++++++++---------------------------- > 1 file changed, 13 insertions(+), 37 deletions(-) > > diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c > index 2001e08253ef..c93810f84ed7 100644 > --- a/drivers/media/i2c/ov2680.c > +++ b/drivers/media/i2c/ov2680.c > @@ -328,11 +328,15 @@ static void ov2680_set_bayer_order(struct ov2680_dev *sensor) > sensor->fmt.code = ov2680_hv_flip_bayer_order[hv_flip]; > } > > -static int ov2680_vflip_enable(struct ov2680_dev *sensor) > +static int ov2680_set_vflip(struct ov2680_dev *sensor, s32 val) > { > int ret; > > - ret = ov2680_mod_reg(sensor, OV2680_REG_FORMAT1, BIT(2), BIT(2)); > + if (sensor->is_streaming) > + return -EBUSY; > + > + ret = ov2680_mod_reg(sensor, OV2680_REG_FORMAT1, > + BIT(2), val ? BIT(2) : 0); > if (ret < 0) > return ret; > > @@ -340,33 +344,15 @@ static int ov2680_vflip_enable(struct ov2680_dev *sensor) > return 0; > } > > -static int ov2680_vflip_disable(struct ov2680_dev *sensor) > +static int ov2680_set_hflip(struct ov2680_dev *sensor, s32 val) > { > int ret; > > - ret = ov2680_mod_reg(sensor, OV2680_REG_FORMAT1, BIT(2), BIT(0)); > - if (ret < 0) > - return ret; > + if (sensor->is_streaming) > + return -EBUSY; > > - return ov2680_bayer_order(sensor); > -} > - > -static int ov2680_hflip_enable(struct ov2680_dev *sensor) > -{ > - int ret; > - > - ret = ov2680_mod_reg(sensor, OV2680_REG_FORMAT2, BIT(2), BIT(2)); > - if (ret < 0) > - return ret; > - > - return ov2680_bayer_order(sensor); > -} > - > -static int ov2680_hflip_disable(struct ov2680_dev *sensor) > -{ > - int ret; > - > - ret = ov2680_mod_reg(sensor, OV2680_REG_FORMAT2, BIT(2), BIT(0)); > + ret = ov2680_mod_reg(sensor, OV2680_REG_FORMAT2, > + BIT(2), val ? BIT(2) : 0); > if (ret < 0) > return ret; > > @@ -720,19 +706,9 @@ static int ov2680_s_ctrl(struct v4l2_ctrl *ctrl) > case V4L2_CID_EXPOSURE: > return ov2680_exposure_set(sensor, ctrl->val); > case V4L2_CID_VFLIP: > - if (sensor->is_streaming) > - return -EBUSY; > - if (ctrl->val) > - return ov2680_vflip_enable(sensor); > - else > - return ov2680_vflip_disable(sensor); > + return ov2680_set_vflip(sensor, ctrl->val); > case V4L2_CID_HFLIP: > - if (sensor->is_streaming) > - return -EBUSY; > - if (ctrl->val) > - return ov2680_hflip_enable(sensor); > - else > - return ov2680_hflip_disable(sensor); > + return ov2680_set_hflip(sensor, ctrl->val); > case V4L2_CID_TEST_PATTERN: > return ov2680_test_pattern_set(sensor, ctrl->val); > default: > -- > 2.41.0 >
Hi Jacopo, On 6/27/23 16:35, Jacopo Mondi wrote: > Hi Hans > > On Tue, Jun 27, 2023 at 03:18:04PM +0200, Hans de Goede wrote: >> ov2680_vflip_disable() / ov2680_hflip_disable() pass BIT(0) instead of >> 0 as value to ov2680_mod_reg(). >> >> While fixing this also: >> >> 1. Stop having separate enable/disable functions for hflip / vflip >> 2. Move the is_streaming check, which is unique to hflip / vflip >> into the ov2680_set_?flip() functions. > > The patch looks good, but one little question on the controls update > procedure. > > Usually s_ctrl() handlers checks for the sensor power state, like the > driver here is already doing by testing the is_enabled[*] flag, but they > usually call __v4l2_ctrl_handler_setup() unconditionally at > s_stream(1) time, not only if a new mode has been applied by calling > s_fmt(). Controls could be updated by userspace while the sensor is > powered off, and new values should be applied regardless if a new mode, > has been applied or not. > > Does it make sense to you ? > > [*] or better, if the sensor is ported to use pm_runtime first (by > dropping support for the deprecated .s_power(), or do you need > s_power() on your platform ?) you can use pm_runtime_get_if_in_use() > instead of keeping track manually of the is_enabled flag. What you are describing above is pretty much exactly what is done later in this patchset. Patches 1-8 are there to fix some glaring bugs, with the possibility of backporting the fixes which is why they are the first patches in the set. Then after patch 9 the real work to get this driver into shape begins :) Regards, Hans >> for a nice code cleanup. >> >> Fixes: 3ee47cad3e69 ("media: ov2680: Add Omnivision OV2680 sensor driver") >> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> >> Acked-by: Rui Miguel Silva <rmfrfs@gmail.com> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/media/i2c/ov2680.c | 50 ++++++++++---------------------------- >> 1 file changed, 13 insertions(+), 37 deletions(-) >> >> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c >> index 2001e08253ef..c93810f84ed7 100644 >> --- a/drivers/media/i2c/ov2680.c >> +++ b/drivers/media/i2c/ov2680.c >> @@ -328,11 +328,15 @@ static void ov2680_set_bayer_order(struct ov2680_dev *sensor) >> sensor->fmt.code = ov2680_hv_flip_bayer_order[hv_flip]; >> } >> >> -static int ov2680_vflip_enable(struct ov2680_dev *sensor) >> +static int ov2680_set_vflip(struct ov2680_dev *sensor, s32 val) >> { >> int ret; >> >> - ret = ov2680_mod_reg(sensor, OV2680_REG_FORMAT1, BIT(2), BIT(2)); >> + if (sensor->is_streaming) >> + return -EBUSY; >> + >> + ret = ov2680_mod_reg(sensor, OV2680_REG_FORMAT1, >> + BIT(2), val ? BIT(2) : 0); >> if (ret < 0) >> return ret; >> >> @@ -340,33 +344,15 @@ static int ov2680_vflip_enable(struct ov2680_dev *sensor) >> return 0; >> } >> >> -static int ov2680_vflip_disable(struct ov2680_dev *sensor) >> +static int ov2680_set_hflip(struct ov2680_dev *sensor, s32 val) >> { >> int ret; >> >> - ret = ov2680_mod_reg(sensor, OV2680_REG_FORMAT1, BIT(2), BIT(0)); >> - if (ret < 0) >> - return ret; >> + if (sensor->is_streaming) >> + return -EBUSY; >> >> - return ov2680_bayer_order(sensor); >> -} >> - >> -static int ov2680_hflip_enable(struct ov2680_dev *sensor) >> -{ >> - int ret; >> - >> - ret = ov2680_mod_reg(sensor, OV2680_REG_FORMAT2, BIT(2), BIT(2)); >> - if (ret < 0) >> - return ret; >> - >> - return ov2680_bayer_order(sensor); >> -} >> - >> -static int ov2680_hflip_disable(struct ov2680_dev *sensor) >> -{ >> - int ret; >> - >> - ret = ov2680_mod_reg(sensor, OV2680_REG_FORMAT2, BIT(2), BIT(0)); >> + ret = ov2680_mod_reg(sensor, OV2680_REG_FORMAT2, >> + BIT(2), val ? BIT(2) : 0); >> if (ret < 0) >> return ret; >> >> @@ -720,19 +706,9 @@ static int ov2680_s_ctrl(struct v4l2_ctrl *ctrl) >> case V4L2_CID_EXPOSURE: >> return ov2680_exposure_set(sensor, ctrl->val); >> case V4L2_CID_VFLIP: >> - if (sensor->is_streaming) >> - return -EBUSY; >> - if (ctrl->val) >> - return ov2680_vflip_enable(sensor); >> - else >> - return ov2680_vflip_disable(sensor); >> + return ov2680_set_vflip(sensor, ctrl->val); >> case V4L2_CID_HFLIP: >> - if (sensor->is_streaming) >> - return -EBUSY; >> - if (ctrl->val) >> - return ov2680_hflip_enable(sensor); >> - else >> - return ov2680_hflip_disable(sensor); >> + return ov2680_set_hflip(sensor, ctrl->val); >> case V4L2_CID_TEST_PATTERN: >> return ov2680_test_pattern_set(sensor, ctrl->val); >> default: >> -- >> 2.41.0 >> >
Hi Hans On Tue, Jun 27, 2023 at 04:38:50PM +0200, Hans de Goede wrote: > Hi Jacopo, > > On 6/27/23 16:35, Jacopo Mondi wrote: > > Hi Hans > > > > On Tue, Jun 27, 2023 at 03:18:04PM +0200, Hans de Goede wrote: > >> ov2680_vflip_disable() / ov2680_hflip_disable() pass BIT(0) instead of > >> 0 as value to ov2680_mod_reg(). > >> > >> While fixing this also: > >> > >> 1. Stop having separate enable/disable functions for hflip / vflip > >> 2. Move the is_streaming check, which is unique to hflip / vflip > >> into the ov2680_set_?flip() functions. > > > > The patch looks good, but one little question on the controls update > > procedure. > > > > Usually s_ctrl() handlers checks for the sensor power state, like the > > driver here is already doing by testing the is_enabled[*] flag, but they > > usually call __v4l2_ctrl_handler_setup() unconditionally at > > s_stream(1) time, not only if a new mode has been applied by calling > > s_fmt(). Controls could be updated by userspace while the sensor is > > powered off, and new values should be applied regardless if a new mode, > > has been applied or not. > > > > Does it make sense to you ? > > > > [*] or better, if the sensor is ported to use pm_runtime first (by > > dropping support for the deprecated .s_power(), or do you need > > s_power() on your platform ?) you can use pm_runtime_get_if_in_use() > > instead of keeping track manually of the is_enabled flag. > > What you are describing above is pretty much exactly what is done > later in this patchset. > > Patches 1-8 are there to fix some glaring bugs, with the possibility > of backporting the fixes which is why they are the first patches > in the set. > > Then after patch 9 the real work to get this driver into shape begins :) Now that I see "media: ov2680: Add runtime-pm support" in the series it all makes sense :) Thanks and sorry for the noise! (not adding my redundant tags, just skimming through the patches) > > Regards, > > Hans > > > > > > >> for a nice code cleanup. > >> > >> Fixes: 3ee47cad3e69 ("media: ov2680: Add Omnivision OV2680 sensor driver") > >> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> > >> Acked-by: Rui Miguel Silva <rmfrfs@gmail.com> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >> --- > >> drivers/media/i2c/ov2680.c | 50 ++++++++++---------------------------- > >> 1 file changed, 13 insertions(+), 37 deletions(-) > >> > >> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c > >> index 2001e08253ef..c93810f84ed7 100644 > >> --- a/drivers/media/i2c/ov2680.c > >> +++ b/drivers/media/i2c/ov2680.c > >> @@ -328,11 +328,15 @@ static void ov2680_set_bayer_order(struct ov2680_dev *sensor) > >> sensor->fmt.code = ov2680_hv_flip_bayer_order[hv_flip]; > >> } > >> > >> -static int ov2680_vflip_enable(struct ov2680_dev *sensor) > >> +static int ov2680_set_vflip(struct ov2680_dev *sensor, s32 val) > >> { > >> int ret; > >> > >> - ret = ov2680_mod_reg(sensor, OV2680_REG_FORMAT1, BIT(2), BIT(2)); > >> + if (sensor->is_streaming) > >> + return -EBUSY; > >> + > >> + ret = ov2680_mod_reg(sensor, OV2680_REG_FORMAT1, > >> + BIT(2), val ? BIT(2) : 0); > >> if (ret < 0) > >> return ret; > >> > >> @@ -340,33 +344,15 @@ static int ov2680_vflip_enable(struct ov2680_dev *sensor) > >> return 0; > >> } > >> > >> -static int ov2680_vflip_disable(struct ov2680_dev *sensor) > >> +static int ov2680_set_hflip(struct ov2680_dev *sensor, s32 val) > >> { > >> int ret; > >> > >> - ret = ov2680_mod_reg(sensor, OV2680_REG_FORMAT1, BIT(2), BIT(0)); > >> - if (ret < 0) > >> - return ret; > >> + if (sensor->is_streaming) > >> + return -EBUSY; > >> > >> - return ov2680_bayer_order(sensor); > >> -} > >> - > >> -static int ov2680_hflip_enable(struct ov2680_dev *sensor) > >> -{ > >> - int ret; > >> - > >> - ret = ov2680_mod_reg(sensor, OV2680_REG_FORMAT2, BIT(2), BIT(2)); > >> - if (ret < 0) > >> - return ret; > >> - > >> - return ov2680_bayer_order(sensor); > >> -} > >> - > >> -static int ov2680_hflip_disable(struct ov2680_dev *sensor) > >> -{ > >> - int ret; > >> - > >> - ret = ov2680_mod_reg(sensor, OV2680_REG_FORMAT2, BIT(2), BIT(0)); > >> + ret = ov2680_mod_reg(sensor, OV2680_REG_FORMAT2, > >> + BIT(2), val ? BIT(2) : 0); > >> if (ret < 0) > >> return ret; > >> > >> @@ -720,19 +706,9 @@ static int ov2680_s_ctrl(struct v4l2_ctrl *ctrl) > >> case V4L2_CID_EXPOSURE: > >> return ov2680_exposure_set(sensor, ctrl->val); > >> case V4L2_CID_VFLIP: > >> - if (sensor->is_streaming) > >> - return -EBUSY; > >> - if (ctrl->val) > >> - return ov2680_vflip_enable(sensor); > >> - else > >> - return ov2680_vflip_disable(sensor); > >> + return ov2680_set_vflip(sensor, ctrl->val); > >> case V4L2_CID_HFLIP: > >> - if (sensor->is_streaming) > >> - return -EBUSY; > >> - if (ctrl->val) > >> - return ov2680_hflip_enable(sensor); > >> - else > >> - return ov2680_hflip_disable(sensor); > >> + return ov2680_set_hflip(sensor, ctrl->val); > >> case V4L2_CID_TEST_PATTERN: > >> return ov2680_test_pattern_set(sensor, ctrl->val); > >> default: > >> -- > >> 2.41.0 > >> > > >
diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c index 2001e08253ef..c93810f84ed7 100644 --- a/drivers/media/i2c/ov2680.c +++ b/drivers/media/i2c/ov2680.c @@ -328,11 +328,15 @@ static void ov2680_set_bayer_order(struct ov2680_dev *sensor) sensor->fmt.code = ov2680_hv_flip_bayer_order[hv_flip]; } -static int ov2680_vflip_enable(struct ov2680_dev *sensor) +static int ov2680_set_vflip(struct ov2680_dev *sensor, s32 val) { int ret; - ret = ov2680_mod_reg(sensor, OV2680_REG_FORMAT1, BIT(2), BIT(2)); + if (sensor->is_streaming) + return -EBUSY; + + ret = ov2680_mod_reg(sensor, OV2680_REG_FORMAT1, + BIT(2), val ? BIT(2) : 0); if (ret < 0) return ret; @@ -340,33 +344,15 @@ static int ov2680_vflip_enable(struct ov2680_dev *sensor) return 0; } -static int ov2680_vflip_disable(struct ov2680_dev *sensor) +static int ov2680_set_hflip(struct ov2680_dev *sensor, s32 val) { int ret; - ret = ov2680_mod_reg(sensor, OV2680_REG_FORMAT1, BIT(2), BIT(0)); - if (ret < 0) - return ret; + if (sensor->is_streaming) + return -EBUSY; - return ov2680_bayer_order(sensor); -} - -static int ov2680_hflip_enable(struct ov2680_dev *sensor) -{ - int ret; - - ret = ov2680_mod_reg(sensor, OV2680_REG_FORMAT2, BIT(2), BIT(2)); - if (ret < 0) - return ret; - - return ov2680_bayer_order(sensor); -} - -static int ov2680_hflip_disable(struct ov2680_dev *sensor) -{ - int ret; - - ret = ov2680_mod_reg(sensor, OV2680_REG_FORMAT2, BIT(2), BIT(0)); + ret = ov2680_mod_reg(sensor, OV2680_REG_FORMAT2, + BIT(2), val ? BIT(2) : 0); if (ret < 0) return ret; @@ -720,19 +706,9 @@ static int ov2680_s_ctrl(struct v4l2_ctrl *ctrl) case V4L2_CID_EXPOSURE: return ov2680_exposure_set(sensor, ctrl->val); case V4L2_CID_VFLIP: - if (sensor->is_streaming) - return -EBUSY; - if (ctrl->val) - return ov2680_vflip_enable(sensor); - else - return ov2680_vflip_disable(sensor); + return ov2680_set_vflip(sensor, ctrl->val); case V4L2_CID_HFLIP: - if (sensor->is_streaming) - return -EBUSY; - if (ctrl->val) - return ov2680_hflip_enable(sensor); - else - return ov2680_hflip_disable(sensor); + return ov2680_set_hflip(sensor, ctrl->val); case V4L2_CID_TEST_PATTERN: return ov2680_test_pattern_set(sensor, ctrl->val); default:
ov2680_vflip_disable() / ov2680_hflip_disable() pass BIT(0) instead of 0 as value to ov2680_mod_reg(). While fixing this also: 1. Stop having separate enable/disable functions for hflip / vflip 2. Move the is_streaming check, which is unique to hflip / vflip into the ov2680_set_?flip() functions. for a nice code cleanup. Fixes: 3ee47cad3e69 ("media: ov2680: Add Omnivision OV2680 sensor driver") Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> Acked-by: Rui Miguel Silva <rmfrfs@gmail.com> Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/media/i2c/ov2680.c | 50 ++++++++++---------------------------- 1 file changed, 13 insertions(+), 37 deletions(-)