From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Message-ID: <5027771F.7060500@metafoo.de> Date: Sun, 12 Aug 2012 11:27:59 +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> <501F8509.1030903@metafoo.de> <201208120111.51639.marex@denx.de> In-Reply-To: <201208120111.51639.marex@denx.de> Content-Type: text/plain; charset=ISO-8859-1 List-ID: On 08/12/2012 01:11 AM, Marek Vasut wrote: > 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 >>> 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. > > Even on NOHZ kernel? Yes HZ is always the number of jiffies per second. > >>> [...] >>> +} >> >> [...] >> >>> + >>> +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. Ok. From mboxrd@z Thu Jan 1 00:00:00 1970 From: lars@metafoo.de (Lars-Peter Clausen) Date: Sun, 12 Aug 2012 11:27:59 +0200 Subject: [PATCH 2/3 V2] IIO: Add basic MXS LRADC driver In-Reply-To: <201208120111.51639.marex@denx.de> References: <1344007721-11595-1-git-send-email-marex@denx.de> <1344007721-11595-3-git-send-email-marex@denx.de> <501F8509.1030903@metafoo.de> <201208120111.51639.marex@denx.de> Message-ID: <5027771F.7060500@metafoo.de> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 08/12/2012 01:11 AM, Marek Vasut wrote: > 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 >>> 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. > > Even on NOHZ kernel? Yes HZ is always the number of jiffies per second. > >>> [...] >>> +} >> >> [...] >> >>> + >>> +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. Ok.