Hi Jonathan,
On 6/23/24 17:11, Jonathan Cameron wrote:
On Tue, 18 Jun 2024 18:08:33 +0200
Olivier Moysan <olivier.moysan@xxxxxxxxxxx> wrote:
Add a generic driver to support sigma delta modulators.
Typically, this device is a hardware connected to an IIO device
in charge of the conversion. The device is exposed as an IIO backend
device. This backend device and the associated conversion device
can be seen as an aggregate device from IIO framework.
Signed-off-by: Olivier Moysan <olivier.moysan@xxxxxxxxxxx>
Trivial comments inline.
diff --git a/drivers/iio/adc/sd_adc_backend.c b/drivers/iio/adc/sd_adc_backend.c
new file mode 100644
index 000000000000..556a49dc537b
--- /dev/null
+++ b/drivers/iio/adc/sd_adc_backend.c
@@ -0,0 +1,110 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Generic sigma delta modulator IIO backend
+ *
+ * Copyright (C) 2024, STMicroelectronics - All Rights Reserved
+ */
+
+#include <linux/iio/backend.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+
+struct iio_sd_backend_priv {
+ struct regulator *vref;
+ int vref_mv;
+};
+
+static int sd_backend_enable(struct iio_backend *backend)
+{
+ struct iio_sd_backend_priv *priv = iio_backend_get_priv(backend);
+
+ return regulator_enable(priv->vref);
+};
+
+static void sd_backend_disable(struct iio_backend *backend)
+{
+ struct iio_sd_backend_priv *priv = iio_backend_get_priv(backend);
+
+ regulator_disable(priv->vref);
+};
+
+static int sd_backend_read(struct iio_backend *backend, int *val, int *val2, long mask)
Nothing to do with this patch as such:
One day I'm going to bit the bullet and fix that naming.
Long long ago when the Earth was young it actually was a bitmap which
I miscalled a mask - it only had one bit ever set, which was a dumb
bit of API. It's not been true for a long time.
Anyhow, one more instances isn't too much of a problem I guess.
I changed the callback .read_raw to .ext_info_get to take Nuno's comment
about iio_backend_read_raw() API, into account.
So, I changed this function to
static int sd_backend_ext_info_get(struct iio_backend *back, uintptr_t
private, const struct iio_chan_spec *chan, char *buf)
for v2 version.
+{
+ struct iio_sd_backend_priv *priv = iio_backend_get_priv(backend);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SCALE:
+ *val = priv->vref_mv;
This doesn't really feel right as what are we calling to? Using it to pass the
reference voltage doesn't make sense under normal handling of these. So at very
least needs a comment.
+ *val2 = 0;
No need to set val2.
Removed in v2 also.
+ return IIO_VAL_INT;
+
+ case IIO_CHAN_INFO_OFFSET:
+ *val = 0;
+ *val2 = 0;
+ return IIO_VAL_INT;
Normally we just don't provide this but I guess you are requiring all of these?
Long term that won't scale, so you need your caller to handle a suitable
error return, -EINVAL will work to say not supported.
Offset support is not strictly required for time being, as reported as
zero. The aim was to anticipate further needs, but it may be dropped for
now. I will remove it from v2, If you suggest keeping only the bare
essentials here.
Error changed to "-EOPNOTSUPP" for unknown requests.
+ }
+
+ return -EINVAL;
+};
+
+static const struct iio_backend_ops sd_backend_ops = {
+ .enable = sd_backend_enable,
+ .disable = sd_backend_disable,
+ .read_raw = sd_backend_read,
+};
+
+static int iio_sd_backend_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct regulator *vref;
+ struct iio_sd_backend_priv *priv;
+ int ret;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ vref = devm_regulator_get_optional(dev, "vref");
New devm_regulator_get_enable_read_voltage() slightly simplifies this
and means you don't need to keep vref around.
+ if (IS_ERR(vref)) {
+ if (PTR_ERR(vref) != -ENODEV)
+ return dev_err_probe(dev, PTR_ERR(vref), "Failed to get vref\n");
+ } else {
+ ret = regulator_get_voltage(vref);
You haven't turned it on so it's not guaranteed to give you a useful
answer.
My understanding is that regulator_get_voltage() always returns the
regulator voltage, whatever the regulator state, as documented in the
API description:
"* NOTE: If the regulator is disabled it will return the voltage value.
* This function should not be used to determine regulator state."
So, my logic was to enable the regulator only when requested, through
enable/disable callbacks to manage power.
Please, let me know if I missed something here.
Use the enable_read_voltage variant and that will handle this for you.
+ if (ret < 0)
+ return ret;
+
+ priv->vref = vref;
+ priv->vref_mv = ret / 1000;
+ }
+
+ ret = devm_iio_backend_register(&pdev->dev, &sd_backend_ops, priv);
+ if (ret)
+ return ret;
+
+ return 0;
return devm_iio_....
Done
+};
+
+static const struct of_device_id sd_backend_of_match[] = {
+ { .compatible = "sd-backend" },
+ { .compatible = "ads1201" },
Conor pointed out ti,ads1201
At least I assume ti?
Ack. changed to ti,ads1201
+ { }
+};
Regards
Olivier
