Re: [PATCH 1/2] ARM: shmobile: Factor out complex condition
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 >> Cc: Geert Uytterhoeven >> Cc: Kuninori Morimoto >> Cc: Simon Horman >> Cc: Wolfram Sang > > There appears to be some review of this series that needs addressing, > I have marked it as "Changes Requested". > Jupp -- Best regards, Marek Vasut
Re: [PATCH 1/2] ARM: shmobile: Factor out complex condition
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 > Cc: Geert Uytterhoeven > Cc: Kuninori Morimoto > Cc: Simon Horman > Cc: Wolfram Sang 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
Hi Marek,
On Sat, Feb 17, 2018 at 12:46 PM, Marek Vasut 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 -- ge...@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
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 1/2] ARM: shmobile: Factor out complex condition
> - 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
signature.asc
Description: PGP signature
