Hi Shimoda-san,
On Tue, Dec 8, 2020 at 9:06 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@xxxxxxxxxxx> wrote:
> From: Khiem Nguyen <khiem.nguyen.xt@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> [shimoda: rebase and refactor]
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
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@xxxxxxxxxxxxxx
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
