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@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?
Did you miss the "for this variant"? Maybe I left the comment in not
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?
> 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?
> 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.
Attachment:
signature.asc
Description: PGP signature
