Re: [PATCH v8 2/6] mfd: pf1550: add core driver

[Christophe JAILLET <christophe.jaillet@wanadoo.fr>]

Le 07/07/2025 à 23:37, Samuel Kayode via B4 Relay a écrit :
> From: Samuel Kayode <samuel.kayode-4ysUXcep3aM1wj+D4I0NRVaTQe2KTcn/@public.gmane.org>
> 
> Add the core driver for pf1550 PMIC. There are 3 subdevices for which the
> drivers will be added in subsequent patches.
> 
> Reviewed-by: Frank Li <Frank.Li-3arQi8VN3Tc@public.gmane.org>
> Signed-off-by: Samuel Kayode <samuel.kayode-4ysUXcep3aM1wj+D4I0NRVaTQe2KTcn/@public.gmane.org>

Hi,

some nitpicks and a few real questions.

CJ

...

> +	/* Add top level interrupts */
> +	ret = devm_regmap_add_irq_chip(pf1550->dev, pf1550->regmap, pf1550->irq,
> +				       IRQF_ONESHOT | IRQF_SHARED |
> +				       IRQF_TRIGGER_FALLING,
> +				       0, &pf1550_irq_chip,
> +				       &pf1550->irq_data);
> +	if (ret)
> +		return ret;
> +
> +	/* Add regulator */
> +	irq = regmap_irq_get_virq(pf1550->irq_data, PF1550_IRQ_REGULATOR);

Same as above.

> +	if (irq < 0)
> +		return dev_err_probe(pf1550->dev, irq,
> +				     "Failed to get parent vIRQ(%d) for chip %s\n",
> +				     PF1550_IRQ_REGULATOR, pf1550_irq_chip.name);
> +
> +	ret = devm_regmap_add_irq_chip(pf1550->dev, pf1550->regmap, irq,
> +				       IRQF_ONESHOT | IRQF_SHARED |
> +				       IRQF_TRIGGER_FALLING, 0,
> +				       &pf1550_regulator_irq_chip,
> +				       &pf1550->irq_data_regulator);
> +	if (ret)
> +		return dev_err_probe(pf1550->dev, ret,
> +				     "Failed to add %s IRQ chip\n",
> +				     pf1550_regulator_irq_chip.name);
> +
> +	domain = regmap_irq_get_domain(pf1550->irq_data_regulator);
> +
> +	ret =  devm_mfd_add_devices(pf1550->dev, PLATFORM_DEVID_NONE, regulator,

2 spaces after =

> +				    1, NULL, 0, domain);
> +	if (ret)
> +		return ret;
> +
> +	/* Add onkey */
> +	irq = regmap_irq_get_virq(pf1550->irq_data, PF1550_IRQ_ONKEY);

Same

> +	if (irq < 0)
> +		return dev_err_probe(pf1550->dev, irq,
> +				     "Failed to get parent vIRQ(%d) for chip %s\n",
> +				     PF1550_IRQ_ONKEY, pf1550_irq_chip.name);
> +
> +	ret = devm_regmap_add_irq_chip(pf1550->dev, pf1550->regmap, irq,
> +				       IRQF_ONESHOT | IRQF_SHARED |
> +				       IRQF_TRIGGER_FALLING, 0,
> +				       &pf1550_onkey_irq_chip,
> +				       &pf1550->irq_data_onkey);
> +	if (ret)
> +		return dev_err_probe(pf1550->dev, ret,
> +				     "Failed to add %s IRQ chip\n",
> +				     pf1550_onkey_irq_chip.name);
> +
> +	domain = regmap_irq_get_domain(pf1550->irq_data_onkey);
> +
> +	ret =  devm_mfd_add_devices(pf1550->dev, PLATFORM_DEVID_NONE, onkey, 1,

2 spaces after =

> +				    NULL, 0, domain);
> +	if (ret)
> +		return ret;
> +
> +	/* Add battery charger */
> +	irq = regmap_irq_get_virq(pf1550->irq_data, PF1550_IRQ_CHG);

This calls irq_create_mapping().
Should irq_dispose_mapping() or another helper be called in the error 
handling path and in the remove function, or is it already handled by a 
devm_ function?

> +	if (irq < 0)
> +		return dev_err_probe(pf1550->dev, irq,
> +				     "Failed to get parent vIRQ(%d) for chip %s\n",
> +				     PF1550_IRQ_CHG, pf1550_irq_chip.name);
> +
> +	ret = devm_regmap_add_irq_chip(pf1550->dev, pf1550->regmap, irq,
> +				       IRQF_ONESHOT | IRQF_SHARED |
> +				       IRQF_TRIGGER_FALLING, 0,
> +				       &pf1550_charger_irq_chip,
> +				       &pf1550->irq_data_charger);
> +	if (ret)
> +		return dev_err_probe(pf1550->dev, ret,
> +				     "Failed to add %s IRQ chip\n",
> +				     pf1550_charger_irq_chip.name);
> +
> +	domain = regmap_irq_get_domain(pf1550->irq_data_charger);
> +
> +	return devm_mfd_add_devices(pf1550->dev, PLATFORM_DEVID_NONE, charger,
> +				    1, NULL, 0, domain);
> +}
> +
> +static int pf1550_suspend(struct device *dev)
> +{
> +	struct pf1550_ddata *pf1550 = dev_get_drvdata(dev);
> +
> +	if (device_may_wakeup(dev)) {
> +		enable_irq_wake(pf1550->irq);
> +		disable_irq(pf1550->irq);
> +	}
> +
> +	return 0;
> +}
> +
> +static int pf1550_resume(struct device *dev)
> +{
> +	struct pf1550_ddata *pf1550 = dev_get_drvdata(dev);
> +
> +	if (device_may_wakeup(dev)) {
> +		disable_irq_wake(pf1550->irq);
> +		enable_irq(pf1550->irq);

Should this 2 lines be inverted?

> +	}
> +
> +	return 0;
> +}

...

> +#define PF1550_CHG_LINEAR_ONLY		12
> +#define PF1550_CHG_SNS_MASK		0xf
> +#define PF1550_CHG_INT_MASK             0x51

Space vs tab

> +
> +#define PF1550_BAT_NO_VBUS		0
> +#define PF1550_BAT_LOW_THAN_PRECHARG	1

...

CJ