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

[SanBuenaventura, Jose]

> -----Original Message-----
> From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
> Sent: Thursday, April 18, 2024 7:55 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 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