* [PATCH 1/2] iio: adc: meson-saradc: switch from polling to interrupt mode
@ 2017-02-14 21:58 Heiner Kallweit
2017-02-14 23:16 ` Martin Blumenstingl
0 siblings, 1 reply; 4+ messages in thread
From: Heiner Kallweit @ 2017-02-14 21:58 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Martin Blumenstingl, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio
Switch from polling to interrupt mode.
Successfully tested on a S905GXBB-based Odroid C2.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/iio/adc/meson_saradc.c | 46 +++++++++++++++++++++++++++++++++++++-----
1 file changed, 41 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
index 89def603..dbd56bcc 100644
--- a/drivers/iio/adc/meson_saradc.c
+++ b/drivers/iio/adc/meson_saradc.c
@@ -18,7 +18,9 @@
#include <linux/io.h>
#include <linux/iio/iio.h>
#include <linux/module.h>
+#include <linux/interrupt.h>
#include <linux/of.h>
+#include <linux/of_irq.h>
#include <linux/of_device.h>
#include <linux/platform_device.h>
#include <linux/regmap.h>
@@ -163,6 +165,7 @@
#define MESON_SAR_ADC_REG13_12BIT_CALIBRATION_MASK GENMASK(13, 8)
#define MESON_SAR_ADC_MAX_FIFO_SIZE 32
+#define MESON_SAR_ADC_TIMEOUT 100 /* ms */
#define MESON_SAR_ADC_CHAN(_chan) { \
.type = IIO_VOLTAGE, \
@@ -229,6 +232,7 @@ struct meson_sar_adc_priv {
struct clk_gate clk_gate;
struct clk *adc_div_clk;
struct clk_divider clk_div;
+ struct completion done;
};
static const struct regmap_config meson_sar_adc_regmap_config = {
@@ -274,11 +278,11 @@ static int meson_sar_adc_read_raw_sample(struct iio_dev *indio_dev,
int *val)
{
struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
- int ret, regval, fifo_chan, fifo_val, sum = 0, count = 0;
+ int regval, fifo_chan, fifo_val, sum = 0, count = 0;
- ret = meson_sar_adc_wait_busy_clear(indio_dev);
- if (ret)
- return ret;
+ if(!wait_for_completion_timeout(&priv->done,
+ msecs_to_jiffies(MESON_SAR_ADC_TIMEOUT)))
+ return -ETIMEDOUT;
while (meson_sar_adc_get_fifo_count(indio_dev) > 0 &&
count < MESON_SAR_ADC_MAX_FIFO_SIZE) {
@@ -456,6 +460,7 @@ static int meson_sar_adc_get_sample(struct iio_dev *indio_dev,
enum meson_sar_adc_num_samples avg_samples,
int *val)
{
+ struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
int ret;
ret = meson_sar_adc_lock(indio_dev);
@@ -465,6 +470,11 @@ static int meson_sar_adc_get_sample(struct iio_dev *indio_dev,
/* clear the FIFO to make sure we're not reading old values */
meson_sar_adc_clear_fifo(indio_dev);
+ reinit_completion(&priv->done);
+ regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG0,
+ MESON_SAR_ADC_REG0_FIFO_IRQ_EN,
+ MESON_SAR_ADC_REG0_FIFO_IRQ_EN);
+
meson_sar_adc_set_averaging(indio_dev, chan, avg_mode, avg_samples);
meson_sar_adc_enable_channel(indio_dev, chan);
@@ -473,6 +483,9 @@ static int meson_sar_adc_get_sample(struct iio_dev *indio_dev,
ret = meson_sar_adc_read_raw_sample(indio_dev, chan, val);
meson_sar_adc_stop_sample_engine(indio_dev);
+ regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG0,
+ MESON_SAR_ADC_REG0_FIFO_IRQ_EN, 0);
+
meson_sar_adc_unlock(indio_dev);
if (ret) {
@@ -577,6 +590,9 @@ static int meson_sar_adc_init(struct iio_dev *indio_dev)
*/
meson_sar_adc_set_chan7_mux(indio_dev, CHAN7_MUX_CH7_INPUT);
+ regval = FIELD_PREP(MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, 1);
+ regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG0,
+ MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, regval);
/*
* leave sampling delay and the input clocks as configured by BL30 to
* make sure BL30 gets the values it expects when reading the
@@ -728,6 +744,16 @@ static int meson_sar_adc_hw_disable(struct iio_dev *indio_dev)
return 0;
}
+static irqreturn_t meson_sar_adc_irq(int irq, void *data)
+{
+ struct iio_dev *indio_dev = data;
+ struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
+
+ complete(&priv->done);
+
+ return IRQ_HANDLED;
+}
+
static const struct iio_info meson_sar_adc_iio_info = {
.read_raw = meson_sar_adc_iio_info_read_raw,
.driver_module = THIS_MODULE,
@@ -770,7 +796,7 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
struct resource *res;
void __iomem *base;
const struct of_device_id *match;
- int ret;
+ int irq, ret;
indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*priv));
if (!indio_dev) {
@@ -779,6 +805,7 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
}
priv = iio_priv(indio_dev);
+ init_completion(&priv->done);
match = of_match_device(meson_sar_adc_of_match, &pdev->dev);
priv->data = match->data;
@@ -797,6 +824,15 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
if (IS_ERR(base))
return PTR_ERR(base);
+ irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
+ if (!irq)
+ return -EINVAL;
+
+ ret = devm_request_irq(&pdev->dev, irq, meson_sar_adc_irq, 0,
+ dev_name(&pdev->dev), indio_dev);
+ if (ret)
+ return ret;
+
priv->regmap = devm_regmap_init_mmio(&pdev->dev, base,
&meson_sar_adc_regmap_config);
if (IS_ERR(priv->regmap))
--
2.11.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] iio: adc: meson-saradc: switch from polling to interrupt mode
2017-02-14 21:58 [PATCH 1/2] iio: adc: meson-saradc: switch from polling to interrupt mode Heiner Kallweit
@ 2017-02-14 23:16 ` Martin Blumenstingl
2017-02-15 6:31 ` Heiner Kallweit
2017-02-15 19:20 ` Heiner Kallweit
0 siblings, 2 replies; 4+ messages in thread
From: Martin Blumenstingl @ 2017-02-14 23:16 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio
Hi Heiner,
thanks for this patch!
I have some questions since I don't have access to my boards until
Saturday - I added them inline below.
On Tue, Feb 14, 2017 at 10:58 PM, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> Switch from polling to interrupt mode.
>
> Successfully tested on a S905GXBB-based Odroid C2.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> drivers/iio/adc/meson_saradc.c | 46 +++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 41 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
> index 89def603..dbd56bcc 100644
> --- a/drivers/iio/adc/meson_saradc.c
> +++ b/drivers/iio/adc/meson_saradc.c
> @@ -18,7 +18,9 @@
> #include <linux/io.h>
> #include <linux/iio/iio.h>
> #include <linux/module.h>
> +#include <linux/interrupt.h>
> #include <linux/of.h>
> +#include <linux/of_irq.h>
> #include <linux/of_device.h>
> #include <linux/platform_device.h>
> #include <linux/regmap.h>
> @@ -163,6 +165,7 @@
> #define MESON_SAR_ADC_REG13_12BIT_CALIBRATION_MASK GENMASK(13, 8)
>
> #define MESON_SAR_ADC_MAX_FIFO_SIZE 32
> +#define MESON_SAR_ADC_TIMEOUT 100 /* ms */
>
> #define MESON_SAR_ADC_CHAN(_chan) { \
> .type = IIO_VOLTAGE, \
> @@ -229,6 +232,7 @@ struct meson_sar_adc_priv {
> struct clk_gate clk_gate;
> struct clk *adc_div_clk;
> struct clk_divider clk_div;
> + struct completion done;
> };
>
> static const struct regmap_config meson_sar_adc_regmap_config = {
> @@ -274,11 +278,11 @@ static int meson_sar_adc_read_raw_sample(struct iio_dev *indio_dev,
> int *val)
> {
> struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
> - int ret, regval, fifo_chan, fifo_val, sum = 0, count = 0;
> + int regval, fifo_chan, fifo_val, sum = 0, count = 0;
>
> - ret = meson_sar_adc_wait_busy_clear(indio_dev);
> - if (ret)
> - return ret;
> + if(!wait_for_completion_timeout(&priv->done,
> + msecs_to_jiffies(MESON_SAR_ADC_TIMEOUT)))
> + return -ETIMEDOUT;
>
> while (meson_sar_adc_get_fifo_count(indio_dev) > 0 &&
> count < MESON_SAR_ADC_MAX_FIFO_SIZE) {
> @@ -456,6 +460,7 @@ static int meson_sar_adc_get_sample(struct iio_dev *indio_dev,
> enum meson_sar_adc_num_samples avg_samples,
> int *val)
> {
> + struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
> int ret;
>
> ret = meson_sar_adc_lock(indio_dev);
> @@ -465,6 +470,11 @@ static int meson_sar_adc_get_sample(struct iio_dev *indio_dev,
> /* clear the FIFO to make sure we're not reading old values */
> meson_sar_adc_clear_fifo(indio_dev);
>
> + reinit_completion(&priv->done);
> + regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG0,
> + MESON_SAR_ADC_REG0_FIFO_IRQ_EN,
> + MESON_SAR_ADC_REG0_FIFO_IRQ_EN);
> +
can't we add this to meson_sar_adc_start_sample_engine()?
> meson_sar_adc_set_averaging(indio_dev, chan, avg_mode, avg_samples);
>
> meson_sar_adc_enable_channel(indio_dev, chan);
> @@ -473,6 +483,9 @@ static int meson_sar_adc_get_sample(struct iio_dev *indio_dev,
> ret = meson_sar_adc_read_raw_sample(indio_dev, chan, val);
> meson_sar_adc_stop_sample_engine(indio_dev);
>
> + regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG0,
> + MESON_SAR_ADC_REG0_FIFO_IRQ_EN, 0);
> +
similar to enabling the IRQ above, can't we add this to
meson_sar_adc_stop_sample_engine()?
> meson_sar_adc_unlock(indio_dev);
>
> if (ret) {
> @@ -577,6 +590,9 @@ static int meson_sar_adc_init(struct iio_dev *indio_dev)
> */
> meson_sar_adc_set_chan7_mux(indio_dev, CHAN7_MUX_CH7_INPUT);
>
> + regval = FIELD_PREP(MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, 1);
> + regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG0,
> + MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, regval);
I'm wondering whether we should do this in meson_sar_adc_hw_enable()
in case u-boot is involved in the "return from suspend" process then
doing it in _init() might be a problem since the vendor u-boot
overwrites REG0, see [0]
please note that this is an actual question, not some kind of
suggestion in question form
> /*
> * leave sampling delay and the input clocks as configured by BL30 to
> * make sure BL30 gets the values it expects when reading the
> @@ -728,6 +744,16 @@ static int meson_sar_adc_hw_disable(struct iio_dev *indio_dev)
> return 0;
> }
>
> +static irqreturn_t meson_sar_adc_irq(int irq, void *data)
> +{
> + struct iio_dev *indio_dev = data;
> + struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
> +
> + complete(&priv->done);
> +
> + return IRQ_HANDLED;
would it make sense to return IRQ_NONE when
meson_sar_adc_get_fifo_count() returns 0?
> +}
> +
> static const struct iio_info meson_sar_adc_iio_info = {
> .read_raw = meson_sar_adc_iio_info_read_raw,
> .driver_module = THIS_MODULE,
> @@ -770,7 +796,7 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
> struct resource *res;
> void __iomem *base;
> const struct of_device_id *match;
> - int ret;
> + int irq, ret;
>
> indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*priv));
> if (!indio_dev) {
> @@ -779,6 +805,7 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
> }
>
> priv = iio_priv(indio_dev);
> + init_completion(&priv->done);
>
> match = of_match_device(meson_sar_adc_of_match, &pdev->dev);
> priv->data = match->data;
> @@ -797,6 +824,15 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
> if (IS_ERR(base))
> return PTR_ERR(base);
>
> + irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
Documentation/devicetree/bindings/iio/adc/amlogic,meson-saradc.txt
should also be updated to reflect that the IRQ is now required (the
.dts already contains the IRQ, so no change is required on that side).
> + if (!irq)
> + return -EINVAL;
> +
> + ret = devm_request_irq(&pdev->dev, irq, meson_sar_adc_irq, 0,
> + dev_name(&pdev->dev), indio_dev);
> + if (ret)
> + return ret;
> +
> priv->regmap = devm_regmap_init_mmio(&pdev->dev, base,
> &meson_sar_adc_regmap_config);
> if (IS_ERR(priv->regmap))
> --
> 2.11.1
>
Regards,
Martin
[0] https://github.com/khadas/u-boot/blob/d09ed9f0c69054e46af45a7aed2b3cb0f6eaa3bc/drivers/adc/saradc.c#L191
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] iio: adc: meson-saradc: switch from polling to interrupt mode
2017-02-14 23:16 ` Martin Blumenstingl
@ 2017-02-15 6:31 ` Heiner Kallweit
2017-02-15 19:20 ` Heiner Kallweit
1 sibling, 0 replies; 4+ messages in thread
From: Heiner Kallweit @ 2017-02-15 6:31 UTC (permalink / raw)
To: Martin Blumenstingl
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio
Am 15.02.2017 um 00:16 schrieb Martin Blumenstingl:
> Hi Heiner,
>
> thanks for this patch!
> I have some questions since I don't have access to my boards until
> Saturday - I added them inline below.
>
> On Tue, Feb 14, 2017 at 10:58 PM, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>> Switch from polling to interrupt mode.
>>
>> Successfully tested on a S905GXBB-based Odroid C2.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> drivers/iio/adc/meson_saradc.c | 46 +++++++++++++++++++++++++++++++++++++-----
>> 1 file changed, 41 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
>> index 89def603..dbd56bcc 100644
>> --- a/drivers/iio/adc/meson_saradc.c
>> +++ b/drivers/iio/adc/meson_saradc.c
>> @@ -18,7 +18,9 @@
>> #include <linux/io.h>
>> #include <linux/iio/iio.h>
>> #include <linux/module.h>
>> +#include <linux/interrupt.h>
>> #include <linux/of.h>
>> +#include <linux/of_irq.h>
>> #include <linux/of_device.h>
>> #include <linux/platform_device.h>
>> #include <linux/regmap.h>
>> @@ -163,6 +165,7 @@
>> #define MESON_SAR_ADC_REG13_12BIT_CALIBRATION_MASK GENMASK(13, 8)
>>
>> #define MESON_SAR_ADC_MAX_FIFO_SIZE 32
>> +#define MESON_SAR_ADC_TIMEOUT 100 /* ms */
>>
>> #define MESON_SAR_ADC_CHAN(_chan) { \
>> .type = IIO_VOLTAGE, \
>> @@ -229,6 +232,7 @@ struct meson_sar_adc_priv {
>> struct clk_gate clk_gate;
>> struct clk *adc_div_clk;
>> struct clk_divider clk_div;
>> + struct completion done;
>> };
>>
>> static const struct regmap_config meson_sar_adc_regmap_config = {
>> @@ -274,11 +278,11 @@ static int meson_sar_adc_read_raw_sample(struct iio_dev *indio_dev,
>> int *val)
>> {
>> struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
>> - int ret, regval, fifo_chan, fifo_val, sum = 0, count = 0;
>> + int regval, fifo_chan, fifo_val, sum = 0, count = 0;
>>
>> - ret = meson_sar_adc_wait_busy_clear(indio_dev);
>> - if (ret)
>> - return ret;
>> + if(!wait_for_completion_timeout(&priv->done,
>> + msecs_to_jiffies(MESON_SAR_ADC_TIMEOUT)))
>> + return -ETIMEDOUT;
>>
>> while (meson_sar_adc_get_fifo_count(indio_dev) > 0 &&
>> count < MESON_SAR_ADC_MAX_FIFO_SIZE) {
>> @@ -456,6 +460,7 @@ static int meson_sar_adc_get_sample(struct iio_dev *indio_dev,
>> enum meson_sar_adc_num_samples avg_samples,
>> int *val)
>> {
>> + struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
>> int ret;
>>
>> ret = meson_sar_adc_lock(indio_dev);
>> @@ -465,6 +470,11 @@ static int meson_sar_adc_get_sample(struct iio_dev *indio_dev,
>> /* clear the FIFO to make sure we're not reading old values */
>> meson_sar_adc_clear_fifo(indio_dev);
>>
>> + reinit_completion(&priv->done);
>> + regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG0,
>> + MESON_SAR_ADC_REG0_FIFO_IRQ_EN,
>> + MESON_SAR_ADC_REG0_FIFO_IRQ_EN);
>> +
> can't we add this to meson_sar_adc_start_sample_engine()?
>
>> meson_sar_adc_set_averaging(indio_dev, chan, avg_mode, avg_samples);
>>
>> meson_sar_adc_enable_channel(indio_dev, chan);
>> @@ -473,6 +483,9 @@ static int meson_sar_adc_get_sample(struct iio_dev *indio_dev,
>> ret = meson_sar_adc_read_raw_sample(indio_dev, chan, val);
>> meson_sar_adc_stop_sample_engine(indio_dev);
>>
>> + regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG0,
>> + MESON_SAR_ADC_REG0_FIFO_IRQ_EN, 0);
>> +
> similar to enabling the IRQ above, can't we add this to
> meson_sar_adc_stop_sample_engine()?
>
>> meson_sar_adc_unlock(indio_dev);
>>
>> if (ret) {
>> @@ -577,6 +590,9 @@ static int meson_sar_adc_init(struct iio_dev *indio_dev)
>> */
>> meson_sar_adc_set_chan7_mux(indio_dev, CHAN7_MUX_CH7_INPUT);
>>
>> + regval = FIELD_PREP(MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, 1);
>> + regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG0,
>> + MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, regval);
> I'm wondering whether we should do this in meson_sar_adc_hw_enable()
> in case u-boot is involved in the "return from suspend" process then
> doing it in _init() might be a problem since the vendor u-boot
> overwrites REG0, see [0]
> please note that this is an actual question, not some kind of
> suggestion in question form
>
>> /*
>> * leave sampling delay and the input clocks as configured by BL30 to
>> * make sure BL30 gets the values it expects when reading the
>> @@ -728,6 +744,16 @@ static int meson_sar_adc_hw_disable(struct iio_dev *indio_dev)
>> return 0;
>> }
>>
>> +static irqreturn_t meson_sar_adc_irq(int irq, void *data)
>> +{
>> + struct iio_dev *indio_dev = data;
>> + struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
>> +
>> + complete(&priv->done);
>> +
>> + return IRQ_HANDLED;
> would it make sense to return IRQ_NONE when
> meson_sar_adc_get_fifo_count() returns 0?
>
>> +}
>> +
>> static const struct iio_info meson_sar_adc_iio_info = {
>> .read_raw = meson_sar_adc_iio_info_read_raw,
>> .driver_module = THIS_MODULE,
>> @@ -770,7 +796,7 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
>> struct resource *res;
>> void __iomem *base;
>> const struct of_device_id *match;
>> - int ret;
>> + int irq, ret;
>>
>> indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*priv));
>> if (!indio_dev) {
>> @@ -779,6 +805,7 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
>> }
>>
>> priv = iio_priv(indio_dev);
>> + init_completion(&priv->done);
>>
>> match = of_match_device(meson_sar_adc_of_match, &pdev->dev);
>> priv->data = match->data;
>> @@ -797,6 +824,15 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
>> if (IS_ERR(base))
>> return PTR_ERR(base);
>>
>> + irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> Documentation/devicetree/bindings/iio/adc/amlogic,meson-saradc.txt
> should also be updated to reflect that the IRQ is now required (the
> .dts already contains the IRQ, so no change is required on that side).
>
>> + if (!irq)
>> + return -EINVAL;
>> +
>> + ret = devm_request_irq(&pdev->dev, irq, meson_sar_adc_irq, 0,
>> + dev_name(&pdev->dev), indio_dev);
>> + if (ret)
>> + return ret;
>> +
>> priv->regmap = devm_regmap_init_mmio(&pdev->dev, base,
>> &meson_sar_adc_regmap_config);
>> if (IS_ERR(priv->regmap))
>> --
>> 2.11.1
>>
>
>
> Regards,
> Martin
>
> [0] https://github.com/khadas/u-boot/blob/d09ed9f0c69054e46af45a7aed2b3cb0f6eaa3bc/drivers/adc/saradc.c#L191
>
All valid points and thanks for the link to the saradc vendor uboot driver.
I'll come up with a v2.
Rgds, Heiner
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] iio: adc: meson-saradc: switch from polling to interrupt mode
2017-02-14 23:16 ` Martin Blumenstingl
2017-02-15 6:31 ` Heiner Kallweit
@ 2017-02-15 19:20 ` Heiner Kallweit
1 sibling, 0 replies; 4+ messages in thread
From: Heiner Kallweit @ 2017-02-15 19:20 UTC (permalink / raw)
To: Martin Blumenstingl
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio
Am 15.02.2017 um 00:16 schrieb Martin Blumenstingl:
> Hi Heiner,
>
> thanks for this patch!
> I have some questions since I don't have access to my boards until
> Saturday - I added them inline below.
>
> On Tue, Feb 14, 2017 at 10:58 PM, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>> Switch from polling to interrupt mode.
>>
>> Successfully tested on a S905GXBB-based Odroid C2.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> drivers/iio/adc/meson_saradc.c | 46 +++++++++++++++++++++++++++++++++++++-----
>> 1 file changed, 41 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
>> index 89def603..dbd56bcc 100644
>> --- a/drivers/iio/adc/meson_saradc.c
>> +++ b/drivers/iio/adc/meson_saradc.c
>> @@ -18,7 +18,9 @@
>> #include <linux/io.h>
>> #include <linux/iio/iio.h>
>> #include <linux/module.h>
>> +#include <linux/interrupt.h>
>> #include <linux/of.h>
>> +#include <linux/of_irq.h>
>> #include <linux/of_device.h>
>> #include <linux/platform_device.h>
>> #include <linux/regmap.h>
>> @@ -163,6 +165,7 @@
>> #define MESON_SAR_ADC_REG13_12BIT_CALIBRATION_MASK GENMASK(13, 8)
>>
>> #define MESON_SAR_ADC_MAX_FIFO_SIZE 32
>> +#define MESON_SAR_ADC_TIMEOUT 100 /* ms */
>>
>> #define MESON_SAR_ADC_CHAN(_chan) { \
>> .type = IIO_VOLTAGE, \
>> @@ -229,6 +232,7 @@ struct meson_sar_adc_priv {
>> struct clk_gate clk_gate;
>> struct clk *adc_div_clk;
>> struct clk_divider clk_div;
>> + struct completion done;
>> };
>>
>> static const struct regmap_config meson_sar_adc_regmap_config = {
>> @@ -274,11 +278,11 @@ static int meson_sar_adc_read_raw_sample(struct iio_dev *indio_dev,
>> int *val)
>> {
>> struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
>> - int ret, regval, fifo_chan, fifo_val, sum = 0, count = 0;
>> + int regval, fifo_chan, fifo_val, sum = 0, count = 0;
>>
>> - ret = meson_sar_adc_wait_busy_clear(indio_dev);
>> - if (ret)
>> - return ret;
>> + if(!wait_for_completion_timeout(&priv->done,
>> + msecs_to_jiffies(MESON_SAR_ADC_TIMEOUT)))
>> + return -ETIMEDOUT;
>>
>> while (meson_sar_adc_get_fifo_count(indio_dev) > 0 &&
>> count < MESON_SAR_ADC_MAX_FIFO_SIZE) {
>> @@ -456,6 +460,7 @@ static int meson_sar_adc_get_sample(struct iio_dev *indio_dev,
>> enum meson_sar_adc_num_samples avg_samples,
>> int *val)
>> {
>> + struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
>> int ret;
>>
>> ret = meson_sar_adc_lock(indio_dev);
>> @@ -465,6 +470,11 @@ static int meson_sar_adc_get_sample(struct iio_dev *indio_dev,
>> /* clear the FIFO to make sure we're not reading old values */
>> meson_sar_adc_clear_fifo(indio_dev);
>>
>> + reinit_completion(&priv->done);
>> + regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG0,
>> + MESON_SAR_ADC_REG0_FIFO_IRQ_EN,
>> + MESON_SAR_ADC_REG0_FIFO_IRQ_EN);
>> +
> can't we add this to meson_sar_adc_start_sample_engine()?
>
>> meson_sar_adc_set_averaging(indio_dev, chan, avg_mode, avg_samples);
>>
>> meson_sar_adc_enable_channel(indio_dev, chan);
>> @@ -473,6 +483,9 @@ static int meson_sar_adc_get_sample(struct iio_dev *indio_dev,
>> ret = meson_sar_adc_read_raw_sample(indio_dev, chan, val);
>> meson_sar_adc_stop_sample_engine(indio_dev);
>>
>> + regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG0,
>> + MESON_SAR_ADC_REG0_FIFO_IRQ_EN, 0);
>> +
> similar to enabling the IRQ above, can't we add this to
> meson_sar_adc_stop_sample_engine()?
>
>> meson_sar_adc_unlock(indio_dev);
>>
>> if (ret) {
>> @@ -577,6 +590,9 @@ static int meson_sar_adc_init(struct iio_dev *indio_dev)
>> */
>> meson_sar_adc_set_chan7_mux(indio_dev, CHAN7_MUX_CH7_INPUT);
>>
>> + regval = FIELD_PREP(MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, 1);
>> + regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG0,
>> + MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, regval);
> I'm wondering whether we should do this in meson_sar_adc_hw_enable()
> in case u-boot is involved in the "return from suspend" process then
> doing it in _init() might be a problem since the vendor u-boot
> overwrites REG0, see [0]
> please note that this is an actual question, not some kind of
> suggestion in question form
>
At a first glance I couldn't see that in uboot saradc_enable is used
somewhere in the boot process. There seems to be a uboot command line
command triggering saradc_enable.
But I'm no expert at all to which extent uboot is involved in system
suspend / resume, so I might miss something.
Moving this register initialization to meson_sar_adc_hw_enable()
doesn't hurt and we're safe in case the scenario you described
can actually occur. So I'd go for it.
>> /*
>> * leave sampling delay and the input clocks as configured by BL30 to
>> * make sure BL30 gets the values it expects when reading the
>> @@ -728,6 +744,16 @@ static int meson_sar_adc_hw_disable(struct iio_dev *indio_dev)
>> return 0;
>> }
>>
>> +static irqreturn_t meson_sar_adc_irq(int irq, void *data)
>> +{
>> + struct iio_dev *indio_dev = data;
>> + struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
>> +
>> + complete(&priv->done);
>> +
>> + return IRQ_HANDLED;
> would it make sense to return IRQ_NONE when
> meson_sar_adc_get_fifo_count() returns 0?
>
>> +}
>> +
>> static const struct iio_info meson_sar_adc_iio_info = {
>> .read_raw = meson_sar_adc_iio_info_read_raw,
>> .driver_module = THIS_MODULE,
>> @@ -770,7 +796,7 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
>> struct resource *res;
>> void __iomem *base;
>> const struct of_device_id *match;
>> - int ret;
>> + int irq, ret;
>>
>> indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*priv));
>> if (!indio_dev) {
>> @@ -779,6 +805,7 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
>> }
>>
>> priv = iio_priv(indio_dev);
>> + init_completion(&priv->done);
>>
>> match = of_match_device(meson_sar_adc_of_match, &pdev->dev);
>> priv->data = match->data;
>> @@ -797,6 +824,15 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
>> if (IS_ERR(base))
>> return PTR_ERR(base);
>>
>> + irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> Documentation/devicetree/bindings/iio/adc/amlogic,meson-saradc.txt
> should also be updated to reflect that the IRQ is now required (the
> .dts already contains the IRQ, so no change is required on that side).
>
>> + if (!irq)
>> + return -EINVAL;
>> +
>> + ret = devm_request_irq(&pdev->dev, irq, meson_sar_adc_irq, 0,
>> + dev_name(&pdev->dev), indio_dev);
>> + if (ret)
>> + return ret;
>> +
>> priv->regmap = devm_regmap_init_mmio(&pdev->dev, base,
>> &meson_sar_adc_regmap_config);
>> if (IS_ERR(priv->regmap))
>> --
>> 2.11.1
>>
>
>
> Regards,
> Martin
>
> [0] https://github.com/khadas/u-boot/blob/d09ed9f0c69054e46af45a7aed2b3cb0f6eaa3bc/drivers/adc/saradc.c#L191
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-02-15 19:20 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-02-14 21:58 [PATCH 1/2] iio: adc: meson-saradc: switch from polling to interrupt mode Heiner Kallweit
2017-02-14 23:16 ` Martin Blumenstingl
2017-02-15 6:31 ` Heiner Kallweit
2017-02-15 19:20 ` Heiner Kallweit
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).