Re: [PATCH v9 6/9] mfd: Add new driver for MAX77705 PMIC

[Krzysztof Kozlowski <krzk@kernel.org>]

On 02/12/2024 10:47, Dzmitry Sankouski wrote:
> Add the core MFD driver for max77705 PMIC. We define five sub-devices
> for which the drivers will be added in subsequent patches.
> 
> Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com>
> 


...

> +
> +static int max77705_i2c_probe(struct i2c_client *i2c)
> +{
> +	struct max77693_dev *max77705;
> +	struct i2c_client *i2c_fg;
> +	struct regmap_irq_chip_data *irq_data;
> +	struct irq_domain *domain;
> +	int ret;
> +	unsigned int pmic_rev_value;
> +	enum max77705_hw_rev pmic_rev;
> +
> +


Only one blank line

> +	max77705 = devm_kzalloc(&i2c->dev, sizeof(*max77705), GFP_KERNEL);
> +	if (!max77705)
> +		return -ENOMEM;
> +
> +	max77705->i2c = i2c;
> +	max77705->dev = &i2c->dev;
> +	max77705->irq = i2c->irq;
> +	max77705->type = TYPE_MAX77705;
> +	i2c_set_clientdata(i2c, max77705);
> +
> +	max77705->regmap = devm_regmap_init_i2c(i2c, &max77705_regmap_config);
> +

No blank line. Theer is never blank line between call and its error check.

> +	if (IS_ERR(max77705->regmap))
> +		return PTR_ERR(max77705->regmap);
> +
> +	if (regmap_read(max77705->regmap, MAX77705_PMIC_REG_PMICREV, &pmic_rev_value) < 0)
> +		return -ENODEV;
> +
> +	pmic_rev = pmic_rev_value & MAX77705_REVISION_MASK;
> +

Drop blank line.

> +	if (pmic_rev != MAX77705_PASS3) {
> +		dev_err(max77705->dev, "rev.0x%x is not tested",
> +			pmic_rev);

Unnecessary line wrap.

> +		return -ENODEV;
> +	}
> +
> +	max77705->regmap_leds = devm_regmap_init_i2c(i2c, &max77705_leds_regmap_config);
> +
> +	if (IS_ERR(max77705->regmap_leds))
> +		return PTR_ERR(max77705->regmap_leds);
> +
> +	ret = devm_regmap_add_irq_chip(max77705->dev, max77705->regmap,
> +					max77705->irq,
> +					IRQF_ONESHOT | IRQF_SHARED, 0,
> +					&max77705_topsys_irq_chip,
> +					&irq_data);
> +

Same issues, all over the code.

> +	if (ret)
> +		dev_err(max77705->dev, "failed to add irq chip: %d\n", ret);
> +
> +	/* Unmask interrupts from all blocks in interrupt source register */
> +	ret = regmap_update_bits(max77705->regmap,
> +				 MAX77705_PMIC_REG_INTSRC_MASK,
> +				 MAX77705_SRC_IRQ_ALL, (unsigned int)~MAX77705_SRC_IRQ_ALL);


The need for cast comes from some compiler warning?

> +
> +	if (ret < 0) {
> +		dev_err(max77705->dev,
> +			"Could not unmask interrupts in INTSRC: %d\n", ret);
> +		return ret;
> +	}
> +
> +	domain = regmap_irq_get_domain(irq_data);
> +
> +	i2c_fg = devm_i2c_new_dummy_device(max77705->dev, max77705->i2c->adapter, I2C_ADDR_FG);
> +
> +	if (IS_ERR(i2c_fg))
> +		return PTR_ERR(i2c_fg);
> +
> +	max77705->i2c_fg = i2c_fg;
> +
> +	for (int i = 0; i < ARRAY_SIZE(max77705_devs); i++) {
> +		if (!strcmp(max77705_devs[i].name, FUEL_GAUGE_NAME)) {
> +			max77705_devs[i].platform_data = &i2c_fg;
> +			max77705_devs[i].pdata_size = sizeof(i2c_fg);
> +		}
> +	}
> +
> +	ret = devm_mfd_add_devices(max77705->dev, PLATFORM_DEVID_NONE,
> +				   max77705_devs, ARRAY_SIZE(max77705_devs),
> +				   NULL, 0, domain);
> +
> +	if (ret) {
> +		dev_err(max77705->dev, "Failed to register child devices: %d\n", ret);
> +		return ret;
> +	}
> +
> +	device_init_wakeup(max77705->dev, true);
> +
> +	return 0;
> +}
> +
> +static int max77705_suspend(struct device *dev)
> +{
> +	struct i2c_client *i2c = to_i2c_client(dev);
> +	struct max77693_dev *max77705 = i2c_get_clientdata(i2c);
> +
> +	disable_irq(max77705->irq);
> +
> +	if (device_may_wakeup(dev))
> +		enable_irq_wake(max77705->irq);
> +
> +	return 0;
> +}
> +
> +static int max77705_resume(struct device *dev)
> +{
> +	struct i2c_client *i2c = to_i2c_client(dev);
> +	struct max77693_dev *max77705 = i2c_get_clientdata(i2c);
> +
> +	if (device_may_wakeup(dev))
> +		disable_irq_wake(max77705->irq);
> +
> +	enable_irq(max77705->irq);
> +
> +	return 0;
> +}
> +DEFINE_SIMPLE_DEV_PM_OPS(max77705_pm_ops, max77705_suspend, max77705_resume);
> +
> +static const struct of_device_id max77705_i2c_of_match[] = {
> +	{ .compatible = "maxim,max77705" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, max77705_i2c_of_match);
> +
> +static struct i2c_driver max77705_i2c_driver = {
> +	.driver = {
> +		.name			= "max77705",
> +		.of_match_table		= max77705_i2c_of_match,
> +		.pm			= pm_sleep_ptr(&max77705_pm_ops),
> +		.suppress_bind_attrs	= true,
> +	},
> +	.probe = max77705_i2c_probe,
> +};
> +module_i2c_driver(max77705_i2c_driver);
> +
> +MODULE_DESCRIPTION("Maxim MAX77705 PMIC core driver");
> +MODULE_AUTHOR("Dzmitry Sankouski <dsankouski@gmail.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/max77693-common.h b/include/linux/mfd/max77693-common.h
> index a5bce099f1ed..8665097892cd 100644
> --- a/include/linux/mfd/max77693-common.h
> +++ b/include/linux/mfd/max77693-common.h
> @@ -1,6 +1,6 @@
>  /* SPDX-License-Identifier: GPL-2.0+ */
>  /*
> - * Common data shared between Maxim 77693 and 77843 drivers
> + * Common data shared between Maxim 77693, 77705 and 77843 drivers
>   *
>   * Copyright (C) 2015 Samsung Electronics
>   */
> @@ -11,6 +11,7 @@
>  enum max77693_types {
>  	TYPE_MAX77693_UNKNOWN,
>  	TYPE_MAX77693,
> +	TYPE_MAX77705,
>  	TYPE_MAX77843,
>  
>  	TYPE_MAX77693_NUM,
> @@ -25,6 +26,7 @@ struct max77693_dev {
>  	struct i2c_client *i2c_muic;	/* 0x4A , MUIC */
>  	struct i2c_client *i2c_haptic;	/* MAX77693: 0x90 , Haptic */
>  	struct i2c_client *i2c_chg;	/* MAX77843: 0xD2, Charger */
> +	struct i2c_client *i2c_fg;	/* MAX77843: 0xD2, Charger */


You mix patchsets. Don't grow 77843 wigth 77705. Or is this not max77843?

>  
>  	enum max77693_types type;
>  
> @@ -32,6 +34,7 @@ struct max77693_dev {
>  	struct regmap *regmap_muic;
>  	struct regmap *regmap_haptic;	/* Only MAX77693 */
>  	struct regmap *regmap_chg;	/* Only MAX77843 */
> +	struct regmap *regmap_leds;	/* Only MAX77705 */
>  
>  	struct regmap_irq_chip_data *irq_data_led;
>  	struct regmap_irq_chip_data *irq_data_topsys;
> diff --git a/include/linux/mfd/max77705-private.h b/include/linux/mfd/max77705-private.h
> new file mode 100644
> index 000000000000..be781a0f9802


...

> +
> +enum max77705_led_reg {
> +	MAX77705_RGBLED_REG_BASE		= 0x30,
> +	MAX77705_RGBLED_REG_LEDEN		= 0,
> +	MAX77705_RGBLED_REG_LED0BRT,
> +	MAX77705_RGBLED_REG_LED1BRT,
> +	MAX77705_RGBLED_REG_LED2BRT,
> +	MAX77705_RGBLED_REG_LED3BRT,
> +	MAX77705_RGBLED_REG_LEDRMP,
> +	MAX77705_RGBLED_REG_LEDBLNK,
> +	MAX77705_LED_REG_END
> +};
> +
> +enum max77705_charger_battery_state {
> +	MAX77705_BATTERY_NOBAT,
> +	MAX77705_BATTERY_PREQUALIFICATION,
> +	MAX77705_BATTERY_DEAD,
> +	MAX77705_BATTERY_GOOD,
> +	MAX77705_BATTERY_LOWVOLTAGE,
> +	MAX77705_BATTERY_OVERVOLTAGE,
> +	MAX77705_BATTERY_RESERVED,
> +};
> +
> +enum max77705_charger_charge_type {
> +	MAX77705_CHARGER_CONSTANT_CURRENT	= 1,
> +	MAX77705_CHARGER_CONSTANT_VOLTAGE,
> +	MAX77705_CHARGER_END_OF_CHARGE,
> +	MAX77705_CHARGER_DONE,
> +};
> +
> +extern const struct dev_pm_ops max77705_pm_ops;


Why do you need it in the header?

> +
> +#endif /* __LINUX_MFD_MAX77705_PRIV_H */
> 


Best regards,
Krzysztof