Received-SPF: pass (google.com: domain of linux-kernel+bounces-207165-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Sat, 8 Jun 2024 18:49:58 +0100
From: Jonathan Cameron <jic23@kernel.org>
To: Alisa-Dariana Roman <alisadariana@gmail.com>
Cc: Alisa-Dariana Roman <alisa.roman@analog.com>, Jonathan Cameron
 <Jonathan.Cameron@huawei.com>, Michael Hennerich
 <michael.hennerich@analog.com>, linux-iio@vger.kernel.org,
 devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Lars-Peter
 Clausen <lars@metafoo.de>, Alexandru Tachici
 <alexandru.tachici@analog.com>, Rob Herring <robh@kernel.org>, Krzysztof
 Kozlowski <krzk+dt@kernel.org>, Conor Dooley <conor+dt@kernel.org>, Liam
 Girdwood <lgirdwood@gmail.com>, Mark Brown <broonie@kernel.org>
Subject: Re: [PATCH v2 3/3] iio: adc: ad7192: Fix clock config
Message-ID: <20240608184958.1c38516a@jic23-huawei>
In-Reply-To: <20240605075154.625123-3-alisa.roman@analog.com>
References: <20240605075154.625123-1-alisa.roman@analog.com>
	<20240605075154.625123-3-alisa.roman@analog.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Wed,  5 Jun 2024 10:51:54 +0300
Alisa-Dariana Roman <alisadariana@gmail.com> 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@analog.com>
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);