| Message ID | 20241014-wip-bl-ad3552r-axi-v0-iio-testing-v6-4-eeef0c1e0e56@baylibre.com (mailing list archive) |
|---|---|
| State | New |
| 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á > >
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);