diff mbox series

[6/9] iio: common: cros_ec_sensors: simplify getting .driver_data

Message ID 20210920090522.23784-7-wsa+renesas@sang-engineering.com (mailing list archive)
State Not Applicable
Headers show
Series treewide: simplify getting .driver_data | expand

Commit Message

Wolfram Sang Sept. 20, 2021, 9:05 a.m. UTC
We should get 'driver_data' from 'struct device' directly. Going via
platform_device is an unneeded step back and forth.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

Build tested only. buildbot is happy.

 drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Enric Balletbo i Serra Sept. 23, 2021, 9:16 a.m. UTC | #1
Hi Wolfram,

On 20/9/21 11:05, Wolfram Sang wrote:
> We should get 'driver_data' from 'struct device' directly. Going via
> platform_device is an unneeded step back and forth.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Acked-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

I'm fine to pick this patch through chrome-platform tree if Jonathan is fine, or
can go through his tree.

I plan also to pick patch  "[PATCH 8/9] platform: chrome: cros_ec_sensorhub:
simplify getting .driver_data"

Thanks,
  Enric

> ---
> 
> Build tested only. buildbot is happy.
> 
>  drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> index 28bde13003b7..b2725c6adc7f 100644
> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> @@ -831,8 +831,7 @@ EXPORT_SYMBOL_GPL(cros_ec_sensors_core_write);
>  
>  static int __maybe_unused cros_ec_sensors_resume(struct device *dev)
>  {
> -	struct platform_device *pdev = to_platform_device(dev);
> -	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>  	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
>  	int ret = 0;
>  
>
Jonathan Cameron Sept. 25, 2021, 2:54 p.m. UTC | #2
On Thu, 23 Sep 2021 11:16:47 +0200
Enric Balletbo i Serra <enric.balletbo@collabora.com> wrote:

> Hi Wolfram,
> 
> On 20/9/21 11:05, Wolfram Sang wrote:
> > We should get 'driver_data' from 'struct device' directly. Going via
> > platform_device is an unneeded step back and forth.
> > 
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>  
> 
> Acked-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> 
> I'm fine to pick this patch through chrome-platform tree if Jonathan is fine, or
> can go through his tree.

Fine by me, though a suggestion follows to take this a little further than done here.

Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

It's not something that ever bothered me that much, but we have had debates in
the past about whether there are semantic issues around this sort of cleanup
as it mixes

platform_set_drvdata() with device_get_drvdata()

Whilst they access the same pointer today, in theory that isn't necessarily
always going to be the case in future and it isn't necessarily apparent
to the casual reader of the code.

In this particular case you could tidy that up by using device_set_drvdata() in
the first place, but then to keep things consistent there is one other place
where platform_get_drvdata is used in a devm_add_action_or_reset() callback.
That one is also easily fixed though if we want to be consistent throughout.

Jonathan

> 
> I plan also to pick patch  "[PATCH 8/9] platform: chrome: cros_ec_sensorhub:
> simplify getting .driver_data"
> 
> Thanks,
>   Enric
> 
> > ---
> > 
> > Build tested only. buildbot is happy.
> > 
> >  drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > index 28bde13003b7..b2725c6adc7f 100644
> > --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > @@ -831,8 +831,7 @@ EXPORT_SYMBOL_GPL(cros_ec_sensors_core_write);
> >  
> >  static int __maybe_unused cros_ec_sensors_resume(struct device *dev)
> >  {
> > -	struct platform_device *pdev = to_platform_device(dev);
> > -	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> > +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> >  	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
> >  	int ret = 0;
> >  
> >
Wolfram Sang Oct. 12, 2021, 7:31 a.m. UTC | #3
Hi Jonathan,

> It's not something that ever bothered me that much, but we have had debates in
> the past about whether there are semantic issues around this sort of cleanup
> as it mixes
> 
> platform_set_drvdata() with device_get_drvdata()

Yeah, I see this concern. Mixing the two makes reading the code a bit
more difficult. As I said, it wasn't so easy to convert set_drvdata, but
I will have another go at this.

> Whilst they access the same pointer today, in theory that isn't necessarily
> always going to be the case in future and it isn't necessarily apparent
> to the casual reader of the code.

That one I don't really see. *_get_drvdata() should always get
'dev->driver_data' and the prefix just tells from what namespace we
come. If you want to change that, a lot of things will break loose, I'd
think. Even in the unlikely case of platform_device gaining a seperate
driver_data(?), it probably should be named *_get_pdrvdata(), or?

Thanks and happy hacking,

   Wolfram
Jonathan Cameron Oct. 12, 2021, 8:31 a.m. UTC | #4
On Tue, 12 Oct 2021 09:31:11 +0200
Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:

> Hi Jonathan,
> 
> > It's not something that ever bothered me that much, but we have had debates in
> > the past about whether there are semantic issues around this sort of cleanup
> > as it mixes
> > 
> > platform_set_drvdata() with device_get_drvdata()  
> 
> Yeah, I see this concern. Mixing the two makes reading the code a bit
> more difficult. As I said, it wasn't so easy to convert set_drvdata, but
> I will have another go at this.
> 
> > Whilst they access the same pointer today, in theory that isn't necessarily
> > always going to be the case in future and it isn't necessarily apparent
> > to the casual reader of the code.  
> 
> That one I don't really see. *_get_drvdata() should always get
> 'dev->driver_data' and the prefix just tells from what namespace we
> come. If you want to change that, a lot of things will break loose, I'd
> think. Even in the unlikely case of platform_device gaining a seperate
> driver_data(?), it probably should be named *_get_pdrvdata(), or?

Agreed. Does indeed seem like any change to this would be a mess so would
require different naming etc.

Thanks,

Jonathan

> 
> Thanks and happy hacking,
> 
>    Wolfram
> 
>
diff mbox series

Patch

diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
index 28bde13003b7..b2725c6adc7f 100644
--- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
+++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
@@ -831,8 +831,7 @@  EXPORT_SYMBOL_GPL(cros_ec_sensors_core_write);
 
 static int __maybe_unused cros_ec_sensors_resume(struct device *dev)
 {
-	struct platform_device *pdev = to_platform_device(dev);
-	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
 	int ret = 0;