Re: [PATCH 1/2] hwmon/pmbus: (isl68137) Add support for Renesas RAA228942 and RAA228943

[Guenter Roeck <linux@roeck-us.net>]

On 3/16/26 19:50, Dawei Liu wrote:
> Hi Guenter,
> 
> I understand that enum chips is not used in the device matching
> logic. However, it provides a clear, centralized list of all
> supported chip models for developers and users reviewing the code.
> 
> I added entries there to maintain consistency with recent commits
> that had been accepted. For example:
> - 71a117d28f87 "hwmon: (pmbus/isl68137) Add support for RAA229141"
> - 2190ad55a601 "hwmon: (pmbus/isl68137) add support for Renesas
>    RAA228244 and RAA228246"
> 
> Both commits follow the same pattern of adding to enum chips.
> I think keeping these entries in enum chips maintains code
> consistency and documentation value. However, if you prefer
> not to add them there, I'm happy to adjust in v2.
> 

What you are saying is that I did not notice the problem when reviewing the
previous patches, and that it should be ok to continue the wrong pattern
because of that. Sorry, that is not how things work.

What should be done is to drop the unnecessary enum values, not to add new ones.

Guenter

> Best regards,
> Dawei Liu
> 
> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Monday, March 16, 2026 11:48 PM
> To: Dawei Liu <dawei.liu.jy@renesas.com>
> Cc: linux-hwmon@vger.kernel.org; linux-kernel@vger.kernel.org; linux-doc@vger.kernel.org; devicetree@vger.kernel.org; linux-renesas-soc@vger.kernel.org; robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org; corbet@lwn.net; skhan@linuxfoundation.org; geert+renesas@glider.be; magnus.damm <magnus.damm@gmail.com>; Grant Peltier <grant.peltier.jg@renesas.com>
> Subject: Re: [PATCH 1/2] hwmon/pmbus: (isl68137) Add support for Renesas RAA228942 and RAA228943
> 
> On Mon, Mar 16, 2026 at 01:35:40PM +0800, Dawei Liu wrote:
>> Both RAA228942 and RAA228943 are digital dual-output 16-Phase(X+Y ≤
>> 16) PWM controllers
>>
>> Signed-off-by: Dawei Liu <dawei.liu.jy@renesas.com>
>> ---
>>   Documentation/hwmon/isl68137.rst | 20 ++++++++++++++++++++
>>   drivers/hwmon/pmbus/isl68137.c   |  6 ++++++
>>   2 files changed, 26 insertions(+)
>>
>> diff --git a/Documentation/hwmon/isl68137.rst
>> b/Documentation/hwmon/isl68137.rst
>> index e77f582c2..0ce20d091 100644
>> --- a/Documentation/hwmon/isl68137.rst
>> +++ b/Documentation/hwmon/isl68137.rst
>> @@ -394,6 +394,26 @@ Supported chips:
>>   
>>         Provided by Renesas upon request and NDA
>>   
>> +  * Renesas RAA228942
>> +
>> +    Prefix: 'raa228942'
>> +
>> +    Addresses scanned: -
>> +
>> +    Datasheet:
>> +
>> +      Provided by Renesas upon request and NDA
>> +
>> +  * Renesas RAA228943
>> +
>> +    Prefix: 'raa228943'
>> +
>> +    Addresses scanned: -
>> +
>> +    Datasheet:
>> +
>> +      Provided by Renesas upon request and NDA
>> +
>>     * Renesas RAA229001
>>   
>>       Prefix: 'raa229001'
>> diff --git a/drivers/hwmon/pmbus/isl68137.c
>> b/drivers/hwmon/pmbus/isl68137.c index 78cff9712..da2484d42 100644
>> --- a/drivers/hwmon/pmbus/isl68137.c
>> +++ b/drivers/hwmon/pmbus/isl68137.c
>> @@ -63,6 +63,8 @@ enum chips {
>>   	raa228228,
>>   	raa228244,
>>   	raa228246,
>> +	raa228942,
>> +	raa228943,
> 
> AI:
> 
>    Is it necessary to add these entries to enum chips?
>    Looking at the rest of the driver, this enum does not appear to be used
>    anywhere. The device data mapping seems to rely on the variants enum
>    (e.g., raa_dmpvr2_2rail_nontc) instead.
> 
> It has a point.
> 
> Guenter
> 
>>   	raa229001,
>>   	raa229004,
>>   	raa229141,
>> @@ -478,6 +480,8 @@ static const struct i2c_device_id raa_dmpvr_id[] = {
>>   	{"raa228228", raa_dmpvr2_2rail_nontc},
>>   	{"raa228244", raa_dmpvr2_2rail_nontc},
>>   	{"raa228246", raa_dmpvr2_2rail_nontc},
>> +	{"raa228942", raa_dmpvr2_2rail_nontc},
>> +	{"raa228943", raa_dmpvr2_2rail_nontc},
>>   	{"raa229001", raa_dmpvr2_2rail},
>>   	{"raa229004", raa_dmpvr2_2rail},
>>   	{"raa229141", raa_dmpvr2_2rail_pmbus}, @@ -529,6 +533,8 @@ static
>> const struct of_device_id isl68137_of_match[] = {
>>   	{ .compatible = "renesas,raa228228", .data = (void *)raa_dmpvr2_2rail_nontc },
>>   	{ .compatible = "renesas,raa228244", .data = (void *)raa_dmpvr2_2rail_nontc },
>>   	{ .compatible = "renesas,raa228246", .data = (void
>> *)raa_dmpvr2_2rail_nontc },
>> +	{ .compatible = "renesas,raa228942", .data = (void *)raa_dmpvr2_2rail_nontc },
>> +	{ .compatible = "renesas,raa228943", .data = (void
>> +*)raa_dmpvr2_2rail_nontc },
>>   	{ .compatible = "renesas,raa229001", .data = (void *)raa_dmpvr2_2rail },
>>   	{ .compatible = "renesas,raa229004", .data = (void *)raa_dmpvr2_2rail },
>>   	{ .compatible = "renesas,raa229621", .data = (void
>> *)raa_dmpvr2_2rail },