linux-kernel - Re: [PATCH 7/8] iio: add sd modulator generic iio backend

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: Sun, 23 Jun 2024 16:11:50 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Olivier Moysan <olivier.moysan@...s.st.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Liam Girdwood
 <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
 <linux-kernel@...r.kernel.org>, <linux-iio@...r.kernel.org>
Subject: Re: [PATCH 7/8] iio: add sd modulator generic iio backend

On Tue, 18 Jun 2024 18:08:33 +0200
Olivier Moysan <olivier.moysan@...s.st.com> 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@...s.st.com>

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.

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

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

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

> +};
> +
> +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?

> +	{ }
> +};