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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <TY2PR01MB3692C55C6CDEFC83D6F8F90DD8CB0@TY2PR01MB3692.jpnprd01.prod.outlook.com>
Date:   Thu, 10 Dec 2020 04:09:55 +0000
From:   Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
CC:     Marek Vasut <marek.vasut+renesas@...il.com>,
        Lee Jones <lee.jones@...aro.org>,
        Khiem Nguyen <khiem.nguyen.xt@...esas.com>,
        Linux-Renesas <linux-renesas-soc@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 2/3] mfd: bd9571mwv: Make the driver more generic

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@...esas.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