Re: [PATCH] regulator: fan53555: add back tcs4526

[Peter Geis]

On Wed, May 26, 2021 at 12:23 PM Rudi Heitbaum <rudi@xxxxxxxxxxxx> wrote:
>
>
> For rk3399pro boards the tcs4526 regulator supports the vdd_gpu
> regulator. The tcs4526 regulator has a chip id of <0>.
> Add the compatibile tcs,tcs4526
>
> without this patch, the dmesg output is:
>   fan53555-regulator 0-0010: Chip ID 0 not supported!
>   fan53555-regulator 0-0010: Failed to setup device!
>   fan53555-regulator: probe of 0-0010 failed with error -22
> with this patch, the dmesg output is:
>   vdd_gpu: supplied by vcc5v0_sys
>
> The regulators are described as:
> - Dedicated power management IC TCS4525
> - Lithium battery protection chip TCS4526
>
> This has been tested with a Radxa Rock Pi N10.
>
> Fixes: f9028dcdf589 ("regulator: fan53555: only bind tcs4525 to correct chip id")
> Signed-off-by: Rudi Heitbaum <rudi@xxxxxxxxxxxx>

Considering the TCS4525 wasn't supported prior to its recent addition,
and the TCS4526 wasn't supported by the driver at all, this isn't a
fix but a feature addition.
Binding only to the correct device ID exists for this reason, to
prevent unsafe voltage setting.

I also don't see the TCS4525/TCS4526 regulators in the current
linux-next device tree for the N10.

> ---
>  drivers/regulator/fan53555.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/regulator/fan53555.c b/drivers/regulator/fan53555.c
> index 2695be617373..ddab9359ea20 100644
> --- a/drivers/regulator/fan53555.c
> +++ b/drivers/regulator/fan53555.c
> @@ -90,6 +90,7 @@ enum {
>  };
>
>  enum {
> +       TCS4525_CHIP_ID_00 = 0,
>         TCS4525_CHIP_ID_12 = 12,

This isn't a TCS4525, but a TCS4526.

>  };
>
> @@ -373,6 +374,7 @@ static int fan53555_voltages_setup_silergy(struct fan53555_device_info *di)
>  static int fan53526_voltages_setup_tcs(struct fan53555_device_info *di)
>  {
>         switch (di->chip_id) {
> +       case TCS4525_CHIP_ID_00:
>         case TCS4525_CHIP_ID_12:
>                 di->slew_reg = TCS4525_TIME;
>                 di->slew_mask = TCS_SLEW_MASK;
> @@ -564,6 +566,9 @@ static const struct of_device_id __maybe_unused fan53555_dt_ids[] = {
>         }, {
>                 .compatible = "tcs,tcs4525",
>                 .data = (void *)FAN53526_VENDOR_TCS
> +       }, {
> +               .compatible = "tcs,tcs4526",
> +               .data = (void *)FAN53526_VENDOR_TCS

Since you aren't adding any functional code, is there a particular
reason you can't just add the chip id and simply use the tcs4525
compatible?
This will prevent you from needing to modify the dt-bindings as well.

>         },
>         { }
>  };
> @@ -672,6 +677,9 @@ static const struct i2c_device_id fan53555_id[] = {
>         }, {
>                 .name = "tcs4525",
>                 .driver_data = FAN53526_VENDOR_TCS
> +       }, {
> +               .name = "tcs4526",
> +               .driver_data = FAN53526_VENDOR_TCS
>         },
>         { },
>  };
> --
> 2.29.2
>