Messages in this thread | | | From | Geert Uytterhoeven <> | Date | Wed, 9 Dec 2020 14:25:56 +0100 | Subject | Re: [PATCH 2/3] mfd: bd9571mwv: Make the driver more generic |
| |
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
| |