Quoting Sakari Ailus (2024-03-11 16:29:23)
> Hi Kieran,
>
> On Mon, Mar 11, 2024 at 12:28:19PM +0000, Kieran Bingham wrote:
> > Hi Sakari, Umang,
> >
> > I've replied inline below to a couple of points that I was responsible for.
> >
> > >
> > > > +
> > > > +struct imx283 {
> > > > + struct device *dev;
> > > > + struct regmap *cci;
> > > > +
> > > > + const struct imx283_input_frequency *freq;
> > > > +
> > > > + struct v4l2_subdev sd;
> > > > + struct media_pad pad;
> > > > +
> > > > + struct clk *xclk;
> > > > +
> > > > + struct gpio_desc *reset_gpio;
> > > > + struct regulator_bulk_data supplies[ARRAY_SIZE(imx283_supply_name)];
> > > > +
> > > > + /* V4L2 Controls */
> > > > + struct v4l2_ctrl_handler ctrl_handler;
> > > > + struct v4l2_ctrl *exposure;
> > > > + struct v4l2_ctrl *vblank;
> > > > + struct v4l2_ctrl *hblank;
> > > > + struct v4l2_ctrl *vflip;
> > > > +
> > > > + unsigned long link_freq_bitmap;
> > > > +
> > > > + u16 hmax;
> > > > + u32 vmax;
> > > > +};
> > > > +
> > > > +static inline struct imx283 *to_imx283(struct v4l2_subdev *_sd)
> > > > +{
> > > > + return container_of(_sd, struct imx283, sd);
> > >
> > > It's a function, you can call _sd sd instead.
> >
> > Except then that could 'look' like it is passed as the first and third
> > argument to container_of...
>
> It's really a non-issue: the third argument is a field name, not a
> variable.
That's not so easy to determine when the args are the same name.., but
it's fine with me either way.
> > But if it's fine / accepted otherwise then sure.
>
> And please use container_of_const(). :)
Ack. Or rather ... I'll leave that to Umang to handle, as he's managing
this driver now.
> > > > +
> > > > +/* Determine the exposure based on current hmax, vmax and a given SHR */
> > > > +static u64 imx283_exposure(struct imx283 *imx283,
> > > > + const struct imx283_mode *mode, u64 shr)
> > > > +{
> > > > + u32 svr = 0; /* SVR feature is not currently supported */
> > >
> > > What does this refer to? I guess you could just drop it as well if it's not
> > > supported?
> >
> > Keeping this will keep the calculation matching the datasheet, and
> > provide clear value for what to update when we/others return to enable
> > long exposures.
> >
> > So it would be nice to keep as it sort of documents/tracks the
> > datasheet.
>
> Ack.
>
> >
> >
> > > > + u32 hmax = imx283->hmax;
> > > > + u64 vmax = imx283->vmax;
> > >
> > > You're not changing the values here. I wouldn't introduce temporary
> > > variables just for that.
> > >
> > > > + u32 offset;
> > > > + u64 numerator;
> > > > +
> > > > + /* Number of clocks per internal offset period */
> > > > + offset = mode->mode == IMX283_MODE_0 ? 209 : 157;
> > >
> > > Shouldn't this be in the mode definition?
> >
> > It could be, but then there would be one copy of 209, and 9 copies of
> > 157.
>
> That would still be specified explicitly. Someone adding a new mode would
> easily miss this.
>
> Or, if you can, derive this from something else that is now a part of the
> mode itself.
I don't understand the above, other than ... That's exactly what we're
doing here.
*Only* MODE_0 has an offset of 209 in the datasheet. All other modes are
157.
This is the table being codified:
https://pasteboard.co/OsKf4VX7rtrS.png
--
Kieran
