Re: [PATCH 2/4] iio: adc: ad7124: do not require mclk

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 29 Jul 2025 11:08:29 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 7/27/25 7:21 AM, Jonathan Cameron wrote:
> > On Thu, 24 Jul 2025 18:25:23 -0500
> > David Lechner <dlechner@baylibre.com> wrote:
> >   
> 
> ...
> 
> >> @@ -1111,21 +1112,49 @@ static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
> >>  static int ad7124_setup(struct ad7124_state *st)
> >>  {
> >>  	struct device *dev = &st->sd.spi->dev;
> >> -	unsigned int fclk, power_mode;
> >> +	unsigned int power_mode;
> >> +	struct clk *mclk;
> >>  	int i, ret;
> >>  
> >> -	fclk = clk_get_rate(st->mclk);
> >> -	if (!fclk)
> >> -		return dev_err_probe(dev, -EINVAL, "Failed to get mclk rate\n");
> >> +	/*
> >> +	 * Always use full power mode for max performance. If needed, the driver
> >> +	 * could be adapted to use a dynamic power mode based on the requested
> >> +	 * output data rate.
> >> +	 */
> >> +	power_mode = AD7124_ADC_CONTROL_POWER_MODE_FULL;
> >>  
> >> -	/* The power mode changes the master clock frequency */
> >> -	power_mode = ad7124_find_closest_match(ad7124_master_clk_freq_hz,
> >> -					ARRAY_SIZE(ad7124_master_clk_freq_hz),
> >> -					fclk);
> >> -	if (fclk != ad7124_master_clk_freq_hz[power_mode]) {
> >> -		ret = clk_set_rate(st->mclk, fclk);
> >> -		if (ret)
> >> -			return dev_err_probe(dev, ret, "Failed to set mclk rate\n");
> >> +	/*
> >> +	 * HACK: This "mclk" business is needed for backwards compatibility with  
> > 
> > I'd drop the HACK bit of this. Whilst I understand the spirit of the comment
> > that term tends to make people try to 'fix' things ;)
> >   
> >> +	 * old devicetrees that specified a fake clock named "mclk" to select
> >> +	 * the power mode.
> >> +	 */
> >> +	mclk = devm_clk_get_optional_enabled(dev, "mclk");
> >> +	if (IS_ERR(mclk))
> >> +		return dev_err_probe(dev, PTR_ERR(mclk), "Failed to get mclk\n");
> >> +
> >> +	if (mclk) {
> >> +		unsigned long mclk_hz;
> >> +
> >> +		mclk_hz = clk_get_rate(mclk);
> >> +		if (!mclk_hz)
> >> +			return dev_err_probe(dev, -EINVAL, "Failed to get mclk rate\n");
> >> +
> >> +		/*
> >> +		 * This logic is a bit backwards, which is why it is considered
> >> +		 * a hack and is only here for backwards compatibility. The
> >> +		 * driver should be able to set the power mode as it sees fit
> >> +		 * and the f_clk/mclk rate should be dynamic accordingly. But
> >> +		 * here, we are selecting a fixed power mode based on the given
> >> +		 * "mclk" rate.  
> > 
> > My assumption is that someone had a board with a fixed rate clock on this pin.
> > So it might not be possible to have the driver do that adjustment.
> > If anyone ever adds that support, we'll have to be careful about handling fixed
> > clocks.  
> 
> In order to use an external clock, you have to program a register field to
> allow that. Since the driver isn't doing that, we can be sure that even if
> someone had an external clock, the driver was still using the internal clock.
Ah. That indeed solves this!

> 
> > 
> > This looks fine though.
> >   
> >> +		 */
> >> +		power_mode = ad7124_find_closest_match(ad7124_master_clk_freq_hz,
> >> +			ARRAY_SIZE(ad7124_master_clk_freq_hz), mclk_hz);
> >> +
> >> +		if (mclk_hz != ad7124_master_clk_freq_hz[power_mode]) {
> >> +			ret = clk_set_rate(mclk, mclk_hz);
> >> +			if (ret)
> >> +				return dev_err_probe(dev, ret, "Failed to set mclk rate\n");
> >> +		}
> >>  	}  
>