[PATCH 0/2] Add adm1281 support

[PATCH 0/2] Add adm1281 support

[Jose Ramon San Buenaventura]

The ADM1281 is also a Hot-swap controller designed to be a drop-in
replacement to the ADM1278. The currently available adm1275 driver supports
the adm1278 and this patch adds support for the adm1281, specifically
adding support for STATUS_CML register reads.
 
An existing patch was found that adds support for the adm1281 but has not 
been followed up since November 22, 2023. This patch aims to serve as a
follow up or an improvement of the previously submitted patch especially
since the code was tested on the actual adm1281 evaluation board.

Jose Ramon San Buenaventura (2):
  dt-bindings: hwmon: adm1275: add adm1281
  hwmon: pmbus: adm1275: add adm1281 support

 .../bindings/hwmon/adi,adm1275.yaml           |  4 ++-
 Documentation/hwmon/adm1275.rst               | 14 +++++++---
 drivers/hwmon/pmbus/Kconfig                   |  4 +--
 drivers/hwmon/pmbus/adm1275.c                 | 27 +++++++++++++++++--
 4 files changed, 41 insertions(+), 8 deletions(-)


base-commit: 96fca68c4fbf77a8185eb10f7557e23352732ea2
-- 
2.39.2

[PATCH 1/2] dt-bindings: hwmon: adm1275: add adm1281

[Jose Ramon San Buenaventura]

Add support for the adm1281 Hot-Swap Controller and Digital Power
and Energy Monitor

Signed-off-by: Jose Ramon San Buenaventura <jose.sanbuenaventura@analog.com>
---
 Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml b/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml
index b68061294..5b076d677 100644
--- a/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml
+++ b/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml
@@ -5,7 +5,7 @@
 $id: http://devicetree.org/schemas/hwmon/adi,adm1275.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: Analog Devices ADM1075/ADM127x/ADM129x digital power monitors
+title: Analog Devices ADM1075/ADM127x/ADM1281/ADM129x digital power monitors
 
 maintainers:
   - Krzysztof Kozlowski <krzk@kernel.org>
@@ -27,6 +27,7 @@ properties:
       - adi,adm1275
       - adi,adm1276
       - adi,adm1278
+      - adi,adm1281
       - adi,adm1293
       - adi,adm1294
 
@@ -91,6 +92,7 @@ allOf:
           contains:
             enum:
               - adi,adm1278
+              - adi,adm1281
               - adi,adm1293
               - adi,adm1294
     then:
-- 
2.39.2

Re: [PATCH 1/2] dt-bindings: hwmon: adm1275: add adm1281

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 308 bytes --]

On Wed, Apr 17, 2024 at 08:07:21AM +0800, Jose Ramon San Buenaventura wrote:
> Add support for the adm1281 Hot-Swap Controller and Digital Power
> and Energy Monitor
> 
> Signed-off-by: Jose Ramon San Buenaventura <jose.sanbuenaventura@analog.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH 2/2] hwmon: pmbus: adm1275: add adm1281 support

[Jose Ramon San Buenaventura]

Adding support for adm1281 which is similar to adm1275

ADM1281 has STATUS_CML read support which is also being added.

Signed-off-by: Jose Ramon San Buenaventura <jose.sanbuenaventura@analog.com>
---
 Documentation/hwmon/adm1275.rst | 14 +++++++++++---
 drivers/hwmon/pmbus/Kconfig     |  4 ++--
 drivers/hwmon/pmbus/adm1275.c   | 27 +++++++++++++++++++++++++--
 3 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/Documentation/hwmon/adm1275.rst b/Documentation/hwmon/adm1275.rst
index 804590eea..467daf8ce 100644
--- a/Documentation/hwmon/adm1275.rst
+++ b/Documentation/hwmon/adm1275.rst
@@ -43,6 +43,14 @@ Supported chips:
 
     Datasheet: www.analog.com/static/imported-files/data_sheets/ADM1278.pdf
 
+  * Analog Devices ADM1281
+
+    Prefix: 'adm1281'
+
+    Addresses scanned: -
+
+    Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adm1281.pdf
+
   * Analog Devices ADM1293/ADM1294
 
     Prefix: 'adm1293', 'adm1294'
@@ -58,10 +66,10 @@ Description
 -----------
 
 This driver supports hardware monitoring for Analog Devices ADM1075, ADM1272,
-ADM1275, ADM1276, ADM1278, ADM1293, and ADM1294 Hot-Swap Controller and
+ADM1275, ADM1276, ADM1278, ADM1281, ADM1293, and ADM1294 Hot-Swap Controller and
 Digital Power Monitors.
 
-ADM1075, ADM1272, ADM1275, ADM1276, ADM1278, ADM1293, and ADM1294 are hot-swap
+ADM1075, ADM1272, ADM1275, ADM1276, ADM1278, ADM1281, ADM1293, and ADM1294 are hot-swap
 controllers that allow a circuit board to be removed from or inserted into
 a live backplane. They also feature current and voltage readback via an
 integrated 12 bit analog-to-digital converter (ADC), accessed using a
@@ -144,5 +152,5 @@ temp1_highest		Highest observed temperature.
 temp1_reset_history	Write any value to reset history.
 
 			Temperature attributes are supported on ADM1272 and
-			ADM1278.
+			ADM1278, and ADM1281.
 ======================= =======================================================
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 557ae0c41..9c1d0d7d5 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -51,8 +51,8 @@ config SENSORS_ADM1275
 	tristate "Analog Devices ADM1275 and compatibles"
 	help
 	  If you say yes here you get hardware monitoring support for Analog
-	  Devices ADM1075, ADM1272, ADM1275, ADM1276, ADM1278, ADM1293,
-	  and ADM1294 Hot-Swap Controller and Digital Power Monitors.
+	  Devices ADM1075, ADM1272, ADM1275, ADM1276, ADM1278, ADM1281,
+	  ADM1293, and ADM1294 Hot-Swap Controller and Digital Power Monitors.
 
 	  This driver can also be built as a module. If so, the module will
 	  be called adm1275.
diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c
index e2c61d6fa..6c3e8840f 100644
--- a/drivers/hwmon/pmbus/adm1275.c
+++ b/drivers/hwmon/pmbus/adm1275.c
@@ -18,7 +18,7 @@
 #include <linux/log2.h>
 #include "pmbus.h"
 
-enum chips { adm1075, adm1272, adm1275, adm1276, adm1278, adm1293, adm1294 };
+enum chips { adm1075, adm1272, adm1275, adm1276, adm1278, adm1281, adm1293, adm1294 };
 
 #define ADM1275_MFR_STATUS_IOUT_WARN2	BIT(0)
 #define ADM1293_MFR_STATUS_VAUX_UV_WARN	BIT(5)
@@ -101,6 +101,7 @@ struct adm1275_data {
 	bool have_pin_max;
 	bool have_temp_max;
 	bool have_power_sampling;
+	bool have_status_cml;
 	struct pmbus_driver_info info;
 };
 
@@ -469,6 +470,22 @@ static int adm1275_read_byte_data(struct i2c_client *client, int page, int reg)
 				ret |= PB_VOLTAGE_UV_WARNING;
 		}
 		break;
+	case PMBUS_STATUS_CML:
+		if (!data->have_status_cml)
+			return -ENXIO;
+
+		ret = pmbus_read_byte_data(client, page, PMBUS_STATUS_BYTE);
+		if (ret < 0)
+			break;
+
+		if (ret & PB_STATUS_CML) {
+			ret = pmbus_read_byte_data(client, page, PMBUS_STATUS_CML);
+			if (ret < 0)
+				break;
+		} else {
+			ret = 0;
+		}
+		break;
 	default:
 		ret = -ENODATA;
 		break;
@@ -482,6 +499,7 @@ static const struct i2c_device_id adm1275_id[] = {
 	{ "adm1275", adm1275 },
 	{ "adm1276", adm1276 },
 	{ "adm1278", adm1278 },
+	{ "adm1281", adm1281 },
 	{ "adm1293", adm1293 },
 	{ "adm1294", adm1294 },
 	{ }
@@ -555,7 +573,8 @@ static int adm1275_probe(struct i2c_client *client)
 			   client->name, mid->name);
 
 	if (mid->driver_data == adm1272 || mid->driver_data == adm1278 ||
-	    mid->driver_data == adm1293 || mid->driver_data == adm1294)
+	    mid->driver_data == adm1281 || mid->driver_data == adm1293 ||
+	    mid->driver_data == adm1294)
 		config_read_fn = i2c_smbus_read_word_data;
 	else
 		config_read_fn = i2c_smbus_read_byte_data;
@@ -703,6 +722,10 @@ static int adm1275_probe(struct i2c_client *client)
 			  PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT;
 		break;
 	case adm1278:
+	case adm1281:
+		if (data->id == adm1281)
+			data->have_status_cml = true;
+
 		data->have_vout = true;
 		data->have_pin_max = true;
 		data->have_temp_max = true;
-- 
2.39.2

Re: [PATCH 2/2] hwmon: pmbus: adm1275: add adm1281 support

[Guenter Roeck]

On Wed, Apr 17, 2024 at 08:07:22AM +0800, Jose Ramon San Buenaventura wrote:
> Adding support for adm1281 which is similar to adm1275
> 
> ADM1281 has STATUS_CML read support which is also being added.
> 
> Signed-off-by: Jose Ramon San Buenaventura <jose.sanbuenaventura@analog.com>
> ---
>  Documentation/hwmon/adm1275.rst | 14 +++++++++++---
>  drivers/hwmon/pmbus/Kconfig     |  4 ++--
>  drivers/hwmon/pmbus/adm1275.c   | 27 +++++++++++++++++++++++++--
>  3 files changed, 38 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/hwmon/adm1275.rst b/Documentation/hwmon/adm1275.rst
> index 804590eea..467daf8ce 100644
> --- a/Documentation/hwmon/adm1275.rst
> +++ b/Documentation/hwmon/adm1275.rst
> @@ -43,6 +43,14 @@ Supported chips:
>  
>      Datasheet: www.analog.com/static/imported-files/data_sheets/ADM1278.pdf
>  
> +  * Analog Devices ADM1281
> +
> +    Prefix: 'adm1281'
> +
> +    Addresses scanned: -
> +
> +    Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adm1281.pdf
> +
>    * Analog Devices ADM1293/ADM1294
>  
>      Prefix: 'adm1293', 'adm1294'
> @@ -58,10 +66,10 @@ Description
>  -----------
>  
>  This driver supports hardware monitoring for Analog Devices ADM1075, ADM1272,
> -ADM1275, ADM1276, ADM1278, ADM1293, and ADM1294 Hot-Swap Controller and
> +ADM1275, ADM1276, ADM1278, ADM1281, ADM1293, and ADM1294 Hot-Swap Controller and
>  Digital Power Monitors.
>  
> -ADM1075, ADM1272, ADM1275, ADM1276, ADM1278, ADM1293, and ADM1294 are hot-swap
> +ADM1075, ADM1272, ADM1275, ADM1276, ADM1278, ADM1281, ADM1293, and ADM1294 are hot-swap
>  controllers that allow a circuit board to be removed from or inserted into
>  a live backplane. They also feature current and voltage readback via an
>  integrated 12 bit analog-to-digital converter (ADC), accessed using a
> @@ -144,5 +152,5 @@ temp1_highest		Highest observed temperature.
>  temp1_reset_history	Write any value to reset history.
>  
>  			Temperature attributes are supported on ADM1272 and
> -			ADM1278.
> +			ADM1278, and ADM1281.
>  ======================= =======================================================
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index 557ae0c41..9c1d0d7d5 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -51,8 +51,8 @@ config SENSORS_ADM1275
>  	tristate "Analog Devices ADM1275 and compatibles"
>  	help
>  	  If you say yes here you get hardware monitoring support for Analog
> -	  Devices ADM1075, ADM1272, ADM1275, ADM1276, ADM1278, ADM1293,
> -	  and ADM1294 Hot-Swap Controller and Digital Power Monitors.
> +	  Devices ADM1075, ADM1272, ADM1275, ADM1276, ADM1278, ADM1281,
> +	  ADM1293, and ADM1294 Hot-Swap Controller and Digital Power Monitors.
>  
>  	  This driver can also be built as a module. If so, the module will
>  	  be called adm1275.
> diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c
> index e2c61d6fa..6c3e8840f 100644
> --- a/drivers/hwmon/pmbus/adm1275.c
> +++ b/drivers/hwmon/pmbus/adm1275.c
> @@ -18,7 +18,7 @@
>  #include <linux/log2.h>
>  #include "pmbus.h"
>  
> -enum chips { adm1075, adm1272, adm1275, adm1276, adm1278, adm1293, adm1294 };
> +enum chips { adm1075, adm1272, adm1275, adm1276, adm1278, adm1281, adm1293, adm1294 };
>  
>  #define ADM1275_MFR_STATUS_IOUT_WARN2	BIT(0)
>  #define ADM1293_MFR_STATUS_VAUX_UV_WARN	BIT(5)
> @@ -101,6 +101,7 @@ struct adm1275_data {
>  	bool have_pin_max;
>  	bool have_temp_max;
>  	bool have_power_sampling;
> +	bool have_status_cml;
>  	struct pmbus_driver_info info;
>  };
>  
> @@ -469,6 +470,22 @@ static int adm1275_read_byte_data(struct i2c_client *client, int page, int reg)
>  				ret |= PB_VOLTAGE_UV_WARNING;
>  		}
>  		break;
> +	case PMBUS_STATUS_CML:
> +		if (!data->have_status_cml)
> +			return -ENXIO;
> +
> +		ret = pmbus_read_byte_data(client, page, PMBUS_STATUS_BYTE);
> +		if (ret < 0)
> +			break;

You'll have to explain why this additional status byte read
is necessary (while it isn't necessary for all other chips supporting
PMBUS_STATUS_CML).

Thanks,
Guenter

Re: [PATCH 2/2] hwmon: pmbus: adm1275: add adm1281 support

[Guenter Roeck]

On Tue, Apr 16, 2024 at 05:32:46PM -0700, Guenter Roeck wrote:
> On Wed, Apr 17, 2024 at 08:07:22AM +0800, Jose Ramon San Buenaventura wrote:
> > Adding support for adm1281 which is similar to adm1275
> > 
> > ADM1281 has STATUS_CML read support which is also being added.
> > 
> > Signed-off-by: Jose Ramon San Buenaventura <jose.sanbuenaventura@analog.com>
> > ---
> >  Documentation/hwmon/adm1275.rst | 14 +++++++++++---
> >  drivers/hwmon/pmbus/Kconfig     |  4 ++--
> >  drivers/hwmon/pmbus/adm1275.c   | 27 +++++++++++++++++++++++++--
> >  3 files changed, 38 insertions(+), 7 deletions(-)
> > 
> > diff --git a/Documentation/hwmon/adm1275.rst b/Documentation/hwmon/adm1275.rst
> > index 804590eea..467daf8ce 100644
> > --- a/Documentation/hwmon/adm1275.rst
> > +++ b/Documentation/hwmon/adm1275.rst
> > @@ -43,6 +43,14 @@ Supported chips:
> >  
> >      Datasheet: www.analog.com/static/imported-files/data_sheets/ADM1278.pdf
> >  
> > +  * Analog Devices ADM1281
> > +
> > +    Prefix: 'adm1281'
> > +
> > +    Addresses scanned: -
> > +
> > +    Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adm1281.pdf
> > +
> >    * Analog Devices ADM1293/ADM1294
> >  
> >      Prefix: 'adm1293', 'adm1294'
> > @@ -58,10 +66,10 @@ Description
> >  -----------
> >  
> >  This driver supports hardware monitoring for Analog Devices ADM1075, ADM1272,
> > -ADM1275, ADM1276, ADM1278, ADM1293, and ADM1294 Hot-Swap Controller and
> > +ADM1275, ADM1276, ADM1278, ADM1281, ADM1293, and ADM1294 Hot-Swap Controller and
> >  Digital Power Monitors.
> >  
> > -ADM1075, ADM1272, ADM1275, ADM1276, ADM1278, ADM1293, and ADM1294 are hot-swap
> > +ADM1075, ADM1272, ADM1275, ADM1276, ADM1278, ADM1281, ADM1293, and ADM1294 are hot-swap
> >  controllers that allow a circuit board to be removed from or inserted into
> >  a live backplane. They also feature current and voltage readback via an
> >  integrated 12 bit analog-to-digital converter (ADC), accessed using a
> > @@ -144,5 +152,5 @@ temp1_highest		Highest observed temperature.
> >  temp1_reset_history	Write any value to reset history.
> >  
> >  			Temperature attributes are supported on ADM1272 and
> > -			ADM1278.
> > +			ADM1278, and ADM1281.
> >  ======================= =======================================================
> > diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> > index 557ae0c41..9c1d0d7d5 100644
> > --- a/drivers/hwmon/pmbus/Kconfig
> > +++ b/drivers/hwmon/pmbus/Kconfig
> > @@ -51,8 +51,8 @@ config SENSORS_ADM1275
> >  	tristate "Analog Devices ADM1275 and compatibles"
> >  	help
> >  	  If you say yes here you get hardware monitoring support for Analog
> > -	  Devices ADM1075, ADM1272, ADM1275, ADM1276, ADM1278, ADM1293,
> > -	  and ADM1294 Hot-Swap Controller and Digital Power Monitors.
> > +	  Devices ADM1075, ADM1272, ADM1275, ADM1276, ADM1278, ADM1281,
> > +	  ADM1293, and ADM1294 Hot-Swap Controller and Digital Power Monitors.
> >  
> >  	  This driver can also be built as a module. If so, the module will
> >  	  be called adm1275.
> > diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c
> > index e2c61d6fa..6c3e8840f 100644
> > --- a/drivers/hwmon/pmbus/adm1275.c
> > +++ b/drivers/hwmon/pmbus/adm1275.c
> > @@ -18,7 +18,7 @@
> >  #include <linux/log2.h>
> >  #include "pmbus.h"
> >  
> > -enum chips { adm1075, adm1272, adm1275, adm1276, adm1278, adm1293, adm1294 };
> > +enum chips { adm1075, adm1272, adm1275, adm1276, adm1278, adm1281, adm1293, adm1294 };
> >  
> >  #define ADM1275_MFR_STATUS_IOUT_WARN2	BIT(0)
> >  #define ADM1293_MFR_STATUS_VAUX_UV_WARN	BIT(5)
> > @@ -101,6 +101,7 @@ struct adm1275_data {
> >  	bool have_pin_max;
> >  	bool have_temp_max;
> >  	bool have_power_sampling;
> > +	bool have_status_cml;
> >  	struct pmbus_driver_info info;
> >  };
> >  
> > @@ -469,6 +470,22 @@ static int adm1275_read_byte_data(struct i2c_client *client, int page, int reg)
> >  				ret |= PB_VOLTAGE_UV_WARNING;
> >  		}
> >  		break;
> > +	case PMBUS_STATUS_CML:
> > +		if (!data->have_status_cml)
> > +			return -ENXIO;
> > +
> > +		ret = pmbus_read_byte_data(client, page, PMBUS_STATUS_BYTE);
> > +		if (ret < 0)
> > +			break;
> 
> You'll have to explain why this additional status byte read
> is necessary (while it isn't necessary for all other chips supporting
> PMBUS_STATUS_CML).
> 

After looking more into the existing PMBus code and into this patch,
I really fail to understand why the above change would be needed.
The PMBus core code already reads the status register to check if
there is a communication error. I fail to see why it would be necessary
to do it again, and why it would be necessary to change behavior for
the other chips supported by this driver.

Guenter

RE: [PATCH 2/2] hwmon: pmbus: adm1275: add adm1281 support

[SanBuenaventura, Jose]

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Wednesday, April 17, 2024 11:25 PM
> To: SanBuenaventura, Jose <Jose.SanBuenaventura@analog.com>
> Cc: linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-doc@vger.kernel.org; linux-i2c@vger.kernel.org;
> Jean Delvare <jdelvare@suse.com>; Rob Herring <robh@kernel.org>; Krzysztof
> Kozlowski <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>;
> Jonathan Corbet <corbet@lwn.net>; Delphine CC Chiu
> <Delphine_CC_Chiu@wiwynn.com>
> Subject: Re: [PATCH 2/2] hwmon: pmbus: adm1275: add adm1281 support
> 
> [External]
> 
> On Tue, Apr 16, 2024 at 05:32:46PM -0700, Guenter Roeck wrote:
> > On Wed, Apr 17, 2024 at 08:07:22AM +0800, Jose Ramon San Buenaventura
> wrote:
> > > Adding support for adm1281 which is similar to adm1275
> > >
> > > ADM1281 has STATUS_CML read support which is also being added.
> > >
> > > Signed-off-by: Jose Ramon San Buenaventura
> > > <jose.sanbuenaventura@analog.com>
> > > ---
> > >  Documentation/hwmon/adm1275.rst | 14 +++++++++++---
> > >  drivers/hwmon/pmbus/Kconfig     |  4 ++--
> > >  drivers/hwmon/pmbus/adm1275.c   | 27 +++++++++++++++++++++++++-
> -
> > >  3 files changed, 38 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/Documentation/hwmon/adm1275.rst
> > > b/Documentation/hwmon/adm1275.rst index 804590eea..467daf8ce
> 100644
> > > --- a/Documentation/hwmon/adm1275.rst
> > > +++ b/Documentation/hwmon/adm1275.rst
> > > @@ -43,6 +43,14 @@ Supported chips:
> > >
> > >      Datasheet:
> > > www.analog.com/static/imported-files/data_sheets/ADM1278.pdf
> > >
> > > +  * Analog Devices ADM1281
> > > +
> > > +    Prefix: 'adm1281'
> > > +
> > > +    Addresses scanned: -
> > > +
> > > +    Datasheet:
> > > + https://www.analog.com/media/en/technical-documentation/data-sheet
> > > + s/adm1281.pdf
> > > +
> > >    * Analog Devices ADM1293/ADM1294
> > >
> > >      Prefix: 'adm1293', 'adm1294'
> > > @@ -58,10 +66,10 @@ Description
> > >  -----------
> > >
> > >  This driver supports hardware monitoring for Analog Devices
> > > ADM1075, ADM1272, -ADM1275, ADM1276, ADM1278, ADM1293, and
> ADM1294
> > > Hot-Swap Controller and
> > > +ADM1275, ADM1276, ADM1278, ADM1281, ADM1293, and ADM1294
> Hot-Swap
> > > +Controller and
> > >  Digital Power Monitors.
> > >
> > > -ADM1075, ADM1272, ADM1275, ADM1276, ADM1278, ADM1293, and
> ADM1294
> > > are hot-swap
> > > +ADM1075, ADM1272, ADM1275, ADM1276, ADM1278, ADM1281,
> ADM1293, and
> > > +ADM1294 are hot-swap
> > >  controllers that allow a circuit board to be removed from or
> > > inserted into  a live backplane. They also feature current and
> > > voltage readback via an  integrated 12 bit analog-to-digital converter (ADC),
> accessed using a
> > > @@ -144,5 +152,5 @@ temp1_highest		Highest observed
> temperature.
> > >  temp1_reset_history	Write any value to reset history.
> > >
> > >  			Temperature attributes are supported on ADM1272
> and
> > > -			ADM1278.
> > > +			ADM1278, and ADM1281.
> > >  =======================
> > > =======================================================
> > > diff --git a/drivers/hwmon/pmbus/Kconfig
> > > b/drivers/hwmon/pmbus/Kconfig index 557ae0c41..9c1d0d7d5 100644
> > > --- a/drivers/hwmon/pmbus/Kconfig
> > > +++ b/drivers/hwmon/pmbus/Kconfig
> > > @@ -51,8 +51,8 @@ config SENSORS_ADM1275
> > >  	tristate "Analog Devices ADM1275 and compatibles"
> > >  	help
> > >  	  If you say yes here you get hardware monitoring support for Analog
> > > -	  Devices ADM1075, ADM1272, ADM1275, ADM1276, ADM1278,
> ADM1293,
> > > -	  and ADM1294 Hot-Swap Controller and Digital Power Monitors.
> > > +	  Devices ADM1075, ADM1272, ADM1275, ADM1276, ADM1278,
> ADM1281,
> > > +	  ADM1293, and ADM1294 Hot-Swap Controller and Digital Power
> Monitors.
> > >
> > >  	  This driver can also be built as a module. If so, the module will
> > >  	  be called adm1275.
> > > diff --git a/drivers/hwmon/pmbus/adm1275.c
> > > b/drivers/hwmon/pmbus/adm1275.c index e2c61d6fa..6c3e8840f 100644
> > > --- a/drivers/hwmon/pmbus/adm1275.c
> > > +++ b/drivers/hwmon/pmbus/adm1275.c
> > > @@ -18,7 +18,7 @@
> > >  #include <linux/log2.h>
> > >  #include "pmbus.h"
> > >
> > > -enum chips { adm1075, adm1272, adm1275, adm1276, adm1278,
> adm1293,
> > > adm1294 };
> > > +enum chips { adm1075, adm1272, adm1275, adm1276, adm1278,
> adm1281,
> > > +adm1293, adm1294 };
> > >
> > >  #define ADM1275_MFR_STATUS_IOUT_WARN2	BIT(0)
> > >  #define ADM1293_MFR_STATUS_VAUX_UV_WARN	BIT(5)
> > > @@ -101,6 +101,7 @@ struct adm1275_data {
> > >  	bool have_pin_max;
> > >  	bool have_temp_max;
> > >  	bool have_power_sampling;
> > > +	bool have_status_cml;
> > >  	struct pmbus_driver_info info;
> > >  };
> > >
> > > @@ -469,6 +470,22 @@ static int adm1275_read_byte_data(struct
> i2c_client *client, int page, int reg)
> > >  				ret |= PB_VOLTAGE_UV_WARNING;
> > >  		}
> > >  		break;
> > > +	case PMBUS_STATUS_CML:
> > > +		if (!data->have_status_cml)
> > > +			return -ENXIO;
> > > +
> > > +		ret = pmbus_read_byte_data(client, page,
> PMBUS_STATUS_BYTE);
> > > +		if (ret < 0)
> > > +			break;
> >
> > You'll have to explain why this additional status byte read is
> > necessary (while it isn't necessary for all other chips supporting
> > PMBUS_STATUS_CML).
> >
> 
> After looking more into the existing PMBus code and into this patch, I really fail
> to understand why the above change would be needed.
> The PMBus core code already reads the status register to check if there is a
> communication error. I fail to see why it would be necessary to do it again, and
> why it would be necessary to change behavior for the other chips supported by
> this driver.
> 

The ADM1281 contains an additional register STATUS_CML (0x78) which provides
more specific information regarding any detected CML related error such as 
invalid command received, invalid data received, PEC check failed, Trim memory
fault, or other.

Upon double checking the PMBus core code, there seems an existing provision
for checking if STATUS_CML register read is provided (lines 3253 to 3261 in 
pmbus_core.c file). The debugfs entry status0_cml also seem to provide the 
data from the STATUS_CML register and is present in the debugfs entries when
using the adm1281 hardware.

The lines mentioned were added initially because the STATUS_CML read capability
seems to be only available in the adm1281 and so reading the said register with
another device shouldn't be permitted. The lines mentioned also checks first if 
the CML_FAULT bit in the status register is set before reading the STATUS_CML
register to avoid reading the STATUS_CML register for info if the error is not
CML related. 

It seems though that the functionality is redundant and is already handled by 
the PMBus core and maybe these lines can be removed and CML related errors
can be checked using the status0 and status0_cml debugfs entries.

Thanks,
Joram

Re: [PATCH 2/2] hwmon: pmbus: adm1275: add adm1281 support

[Guenter Roeck]

On Thu, Apr 18, 2024 at 08:31:42AM +0000, SanBuenaventura, Jose wrote:
> 
> The lines mentioned were added initially because the STATUS_CML read capability
> seems to be only available in the adm1281 and so reading the said register with
> another device shouldn't be permitted.
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Why ? Sure, doing so causes the CML bit to be set, but the PMBus core uses
that method throughout to determine if a command/register is supported.
There are exceptions - some chips react badly if an attempt is made to read
unsupported registers. That is not the case for chips in this series, at
least not for the ones where I have evaluation boards. In such cases,
the chip driver should do nothing and let the PMBus core do its job.

> It seems though that the functionality is redundant and is already handled by 
> the PMBus core and maybe these lines can be removed and CML related errors
> can be checked using the status0 and status0_cml debugfs entries.

This has nothing to do with status0 and status0_cml debugfs entries. The
PMBUs core reads STATUS_CML if the CML status bit is set in the status
byte/word to determine if a command is supported or not. This is as
intended. There is nothing special to be done by a chip driver.

Thanks,
Guenter

RE: [PATCH 2/2] hwmon: pmbus: adm1275: add adm1281 support

[SanBuenaventura, Jose]

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Thursday, April 18, 2024 7:55 PM
> To: SanBuenaventura, Jose <Jose.SanBuenaventura@analog.com>
> Cc: linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-doc@vger.kernel.org; linux-i2c@vger.kernel.org;
> Jean Delvare <jdelvare@suse.com>; Rob Herring <robh@kernel.org>;
> Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley
> <conor+dt@kernel.org>; Jonathan Corbet <corbet@lwn.net>; Delphine CC
> Chiu <Delphine_CC_Chiu@wiwynn.com>
> Subject: Re: [PATCH 2/2] hwmon: pmbus: adm1275: add adm1281 support
> 
> [External]
> 
> On Thu, Apr 18, 2024 at 08:31:42AM +0000, SanBuenaventura, Jose wrote:
> >
> > The lines mentioned were added initially because the STATUS_CML read
> capability
> > seems to be only available in the adm1281 and so reading the said register
> with
> > another device shouldn't be permitted.
>   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> Why ? Sure, doing so causes the CML bit to be set, but the PMBus core uses
> that method throughout to determine if a command/register is supported.
> There are exceptions - some chips react badly if an attempt is made to read
> unsupported registers. That is not the case for chips in this series, at
> least not for the ones where I have evaluation boards. In such cases,
> the chip driver should do nothing and let the PMBus core do its job.
> 
> > It seems though that the functionality is redundant and is already handled
> by
> > the PMBus core and maybe these lines can be removed and CML related
> errors
> > can be checked using the status0 and status0_cml debugfs entries.
> 
> This has nothing to do with status0 and status0_cml debugfs entries. The
> PMBUs core reads STATUS_CML if the CML status bit is set in the status
> byte/word to determine if a command is supported or not. This is as
> intended. There is nothing special to be done by a chip driver.
> 
> Thanks,
> Guenter

Based on the feedback, it seems that the lines mentioned can be removed since
the pmbus_core.c handles the checking of status_cml through different functions
like pmbus_check_status_cml and pmbus_check_register among others.

One thing we observed in the pmbus_core is that the invalid command flag was the
only one being utilized (PB_CML_FAULT_INVALID_COMMAND) but there are other
flags for cml fault in pmbus.h such as PB_CML_FAULT_PROCESSOR or 
PB_CML_FAULT_PACKET_ERROR that were not used. 

Given these observations, it would again seem right to eliminate the lines you 
pointed out since those items are already handled by the pmbus core and that
the status0_cml debugfs entry can be probed to check the register content directly
and see if there's any other cml fault aside from the invalid command that occurs
and the user can address it accordingly.

If ever there would be changes to address the other cml fault errors aside from the
invalid command it seems that the changes should be applied in the pmbus core 
and not on the chip driver itself.

I hope that I understood correctly the points you brought up and identified the 
possible next step to address those which is to eliminate the added case in the 
adm1275_read_byte_data.

Thanks,
Joram 

Re: [PATCH 2/2] hwmon: pmbus: adm1275: add adm1281 support

[Guenter Roeck]

On Fri, Apr 19, 2024 at 02:46:15AM +0000, SanBuenaventura, Jose wrote:
> > -----Original Message-----
> > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> > Sent: Thursday, April 18, 2024 7:55 PM
> > To: SanBuenaventura, Jose <Jose.SanBuenaventura@analog.com>
> > Cc: linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org; linux-
> > kernel@vger.kernel.org; linux-doc@vger.kernel.org; linux-i2c@vger.kernel.org;
> > Jean Delvare <jdelvare@suse.com>; Rob Herring <robh@kernel.org>;
> > Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley
> > <conor+dt@kernel.org>; Jonathan Corbet <corbet@lwn.net>; Delphine CC
> > Chiu <Delphine_CC_Chiu@wiwynn.com>
> > Subject: Re: [PATCH 2/2] hwmon: pmbus: adm1275: add adm1281 support
> > 
> > [External]
> > 
> > On Thu, Apr 18, 2024 at 08:31:42AM +0000, SanBuenaventura, Jose wrote:
> > >
> > > The lines mentioned were added initially because the STATUS_CML read
> > capability
> > > seems to be only available in the adm1281 and so reading the said register
> > with
> > > another device shouldn't be permitted.
> >   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > Why ? Sure, doing so causes the CML bit to be set, but the PMBus core uses
> > that method throughout to determine if a command/register is supported.
> > There are exceptions - some chips react badly if an attempt is made to read
> > unsupported registers. That is not the case for chips in this series, at
> > least not for the ones where I have evaluation boards. In such cases,
> > the chip driver should do nothing and let the PMBus core do its job.
> > 
> > > It seems though that the functionality is redundant and is already handled
> > by
> > > the PMBus core and maybe these lines can be removed and CML related
> > errors
> > > can be checked using the status0 and status0_cml debugfs entries.
> > 
> > This has nothing to do with status0 and status0_cml debugfs entries. The
> > PMBUs core reads STATUS_CML if the CML status bit is set in the status
> > byte/word to determine if a command is supported or not. This is as
> > intended. There is nothing special to be done by a chip driver.
> > 
> > Thanks,
> > Guenter
> 
> Based on the feedback, it seems that the lines mentioned can be removed since
> the pmbus_core.c handles the checking of status_cml through different functions
> like pmbus_check_status_cml and pmbus_check_register among others.
> 
> One thing we observed in the pmbus_core is that the invalid command flag was the
> only one being utilized (PB_CML_FAULT_INVALID_COMMAND) but there are other
> flags for cml fault in pmbus.h such as PB_CML_FAULT_PROCESSOR or 
> PB_CML_FAULT_PACKET_ERROR that were not used. 
> 
> Given these observations, it would again seem right to eliminate the lines you 
> pointed out since those items are already handled by the pmbus core and that
> the status0_cml debugfs entry can be probed to check the register content directly
> and see if there's any other cml fault aside from the invalid command that occurs
> and the user can address it accordingly.
> 
> If ever there would be changes to address the other cml fault errors aside from the
> invalid command it seems that the changes should be applied in the pmbus core 
> and not on the chip driver itself.
> 
> I hope that I understood correctly the points you brought up and identified the 
> possible next step to address those which is to eliminate the added case in the 
> adm1275_read_byte_data.
> 
Correct.

Thanks,
Guenter