Messages in this thread | | | From | Andy Shevchenko <> | Date | Fri, 1 Jul 2022 12:04:42 +0200 | Subject | Re: [PATCH v2 2/2] iio: adc: Add rtq6056 support |
| |
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, ®val); > + if (ret) { > + pm_runtime_put(dev); > + return ret; > + }
You can optimize this to
pm_runtime_get_sync(dev); ret = regmap_read(priv->regmap, addr, ®val); 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
| |