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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 4 Jul 2022 11:16:53 +0800
From:   ChiYuan Huang <u0084500@...il.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Jonathan Cameron <jic23@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        cy_huang <cy_huang@...htek.com>,
        linux-iio <linux-iio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] iio: adc: Add rtq6056 support

Andy Shevchenko <andy.shevchenko@...il.com> 於 2022年7月1日 週五 下午6:05寫道：
>
> On Wed, Jun 29, 2022 at 4:23 AM cy_huang <u0084500@...il.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 ?
>
Yes, unit is 2.5 microvolt. I have all listed all unit comments in the
source code.
> ...
>
> > +               *val2 = 1000;
>
> MILLI ?
>
Yes.
> > +       *val = DIV_ROUND_UP(1000000, sample_time);
>
> USEC_PER_SEC ?
>
No, sample time is (vshunt convesion time + vbus conversion time) *
average sample.
And the sample freq returns the unit by HZ (sample frequency per second)

> > +
> > +       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 ?
>
Yes, for this kind of sense resistor, it will choose 2 milli-Ohm, 1
milli-Ohms,, 0.5 milli-Ohms, or less.
> > +       return iio_format_value(buf, IIO_VAL_FRACTIONAL, 1, vals);
> > +}
>
> ...
>
> > +       ret = rtq6056_set_shunt_resistor(priv, val * 1000000 + val_fract);
>
> MICRO ?
>
Yes
> > +       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.
>
Yap, from the 'max channel', it already guarantee this struct will be
aligned at lease 4.
Actually, It can be removed.
> ...
>
> > +       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.
>
I think the another way is to register all using devm_ and to call the
pm_runtime at the last.
> > +       }
> > +
> > +       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