Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
From: Jonathan Cameron <jic23@kernel.org>
To: "Ardelean, Alexandru" <alexandru.Ardelean@analog.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
	"lars@metafoo.de" <lars@metafoo.de>,
	"pmeerw@pmeerw.net" <pmeerw@pmeerw.net>,
	"knaack.h@gmx.de" <knaack.h@gmx.de>
Subject: Re: [PATCH v4 4/7] iio: core: move debugfs data on the private iio dev info
Date: Wed, 1 Jul 2020 19:42:00 +0100
Message-ID: <20200701194200.716a263b@archlinux> (raw)
In-Reply-To: <3ad8e37bc439f0619f63010809fb0080b61a1b56.camel@analog.com>

On Tue, 30 Jun 2020 04:58:06 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

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

Either way worked.

However this doesn't pass my basic build test. Config condition
is reversed. 

Fixed up and pushed out as testing.


Jonathan

> 
> 
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> >  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)

Don't we want this if it is defined.

> > +/**
> > + * 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)
> >  {  


  reply index

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-30  4:57 [PATCH v4 0/7] iio: core: wrap IIO device into an iio_dev_opaque object Alexandru Ardelean
2020-06-30  4:57 ` [PATCH v4 1/7] iio: core: remove iio_priv_to_dev() helper Alexandru Ardelean
2020-06-30  4:57 ` [PATCH v4 2/7] iio: core: wrap IIO device into an iio_dev_opaque object Alexandru Ardelean
2020-07-21  9:46   ` Dmitry Baryshkov
2020-07-21 10:00     ` Alexandru Ardelean
2020-06-30  4:57 ` [PATCH v4 3/7] iio: core: remove padding from private information Alexandru Ardelean
2020-06-30  4:57 ` [PATCH v4 4/7] iio: core: move debugfs data on the private iio dev info Alexandru Ardelean
2020-06-30  4:58   ` Ardelean, Alexandru
2020-07-01 18:42     ` Jonathan Cameron [this message]
2020-07-02  9:34       ` Ardelean, Alexandru
2020-06-30  4:57 ` [PATCH v4 5/7] iio: core: move channel list & group to private iio device object Alexandru Ardelean
2020-06-30  4:57 ` [PATCH v4 6/7] iio: core: move iio_dev's buffer_list to the " Alexandru Ardelean
2020-06-30  4:57 ` [PATCH v4 7/7] iio: core: move event interface on the opaque struct Alexandru Ardelean

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200701194200.716a263b@archlinux \
    --to=jic23@kernel.org \
    --cc=alexandru.Ardelean@analog.com \
    --cc=knaack.h@gmx.de \
    --cc=lars@metafoo.de \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pmeerw@pmeerw.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-IIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iio/0 linux-iio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iio linux-iio/ https://lore.kernel.org/linux-iio \
		linux-iio@vger.kernel.org
	public-inbox-index linux-iio

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-iio


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git