Re: [PATCH v2 3/3] iio: adc: ad7192: Fix clock config

From: Nuno Sá
Date: Wed Jun 05 2024 - 04:34:57 EST

Next message: Christoph Hellwig: "Re: [PATCH v7 4/9] block: Add core atomic write support"
Previous message: Breno Leitao: "Re: [PATCH] mpt3sas: Avoid test/set_bit() operating in non-allocated memory"
In reply to: Alisa-Dariana Roman: "[PATCH v2 3/3] iio: adc: ad7192: Fix clock config"
Next in thread: Alisa-Dariana Roman: "Re: [PATCH v2 1/3] iio: adc: ad7192: Clean up dev"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, 2024-06-05 at 10:51 +0300, Alisa-Dariana Roman wrote:
> There are actually 4 configuration modes of clock source for AD719X
> devices. Either a crystal can be attached externally between MCLK1 and
> MCLK2 pins, or an external CMOS-compatible clock can drive the MCLK2
> pin. The other 2 modes make use of the 4.92MHz internal clock, which can
> be made available on the MCLK2 pin.
>
> Rename mclk to ext_clk for clarity.
>
> Note that the fix tag is for the commit that moved the driver out of
> staging.
>
> Fixes: b581f748cce0 ("staging: iio: adc: ad7192: move out of staging")
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@xxxxxxxxxx>
> ---
> drivers/iio/adc/ad7192.c | 153 ++++++++++++++++++++++++++++++---------
> 1 file changed, 119 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index f06cb7ac4b42..75b0724142b1 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>
> @@ -202,7 +203,8 @@ struct ad7192_state {
> const struct ad7192_chip_info *chip_info;
> struct regulator *avdd;
> struct regulator *vref;
> - struct clk *mclk;
> + struct clk *ext_clk;
> + struct clk_hw int_clk_hw;
> u16 int_vref_mv;
> u32 aincom_mv;
> u32 fclk;
> @@ -398,27 +400,6 @@ static inline bool ad7192_valid_external_frequency(u32
> freq)
> freq <= AD7192_EXT_FREQ_MHZ_MAX);
> }
>
> -static int ad7192_clock_select(struct ad7192_state *st)
> -{
> - struct device *dev = &st->sd.spi->dev;
> - unsigned int clock_sel;
> -
> - clock_sel = AD7192_CLK_INT;
> -
> - /* use internal clock */
> - if (!st->mclk) {
> - if (device_property_read_bool(dev, "adi,int-clock-output-
> enable"))
> - clock_sel = AD7192_CLK_INT_CO;
> - } else {
> - if (device_property_read_bool(dev, "adi,clock-xtal"))
> - clock_sel = AD7192_CLK_EXT_MCLK1_2;
> - else
> - clock_sel = AD7192_CLK_EXT_MCLK2;
> - }
> -
> - return clock_sel;
> -}
> -
> static int ad7192_setup(struct iio_dev *indio_dev, struct device *dev)
> {
> struct ad7192_state *st = iio_priv(indio_dev);
> @@ -1194,6 +1175,96 @@ static void ad7192_reg_disable(void *reg)
> regulator_disable(reg);
> }
>
> +static const char *const ad7192_clock_names[] = {
> + "xtal",
> + "clk"
> +};

This could be moved closer where it will be used...

> +
> +static struct ad7192_state *clk_hw_to_ad7192(struct clk_hw *hw)
> +{
> + return container_of(hw, struct ad7192_state, int_clk_hw);
> +}
> +
> +static void ad7192_clk_disable_unprepare(void *clk)
> +{
> + clk_disable_unprepare(clk);
> +}
> +
> +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 iio_dev *indio_dev)
> +{
> + struct ad7192_state *st = iio_priv(indio_dev);
> + struct device *dev = indio_dev->dev.parent;
> + struct fwnode_handle *fwnode = dev_fwnode(dev);
> + struct clk_init_data init = {};
> + int ret;
> +
> + if (!IS_ENABLED(CONFIG_COMMON_CLK))
> + return 0;
> +
> + init.name = fwnode_get_name(fwnode);
> + 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);
> +}

The above code is unrelated... Should be in another patch (also needs changes in
the bindings). It's also a new feature which does not match much with the series
subject :)

> +
> static int ad7192_probe(struct spi_device *spi)
> {
> struct device *dev = &spi->dev;
> @@ -1312,20 +1383,34 @@ static int ad7192_probe(struct spi_device *spi)
>
> st->fclk = AD7192_INT_FREQ_MHZ;
>
> - st->mclk = devm_clk_get_optional_enabled(dev, "mclk");
> - if (IS_ERR(st->mclk))
> - return PTR_ERR(st->mclk);
> + ret = device_property_match_property_string(dev, "clock-names",
> +     ad7192_clock_names,
> +
> ARRAY_SIZE(ad7192_clock_names));
> + if (ret < 0) {
> + st->clock_sel = AD7192_CLK_INT;
> + st->fclk = AD7192_INT_FREQ_MHZ;
>
> - st->clock_sel = ad7192_clock_select(st);
> + ret = ad7192_register_clk_provider(indio_dev);
> + if (ret)
> + return dev_err_probe(dev, ret,
> +      "Registration of clock provider
> failed\n");
> + } else {
> + st->clock_sel = AD7192_CLK_EXT_MCLK1_2 + ret;
>
> - if (st->clock_sel == AD7192_CLK_EXT_MCLK1_2 ||
> -     st->clock_sel == AD7192_CLK_EXT_MCLK2) {
> - st->fclk = clk_get_rate(st->mclk);
> - if (!ad7192_valid_external_frequency(st->fclk)) {
> - dev_err(dev,
> - "External clock frequency out of bounds\n");
> - return -EINVAL;
> - }
> + st->ext_clk = devm_clk_get_enabled(dev,
> ad7192_clock_names[ret]);
> + if (IS_ERR(st->ext_clk))
> + return PTR_ERR(st->ext_clk);
> +
> + ret = devm_add_action_or_reset(dev,
> +        ad7192_clk_disable_unprepare,
> +        st->ext_clk);

No need for this... Check what devm_clk_get_enabled() is doing :)

> + if (ret)
> + return ret;
> +
> + st->fclk = clk_get_rate(st->ext_clk);
> + if (!ad7192_valid_external_frequency(st->fclk))
> + return dev_err_probe(dev, -EINVAL,
> +      "External clock frequency out of
> bounds\n");

Maybe the above could be placed in a proper setup function... Like renaming
ad7192_clock_select() -> ad7192_clock_setup()?

One other thing is, if this is a fix, then it should come first in the series.
The reasoning is that we may want to backport the fix but there's no reason to
backport unneeded patches like your first patch that's only about cosmetics.

- Nuno Sá

Next message: Christoph Hellwig: "Re: [PATCH v7 4/9] block: Add core atomic write support"
Previous message: Breno Leitao: "Re: [PATCH] mpt3sas: Avoid test/set_bit() operating in non-allocated memory"
In reply to: Alisa-Dariana Roman: "[PATCH v2 3/3] iio: adc: ad7192: Fix clock config"
Next in thread: Alisa-Dariana Roman: "Re: [PATCH v2 1/3] iio: adc: ad7192: Clean up dev"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]