linux-kernel - Re: [PATCH v7 4/8] iio: dac: adi-axi-dac: extend features

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: <7a4f8c718029c8c57596d950495fcf28562c6e78.camel@gmail.com>
Date: Wed, 23 Oct 2024 16:56:39 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: Conor Dooley <conor@...nel.org>
Cc: Angelo Dureghello <adureghello@...libre.com>, Nuno Sá
	 <nuno.sa@...log.com>, Lars-Peter Clausen <lars@...afoo.de>, Michael
 Hennerich	 <Michael.Hennerich@...log.com>, Jonathan Cameron
 <jic23@...nel.org>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
 <krzk+dt@...nel.org>, Conor Dooley	 <conor+dt@...nel.org>, Olivier Moysan
 <olivier.moysan@...s.st.com>, 	linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, 	linux-kernel@...r.kernel.org,
 dlechner@...libre.com, Mark Brown	 <broonie@...nel.org>
Subject: Re: [PATCH v7 4/8] iio: dac: adi-axi-dac: extend features

On Tue, 2024-10-22 at 18:21 +0100, Conor Dooley wrote:
> On Tue, Oct 22, 2024 at 02:36:44PM +0200, Nuno Sá wrote:
> > On Mon, 2024-10-21 at 14:40 +0200, Angelo Dureghello wrote:
> > > From: Angelo Dureghello <adureghello@...libre.com>
> > > 
> > > Extend AXI-DAC backend with new features required to interface
> > > to the ad3552r DAC. Mainly, a new compatible string is added to
> > > support the ad3552r-axi DAC IP, very similar to the generic DAC
> > > IP but with some customizations to work with the ad3552r.
> > > 
> > > Then, a series of generic functions has been added to match with
> > > ad3552r needs. Function names has been kept generic as much as
> > > possible, to allow re-utilization from other frontend drivers.
> > > 
> > > Signed-off-by: Angelo Dureghello <adureghello@...libre.com>
> > > ---
> > 
> > Looks mostly good,
> > 
> > one minor thing that (I think) could be improved
> > >  drivers/iio/dac/adi-axi-dac.c | 269
> > > +++++++++++++++++++++++++++++++++++++++--
> > > -
> > >  1 file changed, 255 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c
> > > index 04193a98616e..9d6809fe7a67 100644
> > > --- a/drivers/iio/dac/adi-axi-dac.c
> > > +++ b/drivers/iio/dac/adi-axi-dac.c
> > > @@ -46,9 +46,28 @@
> > >  #define AXI_DAC_CNTRL_1_REG			0x0044
> > >  #define   AXI_DAC_CNTRL_1_SYNC			BIT(0)
> > >  #define AXI_DAC_CNTRL_2_REG			0x0048
> > > +#define   AXI_DAC_CNTRL_2_SDR_DDR_N		BIT(16)
> > > +#define   AXI_DAC_CNTRL_2_SYMB_8B		BIT(14)
> > >  #define   ADI_DAC_CNTRL_2_R1_MODE		BIT(5)
> > > +#define   AXI_DAC_CNTRL_2_UNSIGNED_DATA		BIT(4)
> > > +#define AXI_DAC_STATUS_1_REG			0x0054
> > > +#define AXI_DAC_STATUS_2_REG			0x0058
> > >  #define AXI_DAC_DRP_STATUS_REG			0x0074
> > >  #define   AXI_DAC_DRP_STATUS_DRP_LOCKED		BIT(17)
> > > +#define AXI_DAC_CUSTOM_RD_REG			0x0080
> > > +#define AXI_DAC_CUSTOM_WR_REG			0x0084
> > > +#define   AXI_DAC_CUSTOM_WR_DATA_8		GENMASK(23, 16)
> > > +#define   AXI_DAC_CUSTOM_WR_DATA_16		GENMASK(23, 8)
> > > +#define AXI_DAC_UI_STATUS_REG			0x0088
> > > +#define   AXI_DAC_UI_STATUS_IF_BUSY		BIT(4)
> > > +#define AXI_DAC_CUSTOM_CTRL_REG			0x008C
> > > +#define   AXI_DAC_CUSTOM_CTRL_ADDRESS		GENMASK(31, 24)
> > > +#define   AXI_DAC_CUSTOM_CTRL_SYNCED_TRANSFER	BIT(2)
> > > +#define   AXI_DAC_CUSTOM_CTRL_STREAM		BIT(1)
> > > +#define   AXI_DAC_CUSTOM_CTRL_TRANSFER_DATA	BIT(0)
> > 
> > ...
> >  
> > >  static int axi_dac_probe(struct platform_device *pdev)
> > >  {
> > > -	const unsigned int *expected_ver;
> > >  	struct axi_dac_state *st;
> > >  	void __iomem *base;
> > >  	unsigned int ver;
> > > @@ -566,14 +780,29 @@ static int axi_dac_probe(struct platform_device
> > > *pdev)
> > >  	if (!st)
> > >  		return -ENOMEM;
> > >  
> > > -	expected_ver = device_get_match_data(&pdev->dev);
> > > -	if (!expected_ver)
> > > +	st->info = device_get_match_data(&pdev->dev);
> > > +	if (!st->info)
> > >  		return -ENODEV;
> > > +	clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk");
> > > +	if (IS_ERR(clk)) {
> > 
> > If clock-names is not given, then we'll get -EINVAL. Hence we could assume
> > that:
> > 
> > 		if (PTR_ERR(clk) != -EINVAL)
> > 			return dev_err_probe();
> 
> clock-names isn't a required property, but the driver code effectively
> makes it one. Doesn't this lookup need to be by index, unless
> clock-names is made required for this variant?

Likely I'm missing something but the driver is not making clock-names mandatory,
is it?

At least for the s_axi_aclk, we first try to get it using clock-names and if
that fails we backup to what we're doing which is passing NULL (which
effectively get's the first clock in the array).

The reasoning is that on the generic variant we only need the AXI clk and we
can't now enforce clock-names on it. But to keep things flexible, this was
purposed.

Another alternative that might have more lines of code (but simpler to
understand the intent) is to have (for example) a callback get_clocks function
that we set depending on the variant. And this also makes me realize that we
could improve the bindings. I mean, for the generic dac variant we do not need
clock-names but for this new variant, clock-names is mandatory and I'm fairly
sure we can express that in the bindings.

- Nuno Sá