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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <0406698c-6534-4aca-8994-e8a69ecee2b2@wanadoo.fr>
Date: Tue, 8 Jul 2025 20:46:48 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: devnull+samuel.kayode.savoirfairelinux.com@...nel.org
Cc: Frank.li@....com, abelvesa@...nel.org, abelvesa@...ux.com,
 b38343@...escale.com, broonie@...nel.org, conor+dt@...nel.org,
 devicetree@...r.kernel.org, dmitry.torokhov@...il.com, eballetbo@...il.com,
 imx@...ts.linux.dev, krzk+dt@...nel.org, lee@...nel.org,
 lgirdwood@...il.com, linux-input@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org, robh@...nel.org,
 samuel.kayode@...oirfairelinux.com, sre@...nel.org, yibin.gong@....com
Subject: Re: [PATCH v8 2/6] mfd: pf1550: add core driver

Le 07/07/2025 à 23:37, Samuel Kayode via B4 Relay a écrit :
> From: Samuel Kayode <samuel.kayode-4ysUXcep3aM1wj+D4I0NRVaTQe2KTcn/@...lic.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@...lic.gmane.org>
> Signed-off-by: Samuel Kayode <samuel.kayode-4ysUXcep3aM1wj+D4I0NRVaTQe2KTcn/@...lic.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