Received-SPF: pass (google.com: domain of linux-kernel+bounces-14785-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Tue, 2 Jan 2024 19:53:40 +0000
From: Jonathan Cameron <jic23@kernel.org>
To: <cy_huang@richtek.com>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley
 <conor+dt@kernel.org>, Lars-Peter Clausen <lars@metafoo.de>, Rob Herring
 <robh+dt@kernel.org>, Uwe =?UTF-8?B?S2xlaW5lLUvDtm5pZw==?=
 <u.kleine-koenig@pengutronix.de>, <linux-iio@vger.kernel.org>,
 <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 2/2] iio: adc: rtq6056: Add support for the whole
 RTQ6056 family
Message-ID: <20240102195340.33de9a02@jic23-huawei>
In-Reply-To: <bc97e776171d589c9d97541cc7ce8d74c899bc92.1704189363.git.cy_huang@richtek.com>
References: <cover.1704189363.git.cy_huang@richtek.com>
	<bc97e776171d589c9d97541cc7ce8d74c899bc92.1704189363.git.cy_huang@richtek.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Tue, 2 Jan 2024 19:28:22 +0800
<cy_huang@richtek.com> wrote:

> From: ChiYuan Huang <cy_huang@richtek.com>
> 
> RTQ6053 and RTQ6059 are the same series of RTQ6056.
> 
> The respective differences with RTQ6056 are listed below
> RTQ6053
> - chip package type
> 
> RTQ6059
> - Reduce the pinout for vbus sensing pin
> - Some internal ADC scaling change
> 
> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>

I replied to your question on v2. As a general rule don't send a new version
out quite so quickly.  Better to let all questions be answered (or a week or so
to pass) before sending a new version.  That way it saves on review time.

Anyhow, some follow up comments and one or two things I didn't notice before.

Jonathan

> ---
> v3
> - Resotre the enum for control field.
> - Put all the predefined datas/callbacks in dev_data.
> - Remove the unused 'rtq6059_info'.
> - Change 'default_conv_time' to 'default_conv_time_us'.
> - Move the comment for default config above the dev_data setting line.
> 
> v2
> - Remove rtq6053 in DT match table and make rtq6053 fallback compatible
>   with rtq6056
> ---
>  drivers/iio/adc/rtq6056.c | 270 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 251 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/iio/adc/rtq6056.c b/drivers/iio/adc/rtq6056.c
> index ad4cea6839b2..c87d78e161ce 100644
> --- a/drivers/iio/adc/rtq6056.c
> +++ b/drivers/iio/adc/rtq6056.c
> @@ -39,6 +39,16 @@
>  #define RTQ6056_DEFAULT_CONFIG	0x4127
>  #define RTQ6056_CONT_ALLON	7
>  
> +#define RTQ6059_DEFAULT_CONFIG	0x3C47
> +#define RTQ6059_VBUS_LSB_OFFSET	3
> +#define RTQ6059_AVG_BASE	8
> +
> +enum {
> +	RICHTEK_DEV_RTQ6056 = 0,
> +	RICHTEK_DEV_RTQ6059,
> +	RICHTEK_DEV_MAX
> +};
> +

No longer used so drop this enum.

>  static int rtq6056_adc_read_channel(struct rtq6056_priv *priv,
>  				    struct iio_chan_spec const *ch,
>  				    int *val)
>  {
> +	const struct richtek_dev_data *devdata = priv->devdata;
>  	struct device *dev = priv->dev;
>  	unsigned int addr = ch->address;
>  	unsigned int regval;
> @@ -168,10 +287,21 @@ static int rtq6056_adc_read_channel(struct rtq6056_priv *priv,
>  		return ret;
>  
>  	/* Power and VBUS is unsigned 16-bit, others are signed 16-bit */
> -	if (addr == RTQ6056_REG_BUSVOLT || addr == RTQ6056_REG_POWER)
> +	switch (addr) {
> +	case RTQ6056_REG_BUSVOLT:
> +		regval >>= devdata->vbus_offset;
>  		*val = regval;
> -	else
> +		break;
Can just return directly here
		return IIO_VAL_INT;
> +	case RTQ6056_REG_POWER:
> +		*val = regval;
> +		break;
		return IIO_VAL_INT;
> +	case RTQ6056_REG_SHUNTVOLT:
> +	case RTQ6056_REG_CURRENT:
>  		*val = sign_extend32(regval, 16);
> +		break;
		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
>  
>  	return IIO_VAL_INT;
And drop this.
>  }
> @@ -199,6 +329,28 @@ static int rtq6056_adc_read_scale(struct iio_chan_spec const *ch, int *val,
>  	}
>  }
>  

> @@ -334,6 +518,7 @@ static int rtq6056_adc_write_raw(struct iio_dev *indio_dev,
>  				 int val2, long mask)
>  {
>  	struct rtq6056_priv *priv = iio_priv(indio_dev);
> +	const struct richtek_dev_data *devdata = priv->devdata;
>  	int ret;
>  
>  	ret = iio_device_claim_direct_mode(indio_dev);
> @@ -342,10 +527,13 @@ static int rtq6056_adc_write_raw(struct iio_dev *indio_dev,
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> -		ret = rtq6056_adc_set_samp_freq(priv, chan, val);
> +		if (devdata->fixed_samp_freq)
> +			ret = -EINVAL;

Slight preference here for following form to make it easy to see nothing is done
in error paths beyond breaking out of the switch statement.

		if (devdata->fixed_samp_freq) {
			ret = -EINVAL;
			break;
		}

		ret = rtq....
		break;

> +		else
> +			ret = rtq6056_adc_set_samp_freq(priv, chan, val);
>  		break;
>  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> -		ret = rtq6056_adc_set_average(priv, val);
> +		ret = devdata->set_average(priv, val);
>  		break;
>  	default:
>  		ret = -EINVAL;
;
> @@ -598,8 +793,8 @@ static int rtq6056_probe(struct i2c_client *i2c)
>  
>  	indio_dev->name = "rtq6056";
>  	indio_dev->modes = INDIO_DIRECT_MODE;
> -	indio_dev->channels = rtq6056_channels;
> -	indio_dev->num_channels = ARRAY_SIZE(rtq6056_channels);
> +	indio_dev->channels = devdata->channels;
> +	indio_dev->num_channels = RTQ6056_MAX_CHANNEL + 1;

You have added devdata->num_channels but seem to have forgotten to use it.

>  	indio_dev->info = &rtq6056_info;
>  
>  	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> @@ -640,8 +835,45 @@ static int rtq6056_runtime_resume(struct device *dev)
>  static DEFINE_RUNTIME_DEV_PM_OPS(rtq6056_pm_ops, rtq6056_runtime_suspend,
>  				 rtq6056_runtime_resume, NULL);
>