Hi, Johathan:
Most comments are good and will be fixed in next revision.
Still one comment I cannot make sure.
Please see the comment that's below yours.
On Sat, Dec 30, 2023 at 12:03:47PM +0000, Jonathan Cameron wrote:
> On Thu, 28 Dec 2023 19:29:35 +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>
> Hi ChiYuan,
>
> Some comments inline, most focus on not mixing device specific features
> across code and data. You always want them to be fully specified by
> the the device specific const struct directly not by code using an
> ID from there. It ends up more readable and more flexible to have it
> all done via data or callbacks where things are a too complex for data.
>
> Thanks,
>
> Jonathan
>
> > ---
> > v2
> > - Remove rtq6053 in DT match table and make rtq6053 fallback compatible
> > with rtq6056
> > ---
> > drivers/iio/adc/rtq6056.c | 264 ++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 250 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/iio/adc/rtq6056.c b/drivers/iio/adc/rtq6056.c
> > index ad4cea6839b2..5587178cea83 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
> > +};
> > +
> > enum {
> > RTQ6056_CH_VSHUNT = 0,
> > RTQ6056_CH_VBUS,
> > @@ -50,16 +60,29 @@ enum {
> > enum {
> > F_OPMODE = 0,
> > F_VSHUNTCT,
> > + F_SADC = F_VSHUNTCT,
>
> If the devices have different register fields, better to have different enums
> for them as well as that should result in less confusing code.
>
Actually, this is all the same register, just the control naming difference.
If not to define the new eum, I can remain to use the same field to handle rtq6059 part.
>
> > F_VBUSCT,
> > + F_BADC = F_VBUSCT,
> > F_AVG,
> > + F_PGA = F_AVG,
> > F_RESET,
> > F_MAX_FIELDS
> > };
> >
> > +struct richtek_dev_data {
> > + int dev_id;
>
> It almost always turns out to be a bad idea to use a mixture of
> 'data' in a structure like this and a device id plus special casing int he
> code. Better to add more data to this structure or callbacks specific
> to the individual devices types. So I shouldn't see a dev_id field in
> here at all.
>
> > + int default_conv_time;
> > + unsigned int default_config;
> > + unsigned int calib_coefficient;
> > + const struct reg_field *reg_fields;
> > + const struct iio_chan_spec *channels;
> > +};
>
> ...
>
> > 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 +282,18 @@ 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:
> > + if (devdata->dev_id == RICHTEK_DEV_RTQ6059)
> > + regval >>= RTQ6059_VBUS_LSB_OFFSET;
>
> Store the offset as for other case below and apply it unconditionally. If it is
> zero then this is a noop.
>
> > + fallthrough;
> > + case RTQ6056_REG_POWER:
> > *val = regval;
> > - else
> > + break;
> > + default:
> > *val = sign_extend32(regval, 16);
>
> Fallthrough stuff is harder to read so only use it when there is significant saving
> in code. here just repeat the sign_extend32() in both cases.
>
> > + break;
> > + }
> >
> > return IIO_VAL_INT;
> > }
> > @@ -199,6 +321,28 @@ static int rtq6056_adc_read_scale(struct iio_chan_spec const *ch, int *val,
> > }
> > }
> >
>
> > static int rtq6056_adc_get_sample_freq(struct rtq6056_priv *priv,
> > struct iio_chan_spec const *ch, int *val)
> > {
> > @@ -292,11 +464,15 @@ static int rtq6056_adc_read_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;
> >
> > switch (mask) {
> > case IIO_CHAN_INFO_RAW:
> > return rtq6056_adc_read_channel(priv, chan, val);
> > case IIO_CHAN_INFO_SCALE:
> > + if (devdata->dev_id == RICHTEK_DEV_RTQ6059)
> > + return rtq6059_adc_read_scale(chan, val, val2);
>
> Provide a callback for this as for other examples below.
>
> > +
> > return rtq6056_adc_read_scale(chan, val, val2);
> > case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > *val = priv->avg_sample;
> > @@ -313,16 +489,28 @@ static int rtq6056_adc_read_avail(struct iio_dev *indio_dev,
> > const int **vals, int *type, int *length,
> > long mask)
> > {
> > + struct rtq6056_priv *priv = iio_priv(indio_dev);
> > + const struct richtek_dev_data *devdata = priv->devdata;
> > +
> > switch (mask) {
> > case IIO_CHAN_INFO_SAMP_FREQ:
> > + if (devdata->dev_id == RICHTEK_DEV_RTQ6059)
> > + return -EINVAL;
>
> Shouldn't need this protection as the channels won't have relevant
> bitmap bit set and this will never be called.
>
> > +
> > *vals = rtq6056_samp_freq_list;
> > *type = IIO_VAL_INT;
> > *length = ARRAY_SIZE(rtq6056_samp_freq_list);
> > return IIO_AVAIL_LIST;
> > case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > - *vals = rtq6056_avg_sample_list;
> > + if (devdata->dev_id == RICHTEK_DEV_RTQ6059) {
> > + *vals = rtq6059_avg_sample_list;
> > + *length = ARRAY_SIZE(rtq6059_avg_sample_list);
> Store all this in devdata.
>
> *vals = devdata->avg_sample_list;
> *length = devdata->avg_sample_list_length;
>
> > + } else {
> > + *vals = rtq6056_avg_sample_list;
> > + *length = ARRAY_SIZE(rtq6056_avg_sample_list);
> > + }
> > +
> > *type = IIO_VAL_INT;
> > - *length = ARRAY_SIZE(rtq6056_avg_sample_list);
> > return IIO_AVAIL_LIST;
> > default:
> > return -EINVAL;
> > @@ -334,6 +522,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 +531,16 @@ 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->dev_id == RICHTEK_DEV_RTQ6059)
> > + ret = -EINVAL;
> > + else
> > + ret = rtq6056_adc_set_samp_freq(priv, chan, val);
> > break;
> > case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > - ret = rtq6056_adc_set_average(priv, val);
> > + if (devdata->dev_id == RICHTEK_DEV_RTQ6059)
> > + ret = rtq6059_adc_set_average(priv, val);
> > + else
> > + ret = rtq6056_adc_set_average(priv, val);
>
> Provide a callback in devdata so this becomes something like
> ret = devdata->set_averate(priv, val);
>
> > break;
> > default:
>
> > +static const struct iio_info rtq6059_info = {
> > + .attrs = &rtq6056_attribute_group,
>
> This is odd. you don't provide an access functions so you won't be able to read
> channels etc. It isn't used so I guess you should just get rid of it.
>
> > +};
> > +
> > static irqreturn_t rtq6056_buffer_trigger_handler(int irq, void *p)
> > {
> > struct iio_poll_func *pf = p;
> > struct iio_dev *indio_dev = pf->indio_dev;
> > struct rtq6056_priv *priv = iio_priv(indio_dev);
> > + const struct richtek_dev_data *devdata = priv->devdata;
> > struct device *dev = priv->dev;
> > struct {
> > u16 vals[RTQ6056_MAX_CHANNEL];
> > @@ -469,6 +670,10 @@ static irqreturn_t rtq6056_buffer_trigger_handler(int irq, void *p)
> > if (ret)
> > goto out;
> >
> > + if (devdata->dev_id == RICHTEK_DEV_RTQ6059 &&
> > + addr == RTQ6056_REG_BUSVOLT)
>
> Store an offset in the devdata->dev_id and this becomes something like.
> if (addr == RTQ6056_REG_BUS_VOLT)
> raw >>= devdata->vbus_offset;
>
> > + raw >>= RTQ6059_VBUS_LSB_OFFSET;
> > +
> > data.vals[i++] = raw;
> > }
> >
> > @@ -528,20 +733,26 @@ static int rtq6056_probe(struct i2c_client *i2c)
> > struct rtq6056_priv *priv;
> > struct device *dev = &i2c->dev;
> > struct regmap *regmap;
> > + const struct richtek_dev_data *devdata;
> > unsigned int vendor_id, shunt_resistor_uohm;
> > int ret;
> >
> > if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_SMBUS_WORD_DATA))
> > return -EOPNOTSUPP;
> >
> > + devdata = device_get_match_data(dev);
> > + if (!devdata)
> > + return dev_err_probe(dev, -EINVAL, "Invalid dev data\n");
> > +
> > indio_dev = devm_iio_device_alloc(dev, sizeof(*priv));
> > if (!indio_dev)
> > return -ENOMEM;
> >
> > priv = iio_priv(indio_dev);
> > priv->dev = dev;
> > - priv->vshuntct_us = priv->vbusct_us = 1037;
> > + priv->vshuntct_us = priv->vbusct_us = devdata->default_conv_time;
>
> I'd keep a _us postfix for default_conv_time to make the units of that
> self documenting as well.
>
> > priv->avg_sample = 1;
> > + priv->devdata = devdata;
> > i2c_set_clientdata(i2c, priv);
> >
> > regmap = devm_regmap_init_i2c(i2c, &rtq6056_regmap_config);
> > @@ -556,20 +767,26 @@ static int rtq6056_probe(struct i2c_client *i2c)
> > return dev_err_probe(dev, ret,
> > "Failed to get manufacturer info\n");
> >
> > + /* For RTQ6059, this vendor id value is meaningless */
>
> If that is the case, why are you checking it?
> I'd read that comment as meaning this will fail for RTQ6059
>
> > if (vendor_id != RTQ6056_VENDOR_ID)
> > return dev_err_probe(dev, -ENODEV,
> > "Invalid vendor id 0x%04x\n", vendor_id);
> >
> > ret = devm_regmap_field_bulk_alloc(dev, regmap, priv->rm_fields,
> > - rtq6056_reg_fields, F_MAX_FIELDS);
> > + devdata->reg_fields, F_MAX_FIELDS);
> > if (ret)
> > return dev_err_probe(dev, ret, "Failed to init regmap field\n");
> >
> > /*
> > + * RTQ6053 & RTQ6056:
> > * By default, configure average sample as 1, bus and shunt conversion
> > * time as 1037 microsecond, and operating mode to all on.
> > + *
> > + * RTQ6059:
> > + * By default, configure average sample as 1, bus and shunt conversion
> > + * time as 532 microsecond, and operating mode to all on.
> Move this documentation to where devdata->default_config is set.
> It's device specific information, so put it in the device specific place not
> the main code flow.
>
> > */
> > - ret = regmap_write(regmap, RTQ6056_REG_CONFIG, RTQ6056_DEFAULT_CONFIG);
> > + ret = regmap_write(regmap, RTQ6056_REG_CONFIG, devdata->default_config);
> > if (ret)
> > return dev_err_probe(dev, ret,
> > "Failed to enable continuous sensing\n");
> > @@ -598,8 +815,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;
>
> I'd embed the number of channels in devdata as well then the ARRAY_SIZE() code
> that is obviously correct can still be used (just with different things being
> counted depending on which channels are being used). The gain in readability
> is worth the tiny bit of extra code and data.
>
> > indio_dev->info = &rtq6056_info;
> >
> > ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> > @@ -640,8 +857,27 @@ static int rtq6056_runtime_resume(struct device *dev)
> > static DEFINE_RUNTIME_DEV_PM_OPS(rtq6056_pm_ops, rtq6056_runtime_suspend,
> > rtq6056_runtime_resume, NULL);
> >
> > +static const struct richtek_dev_data rtq6056_devdata = {
> > + .dev_id = RICHTEK_DEV_RTQ6056,
> > + .default_conv_time = 1037,
> > + .calib_coefficient = 5120000,
> > + .default_config = RTQ6056_DEFAULT_CONFIG,
> > + .reg_fields = rtq6056_reg_fields,
> > + .channels = rtq6056_channels,
> > +};
> > +
> > +static const struct richtek_dev_data rtq6059_devdata = {
> > + .dev_id = RICHTEK_DEV_RTQ6059,
> As mentioned above, this mix of data and a dev_id is not a good design pattern.
> It tends to end up as insufficiently flexible as support for more devices is
> added to a driver - plus it scatters the device type specific stuff all through
> the driver rather than having it all in one place.
>
> > + .default_conv_time = 532,
> > + .calib_coefficient = 40960000,
> > + .default_config = RTQ6059_DEFAULT_CONFIG,
> > + .reg_fields = rtq6059_reg_fields,
> > + .channels = rtq6059_channels,
> > +};
> > +
> > static const struct of_device_id rtq6056_device_match[] = {
> > - { .compatible = "richtek,rtq6056" },
> > + { .compatible = "richtek,rtq6056", .data = &rtq6056_devdata },
> > + { .compatible = "richtek,rtq6059", .data = &rtq6059_devdata },
> > {}
> > };
> > MODULE_DEVICE_TABLE(of, rtq6056_device_match);
>
