On Sat, Nov 12, 2022 at 04:33:58PM +0100, Jonathan Cameron wrote: > On Sun, 6 Nov 2022 18:16:10 +0100 > "Rafael J. Wysocki" <rafael@xxxxxxxxxx> wrote: > > > On Sun, Nov 6, 2022 at 4:33 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote: > > > > > > On Mon, 31 Oct 2022 17:48:39 +0100 > > > Marten Lindahl <martenli@xxxxxxxx> wrote: > > > > > > > On Tue, Oct 25, 2022 at 06:20:10PM +0200, Rafael J. Wysocki wrote: > > > > > On Thu, Sep 29, 2022 at 4:46 PM Mårten Lindahl <marten.lindahl@xxxxxxxx> wrote: > > > > > > > > Hi! Thanks for your feedback! > > > > > > > > > > > > > > > > A device that creates a child character device with cdev_device_add by > > > > > > default create a PM sysfs group with power attributes for userspace > > > > > > control. This means that the power attributes monitors the child device > > > > > > only and thus does not reflect the parent device PM runtime behavior. > > > > > > > > > > It looks like device_set_pm_not_required() should be used on the child then. > > > > > > > > > > > But as the PM runtime framework (rpm_suspend/rpm_resume) operates not > > > > > > only on a single device that has been enabled for runtime PM, but also > > > > > > on its parent, it should be possible to synchronize the child and the > > > > > > parent so that the power attribute monitoring reflects the child and the > > > > > > parent as one. > > > > > > > > > > > > As an example, if an i2c_client device registers an iio_device, the > > > > > > iio_device will create sysfs power/attribute nodes for userspace > > > > > > control. But if the dev_pm_ops with resume/suspend callbacks is attached > > > > > > to the struct i2c_driver.driver.pm, the PM runtime needs to be enabled > > > > > > for the i2c_client device and not for the child iio_device. > > > > > > > > > > > > In this case PM runtime can be enabled for the i2c_client device and > > > > > > suspend/resume callbacks will be triggered, but the child sysfs power > > > > > > attributes will be visible but marked as 'unsupported' and can not be > > > > > > used for control or monitoring. This can be confusing as the sysfs > > > > > > device node presents the i2c_client and the iio_device as one device. > > > > > > > > > > I don't quite understand the last sentence. > > > > > > > > > > They are separate struct device objects and so they each have a > > > > > directory in sysfs, right? > > > > > > > > > > > > > Yes, they do have separate directories and if using device_set_pm_not_required > > > > on the child it will make it clearer which device is PM runtime regulated, so > > > > I guess that is what should be done. > > > > > > > > I think it all depends on where in sysfs the user accesses the device from. My > > > > point with these patches is that the iio_device may be perceived to be an > > > > iio device that should be possible to manually power control, as the power > > > > directory is visble. If looking at it from here: > > > > > > > > ~# ls /sys/bus/iio/devices/iio:device0/ > > > > in_illuminance_raw in_proximity_raw power > > > > in_illuminance_scale name subsystem > > > > in_proximity_nearlevel of_node uevent > > > > > > > > my idea is to let this power directory inherity the parent power control. But > > > > as you say, it is probably better to not create it at all, as the actual manual > > > > power control can be done here: > > > > > > > > ~# ls /sys/devices/platform/soc/.../i2c-2/2-0060/ > > > > driver modalias of_node subsystem > > > > iio:device1 name power uevent > > > > > > > > where it is more clear which device (the i2c parent) that can be power > > > > controlled. > > > > > > > > > > Add a function to synchronize the runtime PM enable state of a device > > > > > > with its parent. As there already exists a link from the child to its > > > > > > parent and both are enabled, all sysfs control/monitoring can reflect > > > > > > both devices, which from a userspace perspective makes more sense. > > > > > > > > > > Except that user space will be able to change "control" to "on" for > > > > > the parent alone AFAICS which still will be confusing. > > > > > > > > Yes, that is true. > > > > > > > > > > For devices that are pure software constructs it only makes sense to > > > > > expose the PM-runtime interface for them if the plan is to indirectly > > > > > control the parent's runtime PM through them. > > > > > > > > I will abandon this patchset and send a single patch for the iio device. > > > > > > I entirely agree with the statement that these are pointless and should not > > > be exposed. However I don't want to see a per device tweak. If we get > > > rid of these, we get rid of them for all of the iio:device0/ > > > devices (and the various other types of device IIO uses). > > > > > > The risk here is that, although pointless, some userspace is relying on the > > > ABI in sysfs. Do people thing it's worth the gamble of getting rid > > > of this non functioning interface for the whole of IIO? > > > > I think so. > > > > It is better to avoid exposing garbage to user space even if the scope > > of it is limited IMV. > > Marten, do you want to spin a patch doing this in the iio core? > > If not I'll do so sometime soon. Given where we are in the cycle, > and the need to keep such a patch up for review for at least a few weeks, > we can look to get it into next early next cycle and hopefully any issues > will become apparent. Please excuse me Jonathan, I missed this mail. I really do apologize. Considering a patch for the discussed issue. I think I need to dig into the core a bit more before I can do it. I only had the vcnl4000 driver in focus, so if you have an idea of how it should be done in the core, then I think it's better if you do it. If yo ask me to look into it I will try to do it, but I'm afraid it will take some time before I can send anything. Kind regards Mårten > > Jonathan > > > > > > So far I think this is only been done for a few similar cases > > > and for new subsystems. >
