Received-SPF: pass (google.com: domain of linux-kernel+bounces-151412-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Sender: Guenter Roeck <groeck7@gmail.com>
Date: Fri, 19 Apr 2024 05:33:18 -0700
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
Message-ID: <902d73a2-e46a-4f60-bde2-3badc46863da@roeck-us.net>
References: <20240417000722.919-1-jose.sanbuenaventura@analog.com>
 <20240417000722.919-3-jose.sanbuenaventura@analog.com>
 <b36db2c0-db31-4304-8e58-aa358ab811c5@roeck-us.net>
 <62f878f4-a4fb-4e3c-8eec-d1be5ba165a4@roeck-us.net>
 <PH0PR03MB66070CAE5E8D99158003D58FEC0E2@PH0PR03MB6607.namprd03.prod.outlook.com>
 <1221f2fd-758e-4c10-8551-ed571fb1577f@roeck-us.net>
 <SJ0PR03MB66008186E17D3920B5B58B92EC0D2@SJ0PR03MB6600.namprd03.prod.outlook.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
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