From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Message-ID: <501F8509.1030903@metafoo.de> Date: Mon, 06 Aug 2012 10:49:13 +0200 From: Lars-Peter Clausen MIME-Version: 1.0 To: Marek Vasut CC: linux-iio@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Jonathan Cameron , Juergen Beisert , Shawn Guo , Wolfgang Denk Subject: Re: [PATCH 2/3 V2] IIO: Add basic MXS LRADC driver References: <1344007721-11595-1-git-send-email-marex@denx.de> <1344007721-11595-3-git-send-email-marex@denx.de> In-Reply-To: <1344007721-11595-3-git-send-email-marex@denx.de> Content-Type: text/plain; charset=ISO-8859-1 List-ID: 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. > > Signed-off-by: Marek Vasut > Cc: Jonathan Cameron > Cc: Juergen Beisert > Cc: Lars-Peter Clausen > Cc: Shawn Guo > Cc: Wolfgang Denk > --- > 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. > [...] > +} [...] > + > +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. > + if (ret) > + goto err_addr; > + } > + > + dev_set_drvdata(&pdev->dev, iio); platform_set_drvdata > + > + init_completion(&lradc->completion); > + mutex_init(&lradc->lock); > + [...] From mboxrd@z Thu Jan 1 00:00:00 1970 From: lars@metafoo.de (Lars-Peter Clausen) Date: Mon, 06 Aug 2012 10:49:13 +0200 Subject: [PATCH 2/3 V2] IIO: Add basic MXS LRADC driver In-Reply-To: <1344007721-11595-3-git-send-email-marex@denx.de> References: <1344007721-11595-1-git-send-email-marex@denx.de> <1344007721-11595-3-git-send-email-marex@denx.de> Message-ID: <501F8509.1030903@metafoo.de> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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. > > Signed-off-by: Marek Vasut > Cc: Jonathan Cameron > Cc: Juergen Beisert > Cc: Lars-Peter Clausen > Cc: Shawn Guo > Cc: Wolfgang Denk > --- > 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. > [...] > +} [...] > + > +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. > + if (ret) > + goto err_addr; > + } > + > + dev_set_drvdata(&pdev->dev, iio); platform_set_drvdata > + > + init_completion(&lradc->completion); > + mutex_init(&lradc->lock); > + [...]