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

[Jonathan Cameron]

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