| Message ID | 20250321-wip-bl-ad3552r-fixes-v1-4-3c1aa249d163@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: ad3552r-hs: add support for internal ramp generator | expand |
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á
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[] = { >> > >
On 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
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[] = {