[PATCH 1/2] ARM: shmobile: Factor out complex condition

[PATCH 1/2] ARM: shmobile: Factor out complex condition

[Marek Vasut]

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

[PATCH 1/2] ARM: shmobile: Factor out complex condition

[Marek Vasut]

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

[PATCH 2/2] ARM: shmobile: porter: enable R-Car Gen2 regulator quirk

[Marek Vasut]

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

[PATCH 2/2] ARM: shmobile: porter: enable R-Car Gen2 regulator quirk

[Marek Vasut]

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

Re: [PATCH 1/2] ARM: shmobile: Factor out complex condition

[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 --]

[PATCH 1/2] ARM: shmobile: Factor out complex condition

[Wolfram Sang]

> -	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>

Re: [PATCH 1/2] ARM: shmobile: Factor out complex condition

[Marek Vasut]

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

[PATCH 1/2] ARM: shmobile: Factor out complex condition

[Marek Vasut]

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

Re: [PATCH 2/2] ARM: shmobile: porter: enable R-Car Gen2 regulator quirk

[Geert Uytterhoeven]

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

[PATCH 2/2] ARM: shmobile: porter: enable R-Car Gen2 regulator quirk

[Geert Uytterhoeven]

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

Re: [PATCH 2/2] ARM: shmobile: porter: enable R-Car Gen2 regulator quirk

[Marek Vasut]

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

[PATCH 2/2] ARM: shmobile: porter: enable R-Car Gen2 regulator quirk

[Marek Vasut]

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

Re: [PATCH 1/2] ARM: shmobile: Factor out complex condition

[Geert Uytterhoeven]

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

[PATCH 1/2] ARM: shmobile: Factor out complex condition

[Geert Uytterhoeven]

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

Re: [PATCH 1/2] ARM: shmobile: Factor out complex condition

[Simon Horman]

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".

[PATCH 1/2] ARM: shmobile: Factor out complex condition

[Simon Horman]

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".

Re: [PATCH 1/2] ARM: shmobile: Factor out complex condition

[Marek Vasut]

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

[PATCH 1/2] ARM: shmobile: Factor out complex condition

[Marek Vasut]

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