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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1c897cf7-dc31-4e39-84c1-f8ab4b3e0aa8@baylibre.com>
Date: Tue, 29 Jul 2025 11:08:29 -0500
From: David Lechner <dlechner@...libre.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Michael Hennerich <Michael.Hennerich@...log.com>,
 Nuno Sá <nuno.sa@...log.com>,
 Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] iio: adc: ad7124: do not require mclk

On 7/27/25 7:21 AM, Jonathan Cameron wrote:
> On Thu, 24 Jul 2025 18:25:23 -0500
> David Lechner <dlechner@...libre.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.

> 
> 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");
>> +		}
>>  	}