[PATCH] regulator: rn5t618: fix rc5t619 ldo10 enable

[PATCH] regulator: rn5t618: fix rc5t619 ldo10 enable

[Andreas Kemnade]

LDO9 and LDO10 were listed with the same enable bits.
That looks insane and there are no provisions in the code for handling such
a special case. Also other out-of-tree drivers use a separate bit to
enable it.
Example:
https://github.com/brunotl/kernel-kobo-mx6sl-ntx/blob/master/drivers/regulator/ricoh619-regulator.c
So it seems to be clearly a bug.
I cannot fully check it on my board without schematics and just discovered
this during code analysis for another problem.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/regulator/rn5t618-regulator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/rn5t618-regulator.c b/drivers/regulator/rn5t618-regulator.c
index eb807a059479..4a91be0ad5ae 100644
--- a/drivers/regulator/rn5t618-regulator.c
+++ b/drivers/regulator/rn5t618-regulator.c
@@ -90,7 +90,7 @@ static const struct regulator_desc rc5t619_regulators[] = {
 	REG(LDO7, LDOEN1, BIT(6), LDO7DAC, 0x7f, 900000, 3500000, 25000),
 	REG(LDO8, LDOEN1, BIT(7), LDO8DAC, 0x7f, 900000, 3500000, 25000),
 	REG(LDO9, LDOEN2, BIT(0), LDO9DAC, 0x7f, 900000, 3500000, 25000),
-	REG(LDO10, LDOEN2, BIT(0), LDO10DAC, 0x7f, 900000, 3500000, 25000),
+	REG(LDO10, LDOEN2, BIT(1), LDO10DAC, 0x7f, 900000, 3500000, 25000),
 	/* LDO RTC */
 	REG(LDORTC1, LDOEN2, BIT(4), LDORTCDAC, 0x7f, 1700000, 3500000, 25000),
 	REG(LDORTC2, LDOEN2, BIT(5), LDORTC2DAC, 0x7f, 900000, 3500000, 25000),
-- 
2.20.1

Re: [PATCH] regulator: rn5t618: fix rc5t619 ldo10 enable

[Mark Brown]

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

On Wed, Nov 13, 2019 at 07:26:43PM +0100, Andreas Kemnade wrote:
> LDO9 and LDO10 were listed with the same enable bits.
> That looks insane and there are no provisions in the code for handling such
> a special case. Also other out-of-tree drivers use a separate bit to
> enable it.

This definitely looks like a bug but without a datasheet or testing it's
worrying guessing at the register bit to use for the enable for the
second LDO...

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

Re: [PATCH] regulator: rn5t618: fix rc5t619 ldo10 enable

[Andreas Kemnade]

Hi,

On Wed, 13 Nov 2019 18:32:11 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Wed, Nov 13, 2019 at 07:26:43PM +0100, Andreas Kemnade wrote:
> > LDO9 and LDO10 were listed with the same enable bits.
> > That looks insane and there are no provisions in the code for handling such
> > a special case. Also other out-of-tree drivers use a separate bit to
> > enable it.  
> 
> This definitely looks like a bug but without a datasheet or testing it's
> worrying guessing at the register bit to use for the enable for the
> second LDO...

I am hoping for a Tested-By: from the one who has submitted the patch
for the regulator. 

Well, it is not just guessing, it is there in the url I referenced. But
I would of course prefer a better source. At first I wanted to spread
my findings.
I am not pushing anyone to accept it without Tested-By/Reviewed-Bys.
What is a good way to avoid people bumping into this bug?
Maybe I can find the right C on the board to check.

Regards,
Andreas

Re: [PATCH] regulator: rn5t618: fix rc5t619 ldo10 enable

[Mark Brown]

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

On Wed, Nov 13, 2019 at 08:26:33PM +0100, Andreas Kemnade wrote:
> Mark Brown <broonie@kernel.org> wrote:

> > This definitely looks like a bug but without a datasheet or testing it's
> > worrying guessing at the register bit to use for the enable for the
> > second LDO...

> I am hoping for a Tested-By: from the one who has submitted the patch
> for the regulator. 

Or a reviewed-by from someone with access to the datasheet.

> Well, it is not just guessing, it is there in the url I referenced. But
> I would of course prefer a better source. At first I wanted to spread
> my findings.

The URL you provided looked to be for a different part though?

> I am not pushing anyone to accept it without Tested-By/Reviewed-Bys.
> What is a good way to avoid people bumping into this bug?
> Maybe I can find the right C on the board to check.

That'd be good.  Or we could fix it by just removing enable/disable
control for the second LDO entirely and if someone needs that control
they can always re-add it.

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

Re: [PATCH] regulator: rn5t618: fix rc5t619 ldo10 enable

[Stefan Agner]

On 2019-11-14 12:54, Mark Brown wrote:
> On Wed, Nov 13, 2019 at 08:26:33PM +0100, Andreas Kemnade wrote:
>> Mark Brown <broonie@kernel.org> wrote:
> 
>> > This definitely looks like a bug but without a datasheet or testing it's
>> > worrying guessing at the register bit to use for the enable for the
>> > second LDO...
> 
>> I am hoping for a Tested-By: from the one who has submitted the patch
>> for the regulator.
> 
> Or a reviewed-by from someone with access to the datasheet.
> 

I guess Pierre-Hugues should have access, as he introduced the part?

>> Well, it is not just guessing, it is there in the url I referenced. But
>> I would of course prefer a better source. At first I wanted to spread
>> my findings.
> 
> The URL you provided looked to be for a different part though?
> 
>> I am not pushing anyone to accept it without Tested-By/Reviewed-Bys.
>> What is a good way to avoid people bumping into this bug?
>> Maybe I can find the right C on the board to check.
> 
> That'd be good.  Or we could fix it by just removing enable/disable
> control for the second LDO entirely and if someone needs that control
> they can always re-add it.

We use the RN5T567 and I added support for it. Unfortunately I have no
access to the RN5T618 datasheet so I cannot tell. The RN5T567 has both
bits in marked reserved, but looking at how it the enable bit are
distributed otherwise this patch fixes it in the only way it makes
sense...

--
Stefan

Re: [PATCH] regulator: rn5t618: fix rc5t619 ldo10 enable

[Andreas Kemnade]

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

On Thu, 14 Nov 2019 11:54:30 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Wed, Nov 13, 2019 at 08:26:33PM +0100, Andreas Kemnade wrote:
> > Mark Brown <broonie@kernel.org> wrote:  
> 
> > > This definitely looks like a bug but without a datasheet or testing it's
> > > worrying guessing at the register bit to use for the enable for the
> > > second LDO...  
> 
> > I am hoping for a Tested-By: from the one who has submitted the patch
> > for the regulator.   
> 
> Or a reviewed-by from someone with access to the datasheet.
> 
> > Well, it is not just guessing, it is there in the url I referenced. But
> > I would of course prefer a better source. At first I wanted to spread
> > my findings.  
> 
> The URL you provided looked to be for a different part though?
> 
No, they just skip the rc5t in the name. Same situation in the vendor
kernel for my device, but there is only a tarball online, so it is bad
to reference. Everything is named there ricoh619 (or 61x). And on the chip is
printed rc5t619.

Regards,
Andreas

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

Re: [PATCH] regulator: rn5t618: fix rc5t619 ldo10 enable

[Andreas Kemnade]

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

On Thu, 14 Nov 2019 13:13:47 +0100
Stefan Agner <stefan@agner.ch> wrote:

> On 2019-11-14 12:54, Mark Brown wrote:
> > On Wed, Nov 13, 2019 at 08:26:33PM +0100, Andreas Kemnade wrote:  
> >> Mark Brown <broonie@kernel.org> wrote:  
> >   
> >> > This definitely looks like a bug but without a datasheet or testing it's
> >> > worrying guessing at the register bit to use for the enable for the
> >> > second LDO...  
> >   
> >> I am hoping for a Tested-By: from the one who has submitted the patch
> >> for the regulator.  
> > 
> > Or a reviewed-by from someone with access to the datasheet.
> >   
> 
> I guess Pierre-Hugues should have access, as he introduced the part?
> 
> >> Well, it is not just guessing, it is there in the url I referenced. But
> >> I would of course prefer a better source. At first I wanted to spread
> >> my findings.  
> > 
> > The URL you provided looked to be for a different part though?
> >   
> >> I am not pushing anyone to accept it without Tested-By/Reviewed-Bys.
> >> What is a good way to avoid people bumping into this bug?
> >> Maybe I can find the right C on the board to check.  
> > 
> > That'd be good.  Or we could fix it by just removing enable/disable
> > control for the second LDO entirely and if someone needs that control
> > they can always re-add it.  
> 
> We use the RN5T567 and I added support for it. Unfortunately I have no
> access to the RN5T618 datasheet so I cannot tell. The RN5T567 has both
> bits in marked reserved, but looking at how it the enable bit are
> distributed otherwise this patch fixes it in the only way it makes
> sense...
> 
Well, the rn5t618 does not have these regulators, either. It is only the
rc5t619.

Regards,
Andreas

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

Re: [PATCH] regulator: rn5t618: fix rc5t619 ldo10 enable

[Mark Brown]

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

On Thu, Nov 14, 2019 at 03:24:51PM +0100, Andreas Kemnade wrote:
> Mark Brown <broonie@kernel.org> wrote:
> > On Wed, Nov 13, 2019 at 08:26:33PM +0100, Andreas Kemnade wrote:

> > > Well, it is not just guessing, it is there in the url I referenced. But
> > > I would of course prefer a better source. At first I wanted to spread
> > > my findings.  

> > The URL you provided looked to be for a different part though?

> No, they just skip the rc5t in the name. Same situation in the vendor
> kernel for my device, but there is only a tarball online, so it is bad
> to reference. Everything is named there ricoh619 (or 61x). And on the chip is
> printed rc5t619.

Ah, OK - that's probably good enough then.  Let's leave it a bit and see
if we can get some more definite review though.

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

Re: [PATCH] regulator: rn5t618: fix rc5t619 ldo10 enable

[Pierre-Hugues Husson]

Hello,

Sorry for the long time to answer, this thread went to spam...
So, to answer your questions:
- I don't have the datasheet of that chip
- My reference was rockchip vendor tree:
https://github.com/rockchip-linux/kernel/blob/release-3.10/drivers/regulator/ricoh619-regulator.c#L492
- That tree agrees with your fix. Sorry for the error!

I'll try finding the schematics of my board to know what's on LDO10,
and understand. But so far it looks like I've lost it...

Thanks for your series, all of those features are also needed on that
board, though I never got the courage to finish it.

Applied "regulator: rn5t618: fix rc5t619 ldo10 enable" to the regulator tree

[Mark Brown]

The patch

   regulator: rn5t618: fix rc5t619 ldo10 enable

has been applied to the regulator tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-5.4

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 3f1a9e630b6e124c387cc57f6fea517cec68b44f Mon Sep 17 00:00:00 2001
From: Andreas Kemnade <andreas@kemnade.info>
Date: Wed, 13 Nov 2019 19:26:43 +0100
Subject: [PATCH] regulator: rn5t618: fix rc5t619 ldo10 enable

LDO9 and LDO10 were listed with the same enable bits.
That looks insane and there are no provisions in the code for handling such
a special case. Also other out-of-tree drivers use a separate bit to
enable it.
Example:
https://github.com/brunotl/kernel-kobo-mx6sl-ntx/blob/master/drivers/regulator/ricoh619-regulator.c
So it seems to be clearly a bug.
I cannot fully check it on my board without schematics and just discovered
this during code analysis for another problem.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
Link: https://lore.kernel.org/r/20191113182643.23885-1-andreas@kemnade.info
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/rn5t618-regulator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/rn5t618-regulator.c b/drivers/regulator/rn5t618-regulator.c
index eb807a059479..4a91be0ad5ae 100644
--- a/drivers/regulator/rn5t618-regulator.c
+++ b/drivers/regulator/rn5t618-regulator.c
@@ -90,7 +90,7 @@ static const struct regulator_desc rc5t619_regulators[] = {
 	REG(LDO7, LDOEN1, BIT(6), LDO7DAC, 0x7f, 900000, 3500000, 25000),
 	REG(LDO8, LDOEN1, BIT(7), LDO8DAC, 0x7f, 900000, 3500000, 25000),
 	REG(LDO9, LDOEN2, BIT(0), LDO9DAC, 0x7f, 900000, 3500000, 25000),
-	REG(LDO10, LDOEN2, BIT(0), LDO10DAC, 0x7f, 900000, 3500000, 25000),
+	REG(LDO10, LDOEN2, BIT(1), LDO10DAC, 0x7f, 900000, 3500000, 25000),
 	/* LDO RTC */
 	REG(LDORTC1, LDOEN2, BIT(4), LDORTCDAC, 0x7f, 1700000, 3500000, 25000),
 	REG(LDORTC2, LDOEN2, BIT(5), LDORTC2DAC, 0x7f, 900000, 3500000, 25000),
-- 
2.20.1