[PATCH 1/3] dt-bindings: hwmon: Add Infineon TDA38640

[PATCH 1/3] dt-bindings: hwmon: Add Infineon TDA38640

[Naresh Solanki]

From: Patrick Rudolph <patrick.rudolph@9elements.com>

The TDA38640 has a bug in SVID mode and to enable a workaround
remove the TDA38640 from trivial-devices and add a complete schema.

The schema adds the custom property 'infineon,en-pin-fixed-level' to
signal a fixed level on the ENABLE pin and to enable the workaround.
When the ENABLE pin is left floating it's internally pulled low.

If not specified the driver will continue to use the PMBUS_OPERATION
register to enable the regulator. When specified the driver will use
the PMBUS_ON_OFF_CONFIG register to enable the regulator.

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
---
 .../hwmon/pmbus/infineon,tda38640.yaml        | 50 +++++++++++++++++++
 .../devicetree/bindings/trivial-devices.yaml  |  2 -
 2 files changed, 50 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml
new file mode 100644
index 000000000000..520112e4e271
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+
+$id: http://devicetree.org/schemas/hwmon/pmbus/infineon,tda38640.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Infineon TDA38640 Synchronous Buck Regulator with SVID and I2C
+
+description: |
+  The Infineon TDA38640 is a 40A Single-voltage Synchronous Buck
+  Regulator with SVID and I2C designed for Industrial use.
+
+  Datasheet: https://www.infineon.com/dgdl/Infineon-TDA38640-0000-DataSheet-v02_04-EN.pdf?fileId=8ac78c8c80027ecd018042f2337f00c9
+
+properties:
+  compatible:
+    enum:
+      - infineon,tda38640
+
+  reg:
+    maxItems: 1
+
+  infineon,en-pin-fixed-level:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: |
+      Fixed level of the ENABLE pin. When specified the PMBUS_ON_OFF_CONFIG
+      register is used to enable the regulator instead of the PMBUS_OPERATION
+      register to workaround a bug of the tda38640 when operating in SVID-mode.
+      If the ENABLE pin is left floating the internal pull-down causes a low
+      level on the pin.
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        tda38640@40 {
+            compatible = "infineon,tda38640";
+            reg = <0x40>;
+        };
+    };
+
diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index 6e24c4d25ec3..2b1fbb2a672b 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -151,8 +151,6 @@ properties:
           - infineon,slb9645tt
             # Infineon SLB9673 I2C TPM 2.0
           - infineon,slb9673
-            # Infineon TDA38640 Voltage Regulator
-          - infineon,tda38640
             # Infineon TLV493D-A1B6 I2C 3D Magnetic Sensor
           - infineon,tlv493d-a1b6
             # Infineon Multi-phase Digital VR Controller xdpe11280

base-commit: 55612007f16b5d7b1fb83a7b0f5bb686829db7c7
-- 
2.41.0

[PATCH 2/3] hwmon: (pmbus) Add ON_OFF_CONFIG register bits

[Naresh Solanki]

From: Patrick Rudolph <patrick.rudolph@9elements.com>

Add bits found in the ON_OFF_CONFIG register.

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
---
 drivers/hwmon/pmbus/pmbus.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index b0832a4c690d..7a28bac7f171 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -243,6 +243,15 @@ enum pmbus_regs {
  */
 #define PB_OPERATION_CONTROL_ON		BIT(7)
 
+/*
+ * ON_OFF_CONFIG
+ */
+#define PB_ON_OFF_CONFIG_POWERUP_CONTROL	BIT(4)
+#define PB_ON_OFF_CONFIG_OPERATION_REQ		BIT(3)
+#define PB_ON_OFF_CONFIG_EN_PIN_REQ		BIT(2)
+#define PB_ON_OFF_CONFIG_POLARITY_HIGH		BIT(1)
+#define PB_ON_OFF_CONFIG_TURN_OFF_FAST		BIT(0)
+
 /*
  * WRITE_PROTECT
  */
-- 
2.41.0

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

[Naresh Solanki]

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.

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

Re: [PATCH 1/3] dt-bindings: hwmon: Add Infineon TDA38640

[Rob Herring]

On Tue, 25 Jul 2023 13:40:26 +0200, Naresh Solanki wrote:
> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> 
> The TDA38640 has a bug in SVID mode and to enable a workaround
> remove the TDA38640 from trivial-devices and add a complete schema.
> 
> The schema adds the custom property 'infineon,en-pin-fixed-level' to
> signal a fixed level on the ENABLE pin and to enable the workaround.
> When the ENABLE pin is left floating it's internally pulled low.
> 
> If not specified the driver will continue to use the PMBUS_OPERATION
> register to enable the regulator. When specified the driver will use
> the PMBUS_ON_OFF_CONFIG register to enable the regulator.
> 
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> ---
>  .../hwmon/pmbus/infineon,tda38640.yaml        | 50 +++++++++++++++++++
>  .../devicetree/bindings/trivial-devices.yaml  |  2 -
>  2 files changed, 50 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml: 'maintainers' is a required property
	hint: Metaschema for devicetree binding documentation
	from schema $id: http://devicetree.org/meta-schemas/core.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230725114030.1860571-1-Naresh.Solanki@9elements.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 1/3] dt-bindings: hwmon: Add Infineon TDA38640

[Rob Herring]

On Tue, Jul 25, 2023 at 01:40:26PM +0200, Naresh Solanki wrote:
> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> 
> The TDA38640 has a bug in SVID mode and to enable a workaround
> remove the TDA38640 from trivial-devices and add a complete schema.
> 
> The schema adds the custom property 'infineon,en-pin-fixed-level' to
> signal a fixed level on the ENABLE pin and to enable the workaround.
> When the ENABLE pin is left floating it's internally pulled low.
> 
> If not specified the driver will continue to use the PMBUS_OPERATION
> register to enable the regulator. When specified the driver will use
> the PMBUS_ON_OFF_CONFIG register to enable the regulator.
> 
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> ---
>  .../hwmon/pmbus/infineon,tda38640.yaml        | 50 +++++++++++++++++++
>  .../devicetree/bindings/trivial-devices.yaml  |  2 -
>  2 files changed, 50 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml
> new file mode 100644
> index 000000000000..520112e4e271
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml
> @@ -0,0 +1,50 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +
> +$id: http://devicetree.org/schemas/hwmon/pmbus/infineon,tda38640.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Infineon TDA38640 Synchronous Buck Regulator with SVID and I2C
> +
> +description: |
> +  The Infineon TDA38640 is a 40A Single-voltage Synchronous Buck
> +  Regulator with SVID and I2C designed for Industrial use.
> +
> +  Datasheet: https://www.infineon.com/dgdl/Infineon-TDA38640-0000-DataSheet-v02_04-EN.pdf?fileId=8ac78c8c80027ecd018042f2337f00c9
> +
> +properties:
> +  compatible:
> +    enum:
> +      - infineon,tda38640
> +
> +  reg:
> +    maxItems: 1
> +
> +  infineon,en-pin-fixed-level:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      Fixed level of the ENABLE pin. When specified the PMBUS_ON_OFF_CONFIG
> +      register is used to enable the regulator instead of the PMBUS_OPERATION
> +      register to workaround a bug of the tda38640 when operating in SVID-mode.
> +      If the ENABLE pin is left floating the internal pull-down causes a low
> +      level on the pin.

Neither this nor the commit message answers how do I decide if I set 
this property or not? How you work-around it is not that relevant to the 
binding.

> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        tda38640@40 {
> +            compatible = "infineon,tda38640";
> +            reg = <0x40>;
> +        };
> +    };
> +
> diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
> index 6e24c4d25ec3..2b1fbb2a672b 100644
> --- a/Documentation/devicetree/bindings/trivial-devices.yaml
> +++ b/Documentation/devicetree/bindings/trivial-devices.yaml
> @@ -151,8 +151,6 @@ properties:
>            - infineon,slb9645tt
>              # Infineon SLB9673 I2C TPM 2.0
>            - infineon,slb9673
> -            # Infineon TDA38640 Voltage Regulator
> -          - infineon,tda38640
>              # Infineon TLV493D-A1B6 I2C 3D Magnetic Sensor
>            - infineon,tlv493d-a1b6
>              # Infineon Multi-phase Digital VR Controller xdpe11280
> 
> base-commit: 55612007f16b5d7b1fb83a7b0f5bb686829db7c7
> -- 
> 2.41.0
> 

Re: [PATCH 1/3] dt-bindings: hwmon: Add Infineon TDA38640

[Naresh Solanki]

Hi Rob,


On Tue, 25 Jul 2023 at 18:40, Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Jul 25, 2023 at 01:40:26PM +0200, Naresh Solanki wrote:
> > From: Patrick Rudolph <patrick.rudolph@9elements.com>
> >
> > The TDA38640 has a bug in SVID mode and to enable a workaround
> > remove the TDA38640 from trivial-devices and add a complete schema.
> >
> > The schema adds the custom property 'infineon,en-pin-fixed-level' to
> > signal a fixed level on the ENABLE pin and to enable the workaround.
> > When the ENABLE pin is left floating it's internally pulled low.
> >
> > If not specified the driver will continue to use the PMBUS_OPERATION
> > register to enable the regulator. When specified the driver will use
> > the PMBUS_ON_OFF_CONFIG register to enable the regulator.
> >
> > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> > ---
> >  .../hwmon/pmbus/infineon,tda38640.yaml        | 50 +++++++++++++++++++
> >  .../devicetree/bindings/trivial-devices.yaml  |  2 -
> >  2 files changed, 50 insertions(+), 2 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml
> > new file mode 100644
> > index 000000000000..520112e4e271
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml
> > @@ -0,0 +1,50 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +
> > +$id: http://devicetree.org/schemas/hwmon/pmbus/infineon,tda38640.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Infineon TDA38640 Synchronous Buck Regulator with SVID and I2C
> > +
> > +description: |
> > +  The Infineon TDA38640 is a 40A Single-voltage Synchronous Buck
> > +  Regulator with SVID and I2C designed for Industrial use.
> > +
> > +  Datasheet: https://www.infineon.com/dgdl/Infineon-TDA38640-0000-DataSheet-v02_04-EN.pdf?fileId=8ac78c8c80027ecd018042f2337f00c9
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - infineon,tda38640
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  infineon,en-pin-fixed-level:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: |
> > +      Fixed level of the ENABLE pin. When specified the PMBUS_ON_OFF_CONFIG
> > +      register is used to enable the regulator instead of the PMBUS_OPERATION
> > +      register to workaround a bug of the tda38640 when operating in SVID-mode.
> > +      If the ENABLE pin is left floating the internal pull-down causes a low
> > +      level on the pin.
>
> Neither this nor the commit message answers how do I decide if I set
> this property or not? How you work-around it is not that relevant to the
> binding.
Sure will update this as:
The property becomes relevant when dealing with the tda38640 in
SVID-mode, providing an alternative method to enable the regulator by
using the PMBUS_ON_OFF_CONFIG register instead of the PMBUS_OPERATION
register

Regards,
Naresh
>
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        tda38640@40 {
> > +            compatible = "infineon,tda38640";
> > +            reg = <0x40>;
> > +        };
> > +    };
> > +
> > diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
> > index 6e24c4d25ec3..2b1fbb2a672b 100644
> > --- a/Documentation/devicetree/bindings/trivial-devices.yaml
> > +++ b/Documentation/devicetree/bindings/trivial-devices.yaml
> > @@ -151,8 +151,6 @@ properties:
> >            - infineon,slb9645tt
> >              # Infineon SLB9673 I2C TPM 2.0
> >            - infineon,slb9673
> > -            # Infineon TDA38640 Voltage Regulator
> > -          - infineon,tda38640
> >              # Infineon TLV493D-A1B6 I2C 3D Magnetic Sensor
> >            - infineon,tlv493d-a1b6
> >              # Infineon Multi-phase Digital VR Controller xdpe11280
> >
> > base-commit: 55612007f16b5d7b1fb83a7b0f5bb686829db7c7
> > --
> > 2.41.0
> >

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

[Guenter Roeck]

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

Re: [PATCH 1/3] dt-bindings: hwmon: Add Infineon TDA38640

[Guenter Roeck]

On 7/25/23 07:17, Naresh Solanki wrote:
> Hi Rob,
> 
> 
> On Tue, 25 Jul 2023 at 18:40, Rob Herring <robh@kernel.org> wrote:
>>
>> On Tue, Jul 25, 2023 at 01:40:26PM +0200, Naresh Solanki wrote:
>>> From: Patrick Rudolph <patrick.rudolph@9elements.com>
>>>
>>> The TDA38640 has a bug in SVID mode and to enable a workaround
>>> remove the TDA38640 from trivial-devices and add a complete schema.
>>>
>>> The schema adds the custom property 'infineon,en-pin-fixed-level' to
>>> signal a fixed level on the ENABLE pin and to enable the workaround.
>>> When the ENABLE pin is left floating it's internally pulled low.
>>>
>>> If not specified the driver will continue to use the PMBUS_OPERATION
>>> register to enable the regulator. When specified the driver will use
>>> the PMBUS_ON_OFF_CONFIG register to enable the regulator.
>>>
>>> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
>>> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
>>> ---
>>>   .../hwmon/pmbus/infineon,tda38640.yaml        | 50 +++++++++++++++++++
>>>   .../devicetree/bindings/trivial-devices.yaml  |  2 -
>>>   2 files changed, 50 insertions(+), 2 deletions(-)
>>>   create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml
>>> new file mode 100644
>>> index 000000000000..520112e4e271
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml
>>> @@ -0,0 +1,50 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +
>>> +$id: http://devicetree.org/schemas/hwmon/pmbus/infineon,tda38640.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Infineon TDA38640 Synchronous Buck Regulator with SVID and I2C
>>> +
>>> +description: |
>>> +  The Infineon TDA38640 is a 40A Single-voltage Synchronous Buck
>>> +  Regulator with SVID and I2C designed for Industrial use.
>>> +
>>> +  Datasheet: https://www.infineon.com/dgdl/Infineon-TDA38640-0000-DataSheet-v02_04-EN.pdf?fileId=8ac78c8c80027ecd018042f2337f00c9
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - infineon,tda38640
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  infineon,en-pin-fixed-level:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    description: |
>>> +      Fixed level of the ENABLE pin. When specified the PMBUS_ON_OFF_CONFIG
>>> +      register is used to enable the regulator instead of the PMBUS_OPERATION
>>> +      register to workaround a bug of the tda38640 when operating in SVID-mode.
>>> +      If the ENABLE pin is left floating the internal pull-down causes a low
>>> +      level on the pin.
>>
>> Neither this nor the commit message answers how do I decide if I set
>> this property or not? How you work-around it is not that relevant to the
>> binding.
> Sure will update this as:
> The property becomes relevant when dealing with the tda38640 in
> SVID-mode, providing an alternative method to enable the regulator by
> using the PMBUS_ON_OFF_CONFIG register instead of the PMBUS_OPERATION
> register
> 

As mentioned in the other e-mail, I'll want to see an explanation why
this property is even needed. The series claims that the chip has a bug
in SVID mode. It is kind of unusual that we would enable a workaround
for a bug with a devicetree property if the circumstance (SVID mode)
can be determined automatically.

Guenter

> Regards,
> Naresh
>>
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    i2c {
>>> +        #address-cells = <1>;
>>> +        #size-cells = <0>;
>>> +
>>> +        tda38640@40 {
>>> +            compatible = "infineon,tda38640";
>>> +            reg = <0x40>;
>>> +        };
>>> +    };
>>> +
>>> diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
>>> index 6e24c4d25ec3..2b1fbb2a672b 100644
>>> --- a/Documentation/devicetree/bindings/trivial-devices.yaml
>>> +++ b/Documentation/devicetree/bindings/trivial-devices.yaml
>>> @@ -151,8 +151,6 @@ properties:
>>>             - infineon,slb9645tt
>>>               # Infineon SLB9673 I2C TPM 2.0
>>>             - infineon,slb9673
>>> -            # Infineon TDA38640 Voltage Regulator
>>> -          - infineon,tda38640
>>>               # Infineon TLV493D-A1B6 I2C 3D Magnetic Sensor
>>>             - infineon,tlv493d-a1b6
>>>               # Infineon Multi-phase Digital VR Controller xdpe11280
>>>
>>> base-commit: 55612007f16b5d7b1fb83a7b0f5bb686829db7c7
>>> --
>>> 2.41.0
>>>

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

[kernel test robot]

Hi Naresh,

kernel test robot noticed the following build errors:

[auto build test ERROR on 55612007f16b5d7b1fb83a7b0f5bb686829db7c7]

url:    https://github.com/intel-lab-lkp/linux/commits/Naresh-Solanki/hwmon-pmbus-Add-ON_OFF_CONFIG-register-bits/20230725-194318
base:   55612007f16b5d7b1fb83a7b0f5bb686829db7c7
patch link:    https://lore.kernel.org/r/20230725114030.1860571-3-Naresh.Solanki%409elements.com
patch subject: [PATCH 3/3] hwmon: (pmbus/tda38640) Add workaround for bug in SVID mode
config: riscv-randconfig-r042-20230725 (https://download.01.org/0day-ci/archive/20230726/202307260005.nDX1xks3-lkp@intel.com/config)
compiler: riscv32-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230726/202307260005.nDX1xks3-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202307260005.nDX1xks3-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/hwmon/pmbus/tda38640.c: In function 'tda38640_probe':
>> drivers/hwmon/pmbus/tda38640.c:118:14: error: 'CONFIG_SENSORS_TDA38640_REGULATOR' undeclared (first use in this function); did you mean 'CONFIG_SENSORS_TDA38640'?
     118 |         if (!CONFIG_SENSORS_TDA38640_REGULATOR || !np ||
         |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         |              CONFIG_SENSORS_TDA38640
   drivers/hwmon/pmbus/tda38640.c:118:14: note: each undeclared identifier is reported only once for each function it appears in


vim +118 drivers/hwmon/pmbus/tda38640.c

   106	
   107	static int tda38640_probe(struct i2c_client *client)
   108	{
   109		struct device *dev = &client->dev;
   110		struct device_node *np = dev_of_node(dev);
   111		struct tda38640_data *data;
   112	
   113		data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
   114		if (!data)
   115			return -ENOMEM;
   116		memcpy(&data->info, &tda38640_info, sizeof(tda38640_info));
   117	
 > 118		if (!CONFIG_SENSORS_TDA38640_REGULATOR || !np ||
   119		    of_property_read_u32(np, "infineon,en-pin-fixed-level", &data->en_pin_lvl))
   120			return pmbus_do_probe(client, &data->info);
   121	
   122		/*
   123		 * Apply ON_OFF_CONFIG workaround as enabling the regulator using the
   124		 * OPERATION register doesn't work in SVID mode.
   125		 */
   126		data->info.read_byte_data = tda38640_read_byte_data;
   127		data->info.write_byte_data = tda38640_write_byte_data;
   128		/*
   129		 * One should configure PMBUS_ON_OFF_CONFIG here, but
   130		 * PB_ON_OFF_CONFIG_POWERUP_CONTROL, PB_ON_OFF_CONFIG_EN_PIN_REQ and
   131		 * PB_ON_OFF_CONFIG_EN_PIN_REQ are ignored by the device.
   132		 * Only PB_ON_OFF_CONFIG_POLARITY_HIGH has an effect.
   133		 */
   134	
   135		return pmbus_do_probe(client, &data->info);
   136	}
   137	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

Hi Naresh,

kernel test robot noticed the following build errors:

[auto build test ERROR on 55612007f16b5d7b1fb83a7b0f5bb686829db7c7]

url:    https://github.com/intel-lab-lkp/linux/commits/Naresh-Solanki/hwmon-pmbus-Add-ON_OFF_CONFIG-register-bits/20230725-194318
base:   55612007f16b5d7b1fb83a7b0f5bb686829db7c7
patch link:    https://lore.kernel.org/r/20230725114030.1860571-3-Naresh.Solanki%409elements.com
patch subject: [PATCH 3/3] hwmon: (pmbus/tda38640) Add workaround for bug in SVID mode
config: x86_64-randconfig-x014-20230725 (https://download.01.org/0day-ci/archive/20230726/202307260241.BetLbnxd-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce: (https://download.01.org/0day-ci/archive/20230726/202307260241.BetLbnxd-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202307260241.BetLbnxd-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/hwmon/pmbus/tda38640.c:118:7: error: use of undeclared identifier 'CONFIG_SENSORS_TDA38640_REGULATOR'
           if (!CONFIG_SENSORS_TDA38640_REGULATOR || !np ||
                ^
   1 error generated.


vim +/CONFIG_SENSORS_TDA38640_REGULATOR +118 drivers/hwmon/pmbus/tda38640.c

   106	
   107	static int tda38640_probe(struct i2c_client *client)
   108	{
   109		struct device *dev = &client->dev;
   110		struct device_node *np = dev_of_node(dev);
   111		struct tda38640_data *data;
   112	
   113		data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
   114		if (!data)
   115			return -ENOMEM;
   116		memcpy(&data->info, &tda38640_info, sizeof(tda38640_info));
   117	
 > 118		if (!CONFIG_SENSORS_TDA38640_REGULATOR || !np ||
   119		    of_property_read_u32(np, "infineon,en-pin-fixed-level", &data->en_pin_lvl))
   120			return pmbus_do_probe(client, &data->info);
   121	
   122		/*
   123		 * Apply ON_OFF_CONFIG workaround as enabling the regulator using the
   124		 * OPERATION register doesn't work in SVID mode.
   125		 */
   126		data->info.read_byte_data = tda38640_read_byte_data;
   127		data->info.write_byte_data = tda38640_write_byte_data;
   128		/*
   129		 * One should configure PMBUS_ON_OFF_CONFIG here, but
   130		 * PB_ON_OFF_CONFIG_POWERUP_CONTROL, PB_ON_OFF_CONFIG_EN_PIN_REQ and
   131		 * PB_ON_OFF_CONFIG_EN_PIN_REQ are ignored by the device.
   132		 * Only PB_ON_OFF_CONFIG_POLARITY_HIGH has an effect.
   133		 */
   134	
   135		return pmbus_do_probe(client, &data->info);
   136	}
   137	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Naresh Solanki]

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

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

[Guenter Roeck]

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

>      if (ret < 0) {
>          dev_err(&client->dev, "%s:%d i2c_transfer failed. %d", __func__, __LINE__, ret);
>          return ret;

Return type is bool.

>      }
>      /* 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 ?

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

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

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

[Naresh Solanki]

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