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]
Message-ID: <06edd10d-eab4-41db-83d4-232fc43e1759@foss.st.com>
Date: Mon, 24 Jun 2024 18:26:10 +0200
From: Olivier MOYSAN <olivier.moysan@...s.st.com>
To: Nuno Sá <noname.nuno@...il.com>,
        Jonathan Cameron
	<jic23@...nel.org>
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

Hi Nuno,

On 6/24/24 17:22, Nuno Sá wrote:
> Hi Olivier,
> 
> On Mon, 2024-06-24 at 14:43 +0200, Olivier MOYSAN wrote:
>> Hi Jonathan,
>>
>> On 6/23/24 17:11, Jonathan Cameron wrote:
>>> 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.
>>>
>>
>> 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.
>>
> 
> Maybe I'm missing something but I think I did not explained myself very well. What I
> had in mind was that since you're calling .read_raw() from IIO_CHAN_INFO_SCALE and
> IIO_CHAN_INFO_OFFSET, it could make sense to have more dedicated API's. Meaning:
> 
> iio_backend_read_scale(...)
> iio_backend_read_offset(...)
> 
> The iio_backend_read_raw() may make sense when frontends call
> iio_backend_extend_chan_spec() and have no idea what the backend may have added to
> the channel. So, in those cases something like this could make sense:
> 
> switch (mask)
> IIO_CHAN_INFO_RAW:
> 
> ...
> 
> default:
> 	return iio_backend_read_raw();
> 
> but like I said maybe this is me over-complicating and a simple
> iio_backend_read_raw() is sufficient. But I think I never mentioned something like
> .read_raw -> .ext_info_get.
> 

Thanks for clarification. Your previous message was actually clear 
enough regarding iio_backend_read_raw() API.

However, your comment about extend_chan_spec(), let me think that I 
could maybe spare a new API, and just re-use iio_backend_ext_info_get() 
callback.
Nevertheless, this API cannot be used directly, as it can be used only 
for a frontend associated to a single backend. There is a comment in 
iio_backend_ext_info_get() about the need of another API for such case.

So I considered introducing this new API (instead of read_raw):
ssize_t iio_backend_ext_info_get_from_backend(struct iio_backend *back, 
uintptr_t private, const struct iio_chan_spec *chan, char *buf)
(I'm not sure this name is the most relevant).

But if you don't like this alternative too much, I will keep the initial 
"catch all" iio_backend_read_raw() API.

BRs
Olivier

> The other thing I mentioned was to instead of having:
> 
> 
> if (child) {
> 	ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> 				 BIT(IIO_CHAN_INFO_SCALE) |
> 				 BIT(IIO_CHAN_INFO_OFFSET);
> }
> 
> You could use iio_backend_extend_chan_spec() and have the backend set the SCALE and
> OFFSET bits for you as it seems these functionality depends on the backend.
> 
> But none of the above were critical or things that I feel to strong about.
> 
> Anyways, just wanted to give some clarification as it seems there were some
> misunderstandings (I think).
> > - Nuno Sá
> 
>>>