| Message ID | 20241014-wip-bl-ad3552r-axi-v0-iio-testing-v6-4-eeef0c1e0e56@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: add support for the ad3552r AXI DAC IP | expand |
On 10/14/24 5:08 AM, 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 serie of generic functions has been added to match with spelling: series > 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> > --- ... > +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); > + int err, reg; > + > + switch (mask) { > + case IIO_CHAN_INFO_FREQUENCY: > + > + if (!st->info->has_dac_clk) > + return -EOPNOTSUPP; > + > + /* > + * As from ad3552r AXI IP documentation, > + * returning the SCLK depending on the stream mode. > + */ > + err = regmap_read(st->regmap, AXI_DAC_CUSTOM_CTRL_REG, ®); > + if (err) > + return err; > + > + if (reg & AXI_DAC_CUSTOM_CTRL_STREAM) > + *val = st->dac_clk_rate / 2; > + else > + *val = st->dac_clk_rate / 8; To get the DAC sample rate, we only care about the streaming mode rate, so this should just always be / 2 and not / 8. Otherwise the sampling_frequency attribute in the DAC driver will return the wrong value when the buffer is not enabled. We never do buffered writes without enabling streaming mode. > + > + 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; > + > + 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; > + > + /* > + * Both REG_CNTRL_2 and AXI_DAC_CNTRL_DATA_WR need to know I'm guessing these got renamed. REG_CNTRL_2 = AXI_DAC_CNTRL_2_REG and AXI_DAC_CNTRL_DATA_WR = AXI_DAC_CUSTOM_WR_REG? > + * 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(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_CUSTOM_CTRL_REG, ival, > + ival & AXI_DAC_CUSTOM_CTRL_TRANSFER_DATA, > + 10, 100 * KILO); > + if (ret) > + return ret; Should we also clear AXI_DAC_CUSTOM_CTRL_TRANSFER_DATA on timeout so that we don't leave things in a bad state? > + > + return regmap_clear_bits(st->regmap, AXI_DAC_CUSTOM_CTRL_REG, > + AXI_DAC_CUSTOM_CTRL_TRANSFER_DATA); > +} > + ... > 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,15 +793,26 @@ 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, NULL); > + clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk"); This will break existing users that don't have clock-names in the DT. It should be fine to leave it as NULL in which case it will get the clock at index 0 in the clocks array even if there is more than one clock. > 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; > + > + 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"); > + > + st->dac_clk_rate = clk_get_rate(dac_clk); > + } > + > base = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(base)) > return PTR_ERR(base);
On Mon, 2024-10-14 at 16:14 -0500, David Lechner wrote: > On 10/14/24 5:08 AM, 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 serie of generic functions has been added to match with > > spelling: series > > > 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> > > --- > > ... > > > +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); > > + int err, reg; > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_FREQUENCY: > > + > > + if (!st->info->has_dac_clk) > > + return -EOPNOTSUPP; > > + > > + /* > > + * As from ad3552r AXI IP documentation, > > + * returning the SCLK depending on the stream mode. > > + */ > > + err = regmap_read(st->regmap, AXI_DAC_CUSTOM_CTRL_REG, ®); > > + if (err) > > + return err; > > + > > + if (reg & AXI_DAC_CUSTOM_CTRL_STREAM) > > + *val = st->dac_clk_rate / 2; > > + else > > + *val = st->dac_clk_rate / 8; > > To get the DAC sample rate, we only care about the streaming mode > rate, so this should just always be / 2 and not / 8. Otherwise > the sampling_frequency attribute in the DAC driver will return > the wrong value when the buffer is not enabled. We never do buffered > writes without enabling streaming mode. But the question then is, what do we return when streaming mode is off? Dividing by 2 in that case won't report the actual SCLK. But you do have a point and I think a very common pattern from userspace is to first get the sampling frequency and only then starting buffering. In this case, yes, we get the wrong sampling frequency. Bottom line I agree with David and we should just care about returning the max sampling frequency which is the one that apps ultimately care about. So, I would say to divide it by 2 during probe and just return that value in here. - Nuno Sá > > > + > > + 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; > > + > > + 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; > > + > > + /* > > + * Both REG_CNTRL_2 and AXI_DAC_CNTRL_DATA_WR need to know > > I'm guessing these got renamed. REG_CNTRL_2 = AXI_DAC_CNTRL_2_REG > and AXI_DAC_CNTRL_DATA_WR = AXI_DAC_CUSTOM_WR_REG? > > > + * 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(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_CUSTOM_CTRL_REG, ival, > > + ival & AXI_DAC_CUSTOM_CTRL_TRANSFER_DATA, > > + 10, 100 * KILO); > > + if (ret) > > + return ret; > > Should we also clear AXI_DAC_CUSTOM_CTRL_TRANSFER_DATA on timeout > so that we don't leave things in a bad state? > > > + > > + return regmap_clear_bits(st->regmap, AXI_DAC_CUSTOM_CTRL_REG, > > + AXI_DAC_CUSTOM_CTRL_TRANSFER_DATA); > > +} > > + > > ... > > > 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,15 +793,26 @@ 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, NULL); > > + clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk"); > > This will break existing users that don't have clock-names > in the DT. It should be fine to leave it as NULL in which > case it will get the clock at index 0 in the clocks array > even if there is more than one clock. Good catch... - Nuno Sá
On 14.10.2024 16:14, David Lechner wrote: > On 10/14/24 5:08 AM, 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 serie of generic functions has been added to match with > > spelling: series > > > 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> > > --- > > ... > > > +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); > > + int err, reg; > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_FREQUENCY: > > + > > + if (!st->info->has_dac_clk) > > + return -EOPNOTSUPP; > > + > > + /* > > + * As from ad3552r AXI IP documentation, > > + * returning the SCLK depending on the stream mode. > > + */ > > + err = regmap_read(st->regmap, AXI_DAC_CUSTOM_CTRL_REG, ®); > > + if (err) > > + return err; > > + > > + if (reg & AXI_DAC_CUSTOM_CTRL_STREAM) > > + *val = st->dac_clk_rate / 2; > > + else > > + *val = st->dac_clk_rate / 8; > > To get the DAC sample rate, we only care about the streaming mode > rate, so this should just always be / 2 and not / 8. Otherwise > the sampling_frequency attribute in the DAC driver will return > the wrong value when the buffer is not enabled. We never do buffered > writes without enabling streaming mode. > > > + > > + 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; > > + > > + 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; > > + > > + /* > > + * Both REG_CNTRL_2 and AXI_DAC_CNTRL_DATA_WR need to know > > I'm guessing these got renamed. REG_CNTRL_2 = AXI_DAC_CNTRL_2_REG > and AXI_DAC_CNTRL_DATA_WR = AXI_DAC_CUSTOM_WR_REG? > > > + * 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(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_CUSTOM_CTRL_REG, ival, > > + ival & AXI_DAC_CUSTOM_CTRL_TRANSFER_DATA, > > + 10, 100 * KILO); > > + if (ret) > > + return ret; > > Should we also clear AXI_DAC_CUSTOM_CTRL_TRANSFER_DATA on timeout > so that we don't leave things in a bad state? > just realized this poll is wrong and unuseful. It's a check on a bit we just set. Check must be done in AXI_MSK_BUSY of AXI_REG_UI_STATUS. If it fails after 100msecs, looks like things are seriously blocked, not sure clearing any bit would help. > > + > > + return regmap_clear_bits(st->regmap, AXI_DAC_CUSTOM_CTRL_REG, > > + AXI_DAC_CUSTOM_CTRL_TRANSFER_DATA); > > +} > > + > > ... > > > 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,15 +793,26 @@ 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, NULL); > > + clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk"); > > This will break existing users that don't have clock-names > in the DT. It should be fine to leave it as NULL in which > case it will get the clock at index 0 in the clocks array > even if there is more than one clock. > mm, are there existing users except this hs driver right now ? Clock names are actually described in the example, and if missing, also retrieving "dac_clk" would fail. > > 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; > > + > > + 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"); > > + > > + st->dac_clk_rate = clk_get_rate(dac_clk); > > + } > > + > > base = devm_platform_ioremap_resource(pdev, 0); > > if (IS_ERR(base)) > > return PTR_ERR(base);
On Tue, 2024-10-15 at 10:57 +0200, Angelo Dureghello wrote: > On 14.10.2024 16:14, David Lechner wrote: > > On 10/14/24 5:08 AM, 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 serie of generic functions has been added to match with > > > > spelling: series > > > > > 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> > > > --- > > > > ... > > > > > +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); > > > + int err, reg; > > > + > > > + switch (mask) { > > > + case IIO_CHAN_INFO_FREQUENCY: > > > + > > > + if (!st->info->has_dac_clk) > > > + return -EOPNOTSUPP; > > > + > > > + /* > > > + * As from ad3552r AXI IP documentation, > > > + * returning the SCLK depending on the stream mode. > > > + */ > > > + err = regmap_read(st->regmap, AXI_DAC_CUSTOM_CTRL_REG, ®); > > > + if (err) > > > + return err; > > > + > > > + if (reg & AXI_DAC_CUSTOM_CTRL_STREAM) > > > + *val = st->dac_clk_rate / 2; > > > + else > > > + *val = st->dac_clk_rate / 8; > > > > To get the DAC sample rate, we only care about the streaming mode > > rate, so this should just always be / 2 and not / 8. Otherwise > > the sampling_frequency attribute in the DAC driver will return > > the wrong value when the buffer is not enabled. We never do buffered > > writes without enabling streaming mode. > > > > > + > > > + 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; > > > + > > > + 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; > > > + > > > + /* > > > + * Both REG_CNTRL_2 and AXI_DAC_CNTRL_DATA_WR need to know > > > > I'm guessing these got renamed. REG_CNTRL_2 = AXI_DAC_CNTRL_2_REG > > and AXI_DAC_CNTRL_DATA_WR = AXI_DAC_CUSTOM_WR_REG? > > > > > + * 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(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_CUSTOM_CTRL_REG, ival, > > > + ival & > > > AXI_DAC_CUSTOM_CTRL_TRANSFER_DATA, > > > + 10, 100 * KILO); > > > + if (ret) > > > + return ret; > > > > Should we also clear AXI_DAC_CUSTOM_CTRL_TRANSFER_DATA on timeout > > so that we don't leave things in a bad state? > > > > just realized this poll is wrong and unuseful. > It's a check on a bit we just set. > Check must be done in AXI_MSK_BUSY of AXI_REG_UI_STATUS. > > If it fails after 100msecs, looks like things are seriously blocked, > not sure clearing any bit would help. > > > > > + > > > + return regmap_clear_bits(st->regmap, AXI_DAC_CUSTOM_CTRL_REG, > > > + AXI_DAC_CUSTOM_CTRL_TRANSFER_DATA); > > > +} > > > + > > > > ... > > > > > 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,15 +793,26 @@ 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, NULL); > > > + clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk"); > > > > This will break existing users that don't have clock-names > > in the DT. It should be fine to leave it as NULL in which > > case it will get the clock at index 0 in the clocks array > > even if there is more than one clock. > > > > mm, are there existing users except this hs driver right now ? > > Clock names are actually described in the example, and if missing, > also retrieving "dac_clk" would fail. > There's already a frontend DAC using the generic DAC implementation. So, in theory, yes... We can already have users not setting clock-names in DT that would now fail to probe with this patch. David is only suggesting leaving this call to NULL. For dac_clk we do need the *id in devm_clk_get_enabled(). Maybe it would also be worth mentioning in the bindings that s_axi_aclk needs to be the first entry in clocks and clock-names for backward compatibility. - Nuno Sá > >
> > > > 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,15 +793,26 @@ 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, NULL); > > > > + clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk"); > > > > > > This will break existing users that don't have clock-names > > > in the DT. It should be fine to leave it as NULL in which > > > case it will get the clock at index 0 in the clocks array > > > even if there is more than one clock. > > > > > > > mm, are there existing users except this hs driver right now ? > > > > Clock names are actually described in the example, and if missing, > > also retrieving "dac_clk" would fail. > > > > There's already a frontend DAC using the generic DAC implementation. So, in theory, > yes... We can already have users not setting clock-names in DT that would now fail to > probe with this patch. David is only suggesting leaving this call to NULL. For > dac_clk we do need the *id in devm_clk_get_enabled(). > > Maybe it would also be worth mentioning in the bindings that s_axi_aclk needs to be > the first entry in clocks and clock-names for backward compatibility. Usual trick for this is match on clk name first and then fallback to no name to pick up old DT that didn't set clk names. That way should be no need constrain the order when it is specified. Slight hicup is new DT, old kernel. In which case maybe the wrong clock is started but I think you only have the multiple clocks for new cases so this should be fine. Just sprinkle some comments alongside the fallback code to say why it is there. Jonathan > > - Nuno Sá > > > >
diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c index 04193a98616e..b887c6343f96 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,181 @@ 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); + int err, reg; + + switch (mask) { + case IIO_CHAN_INFO_FREQUENCY: + + if (!st->info->has_dac_clk) + return -EOPNOTSUPP; + + /* + * As from ad3552r AXI IP documentation, + * returning the SCLK depending on the stream mode. + */ + err = regmap_read(st->regmap, AXI_DAC_CUSTOM_CTRL_REG, ®); + if (err) + return err; + + if (reg & AXI_DAC_CUSTOM_CTRL_STREAM) + *val = st->dac_clk_rate / 2; + else + *val = st->dac_clk_rate / 8; + + 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; + + 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; + + /* + * Both REG_CNTRL_2 and AXI_DAC_CNTRL_DATA_WR 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(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_CUSTOM_CTRL_REG, ival, + ival & AXI_DAC_CUSTOM_CTRL_TRANSFER_DATA, + 10, 100 * KILO); + if (ret) + return ret; + + 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; + u32 ival; + + /* + * 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; + + ret = regmap_read_poll_timeout(st->regmap, AXI_DAC_UI_STATUS_REG, ival, + FIELD_GET(AXI_DAC_UI_STATUS_IF_BUSY, ival) != + AXI_DAC_UI_STATUS_IF_BUSY, + 10, 100); + 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 +751,29 @@ 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, + .read_raw = axi_dac_read_raw, + .request_buffer = axi_dac_request_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 +783,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,15 +793,26 @@ 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, NULL); + clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk"); 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; + + 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"); + + st->dac_clk_rate = clk_get_rate(dac_clk); + } + base = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(base)) return PTR_ERR(base); @@ -598,12 +836,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 +868,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 +882,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);