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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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