Re: [PATCH v2 1/1] mfd: simple-mfd-i2c: Add support for registering devices via MFD cells

[Lee Jones]

On Fri, 06 Aug 2021, Alistair Francis wrote:

> On Thu, Aug 5, 2021 at 6:56 PM Lee Jones <lee.jones@xxxxxxxxxx> wrote:
> >
> > More devices are cropping up requiring only Regmap initialisation and
> > child registration functionality.  We currently only support that if
> > all required devices are represented by their own Device Tree nodes
> > complete with compatible strings.
> >
> > However, not everyone is happy with adding empty nodes that provide no
> > additional device information into the Device Tree.
> >
> > Rather than have a plethora of mostly empty, function-less drivers in
> > MFD, we'll support those simple cases in here instead via MFD cells.
> >
> > Cc: Michael Walle <michael@xxxxxxxx>
> > Cc: Mark Brown <broonie@xxxxxxxxxx>
> > Cc: Alistair Francis <alistair23@xxxxxxxxx>
> > Signed-off-by: Lee Jones <lee.jones@xxxxxxxxxx>
> > ---
> > v2:
> >  - Check for empty of_device_id .data entry
> >
> >  drivers/mfd/simple-mfd-i2c.c | 41 +++++++++++++++++++++++++++++-------
> >  drivers/mfd/simple-mfd-i2c.h | 32 ++++++++++++++++++++++++++++
> >  2 files changed, 65 insertions(+), 8 deletions(-)
> >  create mode 100644 drivers/mfd/simple-mfd-i2c.h
> >
> > diff --git a/drivers/mfd/simple-mfd-i2c.c b/drivers/mfd/simple-mfd-i2c.c
> > index 87f684cff9a17..583e8c7924af0 100644
> > --- a/drivers/mfd/simple-mfd-i2c.c
> > +++ b/drivers/mfd/simple-mfd-i2c.c
> > @@ -2,39 +2,64 @@
> >  /*
> >   * Simple MFD - I2C
> >   *
> > + * Author(s):
> > + *     Michael Walle <michael@xxxxxxxx>
> > + *     Lee Jones <lee.jones@xxxxxxxxxx>
> > + *
> >   * This driver creates a single register map with the intention for it to be
> >   * shared by all sub-devices.  Children can use their parent's device structure
> >   * (dev.parent) in order to reference it.
> >   *
> >   * Once the register map has been successfully initialised, any sub-devices
> > - * represented by child nodes in Device Tree will be subsequently registered.
> > + * represented by child nodes in Device Tree or via the MFD cells in this file
> > + * will be subsequently registered.
> >   */
> >
> >  #include <linux/i2c.h>
> >  #include <linux/kernel.h>
> > +#include <linux/mfd/core.h>
> >  #include <linux/module.h>
> >  #include <linux/of_platform.h>
> >  #include <linux/regmap.h>
> >
> > -static const struct regmap_config simple_regmap_config = {
> > +#include "simple-mfd-i2c.h"
> > +
> > +static const struct regmap_config regmap_config_8r_8v = {
> >         .reg_bits = 8,
> >         .val_bits = 8,
> >  };
> >
> >  static int simple_mfd_i2c_probe(struct i2c_client *i2c)
> >  {
> > -       const struct regmap_config *config;
> > +       const struct simple_mfd_data *simple_mfd_data;
> > +       const struct regmap_config *regmap_config;
> >         struct regmap *regmap;
> > +       int ret;
> > +
> > +       simple_mfd_data = device_get_match_data(&i2c->dev);
> >
> > -       config = device_get_match_data(&i2c->dev);
> > -       if (!config)
> > -               config = &simple_regmap_config;
> > +       /* If no regmap_config is specified, use the default 8reg and 8val bits */
> > +       if (!simple_mfd_data || !simple_mfd_data->regmap_config)
> > +               regmap_config = &regmap_config_8r_8v;
> > +       else
> > +               regmap_config = simple_mfd_data->regmap_config;
> >
> > -       regmap = devm_regmap_init_i2c(i2c, config);
> > +       regmap = devm_regmap_init_i2c(i2c, regmap_config);
> >         if (IS_ERR(regmap))
> >                 return PTR_ERR(regmap);
> >
> > -       return devm_of_platform_populate(&i2c->dev);
> > +       /* If no MFD cells are spedified, use register the DT child nodes instead */
> > +       if (!simple_mfd_data || !simple_mfd_data->mfd_cell)
> > +               return devm_of_platform_populate(&i2c->dev);
> > +
> > +       ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO,
> > +                                  simple_mfd_data->mfd_cell,
> > +                                  simple_mfd_data->mfd_cell_size,
> > +                                  NULL, 0, NULL);
> > +       if (!ret)
> 
> Shouldn't this be `if (ret)` instead?
> 
> With that changed this works for me as well:

You're right.  Will fix.

> Reviewed-by: Alistair Francis <alistair@xxxxxxxxxxxxx>
> Tested-by: Alistair Francis <alistair@xxxxxxxxxxxxx>

Thanks.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog