Dear Lars-Peter Clausen,
> On 08/03/2012 05:28 PM, Marek Vasut wrote:
> > This driver is very basic. It supports userland trigger, buffer and
> > raw access to channels. The support for delay channels is missing
> > altogether.
>
> Looks mostly good to me. Some comments inline.
>
> I think you need to provide a documentation for the devicetree binding,
> even though it's a rather simple binding.
Definitelly, will add it.
> > Signed-off-by: Marek Vasut <marex@xxxxxxx>
> > Cc: Jonathan Cameron <jic23@xxxxxxxxxx>
> > Cc: Juergen Beisert <jbe@xxxxxxxxxxxxxx>
> > Cc: Lars-Peter Clausen <lars@xxxxxxxxxx>
> > Cc: Shawn Guo <shawn.guo@xxxxxxxxxx>
> > Cc: Wolfgang Denk <wd@xxxxxxx>
> > ---
> >
> > drivers/staging/iio/adc/Kconfig | 12 +
> > drivers/staging/iio/adc/Makefile | 1 +
> > drivers/staging/iio/adc/mxs-lradc.c | 591
> > +++++++++++++++++++++++++++++++++++ 3 files changed, 604 insertions(+)
> > create mode 100644 drivers/staging/iio/adc/mxs-lradc.c
> >
> > V2: Use delay channel 0 in case of buffered sampling so the samples are
> > deployed
> >
> > continuously.
> > Disallow RAW sampling while buffered mode is enabled to simplify
> > code.
> >
> > diff --git a/drivers/staging/iio/adc/Kconfig
> > b/drivers/staging/iio/adc/Kconfig index 67711b7..97ca697 100644
> > --- a/drivers/staging/iio/adc/Kconfig
> > +++ b/drivers/staging/iio/adc/Kconfig
>
> [...]
>
> > + */
> > +static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
> > + const struct iio_chan_spec *chan,
> > + int *val, int *val2, long m)
> > +{
> > [...]
> > +
> > + init_completion(&lradc->completion);
>
> This should rather be INIT_COMPLETION. init_completion should only called
> once in probe, since it will reinitialize the spinlock which opens up race
> conditions.
>
> > [...]
> > +
> > + /* Wait for completion on the channel, 1 second max. */
> > + ret = wait_for_completion_killable_timeout(&lradc->completion,
> > + msecs_to_jiffies(1000));
>
> msecs_to_jiffies(1000) = HZ, but I guess both is OK.
Even on NOHZ kernel?
> > [...]
> > +}
>
> [...]
>
> > +
> > +static int __devinit mxs_lradc_probe(struct platform_device *pdev)
> > +{
> > [...]
> > +
> > + /* Grab all IRQ sources */
> > + for (i = 0; i < 13; i++) {
> > + lradc->irq[i] = platform_get_irq(pdev, i);
> > + if (lradc->irq[i] < 0) {
> > + ret = -EINVAL;
> > + goto err_addr;
> > + }
> > +
> > + ret = devm_request_irq(dev, lradc->irq[i],
> > + mxs_lradc_handle_irq, 0,
> > + mxs_lradc_irq_name[i], iio);
>
> devm_request_irq is a bit dangerous as long we do not have a
> devm_iio_device_alloc. The IRQ will only be freed after the memory for the
> IIO device has been freed, which means there is a slight window where the
> IRQ could fire although the memory has already been freed.
The IRQ is disabled, see mxs_lradc_hw_stop(), so it should be all right.
> > + if (ret)
> > + goto err_addr;
> > + }
> > +
> > + dev_set_drvdata(&pdev->dev, iio);
>
> platform_set_drvdata
>
> > +
> > + init_completion(&lradc->completion);
> > + mutex_init(&lradc->lock);
> > +
>
> [...]
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
