linux-kernel - Re: [PATCH v2] iio: max5487: Add support for Maxim digital potentiometers

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, 10 Apr 2016 15:10:05 +0200
From:	Joachim Eastwood <manabian@...il.com>
To:	Cristina Moraru <cristina.moraru09@...il.com>
Cc:	Jonathan Cameron <jic23@...nel.org>,
	Hartmut Knaack <knaack.h@....de>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Peter Meerwald-Stadler <pmeerw@...erw.net>, peda@...ntia.se,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	linux-iio@...r.kernel.org, daniel.baluta@...el.com,
	octavian.purdila@...el.com
Subject: Re: [PATCH v2] iio: max5487: Add support for Maxim digital potentiometers

On 10 April 2016 at 14:47, Joachim  Eastwood <manabian@...il.com> wrote:
> Hi Cristina,
>
> On 9 April 2016 at 10:24, Cristina Moraru <cristina.moraru09@...il.com> wrote:
>> Add implementation for Maxim MAX5487, MAX5488, MAX5489
>> digital potentiometers.
>>
>> Datasheet:
>> http://datasheets.maximintegrated.com/en/ds/MAX5487-MAX5489.pdf
>>
>> Signed-off-by: Cristina Moraru <cristina.moraru09@...il.com>
>> CC: Daniel Baluta <daniel.baluta@...el.com>
>> ---
> ...
>> +static int max5487_read_raw(struct iio_dev *indio_dev,
>> +                           struct iio_chan_spec const *chan,
>> +                           int *val, int *val2, long mask)
>> +{
>> +       struct max5487_data *data = iio_priv(indio_dev);
>> +
>> +       if (mask != IIO_CHAN_INFO_SCALE)
>> +               return -EINVAL;
>> +
>> +       *val = 1000 * data->kohms;
>> +       *val2 = MAX5487_MAX_POS;
>
> Newline before return.
>
>> +       return IIO_VAL_FRACTIONAL;
>> +}
>> +
>> +static int max5487_write_raw(struct iio_dev *indio_dev,
>> +                            struct iio_chan_spec const *chan,
>> +                            int val, int val2, long mask)
>> +{
>> +       struct max5487_data *data = iio_priv(indio_dev);
>> +
>> +       switch (mask) {
>> +       case IIO_CHAN_INFO_RAW:
>> +               if (val < 0 || val > MAX5487_MAX_POS)
>> +                       return -EINVAL;
>> +               return regmap_write(data->regmap, chan->address, val);
>> +       default:
>> +               return -EINVAL;
>> +       }
>> +       return -EINVAL;
>
> To be consistent with your max5487_read_raw() function you could do a:
>        if (mask != IIO_CHAN_INFO_RAW)
>                return -EINVAL;
>
>
>> +static const struct iio_info max5487_info = {
>> +       .read_raw = &max5487_read_raw,
>> +       .write_raw = &max5487_write_raw,
>
> Address operator should be unnecessary on functions.
>
>
>> +       data->regmap = devm_regmap_init_spi(spi, &max5487_regmap_config);
>> +       if (IS_ERR(data->regmap))
>> +               return PTR_ERR(data->regmap);
>
> Nothing wrong with using regmap here, but since you are only using
> simple regmap_write()'s you might as well have used spi_write()
> directly. I am not telling you to switch, but I don't see the point of
> using regmap here.

Looking again: it seem that spi.h doesn't have a function that do
write(cmd, data) which regmap does. So I guess that is one reason for
using regmap. But it wouldn't be hard to create a write(cmd,
data)-function for spi either. Just wrap spi_write() and have a local
buf var. I am a bit surprised that spi.h doesn't have such a function
as it should be quite a common pattern for spi chips.

>
> Which reminds me; for regmap you need to select REGMAP_SPI in your
> Kconfig entry.
>
>
> regards,
> Joachim Eastwood