[linux-sunxi] Re: [PATCH v2 08/16] iio: adc: sun4i-gpadc-iio: rework: add interrupt support
On 31.01.2018 20:07, Quentin Schulz wrote:
Hi Philipp,
On Mon, Jan 29, 2018 at 12:29:11AM +0100, Philipp Rossak wrote:
This patch rewors the driver to support interrupts for the thermal part
of the sensor.
This is only available for the newer sensor (currently H3 and A83T).
The interrupt will be trigerd on data available and triggers the update
for the thermal sensors. All newer sensors have different amount of
sensors and different interrupts for each device the reset of the
interrupts need to be done different
For the newer sensors is the autosuspend disabled.
Signed-off-by: Philipp Rossak
Acked-by: Jonathan Cameron
---
drivers/iio/adc/sun4i-gpadc-iio.c | 60 +++
include/linux/mfd/sun4i-gpadc.h | 2 ++
2 files changed, 56 insertions(+), 6 deletions(-)
diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c
b/drivers/iio/adc/sun4i-gpadc-iio.c
index 74eeb5cd5218..b7b5451226b0 100644
--- a/drivers/iio/adc/sun4i-gpadc-iio.c
+++ b/drivers/iio/adc/sun4i-gpadc-iio.c
@@ -71,11 +71,14 @@ struct gpadc_data {
unsigned inttemp_data[MAX_SENSOR_COUNT];
int (*sample_start)(struct sun4i_gpadc_iio *info);
int (*sample_end)(struct sun4i_gpadc_iio *info);
+ u32 irq_clear_map;
+ u32 irq_control_map;
I would say to use a regmap_irq_chip for controlling IRQs.
Sounds good for me! I will rework that in the next version.
boolhas_bus_clk;
boolhas_bus_rst;
boolhas_mod_clk;
int sensor_count;
boolsupports_nvmem;
+ boolsupport_irq;
};
static const struct gpadc_data sun4i_gpadc_data = {
@@ -90,6 +93,7 @@ static const struct gpadc_data sun4i_gpadc_data = {
.sample_end = sun4i_gpadc_sample_end,
.sensor_count = 1,
.supports_nvmem = false,
+ .support_irq = false,
False is the default, no need to set support_irq.
[...]
struct sun4i_gpadc_iio {
@@ -332,6 +339,11 @@ static int sun4i_gpadc_temp_read(struct iio_dev
*indio_dev, int *val,
return 0;
}
+ if (info->data->support_irq) {
+ regmap_read(info->regmap, info->data->temp_data[sensor], val);
+ return 0;
+ }
+
Maybe you could define a new thermal_zone_of_device_ops for these new
thermal sensors? That way, you don't even need the boolean support_irq.
Sounds good for me! I will rework that in the next version.
return sun4i_gpadc_read(indio_dev, 0, val, info->temp_data_irq);
}
@@ -429,6 +441,17 @@ static irqreturn_t sun4i_gpadc_fifo_data_irq_handler(int irq, void *dev_id)
return IRQ_HANDLED;
}
+static irqreturn_t sunxi_irq_thread(int irq, void *data)
I think we're trying to avoid sunxi mentions but rather using the name
of the first IP (in term of product release, not support) using this
function.
+{
+ struct sun4i_gpadc_iio *info = data;
+
+ regmap_write(info->regmap, SUN8I_H3_THS_STAT,
info->data->irq_clear_map);
+
Will be handled by regmap_irq_chip.
[...]
- info->no_irq = true;
+ if (info->data->support_irq) {
+ /* only the new versions of ths support right now irqs */
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0) {
+ dev_err(&pdev->dev, "failed to get IRQ: %d\n", irq);
+ return irq;
+ }
+
+ ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+ sunxi_irq_thread, IRQF_ONESHOT,
+ dev_name(&pdev->dev), info);
+ if (ret)
+ return ret;
+
+ } else
+ info->no_irq = true;
+
That's a bit funny to have two booleans named no_irq and support_irq :)
I know this looks very funny. I thought this would be better to keep, to
_not_ break anything. Since I will rework the whole driver and integrate
the mfd part I hope I can remove both.
indio_dev->num_channels = ARRAY_SIZE(sun8i_a33_gpadc_channels);
indio_dev->channels = sun8i_a33_gpadc_channels;
@@ -789,11 +829,13 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
if (ret)
return ret;
- pm_runtime_set_autosuspend_delay(&pdev->dev,
-SUN4I_GPADC_AUTOSUSPEND_DELAY);
- pm_runtime_use_autosuspend(&pdev->dev);
- pm_runtime_set_suspended(&pdev->dev);
- pm_runtime_enable(&pdev->dev);
+ if (!info->data->support_irq) {
+ pm_runtime_set_autosuspend_delay(&pdev->dev,
+SUN4I_GPADC_AUTOSUSPEND_DELAY);
+ pm_runtime_use_autosuspend(&pdev->dev);
+ pm_runtime_set_suspended(&pdev->dev);
+ pm_runtime_enable(&pdev->dev);
+ }
Why would you not want your IP to do runtime PM?
I will ena
[linux-sunxi] Re: [PATCH v2 08/16] iio: adc: sun4i-gpadc-iio: rework: add interrupt support
Hi Philipp,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on robh/for-next]
[also build test ERROR on v4.15 next-20180126]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Philipp-Rossak/IIO-based-thermal-sensor-driver-for-Allwinner-H3-and-A83T-SoC/20180201-043415
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: xtensa-allmodconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 7.2.0
reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=xtensa
Note: the
linux-review/Philipp-Rossak/IIO-based-thermal-sensor-driver-for-Allwinner-H3-and-A83T-SoC/20180201-043415
HEAD e571c3ced84a9d7f150cb2d239919d31d25114e2 builds fine.
It only hurts bisectibility.
All errors (new ones prefixed by >>):
drivers/iio/adc/sun4i-gpadc-iio.c: In function 'sunxi_irq_thread':
>> drivers/iio/adc/sun4i-gpadc-iio.c:448:29: error: 'SUN8I_H3_THS_STAT'
>> undeclared (first use in this function); did you mean 'SUN8I_H3_THS_INTC'?
regmap_write(info->regmap, SUN8I_H3_THS_STAT, info->data->irq_clear_map);
^
SUN8I_H3_THS_INTC
drivers/iio/adc/sun4i-gpadc-iio.c:448:29: note: each undeclared identifier
is reported only once for each function it appears in
vim +448 drivers/iio/adc/sun4i-gpadc-iio.c
443
444 static irqreturn_t sunxi_irq_thread(int irq, void *data)
445 {
446 struct sun4i_gpadc_iio *info = data;
447
> 448 regmap_write(info->regmap, SUN8I_H3_THS_STAT,
> info->data->irq_clear_map);
449
450 thermal_zone_device_update(info->tzd,
THERMAL_EVENT_TEMP_SAMPLE);
451
452 return IRQ_HANDLED;
453 }
454
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
--
You received this message because you are subscribed to the Google Groups
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
.config.gz
Description: application/gzip
[linux-sunxi] Re: [PATCH v2 08/16] iio: adc: sun4i-gpadc-iio: rework: add interrupt support
Hi Philipp,
On Mon, Jan 29, 2018 at 12:29:11AM +0100, Philipp Rossak wrote:
> This patch rewors the driver to support interrupts for the thermal part
> of the sensor.
>
> This is only available for the newer sensor (currently H3 and A83T).
> The interrupt will be trigerd on data available and triggers the update
> for the thermal sensors. All newer sensors have different amount of
> sensors and different interrupts for each device the reset of the
> interrupts need to be done different
>
> For the newer sensors is the autosuspend disabled.
>
> Signed-off-by: Philipp Rossak
> Acked-by: Jonathan Cameron
> ---
> drivers/iio/adc/sun4i-gpadc-iio.c | 60
> +++
> include/linux/mfd/sun4i-gpadc.h | 2 ++
> 2 files changed, 56 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c
> b/drivers/iio/adc/sun4i-gpadc-iio.c
> index 74eeb5cd5218..b7b5451226b0 100644
> --- a/drivers/iio/adc/sun4i-gpadc-iio.c
> +++ b/drivers/iio/adc/sun4i-gpadc-iio.c
> @@ -71,11 +71,14 @@ struct gpadc_data {
> unsigned inttemp_data[MAX_SENSOR_COUNT];
> int (*sample_start)(struct sun4i_gpadc_iio *info);
> int (*sample_end)(struct sun4i_gpadc_iio *info);
> + u32 irq_clear_map;
> + u32 irq_control_map;
I would say to use a regmap_irq_chip for controlling IRQs.
> boolhas_bus_clk;
> boolhas_bus_rst;
> boolhas_mod_clk;
> int sensor_count;
> boolsupports_nvmem;
> + boolsupport_irq;
> };
>
> static const struct gpadc_data sun4i_gpadc_data = {
> @@ -90,6 +93,7 @@ static const struct gpadc_data sun4i_gpadc_data = {
> .sample_end = sun4i_gpadc_sample_end,
> .sensor_count = 1,
> .supports_nvmem = false,
> + .support_irq = false,
False is the default, no need to set support_irq.
[...]
> struct sun4i_gpadc_iio {
> @@ -332,6 +339,11 @@ static int sun4i_gpadc_temp_read(struct iio_dev
> *indio_dev, int *val,
> return 0;
> }
>
> + if (info->data->support_irq) {
> + regmap_read(info->regmap, info->data->temp_data[sensor], val);
> + return 0;
> + }
> +
Maybe you could define a new thermal_zone_of_device_ops for these new
thermal sensors? That way, you don't even need the boolean support_irq.
> return sun4i_gpadc_read(indio_dev, 0, val, info->temp_data_irq);
> }
>
> @@ -429,6 +441,17 @@ static irqreturn_t sun4i_gpadc_fifo_data_irq_handler(int
> irq, void *dev_id)
> return IRQ_HANDLED;
> }
>
> +static irqreturn_t sunxi_irq_thread(int irq, void *data)
I think we're trying to avoid sunxi mentions but rather using the name
of the first IP (in term of product release, not support) using this
function.
> +{
> + struct sun4i_gpadc_iio *info = data;
> +
> + regmap_write(info->regmap, SUN8I_H3_THS_STAT,
> info->data->irq_clear_map);
> +
Will be handled by regmap_irq_chip.
[...]
> - info->no_irq = true;
> + if (info->data->support_irq) {
> + /* only the new versions of ths support right now irqs */
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + dev_err(&pdev->dev, "failed to get IRQ: %d\n", irq);
> + return irq;
> + }
> +
> + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> + sunxi_irq_thread, IRQF_ONESHOT,
> + dev_name(&pdev->dev), info);
> + if (ret)
> + return ret;
> +
> + } else
> + info->no_irq = true;
> +
That's a bit funny to have two booleans named no_irq and support_irq :)
> indio_dev->num_channels = ARRAY_SIZE(sun8i_a33_gpadc_channels);
> indio_dev->channels = sun8i_a33_gpadc_channels;
>
> @@ -789,11 +829,13 @@ static int sun4i_gpadc_probe(struct platform_device
> *pdev)
> if (ret)
> return ret;
>
> - pm_runtime_set_autosuspend_delay(&pdev->dev,
> - SUN4I_GPADC_AUTOSUSPEND_DELAY);
> - pm_runtime_use_autosuspend(&pdev->dev);
> - pm_runtime_set_suspended(&pdev->dev);
> - pm_runtime_enable(&pdev->dev);
> + if (!info->data->support_irq) {
> + pm_runtime_set_autosuspend_delay(&pdev->dev,
> + SUN4I_GPADC_AUTOSUSPEND_DELAY);
> + pm_runtime_use_autosuspend(&pdev->dev);
> + pm_runtime_set_suspended(&pdev->dev);
> + pm_runtime_enable(&pdev->dev);
> + }
Why would you not want your IP to do runtime PM?
Quentin
--
You received this message because you are subscribed to the Google Groups
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com
