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;
Sender: Guenter Roeck <groeck7@gmail.com>
Message-ID: <46f3f44b-5c25-8193-70b5-59303be04ad0@roeck-us.net>
Date:   Tue, 25 Jul 2023 07:21:09 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH 3/3] hwmon: (pmbus/tda38640) Add workaround for bug in
 SVID mode
Content-Language: en-US
To:     Naresh Solanki <naresh.solanki@9elements.com>,
        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>
From:   Guenter Roeck <linux@roeck-us.net>
In-Reply-To: <20230725114030.1860571-3-Naresh.Solanki@9elements.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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 ?

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