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.
> +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.
> +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.
Attachment:
signature.asc
Description: PGP signature
