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