lkml.org 
[lkml]   [2020]   [Dec]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v3 11/12] mfd: bd9571mwv: Make the driver more generic
    Date

    On Wed, 2020-12-16 at 10:25 +0200, Matti Vaittinen wrote:
    > On Wed, 2020-12-16 at 16:37 +0900, Yoshihiro Shimoda 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>
    >
    > Reviewed-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
    >
    > > ---
    > > drivers/mfd/bd9571mwv.c | 95 +++++++++++++++++++++++++++----
    > > ------------
    > > include/linux/mfd/bd9571mwv.h | 18 ++------
    > > 2 files changed, 63 insertions(+), 50 deletions(-)
    > >
    > > diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c
    > > index 49e968e..ccf1a60 100644
    > > --- a/drivers/mfd/bd9571mwv.c
    > > +++ b/drivers/mfd/bd9571mwv.c
    > > @@ -3,6 +3,7 @@
    > > * ROHM BD9571MWV-M MFD driver
    > > *
    > > * Copyright (C) 2017 Marek Vasut <marek.vasut+renesas@gmail.com>
    > > + * Copyright (C) 2020 Renesas Electronics Corporation
    > > *
    > > * Based on the TPS65086 driver
    > > */
    > > @@ -14,6 +15,19 @@
    > >
    > > #include <linux/mfd/bd9571mwv.h>
    > >
    > > +/**
    > > + * struct bd957x_data - internal data for the bd957x driver
    > > + *
    > > + * Internal data to distinguish bd957x variants
    > > + */
    > > +struct bd957x_data {
    > > + char *part_name;
    > > + const struct regmap_config *regmap_config;
    > > + const struct regmap_irq_chip *irq_chip;
    > > + const struct mfd_cell *cells;
    > > + int num_cells;
    > > +};
    > > +
    >
    > I do like the way you placed the variant data in owns structs. Well
    > thought.
    >
    > > static const struct mfd_cell bd9571mwv_cells[] = {
    > > { .name = "bd9571mwv-regulator", },
    > > { .name = "bd9571mwv-gpio", },
    > > @@ -102,13 +116,21 @@ static struct regmap_irq_chip
    > > bd9571mwv_irq_chip = {
    > > .num_irqs = ARRAY_SIZE(bd9571mwv_irqs),
    > > };
    > >
    > > -static int bd9571mwv_identify(struct bd9571mwv *bd)
    > > +static const struct bd957x_data bd9571mwv_data = {
    > > + .part_name = BD9571MWV_PART_NAME,
    > > + .regmap_config = &bd9571mwv_regmap_config,
    > > + .irq_chip = &bd9571mwv_irq_chip,
    > > + .cells = bd9571mwv_cells,
    > > + .num_cells = ARRAY_SIZE(bd9571mwv_cells),
    > > +};
    > > +
    > > +static int bd9571mwv_identify(struct device *dev, struct regmap
    > > *regmap,
    > > + const char *part_name)
    > > {
    > > - struct device *dev = bd->dev;
    > > unsigned int value;
    > > int ret;
    > >
    > > - ret = regmap_read(bd->regmap, BD9571MWV_VENDOR_CODE, &value);
    > > + ret = regmap_read(regmap, BD9571MWV_VENDOR_CODE, &value);
    > > if (ret) {
    > > dev_err(dev, "Failed to read vendor code register
    > > (ret=%i)\n",
    > > ret);
    > > @@ -121,27 +143,20 @@ static int bd9571mwv_identify(struct
    > > bd9571mwv
    > > *bd)
    > > return -EINVAL;
    > > }
    > >
    > > - ret = regmap_read(bd->regmap, BD9571MWV_PRODUCT_CODE, &value);
    > > + ret = regmap_read(regmap, BD9571MWV_PRODUCT_CODE, &value);
    > > if (ret) {
    > > dev_err(dev, "Failed to read product code register
    > > (ret=%i)\n",
    > > ret);
    > > return ret;
    > > }
    > > -
    > > - if (value != BD9571MWV_PRODUCT_CODE_VAL) {
    > > - dev_err(dev, "Invalid product code ID %02x (expected
    > > %02x)\n",
    > > - value, BD9571MWV_PRODUCT_CODE_VAL);
    > > - return -EINVAL;
    > > - }
    > > -
    > > - ret = regmap_read(bd->regmap, BD9571MWV_PRODUCT_REVISION,
    > > &value);
    > > + ret = regmap_read(regmap, BD9571MWV_PRODUCT_REVISION, &value);
    > > if (ret) {
    > > dev_err(dev, "Failed to read revision register
    > > (ret=%i)\n",
    > > ret);
    > > return ret;
    > > }
    > >
    > > - dev_info(dev, "Device: BD9571MWV rev. %d\n", value & 0xff);
    > > + dev_info(dev, "Device: %s rev. %d\n", part_name, value & 0xff);
    > >
    > > return 0;
    > > }
    > > @@ -149,38 +164,48 @@ static int bd9571mwv_identify(struct
    > > bd9571mwv
    > > *bd)
    > > static int bd9571mwv_probe(struct i2c_client *client,
    > > const struct i2c_device_id *ids)
    > > {
    > > - struct bd9571mwv *bd;
    > > - int ret;
    > > -
    > > - bd = devm_kzalloc(&client->dev, sizeof(*bd), GFP_KERNEL);
    > > - if (!bd)
    > > - return -ENOMEM;
    > > -
    > > - i2c_set_clientdata(client, bd);
    > > - bd->dev = &client->dev;
    > > - bd->irq = client->irq;
    > > + const struct bd957x_data *data;
    > > + struct device *dev = &client->dev;
    > > + struct regmap *regmap;
    > > + struct regmap_irq_chip_data *irq_data;
    > > + int ret, irq = client->irq;
    > > +
    > > + /* Read the PMIC product code */
    > > + ret = i2c_smbus_read_byte_data(client, BD9571MWV_PRODUCT_CODE);
    >
    > Having to use the i2c_smbus_read_byte_data for a device which is
    > going
    > to be used with regmap slightly bugs me. But as you want to do the
    > runtime probing, then this access must be done prior regmap
    > registration - so I can't think of a better way. :(

    I just noticed that reading the product code is something one would
    expect the bd9571mwv_identify() be doing. Now when you do read product
    code already here, the additional value brought by bd9571mwv_identify()
    is quite small. If you ever re-spin this, please consider if
    bd9571mwv_identify() is actually usefull or if product code reading
    should be done in that function - or if the usefull checks (if any)
    from the identify() could be inlined here :)

    Best Regards
    Matti Vaittinen
    \
     
     \ /
      Last update: 2020-12-16 09:47    [W:3.521 / U:0.100 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site