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

[Samuel Kayode <samuel.kayode@savoirfairelinux.com>]

On Tue, Jul 08, 2025 at 08:46:48PM +0200, Christophe JAILLET wrote:
> 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 =
>
Will drop.
> > +				    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 =
> 
Will drop.
> > +				    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?
> 
This creates a mapping for the allocated `irq_data` runtime controller by
devm_regmap_add_irq. The `irq_data` is for the top level interrupts. Since it
was allocated with a devm_, I think irq_dispose_mapping is called during a
remove.
> > +	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?
> 
I don't think it matters. disable_irq_wake is 'completely orthogonal' to the
enable/disable(irq). See function irq_set_irq_wake.
> > +	}
> > +
> > +	return 0;
> > +}
> 
> ...
> 
> > +#define PF1550_CHG_LINEAR_ONLY		12
> > +#define PF1550_CHG_SNS_MASK		0xf
> > +#define PF1550_CHG_INT_MASK             0x51
> 
> Space vs tab
> 
Will make changes.
> > +
> > +#define PF1550_BAT_NO_VBUS		0
> > +#define PF1550_BAT_LOW_THAN_PRECHARG	1

Thanks,
Sam