| Message ID | 20221101122151.221029-1-marten.lindahl@axis.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | iio: light: vcnl4000: Don't create sysfs PM nodes for child | expand |
On Tue, 1 Nov 2022 13:21:51 +0100 Mårten Lindahl <marten.lindahl@axis.com> wrote: > PM runtime is enabled for the parent i2c device, but it is disabled for > the iio child device and remains so in this driver. But as the child > sysfs PM directory is created by default by iio_device_register => > cdev_device_add => dpm_sysfs_add it doesn't bring any value exposing it > since it won't give any usable data for the user. > > Tell dpm_sysfs to not create the PM directory for the iio device. > > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> Ah this one.. +CC Sudeep who I am sure has considered such changes in other similar cases (he did the original ones that mean this infastructure exists). So, the problem here is that it drops an ABI that has existed in IIO for a long time. I fully admit the ABI is confusing and pointless. What I'm not sure on is that we are entirely safe to remove it. If we are going to do this, it shouldn't be on a per driver basis, but rather a global change for all IIO drivers with the proviso that we are relying on the 'if no one notices an ABI change, it is fine' exception to never changing userspace ABI. Until now this has more or less only been used for new subsystems or where there were warnings generated due to spurious interface usage in normal flows. In IIO, nothing should touch these and I 'think' they don't do anything if anyone pokes them. Jonathan > --- > drivers/iio/light/vcnl4000.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c > index f6c83ecaad8b..400dc1155559 100644 > --- a/drivers/iio/light/vcnl4000.c > +++ b/drivers/iio/light/vcnl4000.c > @@ -1072,6 +1072,7 @@ static int vcnl4000_probe(struct i2c_client *client, > if (ret < 0) > goto fail_poweroff; > > + device_set_pm_not_required(&indio_dev->dev); > ret = iio_device_register(indio_dev); > if (ret < 0) > goto fail_poweroff;
On Sun, Nov 06, 2022 at 04:23:07PM +0100, Jonathan Cameron wrote: > On Tue, 1 Nov 2022 13:21:51 +0100 > Mårten Lindahl <marten.lindahl@axis.com> wrote: > > > PM runtime is enabled for the parent i2c device, but it is disabled for > > the iio child device and remains so in this driver. But as the child > > sysfs PM directory is created by default by iio_device_register => > > cdev_device_add => dpm_sysfs_add it doesn't bring any value exposing it > > since it won't give any usable data for the user. > > > > Tell dpm_sysfs to not create the PM directory for the iio device. > > > > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> > > Ah this one.. +CC Sudeep who I am sure has considered such changes in other > similar cases (he did the original ones that mean this infastructure exists). > > So, the problem here is that it drops an ABI that has existed in IIO for > a long time. I fully admit the ABI is confusing and pointless. What > I'm not sure on is that we are entirely safe to remove it. > > If we are going to do this, it shouldn't be on a per driver basis, but rather > a global change for all IIO drivers with the proviso that we are relying on > the 'if no one notices an ABI change, it is fine' exception to never changing > userspace ABI. > > Until now this has more or less only been used for new subsystems or where > there were warnings generated due to spurious interface usage in normal flows. > > In IIO, nothing should touch these and I 'think' they don't do anything if > anyone pokes them. > > Jonathan Hi Jonathan! Thanks. I'll drop this change then. Kind regards Mårten > > > > --- > > drivers/iio/light/vcnl4000.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c > > index f6c83ecaad8b..400dc1155559 100644 > > --- a/drivers/iio/light/vcnl4000.c > > +++ b/drivers/iio/light/vcnl4000.c > > @@ -1072,6 +1072,7 @@ static int vcnl4000_probe(struct i2c_client *client, > > if (ret < 0) > > goto fail_poweroff; > > > > + device_set_pm_not_required(&indio_dev->dev); > > ret = iio_device_register(indio_dev); > > if (ret < 0) > > goto fail_poweroff; >
On Sun, Nov 06, 2022 at 03:23:07PM +0000, Jonathan Cameron wrote: > On Tue, 1 Nov 2022 13:21:51 +0100 > Mårten Lindahl <marten.lindahl@axis.com> wrote: > > > PM runtime is enabled for the parent i2c device, but it is disabled for > > the iio child device and remains so in this driver. But as the child > > sysfs PM directory is created by default by iio_device_register => > > cdev_device_add => dpm_sysfs_add it doesn't bring any value exposing it > > since it won't give any usable data for the user. > > > > Tell dpm_sysfs to not create the PM directory for the iio device. > > > > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> > > Ah this one.. +CC Sudeep who I am sure has considered such changes in other > similar cases (he did the original ones that mean this infastructure exists). > Yes it was added for cache devices which are child devices of CPU devices via Commit 85945c28b5a8 ("PM / core: Add support to skip power management in device/driver model") Since CPU and its children are power managed quite differently(via cpuidle or cpu hotplug), the new API(device_set_pm_not_required) fits the usecase there well. I am not sure about this IIO usecase.
On Mon, 7 Nov 2022 10:36:36 +0000 Sudeep Holla <sudeep.holla@arm.com> wrote: > On Sun, Nov 06, 2022 at 03:23:07PM +0000, Jonathan Cameron wrote: > > On Tue, 1 Nov 2022 13:21:51 +0100 > > Mårten Lindahl <marten.lindahl@axis.com> wrote: > > > > > PM runtime is enabled for the parent i2c device, but it is disabled for > > > the iio child device and remains so in this driver. But as the child > > > sysfs PM directory is created by default by iio_device_register => > > > cdev_device_add => dpm_sysfs_add it doesn't bring any value exposing it > > > since it won't give any usable data for the user. > > > > > > Tell dpm_sysfs to not create the PM directory for the iio device. > > > > > > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> > > > > Ah this one.. +CC Sudeep who I am sure has considered such changes in other > > similar cases (he did the original ones that mean this infastructure exists). > > > > Yes it was added for cache devices which are child devices of CPU devices via > Commit 85945c28b5a8 ("PM / core: Add support to skip power management in device/driver model") > > Since CPU and its children are power managed quite differently(via cpuidle > or cpu hotplug), the new API(device_set_pm_not_required) fits the usecase > there well. I am not sure about this IIO usecase. > These are effectively pseudo devices for purposes of the driver model, with parents as the actual physical devices. As such all the PM infrastructure is associate with the appropriate bus specific device rather than the struct iio_dev->dev. So I'm fairly sure we should just not expose the sysfs attributes. This is similar to CXL for which they are only exposed for the PCI devices, not the bunch of subdevices created. Jonathan
On Mon, Nov 07, 2022 at 05:08:57PM +0000, Jonathan Cameron wrote: > On Mon, 7 Nov 2022 10:36:36 +0000 > Sudeep Holla <sudeep.holla@arm.com> wrote: > > > On Sun, Nov 06, 2022 at 03:23:07PM +0000, Jonathan Cameron wrote: > > > On Tue, 1 Nov 2022 13:21:51 +0100 > > > Mårten Lindahl <marten.lindahl@axis.com> wrote: > > > > > > > PM runtime is enabled for the parent i2c device, but it is disabled for > > > > the iio child device and remains so in this driver. But as the child > > > > sysfs PM directory is created by default by iio_device_register => > > > > cdev_device_add => dpm_sysfs_add it doesn't bring any value exposing it > > > > since it won't give any usable data for the user. > > > > > > > > Tell dpm_sysfs to not create the PM directory for the iio device. > > > > > > > > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> > > > > > > Ah this one.. +CC Sudeep who I am sure has considered such changes in other > > > similar cases (he did the original ones that mean this infastructure exists). > > > > > > > Yes it was added for cache devices which are child devices of CPU devices via > > Commit 85945c28b5a8 ("PM / core: Add support to skip power management in device/driver model") > > > > Since CPU and its children are power managed quite differently(via cpuidle > > or cpu hotplug), the new API(device_set_pm_not_required) fits the usecase > > there well. I am not sure about this IIO usecase. > > > > These are effectively pseudo devices for purposes of the driver model, with parents > as the actual physical devices. As such all the PM infrastructure is associate > with the appropriate bus specific device rather than the struct iio_dev->dev. > Ah if there are psuedo devices or companion like devices, then yes I agree worth not exposing the sysfs. > So I'm fairly sure we should just not expose the sysfs attributes. Agreed if it is psuedo device like you mention above. > This is similar to CXL for which they are only exposed for the PCI devices, not > the bunch of subdevices created. > OK
diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c index f6c83ecaad8b..400dc1155559 100644 --- a/drivers/iio/light/vcnl4000.c +++ b/drivers/iio/light/vcnl4000.c @@ -1072,6 +1072,7 @@ static int vcnl4000_probe(struct i2c_client *client, if (ret < 0) goto fail_poweroff; + device_set_pm_not_required(&indio_dev->dev); ret = iio_device_register(indio_dev); if (ret < 0) goto fail_poweroff;
PM runtime is enabled for the parent i2c device, but it is disabled for the iio child device and remains so in this driver. But as the child sysfs PM directory is created by default by iio_device_register => cdev_device_add => dpm_sysfs_add it doesn't bring any value exposing it since it won't give any usable data for the user. Tell dpm_sysfs to not create the PM directory for the iio device. Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> --- drivers/iio/light/vcnl4000.c | 1 + 1 file changed, 1 insertion(+)