Re: [PATCH v4 4/7] iio: core: move debugfs data on the private iio dev info

[Ardelean, Alexandru]

On Tue, 2020-06-30 at 07:57 +0300, Alexandru Ardelean wrote:
> This change moves all iio_dev debugfs fields to the iio_dev_priv object.
> It's not the biggest advantage yet (to the whole thing of
> abstractization)
> but it's a start.
> 
> The iio_get_debugfs_dentry() function (which is moved in
> industrialio-core.c) needs to also be guarded against the CONFIG_DEBUG_FS
> symbol, when it isn't defined. We do want to keep the inline definition
> in
> the iio.h header, so that the compiler can better infer when to compile
> out
> debugfs code that is related to the IIO debugfs directory.
> 

Well, pretty much only this patch changed since V3.
I thought about maybe re-doing just this patch, then I thought maybe I'd
get a minor complaint that I should re-send the series.

Either way, I prefer a complaint on this V4 series-re-send than if I were
to have re-sent just this patch.


> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>
> ---
>  drivers/iio/industrialio-core.c | 46 +++++++++++++++++++++++----------
>  include/linux/iio/iio-opaque.h  | 10 +++++++
>  include/linux/iio/iio.h         | 13 +---------
>  3 files changed, 44 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-
> core.c
> index 27005ba4d09c..64174052641a 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -165,6 +165,19 @@ static const char * const iio_chan_info_postfix[] =
> {
>  	[IIO_CHAN_INFO_THERMOCOUPLE_TYPE] = "thermocouple_type",
>  };
>  
> +#if !defined(CONFIG_DEBUG_FS)
> +/**
> + * There's also a CONFIG_DEBUG_FS guard in include/linux/iio/iio.h for
> + * iio_get_debugfs_dentry() to make it inline if CONFIG_DEBUG_FS is
> undefined
> + */
> +struct dentry *iio_get_debugfs_dentry(struct iio_dev *indio_dev)
> +{
> +	struct iio_dev_opaque *iio_dev_opaque =
> to_iio_dev_opaque(indio_dev);
> +	return iio_dev_opaque->debugfs_dentry;
> +}
> +EXPORT_SYMBOL_GPL(iio_get_debugfs_dentry);
> +#endif
> +
>  /**
>   * iio_find_channel_from_si() - get channel from its scan index
>   * @indio_dev:		device
> @@ -308,35 +321,37 @@ static ssize_t iio_debugfs_read_reg(struct file
> *file, char __user *userbuf,
>  			      size_t count, loff_t *ppos)
>  {
>  	struct iio_dev *indio_dev = file->private_data;
> +	struct iio_dev_opaque *iio_dev_opaque =
> to_iio_dev_opaque(indio_dev);
>  	unsigned val = 0;
>  	int ret;
>  
>  	if (*ppos > 0)
>  		return simple_read_from_buffer(userbuf, count, ppos,
> -					       indio_dev->read_buf,
> -					       indio_dev->read_buf_len);
> +					       iio_dev_opaque->read_buf,
> +					       iio_dev_opaque-
> >read_buf_len);
>  
>  	ret = indio_dev->info->debugfs_reg_access(indio_dev,
> -						  indio_dev-
> >cached_reg_addr,
> +						  iio_dev_opaque-
> >cached_reg_addr,
>  						  0, &val);
>  	if (ret) {
>  		dev_err(indio_dev->dev.parent, "%s: read failed\n",
> __func__);
>  		return ret;
>  	}
>  
> -	indio_dev->read_buf_len = snprintf(indio_dev->read_buf,
> -					   sizeof(indio_dev->read_buf),
> -					   "0x%X\n", val);
> +	iio_dev_opaque->read_buf_len = snprintf(iio_dev_opaque->read_buf,
> +					      sizeof(iio_dev_opaque-
> >read_buf),
> +					      "0x%X\n", val);
>  
>  	return simple_read_from_buffer(userbuf, count, ppos,
> -				       indio_dev->read_buf,
> -				       indio_dev->read_buf_len);
> +				       iio_dev_opaque->read_buf,
> +				       iio_dev_opaque->read_buf_len);
>  }
>  
>  static ssize_t iio_debugfs_write_reg(struct file *file,
>  		     const char __user *userbuf, size_t count, loff_t
> *ppos)
>  {
>  	struct iio_dev *indio_dev = file->private_data;
> +	struct iio_dev_opaque *iio_dev_opaque =
> to_iio_dev_opaque(indio_dev);
>  	unsigned reg, val;
>  	char buf[80];
>  	int ret;
> @@ -351,10 +366,10 @@ static ssize_t iio_debugfs_write_reg(struct file
> *file,
>  
>  	switch (ret) {
>  	case 1:
> -		indio_dev->cached_reg_addr = reg;
> +		iio_dev_opaque->cached_reg_addr = reg;
>  		break;
>  	case 2:
> -		indio_dev->cached_reg_addr = reg;
> +		iio_dev_opaque->cached_reg_addr = reg;
>  		ret = indio_dev->info->debugfs_reg_access(indio_dev, reg,
>  							  val, NULL);
>  		if (ret) {
> @@ -378,23 +393,28 @@ static const struct file_operations
> iio_debugfs_reg_fops = {
>  
>  static void iio_device_unregister_debugfs(struct iio_dev *indio_dev)
>  {
> -	debugfs_remove_recursive(indio_dev->debugfs_dentry);
> +	struct iio_dev_opaque *iio_dev_opaque =
> to_iio_dev_opaque(indio_dev);
> +	debugfs_remove_recursive(iio_dev_opaque->debugfs_dentry);
>  }
>  
>  static void iio_device_register_debugfs(struct iio_dev *indio_dev)
>  {
> +	struct iio_dev_opaque *iio_dev_opaque;
> +
>  	if (indio_dev->info->debugfs_reg_access == NULL)
>  		return;
>  
>  	if (!iio_debugfs_dentry)
>  		return;
>  
> -	indio_dev->debugfs_dentry =
> +	iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> +
> +	iio_dev_opaque->debugfs_dentry =
>  		debugfs_create_dir(dev_name(&indio_dev->dev),
>  				   iio_debugfs_dentry);
>  
>  	debugfs_create_file("direct_reg_access", 0644,
> -			    indio_dev->debugfs_dentry, indio_dev,
> +			    iio_dev_opaque->debugfs_dentry, indio_dev,
>  			    &iio_debugfs_reg_fops);
>  }
>  #else
> diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-
> opaque.h
> index 1375674f14cd..b3f234b4c1e9 100644
> --- a/include/linux/iio/iio-opaque.h
> +++ b/include/linux/iio/iio-opaque.h
> @@ -6,9 +6,19 @@
>  /**
>   * struct iio_dev_opaque - industrial I/O device opaque information
>   * @indio_dev:			public industrial I/O device
> information
> + * @debugfs_dentry:		device specific debugfs dentry
> + * @cached_reg_addr:		cached register address for debugfs reads
> + * @read_buf:			read buffer to be used for the
> initial reg read
> + * @read_buf_len:		data length in @read_buf
>   */
>  struct iio_dev_opaque {
>  	struct iio_dev			indio_dev;
> +#if defined(CONFIG_DEBUG_FS)
> +	struct dentry			*debugfs_dentry;
> +	unsigned			cached_reg_addr;
> +	char				read_buf[20];
> +	unsigned int			read_buf_len;
> +#endif
>  };
>  
>  #define to_iio_dev_opaque(indio_dev)		\
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 86112e35ae5f..bb0aae11a111 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -520,8 +520,6 @@ struct iio_buffer_setup_ops {
>   * @groups:		[INTERN] attribute groups
>   * @groupcounter:	[INTERN] index of next attribute group
>   * @flags:		[INTERN] file ops related flags including busy
> flag.
> - * @debugfs_dentry:	[INTERN] device specific debugfs dentry.
> - * @cached_reg_addr:	[INTERN] cached register address for debugfs reads.
>   * @priv:		[DRIVER] reference to driver's private information
>   *			**MUST** be accessed **ONLY** via iio_priv() helper
>   */
> @@ -567,12 +565,6 @@ struct iio_dev {
>  	int				groupcounter;
>  
>  	unsigned long			flags;
> -#if defined(CONFIG_DEBUG_FS)
> -	struct dentry			*debugfs_dentry;
> -	unsigned			cached_reg_addr;
> -	char				read_buf[20];
> -	unsigned int			read_buf_len;
> -#endif
>  	void				*priv;
>  };
>  
> @@ -727,10 +719,7 @@ static inline bool iio_buffer_enabled(struct iio_dev
> *indio_dev)
>   * @indio_dev:		IIO device structure for device
>   **/
>  #if defined(CONFIG_DEBUG_FS)
> -static inline struct dentry *iio_get_debugfs_dentry(struct iio_dev
> *indio_dev)
> -{
> -	return indio_dev->debugfs_dentry;
> -}
> +struct dentry *iio_get_debugfs_dentry(struct iio_dev *indio_dev);
>  #else
>  static inline struct dentry *iio_get_debugfs_dentry(struct iio_dev
> *indio_dev)
>  {