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

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3260 bytes --]

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@baylibre.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@baylibre.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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]