linux-kernel - Re: [PATCH 3/3] iio: wrapper: unit-converter: new driver

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:   Tue, 27 Mar 2018 09:42:40 +0200
From:   Peter Rosin <peda@...ntia.se>
To:     Jonathan Cameron <jic23@...nel.org>
Cc:     linux-kernel@...r.kernel.org, Hartmut Knaack <knaack.h@....de>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        "David S. Miller" <davem@...emloft.net>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Randy Dunlap <rdunlap@...radead.org>,
        linux-iio@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH 3/3] iio: wrapper: unit-converter: new driver

On 2018-03-24 15:03, Jonathan Cameron wrote:
> On Mon, 19 Mar 2018 18:02:46 +0100
> Peter Rosin <peda@...ntia.se> wrote:
> 
>> If an ADC channel measures the midpoint of a voltage divider, the
>> interesting voltage is often the voltage over the full resistance.
>> E.g. if the full voltage it too big for the ADC to handle.
>> Likewise, if an ADC channel measures the voltage across a resistor,
>> the interesting value is often the current through the resistor.
>>
>> This driver solves both problems by allowing to linearly scale a channel
>> and by allowing changes to the type of the channel. Or both.
>>
>> Signed-off-by: Peter Rosin <peda@...ntia.se>
> A few comments inline, but basically the code looks fine, just questions
> around naming and bindings to answer.
> 
> Thanks,
> 
> Jonathan
> 

*snip*

>> +static int unit_converter_write_raw(struct iio_dev *indio_dev,
>> +				    struct iio_chan_spec const *chan,
>> +				    int val, int val2, long mask)
>> +{
>> +	struct unit_converter *uc = iio_priv(indio_dev);
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		return iio_write_channel_raw(uc->parent, val);
> Talk me through the logic of having this...  Supporting a DAC?
> Bindings don't talk about that possibility...

There's no logic at all, and I don't need it. It's just leftovers,
should I remove it?

>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +

*snip*

>> +static int unit_converter_configure_channel(struct device *dev,
>> +					    struct unit_converter *uc,
>> +					    enum iio_chan_type type)
>> +{
>> +	struct iio_chan_spec *chan = &uc->chan;
>> +	struct iio_chan_spec const *pchan = uc->parent->channel;
>> +	int ret;
>> +
>> +	chan->indexed = 1;
>> +	chan->output = pchan->output;
>> +	chan->ext_info = uc->ext_info;
>> +
>> +	if (type == -1) {
>> +		ret = iio_get_channel_type(uc->parent, &type);
>> +		if (ret < 0) {
>> +			dev_err(dev, "failed to get parent channel type\n");
>> +			return ret;
>> +		}
>> +	}
>> +	chan->type = type;
>> +
>> +	if (iio_channel_has_info(pchan, IIO_CHAN_INFO_RAW))
>> +		chan->info_mask_separate |= BIT(IIO_CHAN_INFO_RAW);
> if the parent doesn't support RAW, is there a lot of point in carrying on?

Nope, better to error out I suppose. But I'm not familiar with channels
without RAW, what alternatives are there anyway?

>> +	if (iio_channel_has_info(pchan, IIO_CHAN_INFO_SCALE))
>> +		chan->info_mask_separate |= BIT(IIO_CHAN_INFO_SCALE);
>> +
>> +	if (iio_channel_has_available(pchan, IIO_CHAN_INFO_RAW))
>> +		chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_RAW);
>> +
>> +	chan->channel = 0;
>> +
>> +	return 0;
>> +}