| Message ID | 20240624124941.113010-6-alisa.roman@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: ad7192: Improvements | expand |
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. > } else { > st->clock_sel = AD7192_CLK_EXT_MCLK1_2 + ret; > > -- > 2.34.1 > >
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á
> > > + > > > 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á > >
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á > > > > >
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"); } else { st->clock_sel = AD7192_CLK_EXT_MCLK1_2 + ret;
Internal clock of AD719X devices can be made available on MCLK2 pin. Add clock provider to support this functionality. Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com> --- drivers/iio/adc/ad7192.c | 89 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 89 insertions(+)