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: <7d2d4c3bc5e8d49fbcd4763487f82db634b06205.camel@apitzsch.eu>
Date: Thu, 15 May 2025 23:21:42 +0200
From: André Apitzsch <git@...tzsch.eu>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>, Laurent Pinchart
	 <laurent.pinchart@...asonboard.com>
Cc: 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, Hi Laurent,

Am Freitag, dem 16.05.2025 um 00:02 +0300 schrieb Sakari Ailus:
> Hi Laurent,
> 
> On Thu, May 15, 2025 at 03:03:40PM +0200, Laurent Pinchart wrote:
> > On Thu, May 15, 2025 at 02:54:54PM +0300, Sakari Ailus wrote:
> > > On Thu, May 15, 2025 at 10:58:46AM +0200, Laurent Pinchart wrote:
> > > > 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..c12996e294dccebb1
> > > > > > 8c608254f1e0d14dc064423 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.
> > > 
> > > If the DTS had assigned-clock-rates set correctly, then yes. How
> > > much can we trust the older DTS did have that?
> > 
> > I am relatively confident that DT-based systems wouldn't have an
> > assigned-clock-rates property with a frequency that doesn't match
> > IMX214_DEFAULT_CLK_FREQ. The real question is whether or not I'm
> > over-confident :-)
> 
> The assigned-clock stuff wasn't always there. But nowadays I guess a
> lot of things in practice depends on it.
> 
> So I guess doing this should be fine then.

Just to be clear, this patch is fine and no changes are needed?

Should the bindings be updated in this series or can that be done
later?

Best regards,
André