Messages in this thread | | | From | Andy Shevchenko <> | Date | Fri, 17 Jun 2022 19:07:46 +0200 | Subject | Re: [PATCH 2/2] iio: adc: Add rtq6056 support |
| |
On Fri, Jun 17, 2022 at 11:37 AM cy_huang <u0084500@gmail.com> wrote: > > From: ChiYuan Huang <cy_huang@richtek.com> > > 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
| |