| Message ID | 20230615141349.172363-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 15/06/2023 15:13, 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. > > for a nice code cleanup. > > Fixes: 3ee47cad3e69 ("media: ov2680: Add Omnivision OV2680 sensor driver") > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- Thanks for the detailed explanation in the last version Reviewed-by: Daniel Scally <dan.scally@ideasonboard.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:
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") Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/media/i2c/ov2680.c | 50 ++++++++++---------------------------- 1 file changed, 13 insertions(+), 37 deletions(-)