Re: [PATCH v2 5/9] mfd: pf1550: add core mfd driver

[Krzysztof Kozlowski <krzk@kernel.org>]

On 16/05/2025 20:54, Samuel Kayode wrote:
> +
> +static int pf1550_i2c_probe(struct i2c_client *i2c)
> +{
> +	struct pf1550_dev *pf1550;
> +	unsigned int reg_data = 0;
> +	int ret = 0;
> +
> +	pf1550 = devm_kzalloc(&i2c->dev,
> +			      sizeof(struct pf1550_dev), GFP_KERNEL);

sizeof(*)

> +	if (!pf1550)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(i2c, pf1550);
> +	pf1550->dev = &i2c->dev;
> +	pf1550->i2c = i2c;
> +	pf1550->irq = i2c->irq;
> +
> +	pf1550->regmap = devm_regmap_init_i2c(i2c, &pf1550_regmap_config);
> +	if (IS_ERR(pf1550->regmap)) {
> +		ret = PTR_ERR(pf1550->regmap);
> +		dev_err(pf1550->dev, "failed to allocate register map: %d\n",
> +			ret);


Syntax is always: return dev_err_probe

> +		return ret;
> +	}
> +
> +	ret = regmap_read(pf1550->regmap, PF1550_PMIC_REG_DEVICE_ID, &reg_data);
> +	if (ret < 0 || reg_data != PF1550_DEVICE_ID) {
> +		dev_err(pf1550->dev, "device not found!\n");
> +		return ret;

Syntax is always: return dev_err_probe

> +	}
> +
> +	pf1550->type = PF1550;
> +	dev_info(pf1550->dev, "pf1550 found.\n");

Drop. Drivers should be silent. This is really useless and just pollutes
log. See also coding style.

> +
> +	ret = devm_regmap_add_irq_chip(pf1550->dev, pf1550->regmap,
> +				       pf1550->irq,
> +				IRQF_ONESHOT | IRQF_SHARED |
> +				IRQF_TRIGGER_FALLING, 0,
> +				&pf1550_regulator_irq_chip,
> +				&pf1550->irq_data_regulator);
> +	if (ret) {
> +		dev_err(pf1550->dev, "failed to add irq1 chip: %d\n", ret);
> +		return ret;

Syntax is always: return dev_err_probe

> +	}
> +
> +	ret = devm_regmap_add_irq_chip(pf1550->dev, pf1550->regmap,
> +				       pf1550->irq,
> +				IRQF_ONESHOT | IRQF_SHARED |
> +				IRQF_TRIGGER_FALLING, 0,
> +				&pf1550_onkey_irq_chip,
> +				&pf1550->irq_data_onkey);
> +	if (ret) {
> +		dev_err(pf1550->dev, "failed to add irq3 chip: %d\n", ret);
> +		return ret;

Syntax is always: return dev_err_probe

> +	}
> +
> +	ret = devm_regmap_add_irq_chip(pf1550->dev, pf1550->regmap,
> +				       pf1550->irq,
> +				IRQF_ONESHOT | IRQF_SHARED |
> +				IRQF_TRIGGER_FALLING, 0,
> +				&pf1550_charger_irq_chip,
> +				&pf1550->irq_data_charger);
> +	if (ret) {
> +		dev_err(pf1550->dev, "failed to add irq4 chip: %d\n", ret);
> +		return ret;

Syntax is always: return dev_err_probe

> +	}
> +
> +	return devm_mfd_add_devices(pf1550->dev, -1, pf1550_devs,
> +			      ARRAY_SIZE(pf1550_devs), NULL, 0, NULL);
> +}
> +
> +static const struct i2c_device_id pf1550_i2c_id[] = {
> +	{ "pf1550", PF1550 },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, pf1550_i2c_id);

Table IDs are next to each other.

> +
> +static int pf1550_suspend(struct device *dev)
> +{
> +	struct i2c_client *i2c = container_of(dev, struct i2c_client, dev);
> +	struct pf1550_dev *pf1550 = i2c_get_clientdata(i2c);
> +
> +	if (device_may_wakeup(dev)) {
> +		enable_irq_wake(pf1550->irq);
> +		disable_irq(pf1550->irq);
> +	}
> +
> +	return 0;
> +}
> +
> +static int pf1550_resume(struct device *dev)
> +{
> +	struct i2c_client *i2c = container_of(dev, struct i2c_client, dev);
> +	struct pf1550_dev *pf1550 = i2c_get_clientdata(i2c);
> +
> +	if (device_may_wakeup(dev)) {
> +		disable_irq_wake(pf1550->irq);
> +		enable_irq(pf1550->irq);
> +	}
> +
> +	return 0;
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(pf1550_pm, pf1550_suspend, pf1550_resume);
> +
> +static const struct of_device_id pf1550_dt_match[] = {
> +	{ .compatible = "fsl,pf1550" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, pf1550_dt_match);
> +
> +static struct i2c_driver pf1550_i2c_driver = {
> +	.driver = {
> +		   .name = "pf1550",
> +		   .pm = pm_sleep_ptr(&pf1550_pm),
> +		   .of_match_table = of_match_ptr(pf1550_dt_match),


Drop of_match_ptr, you have warnings here.

> +	},
> +	.probe = pf1550_i2c_probe,
> +	.id_table = pf1550_i2c_id,
> +};
> +
> +module_i2c_driver(pf1550_i2c_driver);
> +
> +MODULE_DESCRIPTION("Freescale PF1550 multi-function core driver");
> +MODULE_AUTHOR("Robin Gong <yibin.gong@freescale.com>");
> +MODULE_LICENSE("GPL v2");
Best regards,
Krzysztof