lkml.org 
[lkml]   [2016]   [Apr]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v2] iio: max5487: Add support for Maxim digital potentiometers
    From
    On 10 April 2016 at 14:47, Joachim  Eastwood <manabian@gmail.com> wrote:
    > Hi Cristina,
    >
    > On 9 April 2016 at 10:24, Cristina Moraru <cristina.moraru09@gmail.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@gmail.com>
    >> CC: Daniel Baluta <daniel.baluta@intel.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

    \
     
     \ /
      Last update: 2016-04-10 15:21    [W:3.022 / U:0.180 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site