[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <331889b5-caf7-ab68-fea2-6566e550d7e8@9elements.com>
Date: Wed, 26 Jul 2023 17:52:52 +0530
From: Naresh Solanki <naresh.solanki@...ements.com>
To: Guenter Roeck <linux@...ck-us.net>,
Jean Delvare <jdelvare@...e.com>,
krzysztof.kozlowski+dt@...aro.org
Cc: linux-hwmon@...r.kernel.org,
Patrick Rudolph <patrick.rudolph@...ements.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] hwmon: (pmbus/tda38640) Add workaround for bug in
SVID mode
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@...ements.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);
if (ret < 0) {
dev_err(&client->dev, "%s:%d i2c_transfer failed. %d", __func__,
__LINE__, ret);
return ret;
}
/* 0x44[15] determines PMBus Operating Mode */
return !!(read_buf[1] & BIT(7));
}
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);
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@...ements.com>
>> Signed-off-by: Naresh Solanki <Naresh.Solanki@...ements.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[] = {
>
Powered by blists - more mailing lists