On Wed, 5 Jun 2024 10:51:54 +0300
Alisa-Dariana Roman <alisadariana@xxxxxxxxx> 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>
A few comments inline.
> ---
> 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 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)
As Nuno pointed out - new feature, separate patch.
> +{
> + 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);
> +}
> +
> 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,
I'm confused. Why do you need this if using devm_clk_get_enabled()?
I'd assume that did this for you.
> + st->ext_clk);
> + 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");
> }
>
> ret = ad7192_setup(indio_dev, dev);
