linux-kernel - Re: [PATCH v3 2/2] iio: adc: ad7192: Fix clock config

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]

Date: Wed, 12 Jun 2024 17:05:18 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: Alisa-Dariana Roman <alisadariana@...il.com>, Alisa-Dariana Roman
 <alisa.roman@...log.com>, Jonathan Cameron <Jonathan.Cameron@...wei.com>, 
 Michael Hennerich <michael.hennerich@...log.com>,
 linux-iio@...r.kernel.org, devicetree@...r.kernel.org, 
 linux-kernel@...r.kernel.org
Cc: Alexandru Tachici <alexandru.tachici@...log.com>, Lars-Peter Clausen
	 <lars@...afoo.de>, Jonathan Cameron <jic23@...nel.org>, Rob Herring
	 <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
	 <conor+dt@...nel.org>
Subject: Re: [PATCH v3 2/2] iio: adc: ad7192: Fix clock config

On Wed, 2024-06-12 at 17:16 +0300, Alisa-Dariana Roman 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.
> 
> 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@...log.com>
> ---

Hmmm, I did not looked at the datasheet but looked at the older implementation
and I'm not sure this is an actual fix. Can you elaborate on that?

So on the current implementation I can see that we have some properties that are
not documented:

adi,int-clock-output-enable
adi,clock-xtal

So, I see in your series that you're documenting adi,clock-xtal using clk-names.
I do think your code is cleaner but I don't think the older implementation to be
buggy. Am I missing something?

I can also see that you're ignoring AD7192_CLK_INT_CO... That's also removing
functionality from the driver even though the implementation is not as it should
be I think. If I understand that mode correctly, it's just about having the
internal clock in the MCLK pin. Effectively this would then be a clock provider
with a fixed rate of 4.92MHz. So I believe that exposing it as a clock provider
would likely be the way to go.

- Nuno Sá