From: Guenter Roeck <linux@roeck-us.net>
To: "SanBuenaventura, Jose" <Jose.SanBuenaventura@analog.com>
Cc: "linux-hwmon@vger.kernel.org" <linux-hwmon@vger.kernel.org>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
"linux-i2c@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
Date: Fri, 19 Apr 2024 05:33:18 -0700 [thread overview]
Message-ID: <902d73a2-e46a-4f60-bde2-3badc46863da@roeck-us.net> (raw)
In-Reply-To: <SJ0PR03MB66008186E17D3920B5B58B92EC0D2@SJ0PR03MB6600.namprd03.prod.outlook.com>
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
prev parent reply other threads:[~2024-04-19 12:33 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-17 0:07 [PATCH 0/2] Add adm1281 support Jose Ramon San Buenaventura
2024-04-17 0:07 ` [PATCH 1/2] dt-bindings: hwmon: adm1275: add adm1281 Jose Ramon San Buenaventura
2024-04-17 15:15 ` Conor Dooley
2024-04-17 0:07 ` [PATCH 2/2] hwmon: pmbus: adm1275: add adm1281 support Jose Ramon San Buenaventura
2024-04-17 0:32 ` Guenter Roeck
2024-04-17 15:24 ` Guenter Roeck
2024-04-18 8:31 ` SanBuenaventura, Jose
2024-04-18 11:55 ` Guenter Roeck
2024-04-19 2:46 ` SanBuenaventura, Jose
2024-04-19 12:33 ` Guenter Roeck [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=902d73a2-e46a-4f60-bde2-3badc46863da@roeck-us.net \
--to=linux@roeck-us.net \
--cc=Delphine_CC_Chiu@wiwynn.com \
--cc=Jose.SanBuenaventura@analog.com \
--cc=conor+dt@kernel.org \
--cc=corbet@lwn.net \
--cc=devicetree@vger.kernel.org \
--cc=jdelvare@suse.com \
--cc=krzk+dt@kernel.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux-i2c@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=robh@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).