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: <20241026183131.6c75ce56@jic23-huawei>
Date: Sat, 26 Oct 2024 18:31:31 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Angelo Dureghello <adureghello@...libre.com>
Cc: Nuno Sá <noname.nuno@...il.com>, Conor Dooley
 <conor@...nel.org>, Nuno Sá <nuno.sa@...log.com>, Lars-Peter
 Clausen <lars@...afoo.de>, Michael Hennerich
 <Michael.Hennerich@...log.com>, 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 Thu, 24 Oct 2024 12:29:29 +0200
Angelo Dureghello <adureghello@...libre.com> wrote:

> On 24.10.2024 09:04, Nuno Sá wrote:
> > On Wed, 2024-10-23 at 16:22 +0100, Conor Dooley wrote:  
> > > On Wed, Oct 23, 2024 at 04:56:39PM +0200, Nuno Sá wrote:  
> > > > 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?  
> > > 
> > > Did you miss the "for this variant"? Maybe I left the comment in not  
> > 
> > I guess so :)
> >   
> > > exactly the right place, but I don't think the code works correctly for
> > > the new variant if clock-names aren't provided:
> > > 
> > > +	if (st->info->has_dac_clk) {
> > > +		struct clk *dac_clk;
> > > +		dac_clk = devm_clk_get_enabled(&pdev->dev, "dac_clk");
> > > +		if (IS_ERR(dac_clk))
> > > +			return dev_err_probe(&pdev->dev, PTR_ERR(dac_clk),
> > > +					     "failed to get dac_clk clock\n");
> > > +
> > > +		/* We only care about the streaming mode rate */
> > > +		st->dac_clk_rate = clk_get_rate(dac_clk) / 2;
> > > 
> > > Isn't this going to cause a probe failure?  
> > 
> > Exactly. And that goes in line with what I wrote about the bindings not describing
> > (currently) this. So yes, for the new variant (which has 'has_dac_clk' set to true)
> > clock-names is indeed mandatory and probe will fail if it's not given.
> >   
> > >    
> > > > 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.  
> > > 
> > > Why not always just get the first clock by index and avoid the
> > > complexity?  
> > 
> > And that was also suggested in the previous version but then Jonathan suggested this
> > [1]. I agree things now are a bit confusing because we expect clock-names to be
> > optional for the generic but mandatory for this new variant and the code is not being
> > that explicit about it.
> >   

If we need all the clocks for a variant to work, then can instead constrain the clock
names if present.  So basically enforce the indexing.

I think just requiring clock-names for this variant is cleaner though!

Jonathan
> > >   
> > > > 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.  
> > > 
> > > Right. You can "edit" required in the if/then/else branch for the new
> > > variant.  
> > 
> > Yeah, and IMO that should be set in the bindings (it would help understanding what
> > the driver is actually doinfg.
> >  
> 
> ok, thanks, so 
> 
> so modified yaml in this way:
> 
>   clocks:
>     minItems: 1
>     maxItems: 2
> 
>   clock-names:
>     items:
>       - const: s_axi_aclk
>       - const: dac_clk
>     minItems: 1
> 
>   '#io-backend-cells':
>     const: 0
> 
> required:
>   - compatible
>   - dmas
>   - reg
>   - clocks
> 
> allOf:
>   - if:
>       properties:
>         compatible:
>           contains:
>             const: adi,axi-ad3552r
>     then:
>       $ref: /schemas/spi/spi-controller.yaml#
>       properties:
>         clocks:
>           minItems: 2
>         clock-names:
>           minItems: 2
>       required:
>         - clock-names
>     else:
>       properties:
>         clocks:
>           maxItems: 1
>         clock-names:
>           maxItems: 1
> 
>  
> > [1]: https://lore.kernel.org/linux-iio/20241019160817.10c3a2bf@jic23-huawei/
> > 
> > - Nuno Sá
> >   
> 
> Regards,
>   angelo
> 


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ