| Message ID | 20180128232919.12639-1-embed3d@gmail.com |
|---|---|
| Headers | show |
| Series | IIO-based thermal sensor driver for Allwinner H3 and A83T SoC | expand |
Hi, On Mon, Jan 29, 2018 at 12:29:05AM +0100, Philipp Rossak wrote: > This commit enables the SUN4I_GPADC config option and > sets the value to yes. This is needed to enable the ths sensors. > > Signed-off-by: Philipp Rossak <embed3d@gmail.com> > --- > arch/arm/configs/sunxi_defconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm/configs/sunxi_defconfig b/arch/arm/configs/sunxi_defconfig > index 5caaf971fb50..52c3990b9d91 100644 > --- a/arch/arm/configs/sunxi_defconfig > +++ b/arch/arm/configs/sunxi_defconfig > @@ -131,6 +131,7 @@ CONFIG_DMA_SUN6I=y > CONFIG_EXTCON=y > CONFIG_IIO=y > CONFIG_AXP20X_ADC=y > +CONFIG_SUN4I_GPADC=y Unfortunately, we can't do that since TOUCHSCREEN_SUN4I is already enabled and we have a depends on !TOUCHSCREEN_SUN4I. One more reason to stop worrying about replacing that driver DT binding. Maxime
Hi, On Mon, Jan 29, 2018 at 12:29:07AM +0100, Philipp Rossak wrote: > For adding newer sensor some basic rework of the code is necessary. > > This commit reworks the code and allows the sampling start/end code and > the position of value readout register to be altered. Later the start/end > functions will be used to configure the ths and start/stop the > sampling. > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > Signed-off-by: Philipp Rossak <embed3d@gmail.com> That signed-off-by chain doesn't really make much sense. If Icenowy is the author, she should be reported as such in the commit, and if you're the author, you shouldn't have her Signed-off-by. > --- > drivers/iio/adc/sun4i-gpadc-iio.c | 44 ++++++++++++++++++++++++++++++++++----- > 1 file changed, 39 insertions(+), 5 deletions(-) > > diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c > index 03804ff9c006..db57d9fffe48 100644 > --- a/drivers/iio/adc/sun4i-gpadc-iio.c > +++ b/drivers/iio/adc/sun4i-gpadc-iio.c > @@ -49,6 +49,15 @@ static unsigned int sun6i_gpadc_chan_select(unsigned int chan) > return SUN6I_GPADC_CTRL1_ADC_CHAN_SELECT(chan); > } > > +struct sun4i_gpadc_iio; > + > +/* > + * Prototypes for these functions, which enable these functions to be > + * referenced in gpadc_data structures. > + */ > +static int sun4i_gpadc_sample_start(struct sun4i_gpadc_iio *info); > +static int sun4i_gpadc_sample_end(struct sun4i_gpadc_iio *info); > + > struct gpadc_data { > int temp_offset; > int temp_scale; > @@ -56,6 +65,9 @@ struct gpadc_data { > unsigned int tp_adc_select; > unsigned int (*adc_chan_select)(unsigned int chan); > unsigned int adc_chan_mask; > + unsigned int temp_data; > + int (*sample_start)(struct sun4i_gpadc_iio *info); > + int (*sample_end)(struct sun4i_gpadc_iio *info); > }; > > static const struct gpadc_data sun4i_gpadc_data = { > @@ -65,6 +77,9 @@ 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, > + .temp_data = SUN4I_GPADC_TEMP_DATA, You can use a regmap_field there. Thanks! Maxime
On Mon, Jan 29, 2018 at 12:29:08AM +0100, Philipp Rossak wrote: > For adding newer sensor some basic rework of the code is necessary. > > The SoCs after H3 has newer thermal sensor ADCs, which have two clock > inputs (bus clock and sampling clock) and a reset. The registers are > also re-arranged. > > This commit reworks the code, adds the process of the clocks and > resets. > > Signed-off-by: Philipp Rossak <embed3d@gmail.com> > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> Same remark for the SoB > --- > drivers/iio/adc/sun4i-gpadc-iio.c | 71 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 71 insertions(+) > > diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c > index db57d9fffe48..51ec0104d678 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> > > @@ -68,6 +70,9 @@ struct gpadc_data { > unsigned int temp_data; > int (*sample_start)(struct sun4i_gpadc_iio *info); > int (*sample_end)(struct sun4i_gpadc_iio *info); > + bool has_bus_clk; > + bool has_bus_rst; > + bool has_mod_clk; Is there SoCs where this insn't all true, or all false? > }; > > static const struct gpadc_data sun4i_gpadc_data = { > @@ -127,6 +132,9 @@ struct sun4i_gpadc_iio { > atomic_t ignore_temp_data_irq; > const struct gpadc_data *data; > bool no_irq; > + struct clk *bus_clk; > + struct clk *mod_clk; > + struct reset_control *reset; > /* prevents concurrent reads of temperature and ADC */ > struct mutex mutex; > struct thermal_zone_device *tzd; > @@ -420,6 +428,10 @@ static int sun4i_gpadc_runtime_suspend(struct device *dev) > { > struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev)); > > + clk_disable(info->mod_clk); > + Why clk_disable? > + clk_disable(info->bus_clk); > + You can tie the bus_clk to the regmap directly, instead of having to maintain it yourself here. And you can probably put the device in reset and out of reset here as well. > return info->data->sample_end(info); > } > > @@ -446,6 +458,10 @@ static int sun4i_gpadc_runtime_resume(struct device *dev) > { > struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev)); > > + clk_enable(info->mod_clk); > + > + clk_enable(info->bus_clk); > + > return info->data->sample_start(info); > } > > @@ -560,10 +576,59 @@ static int sun4i_gpadc_probe_dt(struct platform_device *pdev, > return ret; > } > > + if (info->data->has_bus_rst) { > + 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; > + } > + > + if (info->data->has_bus_clk) { > + info->bus_clk = devm_clk_get(&pdev->dev, "bus"); > + if (IS_ERR(info->bus_clk)) { > + ret = PTR_ERR(info->bus_clk); > + goto assert_reset; > + } > + > + ret = clk_prepare_enable(info->bus_clk); > + if (ret) > + goto assert_reset; > + } > + > + if (info->data->has_mod_clk) { > + info->mod_clk = devm_clk_get(&pdev->dev, "mod"); > + if (IS_ERR(info->mod_clk)) { > + ret = PTR_ERR(info->mod_clk); > + goto disable_bus_clk; > + } > + > + /* Running at 6MHz */ > + ret = clk_set_rate(info->mod_clk, 4000000); Your comment and the line below doesn't really make much sense :) Maxime
Hi, On Mon, Jan 29, 2018 at 12:29:09AM +0100, Philipp Rossak wrote: > For adding newer sensor some basic rework of the code is necessary. > > This patch reworks the driver to be able to handle more than one > thermal sensor. Newer SoC like the A80 have 4 thermal sensors. > Because of this the maximal sensor count value was set to 4. > > The sensor_id value is set during sensor registration and is for each > registered sensor indiviual. This makes it able to differntiate the > sensors when the value is read from the register. > > In function sun4i_gpadc_read_raw(), the sensor number of the ths sensor > was directly set to 0 (sun4i_gpadc_temp_read(x,x,0)). This selects > in the temp_read function automatically sensor 0. A check for the > sensor_id is here not required since the old sensors only have one > thermal sensor. In addition to that is the sun4i_gpadc_read_raw() > function only used by the "older" sensors (before A33) where the > thermal sensor was a cobination of an adc and a thermal sensor. > > Signed-off-by: Philipp Rossak <embed3d@gmail.com> > --- > drivers/iio/adc/sun4i-gpadc-iio.c | 36 +++++++++++++++++++++++------------- > include/linux/mfd/sun4i-gpadc.h | 3 +++ > 2 files changed, 26 insertions(+), 13 deletions(-) > > diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c > index 51ec0104d678..ac9ad2f8232f 100644 > --- a/drivers/iio/adc/sun4i-gpadc-iio.c > +++ b/drivers/iio/adc/sun4i-gpadc-iio.c > @@ -67,12 +67,13 @@ struct gpadc_data { > unsigned int tp_adc_select; > unsigned int (*adc_chan_select)(unsigned int chan); > unsigned int adc_chan_mask; > - unsigned int temp_data; > + unsigned int temp_data[MAX_SENSOR_COUNT]; > int (*sample_start)(struct sun4i_gpadc_iio *info); > int (*sample_end)(struct sun4i_gpadc_iio *info); > bool has_bus_clk; > bool has_bus_rst; > bool has_mod_clk; > + int sensor_count; > }; > > static const struct gpadc_data sun4i_gpadc_data = { > @@ -82,9 +83,10 @@ 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, > - .temp_data = SUN4I_GPADC_TEMP_DATA, > + .temp_data = {SUN4I_GPADC_TEMP_DATA, 0, 0, 0}, > .sample_start = sun4i_gpadc_sample_start, > .sample_end = sun4i_gpadc_sample_end, > + .sensor_count = 1, > }; > > static const struct gpadc_data sun5i_gpadc_data = { > @@ -94,9 +96,10 @@ 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, > - .temp_data = SUN4I_GPADC_TEMP_DATA, > + .temp_data = {SUN4I_GPADC_TEMP_DATA, 0, 0, 0}, > .sample_start = sun4i_gpadc_sample_start, > .sample_end = sun4i_gpadc_sample_end, > + .sensor_count = 1, > }; > > static const struct gpadc_data sun6i_gpadc_data = { > @@ -106,18 +109,20 @@ 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, > - .temp_data = SUN4I_GPADC_TEMP_DATA, > + .temp_data = {SUN4I_GPADC_TEMP_DATA, 0, 0, 0}, > .sample_start = sun4i_gpadc_sample_start, > .sample_end = sun4i_gpadc_sample_end, > + .sensor_count = 1, > }; > > static const struct gpadc_data sun8i_a33_gpadc_data = { > .temp_offset = -1662, > .temp_scale = 162, > .tp_mode_en = SUN8I_A33_GPADC_CTRL1_CHOP_TEMP_EN, > - .temp_data = SUN4I_GPADC_TEMP_DATA, > + .temp_data = {SUN4I_GPADC_TEMP_DATA, 0, 0, 0}, > .sample_start = sun4i_gpadc_sample_start, > .sample_end = sun4i_gpadc_sample_end, > + .sensor_count = 1, > }; > > struct sun4i_gpadc_iio { > @@ -135,6 +140,7 @@ struct sun4i_gpadc_iio { > struct clk *bus_clk; > struct clk *mod_clk; > struct reset_control *reset; > + int sensor_id; > /* prevents concurrent reads of temperature and ADC */ > struct mutex mutex; > struct thermal_zone_device *tzd; > @@ -302,14 +308,15 @@ 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, > + int sensor) > { > struct sun4i_gpadc_iio *info = iio_priv(indio_dev); > > if (info->no_irq) { > pm_runtime_get_sync(indio_dev->dev.parent); > > - regmap_read(info->regmap, info->data->temp_data, val); > + regmap_read(info->regmap, info->data->temp_data[sensor], val); > > pm_runtime_mark_last_busy(indio_dev->dev.parent); > pm_runtime_put_autosuspend(indio_dev->dev.parent); > @@ -356,7 +363,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; > @@ -470,7 +477,7 @@ static int sun4i_gpadc_get_temp(void *data, int *temp) > struct sun4i_gpadc_iio *info = data; > int val, scale, offset; > > - if (sun4i_gpadc_temp_read(info->indio_dev, &val)) > + if (sun4i_gpadc_temp_read(info->indio_dev, &val, info->sensor_id)) > return -ETIMEDOUT; > > sun4i_gpadc_temp_scale(info->indio_dev, &scale); > @@ -712,7 +719,7 @@ static int sun4i_gpadc_probe(struct platform_device *pdev) > { > struct sun4i_gpadc_iio *info; > struct iio_dev *indio_dev; > - int ret; > + int ret, i; > > indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info)); > if (!indio_dev) > @@ -745,9 +752,12 @@ 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); > + for (i = 0; i < info->data->sensor_count; i++) { > + info->sensor_id = i; > + info->tzd = thermal_zone_of_sensor_register( > + info->sensor_device, > + i, info, &sun4i_ts_tz_ops); > + } I'm not sure how that works. Isn't the info structure shared between all the sensors? The sensor_id value would be always set to the last sensor then. Thanks! Maxime
On Mon, Jan 29, 2018 at 12:29:10AM +0100, Philipp Rossak wrote: > This patch reworks the driver to support nvmem calibration cells. > The driver checks if the nvmem calibration is supported and reads out > the nvmem. > > Signed-off-by: Philipp Rossak <embed3d@gmail.com> > --- > drivers/iio/adc/sun4i-gpadc-iio.c | 44 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c > index ac9ad2f8232f..74eeb5cd5218 100644 > --- a/drivers/iio/adc/sun4i-gpadc-iio.c > +++ b/drivers/iio/adc/sun4i-gpadc-iio.c > @@ -27,6 +27,7 @@ > #include <linux/interrupt.h> > #include <linux/io.h> > #include <linux/module.h> > +#include <linux/nvmem-consumer.h> > #include <linux/of.h> > #include <linux/of_device.h> > #include <linux/platform_device.h> > @@ -74,6 +75,7 @@ struct gpadc_data { > bool has_bus_rst; > bool has_mod_clk; > int sensor_count; > + bool supports_nvmem; I think you should add some documentation along with all the fields you're adding. > }; > > static const struct gpadc_data sun4i_gpadc_data = { > @@ -87,6 +89,7 @@ static const struct gpadc_data sun4i_gpadc_data = { > .sample_start = sun4i_gpadc_sample_start, > .sample_end = sun4i_gpadc_sample_end, > .sensor_count = 1, > + .supports_nvmem = false, That's already its value if you leave it out. > }; > > static const struct gpadc_data sun5i_gpadc_data = { > @@ -100,6 +103,7 @@ static const struct gpadc_data sun5i_gpadc_data = { > .sample_start = sun4i_gpadc_sample_start, > .sample_end = sun4i_gpadc_sample_end, > .sensor_count = 1, > + .supports_nvmem = false, > }; > > static const struct gpadc_data sun6i_gpadc_data = { > @@ -113,6 +117,7 @@ static const struct gpadc_data sun6i_gpadc_data = { > .sample_start = sun4i_gpadc_sample_start, > .sample_end = sun4i_gpadc_sample_end, > .sensor_count = 1, > + .supports_nvmem = false, > }; > > static const struct gpadc_data sun8i_a33_gpadc_data = { > @@ -123,6 +128,7 @@ static const struct gpadc_data sun8i_a33_gpadc_data = { > .sample_start = sun4i_gpadc_sample_start, > .sample_end = sun4i_gpadc_sample_end, > .sensor_count = 1, > + .supports_nvmem = false, > }; > > struct sun4i_gpadc_iio { > @@ -141,6 +147,8 @@ struct sun4i_gpadc_iio { > struct clk *mod_clk; > struct reset_control *reset; > int sensor_id; > + u32 calibration_data[2]; > + bool has_calibration_data[2]; Why do you have two different values here? > /* prevents concurrent reads of temperature and ADC */ > struct mutex mutex; > struct thermal_zone_device *tzd; > @@ -561,6 +569,9 @@ static int sun4i_gpadc_probe_dt(struct platform_device *pdev, > struct resource *mem; > void __iomem *base; > int ret; > + struct nvmem_cell *cell; > + ssize_t cell_size; > + u64 *cell_data; > > info->data = of_device_get_match_data(&pdev->dev); > if (!info->data) > @@ -575,6 +586,39 @@ static int sun4i_gpadc_probe_dt(struct platform_device *pdev, > if (IS_ERR(base)) > return PTR_ERR(base); > > + info->has_calibration_data[0] = false; > + info->has_calibration_data[1] = false; > + > + if (!info->data->supports_nvmem) > + goto no_nvmem; > + > + cell = nvmem_cell_get(&pdev->dev, "calibration"); > + if (IS_ERR(cell)) { > + if (PTR_ERR(cell) == -EPROBE_DEFER) > + return PTR_ERR(cell); > + goto no_nvmem; goto considered evil ? :) > + } > + > + cell_data = (u64 *)nvmem_cell_read(cell, &cell_size); > + nvmem_cell_put(cell); > + switch (cell_size) { > + case 8: > + case 6: > + info->has_calibration_data[1] = true; > + info->calibration_data[1] = be32_to_cpu( > + upper_32_bits(cell_data[0])); > + case 4: > + case 2: > + info->has_calibration_data[0] = true; > + info->calibration_data[0] = be32_to_cpu( > + lower_32_bits(cell_data[0])); Why do you need that switch? Thanks! Maxime
On Mon, Jan 29, 2018 at 12:29:12AM +0100, Philipp Rossak wrote: > This patch adds support for the H3 ths sensor. > > The H3 supports interrupts. The interrupt is configured to update the > the sensor values every second. The calibration data is writen at the > begin of the init process. > > Signed-off-by: Philipp Rossak <embed3d@gmail.com> > --- > drivers/iio/adc/sun4i-gpadc-iio.c | 86 +++++++++++++++++++++++++++++++++++++++ > include/linux/mfd/sun4i-gpadc.h | 22 ++++++++++ > 2 files changed, 108 insertions(+) > > diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c > index b7b5451226b0..8196203d65fe 100644 > --- a/drivers/iio/adc/sun4i-gpadc-iio.c > +++ b/drivers/iio/adc/sun4i-gpadc-iio.c > @@ -61,6 +61,9 @@ struct sun4i_gpadc_iio; > static int sun4i_gpadc_sample_start(struct sun4i_gpadc_iio *info); > static int sun4i_gpadc_sample_end(struct sun4i_gpadc_iio *info); > > +static int sunxi_ths_sample_start(struct sun4i_gpadc_iio *info); > +static int sunxi_ths_sample_end(struct sun4i_gpadc_iio *info); > + > struct gpadc_data { > int temp_offset; > int temp_scale; > @@ -71,6 +74,10 @@ 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 ctrl0_map; > + u32 ctrl2_map; > + u32 sensor_en_map; > + u32 filter_map; You can use a regmap_field for that. > u32 irq_clear_map; > u32 irq_control_map; > bool has_bus_clk; > @@ -138,6 +145,31 @@ static const struct gpadc_data sun8i_a33_gpadc_data = { > .support_irq = false, > }; > > +static const struct gpadc_data sun8i_h3_ths_data = { > + .temp_offset = -1791, > + .temp_scale = -121, > + .temp_data = {SUN8I_H3_THS_TDATA0, 0, 0, 0}, > + .sample_start = sunxi_ths_sample_start, > + .sample_end = sunxi_ths_sample_end, > + .has_bus_clk = true, > + .has_bus_rst = true, > + .has_mod_clk = true, > + .sensor_count = 1, > + .supports_nvmem = true, > + .support_irq = true, > + .ctrl0_map = SUN4I_GPADC_CTRL0_T_ACQ(0xff), > + .ctrl2_map = SUN8I_H3_THS_ACQ1(0x3f), > + .sensor_en_map = SUN8I_H3_THS_TEMP_SENSE_EN0, > + .filter_map = SUN4I_GPADC_CTRL3_FILTER_EN | > + SUN4I_GPADC_CTRL3_FILTER_TYPE(0x2), > + .irq_clear_map = SUN8I_H3_THS_INTS_ALARM_INT_0 | > + SUN8I_H3_THS_INTS_SHUT_INT_0 | > + SUN8I_H3_THS_INTS_TDATA_IRQ_0 | > + SUN8I_H3_THS_INTS_ALARM_OFF_0, > + .irq_control_map = SUN8I_H3_THS_INTC_TDATA_IRQ_EN0 | > + SUN8I_H3_THS_TEMP_PERIOD(0x7), > +}; > + > struct sun4i_gpadc_iio { > struct iio_dev *indio_dev; > struct completion completion; > @@ -462,6 +494,16 @@ static int sun4i_gpadc_sample_end(struct sun4i_gpadc_iio *info) > return 0; > } > > +static int sunxi_ths_sample_end(struct sun4i_gpadc_iio *info) > +{ > + /* Disable ths interrupt */ > + regmap_write(info->regmap, SUN8I_H3_THS_INTC, 0x0); > + /* Disable temperature sensor */ > + regmap_write(info->regmap, SUN8I_H3_THS_CTRL2, 0x0); > + > + return 0; > +} > + > static int sun4i_gpadc_runtime_suspend(struct device *dev) > { > struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev)); > @@ -473,6 +515,17 @@ static int sun4i_gpadc_runtime_suspend(struct device *dev) > return info->data->sample_end(info); > } > > +static void sunxi_calibrate(struct sun4i_gpadc_iio *info) > +{ > + if (info->has_calibration_data[0]) > + regmap_write(info->regmap, SUNXI_THS_CDATA_0_1, > + info->calibration_data[0]); > + > + if (info->has_calibration_data[1]) > + regmap_write(info->regmap, SUNXI_THS_CDATA_2_3, > + info->calibration_data[1]); > +} > + > static int sun4i_gpadc_sample_start(struct sun4i_gpadc_iio *info) > { > /* clkin = 6MHz */ > @@ -492,6 +545,35 @@ static int sun4i_gpadc_sample_start(struct sun4i_gpadc_iio *info) > return 0; > } > > +static int sunxi_ths_sample_start(struct sun4i_gpadc_iio *info) Please remain consistent with the prefixes used in the driver Maxime
On Mon, Jan 29, 2018 at 12:29:13AM +0100, Philipp Rossak wrote: > This patch adds support for the A83T ths sensor. > > The A83T supports interrupts. The interrupt is configured to update the > the sensor values every second. > > Signed-off-by: Philipp Rossak <embed3d@gmail.com> > --- > drivers/iio/adc/sun4i-gpadc-iio.c | 38 ++++++++++++++++++++++++++++++++++++++ > include/linux/mfd/sun4i-gpadc.h | 18 ++++++++++++++++++ > 2 files changed, 56 insertions(+) > > diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c > index 8196203d65fe..9f7895ba1966 100644 > --- a/drivers/iio/adc/sun4i-gpadc-iio.c > +++ b/drivers/iio/adc/sun4i-gpadc-iio.c > @@ -170,6 +170,40 @@ static const struct gpadc_data sun8i_h3_ths_data = { > SUN8I_H3_THS_TEMP_PERIOD(0x7), > }; > > +static const struct gpadc_data sun8i_a83t_ths_data = { > + .temp_offset = -2724, > + .temp_scale = -70, > + .temp_data = {SUN8I_H3_THS_TDATA0, > + SUN8I_A83T_THS_TDATA1, > + SUN8I_A83T_THS_TDATA2, > + 0}, > + .sample_start = sunxi_ths_sample_start, > + .sample_end = sunxi_ths_sample_end, > + .sensor_count = 3, > + .supports_nvmem = false, > + .support_irq = true, > + .ctrl0_map = SUN4I_GPADC_CTRL0_T_ACQ(0x1f3), > + .ctrl2_map = SUN8I_H3_THS_ACQ1(0x1f3), Where are these values coming from? > + .sensor_en_map = SUN8I_H3_THS_TEMP_SENSE_EN0 | > + SUN8I_A83T_THS_TEMP_SENSE_EN1 | > + SUN8I_A83T_THS_TEMP_SENSE_EN2, > + .filter_map = SUN4I_GPADC_CTRL3_FILTER_EN | > + SUN4I_GPADC_CTRL3_FILTER_TYPE(0x2), > + .irq_clear_map = SUN8I_H3_THS_INTS_ALARM_INT_0 | > + SUN8I_A83T_THS_INTS_ALARM_INT_1 | > + SUN8I_A83T_THS_INTS_ALARM_INT_2 | > + SUN8I_H3_THS_INTS_SHUT_INT_0 | > + SUN8I_A83T_THS_INTS_SHUT_INT_1 | > + SUN8I_A83T_THS_INTS_SHUT_INT_2 | > + SUN8I_H3_THS_INTS_TDATA_IRQ_0 | > + SUN8I_A83T_THS_INTS_TDATA_IRQ_1 | > + SUN8I_A83T_THS_INTS_TDATA_IRQ_2, Do you reall need to clear all these interrupts if you're using only one? Maxime
Hi, On Mon, Jan 29, 2018 at 12:29:14AM +0100, Philipp Rossak wrote: > As we have gained the support for the thermal sensor in H3 and H5, > we can now add its device nodes to the device tree. The H3 and H5 share > most of its compatible. The compatible and the thermal sensor cells > will be added in an additional patch per device. > > Signed-off-by: Philipp Rossak <embed3d@gmail.com> > --- > arch/arm/boot/dts/sunxi-h3-h5.dtsi | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/arch/arm/boot/dts/sunxi-h3-h5.dtsi b/arch/arm/boot/dts/sunxi-h3-h5.dtsi > index 7a83b15225c7..413c789b588d 100644 > --- a/arch/arm/boot/dts/sunxi-h3-h5.dtsi > +++ b/arch/arm/boot/dts/sunxi-h3-h5.dtsi > @@ -426,6 +426,15 @@ > }; > }; > > + ths: thermal-sensor@1c25000 { > + reg = <0x01c25000 0x100>; The size is 0x400 Maxime
On Mon, Jan 29, 2018 at 12:29:16AM +0100, Philipp Rossak wrote: > This patch adds the thermal zones to the H3. We have only one sensor and > that is placed in the cpu. > > Signed-off-by: Philipp Rossak <embed3d@gmail.com> > --- > arch/arm/boot/dts/sun8i-h3.dtsi | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi > index fbb007e5798e..3f83f6a27c74 100644 > --- a/arch/arm/boot/dts/sun8i-h3.dtsi > +++ b/arch/arm/boot/dts/sun8i-h3.dtsi > @@ -72,6 +72,15 @@ > }; > }; > > + thermal-zones { > + cpu-thermal { > + /* milliseconds */ > + polling-delay-passive = <250>; > + polling-delay = <1000>; > + thermal-sensors = <&ths 0>; if the thermal-sensor-cells value is indeed 0, the phandle parsing will be broken here. Maxime
On Mon, Jan 29, 2018 at 12:29:17AM +0100, Philipp Rossak wrote: > This patch enables the the sid controller in the H3. It can be used > for thermal calibration data. > > Signed-off-by: Philipp Rossak <embed3d@gmail.com> > --- > arch/arm/boot/dts/sun8i-h3.dtsi | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi > index 3f83f6a27c74..9bb5cc29fec5 100644 > --- a/arch/arm/boot/dts/sun8i-h3.dtsi > +++ b/arch/arm/boot/dts/sun8i-h3.dtsi > @@ -72,6 +72,13 @@ > }; > }; > > + soc { > + sid: eeprom@1c14000 { > + compatible = "allwinner,sun8i-h3-sid"; > + reg = <0x01c14000 0x400>; > + }; > + }; > + Shouldn't you also use a nvmem-cells property to the THS node? Maxime
On 29.01.2018 10:48, Maxime Ripard wrote: > On Mon, Jan 29, 2018 at 12:29:13AM +0100, Philipp Rossak wrote: >> This patch adds support for the A83T ths sensor. >> >> The A83T supports interrupts. The interrupt is configured to update the >> the sensor values every second. >> >> Signed-off-by: Philipp Rossak <embed3d@gmail.com> >> --- >> drivers/iio/adc/sun4i-gpadc-iio.c | 38 ++++++++++++++++++++++++++++++++++++++ >> include/linux/mfd/sun4i-gpadc.h | 18 ++++++++++++++++++ >> 2 files changed, 56 insertions(+) >> >> diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c >> index 8196203d65fe..9f7895ba1966 100644 >> --- a/drivers/iio/adc/sun4i-gpadc-iio.c >> +++ b/drivers/iio/adc/sun4i-gpadc-iio.c >> @@ -170,6 +170,40 @@ static const struct gpadc_data sun8i_h3_ths_data = { >> SUN8I_H3_THS_TEMP_PERIOD(0x7), >> }; >> >> +static const struct gpadc_data sun8i_a83t_ths_data = { >> + .temp_offset = -2724, >> + .temp_scale = -70, >> + .temp_data = {SUN8I_H3_THS_TDATA0, >> + SUN8I_A83T_THS_TDATA1, >> + SUN8I_A83T_THS_TDATA2, >> + 0}, >> + .sample_start = sunxi_ths_sample_start, >> + .sample_end = sunxi_ths_sample_end, >> + .sensor_count = 3, >> + .supports_nvmem = false, >> + .support_irq = true, >> + .ctrl0_map = SUN4I_GPADC_CTRL0_T_ACQ(0x1f3), >> + .ctrl2_map = SUN8I_H3_THS_ACQ1(0x1f3), > > Where are these values coming from? > These values are calculated with the formulas from the datasheet and also tested on hardware. These settings seem ok. >> + .sensor_en_map = SUN8I_H3_THS_TEMP_SENSE_EN0 | >> + SUN8I_A83T_THS_TEMP_SENSE_EN1 | >> + SUN8I_A83T_THS_TEMP_SENSE_EN2, >> + .filter_map = SUN4I_GPADC_CTRL3_FILTER_EN | >> + SUN4I_GPADC_CTRL3_FILTER_TYPE(0x2), >> + .irq_clear_map = SUN8I_H3_THS_INTS_ALARM_INT_0 | >> + SUN8I_A83T_THS_INTS_ALARM_INT_1 | >> + SUN8I_A83T_THS_INTS_ALARM_INT_2 | >> + SUN8I_H3_THS_INTS_SHUT_INT_0 | >> + SUN8I_A83T_THS_INTS_SHUT_INT_1 | >> + SUN8I_A83T_THS_INTS_SHUT_INT_2 | >> + SUN8I_H3_THS_INTS_TDATA_IRQ_0 | >> + SUN8I_A83T_THS_INTS_TDATA_IRQ_1 | >> + SUN8I_A83T_THS_INTS_TDATA_IRQ_2, > > Do you reall need to clear all these interrupts if you're using only > one? > No, I don't think so, I will remove them in the next version. > Maxime > Philipp -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 29.01.2018 10:49, Maxime Ripard wrote: > Hi, > > On Mon, Jan 29, 2018 at 12:29:14AM +0100, Philipp Rossak wrote: >> As we have gained the support for the thermal sensor in H3 and H5, >> we can now add its device nodes to the device tree. The H3 and H5 share >> most of its compatible. The compatible and the thermal sensor cells >> will be added in an additional patch per device. >> >> Signed-off-by: Philipp Rossak <embed3d@gmail.com> >> --- >> arch/arm/boot/dts/sunxi-h3-h5.dtsi | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/arch/arm/boot/dts/sunxi-h3-h5.dtsi b/arch/arm/boot/dts/sunxi-h3-h5.dtsi >> index 7a83b15225c7..413c789b588d 100644 >> --- a/arch/arm/boot/dts/sunxi-h3-h5.dtsi >> +++ b/arch/arm/boot/dts/sunxi-h3-h5.dtsi >> @@ -426,6 +426,15 @@ >> }; >> }; >> >> + ths: thermal-sensor@1c25000 { >> + reg = <0x01c25000 0x100>; > > The size is 0x400 > > Maxime > Ok, I will fix this. Philipp -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 29.01.2018 10:50, Maxime Ripard wrote: > On Mon, Jan 29, 2018 at 12:29:16AM +0100, Philipp Rossak wrote: >> This patch adds the thermal zones to the H3. We have only one sensor and >> that is placed in the cpu. >> >> Signed-off-by: Philipp Rossak <embed3d@gmail.com> >> --- >> arch/arm/boot/dts/sun8i-h3.dtsi | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi >> index fbb007e5798e..3f83f6a27c74 100644 >> --- a/arch/arm/boot/dts/sun8i-h3.dtsi >> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi >> @@ -72,6 +72,15 @@ >> }; >> }; >> >> + thermal-zones { >> + cpu-thermal { >> + /* milliseconds */ >> + polling-delay-passive = <250>; >> + polling-delay = <1000>; >> + thermal-sensors = <&ths 0>; > > if the thermal-sensor-cells value is indeed 0, the phandle parsing > will be broken here. > > Maxime > Ok, then I will remove the 0. Thanks, Philipp -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 29.01.2018 10:52, Maxime Ripard wrote: > On Mon, Jan 29, 2018 at 12:29:17AM +0100, Philipp Rossak wrote: >> This patch enables the the sid controller in the H3. It can be used >> for thermal calibration data. >> >> Signed-off-by: Philipp Rossak <embed3d@gmail.com> >> --- >> arch/arm/boot/dts/sun8i-h3.dtsi | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi >> index 3f83f6a27c74..9bb5cc29fec5 100644 >> --- a/arch/arm/boot/dts/sun8i-h3.dtsi >> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi >> @@ -72,6 +72,13 @@ >> }; >> }; >> >> + soc { >> + sid: eeprom@1c14000 { >> + compatible = "allwinner,sun8i-h3-sid"; >> + reg = <0x01c14000 0x400>; >> + }; >> + }; >> + > > Shouldn't you also use a nvmem-cells property to the THS node? > > Maxime > Oh seems like I forgot that. As related to the wiki [1] this should be 64 bit wide at the address 0x34. I will add that in the next version. [1]: http://linux-sunxi.org/SID_Register_Guide#eFUSE Thanks, Philipp -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 29.01.2018 10:40, Maxime Ripard wrote: > On Mon, Jan 29, 2018 at 12:29:10AM +0100, Philipp Rossak wrote: >> This patch reworks the driver to support nvmem calibration cells. >> The driver checks if the nvmem calibration is supported and reads out >> the nvmem. >> >> Signed-off-by: Philipp Rossak <embed3d@gmail.com> >> --- >> drivers/iio/adc/sun4i-gpadc-iio.c | 44 +++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 44 insertions(+) >> >> diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c >> index ac9ad2f8232f..74eeb5cd5218 100644 >> --- a/drivers/iio/adc/sun4i-gpadc-iio.c >> +++ b/drivers/iio/adc/sun4i-gpadc-iio.c >> @@ -27,6 +27,7 @@ >> #include <linux/interrupt.h> >> #include <linux/io.h> >> #include <linux/module.h> >> +#include <linux/nvmem-consumer.h> >> #include <linux/of.h> >> #include <linux/of_device.h> >> #include <linux/platform_device.h> >> @@ -74,6 +75,7 @@ struct gpadc_data { >> bool has_bus_rst; >> bool has_mod_clk; >> int sensor_count; >> + bool supports_nvmem; > > I think you should add some documentation along with all the fields > you're adding. ok I will add more informations in the next version into the commit message. > >> }; >> >> static const struct gpadc_data sun4i_gpadc_data = { >> @@ -87,6 +89,7 @@ static const struct gpadc_data sun4i_gpadc_data = { >> .sample_start = sun4i_gpadc_sample_start, >> .sample_end = sun4i_gpadc_sample_end, >> .sensor_count = 1, >> + .supports_nvmem = false, > > That's already its value if you leave it out. > >> }; >> >> static const struct gpadc_data sun5i_gpadc_data = { >> @@ -100,6 +103,7 @@ static const struct gpadc_data sun5i_gpadc_data = { >> .sample_start = sun4i_gpadc_sample_start, >> .sample_end = sun4i_gpadc_sample_end, >> .sensor_count = 1, >> + .supports_nvmem = false, >> }; >> >> static const struct gpadc_data sun6i_gpadc_data = { >> @@ -113,6 +117,7 @@ static const struct gpadc_data sun6i_gpadc_data = { >> .sample_start = sun4i_gpadc_sample_start, >> .sample_end = sun4i_gpadc_sample_end, >> .sensor_count = 1, >> + .supports_nvmem = false, >> }; >> >> static const struct gpadc_data sun8i_a33_gpadc_data = { >> @@ -123,6 +128,7 @@ static const struct gpadc_data sun8i_a33_gpadc_data = { >> .sample_start = sun4i_gpadc_sample_start, >> .sample_end = sun4i_gpadc_sample_end, >> .sensor_count = 1, >> + .supports_nvmem = false, >> }; >> >> struct sun4i_gpadc_iio { >> @@ -141,6 +147,8 @@ struct sun4i_gpadc_iio { >> struct clk *mod_clk; >> struct reset_control *reset; >> int sensor_id; >> + u32 calibration_data[2]; >> + bool has_calibration_data[2]; > > Why do you have two different values here? > I think my idea was too complex! I thought it would be better to check if calibration data was read, and is able to be written to hardware. those information were split per register. I think a u64 should be fine for calibration_data. When I write the calibration data I can check on the sensor count and write only the lower 32 bits if there are less than 3 sensors. Is this ok for you? >> /* prevents concurrent reads of temperature and ADC */ >> struct mutex mutex; >> struct thermal_zone_device *tzd; >> @@ -561,6 +569,9 @@ static int sun4i_gpadc_probe_dt(struct platform_device *pdev, >> struct resource *mem; >> void __iomem *base; >> int ret; >> + struct nvmem_cell *cell; >> + ssize_t cell_size; >> + u64 *cell_data; >> >> info->data = of_device_get_match_data(&pdev->dev); >> if (!info->data) >> @@ -575,6 +586,39 @@ static int sun4i_gpadc_probe_dt(struct platform_device *pdev, >> if (IS_ERR(base)) >> return PTR_ERR(base); >> >> + info->has_calibration_data[0] = false; >> + info->has_calibration_data[1] = false; >> + >> + if (!info->data->supports_nvmem) >> + goto no_nvmem; >> + >> + cell = nvmem_cell_get(&pdev->dev, "calibration"); >> + if (IS_ERR(cell)) { >> + if (PTR_ERR(cell) == -EPROBE_DEFER) >> + return PTR_ERR(cell); >> + goto no_nvmem; > > goto considered evil ? :) > this was a suggestion from Jonatan in version one, to make the code better readable. . >> + } >> + >> + cell_data = (u64 *)nvmem_cell_read(cell, &cell_size); >> + nvmem_cell_put(cell); >> + switch (cell_size) { >> + case 8: >> + case 6: >> + info->has_calibration_data[1] = true; >> + info->calibration_data[1] = be32_to_cpu( >> + upper_32_bits(cell_data[0])); >> + case 4: >> + case 2: >> + info->has_calibration_data[0] = true; >> + info->calibration_data[0] = be32_to_cpu( >> + lower_32_bits(cell_data[0])); > > Why do you need that switch? You are right! The calibration reg seems to be always 64 bit wide. [1] So I will just check for the length of 8. > > Thanks! > Maxime > [1]: http://linux-sunxi.org/SID_Register_Guide#eFUSE Thanks, Philipp -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
1;5002;0c On Mon, Jan 29, 2018 at 12:53:48PM +0100, Philipp Rossak wrote: > > > On 29.01.2018 10:48, Maxime Ripard wrote: > > On Mon, Jan 29, 2018 at 12:29:13AM +0100, Philipp Rossak wrote: > > > This patch adds support for the A83T ths sensor. > > > > > > The A83T supports interrupts. The interrupt is configured to update the > > > the sensor values every second. > > > > > > Signed-off-by: Philipp Rossak <embed3d@gmail.com> > > > --- > > > drivers/iio/adc/sun4i-gpadc-iio.c | 38 ++++++++++++++++++++++++++++++++++++++ > > > include/linux/mfd/sun4i-gpadc.h | 18 ++++++++++++++++++ > > > 2 files changed, 56 insertions(+) > > > > > > diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c > > > index 8196203d65fe..9f7895ba1966 100644 > > > --- a/drivers/iio/adc/sun4i-gpadc-iio.c > > > +++ b/drivers/iio/adc/sun4i-gpadc-iio.c > > > @@ -170,6 +170,40 @@ static const struct gpadc_data sun8i_h3_ths_data = { > > > SUN8I_H3_THS_TEMP_PERIOD(0x7), > > > }; > > > +static const struct gpadc_data sun8i_a83t_ths_data = { > > > + .temp_offset = -2724, > > > + .temp_scale = -70, > > > + .temp_data = {SUN8I_H3_THS_TDATA0, > > > + SUN8I_A83T_THS_TDATA1, > > > + SUN8I_A83T_THS_TDATA2, > > > + 0}, > > > + .sample_start = sunxi_ths_sample_start, > > > + .sample_end = sunxi_ths_sample_end, > > > + .sensor_count = 3, > > > + .supports_nvmem = false, > > > + .support_irq = true, > > > + .ctrl0_map = SUN4I_GPADC_CTRL0_T_ACQ(0x1f3), > > > + .ctrl2_map = SUN8I_H3_THS_ACQ1(0x1f3), > > > > Where are these values coming from? > > > > These values are calculated with the formulas from the datasheet and also > tested on hardware. These settings seem ok. You should at least put a comment explaining how you got to these values. Maxime
On Mon, Jan 29, 2018 at 01:33:12PM +0100, Philipp Rossak wrote: > > > static const struct gpadc_data sun4i_gpadc_data = { > > > @@ -87,6 +89,7 @@ static const struct gpadc_data sun4i_gpadc_data = { > > > .sample_start = sun4i_gpadc_sample_start, > > > .sample_end = sun4i_gpadc_sample_end, > > > .sensor_count = 1, > > > + .supports_nvmem = false, > > > > That's already its value if you leave it out. > > > > > }; > > > static const struct gpadc_data sun5i_gpadc_data = { > > > @@ -100,6 +103,7 @@ static const struct gpadc_data sun5i_gpadc_data = { > > > .sample_start = sun4i_gpadc_sample_start, > > > .sample_end = sun4i_gpadc_sample_end, > > > .sensor_count = 1, > > > + .supports_nvmem = false, > > > }; > > > static const struct gpadc_data sun6i_gpadc_data = { > > > @@ -113,6 +117,7 @@ static const struct gpadc_data sun6i_gpadc_data = { > > > .sample_start = sun4i_gpadc_sample_start, > > > .sample_end = sun4i_gpadc_sample_end, > > > .sensor_count = 1, > > > + .supports_nvmem = false, > > > }; > > > static const struct gpadc_data sun8i_a33_gpadc_data = { > > > @@ -123,6 +128,7 @@ static const struct gpadc_data sun8i_a33_gpadc_data = { > > > .sample_start = sun4i_gpadc_sample_start, > > > .sample_end = sun4i_gpadc_sample_end, > > > .sensor_count = 1, > > > + .supports_nvmem = false, > > > }; > > > struct sun4i_gpadc_iio { > > > @@ -141,6 +147,8 @@ struct sun4i_gpadc_iio { > > > struct clk *mod_clk; > > > struct reset_control *reset; > > > int sensor_id; > > > + u32 calibration_data[2]; > > > + bool has_calibration_data[2]; > > > > Why do you have two different values here? > > I think my idea was too complex! I thought it would be better to check if > calibration data was read, and is able to be written to hardware. those > information were split per register. > > I think a u64 should be fine for calibration_data. When I write the > calibration data I can check on the sensor count and write only the lower 32 > bits if there are less than 3 sensors. > > Is this ok for you? I'd need to see the implementation, but make sure that this is documented in your driver :) > > > > /* prevents concurrent reads of temperature and ADC */ > > > struct mutex mutex; > > > struct thermal_zone_device *tzd; > > > @@ -561,6 +569,9 @@ static int sun4i_gpadc_probe_dt(struct platform_device *pdev, > > > struct resource *mem; > > > void __iomem *base; > > > int ret; > > > + struct nvmem_cell *cell; > > > + ssize_t cell_size; > > > + u64 *cell_data; > > > info->data = of_device_get_match_data(&pdev->dev); > > > if (!info->data) > > > @@ -575,6 +586,39 @@ static int sun4i_gpadc_probe_dt(struct platform_device *pdev, > > > if (IS_ERR(base)) > > > return PTR_ERR(base); > > > + info->has_calibration_data[0] = false; > > > + info->has_calibration_data[1] = false; > > > + > > > + if (!info->data->supports_nvmem) > > > + goto no_nvmem; > > > + > > > + cell = nvmem_cell_get(&pdev->dev, "calibration"); > > > + if (IS_ERR(cell)) { > > > + if (PTR_ERR(cell) == -EPROBE_DEFER) > > > + return PTR_ERR(cell); > > > + goto no_nvmem; > > > > goto considered evil ? :) > > this was a suggestion from Jonatan in version one, to make the code better > readable. Isn't if (info->data->supports_nvmem && IS_ERR(cell = nvmem_cell_get())) pretty much the same thing? Maxime
Hi Philipp, On Mon, Jan 29, 2018 at 12:29:07AM +0100, Philipp Rossak wrote: > For adding newer sensor some basic rework of the code is necessary. > > This commit reworks the code and allows the sampling start/end code and > the position of value readout register to be altered. Later the start/end > functions will be used to configure the ths and start/stop the > sampling. > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > Signed-off-by: Philipp Rossak <embed3d@gmail.com> > --- > drivers/iio/adc/sun4i-gpadc-iio.c | 44 ++++++++++++++++++++++++++++++++++----- > 1 file changed, 39 insertions(+), 5 deletions(-) > > diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c > index 03804ff9c006..db57d9fffe48 100644 > --- a/drivers/iio/adc/sun4i-gpadc-iio.c > +++ b/drivers/iio/adc/sun4i-gpadc-iio.c > @@ -49,6 +49,15 @@ static unsigned int sun6i_gpadc_chan_select(unsigned int chan) > return SUN6I_GPADC_CTRL1_ADC_CHAN_SELECT(chan); > } > > +struct sun4i_gpadc_iio; > + > +/* > + * Prototypes for these functions, which enable these functions to be > + * referenced in gpadc_data structures. > + */ Comment not needed. > +static int sun4i_gpadc_sample_start(struct sun4i_gpadc_iio *info); > +static int sun4i_gpadc_sample_end(struct sun4i_gpadc_iio *info); > + > struct gpadc_data { > int temp_offset; > int temp_scale; > @@ -56,6 +65,9 @@ struct gpadc_data { > unsigned int tp_adc_select; > unsigned int (*adc_chan_select)(unsigned int chan); > unsigned int adc_chan_mask; > + unsigned int temp_data; Does not really have anything to do with sample_start/end. I would have made a different commit for it. Otherwise, Reviewed-by: Quentin Schulz <quentin.schulz@free-electrons.com> Quentin
On 31.01.2018 18:51, Quentin Schulz wrote: > Hi Philipp, > > On Mon, Jan 29, 2018 at 12:29:07AM +0100, Philipp Rossak wrote: >> For adding newer sensor some basic rework of the code is necessary. >> >> This commit reworks the code and allows the sampling start/end code and >> the position of value readout register to be altered. Later the start/end >> functions will be used to configure the ths and start/stop the >> sampling. >> >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io> >> Signed-off-by: Philipp Rossak <embed3d@gmail.com> >> --- >> drivers/iio/adc/sun4i-gpadc-iio.c | 44 ++++++++++++++++++++++++++++++++++----- >> 1 file changed, 39 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c >> index 03804ff9c006..db57d9fffe48 100644 >> --- a/drivers/iio/adc/sun4i-gpadc-iio.c >> +++ b/drivers/iio/adc/sun4i-gpadc-iio.c >> @@ -49,6 +49,15 @@ static unsigned int sun6i_gpadc_chan_select(unsigned int chan) >> return SUN6I_GPADC_CTRL1_ADC_CHAN_SELECT(chan); >> } >> >> +struct sun4i_gpadc_iio; >> + >> +/* >> + * Prototypes for these functions, which enable these functions to be >> + * referenced in gpadc_data structures. >> + */ > > Comment not needed. > >> +static int sun4i_gpadc_sample_start(struct sun4i_gpadc_iio *info); >> +static int sun4i_gpadc_sample_end(struct sun4i_gpadc_iio *info); >> + >> struct gpadc_data { >> int temp_offset; >> int temp_scale; >> @@ -56,6 +65,9 @@ struct gpadc_data { >> unsigned int tp_adc_select; >> unsigned int (*adc_chan_select)(unsigned int chan); >> unsigned int adc_chan_mask; >> + unsigned int temp_data; > > Does not really have anything to do with sample_start/end. I would have > made a different commit for it. > > Otherwise, > Reviewed-by: Quentin Schulz <quentin.schulz@free-electrons.com> > > Quentin > Ok I will split this. Thanks, Philipp -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Philipp, On Mon, Jan 29, 2018 at 12:29:09AM +0100, Philipp Rossak wrote: > For adding newer sensor some basic rework of the code is necessary. > > This patch reworks the driver to be able to handle more than one > thermal sensor. Newer SoC like the A80 have 4 thermal sensors. > Because of this the maximal sensor count value was set to 4. > > The sensor_id value is set during sensor registration and is for each > registered sensor indiviual. This makes it able to differntiate the > sensors when the value is read from the register. > > In function sun4i_gpadc_read_raw(), the sensor number of the ths sensor > was directly set to 0 (sun4i_gpadc_temp_read(x,x,0)). This selects > in the temp_read function automatically sensor 0. A check for the > sensor_id is here not required since the old sensors only have one > thermal sensor. In addition to that is the sun4i_gpadc_read_raw() > function only used by the "older" sensors (before A33) where the > thermal sensor was a cobination of an adc and a thermal sensor. > > Signed-off-by: Philipp Rossak <embed3d@gmail.com> > --- > drivers/iio/adc/sun4i-gpadc-iio.c | 36 +++++++++++++++++++++++------------- > include/linux/mfd/sun4i-gpadc.h | 3 +++ > 2 files changed, 26 insertions(+), 13 deletions(-) > > diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c > index 51ec0104d678..ac9ad2f8232f 100644 > --- a/drivers/iio/adc/sun4i-gpadc-iio.c > +++ b/drivers/iio/adc/sun4i-gpadc-iio.c > @@ -67,12 +67,13 @@ struct gpadc_data { > unsigned int tp_adc_select; > unsigned int (*adc_chan_select)(unsigned int chan); > unsigned int adc_chan_mask; > - unsigned int temp_data; > + unsigned int temp_data[MAX_SENSOR_COUNT]; > int (*sample_start)(struct sun4i_gpadc_iio *info); > int (*sample_end)(struct sun4i_gpadc_iio *info); > bool has_bus_clk; > bool has_bus_rst; > bool has_mod_clk; > + int sensor_count; > }; > I've noticed that for H3, A83T, A64 (at least), if DATA reg of sensor 0 is e.g. 0x80, DATA reg of sensor N is at 0x80 + 0x04 * N. Is that verified for other SoCs? Does anyone have some input on this? We could then just use temp_data as the DATA reg "base" and increment by 0x4 depending on the sensor id instead of using a fixed-size array. > static const struct gpadc_data sun4i_gpadc_data = { > @@ -82,9 +83,10 @@ 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, > - .temp_data = SUN4I_GPADC_TEMP_DATA, > + .temp_data = {SUN4I_GPADC_TEMP_DATA, 0, 0, 0}, > .sample_start = sun4i_gpadc_sample_start, > .sample_end = sun4i_gpadc_sample_end, > + .sensor_count = 1, If the solution above is not desirable/possible, could we use something like: unsigned int sun4i_temp_data[] = {SUN4I_GPADC_TEMP_DATA,}; static const struct gpadc_data sun4i_gpadc_data = { .temp_data = &sun4i_temp_data, .sensor_count = ARRAY_SIZE(sun4i_temp_data), }; That avoids 1) inconsistencies between the array size and the array itself, 2) does not require to pad the array with zeroes. [...] > @@ -745,9 +752,12 @@ 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); > + for (i = 0; i < info->data->sensor_count; i++) { > + info->sensor_id = i; > + info->tzd = thermal_zone_of_sensor_register( > + info->sensor_device, > + i, info, &sun4i_ts_tz_ops); > + } As Maxime said, this does not work. One way would be to have a new structure being: struct sun4i_sensor_info { struct sun4i_gpadc_iio *info; unsigned int sensor_id; }; Or since we only use the iio_dev within the sun4i_gpadc_iio in the .get_temp function, we may replace info by struct iio_dev *indio_dev above. Quentin
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, On Mon, Jan 29, 2018 at 12:29:12AM +0100, Philipp Rossak wrote: > This patch adds support for the H3 ths sensor. > > The H3 supports interrupts. The interrupt is configured to update the > the sensor values every second. The calibration data is writen at the > begin of the init process. > > Signed-off-by: Philipp Rossak <embed3d@gmail.com> > --- > drivers/iio/adc/sun4i-gpadc-iio.c | 86 +++++++++++++++++++++++++++++++++++++++ > include/linux/mfd/sun4i-gpadc.h | 22 ++++++++++ > 2 files changed, 108 insertions(+) > > diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c > index b7b5451226b0..8196203d65fe 100644 > --- a/drivers/iio/adc/sun4i-gpadc-iio.c > +++ b/drivers/iio/adc/sun4i-gpadc-iio.c > @@ -61,6 +61,9 @@ struct sun4i_gpadc_iio; > static int sun4i_gpadc_sample_start(struct sun4i_gpadc_iio *info); > static int sun4i_gpadc_sample_end(struct sun4i_gpadc_iio *info); > > +static int sunxi_ths_sample_start(struct sun4i_gpadc_iio *info); > +static int sunxi_ths_sample_end(struct sun4i_gpadc_iio *info); > + We try to avoid using the generic sunxi prefix. > struct gpadc_data { > int temp_offset; > int temp_scale; > @@ -71,6 +74,10 @@ 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 ctrl0_map; > + u32 ctrl2_map; > + u32 sensor_en_map; > + u32 filter_map; > u32 irq_clear_map; > u32 irq_control_map; > bool has_bus_clk; > @@ -138,6 +145,31 @@ static const struct gpadc_data sun8i_a33_gpadc_data = { > .support_irq = false, > }; > > +static const struct gpadc_data sun8i_h3_ths_data = { > + .temp_offset = -1791, > + .temp_scale = -121, > + .temp_data = {SUN8I_H3_THS_TDATA0, 0, 0, 0}, > + .sample_start = sunxi_ths_sample_start, > + .sample_end = sunxi_ths_sample_end, > + .has_bus_clk = true, > + .has_bus_rst = true, > + .has_mod_clk = true, > + .sensor_count = 1, > + .supports_nvmem = true, > + .support_irq = true, > + .ctrl0_map = SUN4I_GPADC_CTRL0_T_ACQ(0xff), > + .ctrl2_map = SUN8I_H3_THS_ACQ1(0x3f), > + .sensor_en_map = SUN8I_H3_THS_TEMP_SENSE_EN0, > + .filter_map = SUN4I_GPADC_CTRL3_FILTER_EN | > + SUN4I_GPADC_CTRL3_FILTER_TYPE(0x2), > + .irq_clear_map = SUN8I_H3_THS_INTS_ALARM_INT_0 | > + SUN8I_H3_THS_INTS_SHUT_INT_0 | > + SUN8I_H3_THS_INTS_TDATA_IRQ_0 | > + SUN8I_H3_THS_INTS_ALARM_OFF_0, > + .irq_control_map = SUN8I_H3_THS_INTC_TDATA_IRQ_EN0 | > + SUN8I_H3_THS_TEMP_PERIOD(0x7), From what I've understood, ACQ regs are basically clock dividers. We should make a better job at explaining it :) > +}; > + > struct sun4i_gpadc_iio { > struct iio_dev *indio_dev; > struct completion completion; > @@ -462,6 +494,16 @@ static int sun4i_gpadc_sample_end(struct sun4i_gpadc_iio *info) > return 0; > } > > +static int sunxi_ths_sample_end(struct sun4i_gpadc_iio *info) > +{ > + /* Disable ths interrupt */ > + regmap_write(info->regmap, SUN8I_H3_THS_INTC, 0x0); > + /* Disable temperature sensor */ > + regmap_write(info->regmap, SUN8I_H3_THS_CTRL2, 0x0); > + > + return 0; > +} > + > static int sun4i_gpadc_runtime_suspend(struct device *dev) > { > struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev)); > @@ -473,6 +515,17 @@ static int sun4i_gpadc_runtime_suspend(struct device *dev) > return info->data->sample_end(info); > } > > +static void sunxi_calibrate(struct sun4i_gpadc_iio *info) > +{ > + if (info->has_calibration_data[0]) > + regmap_write(info->regmap, SUNXI_THS_CDATA_0_1, > + info->calibration_data[0]); > + > + if (info->has_calibration_data[1]) > + regmap_write(info->regmap, SUNXI_THS_CDATA_2_3, > + info->calibration_data[1]); > +} > + > static int sun4i_gpadc_sample_start(struct sun4i_gpadc_iio *info) > { > /* clkin = 6MHz */ > @@ -492,6 +545,35 @@ static int sun4i_gpadc_sample_start(struct sun4i_gpadc_iio *info) > return 0; > } > > +static int sunxi_ths_sample_start(struct sun4i_gpadc_iio *info) > +{ > + u32 value; > + sunxi_calibrate(info); > + > + if (info->data->ctrl0_map) > + regmap_write(info->regmap, SUN8I_H3_THS_CTRL0, > + info->data->ctrl0_map); > + > + regmap_write(info->regmap, SUN8I_H3_THS_CTRL2, > + info->data->ctrl2_map); > + > + regmap_write(info->regmap, SUN8I_H3_THS_STAT, > + info->data->irq_clear_map); > + > + regmap_write(info->regmap, SUN8I_H3_THS_FILTER, > + info->data->filter_map); > + > + regmap_write(info->regmap, SUN8I_H3_THS_INTC, > + info->data->irq_control_map); > + > + regmap_read(info->regmap, SUN8I_H3_THS_CTRL2, &value); > + > + regmap_write(info->regmap, SUN8I_H3_THS_CTRL2, > + info->data->sensor_en_map | value); > + > + return 0; > +} > + I'm a bit worried. Will all the sensors have the same sample start? Or will we need at some point also entries in info->data for register addresses, if they have ctrl2 or filter, etc... Maybe we could define a sample_start for the h3 only and reuse-it if possible and if not declare another sample start? All depends if the sample start is most likely to change in the near future or not. If it is, then we should avoid adding a lot of variables in info->data and just go for a single sample_start per SoC. > static int sun4i_gpadc_runtime_resume(struct device *dev) > { > struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev)); > @@ -582,6 +664,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_ths_data, > + }, > { /* sentinel */ } > }; > > diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h > index 458f2159a95a..80b79c31cea3 100644 > --- a/include/linux/mfd/sun4i-gpadc.h > +++ b/include/linux/mfd/sun4i-gpadc.h > @@ -91,7 +91,29 @@ > #define SUN4I_GPADC_AUTOSUSPEND_DELAY 10000 > > /* SUNXI_THS COMMON REGISTERS + DEFINES */ > +#define SUN8I_H3_THS_CTRL0 0x00 > +#define SUN8I_H3_THS_CTRL2 0x40 > #define SUN8I_H3_THS_INTC 0x44 > +#define SUN8I_H3_THS_STAT 0x48 > +#define SUN8I_H3_THS_FILTER 0x70 > +#define SUNXI_THS_CDATA_0_1 0x74 > +#define SUNXI_THS_CDATA_2_3 0x78 > +#define SUN8I_H3_THS_TDATA0 0x80 > + > +#define SUN8I_H3_THS_ACQ1(x) (GENMASK(31, 16) & ((x) << 16)) > + > +#define SUN8I_H3_THS_TEMP_SENSE_EN0 BIT(0) > + > +#define SUN8I_H3_THS_TEMP_PERIOD(x) (GENMASK(31, 12) & ((x) << 12)) > + > +#define SUN8I_H3_THS_INTS_ALARM_INT_0 BIT(0) > +#define SUN8I_H3_THS_INTS_SHUT_INT_0 BIT(4) > +#define SUN8I_H3_THS_INTS_TDATA_IRQ_0 BIT(8) > +#define SUN8I_H3_THS_INTS_ALARM_OFF_0 BIT(12) > + > +#define SUN8I_H3_THS_INTC_ALARM_INT_EN0 BIT(0) > +#define SUN8I_H3_THS_INTC_SHUT_INT_EN0 BIT(4) > +#define SUN8I_H3_THS_INTC_TDATA_IRQ_EN0 BIT(8) > Personal taste here but I prefer the register bits to be defined just below the register address define. i.e.: #define SUN8I_H3_THS_CTRL2 0x40 #define SUN8I_H3_THS_ACQ1(x) (GENMASK(31, 16) & ((x) << 16)) #define SUN8I_H3_THS_TEMP_SENSE_EN0 BIT(0) that way we know for which register we should use which constants. Quentin
On 31.01.2018 19:42, Quentin Schulz wrote: > Hi Philipp, > > On Mon, Jan 29, 2018 at 12:29:09AM +0100, Philipp Rossak wrote: >> For adding newer sensor some basic rework of the code is necessary. >> >> This patch reworks the driver to be able to handle more than one >> thermal sensor. Newer SoC like the A80 have 4 thermal sensors. >> Because of this the maximal sensor count value was set to 4. >> >> The sensor_id value is set during sensor registration and is for each >> registered sensor indiviual. This makes it able to differntiate the >> sensors when the value is read from the register. >> >> In function sun4i_gpadc_read_raw(), the sensor number of the ths sensor >> was directly set to 0 (sun4i_gpadc_temp_read(x,x,0)). This selects >> in the temp_read function automatically sensor 0. A check for the >> sensor_id is here not required since the old sensors only have one >> thermal sensor. In addition to that is the sun4i_gpadc_read_raw() >> function only used by the "older" sensors (before A33) where the >> thermal sensor was a cobination of an adc and a thermal sensor. >> >> Signed-off-by: Philipp Rossak <embed3d@gmail.com> >> --- >> drivers/iio/adc/sun4i-gpadc-iio.c | 36 +++++++++++++++++++++++------------- >> include/linux/mfd/sun4i-gpadc.h | 3 +++ >> 2 files changed, 26 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c >> index 51ec0104d678..ac9ad2f8232f 100644 >> --- a/drivers/iio/adc/sun4i-gpadc-iio.c >> +++ b/drivers/iio/adc/sun4i-gpadc-iio.c >> @@ -67,12 +67,13 @@ struct gpadc_data { >> unsigned int tp_adc_select; >> unsigned int (*adc_chan_select)(unsigned int chan); >> unsigned int adc_chan_mask; >> - unsigned int temp_data; >> + unsigned int temp_data[MAX_SENSOR_COUNT]; >> int (*sample_start)(struct sun4i_gpadc_iio *info); >> int (*sample_end)(struct sun4i_gpadc_iio *info); >> bool has_bus_clk; >> bool has_bus_rst; >> bool has_mod_clk; >> + int sensor_count; >> }; >> > > I've noticed that for H3, A83T, A64 (at least), if DATA reg of sensor 0 > is e.g. 0x80, DATA reg of sensor N is at 0x80 + 0x04 * N. > > Is that verified for other SoCs? Does anyone have some input on this? > > We could then just use temp_data as the DATA reg "base" and increment by > 0x4 depending on the sensor id instead of using a fixed-size array. > This sounds like a good idea! I will add this to the next version. I can verify this with a table, I created during development. I will upload it during the weekend here: [1] >> static const struct gpadc_data sun4i_gpadc_data = { >> @@ -82,9 +83,10 @@ 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, >> - .temp_data = SUN4I_GPADC_TEMP_DATA, >> + .temp_data = {SUN4I_GPADC_TEMP_DATA, 0, 0, 0}, >> .sample_start = sun4i_gpadc_sample_start, >> .sample_end = sun4i_gpadc_sample_end, >> + .sensor_count = 1, > > If the solution above is not desirable/possible, could we use something > like: > > unsigned int sun4i_temp_data[] = {SUN4I_GPADC_TEMP_DATA,}; > > static const struct gpadc_data sun4i_gpadc_data = { > .temp_data = &sun4i_temp_data, > .sensor_count = ARRAY_SIZE(sun4i_temp_data), > }; > > That avoids 1) inconsistencies between the array size and the array > itself, 2) does not require to pad the array with zeroes. > > [...] > >> @@ -745,9 +752,12 @@ 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); >> + for (i = 0; i < info->data->sensor_count; i++) { >> + info->sensor_id = i; >> + info->tzd = thermal_zone_of_sensor_register( >> + info->sensor_device, >> + i, info, &sun4i_ts_tz_ops); >> + } > > As Maxime said, this does not work. > > One way would be to have a new structure being: > struct sun4i_sensor_info { > struct sun4i_gpadc_iio *info; > unsigned int sensor_id; > }; > > Or since we only use the iio_dev within the sun4i_gpadc_iio in the > .get_temp function, we may replace info by struct iio_dev *indio_dev > above. > > Quentin > I will have a closer look on this next week, when I start to work on the next version.. Thanks, Philipp [1]: http://linux-sunxi.org/Thermal_Sensor -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 31.01.2018 20:23, Quentin Schulz wrote: > Hi Philipp, > > On Mon, Jan 29, 2018 at 12:29:12AM +0100, Philipp Rossak wrote: >> This patch adds support for the H3 ths sensor. >> >> The H3 supports interrupts. The interrupt is configured to update the >> the sensor values every second. The calibration data is writen at the >> begin of the init process. >> >> Signed-off-by: Philipp Rossak <embed3d@gmail.com> >> --- >> drivers/iio/adc/sun4i-gpadc-iio.c | 86 +++++++++++++++++++++++++++++++++++++++ >> include/linux/mfd/sun4i-gpadc.h | 22 ++++++++++ >> 2 files changed, 108 insertions(+) >> >> diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c >> index b7b5451226b0..8196203d65fe 100644 >> --- a/drivers/iio/adc/sun4i-gpadc-iio.c >> +++ b/drivers/iio/adc/sun4i-gpadc-iio.c >> @@ -61,6 +61,9 @@ struct sun4i_gpadc_iio; >> static int sun4i_gpadc_sample_start(struct sun4i_gpadc_iio *info); >> static int sun4i_gpadc_sample_end(struct sun4i_gpadc_iio *info); >> >> +static int sunxi_ths_sample_start(struct sun4i_gpadc_iio *info); >> +static int sunxi_ths_sample_end(struct sun4i_gpadc_iio *info); >> + > > We try to avoid using the generic sunxi prefix. > >> struct gpadc_data { >> int temp_offset; >> int temp_scale; >> @@ -71,6 +74,10 @@ 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 ctrl0_map; >> + u32 ctrl2_map; >> + u32 sensor_en_map; >> + u32 filter_map; >> u32 irq_clear_map; >> u32 irq_control_map; >> bool has_bus_clk; >> @@ -138,6 +145,31 @@ static const struct gpadc_data sun8i_a33_gpadc_data = { >> .support_irq = false, >> }; >> >> +static const struct gpadc_data sun8i_h3_ths_data = { >> + .temp_offset = -1791, >> + .temp_scale = -121, >> + .temp_data = {SUN8I_H3_THS_TDATA0, 0, 0, 0}, >> + .sample_start = sunxi_ths_sample_start, >> + .sample_end = sunxi_ths_sample_end, >> + .has_bus_clk = true, >> + .has_bus_rst = true, >> + .has_mod_clk = true, >> + .sensor_count = 1, >> + .supports_nvmem = true, >> + .support_irq = true, >> + .ctrl0_map = SUN4I_GPADC_CTRL0_T_ACQ(0xff), >> + .ctrl2_map = SUN8I_H3_THS_ACQ1(0x3f), >> + .sensor_en_map = SUN8I_H3_THS_TEMP_SENSE_EN0, >> + .filter_map = SUN4I_GPADC_CTRL3_FILTER_EN | >> + SUN4I_GPADC_CTRL3_FILTER_TYPE(0x2), >> + .irq_clear_map = SUN8I_H3_THS_INTS_ALARM_INT_0 | >> + SUN8I_H3_THS_INTS_SHUT_INT_0 | >> + SUN8I_H3_THS_INTS_TDATA_IRQ_0 | >> + SUN8I_H3_THS_INTS_ALARM_OFF_0, >> + .irq_control_map = SUN8I_H3_THS_INTC_TDATA_IRQ_EN0 | >> + SUN8I_H3_THS_TEMP_PERIOD(0x7), > > From what I've understood, ACQ regs are basically clock dividers. We > should make a better job at explaining it :) > I agree, I will add this in the next version in the commit message. >> +}; >> + >> struct sun4i_gpadc_iio { >> struct iio_dev *indio_dev; >> struct completion completion; >> @@ -462,6 +494,16 @@ static int sun4i_gpadc_sample_end(struct sun4i_gpadc_iio *info) >> return 0; >> } >> >> +static int sunxi_ths_sample_end(struct sun4i_gpadc_iio *info) >> +{ >> + /* Disable ths interrupt */ >> + regmap_write(info->regmap, SUN8I_H3_THS_INTC, 0x0); >> + /* Disable temperature sensor */ >> + regmap_write(info->regmap, SUN8I_H3_THS_CTRL2, 0x0); >> + >> + return 0; >> +} >> + >> static int sun4i_gpadc_runtime_suspend(struct device *dev) >> { >> struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev)); >> @@ -473,6 +515,17 @@ static int sun4i_gpadc_runtime_suspend(struct device *dev) >> return info->data->sample_end(info); >> } >> >> +static void sunxi_calibrate(struct sun4i_gpadc_iio *info) >> +{ >> + if (info->has_calibration_data[0]) >> + regmap_write(info->regmap, SUNXI_THS_CDATA_0_1, >> + info->calibration_data[0]); >> + >> + if (info->has_calibration_data[1]) >> + regmap_write(info->regmap, SUNXI_THS_CDATA_2_3, >> + info->calibration_data[1]); >> +} >> + >> static int sun4i_gpadc_sample_start(struct sun4i_gpadc_iio *info) >> { >> /* clkin = 6MHz */ >> @@ -492,6 +545,35 @@ static int sun4i_gpadc_sample_start(struct sun4i_gpadc_iio *info) >> return 0; >> } >> >> +static int sunxi_ths_sample_start(struct sun4i_gpadc_iio *info) >> +{ >> + u32 value; >> + sunxi_calibrate(info); >> + >> + if (info->data->ctrl0_map) >> + regmap_write(info->regmap, SUN8I_H3_THS_CTRL0, >> + info->data->ctrl0_map); >> + >> + regmap_write(info->regmap, SUN8I_H3_THS_CTRL2, >> + info->data->ctrl2_map); >> + >> + regmap_write(info->regmap, SUN8I_H3_THS_STAT, >> + info->data->irq_clear_map); >> + >> + regmap_write(info->regmap, SUN8I_H3_THS_FILTER, >> + info->data->filter_map); >> + >> + regmap_write(info->regmap, SUN8I_H3_THS_INTC, >> + info->data->irq_control_map); >> + >> + regmap_read(info->regmap, SUN8I_H3_THS_CTRL2, &value); >> + >> + regmap_write(info->regmap, SUN8I_H3_THS_CTRL2, >> + info->data->sensor_en_map | value); >> + >> + return 0; >> +} >> + > > I'm a bit worried. Will all the sensors have the same sample start? Or > will we need at some point also entries in info->data for register > addresses, if they have ctrl2 or filter, etc... > > Maybe we could define a sample_start for the h3 only and reuse-it if > possible and if not declare another sample start? All depends if the > sample start is most likely to change in the near future or not. If it > is, then we should avoid adding a lot of variables in info->data and > just go for a single sample_start per SoC. > for H3, A83T, H5, A64 we can use the same sample start. So I would use here a H3 sample start function and reuse it. A80 is special since it has no crtl0 register, thus it would get also its own function. H6 will need also an own sample start function (looking in the near future). >> static int sun4i_gpadc_runtime_resume(struct device *dev) >> { >> struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev)); >> @@ -582,6 +664,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_ths_data, >> + }, >> { /* sentinel */ } >> }; >> >> diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h >> index 458f2159a95a..80b79c31cea3 100644 >> --- a/include/linux/mfd/sun4i-gpadc.h >> +++ b/include/linux/mfd/sun4i-gpadc.h >> @@ -91,7 +91,29 @@ >> #define SUN4I_GPADC_AUTOSUSPEND_DELAY 10000 >> >> /* SUNXI_THS COMMON REGISTERS + DEFINES */ >> +#define SUN8I_H3_THS_CTRL0 0x00 >> +#define SUN8I_H3_THS_CTRL2 0x40 >> #define SUN8I_H3_THS_INTC 0x44 >> +#define SUN8I_H3_THS_STAT 0x48 >> +#define SUN8I_H3_THS_FILTER 0x70 >> +#define SUNXI_THS_CDATA_0_1 0x74 >> +#define SUNXI_THS_CDATA_2_3 0x78 >> +#define SUN8I_H3_THS_TDATA0 0x80 >> + >> +#define SUN8I_H3_THS_ACQ1(x) (GENMASK(31, 16) & ((x) << 16)) >> + >> +#define SUN8I_H3_THS_TEMP_SENSE_EN0 BIT(0) >> + >> +#define SUN8I_H3_THS_TEMP_PERIOD(x) (GENMASK(31, 12) & ((x) << 12)) >> + >> +#define SUN8I_H3_THS_INTS_ALARM_INT_0 BIT(0) >> +#define SUN8I_H3_THS_INTS_SHUT_INT_0 BIT(4) >> +#define SUN8I_H3_THS_INTS_TDATA_IRQ_0 BIT(8) >> +#define SUN8I_H3_THS_INTS_ALARM_OFF_0 BIT(12) >> + >> +#define SUN8I_H3_THS_INTC_ALARM_INT_EN0 BIT(0) >> +#define SUN8I_H3_THS_INTC_SHUT_INT_EN0 BIT(4) >> +#define SUN8I_H3_THS_INTC_TDATA_IRQ_EN0 BIT(8) >> > > Personal taste here but I prefer the register bits to be defined just > below the register address define. > > i.e.: > > #define SUN8I_H3_THS_CTRL2 0x40 > #define SUN8I_H3_THS_ACQ1(x) (GENMASK(31, 16) & ((x) << 16)) > #define SUN8I_H3_THS_TEMP_SENSE_EN0 BIT(0) > > that way we know for which register we should use which constants. > > Quentin > I agree, this the better way to do it. Thanks, Philipp -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> >>>> /* prevents concurrent reads of temperature and ADC */ >>>> struct mutex mutex; >>>> struct thermal_zone_device *tzd; >>>> @@ -561,6 +569,9 @@ static int sun4i_gpadc_probe_dt(struct platform_device *pdev, >>>> struct resource *mem; >>>> void __iomem *base; >>>> int ret; >>>> + struct nvmem_cell *cell; >>>> + ssize_t cell_size; >>>> + u64 *cell_data; >>>> info->data = of_device_get_match_data(&pdev->dev); >>>> if (!info->data) >>>> @@ -575,6 +586,39 @@ static int sun4i_gpadc_probe_dt(struct platform_device *pdev, >>>> if (IS_ERR(base)) >>>> return PTR_ERR(base); >>>> + info->has_calibration_data[0] = false; >>>> + info->has_calibration_data[1] = false; >>>> + >>>> + if (!info->data->supports_nvmem) >>>> + goto no_nvmem; >>>> + >>>> + cell = nvmem_cell_get(&pdev->dev, "calibration"); >>>> + if (IS_ERR(cell)) { >>>> + if (PTR_ERR(cell) == -EPROBE_DEFER) >>>> + return PTR_ERR(cell); >>>> + goto no_nvmem; >>> >>> goto considered evil ? :) >> >> this was a suggestion from Jonatan in version one, to make the code better >> readable. > > Isn't > > if (info->data->supports_nvmem && IS_ERR(cell = nvmem_cell_get())) > > pretty much the same thing? > > Maxime > I would say : if (info->data->supports_nvmem && !IS_ERR(cell = nvmem_cell_get())) is the same. This would require an else if statement like this: else if (info->data->supports_nvmem && PTR_ERR(cell) == -EPROBE_DEFER) return PTR_ERR(cell); to avoid errors if the thermal sensor is probed before the sid driver. Philipp -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 29, 2018 at 6:03 AM, Philipp Rossak <embed3d@gmail.com> wrote: > > > On 29.01.2018 10:52, Maxime Ripard wrote: >> >> On Mon, Jan 29, 2018 at 12:29:17AM +0100, Philipp Rossak wrote: >>> >>> This patch enables the the sid controller in the H3. It can be used >>> for thermal calibration data. >>> >>> Signed-off-by: Philipp Rossak <embed3d@gmail.com> >>> --- >>> arch/arm/boot/dts/sun8i-h3.dtsi | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi >>> b/arch/arm/boot/dts/sun8i-h3.dtsi >>> index 3f83f6a27c74..9bb5cc29fec5 100644 >>> --- a/arch/arm/boot/dts/sun8i-h3.dtsi >>> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi >>> @@ -72,6 +72,13 @@ >>> }; >>> }; >>> + soc { >>> + sid: eeprom@1c14000 { >>> + compatible = "allwinner,sun8i-h3-sid"; >>> + reg = <0x01c14000 0x400>; >>> + }; >>> + }; >>> + >> >> >> Shouldn't you also use a nvmem-cells property to the THS node? >> >> Maxime >> > > Oh seems like I forgot that. > As related to the wiki [1] this should be 64 bit wide at the address 0x34. I > will add that in the next version. > > > [1]: http://linux-sunxi.org/SID_Register_Guide#eFUSE > > Thanks, > Philipp > Hi, Any chance this will see a v3 soon? I'm kind of interested in sid node for h3. =) Thanks, Kyle Evans -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
于 2018年4月19日 GMT+08:00 下午11:11:22, Kyle Evans <kevans@freebsd.org> 写到: >On Mon, Jan 29, 2018 at 6:03 AM, Philipp Rossak <embed3d@gmail.com> >wrote: >> >> >> On 29.01.2018 10:52, Maxime Ripard wrote: >>> >>> On Mon, Jan 29, 2018 at 12:29:17AM +0100, Philipp Rossak wrote: >>>> >>>> This patch enables the the sid controller in the H3. It can be used >>>> for thermal calibration data. >>>> >>>> Signed-off-by: Philipp Rossak <embed3d@gmail.com> >>>> --- >>>> arch/arm/boot/dts/sun8i-h3.dtsi | 7 +++++++ >>>> 1 file changed, 7 insertions(+) >>>> >>>> diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi >>>> b/arch/arm/boot/dts/sun8i-h3.dtsi >>>> index 3f83f6a27c74..9bb5cc29fec5 100644 >>>> --- a/arch/arm/boot/dts/sun8i-h3.dtsi >>>> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi >>>> @@ -72,6 +72,13 @@ >>>> }; >>>> }; >>>> + soc { >>>> + sid: eeprom@1c14000 { >>>> + compatible = "allwinner,sun8i-h3-sid"; >>>> + reg = <0x01c14000 0x400>; >>>> + }; >>>> + }; >>>> + >>> >>> >>> Shouldn't you also use a nvmem-cells property to the THS node? >>> >>> Maxime >>> >> >> Oh seems like I forgot that. >> As related to the wiki [1] this should be 64 bit wide at the address >0x34. I >> will add that in the next version. >> >> >> [1]: http://linux-sunxi.org/SID_Register_Guide#eFUSE >> >> Thanks, >> Philipp >> > >Hi, > >Any chance this will see a v3 soon? I'm kind of interested in sid node >for h3. =) This patch is independent and can be easily sent out by its own. > >Thanks, > >Kyle Evans -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 19, 2018 at 10:13 AM, Icenowy Zheng <icenowy@aosc.io> wrote: > > > 于 2018年4月19日 GMT+08:00 下午11:11:22, Kyle Evans <kevans@freebsd.org> 写到: >>On Mon, Jan 29, 2018 at 6:03 AM, Philipp Rossak <embed3d@gmail.com> >>wrote: >>> >>> >>> On 29.01.2018 10:52, Maxime Ripard wrote: >>>> >>>> On Mon, Jan 29, 2018 at 12:29:17AM +0100, Philipp Rossak wrote: >>>>> >>>>> This patch enables the the sid controller in the H3. It can be used >>>>> for thermal calibration data. >>>>> >>>>> Signed-off-by: Philipp Rossak <embed3d@gmail.com> >>>>> --- >>>>> arch/arm/boot/dts/sun8i-h3.dtsi | 7 +++++++ >>>>> 1 file changed, 7 insertions(+) >>>>> >>>>> diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi >>>>> b/arch/arm/boot/dts/sun8i-h3.dtsi >>>>> index 3f83f6a27c74..9bb5cc29fec5 100644 >>>>> --- a/arch/arm/boot/dts/sun8i-h3.dtsi >>>>> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi >>>>> @@ -72,6 +72,13 @@ >>>>> }; >>>>> }; >>>>> + soc { >>>>> + sid: eeprom@1c14000 { >>>>> + compatible = "allwinner,sun8i-h3-sid"; >>>>> + reg = <0x01c14000 0x400>; >>>>> + }; >>>>> + }; >>>>> + >>>> >>>> >>>> Shouldn't you also use a nvmem-cells property to the THS node? >>>> >>>> Maxime >>>> >>> >>> Oh seems like I forgot that. >>> As related to the wiki [1] this should be 64 bit wide at the address >>0x34. I >>> will add that in the next version. >>> >>> >>> [1]: http://linux-sunxi.org/SID_Register_Guide#eFUSE >>> >>> Thanks, >>> Philipp >>> >> >>Hi, >> >>Any chance this will see a v3 soon? I'm kind of interested in sid node >>for h3. =) > > This patch is independent and can be easily sent out > by its own. > Right- I had considered doing so, but wanted to make sure I wasn't going to collide with this series if a v3 is imminent. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Kyle, I'm already working on a Version 3 of this patch series. Right now this slowed down since I'm very busy and the ToDo-List is still very long. My plan is to send out a version during this release cycle. If you need it right now feel free to submit patches! Philipp On 19.04.2018 17:19, Kyle Evans wrote: > On Thu, Apr 19, 2018 at 10:13 AM, Icenowy Zheng <icenowy@aosc.io> wrote: >> >> >> 于 2018年4月19日 GMT+08:00 下午11:11:22, Kyle Evans <kevans@freebsd.org> 写到: >>> On Mon, Jan 29, 2018 at 6:03 AM, Philipp Rossak <embed3d@gmail.com> >>> wrote: >>>> >>>> >>>> On 29.01.2018 10:52, Maxime Ripard wrote: >>>>> >>>>> On Mon, Jan 29, 2018 at 12:29:17AM +0100, Philipp Rossak wrote: >>>>>> >>>>>> This patch enables the the sid controller in the H3. It can be used >>>>>> for thermal calibration data. >>>>>> >>>>>> Signed-off-by: Philipp Rossak <embed3d@gmail.com> >>>>>> --- >>>>>> arch/arm/boot/dts/sun8i-h3.dtsi | 7 +++++++ >>>>>> 1 file changed, 7 insertions(+) >>>>>> >>>>>> diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi >>>>>> b/arch/arm/boot/dts/sun8i-h3.dtsi >>>>>> index 3f83f6a27c74..9bb5cc29fec5 100644 >>>>>> --- a/arch/arm/boot/dts/sun8i-h3.dtsi >>>>>> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi >>>>>> @@ -72,6 +72,13 @@ >>>>>> }; >>>>>> }; >>>>>> + soc { >>>>>> + sid: eeprom@1c14000 { >>>>>> + compatible = "allwinner,sun8i-h3-sid"; >>>>>> + reg = <0x01c14000 0x400>; >>>>>> + }; >>>>>> + }; >>>>>> + >>>>> >>>>> >>>>> Shouldn't you also use a nvmem-cells property to the THS node? >>>>> >>>>> Maxime >>>>> >>>> >>>> Oh seems like I forgot that. >>>> As related to the wiki [1] this should be 64 bit wide at the address >>> 0x34. I >>>> will add that in the next version. >>>> >>>> >>>> [1]: http://linux-sunxi.org/SID_Register_Guide#eFUSE >>>> >>>> Thanks, >>>> Philipp >>>> >>> >>> Hi, >>> >>> Any chance this will see a v3 soon? I'm kind of interested in sid node >>> for h3. =) >> >> This patch is independent and can be easily sent out >> by its own. >> > > Right- I had considered doing so, but wanted to make sure I wasn't > going to collide with this series if a v3 is imminent. > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Le vendredi 20 avril 2018 à 11:35 +0200, Philipp Rossak a écrit : > Hi Kyle, > > I'm already working on a Version 3 of this patch series. Right now this > slowed down since I'm very busy and the ToDo-List is still very long. > > My plan is to send out a version during this release cycle. > > If you need it right now feel free to submit patches! I just came around this patch while testing SID support for H3, that is not enabled in the dts at this point despite driver support. Maxime, would it make sense to merge this patch as-is, without support for the thermal calibration section, since the two aspects seem rather independent (and some other SoCs also have a SID node without linkage for thermal support anyway)? Cheers, Paul > Philipp > > On 19.04.2018 17:19, Kyle Evans wrote: > > On Thu, Apr 19, 2018 at 10:13 AM, Icenowy Zheng <icenowy@aosc.io> wrote: > > > > > > > > > 于 2018年4月19日 GMT+08:00 下午11:11:22, Kyle Evans <kevans@freebsd.org> 写到: > > > > On Mon, Jan 29, 2018 at 6:03 AM, Philipp Rossak <embed3d@gmail.com> > > > > wrote: > > > > > > > > > > > > > > > On 29.01.2018 10:52, Maxime Ripard wrote: > > > > > > > > > > > > On Mon, Jan 29, 2018 at 12:29:17AM +0100, Philipp Rossak wrote: > > > > > > > > > > > > > > This patch enables the the sid controller in the H3. It can be used > > > > > > > for thermal calibration data. > > > > > > > > > > > > > > Signed-off-by: Philipp Rossak <embed3d@gmail.com> > > > > > > > --- > > > > > > > arch/arm/boot/dts/sun8i-h3.dtsi | 7 +++++++ > > > > > > > 1 file changed, 7 insertions(+) > > > > > > > > > > > > > > diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi > > > > > > > b/arch/arm/boot/dts/sun8i-h3.dtsi > > > > > > > index 3f83f6a27c74..9bb5cc29fec5 100644 > > > > > > > --- a/arch/arm/boot/dts/sun8i-h3.dtsi > > > > > > > +++ b/arch/arm/boot/dts/sun8i-h3.dtsi > > > > > > > @@ -72,6 +72,13 @@ > > > > > > > }; > > > > > > > }; > > > > > > > + soc { > > > > > > > + sid: eeprom@1c14000 { > > > > > > > + compatible = "allwinner,sun8i-h3-sid"; > > > > > > > + reg = <0x01c14000 0x400>; > > > > > > > + }; > > > > > > > + }; > > > > > > > + > > > > > > > > > > > > > > > > > > Shouldn't you also use a nvmem-cells property to the THS node? > > > > > > > > > > > > Maxime > > > > > > > > > > > > > > > > Oh seems like I forgot that. > > > > > As related to the wiki [1] this should be 64 bit wide at the address > > > > > > > > 0x34. I > > > > > will add that in the next version. > > > > > > > > > > > > > > > [1]: http://linux-sunxi.org/SID_Register_Guide#eFUSE > > > > > > > > > > Thanks, > > > > > Philipp > > > > > > > > > > > > > Hi, > > > > > > > > Any chance this will see a v3 soon? I'm kind of interested in sid node > > > > for h3. =) > > > > > > This patch is independent and can be easily sent out > > > by its own. > > > > > > > Right- I had considered doing so, but wanted to make sure I wasn't > > going to collide with this series if a v3 is imminent. > > > >
On Tue, 24 Jul 2018 19:19:54 +0200 Paul Kocialkowski <contact@paulk.fr> wrote: > Hi, > > Le vendredi 20 avril 2018 à 11:35 +0200, Philipp Rossak a écrit : > > Hi Kyle, > > > > I'm already working on a Version 3 of this patch series. Right now this > > slowed down since I'm very busy and the ToDo-List is still very long. > > > > My plan is to send out a version during this release cycle. > > > > If you need it right now feel free to submit patches! > > I just came around this patch while testing SID support for H3, that is > not enabled in the dts at this point despite driver support. > > Maxime, would it make sense to merge this patch as-is, without support > for the thermal calibration section, since the two aspects seem rather > independent (and some other SoCs also have a SID node without linkage > for thermal support anyway)? > > Cheers, > > Paul Hello Paul, I've sent a serie yesterday for SID on A64/H3/H5. https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=761 Cheers, > > Philipp > > > > On 19.04.2018 17:19, Kyle Evans wrote: > > > On Thu, Apr 19, 2018 at 10:13 AM, Icenowy Zheng <icenowy@aosc.io> wrote: > > > > > > > > > > > > ? 2018?4?19? GMT+08:00 ??11:11:22, Kyle Evans <kevans@freebsd.org> ??: > > > > > On Mon, Jan 29, 2018 at 6:03 AM, Philipp Rossak <embed3d@gmail.com> > > > > > wrote: > > > > > > > > > > > > > > > > > > On 29.01.2018 10:52, Maxime Ripard wrote: > > > > > > > > > > > > > > On Mon, Jan 29, 2018 at 12:29:17AM +0100, Philipp Rossak wrote: > > > > > > > > > > > > > > > > This patch enables the the sid controller in the H3. It can be used > > > > > > > > for thermal calibration data. > > > > > > > > > > > > > > > > Signed-off-by: Philipp Rossak <embed3d@gmail.com> > > > > > > > > --- > > > > > > > > arch/arm/boot/dts/sun8i-h3.dtsi | 7 +++++++ > > > > > > > > 1 file changed, 7 insertions(+) > > > > > > > > > > > > > > > > diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi > > > > > > > > b/arch/arm/boot/dts/sun8i-h3.dtsi > > > > > > > > index 3f83f6a27c74..9bb5cc29fec5 100644 > > > > > > > > --- a/arch/arm/boot/dts/sun8i-h3.dtsi > > > > > > > > +++ b/arch/arm/boot/dts/sun8i-h3.dtsi > > > > > > > > @@ -72,6 +72,13 @@ > > > > > > > > }; > > > > > > > > }; > > > > > > > > + soc { > > > > > > > > + sid: eeprom@1c14000 { > > > > > > > > + compatible = "allwinner,sun8i-h3-sid"; > > > > > > > > + reg = <0x01c14000 0x400>; > > > > > > > > + }; > > > > > > > > + }; > > > > > > > > + > > > > > > > > > > > > > > > > > > > > > Shouldn't you also use a nvmem-cells property to the THS node? > > > > > > > > > > > > > > Maxime > > > > > > > > > > > > > > > > > > > Oh seems like I forgot that. > > > > > > As related to the wiki [1] this should be 64 bit wide at the address > > > > > > > > > > 0x34. I > > > > > > will add that in the next version. > > > > > > > > > > > > > > > > > > [1]: http://linux-sunxi.org/SID_Register_Guide#eFUSE > > > > > > > > > > > > Thanks, > > > > > > Philipp > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > Any chance this will see a v3 soon? I'm kind of interested in sid node > > > > > for h3. =) > > > > > > > > This patch is independent and can be easily sent out > > > > by its own. > > > > > > > > > > Right- I had considered doing so, but wanted to make sure I wasn't > > > going to collide with this series if a v3 is imminent. > > > > > > > > -- > Developer of free digital technology and hardware support. > > Website: https://www.paulk.fr/ > Coding blog: https://code.paulk.fr/ > Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/
Hi, On Wed, 2018-07-25 at 11:05 +0200, Emmanuel Vadot wrote: [...] > Hello Paul, > > I've sent a serie yesterday for SID on A64/H3/H5. > https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=761 Awesome, thanks for taking care of that :) Cheers, Paul > > > On 19.04.2018 17:19, Kyle Evans wrote: > > > > On Thu, Apr 19, 2018 at 10:13 AM, Icenowy Zheng <icenowy@aosc.io> wrote: > > > > > > > > > > > > > > > ? 2018?4?19? GMT+08:00 ??11:11:22, Kyle Evans <kevans@freebsd.org> ??: > > > > > > On Mon, Jan 29, 2018 at 6:03 AM, Philipp Rossak <embed3d@gmail.com> > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > On 29.01.2018 10:52, Maxime Ripard wrote: > > > > > > > > > > > > > > > > On Mon, Jan 29, 2018 at 12:29:17AM +0100, Philipp Rossak wrote: > > > > > > > > > > > > > > > > > > This patch enables the the sid controller in the H3. It can be used > > > > > > > > > for thermal calibration data. > > > > > > > > > > > > > > > > > > Signed-off-by: Philipp Rossak <embed3d@gmail.com> > > > > > > > > > --- > > > > > > > > > arch/arm/boot/dts/sun8i-h3.dtsi | 7 +++++++ > > > > > > > > > 1 file changed, 7 insertions(+) > > > > > > > > > > > > > > > > > > diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi > > > > > > > > > b/arch/arm/boot/dts/sun8i-h3.dtsi > > > > > > > > > index 3f83f6a27c74..9bb5cc29fec5 100644 > > > > > > > > > --- a/arch/arm/boot/dts/sun8i-h3.dtsi > > > > > > > > > +++ b/arch/arm/boot/dts/sun8i-h3.dtsi > > > > > > > > > @@ -72,6 +72,13 @@ > > > > > > > > > }; > > > > > > > > > }; > > > > > > > > > + soc { > > > > > > > > > + sid: eeprom@1c14000 { > > > > > > > > > + compatible = "allwinner,sun8i-h3-sid"; > > > > > > > > > + reg = <0x01c14000 0x400>; > > > > > > > > > + }; > > > > > > > > > + }; > > > > > > > > > + > > > > > > > > > > > > > > > > > > > > > > > > Shouldn't you also use a nvmem-cells property to the THS node? > > > > > > > > > > > > > > > > Maxime > > > > > > > > > > > > > > > > > > > > > > Oh seems like I forgot that. > > > > > > > As related to the wiki [1] this should be 64 bit wide at the address > > > > > > > > > > > > 0x34. I > > > > > > > will add that in the next version. > > > > > > > > > > > > > > > > > > > > > [1]: http://linux-sunxi.org/SID_Register_Guide#eFUSE > > > > > > > > > > > > > > Thanks, > > > > > > > Philipp > > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > Any chance this will see a v3 soon? I'm kind of interested in sid node > > > > > > for h3. =) > > > > > > > > > > This patch is independent and can be easily sent out > > > > > by its own. > > > > > > > > > > > > > Right- I had considered doing so, but wanted to make sure I wasn't > > > > going to collide with this series if a v3 is imminent. > > > > > > > > > > > > > > -- > > Developer of free digital technology and hardware support. > > > > Website: https://www.paulk.fr/ > > Coding blog: https://code.paulk.fr/ > > Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/ > >
Allwiner H3 and A83T SoCs have a thermal sensor, which is a large refactored version of the old Allwinner "GPADC" (although it have already only thermal part left in A33). This patch tried to add support for the sensor in H3 and A83T based on the A33 thermal sensor driver by Quentin Schulz, which is already merged. This Patchseries is based on Icenowy Zhengs v4 patchseries [1]. The first 5 patches are reworked patches from the v4 patchseries. The rest of the patches add step by step a feature to support multible sensors, nvmem calibration and interupts. This patchseries should make it easy also to add other sunxi SoCs, like the H5, A64 and A80. Patches that adds support for H5, A64 and A80 SoCs are allready prepared, and will be upstreamed if this patchseries is applied and the testing is done. I tried to pick up all the feedback from [1]. I hope I didn't miss anything. Regards, Philipp @Jonathan Could you please check Patch 8 again. I moved some code from Patch 8 to 9. Please chek it again, if your still ok with it. But I think it sould be ok. changes since v1: * collecting all acks * rewording commits/fix typos * move code in place where it is used * fix naming conventions of defines * clarify commits * update documentation to cover the new nvmem calibraion * change nvmem calibration Icenowy Zheng (1): iio: adc: sun4i-gpadc-iio: rename A33-specified registers to contain A33 Philipp Rossak (15): dt-bindings: update the Allwinner GPADC device tree binding for H3 & A83T arm: config: sunxi_defconfig: enable SUN4I_GPADC iio: adc: sun4i-gpadc-iio: rework: sampling start/end code readout reg iio: adc: sun4i-gpadc-iio: rework: support clocks and reset iio: adc: sun4i-gpadc-iio: rework: support multiple sensors iio: adc: sun4i-gpadc-iio: rework: support nvmem calibration data iio: adc: sun4i-gpadc-iio: rework: add interrupt support iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor iio: adc: sun4i-gpadc-iio: add support for A83T thermal sensor arm: dts: sunxi-h3-h5: add support for the thermal sensor in H3 and H5 arm: dts: sun8i: h3: add support for the thermal sensor in H3 arm: dts: sun8i: h3: add thermal zone to H3 arm: dts: sun8i: h3: enable H3 sid controller arm: dts: sun8i: a83t: add support for the thermal sensor in A83T arm: dts: sun8i: a83t: add thermal zone to A83T .../devicetree/bindings/mfd/sun4i-gpadc.txt | 50 ++- arch/arm/boot/dts/sun8i-a83t.dtsi | 28 ++ arch/arm/boot/dts/sun8i-h3.dtsi | 21 ++ arch/arm/boot/dts/sunxi-h3-h5.dtsi | 9 + arch/arm/configs/sunxi_defconfig | 1 + drivers/iio/adc/sun4i-gpadc-iio.c | 367 ++++++++++++++++++++- include/linux/mfd/sun4i-gpadc.h | 51 ++- 7 files changed, 503 insertions(+), 24 deletions(-)