RE: [PATCH v3 11/12] mfd: bd9571mwv: Make the driver more generic
Hi Lee,
Thank you for your review!
> From: Lee Jones, Sent: Thursday, December 17, 2020 12:35 AM
>
> On Wed, 16 Dec 2020, Yoshihiro Shimoda wrote:
>
> > From: Khiem Nguyen
> >
> > 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
> > [shimoda: rebase and refactor]
>
> This is kind of expected. Please just add Co-developed-by instead.
I got it.
> > Signed-off-by: Yoshihiro Shimoda
> > ---
> > drivers/mfd/bd9571mwv.c | 95
> > +++
> > include/linux/mfd/bd9571mwv.h | 18 ++--
> > 2 files changed, 63 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c
> > index 49e968e..ccf1a60 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
> > + * Copyright (C) 2020 Renesas Electronics Corporation
> > *
> > * Based on the TPS65086 driver
> > */
> > @@ -14,6 +15,19 @@
> >
> > #include
> >
> > +/**
>
> This is wrong. Please do not abuse kernel-doc formatting.
Oops. I'll use just "/*" here.
> > + * struct bd957x_data - internal data for the bd957x driverbd957x_data
> > + *
> > + * Internal data to distinguish bd957x variants
> > + */
> > +struct bd957x_data {
>
> Call this bd957x_ddata please.
>
> ddata == driver data.
I got it.
> > + char *part_name;
>
> What is this used for besides a print? Those kinds of log messages
> are usually frowned upon anyway. Probably best to just remove the
> print, along with the variable.
I got it. I'll remove the print.
> > + const struct regmap_config *regmap_config;
> > + const struct regmap_irq_chip *irq_chip;
> > + const struct mfd_cell *cells;
> > + int num_cells;
> > +};
> > +
> > static const struct mfd_cell bd9571mwv_cells[] = {
> > { .name = "bd9571mwv-regulator", },
> > { .name = "bd9571mwv-gpio", },
> > @@ -102,13 +116,21 @@ static struct regmap_irq_chip bd9571mwv_irq_chip = {
> > .num_irqs = ARRAY_SIZE(bd9571mwv_irqs),
> > };
> >
> > -static int bd9571mwv_identify(struct bd9571mwv *bd)
> > +static const struct bd957x_data bd9571mwv_data = {
> > + .part_name = BD9571MWV_PART_NAME,
> > + .regmap_config = &bd9571mwv_regmap_config,
> > + .irq_chip = &bd9571mwv_irq_chip,
> > + .cells = bd9571mwv_cells,
> > + .num_cells = ARRAY_SIZE(bd9571mwv_cells),
> > +};
> > +
> > +static int bd9571mwv_identify(struct device *dev, struct regmap *regmap,
>
> I guess this function name also needs to change?
>
> And all other occurences of bd9571mwv?
Hmm, "bd957x" prefix is already used on a regulator driver (bd9576-regulator.c)
so that I'm thinking keep "bd9571mwv" is better to avoid confusing.
But, this is not a strong opinion so that if you prefer "bd957x" here,
I'll rename.
> > + const char *part_name)
> > {
> > - struct device *dev = bd->dev;
> > unsigned int value;
> > int ret;
> >
> > - ret = regmap_read(bd->regmap, BD9571MWV_VENDOR_CODE, &value);
> > + ret = regmap_read(regmap, BD9571MWV_VENDOR_CODE, &value);
> > if (ret) {
> > dev_err(dev, "Failed to read vendor code register (ret=%i)\n",
> > ret);
> > @@ -121,27 +143,20 @@ static int bd9571mwv_identify(struct bd9571mwv *bd)
> > return -EINVAL;
> > }
> >
> > - ret = regmap_read(bd->regmap, BD9571MWV_PRODUCT_CODE, &value);
> > + ret = regmap_read(regmap, BD9571MWV_PRODUCT_CODE, &value);
> > if (ret) {
> > dev_err(dev, "Failed to read product code register (ret=%i)\n",
> > ret);
> > return ret;
> > }
> > -
> > - if (value != BD9571MWV_PRODUCT_CODE_VAL) {
> > - dev_err(dev, "Invalid product code ID %02x (expected %02x)\n",
> > - value, BD9571MWV_PRODUCT_CODE_VAL);
> > - return -EINVAL;
> > - }
> > -
> > - ret = regmap_read(bd->regmap, BD9571MWV_PRODUCT_REVISION, &value);
> > + ret = regmap_read(regmap, BD9571MWV_PRODUCT_REVISION, &value);
> > if (ret) {
> > dev_err(dev, "Failed to read revision register (ret=%i)\n",
> > ret);
> > return ret;
> > }
> >
> > - dev_info(dev, "Device: BD9571MWV rev. %d\n", value & 0xff);
> > + dev_info(dev, "Device: %s rev. %d\n", part_name, value & 0xff);
> >
> > return 0;
> > }
> > @@ -149,38 +164,48 @@ static int bd9571mwv_identify(struct bd9571mwv *bd)
> > static int bd9571mwv_probe(struct i2c_client *client,
> > const struct i2c_device_id *ids)
> > {
> > - struct bd9571mwv *bd;
> > - int ret;
> > -
> > - bd = devm_kzalloc(&client->dev, sizeof(*bd), GFP_KERNEL);
> > - if (!bd)
> > - return -ENOMEM;
> > -
> > - i2c_set_clientdata(client, bd);
> > - bd->dev = &
Re: [PATCH v3 11/12] mfd: bd9571mwv: Make the driver more generic
On Wed, 16 Dec 2020, Yoshihiro Shimoda wrote:
> From: Khiem Nguyen
>
> 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
> [shimoda: rebase and refactor]
This is kind of expected. Please just add Co-developed-by instead.
> Signed-off-by: Yoshihiro Shimoda
> ---
> drivers/mfd/bd9571mwv.c | 95
> +++
> include/linux/mfd/bd9571mwv.h | 18 ++--
> 2 files changed, 63 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c
> index 49e968e..ccf1a60 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
> + * Copyright (C) 2020 Renesas Electronics Corporation
> *
> * Based on the TPS65086 driver
> */
> @@ -14,6 +15,19 @@
>
> #include
>
> +/**
This is wrong. Please do not abuse kernel-doc formatting.
> + * struct bd957x_data - internal data for the bd957x driverbd957x_data
> + *
> + * Internal data to distinguish bd957x variants
> + */
> +struct bd957x_data {
Call this bd957x_ddata please.
ddata == driver data.
> + char *part_name;
What is this used for besides a print? Those kinds of log messages
are usually frowned upon anyway. Probably best to just remove the
print, along with the variable.
> + const struct regmap_config *regmap_config;
> + const struct regmap_irq_chip *irq_chip;
> + const struct mfd_cell *cells;
> + int num_cells;
> +};
> +
> static const struct mfd_cell bd9571mwv_cells[] = {
> { .name = "bd9571mwv-regulator", },
> { .name = "bd9571mwv-gpio", },
> @@ -102,13 +116,21 @@ static struct regmap_irq_chip bd9571mwv_irq_chip = {
> .num_irqs = ARRAY_SIZE(bd9571mwv_irqs),
> };
>
> -static int bd9571mwv_identify(struct bd9571mwv *bd)
> +static const struct bd957x_data bd9571mwv_data = {
> + .part_name = BD9571MWV_PART_NAME,
> + .regmap_config = &bd9571mwv_regmap_config,
> + .irq_chip = &bd9571mwv_irq_chip,
> + .cells = bd9571mwv_cells,
> + .num_cells = ARRAY_SIZE(bd9571mwv_cells),
> +};
> +
> +static int bd9571mwv_identify(struct device *dev, struct regmap *regmap,
I guess this function name also needs to change?
And all other occurences of bd9571mwv?
> + const char *part_name)
> {
> - struct device *dev = bd->dev;
> unsigned int value;
> int ret;
>
> - ret = regmap_read(bd->regmap, BD9571MWV_VENDOR_CODE, &value);
> + ret = regmap_read(regmap, BD9571MWV_VENDOR_CODE, &value);
> if (ret) {
> dev_err(dev, "Failed to read vendor code register (ret=%i)\n",
> ret);
> @@ -121,27 +143,20 @@ static int bd9571mwv_identify(struct bd9571mwv *bd)
> return -EINVAL;
> }
>
> - ret = regmap_read(bd->regmap, BD9571MWV_PRODUCT_CODE, &value);
> + ret = regmap_read(regmap, BD9571MWV_PRODUCT_CODE, &value);
> if (ret) {
> dev_err(dev, "Failed to read product code register (ret=%i)\n",
> ret);
> return ret;
> }
> -
> - if (value != BD9571MWV_PRODUCT_CODE_VAL) {
> - dev_err(dev, "Invalid product code ID %02x (expected %02x)\n",
> - value, BD9571MWV_PRODUCT_CODE_VAL);
> - return -EINVAL;
> - }
> -
> - ret = regmap_read(bd->regmap, BD9571MWV_PRODUCT_REVISION, &value);
> + ret = regmap_read(regmap, BD9571MWV_PRODUCT_REVISION, &value);
> if (ret) {
> dev_err(dev, "Failed to read revision register (ret=%i)\n",
> ret);
> return ret;
> }
>
> - dev_info(dev, "Device: BD9571MWV rev. %d\n", value & 0xff);
> + dev_info(dev, "Device: %s rev. %d\n", part_name, value & 0xff);
>
> return 0;
> }
> @@ -149,38 +164,48 @@ static int bd9571mwv_identify(struct bd9571mwv *bd)
> static int bd9571mwv_probe(struct i2c_client *client,
> const struct i2c_device_id *ids)
> {
> - struct bd9571mwv *bd;
> - int ret;
> -
> - bd = devm_kzalloc(&client->dev, sizeof(*bd), GFP_KERNEL);
> - if (!bd)
> - return -ENOMEM;
> -
> - i2c_set_clientdata(client, bd);
> - bd->dev = &client->dev;
> - bd->irq = client->irq;
> + const struct bd957x_data *data;
ddata
> + struct device *dev = &client->dev;
> + struct regmap *regmap;
> + struct regmap_irq_chip_data *irq_data;
> + int ret, irq = client->irq;
> +
> + /* Read the PMIC product code */
> + ret = i2c_smbus_read_byte_data(client, BD9571MWV_PRODUCT_CODE);
> + if (ret < 0) {
> + dev_err(dev, "failed reading at 0x%02x\n",
> + BD9571MWV_PRODUCT_CODE);
"Failed to read product code" is more user friendly.
> +
Re: [PATCH v3 11/12] mfd: bd9571mwv: Make the driver more generic
On Wed, 2020-12-16 at 09:00 +, Lee Jones wrote: > On Wed, 16 Dec 2020, Vaittinen, Matti wrote: > > > On Wed, 2020-12-16 at 16:37 +0900, Yoshihiro Shimoda wrote: > > > From: Khiem Nguyen > > > > > > 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 > > > [shimoda: rebase and refactor] > > > Signed-off-by: Yoshihiro Shimoda < > > > yoshihiro.shimoda...@renesas.com> > > > > Reviewed-by: Matti Vaittinen > > Please place any *-by tags *after* the other comments. > > Fortunately, the first one below was still on my screen, else I would > have stopped reading here. > Good point. I'd better keep that on mind. Although missing stuff after a Reviewed-by is not that fatal ;) > > > --- > > > drivers/mfd/bd9571mwv.c | 95 +++ > > > > > > > > > include/linux/mfd/bd9571mwv.h | 18 ++-- > > > 2 files changed, 63 insertions(+), 50 deletions(-)
Re: [PATCH v3 11/12] mfd: bd9571mwv: Make the driver more generic
On Wed, 16 Dec 2020, Vaittinen, Matti wrote: > > On Wed, 2020-12-16 at 16:37 +0900, Yoshihiro Shimoda wrote: > > From: Khiem Nguyen > > > > 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 > > [shimoda: rebase and refactor] > > Signed-off-by: Yoshihiro Shimoda > > Reviewed-by: Matti Vaittinen Please place any *-by tags *after* the other comments. Fortunately, the first one below was still on my screen, else I would have stopped reading here. > > --- > > drivers/mfd/bd9571mwv.c | 95 +++ > > > > include/linux/mfd/bd9571mwv.h | 18 ++-- > > 2 files changed, 63 insertions(+), 50 deletions(-) -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH v3 11/12] mfd: bd9571mwv: Make the driver more generic
On Wed, 2020-12-16 at 10:25 +0200, Matti Vaittinen wrote:
> On Wed, 2020-12-16 at 16:37 +0900, Yoshihiro Shimoda wrote:
> > From: Khiem Nguyen
> >
> > 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
> > [shimoda: rebase and refactor]
> > Signed-off-by: Yoshihiro Shimoda
>
> Reviewed-by: Matti Vaittinen
>
> > ---
> > drivers/mfd/bd9571mwv.c | 95 +++
> >
> > include/linux/mfd/bd9571mwv.h | 18 ++--
> > 2 files changed, 63 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c
> > index 49e968e..ccf1a60 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
> > + * Copyright (C) 2020 Renesas Electronics Corporation
> > *
> > * Based on the TPS65086 driver
> > */
> > @@ -14,6 +15,19 @@
> >
> > #include
> >
> > +/**
> > + * struct bd957x_data - internal data for the bd957x driver
> > + *
> > + * Internal data to distinguish bd957x variants
> > + */
> > +struct bd957x_data {
> > + char *part_name;
> > + const struct regmap_config *regmap_config;
> > + const struct regmap_irq_chip *irq_chip;
> > + const struct mfd_cell *cells;
> > + int num_cells;
> > +};
> > +
>
> I do like the way you placed the variant data in owns structs. Well
> thought.
>
> > static const struct mfd_cell bd9571mwv_cells[] = {
> > { .name = "bd9571mwv-regulator", },
> > { .name = "bd9571mwv-gpio", },
> > @@ -102,13 +116,21 @@ static struct regmap_irq_chip
> > bd9571mwv_irq_chip = {
> > .num_irqs = ARRAY_SIZE(bd9571mwv_irqs),
> > };
> >
> > -static int bd9571mwv_identify(struct bd9571mwv *bd)
> > +static const struct bd957x_data bd9571mwv_data = {
> > + .part_name = BD9571MWV_PART_NAME,
> > + .regmap_config = &bd9571mwv_regmap_config,
> > + .irq_chip = &bd9571mwv_irq_chip,
> > + .cells = bd9571mwv_cells,
> > + .num_cells = ARRAY_SIZE(bd9571mwv_cells),
> > +};
> > +
> > +static int bd9571mwv_identify(struct device *dev, struct regmap
> > *regmap,
> > + const char *part_name)
> > {
> > - struct device *dev = bd->dev;
> > unsigned int value;
> > int ret;
> >
> > - ret = regmap_read(bd->regmap, BD9571MWV_VENDOR_CODE, &value);
> > + ret = regmap_read(regmap, BD9571MWV_VENDOR_CODE, &value);
> > if (ret) {
> > dev_err(dev, "Failed to read vendor code register
> > (ret=%i)\n",
> > ret);
> > @@ -121,27 +143,20 @@ static int bd9571mwv_identify(struct
> > bd9571mwv
> > *bd)
> > return -EINVAL;
> > }
> >
> > - ret = regmap_read(bd->regmap, BD9571MWV_PRODUCT_CODE, &value);
> > + ret = regmap_read(regmap, BD9571MWV_PRODUCT_CODE, &value);
> > if (ret) {
> > dev_err(dev, "Failed to read product code register
> > (ret=%i)\n",
> > ret);
> > return ret;
> > }
> > -
> > - if (value != BD9571MWV_PRODUCT_CODE_VAL) {
> > - dev_err(dev, "Invalid product code ID %02x (expected
> > %02x)\n",
> > - value, BD9571MWV_PRODUCT_CODE_VAL);
> > - return -EINVAL;
> > - }
> > -
> > - ret = regmap_read(bd->regmap, BD9571MWV_PRODUCT_REVISION,
> > &value);
> > + ret = regmap_read(regmap, BD9571MWV_PRODUCT_REVISION, &value);
> > if (ret) {
> > dev_err(dev, "Failed to read revision register
> > (ret=%i)\n",
> > ret);
> > return ret;
> > }
> >
> > - dev_info(dev, "Device: BD9571MWV rev. %d\n", value & 0xff);
> > + dev_info(dev, "Device: %s rev. %d\n", part_name, value & 0xff);
> >
> > return 0;
> > }
> > @@ -149,38 +164,48 @@ static int bd9571mwv_identify(struct
> > bd9571mwv
> > *bd)
> > static int bd9571mwv_probe(struct i2c_client *client,
> > const struct i2c_device_id *ids)
> > {
> > - struct bd9571mwv *bd;
> > - int ret;
> > -
> > - bd = devm_kzalloc(&client->dev, sizeof(*bd), GFP_KERNEL);
> > - if (!bd)
> > - return -ENOMEM;
> > -
> > - i2c_set_clientdata(client, bd);
> > - bd->dev = &client->dev;
> > - bd->irq = client->irq;
> > + const struct bd957x_data *data;
> > + struct device *dev = &client->dev;
> > + struct regmap *regmap;
> > + struct regmap_irq_chip_data *irq_data;
> > + int ret, irq = client->irq;
> > +
> > + /* Read the PMIC product code */
> > + ret = i2c_smbus_read_byte_data(client, BD9571MWV_PRODUCT_CODE);
>
> Having to use the i2c_smbus_read_byte_data for a device which is
> going
> to be used with regmap slightly bugs me. But as you want to do the
> runtime probing, then this access must be done prior regmap
> registration - so I can't think of a better way. :(
I just noticed that reading t
Re: [PATCH v3 11/12] mfd: bd9571mwv: Make the driver more generic
On Wed, 2020-12-16 at 16:37 +0900, Yoshihiro Shimoda wrote:
> From: Khiem Nguyen
>
> 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
> [shimoda: rebase and refactor]
> Signed-off-by: Yoshihiro Shimoda
Reviewed-by: Matti Vaittinen
> ---
> drivers/mfd/bd9571mwv.c | 95 +++
>
> include/linux/mfd/bd9571mwv.h | 18 ++--
> 2 files changed, 63 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c
> index 49e968e..ccf1a60 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
> + * Copyright (C) 2020 Renesas Electronics Corporation
> *
> * Based on the TPS65086 driver
> */
> @@ -14,6 +15,19 @@
>
> #include
>
> +/**
> + * struct bd957x_data - internal data for the bd957x driver
> + *
> + * Internal data to distinguish bd957x variants
> + */
> +struct bd957x_data {
> + char *part_name;
> + const struct regmap_config *regmap_config;
> + const struct regmap_irq_chip *irq_chip;
> + const struct mfd_cell *cells;
> + int num_cells;
> +};
> +
I do like the way you placed the variant data in owns structs. Well
thought.
> static const struct mfd_cell bd9571mwv_cells[] = {
> { .name = "bd9571mwv-regulator", },
> { .name = "bd9571mwv-gpio", },
> @@ -102,13 +116,21 @@ static struct regmap_irq_chip
> bd9571mwv_irq_chip = {
> .num_irqs = ARRAY_SIZE(bd9571mwv_irqs),
> };
>
> -static int bd9571mwv_identify(struct bd9571mwv *bd)
> +static const struct bd957x_data bd9571mwv_data = {
> + .part_name = BD9571MWV_PART_NAME,
> + .regmap_config = &bd9571mwv_regmap_config,
> + .irq_chip = &bd9571mwv_irq_chip,
> + .cells = bd9571mwv_cells,
> + .num_cells = ARRAY_SIZE(bd9571mwv_cells),
> +};
> +
> +static int bd9571mwv_identify(struct device *dev, struct regmap
> *regmap,
> + const char *part_name)
> {
> - struct device *dev = bd->dev;
> unsigned int value;
> int ret;
>
> - ret = regmap_read(bd->regmap, BD9571MWV_VENDOR_CODE, &value);
> + ret = regmap_read(regmap, BD9571MWV_VENDOR_CODE, &value);
> if (ret) {
> dev_err(dev, "Failed to read vendor code register
> (ret=%i)\n",
> ret);
> @@ -121,27 +143,20 @@ static int bd9571mwv_identify(struct bd9571mwv
> *bd)
> return -EINVAL;
> }
>
> - ret = regmap_read(bd->regmap, BD9571MWV_PRODUCT_CODE, &value);
> + ret = regmap_read(regmap, BD9571MWV_PRODUCT_CODE, &value);
> if (ret) {
> dev_err(dev, "Failed to read product code register
> (ret=%i)\n",
> ret);
> return ret;
> }
> -
> - if (value != BD9571MWV_PRODUCT_CODE_VAL) {
> - dev_err(dev, "Invalid product code ID %02x (expected
> %02x)\n",
> - value, BD9571MWV_PRODUCT_CODE_VAL);
> - return -EINVAL;
> - }
> -
> - ret = regmap_read(bd->regmap, BD9571MWV_PRODUCT_REVISION,
> &value);
> + ret = regmap_read(regmap, BD9571MWV_PRODUCT_REVISION, &value);
> if (ret) {
> dev_err(dev, "Failed to read revision register
> (ret=%i)\n",
> ret);
> return ret;
> }
>
> - dev_info(dev, "Device: BD9571MWV rev. %d\n", value & 0xff);
> + dev_info(dev, "Device: %s rev. %d\n", part_name, value & 0xff);
>
> return 0;
> }
> @@ -149,38 +164,48 @@ static int bd9571mwv_identify(struct bd9571mwv
> *bd)
> static int bd9571mwv_probe(struct i2c_client *client,
> const struct i2c_device_id *ids)
> {
> - struct bd9571mwv *bd;
> - int ret;
> -
> - bd = devm_kzalloc(&client->dev, sizeof(*bd), GFP_KERNEL);
> - if (!bd)
> - return -ENOMEM;
> -
> - i2c_set_clientdata(client, bd);
> - bd->dev = &client->dev;
> - bd->irq = client->irq;
> + const struct bd957x_data *data;
> + struct device *dev = &client->dev;
> + struct regmap *regmap;
> + struct regmap_irq_chip_data *irq_data;
> + int ret, irq = client->irq;
> +
> + /* Read the PMIC product code */
> + ret = i2c_smbus_read_byte_data(client, BD9571MWV_PRODUCT_CODE);
Having to use the i2c_smbus_read_byte_data for a device which is going
to be used with regmap slightly bugs me. But as you want to do the
runtime probing, then this access must be done prior regmap
registration - so I can't think of a better way. :(
> + if (ret < 0) {
> + dev_err(dev, "failed reading at 0x%02x\n",
> + BD9571MWV_PRODUCT_CODE);
> + return ret;
> + }
> + switch (ret) {
> + case BD9571MWV_PRODUCT_CODE_VAL:
> + data = &bd9571mwv_data
[PATCH v3 11/12] mfd: bd9571mwv: Make the driver more generic
From: Khiem Nguyen
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
[shimoda: rebase and refactor]
Signed-off-by: Yoshihiro Shimoda
---
drivers/mfd/bd9571mwv.c | 95 +++
include/linux/mfd/bd9571mwv.h | 18 ++--
2 files changed, 63 insertions(+), 50 deletions(-)
diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c
index 49e968e..ccf1a60 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
+ * Copyright (C) 2020 Renesas Electronics Corporation
*
* Based on the TPS65086 driver
*/
@@ -14,6 +15,19 @@
#include
+/**
+ * struct bd957x_data - internal data for the bd957x driver
+ *
+ * Internal data to distinguish bd957x variants
+ */
+struct bd957x_data {
+ char *part_name;
+ const struct regmap_config *regmap_config;
+ const struct regmap_irq_chip *irq_chip;
+ const struct mfd_cell *cells;
+ int num_cells;
+};
+
static const struct mfd_cell bd9571mwv_cells[] = {
{ .name = "bd9571mwv-regulator", },
{ .name = "bd9571mwv-gpio", },
@@ -102,13 +116,21 @@ static struct regmap_irq_chip bd9571mwv_irq_chip = {
.num_irqs = ARRAY_SIZE(bd9571mwv_irqs),
};
-static int bd9571mwv_identify(struct bd9571mwv *bd)
+static const struct bd957x_data bd9571mwv_data = {
+ .part_name = BD9571MWV_PART_NAME,
+ .regmap_config = &bd9571mwv_regmap_config,
+ .irq_chip = &bd9571mwv_irq_chip,
+ .cells = bd9571mwv_cells,
+ .num_cells = ARRAY_SIZE(bd9571mwv_cells),
+};
+
+static int bd9571mwv_identify(struct device *dev, struct regmap *regmap,
+ const char *part_name)
{
- struct device *dev = bd->dev;
unsigned int value;
int ret;
- ret = regmap_read(bd->regmap, BD9571MWV_VENDOR_CODE, &value);
+ ret = regmap_read(regmap, BD9571MWV_VENDOR_CODE, &value);
if (ret) {
dev_err(dev, "Failed to read vendor code register (ret=%i)\n",
ret);
@@ -121,27 +143,20 @@ static int bd9571mwv_identify(struct bd9571mwv *bd)
return -EINVAL;
}
- ret = regmap_read(bd->regmap, BD9571MWV_PRODUCT_CODE, &value);
+ ret = regmap_read(regmap, BD9571MWV_PRODUCT_CODE, &value);
if (ret) {
dev_err(dev, "Failed to read product code register (ret=%i)\n",
ret);
return ret;
}
-
- if (value != BD9571MWV_PRODUCT_CODE_VAL) {
- dev_err(dev, "Invalid product code ID %02x (expected %02x)\n",
- value, BD9571MWV_PRODUCT_CODE_VAL);
- return -EINVAL;
- }
-
- ret = regmap_read(bd->regmap, BD9571MWV_PRODUCT_REVISION, &value);
+ ret = regmap_read(regmap, BD9571MWV_PRODUCT_REVISION, &value);
if (ret) {
dev_err(dev, "Failed to read revision register (ret=%i)\n",
ret);
return ret;
}
- dev_info(dev, "Device: BD9571MWV rev. %d\n", value & 0xff);
+ dev_info(dev, "Device: %s rev. %d\n", part_name, value & 0xff);
return 0;
}
@@ -149,38 +164,48 @@ static int bd9571mwv_identify(struct bd9571mwv *bd)
static int bd9571mwv_probe(struct i2c_client *client,
const struct i2c_device_id *ids)
{
- struct bd9571mwv *bd;
- int ret;
-
- bd = devm_kzalloc(&client->dev, sizeof(*bd), GFP_KERNEL);
- if (!bd)
- return -ENOMEM;
-
- i2c_set_clientdata(client, bd);
- bd->dev = &client->dev;
- bd->irq = client->irq;
+ const struct bd957x_data *data;
+ struct device *dev = &client->dev;
+ struct regmap *regmap;
+ struct regmap_irq_chip_data *irq_data;
+ int ret, irq = client->irq;
+
+ /* Read the PMIC product code */
+ ret = i2c_smbus_read_byte_data(client, BD9571MWV_PRODUCT_CODE);
+ if (ret < 0) {
+ dev_err(dev, "failed reading at 0x%02x\n",
+ BD9571MWV_PRODUCT_CODE);
+ return ret;
+ }
+ switch (ret) {
+ case BD9571MWV_PRODUCT_CODE_VAL:
+ data = &bd9571mwv_data;
+ break;
+ default:
+ dev_err(dev, "Unsupported device 0x%x\n", ret);
+ return -ENOENT;
+ }
- bd->regmap = devm_regmap_init_i2c(client, &bd9571mwv_regmap_config);
- if (IS_ERR(bd->regmap)) {
- dev_err(bd->dev, "Failed to initialize register map\n");
- return PTR_ERR(bd->regmap);
+ regmap = devm_regmap_init_i2c(client, data->regmap_config);
+ if (IS_ERR(regmap)) {
+ dev_err(dev, "Failed to initialize register map\n");
+ return P
