| Message ID | 20180128232919.12639-9-embed3d@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <embed3d@gmail.com> > Acked-by: Jonathan Cameron <jonathan.cameron@huawei.com> > --- > 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 int temp_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. > bool has_bus_clk; > bool has_bus_rst; > bool has_mod_clk; > int sensor_count; > bool supports_nvmem; > + bool support_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
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 infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
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 <embed3d@gmail.com> >> Acked-by: Jonathan Cameron <jonathan.cameron@huawei.com> >> --- >> 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 int temp_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. >> bool has_bus_clk; >> bool has_bus_rst; >> bool has_mod_clk; >> int sensor_count; >> bool supports_nvmem; >> + bool support_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 enable this again, in the next version! I had some issues with this, thus I disabled this, but I know now what I did wrong! Thanks, Philipp
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 int temp_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; bool has_bus_clk; bool has_bus_rst; bool has_mod_clk; int sensor_count; bool supports_nvmem; + bool support_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, }; static const struct gpadc_data sun5i_gpadc_data = { @@ -104,6 +108,7 @@ static const struct gpadc_data sun5i_gpadc_data = { .sample_end = sun4i_gpadc_sample_end, .sensor_count = 1, .supports_nvmem = false, + .support_irq = false, }; static const struct gpadc_data sun6i_gpadc_data = { @@ -118,6 +123,7 @@ static const struct gpadc_data sun6i_gpadc_data = { .sample_end = sun4i_gpadc_sample_end, .sensor_count = 1, .supports_nvmem = false, + .support_irq = false, }; static const struct gpadc_data sun8i_a33_gpadc_data = { @@ -129,6 +135,7 @@ static const struct gpadc_data sun8i_a33_gpadc_data = { .sample_end = sun4i_gpadc_sample_end, .sensor_count = 1, .supports_nvmem = false, + .support_irq = false, }; 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; + } + 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) +{ + struct sun4i_gpadc_iio *info = data; + + regmap_write(info->regmap, SUN8I_H3_THS_STAT, info->data->irq_clear_map); + + thermal_zone_device_update(info->tzd, THERMAL_EVENT_TEMP_SAMPLE); + + return IRQ_HANDLED; +} + static int sun4i_gpadc_sample_end(struct sun4i_gpadc_iio *info) { /* Disable the ADC on IP */ @@ -572,12 +595,29 @@ static int sun4i_gpadc_probe_dt(struct platform_device *pdev, struct nvmem_cell *cell; ssize_t cell_size; u64 *cell_data; + int irq; info->data = of_device_get_match_data(&pdev->dev); if (!info->data) return -ENODEV; - 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; + 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); + } if (IS_ENABLED(CONFIG_THERMAL_OF)) { for (i = 0; i < info->data->sensor_count; i++) { @@ -814,6 +856,9 @@ static int sun4i_gpadc_probe(struct platform_device *pdev) } } + if (info->data->support_irq) + info->data->sample_start(info); + ret = devm_iio_device_register(&pdev->dev, indio_dev); if (ret < 0) { dev_err(&pdev->dev, "could not register the device\n"); @@ -843,6 +888,9 @@ static int sun4i_gpadc_remove(struct platform_device *pdev) if (!IS_ENABLED(CONFIG_THERMAL_OF)) return 0; + if (info->data->support_irq) + info->data->sample_end(info); + thermal_zone_of_sensor_unregister(info->sensor_device, info->tzd); if (!info->no_irq) diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h index 20fa02040007..458f2159a95a 100644 --- a/include/linux/mfd/sun4i-gpadc.h +++ b/include/linux/mfd/sun4i-gpadc.h @@ -91,6 +91,8 @@ #define SUN4I_GPADC_AUTOSUSPEND_DELAY 10000 /* SUNXI_THS COMMON REGISTERS + DEFINES */ +#define SUN8I_H3_THS_INTC 0x44 + #define MAX_SENSOR_COUNT 4 struct sun4i_gpadc_dev {