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
