Re: [PATCH v2] iio: adc: mt6370: Fix ibus and ibat scaling value of some specific vendor ID chips

[Jonathan Cameron]

On Thu, 6 Apr 2023 11:08:51 +0800
ChiaEn Wu <chiaen_wu@xxxxxxxxxxx> wrote:

> The scale value of ibus and ibat on the datasheet is incorrect due to the
> customer report after the experimentation with some specific vendor ID
> chips.
> 
> Signed-off-by: ChiaEn Wu <chiaen_wu@xxxxxxxxxxx>

Hi,

Please give a fixes tag to aid backporting to right stable branches.

A minor formatting suggestion inline.  Otherwise looks fine to me.

Thanks,

Jonathan


> ---
> v2:
> - Move 'get_vendor_info' after iio_priv allocation done
> - For the special case, bypass indent, make it out-of-line
> - For a simple return, add a blank line
> - Drop else
> ---
>  drivers/iio/adc/mt6370-adc.c | 45 ++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/mt6370-adc.c b/drivers/iio/adc/mt6370-adc.c
> index bc62e5a..ae5cdff 100644
> --- a/drivers/iio/adc/mt6370-adc.c
> +++ b/drivers/iio/adc/mt6370-adc.c
> @@ -19,6 +19,7 @@
>  
>  #include <dt-bindings/iio/adc/mediatek,mt6370_adc.h>
>  
> +#define MT6370_REG_DEV_INFO		0x100
>  #define MT6370_REG_CHG_CTRL3		0x113
>  #define MT6370_REG_CHG_CTRL7		0x117
>  #define MT6370_REG_CHG_ADC		0x121
> @@ -27,6 +28,7 @@
>  #define MT6370_ADC_START_MASK		BIT(0)
>  #define MT6370_ADC_IN_SEL_MASK		GENMASK(7, 4)
>  #define MT6370_AICR_ICHG_MASK		GENMASK(7, 2)
> +#define MT6370_VENID_MASK		GENMASK(7, 4)
>  
>  #define MT6370_AICR_100_mA		0x0
>  #define MT6370_AICR_150_mA		0x1
> @@ -47,6 +49,10 @@
>  #define ADC_CONV_TIME_MS		35
>  #define ADC_CONV_POLLING_TIME_US	1000
>  
> +#define MT6370_VID_RT5081		0x8
> +#define MT6370_VID_RT5081A		0xA
> +#define MT6370_VID_MT6370		0xE
> +
>  struct mt6370_adc_data {
>  	struct device *dev;
>  	struct regmap *regmap;
> @@ -55,6 +61,7 @@ struct mt6370_adc_data {
>  	 * from being read at the same time.
>  	 */
>  	struct mutex adc_lock;
> +	unsigned int vid;
>  };
>  
>  static int mt6370_adc_read_channel(struct mt6370_adc_data *priv, int chan,
> @@ -98,6 +105,22 @@ static int mt6370_adc_read_channel(struct mt6370_adc_data *priv, int chan,
>  	return ret;
>  }
>  
> +static int mt6370_adc_get_ibus_scale(struct mt6370_adc_data *priv)
> +{
> +	if (priv->vid == MT6370_VID_RT5081  || priv->vid == MT6370_VID_RT5081A || priv->vid == MT6370_VID_MT6370)
> +		return 3350;
> +
> +	return 3875;
> +}
> +
> +static int mt6370_adc_get_ibat_scale(struct mt6370_adc_data *priv)
> +{
> +	if (priv->vid == MT6370_VID_RT5081  || priv->vid == MT6370_VID_RT5081A || priv->vid == MT6370_VID_MT6370)
Line too long.  You could wrap it, but perhaps even better to just make this a switch
statement as that will be easier to enhance if we have more cases in future without
a lot of churn.

	switch (priv->vid) {
	case MT6370_VID_RT5081:
	case MT6370_VID_RT5081A:
	case MT6370_VID_RT6370:
		return 2680;
	default:
		return 3870; 
	}

Same for the other similar case above.

> +		return 2680;
> +
> +	return 3870;
> +}
> +
>  static int mt6370_adc_read_scale(struct mt6370_adc_data *priv,
>  				 int chan, int *val1, int *val2)
>  {
> @@ -123,7 +146,7 @@ static int mt6370_adc_read_scale(struct mt6370_adc_data *priv,
>  		case MT6370_AICR_250_mA:
>  		case MT6370_AICR_300_mA:
>  		case MT6370_AICR_350_mA:
> -			*val1 = 3350;
> +			*val1 = mt6370_adc_get_ibus_scale(priv);
>  			break;
>  		default:
>  			*val1 = 5000;
> @@ -150,7 +173,7 @@ static int mt6370_adc_read_scale(struct mt6370_adc_data *priv,
>  		case MT6370_ICHG_600_mA:
>  		case MT6370_ICHG_700_mA:
>  		case MT6370_ICHG_800_mA:
> -			*val1 = 2680;
> +			*val1 = mt6370_adc_get_ibat_scale(priv);
>  			break;
>  		default:
>  			*val1 = 5000;
> @@ -251,6 +274,20 @@ static const struct iio_chan_spec mt6370_adc_channels[] = {
>  	MT6370_ADC_CHAN(TEMP_JC, IIO_TEMP, 12, BIT(IIO_CHAN_INFO_OFFSET)),
>  };
>  
> +static int mt6370_get_vendor_info(struct mt6370_adc_data *priv)
> +{
> +	unsigned int dev_info;
> +	int ret;
> +
> +	ret = regmap_read(priv->regmap, MT6370_REG_DEV_INFO, &dev_info);
> +	if (ret)
> +		return ret;
> +
> +	priv->vid = FIELD_GET(MT6370_VENID_MASK, dev_info);
> +
> +	return 0;
> +}
> +
>  static int mt6370_adc_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -272,6 +309,10 @@ static int mt6370_adc_probe(struct platform_device *pdev)
>  	priv->regmap = regmap;
>  	mutex_init(&priv->adc_lock);
>  
> +	ret = mt6370_get_vendor_info(priv);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get vid\n");
> +
>  	ret = regmap_write(priv->regmap, MT6370_REG_CHG_ADC, 0);
>  	if (ret)
>  		return dev_err_probe(dev, ret, "Failed to reset ADC\n");