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>
---
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)
+
+#define AXI_DAC_CUSTOM_CTRL_STREAM_ENABLE (AXI_DAC_CUSTOM_CTRL_TRANSFER_DATA | \
+ AXI_DAC_CUSTOM_CTRL_STREAM)
/* DAC Channel controls */
#define AXI_DAC_CHAN_CNTRL_1_REG(c) (0x0400 + (c) * 0x40)
@@ -63,12 +82,21 @@
#define AXI_DAC_CHAN_CNTRL_7_REG(c) (0x0418 + (c) * 0x40)
#define AXI_DAC_CHAN_CNTRL_7_DATA_SEL GENMASK(3, 0)
+#define AXI_DAC_RD_ADDR(x) (BIT(7) | (x))
+
/* 360 degrees in rad */
#define AXI_DAC_2_PI_MEGA 6283190
enum {
AXI_DAC_DATA_INTERNAL_TONE,
AXI_DAC_DATA_DMA = 2,
+ AXI_DAC_DATA_INTERNAL_RAMP_16BIT = 11,
+};
+
+struct axi_dac_info {
+ unsigned int version;
+ const struct iio_backend_info *backend_info;
+ bool has_dac_clk;
};
struct axi_dac_state {
@@ -79,9 +107,11 @@ struct axi_dac_state {
* data/variables.
*/
struct mutex lock;
+ const struct axi_dac_info *info;
u64 dac_clk;
u32 reg_config;
bool int_tone;
+ int dac_clk_rate;
};
static int axi_dac_enable(struct iio_backend *back)
@@ -471,6 +501,11 @@ static int axi_dac_data_source_set(struct iio_backend *back, unsigned int chan,
AXI_DAC_CHAN_CNTRL_7_REG(chan),
AXI_DAC_CHAN_CNTRL_7_DATA_SEL,
AXI_DAC_DATA_DMA);
+ case IIO_BACKEND_INTERNAL_RAMP_16BIT:
+ return regmap_update_bits(st->regmap,
+ AXI_DAC_CHAN_CNTRL_7_REG(chan),
+ AXI_DAC_CHAN_CNTRL_7_DATA_SEL,
+ AXI_DAC_DATA_INTERNAL_RAMP_16BIT);
default:
return -EINVAL;
}
@@ -528,6 +563,166 @@ static int axi_dac_reg_access(struct iio_backend *back, unsigned int reg,
return regmap_write(st->regmap, reg, writeval);
}
+static int axi_dac_ddr_enable(struct iio_backend *back)
+{
+ struct axi_dac_state *st = iio_backend_get_priv(back);
+
+ return regmap_clear_bits(st->regmap, AXI_DAC_CNTRL_2_REG,
+ AXI_DAC_CNTRL_2_SDR_DDR_N);
+}
+
+static int axi_dac_ddr_disable(struct iio_backend *back)
+{
+ struct axi_dac_state *st = iio_backend_get_priv(back);
+
+ return regmap_set_bits(st->regmap, AXI_DAC_CNTRL_2_REG,
+ AXI_DAC_CNTRL_2_SDR_DDR_N);
+}
+
+static int axi_dac_data_stream_enable(struct iio_backend *back)
+{
+ struct axi_dac_state *st = iio_backend_get_priv(back);
+
+ return regmap_set_bits(st->regmap, AXI_DAC_CUSTOM_CTRL_REG,
+ AXI_DAC_CUSTOM_CTRL_STREAM_ENABLE);
+}
+
+static int axi_dac_data_stream_disable(struct iio_backend *back)
+{
+ struct axi_dac_state *st = iio_backend_get_priv(back);
+
+ return regmap_clear_bits(st->regmap, AXI_DAC_CUSTOM_CTRL_REG,
+ AXI_DAC_CUSTOM_CTRL_STREAM_ENABLE);
+}
+
+static int axi_dac_data_transfer_addr(struct iio_backend *back, u32 address)
+{
+ struct axi_dac_state *st = iio_backend_get_priv(back);
+
+ if (address > FIELD_MAX(AXI_DAC_CUSTOM_CTRL_ADDRESS))
+ return -EINVAL;
+
+ /*
+ * Sample register address, when the DAC is configured, or stream
+ * start address when the FSM is in stream state.
+ */
+ return regmap_update_bits(st->regmap, AXI_DAC_CUSTOM_CTRL_REG,
+ AXI_DAC_CUSTOM_CTRL_ADDRESS,
+ FIELD_PREP(AXI_DAC_CUSTOM_CTRL_ADDRESS,
+ address));
+}
+
+static int axi_dac_data_format_set(struct iio_backend *back, unsigned int ch,
+ const struct iio_backend_data_fmt *data)
+{
+ struct axi_dac_state *st = iio_backend_get_priv(back);
+
+ switch (data->type) {
+ case IIO_BACKEND_DATA_UNSIGNED:
+ return regmap_clear_bits(st->regmap, AXI_DAC_CNTRL_2_REG,
+ AXI_DAC_CNTRL_2_UNSIGNED_DATA);
+ default:
+ return -EINVAL;
+ }
+}
+
+static int axi_dac_read_raw(struct iio_backend *back,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct axi_dac_state *st = iio_backend_get_priv(back);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_FREQUENCY:
+
+ if (!st->info->has_dac_clk)
+ return -EOPNOTSUPP;
+
+ /*
+ * Returning here always the maximum (buffering mode)
+ * clock rate.
+ */
+ *val = st->dac_clk_rate;
+
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int axi_dac_bus_reg_write(struct iio_backend *back, u32 reg, u32 val,
+ size_t data_size)
+{
+ struct axi_dac_state *st = iio_backend_get_priv(back);
+ int ret;
+ u32 ival;
+
+ /*
+ * Both AXI_DAC_CNTRL_2_REG and AXI_DAC_CUSTOM_WR_REG need to know
+ * the data size. So keeping data size control here only,
+ * since data size is mandatory for the current transfer.
+ * DDR state handled separately by specific backend calls,
+ * generally all raw register writes are SDR.
+ */
+ if (data_size == sizeof(u16))
+ ival = FIELD_PREP(AXI_DAC_CUSTOM_WR_DATA_16, val);
+ else
+ ival = FIELD_PREP(AXI_DAC_CUSTOM_WR_DATA_8, val);
+
+ ret = regmap_write(st->regmap, AXI_DAC_CUSTOM_WR_REG, ival);
+ if (ret)
+ return ret;
+
+ if (data_size == sizeof(u8))
+ ret = regmap_set_bits(st->regmap, AXI_DAC_CNTRL_2_REG,
+ AXI_DAC_CNTRL_2_SYMB_8B);
+ else
+ ret = regmap_clear_bits(st->regmap, AXI_DAC_CNTRL_2_REG,
+ AXI_DAC_CNTRL_2_SYMB_8B);
+ if (ret)
+ return ret;
+
+ ret = regmap_update_bits(st->regmap, AXI_DAC_CUSTOM_CTRL_REG,
+ AXI_DAC_CUSTOM_CTRL_ADDRESS,
+ FIELD_PREP(AXI_DAC_CUSTOM_CTRL_ADDRESS, reg));
+ if (ret)
+ return ret;
+
+ ret = regmap_update_bits(st->regmap, AXI_DAC_CUSTOM_CTRL_REG,
+ AXI_DAC_CUSTOM_CTRL_TRANSFER_DATA,
+ AXI_DAC_CUSTOM_CTRL_TRANSFER_DATA);
+ if (ret)
+ return ret;
+
+ ret = regmap_read_poll_timeout(st->regmap,
+ AXI_DAC_UI_STATUS_REG, ival,
+ FIELD_GET(AXI_DAC_UI_STATUS_IF_BUSY, ival) == 0,
+ 10, 100 * KILO);
+ if (ret == -ETIMEDOUT)
+ dev_err(st->dev, "AXI read timeout\n");
+
+ /* Cleaning always AXI_DAC_CUSTOM_CTRL_TRANSFER_DATA */
+ return regmap_clear_bits(st->regmap, AXI_DAC_CUSTOM_CTRL_REG,
+ AXI_DAC_CUSTOM_CTRL_TRANSFER_DATA);
+}
+
+static int axi_dac_bus_reg_read(struct iio_backend *back, u32 reg, u32 *val,
+ size_t data_size)
+{
+ struct axi_dac_state *st = iio_backend_get_priv(back);
+ int ret;
+
+ /*
+ * SPI, we write with read flag, then we read just at the AXI
+ * io address space to get data read.
+ */
+ ret = axi_dac_bus_reg_write(back, AXI_DAC_RD_ADDR(reg), 0, data_size);
+ if (ret)
+ return ret;
+
+ return regmap_read(st->regmap, AXI_DAC_CUSTOM_RD_REG, val);
+}
+
static const struct iio_backend_ops axi_dac_generic_ops = {
.enable = axi_dac_enable,
.disable = axi_dac_disable,
@@ -541,11 +736,31 @@ static const struct iio_backend_ops axi_dac_generic_ops = {
.debugfs_reg_access = iio_backend_debugfs_ptr(axi_dac_reg_access),
};
+static const struct iio_backend_ops axi_ad3552r_ops = {
+ .enable = axi_dac_enable,
+ .disable = axi_dac_disable,
+ .read_raw = axi_dac_read_raw,
+ .request_buffer = axi_dac_request_buffer,
+ .free_buffer = axi_dac_free_buffer,
+ .data_source_set = axi_dac_data_source_set,
+ .ddr_enable = axi_dac_ddr_enable,
+ .ddr_disable = axi_dac_ddr_disable,
+ .data_stream_enable = axi_dac_data_stream_enable,
+ .data_stream_disable = axi_dac_data_stream_disable,
+ .data_format_set = axi_dac_data_format_set,
+ .data_transfer_addr = axi_dac_data_transfer_addr,
+};
+
static const struct iio_backend_info axi_dac_generic = {
.name = "axi-dac",
.ops = &axi_dac_generic_ops,
};
+static const struct iio_backend_info axi_ad3552r = {
+ .name = "axi-ad3552r",
+ .ops = &axi_ad3552r_ops,
+};
+
static const struct regmap_config axi_dac_regmap_config = {
.val_bits = 32,
.reg_bits = 32,
@@ -555,7 +770,6 @@ static const struct regmap_config axi_dac_regmap_config = {
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)) {
+ /* Backward compat., old fdt versions without clock-names. */
+ clk = devm_clk_get_enabled(&pdev->dev, NULL);
+ if (IS_ERR(clk))
+ return dev_err_probe(&pdev->dev, PTR_ERR(clk),
+ "failed to get clock\n");
+ }
+
+ if (st->info->has_dac_clk) {
+ struct clk *dac_clk;
- clk = devm_clk_get_enabled(&pdev->dev, NULL);
- if (IS_ERR(clk))
- return dev_err_probe(&pdev->dev, PTR_ERR(clk),
- "failed to get clock\n");
+ 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;
+ }
base = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(base))
@@ -598,12 +827,13 @@ static int axi_dac_probe(struct platform_device *pdev)
if (ret)
return ret;
- if (ADI_AXI_PCORE_VER_MAJOR(ver) != ADI_AXI_PCORE_VER_MAJOR(*expected_ver)) {
+ if (ADI_AXI_PCORE_VER_MAJOR(ver) !=
+ ADI_AXI_PCORE_VER_MAJOR(st->info->version)) {
dev_err(&pdev->dev,
"Major version mismatch. Expected %d.%.2d.%c, Reported %d.%.2d.%c\n",
- ADI_AXI_PCORE_VER_MAJOR(*expected_ver),
- ADI_AXI_PCORE_VER_MINOR(*expected_ver),
- ADI_AXI_PCORE_VER_PATCH(*expected_ver),
+ ADI_AXI_PCORE_VER_MAJOR(st->info->version),
+ ADI_AXI_PCORE_VER_MINOR(st->info->version),
+ ADI_AXI_PCORE_VER_PATCH(st->info->version),
ADI_AXI_PCORE_VER_MAJOR(ver),
ADI_AXI_PCORE_VER_MINOR(ver),
ADI_AXI_PCORE_VER_PATCH(ver));
@@ -629,7 +859,8 @@ static int axi_dac_probe(struct platform_device *pdev)
return ret;
mutex_init(&st->lock);
- ret = devm_iio_backend_register(&pdev->dev, &axi_dac_generic, st);
+
+ ret = devm_iio_backend_register(&pdev->dev, st->info->backend_info, st);
if (ret)
return dev_err_probe(&pdev->dev, ret,
"failed to register iio backend\n");
@@ -642,10 +873,20 @@ static int axi_dac_probe(struct platform_device *pdev)
return 0;
}
-static unsigned int axi_dac_9_1_b_info = ADI_AXI_PCORE_VER(9, 1, 'b');
+static const struct axi_dac_info dac_generic = {
+ .version = ADI_AXI_PCORE_VER(9, 1, 'b'),
+ .backend_info = &axi_dac_generic,
+};
+
+static const struct axi_dac_info dac_ad3552r = {
+ .version = ADI_AXI_PCORE_VER(9, 1, 'b'),
+ .backend_info = &axi_ad3552r,
+ .has_dac_clk = true,
+};
static const struct of_device_id axi_dac_of_match[] = {
- { .compatible = "adi,axi-dac-9.1.b", .data = &axi_dac_9_1_b_info },
+ { .compatible = "adi,axi-dac-9.1.b", .data = &dac_generic },
+ { .compatible = "adi,axi-ad3552r", .data = &dac_ad3552r },
{}
};
MODULE_DEVICE_TABLE(of, axi_dac_of_match);
--
2.45.0.rc1On 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();
- Nuno Sá
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?
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@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?
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á
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@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?
>
> 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.
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@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?
> >
> > 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.
>
> > 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.
[1]: https://lore.kernel.org/linux-iio/20241019160817.10c3a2bf@jic23-huawei/
- Nuno Sá
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@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?
> > >
> > > 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.
>
> >
> > > 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
On Thu, 24 Oct 2024 12:29:29 +0200
Angelo Dureghello <adureghello@baylibre.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@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?
> > > >
> > > > 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
>
© 2016 - 2024 Red Hat, Inc.