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

[Olivier MOYSAN <olivier.moysan@foss.st.com>]

Hi Jonathan,

On 6/23/24 17:11, Jonathan Cameron wrote:
> On Tue, 18 Jun 2024 18:08:33 +0200
> Olivier Moysan <olivier.moysan@foss.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@foss.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.
> 

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