diff mbox series

drivers/regulator: Notify sysfs about status changes

Message ID 20231005133059.917577-1-naresh.solanki@9elements.com
State New
Headers
Series drivers/regulator: Notify sysfs about status changes |

Commit Message

Naresh Solanki Oct. 5, 2023, 1:30 p.m. UTC 
  From: Patrick Rudolph <patrick.rudolph@9elements.com>

Notify sysfs for state, status & microvolts.

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com>
---
 drivers/regulator/core.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)


base-commit: f9a1d31874c383f58bb4f89bfe79b764682cd026

Comments

Mark Brown Oct. 5, 2023, 5 p.m. UTC | #1 
On Thu, Oct 05, 2023 at 03:30:58PM +0200, Naresh Solanki wrote:

>  static int _notifier_call_chain(struct regulator_dev *rdev,
>  				  unsigned long event, void *data)
>  {
> +	const char *name;
> +	int ret;
> +
>  	/* call rdev chain first */
> -	return blocking_notifier_call_chain(&rdev->notifier, event, data);
> +	ret =  blocking_notifier_call_chain(&rdev->notifier, event, data);
> +
> +	if (event & REGULATOR_EVENT_VOLTAGE_CHANGE) {
> +		name = dev_attr_microvolts.attr.name;
> +		sysfs_notify(&rdev->dev.kobj, NULL, name);
> +	} else {
> +		name = dev_attr_status.attr.name;
> +		sysfs_notify(&rdev->dev.kobj, NULL, name);
> +	}

We probably should filter the events more, there's events for pre and
post voltage change for example which aren't status changes so would be
spurious.  It ought not to break anything but we should still avoid
unneeded work.
Naresh Solanki Nov. 2, 2023, 12:05 p.m. UTC | #2 
Hi Mark,

On Thu, 5 Oct 2023 at 22:30, Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Oct 05, 2023 at 03:30:58PM +0200, Naresh Solanki wrote:
>
> >  static int _notifier_call_chain(struct regulator_dev *rdev,
> >                                 unsigned long event, void *data)
> >  {
> > +     const char *name;
> > +     int ret;
> > +
> >       /* call rdev chain first */
> > -     return blocking_notifier_call_chain(&rdev->notifier, event, data);
> > +     ret =  blocking_notifier_call_chain(&rdev->notifier, event, data);
> > +
> > +     if (event & REGULATOR_EVENT_VOLTAGE_CHANGE) {
> > +             name = dev_attr_microvolts.attr.name;
> > +             sysfs_notify(&rdev->dev.kobj, NULL, name);
> > +     } else {
> > +             name = dev_attr_status.attr.name;
> > +             sysfs_notify(&rdev->dev.kobj, NULL, name);
> > +     }
>
> We probably should filter the events more, there's events for pre and
> post voltage change for example which aren't status changes so would be
> spurious.  It ought not to break anything but we should still avoid
> unneeded work.
Can you please provide me inputs on the additional filtering needed for this.
Like some list of events for notify on status?
Mark Brown Nov. 2, 2023, 1:06 p.m. UTC | #3 
On Thu, Nov 02, 2023 at 05:35:42PM +0530, Naresh Solanki wrote:
> On Thu, 5 Oct 2023 at 22:30, Mark Brown <broonie@kernel.org> wrote:
> > On Thu, Oct 05, 2023 at 03:30:58PM +0200, Naresh Solanki wrote:

> > We probably should filter the events more, there's events for pre and
> > post voltage change for example which aren't status changes so would be
> > spurious.  It ought not to break anything but we should still avoid
> > unneeded work.

> Can you please provide me inputs on the additional filtering needed for this.
> Like some list of events for notify on status?

I think I'd start off with just reporting things that are obviously
errors and not things that should ever go off during normal operation.
Naresh Solanki Nov. 2, 2023, 2:47 p.m. UTC | #4 
Hi Mark,

On Thu, 2 Nov 2023 at 18:36, Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Nov 02, 2023 at 05:35:42PM +0530, Naresh Solanki wrote:
> > On Thu, 5 Oct 2023 at 22:30, Mark Brown <broonie@kernel.org> wrote:
> > > On Thu, Oct 05, 2023 at 03:30:58PM +0200, Naresh Solanki wrote:
>
> > > We probably should filter the events more, there's events for pre and
> > > post voltage change for example which aren't status changes so would be
> > > spurious.  It ought not to break anything but we should still avoid
> > > unneeded work.
>
> > Can you please provide me inputs on the additional filtering needed for this.
> > Like some list of events for notify on status?
>
> I think I'd start off with just reporting things that are obviously
> errors and not things that should ever go off during normal operation.
This is what I could come up with:
        if (event & REGULATOR_EVENT_VOLTAGE_CHANGE) {
                name = dev_attr_microvolts.attr.name;
                sysfs_notify(&rdev->dev.kobj, NULL, name);
        } else if (event & (REGULATOR_EVENT_DISABLE | REGULATOR_EVENT_ENABLE)){
                name = dev_attr_status.attr.name;
                sysfs_notify(&rdev->dev.kobj, NULL, name);
        }

Regards,
Naresh
Mark Brown Nov. 2, 2023, 3:01 p.m. UTC | #5 
On Thu, Nov 02, 2023 at 08:17:35PM +0530, Naresh Solanki wrote:
> On Thu, 2 Nov 2023 at 18:36, Mark Brown <broonie@kernel.org> wrote:
> > On Thu, Nov 02, 2023 at 05:35:42PM +0530, Naresh Solanki wrote:

> > > > We probably should filter the events more, there's events for pre and
> > > > post voltage change for example which aren't status changes so would be
> > > > spurious.  It ought not to break anything but we should still avoid
> > > > unneeded work.

> > > Can you please provide me inputs on the additional filtering needed for this.
> > > Like some list of events for notify on status?

> > I think I'd start off with just reporting things that are obviously
> > errors and not things that should ever go off during normal operation.

> This is what I could come up with:
>         if (event & REGULATOR_EVENT_VOLTAGE_CHANGE) {
>                 name = dev_attr_microvolts.attr.name;
>                 sysfs_notify(&rdev->dev.kobj, NULL, name);
>         } else if (event & (REGULATOR_EVENT_DISABLE | REGULATOR_EVENT_ENABLE)){
>                 name = dev_attr_status.attr.name;
>                 sysfs_notify(&rdev->dev.kobj, NULL, name);
>         }

That's the opposite sense to what I was thinking of - we're reporting
voltage changes and enables to userspace rather than just errors.  My
concern here is that this could generate an awful lot of notificaitons
for normal operation on systems that don't use the uevents, I was
expecting this to be used for errors.  Could you remind me what the use
case is here, I think I might've got myself confused sorry?
Naresh Solanki Nov. 2, 2023, 3:33 p.m. UTC | #6 
Hi Mark,

On Thu, 2 Nov 2023 at 20:31, Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Nov 02, 2023 at 08:17:35PM +0530, Naresh Solanki wrote:
> > On Thu, 2 Nov 2023 at 18:36, Mark Brown <broonie@kernel.org> wrote:
> > > On Thu, Nov 02, 2023 at 05:35:42PM +0530, Naresh Solanki wrote:
>
> > > > > We probably should filter the events more, there's events for pre and
> > > > > post voltage change for example which aren't status changes so would be
> > > > > spurious.  It ought not to break anything but we should still avoid
> > > > > unneeded work.
>
> > > > Can you please provide me inputs on the additional filtering needed for this.
> > > > Like some list of events for notify on status?
>
> > > I think I'd start off with just reporting things that are obviously
> > > errors and not things that should ever go off during normal operation.
>
> > This is what I could come up with:
> >         if (event & REGULATOR_EVENT_VOLTAGE_CHANGE) {
> >                 name = dev_attr_microvolts.attr.name;
> >                 sysfs_notify(&rdev->dev.kobj, NULL, name);
> >         } else if (event & (REGULATOR_EVENT_DISABLE | REGULATOR_EVENT_ENABLE)){
> >                 name = dev_attr_status.attr.name;
> >                 sysfs_notify(&rdev->dev.kobj, NULL, name);
> >         }
>
> That's the opposite sense to what I was thinking of - we're reporting
> voltage changes and enables to userspace rather than just errors.  My
> concern here is that this could generate an awful lot of notificaitons
> for normal operation on systems that don't use the uevents, I was
> expecting this to be used for errors.  Could you remind me what the use
> case is here, I think I might've got myself confused sorry?
Sorry for confusion caused because I should first described my application
requirements.
Currently my application is interested in know regulator status i.e.,
ENABLE, DISABLE or ERROR.
Also events are needed specifically to get them logged like
UNDER_VOLTAGE, OVER_CURRENT, REGULATION_OUT,
OVER_TEMP.


Regards,
Naresh
Mark Brown Nov. 2, 2023, 4:50 p.m. UTC | #7 
On Thu, Nov 02, 2023 at 09:03:40PM +0530, Naresh Solanki wrote:
> On Thu, 2 Nov 2023 at 20:31, Mark Brown <broonie@kernel.org> wrote:

> > That's the opposite sense to what I was thinking of - we're reporting
> > voltage changes and enables to userspace rather than just errors.  My
> > concern here is that this could generate an awful lot of notificaitons
> > for normal operation on systems that don't use the uevents, I was
> > expecting this to be used for errors.  Could you remind me what the use
> > case is here, I think I might've got myself confused sorry?

> Sorry for confusion caused because I should first described my application
> requirements.
> Currently my application is interested in know regulator status i.e.,
> ENABLE, DISABLE or ERROR.
> Also events are needed specifically to get them logged like
> UNDER_VOLTAGE, OVER_CURRENT, REGULATION_OUT,
> OVER_TEMP.

Ah, right.  Everything except for the enable and disable there looks
like it should be OK since they should normally just not happen but the
enables and disables might get a bit frequent with runtime PM - not
*super* frequent like voltage scaling but enough that people could have
an issue with it.

Netlink feels like it might be a better fit?  Not really looked at the
kernel side of implementing that and how sensible that ends up looking.
Naresh Solanki Nov. 9, 2023, 10:38 a.m. UTC | #8 
Hi Mark,

On Thu, 2 Nov 2023 at 22:20, Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Nov 02, 2023 at 09:03:40PM +0530, Naresh Solanki wrote:
> > On Thu, 2 Nov 2023 at 20:31, Mark Brown <broonie@kernel.org> wrote:
>
> > > That's the opposite sense to what I was thinking of - we're reporting
> > > voltage changes and enables to userspace rather than just errors.  My
> > > concern here is that this could generate an awful lot of notificaitons
> > > for normal operation on systems that don't use the uevents, I was
> > > expecting this to be used for errors.  Could you remind me what the use
> > > case is here, I think I might've got myself confused sorry?
>
> > Sorry for confusion caused because I should first described my application
> > requirements.
> > Currently my application is interested in know regulator status i.e.,
> > ENABLE, DISABLE or ERROR.
> > Also events are needed specifically to get them logged like
> > UNDER_VOLTAGE, OVER_CURRENT, REGULATION_OUT,
> > OVER_TEMP.
>
> Ah, right.  Everything except for the enable and disable there looks
> like it should be OK since they should normally just not happen but the
> enables and disables might get a bit frequent with runtime PM - not
> *super* frequent like voltage scaling but enough that people could have
> an issue with it.
I aim for a straightforward implementation.
Using attributes such as status or state seems ideal for receiving
notifications.
In my case, the application focuses on status changes—whether it's on, off,
or encountering an error.
These changes are driven by events originating from the regulator.
So below change is what I see fit well.

        status_events = REGULATOR_EVENT_DISABLE;
        status_events |= REGULATOR_EVENT_ENABLE;
        status_events |= REGULATOR_EVENT_FAIL;
        status_events |= REGULATOR_EVENT_FORCE_DISABLE;
        status_events |= REGULATOR_EVENT_ABORT_DISABLE;

        if (event & status_events) {
                name = dev_attr_status.attr.name;
                sysfs_notify(&rdev->dev.kobj, NULL, name);
        }

Let me know if this can be further tuned to be better..

Regards,
Naresh

>
> Netlink feels like it might be a better fit?  Not really looked at the
> kernel side of implementing that and how sensible that ends up looking.
Mark Brown Nov. 9, 2023, 11:31 a.m. UTC | #9 
On Thu, Nov 09, 2023 at 04:08:06PM +0530, Naresh Solanki wrote:
> On Thu, 2 Nov 2023 at 22:20, Mark Brown <broonie@kernel.org> wrote:

> > Ah, right.  Everything except for the enable and disable there looks
> > like it should be OK since they should normally just not happen but the
> > enables and disables might get a bit frequent with runtime PM - not
> > *super* frequent like voltage scaling but enough that people could have
> > an issue with it.

> I aim for a straightforward implementation.
> Using attributes such as status or state seems ideal for receiving
> notifications.
> In my case, the application focuses on status changes—whether it's on, off,
> or encountering an error.
> These changes are driven by events originating from the regulator.
> So below change is what I see fit well.
> 
>         status_events = REGULATOR_EVENT_DISABLE;
>         status_events |= REGULATOR_EVENT_ENABLE;
>         status_events |= REGULATOR_EVENT_FAIL;
>         status_events |= REGULATOR_EVENT_FORCE_DISABLE;
>         status_events |= REGULATOR_EVENT_ABORT_DISABLE;

In terms of the implementation for delivering uevents this looks fine,
my concern here is that for some systems the enable and disable events
might happen more often than is really a good fir for delivering via
uevents, if a device like say a SD card is getting powered up and down
via runtime PM that might happen relatively often and then the system
would get a lot of uevents which it would most likely end up ignoring.
That could have a noticable impact on power or performance.
Naresh Solanki Nov. 9, 2023, 3:11 p.m. UTC | #10 
Hi Mark,


On Thu, 9 Nov 2023 at 17:01, Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Nov 09, 2023 at 04:08:06PM +0530, Naresh Solanki wrote:
> > On Thu, 2 Nov 2023 at 22:20, Mark Brown <broonie@kernel.org> wrote:
>
> > > Ah, right.  Everything except for the enable and disable there looks
> > > like it should be OK since they should normally just not happen but the
> > > enables and disables might get a bit frequent with runtime PM - not
> > > *super* frequent like voltage scaling but enough that people could have
> > > an issue with it.
>
> > I aim for a straightforward implementation.
> > Using attributes such as status or state seems ideal for receiving
> > notifications.
> > In my case, the application focuses on status changes—whether it's on, off,
> > or encountering an error.
> > These changes are driven by events originating from the regulator.
> > So below change is what I see fit well.
> >
> >         status_events = REGULATOR_EVENT_DISABLE;
> >         status_events |= REGULATOR_EVENT_ENABLE;
> >         status_events |= REGULATOR_EVENT_FAIL;
> >         status_events |= REGULATOR_EVENT_FORCE_DISABLE;
> >         status_events |= REGULATOR_EVENT_ABORT_DISABLE;
>
> In terms of the implementation for delivering uevents this looks fine,
> my concern here is that for some systems the enable and disable events
> might happen more often than is really a good fir for delivering via
> uevents, if a device like say a SD card is getting powered up and down
> via runtime PM that might happen relatively often and then the system
> would get a lot of uevents which it would most likely end up ignoring.
> That could have a noticable impact on power or performance.

May be in that case should we consider adding a kernel parameter or
some property in sysfs attribute to allow getting events ?

Regards,
Naresh
Naresh Solanki Nov. 9, 2023, 3:21 p.m. UTC | #11 
Hi Mark,

On Thu, 9 Nov 2023 at 20:41, Naresh Solanki
<naresh.solanki@9elements.com> wrote:
>
> Hi Mark,
>
>
> On Thu, 9 Nov 2023 at 17:01, Mark Brown <broonie@kernel.org> wrote:
> >
> > On Thu, Nov 09, 2023 at 04:08:06PM +0530, Naresh Solanki wrote:
> > > On Thu, 2 Nov 2023 at 22:20, Mark Brown <broonie@kernel.org> wrote:
> >
> > > > Ah, right.  Everything except for the enable and disable there looks
> > > > like it should be OK since they should normally just not happen but the
> > > > enables and disables might get a bit frequent with runtime PM - not
> > > > *super* frequent like voltage scaling but enough that people could have
> > > > an issue with it.
> >
> > > I aim for a straightforward implementation.
> > > Using attributes such as status or state seems ideal for receiving
> > > notifications.
> > > In my case, the application focuses on status changes—whether it's on, off,
> > > or encountering an error.
> > > These changes are driven by events originating from the regulator.
> > > So below change is what I see fit well.
> > >
> > >         status_events = REGULATOR_EVENT_DISABLE;
> > >         status_events |= REGULATOR_EVENT_ENABLE;
> > >         status_events |= REGULATOR_EVENT_FAIL;
> > >         status_events |= REGULATOR_EVENT_FORCE_DISABLE;
> > >         status_events |= REGULATOR_EVENT_ABORT_DISABLE;
> >
> > In terms of the implementation for delivering uevents this looks fine,
> > my concern here is that for some systems the enable and disable events
> > might happen more often than is really a good fir for delivering via
> > uevents, if a device like say a SD card is getting powered up and down
> > via runtime PM that might happen relatively often and then the system
> > would get a lot of uevents which it would most likely end up ignoring.
> > That could have a noticable impact on power or performance.
>
> May be in that case should we consider adding a kernel parameter or
> some property in sysfs attribute to allow getting events ?
Or may be even Kconfig option ?
>
> Regards,
> Naresh
Mark Brown Nov. 13, 2023, 7:56 p.m. UTC | #12 
On Thu, Nov 09, 2023 at 08:51:40PM +0530, Naresh Solanki wrote:
> On Thu, 9 Nov 2023 at 20:41, Naresh Solanki

> > May be in that case should we consider adding a kernel parameter or
> > some property in sysfs attribute to allow getting events ?

> Or may be even Kconfig option ?

It's not great but a sysfs attribute *might* work, possibly sysfs files
per event like for tracing.  However it really does feel like a fit for
netlink - is there an issue you see with using that?
diff mbox series

Patch

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 3137e40fcd3e..ef5fa70ae2f1 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -98,6 +98,10 @@  static struct regulator *create_regulator(struct regulator_dev *rdev,
 static void destroy_regulator(struct regulator *regulator);
 static void _regulator_put(struct regulator *regulator);
 
+static struct device_attribute dev_attr_status;
+static struct device_attribute dev_attr_microvolts;
+static struct device_attribute dev_attr_state;
+
 const char *rdev_get_name(struct regulator_dev *rdev)
 {
 	if (rdev->constraints && rdev->constraints->name)
@@ -2798,6 +2802,8 @@  static int _regulator_do_enable(struct regulator_dev *rdev)
 		_regulator_delay_helper(delay);
 	}
 
+	sysfs_notify(&rdev->dev.kobj, NULL, dev_attr_state.attr.name);
+
 	trace_regulator_enable_complete(rdev_get_name(rdev));
 
 	return 0;
@@ -2980,6 +2986,8 @@  static int _regulator_do_disable(struct regulator_dev *rdev)
 	if (rdev->desc->off_on_delay)
 		rdev->last_off = ktime_get_boottime();
 
+	sysfs_notify(&rdev->dev.kobj, NULL, dev_attr_state.attr.name);
+
 	trace_regulator_disable_complete(rdev_get_name(rdev));
 
 	return 0;
@@ -4848,8 +4856,21 @@  EXPORT_SYMBOL_GPL(regulator_unregister_notifier);
 static int _notifier_call_chain(struct regulator_dev *rdev,
 				  unsigned long event, void *data)
 {
+	const char *name;
+	int ret;
+
 	/* call rdev chain first */
-	return blocking_notifier_call_chain(&rdev->notifier, event, data);
+	ret =  blocking_notifier_call_chain(&rdev->notifier, event, data);
+
+	if (event & REGULATOR_EVENT_VOLTAGE_CHANGE) {
+		name = dev_attr_microvolts.attr.name;
+		sysfs_notify(&rdev->dev.kobj, NULL, name);
+	} else {
+		name = dev_attr_status.attr.name;
+		sysfs_notify(&rdev->dev.kobj, NULL, name);
+	}
+
+	return ret;
 }
 
 int _regulator_bulk_get(struct device *dev, int num_consumers,