Re: [PATCH v3 2/3] media: i2c: add os02g10 image sensor driver

[Vladimir Zapolskiy]

On 6/18/26 09:22, Elgin Perumbilly wrote:
> Hi Vladimir,
>   
> Thank you for the review.
>   
> I have addressed all of the comments except for two, where I am not entirely
> sure about the requested changes. Could you please take a look at the points
> below and let me know your opinion?
>   
> > On 4/24/26 12:25, Elgin Perumbilly wrote:
> > > Add a v4l2 subdevice driver for the Omnivision os02g10 sensor.
> > >
> > > The Omnivision os02g10 is a CMOS image sensor with an active array size of
> > > 1920 x 1080.
> > >
> > > The following features are supported:
> > > - Manual exposure an gain control support
> > > - vblank/hblank control support
> > > - vflip/hflip control support
> > > - Test pattern control support
> > > - Supported resolution: 1920 x 1080 @ 30fps (SBGGR10)
> > >
> > > Signed-off-by: Elgin Perumbilly <elgin.perumbilly@xxxxxxxxxxxxxxxxx>
> > > Reviewed-by: Tarang Raval <tarang.raval@xxxxxxxxxxxxxxxxx>
>   
> ...
>   
> > > +#include <linux/array_size.h>
> > > +#include <linux/bitops.h>
> > > +#include <linux/cleanup.h>
> > > +#include <linux/clk.h>
> > > +#include <linux/container_of.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/err.h>
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/module.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/pm_runtime.h>
> > > +#include <linux/property.h>
> > > +#include <linux/regulator/consumer.h>
> > > +#include <linux/units.h>
> > > +#include <linux/types.h>
> > > +#include <linux/time.h>
> > > +#include <linux/regmap.h>
> >
> > Please sort the list of includes in alphabetical order, also you
> > may consider to shrink the list by removing quite many inherited
> > includes.
>   
> Some maintainers prefer the "include what you use" approach, like Andy,
> so I added all the headers that are directly used. Should I now remove
> any inherited includes?

Yes, here opinions may vary, that's why I asked for sorting and to
consider to remove some of the redundant headers. In my personal opinion
this type of excessive information is not needed, especially if it is
justified only by probable and far future trivial clean-up work.

> > > +#include <media/v4l2-cci.h>
> > > +#include <media/v4l2-ctrls.h>
> > > +#include <media/v4l2-device.h>
> > > +#include <media/v4l2-fwnode.h>
> > > +#include <media/v4l2-mediabus.h>
>   
> ...
>   
> > > +static int os02g10_set_framefmt(struct os02g10 *os02g10,
> > > +                             struct v4l2_subdev_state *state)
> > > +{
> > > +     const struct v4l2_mbus_framefmt *format;
> > > +     const struct os02g10_mode *mode;
> > > +     int ret = 0;
> > > +
> > > +     format = v4l2_subdev_state_get_format(state, 0);
> > > +     mode = v4l2_find_nearest_size(supported_modes,
> > > +                                   ARRAY_SIZE(supported_modes), width,
> > > +                                   height, format->width, format->height);
> > > +
> > > +     cci_write(os02g10->cci, OS02G10_REG_V_START, mode->y_start, &ret);
> > > +     cci_write(os02g10->cci, OS02G10_REG_V_SIZE, mode->height, &ret);
> > > +     cci_write(os02g10->cci, OS02G10_REG_V_SIZE_MIPI, mode->height, &ret);
> > > +     cci_write(os02g10->cci, OS02G10_REG_H_START, mode->x_start, &ret);
> > > +     cci_write(os02g10->cci, OS02G10_REG_H_SIZE, mode->width, &ret);
> > > +     cci_write(os02g10->cci, OS02G10_REG_H_SIZE_MIPI, mode->width, &ret);
> > > +
> > > +     return ret;
> >
> > Just "return 0" here, and remove the local variable.
>   
> Could you clarify why this should return 0? The local ret is passed to all
> cci_write() calls so that any write error is propagated. Returning 0 here
> would appear to suppress those errors and always report success.

My bad, yes, here please leave 'return ret' as is, I was confused and
misleaded by initialization of the local variable to zero, which is
redundant, and I'd suggest to remove this initialization.

--
Best wishes,
Vladimir