| 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
| ||
|
Message-ID: <aG_JUhEQaiYQfJmz@fedora>
Date: Thu, 10 Jul 2025 10:08:18 -0400
From: Samuel Kayode <samuel.kayode@...oirfairelinux.com>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc: devnull+samuel.kayode.savoirfairelinux.com@...nel.org, 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, sre@...nel.org,
yibin.gong@....com
Subject: Re: [PATCH v8 2/6] mfd: pf1550: add core driver
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/@...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 =
>
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
Powered by blists - more mailing lists