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

Andy Shevchenko
2022-07-01T10:05:28Z

On Wed, Jun 29, 2022 at 4:23 AM cy_huang <u0084500@gmail.com> wrote:

> Add Richtek rtq6056 supporting.
>
> It can be used for the system to monitor load current and power with 16-bit
> resolution.

...

> +static int rtq6056_adc_read_channel(struct rtq6056_priv *priv,
> + struct iio_chan_spec const *ch,
> + int *val)
> +{
> + struct device *dev = priv->dev;
> + unsigned int addr = ch->address;
> + unsigned int regval;
> + int ret;
> +
> + pm_runtime_get_sync(dev);
> +
> + ret = regmap_read(priv->regmap, addr, &regval);
> + if (ret) {
> + pm_runtime_put(dev);
> + return ret;
> + }

You can optimize this to

pm_runtime_get_sync(dev);
ret = regmap_read(priv->regmap, addr, &regval);
pm_runtime_mark_last_busy(dev);
pm_runtime_put(dev);
if (ret)
return ret;

> + /* Power and VBUS is unsigned 16-bit, others are signed 16-bit */
> + if (addr == RTQ6056_REG_BUSVOLT || addr == RTQ6056_REG_POWER)
> + *val = regval;
> + else
> + *val = sign_extend32(regval, 16);

> + pm_runtime_mark_last_busy(dev);
> + pm_runtime_put(dev);

...and get rid of these.

> + return IIO_VAL_INT;
> +}

...

> + *val2 = 1000000000;

NANO ?

...

> + *val2 = 1000;

MILLI ?

> + *val = DIV_ROUND_UP(1000000, sample_time);

USEC_PER_SEC ?

> +
> + return IIO_VAL_INT;
> +}

...

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

...

> + /* calibration = 5120000 / (Rshunt (uohm) * current lsb (1mA)) */

uOhm

...

> +static ssize_t shunt_resistor_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct rtq6056_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + int vals[2] = { priv->shunt_resistor_uohm, 1000000 };

MICRO ?

> + return iio_format_value(buf, IIO_VAL_FRACTIONAL, 1, vals);
> +}

...

> + ret = rtq6056_set_shunt_resistor(priv, val * 1000000 + val_fract);

MICRO ?

> + if (ret)
> + return ret;

...

> + struct {
> + u16 vals[RTQ6056_MAX_CHANNEL];
> + int64_t timestamp;
> + } data __aligned(8);

Hmm... alignment of this struct will be at least 4 bytes, but
shouldn't we rather be sure that the timestamp member is aligned
properly? Otherwise this seems fragile and dependent on
RTQ6056_MAX_CHANNEL % 4 == 0.

...

> + pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
> + pm_runtime_use_autosuspend(dev);
> + pm_runtime_set_active(dev);
> + pm_runtime_mark_last_busy(dev);
> + pm_runtime_enable(dev);
> +
> + /* By default, use 2000 micro-ohm resistor */
> + shunt_resistor_uohm = 2000;
> + device_property_read_u32(dev, "shunt-resistor-micro-ohms",
> + &shunt_resistor_uohm);
> +
> + ret = rtq6056_set_shunt_resistor(priv, shunt_resistor_uohm);
> + if (ret) {
> + dev_err(dev, "Failed to init shunt resistor\n");
> + goto err_probe;

return dev_err_probe();

(see below how)

> + }
> +
> + 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->info = &rtq6056_info;
> +
> + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> + rtq6056_buffer_trigger_handler,
> + NULL);
> + if (ret) {
> + dev_err(dev, "Failed to allocate iio trigger buffer\n");

Ditto.

> + goto err_probe;

It is a sign of wrong ordering, either do not use devm_ calls after
non-devm_ or make the latter wrapped into devm_add_action_or_reset().
See below for additional information.

> + }
> +
> + ret = devm_iio_device_register(dev, indio_dev);
> + if (ret) {
> + dev_err(dev, "Failed to allocate iio device\n");
> + goto err_probe;
> + }
> +
> + return 0;
> +
> +err_probe:
> + pm_runtime_dont_use_autosuspend(dev);
> + pm_runtime_disable(dev);
> + pm_runtime_set_suspended(dev);
> +
> + return ret;

...

> +static int rtq6056_remove(struct i2c_client *i2c)
> +{
> + struct device *dev = &i2c->dev;

Another (but usually not good option) is to call devm_..._unregister() here.

> + pm_runtime_dont_use_autosuspend(dev);
> + pm_runtime_disable(dev);
> + pm_runtime_set_suspended(dev);
> +
> + return 0;
> +}

...

> +static const struct dev_pm_ops rtq6056_pm_ops = {
> + SET_RUNTIME_PM_OPS(rtq6056_runtime_suspend, rtq6056_runtime_resume, NULL)

RUNTIME_PM_OPS()

> +};

...

> +static const struct of_device_id rtq6056_device_match[] = {
> + { .compatible = "richtek,rtq6056", },

In this case the inner comma is not needed.

> + {}
> +};

...

> +static struct i2c_driver rtq6056_driver = {
> + .driver = {
> + .name = "rtq6056",
> + .of_match_table = rtq6056_device_match,

> + .pm = &rtq6056_pm_ops,

pm_ptr()

> + },

--
With Best Regards,
Andy Shevchenko