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;
Message-ID: <331889b5-caf7-ab68-fea2-6566e550d7e8@9elements.com>
Date:   Wed, 26 Jul 2023 17:52:52 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.1
Subject: Re: [PATCH 3/3] hwmon: (pmbus/tda38640) Add workaround for bug in
 SVID mode
To:     Guenter Roeck <linux@roeck-us.net>,
        Jean Delvare <jdelvare@suse.com>,
        krzysztof.kozlowski+dt@linaro.org
Cc:     linux-hwmon@vger.kernel.org,
        Patrick Rudolph <patrick.rudolph@9elements.com>,
        linux-kernel@vger.kernel.org
References: <20230725114030.1860571-1-Naresh.Solanki@9elements.com>
 <20230725114030.1860571-3-Naresh.Solanki@9elements.com>
 <46f3f44b-5c25-8193-70b5-59303be04ad0@roeck-us.net>
Content-Language: en-US
From:   Naresh Solanki <naresh.solanki@9elements.com>
In-Reply-To: <46f3f44b-5c25-8193-70b5-59303be04ad0@roeck-us.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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);

	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@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[] = {
>