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@xxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxx>
> > > ---
> >
> > 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á
