Hi Mark Brown,
Thanks for the feedback.
> Subject: Re: [PATCH v5 10/11] regulator: Add Renesas PMIC RAA215300
> driver
>
> On Mon, May 22, 2023 at 11:18:48AM +0100, Biju Das wrote:
>
> > +++ b/drivers/regulator/raa215300.c
> > @@ -0,0 +1,102 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Renesas RAA215300 PMIC driver
>
> Please make the entire comment block a C++ one so things look more
> intentional.
OK, will change to C++ style comment block.
>
> > +static bool raa215300_is_volatile_reg(struct device *dev, unsigned
> > +int reg) {
> > + return true;
> > +}
> > +
> > +static const struct regmap_config raa215300_regmap_config = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > + .max_register = 0xff,
> > + .volatile_reg = raa215300_is_volatile_reg,
> > + .cache_type = REGCACHE_FLAT,
> > +};
>
> This does not seem to make any sense, the device is configured to have a
> cache but every single register is marked as volatile so nothing will be
> actually be cached? Either some registers should be cacheable or there
> should be no cache.
Will change it to no cache.
static const struct regmap_config raa215300_regmap_config = {
.reg_bits = 8,
.val_bits = 8,
.max_register = 0xff,
};
>
> > +static int raa215300_i2c_probe(struct i2c_client *client) {
> > + struct device *dev = &client->dev;
> > + unsigned int pmic_version;
> > + struct regmap *regmap;
> > + int ret;
> > +
> > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> > + return -EOPNOTSUPP;
> > +
> > + regmap = devm_regmap_init_i2c(client, &raa215300_regmap_config);
> > + if (IS_ERR(regmap))
> > + return dev_err_probe(dev, PTR_ERR(regmap),
> > + "regmap i2c init failed\n");
>
> Why is there a check for I2C functionality here? regmap will do the
> checks it needs, and it looks like the check is over zealous since it's
> requiring full I2C support but since the device has 8 bit registers and
> values it should interoperate happily with a smbus controller.
Agreed, will drop check for I2C functionality.
Cheers,
Biju
