From: Khiem Nguyen <[email protected]>
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 <[email protected]>
[shimoda: rebase and refactor]
Signed-off-by: Yoshihiro Shimoda <[email protected]>
---
drivers/mfd/bd9571mwv.c | 47 ++++++++++++++++++++++++++++++++++---------
include/linux/mfd/bd9571mwv.h | 19 +++++++++++++++++
2 files changed, 57 insertions(+), 9 deletions(-)
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 <[email protected]>
+ * 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 <[email protected]>
+ * 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;
--
2.7.4
Hi Shimoda-san,
On Tue, Dec 8, 2020 at 9:06 AM Yoshihiro Shimoda
<[email protected]> wrote:
> From: Khiem Nguyen <[email protected]>
>
> 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 <[email protected]>
> [shimoda: rebase and refactor]
> Signed-off-by: Yoshihiro Shimoda <[email protected]>
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 -- [email protected]
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
> <[email protected]> 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
<[email protected]> wrote:
> > From: Geert Uytterhoeven, Sent: Wednesday, December 9, 2020 10:26 PM
> > On Tue, Dec 8, 2020 at 9:06 AM Yoshihiro Shimoda
> > <[email protected]> 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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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,
> 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