| 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: <aFWrxtArHjb5nc0M@fedora>
Date: Fri, 20 Jun 2025 14:43:18 -0400
From: Samuel Kayode <samuel.kayode@...oirfairelinux.com>
To: Lee Jones <lee@...nel.org>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>,
Dmitry Torokhov <dmitry.torokhov@...il.com>,
Sebastian Reichel <sre@...nel.org>, Frank Li <Frank.li@....com>,
imx@...ts.linux.dev, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-input@...r.kernel.org,
linux-pm@...r.kernel.org, Abel Vesa <abelvesa@...nel.org>,
Abel Vesa <abelvesa@...ux.com>, Robin Gong <b38343@...escale.com>,
Robin Gong <yibin.gong@....com>,
Enric Balletbo i Serra <eballetbo@...il.com>
Subject: Re: [PATCH v7 2/6] mfd: pf1550: add core mfd driver
Hi Lee,
Thanks a lot for the review.
On Thu, Jun 19, 2025 at 02:03:37PM +0100, Lee Jones wrote:
> > +static int pf1550_read_otp(const struct pf1550_dev *pf1550, unsigned int index,
>
> What does OTP mean?
>
It's a One-Time Programmable memory with configuration for the pf1550. I will
expand on this in the commit description of the next version.
> Why do you have to write to 4 registers first?
>
The pf1550 was designed such that the registers of the accompanying OTP is
accessed indirectly. Valid keys have to be written to specific pf1550
registers. After writing the keys, the address of the OTP register to be read
is then written to PF1550_TEST_REG_FMRADDR and its corresponding value read from
PF1550_TEST_REG_FMRDATA.
> This should all be made clear in some way or another.
>
I'll be adding comments on this in the next version.
> > + unsigned int *val)
> > +{
> > + int ret = 0;
> > +
> > + ret = regmap_write(pf1550->regmap, PF1550_PMIC_REG_KEY, 0x15);
>
> No magic numbers. These should all be defined.
Will do.
>
> > + if (ret)
> > + goto read_err;
> > + ret = regmap_write(pf1550->regmap, PF1550_CHARG_REG_CHGR_KEY2, 0x50);
> > + if (ret)
> > + goto read_err;
> > + ret = regmap_write(pf1550->regmap, PF1550_TEST_REG_KEY3, 0xab);
> > + if (ret)
> > + goto read_err;
> > + ret = regmap_write(pf1550->regmap, PF1550_TEST_REG_FMRADDR, index);
> > + if (ret)
> > + goto read_err;
> > + ret = regmap_read(pf1550->regmap, PF1550_TEST_REG_FMRDATA, val);
> > + if (ret)
> > + goto read_err;
> > +
> > + return 0;
> > +
> > +read_err:
> > + dev_err_probe(pf1550->dev, ret, "read otp reg %x found!\n", index);
...
> > +static int pf1550_add_child_device(struct pf1550_dev *pmic,
> > + const struct mfd_cell *cell,
> > + struct regmap_irq_chip_data *pdata,
>
> This is not pdata.
>
> > + int pirq,
> > + const struct regmap_irq_chip *chip,
> > + struct regmap_irq_chip_data **data)
> > +{
> > + struct device *dev = pmic->dev;
> > + struct irq_domain *domain;
> > + int irq, ret;
> > +
> > + irq = regmap_irq_get_virq(pdata, pirq);
> > + if (irq < 0)
> > + return dev_err_probe(dev, irq,
> > + "Failed to get parent vIRQ(%d) for chip %s\n",
> > + pirq, chip->name);
> > +
> > + ret = devm_regmap_add_irq_chip(dev, pmic->regmap, irq,
> > + IRQF_ONESHOT | IRQF_SHARED |
> > + IRQF_TRIGGER_FALLING, 0, chip, data);
> > + if (ret)
> > + return dev_err_probe(dev, ret,
> > + "Failed to add %s IRQ chip\n",
> > + chip->name);
> > +
> > + domain = regmap_irq_get_domain(*data);
> > +
> > + return devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, cell, 1,
> > + NULL, 0, domain);
>
> Why can't all 3 devices be registered in one call?
>
The 3 devices use different regmap_irq_chip s. I have to register them
separately cause they have different irq domains but perhaps there is a better
way to handle this?
> > +}
>
> To be honest, the premise around this function is a bit of a mess.
>
> Please move all of this into .probe().
Will do.
>
> > +static int pf1550_i2c_probe(struct i2c_client *i2c)
> > +{
> > + const struct mfd_cell *regulator = &pf1550_regulator_cell;
> > + const struct mfd_cell *charger = &pf1550_charger_cell;
> > + const struct mfd_cell *onkey = &pf1550_onkey_cell;
> > + unsigned int reg_data = 0, otp_data = 0;
> > + struct pf1550_dev *pf1550;
> > + int ret = 0;
> > +
> > + pf1550 = devm_kzalloc(&i2c->dev, sizeof(*pf1550), GFP_KERNEL);
> > + if (!pf1550)
> > + return -ENOMEM;
> > +
> > + i2c_set_clientdata(i2c, pf1550);
> > + pf1550->dev = &i2c->dev;
> > + pf1550->i2c = i2c;
>
> What are you storing i2c for?
>
It doesn't need to be stored.
> Either store dev and irq OR i2c. You don't need all three.
>
Will do.
> > + ret = regmap_read(pf1550->regmap, PF1550_PMIC_REG_DEVICE_ID, ®_data);
> > + if (ret < 0 || reg_data != PF1550_DEVICE_ID)
> > + return dev_err_probe(pf1550->dev, ret ?: -EINVAL,
> > + "device not found!\n");
>
> Are you sure? What if the wrong device was found?
>
I can change the error log here to "Invalid device ID: ..."?
>
...
> > + /* 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;
> > +
> > + ret = pf1550_add_child_device(pf1550, regulator, pf1550->irq_data,
> > + PF1550_IRQ_REGULATOR,
> > + &pf1550_regulator_irq_chip,
> > + &pf1550->irq_data_regulator);
> > + if (ret)
> > + return ret;
> > +
> > + ret = pf1550_add_child_device(pf1550, onkey, pf1550->irq_data,
> > + PF1550_IRQ_ONKEY,
> > + &pf1550_onkey_irq_chip,
> > + &pf1550->irq_data_onkey);
> > + if (ret)
> > + return ret;
> > +
> > + ret = pf1550_add_child_device(pf1550, charger, pf1550->irq_data,
> > + PF1550_IRQ_CHG,
> > + &pf1550_charger_irq_chip,
> > + &pf1550->irq_data_charger);
> > + return ret;
> > +}
> > +
> > +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);
>
> You can swap all of this for:
>
> struct pf1550_dev *pf1550 = dev_get_drvdata(dev).
>
Will do.
> > +
> > + 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);
>
> As above.
>
> > +
> > + if (device_may_wakeup(dev)) {
> > + disable_irq_wake(pf1550->irq);
> > + enable_irq(pf1550->irq);
>
> I would normally expect these to be around the opposite way to the ones
> in .suspend().
Do you mean enable_irq_wake and disable_irq in .resume() and the opposite for
.suspend()?
>
> > + }
> > +
> > + return 0;
> > +}
> > +
Thanks,
Sam
Powered by blists - more mailing lists