[PATCH v2 0/2] regulator: bd9571mwv: Add support for toggle power switches

[PATCH v2 0/2] regulator: bd9571mwv: Add support for toggle power switches

[Geert Uytterhoeven]

	Hi all,

The ROHM BD9571MWV PMIC on the Renesas Salvator-X(S) and ULCB
development boards supports DDR Backup Power, which means that the DDR
power rails can be kept powered while the main SoC is powered down.

This patch series extends the support for DDR backup mode (see commit
6eb0bfae6973eb6a ("regulator: bd9571mwv: Add support for backup mode"))
to systems with toggle instead of momentary power switches.

With a toggle power switch (or level signal), the following steps must
be followed exactly:
   1. Configure PMIC for backup mode, which changes the role of the
      power switch to a wake-up switch, 
   2. Switch accessory power switch off, to prepare for system suspend,
      which is a manual step not controlled by software,
   3. Suspend system,
   4. Switch accessory power switch on, to resume.

Unlike on systems with a momentary toggle switch, an additional step 2
must be performed in between step 1 and step 3.  Hence step 1 can no
longer be handled in the PMIC's suspend callback.

This patch series proposes a new callback for wake-up change
notification, which allows performing step 1 when the user writes
"enabled" to the PMIC's "wakeup" virtual file in sysfs, e.g.

    echo enabled > /sys/devices/platform/soc/e60b0000.i2c/i2c-7/7-0030/bd9571mwv-regulator.2.auto/power/wakeup

Conversely, writing "disabled" reverts the role of the accessory switch
to a power switch.
Note that unlike with momentary switches, backup mode is not enabled by
default, as enabling it prevents the board from being powered off using
the power switch, which may confuse the user.

Changes compared to v1:
  - Improve patch descriptions,
  - Drop "return;" at end of function.

This has been tested on M3ULCB (thanks Jacopo!), and on Salvator-X(S).

For testing, this series is also available in the
topic/bd9571-toggle-power-switch-v2 branch of my renesas-drivers git
repository at
git://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git.

Thanks!

Geert Uytterhoeven (2):
  PM / wakeup: Add callback for wake-up change notification
  regulator: bd9571mwv: Add support for toggle power switches

 drivers/base/power/wakeup.c             |  4 ++++
 drivers/regulator/bd9571mwv-regulator.c | 23 +++++++++++++++++++++++
 include/linux/pm.h                      |  1 +
 3 files changed, 28 insertions(+)

-- 
2.17.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH v2 1/2] PM / wakeup: Add callback for wake-up change notification

[Geert Uytterhoeven]

Add a callback to inform a device that its wake-up setting has been
changed.  This allows a device to synchronize device configuration with
an external user action.

E.g. on systems using a Rohm BD9571MWV PMIC and a toggle accessory power
switch, the system suspend/resume procedure is:
  1. Configure PMIC for DDR backup mode (by software), which changes the
     role of the accessory power switch from a power to a wake-up
     switch,
  2. Switch accessory power switch off (manually), to prepare for system
     suspend,
  3. Suspend system (by software),
  4. Switch accessory power switch on (manually), to wake up the system.

As step 2 involves a manual operation, step 1 cannot be combined
with step 3 and performed in the PMIC's suspend callback (unlike on
systems with a momentary power switch).

Adding the new callback allows to move step 1 to the new callback, to be
performed in response to the user writing "enabled" to the PMIC's
"wakeup" virtual file in sysfs.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Is there a better way to handle this?

Currently step 1 is done in userspace using i2set, which is very
user-unfriendly
(https://elinux.org/R-Car/Boards/Salvator-XS#PSCI_System_Suspend).

v2:
  - Improve patch description.
---
 drivers/base/power/wakeup.c | 4 ++++
 include/linux/pm.h          | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index 5fa1898755a34878..933560d658692fe3 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -255,6 +255,8 @@ static int device_wakeup_attach(struct device *dev, struct wakeup_source *ws)
 	if (dev->power.wakeirq)
 		device_wakeup_attach_irq(dev, dev->power.wakeirq);
 	spin_unlock_irq(&dev->power.lock);
+	if (dev->power.wakeup_change_notify)
+		dev->power.wakeup_change_notify(dev, true);
 	return 0;
 }
 
@@ -372,6 +374,8 @@ static struct wakeup_source *device_wakeup_detach(struct device *dev)
 {
 	struct wakeup_source *ws;
 
+	if (dev->power.wakeup_change_notify)
+		dev->power.wakeup_change_notify(dev, false);
 	spin_lock_irq(&dev->power.lock);
 	ws = dev->power.wakeup;
 	dev->power.wakeup = NULL;
diff --git a/include/linux/pm.h b/include/linux/pm.h
index e723b78d835706f2..3dec274bffffc6f8 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -599,6 +599,7 @@ struct dev_pm_info {
 	struct list_head	entry;
 	struct completion	completion;
 	struct wakeup_source	*wakeup;
+	void (*wakeup_change_notify)(struct device *dev, bool enable);
 	bool			wakeup_path:1;
 	bool			syscore:1;
 	bool			no_pm_callbacks:1;	/* Owned by the PM core */
-- 
2.17.1

[PATCH v2 2/2] regulator: bd9571mwv: Add support for toggle power switches

[Geert Uytterhoeven]

Extend the existing support for backup mode to toggle power switches.
With a toggle power switch (or level signal), the following steps must
be followed exactly:
   1. Configure PMIC for backup mode,
   2. Switch accessory power switch off, to prepare for system suspend,
      which is a manual step not controlled by software,
   3. Suspend system,
   4. Switch accessory power switch on to wake up system.

Hence the PMIC is configured for backup mode when "enabled" is written
to the PMIC's "wakeup" virtual file in sysfs, using the newly introduced
callback for wake-up change notification.  Conversely, writing
"disabled" reverts the role of the accessory switch to a power switch.

Unlike with momentary switches, backup mode is not enabled by default,
as enabling it prevents the board from being powered off using the power
switch, which may confuse the user.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - Improve patch description,
  - Drop "return;" at end of function.
---
 drivers/regulator/bd9571mwv-regulator.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/regulator/bd9571mwv-regulator.c b/drivers/regulator/bd9571mwv-regulator.c
index be574eb444ebda97..403bc3fa1f2513de 100644
--- a/drivers/regulator/bd9571mwv-regulator.c
+++ b/drivers/regulator/bd9571mwv-regulator.c
@@ -171,6 +171,24 @@ static int bd9571mwv_bkup_mode_write(struct bd9571mwv *bd, unsigned int mode)
 	return 0;
 }
 
+static void bd9571mwv_wakeup_change_notify(struct device *dev, bool enable)
+{
+	struct bd9571mwv_reg *bdreg = dev_get_drvdata(dev);
+
+	unsigned int mode;
+	int ret;
+
+	ret = bd9571mwv_bkup_mode_read(bdreg->bd, &mode);
+	if (ret)
+		return;
+
+	mode &= ~BD9571MWV_BKUP_MODE_CNT_KEEPON_MASK;
+	if (enable)
+		mode |= bdreg->bkup_mode_cnt_keepon;
+
+	bd9571mwv_bkup_mode_write(bdreg->bd, mode);
+}
+
 static int bd9571mwv_suspend(struct device *dev)
 {
 	struct bd9571mwv_reg *bdreg = dev_get_drvdata(dev);
@@ -277,6 +295,11 @@ static int bd9571mwv_regulator_probe(struct platform_device *pdev)
 		 * explicit user setup in level mode.
 		 */
 		device_set_wakeup_enable(&pdev->dev, bdreg->rstbmode_pulse);
+#ifdef CONFIG_PM_SLEEP
+		if (bdreg->rstbmode_level)
+			pdev->dev.power.wakeup_change_notify =
+				bd9571mwv_wakeup_change_notify;
+#endif /* CONFIG_PM_SLEEP */
 	}
 
 	return 0;
-- 
2.17.1

Re: [PATCH v2 1/2] PM / wakeup: Add callback for wake-up change notification

[Rafael J. Wysocki]

On Tue, Jun 19, 2018 at 3:55 PM, Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
> Add a callback to inform a device that its wake-up setting has been
> changed.  This allows a device to synchronize device configuration with
> an external user action.
>
> E.g. on systems using a Rohm BD9571MWV PMIC and a toggle accessory power
> switch, the system suspend/resume procedure is:
>   1. Configure PMIC for DDR backup mode (by software), which changes the
>      role of the accessory power switch from a power to a wake-up
>      switch,
>   2. Switch accessory power switch off (manually), to prepare for system
>      suspend,
>   3. Suspend system (by software),
>   4. Switch accessory power switch on (manually), to wake up the system.
>
> As step 2 involves a manual operation, step 1 cannot be combined
> with step 3 and performed in the PMIC's suspend callback (unlike on
> systems with a momentary power switch).
>
> Adding the new callback allows to move step 1 to the new callback, to be
> performed in response to the user writing "enabled" to the PMIC's
> "wakeup" virtual file in sysfs.

I still don't quite understand this TBH.

In particular, why do you want a write to "wakeup" trigger this
instead of having a special sysfs attr for that exposed by your PMIC
driver?

Writing "enabled" to "wakeup" for the PMIC should enable the PMIC
itself to wake up the system, which isn't quite the case, or is it?

Thanks,
Rafael

Re: [PATCH v2 1/2] PM / wakeup: Add callback for wake-up change notification

[Geert Uytterhoeven]

Hi Rafael,

On Tue, Jun 19, 2018 at 4:48 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Tue, Jun 19, 2018 at 3:55 PM, Geert Uytterhoeven
> <geert+renesas@glider.be> wrote:
> > Add a callback to inform a device that its wake-up setting has been
> > changed.  This allows a device to synchronize device configuration with
> > an external user action.
> >
> > E.g. on systems using a Rohm BD9571MWV PMIC and a toggle accessory power
> > switch, the system suspend/resume procedure is:
> >   1. Configure PMIC for DDR backup mode (by software), which changes the
> >      role of the accessory power switch from a power to a wake-up
> >      switch,
> >   2. Switch accessory power switch off (manually), to prepare for system
> >      suspend,
> >   3. Suspend system (by software),
> >   4. Switch accessory power switch on (manually), to wake up the system.
> >
> > As step 2 involves a manual operation, step 1 cannot be combined
> > with step 3 and performed in the PMIC's suspend callback (unlike on
> > systems with a momentary power switch).
> >
> > Adding the new callback allows to move step 1 to the new callback, to be
> > performed in response to the user writing "enabled" to the PMIC's
> > "wakeup" virtual file in sysfs.
>
> I still don't quite understand this TBH.
>
> In particular, why do you want a write to "wakeup" trigger this
> instead of having a special sysfs attr for that exposed by your PMIC
> driver?

In v1 (https://patchwork.kernel.org/patch/9996567/), I had a
driver-specific "backup_mode" sysfs file.
In v2 and later of the driver series (https://lkml.org/lkml/2018/4/18/345),
I changed that to use the standard "wakeup" file in sysfs, in response to
a comment from Mark Brown.

> Writing "enabled" to "wakeup" for the PMIC should enable the PMIC
> itself to wake up the system, which isn't quite the case, or is it?

Actually the PMIC cannot wake up the system if backup mode is not enabled.
When suspending the system (PSCI suspend) without enabling backup mode
first, the system will just crash (which cannot be distinguished from being
suspended, as PSCI doesn't support any other wake-up sources anyway[*] ;-)
So in that sense writing "enabled" to the "wakeup" file does enable the
PMIC to wake up the system.

Do you still prefer a driver-specific sysfs file?
Thanks!

[*] Perhaps you remember my ill-received attempt to prevent the system from
    using PSCI system suspend when any other wake-up sources were enabled?
    ("[PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts
     power", https://lkml.org/lkml/2017/2/20/566).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 1/2] PM / wakeup: Add callback for wake-up change notification

[Rafael J. Wysocki]

On Tuesday, June 19, 2018 5:22:06 PM CEST Geert Uytterhoeven wrote:
> Hi Rafael,
> 
> On Tue, Jun 19, 2018 at 4:48 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Tue, Jun 19, 2018 at 3:55 PM, Geert Uytterhoeven
> > <geert+renesas@glider.be> wrote:
> > > Add a callback to inform a device that its wake-up setting has been
> > > changed.  This allows a device to synchronize device configuration with
> > > an external user action.
> > >
> > > E.g. on systems using a Rohm BD9571MWV PMIC and a toggle accessory power
> > > switch, the system suspend/resume procedure is:
> > >   1. Configure PMIC for DDR backup mode (by software), which changes the
> > >      role of the accessory power switch from a power to a wake-up
> > >      switch,
> > >   2. Switch accessory power switch off (manually), to prepare for system
> > >      suspend,
> > >   3. Suspend system (by software),
> > >   4. Switch accessory power switch on (manually), to wake up the system.
> > >
> > > As step 2 involves a manual operation, step 1 cannot be combined
> > > with step 3 and performed in the PMIC's suspend callback (unlike on
> > > systems with a momentary power switch).
> > >
> > > Adding the new callback allows to move step 1 to the new callback, to be
> > > performed in response to the user writing "enabled" to the PMIC's
> > > "wakeup" virtual file in sysfs.
> >
> > I still don't quite understand this TBH.
> >
> > In particular, why do you want a write to "wakeup" trigger this
> > instead of having a special sysfs attr for that exposed by your PMIC
> > driver?
> 
> In v1 (https://patchwork.kernel.org/patch/9996567/), I had a
> driver-specific "backup_mode" sysfs file.
> In v2 and later of the driver series (https://lkml.org/lkml/2018/4/18/345),
> I changed that to use the standard "wakeup" file in sysfs, in response to
> a comment from Mark Brown.

Well, I'm not convinced that this is the right approach, though.

> > Writing "enabled" to "wakeup" for the PMIC should enable the PMIC
> > itself to wake up the system, which isn't quite the case, or is it?
> 
> Actually the PMIC cannot wake up the system if backup mode is not enabled.

That doesn't really matter.  "enabled" in "wakeup" only means that user space
allows the device to wake up the system from sleep, but it still may not be
able to do that for certain reasons (like in this case).  By adding this
callback you're changing the meaning of the attribute.

> When suspending the system (PSCI suspend) without enabling backup mode
> first, the system will just crash (which cannot be distinguished from being
> suspended, as PSCI doesn't support any other wake-up sources anyway[*] ;-)
> So in that sense writing "enabled" to the "wakeup" file does enable the
> PMIC to wake up the system.
> 
> Do you still prefer a driver-specific sysfs file?

Yes, I do.

Thanks,
Rafael

Re: [PATCH v2 1/2] PM / wakeup: Add callback for wake-up change notification

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1208 bytes --]

On Wed, Jun 20, 2018 at 10:02:41AM +0200, Rafael J. Wysocki wrote:
> On Tuesday, June 19, 2018 5:22:06 PM CEST Geert Uytterhoeven wrote:
> > On Tue, Jun 19, 2018 at 4:48 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > On Tue, Jun 19, 2018 at 3:55 PM, Geert Uytterhoeven

> > In v1 (https://patchwork.kernel.org/patch/9996567/), I had a
> > driver-specific "backup_mode" sysfs file.
> > In v2 and later of the driver series (https://lkml.org/lkml/2018/4/18/345),
> > I changed that to use the standard "wakeup" file in sysfs, in response to
> > a comment from Mark Brown.

> Well, I'm not convinced that this is the right approach, though.

The original description of the changes made it sound like this
controlled if the power switch would make the device resume from suspend
or not which seems like a wakeup to me.  Honestly as things are I've no
idea what the hardware designers were thinking or how to explain what
this stuff is doing.

> > Do you still prefer a driver-specific sysfs file?

> Yes, I do.

The flip side of that is that either suspend and resume or poweroff are
broken for userspace unless they know about this magic sysfs file which
isn't great either.  

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/2] PM / wakeup: Add callback for wake-up change notification

[Rafael J. Wysocki]

On Wed, Jun 20, 2018 at 12:35 PM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Jun 20, 2018 at 10:02:41AM +0200, Rafael J. Wysocki wrote:
>> On Tuesday, June 19, 2018 5:22:06 PM CEST Geert Uytterhoeven wrote:
>> > On Tue, Jun 19, 2018 at 4:48 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>> > > On Tue, Jun 19, 2018 at 3:55 PM, Geert Uytterhoeven
>
>> > In v1 (https://patchwork.kernel.org/patch/9996567/), I had a
>> > driver-specific "backup_mode" sysfs file.
>> > In v2 and later of the driver series (https://lkml.org/lkml/2018/4/18/345),
>> > I changed that to use the standard "wakeup" file in sysfs, in response to
>> > a comment from Mark Brown.
>
>> Well, I'm not convinced that this is the right approach, though.
>
> The original description of the changes made it sound like this
> controlled if the power switch would make the device resume from suspend
> or not which seems like a wakeup to me.  Honestly as things are I've no
> idea what the hardware designers were thinking or how to explain what
> this stuff is doing.
>
>> > Do you still prefer a driver-specific sysfs file?
>
>> Yes, I do.
>
> The flip side of that is that either suspend and resume or poweroff are
> broken for userspace unless they know about this magic sysfs file which
> isn't great either.

But to me that isn't that much different from an RTC wake alarm, say.

Enabling it to wake up the system in general isn't sufficient, you
also need to actually set the alarm using a different interface.

Re: [PATCH v2 1/2] PM / wakeup: Add callback for wake-up change notification

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1047 bytes --]

On Wed, Jun 20, 2018 at 02:15:38PM +0200, Rafael J. Wysocki wrote:
> On Wed, Jun 20, 2018 at 12:35 PM, Mark Brown <broonie@kernel.org> wrote:

> > The flip side of that is that either suspend and resume or poweroff are
> > broken for userspace unless they know about this magic sysfs file which
> > isn't great either.

> But to me that isn't that much different from an RTC wake alarm, say.

> Enabling it to wake up the system in general isn't sufficient, you
> also need to actually set the alarm using a different interface.

It seems more like hardware breakage we're trying to fix than a feature
- it's not like it's adding something we didn't have already (like
setting a time in an alarm where the alarm is an additional thing), more
just trying to execute on an existing user interface successfully.  I
can see that there's a case that it doesn't map very well onto the
standard interfaces so perhaps we have to add something on the side as
the hardware is just too horrible to fit in with the standard interfaces
and we have to do that.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/2] PM / wakeup: Add callback for wake-up change notification

[Geert Uytterhoeven]

On Wed, Jun 20, 2018 at 3:25 PM Mark Brown <broonie@kernel.org> wrote:
> On Wed, Jun 20, 2018 at 02:15:38PM +0200, Rafael J. Wysocki wrote:
> > On Wed, Jun 20, 2018 at 12:35 PM, Mark Brown <broonie@kernel.org> wrote:
>
> > > The flip side of that is that either suspend and resume or poweroff are
> > > broken for userspace unless they know about this magic sysfs file which
> > > isn't great either.
>
> > But to me that isn't that much different from an RTC wake alarm, say.
>
> > Enabling it to wake up the system in general isn't sufficient, you
> > also need to actually set the alarm using a different interface.

The RTC wake alarm time is indeed different, as it is not a simple boolean flag.
It is also more natural for the user, who expects to need to find some way to
configure the wake-up time.

> It seems more like hardware breakage we're trying to fix than a feature
> - it's not like it's adding something we didn't have already (like
> setting a time in an alarm where the alarm is an additional thing), more
> just trying to execute on an existing user interface successfully.  I
> can see that there's a case that it doesn't map very well onto the
> standard interfaces so perhaps we have to add something on the side as
> the hardware is just too horrible to fit in with the standard interfaces
> and we have to do that.

My main worry is usability: with a separate sysfs file, we need to document the
file, and the user needs to be aware of it.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 1/2] PM / wakeup: Add callback for wake-up change notification

[Rafael J. Wysocki]

On Tuesday, June 26, 2018 12:06:16 PM CEST Geert Uytterhoeven wrote:
> On Wed, Jun 20, 2018 at 3:25 PM Mark Brown <broonie@kernel.org> wrote:
> > On Wed, Jun 20, 2018 at 02:15:38PM +0200, Rafael J. Wysocki wrote:
> > > On Wed, Jun 20, 2018 at 12:35 PM, Mark Brown <broonie@kernel.org> wrote:
> >
> > > > The flip side of that is that either suspend and resume or poweroff are
> > > > broken for userspace unless they know about this magic sysfs file which
> > > > isn't great either.
> >
> > > But to me that isn't that much different from an RTC wake alarm, say.
> >
> > > Enabling it to wake up the system in general isn't sufficient, you
> > > also need to actually set the alarm using a different interface.
> 
> The RTC wake alarm time is indeed different, as it is not a simple boolean flag.
> It is also more natural for the user, who expects to need to find some way to
> configure the wake-up time.

OK, take Ethernet.  You need to configure WoL on that to wake up the system
in addition to setting power/wakeup for it.

Take WiFi: You need to set up WoW on that.

And so on.

> > It seems more like hardware breakage we're trying to fix than a feature
> > - it's not like it's adding something we didn't have already (like
> > setting a time in an alarm where the alarm is an additional thing), more
> > just trying to execute on an existing user interface successfully.  I
> > can see that there's a case that it doesn't map very well onto the
> > standard interfaces so perhaps we have to add something on the side as
> > the hardware is just too horrible to fit in with the standard interfaces
> > and we have to do that.
> 
> My main worry is usability: with a separate sysfs file, we need to document the
> file, and the user needs to be aware of it.

That's right, but it will be very hard to convince me that changing the
meaning of the "wakeup" attribute just in order to work around this issue
(which arguably is a consequence of "unfortunate" hardware design) is a
good idea. :-)

Re: [PATCH v2 1/2] PM / wakeup: Add callback for wake-up change notification

[Geert Uytterhoeven]

Hi Rafael,

On Tue, Jun 26, 2018 at 12:17 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Tuesday, June 26, 2018 12:06:16 PM CEST Geert Uytterhoeven wrote:
> > On Wed, Jun 20, 2018 at 3:25 PM Mark Brown <broonie@kernel.org> wrote:
> > > On Wed, Jun 20, 2018 at 02:15:38PM +0200, Rafael J. Wysocki wrote:
> > > > On Wed, Jun 20, 2018 at 12:35 PM, Mark Brown <broonie@kernel.org> wrote:
> > >
> > > > > The flip side of that is that either suspend and resume or poweroff are
> > > > > broken for userspace unless they know about this magic sysfs file which
> > > > > isn't great either.
> > >
> > > > But to me that isn't that much different from an RTC wake alarm, say.
> > >
> > > > Enabling it to wake up the system in general isn't sufficient, you
> > > > also need to actually set the alarm using a different interface.
> >
> > The RTC wake alarm time is indeed different, as it is not a simple boolean flag.
> > It is also more natural for the user, who expects to need to find some way to
> > configure the wake-up time.
>
> OK, take Ethernet.  You need to configure WoL on that to wake up the system
> in addition to setting power/wakeup for it.
>
> Take WiFi: You need to set up WoW on that.
>
> And so on.

I always found it strange that you have both "ethtool wol" and and a
"wakeup" file
in sysfs (does "ethtool wol" predate the wakeup file in sysfs?)

I believe originally WoL supported MagicPacket only (many systems still
support only that), so originally it was boolean.

> > > It seems more like hardware breakage we're trying to fix than a feature
> > > - it's not like it's adding something we didn't have already (like
> > > setting a time in an alarm where the alarm is an additional thing), more
> > > just trying to execute on an existing user interface successfully.  I
> > > can see that there's a case that it doesn't map very well onto the
> > > standard interfaces so perhaps we have to add something on the side as
> > > the hardware is just too horrible to fit in with the standard interfaces
> > > and we have to do that.
> >
> > My main worry is usability: with a separate sysfs file, we need to document the
> > file, and the user needs to be aware of it.
>
> That's right, but it will be very hard to convince me that changing the
> meaning of the "wakeup" attribute just in order to work around this issue
> (which arguably is a consequence of "unfortunate" hardware design) is a
> good idea. :-)

OK.

Next question: where to document device-specific sysfs files for regulators?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 1/2] PM / wakeup: Add callback for wake-up change notification

[Rafael J. Wysocki]

Hi Geert,

On Tue, Jun 26, 2018 at 12:29 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Rafael,
>
> On Tue, Jun 26, 2018 at 12:17 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Tuesday, June 26, 2018 12:06:16 PM CEST Geert Uytterhoeven wrote:
>> > On Wed, Jun 20, 2018 at 3:25 PM Mark Brown <broonie@kernel.org> wrote:
>> > > On Wed, Jun 20, 2018 at 02:15:38PM +0200, Rafael J. Wysocki wrote:
>> > > > On Wed, Jun 20, 2018 at 12:35 PM, Mark Brown <broonie@kernel.org> wrote:
>> > >
>> > > > > The flip side of that is that either suspend and resume or poweroff are
>> > > > > broken for userspace unless they know about this magic sysfs file which
>> > > > > isn't great either.
>> > >
>> > > > But to me that isn't that much different from an RTC wake alarm, say.
>> > >
>> > > > Enabling it to wake up the system in general isn't sufficient, you
>> > > > also need to actually set the alarm using a different interface.
>> >
>> > The RTC wake alarm time is indeed different, as it is not a simple boolean flag.
>> > It is also more natural for the user, who expects to need to find some way to
>> > configure the wake-up time.
>>
>> OK, take Ethernet.  You need to configure WoL on that to wake up the system
>> in addition to setting power/wakeup for it.
>>
>> Take WiFi: You need to set up WoW on that.
>>
>> And so on.
>
> I always found it strange that you have both "ethtool wol" and and a
> "wakeup" file
> in sysfs (does "ethtool wol" predate the wakeup file in sysfs?)

Yes, it does.

> I believe originally WoL supported MagicPacket only (many systems still
> support only that), so originally it was boolean.

I don't recall the details here.  When looked at it first time
multiple options had been there already.

>> > > It seems more like hardware breakage we're trying to fix than a feature
>> > > - it's not like it's adding something we didn't have already (like
>> > > setting a time in an alarm where the alarm is an additional thing), more
>> > > just trying to execute on an existing user interface successfully.  I
>> > > can see that there's a case that it doesn't map very well onto the
>> > > standard interfaces so perhaps we have to add something on the side as
>> > > the hardware is just too horrible to fit in with the standard interfaces
>> > > and we have to do that.
>> >
>> > My main worry is usability: with a separate sysfs file, we need to document the
>> > file, and the user needs to be aware of it.
>>
>> That's right, but it will be very hard to convince me that changing the
>> meaning of the "wakeup" attribute just in order to work around this issue
>> (which arguably is a consequence of "unfortunate" hardware design) is a
>> good idea. :-)
>
> OK.
>
> Next question: where to document device-specific sysfs files for regulators?

Under Documentation/ABI/ I suppose.