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