Re: [PATCH v4 5/5] iio: dac: ad3552r-hs: add support for internal ramp

[Nuno Sá]

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 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..9a8eed7a06e4f2e7b23d59764b8f2fc21e2c4537
> 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.
>  

...

> +
> +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.

- 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[] = {
>