Re: [PATCH v7 2/2] media: i2c: add Himax HM1246 image sensor driver

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Tue, Jan 13, 2026 at 10:06:36AM +0100, Matthias Fend wrote:
> Hi Andy,
> Am 12.01.2026 um 20:01 schrieb Andy Shevchenko:
> > On Mon, Jan 12, 2026 at 03:49:33PM +0100, Matthias Fend wrote:

...

> > > +struct hm1246_mode {
> > > +	u32 codes[4];
> > > +	u32 clocks_per_pixel;
> > 
> > > +	u32 top;
> > > +	u32 left;
> > > +	u32 width;
> > > +	u32 height;
> > 
> > Why not use struct v4l2_rect?
> 
> Valid question. I would save something in six places, but add something in
> about 27 others. Because of this ratio, I opted for the current way.

It's more about standardization. Can you provide an example of the place where
you need to add something?

> > > +	u32 hts;
> > > +	u32 vts_min;
> > > +	const struct hm1246_reg_list reg_list;
> > > +};

...

> > > +static int hm1246_get_selection(struct v4l2_subdev *sd,
> > > +				struct v4l2_subdev_state *state,
> > > +				struct v4l2_subdev_selection *sel)
> > > +{
> > > +	const struct v4l2_mbus_framefmt *format;
> > > +	const struct hm1246_mode *mode;
> > > +
> > > +	format = v4l2_subdev_state_get_format(state, 0);
> > > +	mode = v4l2_find_nearest_size(hm1246_modes, ARRAY_SIZE(hm1246_modes),
> > > +				      width, height, format->width,
> > > +				      format->height);
> > > +
> > > +	switch (sel->target) {
> > > +	case V4L2_SEL_TGT_CROP:
> > > +		sel->r = *v4l2_subdev_state_get_crop(state, 0);
> > > +		return 0;
> > > +
> > > +	case V4L2_SEL_TGT_NATIVE_SIZE:
> > > +		sel->r.top = 0;
> > > +		sel->r.left = 0;
> > > +		sel->r.width = HM1246_NATIVE_WIDTH;
> > > +		sel->r.height = HM1246_NATIVE_HEIGHT;
> > > +		return 0;
> > > +
> > > +	case V4L2_SEL_TGT_CROP_DEFAULT:
> > > +	case V4L2_SEL_TGT_CROP_BOUNDS:
> > 
> > > +		sel->r.top = mode->top;
> > > +		sel->r.left = mode->left;
> > > +		sel->r.width = mode->width;
> > > +		sel->r.height = mode->height;
> > 
> > Seems in the same way here.
> > 
> > > +		return 0;
> > > +	}
> > 
> > > +	return -EINVAL;
> > 
> > Why not making it a default case?
> 
> I prefer it when the return statement is at the end of the function. Do you
> see a problem here?

For the matter of fact I do see a problem here. But it's not how code works
right now, it's about maintenance. The disrupted returns like this may lead
to subtle mistakes when the code gets changed (grows) and more cases added
including ones that might want to share something as a success path.

> > > +}

...

> > > +	hm1246->reset_gpio =
> > > +		devm_gpiod_get_optional(hm1246->dev, "reset", GPIOD_OUT_HIGH);
> > > +	if (IS_ERR(hm1246->reset_gpio))
> > > +		return dev_err_probe(hm1246->dev, PTR_ERR(hm1246->reset_gpio),
> > > +				     "failed to get reset GPIO\n");
> > 
> > Can it be GPIO reset driver used instead? (Note, it's made agnostic now.)
> 
> That would probably be possible, but I currently don't see any advantage for
> I2C image sensors. If I understand correctly, you would first have to define
> a reset controller that could then be used in the sensor – instead of simply
> specifying the GPIO directly.

Again, standardization.

> The advantage of being able to share the reset line with other components
> probably doesn't make sense for these sensors in most cases. That's perhaps
> also the reason why it hasn't been used before.
> 
> Maybe the media maintainers have an opinion on this?

-- 
With Best Regards,
Andy Shevchenko