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

[Pavel Machek <pavel@denx.de>]

[-- 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 --]