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

[Lee Jones <lee@kernel.org>]

On Fri, 20 Jun 2025, Samuel Kayode wrote:

> 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.

Place it in a comment please.

> > 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.

In a comment please.  If I wondered, so with others.

> > This should all be made clear in some way or another.
> > 
> I'll be adding comments on this in the next version.

Great!

> > > +			   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?

That's okay, just do 3 calls.

Must neater than what we have here.

> > > +}
> > 
> > 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, &reg_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: ..."?

Right.  Invalid or unsupported, etc.

> >
> ...
> > > +	/* 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()?

Yes.  Or whatever fits best.

Maybe the h/w doesn't work that way, I just found it odd.

-- 
Lee Jones [李琼斯]