> -----Original Message----- > From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck > Sent: Wednesday, April 17, 2024 11:25 PM > To: SanBuenaventura, Jose <Jose.SanBuenaventura@xxxxxxxxxx> > Cc: linux-hwmon@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx; > Jean Delvare <jdelvare@xxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>; Krzysztof > Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>; > Jonathan Corbet <corbet@xxxxxxx>; Delphine CC Chiu > <Delphine_CC_Chiu@xxxxxxxxxx> > 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@xxxxxxxxxx> > > > --- > > > 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
