| Message ID | 1607414643-25498-3-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | mfd: bd9571mwv: Add support for BD9574MWF | expand |
Hi Shimoda-san, On Tue, Dec 8, 2020 at 9:06 AM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > From: Khiem Nguyen <khiem.nguyen.xt@renesas.com> > > Since the driver supports BD9571MWV PMIC only, > this patch makes the functions and data structure become more generic > so that it can support other PMIC variants as well. > > Signed-off-by: Khiem Nguyen <khiem.nguyen.xt@renesas.com> > [shimoda: rebase and refactor] > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> Thanks for your patch! > index 80c6ef0..57bdb6a 100644 > --- a/drivers/mfd/bd9571mwv.c > +++ b/drivers/mfd/bd9571mwv.c > @@ -127,13 +137,12 @@ static int bd9571mwv_identify(struct bd9571mwv *bd) > ret); > return ret; > } > - > - if (value != BD9571MWV_PRODUCT_CODE_VAL) { > + /* Confirm the product code */ > + if (value != bd->data->product_code_val) { > dev_err(dev, "Invalid product code ID %02x (expected %02x)\n", > - value, BD9571MWV_PRODUCT_CODE_VAL); > + value, bd->data->product_code_val); > return -EINVAL; > } Reading the product code register, and checking the product code value can be removed, as bd9571mwv_probe() has verified it already. > @@ -150,6 +160,7 @@ static int bd9571mwv_probe(struct i2c_client *client, > const struct i2c_device_id *ids) > { > struct bd9571mwv *bd; > + unsigned int product_code; No need for a new variable... > int ret; > > bd = devm_kzalloc(&client->dev, sizeof(*bd), GFP_KERNEL); > @@ -160,7 +171,25 @@ static int bd9571mwv_probe(struct i2c_client *client, > bd->dev = &client->dev; > bd->irq = client->irq; > > - bd->regmap = devm_regmap_init_i2c(client, &bd9571mwv_regmap_config); > + /* Read the PMIC product code */ > + ret = i2c_smbus_read_byte_data(client, BD9571MWV_PRODUCT_CODE); > + if (ret < 0) { > + dev_err(&client->dev, "failed reading at 0x%02x\n", > + BD9571MWV_PRODUCT_CODE); > + return ret; > + } > + > + product_code = (unsigned int)ret; > + if (product_code == BD9571MWV_PRODUCT_CODE_VAL) ... as you can just check if ret == BD9571MWV_PRODUCT_CODE_VAL here. > + bd->data = &bd9571mwv_data; > + > + if (!bd->data) { > + dev_err(bd->dev, "No found supported device %d\n", "Unsupported device 0x%x"? > + product_code); > + return -ENOENT; > + } The construct above may be easier to read and extend by using a switch() statement, with a default case for unsupported devices. > --- a/include/linux/mfd/bd9571mwv.h > +++ b/include/linux/mfd/bd9571mwv.h > @@ -83,6 +85,8 @@ > > #define BD9571MWV_ACCESS_KEY 0xff > > +#define BD9571MWV_PART_NUMBER "BD9571MWV" BD9571MWV_PART_NAME? > + > /* Define the BD9571MWV IRQ numbers */ > enum bd9571mwv_irqs { > BD9571MWV_IRQ_MD1, > @@ -96,6 +100,20 @@ enum bd9571mwv_irqs { > }; > > /** > + * struct bd957x_data - internal data for the bd957x driver > + * > + * Internal data to distinguish bd9571mwv chip and bd9574mwf chip ... distinguish bd957x variants? > + */ > +struct bd957x_data { > + int product_code_val; unsigned int? > + char *part_number; part_name? > + const struct regmap_config *regmap_config; > + const struct regmap_irq_chip *irq_chip; > + const struct mfd_cell *cells; > + int num_cells; I'd say "unsigned int", but mfd_add_devices() takes plain "int". Please put the "product_code_val" and "num_cells" fields next to each other, to avoid two 4-byte gaps on 64-bit platforms. 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
Hi Geert-san, Thank you for your review! > From: Geert Uytterhoeven, Sent: Wednesday, December 9, 2020 10:26 PM > > Hi Shimoda-san, > > On Tue, Dec 8, 2020 at 9:06 AM Yoshihiro Shimoda > <yoshihiro.shimoda.uh@renesas.com> wrote: <snip> > > index 80c6ef0..57bdb6a 100644 > > --- a/drivers/mfd/bd9571mwv.c > > +++ b/drivers/mfd/bd9571mwv.c > > > @@ -127,13 +137,12 @@ static int bd9571mwv_identify(struct bd9571mwv *bd) > > ret); > > return ret; > > } > > - > > - if (value != BD9571MWV_PRODUCT_CODE_VAL) { > > + /* Confirm the product code */ > > + if (value != bd->data->product_code_val) { > > dev_err(dev, "Invalid product code ID %02x (expected %02x)\n", > > - value, BD9571MWV_PRODUCT_CODE_VAL); > > + value, bd->data->product_code_val); > > return -EINVAL; > > } > > Reading the product code register, and checking the product code value > can be removed, as bd9571mwv_probe() has verified it already. Indeed. I'll remove this. > > @@ -150,6 +160,7 @@ static int bd9571mwv_probe(struct i2c_client *client, > > const struct i2c_device_id *ids) > > { > > struct bd9571mwv *bd; > > + unsigned int product_code; > > No need for a new variable... > > > int ret; > > > > bd = devm_kzalloc(&client->dev, sizeof(*bd), GFP_KERNEL); > > @@ -160,7 +171,25 @@ static int bd9571mwv_probe(struct i2c_client *client, > > bd->dev = &client->dev; > > bd->irq = client->irq; > > > > - bd->regmap = devm_regmap_init_i2c(client, &bd9571mwv_regmap_config); > > + /* Read the PMIC product code */ > > + ret = i2c_smbus_read_byte_data(client, BD9571MWV_PRODUCT_CODE); > > + if (ret < 0) { > > + dev_err(&client->dev, "failed reading at 0x%02x\n", > > + BD9571MWV_PRODUCT_CODE); > > + return ret; > > + } > > + > > + product_code = (unsigned int)ret; > > + if (product_code == BD9571MWV_PRODUCT_CODE_VAL) > > ... as you can just check if ret == BD9571MWV_PRODUCT_CODE_VAL here. I got it. > > + bd->data = &bd9571mwv_data; > > + > > + if (!bd->data) { > > + dev_err(bd->dev, "No found supported device %d\n", > > "Unsupported device 0x%x"? I'll fix it. > > + product_code); > > + return -ENOENT; > > + } > > The construct above may be easier to read and extend by using a switch() > statement, with a default case for unsupported devices. I think so. I'll fix it. > > --- a/include/linux/mfd/bd9571mwv.h > > +++ b/include/linux/mfd/bd9571mwv.h > > > @@ -83,6 +85,8 @@ > > > > #define BD9571MWV_ACCESS_KEY 0xff > > > > +#define BD9571MWV_PART_NUMBER "BD9571MWV" > > BD9571MWV_PART_NAME? I'll rename it. > > + > > /* Define the BD9571MWV IRQ numbers */ > > enum bd9571mwv_irqs { > > BD9571MWV_IRQ_MD1, > > @@ -96,6 +100,20 @@ enum bd9571mwv_irqs { > > }; > > > > /** > > + * struct bd957x_data - internal data for the bd957x driver > > + * > > + * Internal data to distinguish bd9571mwv chip and bd9574mwf chip > > ... distinguish bd957x variants? Thanks. I'll modify it. > > + */ > > +struct bd957x_data { > > + int product_code_val; > > unsigned int? We can remove this member. Or, keeping this member and then we check the product code by this member instead of switch() like below? /* No build test, JFYI */ struct bd957x_data *data_array[] = { &bd9571mwv_data, &bd9574mwf_data, }; for (i = 0; I < ARRAY_SIZE(data_array); i++) { if (val == data_array[i].product_code_val) { bd->data = data_array[i]; break; } } > > + char *part_number; > > part_name? Yes, I'll fix it. > > + const struct regmap_config *regmap_config; > > + const struct regmap_irq_chip *irq_chip; > > + const struct mfd_cell *cells; > > + int num_cells; > > I'd say "unsigned int", but mfd_add_devices() takes plain "int". > > Please put the "product_code_val" and "num_cells" fields next to > each other, to avoid two 4-byte gaps on 64-bit platforms. I'll fix it if we kept "product_code_val". Best regards, Yoshihiro Shimoda
Hi Shimoda-san, On Thu, Dec 10, 2020 at 5:10 AM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > > From: Geert Uytterhoeven, Sent: Wednesday, December 9, 2020 10:26 PM > > On Tue, Dec 8, 2020 at 9:06 AM Yoshihiro Shimoda > > <yoshihiro.shimoda.uh@renesas.com> wrote: > <snip> > > > index 80c6ef0..57bdb6a 100644 > > > --- a/drivers/mfd/bd9571mwv.c > > > +++ b/drivers/mfd/bd9571mwv.c > > > > > @@ -127,13 +137,12 @@ static int bd9571mwv_identify(struct bd9571mwv *bd) > > > ret); > > > return ret; > > > } > > > - > > > - if (value != BD9571MWV_PRODUCT_CODE_VAL) { > > > + /* Confirm the product code */ > > > + if (value != bd->data->product_code_val) { > > > dev_err(dev, "Invalid product code ID %02x (expected %02x)\n", > > > - value, BD9571MWV_PRODUCT_CODE_VAL); > > > + value, bd->data->product_code_val); > > > return -EINVAL; > > > } > > > > Reading the product code register, and checking the product code value > > can be removed, as bd9571mwv_probe() has verified it already. > > Indeed. I'll remove this. OK. > > > --- a/include/linux/mfd/bd9571mwv.h > > > +++ b/include/linux/mfd/bd9571mwv.h > > > + */ > > > +struct bd957x_data { > > > + int product_code_val; > > > > unsigned int? > > We can remove this member. True. > Or, keeping this member and then we check the product code by this member > instead of switch() like below? > > /* No build test, JFYI */ > struct bd957x_data *data_array[] = { > &bd9571mwv_data, > &bd9574mwf_data, > }; > > for (i = 0; I < ARRAY_SIZE(data_array); i++) { > if (val == data_array[i].product_code_val) { > bd->data = data_array[i]; > break; > } > } Given we probably won't have more than a handful variants, I'm leaning towards the switch() approach. Gr{oetje,eeting}s, Geert
Hi Geert-san, > From: Geert Uytterhoeven, Sent: Thursday, December 10, 2020 5:23 PM <snip> > > Or, keeping this member and then we check the product code by this member > > instead of switch() like below? > > > > /* No build test, JFYI */ > > struct bd957x_data *data_array[] = { > > &bd9571mwv_data, > > &bd9574mwf_data, > > }; > > > > for (i = 0; I < ARRAY_SIZE(data_array); i++) { > > if (val == data_array[i].product_code_val) { > > bd->data = data_array[i]; > > break; > > } > > } > > Given we probably won't have more than a handful variants, I'm > leaning towards the switch() approach. OK. I got it. I'll use switch(). Best regards, Yoshihiro Shimoda
diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c index 80c6ef0..57bdb6a 100644 --- a/drivers/mfd/bd9571mwv.c +++ b/drivers/mfd/bd9571mwv.c @@ -3,6 +3,7 @@ * ROHM BD9571MWV-M MFD driver * * Copyright (C) 2017 Marek Vasut <marek.vasut+renesas@gmail.com> + * Copyright (C) 2020 Renesas Electronics Corporation * * Based on the TPS65086 driver */ @@ -102,6 +103,15 @@ static struct regmap_irq_chip bd9571mwv_irq_chip = { .num_irqs = ARRAY_SIZE(bd9571mwv_irqs), }; +static const struct bd957x_data bd9571mwv_data = { + .product_code_val = BD9571MWV_PRODUCT_CODE_VAL, + .part_number = BD9571MWV_PART_NUMBER, + .regmap_config = &bd9571mwv_regmap_config, + .irq_chip = &bd9571mwv_irq_chip, + .cells = bd9571mwv_cells, + .num_cells = ARRAY_SIZE(bd9571mwv_cells), +}; + static int bd9571mwv_identify(struct bd9571mwv *bd) { struct device *dev = bd->dev; @@ -127,13 +137,12 @@ static int bd9571mwv_identify(struct bd9571mwv *bd) ret); return ret; } - - if (value != BD9571MWV_PRODUCT_CODE_VAL) { + /* Confirm the product code */ + if (value != bd->data->product_code_val) { dev_err(dev, "Invalid product code ID %02x (expected %02x)\n", - value, BD9571MWV_PRODUCT_CODE_VAL); + value, bd->data->product_code_val); return -EINVAL; } - ret = regmap_read(bd->regmap, BD9571MWV_PRODUCT_REVISION, &value); if (ret) { dev_err(dev, "Failed to read revision register (ret=%i)\n", @@ -141,7 +150,8 @@ static int bd9571mwv_identify(struct bd9571mwv *bd) return ret; } - dev_info(dev, "Device: BD9571MWV rev. %d\n", value & 0xff); + dev_info(dev, "Device: %s rev. %d\n", bd->data->part_number, + value & 0xff); return 0; } @@ -150,6 +160,7 @@ static int bd9571mwv_probe(struct i2c_client *client, const struct i2c_device_id *ids) { struct bd9571mwv *bd; + unsigned int product_code; int ret; bd = devm_kzalloc(&client->dev, sizeof(*bd), GFP_KERNEL); @@ -160,7 +171,25 @@ static int bd9571mwv_probe(struct i2c_client *client, bd->dev = &client->dev; bd->irq = client->irq; - bd->regmap = devm_regmap_init_i2c(client, &bd9571mwv_regmap_config); + /* Read the PMIC product code */ + ret = i2c_smbus_read_byte_data(client, BD9571MWV_PRODUCT_CODE); + if (ret < 0) { + dev_err(&client->dev, "failed reading at 0x%02x\n", + BD9571MWV_PRODUCT_CODE); + return ret; + } + + product_code = (unsigned int)ret; + if (product_code == BD9571MWV_PRODUCT_CODE_VAL) + bd->data = &bd9571mwv_data; + + if (!bd->data) { + dev_err(bd->dev, "No found supported device %d\n", + product_code); + return -ENOENT; + } + + bd->regmap = devm_regmap_init_i2c(client, bd->data->regmap_config); if (IS_ERR(bd->regmap)) { dev_err(bd->dev, "Failed to initialize register map\n"); return PTR_ERR(bd->regmap); @@ -171,14 +200,14 @@ static int bd9571mwv_probe(struct i2c_client *client, return ret; ret = regmap_add_irq_chip(bd->regmap, bd->irq, IRQF_ONESHOT, 0, - &bd9571mwv_irq_chip, &bd->irq_data); + bd->data->irq_chip, &bd->irq_data); if (ret) { dev_err(bd->dev, "Failed to register IRQ chip\n"); return ret; } - ret = mfd_add_devices(bd->dev, PLATFORM_DEVID_AUTO, bd9571mwv_cells, - ARRAY_SIZE(bd9571mwv_cells), NULL, 0, + ret = mfd_add_devices(bd->dev, PLATFORM_DEVID_AUTO, bd->data->cells, + bd->data->num_cells, NULL, 0, regmap_irq_get_domain(bd->irq_data)); if (ret) { regmap_del_irq_chip(bd->irq, bd->irq_data); diff --git a/include/linux/mfd/bd9571mwv.h b/include/linux/mfd/bd9571mwv.h index bcc7092..0126b52 100644 --- a/include/linux/mfd/bd9571mwv.h +++ b/include/linux/mfd/bd9571mwv.h @@ -3,6 +3,7 @@ * ROHM BD9571MWV-M driver * * Copyright (C) 2017 Marek Vasut <marek.vasut+renesas@gmail.com> + * Copyright (C) 2019 Renesas Electronics Corporation * * Based on the TPS65086 driver */ @@ -11,6 +12,7 @@ #define __LINUX_MFD_BD9571MWV_H #include <linux/device.h> +#include <linux/mfd/core.h> #include <linux/regmap.h> /* List of registers for BD9571MWV */ @@ -83,6 +85,8 @@ #define BD9571MWV_ACCESS_KEY 0xff +#define BD9571MWV_PART_NUMBER "BD9571MWV" + /* Define the BD9571MWV IRQ numbers */ enum bd9571mwv_irqs { BD9571MWV_IRQ_MD1, @@ -96,6 +100,20 @@ enum bd9571mwv_irqs { }; /** + * struct bd957x_data - internal data for the bd957x driver + * + * Internal data to distinguish bd9571mwv chip and bd9574mwf chip + */ +struct bd957x_data { + int product_code_val; + char *part_number; + const struct regmap_config *regmap_config; + const struct regmap_irq_chip *irq_chip; + const struct mfd_cell *cells; + int num_cells; +}; + +/** * struct bd9571mwv - state holder for the bd9571mwv driver * * Device data may be used to access the BD9571MWV chip @@ -103,6 +121,7 @@ enum bd9571mwv_irqs { struct bd9571mwv { struct device *dev; struct regmap *regmap; + const struct bd957x_data *data; /* IRQ Data */ int irq;