Re: [PATCH 20/57] media: atomisp: Fix regulator registers on BYT devices with CRC PMIC

Andy Shevchenko
2023-01-23T17:52:03Z

On Mon, Jan 23, 2023 at 01:51:28PM +0100, Hans de Goede wrote:
> The Crystal Cove PMIC used on some BYT/CHT devices has different revisions
> when paired with Bay Trail (BYT) vs Cherry Trail (CHT) SoCs.
>
> The current hardcoded values are only valid for CHT devices, change
> the code so that it uses the correct register values on both BYT and CHT.

Reviewed-by: Andy Shevchenko <andy@kernel.org>
One nit-pick below.

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> .../media/atomisp/pci/atomisp_gmin_platform.c | 22 +++++++++++++++----
> 1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> index 3d41fab661cf..6116d3c62315 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> @@ -57,8 +57,10 @@ enum clock_rate {
> #define LDO_1P8V_OFF 0x58 /* ... bottom bit is "enabled" */
>
> /* CRYSTAL COVE PMIC register set */
> -#define CRYSTAL_1P8V_REG 0x57
> -#define CRYSTAL_2P8V_REG 0x5d
> +#define CRYSTAL_BYT_1P8V_REG 0x5d
> +#define CRYSTAL_BYT_2P8V_REG 0x66
> +#define CRYSTAL_CHT_1P8V_REG 0x57
> +#define CRYSTAL_CHT_2P8V_REG 0x5d
> #define CRYSTAL_ON 0x63
> #define CRYSTAL_OFF 0x62

I would split to groups like this

#define CRYSTAL_BYT_1P8V_REG 0x5d
#define CRYSTAL_BYT_2P8V_REG 0x66

#define CRYSTAL_CHT_1P8V_REG 0x57
#define CRYSTAL_CHT_2P8V_REG 0x5d

#define CRYSTAL_ON 0x63
#define CRYSTAL_OFF 0x62

> @@ -843,6 +845,7 @@ static int gmin_v1p8_ctrl(struct v4l2_subdev *subdev, int on)
> struct gmin_subdev *gs = find_gmin_subdev(subdev);
> int ret;
> int value;
> + int reg;
>
> if (!gs || gs->v1p8_on == on)
> return 0;
> @@ -898,10 +901,15 @@ static int gmin_v1p8_ctrl(struct v4l2_subdev *subdev, int on)
> LDO10_REG, value, 0xff);
> break;
> case PMIC_CRYSTALCOVE:
> + if (IS_ISP2401)
> + reg = CRYSTAL_CHT_1P8V_REG;
> + else
> + reg = CRYSTAL_BYT_1P8V_REG;
> +
> value = on ? CRYSTAL_ON : CRYSTAL_OFF;
>
> ret = gmin_i2c_write(subdev->dev, gs->pwm_i2c_addr,
> - CRYSTAL_1P8V_REG, value, 0xff);
> + reg, value, 0xff);
> break;
> default:
> dev_err(subdev->dev, "Couldn't set power mode for v1p8\n");
> @@ -918,6 +926,7 @@ static int gmin_v2p8_ctrl(struct v4l2_subdev *subdev, int on)
> struct gmin_subdev *gs = find_gmin_subdev(subdev);
> int ret;
> int value;
> + int reg;
>
> if (WARN_ON(!gs))
> return -ENODEV;
> @@ -974,10 +983,15 @@ static int gmin_v2p8_ctrl(struct v4l2_subdev *subdev, int on)
> LDO9_REG, value, 0xff);
> break;
> case PMIC_CRYSTALCOVE:
> + if (IS_ISP2401)
> + reg = CRYSTAL_CHT_2P8V_REG;
> + else
> + reg = CRYSTAL_BYT_2P8V_REG;
> +
> value = on ? CRYSTAL_ON : CRYSTAL_OFF;
>
> ret = gmin_i2c_write(subdev->dev, gs->pwm_i2c_addr,
> - CRYSTAL_2P8V_REG, value, 0xff);
> + reg, value, 0xff);
> break;
> default:
> dev_err(subdev->dev, "Couldn't set power mode for v2p8\n");
> --
> 2.39.0
>

--
With Best Regards,
Andy Shevchenko