Hello, Am Mittwoch, 5. Juli 2023, 08:50:24 CEST schrieb Joy Zou: > > > -----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. There is only one part mentioned in the ordering options. How can I distinguish them? Any chip ID, date codes, markings? > 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? Best regards, Alexander > > > + > > > > > > 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 = ®ulator_desc[i]; > > > + if (type == PCA9450_TYPE_PCA9451A && > > > + !strcmp((®ulator_desc[i])->desc.name, "buck1") && > > > > pmic_trim) { > > > > > + r = ®ulator_desc[i + 1]; > > > + i = i + 1; > > > + } else if (type == PCA9450_TYPE_PCA9451A && > > > + !strcmp((®ulator_desc[i])->desc.name, > > > > "buck1")) { > > > > > + r = ®ulator_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 = ®ulator_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 > > > > > > -- 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/