On Tue, Jan 02, 2024 at 07:53:40PM +0000, Jonathan Cameron wrote:
> 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.
OK. I'll send the new one after all the answers were got.
>
> Anyhow, some follow up comments and one or two things I didn't notice before.
>
All wii be fixed in v4.
> 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);
> >
