| Message ID | 20180215100208.24646-1-marek.vasut+renesas@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Simon Horman |
| Headers | show |
On Thu, Feb 15, 2018 at 11:02:08AM +0100, Marek Vasut wrote: > Regulator setup is suboptimal on H2 Stout too. The Stout newly has > two DA9210 regulators, so the quirk is extended to handle another > DA9210 at i2c address 0x70. > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > Cc: Simon Horman <horms+renesas@verge.net.au> > Cc: Wolfram Sang <wsa+renesas@sang-engineering.com> Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Hi Marek, On Thu, Feb 15, 2018 at 11:02 AM, Marek Vasut <marek.vasut@gmail.com> wrote: > Regulator setup is suboptimal on H2 Stout too. The Stout newly has > two DA9210 regulators, so the quirk is extended to handle another > DA9210 at i2c address 0x70. > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > Cc: Simon Horman <horms+renesas@verge.net.au> > Cc: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > V2: - Handle another DA9210 at 0x70 > - Drop explicit board list from the leading comment in the file Thanks for the update! > --- a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c > +++ b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c > @@ -1,9 +1,9 @@ > /* > * R-Car Generation 2 da9063/da9210 regulator quirk > * > - * The r8a7790/lager and r8a7791/koelsch development boards have da9063 and > - * da9210 regulators. Both regulators have their interrupt request lines tied > - * to the same interrupt pin (IRQ2) on the SoC. > + * Certain Gen2 development boards have an da9063 and one or more da9210 > + * regulators. All of these regulators have their interrupt request lines > + * tied to the same interrupt pin (IRQ2) on the SoC. > * > * After cold boot or da9063-induced restart, both the da9063 and da9210 seem > * to assert their interrupt request lines. Hence as soon as one driver > @@ -59,6 +59,10 @@ static struct i2c_msg da9xxx_msgs[2] = { ^^^ > .addr = 0x68, > .len = ARRAY_SIZE(da9210_irq_clr), > .buf = da9210_irq_clr, > + }, { > + .addr = 0x70, > + .len = ARRAY_SIZE(da9210_irq_clr), > + .buf = da9210_irq_clr, Does this really work, without increasing the array size from [2] to [3]? > @@ -85,7 +89,8 @@ static int regulator_quirk_notify(struct notifier_block *nb, > dev_dbg(dev, "Detected %s\n", client->name); > > if ((client->addr == 0x58 && !strcmp(client->name, "da9063")) || > - (client->addr == 0x68 && !strcmp(client->name, "da9210"))) { > + (client->addr == 0x68 && !strcmp(client->name, "da9210")) || > + (client->addr == 0x70 && !strcmp(client->name, "da9210"))) { > int ret; > > dev_info(&client->dev, "clearing da9063/da9210 interrupts\n"); As the code now always sends 3 i2c msgs to all 3 possible regulators, we may accidentally talk to another device at 0x70 on other boards than Stout. 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
> As the code now always sends 3 i2c msgs to all 3 possible regulators, we > may accidentally talk to another device at 0x70 on other boards than Stout. It sends per device, so unless there is a device named "da9210" at addr 0x70, nothing will happen? Am I missing something?
Hi Wolfram, On Thu, Feb 15, 2018 at 11:19 AM, Wolfram Sang <wsa@the-dreams.de> wrote: >> As the code now always sends 3 i2c msgs to all 3 possible regulators, we >> may accidentally talk to another device at 0x70 on other boards than Stout. > > It sends per device, so unless there is a device named "da9210" at > addr 0x70, nothing will happen? Am I missing something? As soon as one of the affected devices is instantiated, it has to shut up all devices. This is done by sending the whole da9xxx_msgs[] array, which needs modification now we have boards with two or three devices. 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
> > It sends per device, so unless there is a device named "da9210" at > > addr 0x70, nothing will happen? Am I missing something? > > As soon as one of the affected devices is instantiated, it has to shut up > all devices. This is done by sending the whole da9xxx_msgs[] array, > which needs modification now we have boards with two or three devices. Ah, I missed the "done = true;" part in remove Thanks!
Hi Wolfram, On Thu, Feb 15, 2018 at 11:43 AM, Wolfram Sang <wsa@the-dreams.de> wrote: >> > It sends per device, so unless there is a device named "da9210" at >> > addr 0x70, nothing will happen? Am I missing something? >> >> As soon as one of the affected devices is instantiated, it has to shut up >> all devices. This is done by sending the whole da9xxx_msgs[] array, >> which needs modification now we have boards with two or three devices. > > Ah, I missed the "done = true;" part in remove Perhaps the code should be rewritten to not match board compatible values, but scan for da9063 and da9210 regulators, keep track of how many are found, and activate if more than one is found using the same interrupt? Yes, more complex code. But I have no idea how many Renesas customers copied this part of the board design... 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
On Thu, Feb 15, 2018 at 11:02:08AM +0100, Marek Vasut wrote: > Regulator setup is suboptimal on H2 Stout too. The Stout newly has > two DA9210 regulators, so the quirk is extended to handle another > DA9210 at i2c address 0x70. It looks like this patch has received some review that needs addressing. I have marked it as "Changes Requested".
On Thu, Feb 15, 2018 at 06:03:56PM +0100, Simon Horman wrote: > On Thu, Feb 15, 2018 at 11:02:08AM +0100, Marek Vasut wrote: > > Regulator setup is suboptimal on H2 Stout too. The Stout newly has > > two DA9210 regulators, so the quirk is extended to handle another > > DA9210 at i2c address 0x70. > > It looks like this patch has received some review that needs addressing. > I have marked it as "Changes Requested". ... and now I see v3 in my inbox, thanks.
diff --git a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c index 44438f344dc8..8324f9820fde 100644 --- a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c +++ b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c @@ -1,9 +1,9 @@ /* * R-Car Generation 2 da9063/da9210 regulator quirk * - * The r8a7790/lager and r8a7791/koelsch development boards have da9063 and - * da9210 regulators. Both regulators have their interrupt request lines tied - * to the same interrupt pin (IRQ2) on the SoC. + * Certain Gen2 development boards have an da9063 and one or more da9210 + * regulators. All of these regulators have their interrupt request lines + * tied to the same interrupt pin (IRQ2) on the SoC. * * After cold boot or da9063-induced restart, both the da9063 and da9210 seem * to assert their interrupt request lines. Hence as soon as one driver @@ -59,6 +59,10 @@ static struct i2c_msg da9xxx_msgs[2] = { .addr = 0x68, .len = ARRAY_SIZE(da9210_irq_clr), .buf = da9210_irq_clr, + }, { + .addr = 0x70, + .len = ARRAY_SIZE(da9210_irq_clr), + .buf = da9210_irq_clr, }, }; @@ -85,7 +89,8 @@ static int regulator_quirk_notify(struct notifier_block *nb, dev_dbg(dev, "Detected %s\n", client->name); if ((client->addr == 0x58 && !strcmp(client->name, "da9063")) || - (client->addr == 0x68 && !strcmp(client->name, "da9210"))) { + (client->addr == 0x68 && !strcmp(client->name, "da9210")) || + (client->addr == 0x70 && !strcmp(client->name, "da9210"))) { int ret; dev_info(&client->dev, "clearing da9063/da9210 interrupts\n"); @@ -118,6 +123,7 @@ static int __init rcar_gen2_regulator_quirk(void) if (!of_machine_is_compatible("renesas,koelsch") && !of_machine_is_compatible("renesas,lager") && + !of_machine_is_compatible("renesas,stout") && !of_machine_is_compatible("renesas,gose")) return -ENODEV;
Regulator setup is suboptimal on H2 Stout too. The Stout newly has two DA9210 regulators, so the quirk is extended to handle another DA9210 at i2c address 0x70. Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> Cc: Geert Uytterhoeven <geert+renesas@glider.be> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> Cc: Simon Horman <horms+renesas@verge.net.au> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com> --- V2: - Handle another DA9210 at 0x70 - Drop explicit board list from the leading comment in the file --- arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)