linux-kernel - Re: [PATCH v3 2/3] media: imx-mipi-csis: Store the number of data

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250922080723.GI10540@pendragon.ideasonboard.com>
Date: Mon, 22 Sep 2025 11:07:23 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Sakari Ailus <sakari.ailus@....fi>
Cc: Isaac Scott <isaac.scott@...asonboard.com>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	Rui Miguel Silva <rmfrfs@...il.com>,
	Martin Kepplinger <martink@...teo.de>,
	Purism Kernel Team <kernel@...i.sm>,
	Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>, linux-media@...r.kernel.org,
	linux-kernel@...r.kernel.org, imx@...ts.linux.dev,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v3 2/3] media: imx-mipi-csis: Store the number of
 data_lanes configured in dt

On Mon, Sep 22, 2025 at 09:49:27AM +0300, Sakari Ailus wrote:
> On Mon, Sep 15, 2025 at 02:18:34PM +0100, Isaac Scott wrote:
> > The number of lanes actively used by a MIPI CSI transmitter may differ
> > from that which is defined in device tree. To allow us to be able to set
> > the number of configured lanes without changing the maximum lane count,
> > store the number of lanes configured in device tree, and adjust the
> > debug print to reflect the device tree value.
> > 
> > Signed-off-by: Isaac Scott <isaac.scott@...asonboard.com>
> > ---
> >  drivers/media/platform/nxp/imx-mipi-csis.c | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> > index 2beb5f43c2c0..6afbedfe131e 100644
> > --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> > +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> > @@ -313,6 +313,8 @@ struct mipi_csis_device {
> >  	u32 hs_settle;
> >  	u32 clk_settle;
> >  
> > +	unsigned int num_data_lanes;
> > +
> >  	spinlock_t slock;	/* Protect events */
> >  	struct mipi_csis_event events[MIPI_CSIS_NUM_EVENTS];
> >  	struct dentry *debugfs_root;
> > @@ -535,7 +537,7 @@ static void mipi_csis_system_enable(struct mipi_csis_device *csis, int on)
> >  	val = mipi_csis_read(csis, MIPI_CSIS_DPHY_CMN_CTRL);
> >  	val &= ~MIPI_CSIS_DPHY_CMN_CTRL_ENABLE;
> >  	if (on) {
> > -		mask = (1 << (csis->bus.num_data_lanes + 1)) - 1;
> > +		mask = (1 << (csis->num_data_lanes + 1)) - 1;
> 
> Please use 1U or BIT() for bit-shifted values.

BIT() isn't appropriate here. GENMASK(csis->num_data_lanes, 0) could be
fine, but this patch just changes the variable, I wouldn't insist in
fixing separate issues.

> >  		val |= (mask & MIPI_CSIS_DPHY_CMN_CTRL_ENABLE);
> >  	}
> >  	mipi_csis_write(csis, MIPI_CSIS_DPHY_CMN_CTRL, val);
> > @@ -586,7 +588,7 @@ static int mipi_csis_calculate_params(struct mipi_csis_device *csis,
> >  
> >  	/* Calculate the line rate from the pixel rate. */
> >  	link_freq = v4l2_get_link_freq(src_pad, csis_fmt->width,
> > -				       csis->bus.num_data_lanes * 2);
> > +				       csis->num_data_lanes * 2);
> >  	if (link_freq < 0) {
> >  		dev_err(csis->dev, "Unable to obtain link frequency: %d\n",
> >  			(int)link_freq);
> > @@ -631,7 +633,7 @@ static void mipi_csis_set_params(struct mipi_csis_device *csis,
> >  				 const struct v4l2_mbus_framefmt *format,
> >  				 const struct csis_pix_format *csis_fmt)
> >  {
> > -	int lanes = csis->bus.num_data_lanes;
> > +	int lanes = csis->num_data_lanes;
> >  	u32 val;
> >  
> >  	val = mipi_csis_read(csis, MIPI_CSIS_CMN_CTRL);
> > @@ -1299,8 +1301,10 @@ static int mipi_csis_async_register(struct mipi_csis_device *csis)
> >  	}
> >  
> >  	csis->bus = vep.bus.mipi_csi2;
> > +	csis->bus.num_data_lanes = vep.bus.mipi_csi2.num_data_lanes;

That doesn't seem to be needed.

> > +	csis->num_data_lanes = csis->bus.num_data_lanes;
> >  
> > -	dev_dbg(csis->dev, "data lanes: %d\n", csis->bus.num_data_lanes);
> > +	dev_dbg(csis->dev, "data lanes: %d\n", csis->num_data_lanes);

Neither is this change. What you print here is the number of connected
data lanes, not the number of effectively used lanes. You could change
it to 

	dev_dbg(csis->dev, "max data lanes: %u\n", csis->bus.num_data_lanes);

You'll want to also update the commit message, which I think needs some
improvement regardless.

> >  	dev_dbg(csis->dev, "flags: 0x%08x\n", csis->bus.flags);
> >  
> >  	asd = v4l2_async_nf_add_fwnode_remote(&csis->notifier, ep,
> > @@ -1498,7 +1502,7 @@ static int mipi_csis_probe(struct platform_device *pdev)
> >  	}
> >  
> >  	dev_info(dev, "lanes: %d, freq: %u\n",
> > -		 csis->bus.num_data_lanes, csis->clk_frequency);
> > +		 csis->num_data_lanes, csis->clk_frequency);

Drop this change too. This message is actually redundant, the number of
lanes is printed in mipi_csis_async_register(). You could submit a
separate patch to remove this, possibly replacing it with a dev_dbg() in
mipi_csis_parse_dt() to print clk_frequency.

> >  
> >  	return 0;
> >  

-- 
Regards,

Laurent Pinchart