LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH RFC] regulator: core: fix handling negative voltages e.g. in EPD PMICs
@ 2020-02-23 15:35 Andreas Kemnade
  2020-02-24 12:05 ` Mark Brown
  2020-02-24 15:21 ` Denis 'GNUtoo' Carikli
  0 siblings, 2 replies; 7+ messages in thread
From: Andreas Kemnade @ 2020-02-23 15:35 UTC (permalink / raw)
  To: hns, j.neuschaefer, contact, GNUtoo, josua.mayer, lgirdwood,
	broonie, linux-kernel
  Cc: Andreas Kemnade

Basically regulator_get_voltage() returns the voltage and
 -Esomething on error. If the voltage is negative, results of
regulator_ops->get_voltage() are interpreted as error in the core,
so even if a consumer can handle negative voltages, it won't
get access to the regulator because the regulator will not be
registered at all due to a failing set_machine_constraints() call.

An alternative would be to handle voltages as absolute values.
There are probably no regulators with support both negative
and positive output.

The patch solves only the registration problem and does not fix
everything involved.

It has to be checked whether there is anything in mainline kernel
which uses negative voltages.

There are several regulator drivers found in the wild (often with
kernel version <= 4.1.15) for EPD PMICs which use negative
voltages for their VCOM regulator:
- sy7636
- fp9928
- tps6518x
- max17135

consumer is e.g. the EPDC of imx6 SoCs which is not in mainline.
Typical out-of-tree devicetrees contain snippets like this:
 VCOM_reg: VCOM {
      regulator-name = "VCOM";
      /* 2's-compliment, -4325000 */
      regulator-min-microvolt = <0xffbe0178>;
      /* 2's-compliment, -500000 */
      regulator-max-microvolt = <0xfff85ee0>;
 };

This kind of description will cause trouble as soon as these
uint32_t are casted to an int64_t (now they are casted to int
on 32bit architectures).

For the following devices which contain the tps6518x there are
partial devicetrees in mainline:
- Kobo Aura (N514)
- Kobo Clara HD
- Tolino Shine 3

No idea about the "Amazon Kindle Fire (first generation)"
which also has a partial devicetree in mainline.

Before cleaning up these drivers for upstreaming it would be
good to know which road to go in regards of negative voltages.

If we go the road with absolute voltages, there is a risk of
accidential booting a vendor devicetree with mainline kernel
this kind of negative voltages, which might cause bogous setups
so maybe voltages with bit 31 set should be filtered by the core,
besides of additional checking in the drivers itself.
Lukily form my experience, these out-of-tree devicetrees for
imx6 systems are incompatible enough to not boot even to a serial
console.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/regulator/core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index d015d99cb59d..2f2f31c3b9f1 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1129,7 +1129,7 @@ static int machine_constraints_voltage(struct regulator_dev *rdev,
 			current_uV = regulator_get_voltage_rdev(rdev);
 		}
 
-		if (current_uV < 0) {
+		if ((current_uV < 0) && (current_uV > -MAX_ERRNO)) {
 			rdev_err(rdev,
 				 "failed to get the current voltage(%d)\n",
 				 current_uV);
@@ -3022,7 +3022,7 @@ int regulator_is_supported_voltage(struct regulator *regulator,
 	/* If we can't change voltage check the current voltage */
 	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_VOLTAGE)) {
 		ret = regulator_get_voltage(regulator);
-		if (ret >= 0)
+		if ((ret >= 0) || (ret < -MAX_ERRNO))
 			return min_uV <= ret && ret <= max_uV;
 		else
 			return ret;
@@ -4031,7 +4031,7 @@ int regulator_get_voltage_rdev(struct regulator_dev *rdev)
 		return -EINVAL;
 	}
 
-	if (ret < 0)
+	if ((ret < 0) && (ret > -MAX_ERRNO))
 		return ret;
 	return ret - rdev->constraints->uV_offset;
 }
-- 
2.20.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH RFC] regulator: core: fix handling negative voltages e.g. in EPD PMICs
  2020-02-23 15:35 [PATCH RFC] regulator: core: fix handling negative voltages e.g. in EPD PMICs Andreas Kemnade
@ 2020-02-24 12:05 ` Mark Brown
  2020-02-24 12:22   ` H. Nikolaus Schaller
  2020-02-24 18:46   ` Andreas Kemnade
  2020-02-24 15:21 ` Denis 'GNUtoo' Carikli
  1 sibling, 2 replies; 7+ messages in thread
From: Mark Brown @ 2020-02-24 12:05 UTC (permalink / raw)
  To: Andreas Kemnade
  Cc: hns, j.neuschaefer, contact, GNUtoo, josua.mayer, lgirdwood,
	linux-kernel

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

On Sun, Feb 23, 2020 at 04:35:01PM +0100, Andreas Kemnade wrote:

> An alternative would be to handle voltages as absolute values.
> There are probably no regulators with support both negative
> and positive output.

This is what'd be needed, your approach here is a bit of a hack and
leaves some values unrepresentable if they overlap with errnos which
obviously has issues if someone has a need for those values.  Ground is
to a large extent somewhat arbatrary anyway and some systems do just
redefine it as part of their normal operation (eg, VMID based audio
systems) so this wouldn't be a huge departure.

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH RFC] regulator: core: fix handling negative voltages e.g. in EPD PMICs
  2020-02-24 12:05 ` Mark Brown
@ 2020-02-24 12:22   ` H. Nikolaus Schaller
  2020-02-24 12:31     ` Mark Brown
  2020-02-24 18:46   ` Andreas Kemnade
  1 sibling, 1 reply; 7+ messages in thread
From: H. Nikolaus Schaller @ 2020-02-24 12:22 UTC (permalink / raw)
  To: Mark Brown
  Cc: Andreas Kemnade, j.neuschaefer, contact, GNUtoo, josua.mayer,
	lgirdwood, linux-kernel


> Am 24.02.2020 um 13:05 schrieb Mark Brown <broonie@kernel.org>:
> 
> On Sun, Feb 23, 2020 at 04:35:01PM +0100, Andreas Kemnade wrote:
> 
>> An alternative would be to handle voltages as absolute values.
>> There are probably no regulators with support both negative
>> and positive output.
> 
> This is what'd be needed, your approach here is a bit of a hack and
> leaves some values unrepresentable if they overlap with errnos which
> obviously has issues if someone has a need for those values.

Negative ERRNOs have BIT(31) set.

Since voltage integers on a 32 bit architecture represent µV this would
still allow voltages up to (2^31 - 1) µV i.e. 2 kV with BIT(31) not set.

Therefore it seems very unlikely that anyone needs to represent voltages
above 2 kV within a Linux system through a regulator.

So I'd say any negative return value got regulator_get_voltage() can be
treated as an error.

And regulators for negative voltages could be represented by
their absolute value (and maybe a _neg component in the regulator
name).

But then it seems to be a little inconsistent that the voltage
parameters of regulator_set_voltage_unlocked() are signed integers
and not unsigned.

So shouldn't that be protected against attempting to set negative voltages?

Just my 2 cts.

BR,
Nikolaus

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH RFC] regulator: core: fix handling negative voltages e.g. in EPD PMICs
  2020-02-24 12:22   ` H. Nikolaus Schaller
@ 2020-02-24 12:31     ` Mark Brown
  0 siblings, 0 replies; 7+ messages in thread
From: Mark Brown @ 2020-02-24 12:31 UTC (permalink / raw)
  To: H. Nikolaus Schaller
  Cc: Andreas Kemnade, j.neuschaefer, contact, GNUtoo, josua.mayer,
	lgirdwood, linux-kernel

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

On Mon, Feb 24, 2020 at 01:22:21PM +0100, H. Nikolaus Schaller wrote:
> > Am 24.02.2020 um 13:05 schrieb Mark Brown <broonie@kernel.org>:

> > This is what'd be needed, your approach here is a bit of a hack and
> > leaves some values unrepresentable if they overlap with errnos which
> > obviously has issues if someone has a need for those values.

> Negative ERRNOs have BIT(31) set.

This code is working with the numberic representation, not with the
bitwise representation - it's using -MAX_ERRNO.  

> But then it seems to be a little inconsistent that the voltage
> parameters of regulator_set_voltage_unlocked() are signed integers
> and not unsigned.

> So shouldn't that be protected against attempting to set negative voltages?

Or just convert it to unsigned, I don't recall there being any
particular reason why it's signed.

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH RFC] regulator: core: fix handling negative voltages e.g. in EPD PMICs
  2020-02-23 15:35 [PATCH RFC] regulator: core: fix handling negative voltages e.g. in EPD PMICs Andreas Kemnade
  2020-02-24 12:05 ` Mark Brown
@ 2020-02-24 15:21 ` Denis 'GNUtoo' Carikli
  2020-02-24 17:00   ` Mark Brown
  1 sibling, 1 reply; 7+ messages in thread
From: Denis 'GNUtoo' Carikli @ 2020-02-24 15:21 UTC (permalink / raw)
  To: Andreas Kemnade
  Cc: hns, j.neuschaefer, contact, josua.mayer, lgirdwood, broonie,
	linux-kernel

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

On Sun, 23 Feb 2020 16:35:01 +0100
Andreas Kemnade <andreas@kemnade.info> wrote:

> No idea about the "Amazon Kindle Fire (first generation)"
> which also has a partial devicetree in mainline.
I think that the first generation Kindle Fire (omap4-kc1.dts) are
regular tablet and have a regular display (no e-ink).

Denis.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH RFC] regulator: core: fix handling negative voltages e.g. in EPD PMICs
  2020-02-24 15:21 ` Denis 'GNUtoo' Carikli
@ 2020-02-24 17:00   ` Mark Brown
  0 siblings, 0 replies; 7+ messages in thread
From: Mark Brown @ 2020-02-24 17:00 UTC (permalink / raw)
  To: Denis 'GNUtoo' Carikli
  Cc: Andreas Kemnade, hns, j.neuschaefer, contact, josua.mayer,
	lgirdwood, linux-kernel

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

On Mon, Feb 24, 2020 at 04:21:31PM +0100, Denis 'GNUtoo' Carikli wrote:
> Andreas Kemnade <andreas@kemnade.info> wrote:

> > No idea about the "Amazon Kindle Fire (first generation)"
> > which also has a partial devicetree in mainline.

> I think that the first generation Kindle Fire (omap4-kc1.dts) are
> regular tablet and have a regular display (no e-ink).

Right, all Kindle Fire devices have regular displays.

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH RFC] regulator: core: fix handling negative voltages e.g. in EPD PMICs
  2020-02-24 12:05 ` Mark Brown
  2020-02-24 12:22   ` H. Nikolaus Schaller
@ 2020-02-24 18:46   ` Andreas Kemnade
  1 sibling, 0 replies; 7+ messages in thread
From: Andreas Kemnade @ 2020-02-24 18:46 UTC (permalink / raw)
  To: Mark Brown
  Cc: hns, j.neuschaefer, contact, GNUtoo, josua.mayer, lgirdwood,
	linux-kernel

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

Hi,

On Mon, 24 Feb 2020 12:05:12 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Sun, Feb 23, 2020 at 04:35:01PM +0100, Andreas Kemnade wrote:
> 
> > An alternative would be to handle voltages as absolute values.
> > There are probably no regulators with support both negative
> > and positive output.  
> 
> This is what'd be needed, your approach here is a bit of a hack and
> leaves some values unrepresentable if they overlap with errnos which
> obviously has issues if someone has a need for those values.  Ground is
> to a large extent somewhat arbatrary anyway and some systems do just
> redefine it as part of their normal operation (eg, VMID based audio
> systems) so this wouldn't be a huge departure.

Thanks for clarification, outputs from 0mV to -4mV would not be be
representable. I am now converting stuff to think positive ;-) That
is good to decide early before pushing my huge pile of things needed
for EPD support on top of mainline somewhere.
Parallel I am evaluating upstreaming of the tps65181 driver which
probably ends up in a rewrite...

Regards,
Andreas

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-02-24 18:46 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-23 15:35 [PATCH RFC] regulator: core: fix handling negative voltages e.g. in EPD PMICs Andreas Kemnade
2020-02-24 12:05 ` Mark Brown
2020-02-24 12:22   ` H. Nikolaus Schaller
2020-02-24 12:31     ` Mark Brown
2020-02-24 18:46   ` Andreas Kemnade
2020-02-24 15:21 ` Denis 'GNUtoo' Carikli
2020-02-24 17:00   ` Mark Brown

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).