Re: [PATCH 2/2] regulator: rt5759: Add support for Richtek RT5759 DCDC converter

[Krzysztof Kozlowski <krzk@kernel.org>]

On 25/03/2022 02:06, cy_huang wrote:
> From: ChiYuan Huang <cy_huang@richtek.com>
> 
> Add support for Richtek RT5759 high-performance DCDC converter.
> 
> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> ---
>  drivers/regulator/Kconfig            |  10 +
>  drivers/regulator/Makefile           |   1 +
>  drivers/regulator/rt5759-regulator.c | 372 +++++++++++++++++++++++++++++++++++
>  3 files changed, 383 insertions(+)
>  create mode 100644 drivers/regulator/rt5759-regulator.c
> 

(...)

> +static int rt5759_init_device_property(struct rt5759_priv *priv)
> +{
> +	unsigned int val = 0;
> +	bool wdt_enable;
> +
> +	/*
> +	 * Only RT5759A support external watchdog input
> +	 */
> +	if (priv->chip_type != CHIP_TYPE_RT5759A)
> +		return 0;
> +
> +	wdt_enable = device_property_read_bool(priv->dev,
> +					       "richtek,watchdog-enable");
> +	if (wdt_enable)

No need for separate wdt_enable variable.

> +		val = RT5759A_WDTEN_MASK;
> +
> +	return regmap_update_bits(priv->regmap, RT5759A_REG_WDTEN,
> +				  RT5759A_WDTEN_MASK, val);
> +}
> +
> +static int rt5759_manufacturer_check(struct rt5759_priv *priv)
> +{
> +	unsigned int vendor;
> +	int ret;
> +
> +	ret = regmap_read(priv->regmap, RT5759_REG_VENDORINFO, &vendor);
> +	if (ret)
> +		return ret;
> +
> +	if (vendor != RT5759_MANUFACTURER_ID) {
> +		dev_err(priv->dev, "vendor info not correct (%d)\n", vendor);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static bool rt5759_is_accessible_reg(struct device *dev, unsigned int reg)
> +{
> +	struct rt5759_priv *priv = dev_get_drvdata(dev);
> +
> +	if (reg <= RT5759_REG_DCDCSET)
> +		return true;
> +
> +	if (priv->chip_type == CHIP_TYPE_RT5759A && reg == RT5759A_REG_WDTEN)
> +		return true;
> +
> +	return false;
> +}
> +
> +static const struct regmap_config rt5759_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = RT5759A_REG_WDTEN,
> +	.readable_reg = rt5759_is_accessible_reg,
> +	.writeable_reg = rt5759_is_accessible_reg,
> +};
> +
> +static int rt5759_probe(struct i2c_client *i2c)
> +{
> +	struct rt5759_priv *priv;
> +	int ret;
> +
> +	priv = devm_kzalloc(&i2c->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = &i2c->dev;
> +	priv->chip_type = (unsigned long)of_device_get_match_data(&i2c->dev);
> +	i2c_set_clientdata(i2c, priv);
> +
> +	priv->regmap = devm_regmap_init_i2c(i2c, &rt5759_regmap_config);
> +	if (IS_ERR(priv->regmap)) {
> +		ret = PTR_ERR(priv->regmap);
> +		dev_err(&i2c->dev, "Failed to allocate regmap (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	ret = rt5759_manufacturer_check(priv);
> +	if (ret) {
> +		dev_err(&i2c->dev, "Failed to check device (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	ret = rt5759_init_device_property(priv);
> +	if (ret) {
> +		dev_err(&i2c->dev, "Failed to init device (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	return rt5759_regulator_register(priv);
> +}
> +
> +static const struct of_device_id __maybe_unused rt5759_device_table[] = {

I don't think this can be __maybe_unused. It is always referenced via
of_match_table, isn't it?

> +	{ .compatible = "richtek,rt5759", .data = (void *)CHIP_TYPE_RT5759 },
> +	{ .compatible = "richtek,rt5759a", .data = (void *)CHIP_TYPE_RT5759A },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, rt5759_device_table);
> +
> +static struct i2c_driver rt5759_driver = {
> +	.driver = {
> +		.name = "rt5759",
> +		.of_match_table = rt5759_device_table,
> +	},
> +	.probe_new = rt5759_probe,
> +};
> +module_i2c_driver(rt5759_driver);
> +
> +MODULE_AUTHOR("ChiYuan Huang <cy_huang@richtek.com>");
> +MODULE_DESCRIPTION("Richtek RT5759 Regulator Driver");
> +MODULE_LICENSE("GPL v2");


Best regards,
Krzysztof