| 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 |
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; > >
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; > > > >
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
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 --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;
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(-)