| Message ID | 20221028160902.2696973-6-dave.stevenson@raspberrypi.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Updates to ov9282 sensor driver | expand |
Hi Dave On Fri, Oct 28, 2022 at 05:08:51PM +0100, Dave Stevenson wrote: > The driver currently has multiple assumptions that there is > only one supported mode. > > Convert supported_mode to an array, and fix up all references > to correctly look at that array. > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > --- > drivers/media/i2c/ov9282.c | 46 +++++++++++++++++++++++--------------- > 1 file changed, 28 insertions(+), 18 deletions(-) > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c > index 123aa20951b7..1524189cf3e5 100644 > --- a/drivers/media/i2c/ov9282.c > +++ b/drivers/media/i2c/ov9282.c > @@ -217,6 +217,10 @@ struct ov9282_reg_list common_regs_list = { > .regs = common_regs, > }; > > +#define MODE_1280_720 0 > + > +#define DEFAULT_MODE MODE_1280_720 > + I don't mind this considering there will be multiple modes > /* Sensor mode registers */ > static const struct ov9282_reg mode_1280x720_regs[] = { > {0x3778, 0x00}, > @@ -252,17 +256,19 @@ static const struct ov9282_reg mode_1280x720_regs[] = { > }; > > /* Supported sensor mode configurations */ > -static const struct ov9282_mode supported_mode = { > - .width = 1280, > - .height = 720, > - .hblank = 250, > - .vblank = 1022, > - .vblank_min = 151, > - .vblank_max = 51540, > - .link_freq_idx = 0, > - .reg_list = { > - .num_of_regs = ARRAY_SIZE(mode_1280x720_regs), > - .regs = mode_1280x720_regs, > +static const struct ov9282_mode supported_modes[] = { > + [MODE_1280_720] = { > + .width = 1280, > + .height = 720, > + .hblank = 250, > + .vblank = 1022, > + .vblank_min = 151, > + .vblank_max = 51540, > + .link_freq_idx = 0, > + .reg_list = { > + .num_of_regs = ARRAY_SIZE(mode_1280x720_regs), > + .regs = mode_1280x720_regs, > + }, > }, > }; > > @@ -526,15 +532,15 @@ static int ov9282_enum_frame_size(struct v4l2_subdev *sd, > struct v4l2_subdev_state *sd_state, > struct v4l2_subdev_frame_size_enum *fsize) > { > - if (fsize->index > 0) > + if (fsize->index >= ARRAY_SIZE(supported_modes)) > return -EINVAL; > > if (fsize->code != MEDIA_BUS_FMT_Y10_1X10) > return -EINVAL; > > - fsize->min_width = supported_mode.width; > + fsize->min_width = supported_modes[fsize->index].width; > fsize->max_width = fsize->min_width; > - fsize->min_height = supported_mode.height; > + fsize->min_height = supported_modes[fsize->index].height; > fsize->max_height = fsize->min_height; > > return 0; > @@ -609,7 +615,11 @@ static int ov9282_set_pad_format(struct v4l2_subdev *sd, > > mutex_lock(&ov9282->mutex); > > - mode = &supported_mode; > + mode = v4l2_find_nearest_size(supported_modes, > + ARRAY_SIZE(supported_modes), > + width, height, > + fmt->format.width, > + fmt->format.height); > ov9282_fill_pad_format(ov9282, mode, fmt); > > if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { > @@ -642,7 +652,7 @@ static int ov9282_init_pad_cfg(struct v4l2_subdev *sd, > struct v4l2_subdev_format fmt = { 0 }; > > fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE; > - ov9282_fill_pad_format(ov9282, &supported_mode, &fmt); > + ov9282_fill_pad_format(ov9282, &supported_modes[DEFAULT_MODE], &fmt); > > return ov9282_set_pad_format(sd, sd_state, &fmt); > } > @@ -1043,8 +1053,8 @@ static int ov9282_probe(struct i2c_client *client) > goto error_power_off; > } > > - /* Set default mode to max resolution */ > - ov9282->cur_mode = &supported_mode; > + /* Set default mode to first mode */ > + ov9282->cur_mode = &supported_modes[DEFAULT_MODE]; Looks good Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > ov9282->vblank = ov9282->cur_mode->vblank; > > ret = ov9282_init_controls(ov9282); > -- > 2.34.1 >
On Fri, Oct 28, 2022 at 05:08:51PM +0100, Dave Stevenson wrote: > The driver currently has multiple assumptions that there is > only one supported mode. > > Convert supported_mode to an array, and fix up all references > to correctly look at that array. > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> This probably should have been ov9282 and not ov9281? I'll fix that when applying it.
Hi Sakari On Tue, 1 Nov 2022 at 10:12, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > On Fri, Oct 28, 2022 at 05:08:51PM +0100, Dave Stevenson wrote: > > The driver currently has multiple assumptions that there is > > only one supported mode. > > > > Convert supported_mode to an array, and fix up all references > > to correctly look at that array. > > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > This probably should have been ov9282 and not ov9281? I'll fix that when > applying it. Yes, thanks. As discussed in the patch set at [1], ov9281 and ov9282 are the same except for CRA in the optical path. Largely we see OV9281 modules, hence the slip of the keys. Dave [1] https://www.spinics.net/lists/devicetree/msg518160.html > -- > Sakari Ailus
diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c index 123aa20951b7..1524189cf3e5 100644 --- a/drivers/media/i2c/ov9282.c +++ b/drivers/media/i2c/ov9282.c @@ -217,6 +217,10 @@ struct ov9282_reg_list common_regs_list = { .regs = common_regs, }; +#define MODE_1280_720 0 + +#define DEFAULT_MODE MODE_1280_720 + /* Sensor mode registers */ static const struct ov9282_reg mode_1280x720_regs[] = { {0x3778, 0x00}, @@ -252,17 +256,19 @@ static const struct ov9282_reg mode_1280x720_regs[] = { }; /* Supported sensor mode configurations */ -static const struct ov9282_mode supported_mode = { - .width = 1280, - .height = 720, - .hblank = 250, - .vblank = 1022, - .vblank_min = 151, - .vblank_max = 51540, - .link_freq_idx = 0, - .reg_list = { - .num_of_regs = ARRAY_SIZE(mode_1280x720_regs), - .regs = mode_1280x720_regs, +static const struct ov9282_mode supported_modes[] = { + [MODE_1280_720] = { + .width = 1280, + .height = 720, + .hblank = 250, + .vblank = 1022, + .vblank_min = 151, + .vblank_max = 51540, + .link_freq_idx = 0, + .reg_list = { + .num_of_regs = ARRAY_SIZE(mode_1280x720_regs), + .regs = mode_1280x720_regs, + }, }, }; @@ -526,15 +532,15 @@ static int ov9282_enum_frame_size(struct v4l2_subdev *sd, struct v4l2_subdev_state *sd_state, struct v4l2_subdev_frame_size_enum *fsize) { - if (fsize->index > 0) + if (fsize->index >= ARRAY_SIZE(supported_modes)) return -EINVAL; if (fsize->code != MEDIA_BUS_FMT_Y10_1X10) return -EINVAL; - fsize->min_width = supported_mode.width; + fsize->min_width = supported_modes[fsize->index].width; fsize->max_width = fsize->min_width; - fsize->min_height = supported_mode.height; + fsize->min_height = supported_modes[fsize->index].height; fsize->max_height = fsize->min_height; return 0; @@ -609,7 +615,11 @@ static int ov9282_set_pad_format(struct v4l2_subdev *sd, mutex_lock(&ov9282->mutex); - mode = &supported_mode; + mode = v4l2_find_nearest_size(supported_modes, + ARRAY_SIZE(supported_modes), + width, height, + fmt->format.width, + fmt->format.height); ov9282_fill_pad_format(ov9282, mode, fmt); if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { @@ -642,7 +652,7 @@ static int ov9282_init_pad_cfg(struct v4l2_subdev *sd, struct v4l2_subdev_format fmt = { 0 }; fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE; - ov9282_fill_pad_format(ov9282, &supported_mode, &fmt); + ov9282_fill_pad_format(ov9282, &supported_modes[DEFAULT_MODE], &fmt); return ov9282_set_pad_format(sd, sd_state, &fmt); } @@ -1043,8 +1053,8 @@ static int ov9282_probe(struct i2c_client *client) goto error_power_off; } - /* Set default mode to max resolution */ - ov9282->cur_mode = &supported_mode; + /* Set default mode to first mode */ + ov9282->cur_mode = &supported_modes[DEFAULT_MODE]; ov9282->vblank = ov9282->cur_mode->vblank; ret = ov9282_init_controls(ov9282);
The driver currently has multiple assumptions that there is only one supported mode. Convert supported_mode to an array, and fix up all references to correctly look at that array. Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> --- drivers/media/i2c/ov9282.c | 46 +++++++++++++++++++++++--------------- 1 file changed, 28 insertions(+), 18 deletions(-)