diff mbox series

[v2,2/2] iio: adc: Add rtq6056 support

Message ID 1656469212-12717-3-git-send-email-u0084500@gmail.com (mailing list archive)
State Superseded
Headers show
Series Add Richtek RTQ6056 support | expand

Commit Message

ChiYuan Huang June 29, 2022, 2:20 a.m. UTC
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 16-bit
resolution.

Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
---
Since v2
- Rename file from 'rtq6056-adc' to 'rtq6056'.
- Refine the ABI, if generic already defined it, remove it and check the channel
  report unit.
- Add copyright text.
- include the correct header.
- change the property parsing name.
- To use iio_chan_spec address field.
- Refine each channel separate and shared_by_all.
- Use pm_runtime and pm_runtime_autosuspend.
- Remove the shutdown callback. From the HW suggestion, it's not recommended to
  use battery as the power supply.
- Check all scale unit (voltage->mV, current->mA, power->milliWatt).
- Use the read_avail to provide the interface for attribute value list.
- Add comma for the last element in the const integer array.
- Refine each ADC label text.
- In read_label callback, replace snprintf to sysfs_emit.

---
 .../ABI/testing/sysfs-bus-iio-adc-rtq6056          |   6 +
 drivers/iio/adc/Kconfig                            |  15 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/rtq6056.c                          | 670 +++++++++++++++++++++
 4 files changed, 692 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056
 create mode 100644 drivers/iio/adc/rtq6056.c

Comments

Andy Shevchenko July 1, 2022, 10:04 a.m. UTC | #1
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()

> +       },
ChiYuan Huang July 4, 2022, 3:16 a.m. UTC | #2
Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年7月1日 週五 下午6:05寫道:
>
> 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 ?
>
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
ChiYuan Huang July 4, 2022, 7:27 a.m. UTC | #3
ChiYuan Huang <u0084500@gmail.com> 於 2022年7月4日 週一 上午11:16寫道:
>
> Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年7月1日 週五 下午6:05寫道:
> >
> > 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 ?
> >
> Yes, unit is 2.5 microvolt. I have all listed all unit comments in the
> source code.a
Sorry, I found this scale is wrong.
For voltage channel, standard binding uses millivolt as the reported value.
From this case, It must be (val * 2500) /1000000
So the '*val2' must equal to 1000000
> > ...
> >
> > > +               *val2 = 1000;
> >
> > MILLI ?
> >
> Yes.
As the above one. Must be millivolt as the unit.
Channel unit is 2.5mV.
This value is correct and equal to (val * 2500) / 1000
> > > +       *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)
>
The 'sample time' is unit by micro-second like as you mentioned.
> > > +
> > > +       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
Andy Shevchenko July 4, 2022, 9:51 p.m. UTC | #4
On Mon, Jul 4, 2022 at 9:27 AM ChiYuan Huang <u0084500@gmail.com> wrote:
> ChiYuan Huang <u0084500@gmail.com> 於 2022年7月4日 週一 上午11:16寫道:
> > Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年7月1日 週五 下午6:05寫道:
> > > On Wed, Jun 29, 2022 at 4:23 AM cy_huang <u0084500@gmail.com> wrote:

...

> > > > +       *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)
> >
> The 'sample time' is unit by micro-second like as you mentioned.

Ah, then it should be MICRO, so we will get Hz.

> > > > +       return IIO_VAL_INT;
> > > > +}

...

> > > > +       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.

I think for the safest side it should be given to the timestamp member. No?
ChiYuan Huang July 5, 2022, 1:41 a.m. UTC | #5
Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年7月5日 週二 清晨5:52寫道:
>
> On Mon, Jul 4, 2022 at 9:27 AM ChiYuan Huang <u0084500@gmail.com> wrote:
> > ChiYuan Huang <u0084500@gmail.com> 於 2022年7月4日 週一 上午11:16寫道:
> > > Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年7月1日 週五 下午6:05寫道:
> > > > On Wed, Jun 29, 2022 at 4:23 AM cy_huang <u0084500@gmail.com> wrote:
>
> ...
>
> > > > > +       *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)
> > >
> > The 'sample time' is unit by micro-second like as you mentioned.
>
> Ah, then it should be MICRO, so we will get Hz.
>
> > > > > +       return IIO_VAL_INT;
> > > > > +}
>
> ...
>
> > > > > +       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.
>
> I think for the safest side it should be given to the timestamp member. No?
>
Sorry, following your comment, Why to use 'align' for the timestamp member?
the data member already guarantee 2 * 4 = 8 byte, then timestamp will
be 8 byte aligned, right?

what you mentioned is to put __aligned(8) only for timestamp.

I try to put aligned in two ways ( one is only for timestamp, another
is the whole struct). the result is the same.
From my thinking, in this case, the struct is already 8 byte aligned
for timestamp member. don't you think to put 'aligned' is redundant?
> --
> With Best Regards,
> Andy Shevchenko
Andy Shevchenko July 5, 2022, 9:04 a.m. UTC | #6
On Tue, Jul 5, 2022 at 3:41 AM ChiYuan Huang <u0084500@gmail.com> wrote:
> Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年7月5日 週二 清晨5:52寫道:
> > On Mon, Jul 4, 2022 at 9:27 AM ChiYuan Huang <u0084500@gmail.com> wrote:
> > > ChiYuan Huang <u0084500@gmail.com> 於 2022年7月4日 週一 上午11:16寫道:
> > > > Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年7月1日 週五 下午6:05寫道:
> > > > > On Wed, Jun 29, 2022 at 4:23 AM cy_huang <u0084500@gmail.com> wrote:

...

> > > > > > +       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.
> >
> > I think for the safest side it should be given to the timestamp member. No?
> >
> Sorry, following your comment, Why to use 'align' for the timestamp member?
> the data member already guarantee 2 * 4 = 8 byte, then timestamp will
> be 8 byte aligned, right?

Today it's true, tomorrow it might be different. Imagine if this
driver will cover a new (version of) hardware and needs an additional
channel, how do you guarantee alignment in that case? So, current
approach is working, but fragile.

> what you mentioned is to put __aligned(8) only for timestamp.

Yes.

> I try to put aligned in two ways ( one is only for timestamp, another
> is the whole struct). the result is the same.
> From my thinking, in this case, the struct is already 8 byte aligned
> for timestamp member. don't you think to put 'aligned' is redundant?

No.
ChiYuan Huang July 5, 2022, 9:30 a.m. UTC | #7
Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年7月5日 週二 下午5:05寫道:
>
> On Tue, Jul 5, 2022 at 3:41 AM ChiYuan Huang <u0084500@gmail.com> wrote:
> > Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年7月5日 週二 清晨5:52寫道:
> > > On Mon, Jul 4, 2022 at 9:27 AM ChiYuan Huang <u0084500@gmail.com> wrote:
> > > > ChiYuan Huang <u0084500@gmail.com> 於 2022年7月4日 週一 上午11:16寫道:
> > > > > Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年7月1日 週五 下午6:05寫道:
> > > > > > On Wed, Jun 29, 2022 at 4:23 AM cy_huang <u0084500@gmail.com> wrote:
>
> ...
>
> > > > > > > +       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.
> > >
> > > I think for the safest side it should be given to the timestamp member. No?
> > >
> > Sorry, following your comment, Why to use 'align' for the timestamp member?
> > the data member already guarantee 2 * 4 = 8 byte, then timestamp will
> > be 8 byte aligned, right?
>
> Today it's true, tomorrow it might be different. Imagine if this
> driver will cover a new (version of) hardware and needs an additional
> channel, how do you guarantee alignment in that case? So, current
> approach is working, but fragile.
>
> > what you mentioned is to put __aligned(8) only for timestamp.
>
> Yes.
>
> > I try to put aligned in two ways ( one is only for timestamp, another
> > is the whole struct). the result is the same.
> > From my thinking, in this case, the struct is already 8 byte aligned
> > for timestamp member. don't you think to put 'aligned' is redundant?
>
> No.
>
Thanks, I think I can get your point. if it need to be compatible with
others, this part will be a trap.
Then it's better to add the align for timestamp member.

I'll submit the v4 for this.

And very sorry about another mail.
I just confused about the text.

> --
> With Best Regards,
> Andy Shevchenko
Andy Shevchenko July 5, 2022, 11:01 a.m. UTC | #8
On Tue, Jul 5, 2022 at 12:33 PM ChiYuan Huang <u0084500@gmail.com> wrote:
> ChiYuan Huang <u0084500@gmail.com> 於 2022年7月5日 週二 下午5:30寫道:

...

> Sorry to bother you by the personal mail.

No problem asking this in ML, it's normal development process.
Moreover it's good that others may browse mail archives and find the
answers. Private mails won't help in that sense. Hence Cc'ing mailing
list again.

> I try to put the 'align' in timestamp member.
> checkpatch.pl show me the below warning.
>
> WARNING: Missing a blank line after declarations
> #448: FILE: drivers/iio/adc/rtq6056.c:448:
> + u16 vals[RTQ6056_MAX_CHANNEL];
> + int64_t timestamp __aligned(8);
>
> WARNING: externs should be avoided in .c files
> #448: FILE: drivers/iio/adc/rtq6056.c:448:
> + int64_t timestamp __aligned(8);
>
> total: 0 errors, 2 warnings, 651 lines checked

You may run `git grep -n 'align(8)'

> Does it mean that I only can put the align for the whole struct?

It means that checkpatch is full of false positives.

Btw, use s64 as a type for timestamp as it's more common.

1. Jonathan, shan't we unify this across the IIO drivers? It seems
they are using (besides s64): int64_t, u64.

2. Also:

drivers/iio/light/vcnl4000.c:911:       u16 buffer[8] __aligned(8)

drivers/iio/light/vcnl4035.c:106:       u8 buffer[ALIGN(sizeof(u16),
sizeof(s64)) + sizeof(s64)]  __aligned(8);

drivers/iio/light/si1145.c:190: u8 buffer[24] __aligned(8);

// misplaced aligned()
drivers/iio/adc/mt6360-adc.c-265-               u16 values[MT6360_CHAN_MAX];
drivers/iio/adc/mt6360-adc.c-266-               int64_t timestamp;
drivers/iio/adc/mt6360-adc.c:267:       } data __aligned(8);

drivers/iio/adc/stm32-adc.c:250:        u16
buffer[STM32_ADC_MAX_SQ + 4] __aligned(8);

drivers/iio/adc/mxs-lradc-adc.c:119:    u32
buffer[10] __aligned(8);

drivers/iio/adc/ti-adc0832.c:37:        u8 data[24] __aligned(8);

drivers/iio/adc/ti-ads124s08.c:108:     u32
buffer[ADS124S08_MAX_CHANNELS + sizeof(s64)/sizeof(u32)] __aligned(8);

drivers/iio/amplifiers/ada4250.c:304:   u8 data[2] __aligned(8) = {};

drivers/iio/chemical/atlas-sensor.c:95: __be32 buffer[6] __aligned(8);

drivers/iio/health/afe4403.c:79:        s32 buffer[8] __aligned(8);

drivers/iio/health/afe4404.c:95:        s32 buffer[10] __aligned(8);

drivers/iio/imu/adis16475.c:107:        __be16
data[ADIS16475_MAX_SCAN_DATA] __aligned(8);

drivers/iio/imu/adis16480.c:174:        __be16
data[ADIS16495_BURST_MAX_DATA] __aligned(8);

drivers/iio/imu/bmi160/bmi160.h:19:     __le16 buf[12] __aligned(8);

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c:568:     u8
iio_buff[ST_LSM6DSX_IIO_BUFF_SIZE] __aligned(8);

// ??? haven't checked context
drivers/iio/light/acpi-als.c:65:        s32
evt_buffer[ACPI_ALS_EVT_BUFFER_SIZE / sizeof(s32)]  __aligned(8);

drivers/iio/light/si1145.c:190: u8 buffer[24] __aligned(8);

drivers/iio/magnetometer/rm3100-core.c:82:      u8
buffer[RM3100_SCAN_BYTES] __aligned(8);

drivers/iio/potentiostat/lmp91000.c:75: u32 buffer[4] __aligned(8);

drivers/iio/pressure/mpl3115.c:160:     u8 buffer[16] __aligned(8);

drivers/iio/proximity/isl29501.c:941:   u32 buffer[4] __aligned(8) =
{}; /* 1x16-bit + naturally aligned ts */


need conversion.

3. Why do we need explicit garbage in the code? :-)

drivers/iio/light/rpr0521.c-204-                __le16 channels[3];
drivers/iio/light/rpr0521.c-205-                u8 garbage;
drivers/iio/light/rpr0521.c:206:                s64 ts __aligned(8);
Jonathan Cameron July 7, 2022, 5:02 p.m. UTC | #9
On Tue, 5 Jul 2022 09:41:39 +0800
ChiYuan Huang <u0084500@gmail.com> wrote:

> Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年7月5日 週二 清晨5:52寫道:
> >
> > On Mon, Jul 4, 2022 at 9:27 AM ChiYuan Huang <u0084500@gmail.com> wrote:  
> > > ChiYuan Huang <u0084500@gmail.com> 於 2022年7月4日 週一 上午11:16寫道:  
> > > > Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年7月1日 週五 下午6:05寫道:  
> > > > > On Wed, Jun 29, 2022 at 4:23 AM cy_huang <u0084500@gmail.com> wrote:  
> >
> > ...
> >  
> > > > > > +       *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)
> > > >  
> > > The 'sample time' is unit by micro-second like as you mentioned.  
> >
> > Ah, then it should be MICRO, so we will get Hz.
> >  
> > > > > > +       return IIO_VAL_INT;
> > > > > > +}  
> >
> > ...
> >  
> > > > > > +       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.  
> >
> > I think for the safest side it should be given to the timestamp member. No?
> >  
> Sorry, following your comment, Why to use 'align' for the timestamp member?
> the data member already guarantee 2 * 4 = 8 byte, then timestamp will
> be 8 byte aligned, right?
> 
> what you mentioned is to put __aligned(8) only for timestamp.
> 
> I try to put aligned in two ways ( one is only for timestamp, another
> is the whole struct). the result is the same.
> From my thinking, in this case, the struct is already 8 byte aligned
> for timestamp member. don't you think to put 'aligned' is redundant?

On the 8 byte alignment question...  Look up alignment of s64 on x86_32...
It's 4 byte aligned. We had a lot of 'fun' fixing this a few years ago.

So the marking of __aligned(8) for the timestamp does 2 things (and it
takes a fairly close reading of the c spec to check this).

1) Forces alignment of the timestamp. Needed so we can cheaply write
the timestamp
2) Forces alignment of the containing structure.

The combination of these 2 enforces the padding being
consistent across architectures whether or not they align s64 to
4 or 8 bytes.  This last part is the subtle element that
explains why on some architectures you need the __aligned(8) on the
timestamp not the outer structure.

Jonathan




> > --
> > With Best Regards,
> > Andy Shevchenko
Jonathan Cameron July 7, 2022, 5:05 p.m. UTC | #10
On Tue, 5 Jul 2022 13:01:43 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Jul 5, 2022 at 12:33 PM ChiYuan Huang <u0084500@gmail.com> wrote:
> > ChiYuan Huang <u0084500@gmail.com> 於 2022年7月5日 週二 下午5:30寫道:  
> 
> ...
> 
> > Sorry to bother you by the personal mail.  
> 
> No problem asking this in ML, it's normal development process.
> Moreover it's good that others may browse mail archives and find the
> answers. Private mails won't help in that sense. Hence Cc'ing mailing
> list again.
> 
> > I try to put the 'align' in timestamp member.
> > checkpatch.pl show me the below warning.
> >
> > WARNING: Missing a blank line after declarations
> > #448: FILE: drivers/iio/adc/rtq6056.c:448:
> > + u16 vals[RTQ6056_MAX_CHANNEL];
> > + int64_t timestamp __aligned(8);
> >
> > WARNING: externs should be avoided in .c files
> > #448: FILE: drivers/iio/adc/rtq6056.c:448:
> > + int64_t timestamp __aligned(8);
> >
> > total: 0 errors, 2 warnings, 651 lines checked  
> 
> You may run `git grep -n 'align(8)'
> 
> > Does it mean that I only can put the align for the whole struct?  
> 
> It means that checkpatch is full of false positives.

I fixed some of those in the past by fixing checkpatch, but I guess
not all of them...

> 
> Btw, use s64 as a type for timestamp as it's more common.
> 
> 1. Jonathan, shan't we unify this across the IIO drivers? It seems
> they are using (besides s64): int64_t, u64.

Should all be s64. Tidying that up would be good (be careful, there
are a few hardware timestamps where the format is defined by the
hardware though!)

> 
> 2. Also:
> 
> drivers/iio/light/vcnl4000.c:911:       u16 buffer[8] __aligned(8)
For these, what are you proposing changing?  Moving the __aligned earlier?

The oddity of all this lot is that the timestamp isn't at a fixed
location. It depends on what channels are enabled.  Hence we can't
enforce things via a more readable structure.

> 
> drivers/iio/light/vcnl4035.c:106:       u8 buffer[ALIGN(sizeof(u16),
> sizeof(s64)) + sizeof(s64)]  __aligned(8);
> 
> drivers/iio/light/si1145.c:190: u8 buffer[24] __aligned(8);
> 
> // misplaced aligned()
> drivers/iio/adc/mt6360-adc.c-265-               u16 values[MT6360_CHAN_MAX];
> drivers/iio/adc/mt6360-adc.c-266-               int64_t timestamp;
> drivers/iio/adc/mt6360-adc.c:267:       } data __aligned(8);

This one is indeed a bug.

> 
> drivers/iio/adc/stm32-adc.c:250:        u16
> buffer[STM32_ADC_MAX_SQ + 4] __aligned(8);
> 
> drivers/iio/adc/mxs-lradc-adc.c:119:    u32
> buffer[10] __aligned(8);
> 
> drivers/iio/adc/ti-adc0832.c:37:        u8 data[24] __aligned(8);
> 
> drivers/iio/adc/ti-ads124s08.c:108:     u32
> buffer[ADS124S08_MAX_CHANNELS + sizeof(s64)/sizeof(u32)] __aligned(8);
> 
> drivers/iio/amplifiers/ada4250.c:304:   u8 data[2] __aligned(8) = {};
> 
> drivers/iio/chemical/atlas-sensor.c:95: __be32 buffer[6] __aligned(8);
> 
> drivers/iio/health/afe4403.c:79:        s32 buffer[8] __aligned(8);
> 
> drivers/iio/health/afe4404.c:95:        s32 buffer[10] __aligned(8);
> 
> drivers/iio/imu/adis16475.c:107:        __be16
> data[ADIS16475_MAX_SCAN_DATA] __aligned(8);
> 
> drivers/iio/imu/adis16480.c:174:        __be16
> data[ADIS16495_BURST_MAX_DATA] __aligned(8);
> 
> drivers/iio/imu/bmi160/bmi160.h:19:     __le16 buf[12] __aligned(8);
> 
> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c:568:     u8
> iio_buff[ST_LSM6DSX_IIO_BUFF_SIZE] __aligned(8);
> 
> // ??? haven't checked context
> drivers/iio/light/acpi-als.c:65:        s32
> evt_buffer[ACPI_ALS_EVT_BUFFER_SIZE / sizeof(s32)]  __aligned(8);
> 
> drivers/iio/light/si1145.c:190: u8 buffer[24] __aligned(8);
> 
> drivers/iio/magnetometer/rm3100-core.c:82:      u8
> buffer[RM3100_SCAN_BYTES] __aligned(8);
> 
> drivers/iio/potentiostat/lmp91000.c:75: u32 buffer[4] __aligned(8);
> 
> drivers/iio/pressure/mpl3115.c:160:     u8 buffer[16] __aligned(8);
> 
> drivers/iio/proximity/isl29501.c:941:   u32 buffer[4] __aligned(8) =
> {}; /* 1x16-bit + naturally aligned ts */
> 
> 
> need conversion.
> 
> 3. Why do we need explicit garbage in the code? :-)

If I recall correctly, because the hardware explicitly writes garbage into
that location during the read (it probably claims it's status flags
of some such, but as far as we are concerned it's garbage).

Note there is a nice comment just above this structure explaining why :)

Jonathan

> 
> drivers/iio/light/rpr0521.c-204-                __le16 channels[3];
> drivers/iio/light/rpr0521.c-205-                u8 garbage;
> drivers/iio/light/rpr0521.c:206:                s64 ts __aligned(8);
>
diff mbox series

Patch

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 b/Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056
new file mode 100644
index 00000000..db54343
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056
@@ -0,0 +1,6 @@ 
+What:		/sys/bus/iio/devices/iio:deviceX/in_voltage0_integration_time
+What:		/sys/bus/iio/devices/iio:deviceX/in_voltage1_integration_time
+KernelVersion:	5.15.31
+Contact:	cy_huang@richtek.com
+Description:
+		Each voltage conversion time in uS
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 48ace74..caebd1a 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -908,6 +908,21 @@  config ROCKCHIP_SARADC
 	  To compile this driver as a module, choose M here: the
 	  module will be called rockchip_saradc.
 
+config RICHTEK_RTQ6056
+	tristate "Richtek RTQ6056 Current and Power Monitor ADC"
+	depends on I2C
+	select REGMAP_I2C
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	help
+	  Say yes here to enable RQT6056 ADC support.
+	  RTQ6056 is a high accuracy current-sense monitor with I2C and SMBus
+	  compatible interface, and the device provides full information for
+	  system by reading out the load current and power.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called rtq6056.
+
 config RZG2L_ADC
 	tristate "Renesas RZ/G2L ADC driver"
 	depends on ARCH_RZG2L || COMPILE_TEST
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 39d806f..cda7580 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -84,6 +84,7 @@  obj-$(CONFIG_QCOM_PM8XXX_XOADC) += qcom-pm8xxx-xoadc.o
 obj-$(CONFIG_RCAR_GYRO_ADC) += rcar-gyroadc.o
 obj-$(CONFIG_RN5T618_ADC) += rn5t618-adc.o
 obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
+obj-$(CONFIG_RICHTEK_RTQ6056) += rtq6056.o
 obj-$(CONFIG_RZG2L_ADC) += rzg2l_adc.o
 obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o
 obj-$(CONFIG_SPEAR_ADC) += spear_adc.o
diff --git a/drivers/iio/adc/rtq6056.c b/drivers/iio/adc/rtq6056.c
new file mode 100644
index 00000000..80b9c5f
--- /dev/null
+++ b/drivers/iio/adc/rtq6056.c
@@ -0,0 +1,670 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2022 Richtek Technology Corp.
+ *
+ * ChiYuan Huang <cy_huang@richtek.com>
+ */
+
+#include <linux/bitops.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/sysfs.h>
+#include <linux/types.h>
+#include <linux/util_macros.h>
+
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#define RTQ6056_REG_CONFIG	0x00
+#define RTQ6056_REG_SHUNTVOLT	0x01
+#define RTQ6056_REG_BUSVOLT	0x02
+#define RTQ6056_REG_POWER	0x03
+#define RTQ6056_REG_CURRENT	0x04
+#define RTQ6056_REG_CALIBRATION	0x05
+#define RTQ6056_REG_MASKENABLE	0x06
+#define RTQ6056_REG_ALERTLIMIT	0x07
+#define RTQ6056_REG_MANUFACTID	0xFE
+#define RTQ6056_REG_DIEID	0xFF
+
+#define RTQ6056_VENDOR_ID	0x1214
+#define RTQ6056_DEFAULT_CONFIG	0x4127
+#define RTQ6056_CONT_ALLON	7
+
+enum {
+	RTQ6056_CH_VSHUNT = 0,
+	RTQ6056_CH_VBUS,
+	RTQ6056_CH_POWER,
+	RTQ6056_CH_CURRENT,
+	RTQ6056_MAX_CHANNEL
+};
+
+enum {
+	F_OPMODE = 0,
+	F_VSHUNTCT,
+	F_VBUSCT,
+	F_AVG,
+	F_RESET,
+	F_MAX_FIELDS
+};
+
+struct rtq6056_priv {
+	struct device *dev;
+	struct regmap *regmap;
+	struct regmap_field *rm_fields[F_MAX_FIELDS];
+	u32 shunt_resistor_uohm;
+	int vshuntct_us;
+	int vbusct_us;
+	int avg_sample;
+};
+
+static const struct reg_field rtq6056_reg_fields[F_MAX_FIELDS] = {
+	[F_OPMODE] = REG_FIELD(RTQ6056_REG_CONFIG, 0, 2),
+	[F_VSHUNTCT] = REG_FIELD(RTQ6056_REG_CONFIG, 3, 5),
+	[F_VBUSCT] = REG_FIELD(RTQ6056_REG_CONFIG, 6, 8),
+	[F_AVG]	= REG_FIELD(RTQ6056_REG_CONFIG, 9, 11),
+	[F_RESET] = REG_FIELD(RTQ6056_REG_CONFIG, 15, 15),
+};
+
+static const struct iio_chan_spec rtq6056_channels[RTQ6056_MAX_CHANNEL + 1] = {
+	{
+		.type = IIO_VOLTAGE,
+		.indexed = 1,
+		.channel = 0,
+		.address = RTQ6056_REG_SHUNTVOLT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
+				      BIT(IIO_CHAN_INFO_INT_TIME),
+		.info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
+					   BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.scan_index = 0,
+		.scan_type = {
+			.sign = 's',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_CPU,
+		},
+	},
+	{
+		.type = IIO_VOLTAGE,
+		.indexed = 1,
+		.channel = 1,
+		.address = RTQ6056_REG_BUSVOLT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
+				      BIT(IIO_CHAN_INFO_INT_TIME),
+		.info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
+					   BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.scan_index = 1,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_CPU,
+		},
+	},
+	{
+		.type = IIO_POWER,
+		.indexed = 1,
+		.channel = 2,
+		.address = RTQ6056_REG_POWER,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
+					   BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.scan_index = 2,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_CPU,
+		},
+	},
+	{
+		.type = IIO_CURRENT,
+		.indexed = 1,
+		.channel = 3,
+		.address = RTQ6056_REG_CURRENT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
+					   BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.scan_index = 3,
+		.scan_type = {
+			.sign = 's',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_CPU,
+		},
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(RTQ6056_MAX_CHANNEL),
+};
+
+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;
+	}
+
+	/* 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);
+
+	return IIO_VAL_INT;
+}
+
+static int rtq6056_adc_read_scale(struct iio_chan_spec const *ch, int *val,
+				  int *val2)
+{
+	switch (ch->address) {
+	case RTQ6056_REG_SHUNTVOLT:
+		/* VSHUNT lsb  2.5uV */
+		*val = 2500;
+		*val2 = 1000000000;
+		return IIO_VAL_FRACTIONAL;
+	case RTQ6056_REG_BUSVOLT:
+		/* VBUS lsb 1.25mV */
+		*val = 1250;
+		*val2 = 1000;
+		return IIO_VAL_FRACTIONAL;
+	case RTQ6056_REG_POWER:
+		/* Power lsb 25mV */
+		*val = 25;
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+/*
+ * Conversion time in uS for channel VSHUNT and VBUS. The indices correspond
+ * with the bit value expected by the chip. And it can be found at
+ * https://www.richtek.com/assets/product_file/RTQ6056/DSQ6056-00.pdf
+ */
+static const int rtq6056_conv_time_list[] = {
+	139, 203, 269, 525, 1037, 2061, 4109, 8205,
+};
+
+static int rtq6056_adc_set_conv_time(struct rtq6056_priv *priv,
+				     struct iio_chan_spec const *ch,
+				     int val)
+{
+	struct regmap_field *rm_field;
+	unsigned int selector;
+	int *ct, ret;
+
+	if (val > 8205 || val < 139)
+		return -EINVAL;
+
+	if (ch->address == RTQ6056_REG_SHUNTVOLT) {
+		rm_field = priv->rm_fields[F_VSHUNTCT];
+		ct = &priv->vshuntct_us;
+	} else {
+		rm_field = priv->rm_fields[F_VBUSCT];
+		ct = &priv->vbusct_us;
+	}
+
+	selector = find_closest(val, rtq6056_conv_time_list,
+				ARRAY_SIZE(rtq6056_conv_time_list));
+
+	ret = regmap_field_write(rm_field, selector);
+	if (ret)
+		return ret;
+
+	*ct = rtq6056_conv_time_list[selector];
+
+	return 0;
+}
+
+/*
+ * Available averaging rate for rtq6056. The indices correspond with the bit
+ * value expected by the chip. And it can be found at
+ * https://www.richtek.com/assets/product_file/RTQ6056/DSQ6056-00.pdf
+ */
+static const int rtq6056_avg_sample_list[] = {
+	1, 4, 16, 64, 128, 256, 512, 1024,
+};
+
+static int rtq6056_adc_set_average(struct rtq6056_priv *priv, int val)
+{
+	unsigned int selector;
+	int ret;
+
+	if (val > 1024 || val < 1)
+		return -EINVAL;
+
+	selector = find_closest(val, rtq6056_avg_sample_list,
+				ARRAY_SIZE(rtq6056_avg_sample_list));
+
+	ret = regmap_field_write(priv->rm_fields[F_AVG], selector);
+	if (ret)
+		return ret;
+
+	priv->avg_sample = rtq6056_avg_sample_list[selector];
+
+	return 0;
+}
+
+static int rtq6056_adc_get_sample_freq(struct rtq6056_priv *priv, int *val)
+{
+	int sample_time;
+
+	sample_time = priv->vshuntct_us + priv->vbusct_us;
+	sample_time *= priv->avg_sample;
+
+	*val = DIV_ROUND_UP(1000000, sample_time);
+
+	return IIO_VAL_INT;
+}
+
+static int rtq6056_adc_read_raw(struct iio_dev *indio_dev,
+				struct iio_chan_spec const *chan, int *val,
+				int *val2, long mask)
+{
+	struct rtq6056_priv *priv = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		return rtq6056_adc_read_channel(priv, chan, val);
+	case IIO_CHAN_INFO_SCALE:
+		return rtq6056_adc_read_scale(chan, val, val2);
+	case IIO_CHAN_INFO_INT_TIME:
+		if (chan->address == RTQ6056_REG_SHUNTVOLT)
+			*val = priv->vshuntct_us;
+		else
+			*val = priv->vbusct_us;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		*val = priv->avg_sample;
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		return rtq6056_adc_get_sample_freq(priv, val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int rtq6056_adc_read_avail(struct iio_dev *indio_dev,
+				  struct iio_chan_spec const *chan,
+				  const int **vals, int *type, int *length,
+				  long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		*vals = rtq6056_conv_time_list;
+		*type = IIO_VAL_INT;
+		*length = ARRAY_SIZE(rtq6056_conv_time_list);
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		*vals = rtq6056_avg_sample_list;
+		*type = IIO_VAL_INT;
+		*length = ARRAY_SIZE(rtq6056_avg_sample_list);
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int rtq6056_adc_write_raw(struct iio_dev *indio_dev,
+				 struct iio_chan_spec const *chan, int val,
+				 int val2, long mask)
+{
+	struct rtq6056_priv *priv = iio_priv(indio_dev);
+
+	if (iio_buffer_enabled(indio_dev))
+		return -EBUSY;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		return rtq6056_adc_set_conv_time(priv, chan, val);
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		return rtq6056_adc_set_average(priv, val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static const char *rtq6056_channel_labels[RTQ6056_MAX_CHANNEL] = {
+	[RTQ6056_CH_VSHUNT] = "Vshunt",
+	[RTQ6056_CH_VBUS] = "Vbus",
+	[RTQ6056_CH_POWER] = "Power",
+	[RTQ6056_CH_CURRENT] = "Current",
+};
+
+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]);
+}
+
+static int rtq6056_set_shunt_resistor(struct rtq6056_priv *priv,
+				      int resistor_uohm)
+{
+	unsigned int calib_val;
+	int ret;
+
+	if (resistor_uohm <= 0) {
+		dev_err(priv->dev, "Invalid resistor [%d]\n", resistor_uohm);
+		return -EINVAL;
+	}
+
+	/* calibration = 5120000 / (Rshunt (uohm) * current lsb (1mA)) */
+	calib_val = 5120000 / resistor_uohm;
+	ret = regmap_write(priv->regmap, RTQ6056_REG_CALIBRATION, calib_val);
+	if (ret)
+		return ret;
+
+	priv->shunt_resistor_uohm = resistor_uohm;
+
+	return 0;
+}
+
+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 };
+
+	return iio_format_value(buf, IIO_VAL_FRACTIONAL, 1, vals);
+}
+
+static ssize_t shunt_resistor_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct rtq6056_priv *priv = iio_priv(indio_dev);
+	int val, val_fract, ret;
+
+	if (iio_buffer_enabled(indio_dev))
+		return -EBUSY;
+
+	ret = iio_str_to_fixpoint(buf, 100000, &val, &val_fract);
+	if (ret)
+		return ret;
+
+	ret = rtq6056_set_shunt_resistor(priv, val * 1000000 + val_fract);
+	if (ret)
+		return ret;
+
+	return len;
+}
+
+static IIO_DEVICE_ATTR_RW(shunt_resistor, 0);
+
+static struct attribute *rtq6056_attributes[] = {
+	&iio_dev_attr_shunt_resistor.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group rtq6056_attribute_group = {
+	.attrs = rtq6056_attributes,
+};
+
+static const struct iio_info rtq6056_info = {
+	.attrs = &rtq6056_attribute_group,
+	.read_raw = rtq6056_adc_read_raw,
+	.read_avail = rtq6056_adc_read_avail,
+	.write_raw = rtq6056_adc_write_raw,
+	.read_label = rtq6056_adc_read_label,
+};
+
+static irqreturn_t rtq6056_buffer_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct rtq6056_priv *priv = iio_priv(indio_dev);
+	struct device *dev = priv->dev;
+	struct {
+		u16 vals[RTQ6056_MAX_CHANNEL];
+		int64_t timestamp;
+	} data __aligned(8);
+	unsigned int raw;
+	int i = 0, bit, ret;
+
+	memset(&data, 0, sizeof(data));
+
+	pm_runtime_get_sync(dev);
+
+	for_each_set_bit(bit, indio_dev->active_scan_mask, indio_dev->masklength) {
+		unsigned int addr = rtq6056_channels[bit].address;
+
+		ret = regmap_read(priv->regmap, addr, &raw);
+		if (ret)
+			goto out;
+
+		data.vals[i++] = raw;
+	}
+
+	iio_push_to_buffers_with_timestamp(indio_dev, &data, iio_get_time_ns(indio_dev));
+
+out:
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put(dev);
+
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static bool rtq6056_is_readable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case RTQ6056_REG_CONFIG ... RTQ6056_REG_ALERTLIMIT:
+	case RTQ6056_REG_MANUFACTID ... RTQ6056_REG_DIEID:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool rtq6056_is_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case RTQ6056_REG_CONFIG:
+	case RTQ6056_REG_CALIBRATION ... RTQ6056_REG_ALERTLIMIT:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct regmap_config rtq6056_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+	.val_format_endian = REGMAP_ENDIAN_BIG,
+	.max_register = RTQ6056_REG_DIEID,
+	.readable_reg = rtq6056_is_readable_reg,
+	.writeable_reg = rtq6056_is_writeable_reg,
+};
+
+static int rtq6056_probe(struct i2c_client *i2c)
+{
+	struct iio_dev *indio_dev;
+	struct rtq6056_priv *priv;
+	struct device *dev = &i2c->dev;
+	struct regmap *regmap;
+	unsigned int vendor_id, shunt_resistor_uohm;
+	int ret;
+
+	if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_SMBUS_WORD_DATA))
+		return -EOPNOTSUPP;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*priv));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	priv = iio_priv(indio_dev);
+	priv->dev = dev;
+	priv->vshuntct_us = priv->vbusct_us = 1037;
+	priv->avg_sample = 1;
+	i2c_set_clientdata(i2c, priv);
+
+	regmap = devm_regmap_init_i2c(i2c, &rtq6056_regmap_config);
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap),
+				     "Failed to init regmap\n");
+
+	priv->regmap = regmap;
+
+	ret = regmap_read(regmap, RTQ6056_REG_MANUFACTID, &vendor_id);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to get manufacturer info\n");
+
+	if (vendor_id != RTQ6056_VENDOR_ID)
+		return dev_err_probe(dev, -ENODEV,
+				     "Invalid vendor id 0x%04x\n", vendor_id);
+
+	ret = devm_regmap_field_bulk_alloc(dev, regmap, priv->rm_fields,
+					   rtq6056_reg_fields, F_MAX_FIELDS);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to init regmap field\n");
+
+	/*
+	 * By default, configure average sample as 1, bus and shunt conversion
+	 * timea as 1037 microsecond, and operating mode to all on.
+	 */
+	ret = regmap_write(regmap, RTQ6056_REG_CONFIG, RTQ6056_DEFAULT_CONFIG);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to enable continuous sensing\n");
+
+	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;
+	}
+
+	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");
+		goto err_probe;
+	}
+
+	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;
+
+	pm_runtime_dont_use_autosuspend(dev);
+	pm_runtime_disable(dev);
+	pm_runtime_set_suspended(dev);
+
+	return 0;
+}
+
+static int rtq6056_runtime_suspend(struct device *dev)
+{
+	struct rtq6056_priv *priv = dev_get_drvdata(dev);
+
+	/* Configure to shutdown mode */
+	return regmap_field_write(priv->rm_fields[F_OPMODE], 0);
+}
+
+static int rtq6056_runtime_resume(struct device *dev)
+{
+	struct rtq6056_priv *priv = dev_get_drvdata(dev);
+	int sample_rdy_time_us, ret;
+
+	ret = regmap_field_write(priv->rm_fields[F_OPMODE], RTQ6056_CONT_ALLON);
+	if (ret)
+		return ret;
+
+	sample_rdy_time_us = priv->vbusct_us + priv->vshuntct_us;
+	sample_rdy_time_us *= priv->avg_sample;
+
+	usleep_range(sample_rdy_time_us, sample_rdy_time_us + 100);
+
+	return 0;
+}
+
+static const struct dev_pm_ops rtq6056_pm_ops = {
+	SET_RUNTIME_PM_OPS(rtq6056_runtime_suspend, rtq6056_runtime_resume, NULL)
+};
+
+static const struct of_device_id rtq6056_device_match[] = {
+	{ .compatible = "richtek,rtq6056", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, rtq6056_device_match);
+
+static struct i2c_driver rtq6056_driver = {
+	.driver = {
+		.name = "rtq6056",
+		.of_match_table = rtq6056_device_match,
+		.pm = &rtq6056_pm_ops,
+	},
+	.probe_new = rtq6056_probe,
+	.remove = rtq6056_remove,
+};
+module_i2c_driver(rtq6056_driver);
+
+MODULE_AUTHOR("ChiYuan Huang <cy_huang@richtek.com>");
+MODULE_DESCRIPTION("Richtek RTQ6056 Driver");
+MODULE_LICENSE("GPL v2");