linux-kernel - Re: [PATCH v6 5/6] iio: adc: ad7192: Add clock provider

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]

Date: Sun, 30 Jun 2024 10:54:48 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Nuno Sá <noname.nuno@...il.com>
Cc: Alexandru Ardelean <aardelean@...libre.com>, Alisa-Dariana Roman 
 <alisadariana@...il.com>, Alisa-Dariana Roman <alisa.roman@...log.com>,
 Jonathan Cameron <Jonathan.Cameron@...wei.com>, Michael Hennerich
 <michael.hennerich@...log.com>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, Lars-Peter
 Clausen <lars@...afoo.de>, Rob Herring <robh@...nel.org>, Krzysztof
 Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Liam
 Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH v6 5/6] iio: adc: ad7192: Add clock provider


> > > +
> > >  static int ad7192_clock_setup(struct ad7192_state *st)
> > >  {
> > >         struct device *dev = &st->sd.spi->dev;
> > > @@ -412,6 +496,11 @@ static int ad7192_clock_setup(struct ad7192_state *st)
> > >         if (ret < 0) {
> > >                 st->clock_sel = AD7192_CLK_INT;
> > >                 st->fclk = AD7192_INT_FREQ_MHZ;
> > > +
> > > +               ret = ad7192_register_clk_provider(st);
> > > +               if (ret)
> > > +                       return dev_err_probe(dev, ret,
> > > +                                            "Failed to register clock
> > > provider\n");  
> > 
> > A question here: do we want to fail the probe of this driver when it
> > cannot register a clock provider?
> > Or should we ignore it?
> > No preference from my side.  
> 
> Sensible question... I would say it depends. On one side this is an optional
> feature so we should not (arguably) error out. OTOH, someone may really want
> (and relies on) this feature so failing makes sense.
> 
> Maybe we should have
> 
> if (!device_property_present(&spi->dev, "#clock-cells"))
> 	return 0;

I'm not 100% sure from looking at the code, but if the absence of this property
(because the DT writer doesn't care about this) is sufficient to make the
calls in ad7192_register_clk_provider() fail then we should check this.
I don't think we need the complexity of get_provider_clk_node() as there is
no reason to look in a parent of this device (it's not an mfd or similar) so
this check should be sufficient.

Does this also mean the binding should not require this?  I suspect it shouldn't.
 
> 
> in ad7192_register_clk_provider(). So that if we fail the function, then yes, we
> should fail probing as FW wants this to be a provider. Also, not providing
> #clock-cells means we don't register the clock.
> 
> Having said the above I think that failing devm_clk_hw_register() means that
> something is already really wrong (or we have a bug in the driver) so likely we
> should keep it simple and just always provide the clock and return an error if
> we fail to do so.
> 
> my 2 cents...
> 
> - Nuno Sá
> 
>