| Message ID | 20190503072813.2719-1-tiny.windzz@gmail.com |
|---|---|
| Headers | show |
| Series | Add support for H6 thermal sensor | expand |
On Fri, May 03, 2019 at 03:28:13AM -0400, Yangtao Li wrote: > Updates license to use SPDX-License-Identifier. > > Signed-off-by: Yangtao Li <tiny.windzz@gmail.com> > --- > drivers/iio/adc/sun4i-gpadc-iio.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c > index 9b6fc592f54c..cf2bf3ab3342 100644 > --- a/drivers/iio/adc/sun4i-gpadc-iio.c > +++ b/drivers/iio/adc/sun4i-gpadc-iio.c > @@ -1,11 +1,8 @@ > +// SPDX-License-Identifier: GPL-2.0+ > /* ADC driver for sunxi platforms' (A10, A13 and A31) GPADC > * > * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons.com> > * > - * This program is free software; you can redistribute it and/or modify it under > - * the terms of the GNU General Public License version 2 as published by the > - * Free Software Foundation. > - * The license here is GPL2 only, not GPL2 or later. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Fri, 3 May 2019 03:28:07 -0400 Yangtao Li <tiny.windzz@gmail.com> wrote: > For some SOCs, there are more than one thermal sensor, and there are > currently four sensors on the A80. So we need to do some work in order > to support multiple thermal sensors: > > 1) add sensor_count in gpadc_data. > 2) introduce sun4i_sensor_tzd in sun4i_gpadc_iio, to support multiple > thermal_zone_device and distinguish between different sensors. > 3) modify read temperature and initialization function. This comment doesn't mention the devm change. If it had it would have raised immediate alarm bells. I'm also not keen on the web of pointers that this driver is steadily evolving. I can't immediately see how to reduce that complexity however. Jonathan > > Signed-off-by: Yangtao Li <tiny.windzz@gmail.com> > --- > drivers/iio/adc/sun4i-gpadc-iio.c | 61 +++++++++++++++++++++++-------- > 1 file changed, 45 insertions(+), 16 deletions(-) > > diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c > index 04d7147e0110..844fd52bd22f 100644 > --- a/drivers/iio/adc/sun4i-gpadc-iio.c > +++ b/drivers/iio/adc/sun4i-gpadc-iio.c > @@ -56,6 +56,7 @@ struct gpadc_data { > unsigned int tp_adc_select; > unsigned int (*adc_chan_select)(unsigned int chan); > unsigned int adc_chan_mask; > + unsigned int sensor_count; > }; > > static const struct gpadc_data sun4i_gpadc_data = { > @@ -65,6 +66,7 @@ static const struct gpadc_data sun4i_gpadc_data = { > .tp_adc_select = SUN4I_GPADC_CTRL1_TP_ADC_SELECT, > .adc_chan_select = &sun4i_gpadc_chan_select, > .adc_chan_mask = SUN4I_GPADC_CTRL1_ADC_CHAN_MASK, > + .sensor_count = 1, > }; > > static const struct gpadc_data sun5i_gpadc_data = { > @@ -74,6 +76,7 @@ static const struct gpadc_data sun5i_gpadc_data = { > .tp_adc_select = SUN4I_GPADC_CTRL1_TP_ADC_SELECT, > .adc_chan_select = &sun4i_gpadc_chan_select, > .adc_chan_mask = SUN4I_GPADC_CTRL1_ADC_CHAN_MASK, > + .sensor_count = 1, > }; > > static const struct gpadc_data sun6i_gpadc_data = { > @@ -83,14 +86,24 @@ static const struct gpadc_data sun6i_gpadc_data = { > .tp_adc_select = SUN6I_GPADC_CTRL1_TP_ADC_SELECT, > .adc_chan_select = &sun6i_gpadc_chan_select, > .adc_chan_mask = SUN6I_GPADC_CTRL1_ADC_CHAN_MASK, > + .sensor_count = 1, > }; > > static const struct gpadc_data sun8i_a33_gpadc_data = { > .temp_offset = -1662, > .temp_scale = 162, > .tp_mode_en = SUN8I_GPADC_CTRL1_CHOP_TEMP_EN, > + .sensor_count = 1, > }; > > +struct sun4i_sensor_tzd { > + struct sun4i_gpadc_iio *info; > + struct thermal_zone_device *tzd; > + unsigned int sensor_id; > +}; > + > +#define MAX_SENSOR_COUNT 4 > + > struct sun4i_gpadc_iio { > struct iio_dev *indio_dev; > struct completion completion; > @@ -105,7 +118,7 @@ struct sun4i_gpadc_iio { > bool no_irq; > /* prevents concurrent reads of temperature and ADC */ > struct mutex mutex; > - struct thermal_zone_device *tzd; > + struct sun4i_sensor_tzd tzds[MAX_SENSOR_COUNT]; > struct device *sensor_device; > }; > > @@ -270,7 +283,8 @@ static int sun4i_gpadc_adc_read(struct iio_dev *indio_dev, int channel, > return sun4i_gpadc_read(indio_dev, channel, val, info->fifo_data_irq); > } > > -static int sun4i_gpadc_temp_read(struct iio_dev *indio_dev, int *val) > +static int sun4i_gpadc_temp_read(struct iio_dev *indio_dev, int *val, > + unsigned int sensor) > { > struct sun4i_gpadc_iio *info = iio_priv(indio_dev); > > @@ -324,7 +338,7 @@ static int sun4i_gpadc_read_raw(struct iio_dev *indio_dev, > ret = sun4i_gpadc_adc_read(indio_dev, chan->channel, > val); > else > - ret = sun4i_gpadc_temp_read(indio_dev, val); > + ret = sun4i_gpadc_temp_read(indio_dev, val, 0); > > if (ret) > return ret; > @@ -417,10 +431,11 @@ static int sun4i_gpadc_runtime_resume(struct device *dev) > > static int sun4i_gpadc_get_temp(void *data, int *temp) > { > - struct sun4i_gpadc_iio *info = data; > + struct sun4i_sensor_tzd *tzd = data; > + struct sun4i_gpadc_iio *info = tzd->info; > int val, scale, offset; > > - if (sun4i_gpadc_temp_read(info->indio_dev, &val)) > + if (sun4i_gpadc_temp_read(info->indio_dev, &val, tzd->sensor_id)) > return -ETIMEDOUT; > > sun4i_gpadc_temp_scale(info->indio_dev, &scale); > @@ -609,6 +624,28 @@ static int sun4i_gpadc_probe_mfd(struct platform_device *pdev, > return 0; > } > > +static int sun4i_sensor_init(struct sun4i_gpadc_iio *info) > +{ > + int i = 0; > + > + for (; i < info->data->sensor_count; i++) { > + info->tzds[i].info = info; > + info->tzds[i].sensor_id = i; > + info->tzds[i].tzd = devm_thermal_zone_of_sensor_register( > + info->sensor_device, i, &info->tzds[i], > + &sun4i_ts_tz_ops); Using devm here has the result that the thermal_zones are remove 'after' the end of the remove function for this driver. That means you have them still registered after several other commands are run in the remove function. Please ensure that the probe and remove orders are mirror images of each other. Here it doesn't make a huge amount of actual difference but it does make the code harder to reason about. You 'could' propose a devm_ version of iio_map_array_register and then use that to ensure the ordering is correctly maintained. > + > + if (IS_ERR(info->tzds[i].tzd)) { > + dev_err(info->sensor_device, > + "could not register thermal sensor: %ld\n", > + PTR_ERR(info->tzds[i].tzd)); > + return PTR_ERR(info->tzds[i].tzd); > + } > + } > + > + return 0; > +} > + > static int sun4i_gpadc_probe(struct platform_device *pdev) > { > struct sun4i_gpadc_iio *info; > @@ -646,19 +683,13 @@ static int sun4i_gpadc_probe(struct platform_device *pdev) > pm_runtime_enable(&pdev->dev); > > if (IS_ENABLED(CONFIG_THERMAL_OF)) { > - info->tzd = thermal_zone_of_sensor_register(info->sensor_device, > - 0, info, > - &sun4i_ts_tz_ops); > + ret = sun4i_sensor_init(info); > /* > * Do not fail driver probing when failing to register in > * thermal because no thermal DT node is found. > */ > - if (IS_ERR(info->tzd) && PTR_ERR(info->tzd) != -ENODEV) { > - dev_err(&pdev->dev, > - "could not register thermal sensor: %ld\n", > - PTR_ERR(info->tzd)); > - return PTR_ERR(info->tzd); > - } > + if (ret && ret != -ENODEV) > + return ret; > } > > ret = devm_iio_device_register(&pdev->dev, indio_dev); > @@ -690,8 +721,6 @@ static int sun4i_gpadc_remove(struct platform_device *pdev) > if (!IS_ENABLED(CONFIG_THERMAL_OF)) > return 0; > > - thermal_zone_of_sensor_unregister(info->sensor_device, info->tzd); > - > if (!info->no_irq) > iio_map_array_unregister(indio_dev); >
On Fri, 3 May 2019 03:28:12 -0400 Yangtao Li <tiny.windzz@gmail.com> wrote: > This patch adds support for the H6 ths sensor. > > TODO: calibrate thermal sensor by using information from sid. > > Signed-off-by: Yangtao Li <tiny.windzz@gmail.com> This and the patches before it that I haven't comment on look fine to me. thanks, Jonathan > --- > drivers/iio/adc/sun4i-gpadc-iio.c | 65 +++++++++++++++++++++++++++++++ > include/linux/mfd/sun4i-gpadc.h | 9 +++++ > 2 files changed, 74 insertions(+) > > diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c > index f24eb76d65c0..9b6fc592f54c 100644 > --- a/drivers/iio/adc/sun4i-gpadc-iio.c > +++ b/drivers/iio/adc/sun4i-gpadc-iio.c > @@ -120,6 +120,20 @@ static const struct gpadc_data sun8i_a33_gpadc_data = { > .temp_data_base = SUN4I_GPADC_TEMP_DATA, > }; > > +static int sun50i_gpadc_disable(struct sun4i_gpadc_iio *info); > +static int sun50i_gpadc_enable(struct sun4i_gpadc_iio *info); > + > +static const struct gpadc_data sun50i_h6_gpadc_data = { > + .temp_offset = -2809, > + .temp_scale = -67, > + .has_bus_clk = true, > + .has_bus_rst = true, > + .gpadc_enable = sun50i_gpadc_enable, > + .gpadc_disable = sun50i_gpadc_disable, > + .sensor_count = 2, > + .temp_data_base = SUN50I_H6_GPADC_TEMP_DATA, > +}; > + > struct sun4i_sensor_tzd { > struct sun4i_gpadc_iio *info; > struct thermal_zone_device *tzd; > @@ -452,6 +466,53 @@ static int sun4i_gpadc_enable(struct sun4i_gpadc_iio *info) > return 0; > } > > +static int sun50i_gpadc_enable(struct sun4i_gpadc_iio *info) > +{ > + int ret, val; > + > + ret = reset_control_deassert(info->reset); > + if (ret) > + return ret; > + > + ret = clk_prepare_enable(info->bus_clk); > + if (ret) > + goto assert_reset; > + > + /* > + * clkin = 24MHz > + * T acquire = clkin / (SUN50I_GPADC_CTRL0_T_ACQ + 1) > + * = 20us > + */ > + regmap_write(info->regmap, SUN4I_GPADC_CTRL0, > + SUN50I_GPADC_CTRL0_T_ACQ(479)); > + /* average over 4 samples */ > + regmap_write(info->regmap, SUN50I_H6_GPADC_CTRL3, > + SUN4I_GPADC_CTRL3_FILTER_EN | > + SUN4I_GPADC_CTRL3_FILTER_TYPE(1)); > + /* period = (SUN50I_GPADC_TPR_TEMP_PERIOD + 1) * 4096 / clkin; ~10ms */ > + regmap_write(info->regmap, SUN50I_GPADC_TPR, > + SUN50I_GPADC_TPR_TEMP_PERIOD(58)); > + /* TODO: calibrate ths */ > + /* enable sensor */ > + val = GENMASK(info->data->sensor_count - 1, 0); > + regmap_write(info->regmap, SUN4I_GPADC_CTRL1, val); > + > + return 0; > + > +assert_reset: > + reset_control_assert(info->reset); > + > + return ret; > +} > + > +static int sun50i_gpadc_disable(struct sun4i_gpadc_iio *info) > +{ > + clk_disable_unprepare(info->bus_clk); > + reset_control_assert(info->reset); > + > + return 0; > +} > + > static int sun4i_gpadc_runtime_suspend(struct device *dev) > { > struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev)); > @@ -546,6 +607,10 @@ static const struct of_device_id sun4i_gpadc_of_id[] = { > .compatible = "allwinner,sun8i-a33-ths", > .data = &sun8i_a33_gpadc_data, > }, > + { > + .compatible = "allwinner,sun50i-h6-ths", > + .data = &sun50i_h6_gpadc_data, > + }, > { /* sentinel */ } > }; > > diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h > index 139872c2e0fe..f505013e9c0d 100644 > --- a/include/linux/mfd/sun4i-gpadc.h > +++ b/include/linux/mfd/sun4i-gpadc.h > @@ -19,6 +19,9 @@ > #define SUN4I_GPADC_CTRL0_FS_DIV(x) ((GENMASK(3, 0) & (x)) << 16) > #define SUN4I_GPADC_CTRL0_T_ACQ(x) (GENMASK(15, 0) & (x)) > > +/* TP_CTRL0 bits for sun50i SOCs */ > +#define SUN50I_GPADC_CTRL0_T_ACQ(x) ((GENMASK(15, 0) & (x)) << 16) > + > #define SUN4I_GPADC_CTRL1 0x04 > > #define SUN4I_GPADC_CTRL1_STYLUS_UP_DEBOUNCE(x) ((GENMASK(7, 0) & (x)) << 12) > @@ -49,6 +52,9 @@ > #define SUN4I_GPADC_CTRL2_PRE_MEA_EN BIT(24) > #define SUN4I_GPADC_CTRL2_PRE_MEA_THRE_CNT(x) (GENMASK(23, 0) & (x)) > > +#define SUN50I_GPADC_TPR 0x08 > +#define SUN50I_GPADC_TPR_TEMP_PERIOD(x) ((GENMASK(19, 0) & (x)) << 12) > + > #define SUN4I_GPADC_CTRL3 0x0c > > #define SUN4I_GPADC_CTRL3_FILTER_EN BIT(2) > @@ -84,6 +90,9 @@ > #define SUN4I_GPADC_TEMP_DATA 0x20 > #define SUN4I_GPADC_DATA 0x24 > > +#define SUN50I_H6_GPADC_CTRL3 0x30 > +#define SUN50I_H6_GPADC_TEMP_DATA 0xc0 > + > #define SUN4I_GPADC_IRQ_FIFO_DATA 0 > #define SUN4I_GPADC_IRQ_TEMP_DATA 1 >
Hi, On Sun, May 05, 2019 at 04:22:15PM +0100, Jonathan Cameron wrote: > On Fri, 3 May 2019 03:28:07 -0400 > Yangtao Li <tiny.windzz@gmail.com> wrote: > > > For some SOCs, there are more than one thermal sensor, and there are > > currently four sensors on the A80. So we need to do some work in order > > to support multiple thermal sensors: > > > > 1) add sensor_count in gpadc_data. > > 2) introduce sun4i_sensor_tzd in sun4i_gpadc_iio, to support multiple > > thermal_zone_device and distinguish between different sensors. > > 3) modify read temperature and initialization function. > > This comment doesn't mention the devm change. If it had it would have > raised immediate alarm bells. > > I'm also not keen on the web of pointers that this driver is steadily > evolving. I can't immediately see how to reduce that complexity however. So I might be responsible for that, and looking back, this has been a mistake. This driver was initally put together to support a controller found in older (A10 up to A31) Allwinner SoCs. This controller had an ADC driver that could be operated as a touchscreen controller, and was providing a CPU temperature sensor and a general purpose ADC. However, we already had a driver for that controller in drivers/input to report the CPU temperature, and the one in IIO was introduced to support the general purpose ADC (and the CPU temperature). The long term goal was to add the touchscreen feature as well eventually so that we could remove the one in drivers/input. That didn't happen. At the same time, the Allwinner hardware slowly evolved to remove the touchscreen and ADC features, and only keep the CPU temperature readout. It then evolved further on to support multiple temperatures (for different clusters, the GPU, and so on). So, today, we're in a situation where I was pushing everything into that IIO drivers since there was similiraties between all the generations, but the fact that we have to support so many odd cases (DT bindings compatibility, controllers with and without ADC, etc) that it becomes a real mess. And that mess isn't really used by anybody, since we want to have the touchscreen. There's only one SoC that is supported only by that driver, which is the A33 that only had a CPU temperature readout, and is still pretty similar to the latest SoC from Allwinner (that is supported by this series). I guess, for everyone's sanity and in order to not stall this further, it would just be better to create an hwmon driver for the A33 (and onwards, including the H6) for the SoC that just have the temperature readout feature. And for the older SoC, we just keep the older driver under input/. Once the A33 is supported, we'll remove the driver in IIO (and the related bits in drivers/mfd). Armbian already has a driver for that they never upstreamed iirc, so it might be a good starting point, and we would add the support for the H6. How does that sound? Sorry for wasting everybody's time on this. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
在 2019-05-06一的 14:28 +0200，Maxime Ripard写道： > Hi, > > On Sun, May 05, 2019 at 04:22:15PM +0100, Jonathan Cameron wrote: > > On Fri, 3 May 2019 03:28:07 -0400 > > Yangtao Li <tiny.windzz@gmail.com> wrote: > > > > > For some SOCs, there are more than one thermal sensor, and there > > > are > > > currently four sensors on the A80. So we need to do some work in > > > order > > > to support multiple thermal sensors: > > > > > > 1) add sensor_count in gpadc_data. > > > 2) introduce sun4i_sensor_tzd in sun4i_gpadc_iio, to support > > > multiple > > > thermal_zone_device and distinguish between different > > > sensors. > > > 3) modify read temperature and initialization function. > > > > This comment doesn't mention the devm change. If it had it would > > have > > raised immediate alarm bells. > > > > I'm also not keen on the web of pointers that this driver is > > steadily > > evolving. I can't immediately see how to reduce that complexity > > however. > > So I might be responsible for that, and looking back, this has been a > mistake. > > This driver was initally put together to support a controller found > in > older (A10 up to A31) Allwinner SoCs. This controller had an ADC > driver that could be operated as a touchscreen controller, and was > providing a CPU temperature sensor and a general purpose ADC. > > However, we already had a driver for that controller in drivers/input > to report the CPU temperature, and the one in IIO was introduced to > support the general purpose ADC (and the CPU temperature). The long > term goal was to add the touchscreen feature as well eventually so > that we could remove the one in drivers/input. That didn't happen. > > At the same time, the Allwinner hardware slowly evolved to remove the > touchscreen and ADC features, and only keep the CPU temperature > readout. It then evolved further on to support multiple temperatures > (for different clusters, the GPU, and so on). > > So, today, we're in a situation where I was pushing everything into > that IIO drivers since there was similiraties between all the > generations, but the fact that we have to support so many odd cases > (DT bindings compatibility, controllers with and without ADC, etc) > that it becomes a real mess. > > And that mess isn't really used by anybody, since we want to have the > touchscreen. > > There's only one SoC that is supported only by that driver, which is > the A33 that only had a CPU temperature readout, and is still pretty > similar to the latest SoC from Allwinner (that is supported by this > series). > > I guess, for everyone's sanity and in order to not stall this > further, > it would just be better to create an hwmon driver for the A33 (and > onwards, including the H6) for the SoC that just have the temperature > readout feature. And for the older SoC, we just keep the older driver > under input/. Once the A33 is supported, we'll remove the driver in > IIO (and the related bits in drivers/mfd). I think a thermal driver is better. Other SoCs' thermal sensor drivers are all thermal drivers. > > Armbian already has a driver for that they never upstreamed iirc, so > it might be a good starting point, and we would add the support for > the H6. How does that sound? I think the developer abandoned to upstream it because of the previous problem ;-) Maybe it can be taken and add A33&H6 support. > > Sorry for wasting everybody's time on this. > > Maxime > > -- > Maxime Ripard, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, May 7, 2019 at 12:52 AM Icenowy Zheng <icenowy@aosc.io> wrote: > > 在 2019-05-06一的 14:28 +0200，Maxime Ripard写道： > > Hi, > > > > On Sun, May 05, 2019 at 04:22:15PM +0100, Jonathan Cameron wrote: > > > On Fri, 3 May 2019 03:28:07 -0400 > > > Yangtao Li <tiny.windzz@gmail.com> wrote: > > > > > > > For some SOCs, there are more than one thermal sensor, and there > > > > are > > > > currently four sensors on the A80. So we need to do some work in > > > > order > > > > to support multiple thermal sensors: > > > > > > > > 1) add sensor_count in gpadc_data. > > > > 2) introduce sun4i_sensor_tzd in sun4i_gpadc_iio, to support > > > > multiple > > > > thermal_zone_device and distinguish between different > > > > sensors. > > > > 3) modify read temperature and initialization function. > > > > > > This comment doesn't mention the devm change. If it had it would > > > have > > > raised immediate alarm bells. > > > > > > I'm also not keen on the web of pointers that this driver is > > > steadily > > > evolving. I can't immediately see how to reduce that complexity > > > however. > > > > So I might be responsible for that, and looking back, this has been a > > mistake. > > > > This driver was initally put together to support a controller found > > in > > older (A10 up to A31) Allwinner SoCs. This controller had an ADC > > driver that could be operated as a touchscreen controller, and was > > providing a CPU temperature sensor and a general purpose ADC. > > > > However, we already had a driver for that controller in drivers/input > > to report the CPU temperature, and the one in IIO was introduced to > > support the general purpose ADC (and the CPU temperature). The long > > term goal was to add the touchscreen feature as well eventually so > > that we could remove the one in drivers/input. That didn't happen. > > > > At the same time, the Allwinner hardware slowly evolved to remove the > > touchscreen and ADC features, and only keep the CPU temperature > > readout. It then evolved further on to support multiple temperatures > > (for different clusters, the GPU, and so on). > > > > So, today, we're in a situation where I was pushing everything into > > that IIO drivers since there was similiraties between all the > > generations, but the fact that we have to support so many odd cases > > (DT bindings compatibility, controllers with and without ADC, etc) > > that it becomes a real mess. > > > > And that mess isn't really used by anybody, since we want to have the > > touchscreen. > > > > There's only one SoC that is supported only by that driver, which is > > the A33 that only had a CPU temperature readout, and is still pretty > > similar to the latest SoC from Allwinner (that is supported by this > > series). > > > > I guess, for everyone's sanity and in order to not stall this > > further, > > it would just be better to create an hwmon driver for the A33 (and > > onwards, including the H6) for the SoC that just have the temperature > > readout feature. And for the older SoC, we just keep the older driver > > under input/. Once the A33 is supported, we'll remove the driver in > > IIO (and the related bits in drivers/mfd). a hwmon driver or a thermal driver？ > > I think a thermal driver is better. This is what I hope to see a few months ago. > > Other SoCs' thermal sensor drivers are all thermal drivers. > > > > > Armbian already has a driver for that they never upstreamed iirc, so > > it might be a good starting point, and we would add the support for > > the H6. How does that sound? > > I think the developer abandoned to upstream it because of the previous > problem ;-) > > Maybe it can be taken and add A33&H6 support. If OK, I am going to start some thermal driver work this weekend. : ) Cheers, Yangtao > > > > > Sorry for wasting everybody's time on this. > > > > Maxime > > > > -- > > Maxime Ripard, Bootlin > > Embedded Linux and Kernel engineering > > https://bootlin.com > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
Hi, On Tue, May 07, 2019 at 01:08:39AM +0800, Frank Lee wrote: > On Tue, May 7, 2019 at 12:52 AM Icenowy Zheng <icenowy@aosc.io> wrote: > > > > 在 2019-05-06一的 14:28 +0200，Maxime Ripard写道： > > > Hi, > > > > > > On Sun, May 05, 2019 at 04:22:15PM +0100, Jonathan Cameron wrote: > > > > On Fri, 3 May 2019 03:28:07 -0400 > > > > Yangtao Li <tiny.windzz@gmail.com> wrote: > > > > > > > > > For some SOCs, there are more than one thermal sensor, and there > > > > > are > > > > > currently four sensors on the A80. So we need to do some work in > > > > > order > > > > > to support multiple thermal sensors: > > > > > > > > > > 1) add sensor_count in gpadc_data. > > > > > 2) introduce sun4i_sensor_tzd in sun4i_gpadc_iio, to support > > > > > multiple > > > > > thermal_zone_device and distinguish between different > > > > > sensors. > > > > > 3) modify read temperature and initialization function. > > > > > > > > This comment doesn't mention the devm change. If it had it would > > > > have > > > > raised immediate alarm bells. > > > > > > > > I'm also not keen on the web of pointers that this driver is > > > > steadily > > > > evolving. I can't immediately see how to reduce that complexity > > > > however. > > > > > > So I might be responsible for that, and looking back, this has been a > > > mistake. > > > > > > This driver was initally put together to support a controller found > > > in > > > older (A10 up to A31) Allwinner SoCs. This controller had an ADC > > > driver that could be operated as a touchscreen controller, and was > > > providing a CPU temperature sensor and a general purpose ADC. > > > > > > However, we already had a driver for that controller in drivers/input > > > to report the CPU temperature, and the one in IIO was introduced to > > > support the general purpose ADC (and the CPU temperature). The long > > > term goal was to add the touchscreen feature as well eventually so > > > that we could remove the one in drivers/input. That didn't happen. > > > > > > At the same time, the Allwinner hardware slowly evolved to remove the > > > touchscreen and ADC features, and only keep the CPU temperature > > > readout. It then evolved further on to support multiple temperatures > > > (for different clusters, the GPU, and so on). > > > > > > So, today, we're in a situation where I was pushing everything into > > > that IIO drivers since there was similiraties between all the > > > generations, but the fact that we have to support so many odd cases > > > (DT bindings compatibility, controllers with and without ADC, etc) > > > that it becomes a real mess. > > > > > > And that mess isn't really used by anybody, since we want to have the > > > touchscreen. > > > > > > There's only one SoC that is supported only by that driver, which is > > > the A33 that only had a CPU temperature readout, and is still pretty > > > similar to the latest SoC from Allwinner (that is supported by this > > > series). > > > > > > I guess, for everyone's sanity and in order to not stall this > > > further, > > > it would just be better to create an hwmon driver for the A33 (and > > > onwards, including the H6) for the SoC that just have the temperature > > > readout feature. And for the older SoC, we just keep the older driver > > > under input/. Once the A33 is supported, we'll remove the driver in > > > IIO (and the related bits in drivers/mfd). > > a hwmon driver or a thermal driver？ > > > > > I think a thermal driver is better. > > This is what I hope to see a few months ago. > > > > > Other SoCs' thermal sensor drivers are all thermal drivers. > > > > > > > > Armbian already has a driver for that they never upstreamed iirc, so > > > it might be a good starting point, and we would add the support for > > > the H6. How does that sound? > > > > I think the developer abandoned to upstream it because of the previous > > problem ;-) > > > > Maybe it can be taken and add A33&H6 support. > > If OK, I am going to start some thermal driver work this weekend. : ) There are plenty of thermal drivers flying around, with varying levels of support for various SoCs: - H3/H5: https://megous.com/git/linux/commit/?h=ths-5.1&id=b8e20c5da7a00b3a3fa1b274fc8d5bea95872b0a - A83T: https://megous.com/git/linux/commit/?h=ths-5.1&id=796dff9a946fd475cc1e4bb948a723ea841c640c - H6: https://megous.com/git/linux/commit/?h=opi3-5.1&id=aeab762c19b4aa228a295258c9d6b2e1f143bf86 For H3/H5 Icenowy also tried to upstream some variant of my THS driver, with better SID/calibration data reading support. I'd suggest starting with the H6 driver above (as that implements the calibration data readout correctly), and make it so that it can support multiple SoCs. regards, o. > Cheers, > Yangtao > > > > > > > > > Sorry for wasting everybody's time on this. > > > > > > Maxime > > > > > > -- > > > Maxime Ripard, Bootlin > > > Embedded Linux and Kernel engineering > > > https://bootlin.com > > > _______________________________________________ > > > linux-arm-kernel mailing list > > > linux-arm-kernel@lists.infradead.org > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, May 06, 2019 at 07:55:25PM +0200, Ondřej Jirman wrote: > On Tue, May 07, 2019 at 01:08:39AM +0800, Frank Lee wrote: > > On Tue, May 7, 2019 at 12:52 AM Icenowy Zheng <icenowy@aosc.io> wrote: > > > > > > 在 2019-05-06一的 14:28 +0200，Maxime Ripard写道： > > > > Hi, > > > > > > > > On Sun, May 05, 2019 at 04:22:15PM +0100, Jonathan Cameron wrote: > > > > > On Fri, 3 May 2019 03:28:07 -0400 > > > > > Yangtao Li <tiny.windzz@gmail.com> wrote: > > > > > > > > > > > For some SOCs, there are more than one thermal sensor, and there > > > > > > are > > > > > > currently four sensors on the A80. So we need to do some work in > > > > > > order > > > > > > to support multiple thermal sensors: > > > > > > > > > > > > 1) add sensor_count in gpadc_data. > > > > > > 2) introduce sun4i_sensor_tzd in sun4i_gpadc_iio, to support > > > > > > multiple > > > > > > thermal_zone_device and distinguish between different > > > > > > sensors. > > > > > > 3) modify read temperature and initialization function. > > > > > > > > > > This comment doesn't mention the devm change. If it had it would > > > > > have > > > > > raised immediate alarm bells. > > > > > > > > > > I'm also not keen on the web of pointers that this driver is > > > > > steadily > > > > > evolving. I can't immediately see how to reduce that complexity > > > > > however. > > > > > > > > So I might be responsible for that, and looking back, this has been a > > > > mistake. > > > > > > > > This driver was initally put together to support a controller found > > > > in > > > > older (A10 up to A31) Allwinner SoCs. This controller had an ADC > > > > driver that could be operated as a touchscreen controller, and was > > > > providing a CPU temperature sensor and a general purpose ADC. > > > > > > > > However, we already had a driver for that controller in drivers/input > > > > to report the CPU temperature, and the one in IIO was introduced to > > > > support the general purpose ADC (and the CPU temperature). The long > > > > term goal was to add the touchscreen feature as well eventually so > > > > that we could remove the one in drivers/input. That didn't happen. > > > > > > > > At the same time, the Allwinner hardware slowly evolved to remove the > > > > touchscreen and ADC features, and only keep the CPU temperature > > > > readout. It then evolved further on to support multiple temperatures > > > > (for different clusters, the GPU, and so on). > > > > > > > > So, today, we're in a situation where I was pushing everything into > > > > that IIO drivers since there was similiraties between all the > > > > generations, but the fact that we have to support so many odd cases > > > > (DT bindings compatibility, controllers with and without ADC, etc) > > > > that it becomes a real mess. > > > > > > > > And that mess isn't really used by anybody, since we want to have the > > > > touchscreen. > > > > > > > > There's only one SoC that is supported only by that driver, which is > > > > the A33 that only had a CPU temperature readout, and is still pretty > > > > similar to the latest SoC from Allwinner (that is supported by this > > > > series). > > > > > > > > I guess, for everyone's sanity and in order to not stall this > > > > further, > > > > it would just be better to create an hwmon driver for the A33 (and > > > > onwards, including the H6) for the SoC that just have the temperature > > > > readout feature. And for the older SoC, we just keep the older driver > > > > under input/. Once the A33 is supported, we'll remove the driver in > > > > IIO (and the related bits in drivers/mfd). > > > > a hwmon driver or a thermal driver？ > > > > > > > > I think a thermal driver is better. > > > > This is what I hope to see a few months ago. > > > > > > > > Other SoCs' thermal sensor drivers are all thermal drivers. > > > > > > > > > > > Armbian already has a driver for that they never upstreamed iirc, so > > > > it might be a good starting point, and we would add the support for > > > > the H6. How does that sound? > > > > > > I think the developer abandoned to upstream it because of the previous > > > problem ;-) > > > > > > Maybe it can be taken and add A33&H6 support. > > > > If OK, I am going to start some thermal driver work this weekend. : ) > > There are plenty of thermal drivers flying around, with varying levels > of support for various SoCs: > > - H3/H5: https://megous.com/git/linux/commit/?h=ths-5.1&id=b8e20c5da7a00b3a3fa1b274fc8d5bea95872b0a > - A83T: https://megous.com/git/linux/commit/?h=ths-5.1&id=796dff9a946fd475cc1e4bb948a723ea841c640c > - H6: https://megous.com/git/linux/commit/?h=opi3-5.1&id=aeab762c19b4aa228a295258c9d6b2e1f143bf86 > > For H3/H5 Icenowy also tried to upstream some variant of my THS driver, with > better SID/calibration data reading support. > > I'd suggest starting with the H6 driver above (as that implements the > calibration data readout correctly), and make it so that it can support multiple > SoCs. Yeah, that seems like a good plan Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Fri, 03 May 2019, Yangtao Li wrote: > This patch adds support for the H6 ths sensor. > > TODO: calibrate thermal sensor by using information from sid. > > Signed-off-by: Yangtao Li <tiny.windzz@gmail.com> > --- > drivers/iio/adc/sun4i-gpadc-iio.c | 65 +++++++++++++++++++++++++++++++ > include/linux/mfd/sun4i-gpadc.h | 9 +++++ For my own reference: Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
This patchset adds support for the H6 ths sensor. Based on IIO-based thermal sensor driver for Allwinner H3 and A83T SoC, thx to Philipp Rossak's work. TODO: calibrate thermal sensor by using information from sid. Yangtao Li (7): iio: adc: sun4i-gpadc: rework for support multiple thermal sensor iio: adc: sun4i-gpadc: introduce temp_data in gpadc_data iio: adc: sun4i-gpadc: introduce gpadc_enable and gpadc_disable in gpadc_data iio: adc: sun4i-gpadc-iio: support clocks and reset dt-bindings: mfd: Add H6 GPADC binding iio: adc: sun4i-gpadc-iio: add support for H6 thermal sensor iio: adc: sun4i-gpadc-iio convert to SPDX license tags .../devicetree/bindings/mfd/sun4i-gpadc.txt | 27 ++- drivers/iio/adc/sun4i-gpadc-iio.c | 208 +++++++++++++++--- include/linux/mfd/sun4i-gpadc.h | 9 + 3 files changed, 213 insertions(+), 31 deletions(-)