Re: [PATCH 6.1.y-cip 02/13] regulator: Add Renesas PMIC RAA215300 driver

Pavel Machek
2023-08-17T11:06:58Z

[-- Attachment #1: Type: text/plain, Size: 2325 bytes --]

Hi!

> +++ b/drivers/regulator/Kconfig
> @@ -1016,6 +1016,13 @@ config REGULATOR_QCOM_USB_VBUS
> Say M here if you want to include support for enabling the VBUS output
> as a module. The module will be named "qcom_usb_vbus_regulator".
>
> +config REGULATOR_RAA215300
> + tristate "Renesas RAA215300 driver"
> + select REGMAP_I2C
> + depends on I2C
> + help
> + Support for the Renesas RAA215300 PMIC.
> +

"Say M here ... the module will be named..."?

> --- /dev/null
> +++ b/drivers/regulator/raa215300.c
> @@ -0,0 +1,190 @@

> +static void raa215300_rtc_unregister_device(void *data)
> +{
> + i2c_unregister_device(data);
> + if (!clk) {
> + clk_unregister_fixed_rate(clk);
> + clk = NULL;
> + }
> +}

This one looks suspect. Should be (clk)?

> +static int raa215300_i2c_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + const char *clk_name = xin_name;
> + unsigned int pmic_version, val;
> + struct regmap *regmap;
> + int ret;
> +
> + 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");
> +
> + ret = regmap_read(regmap, RAA215300_HW_REV, &pmic_version);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "HW rev read failed\n");
> +
> + dev_dbg(dev, "RAA215300 PMIC version 0x%04x\n", pmic_version);
> +
> + /* Clear all blocks except RTC, if enabled */
> + regmap_read(regmap, RAA215300_REG_BLOCK_EN, &val);
> + val &= RAA215300_REG_BLOCK_EN_RTC_EN;
> + regmap_write(regmap, RAA215300_REG_BLOCK_EN, val);
> +
> + /*Clear the latched registers */

"/* Clear". I would not mind checking errors from regmap here.

> + ret = raa215300_clk_present(client, xin_name);
> + if (ret < 0) {
> + return ret;
> + } else if (!ret) {
> + ret = raa215300_clk_present(client, clkin_name);
> + if (ret < 0)
> + return ret;
> +

dev_err_probe would be consistent/useful for the other returns?

> + clk = clk_register_fixed_rate(NULL, clk_name, NULL, 0, 32000);

Elsewhere, you use 32767 for a rate.

Best regards,
Pavel
--
DENX Software Engineering GmbH, Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]