| Message ID | 20200522065322.9611-1-alexandru.ardelean@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: position: iqs624: remove usage of iio_priv_to_dev() | expand |
Hi Alexandru, On Fri, May 22, 2020 at 09:53:22AM +0300, Alexandru Ardelean wrote: > We may want to get rid of the iio_priv_to_dev() helper. That's a bit > uncertain at this point. The reason is that we will hide some of the > members of the iio_dev structure (to prevent drivers from accessing them > directly), and that will also mean hiding the implementation of the > iio_priv_to_dev() helper inside the IIO core. > > Hiding the implementation of iio_priv_to_dev() implies that some fast-paths > may not be fast anymore, so a general idea is to try to get rid of the > iio_priv_to_dev() altogether. > > For this driver, removing iio_priv_to_dev() also means keeping a reference > on the state struct. > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > --- > drivers/iio/position/iqs624-pos.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) As a customer of iio, I find it handy that there is an "inverse" to iio_priv. In this particular case it saves the container iio_dev from storing a pointer to itself. That being said, this patch is perfectly fine and I have no objection if this is the route you and Jonathan opt to take. And so: Acked-by: Jeff LaBundy <jeff@labundy.com> > > diff --git a/drivers/iio/position/iqs624-pos.c b/drivers/iio/position/iqs624-pos.c > index 77096c31c2ba..520dafbdc48f 100644 > --- a/drivers/iio/position/iqs624-pos.c > +++ b/drivers/iio/position/iqs624-pos.c > @@ -23,6 +23,7 @@ > > struct iqs624_pos_private { > struct iqs62x_core *iqs62x; > + struct iio_dev *indio_dev; > struct notifier_block notifier; > struct mutex lock; > bool angle_en; > @@ -59,7 +60,7 @@ static int iqs624_pos_notifier(struct notifier_block *notifier, > > iqs624_pos = container_of(notifier, struct iqs624_pos_private, > notifier); > - indio_dev = iio_priv_to_dev(iqs624_pos); > + indio_dev = iqs624_pos->indio_dev; > timestamp = iio_get_time_ns(indio_dev); > > iqs62x = iqs624_pos->iqs62x; > @@ -98,7 +99,7 @@ static int iqs624_pos_notifier(struct notifier_block *notifier, > static void iqs624_pos_notifier_unregister(void *context) > { > struct iqs624_pos_private *iqs624_pos = context; > - struct iio_dev *indio_dev = iio_priv_to_dev(iqs624_pos); > + struct iio_dev *indio_dev = iqs624_pos->indio_dev; > int ret; > > ret = blocking_notifier_chain_unregister(&iqs624_pos->iqs62x->nh, > @@ -243,6 +244,7 @@ static int iqs624_pos_probe(struct platform_device *pdev) > > iqs624_pos = iio_priv(indio_dev); > iqs624_pos->iqs62x = iqs62x; > + iqs624_pos->indio_dev = indio_dev; > > indio_dev->modes = INDIO_DIRECT_MODE; > indio_dev->dev.parent = &pdev->dev; > -- > 2.25.1 > Kind regards, Jeff LaBundy
On Fri, 22 May 2020 20:53:40 -0500 Jeff LaBundy <jeff@labundy.com> wrote: > Hi Alexandru, > > On Fri, May 22, 2020 at 09:53:22AM +0300, Alexandru Ardelean wrote: > > We may want to get rid of the iio_priv_to_dev() helper. That's a bit > > uncertain at this point. The reason is that we will hide some of the > > members of the iio_dev structure (to prevent drivers from accessing them > > directly), and that will also mean hiding the implementation of the > > iio_priv_to_dev() helper inside the IIO core. > > > > Hiding the implementation of iio_priv_to_dev() implies that some fast-paths > > may not be fast anymore, so a general idea is to try to get rid of the > > iio_priv_to_dev() altogether. > > > > For this driver, removing iio_priv_to_dev() also means keeping a reference > > on the state struct. > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > --- > > drivers/iio/position/iqs624-pos.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > As a customer of iio, I find it handy that there is an "inverse" to iio_priv. > In this particular case it saves the container iio_dev from storing a pointer > to itself. Hi Jeff, The actual usecases for this function are rare - this being one of the few where it is justified due to the notifier chain stuff and need to embed that in a driver structure. So I'd rather we did something a bit special in these few cases and made it harder for people to submit new drivers that jump backwards and forwards between the iio_dev and the iio_priv structure. Doing it now is a side effect of Alex's work to make a large chunk of struct iio_dev opaque. It's easy to handle the forwards case with a nice macro / inline function as we can embed a pointer to the iio_priv in the 'internal' structure but the other way around has to involve an IIO core function call. We could keep the function around and rely on an offputting name __iio_priv_to_dev maybe but those tend to get cut and past all over the place. Thanks for being understanding as indeed this is less than elegant in this particular case! Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it. Thanks, Jonathan > > That being said, this patch is perfectly fine and I have no objection if this > is the route you and Jonathan opt to take. And so: > > Acked-by: Jeff LaBundy <jeff@labundy.com> > > > > > diff --git a/drivers/iio/position/iqs624-pos.c b/drivers/iio/position/iqs624-pos.c > > index 77096c31c2ba..520dafbdc48f 100644 > > --- a/drivers/iio/position/iqs624-pos.c > > +++ b/drivers/iio/position/iqs624-pos.c > > @@ -23,6 +23,7 @@ > > > > struct iqs624_pos_private { > > struct iqs62x_core *iqs62x; > > + struct iio_dev *indio_dev; > > struct notifier_block notifier; > > struct mutex lock; > > bool angle_en; > > @@ -59,7 +60,7 @@ static int iqs624_pos_notifier(struct notifier_block *notifier, > > > > iqs624_pos = container_of(notifier, struct iqs624_pos_private, > > notifier); > > - indio_dev = iio_priv_to_dev(iqs624_pos); > > + indio_dev = iqs624_pos->indio_dev; > > timestamp = iio_get_time_ns(indio_dev); > > > > iqs62x = iqs624_pos->iqs62x; > > @@ -98,7 +99,7 @@ static int iqs624_pos_notifier(struct notifier_block *notifier, > > static void iqs624_pos_notifier_unregister(void *context) > > { > > struct iqs624_pos_private *iqs624_pos = context; > > - struct iio_dev *indio_dev = iio_priv_to_dev(iqs624_pos); > > + struct iio_dev *indio_dev = iqs624_pos->indio_dev; > > int ret; > > > > ret = blocking_notifier_chain_unregister(&iqs624_pos->iqs62x->nh, > > @@ -243,6 +244,7 @@ static int iqs624_pos_probe(struct platform_device *pdev) > > > > iqs624_pos = iio_priv(indio_dev); > > iqs624_pos->iqs62x = iqs62x; > > + iqs624_pos->indio_dev = indio_dev; > > > > indio_dev->modes = INDIO_DIRECT_MODE; > > indio_dev->dev.parent = &pdev->dev; > > -- > > 2.25.1 > > > > Kind regards, > Jeff LaBundy
diff --git a/drivers/iio/position/iqs624-pos.c b/drivers/iio/position/iqs624-pos.c index 77096c31c2ba..520dafbdc48f 100644 --- a/drivers/iio/position/iqs624-pos.c +++ b/drivers/iio/position/iqs624-pos.c @@ -23,6 +23,7 @@ struct iqs624_pos_private { struct iqs62x_core *iqs62x; + struct iio_dev *indio_dev; struct notifier_block notifier; struct mutex lock; bool angle_en; @@ -59,7 +60,7 @@ static int iqs624_pos_notifier(struct notifier_block *notifier, iqs624_pos = container_of(notifier, struct iqs624_pos_private, notifier); - indio_dev = iio_priv_to_dev(iqs624_pos); + indio_dev = iqs624_pos->indio_dev; timestamp = iio_get_time_ns(indio_dev); iqs62x = iqs624_pos->iqs62x; @@ -98,7 +99,7 @@ static int iqs624_pos_notifier(struct notifier_block *notifier, static void iqs624_pos_notifier_unregister(void *context) { struct iqs624_pos_private *iqs624_pos = context; - struct iio_dev *indio_dev = iio_priv_to_dev(iqs624_pos); + struct iio_dev *indio_dev = iqs624_pos->indio_dev; int ret; ret = blocking_notifier_chain_unregister(&iqs624_pos->iqs62x->nh, @@ -243,6 +244,7 @@ static int iqs624_pos_probe(struct platform_device *pdev) iqs624_pos = iio_priv(indio_dev); iqs624_pos->iqs62x = iqs62x; + iqs624_pos->indio_dev = indio_dev; indio_dev->modes = INDIO_DIRECT_MODE; indio_dev->dev.parent = &pdev->dev;
We may want to get rid of the iio_priv_to_dev() helper. That's a bit uncertain at this point. The reason is that we will hide some of the members of the iio_dev structure (to prevent drivers from accessing them directly), and that will also mean hiding the implementation of the iio_priv_to_dev() helper inside the IIO core. Hiding the implementation of iio_priv_to_dev() implies that some fast-paths may not be fast anymore, so a general idea is to try to get rid of the iio_priv_to_dev() altogether. For this driver, removing iio_priv_to_dev() also means keeping a reference on the state struct. Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> --- drivers/iio/position/iqs624-pos.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)