Re: [PATCH v3 07/10] iio: adc: ad4170: Add clock provider support

[David Lechner <dlechner@baylibre.com>]

On 5/13/25 7:35 AM, Marcelo Schmitt wrote:
> The AD4170 chip can use an externally supplied clock at the XTAL2 pin, or
> an external crystal connected to the XTAL1 and XTAL2 pins. Alternatively,
> the AD4170 can provide its 16 MHz internal clock at the XTAL2 pin. Extend
> the AD4170 driver so it effectively uses the provided external clock, if
> any, or supplies its own clock as a clock provider.

Is support for CLKDIV intentionally omitted? Might be worth mentioning
if that is the case.

> 
> Reviewed-by: Nuno Sá <nuno.sa@analog.com>
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> ---

...

> @@ -329,6 +348,9 @@ struct ad4170_state {
>  	struct completion completion;
>  	int pins_fn[AD4170_NUM_ANALOG_PINS];
>  	u32 int_pin_sel;
> +	struct clk_hw int_clk_hw;
> +	struct clk *ext_clk;

This isn't used outside of ad4170_clock_select() so can be made a local variable there.

> +	unsigned int clock_ctrl;
>  	/*
>  	 * DMA (thus cache coherency maintenance) requires the transfer buffers
>  	 * to live in their own cache lines.
> @@ -1656,6 +1678,120 @@ static int ad4170_parse_channels(struct iio_dev *indio_dev)
>  	return 0;
>  }
>  
> +static struct ad4170_state *clk_hw_to_ad4170(struct clk_hw *hw)
> +{
> +	return container_of(hw, struct ad4170_state, int_clk_hw);
> +}
> +
> +static unsigned long ad4170_sel_clk(struct ad4170_state *st,
> +				    unsigned int clk_sel)
> +{
> +	st->clock_ctrl &= ~AD4170_CLOCK_CTRL_CLOCKSEL_MSK;
> +	st->clock_ctrl |= FIELD_PREP(AD4170_CLOCK_CTRL_CLOCKSEL_MSK, clk_sel);

Do we need to claim direct mode here to avoid poking registers during
a buffered read?

> +	return regmap_write(st->regmap, AD4170_CLOCK_CTRL_REG, st->clock_ctrl);
> +}
> +
> +static unsigned long ad4170_clk_recalc_rate(struct clk_hw *hw,
> +					    unsigned long parent_rate)
> +{
> +	return AD4170_INT_CLOCK_16MHZ;
> +}
> +
> +static int ad4170_clk_output_is_enabled(struct clk_hw *hw)
> +{
> +	struct ad4170_state *st = clk_hw_to_ad4170(hw);
> +	u32 clk_sel;
> +
> +	clk_sel = FIELD_GET(AD4170_CLOCK_CTRL_CLOCKSEL_MSK, st->clock_ctrl);
> +	return clk_sel == AD4170_CLOCK_CTRL_CLOCKSEL_INT_OUT;
> +}
> +
> +static int ad4170_clk_output_prepare(struct clk_hw *hw)
> +{
> +	struct ad4170_state *st = clk_hw_to_ad4170(hw);
> +
> +	return ad4170_sel_clk(st, AD4170_CLOCK_CTRL_CLOCKSEL_INT_OUT);
> +}
> +
> +static void ad4170_clk_output_unprepare(struct clk_hw *hw)
> +{
> +	struct ad4170_state *st = clk_hw_to_ad4170(hw);
> +
> +	ad4170_sel_clk(st, AD4170_CLOCK_CTRL_CLOCKSEL_INT);
> +}
> +
> +static const struct clk_ops ad4170_int_clk_ops = {
> +	.recalc_rate = ad4170_clk_recalc_rate,
> +	.is_enabled = ad4170_clk_output_is_enabled,
> +	.prepare = ad4170_clk_output_prepare,
> +	.unprepare = ad4170_clk_output_unprepare,
> +};
> +
> +static int ad4170_register_clk_provider(struct iio_dev *indio_dev)
> +{
> +	struct ad4170_state *st = iio_priv(indio_dev);
> +	struct device *dev = indio_dev->dev.parent;
> +	struct clk_init_data init = {};
> +	int ret;
> +
> +	if (!IS_ENABLED(CONFIG_COMMON_CLK))

Driver depends on COMMON_CLK so isn't this dead code?

> +		return 0;
> +
> +	if (device_property_read_string(dev, "clock-output-names", &init.name)) {
> +		init.name = devm_kasprintf(dev, GFP_KERNEL, "%pfw",
> +					   dev_fwnode(dev));
> +		if (!init.name)
> +			return -ENOMEM;
> +	}
> +
> +	init.ops = &ad4170_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 ad4170_clock_select(struct iio_dev *indio_dev)
> +{
> +	struct ad4170_state *st = iio_priv(indio_dev);
> +	struct device *dev = &st->spi->dev;
> +	int ret;
> +
> +	st->mclk_hz = AD4170_INT_CLOCK_16MHZ;

Would make more sense to move this inside of the if (ret < 0) instead of
writing over it later.

> +	ret = device_property_match_property_string(dev, "clock-names",
> +						    ad4170_clk_sel,
> +						    ARRAY_SIZE(ad4170_clk_sel));
> +	if (ret < 0) {
> +		/* Use internal clock reference */
> +		st->clock_ctrl |= FIELD_PREP(AD4170_CLOCK_CTRL_CLOCKSEL_MSK,
> +					     AD4170_CLOCK_CTRL_CLOCKSEL_INT_OUT);

Seems like we would want to start in the AD4170_CLOCK_CTRL_CLOCKSEL_INT state.

Also, could skip registering clock provider if #clock-cells is not present.

> +		return ad4170_register_clk_provider(indio_dev);
> +	}
> +
> +	/* Use external clock reference */
> +	st->ext_clk = devm_clk_get_enabled(dev, ad4170_clk_sel[ret]);
> +	if (IS_ERR(st->ext_clk))
> +		return dev_err_probe(dev, PTR_ERR(st->ext_clk),
> +				     "Failed to get external clock\n");
> +
> +	st->clock_ctrl |= FIELD_PREP(AD4170_CLOCK_CTRL_CLOCKSEL_MSK,
> +				     AD4170_CLOCK_CTRL_CLOCKSEL_EXT + ret);
> +
> +	st->mclk_hz = clk_get_rate(st->ext_clk);
> +	if (st->mclk_hz < AD4170_EXT_CLOCK_MHZ_MIN ||
> +	    st->mclk_hz > AD4170_EXT_CLOCK_MHZ_MAX) {
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Invalid external clock frequency %u\n",
> +				     st->mclk_hz);
> +	}
> +
> +	return 0;
> +}
> +
>  static int ad4170_parse_firmware(struct iio_dev *indio_dev)
>  {
>  	struct ad4170_state *st = iio_priv(indio_dev);
> @@ -1663,7 +1799,13 @@ static int ad4170_parse_firmware(struct iio_dev *indio_dev)
>  	int reg_data, ret;
>  	unsigned int i;
>  
> -	st->mclk_hz = AD4170_INT_CLOCK_16MHZ;
> +	ret = ad4170_clock_select(indio_dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to setup device clock\n");
> +
> +	ret = regmap_write(st->regmap, AD4170_CLOCK_CTRL_REG, st->clock_ctrl);
> +	if (ret)
> +		return ret;

Why not just do the regmap_write() in ad4170_clock_select() to keep it all
together?

>  
>  	for (i = 0; i < AD4170_NUM_ANALOG_PINS; i++)
>  		st->pins_fn[i] = AD4170_PIN_UNASIGNED;