Received-SPF: pass (google.com: domain of linux-kernel+bounces-99291-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Mon, 11 Mar 2024 17:10:55 +0000
From: Sakari Ailus <sakari.ailus@linux.intel.com>
To: Kieran Bingham <kieran.bingham@ideasonboard.com>
Cc: Umang Jain <umang.jain@ideasonboard.com>, linux-media@vger.kernel.org,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	willl will <will@willwhang.com>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Hans Verkuil <hverkuil-cisco@xs4all.nl>,
	Hans de Goede <hdegoede@redhat.com>,
	Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>,
	Alain Volmat <alain.volmat@foss.st.com>,
	Paul Elder <paul.elder@ideasonboard.com>,
	Mehdi Djait <mehdi.djait@bootlin.com>,
	Bingbu Cao <bingbu.cao@intel.com>,
	Andy Shevchenko <andy.shevchenko@gmail.com>,
	Sebastian Reichel <sre@kernel.org>,
	Jacopo Mondi <jacopo.mondi@ideasonboard.com>,
	open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 2/2] media: i2c: Add imx283 camera sensor driver
Message-ID: <Ze87Hya-cqmkqjMC@kekkonen.localdomain>
References: <20240307214048.858318-1-umang.jain@ideasonboard.com>
 <20240307214048.858318-3-umang.jain@ideasonboard.com>
 <Zeq7HBMqqrw4nSPj@kekkonen.localdomain>
 <171016009901.2924028.16001544322304093037@ping.linuxembedded.co.uk>
 <Ze8xY1bqTiXzRvKp@kekkonen.localdomain>
 <171017536692.2924028.6522729664515712567@ping.linuxembedded.co.uk>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <171017536692.2924028.6522729664515712567@ping.linuxembedded.co.uk>

Hi Kieran,

On Mon, Mar 11, 2024 at 04:42:46PM +0000, Kieran Bingham wrote:
> 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.

Index of the mode, not the mode itself. They're different.

> 
> *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

Ok, fine by me. Maybe a comment at the end of the mode list to check this
when adding new modes? There are other sources of modes than the datasheet.

-- 
Sakari Ailus