On Tue, 2 Jan 2024 19:28:22 +0800
<cy_huang@xxxxxxxxxxx> wrote:
> From: ChiYuan Huang <cy_huang@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
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);
>
