On Tue, Mar 19, 2019 at 12:37:28AM +0200, Andy Shevchenko wrote:
> 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?
>
Have a look at other ADCs. Most of them support what you're looking
for (specifically have a look at iio_triggered_buffer_setup()).
> > 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?
>
Yes.
> > 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?
>
I guess that would be it unless you plan to add other functionality.
> > > +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.
>
I was thinking about be16_to_cpu(*(__be16 *)buf).
Given this is local array and you do not do any pointer arithmetic
before casting I would be surprised if that caused unaligned access.
>
> 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
>
>
