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

Biju Das
2023-08-17T11:42:58Z

Hi Pavel Machek,

Thanks for the feedback.

> Subject: Re: [PATCH 6.1.y-cip 02/13] regulator: Add Renesas PMIC RAA215300
> driver
>
> 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..."?

Ok. Will fix this.

>
> > --- /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)?

This is fixed in [1]
[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20230817&id=e21ac64e669e960688e79bf5babeed63132dac8a

>
> > +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.

OK, will fix it. I checked most of the drivers are not doing
Checks for regmap in kernel. As you suggested will do it 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?

OK.

>
> > + clk = clk_register_fixed_rate(NULL, clk_name, NULL, 0, 32000);
>
> Elsewhere, you use 32767 for a rate.

I need to check with hardware people, whether 32K means 32000 or 32 *1024= 32768? and then update.

Cheers,
Biju