Re: [PATCH v1] iio: adc: intel_mrfld_adc: Add Basin Cove ADC driver

[Tomasz Duszynski <tduszyns@gmail.com>]

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
>
>