| Message ID | 20180217020642.21375-1-marek.vasut+renesas@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Horman |
| Headers | show |
> - 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
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.
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
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".
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
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. */
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(-)