Re: [PATCH 2/2] iio: adc: Add rtq6056 support

[Andy Shevchenko]

On Fri, Jun 17, 2022 at 11:37 AM cy_huang <u0084500@xxxxxxxxx> wrote:
>
> From: ChiYuan Huang <cy_huang@xxxxxxxxxxx>
>
> Add Richtek RTQ6056 supporting.
>
> It can be used for the system to monitor load current and power with 16bit

16-bit

> resolution.

Overall looks good, needs some cosmetic work.

...

> +KernelVersion: 5.18.2

Wrong version, this won't be part of a stable kernel.

...

> +#include <linux/of.h>

Any users of this?

But for sure you missed

  mod_devicetable.h
  types.h

...

> +#define RTQ6056_DEFAULT_RSHUNT 2000

_mOHMs ?

...

> +enum {
> +       F_OPMODE = 0, F_VSHUNTCT, F_VBUSCT, F_AVG, F_RESET,
> +       F_MAX_FIELDS

Hard to read this way. Split to be one emum entry per line.

> +};

...

> +struct rtq6056_priv {
> +       struct device *dev;
> +       struct regmap *regmap;

Swapping these two might give less code in the generated binary. Have
you run bloat-o-meter?

> +       struct regmap_field *rm_fields[F_MAX_FIELDS];
> +       u32 shunt_resistor_uohm;
> +       int vshuntct; /* vshunt conversion time in uS */
> +       int vbusct; /* vbus conversion time in uS */
> +       int avg_sample;
> +};

...

> +       IIO_CHAN_SOFT_TIMESTAMP(RTQ6056_MAX_CHANNEL)

Keep a comma.

...


> +       /* Only power and vbus channel is unsigned */
> +       if (channel == RTQ6056_CH_VBUS || channel == RTQ6056_CH_POWER)
> +               *val = regval;
> +       else

> +               *val = (s16)regval;

Why casting? At very minimum this requires a comment.

...

> +       if (val > 8205 || val < 139)
> +               return -EINVAL;

This strange range requires a good comment with possible references to
the datasheet.

...

> +static const int rtq6056_avg_sample_list[] = {
> +       1, 4, 16, 64, 128, 256, 512, 1024

Keep a comma at the end.

> +};

...

> +static int rtq6056_adc_read_label(struct iio_dev *indio_dev,
> +                                 struct iio_chan_spec const *chan,
> +                                 char *label)
> +{
> +       return snprintf(label, PAGE_SIZE, "%s\n",
> +                       rtq6056_channel_labels[chan->channel]);

sysfs_emit()

> +}

...

> +static IIO_DEVICE_ATTR(shunt_resistor, 0644,
> +                      rtq6056_shunt_resistor_show,
> +                      rtq6056_shunt_resistor_store, 0);

IIO_DEVICE_ATTR_RW()

...

> +       for_each_set_bit(bit, indio_dev->active_scan_mask,
> +                        indio_dev->masklength) {

On one line it's better.

> +

Redundant blank line.

> +               ret = regmap_read(priv->regmap, RTQ6056_REG_SHUNTVOLT + bit,
> +                                 &raw);
> +               if (ret)
> +                       goto out;
> +
> +               data.vals[i++] = raw;
> +       }

> +       ret = of_property_read_u32(i2c->dev.of_node,
> +                                  "richtek,shunt-resistor-uohm",
> +                                  &shunt_resistor_uohm);

device_property_read()

> +       if (ret)
> +               shunt_resistor_uohm = RTQ6056_DEFAULT_RSHUNT;

Can be done without branch

... = DEFAULT;
device_property_read_u32(...); // no error checking.

...

> +static int rtq6056_remove(struct i2c_client *i2c)
> +{
> +       struct rtq6056_priv *priv = i2c_get_clientdata(i2c);
> +
> +       /* Config opmode to 'shutdown' mode to minimize quiescient current */

quiescent

> +       return regmap_field_write(priv->rm_fields[F_OPMODE], 0);
> +}
> +
> +static void rtq6056_shutdown(struct i2c_client *i2c)
> +{
> +       struct rtq6056_priv *priv = i2c_get_clientdata(i2c);
> +
> +       /* Config opmode to 'shutdown' mode to minimize quiescient current */

quiescent

> +       regmap_field_write(priv->rm_fields[F_OPMODE], 0);
> +}

-- 
With Best Regards,
Andy Shevchenko