Re: [PATCH 3/3] hwmon: (pmbus/tda38640) Add workaround for bug in SVID mode

From: Naresh Solanki
Date: Thu Jul 27 2023 - 04:49:30 EST

Hi Guenter,


On Wed, 26 Jul 2023 at 19:49, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
> On 7/26/23 05:22, Naresh Solanki wrote:
> > Hi Guenter,
> >
> > On 25-07-2023 07:51 pm, Guenter Roeck wrote:
> >> On 7/25/23 04:40, Naresh Solanki wrote:
> >>> From: Patrick Rudolph <patrick.rudolph@xxxxxxxxxxxxx>
> >>>
> >>> The TDA38640 supports two operating modes to set the output voltage:
> >>> - PMBUS
> >>> - SVID
> >>>
> >>> Due to an undocumented bug the regulator cannot be enabled using BIT7
> >>> of OPERATION(01h) register when in SVID mode.
> >>> It works when in PMBUS mode. In SVID mode the regulator only cares
> >>> about the ENABLE pin.
> >>>
> >>> Add a workaround that needs the ENABLE pin to be left floating or
> >>> tied to a fixed level. The DT must contain the newly introduced
> >>> property 'infineon,en-pin-fixed-level' to enable this workaround.
> >>>
> >>
> >> Why is that property even needed ? Isn't the workaround always (and only)
> >> required if the chip is in SVID mode ?
> > Will add below function to detect SVID mode.
> > static bool svid_mode(struct i2c_client *client)
> > {
> > /* PMBUS_MFR_READ(0xD0) + Address */
> > u8 write_buf[] = {0xd0, 0x44, 0x00};
> > u8 read_buf[2];
> > int ret;
> >
> > struct i2c_msg msgs[2] = {
> > {
> > .addr = client->addr,
> > .flags = 0,
> > .buf = write_buf,
> > .len = sizeof(write_buf),
> > },
> > {
> > .addr = client->addr,
> > .flags = I2C_M_RD,
> > .buf = read_buf,
> > .len = sizeof(read_buf),
> > }
> > };
> >
> > ret = i2c_transfer(client->adapter, msgs, 2);
> >
> drop empty line
Sure
>
> > if (ret < 0) {
> > dev_err(&client->dev, "%s:%d i2c_transfer failed. %d", __func__, __LINE__, ret);
> > return ret;
>
> Return type is bool.
Yes. I've changed it to int so that return value can be validated.
>
> > }
> > /* 0x44[15] determines PMBus Operating Mode */
> > return !!(read_buf[1] & BIT(7));
> > }
> >
>
> "The application note AN_2203_PL12_2204_210835 having information on the register map
> of TDA38640 is under review. The document will be uploaded to the Infineon website
> once the review is completed."
>
> How annoying. Is that really the only way to get that information ?
Datasheet does mention MTP register offset 0x44 bit 15 for PMBUS operating mode.
I validated those in my setup which has 4 tda38640 in SVID mode & 7 in
PMBUS mode.
>
> > Based on svid_mode will init accordingly:
> > if (!IS_ENABLED(CONFIG_SENSORS_TDA38640_REGULATOR) || !np || !svid_mode(client))
> > return pmbus_do_probe(client, &data->info);
> >
>
> This is unnecessary complexity. Just add the local read/write
> commands and be done with it.
>
> if (svid_mode(client)) {
> data->info.read_byte_data = tda38640_read_byte_data;
> data->info.write_byte_data = tda38640_write_byte_data;
> }
>
> though it would be useful to error check the return value.
>
> ret = svid_mode();
> if (ret < 0)
> return ret;
> if (ret) {
> /* consider adding comments here */
> data->info.read_byte_data = tda38640_read_byte_data;
> data->info.write_byte_data = tda38640_write_byte_data;
> }
Yes. I've updated accordingly.

Regards,
Naresh
>
> Guenter
>
> > dev_dbg(&client->dev, "SVID mode");
> >
> > Regards,
> > Naresh
> >>
> >> Guenter
> >>
> >>> The workaround will map the PB_OPERATION_CONTROL_ON bit to the
> >>> PB_ON_OFF_CONFIG_EN_PIN_REQ bit of the ON_OFF_CONFIG register.
> >>> In combination with the fixed level on the ENABLE pin the regulator
> >>> can be controlled as expected.
> >>>
> >>> Signed-off-by: Patrick Rudolph <patrick.rudolph@xxxxxxxxxxxxx>
> >>> Signed-off-by: Naresh Solanki <Naresh.Solanki@xxxxxxxxxxxxx>
> >>> ---
> >>> drivers/hwmon/pmbus/tda38640.c | 95 +++++++++++++++++++++++++++++++++-
> >>> 1 file changed, 93 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/hwmon/pmbus/tda38640.c b/drivers/hwmon/pmbus/tda38640.c
> >>> index 450b0273fb59..9d3b89d9845c 100644
> >>> --- a/drivers/hwmon/pmbus/tda38640.c
> >>> +++ b/drivers/hwmon/pmbus/tda38640.c
> >>> @@ -18,6 +18,72 @@ static const struct regulator_desc __maybe_unused tda38640_reg_desc[] = {
> >>> PMBUS_REGULATOR("vout", 0),
> >>> };
> >>> +struct tda38640_data {
> >>> + struct pmbus_driver_info info;
> >>> + u32 en_pin_lvl;
> >>> +};
> >>> +
> >>> +#define to_tda38640_data(x) container_of(x, struct tda38640_data, info)
> >>> +
> >>> +/*
> >>> + * Map PB_ON_OFF_CONFIG_POLARITY_HIGH to PB_OPERATION_CONTROL_ON.
> >>> + */
> >>> +static int tda38640_read_byte_data(struct i2c_client *client, int page, int reg)
> >>> +{
> >>> + const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
> >>> + struct tda38640_data *data = to_tda38640_data(info);
> >>> + int ret, on_off_config, enabled;
> >>> +
> >>> + if (reg != PMBUS_OPERATION)
> >>> + return -ENODATA;
> >>> +
> >>> + ret = pmbus_read_byte_data(client, page, reg);
> >>> + if (ret < 0)
> >>> + return ret;
> >>> +
> >>> + on_off_config = pmbus_read_byte_data(client, page,
> >>> + PMBUS_ON_OFF_CONFIG);
> >>> + if (on_off_config < 0)
> >>> + return on_off_config;
> >>> +
> >>> + enabled = !!(on_off_config & PB_ON_OFF_CONFIG_POLARITY_HIGH);
> >>> +
> >>> + enabled ^= data->en_pin_lvl;
> >>> + if (enabled)
> >>> + ret &= ~PB_OPERATION_CONTROL_ON;
> >>> + else
> >>> + ret |= PB_OPERATION_CONTROL_ON;
> >>> +
> >>> + return ret;
> >>> +}
> >>> +
> >>> +/*
> >>> + * Map PB_OPERATION_CONTROL_ON to PB_ON_OFF_CONFIG_POLARITY_HIGH.
> >>> + */
> >>> +static int tda38640_write_byte_data(struct i2c_client *client, int page,
> >>> + int reg, u8 byte)
> >>> +{
> >>> + const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
> >>> + struct tda38640_data *data = to_tda38640_data(info);
> >>> + int enable, ret;
> >>> +
> >>> + if (reg != PMBUS_OPERATION)
> >>> + return -ENODATA;
> >>> +
> >>> + enable = !!(byte & PB_OPERATION_CONTROL_ON);
> >>> +
> >>> + byte &= ~PB_OPERATION_CONTROL_ON;
> >>> + ret = pmbus_write_byte_data(client, page, reg, byte);
> >>> + if (ret < 0)
> >>> + return ret;
> >>> +
> >>> + enable ^= data->en_pin_lvl;
> >>> +
> >>> + return pmbus_update_byte_data(client, page, PMBUS_ON_OFF_CONFIG,
> >>> + PB_ON_OFF_CONFIG_POLARITY_HIGH,
> >>> + enable ? 0 : PB_ON_OFF_CONFIG_POLARITY_HIGH);
> >>> +}
> >>> +
> >>> static struct pmbus_driver_info tda38640_info = {
> >>> .pages = 1,
> >>> .format[PSC_VOLTAGE_IN] = linear,
> >>> @@ -26,7 +92,6 @@ static struct pmbus_driver_info tda38640_info = {
> >>> .format[PSC_CURRENT_IN] = linear,
> >>> .format[PSC_POWER] = linear,
> >>> .format[PSC_TEMPERATURE] = linear,
> >>> -
> >>> .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_STATUS_INPUT
> >>> | PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP
> >>> | PMBUS_HAVE_IIN
> >>> @@ -41,7 +106,33 @@ static struct pmbus_driver_info tda38640_info = {
> >>> static int tda38640_probe(struct i2c_client *client)
> >>> {
> >>> - return pmbus_do_probe(client, &tda38640_info);
> >>> + struct device *dev = &client->dev;
> >>> + struct device_node *np = dev_of_node(dev);
> >>> + struct tda38640_data *data;
> >>> +
> >>> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> >>> + if (!data)
> >>> + return -ENOMEM;
> >>> + memcpy(&data->info, &tda38640_info, sizeof(tda38640_info));
> >>> +
> >>> + if (!CONFIG_SENSORS_TDA38640_REGULATOR || !np ||
> >>> + of_property_read_u32(np, "infineon,en-pin-fixed-level", &data->en_pin_lvl))
> >>> + return pmbus_do_probe(client, &data->info);
> >>> +
> >>> + /*
> >>> + * Apply ON_OFF_CONFIG workaround as enabling the regulator using the
> >>> + * OPERATION register doesn't work in SVID mode.
> >>> + */
> >>> + data->info.read_byte_data = tda38640_read_byte_data;
> >>> + data->info.write_byte_data = tda38640_write_byte_data;
> >>> + /*
> >>> + * One should configure PMBUS_ON_OFF_CONFIG here, but
> >>> + * PB_ON_OFF_CONFIG_POWERUP_CONTROL, PB_ON_OFF_CONFIG_EN_PIN_REQ and
> >>> + * PB_ON_OFF_CONFIG_EN_PIN_REQ are ignored by the device.
> >>> + * Only PB_ON_OFF_CONFIG_POLARITY_HIGH has an effect.
> >>> + */
> >>> +
> >>> + return pmbus_do_probe(client, &data->info);
> >>> }
> >>> static const struct i2c_device_id tda38640_id[] = {
> >>
>