On Wed, 29 Mar 2017, Icenowy Zheng wrote:

2017?3?29? 14:54? Quentin Schulz [off-list ref]???

quoted

Hi, 

On 28/03/2017 19:30, Icenowy Zheng wrote: 

quoted

This adds support for the Allwinner H3 thermal sensor. 

Allwinner H3 has a thermal sensor like the one in A33, but have its 
registers nearly all re-arranged, sample clock moved to CCU and a pair 
of bus clock and reset added. It's also the base of newer SoCs' thermal 
sensors. 

An option is added to gpadc_data struct, to indicate whether this device 
is a new-generation Allwinner thermal sensor. 

The thermal sensors on A64 and H5 is like the one on H3, but with of 
course different formula factors. 

Signed-off-by: Icenowy Zheng <icenowy@aosc.io> 
--- 
? drivers/iio/adc/sun4i-gpadc-iio.c | 130 ++++++++++++++++++++++++++++++++------ 
? include/linux/mfd/sun4i-gpadc.h?? |? 33 +++++++++- 
? 2 files changed, 141 insertions(+), 22 deletions(-) 

How have you managed to reply un-threaded?

Please ensure you mailer attaches your reply to the rest of the
thread, or things will become very confusing, very quickly.

quoted

quoted

diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c 
index 74705aa37982..7512b1cae877 100644 
--- a/drivers/iio/adc/sun4i-gpadc-iio.c 
+++ b/drivers/iio/adc/sun4i-gpadc-iio.c 

@@ -22,6 +22,7 @@ 

?? * shutdown for not being used. 
?? */ 
? 
+#include <linux/clk.h> 
? #include <linux/completion.h> 
? #include <linux/interrupt.h> 
? #include <linux/io.h> 

@@ -31,6 +32,7 @@ 

? #include <linux/platform_device.h> 
? #include <linux/pm_runtime.h> 
? #include <linux/regmap.h> 
+#include <linux/reset.h> 
? #include <linux/thermal.h> 
? #include <linux/delay.h> 
? 

@@ -56,6 +58,7 @@ struct gpadc_data { 

? unsigned int tp_adc_select; 
? unsigned int (*adc_chan_select)(unsigned int chan); 
? unsigned int adc_chan_mask; 
+ bool gen2_ths; 
? }; 
? 

Instead of a boolean, give the TEMP_DATA register address. 

quoted

? static const struct gpadc_data sun4i_gpadc_data = { 

@@ -88,7 +91,20 @@ static const struct gpadc_data sun6i_gpadc_data = { 

? static const struct gpadc_data sun8i_a33_gpadc_data = { 
? .temp_offset = -1662, 
? .temp_scale = 162, 
- .tp_mode_en = SUN8I_GPADC_CTRL1_CHOP_TEMP_EN, 
+ .tp_mode_en = SUN8I_A23_GPADC_CTRL1_CHOP_TEMP_EN, 
+}; 

Separate patch for this? 

quoted

+ 
+static const struct gpadc_data sun8i_h3_gpadc_data = { 
+ /* 
+ * The original formula on the datasheet seems to be wrong. 
+ * These factors are calculated based on the formula in the BSP 
+ * kernel, which is originally Tem = 217 - (T / 8.253), in which Tem 
+ * is the temperature in Celsius degree and T is the raw value 
+ * from the sensor. 
+ */ 
+ .temp_offset = -1791, 
+ .temp_scale = -121, 
+ .gen2_ths = true, 
? }; 
? 
? struct sun4i_gpadc_iio { 

@@ -103,6 +119,9 @@ struct sun4i_gpadc_iio { 

? atomic_t ignore_temp_data_irq; 
? const struct gpadc_data *data; 
? bool no_irq; 
+ struct clk *ths_bus_clk; 
+ struct clk *ths_clk; 
+ struct reset_control *reset; 
? /* prevents concurrent reads of temperature and ADC */ 
? struct mutex mutex; 
? }; 

@@ -274,7 +293,11 @@ static int sun4i_gpadc_temp_read(struct iio_dev *indio_dev, int *val) 

? if (info->no_irq) { 
? pm_runtime_get_sync(indio_dev->dev.parent); 
? 
- regmap_read(info->regmap, SUN4I_GPADC_TEMP_DATA, val); 
+ if (info->data->gen2_ths) 
+ regmap_read(info->regmap, SUN8I_H3_GPADC_TEMP_DATA, 
+ ??? val); 
+ else 
+ regmap_read(info->regmap, SUN4I_GPADC_TEMP_DATA, val); 
? 

Instead of gen2_ths, use the TEMP_DATA register address. 

quoted

? pm_runtime_mark_last_busy(indio_dev->dev.parent); 
? pm_runtime_put_autosuspend(indio_dev->dev.parent); 

@@ -386,10 +409,15 @@ static int sun4i_gpadc_runtime_suspend(struct device *dev) 

? { 
? struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev)); 
? 
- /* Disable the ADC on IP */ 
- regmap_write(info->regmap, SUN4I_GPADC_CTRL1, 0); 
- /* Disable temperature sensor on IP */ 
- regmap_write(info->regmap, SUN4I_GPADC_TPR, 0); 
+ if (info->data->gen2_ths) { 
+ /* Disable temperature sensor */ 
+ regmap_write(info->regmap, SUN8I_H3_GPADC_CTRL2, 0); 
+ } else { 
+ /* Disable the ADC on IP */ 
+ regmap_write(info->regmap, SUN4I_GPADC_CTRL1, 0); 
+ /* Disable temperature sensor on IP */ 
+ regmap_write(info->regmap, SUN4I_GPADC_TPR, 0); 
+ } 
? 

Either use another register address or add a suspend function to struct 
gpadc_data which will be different for each version of the IP. 

quoted

? return 0; 
? } 

@@ -398,19 +426,36 @@ static int sun4i_gpadc_runtime_resume(struct device *dev) 

? { 
? struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev)); 
? 
- /* clkin = 6MHz */ 
- regmap_write(info->regmap, SUN4I_GPADC_CTRL0, 
- ???? SUN4I_GPADC_CTRL0_ADC_CLK_DIVIDER(2) | 
- ???? SUN4I_GPADC_CTRL0_FS_DIV(7) | 
- ???? SUN4I_GPADC_CTRL0_T_ACQ(63)); 
- regmap_write(info->regmap, SUN4I_GPADC_CTRL1, info->data->tp_mode_en); 
- regmap_write(info->regmap, SUN4I_GPADC_CTRL3, 
- ???? SUN4I_GPADC_CTRL3_FILTER_EN | 
- ???? SUN4I_GPADC_CTRL3_FILTER_TYPE(1)); 
- /* period = SUN4I_GPADC_TPR_TEMP_PERIOD * 256 * 16 / clkin; ~0.6s */ 
- regmap_write(info->regmap, SUN4I_GPADC_TPR, 
- ???? SUN4I_GPADC_TPR_TEMP_ENABLE | 
- ???? SUN4I_GPADC_TPR_TEMP_PERIOD(800)); 
+ if (info->data->gen2_ths) { 
+ regmap_write(info->regmap, SUN8I_H3_GPADC_CTRL2, 
+ ???? SUN8I_H3_GPADC_CTRL2_TEMP_SENSE_EN | 
+ ???? SUN8I_H3_GPADC_CTRL2_T_ACQ1(31)); 
+ regmap_write(info->regmap, SUN4I_GPADC_CTRL0, 
+ ???? SUN4I_GPADC_CTRL0_T_ACQ(31)); 
+ regmap_write(info->regmap, SUN8I_H3_GPADC_CTRL3, 
+ ???? SUN4I_GPADC_CTRL3_FILTER_EN | 
+ ???? SUN4I_GPADC_CTRL3_FILTER_TYPE(1)); 
+ regmap_write(info->regmap, SUN8I_H3_GPADC_INTC, 
+ ???? SUN8I_H3_GPADC_INTC_TEMP_PERIOD(800)); 
+ } else { 
+ /* clkin = 6MHz */ 
+ regmap_write(info->regmap, SUN4I_GPADC_CTRL0, 
+ ???? SUN4I_GPADC_CTRL0_ADC_CLK_DIVIDER(2) | 
+ ???? SUN4I_GPADC_CTRL0_FS_DIV(7) | 
+ ???? SUN4I_GPADC_CTRL0_T_ACQ(63)); 
+ regmap_write(info->regmap, SUN4I_GPADC_CTRL1, 
+ ???? info->data->tp_mode_en); 
+ regmap_write(info->regmap, SUN4I_GPADC_CTRL3, 
+ ???? SUN4I_GPADC_CTRL3_FILTER_EN | 
+ ???? SUN4I_GPADC_CTRL3_FILTER_TYPE(1)); 
+ /* 
+ * period = SUN4I_GPADC_TPR_TEMP_PERIOD * 256 * 16 / clkin; 
+ * ~0.6s 
+ */ 
+ regmap_write(info->regmap, SUN4I_GPADC_TPR, 
+ ???? SUN4I_GPADC_TPR_TEMP_ENABLE | 
+ ???? SUN4I_GPADC_TPR_TEMP_PERIOD(800)); 
+ } 
? 

Same here as suspend function? 

quoted

? return 0; 
? } 

@@ -494,6 +539,10 @@ static const struct of_device_id sun4i_gpadc_of_id[] = { 

? .compatible = "allwinner,sun8i-a33-ths", 
? .data = &sun8i_a33_gpadc_data, 
? }, 
+ { 
+ .compatible = "allwinner,sun8i-h3-ths", 
+ .data = &sun8i_h3_gpadc_data, 
+ }, 
? { /* sentinel */ } 
? }; 
? 

@@ -529,6 +578,43 @@ static int sun4i_gpadc_probe_dt(struct platform_device *pdev, 

? return ret; 
? } 
? 
+ if (info->data->gen2_ths) { 
+ info->reset = devm_reset_control_get(&pdev->dev, NULL); 
+ if (IS_ERR(info->reset)) { 
+ ret = PTR_ERR(info->reset); 
+ return ret; 
+ } 
+ 
+ ret = reset_control_deassert(info->reset); 
+ if (ret) 
+ return ret; 
+ 
+ info->ths_bus_clk = devm_clk_get(&pdev->dev, "bus"); 
+ if (IS_ERR(info->ths_bus_clk)) { 
+ ret = PTR_ERR(info->ths_bus_clk); 
+ return ret; 
+ } 
+ 
+ ret = clk_prepare_enable(info->ths_bus_clk); 
+ if (ret) 
+ return ret; 
+ 
+ info->ths_clk = devm_clk_get(&pdev->dev, "ths"); 
+ if (IS_ERR(info->ths_clk)) { 
+ ret = PTR_ERR(info->ths_clk); 
+ return ret; 
+ } 
+ 
+ /* Running at 6MHz */ 
+ ret = clk_set_rate(info->ths_clk, 6000000); 
+ if (ret) 
+ return ret; 
+ 
+ ret = clk_prepare_enable(info->ths_clk); 
+ if (ret) 
+ return ret; 
+ } 
+ 
? if (!IS_ENABLED(CONFIG_THERMAL_OF)) 
? return 0; 
? 

@@ -691,6 +777,12 @@ static int sun4i_gpadc_remove(struct platform_device *pdev) 

? if (!info->no_irq && IS_ENABLED(CONFIG_THERMAL_OF)) 
? iio_map_array_unregister(indio_dev); 
? 
+ if (info->data->gen2_ths) { 
+ clk_disable_unprepare(info->ths_clk); 
+ clk_disable_unprepare(info->ths_bus_clk); 
+ reset_control_deassert(info->reset); 
+ } 
+ 

I'm not really fond of using this boolean as I don't see it being 
possibly reused for any other SoCs that has a GPADC or THS. 

Because you didn't care new SoCs :-)

All SoCs after H3 (A64, H5, R40) uses the same THS architecture with H3.

quoted

Here, you could make use of a list/array of clk which then can be reused 
for other SoCs just by changing the list. Add a default rate to the 
gpadc_data structure and you're go to go. 

quoted

? return 0; 
? } 
? 

diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h 
index 139872c2e0fe..f794a2988a93 100644 
--- a/include/linux/mfd/sun4i-gpadc.h 
+++ b/include/linux/mfd/sun4i-gpadc.h 

@@ -38,9 +38,12 @@ 

? #define SUN6I_GPADC_CTRL1_ADC_CHAN_SELECT(x) (GENMASK(3, 0) & BIT(x)) 
? #define SUN6I_GPADC_CTRL1_ADC_CHAN_MASK GENMASK(3, 0) 
? 
-/* TP_CTRL1 bits for sun8i SoCs */ 
-#define SUN8I_GPADC_CTRL1_CHOP_TEMP_EN BIT(8) 
-#define SUN8I_GPADC_CTRL1_GPADC_CALI_EN BIT(7) 
+/* TP_CTRL1 bits for sun8i A23/A33 SoCs */ 
+#define SUN8I_A23_GPADC_CTRL1_CHOP_TEMP_EN BIT(8) 
+#define SUN8I_A23_GPADC_CTRL1_GPADC_CALI_EN BIT(7) 
+ 

Different patch for these? 

Thanks, 
Quentin 

_______________________________________________ 
linux-arm-kernel mailing list 
linux-arm-kernel at lists.infradead.org 
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog