On Wed, 2025-04-09 at 11:24 +0200, Angelo Dureghello wrote:
> On 09.04.2025 07:32, Nuno Sá wrote:
> > On Tue, 2025-04-08 at 12:18 +0200, Angelo Dureghello wrote:
> > > From: Angelo Dureghello <adureghello@xxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxx>
> > > ---
>
> Hi Nuno,
> >
> > Hi Angelo,
> >
> > One issue that needs a respin and then a minor comment... With it,
> >
> > Reviewed-by: Nuno Sá <nuno.sa@xxxxxxxxxx>
> >
> > > drivers/iio/dac/ad3552r-hs.c | 166
> > > +++++++++++++++++++++++++++++++++++++++++--
> > > 1 file changed, 160 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c
> > > index
> > > 37397e188f225a8099745ec03f7c604da76960b1..9a8eed7a06e4f2e7b23d59764b8f2fc2
> > > 1e2c4537
> > > 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>
> > > @@ -54,6 +55,18 @@ struct ad3552r_hs_state {
> > > struct ad3552r_hs_platform_data *data;
> > > /* INTERFACE_CONFIG_D register cache, in DDR we cannot read
> > > values. */
> > > u32 config_d;
> > > + /* Protects backend I/O operations from concurrent accesses. */
> > > + struct mutex lock;
> > > +};
> > > +
> > > +enum ad3552r_sources {
> > > + AD3552R_SRC_IIO_BUFFER,
> > > + AD3552R_SRC_BACKEND_RAMP_GEN,
> > > +};
> > > +
> > > +static const char * const dbgfs_attr_source[] = {
> > > + [AD3552R_SRC_IIO_BUFFER] = "iio-buffer",
> > > + [AD3552R_SRC_BACKEND_RAMP_GEN] = "backend-ramp-generator",
> > > };
> >
> > nit: I would use more generic strings. I assume "iio-buffer" is just the
> > "normal"
> > data so use something like that. For the ramp, is it 16 bits? I would just
> > use ex:
> > RAMP_16. I do not thing that the "backend" prefix (as well as "-generator")
> > to add
> > much.
> > >
>
> are
> "normal", "ramp-16bit"
> ok ?
> Or please let me know the names you prefer.
I'm also not great with naming :). But yes, I think the above is a better choice
- Nuno Sá
>
> >
> > ...
> >
> > > +
> > > +static ssize_t ad3552r_hs_show_data_source_avail(struct file *f,
> > > + char __user *userbuf,
> > > + size_t count, loff_t
> > > *ppos)
> > > +{
> > > + ssize_t len = 0;
> > > + char *buf;
> > > + int i;
> > > +
> > > + buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
> > > + if (!buf)
> > > + return -ENOMEM;
> > > +
> >
> > When are we freeing this memory? I also do not see the point for a PAGE_SIZE
> > allocation for such a small string table. I would say to simplify things and
> > use a
> > local buffer with 64/128 bytes (should be more than enough). If you see this
> > growing
> > in the future, you can also go with seq_file.
> >
> agh, frogot the free. Sorry.
> Ok, i'll use 128.
>
> > - Nuno Sá
> >
> > > + for (i = 0; i < ARRAY_SIZE(dbgfs_attr_source); i++) {
> > > + len += scnprintf(buf + len, PAGE_SIZE - len, "%s ",
> > > + dbgfs_attr_source[i]);
> > > + }
> > > + buf[len - 1] = '\n';
> > > +
> > > + return simple_read_from_buffer(userbuf, count, ppos, buf, len);
> > > +}
> > > +
> > > +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 const struct file_operations ad3552r_hs_data_source_avail_fops = {
> > > + .owner = THIS_MODULE,
> > > + .read = ad3552r_hs_show_data_source_avail,
> > > +};
> > > +
> > > static int ad3552r_hs_setup(struct ad3552r_hs_state *st)
> > > {
> > > u16 id;
> > > @@ -550,11 +678,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 +785,26 @@ 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);
> > > + debugfs_create_file("data_source_available", 0600, d, st,
> > > + &ad3552r_hs_data_source_avail_fops);
> > > +}
> > > +
> > > static int ad3552r_hs_probe(struct platform_device *pdev)
> > > {
> > > struct ad3552r_hs_state *st;
> > > @@ -705,7 +849,17 @@ 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;
> > > +
> > > + ret = devm_mutex_init(&pdev->dev, &st->lock);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ad3552r_hs_debugfs_init(indio_dev);
> > > +
> > > + return ret;
> > > }
> > >
> > > static const struct of_device_id ad3552r_hs_of_id[] = {
> > >
> >
