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)
> {
