Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230725114030.1860571-1-Naresh.Solanki@9elements.com>
 <20230725114030.1860571-3-Naresh.Solanki@9elements.com> <46f3f44b-5c25-8193-70b5-59303be04ad0@roeck-us.net>
 <331889b5-caf7-ab68-fea2-6566e550d7e8@9elements.com> <a8b53088-70a0-f693-2081-4b9576ac492f@roeck-us.net>
In-Reply-To: <a8b53088-70a0-f693-2081-4b9576ac492f@roeck-us.net>
From:   Naresh Solanki <naresh.solanki@9elements.com>
Date:   Thu, 27 Jul 2023 14:00:31 +0530
Message-ID: <CABqG17h8mcdp8t4Zh2=XrvRPQp_862i_pnp0Jx=g8dyftda7oA@mail.gmail.com>
Subject: Re: [PATCH 3/3] hwmon: (pmbus/tda38640) Add workaround for bug in
 SVID mode
To:     Guenter Roeck <linux@roeck-us.net>
Cc:     Jean Delvare <jdelvare@suse.com>,
        krzysztof.kozlowski+dt@linaro.org, linux-hwmon@vger.kernel.org,
        Patrick Rudolph <patrick.rudolph@9elements.com>,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Guenter,


On Wed, 26 Jul 2023 at 19:49, Guenter Roeck <linux@roeck-us.net> 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@9elements.com>
> >>>
> >>> 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@9elements.com>
> >>> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> >>> ---
> >>>   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[] = {
> >>
>