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

["Nuno Sá" <noname.nuno@gmail.com>]

On Tue, 2024-06-25 at 08:48 +0300, Alexandru Ardelean wrote:
> On Mon, Jun 24, 2024 at 3:51 PM Alisa-Dariana Roman
> <alisadariana@gmail.com> wrote:
> > 
> > Internal clock of AD719X devices can be made available on MCLK2 pin. Add
> > clock provider to support this functionality.
> > 
> 
> Just a question that should be addressed by the failing of the
> clock-provider registration.
> With that addressed:
> 
> Reviewed-by: Alexandru Ardelean <aardelean@baylibre.com>
> 
> > Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> > ---
> >  drivers/iio/adc/ad7192.c | 89 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 89 insertions(+)
> > 
> > diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> > index 940517df5429..90763c14679d 100644
> > --- a/drivers/iio/adc/ad7192.c
> > +++ b/drivers/iio/adc/ad7192.c
> > @@ -8,6 +8,7 @@
> >  #include <linux/interrupt.h>
> >  #include <linux/bitfield.h>
> >  #include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> >  #include <linux/device.h>
> >  #include <linux/kernel.h>
> >  #include <linux/slab.h>
> > @@ -201,6 +202,7 @@ struct ad7192_chip_info {
> >  struct ad7192_state {
> >         const struct ad7192_chip_info   *chip_info;
> >         struct clk                      *mclk;
> > +       struct clk_hw                   int_clk_hw;
> >         u16                             int_vref_mv;
> >         u32                             aincom_mv;
> >         u32                             fclk;
> > @@ -401,6 +403,88 @@ static const char *const ad7192_clock_names[] = {
> >         "mclk"
> >  };
> > 
> > +static struct ad7192_state *clk_hw_to_ad7192(struct clk_hw *hw)
> > +{
> > +       return container_of(hw, struct ad7192_state, int_clk_hw);
> > +}
> > +
> > +static unsigned long ad7192_clk_recalc_rate(struct clk_hw *hw,
> > +                                           unsigned long parent_rate)
> > +{
> > +       return AD7192_INT_FREQ_MHZ;
> > +}
> > +
> > +static int ad7192_clk_output_is_enabled(struct clk_hw *hw)
> > +{
> > +       struct ad7192_state *st = clk_hw_to_ad7192(hw);
> > +
> > +       return st->clock_sel == AD7192_CLK_INT_CO;
> > +}
> > +
> > +static int ad7192_clk_prepare(struct clk_hw *hw)
> > +{
> > +       struct ad7192_state *st = clk_hw_to_ad7192(hw);
> > +       int ret;
> > +
> > +       st->mode &= ~AD7192_MODE_CLKSRC_MASK;
> > +       st->mode |= AD7192_CLK_INT_CO;
> > +
> > +       ret = ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode);
> > +       if (ret)
> > +               return ret;
> > +
> > +       st->clock_sel = AD7192_CLK_INT_CO;
> > +
> > +       return 0;
> > +}
> > +
> > +static void ad7192_clk_unprepare(struct clk_hw *hw)
> > +{
> > +       struct ad7192_state *st = clk_hw_to_ad7192(hw);
> > +       int ret;
> > +
> > +       st->mode &= ~AD7192_MODE_CLKSRC_MASK;
> > +       st->mode |= AD7192_CLK_INT;
> > +
> > +       ret = ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode);
> > +       if (ret)
> > +               return;
> > +
> > +       st->clock_sel = AD7192_CLK_INT;
> > +}
> > +
> > +static const struct clk_ops ad7192_int_clk_ops = {
> > +       .recalc_rate = ad7192_clk_recalc_rate,
> > +       .is_enabled = ad7192_clk_output_is_enabled,
> > +       .prepare = ad7192_clk_prepare,
> > +       .unprepare = ad7192_clk_unprepare,
> > +};
> > +
> > +static int ad7192_register_clk_provider(struct ad7192_state *st)
> > +{
> > +       struct device *dev = &st->sd.spi->dev;
> > +       struct clk_init_data init = {};
> > +       int ret;
> > +
> > +       if (!IS_ENABLED(CONFIG_COMMON_CLK))
> > +               return 0;
> > +
> > +       init.name = devm_kasprintf(dev, GFP_KERNEL, "%s-clk",
> > +                                  fwnode_get_name(dev_fwnode(dev)));
> > +       if (!init.name)
> > +               return -ENOMEM;
> > +
> > +       init.ops = &ad7192_int_clk_ops;
> > +
> > +       st->int_clk_hw.init = &init;
> > +       ret = devm_clk_hw_register(dev, &st->int_clk_hw);
> > +       if (ret)
> > +               return ret;
> > +
> > +       return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get,
> > +                                          &st->int_clk_hw);
> > +}
> > +
> >  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;

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á