Received-SPF: pass (google.com: domain of linux-kernel+bounces-150947-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
From: "SanBuenaventura, Jose" <Jose.SanBuenaventura@analog.com>
To: Guenter Roeck <linux@roeck-us.net>
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
Thread-Topic: [PATCH 2/2] hwmon: pmbus: adm1275: add adm1281 support
Thread-Index: AQHakFtauod9nx2ctkmgfFu2ML+2/rFrnNIAgAD5SICAARPjQIAAQ8cAgADsb6A=
Date: Fri, 19 Apr 2024 02:46:15 +0000
Message-ID: 
 <SJ0PR03MB66008186E17D3920B5B58B92EC0D2@SJ0PR03MB6600.namprd03.prod.outlook.com>
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>
In-Reply-To: <1221f2fd-758e-4c10-8551-ed571fb1577f@roeck-us.net>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: 
 =?us-ascii?Q?tTLoc7ysxU4BlQ7p6YYOHT2BNBFD02wREeo7cSYvB21IhqFG5tKk+QMJVUEf?=
 =?us-ascii?Q?9WrpRCQlmp7x5yrcmAdKXXmyGL4HrFGBbVswBm4qHcfyNk0FWxkXDJ4zWroA?=
 =?us-ascii?Q?62jmR13kTDvjEMyA3a3TUnQetFnRTIpoDBTMQrGoI5CihKpAbQyzjNZHJuqv?=
 =?us-ascii?Q?jYACZfv/WG8KzINCTXw/Knv4MROLDQBrukwEUYRtVd+GXhXXrvLRkhGWsr7l?=
 =?us-ascii?Q?PP0XPaA0j/LNzKThMbwG5Iw+2E+3tzCW+0/ibN4Wk+ng67CPKHxD993LPxGR?=
 =?us-ascii?Q?nRwCEQo5nFXTfpoz15oZtTgG9oQZLhxU1hnz/csQiEZa81tdvYfGCXuv932V?=
 =?us-ascii?Q?HOIQws54NLBDdc49QidxCRpKFyTmXzcbHoOrxQHA3TFp8dqvxEhJh+4BEfaJ?=
 =?us-ascii?Q?YMttB1t6miallU9aii1n8gn4LWm75mWNNoeehSJb6nRId4SKot83DFq+zPnC?=
 =?us-ascii?Q?7jRNvneB5+3xNeS8vBv+CrO7y9J66EnA06akwgTXB42OGvLd6MfSOylv3sg7?=
 =?us-ascii?Q?u2AX+/WOWPJq1HawDAp8z7VmtYHpPCww/RsrP8DChPjlFZIfOZhdAyiQyrRM?=
 =?us-ascii?Q?/jN3QtoaanOcN5QEb3k7uPpDwhi4y9gQ5YtdQE8DjyMzG4VGCF1KS6Ph2br6?=
 =?us-ascii?Q?T8RsBeINrZA+IBB7RSLC0BEcyjlVrtLezgpm6oxKf5unY9naIzx0TmKuRzWX?=
 =?us-ascii?Q?1BOxluZ1kaTnfc47851hA/2spcdsxPJFaCVYEnc9acNvmAi98nhwMxn7heOV?=
 =?us-ascii?Q?9WjoX0THgDj+Bn+FzrFm4ST73ZEWy2tlDLD3ZcZlViB+Vw0boF4W+OwFKVLc?=
 =?us-ascii?Q?cil6/xR0dNCGcVkBDSBNfVozspNufxX6iisfD7dskmUv4t7bm7JEY2hdiDbD?=
 =?us-ascii?Q?/n/othLfQNA5bW3IPYRvmPwmQrZCk5MQbaecRi6ZyOeiM9ineZexrWlIzm2I?=
 =?us-ascii?Q?ULEBfrvjY5masvoxplIGyPbBuEsSbnS8cN4R7Jb/1+fTempH/l0yDPe8laTv?=
 =?us-ascii?Q?gyPaI3CMhPLbh1am6l6LzwmOHUqaDyx+JotD4sv89xjH3o3fWbWYRP5y07Ca?=
 =?us-ascii?Q?+GUJAtjsYFIMUj3N8EHI/0wPJQZNTfXKsD4M3I+VV81SykGaloQaqmk18Hu5?=
 =?us-ascii?Q?x/5J6FBdrRwSC0U8G7ZHUro0BLzo3D5Zvx8IKCkbf3s8y1mq976Ssv1UBoLT?=
 =?us-ascii?Q?wSH4cslJhRUQaDo4sAV5breoKpmZuLmpCEtSwS+IcYV34ysyRHYlk99aEEgr?=
 =?us-ascii?Q?fH0aBl8f/xxyW+OtZr7G3kKjA8eM66fjSKV7pgx9A9Q/A80k4KUkQJKsVy+X?=
 =?us-ascii?Q?Kl/u++1m13yfxbdPVad6FlqZyRW1fpRp6V8q3ul4ti96J1vT2pPRPbaZohbo?=
 =?us-ascii?Q?oeEGgCJ1opMqlXVXMTonNhL0GNsJuD9rxbf1MQQ1szoEzM/vFWpmsMwOPyn5?=
 =?us-ascii?Q?LKX0cjTECotz5UCk73zLyx2FIbAleLiTfLHek2j/493JJGhf6jRjCocRczM6?=
 =?us-ascii?Q?Lh8bqrhc7EHwTtwHqqAZBC8l5qqLtWFXJgozVqSVUdbcDdEnevlxvc+BJvAt?=
 =?us-ascii?Q?/TC5LGzAdTrVZu5nMqWEPmNcXF+oABRKJsIYSk+tatwsKxvi3tXAUeOsAj4I?=
 =?us-ascii?Q?3Q=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-OriginatorOrg: analog.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR03MB6600.namprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5a6d3bb0-b54d-463e-41de-08dc601ae23d
X-MS-Exchange-CrossTenant-originalarrivaltime: 19 Apr 2024 02:46:15.9417
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: eaa689b4-8f87-40e0-9c6f-7228de4d754a
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 5Khr6YLsC+DYLCv5DLHa3J1FitO791pNbRvgGuCt/CNbkjBjkHCgq6j9/9YzWYcdKSff8CC5qGRz94EIyNDtt+lpvefBkLcl7Leb2tqEJlE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR03MB6545
X-Proofpoint-ORIG-GUID: 8ZhvK84037sUy3-4L5KridXIlgf_pcqN
X-Proofpoint-GUID: 8ZhvK84037sUy3-4L5KridXIlgf_pcqN
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-04-18_22,2024-04-17_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 clxscore=1015 suspectscore=0
 malwarescore=0 priorityscore=1501 bulkscore=0 mlxscore=0 phishscore=0
 mlxlogscore=999 lowpriorityscore=0 spamscore=0 adultscore=0
 impostorscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.19.0-2404010003 definitions=main-2404190019

> -----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
>=20
> [External]
>=20
> 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 regis=
ter
> with
> > another device shouldn't be permitted.
>   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>=20
> Why ? Sure, doing so causes the CML bit to be set, but the PMBus core use=
s
> that method throughout to determine if a command/register is supported.
> There are exceptions - some chips react badly if an attempt is made to re=
ad
> 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.
>=20
> > It seems though that the functionality is redundant and is already hand=
led
> 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.
>=20
> 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.
>=20
> Thanks,
> Guenter

Based on the feedback, it seems that the lines mentioned can be removed sin=
ce
the pmbus_core.c handles the checking of status_cml through different funct=
ions
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 wa=
s 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=20
PB_CML_FAULT_PACKET_ERROR that were not used.=20

Given these observations, it would again seem right to eliminate the lines =
you=20
pointed out since those items are already handled by the pmbus core and tha=
t
the status0_cml debugfs entry can be probed to check the register content d=
irectly
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 co=
re=20
and not on the chip driver itself.

I hope that I understood correctly the points you brought up and identified=
 the=20
possible next step to address those which is to eliminate the added case in=
 the=20
adm1275_read_byte_data.

Thanks,
Joram=20