Re: [PATCH v1 2/3] regulator: pca9450: add pca9451a support

[Alexander Stein]

Hi,

Am Dienstag, 1. August 2023, 12:17:20 CEST schrieb Joy Zou:
> > -----Original Message-----
> > From: Alexander Stein
> > <alexander.stein@xxxxxxxxxxxxxxx<mailto:alexander.stein@xxxxxxxxxxxxxxx>>
> > 
 Sent: 2023年7月5日 21:13
> > To: Jacky Bai <ping.bai@xxxxxxx<mailto:ping.bai@xxxxxxx>>;
> > lgirdwood@xxxxxxxxx<mailto:lgirdwood@xxxxxxxxx>;
> > broonie@xxxxxxxxxx<mailto:broonie@xxxxxxxxxx>;
> > robh+dt@xxxxxxxxxx<mailto:robh+dt@xxxxxxxxxx>;
> > krzysztof.kozlowski+dt@xxxxxxxxxx<mailto:krzysztof.kozlowski+dt@xxxxxxxxx
> > g>; conor+dt@xxxxxxxxxx<mailto:conor+dt@xxxxxxxxxx>;
> > shawnguo@xxxxxxxxxx<mailto:shawnguo@xxxxxxxxxx>;
> > s.hauer@xxxxxxxxxxxxxx<mailto:s.hauer@xxxxxxxxxxxxxx>;
> > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx<mailto:linux-arm-kernel@lists.infrad
> > ead.org>; Joy Zou <joy.zou@xxxxxxx<mailto:joy.zou@xxxxxxx>> Cc:
> > kernel@xxxxxxxxxxxxxx<mailto:kernel@xxxxxxxxxxxxxx>;
> > festevam@xxxxxxxxx<mailto:festevam@xxxxxxxxx>; dl-linux-imx
> > <linux-imx@xxxxxxx<mailto:linux-imx@xxxxxxx>>;
> > devicetree@xxxxxxxxxxxxxxx<mailto:devicetree@xxxxxxxxxxxxxxx>;
> > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx<mailto:linux-arm-kernel@lists.infrad
> > ead.org>;
> > linux-kernel@xxxxxxxxxxxxxxx<mailto:linux-kernel@xxxxxxxxxxxxxxx>
> > Subject: [EXT] Re: [PATCH v1 2/3] regulator: pca9450: add pca9451a
> > support>
> >
> >
> > Caution: This is an external email. Please take care when clicking links
> > or
 opening attachments. When in doubt, report the message using the
> > 'Report this email' button
> >
> >
> >
> >
> > Hello,
> >
> >
> >
> > Am Mittwoch, 5. Juli 2023, 08:50:24 CEST schrieb Joy Zou:
> > 
> > >
> > >
> > > > -----Original Message-----
> > > > From: Alexander Stein
> > > > <alexander.stein@xxxxxxxxxxxxxxx<mailto:alexander.stein@xxxxx-group.c
> > > > om>>
 Sent: 2023年5月31日 19:35
> > > > To: Jacky Bai <ping.bai@xxxxxxx<mailto:ping.bai@xxxxxxx>>;
> > > > lgirdwood@xxxxxxxxx<mailto:lgirdwood@xxxxxxxxx>;
> > > > broonie@xxxxxxxxxx<mailto:broonie@xxxxxxxxxx>;
> > > > robh+dt@xxxxxxxxxx<mailto:robh+dt@xxxxxxxxxx>;
> > > > krzysztof.kozlowski+dt@xxxxxxxxxx<mailto:krzysztof.kozlowski+dt@linar
> > > > o.org>; conor+dt@xxxxxxxxxx<mailto:conor+dt@xxxxxxxxxx>;
> > > > shawnguo@xxxxxxxxxx<mailto:shawnguo@xxxxxxxxxx>;
> > > > s.hauer@xxxxxxxxxxxxxx<mailto:s.hauer@xxxxxxxxxxxxxx>;
> > > > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx<mailto:linux-arm-kernel@xxxxxxxx
> > > > fradead.org> Cc: kernel@xxxxxxxxxxxxxx<mailto:kernel@xxxxxxxxxxxxxx>;
> > > > festevam@xxxxxxxxx<mailto:festevam@xxxxxxxxx>; dl-linux-imx
> > > > <linux-imx@xxxxxxx<mailto:linux-imx@xxxxxxx>>;
> > > > devicetree@xxxxxxxxxxxxxxx<mailto:devicetree@xxxxxxxxxxxxxxx>;
> > > > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx<mailto:linux-arm-kernel@xxxxxxxx
> > > > fradead.org>;
> > > > linux-kernel@xxxxxxxxxxxxxxx<mailto:linux-kernel@xxxxxxxxxxxxxxx>;
> > > > Joy Zou
> >  
> >  <joy.zou@xxxxxxx<mailto:joy.zou@xxxxxxx>>
> >  
> > > > Subject: Re: [PATCH v1 2/3] regulator: pca9450: add pca9451a
> > > > support
> > > >
> > > >
> > > >
> > > > Hi,
> > > > 
> > > > > @@ -104,7 +104,15 @@ static const struct regulator_ops
> > > > > pca9450_ldo_regulator_ops = { * 0.60 to 2.1875V (12.5mV step)
> > > > >
> > > > >
> > > > >
> > > > >   */
> > > > >
> > > > >
> > > > >
> > > > >  static const struct linear_range pca9450_dvs_buck_volts[] = {
> > > > >
> > > > >
> > > > >
> > > > > -     REGULATOR_LINEAR_RANGE(600000,  0x00, 0x7F, 12500),
> > > > > +     REGULATOR_LINEAR_RANGE(600000, 0x00, 0x7F, 12500), };
> > > > > +
> > > > > +/*
> > > > > + * BUCK1/3
> > > > > + * 0.65 to 2.2375V (12.5mV step)
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > Reading this comment, it seems the same distinction needs to be done
> > > > for
> > > > BUCK3 as well, no?
> > >
> > >
> > >
> > > Sorry for the late reply!
> > > The BUCK1 and BUCK3 are dual phase, so don't need to be done for BUCK3.
> > >
> > >
> > >
> > > >
> > > >
> > > >
> > > > > + */
> > > > > +static const struct linear_range pca9450_trim_dvs_buck_volts[] = {
> > > > > +     REGULATOR_LINEAR_RANGE(650000, 0x00, 0x7F, 12500),
> > > > >
> > > > >
> > > > >
> > > > >  };
> 
> 
> 
> > > > > @@ -708,8 +917,9 @@ static int pca9450_i2c_probe(struct i2c_client
> > > > > *i2c)
> > > > >
> > > > >
> > > > >
> > > > >       const struct pca9450_regulator_desc     *regulator_desc;
> > > > >       struct regulator_config config = { };
> > > > >       struct pca9450 *pca9450;
> > > > >
> > > > >
> > > > >
> > > > > -     unsigned int device_id, i;
> > > > > +     unsigned int device_id, i, val;
> > > > >
> > > > >
> > > > >
> > > > >       unsigned int reset_ctrl;
> > > > >
> > > > >
> > > > >
> > > > > +     bool pmic_trim = false;
> > > > >
> > > > >
> > > > >
> > > > >       int ret;
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >       if (!i2c->irq) {
> > > > >
> > > > >
> > > > >
> > > > > @@ -721,6 +931,22 @@ static int pca9450_i2c_probe(struct
> > > > > i2c_client
> > > > > *i2c)
> > > > >
> > > > >
> > > > >
> > > > >       if (!pca9450)
> > > > >
> > > > >
> > > > >
> > > > >               return -ENOMEM;
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > +     pca9450->regmap = devm_regmap_init_i2c(i2c,
> > > > > +
> > > >
> > > >
> > > >
> > > > &pca9450_regmap_config);
> > > >
> > > >
> > > >
> > > > > +     if (IS_ERR(pca9450->regmap)) {
> > > > > +             dev_err(&i2c->dev, "regmap initialization failed\n");
> > > > > +             return PTR_ERR(pca9450->regmap);
> > > > > +     }
> > > > > +
> > > > > +     ret = regmap_read(pca9450->regmap, PCA9450_REG_PWRCTRL,
> > > >
> > > >
> > > >
> > > > &val);
> > > >
> > > >
> > > >
> > > > > +     if (ret) {
> > > > > +             dev_err(&i2c->dev, "Read device id error\n");
> > > > > +             return ret;
> > > > > +     }
> > > > > +
> > > > > +     if (val & PCA9450_REG_PWRCTRL_TOFF_DEB)
> > > > > +             pmic_trim = true;
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > PCA9450_REG_PWRCTRL is a read/write register. How is it possible to
> > > > detect a chip revision using a bit which can be changed by software
> > > > e.g.
> > > > bootloader? Despite that this bit sets debounce time for
> > > > PMIC_ON_REQ, how is this related to BUCK1 voltage range?
> > >
> > >
> > >
> > > There are old and new two kind PMIC pca9451a.
> >
> >
> >
> > There is only one part mentioned in the ordering options. How can I
> > distinguish them? Any chip ID, date codes, markings?
> 
> Yes, there is only one part. We distinguish the new and old part by this
> bit
 Toff_Deb of PCA9450_REG_PWRCTRL reset value. The reset value 0 means
> it's old part, and the reset value 1 means it's new part.

Is the "old" part by coincidence an unofficial prerelease/sample chip?

> >
> >
> > > This bit sets debounce time in
> > > PCA9450_REG_PWRCTRL was set different value by hardware in order to
> > > only distinguish the old and new PMIC. This bit isn't related to the
> > > BUCK1 voltage range. If the pmic_trim is true that means it's new
> > > pca9451a.
>
> >
> >
> > But this bit is writable. How do you know it has not been modified since
> > reset?
> Yes, we don't consider modify the debounce bit case. Modify the Toff_deb
> value
 will influence the old and new part judgement.

This judgement seems broken to me. How can I know offline whether I have old 
or new parts? I would like to know if there is a difference on some my boards.

> For example, this default value of Toff_deb is 1 in the new part, if the
> customers
 change the Toff_deb value from 1 to 0, and then make the board
> warm reset, the Toff_deb value still keep 0, if the Toff_deb value is 0,
> the PMIC driver will think this part is old part. But this part is new part
> in fact.

This should show you it's a bad idea to decide the chip revision depending on 
Toff_deb.

> Have discussed this issue with our internal team member, we will add a note
> to PCA9451
 datasheet – “Please contract NXP If you want to change
> Toff_deb.” But till now, we am not aware any customers case which need to
> adjust Toff_deb.
> 
> Make it more clear: If customers do need to manually adjust Toff_deb, It
> need PMIC driver
 update to bypass this bit check and directly apply
> corresponding voltage config table old or new. Thank you very much for your
> comments and efforts.

In this case I need to know if I use old, new or both revision of these parts.

Best regards,
Alexander
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/