| Message ID | 20240417000722.919-3-jose.sanbuenaventura@analog.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add adm1281 support | expand |
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
> -----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
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
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;
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(-)