On Mon, Mar 18, 2019 at 09:52:42PM +0100, Tomasz Duszynski wrote:
> A few comments inline.
Thanks for review, my answers below.
> On Mon, Mar 18, 2019 at 12:51:03PM +0300, Andy Shevchenko wrote:
> > +config INTEL_MRFLD_ADC
> > + tristate "Intel Merrifield Basin Cove ADC driver"
> > + depends on INTEL_SOC_PMIC_MRFLD
> > + select IIO_BUFFER
> > + select IIO_TRIGGERED_BUFFER
>
> Looks you're not using iio buffering hence these should be dropped.
Or implemented? Is there any good example which could be used?
> Instead you should select regmap here.
No need, the depends part does this for us.
> > +#include <linux/bitops.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/driver.h>
> > +#include <linux/iio/machine.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/mfd/intel_soc_pmic.h>
> > +#include <linux/mfd/intel_soc_pmic_mrfld.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
>
> Alphabetical order is preferred.
Ah, you meant iio.h vs. driver.h?
> Generally it's also good idea to include
> all headers which export functionality used by the driver. At least a few
> seems to be missing from the list.
Hmm... Perhaps you meant completion and regmap APIs. Anything else I missed?
> > +static int mrfld_adc_single_conv(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *result)
> > +{
> > + struct mrfld_adc *adc = iio_priv(indio_dev);
> > + struct regmap *regmap = adc->regmap;
> > + unsigned int req;
> > + long timeout;
> > + u8 buf[2];
> > + int ret;
> > + ret = regmap_bulk_read(regmap, chan->address, buf, 2);
> > + if (ret)
> > + goto done;
> > +
> > + *result = (buf[0] << 8) | buf[1];
>
> be/le16_to_cpu() will do it for you.
I think get_unaligned_le16() will be better here. Otherwise we need to define
__le16 variable and cast around it.
And actually it should be __be16.
> > + ret = IIO_VAL_INT;
> > +
> > +done:
> > + regmap_update_bits(regmap, BCOVE_MIRQLVL1, BCOVE_LVL1_ADC, 0xff);
> > + regmap_update_bits(regmap, BCOVE_MADCIRQ, BCOVE_ADCIRQ_ALL, 0xff);
> > +
> > + return ret;
> > +}
> > +
> > +static int mrfld_adc_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2, long mask)
> > +{
> > + int ret;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + ret = iio_device_claim_direct_mode(indio_dev);
> > + if (ret)
> > + return ret;
> > +
> > + ret = mrfld_adc_single_conv(indio_dev, chan, val);
> > +
> > + iio_device_release_direct_mode(indio_dev);
>
> claim/release api is slightly abused here. Legitimate usecase is when
> you want protect against transitions between driver modes and not
> concurrent reads. Mutex will work here just fine.
I see, I will fix this.
>
> > + return ret;
> > +
> > + default:
> > + return -EINVAL;
> > + }
> > +}
--
With Best Regards,
Andy Shevchenko
