Re: [PATCH v1] hwmon: Add RAA229621 for renesas spec

[Guenter Roeck <linux@xxxxxxxxxxxx>]

On 12/25/24 01:33, ChiangBrian 江泳緻 TAO wrote:
> From: Brian Chiang<chiang.brian@xxxxxxxxxxxx>
>
> According to the RAA229621 datasheet, add support for reading.

Please read and follow Documentation/process/submitting-patches.rst,
"The canonical patch format".

Neither subject nor description meet the requirements listed in that
document. Subject should be something like "hwmon: (pmbus/isl68137)
Add support for RAA229621", and the description should be something like

 RAA229621 is a <describe chip here>.
 Add support for it to the isl68137 driver.

> Signed-off-by: Brian Chiang<chiang.brian@xxxxxxxxxxxx>
> --- This modification is added RAA229621 chip to isl68137 list ---
> drivers/hwmon/pmbus/isl68137.c        | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/hwmon/pmbus/isl68137.c b/drivers/hwmon/pmbus/isl68137.c
> index 2af921039309..43e860d95280 100644
> --- a/drivers/hwmon/pmbus/isl68137.c
> +++ b/drivers/hwmon/pmbus/isl68137.c
> @@ -63,6 +63,7 @@ enum chips {
>          raa228228,
>          raa229001,
>          raa229004,
> +       raa229621,
>   };
>
>   enum variants {
> @@ -72,6 +73,7 @@ enum variants {
>          raa_dmpvr2_2rail_nontc,
>          raa_dmpvr2_3rail,
>          raa_dmpvr2_hv,
> +       raa_dmpvr2_hvt,
>   };
>
>   struct isl68137_channel {
> @@ -412,6 +414,12 @@ static int isl68137_probe(struct i2c_client *client)
>                  info->read_word_data = raa_dmpvr2_read_word_data;
>                  info->write_word_data = raa_dmpvr2_write_word_data;
>                  break;
> +       case raa_dmpvr2_hvt:
> +               info->pages = 2;
> +               info->func[0] &= ~PMBUS_HAVE_TEMP3;
> +               info->func[1] &= ~PMBUS_HAVE_TEMP3;
> +               info->read_word_data = raa_dmpvr2_read_word_data;

I don't have a datasheet, but it seems unlikely that the write
command does not need an override. Also, it is somewhat unlikely
that m/R do not need to be set if the chip is indeed similar to
raa_dmpvr2_hv.

Otherwise, please explain why the existing raa_dmpvr2_2rail_nontc
variant would not work.

> +               break;
>          default:
>                  return -ENODEV;
>          }
> @@ -465,6 +473,7 @@ static const struct i2c_device_id raa_dmpvr_id[] = {
>          {"raa228228", raa_dmpvr2_2rail_nontc},
>          {"raa229001", raa_dmpvr2_2rail},
>          {"raa229004", raa_dmpvr2_2rail},
> +       {"raa229621", raa_dmpvr2_hvt},
>          {}
>   };