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

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3165 bytes --]

On Sun, Jun 30, 2024 at 10:54:48AM +0100, Jonathan Cameron wrote:
> 
> > > > +
> > > >  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.

Per the binding (proposed and current) I think the code here is fine
w.r.t. probe failures. Before the series, it looks like clocks/clock-names
were required by the binding and the driver would fail to probe if they were
not provided. The current code only fails to probe if neither clocks
or clock-names and #clock-cells are not provided, so it is a weaker
restriction than before. The binding doesn't require #clock-cells at all
times, only if the clock consumer properties are not present, so it is
both fine backwards compatibility wise and seems to match how the driver
is behaving. I'm biased, but I don't buy "the DT writer doesn't care" as
an argument cos if they didn't care about adding the required clock
consumer properties now then they'd not have probed before either...

Cheers,
Conor.

> > 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á
> > 
> > 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]