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@xxxxxxxxxxx> 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
