Received-SPF: pass (google.com: domain of linux-kernel+bounces-211734-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <26b29525e41aad25e144f98994640585c4c8b705.camel@gmail.com>
Subject: Re: [PATCH v3 2/2] iio: adc: ad7192: Fix clock config
From: Nuno =?ISO-8859-1?Q?S=E1?= <noname.nuno@gmail.com>
To: Alisa-Dariana Roman <alisadariana@gmail.com>, 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
Cc: Alexandru Tachici <alexandru.tachici@analog.com>, Lars-Peter Clausen
	 <lars@metafoo.de>, Jonathan Cameron <jic23@kernel.org>, Rob Herring
	 <robh@kernel.org>, Krzysztof Kozlowski <krzk+dt@kernel.org>, Conor Dooley
	 <conor+dt@kernel.org>
Date: Wed, 12 Jun 2024 17:05:18 +0200
In-Reply-To: <20240612141637.175709-3-alisa.roman@analog.com>
References: <20240612141637.175709-1-alisa.roman@analog.com>
	 <20240612141637.175709-3-alisa.roman@analog.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.52.2 
Precedence: bulk
MIME-Version: 1.0

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.
>=20
> Note that the fix tag is for the commit that moved the driver out of
> staging.
>=20
> Fixes: b581f748cce0 ("staging: iio: adc: ad7192: move out of staging")
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> ---

Hmmm, I did not looked at the datasheet but looked at the older implementat=
ion
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 tha=
t 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-n=
ames.
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 removi=
ng
functionality from the driver even though the implementation is not as it s=
hould
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 prov=
ider
with a fixed rate of 4.92MHz. So I believe that exposing it as a clock prov=
ider
would likely be the way to go.

- Nuno S=C3=A1=20