* [PATCH 1/2] ARM: shmobile: Factor out complex condition
@ 2018-02-17 2:06 ` Marek Vasut
0 siblings, 0 replies; 18+ messages in thread
From: Marek Vasut @ 2018-02-17 2:06 UTC (permalink / raw)
To: linux-renesas-soc
Cc: linux-arm-kernel, Marek Vasut, Geert Uytterhoeven,
Kuninori Morimoto, Simon Horman, Wolfram Sang
Pull the complex condition in regulator_quirk_notify() into
regulator_quirk_check(). Moreover, do not hard-code the I2C
address, but rather use the one in da9xxx_msgs[].
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>
---
arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
index 27fb3a5ec73e..862f7757ef5d 100644
--- a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
+++ b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
@@ -66,6 +66,13 @@ static struct i2c_msg da9xxx_msgs[3] = {
},
};
+static int regulator_quirk_check(struct i2c_client *client, u8 i2c_id,
+ char *compat_string)
+{
+ return client->addr == da9xxx_msgs[i2c_id].addr &&
+ !strcmp(client->name, compat_string);
+}
+
static int regulator_quirk_notify(struct notifier_block *nb,
unsigned long action, void *data)
{
@@ -88,9 +95,9 @@ static int regulator_quirk_notify(struct notifier_block *nb,
client = to_i2c_client(dev);
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 == 0x70 && !strcmp(client->name, "da9210"))) {
+ if (regulator_quirk_check(client, 0, "da9063") ||
+ regulator_quirk_check(client, 1, "da9210") ||
+ regulator_quirk_check(client, 2, "da9210")) {
int ret, len;
/* There are two DA9210 on Stout, one on the other boards. */
--
2.15.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 1/2] ARM: shmobile: Factor out complex condition
@ 2018-02-17 2:06 ` Marek Vasut
0 siblings, 0 replies; 18+ messages in thread
From: Marek Vasut @ 2018-02-17 2:06 UTC (permalink / raw)
To: linux-arm-kernel
Pull the complex condition in regulator_quirk_notify() into
regulator_quirk_check(). Moreover, do not hard-code the I2C
address, but rather use the one in da9xxx_msgs[].
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>
---
arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
index 27fb3a5ec73e..862f7757ef5d 100644
--- a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
+++ b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
@@ -66,6 +66,13 @@ static struct i2c_msg da9xxx_msgs[3] = {
},
};
+static int regulator_quirk_check(struct i2c_client *client, u8 i2c_id,
+ char *compat_string)
+{
+ return client->addr == da9xxx_msgs[i2c_id].addr &&
+ !strcmp(client->name, compat_string);
+}
+
static int regulator_quirk_notify(struct notifier_block *nb,
unsigned long action, void *data)
{
@@ -88,9 +95,9 @@ static int regulator_quirk_notify(struct notifier_block *nb,
client = to_i2c_client(dev);
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 == 0x70 && !strcmp(client->name, "da9210"))) {
+ if (regulator_quirk_check(client, 0, "da9063") ||
+ regulator_quirk_check(client, 1, "da9210") ||
+ regulator_quirk_check(client, 2, "da9210")) {
int ret, len;
/* There are two DA9210 on Stout, one on the other boards. */
--
2.15.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 2/2] ARM: shmobile: porter: enable R-Car Gen2 regulator quirk
2018-02-17 2:06 ` Marek Vasut
@ 2018-02-17 2:06 ` Marek Vasut
-1 siblings, 0 replies; 18+ messages in thread
From: Marek Vasut @ 2018-02-17 2:06 UTC (permalink / raw)
To: linux-renesas-soc
Cc: linux-arm-kernel, Marek Vasut, Geert Uytterhoeven,
Kuninori Morimoto, Simon Horman, Wolfram Sang
Porter needs the regulator quirk, just like the other boards,
the DA9063 and DA9210 IRQ line is connected to CPU IRQ2 . But
unlike the other boards, the DA9063 is at 0x5a on Porter.
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>
---
arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
index 862f7757ef5d..7963f0eea9e1 100644
--- a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
+++ b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
@@ -132,11 +132,16 @@ static int __init rcar_gen2_regulator_quirk(void)
u32 mon;
if (!of_machine_is_compatible("renesas,koelsch") &&
+ !of_machine_is_compatible("renesas,porter") &&
!of_machine_is_compatible("renesas,lager") &&
!of_machine_is_compatible("renesas,stout") &&
!of_machine_is_compatible("renesas,gose"))
return -ENODEV;
+ /* DA9063 on M2W Porter is at 0x5a */
+ if (of_machine_is_compatible("renesas,porter"))
+ da9xxx_msgs[0].addr = 0x5a;
+
irqc = ioremap(IRQC_BASE, PAGE_SIZE);
if (!irqc)
return -ENOMEM;
--
2.15.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 2/2] ARM: shmobile: porter: enable R-Car Gen2 regulator quirk
@ 2018-02-17 2:06 ` Marek Vasut
0 siblings, 0 replies; 18+ messages in thread
From: Marek Vasut @ 2018-02-17 2:06 UTC (permalink / raw)
To: linux-arm-kernel
Porter needs the regulator quirk, just like the other boards,
the DA9063 and DA9210 IRQ line is connected to CPU IRQ2 . But
unlike the other boards, the DA9063 is at 0x5a on Porter.
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>
---
arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
index 862f7757ef5d..7963f0eea9e1 100644
--- a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
+++ b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
@@ -132,11 +132,16 @@ static int __init rcar_gen2_regulator_quirk(void)
u32 mon;
if (!of_machine_is_compatible("renesas,koelsch") &&
+ !of_machine_is_compatible("renesas,porter") &&
!of_machine_is_compatible("renesas,lager") &&
!of_machine_is_compatible("renesas,stout") &&
!of_machine_is_compatible("renesas,gose"))
return -ENODEV;
+ /* DA9063 on M2W Porter is at 0x5a */
+ if (of_machine_is_compatible("renesas,porter"))
+ da9xxx_msgs[0].addr = 0x5a;
+
irqc = ioremap(IRQC_BASE, PAGE_SIZE);
if (!irqc)
return -ENOMEM;
--
2.15.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 2/2] ARM: shmobile: porter: enable R-Car Gen2 regulator quirk
2018-02-17 2:06 ` Marek Vasut
@ 2018-02-19 8:42 ` Geert Uytterhoeven
-1 siblings, 0 replies; 18+ messages in thread
From: Geert Uytterhoeven @ 2018-02-19 8:42 UTC (permalink / raw)
To: Marek Vasut
Cc: Linux-Renesas, linux-arm-kernel, Marek Vasut, Geert Uytterhoeven,
Kuninori Morimoto, Simon Horman, Wolfram Sang
On Sat, Feb 17, 2018 at 3:06 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
> Porter needs the regulator quirk, just like the other boards,
> the DA9063 and DA9210 IRQ line is connected to CPU IRQ2 . But
> unlike the other boards, the DA9063 is at 0x5a on Porter.
>
> 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>
> ---
> arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
> index 862f7757ef5d..7963f0eea9e1 100644
> --- a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
> +++ b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
> @@ -132,11 +132,16 @@ static int __init rcar_gen2_regulator_quirk(void)
> u32 mon;
>
> if (!of_machine_is_compatible("renesas,koelsch") &&
> + !of_machine_is_compatible("renesas,porter") &&
> !of_machine_is_compatible("renesas,lager") &&
> !of_machine_is_compatible("renesas,stout") &&
> !of_machine_is_compatible("renesas,gose"))
> return -ENODEV;
>
> + /* DA9063 on M2W Porter is at 0x5a */
> + if (of_machine_is_compatible("renesas,porter"))
> + da9xxx_msgs[0].addr = 0x5a;
> +
> irqc = ioremap(IRQC_BASE, PAGE_SIZE);
> if (!irqc)
> return -ENOMEM;
Doing the same of_machine_is_compatible() check again is an indicator that
you should switch to an of_device_id-based match interface...
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
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH 2/2] ARM: shmobile: porter: enable R-Car Gen2 regulator quirk
@ 2018-02-19 8:42 ` Geert Uytterhoeven
0 siblings, 0 replies; 18+ messages in thread
From: Geert Uytterhoeven @ 2018-02-19 8:42 UTC (permalink / raw)
To: linux-arm-kernel
On Sat, Feb 17, 2018 at 3:06 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
> Porter needs the regulator quirk, just like the other boards,
> the DA9063 and DA9210 IRQ line is connected to CPU IRQ2 . But
> unlike the other boards, the DA9063 is at 0x5a on Porter.
>
> 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>
> ---
> arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
> index 862f7757ef5d..7963f0eea9e1 100644
> --- a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
> +++ b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
> @@ -132,11 +132,16 @@ static int __init rcar_gen2_regulator_quirk(void)
> u32 mon;
>
> if (!of_machine_is_compatible("renesas,koelsch") &&
> + !of_machine_is_compatible("renesas,porter") &&
> !of_machine_is_compatible("renesas,lager") &&
> !of_machine_is_compatible("renesas,stout") &&
> !of_machine_is_compatible("renesas,gose"))
> return -ENODEV;
>
> + /* DA9063 on M2W Porter is at 0x5a */
> + if (of_machine_is_compatible("renesas,porter"))
> + da9xxx_msgs[0].addr = 0x5a;
> +
> irqc = ioremap(IRQC_BASE, PAGE_SIZE);
> if (!irqc)
> return -ENOMEM;
Doing the same of_machine_is_compatible() check again is an indicator that
you should switch to an of_device_id-based match interface...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at 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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 2/2] ARM: shmobile: porter: enable R-Car Gen2 regulator quirk
2018-02-19 8:42 ` Geert Uytterhoeven
@ 2018-02-19 8:51 ` Marek Vasut
-1 siblings, 0 replies; 18+ messages in thread
From: Marek Vasut @ 2018-02-19 8:51 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Linux-Renesas, linux-arm-kernel, Marek Vasut, Geert Uytterhoeven,
Kuninori Morimoto, Simon Horman, Wolfram Sang
On 02/19/2018 09:42 AM, Geert Uytterhoeven wrote:
> On Sat, Feb 17, 2018 at 3:06 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> Porter needs the regulator quirk, just like the other boards,
>> the DA9063 and DA9210 IRQ line is connected to CPU IRQ2 . But
>> unlike the other boards, the DA9063 is at 0x5a on Porter.
>>
>> 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>
>> ---
>> arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
>> index 862f7757ef5d..7963f0eea9e1 100644
>> --- a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
>> +++ b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
>> @@ -132,11 +132,16 @@ static int __init rcar_gen2_regulator_quirk(void)
>> u32 mon;
>>
>> if (!of_machine_is_compatible("renesas,koelsch") &&
>> + !of_machine_is_compatible("renesas,porter") &&
>> !of_machine_is_compatible("renesas,lager") &&
>> !of_machine_is_compatible("renesas,stout") &&
>> !of_machine_is_compatible("renesas,gose"))
>> return -ENODEV;
>>
>> + /* DA9063 on M2W Porter is at 0x5a */
>> + if (of_machine_is_compatible("renesas,porter"))
>> + da9xxx_msgs[0].addr = 0x5a;
>> +
>> irqc = ioremap(IRQC_BASE, PAGE_SIZE);
>> if (!irqc)
>> return -ENOMEM;
>
> Doing the same of_machine_is_compatible() check again is an indicator that
> you should switch to an of_device_id-based match interface...
Indeed. Also cfr my other suggestion in reply to WSA in this thread.
Thoughts on that welcome.
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH 2/2] ARM: shmobile: porter: enable R-Car Gen2 regulator quirk
@ 2018-02-19 8:51 ` Marek Vasut
0 siblings, 0 replies; 18+ messages in thread
From: Marek Vasut @ 2018-02-19 8:51 UTC (permalink / raw)
To: linux-arm-kernel
On 02/19/2018 09:42 AM, Geert Uytterhoeven wrote:
> On Sat, Feb 17, 2018 at 3:06 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> Porter needs the regulator quirk, just like the other boards,
>> the DA9063 and DA9210 IRQ line is connected to CPU IRQ2 . But
>> unlike the other boards, the DA9063 is at 0x5a on Porter.
>>
>> 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>
>> ---
>> arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
>> index 862f7757ef5d..7963f0eea9e1 100644
>> --- a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
>> +++ b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
>> @@ -132,11 +132,16 @@ static int __init rcar_gen2_regulator_quirk(void)
>> u32 mon;
>>
>> if (!of_machine_is_compatible("renesas,koelsch") &&
>> + !of_machine_is_compatible("renesas,porter") &&
>> !of_machine_is_compatible("renesas,lager") &&
>> !of_machine_is_compatible("renesas,stout") &&
>> !of_machine_is_compatible("renesas,gose"))
>> return -ENODEV;
>>
>> + /* DA9063 on M2W Porter is at 0x5a */
>> + if (of_machine_is_compatible("renesas,porter"))
>> + da9xxx_msgs[0].addr = 0x5a;
>> +
>> irqc = ioremap(IRQC_BASE, PAGE_SIZE);
>> if (!irqc)
>> return -ENOMEM;
>
> Doing the same of_machine_is_compatible() check again is an indicator that
> you should switch to an of_device_id-based match interface...
Indeed. Also cfr my other suggestion in reply to WSA in this thread.
Thoughts on that welcome.
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] ARM: shmobile: Factor out complex condition
2018-02-17 2:06 ` Marek Vasut
@ 2018-02-17 8:18 ` Wolfram Sang
-1 siblings, 0 replies; 18+ messages in thread
From: Wolfram Sang @ 2018-02-17 8:18 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-renesas-soc, linux-arm-kernel, Marek Vasut,
Geert Uytterhoeven, Kuninori Morimoto, Simon Horman,
Wolfram Sang
[-- Attachment #1: Type: text/plain, Size: 861 bytes --]
> - if ((client->addr == 0x58 && !strcmp(client->name, "da9063")) ||
> - (client->addr == 0x68 && !strcmp(client->name, "da9210")) ||
> - (client->addr == 0x70 && !strcmp(client->name, "da9210"))) {
> + if (regulator_quirk_check(client, 0, "da9063") ||
> + regulator_quirk_check(client, 1, "da9210") ||
> + regulator_quirk_check(client, 2, "da9210")) {
I am afraid I don't think this makes the code better, just different.
The index is as magic as the client address IMO. I was not super happy
with the array size depending on the detected board from a previous
patch already. But given the next patch which modifies the msg array
depending on the board, I think we really need to switch to seperate
message arrays per board. Everything else is too error prone and
unnecessarily cumbersome to understand.
Other opinions?
Regards,
Wolfram
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH 1/2] ARM: shmobile: Factor out complex condition
@ 2018-02-17 8:18 ` Wolfram Sang
0 siblings, 0 replies; 18+ messages in thread
From: Wolfram Sang @ 2018-02-17 8:18 UTC (permalink / raw)
To: linux-arm-kernel
> - if ((client->addr == 0x58 && !strcmp(client->name, "da9063")) ||
> - (client->addr == 0x68 && !strcmp(client->name, "da9210")) ||
> - (client->addr == 0x70 && !strcmp(client->name, "da9210"))) {
> + if (regulator_quirk_check(client, 0, "da9063") ||
> + regulator_quirk_check(client, 1, "da9210") ||
> + regulator_quirk_check(client, 2, "da9210")) {
I am afraid I don't think this makes the code better, just different.
The index is as magic as the client address IMO. I was not super happy
with the array size depending on the detected board from a previous
patch already. But given the next patch which modifies the msg array
depending on the board, I think we really need to switch to seperate
message arrays per board. Everything else is too error prone and
unnecessarily cumbersome to understand.
Other opinions?
Regards,
Wolfram
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180217/9a5d0088/attachment.sig>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 1/2] ARM: shmobile: Factor out complex condition
2018-02-17 8:18 ` Wolfram Sang
@ 2018-02-17 11:46 ` Marek Vasut
-1 siblings, 0 replies; 18+ messages in thread
From: Marek Vasut @ 2018-02-17 11:46 UTC (permalink / raw)
To: Wolfram Sang
Cc: linux-renesas-soc, linux-arm-kernel, Marek Vasut,
Geert Uytterhoeven, Kuninori Morimoto, Simon Horman,
Wolfram Sang
On 02/17/2018 09:18 AM, Wolfram Sang wrote:
>> - if ((client->addr == 0x58 && !strcmp(client->name, "da9063")) ||
>> - (client->addr == 0x68 && !strcmp(client->name, "da9210")) ||
>> - (client->addr == 0x70 && !strcmp(client->name, "da9210"))) {
>> + if (regulator_quirk_check(client, 0, "da9063") ||
>> + regulator_quirk_check(client, 1, "da9210") ||
>> + regulator_quirk_check(client, 2, "da9210")) {
>
> I am afraid I don't think this makes the code better, just different.
> The index is as magic as the client address IMO. I was not super happy
> with the array size depending on the detected board from a previous
> patch already. But given the next patch which modifies the msg array
> depending on the board, I think we really need to switch to seperate
> message arrays per board. Everything else is too error prone and
> unnecessarily cumbersome to understand.
>
> Other opinions?
The code is awful, yes.
I wonder if we could rather find all DA0063 and DA9210 PMICs in the DT,
check if their IRQ lines are tied together and then generate the table
above dynamically for whatever PMICs are present in the system. Then all
that special-casing would go away as we'd extract the information from DT.
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH 1/2] ARM: shmobile: Factor out complex condition
@ 2018-02-17 11:46 ` Marek Vasut
0 siblings, 0 replies; 18+ messages in thread
From: Marek Vasut @ 2018-02-17 11:46 UTC (permalink / raw)
To: linux-arm-kernel
On 02/17/2018 09:18 AM, Wolfram Sang wrote:
>> - if ((client->addr == 0x58 && !strcmp(client->name, "da9063")) ||
>> - (client->addr == 0x68 && !strcmp(client->name, "da9210")) ||
>> - (client->addr == 0x70 && !strcmp(client->name, "da9210"))) {
>> + if (regulator_quirk_check(client, 0, "da9063") ||
>> + regulator_quirk_check(client, 1, "da9210") ||
>> + regulator_quirk_check(client, 2, "da9210")) {
>
> I am afraid I don't think this makes the code better, just different.
> The index is as magic as the client address IMO. I was not super happy
> with the array size depending on the detected board from a previous
> patch already. But given the next patch which modifies the msg array
> depending on the board, I think we really need to switch to seperate
> message arrays per board. Everything else is too error prone and
> unnecessarily cumbersome to understand.
>
> Other opinions?
The code is awful, yes.
I wonder if we could rather find all DA0063 and DA9210 PMICs in the DT,
check if their IRQ lines are tied together and then generate the table
above dynamically for whatever PMICs are present in the system. Then all
that special-casing would go away as we'd extract the information from DT.
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 1/2] ARM: shmobile: Factor out complex condition
2018-02-17 11:46 ` Marek Vasut
@ 2018-02-19 8:53 ` Geert Uytterhoeven
-1 siblings, 0 replies; 18+ messages in thread
From: Geert Uytterhoeven @ 2018-02-19 8:53 UTC (permalink / raw)
To: Marek Vasut
Cc: Wolfram Sang, Linux-Renesas, linux-arm-kernel, Marek Vasut,
Geert Uytterhoeven, Kuninori Morimoto, Simon Horman,
Wolfram Sang
Hi Marek,
On Sat, Feb 17, 2018 at 12:46 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On 02/17/2018 09:18 AM, Wolfram Sang wrote:
>>> - if ((client->addr == 0x58 && !strcmp(client->name, "da9063")) ||
>>> - (client->addr == 0x68 && !strcmp(client->name, "da9210")) ||
>>> - (client->addr == 0x70 && !strcmp(client->name, "da9210"))) {
>>> + if (regulator_quirk_check(client, 0, "da9063") ||
>>> + regulator_quirk_check(client, 1, "da9210") ||
>>> + regulator_quirk_check(client, 2, "da9210")) {
>>
>> I am afraid I don't think this makes the code better, just different.
>> The index is as magic as the client address IMO. I was not super happy
>> with the array size depending on the detected board from a previous
>> patch already. But given the next patch which modifies the msg array
>> depending on the board, I think we really need to switch to seperate
>> message arrays per board. Everything else is too error prone and
>> unnecessarily cumbersome to understand.
>>
>> Other opinions?
>
> The code is awful, yes.
>
> I wonder if we could rather find all DA0063 and DA9210 PMICs in the DT,
> check if their IRQ lines are tied together and then generate the table
> above dynamically for whatever PMICs are present in the system. Then all
> that special-casing would go away as we'd extract the information from DT.
Yes, like I suggested 4 days ago ;-)
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
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH 1/2] ARM: shmobile: Factor out complex condition
@ 2018-02-19 8:53 ` Geert Uytterhoeven
0 siblings, 0 replies; 18+ messages in thread
From: Geert Uytterhoeven @ 2018-02-19 8:53 UTC (permalink / raw)
To: linux-arm-kernel
Hi Marek,
On Sat, Feb 17, 2018 at 12:46 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On 02/17/2018 09:18 AM, Wolfram Sang wrote:
>>> - if ((client->addr == 0x58 && !strcmp(client->name, "da9063")) ||
>>> - (client->addr == 0x68 && !strcmp(client->name, "da9210")) ||
>>> - (client->addr == 0x70 && !strcmp(client->name, "da9210"))) {
>>> + if (regulator_quirk_check(client, 0, "da9063") ||
>>> + regulator_quirk_check(client, 1, "da9210") ||
>>> + regulator_quirk_check(client, 2, "da9210")) {
>>
>> I am afraid I don't think this makes the code better, just different.
>> The index is as magic as the client address IMO. I was not super happy
>> with the array size depending on the detected board from a previous
>> patch already. But given the next patch which modifies the msg array
>> depending on the board, I think we really need to switch to seperate
>> message arrays per board. Everything else is too error prone and
>> unnecessarily cumbersome to understand.
>>
>> Other opinions?
>
> The code is awful, yes.
>
> I wonder if we could rather find all DA0063 and DA9210 PMICs in the DT,
> check if their IRQ lines are tied together and then generate the table
> above dynamically for whatever PMICs are present in the system. Then all
> that special-casing would go away as we'd extract the information from DT.
Yes, like I suggested 4 days ago ;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] ARM: shmobile: Factor out complex condition
2018-02-17 2:06 ` Marek Vasut
@ 2018-02-19 8:58 ` Simon Horman
-1 siblings, 0 replies; 18+ messages in thread
From: Simon Horman @ 2018-02-19 8:58 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-renesas-soc, linux-arm-kernel, Marek Vasut,
Geert Uytterhoeven, Kuninori Morimoto, Wolfram Sang
On Sat, Feb 17, 2018 at 03:06:41AM +0100, Marek Vasut wrote:
> Pull the complex condition in regulator_quirk_notify() into
> regulator_quirk_check(). Moreover, do not hard-code the I2C
> address, but rather use the one in da9xxx_msgs[].
>
> 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>
There appears to be some review of this series that needs addressing,
I have marked it as "Changes Requested".
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2] ARM: shmobile: Factor out complex condition
@ 2018-02-19 8:58 ` Simon Horman
0 siblings, 0 replies; 18+ messages in thread
From: Simon Horman @ 2018-02-19 8:58 UTC (permalink / raw)
To: linux-arm-kernel
On Sat, Feb 17, 2018 at 03:06:41AM +0100, Marek Vasut wrote:
> Pull the complex condition in regulator_quirk_notify() into
> regulator_quirk_check(). Moreover, do not hard-code the I2C
> address, but rather use the one in da9xxx_msgs[].
>
> 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>
There appears to be some review of this series that needs addressing,
I have marked it as "Changes Requested".
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] ARM: shmobile: Factor out complex condition
2018-02-19 8:58 ` Simon Horman
@ 2018-02-21 19:59 ` Marek Vasut
-1 siblings, 0 replies; 18+ messages in thread
From: Marek Vasut @ 2018-02-21 19:59 UTC (permalink / raw)
To: Simon Horman
Cc: linux-renesas-soc, linux-arm-kernel, Marek Vasut,
Geert Uytterhoeven, Kuninori Morimoto, Wolfram Sang
On 02/19/2018 09:58 AM, Simon Horman wrote:
> On Sat, Feb 17, 2018 at 03:06:41AM +0100, Marek Vasut wrote:
>> Pull the complex condition in regulator_quirk_notify() into
>> regulator_quirk_check(). Moreover, do not hard-code the I2C
>> address, but rather use the one in da9xxx_msgs[].
>>
>> 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>
>
> There appears to be some review of this series that needs addressing,
> I have marked it as "Changes Requested".
>
Jupp
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2] ARM: shmobile: Factor out complex condition
@ 2018-02-21 19:59 ` Marek Vasut
0 siblings, 0 replies; 18+ messages in thread
From: Marek Vasut @ 2018-02-21 19:59 UTC (permalink / raw)
To: linux-arm-kernel
On 02/19/2018 09:58 AM, Simon Horman wrote:
> On Sat, Feb 17, 2018 at 03:06:41AM +0100, Marek Vasut wrote:
>> Pull the complex condition in regulator_quirk_notify() into
>> regulator_quirk_check(). Moreover, do not hard-code the I2C
>> address, but rather use the one in da9xxx_msgs[].
>>
>> 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>
>
> There appears to be some review of this series that needs addressing,
> I have marked it as "Changes Requested".
>
Jupp
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2018-02-21 20:14 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-17 2:06 [PATCH 1/2] ARM: shmobile: Factor out complex condition Marek Vasut
2018-02-17 2:06 ` Marek Vasut
2018-02-17 2:06 ` [PATCH 2/2] ARM: shmobile: porter: enable R-Car Gen2 regulator quirk Marek Vasut
2018-02-17 2:06 ` Marek Vasut
2018-02-19 8:42 ` Geert Uytterhoeven
2018-02-19 8:42 ` Geert Uytterhoeven
2018-02-19 8:51 ` Marek Vasut
2018-02-19 8:51 ` Marek Vasut
2018-02-17 8:18 ` [PATCH 1/2] ARM: shmobile: Factor out complex condition Wolfram Sang
2018-02-17 8:18 ` Wolfram Sang
2018-02-17 11:46 ` Marek Vasut
2018-02-17 11:46 ` Marek Vasut
2018-02-19 8:53 ` Geert Uytterhoeven
2018-02-19 8:53 ` Geert Uytterhoeven
2018-02-19 8:58 ` Simon Horman
2018-02-19 8:58 ` Simon Horman
2018-02-21 19:59 ` Marek Vasut
2018-02-21 19:59 ` Marek Vasut
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.