linux-kernel - Re: [PATCH v2 4/4] media: i2c: imx214: Remove hard-coded external clock frequency

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250515085846.GR23592@pendragon.ideasonboard.com>
Date: Thu, 15 May 2025 10:58:46 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: git@...tzsch.eu, Ricardo Ribalda <ribalda@...nel.org>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	~postmarketos/upstreaming@...ts.sr.ht, phone-devel@...r.kernel.org,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/4] media: i2c: imx214: Remove hard-coded external
 clock frequency

Hi Sakari,

On Tue, May 06, 2025 at 08:24:03AM +0000, Sakari Ailus wrote:
> On Mon, May 05, 2025 at 11:05:56PM +0200, André Apitzsch via B4 Relay wrote:
> > From: André Apitzsch <git@...tzsch.eu>
> > 
> > Instead rely on the rate set on the clock (using assigned-clock-rates
> > etc.)
> > 
> > Signed-off-by: André Apitzsch <git@...tzsch.eu>
> > ---
> >  drivers/media/i2c/imx214.c | 6 ------
> >  1 file changed, 6 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> > index 9e9be47394ec768a5b34d44b06b5bbb0988da5a1..c12996e294dccebb18c608254f1e0d14dc064423 100644
> > --- a/drivers/media/i2c/imx214.c
> > +++ b/drivers/media/i2c/imx214.c
> > @@ -32,7 +32,6 @@
> >  
> >  #define IMX214_REG_FAST_STANDBY_CTRL	CCI_REG8(0x0106)
> >  
> > -#define IMX214_DEFAULT_CLK_FREQ	24000000
> >  #define IMX214_DEFAULT_LINK_FREQ	600000000
> >  /* Keep wrong link frequency for backward compatibility */
> >  #define IMX214_DEFAULT_LINK_FREQ_LEGACY	480000000
> > @@ -1405,11 +1404,6 @@ static int imx214_probe(struct i2c_client *client)
> >  		return dev_err_probe(dev, PTR_ERR(imx214->xclk),
> >  				     "failed to get xclk\n");
> >  
> > -	ret = clk_set_rate(imx214->xclk, IMX214_DEFAULT_CLK_FREQ);
> > -	if (ret)
> > -		return dev_err_probe(dev, ret,
> > -				     "failed to set xclk frequency\n");
> > -
> 
> Oops. I missed this is what the driver was doing already. Indeed, this is
> one of the historic sensor drivers that do set the frequency in DT systems.
> 
> The driver never used the clock-frequency property and instead used a fixed
> frequency. Changing the behaviour now could be problematic.
> 
> There are options here that I think we could do:
> 
> 1) use your v1 patch (4) which uses "clock-frequency" if it exists and
> otherwise uses the default, fixed frequency or
> 
> 2) set the frequency only if the "clock-frequency" property exists. The DT
> currently requires clock-frequency and the YAML conversion was done in 2020
> whereas the driver is from 2018. If we do this, the clock-frequency should
> be deprecated (or even removed from bingings).
> 
> I wonder what others think. Cc'd Laurent in any case.

Maybe I'm missing something, but I don't really see the issue here. The
clock-frequency DT property is currently ignored, and this patch doesn't
change that situation, does it ?

The change of behaviour here is related to the assigned-clock-rates
property. If that property is specified today, it will set the clock
rate, and the driver will override it to 24MHz right after. With this
patch, the clock rate won't be overridden. I think the risk of
regression is very low here, as I don't expect systems to set
assigned-clock-rates in DT to a value different than 24MHz and expect
the driver to override it.

> >  	ret = imx214_get_regulators(dev, imx214);
> >  	if (ret < 0)
> >  		return dev_err_probe(dev, ret, "failed to get regulators\n");

-- 
Regards,

Laurent Pinchart