From: Angelo Dureghello <adureghello@baylibre.com>
The ad3552r can be feeded from the HDL controller by an internally
generated 16bit ramp, useful for debug pourposes. Add debugfs a file
to enable or disable it.
Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
---
drivers/iio/dac/ad3552r-hs.c | 106 ++++++++++++++++++++++++++++++++++++++++---
1 file changed, 100 insertions(+), 6 deletions(-)
diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c
index 37397e188f225a8099745ec03f7c604da76960b1..41fe78d982a68980db059b095fc27b37ea1a461b 100644
--- a/drivers/iio/dac/ad3552r-hs.c
+++ b/drivers/iio/dac/ad3552r-hs.c
@@ -7,6 +7,7 @@
*/
#include <linux/bitfield.h>
+#include <linux/debugfs.h>
#include <linux/delay.h>
#include <linux/gpio/consumer.h>
#include <linux/iio/backend.h>
@@ -65,6 +66,18 @@ static int ad3552r_hs_reg_read(struct ad3552r_hs_state *st, u32 reg, u32 *val,
return st->data->bus_reg_read(st->back, reg, val, xfer_size);
}
+static int ad3552r_hs_set_data_source(struct ad3552r_hs_state *st,
+ enum iio_backend_data_source type)
+{
+ int ret;
+
+ ret = iio_backend_data_source_set(st->back, 0, type);
+ if (ret)
+ return ret;
+
+ return iio_backend_data_source_set(st->back, 1, type);
+}
+
static int ad3552r_hs_update_reg_bits(struct ad3552r_hs_state *st, u32 reg,
u32 mask, u32 val, size_t xfer_size)
{
@@ -483,6 +496,66 @@ static int ad3552r_hs_reg_access(struct iio_dev *indio_dev, unsigned int reg,
return st->data->bus_reg_write(st->back, reg, writeval, 1);
}
+static ssize_t ad3552r_hs_show_data_source(struct file *f, char __user *userbuf,
+ size_t count, loff_t *ppos)
+{
+ struct ad3552r_hs_state *st = file_inode(f)->i_private;
+ enum iio_backend_data_source type;
+ int ret;
+
+ ret = iio_backend_data_source_get(st->back, 0, &type);
+ if (ret)
+ return ret;
+
+ switch (type) {
+ case IIO_BACKEND_INTERNAL_RAMP_16BIT:
+ return simple_read_from_buffer(userbuf, count, ppos,
+ "backend-ramp-generator", 22);
+ case IIO_BACKEND_EXTERNAL:
+ return simple_read_from_buffer(userbuf, count, ppos,
+ "iio-buffer", 10);
+ default:
+ return -EINVAL;
+ }
+}
+
+static ssize_t ad3552r_hs_write_data_source(struct file *f,
+ const char __user *userbuf,
+ size_t count, loff_t *ppos)
+{
+ struct ad3552r_hs_state *st = file_inode(f)->i_private;
+ char buf[64];
+ int ret;
+
+ ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf,
+ count);
+ if (ret < 0)
+ return ret;
+
+ buf[count] = 0;
+
+ if (count == 10 && !strncmp(buf, "iio-buffer", 10)) {
+ ret = ad3552r_hs_set_data_source(st, IIO_BACKEND_EXTERNAL);
+ if (ret)
+ return ret;
+ } else if (count == 22 && !strncmp(buf, "backend-ramp-generator", 22)) {
+ ret = ad3552r_hs_set_data_source(st,
+ IIO_BACKEND_INTERNAL_RAMP_16BIT);
+ if (ret)
+ return ret;
+ } else {
+ return -EINVAL;
+ }
+
+ return count;
+}
+
+static const struct file_operations ad3552r_hs_data_source_fops = {
+ .owner = THIS_MODULE,
+ .write = ad3552r_hs_write_data_source,
+ .read = ad3552r_hs_show_data_source,
+};
+
static int ad3552r_hs_setup(struct ad3552r_hs_state *st)
{
u16 id;
@@ -550,11 +623,7 @@ static int ad3552r_hs_setup(struct ad3552r_hs_state *st)
if (ret)
return ret;
- ret = iio_backend_data_source_set(st->back, 0, IIO_BACKEND_EXTERNAL);
- if (ret)
- return ret;
-
- ret = iio_backend_data_source_set(st->back, 1, IIO_BACKEND_EXTERNAL);
+ ret = ad3552r_hs_set_data_source(st, IIO_BACKEND_EXTERNAL);
if (ret)
return ret;
@@ -661,6 +730,24 @@ static const struct iio_info ad3552r_hs_info = {
.debugfs_reg_access = &ad3552r_hs_reg_access,
};
+static void ad3552r_hs_debugfs_init(struct iio_dev *indio_dev)
+{
+ struct ad3552r_hs_state *st = iio_priv(indio_dev);
+ struct dentry *d = iio_get_debugfs_dentry(indio_dev);
+
+ if (!IS_ENABLED(CONFIG_DEBUG_FS))
+ return;
+
+ d = iio_get_debugfs_dentry(indio_dev);
+ if (!d) {
+ dev_warn(st->dev, "can't set debugfs in driver dir\n");
+ return;
+ }
+
+ debugfs_create_file("data_source", 0600, d, st,
+ &ad3552r_hs_data_source_fops);
+}
+
static int ad3552r_hs_probe(struct platform_device *pdev)
{
struct ad3552r_hs_state *st;
@@ -705,7 +792,14 @@ static int ad3552r_hs_probe(struct platform_device *pdev)
if (ret)
return ret;
- return devm_iio_device_register(&pdev->dev, indio_dev);
+ ret = devm_iio_device_register(&pdev->dev, indio_dev);
+ if (ret)
+ return ret;
+
+ ad3552r_hs_debugfs_init(indio_dev);
+
+ return ret;
+
}
static const struct of_device_id ad3552r_hs_of_id[] = {
--
2.49.0On Fri, 21 Mar 2025 21:28:51 +0100
Angelo Dureghello <adureghello@baylibre.com> wrote:
> From: Angelo Dureghello <adureghello@baylibre.com>
>
> The ad3552r can be feeded from the HDL controller by an internally
> generated 16bit ramp, useful for debug pourposes. Add debugfs a file
> to enable or disable it.
>
> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> ---
> drivers/iio/dac/ad3552r-hs.c | 106 ++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 100 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c
> index 37397e188f225a8099745ec03f7c604da76960b1..41fe78d982a68980db059b095fc27b37ea1a461b 100644
> --- a/drivers/iio/dac/ad3552r-hs.c
> +++ b/drivers/iio/dac/ad3552r-hs.c
> @@ -7,6 +7,7 @@
> */
>
> #include <linux/bitfield.h>
> +#include <linux/debugfs.h>
> #include <linux/delay.h>
> #include <linux/gpio/consumer.h>
> #include <linux/iio/backend.h>
> @@ -65,6 +66,18 @@ static int ad3552r_hs_reg_read(struct ad3552r_hs_state *st, u32 reg, u32 *val,
> return st->data->bus_reg_read(st->back, reg, val, xfer_size);
> }
>
> +static int ad3552r_hs_set_data_source(struct ad3552r_hs_state *st,
> + enum iio_backend_data_source type)
> +{
> + int ret;
> +
> + ret = iio_backend_data_source_set(st->back, 0, type);
> + if (ret)
> + return ret;
> +
> + return iio_backend_data_source_set(st->back, 1, type);
> +}
> +
> static int ad3552r_hs_update_reg_bits(struct ad3552r_hs_state *st, u32 reg,
> u32 mask, u32 val, size_t xfer_size)
> {
> @@ -483,6 +496,66 @@ static int ad3552r_hs_reg_access(struct iio_dev *indio_dev, unsigned int reg,
> return st->data->bus_reg_write(st->back, reg, writeval, 1);
> }
>
> +static ssize_t ad3552r_hs_show_data_source(struct file *f, char __user *userbuf,
> + size_t count, loff_t *ppos)
> +{
> + struct ad3552r_hs_state *st = file_inode(f)->i_private;
> + enum iio_backend_data_source type;
> + int ret;
> +
> + ret = iio_backend_data_source_get(st->back, 0, &type);
> + if (ret)
> + return ret;
> +
> + switch (type) {
> + case IIO_BACKEND_INTERNAL_RAMP_16BIT:
> + return simple_read_from_buffer(userbuf, count, ppos,
> + "backend-ramp-generator", 22);
Probably better to use a const string and then you can use strlen() to get
the length from that. I don't much like counting characters!
> + case IIO_BACKEND_EXTERNAL:
> + return simple_read_from_buffer(userbuf, count, ppos,
> + "iio-buffer", 10);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static ssize_t ad3552r_hs_write_data_source(struct file *f,
> + const char __user *userbuf,
> + size_t count, loff_t *ppos)
> +{
> + struct ad3552r_hs_state *st = file_inode(f)->i_private;
> + char buf[64];
> + int ret;
> +
> + ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf,
> + count);
> + if (ret < 0)
> + return ret;
> +
> + buf[count] = 0;
> +
> + if (count == 10 && !strncmp(buf, "iio-buffer", 10)) {
> + ret = ad3552r_hs_set_data_source(st, IIO_BACKEND_EXTERNAL);
> + if (ret)
> + return ret;
> + } else if (count == 22 && !strncmp(buf, "backend-ramp-generator", 22)) {
As above, I'd like to see some strlen() on const strings for this.
FWIW strncmp doesn't care about NULL terminators as such so just ensure you only
compare the characters.
> + ret = ad3552r_hs_set_data_source(st,
> + IIO_BACKEND_INTERNAL_RAMP_16BIT);
> + if (ret)
> + return ret;
> + } else {
> + return -EINVAL;
> + }
> +
> + return count;
> +}
...
Thanks,
Jonathan
On Fri, 2025-03-21 at 21:28 +0100, Angelo Dureghello wrote:
> From: Angelo Dureghello <adureghello@baylibre.com>
>
> The ad3552r can be feeded from the HDL controller by an internally
> generated 16bit ramp, useful for debug pourposes. Add debugfs a file
> to enable or disable it.
>
> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> ---
> drivers/iio/dac/ad3552r-hs.c | 106 ++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 100 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c
> index
> 37397e188f225a8099745ec03f7c604da76960b1..41fe78d982a68980db059b095fc27b37ea1a461b
> 100644
> --- a/drivers/iio/dac/ad3552r-hs.c
> +++ b/drivers/iio/dac/ad3552r-hs.c
> @@ -7,6 +7,7 @@
> */
>
> #include <linux/bitfield.h>
> +#include <linux/debugfs.h>
> #include <linux/delay.h>
> #include <linux/gpio/consumer.h>
> #include <linux/iio/backend.h>
> @@ -65,6 +66,18 @@ static int ad3552r_hs_reg_read(struct ad3552r_hs_state *st, u32
> reg, u32 *val,
> return st->data->bus_reg_read(st->back, reg, val, xfer_size);
> }
>
> +static int ad3552r_hs_set_data_source(struct ad3552r_hs_state *st,
> + enum iio_backend_data_source type)
> +{
> + int ret;
> +
> + ret = iio_backend_data_source_set(st->back, 0, type);
> + if (ret)
> + return ret;
> +
> + return iio_backend_data_source_set(st->back, 1, type);
>
I know it's a debug thing but we could use some locking in the above...
> +}
> +
> static int ad3552r_hs_update_reg_bits(struct ad3552r_hs_state *st, u32 reg,
> u32 mask, u32 val, size_t xfer_size)
> {
> @@ -483,6 +496,66 @@ static int ad3552r_hs_reg_access(struct iio_dev *indio_dev,
> unsigned int reg,
> return st->data->bus_reg_write(st->back, reg, writeval, 1);
> }
>
> +static ssize_t ad3552r_hs_show_data_source(struct file *f, char __user *userbuf,
> + size_t count, loff_t *ppos)
> +{
> + struct ad3552r_hs_state *st = file_inode(f)->i_private;
> + enum iio_backend_data_source type;
> + int ret;
> +
> + ret = iio_backend_data_source_get(st->back, 0, &type);
> + if (ret)
> + return ret;
> +
> + switch (type) {
> + case IIO_BACKEND_INTERNAL_RAMP_16BIT:
> + return simple_read_from_buffer(userbuf, count, ppos,
> + "backend-ramp-generator", 22);
> + case IIO_BACKEND_EXTERNAL:
> + return simple_read_from_buffer(userbuf, count, ppos,
> + "iio-buffer", 10);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static ssize_t ad3552r_hs_write_data_source(struct file *f,
> + const char __user *userbuf,
> + size_t count, loff_t *ppos)
> +{
> + struct ad3552r_hs_state *st = file_inode(f)->i_private;
> + char buf[64];
> + int ret;
> +
> + ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf,
> + count);
> + if (ret < 0)
> + return ret;
> +
> + buf[count] = 0;
> +
> + if (count == 10 && !strncmp(buf, "iio-buffer", 10)) {
> + ret = ad3552r_hs_set_data_source(st, IIO_BACKEND_EXTERNAL);
> + if (ret)
> + return ret;
> + } else if (count == 22 && !strncmp(buf, "backend-ramp-generator", 22)) {
> + ret = ad3552r_hs_set_data_source(st,
> + IIO_BACKEND_INTERNAL_RAMP_16BIT);
> + if (ret)
> + return ret;
> + } else {
> + return -EINVAL;
> + }
Are we expected to add more data types in the future? If not, this could be simply an
enable/disable ramp generator thing... It would be much simpler.
Anyways, you could define a static array and use match_string()?
Lastly, for insterfaces like this, it's always helpful to have an _available kind of
attribute.
- Nuno Sá
>
> static const struct of_device_id ad3552r_hs_of_id[] = {
>
On 3/28/25 3:28 AM, Nuno Sá wrote:
> On Fri, 2025-03-21 at 21:28 +0100, Angelo Dureghello wrote:
>> From: Angelo Dureghello <adureghello@baylibre.com>
>>
>> The ad3552r can be feeded from the HDL controller by an internally
>> generated 16bit ramp, useful for debug pourposes. Add debugfs a file
>> to enable or disable it.
>>
>> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
>> ---
...
>> +static ssize_t ad3552r_hs_write_data_source(struct file *f,
>> + const char __user *userbuf,
>> + size_t count, loff_t *ppos)
>> +{
>> + struct ad3552r_hs_state *st = file_inode(f)->i_private;
>> + char buf[64];
>> + int ret;
>> +
>> + ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf,
>> + count);
>> + if (ret < 0)
>> + return ret;
>> +
>> + buf[count] = 0;
>> +
>> + if (count == 10 && !strncmp(buf, "iio-buffer", 10)) {
>> + ret = ad3552r_hs_set_data_source(st, IIO_BACKEND_EXTERNAL);
>> + if (ret)
>> + return ret;
>> + } else if (count == 22 && !strncmp(buf, "backend-ramp-generator", 22)) {
>> + ret = ad3552r_hs_set_data_source(st,
>> + IIO_BACKEND_INTERNAL_RAMP_16BIT);
>> + if (ret)
>> + return ret;
>> + } else {
>> + return -EINVAL;
>> + }
>
> Are we expected to add more data types in the future? If not, this could be simply an
> enable/disable ramp generator thing... It would be much simpler.
Angelo actually had implemented it that way originally. :-)
I suggested to change it to a string because the HDL project for this family
of DACs actually has 3 possibilities for the data source:
* Selectable input source: DMA/ADC/TEST_RAMP;
And there are other potential sources from the generic AXI DAC like
0x00: internal tone (DDS) that seems somewhat likely to be seen in the future.
>
> Anyways, you could define a static array and use match_string()?
>
> Lastly, for insterfaces like this, it's always helpful to have an _available kind of
> attribute.
>
> - Nuno Sá
>
>
>
>>
>> static const struct of_device_id ad3552r_hs_of_id[] = {
>>
>
>
Hello Angelo,
Patch looks good to me.
One minor comment bellow.
On 03/21, Angelo Dureghello wrote:
> From: Angelo Dureghello <adureghello@baylibre.com>
>
> The ad3552r can be feeded from the HDL controller by an internally
> generated 16bit ramp, useful for debug pourposes. Add debugfs a file
> to enable or disable it.
>
> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> ---
> drivers/iio/dac/ad3552r-hs.c | 106 ++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 100 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c
> index 37397e188f225a8099745ec03f7c604da76960b1..41fe78d982a68980db059b095fc27b37ea1a461b 100644
> --- a/drivers/iio/dac/ad3552r-hs.c
> +++ b/drivers/iio/dac/ad3552r-hs.c
> @@ -7,6 +7,7 @@
...
> +
> +static ssize_t ad3552r_hs_write_data_source(struct file *f,
> + const char __user *userbuf,
> + size_t count, loff_t *ppos)
> +{
> + struct ad3552r_hs_state *st = file_inode(f)->i_private;
> + char buf[64];
> + int ret;
> +
> + ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf,
> + count);
> + if (ret < 0)
> + return ret;
> +
> + buf[count] = 0;
Shouldn't it be
buf[count] = '\0';
?
On 26.03.2025 18:52, Marcelo Schmitt wrote:
> Hello Angelo,
>
> Patch looks good to me.
> One minor comment bellow.
>
> On 03/21, Angelo Dureghello wrote:
> > From: Angelo Dureghello <adureghello@baylibre.com>
> >
> > The ad3552r can be feeded from the HDL controller by an internally
> > generated 16bit ramp, useful for debug pourposes. Add debugfs a file
> > to enable or disable it.
> >
> > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> > ---
> > drivers/iio/dac/ad3552r-hs.c | 106 ++++++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 100 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c
> > index 37397e188f225a8099745ec03f7c604da76960b1..41fe78d982a68980db059b095fc27b37ea1a461b 100644
> > --- a/drivers/iio/dac/ad3552r-hs.c
> > +++ b/drivers/iio/dac/ad3552r-hs.c
> > @@ -7,6 +7,7 @@
> ...
> > +
> > +static ssize_t ad3552r_hs_write_data_source(struct file *f,
> > + const char __user *userbuf,
> > + size_t count, loff_t *ppos)
> > +{
> > + struct ad3552r_hs_state *st = file_inode(f)->i_private;
> > + char buf[64];
> > + int ret;
> > +
> > + ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf,
> > + count);
> > + if (ret < 0)
> > + return ret;
> > +
> > + buf[count] = 0;
> Shouldn't it be
> buf[count] = '\0';
Why ? I am zero-terminating the string properly.
> ?
Regards,
angelo
On 03/27, Angelo Dureghello wrote:
> On 26.03.2025 18:52, Marcelo Schmitt wrote:
> > Hello Angelo,
> >
> > Patch looks good to me.
> > One minor comment bellow.
> >
> > On 03/21, Angelo Dureghello wrote:
> > > From: Angelo Dureghello <adureghello@baylibre.com>
> > >
...
> > > +
> > > +static ssize_t ad3552r_hs_write_data_source(struct file *f,
> > > + const char __user *userbuf,
> > > + size_t count, loff_t *ppos)
> > > +{
> > > + struct ad3552r_hs_state *st = file_inode(f)->i_private;
> > > + char buf[64];
> > > + int ret;
> > > +
> > > + ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf,
> > > + count);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + buf[count] = 0;
> > Shouldn't it be
> > buf[count] = '\0';
>
> Why ? I am zero-terminating the string properly.
Oh, okay. I was just more used to see '\0' as buffer/string terminator.
I see now buf[count] = 0; should work too.
>
> > ?
>
> Regards,
> angelo
Regards,
Marcelo
On Thu, 2025-03-27 at 09:09 -0300, Marcelo Schmitt wrote:
> On 03/27, Angelo Dureghello wrote:
> > On 26.03.2025 18:52, Marcelo Schmitt wrote:
> > > Hello Angelo,
> > >
> > > Patch looks good to me.
> > > One minor comment bellow.
> > >
> > > On 03/21, Angelo Dureghello wrote:
> > > > From: Angelo Dureghello <adureghello@baylibre.com>
> > > >
> ...
> > > > +
> > > > +static ssize_t ad3552r_hs_write_data_source(struct file *f,
> > > > + const char __user *userbuf,
> > > > + size_t count, loff_t *ppos)
> > > > +{
> > > > + struct ad3552r_hs_state *st = file_inode(f)->i_private;
> > > > + char buf[64];
> > > > + int ret;
> > > > +
> > > > + ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf,
> > > > + count);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + buf[count] = 0;
> > > Shouldn't it be
> > > buf[count] = '\0';
> >
> > Why ? I am zero-terminating the string properly.
>
> Oh, okay. I was just more used to see '\0' as buffer/string terminator.
> I see now buf[count] = 0; should work too.
>
I agree with Marcelo that the more natural/readable way for terminating a string is
using the corresponding null character (ascii). Probably not a reason for a v2
though...
- Nuno Sá
© 2016 - 2026 Red Hat, Inc.