RE: [PATCH 2/3] mfd: bd9571mwv: Make the driver more generic

[Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>]

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