linux-kernel - Re: [PATCH 5/5] media/i2c: max96717: allow user to override operation mode from DT

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <twfyc6qllxcssrw7ydfinby56azxadw6zjcinzukncoxnunixu@swknfqwn2qez>
Date: Thu, 6 Mar 2025 10:42:06 +0200
From: Laurentiu Palcu <laurentiu.palcu@....nxp.com>
To: Julien Massot <julien.massot@...labora.com>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>, 
	linux-kernel@...r.kernel.org, linux-media@...r.kernel.org
Subject: Re: [PATCH 5/5] media/i2c: max96717: allow user to override
 operation mode from DT

Hi Julien,

On Tue, Feb 18, 2025 at 04:21:36PM +0100, Julien Massot wrote:
> Hi Laurentiu,
> 
> On Fri, 2025-02-07 at 13:29 +0200, Laurentiu Palcu wrote:
> > There are situations when the CFG pins set the chip up for a certain
> > mode of operation (ie: pixel mode or tunneling mode), because the HW
> > designers decided this way, and we, the users, want to change that. For
> > that, add an optional DT property that would allow toggling the
> > operation mode from the configured one to the other one.
> > 
> > The driver still only supports tunneling mode, that didn't change.
> > 
> > Signed-off-by: Laurentiu Palcu <laurentiu.palcu@....nxp.com>
> > ---
> >  drivers/media/i2c/max96717.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/media/i2c/max96717.c b/drivers/media/i2c/max96717.c
> > index 47a3be195a971..a591ca5d5f44f 100644
> > --- a/drivers/media/i2c/max96717.c
> > +++ b/drivers/media/i2c/max96717.c
> 
> 
> enum gmsl2_mode {
>   GMSL2_PIXEL_MODE,
>   GMSL2_MODE_TUNNEL,
> };
> 
> > @@ -161,6 +161,7 @@ struct max96717_priv {
> >  	struct clk_hw                     clk_hw;
> >  	struct gpio_chip                  gpio_chip;
> >  	enum max96717_vpg_mode            pattern;
> > +	bool				  mode_override;
> 	enum gmsl2_mode                   mode;
> I would prefer to set the mode in an explicit way instead of toggling
> the bit in the register.
> 
> >  	struct max96717_fsync_desc	  fsync;
> >  };
> >  
> > @@ -1066,6 +1067,14 @@ static int max96717_hw_init(struct max96717_priv *priv)
> >  		return dev_err_probe(dev, ret,
> >  				     "Fail to read mipi rx extension");
> >  
> > +	if (priv->mode_override) {
>         if (priv->mode_override && priv->mode == GMSL2_MODE_TUNNEL) {
> > +		
> > +
> > +		ret = cci_write(priv->regmap, MAX96717_MIPI_RX_EXT11, val, NULL);
> 		ret = cci_update_bits(priv->regmap, MAX96717_MIPI_RX_EXT11, MAX96717_TUN_MODE,
> 					MAX96717_TUN_MODE, NULL);
> > +		if (ret)
> > +			return dev_err_probe(dev, ret, "Unable to update operation mode\n");
> > +	}
> > +
> In case we are overwriting the mode to tunnel mode then no need to read the EXT11 register.
> 
> >  	if (!(val & MAX96717_TUN_MODE))
> >  		return dev_err_probe(dev, -EOPNOTSUPP,
> >  				     "Only supporting tunnel mode");
> 
> In fact the driver can works in pixel mode, but since we don't set the "stream id" the
> deserializer have to configured with the right one :)
> 
> > @@ -1101,6 +1110,9 @@ static int max96717_parse_dt(struct max96717_priv *priv)
> >  
> >  	priv->mipi_csi2 = vep.bus.mipi_csi2;
> >  
> > +	if (fwnode_property_present(dev_fwnode(dev), "maxim,cfg-mode-override"))
> > +		priv->mode_override = true;
> > +
> 	source_fwnode = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev),
> 							MAX96717_PAD_SOURCE, 0, 0);
> 	if (fwnode_property_present(source_fwnode, "maxim,tunnel-mode")) {
> 		priv->mode_override = true;
> 		priv->mode = GMSL2_MODE_TUNNEL;
> 	}

So, I don't think the boolean 'maxim,tunnel-mode' would work well when
the pin configuration is 'tunnel' and the user wants to switch to
'pixel'. Maybe, replace the boolean 'maxim,cfg-mode-override' property
with an optional enum property 'maxim,cfg-mode'? Does that sound better?

Thanks,
Laurentiu

> So we can parse the tunnel property from the GMSL port.
> 
> >  	priv->fsync.pin = -1;
> >  	count = fwnode_property_present(dev_fwnode(dev), "maxim,fsync-config");
> >  	if (count > 0) {
> 
> Best Regards,
> 
> -- 
> Julien