On Fri, Apr 19, 2024 at 02:46:15AM +0000, SanBuenaventura, Jose wrote: > > -----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. > Correct. Thanks, Guenter
