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

From: Joy Zou
Date: Wed Jul 05 2023 - 02:52:06 EST

Next message: Mauro Carvalho Chehab: "[GIT PULL for v6.5-rc1] media updates"
Previous message: Yong Wu (吴勇): "Re: [PATCH] iommu/mediatek: Remove a unnecessary checking for larbid"
Next in thread: Alexander Stein: "Re: [PATCH v1 2/3] regulator: pca9450: add pca9451a support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> -----Original Message-----
> From: Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx>
> Sent: 2023年5月31日 19:35
> To: Jacky Bai <ping.bai@xxxxxxx>; lgirdwood@xxxxxxxxx;
> broonie@xxxxxxxxxx; robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx;
> conor+dt@xxxxxxxxxx; shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Cc: kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx; dl-linux-imx
> <linux-imx@xxxxxxx>; devicetree@xxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Joy Zou
> <joy.zou@xxxxxxx>
> 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
>
>
> Hi,
>
> Am Mittwoch, 31. Mai 2023, 08:57:23 CEST schrieb Joy Zou:
> > Adding support for pmic pca9451a.
> >
> > This patch support old and new pmic pca9451a. The new pmic trimed
> BUCK1.
> > The default value of Toff_Deb is used to distinguish the old and new pmic.
> >
> > Signed-off-by: Joy Zou <joy.zou@xxxxxxx>
> > ---
> > drivers/regulator/pca9450-regulator.c | 262
> ++++++++++++++++++++++++--
> > include/linux/regulator/pca9450.h | 2 +
> > 2 files changed, 252 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/regulator/pca9450-regulator.c
> > b/drivers/regulator/pca9450-regulator.c index
> > 91bfb7e026c9..654aa4fbe494
> > 100644
> > --- a/drivers/regulator/pca9450-regulator.c
> > +++ b/drivers/regulator/pca9450-regulator.c
> > @@ -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),
> > };
> >
> > /*
> > @@ -663,6 +671,207 @@ static const struct pca9450_regulator_desc
> > pca9450bc_regulators[] = { }, };
> >
> > +static const struct pca9450_regulator_desc pca9451a_regulators[] = {
> > + {
> > + .desc = {
> > + .name = "buck1",
> > + .of_match = of_match_ptr("BUCK1"),
> > + .regulators_node = of_match_ptr("regulators"),
> > + .id = PCA9450_BUCK1,
> > + .ops = &pca9450_dvs_buck_regulator_ops,
> > + .type = REGULATOR_VOLTAGE,
> > + .n_voltages = PCA9450_BUCK1_VOLTAGE_NUM,
> > + .linear_ranges = pca9450_dvs_buck_volts,
> > + .n_linear_ranges =
> ARRAY_SIZE(pca9450_dvs_buck_volts),
> > + .vsel_reg = PCA9450_REG_BUCK1OUT_DVS0,
> > + .vsel_mask = BUCK1OUT_DVS0_MASK,
> > + .enable_reg = PCA9450_REG_BUCK1CTRL,
> > + .enable_mask = BUCK1_ENMODE_MASK,
> > + .enable_val = BUCK_ENMODE_ONREQ,
> > + .ramp_mask = BUCK1_RAMP_MASK,
> > + .ramp_delay_table =
> pca9450_dvs_buck_ramp_table,
> > + .n_ramp_values =
> ARRAY_SIZE(pca9450_dvs_buck_ramp_table),
> > + .owner = THIS_MODULE,
> > + .of_parse_cb = pca9450_set_dvs_levels,
> > + },
> > + .dvs = {
> > + .run_reg = PCA9450_REG_BUCK1OUT_DVS0,
> > + .run_mask = BUCK1OUT_DVS0_MASK,
> > + .standby_reg = PCA9450_REG_BUCK1OUT_DVS1,
> > + .standby_mask = BUCK1OUT_DVS1_MASK,
> > + },
> > + },
> > + {
> > + .desc = {
> > + .name = "buck1_trim",
> > + .of_match = of_match_ptr("BUCK1"),
> > + .regulators_node = of_match_ptr("regulators"),
> > + .id = PCA9450_BUCK1,
> > + .ops = &pca9450_dvs_buck_regulator_ops,
> > + .type = REGULATOR_VOLTAGE,
> > + .n_voltages = PCA9450_BUCK1_VOLTAGE_NUM,
> > + .linear_ranges = pca9450_trim_dvs_buck_volts,
> > + .n_linear_ranges =
> ARRAY_SIZE(pca9450_trim_dvs_buck_volts),
> > + .vsel_reg = PCA9450_REG_BUCK1OUT_DVS0,
> > + .vsel_mask = BUCK1OUT_DVS0_MASK,
> > + .enable_reg = PCA9450_REG_BUCK1CTRL,
> > + .enable_mask = BUCK1_ENMODE_MASK,
> > + .enable_val = BUCK_ENMODE_ONREQ,
> > + .ramp_mask = BUCK1_RAMP_MASK,
> > + .ramp_delay_table =
> pca9450_dvs_buck_ramp_table,
> > + .n_ramp_values =
> ARRAY_SIZE(pca9450_dvs_buck_ramp_table),
> > + .owner = THIS_MODULE,
> > + .of_parse_cb = pca9450_set_dvs_levels,
> > + },
> > + .dvs = {
> > + .run_reg = PCA9450_REG_BUCK1OUT_DVS0,
> > + .run_mask = BUCK1OUT_DVS0_MASK,
> > + .standby_reg = PCA9450_REG_BUCK1OUT_DVS1,
> > + .standby_mask = BUCK1OUT_DVS1_MASK,
> > + },
> > + },
> > + {
> > + .desc = {
> > + .name = "buck2",
> > + .of_match = of_match_ptr("BUCK2"),
> > + .regulators_node = of_match_ptr("regulators"),
> > + .id = PCA9450_BUCK2,
> > + .ops = &pca9450_dvs_buck_regulator_ops,
> > + .type = REGULATOR_VOLTAGE,
> > + .n_voltages = PCA9450_BUCK2_VOLTAGE_NUM,
> > + .linear_ranges = pca9450_dvs_buck_volts,
> > + .n_linear_ranges =
> ARRAY_SIZE(pca9450_dvs_buck_volts),
> > + .vsel_reg = PCA9450_REG_BUCK2OUT_DVS0,
> > + .vsel_mask = BUCK2OUT_DVS0_MASK,
> > + .enable_reg = PCA9450_REG_BUCK2CTRL,
> > + .enable_mask = BUCK2_ENMODE_MASK,
> > + .enable_val = BUCK_ENMODE_ONREQ_STBYREQ,
> > + .ramp_mask = BUCK2_RAMP_MASK,
> > + .ramp_delay_table =
> pca9450_dvs_buck_ramp_table,
> > + .n_ramp_values =
> ARRAY_SIZE(pca9450_dvs_buck_ramp_table),
> > + .owner = THIS_MODULE,
> > + .of_parse_cb = pca9450_set_dvs_levels,
> > + },
> > + .dvs = {
> > + .run_reg = PCA9450_REG_BUCK2OUT_DVS0,
> > + .run_mask = BUCK2OUT_DVS0_MASK,
> > + .standby_reg = PCA9450_REG_BUCK2OUT_DVS1,
> > + .standby_mask = BUCK2OUT_DVS1_MASK,
> > + },
> > + },
> > + {
> > + .desc = {
> > + .name = "buck4",
> > + .of_match = of_match_ptr("BUCK4"),
> > + .regulators_node = of_match_ptr("regulators"),
> > + .id = PCA9450_BUCK4,
> > + .ops = &pca9450_buck_regulator_ops,
> > + .type = REGULATOR_VOLTAGE,
> > + .n_voltages = PCA9450_BUCK4_VOLTAGE_NUM,
> > + .linear_ranges = pca9450_buck_volts,
> > + .n_linear_ranges =
> ARRAY_SIZE(pca9450_buck_volts),
> > + .vsel_reg = PCA9450_REG_BUCK4OUT,
> > + .vsel_mask = BUCK4OUT_MASK,
> > + .enable_reg = PCA9450_REG_BUCK4CTRL,
> > + .enable_mask = BUCK4_ENMODE_MASK,
> > + .enable_val = BUCK_ENMODE_ONREQ,
> > + .owner = THIS_MODULE,
> > + },
> > + },
> > + {
> > + .desc = {
> > + .name = "buck5",
> > + .of_match = of_match_ptr("BUCK5"),
> > + .regulators_node = of_match_ptr("regulators"),
> > + .id = PCA9450_BUCK5,
> > + .ops = &pca9450_buck_regulator_ops,
> > + .type = REGULATOR_VOLTAGE,
> > + .n_voltages = PCA9450_BUCK5_VOLTAGE_NUM,
> > + .linear_ranges = pca9450_buck_volts,
> > + .n_linear_ranges =
> ARRAY_SIZE(pca9450_buck_volts),
> > + .vsel_reg = PCA9450_REG_BUCK5OUT,
> > + .vsel_mask = BUCK5OUT_MASK,
> > + .enable_reg = PCA9450_REG_BUCK5CTRL,
> > + .enable_mask = BUCK5_ENMODE_MASK,
> > + .enable_val = BUCK_ENMODE_ONREQ,
> > + .owner = THIS_MODULE,
> > + },
> > + },
> > + {
> > + .desc = {
> > + .name = "buck6",
> > + .of_match = of_match_ptr("BUCK6"),
> > + .regulators_node = of_match_ptr("regulators"),
> > + .id = PCA9450_BUCK6,
> > + .ops = &pca9450_buck_regulator_ops,
> > + .type = REGULATOR_VOLTAGE,
> > + .n_voltages = PCA9450_BUCK6_VOLTAGE_NUM,
> > + .linear_ranges = pca9450_buck_volts,
> > + .n_linear_ranges =
> ARRAY_SIZE(pca9450_buck_volts),
> > + .vsel_reg = PCA9450_REG_BUCK6OUT,
> > + .vsel_mask = BUCK6OUT_MASK,
> > + .enable_reg = PCA9450_REG_BUCK6CTRL,
> > + .enable_mask = BUCK6_ENMODE_MASK,
> > + .enable_val = BUCK_ENMODE_ONREQ,
> > + .owner = THIS_MODULE,
> > + },
> > + },
> > + {
> > + .desc = {
> > + .name = "ldo1",
> > + .of_match = of_match_ptr("LDO1"),
> > + .regulators_node = of_match_ptr("regulators"),
> > + .id = PCA9450_LDO1,
> > + .ops = &pca9450_ldo_regulator_ops,
> > + .type = REGULATOR_VOLTAGE,
> > + .n_voltages = PCA9450_LDO1_VOLTAGE_NUM,
> > + .linear_ranges = pca9450_ldo1_volts,
> > + .n_linear_ranges =
> ARRAY_SIZE(pca9450_ldo1_volts),
> > + .vsel_reg = PCA9450_REG_LDO1CTRL,
> > + .vsel_mask = LDO1OUT_MASK,
> > + .enable_reg = PCA9450_REG_LDO1CTRL,
> > + .enable_mask = LDO1_EN_MASK,
> > + .owner = THIS_MODULE,
> > + },
> > + },
> > + {
> > + .desc = {
> > + .name = "ldo4",
> > + .of_match = of_match_ptr("LDO4"),
> > + .regulators_node = of_match_ptr("regulators"),
> > + .id = PCA9450_LDO4,
> > + .ops = &pca9450_ldo_regulator_ops,
> > + .type = REGULATOR_VOLTAGE,
> > + .n_voltages = PCA9450_LDO4_VOLTAGE_NUM,
> > + .linear_ranges = pca9450_ldo34_volts,
> > + .n_linear_ranges =
> ARRAY_SIZE(pca9450_ldo34_volts),
> > + .vsel_reg = PCA9450_REG_LDO4CTRL,
> > + .vsel_mask = LDO4OUT_MASK,
> > + .enable_reg = PCA9450_REG_LDO4CTRL,
> > + .enable_mask = LDO4_EN_MASK,
> > + .owner = THIS_MODULE,
> > + },
> > + },
> > + {
> > + .desc = {
> > + .name = "ldo5",
> > + .of_match = of_match_ptr("LDO5"),
> > + .regulators_node = of_match_ptr("regulators"),
> > + .id = PCA9450_LDO5,
> > + .ops = &pca9450_ldo_regulator_ops,
> > + .type = REGULATOR_VOLTAGE,
> > + .n_voltages = PCA9450_LDO5_VOLTAGE_NUM,
> > + .linear_ranges = pca9450_ldo5_volts,
> > + .n_linear_ranges =
> ARRAY_SIZE(pca9450_ldo5_volts),
> > + .vsel_reg = PCA9450_REG_LDO5CTRL_H,
> > + .vsel_mask = LDO5HOUT_MASK,
> > + .enable_reg = PCA9450_REG_LDO5CTRL_H,
> > + .enable_mask = LDO5H_EN_MASK,
> > + .owner = THIS_MODULE,
> > + },
> > + },
> > +};
> > +
> > static irqreturn_t pca9450_irq_handler(int irq, void *data) {
> > struct pca9450 *pca9450 = data;
> > @@ -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. 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.
>
> > +
> > switch (type) {
> > case PCA9450_TYPE_PCA9450A:
> > regulator_desc = pca9450a_regulators; @@ -730,6 +956,10
> > @@ static int pca9450_i2c_probe(struct i2c_client *i2c)
> > regulator_desc = pca9450bc_regulators;
> > pca9450->rcnt = ARRAY_SIZE(pca9450bc_regulators);
> > break;
> > + case PCA9450_TYPE_PCA9451A:
> > + regulator_desc = pca9451a_regulators;
> > + pca9450->rcnt = ARRAY_SIZE(pca9451a_regulators);
> > + break;
> > default:
> > dev_err(&i2c->dev, "Unknown device type");
> > return -EINVAL;
> > @@ -741,13 +971,6 @@ static int pca9450_i2c_probe(struct i2c_client
> > *i2c)
> >
> > dev_set_drvdata(&i2c->dev, pca9450);
> >
> > - 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_DEV_ID,
> &device_id);
> > if (ret) {
> > dev_err(&i2c->dev, "Read device id error\n"); @@ -756,7
> > +979,8 @@ static int pca9450_i2c_probe(struct i2c_client *i2c)
> >
> > /* Check your board and dts for match the right pmic */
> > if (((device_id >> 4) != 0x1 && type == PCA9450_TYPE_PCA9450A) ||
> > - ((device_id >> 4) != 0x3 && type == PCA9450_TYPE_PCA9450BC))
> {
> > + ((device_id >> 4) != 0x3 && type == PCA9450_TYPE_PCA9450BC)
> ||
> > + ((device_id >> 4) != 0x9 && type == PCA9450_TYPE_PCA9451A))
> > + {
> > dev_err(&i2c->dev, "Device id(%x) mismatched\n",
> > device_id >> 4);
> > return -EINVAL;
> > @@ -767,7 +991,16 @@ static int pca9450_i2c_probe(struct i2c_client *i2c)
> > struct regulator_dev *rdev;
> > const struct pca9450_regulator_desc *r;
> >
> > - r = &regulator_desc[i];
> > + if (type == PCA9450_TYPE_PCA9451A &&
> > + !strcmp((&regulator_desc[i])->desc.name, "buck1") &&
> pmic_trim) {
> > + r = &regulator_desc[i + 1];
> > + i = i + 1;
> > + } else if (type == PCA9450_TYPE_PCA9451A &&
> > + !strcmp((&regulator_desc[i])->desc.name,
> "buck1")) {
> > + r = &regulator_desc[i];
> > + i = i + 1;
>
> I would put this in a single 'type == PCA9450_TYPE_PCA9451A' branch, to
> indicate that only PCA9451A needs some kind of special handling.
Yes, this makes the logic clearer. I will change in next version.
>
> > + } else
> > + r = &regulator_desc[i];
> > desc = &r->desc;
> >
> > config.regmap = pca9450->regmap; @@ -847,7 +1080,8
> @@
> > static int pca9450_i2c_probe(struct i2c_client *i2c)
> > }
> >
> > dev_info(&i2c->dev, "%s probed.\n",
> > - type == PCA9450_TYPE_PCA9450A ? "pca9450a" :
> "pca9450bc");
> > + type == PCA9450_TYPE_PCA9450A ? "pca9450a" :
> > + (type == PCA9450_TYPE_PCA9451A ? "pca9451a" :
> "pca9450bc"));
> >
> > return 0;
> > }
> > @@ -865,6 +1099,10 @@ static const struct of_device_id
> > pca9450_of_match[] = { .compatible = "nxp,pca9450c",
> > .data = (void *)PCA9450_TYPE_PCA9450BC,
> > },
> > + {
> > + .compatible = "nxp,pca9451a",
> > + .data = (void *)PCA9450_TYPE_PCA9451A,
> > + },
> > { }
> > };
> > MODULE_DEVICE_TABLE(of, pca9450_of_match); diff --git
> > a/include/linux/regulator/pca9450.h
> > b/include/linux/regulator/pca9450.h index 3c01c2bf84f5..5dd79f52165a
> > 100644
> > --- a/include/linux/regulator/pca9450.h
> > +++ b/include/linux/regulator/pca9450.h
> > @@ -9,6 +9,7 @@
> > enum pca9450_chip_type {
> > PCA9450_TYPE_PCA9450A = 0,
> > PCA9450_TYPE_PCA9450BC,
> > + PCA9450_TYPE_PCA9451A,
> > PCA9450_TYPE_AMOUNT,
> > };
> >
> > @@ -93,6 +94,7 @@ enum {
> > PCA9450_MAX_REGISTER = 0x2F,
> > };
> >
> > +#define PCA9450_REG_PWRCTRL_TOFF_DEB BIT(5)
>
> Newline here please.
Ok, will modify it.
Thank you very much for your comments and efforts.
BR
Joy Zou
>
> Best regards,
> Alexander
>
> > /* PCA9450 BUCK ENMODE bits */
> > #define BUCK_ENMODE_OFF 0x00
> > #define BUCK_ENMODE_ONREQ 0x01
>
>
> --
> 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%2F&data=05%7C01%7Cjoy.zou%40nxp.com%7C1ecbe75c432d464
> cb45f08db61cb08bc%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7
> C638211296883052738%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw
> MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%
> 7C&sdata=mz7vtBiDvAxY9FO7tTZO3oPBA8zvYS6f8hLgAwrU8uk%3D&reserved
> =0
>

Next message: Mauro Carvalho Chehab: "[GIT PULL for v6.5-rc1] media updates"
Previous message: Yong Wu (吴勇): "Re: [PATCH] iommu/mediatek: Remove a unnecessary checking for larbid"
Next in thread: Alexander Stein: "Re: [PATCH v1 2/3] regulator: pca9450: add pca9451a support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]