| Message ID | 20180830154518.29507-1-embed3d@gmail.com |
|---|---|
| Headers | show |
| Series | IIO-based thermal sensor driver for Allwinner H3 and A83T SoC | expand |
Hello, On Thu, Aug 30, 2018 at 05:45:09PM +0200, 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 | 91 +++++++++++++++++++++++++++++++++++++ > include/linux/iio/adc/sun4i-gpadc.h | 18 ++++++++ > 2 files changed, 109 insertions(+) > > diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c > index c7b46c82e3e5..d5c7971b2558 100644 > --- a/drivers/iio/adc/sun4i-gpadc-iio.c > +++ b/drivers/iio/adc/sun4i-gpadc-iio.c > @@ -72,6 +72,7 @@ struct gpadc_data { > u32 temp_data_base; > int sensor_count; > bool supports_nvmem; > + u32 ths_irq_clear; > }; > > static irqreturn_t sun4i_gpadc_data_irq_handler(int irq, void *dev_id); > @@ -79,6 +80,10 @@ static irqreturn_t sun4i_gpadc_data_irq_handler(int irq, void *dev_id); > static int sun4i_ths_resume(struct sun4i_gpadc_iio *info); > static int sun4i_ths_suspend(struct sun4i_gpadc_iio *info); > > +static int sun8i_h3_ths_resume(struct sun4i_gpadc_iio *info); > +static int sun8i_h3_ths_suspend(struct sun4i_gpadc_iio *info); > +static irqreturn_t sunx8i_h3_irq_thread(int irq, void *data); > + > static const struct gpadc_data sun4i_gpadc_data = { > .temp_offset = -1932, > .temp_scale = 133, > @@ -137,6 +142,22 @@ static const struct gpadc_data sun8i_a33_gpadc_data = { > .sensor_count = 1, > }; > > +static const struct gpadc_data sun8i_h3_ths_data = { > + .temp_offset = -1791, > + .temp_scale = -121, > + .temp_data_base = SUN8I_H3_THS_TDATA0, > + .ths_irq_thread = sunx8i_h3_irq_thread, > + .support_irq = true, > + .has_bus_clk = true, > + .has_bus_rst = true, > + .has_mod_clk = true, > + .sensor_count = 1, > + .supports_nvmem = true, > + .ths_resume = sun8i_h3_ths_resume, > + .ths_suspend = sun8i_h3_ths_suspend, > + .ths_irq_clear = SUN8I_H3_THS_INTS_TDATA_IRQ_0, > +}; > + > struct sun4i_sensor_tzd { > struct sun4i_gpadc_iio *info; > struct thermal_zone_device *tzd; > @@ -409,6 +430,31 @@ static irqreturn_t sun4i_gpadc_data_irq_handler(int irq, void *dev_id) > return IRQ_HANDLED; > } > > +static irqreturn_t sunx8i_h3_irq_thread(int irq, void *data) > +{ > + struct sun4i_gpadc_iio *info = data; > + int i; > + > + regmap_write(info->regmap, SUN8I_H3_THS_STAT, > + info->data->ths_irq_clear); > + > + for (i = 0; i < info->data->sensor_count; i++) > + thermal_zone_device_update(info->tzds[i].tzd, > + THERMAL_EVENT_TEMP_SAMPLE); > + > + return IRQ_HANDLED; > +} > + > +static int sun8i_h3_calibrate(struct sun4i_gpadc_iio *info) > +{ > +// regmap_write(info->regmap, SUNXI_THS_CDATA_0_1, > +// info->calibration_data[0]); > +// regmap_write(info->regmap, SUNXI_THS_CDATA_2_3, > +// info->calibration_data[1]); This should probably be implemented, or left out completely. regards, o. > + return 0; > +} > + > static int sun4i_gpadc_runtime_suspend(struct device *dev) > { > struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev)); > @@ -428,6 +474,16 @@ static int sun4i_ths_suspend(struct sun4i_gpadc_iio *info) > return 0; > } > > +static int sun8i_h3_ths_suspend(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_resume(struct device *dev) > { > struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev)); > @@ -454,6 +510,37 @@ static int sun4i_ths_resume(struct sun4i_gpadc_iio *info) > return 0; > } > > +static int sun8i_h3_ths_resume(struct sun4i_gpadc_iio *info) > +{ > + u32 value; > + > + sun8i_h3_calibrate(info); > + > + regmap_write(info->regmap, SUN8I_H3_THS_CTRL0, > + SUN4I_GPADC_CTRL0_T_ACQ(0xff)); > + > + regmap_write(info->regmap, SUN8I_H3_THS_CTRL2, > + SUN8I_H3_THS_ACQ1(0x3f)); > + > + regmap_write(info->regmap, SUN8I_H3_THS_STAT, > + SUN8I_H3_THS_INTS_TDATA_IRQ_0); > + > + regmap_write(info->regmap, SUN8I_H3_THS_FILTER, > + SUN4I_GPADC_CTRL3_FILTER_EN | > + SUN4I_GPADC_CTRL3_FILTER_TYPE(0x2)); > + > + regmap_write(info->regmap, SUN8I_H3_THS_INTC, > + SUN8I_H3_THS_INTC_TDATA_IRQ_EN0 | > + SUN8I_H3_THS_TEMP_PERIOD(0x55)); > + > + regmap_read(info->regmap, SUN8I_H3_THS_CTRL2, &value); > + > + regmap_write(info->regmap, SUN8I_H3_THS_CTRL2, > + SUN8I_H3_THS_TEMP_SENSE_EN0 | value); > + > + return 0; > +} > + > static int sun4i_gpadc_get_temp(void *data, int *temp) > { > struct sun4i_sensor_tzd *tzd = data; > @@ -497,6 +584,10 @@ static const struct of_device_id sun4i_gpadc_of_id[] = { > .compatible = "allwinner,sun6i-a31-gpadc", > .data = &sun6i_gpadc_data > }, > + { > + .compatible = "allwinner,sun8i-h3-ths", > + .data = &sun8i_h3_ths_data, > + }, > { /* sentinel */ } > }; > > diff --git a/include/linux/iio/adc/sun4i-gpadc.h b/include/linux/iio/adc/sun4i-gpadc.h > index 99feeba28105..169b4de9a34d 100644 > --- a/include/linux/iio/adc/sun4i-gpadc.h > +++ b/include/linux/iio/adc/sun4i-gpadc.h > @@ -100,6 +100,24 @@ > } > > /* SUNXI_THS COMMON REGISTERS + DEFINES */ > +#define SUN8I_H3_THS_CTRL0 0x00 > + > +#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) > + > +#define SUN8I_H3_THS_INTC 0x44 > +#define SUN8I_H3_THS_TEMP_PERIOD(x) (GENMASK(31, 12) & ((x) << 12)) > +#define SUN8I_H3_THS_INTC_TDATA_IRQ_EN0 BIT(8) > + > +#define SUN8I_H3_THS_STAT 0x48 > +#define SUN8I_H3_THS_INTS_TDATA_IRQ_0 BIT(8) > + > +#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 MAX_SENSOR_COUNT 4 > > #endif > -- > 2.11.0 > > -- > You received this message because you are subscribed to the Google Groups "linux-sunxi" group. > To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout.
Hello, On Thu, Aug 30, 2018 at 05:45:18PM +0200, Philipp Rossak wrote: > Since we have now thermal trotteling enabeled we can now add the full > range of the OPP table. I'm not sure we can. I have a tablet with A83T SoC and it gets unstable at these frequencies even with thermal throttling on mainline kernel. (Though I have my own THS driver, but I doubt a different driver will change much.) There might be some other issue left in the cpufreq code. I'll let others test this on a better cooled boards though. Did you/someone test this? regards, o. > The operating points were found in Allwinner BSP and fex files. > > Signed-off-by: Philipp Rossak <embed3d@gmail.com> > --- > arch/arm/boot/dts/sun8i-a83t.dtsi | 32 ++++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/arch/arm/boot/dts/sun8i-a83t.dtsi b/arch/arm/boot/dts/sun8i-a83t.dtsi > index 78aa448e869f..ddcf404f9c80 100644 > --- a/arch/arm/boot/dts/sun8i-a83t.dtsi > +++ b/arch/arm/boot/dts/sun8i-a83t.dtsi > @@ -250,6 +250,22 @@ > opp-microvolt = <840000>; > clock-latency-ns = <244144>; /* 8 32k periods */ > }; > + > + opp-1608000000 { > + opp-hz = /bits/ 64 <1608000000>; > + opp-microvolt = <920000>; > + clock-latency-ns = <244144>; /* 8 32k periods */ > + }; > + opp-1800000000 { /* BOOT FREQ */ > + opp-hz = /bits/ 64 <1800000000>; > + opp-microvolt = <1000000>; > + clock-latency-ns = <244144>; /* 8 32k periods */ > + }; > + opp-2016000000 { > + opp-hz = /bits/ 64 <2016000000>; > + opp-microvolt = <1080000>; > + clock-latency-ns = <244144>; /* 8 32k periods */ > + }; > }; > > cpu1_opp_table: opp_table1 { > @@ -303,6 +319,22 @@ > opp-microvolt = <840000>; > clock-latency-ns = <244144>; /* 8 32k periods */ > }; > + > + opp-1608000000 { > + opp-hz = /bits/ 64 <1608000000>; > + opp-microvolt = <920000>; > + clock-latency-ns = <244144>; /* 8 32k periods */ > + }; > + opp-1800000000 { /* BOOT FREQ */ > + opp-hz = /bits/ 64 <1800000000>; > + opp-microvolt = <1000000>; > + clock-latency-ns = <244144>; /* 8 32k periods */ > + }; > + opp-2016000000 { > + opp-hz = /bits/ 64 <2016000000>; > + opp-microvolt = <1080000>; > + clock-latency-ns = <244144>; /* 8 32k periods */ > + }; > }; > > soc { > -- > 2.11.0 > > -- > You received this message because you are subscribed to the Google Groups "linux-sunxi" group. > To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout.
On Thu, Aug 30, 2018 at 05:44:57PM +0200, Philipp Rossak wrote: > We want to use this driver mostly as thermal sensor, that still supports > the adc for the older chips, thus we threat the A33 as thermal sensor. > We also remove the adc channel without thermal support. Threat -> treat (in the title and in the message body too) > Signed-off-by: Philipp Rossak <embed3d@gmail.com> > --- > drivers/iio/adc/sun4i-gpadc-iio.c | 19 ------------------- > 1 file changed, 19 deletions(-) > > diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c > index ab474ce86fb6..658a7e3e3370 100644 > --- a/drivers/iio/adc/sun4i-gpadc-iio.c > +++ b/drivers/iio/adc/sun4i-gpadc-iio.c > @@ -123,23 +123,6 @@ static const struct iio_chan_spec sun4i_gpadc_channels[] = { > }, > }; > > -static const struct iio_chan_spec sun4i_gpadc_channels_no_temp[] = { > - SUN4I_GPADC_ADC_CHANNEL(0, "adc_chan0"), > - SUN4I_GPADC_ADC_CHANNEL(1, "adc_chan1"), > - SUN4I_GPADC_ADC_CHANNEL(2, "adc_chan2"), > - SUN4I_GPADC_ADC_CHANNEL(3, "adc_chan3"), > -}; > - > -static const struct iio_chan_spec sun8i_a33_gpadc_channels[] = { > - { > - .type = IIO_TEMP, > - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > - BIT(IIO_CHAN_INFO_SCALE) | > - BIT(IIO_CHAN_INFO_OFFSET), > - .datasheet_name = "temp_adc", > - }, > -}; > - > static const struct regmap_config sun4i_gpadc_regmap_config = { > .reg_bits = 32, > .val_bits = 32, > @@ -444,8 +427,6 @@ static int sun4i_gpadc_probe_dt(struct platform_device *pdev, > return -ENODEV; > > info->no_irq = true; > - indio_dev->num_channels = ARRAY_SIZE(sun8i_a33_gpadc_channels); > - indio_dev->channels = sun8i_a33_gpadc_channels; > > mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > base = devm_ioremap_resource(&pdev->dev, mem); > -- > 2.11.0 > > -- > You received this message because you are subscribed to the Google Groups "linux-sunxi" group. > To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout.
On 30.08.2018 18:27, Ondřej Jirman wrote: >> +static int sun8i_h3_calibrate(struct sun4i_gpadc_iio *info) >> +{ >> +// regmap_write(info->regmap, SUNXI_THS_CDATA_0_1, >> +// info->calibration_data[0]); >> +// regmap_write(info->regmap, SUNXI_THS_CDATA_2_3, >> +// info->calibration_data[1]); > This should probably be implemented, or left out completely. > > regards, > o. > Thanks you are right! This should be implemented! I will fix this in the next version! Thanks, Philipp
On 30.08.2018 18:38, Ondřej Jirman wrote: > Hello, > > On Thu, Aug 30, 2018 at 05:45:18PM +0200, Philipp Rossak wrote: >> Since we have now thermal trotteling enabeled we can now add the full >> range of the OPP table. > > I'm not sure we can. I have a tablet with A83T SoC and it gets unstable > at these frequencies even with thermal throttling on mainline kernel. (Though > I have my own THS driver, but I doubt a different driver will change much.) > > There might be some other issue left in the cpufreq code. I'll let others > test this on a better cooled boards though. > > Did you/someone test this? > > regards, > o. I have a good cooled device, with big heatsinks and a fan blowing directly on it. But there is some big issue left! cat /sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_max_freq [ 85.076270] Unable to handle kernel paging request at virtual address 2e83c684 [ 85.083519] pgd = (ptrval) [ 85.086220] [2e83c684] *pgd=00000000 [ 85.089813] Internal error: Oops: 5 [#3] SMP ARM [ 85.094429] Modules linked in: [ 85.097483] CPU: 4 PID: 127 Comm: sh Tainted: G D W 4.18.0-00031-g8f59917020b9-dirty #2 [ 85.106597] Hardware name: Allwinner A83t board [ 85.111130] PC is at down_write+0x14/0x54 [ 85.115135] LR is at anon_vma_clone+0x9c/0x1e4 [ 85.119571] pc : [<c066a3d4>] lr : [<c01e71b8>] psr: 60000013 [ 85.125826] sp : ede45e70 ip : 000001a0 fp : eea3c690 [ 85.131041] r10: eea4193c r9 : 00400200 r8 : c0a942a4 [ 85.136255] r7 : 2e83c680 r6 : eea3aea0 r5 : eea3b220 r4 : 2e83c684 [ 85.142771] r3 : ffff0001 r2 : 2e83c680 r1 : ede45e58 r0 : 2e83c684 [ 85.149287] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none [ 85.156410] Control: 10c5387d Table: 6df1806a DAC: 00000051 [ 85.162145] Process sh (pid: 127, stack limit = 0x(ptrval)) [ 85.167706] Stack: (0xede45e70 to 0xede46000) [ 85.172057] 5e60: eea3b900 c01e71b8 eea41900 ffebd684 [ 85.180221] 5e80: c0a637b0 c07ea07c c0a10754 eea3aea0 eea41900 c0a04c48 edd71880 00000000 [ 85.188385] 5ea0: eea3aea0 eea41900 c0a69030 c01e7324 00000002 edd701c0 c0a04c48 edd71880 [ 85.196548] 5ec0: 00000000 c011c2f4 00000069 00000000 00000002 00000000 00000000 00000000 [ 85.204711] 5ee0: 00000000 edd701c0 edd82280 eea3af00 edd701f8 edd718b8 eea3af08 eea3af14 [ 85.212875] 5f00: eea3af10 ede44000 edd8255c 01200011 00000000 ede45f14 ede45f14 b61ea11a [ 85.221039] 5f20: edd805c0 01200011 c0a04c48 beef38b0 00000000 00000000 00000000 00000078 [ 85.229202] 5f40: beef38dc c011ce0c 00000000 00000000 00000000 ede44000 000000ae c012d9e0 [ 85.237365] 5f60: eea43480 00000000 04000000 b61ea11a 00000000 b6fb5068 b6f8b670 beef38b0 [ 85.245529] 5f80: 00000078 c0101204 ede44000 00000078 beef38dc c011d1bc b6fb5068 00000000 [ 85.253692] 5fa0: 000000ae c0101000 b6fb5068 b6f8b670 01200011 00000000 00000000 00000000 [ 85.261856] 5fc0: b6fb5068 b6f8b670 beef38b0 00000078 b6f8ac4c b6fb5490 ffffffff beef38dc [ 85.270020] 5fe0: 00000002 beef38b0 00000000 b6f5bfd0 60000010 01200011 00000000 00000000 [ 85.278191] [<c066a3d4>] (down_write) from [<c01e71b8>] (anon_vma_clone+0x9c/0x1e4) [ 85.285836] [<c01e71b8>] (anon_vma_clone) from [<c01e7324>] (anon_vma_fork+0x24/0x160) [ 85.293741] [<c01e7324>] (anon_vma_fork) from [<c011c2f4>] (copy_process.part.3+0xbc4/0x158c) [ 85.302253] [<c011c2f4>] (copy_process.part.3) from [<c011ce0c>] (_do_fork+0xb0/0x394) [ 85.310157] [<c011ce0c>] (_do_fork) from [<c011d1bc>] (sys_clone+0x20/0x28) [ 85.317107] [<c011d1bc>] (sys_clone) from [<c0101000>] (ret_fast_syscall+0x0/0x54) [ 85.324661] Exception stack(0xede45fa8 to 0xede45ff0) [ 85.329704] 5fa0: b6fb5068 b6f8b670 01200011 00000000 00000000 00000000 [ 85.337868] 5fc0: b6fb5068 b6f8b670 beef38b0 00000078 b6f8ac4c b6fb5490 ffffffff beef38dc [ 85.346021] 5fe0: 00000002 beef38b0 00000000 b6f5bfd0 [ 85.351068] Code: e1a04000 f590f000 e3a03001 e34f3fff (e1902f9f) [ 85.357200] ---[ end trace aad10e0b4fcbf194 ]--- OR cat /sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_cur_freq [ 73.873939] ------------[ cut here ]------------ [ 73.878584] WARNING: CPU: 5 PID: 132 at mm/rmap.c:235 unlink_anon_vmas+0x1f4/0x1fc [ 73.886210] Modules linked in: [ 73.889276] CPU: 5 PID: 132 Comm: sh Tainted: G D 4.18.0-00031-g8f59917020b9-dirty #2 [ 73.898391] Hardware name: Allwinner A83t board [ 73.902934] [<c010f310>] (unwind_backtrace) from [<c010bd54>] (show_stack+0x10/0x14) [ 73.910671] [<c010bd54>] (show_stack) from [<c0652f98>] (dump_stack+0x84/0x98) [ 73.917887] [<c0652f98>] (dump_stack) from [<c011dd14>] (__warn+0xfc/0x114) [ 73.924840] [<c011dd14>] (__warn) from [<c011de44>] (warn_slowpath_null+0x40/0x48) [ 73.932398] [<c011de44>] (warn_slowpath_null) from [<c01e7114>] (unlink_anon_vmas+0x1f4/0x1fc) [ 73.941006] [<c01e7114>] (unlink_anon_vmas) from [<c01da874>] (free_pgtables+0x78/0xcc) [ 73.948999] [<c01da874>] (free_pgtables) from [<c01e1e88>] (exit_mmap+0xe4/0x188) [ 73.956471] [<c01e1e88>] (exit_mmap) from [<c011b3a8>] (mmput+0x40/0xf0) [ 73.963169] [<c011b3a8>] (mmput) from [<c0208f1c>] (flush_old_exec+0x550/0x6e0) [ 73.970473] [<c0208f1c>] (flush_old_exec) from [<c0250e74>] (load_elf_binary+0x2f0/0x1324) [ 73.978726] [<c0250e74>] (load_elf_binary) from [<c02086c4>] (search_binary_handler+0xa0/0x234) [ 73.987412] [<c02086c4>] (search_binary_handler) from [<c02098b0>] (__do_execve_file+0x57c/0x6bc) [ 73.996271] [<c02098b0>] (__do_execve_file) from [<c0209c54>] (sys_execve+0x34/0x3c) [ 74.004003] [<c0209c54>] (sys_execve) from [<c0101000>] (ret_fast_syscall+0x0/0x54) [ 74.011645] Exception stack(0xede71fa8 to 0xede71ff0) [ 74.016690] 1fa0: 000c530c 000c52d0 000c530c 000c52d0 000c52dc 00000000 [ 74.024853] 1fc0: 000c530c 000c52d0 000a2a0c 0000000b 000c52dc 000c4b5c 000c4b5c 000c530c [ 74.033016] 1fe0: b6f25ed4 beef38b8 00042038 b6f25ee0 [ 74.038086] ---[ end trace aad10e0b4fcbf192 ]--- [ 74.042726] Unable to handle kernel paging request at virtual address 2e83c684 [ 74.049958] pgd = (ptrval) [ 74.052665] [2e83c684] *pgd=00000000 [ 74.056243] Internal error: Oops: 5 [#2] SMP ARM [ 74.060851] Modules linked in: [ 74.063904] CPU: 5 PID: 132 Comm: sh Tainted: G D W 4.18.0-00031-g8f59917020b9-dirty #2 [ 74.073019] Hardware name: Allwinner A83t board [ 74.077548] PC is at down_write+0x14/0x54 [ 74.081553] LR is at unlink_anon_vmas+0xa8/0x1fc [ 74.086160] pc : [<c066a3d4>] lr : [<c01e6fc8>] psr: 60000013 [ 74.092416] sp : ede71d88 ip : 00000000 fp : eea3b680 [ 74.097630] r10: eea3c690 r9 : c0a637b0 r8 : eea41e40 [ 74.102845] r7 : c0a942a4 r6 : 2e83c680 r5 : eea41e7c r4 : 2e83c684 [ 74.109360] r3 : ffff0001 r2 : ffff0001 r1 : 00000000 r0 : 2e83c684 [ 74.115878] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none [ 74.123000] Control: 10c5387d Table: 6de6406a DAC: 00000051 [ 74.128735] Process sh (pid: 132, stack limit = 0x(ptrval)) [ 74.134296] Stack: (0xede71d88 to 0xede72000) [ 74.138645] 1d80: eea41e74 c01e6fc8 c07ea07c eea3c690 eea41f00 eea41e40 [ 74.146809] 1da0: eea41d20 b6f17000 ede71df4 00002000 00000000 edd9c300 ede71e90 c01da874 [ 74.154973] 1dc0: b6f17000 c01dbf94 00000000 eea41360 00000000 c0a04c48 edd716c0 edd826b0 [ 74.163136] 1de0: edd90540 c01e1e88 eeabc6c8 00000001 00000000 edd716c0 00000001 00000000 [ 74.171299] 1e00: 00000000 ffffffff c0a68f40 00000000 000000ac 00000400 eeacf000 eeb9f038 [ 74.179463] 1e20: 00000100 b61ea11a 00000000 edd716c0 00000000 edda8540 edd716c0 b61ea11a [ 74.187627] 1e40: edd716c0 00000000 edda8540 c011b3a8 edd716c0 edd82280 edda8540 c0208f1c [ 74.195790] 1e60: 000000a0 c024f610 000000d4 c0a04c48 eeab4100 00000000 edd9c300 eeab4134 [ 74.203954] 1e80: eeabc6c0 edd9c400 ede71e90 c0250e74 effcec80 effcec80 00000001 eeabc6c0 [ 74.212118] 1ea0: eeaab180 eeabb000 00000000 c01d9704 effcec80 80000013 beffff22 00000000 [ 74.220281] 1ec0: ede70000 effcec80 edd9c300 c01d9344 00000000 00000004 beffff22 00000000 [ 74.228445] 1ee0: 00000034 00000000 00000011 ede71f20 00000000 00000000 00000051 b61ea11a [ 74.236609] 1f00: befff000 c0a1130c edd9c300 c0a9558c fffffff8 c0a10e3c c0a9558c c07ebbb4 [ 74.244772] 1f20: 00000001 c02086c4 c0a0bbd4 c0a04c48 edda2000 ffffe000 00000084 edd9c300 [ 74.252936] 1f40: 00000001 00000000 00000000 c02098b0 c0a04f34 edda8540 eeab8de0 edda8578 [ 74.261100] 1f60: 00000000 b61ea11a 000c530c 000c52d0 000c52dc 000a2a0c 0000000b c0101204 [ 74.269263] 1f80: ede70000 0000000b 000c530c c0209c54 00000000 00000000 20000010 000c530c [ 74.277428] 1fa0: 000c52d0 c0101000 000c530c 000c52d0 000c530c 000c52d0 000c52dc 00000000 [ 74.285592] 1fc0: 000c530c 000c52d0 000a2a0c 0000000b 000c52dc 000c4b5c 000c4b5c 000c530c [ 74.293755] 1fe0: b6f25ed4 beef38b8 00042038 b6f25ee0 a0000010 000c530c 00000000 00000000 [ 74.301924] [<c066a3d4>] (down_write) from [<c01e6fc8>] (unlink_anon_vmas+0xa8/0x1fc) [ 74.309743] [<c01e6fc8>] (unlink_anon_vmas) from [<c01da874>] (free_pgtables+0x78/0xcc) [ 74.317733] [<c01da874>] (free_pgtables) from [<c01e1e88>] (exit_mmap+0xe4/0x188) [ 74.325204] [<c01e1e88>] (exit_mmap) from [<c011b3a8>] (mmput+0x40/0xf0) [ 74.331898] [<c011b3a8>] (mmput) from [<c0208f1c>] (flush_old_exec+0x550/0x6e0) [ 74.339195] [<c0208f1c>] (flush_old_exec) from [<c0250e74>] (load_elf_binary+0x2f0/0x1324) [ 74.347447] [<c0250e74>] (load_elf_binary) from [<c02086c4>] (search_binary_handler+0xa0/0x234) [ 74.356134] [<c02086c4>] (search_binary_handler) from [<c02098b0>] (__do_execve_file+0x57c/0x6bc) [ 74.364993] [<c02098b0>] (__do_execve_file) from [<c0209c54>] (sys_execve+0x34/0x3c) [ 74.372724] [<c0209c54>] (sys_execve) from [<c0101000>] (ret_fast_syscall+0x0/0x54) [ 74.380365] Exception stack(0xede71fa8 to 0xede71ff0) [ 74.385407] 1fa0: 000c530c 000c52d0 000c530c 000c52d0 000c52dc 00000000 [ 74.393571] 1fc0: 000c530c 000c52d0 000a2a0c 0000000b 000c52dc 000c4b5c 000c4b5c 000c530c [ 74.401733] 1fe0: b6f25ed4 beef38b8 00042038 b6f25ee0 [ 74.406778] Code: e1a04000 f590f000 e3a03001 e34f3fff (e1902f9f) [ 74.412895] ---[ end trace aad10e0b4fcbf193 ]--- I think I will drop this patch until this issue is fixed. Philipp
On 30.08.2018 22:00, Philipp Rossak wrote: > On 30.08.2018 18:27, Ondřej Jirman wrote: >>> +static int sun8i_h3_calibrate(struct sun4i_gpadc_iio *info) >>> +{ >>> +// regmap_write(info->regmap, SUNXI_THS_CDATA_0_1, >>> +// info->calibration_data[0]); >>> +// regmap_write(info->regmap, SUNXI_THS_CDATA_2_3, >>> +// info->calibration_data[1]); >> This should probably be implemented, or left out completely. >> >> regards, >> o. >> > Thanks you are right! > This should be implemented! I will fix this in the next version! > > Thanks, > Philipp I just realized this function need to check if calibration datas are available. Writing zeros to the calibration data regs "breaks" the thermal sensor. Philipp
Hi Philipp, First, thanks for doing that rework. It was needed, and it's very much appreciated :) As you can imagine though, I have a bunch of comments. On Thu, Aug 30, 2018 at 05:44:49PM +0200, Philipp Rossak wrote: > Since we are merging the mfd driver into the sun4i-gpadc driver we need > to remove the build options for the sun4i-gpadc driver. > > Signed-off-by: Philipp Rossak <embed3d@gmail.com> > --- > drivers/mfd/Makefile | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index e9fd20dba18d..c680994db988 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -220,7 +220,6 @@ obj-$(CONFIG_INTEL_SOC_PMIC_CHTDC_TI) += intel_soc_pmic_chtdc_ti.o > obj-$(CONFIG_MFD_MT6397) += mt6397-core.o > > obj-$(CONFIG_MFD_ALTERA_A10SR) += altera-a10sr.o > -obj-$(CONFIG_MFD_SUN4I_GPADC) += sun4i-gpadc.o One of the things we should strive for is bisectability, which means being able to have a working driver at every point in time while introducing the features. In this particular case, this isn't really a problem since you're removing part of code that were never really enabled, but you should at least document why in your commit log. Thanks! Maxime
On Thu, Aug 30, 2018 at 05:44:52PM +0200, Philipp Rossak wrote: > Since we are merging the mfd driver into the iio adc driver we need to > update the Kconfig build options. Most of your commit logs a pretty short, and this is an issue for people lacking context. This would be the reviewers, but also users bisecting to see where there's a bug, or you in a couple of years from now :) Every commit log should be as complete as possible. In this case, you are saying what you are doing, but this is actually redundant with the patch, I can already tell what you are doing by reading the rest of the mail. However, what is really important is *why* you are doing it. > Signed-off-by: Philipp Rossak <embed3d@gmail.com> > --- > drivers/iio/adc/Kconfig | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index 9da79070357c..5d0cffd6d2e4 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -713,13 +713,16 @@ config STX104 > array module parameter. > > config SUN4I_GPADC > - tristate "Support for the Allwinner SoCs GPADC" > + tristate "Allwinner sunxi platforms' GPADC/Thermal driver" I'm really not sure this is worth updating > + select REGMAP_MMIO > + select REGMAP_IRQ > depends on IIO > - depends on MFD_SUN4I_GPADC || MACH_SUN8I > - depends on THERMAL || !THERMAL_OF > + depends on ARCH_SUNXI || MACH_SUN8I > + depends on THERMAL && THERMAL_OF For example, why you are changing the thermal dependency is not really obvious to anyone. > help > Say yes here to build support for Allwinner (A10, A13 and A31) SoCs > - GPADC. This ADC provides 4 channels which can be used as an ADC or as > + GPADC or newer SOCs (A33, H3, A83T, ...) Thermal sensor driver. > + This ADC provides 4 channels which can be used as an ADC or as I'm not sure this is worth updating either, especially when there's information that has been dropped. Maxime
On Thu, Aug 30, 2018 at 05:44:55PM +0200, Philipp Rossak wrote: > In the previous commit we removed the function call, now we remove the > unused functions. > > Signed-off-by: Philipp Rossak <embed3d@gmail.com> This will create a warning between the two commits, it should be merged in the previous patch. Maxime
On Thu, Aug 30, 2018 at 05:44:56PM +0200, Philipp Rossak wrote: > We remove the hwmon structure that was requiered for the mfd driver. > > Signed-off-by: Philipp Rossak <embed3d@gmail.com> Same thing here, you should merge that in the commit that removes the probe_mfd call. Maxime
On Thu, Aug 30, 2018 at 05:44:57PM +0200, Philipp Rossak wrote: > We want to use this driver mostly as thermal sensor, that still supports > the adc for the older chips, thus we threat the A33 as thermal sensor. > We also remove the adc channel without thermal support. The A33 can definitely be used as an ADC, so we need to keep that support. Maxime
Hi, On Thu, Aug 30, 2018 at 05:44:58PM +0200, Philipp Rossak wrote: > We rework the irq handling and the adc_channel handling. > This is requiered since we merge the mfd driver into the adc driver. > > Signed-off-by: Philipp Rossak <embed3d@gmail.com> This patch is actually the opposite of the previous ones, it has too many things in it, and sohuld be split :) > --- > drivers/iio/adc/sun4i-gpadc-iio.c | 157 ++++++++++++++++++++++++-------------- > include/linux/mfd/sun4i-gpadc.h | 7 -- > 2 files changed, 98 insertions(+), 66 deletions(-) > > diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c > index 658a7e3e3370..a2027614ee0c 100644 > --- a/drivers/iio/adc/sun4i-gpadc-iio.c > +++ b/drivers/iio/adc/sun4i-gpadc-iio.c > @@ -49,6 +49,8 @@ static unsigned int sun6i_gpadc_chan_select(unsigned int chan) > return SUN6I_GPADC_CTRL1_ADC_CHAN_SELECT(chan); > } > > +struct sun4i_gpadc_iio; > + > struct gpadc_data { > int temp_offset; > int temp_scale; > @@ -56,8 +58,15 @@ struct gpadc_data { > unsigned int tp_adc_select; > unsigned int (*adc_chan_select)(unsigned int chan); > unsigned int adc_chan_mask; > + bool adc_channel; > + irqreturn_t (*ths_irq_thread)(int irq, void *dev_id); > + int (*ths_suspend)(struct sun4i_gpadc_iio *info); > + int (*ths_resume)(struct sun4i_gpadc_iio *info); > + bool support_irq; > }; > > +static irqreturn_t sun4i_gpadc_data_irq_handler(int irq, void *dev_id); > + > static const struct gpadc_data sun4i_gpadc_data = { > .temp_offset = -1932, > .temp_scale = 133, > @@ -65,6 +74,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, > + .adc_channel = true, > + .ths_irq_thread = sun4i_gpadc_data_irq_handler, > + .support_irq = true, > }; > > static const struct gpadc_data sun5i_gpadc_data = { > @@ -74,6 +86,9 @@ 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, > + .adc_channel = true, > + .ths_irq_thread = sun4i_gpadc_data_irq_handler, > + .support_irq = true, > }; > > static const struct gpadc_data sun6i_gpadc_data = { > @@ -83,6 +98,9 @@ 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, > + .adc_channel = true, > + .ths_irq_thread = sun4i_gpadc_data_irq_handler, > + .support_irq = true, So this seems to enable the support for the interrupts without going through the MFD regmap that was removed in the previous patches. > }; > > static const struct gpadc_data sun8i_a33_gpadc_data = { > @@ -96,13 +114,10 @@ struct sun4i_gpadc_iio { > struct completion completion; > int temp_data; > u32 adc_data; > + unsigned int irq_data_type; > struct regmap *regmap; > - unsigned int fifo_data_irq; > - atomic_t ignore_fifo_data_irq; > - unsigned int temp_data_irq; > - atomic_t ignore_temp_data_irq; > + unsigned int irq; > const struct gpadc_data *data; > - bool no_irq; But at the same time, you remove some fields of that structure, and rename some other without any explanation. > /* prevents concurrent reads of temperature and ADC */ > struct mutex mutex; > struct thermal_zone_device *tzd; > @@ -130,6 +145,20 @@ static const struct regmap_config sun4i_gpadc_regmap_config = { > .fast_io = true, > }; > > +static int sun4i_gpadc_irq_init(struct sun4i_gpadc_iio *info) > +{ > + u32 reg; > + > + if (info->irq_data_type == SUN4I_GPADC_IRQ_FIFO_DATA) > + reg = SUN4I_GPADC_INT_FIFOC_TEMP_IRQ_EN; > + else > + reg = SUN4I_GPADC_INT_FIFOC_TEMP_IRQ_EN; > + > + regmap_write(info->regmap, SUN4I_GPADC_INT_FIFOC, reg); > + > + return 0; > +} > + > static int sun4i_prepare_for_irq(struct iio_dev *indio_dev, int channel, > unsigned int irq) > { > @@ -151,7 +180,7 @@ static int sun4i_prepare_for_irq(struct iio_dev *indio_dev, int channel, > if (ret) > return ret; > > - if (irq == info->fifo_data_irq) { > + if (irq == SUN4I_GPADC_IRQ_FIFO_DATA) { > ret = regmap_write(info->regmap, SUN4I_GPADC_CTRL1, > info->data->tp_mode_en | > info->data->tp_adc_select | > @@ -172,6 +201,8 @@ static int sun4i_prepare_for_irq(struct iio_dev *indio_dev, int channel, > ret = regmap_write(info->regmap, SUN4I_GPADC_CTRL1, > info->data->tp_mode_en); > } > + if (info->data->support_irq) > + sun4i_gpadc_irq_init(info); > > if (ret) > return ret; > @@ -194,11 +225,12 @@ static int sun4i_gpadc_read(struct iio_dev *indio_dev, int channel, int *val, > > mutex_lock(&info->mutex); > > + info->irq_data_type = irq; This would need to be explained too > ret = sun4i_prepare_for_irq(indio_dev, channel, irq); > if (ret) > goto err; > > - enable_irq(irq); > + enable_irq(info->irq); > > /* > * The temperature sensor throws an interruption periodically (currently > @@ -212,7 +244,7 @@ static int sun4i_gpadc_read(struct iio_dev *indio_dev, int channel, int *val, > goto err; > } > > - if (irq == info->fifo_data_irq) > + if (irq == SUN4I_GPADC_IRQ_FIFO_DATA) > *val = info->adc_data; > else > *val = info->temp_data; > @@ -222,7 +254,7 @@ static int sun4i_gpadc_read(struct iio_dev *indio_dev, int channel, int *val, > > err: > pm_runtime_put_autosuspend(indio_dev->dev.parent); > - disable_irq(irq); > + disable_irq(info->irq); > mutex_unlock(&info->mutex); > > return ret; > @@ -231,16 +263,15 @@ static int sun4i_gpadc_read(struct iio_dev *indio_dev, int channel, int *val, > static int sun4i_gpadc_adc_read(struct iio_dev *indio_dev, int channel, > int *val) > { > - struct sun4i_gpadc_iio *info = iio_priv(indio_dev); > - > - return sun4i_gpadc_read(indio_dev, channel, val, info->fifo_data_irq); > + return sun4i_gpadc_read(indio_dev, channel, val, > + SUN4I_GPADC_IRQ_FIFO_DATA); > } > > static int sun4i_gpadc_temp_read(struct iio_dev *indio_dev, int *val) > { > struct sun4i_gpadc_iio *info = iio_priv(indio_dev); > > - if (info->no_irq) { > + if (!info->data->support_irq) { > pm_runtime_get_sync(indio_dev->dev.parent); > > regmap_read(info->regmap, SUN4I_GPADC_TEMP_DATA, val); > @@ -251,7 +282,7 @@ static int sun4i_gpadc_temp_read(struct iio_dev *indio_dev, int *val) > return 0; > } > > - return sun4i_gpadc_read(indio_dev, 0, val, info->temp_data_irq); > + return sun4i_gpadc_read(indio_dev, 0, val, SUN4I_GPADC_IRQ_TEMP_DATA); > } > > static int sun4i_gpadc_temp_offset(struct iio_dev *indio_dev, int *val) > @@ -320,31 +351,21 @@ static const struct iio_info sun4i_gpadc_iio_info = { > .read_raw = sun4i_gpadc_read_raw, > }; > > -static irqreturn_t sun4i_gpadc_temp_data_irq_handler(int irq, void *dev_id) > +static irqreturn_t sun4i_gpadc_data_irq_handler(int irq, void *dev_id) > { > struct sun4i_gpadc_iio *info = dev_id; > > - if (atomic_read(&info->ignore_temp_data_irq)) > - goto out; > - > - if (!regmap_read(info->regmap, SUN4I_GPADC_TEMP_DATA, &info->temp_data)) > - complete(&info->completion); > - > -out: > - return IRQ_HANDLED; > -} > - > -static irqreturn_t sun4i_gpadc_fifo_data_irq_handler(int irq, void *dev_id) > -{ > - struct sun4i_gpadc_iio *info = dev_id; > - > - if (atomic_read(&info->ignore_fifo_data_irq)) > - goto out; > - > - if (!regmap_read(info->regmap, SUN4I_GPADC_DATA, &info->adc_data)) > - complete(&info->completion); > - > -out: > + if (info->irq_data_type == SUN4I_GPADC_IRQ_FIFO_DATA) { > + /* read fifo data */ > + if (!regmap_read(info->regmap, SUN4I_GPADC_DATA, > + &info->adc_data)) > + complete(&info->completion); > + } else { > + /* read temp data */ > + if (!regmap_read(info->regmap, SUN4I_GPADC_TEMP_DATA, > + &info->temp_data)) > + complete(&info->completion); > + } > return IRQ_HANDLED; > } > > @@ -356,6 +377,8 @@ static int sun4i_gpadc_runtime_suspend(struct device *dev) > regmap_write(info->regmap, SUN4I_GPADC_CTRL1, 0); > /* Disable temperature sensor on IP */ > regmap_write(info->regmap, SUN4I_GPADC_TPR, 0); > + /* Disable irq*/ > + regmap_write(info->regmap, SUN4I_GPADC_INT_FIFOC, 0); > > return 0; > } > @@ -378,6 +401,7 @@ static int sun4i_gpadc_runtime_resume(struct device *dev) > SUN4I_GPADC_TPR_TEMP_ENABLE | > SUN4I_GPADC_TPR_TEMP_PERIOD(800)); > > + > return 0; > } > > @@ -426,8 +450,6 @@ static int sun4i_gpadc_probe_dt(struct platform_device *pdev, > if (!info->data) > return -ENODEV; > > - info->no_irq = true; > - > mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > base = devm_ioremap_resource(&pdev->dev, mem); > if (IS_ERR(base)) > @@ -441,8 +463,25 @@ static int sun4i_gpadc_probe_dt(struct platform_device *pdev, > return ret; > } > > - if (IS_ENABLED(CONFIG_THERMAL_OF)) > - info->sensor_device = &pdev->dev; You're also dropping the THERMAL_OF conditions > + if (info->data->support_irq) { > + > + /* ths interrupt */ > + info->irq = platform_get_irq(pdev, 0); > + > + ret = devm_request_threaded_irq(&pdev->dev, info->irq, > + NULL, info->data->ths_irq_thread, > + IRQF_ONESHOT, dev_name(&pdev->dev), info); Why do you need a threaded interrupt? > + if (info->data->adc_channel) > + disable_irq(info->irq); Does it still needs to be disabled? Why? > + if (ret) { > + dev_err(&pdev->dev, "failed to add ths irq: %d\n", ret); > + return ret; > + } > + } > + > + info->sensor_device = &pdev->dev; > > return 0; > } > @@ -469,6 +508,11 @@ static int sun4i_gpadc_probe(struct platform_device *pdev) > indio_dev->info = &sun4i_gpadc_iio_info; > indio_dev->modes = INDIO_DIRECT_MODE; > > + if (&info->data->adc_channel) { > + indio_dev->num_channels = ARRAY_SIZE(sun4i_gpadc_channels); > + indio_dev->channels = sun4i_gpadc_channels; > + } > + > ret = sun4i_gpadc_probe_dt(pdev, indio_dev); > > if (ret) > @@ -480,20 +524,18 @@ static int sun4i_gpadc_probe(struct platform_device *pdev) > pm_runtime_set_suspended(&pdev->dev); > 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); > - /* > - * Do not fail driver probing when failing to register in > - * thermal because no thermal DT node is found. > - */ > - if (IS_ERR(info->tzd) && PTR_ERR(info->tzd) != -ENODEV) { > - dev_err(&pdev->dev, > - "could not register thermal sensor: %ld\n", > - PTR_ERR(info->tzd)); > - return PTR_ERR(info->tzd); > - } > + info->tzd = thermal_zone_of_sensor_register(info->sensor_device, > + 0, info, > + &sun4i_ts_tz_ops); > + /* > + * Do not fail driver probing when failing to register in > + * thermal because no thermal DT node is found. > + */ > + if (IS_ERR(info->tzd) && PTR_ERR(info->tzd) != -ENODEV) { > + dev_err(&pdev->dev, > + "could not register thermal sensor: %ld\n", > + PTR_ERR(info->tzd)); > + return PTR_ERR(info->tzd); > } > > ret = devm_iio_device_register(&pdev->dev, indio_dev); > @@ -505,7 +547,7 @@ static int sun4i_gpadc_probe(struct platform_device *pdev) > return 0; > > err_map: > - if (!info->no_irq && IS_ENABLED(CONFIG_THERMAL_OF)) > + if (!info->data->support_irq) > iio_map_array_unregister(indio_dev); > > pm_runtime_put(&pdev->dev); > @@ -522,12 +564,9 @@ static int sun4i_gpadc_remove(struct platform_device *pdev) > pm_runtime_put(&pdev->dev); > pm_runtime_disable(&pdev->dev); > > - if (!IS_ENABLED(CONFIG_THERMAL_OF)) > - return 0; > - > thermal_zone_of_sensor_unregister(info->sensor_device, info->tzd); > > - if (!info->no_irq) > + if (!info->data->support_irq) > iio_map_array_unregister(indio_dev); > > return 0; > diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h > index 54c7c9375c1b..ca59336f246b 100644 > --- a/include/linux/mfd/sun4i-gpadc.h > +++ b/include/linux/mfd/sun4i-gpadc.h > @@ -99,11 +99,4 @@ > .datasheet_name = _name, \ > } > > -struct sun4i_gpadc_dev { > - struct device *dev; > - struct regmap *regmap; > - struct regmap_irq_chip_data *regmap_irqc; > - void __iomem *base; > -}; And you're dropping that structure.
On Thu, Aug 30, 2018 at 05:44:59PM +0200, Philipp Rossak wrote: > We are now adding the new compatibles. > > Signed-off-by: Philipp Rossak <embed3d@gmail.com> > --- > drivers/iio/adc/sun4i-gpadc-iio.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c > index a2027614ee0c..79b8efdab803 100644 > --- a/drivers/iio/adc/sun4i-gpadc-iio.c > +++ b/drivers/iio/adc/sun4i-gpadc-iio.c > @@ -435,6 +435,18 @@ static const struct of_device_id sun4i_gpadc_of_id[] = { > .compatible = "allwinner,sun8i-a33-ths", > .data = &sun8i_a33_gpadc_data, > }, > + { > + .compatible = "allwinner,sun4i-a10-gpadc", > + .data = &sun4i_gpadc_data > + }, > + { > + .compatible = "allwinner,sun5i-a13-gpadc", > + .data = &sun5i_gpadc_data > + }, > + { > + .compatible = "allwinner,sun6i-a31-gpadc", > + .data = &sun6i_gpadc_data > + }, Usually the bindings come before the code that use them, and the commit log is also lacking to explain how we should use them, that they won't be used at the moment, why, etc. Maxime
On Thu, Aug 30, 2018 at 05:45:04PM +0200, Philipp Rossak wrote: > For adding newer sensor some basic rework of the code is necessary. > > This commit reworks the code and uses regmap field to read out > temp_data. > > Signed-off-by: Philipp Rossak <embed3d@gmail.com> > --- > drivers/iio/adc/sun4i-gpadc-iio.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) > > diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c > index d48f338af563..c278e165e161 100644 > --- a/drivers/iio/adc/sun4i-gpadc-iio.c > +++ b/drivers/iio/adc/sun4i-gpadc-iio.c > @@ -63,6 +63,7 @@ struct gpadc_data { > int (*ths_suspend)(struct sun4i_gpadc_iio *info); > int (*ths_resume)(struct sun4i_gpadc_iio *info); > bool support_irq; > + u32 temp_data_base; > }; > > static irqreturn_t sun4i_gpadc_data_irq_handler(int irq, void *dev_id); > @@ -77,6 +78,7 @@ static const struct gpadc_data sun4i_gpadc_data = { > .adc_channel = true, > .ths_irq_thread = sun4i_gpadc_data_irq_handler, > .support_irq = true, > + .temp_data_base = SUN4I_GPADC_TEMP_DATA, > }; > > static const struct gpadc_data sun5i_gpadc_data = { > @@ -89,6 +91,7 @@ static const struct gpadc_data sun5i_gpadc_data = { > .adc_channel = true, > .ths_irq_thread = sun4i_gpadc_data_irq_handler, > .support_irq = true, > + .temp_data_base = SUN4I_GPADC_TEMP_DATA, > }; > > static const struct gpadc_data sun6i_gpadc_data = { > @@ -101,12 +104,14 @@ static const struct gpadc_data sun6i_gpadc_data = { > .adc_channel = true, > .ths_irq_thread = sun4i_gpadc_data_irq_handler, > .support_irq = true, > + .temp_data_base = SUN4I_GPADC_TEMP_DATA, > }; > > 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_base = SUN4I_GPADC_TEMP_DATA, > }; > > struct sun4i_gpadc_iio { > @@ -271,18 +276,18 @@ static int sun4i_gpadc_temp_read(struct iio_dev *indio_dev, int *val) > { > struct sun4i_gpadc_iio *info = iio_priv(indio_dev); > > - if (!info->data->support_irq) { > - pm_runtime_get_sync(indio_dev->dev.parent); > + if (info->data->adc_channel) > + return sun4i_gpadc_read(indio_dev, 0, val, > + SUN4I_GPADC_IRQ_TEMP_DATA); > > - regmap_read(info->regmap, SUN4I_GPADC_TEMP_DATA, val); > + pm_runtime_get_sync(indio_dev->dev.parent); > > - pm_runtime_mark_last_busy(indio_dev->dev.parent); > - pm_runtime_put_autosuspend(indio_dev->dev.parent); > + regmap_read(info->regmap, info->data->temp_data_base, val); > > - return 0; > - } > + pm_runtime_mark_last_busy(indio_dev->dev.parent); > + pm_runtime_put_autosuspend(indio_dev->dev.parent); > > - return sun4i_gpadc_read(indio_dev, 0, val, SUN4I_GPADC_IRQ_TEMP_DATA); > + return 0; You should make this patch as small as possible. If you want to add a variable and to invert a condition, please make two patches. Maxime
On Thu, Aug 30, 2018 at 05:45:05PM +0200, 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> > --- > drivers/iio/adc/sun4i-gpadc-iio.c | 72 +++++++++++++++++++++++++++++++++++++-- > 1 file changed, 70 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c > index c278e165e161..c12de48c4e86 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> > > @@ -63,6 +65,9 @@ struct gpadc_data { > int (*ths_suspend)(struct sun4i_gpadc_iio *info); > int (*ths_resume)(struct sun4i_gpadc_iio *info); > bool support_irq; > + bool has_bus_clk; > + bool has_bus_rst; > + bool has_mod_clk; > u32 temp_data_base; > }; > > @@ -127,6 +132,9 @@ struct sun4i_gpadc_iio { > struct mutex mutex; > struct thermal_zone_device *tzd; > struct device *sensor_device; > + struct clk *bus_clk; > + struct clk *mod_clk; > + struct reset_control *reset; > }; > > static const struct iio_chan_spec sun4i_gpadc_channels[] = { > @@ -472,8 +480,13 @@ static int sun4i_gpadc_probe_dt(struct platform_device *pdev, > if (IS_ERR(base)) > return PTR_ERR(base); > > - info->regmap = devm_regmap_init_mmio(&pdev->dev, base, > - &sun4i_gpadc_regmap_config); > + if (info->data->has_bus_clk) > + info->regmap = devm_regmap_init_mmio_clk(&pdev->dev, "bus", > + base, &sun4i_gpadc_regmap_config); > + else > + info->regmap = devm_regmap_init_mmio(&pdev->dev, base, > + &sun4i_gpadc_regmap_config); > + > if (IS_ERR(info->regmap)) { > ret = PTR_ERR(info->regmap); > dev_err(&pdev->dev, "failed to init regmap: %d\n", ret); > @@ -498,9 +511,58 @@ static int sun4i_gpadc_probe_dt(struct platform_device *pdev, > } > } > > + 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; That should be done in the runtime_resume hook > + } > + > + 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 4MHz */ > + ret = clk_set_rate(info->mod_clk, 4000000); > + if (ret) > + goto disable_bus_clk; Why? > + ret = clk_prepare_enable(info->mod_clk); > + if (ret) > + goto disable_bus_clk; > + } > + > info->sensor_device = &pdev->dev; > > return 0; > + > +disable_bus_clk: > + clk_disable_unprepare(info->bus_clk); > + > +assert_reset: > + reset_control_assert(info->reset); You should check for the variables here before calling those functions, if you end up here with your variable not set, you'll have improper refcounting. Maxime
On Thu, Aug 30, 2018 at 05:45:06PM +0200, 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 | 63 +++++++++++++++++++++++++------------ > include/linux/iio/adc/sun4i-gpadc.h | 3 ++ > 2 files changed, 46 insertions(+), 20 deletions(-) > > diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c > index c12de48c4e86..18ab72e52d78 100644 > --- a/drivers/iio/adc/sun4i-gpadc-iio.c > +++ b/drivers/iio/adc/sun4i-gpadc-iio.c > @@ -69,6 +69,7 @@ struct gpadc_data { > bool has_bus_rst; > bool has_mod_clk; > u32 temp_data_base; > + int sensor_count; > }; > > static irqreturn_t sun4i_gpadc_data_irq_handler(int irq, void *dev_id); > @@ -84,6 +85,7 @@ static const struct gpadc_data sun4i_gpadc_data = { > .ths_irq_thread = sun4i_gpadc_data_irq_handler, > .support_irq = true, > .temp_data_base = SUN4I_GPADC_TEMP_DATA, > + .sensor_count = 1, > }; > > static const struct gpadc_data sun5i_gpadc_data = { > @@ -97,6 +99,7 @@ static const struct gpadc_data sun5i_gpadc_data = { > .ths_irq_thread = sun4i_gpadc_data_irq_handler, > .support_irq = true, > .temp_data_base = SUN4I_GPADC_TEMP_DATA, > + .sensor_count = 1, > }; > > static const struct gpadc_data sun6i_gpadc_data = { > @@ -110,6 +113,7 @@ static const struct gpadc_data sun6i_gpadc_data = { > .ths_irq_thread = sun4i_gpadc_data_irq_handler, > .support_irq = true, > .temp_data_base = SUN4I_GPADC_TEMP_DATA, > + .sensor_count = 1, > }; > > static const struct gpadc_data sun8i_a33_gpadc_data = { > @@ -117,6 +121,13 @@ static const struct gpadc_data sun8i_a33_gpadc_data = { > .temp_scale = 162, > .tp_mode_en = SUN8I_A33_GPADC_CTRL1_CHOP_TEMP_EN, > .temp_data_base = SUN4I_GPADC_TEMP_DATA, > + .sensor_count = 1, > +}; > + > +struct sun4i_sensor_tzd { > + struct sun4i_gpadc_iio *info; > + struct thermal_zone_device *tzd; > + unsigned int sensor_id; > }; > > struct sun4i_gpadc_iio { > @@ -130,7 +141,7 @@ struct sun4i_gpadc_iio { > const struct gpadc_data *data; > /* prevents concurrent reads of temperature and ADC */ > struct mutex mutex; > - struct thermal_zone_device *tzd; > + struct sun4i_sensor_tzd tzds[MAX_SENSOR_COUNT]; > struct device *sensor_device; > struct clk *bus_clk; > struct clk *mod_clk; > @@ -280,7 +291,8 @@ static int sun4i_gpadc_adc_read(struct iio_dev *indio_dev, int channel, > SUN4I_GPADC_IRQ_FIFO_DATA); > } > > -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); > > @@ -290,7 +302,8 @@ static int sun4i_gpadc_temp_read(struct iio_dev *indio_dev, int *val) > > pm_runtime_get_sync(indio_dev->dev.parent); > > - regmap_read(info->regmap, info->data->temp_data_base, val); > + regmap_read(info->regmap, info->data->temp_data_base + 0x4 * sensor, > + val); > > pm_runtime_mark_last_busy(indio_dev->dev.parent); > pm_runtime_put_autosuspend(indio_dev->dev.parent); > @@ -334,7 +347,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; > @@ -420,10 +433,11 @@ static int sun4i_gpadc_runtime_resume(struct device *dev) > > static int sun4i_gpadc_get_temp(void *data, int *temp) > { > - struct sun4i_gpadc_iio *info = data; > + struct sun4i_sensor_tzd *tzd = data; > + struct sun4i_gpadc_iio *info = tzd->info; > int val, scale, offset; > > - if (sun4i_gpadc_temp_read(info->indio_dev, &val)) > + if (sun4i_gpadc_temp_read(info->indio_dev, &val, tzd->sensor_id)) > return -ETIMEDOUT; > > sun4i_gpadc_temp_scale(info->indio_dev, &scale); > @@ -569,7 +583,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) > @@ -603,18 +617,24 @@ static int sun4i_gpadc_probe(struct platform_device *pdev) > pm_runtime_set_suspended(&pdev->dev); > pm_runtime_enable(&pdev->dev); > > - info->tzd = thermal_zone_of_sensor_register(info->sensor_device, > - 0, info, > - &sun4i_ts_tz_ops); > - /* > - * Do not fail driver probing when failing to register in > - * thermal because no thermal DT node is found. > - */ > - if (IS_ERR(info->tzd) && PTR_ERR(info->tzd) != -ENODEV) { > - dev_err(&pdev->dev, > - "could not register thermal sensor: %ld\n", > - PTR_ERR(info->tzd)); > - return PTR_ERR(info->tzd); > + for (i = 0; i < info->data->sensor_count; i++) { > + info->tzds[i].info = info; > + info->tzds[i].sensor_id = i; > + > + info->tzds[i].tzd = thermal_zone_of_sensor_register( This isn't where should wrap your line. > + info->sensor_device, > + i, &info->tzds[i], &sun4i_ts_tz_ops); > + /* > + * Do not fail driver probing when failing to register in > + * thermal because no thermal DT node is found. > + */ > + if (IS_ERR(info->tzds[i].tzd) && \ And you don't need the \ here. > + PTR_ERR(info->tzds[i].tzd) != -ENODEV) { > + dev_err(&pdev->dev, > + "could not register thermal sensor: %ld\n", > + PTR_ERR(info->tzds[i].tzd)); > + return PTR_ERR(info->tzds[i].tzd); > + } If you're failing half way through the registration, you won't free the first sensors registered. Maxime
On Thu, Aug 30, 2018 at 05:45:07PM +0200, 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 | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c > index 18ab72e52d78..2fd73d143815 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> > @@ -70,6 +71,7 @@ struct gpadc_data { > bool has_mod_clk; > u32 temp_data_base; > int sensor_count; > + bool supports_nvmem; > }; > > static irqreturn_t sun4i_gpadc_data_irq_handler(int irq, void *dev_id); > @@ -146,6 +148,7 @@ struct sun4i_gpadc_iio { > struct clk *bus_clk; > struct clk *mod_clk; > struct reset_control *reset; > + u32 calibration_data[2]; > }; > > static const struct iio_chan_spec sun4i_gpadc_channels[] = { > @@ -484,6 +487,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; > + u32 *cell_data; > > info->data = of_device_get_match_data(&pdev->dev); > if (!info->data) > @@ -494,6 +500,24 @@ static int sun4i_gpadc_probe_dt(struct platform_device *pdev, > if (IS_ERR(base)) > return PTR_ERR(base); > > + if (info->data->supports_nvmem) { > + > + cell = nvmem_cell_get(&pdev->dev, "calibration"); > + if (IS_ERR(cell)) { > + if (PTR_ERR(cell) == -EPROBE_DEFER) > + return PTR_ERR(cell); You're masking real errors here, if the issue isn't that the property isn't found. > + } else { > + cell_data = (u32 *)nvmem_cell_read(cell, &cell_size); > + if (cell_size != 8) > + dev_err(&pdev->dev, > + "Calibration data has wrong size\n"); > + else { > + info->calibration_data[0] = cell_data[0]; > + info->calibration_data[1] = cell_data[1]; How are those calibration data organized when there is multiple channels? Maxime
On Thu, Aug 30, 2018 at 05:45:08PM +0200, Philipp Rossak wrote: > Different sensors will have different suspend and resume functions. So > we are modularize the suspend and resume functions. > > The resume function configures and initializes the thermal sensor and > the suspend function disables the sensors. > > Signed-off-by: Philipp Rossak <embed3d@gmail.com> > --- > drivers/iio/adc/sun4i-gpadc-iio.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c > index 2fd73d143815..c7b46c82e3e5 100644 > --- a/drivers/iio/adc/sun4i-gpadc-iio.c > +++ b/drivers/iio/adc/sun4i-gpadc-iio.c > @@ -76,6 +76,9 @@ struct gpadc_data { > > static irqreturn_t sun4i_gpadc_data_irq_handler(int irq, void *dev_id); > > +static int sun4i_ths_resume(struct sun4i_gpadc_iio *info); > +static int sun4i_ths_suspend(struct sun4i_gpadc_iio *info); > + > static const struct gpadc_data sun4i_gpadc_data = { > .temp_offset = -1932, > .temp_scale = 133, > @@ -87,6 +90,8 @@ static const struct gpadc_data sun4i_gpadc_data = { > .ths_irq_thread = sun4i_gpadc_data_irq_handler, > .support_irq = true, > .temp_data_base = SUN4I_GPADC_TEMP_DATA, > + .ths_resume = sun4i_ths_resume, > + .ths_suspend = sun4i_ths_suspend, > .sensor_count = 1, > }; > > @@ -101,6 +106,8 @@ static const struct gpadc_data sun5i_gpadc_data = { > .ths_irq_thread = sun4i_gpadc_data_irq_handler, > .support_irq = true, > .temp_data_base = SUN4I_GPADC_TEMP_DATA, > + .ths_resume = sun4i_ths_resume, > + .ths_suspend = sun4i_ths_suspend, > .sensor_count = 1, > }; > > @@ -115,6 +122,8 @@ static const struct gpadc_data sun6i_gpadc_data = { > .ths_irq_thread = sun4i_gpadc_data_irq_handler, > .support_irq = true, > .temp_data_base = SUN4I_GPADC_TEMP_DATA, > + .ths_resume = sun4i_ths_resume, > + .ths_suspend = sun4i_ths_suspend, > .sensor_count = 1, > }; > > @@ -123,6 +132,8 @@ static const struct gpadc_data sun8i_a33_gpadc_data = { > .temp_scale = 162, > .tp_mode_en = SUN8I_A33_GPADC_CTRL1_CHOP_TEMP_EN, > .temp_data_base = SUN4I_GPADC_TEMP_DATA, > + .ths_resume = sun4i_ths_resume, > + .ths_suspend = sun4i_ths_suspend, > .sensor_count = 1, > }; > > @@ -401,6 +412,11 @@ static irqreturn_t sun4i_gpadc_data_irq_handler(int irq, void *dev_id) > static int sun4i_gpadc_runtime_suspend(struct device *dev) > { > struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev)); > + return info->data->ths_suspend(info); > +} > + > +static int sun4i_ths_suspend(struct sun4i_gpadc_iio *info) suspend is already a hook in the kernel, which hasn't the same meaning than runtime_suspend (and the same applies to resume), so we'd rather pick a better name. And all the functions (and the driver) use gpadc, please continue to use that prefix. Maxime
On Thu, Aug 30, 2018 at 05:45:09PM +0200, 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 | 91 +++++++++++++++++++++++++++++++++++++ > include/linux/iio/adc/sun4i-gpadc.h | 18 ++++++++ > 2 files changed, 109 insertions(+) > > diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c > index c7b46c82e3e5..d5c7971b2558 100644 > --- a/drivers/iio/adc/sun4i-gpadc-iio.c > +++ b/drivers/iio/adc/sun4i-gpadc-iio.c > @@ -72,6 +72,7 @@ struct gpadc_data { > u32 temp_data_base; > int sensor_count; > bool supports_nvmem; > + u32 ths_irq_clear; > }; > > static irqreturn_t sun4i_gpadc_data_irq_handler(int irq, void *dev_id); > @@ -79,6 +80,10 @@ static irqreturn_t sun4i_gpadc_data_irq_handler(int irq, void *dev_id); > static int sun4i_ths_resume(struct sun4i_gpadc_iio *info); > static int sun4i_ths_suspend(struct sun4i_gpadc_iio *info); > > +static int sun8i_h3_ths_resume(struct sun4i_gpadc_iio *info); > +static int sun8i_h3_ths_suspend(struct sun4i_gpadc_iio *info); > +static irqreturn_t sunx8i_h3_irq_thread(int irq, void *data); > + > static const struct gpadc_data sun4i_gpadc_data = { > .temp_offset = -1932, > .temp_scale = 133, > @@ -137,6 +142,22 @@ static const struct gpadc_data sun8i_a33_gpadc_data = { > .sensor_count = 1, > }; > > +static const struct gpadc_data sun8i_h3_ths_data = { > + .temp_offset = -1791, > + .temp_scale = -121, > + .temp_data_base = SUN8I_H3_THS_TDATA0, > + .ths_irq_thread = sunx8i_h3_irq_thread, > + .support_irq = true, > + .has_bus_clk = true, > + .has_bus_rst = true, > + .has_mod_clk = true, > + .sensor_count = 1, > + .supports_nvmem = true, > + .ths_resume = sun8i_h3_ths_resume, > + .ths_suspend = sun8i_h3_ths_suspend, > + .ths_irq_clear = SUN8I_H3_THS_INTS_TDATA_IRQ_0, > +}; > + > struct sun4i_sensor_tzd { > struct sun4i_gpadc_iio *info; > struct thermal_zone_device *tzd; > @@ -409,6 +430,31 @@ static irqreturn_t sun4i_gpadc_data_irq_handler(int irq, void *dev_id) > return IRQ_HANDLED; > } > > +static irqreturn_t sunx8i_h3_irq_thread(int irq, void *data) > +{ > + struct sun4i_gpadc_iio *info = data; > + int i; > + > + regmap_write(info->regmap, SUN8I_H3_THS_STAT, > + info->data->ths_irq_clear); > + > + for (i = 0; i < info->data->sensor_count; i++) > + thermal_zone_device_update(info->tzds[i].tzd, > + THERMAL_EVENT_TEMP_SAMPLE); > + > + return IRQ_HANDLED; > +} > + > +static int sun8i_h3_calibrate(struct sun4i_gpadc_iio *info) > +{ > +// regmap_write(info->regmap, SUNXI_THS_CDATA_0_1, > +// info->calibration_data[0]); > +// regmap_write(info->regmap, SUNXI_THS_CDATA_2_3, > +// info->calibration_data[1]); Don't put commented code. > + > + return 0; > +} > + > static int sun4i_gpadc_runtime_suspend(struct device *dev) > { > struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev)); > @@ -428,6 +474,16 @@ static int sun4i_ths_suspend(struct sun4i_gpadc_iio *info) > return 0; > } > > +static int sun8i_h3_ths_suspend(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_resume(struct device *dev) > { > struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev)); > @@ -454,6 +510,37 @@ static int sun4i_ths_resume(struct sun4i_gpadc_iio *info) > return 0; > } > > +static int sun8i_h3_ths_resume(struct sun4i_gpadc_iio *info) > +{ > + u32 value; > + > + sun8i_h3_calibrate(info); > + > + regmap_write(info->regmap, SUN8I_H3_THS_CTRL0, > + SUN4I_GPADC_CTRL0_T_ACQ(0xff)); > + > + regmap_write(info->regmap, SUN8I_H3_THS_CTRL2, > + SUN8I_H3_THS_ACQ1(0x3f)); > + > + regmap_write(info->regmap, SUN8I_H3_THS_STAT, > + SUN8I_H3_THS_INTS_TDATA_IRQ_0); > + > + regmap_write(info->regmap, SUN8I_H3_THS_FILTER, > + SUN4I_GPADC_CTRL3_FILTER_EN | > + SUN4I_GPADC_CTRL3_FILTER_TYPE(0x2)); > + > + regmap_write(info->regmap, SUN8I_H3_THS_INTC, > + SUN8I_H3_THS_INTC_TDATA_IRQ_EN0 | > + SUN8I_H3_THS_TEMP_PERIOD(0x55)); > + > + regmap_read(info->regmap, SUN8I_H3_THS_CTRL2, &value); > + > + regmap_write(info->regmap, SUN8I_H3_THS_CTRL2, > + SUN8I_H3_THS_TEMP_SENSE_EN0 | value); Ideally, all these values should have a comment explaining what they are. And we really start to have a lot of registers defines. We'd be better off using regmap_fields.
On Thu, Aug 30, 2018 at 05:45:13PM +0200, 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 | 31 +++++++++++++++++++++++++++++++ > arch/arm/boot/dts/sunxi-h3-h5.dtsi | 1 + > 2 files changed, 32 insertions(+) > > diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi > index 5b7994cb1471..954848d5df50 100644 > --- a/arch/arm/boot/dts/sun8i-h3.dtsi > +++ b/arch/arm/boot/dts/sun8i-h3.dtsi > @@ -78,6 +78,8 @@ > clock-names = "cpu"; > operating-points-v2 = <&cpu0_opp_table>; > #cooling-cells = <2>; > + cooling-min-level = <0>; > + cooling-max-level = <15>; > }; > > cpu@1 { > @@ -102,6 +104,35 @@ > }; > }; > > + thermal-zones { > + cpu-thermal { > + /* milliseconds */ > + polling-delay-passive = <250>; > + polling-delay = <1000>; > + thermal-sensors = <&ths>; > + > + trips { > + cpu_hot_trip: cpu-warm { > + temperature = <65000>; > + hysteresis = <2000>; > + type = "passive"; > + }; > + cpu_very_hot_trip: cpu-very-hot { > + temperature = <90000>; > + hysteresis = <2000>; > + type = "critical"; > + }; > + }; Where are those trip points coming from? Maxime
在 2018-08-31五的 11:11 +0200，Maxime Ripard写道： > On Thu, Aug 30, 2018 at 05:45:09PM +0200, 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 | 91 > > +++++++++++++++++++++++++++++++++++++ > > include/linux/iio/adc/sun4i-gpadc.h | 18 ++++++++ > > 2 files changed, 109 insertions(+) > > > > diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c > > b/drivers/iio/adc/sun4i-gpadc-iio.c > > index c7b46c82e3e5..d5c7971b2558 100644 > > --- a/drivers/iio/adc/sun4i-gpadc-iio.c > > +++ b/drivers/iio/adc/sun4i-gpadc-iio.c > > @@ -72,6 +72,7 @@ struct gpadc_data { > > u32 temp_data_base; > > int sensor_count; > > bool supports_nvmem; > > + u32 ths_irq_clear; > > }; > > > > static irqreturn_t sun4i_gpadc_data_irq_handler(int irq, void > > *dev_id); > > @@ -79,6 +80,10 @@ static irqreturn_t > > sun4i_gpadc_data_irq_handler(int irq, void *dev_id); > > static int sun4i_ths_resume(struct sun4i_gpadc_iio *info); > > static int sun4i_ths_suspend(struct sun4i_gpadc_iio *info); > > > > +static int sun8i_h3_ths_resume(struct sun4i_gpadc_iio *info); > > +static int sun8i_h3_ths_suspend(struct sun4i_gpadc_iio *info); > > +static irqreturn_t sunx8i_h3_irq_thread(int irq, void *data); > > + > > static const struct gpadc_data sun4i_gpadc_data = { > > .temp_offset = -1932, > > .temp_scale = 133, > > @@ -137,6 +142,22 @@ static const struct gpadc_data > > sun8i_a33_gpadc_data = { > > .sensor_count = 1, > > }; > > > > +static const struct gpadc_data sun8i_h3_ths_data = { > > + .temp_offset = -1791, > > + .temp_scale = -121, > > + .temp_data_base = SUN8I_H3_THS_TDATA0, > > + .ths_irq_thread = sunx8i_h3_irq_thread, > > + .support_irq = true, > > + .has_bus_clk = true, > > + .has_bus_rst = true, > > + .has_mod_clk = true, > > + .sensor_count = 1, > > + .supports_nvmem = true, > > + .ths_resume = sun8i_h3_ths_resume, > > + .ths_suspend = sun8i_h3_ths_suspend, > > + .ths_irq_clear = SUN8I_H3_THS_INTS_TDATA_IRQ_0, > > +}; > > + > > struct sun4i_sensor_tzd { > > struct sun4i_gpadc_iio *info; > > struct thermal_zone_device *tzd; > > @@ -409,6 +430,31 @@ static irqreturn_t > > sun4i_gpadc_data_irq_handler(int irq, void *dev_id) > > return IRQ_HANDLED; > > } > > > > +static irqreturn_t sunx8i_h3_irq_thread(int irq, void *data) > > +{ > > + struct sun4i_gpadc_iio *info = data; > > + int i; > > + > > + regmap_write(info->regmap, SUN8I_H3_THS_STAT, > > + info->data->ths_irq_clear); > > + > > + for (i = 0; i < info->data->sensor_count; i++) > > + thermal_zone_device_update(info->tzds[i].tzd, > > + THERMAL_EVENT_TEMP_SAMP > > LE); > > + > > + return IRQ_HANDLED; > > +} > > + > > +static int sun8i_h3_calibrate(struct sun4i_gpadc_iio *info) > > +{ > > +// regmap_write(info->regmap, SUNXI_THS_CDATA_0_1, > > +// info->calibration_data[0]); > > +// regmap_write(info->regmap, SUNXI_THS_CDATA_2_3, > > +// info->calibration_data[1]); > > Don't put commented code. Personally I suggest to leave out all SID or calibration related patches here. Currently we seems to be wrongly converting SID to big endian, however, the orgnization of the THS calibration data on H6 shows that it's surely little endian: It consists a temperature value in 1/10 celsuis as unit, and some thermal register readout values, which are the values read out at the given temperature, and every value here (the temperature and the readout) are all half word length. Let the temperature value be AABB, the two readout values be XXYY and ZZWW, the oragnization is: BB AA YY XX WW ZZ ** ** . When converting the SID to big endian, it becomes: XX YY AA BB ** ** ZZ WW , which is non-sense, and not able to do sub-word cell addressing. Maxime, should I drop the LE2BE conversion in SID driver? (I doubt whether it will break compatibility.) Philipp, could you delay to send any code that uses SID? > > > + > > + return 0; > > +} > > + > > static int sun4i_gpadc_runtime_suspend(struct device *dev) > > { > > struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev)); > > @@ -428,6 +474,16 @@ static int sun4i_ths_suspend(struct > > sun4i_gpadc_iio *info) > > return 0; > > } > > > > +static int sun8i_h3_ths_suspend(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_resume(struct device *dev) > > { > > struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev)); > > @@ -454,6 +510,37 @@ static int sun4i_ths_resume(struct > > sun4i_gpadc_iio *info) > > return 0; > > } > > > > +static int sun8i_h3_ths_resume(struct sun4i_gpadc_iio *info) > > +{ > > + u32 value; > > + > > + sun8i_h3_calibrate(info); > > + > > + regmap_write(info->regmap, SUN8I_H3_THS_CTRL0, > > + SUN4I_GPADC_CTRL0_T_ACQ(0xff)); > > + > > + regmap_write(info->regmap, SUN8I_H3_THS_CTRL2, > > + SUN8I_H3_THS_ACQ1(0x3f)); > > + > > + regmap_write(info->regmap, SUN8I_H3_THS_STAT, > > + SUN8I_H3_THS_INTS_TDATA_IRQ_0); > > + > > + regmap_write(info->regmap, SUN8I_H3_THS_FILTER, > > + SUN4I_GPADC_CTRL3_FILTER_EN | > > + SUN4I_GPADC_CTRL3_FILTER_TYPE(0x2)); > > + > > + regmap_write(info->regmap, SUN8I_H3_THS_INTC, > > + SUN8I_H3_THS_INTC_TDATA_IRQ_EN0 | > > + SUN8I_H3_THS_TEMP_PERIOD(0x55)); > > + > > + regmap_read(info->regmap, SUN8I_H3_THS_CTRL2, &value); > > + > > + regmap_write(info->regmap, SUN8I_H3_THS_CTRL2, > > + SUN8I_H3_THS_TEMP_SENSE_EN0 | value); > > Ideally, all these values should have a comment explaining what they > are. > > And we really start to have a lot of registers defines. We'd be > better > off using regmap_fields. > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 31.08.2018 11:51, Icenowy Zheng wrote: > Personally I suggest to leave out all SID or calibration related > patches here. > > Currently we seems to be wrongly converting SID to big endian, however, > the orgnization of the THS calibration data on H6 shows that it's > surely little endian: > > It consists a temperature value in 1/10 celsuis as unit, and some > thermal register readout values, which are the values read out at the > given temperature, and every value here (the temperature and the > readout) are all half word length. > > Let the temperature value be AABB, the two readout values be XXYY and > ZZWW, the oragnization is: > BB AA YY XX WW ZZ ** ** . > > When converting the SID to big endian, it becomes: > XX YY AA BB ** ** ZZ WW , > which is non-sense, and not able to do sub-word cell addressing. > > Maxime, should I drop the LE2BE conversion in SID driver? (I doubt > whether it will break compatibility.) > > Philipp, could you delay to send any code that uses SID? Yes for sure! I will move the related patches to the end of the series and add a DO-NOT-MERGE flag in the title. So I can get those also ready/reviewed but not merged. Icenowy, do you know more about the A83T SID? From the general specs it could be the same like on the A64 or the H3. Thanks, Philipp
On 31.08.2018 11:11, Maxime Ripard wrote: >> + regmap_write(info->regmap, SUN8I_H3_THS_CTRL0, >> + SUN4I_GPADC_CTRL0_T_ACQ(0xff)); >> + >> + regmap_write(info->regmap, SUN8I_H3_THS_CTRL2, >> + SUN8I_H3_THS_ACQ1(0x3f)); >> + >> + regmap_write(info->regmap, SUN8I_H3_THS_STAT, >> + SUN8I_H3_THS_INTS_TDATA_IRQ_0); >> + >> + regmap_write(info->regmap, SUN8I_H3_THS_FILTER, >> + SUN4I_GPADC_CTRL3_FILTER_EN | >> + SUN4I_GPADC_CTRL3_FILTER_TYPE(0x2)); >> + >> + regmap_write(info->regmap, SUN8I_H3_THS_INTC, >> + SUN8I_H3_THS_INTC_TDATA_IRQ_EN0 | >> + SUN8I_H3_THS_TEMP_PERIOD(0x55)); >> + >> + regmap_read(info->regmap, SUN8I_H3_THS_CTRL2, &value); >> + >> + regmap_write(info->regmap, SUN8I_H3_THS_CTRL2, >> + SUN8I_H3_THS_TEMP_SENSE_EN0 | value); > Ideally, all these values should have a comment explaining what they > are. > > And we really start to have a lot of registers defines. We'd be better > off using regmap_fields. I will rework this in the next version. Thanks, Philipp
On 31.08.2018 11:09, Maxime Ripard wrote: >> +static int sun4i_ths_suspend(struct sun4i_gpadc_iio *info) > suspend is already a hook in the kernel, which hasn't the same meaning > than runtime_suspend (and the same applies to resume), so we'd rather > pick a better name. And all the functions (and the driver) use gpadc, > please continue to use that prefix. I agree. For the newer sensors (from H3) the Sensor is referenced in the datasheets as Thermal Sensor short THS. So I would like to use for the newer sensors that prefix. Is that ok? Philipp
On Thu, 30 Aug 2018 17:44:50 +0200 Philipp Rossak <embed3d@gmail.com> wrote: > We are merging the mfd:sun4i-gpadc driver into the > iio/adc/sun4i-gpadc driver. So we need to remove the MFD_SUN4I_GPADC > config option. > > Signed-off-by: Philipp Rossak <embed3d@gmail.com> Do need to separate this and patch 1 as they are really two facets of the same thing. Jonathan > --- > drivers/mfd/Kconfig | 17 ----------------- > 1 file changed, 17 deletions(-) > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index b860eb5aa194..c7ab57d65610 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -40,23 +40,6 @@ config MFD_ACT8945A > linear regulators, along with a complete ActivePath battery > charger. > > -config MFD_SUN4I_GPADC > - tristate "Allwinner sunxi platforms' GPADC MFD driver" > - select MFD_CORE > - select REGMAP_MMIO > - select REGMAP_IRQ > - depends on ARCH_SUNXI || COMPILE_TEST > - depends on !TOUCHSCREEN_SUN4I > - help > - Select this to get support for Allwinner SoCs (A10, A13 and A31) ADC. > - This driver will only map the hardware interrupt and registers, you > - have to select individual drivers based on this MFD to be able to use > - the ADC or the thermal sensor. This will try to probe the ADC driver > - sun4i-gpadc-iio and the hwmon driver iio_hwmon. > - > - To compile this driver as a module, choose M here: the module will be > - called sun4i-gpadc. > - > config MFD_AS3711 > bool "AMS AS3711" > select MFD_CORE
On Fri, 31 Aug 2018 10:25:45 +0200 Maxime Ripard <maxime.ripard@bootlin.com> wrote: > Hi Philipp, > > First, thanks for doing that rework. It was needed, and it's very much > appreciated :) > > As you can imagine though, I have a bunch of comments. > > On Thu, Aug 30, 2018 at 05:44:49PM +0200, Philipp Rossak wrote: > > Since we are merging the mfd driver into the sun4i-gpadc driver we need > > to remove the build options for the sun4i-gpadc driver. > > > > Signed-off-by: Philipp Rossak <embed3d@gmail.com> > > --- > > drivers/mfd/Makefile | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > > index e9fd20dba18d..c680994db988 100644 > > --- a/drivers/mfd/Makefile > > +++ b/drivers/mfd/Makefile > > @@ -220,7 +220,6 @@ obj-$(CONFIG_INTEL_SOC_PMIC_CHTDC_TI) += intel_soc_pmic_chtdc_ti.o > > obj-$(CONFIG_MFD_MT6397) += mt6397-core.o > > > > obj-$(CONFIG_MFD_ALTERA_A10SR) += altera-a10sr.o > > -obj-$(CONFIG_MFD_SUN4I_GPADC) += sun4i-gpadc.o > > One of the things we should strive for is bisectability, which means > being able to have a working driver at every point in time while > introducing the features. > > In this particular case, this isn't really a problem since you're > removing part of code that were never really enabled, but you should > at least document why in your commit log. > Agreed. I for one don't know / can't remember why this would make sense! Jonathan > Thanks! > Maxime >
On Thu, 30 Aug 2018 17:44:53 +0200 Philipp Rossak <embed3d@gmail.com> wrote: > We are moving the SUN4I_GPADC_CHANNEL define to the header file. Maxime has raised this point in other patches... Why? Obvious what but I have no idea why you are doing this. Thanks, Jonathan > > Signed-off-by: Philipp Rossak <embed3d@gmail.com> > --- > drivers/iio/adc/sun4i-gpadc-iio.c | 9 --------- > include/linux/mfd/sun4i-gpadc.h | 9 +++++++++ > 2 files changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c > index d95dd0fde2a6..666329940e1e 100644 > --- a/drivers/iio/adc/sun4i-gpadc-iio.c > +++ b/drivers/iio/adc/sun4i-gpadc-iio.c > @@ -109,15 +109,6 @@ struct sun4i_gpadc_iio { > struct device *sensor_device; > }; > > -#define SUN4I_GPADC_ADC_CHANNEL(_channel, _name) { \ > - .type = IIO_VOLTAGE, \ > - .indexed = 1, \ > - .channel = _channel, \ > - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > - .datasheet_name = _name, \ > -} > - > static struct iio_map sun4i_gpadc_hwmon_maps[] = { > { > .adc_channel_label = "temp_adc", > diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h > index 139872c2e0fe..54c7c9375c1b 100644 > --- a/include/linux/mfd/sun4i-gpadc.h > +++ b/include/linux/mfd/sun4i-gpadc.h > @@ -90,6 +90,15 @@ > /* 10s delay before suspending the IP */ > #define SUN4I_GPADC_AUTOSUSPEND_DELAY 10000 > > +#define SUN4I_GPADC_ADC_CHANNEL(_channel, _name) { \ > + .type = IIO_VOLTAGE, \ > + .indexed = 1, \ > + .channel = _channel, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > + .datasheet_name = _name, \ > +} > + > struct sun4i_gpadc_dev { > struct device *dev; > struct regmap *regmap;
On Thu, 30 Aug 2018 17:45:06 +0200 Philipp Rossak <embed3d@gmail.com> 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> One additional comment inline.. Just a suggestion to make things slightly easier to read / review. Thanks, Jonathan > --- > drivers/iio/adc/sun4i-gpadc-iio.c | 63 +++++++++++++++++++++++++------------ > include/linux/iio/adc/sun4i-gpadc.h | 3 ++ > 2 files changed, 46 insertions(+), 20 deletions(-) > > diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c > index c12de48c4e86..18ab72e52d78 100644 > --- a/drivers/iio/adc/sun4i-gpadc-iio.c > +++ b/drivers/iio/adc/sun4i-gpadc-iio.c > @@ -69,6 +69,7 @@ struct gpadc_data { > bool has_bus_rst; > bool has_mod_clk; > u32 temp_data_base; > + int sensor_count; > }; > > static irqreturn_t sun4i_gpadc_data_irq_handler(int irq, void *dev_id); > @@ -84,6 +85,7 @@ static const struct gpadc_data sun4i_gpadc_data = { > .ths_irq_thread = sun4i_gpadc_data_irq_handler, > .support_irq = true, > .temp_data_base = SUN4I_GPADC_TEMP_DATA, > + .sensor_count = 1, > }; > > static const struct gpadc_data sun5i_gpadc_data = { > @@ -97,6 +99,7 @@ static const struct gpadc_data sun5i_gpadc_data = { > .ths_irq_thread = sun4i_gpadc_data_irq_handler, > .support_irq = true, > .temp_data_base = SUN4I_GPADC_TEMP_DATA, > + .sensor_count = 1, > }; > > static const struct gpadc_data sun6i_gpadc_data = { > @@ -110,6 +113,7 @@ static const struct gpadc_data sun6i_gpadc_data = { > .ths_irq_thread = sun4i_gpadc_data_irq_handler, > .support_irq = true, > .temp_data_base = SUN4I_GPADC_TEMP_DATA, > + .sensor_count = 1, > }; > > static const struct gpadc_data sun8i_a33_gpadc_data = { > @@ -117,6 +121,13 @@ static const struct gpadc_data sun8i_a33_gpadc_data = { > .temp_scale = 162, > .tp_mode_en = SUN8I_A33_GPADC_CTRL1_CHOP_TEMP_EN, > .temp_data_base = SUN4I_GPADC_TEMP_DATA, > + .sensor_count = 1, > +}; > + > +struct sun4i_sensor_tzd { > + struct sun4i_gpadc_iio *info; > + struct thermal_zone_device *tzd; > + unsigned int sensor_id; > }; > > struct sun4i_gpadc_iio { > @@ -130,7 +141,7 @@ struct sun4i_gpadc_iio { > const struct gpadc_data *data; > /* prevents concurrent reads of temperature and ADC */ > struct mutex mutex; > - struct thermal_zone_device *tzd; > + struct sun4i_sensor_tzd tzds[MAX_SENSOR_COUNT]; > struct device *sensor_device; > struct clk *bus_clk; > struct clk *mod_clk; > @@ -280,7 +291,8 @@ static int sun4i_gpadc_adc_read(struct iio_dev *indio_dev, int channel, > SUN4I_GPADC_IRQ_FIFO_DATA); > } > > -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); > > @@ -290,7 +302,8 @@ static int sun4i_gpadc_temp_read(struct iio_dev *indio_dev, int *val) > > pm_runtime_get_sync(indio_dev->dev.parent); > > - regmap_read(info->regmap, info->data->temp_data_base, val); > + regmap_read(info->regmap, info->data->temp_data_base + 0x4 * sensor, > + val); > > pm_runtime_mark_last_busy(indio_dev->dev.parent); > pm_runtime_put_autosuspend(indio_dev->dev.parent); > @@ -334,7 +347,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; > @@ -420,10 +433,11 @@ static int sun4i_gpadc_runtime_resume(struct device *dev) > > static int sun4i_gpadc_get_temp(void *data, int *temp) > { > - struct sun4i_gpadc_iio *info = data; > + struct sun4i_sensor_tzd *tzd = data; > + struct sun4i_gpadc_iio *info = tzd->info; > int val, scale, offset; > > - if (sun4i_gpadc_temp_read(info->indio_dev, &val)) > + if (sun4i_gpadc_temp_read(info->indio_dev, &val, tzd->sensor_id)) > return -ETIMEDOUT; > > sun4i_gpadc_temp_scale(info->indio_dev, &scale); > @@ -569,7 +583,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) > @@ -603,18 +617,24 @@ static int sun4i_gpadc_probe(struct platform_device *pdev) > pm_runtime_set_suspended(&pdev->dev); > pm_runtime_enable(&pdev->dev); > > - info->tzd = thermal_zone_of_sensor_register(info->sensor_device, > - 0, info, > - &sun4i_ts_tz_ops); > - /* > - * Do not fail driver probing when failing to register in > - * thermal because no thermal DT node is found. > - */ > - if (IS_ERR(info->tzd) && PTR_ERR(info->tzd) != -ENODEV) { > - dev_err(&pdev->dev, > - "could not register thermal sensor: %ld\n", > - PTR_ERR(info->tzd)); > - return PTR_ERR(info->tzd); > + for (i = 0; i < info->data->sensor_count; i++) { This feels like a good place to factor out the code into a utility function that just does one of them. That should hopefully reduce the indenting etc enough to make the code easier to read. > + info->tzds[i].info = info; > + info->tzds[i].sensor_id = i; > + > + info->tzds[i].tzd = thermal_zone_of_sensor_register( > + info->sensor_device, > + i, &info->tzds[i], &sun4i_ts_tz_ops); > + /* > + * Do not fail driver probing when failing to register in > + * thermal because no thermal DT node is found. > + */ > + if (IS_ERR(info->tzds[i].tzd) && \ > + PTR_ERR(info->tzds[i].tzd) != -ENODEV) { > + dev_err(&pdev->dev, > + "could not register thermal sensor: %ld\n", > + PTR_ERR(info->tzds[i].tzd)); > + return PTR_ERR(info->tzds[i].tzd); > + } > } > > ret = devm_iio_device_register(&pdev->dev, indio_dev); > @@ -639,11 +659,14 @@ static int sun4i_gpadc_remove(struct platform_device *pdev) > { > struct iio_dev *indio_dev = platform_get_drvdata(pdev); > struct sun4i_gpadc_iio *info = iio_priv(indio_dev); > + int i; > > pm_runtime_put(&pdev->dev); > pm_runtime_disable(&pdev->dev); > > - thermal_zone_of_sensor_unregister(info->sensor_device, info->tzd); > + for (i = 0; i < info->data->sensor_count; i++) > + thermal_zone_of_sensor_unregister(info->sensor_device, > + info->tzds[i].tzd); > > if (!info->data->support_irq) > iio_map_array_unregister(indio_dev); > diff --git a/include/linux/iio/adc/sun4i-gpadc.h b/include/linux/iio/adc/sun4i-gpadc.h > index d6850f39dcfb..99feeba28105 100644 > --- a/include/linux/iio/adc/sun4i-gpadc.h > +++ b/include/linux/iio/adc/sun4i-gpadc.h > @@ -99,4 +99,7 @@ > .datasheet_name = _name, \ > } > > +/* SUNXI_THS COMMON REGISTERS + DEFINES */ > +#define MAX_SENSOR_COUNT 4 > + > #endif
On Fri, Aug 31, 2018 at 02:05:59PM +0200, Philipp Rossak wrote: > > > On 31.08.2018 11:09, Maxime Ripard wrote: > > > +static int sun4i_ths_suspend(struct sun4i_gpadc_iio *info) > > suspend is already a hook in the kernel, which hasn't the same meaning > > than runtime_suspend (and the same applies to resume), so we'd rather > > pick a better name. And all the functions (and the driver) use gpadc, > > please continue to use that prefix. > > I agree. > For the newer sensors (from H3) the Sensor is referenced in the datasheets > as Thermal Sensor short THS. So I would like to use for the newer sensors > that prefix. Is that ok? Not really. The consistency within the driver is more important. Maxiem
On Fri, Aug 31, 2018 at 05:51:41PM +0800, Icenowy Zheng wrote: > Personally I suggest to leave out all SID or calibration related > patches here. > > Currently we seems to be wrongly converting SID to big endian, however, > the orgnization of the THS calibration data on H6 shows that it's > surely little endian: > > It consists a temperature value in 1/10 celsuis as unit, and some > thermal register readout values, which are the values read out at the > given temperature, and every value here (the temperature and the > readout) are all half word length. > > Let the temperature value be AABB, the two readout values be XXYY and > ZZWW, the oragnization is: > BB AA YY XX WW ZZ ** ** . > > When converting the SID to big endian, it becomes: > XX YY AA BB ** ** ZZ WW , > which is non-sense, and not able to do sub-word cell addressing. > > Maxime, should I drop the LE2BE conversion in SID driver? (I doubt > whether it will break compatibility.) This is exposed to the userspace, so no. Maxime >
于 2018年9月3日 GMT+08:00 下午6:20:22, Maxime Ripard <maxime.ripard@bootlin.com> 写到: >On Fri, Aug 31, 2018 at 05:51:41PM +0800, Icenowy Zheng wrote: >> Personally I suggest to leave out all SID or calibration related >> patches here. >> >> Currently we seems to be wrongly converting SID to big endian, >however, >> the orgnization of the THS calibration data on H6 shows that it's >> surely little endian: >> >> It consists a temperature value in 1/10 celsuis as unit, and some >> thermal register readout values, which are the values read out at the >> given temperature, and every value here (the temperature and the >> readout) are all half word length. >> >> Let the temperature value be AABB, the two readout values be XXYY and >> ZZWW, the oragnization is: >> BB AA YY XX WW ZZ ** ** . >> >> When converting the SID to big endian, it becomes: >> XX YY AA BB ** ** ZZ WW , >> which is non-sense, and not able to do sub-word cell addressing. >> >> Maxime, should I drop the LE2BE conversion in SID driver? (I doubt >> whether it will break compatibility.) > >This is exposed to the userspace, so no. Please note the LE2BE totally breaks the SID addressing. Without it dropped all cells must be referenced with 4 byte word as unit, and half word addressing of SID is thus not possible. The driver will also need then to split the half words if needed. I think this is kind of hardware misbehavior, and not a simple UAPI change, so the UAPI stability shouldn't affect this change. > >Maxime >>
On 02.09.2018 22:11, Jonathan Cameron wrote: > This feels like a good place to factor out the code into a utility > function that just does one of them. That should hopefully > reduce the indenting etc enough to make the code easier to read. > >> + info->tzds[i].info = info; >> + info->tzds[i].sensor_id = i; >> + >> + info->tzds[i].tzd = thermal_zone_of_sensor_register( >> + info->sensor_device, >> + i, &info->tzds[i], &sun4i_ts_tz_ops); >> + /* >> + * Do not fail driver probing when failing to register in >> + * thermal because no thermal DT node is found. >> + */ >> + if (IS_ERR(info->tzds[i].tzd) && \ >> + PTR_ERR(info->tzds[i].tzd) != -ENODEV) { >> + dev_err(&pdev->dev, >> + "could not register thermal sensor: %ld\n", >> + PTR_ERR(info->tzds[i].tzd)); >> + return PTR_ERR(info->tzds[i].tzd); >> + } So this code above should be placed in a separate function and called by the for loop? Did I understand that right? Philipp
On 02.09.2018 22:01, Jonathan Cameron wrote: > On Thu, 30 Aug 2018 17:44:53 +0200 > Philipp Rossak <embed3d@gmail.com> wrote: > >> We are moving the SUN4I_GPADC_CHANNEL define to the header file. > Maxime has raised this point in other patches... > > Why? Obvious what but I have no idea why you are doing this. > > Thanks, > > Jonathan There are two reasons: 1. Personal taste: I like to have the #define stuff in the header file. 2. When I started the rework I had to get some better overview, so I moved it... Since those two reasons are no good reasons to submit a patch I will drop it and keep it in the *.c file. Philipp >> >> Signed-off-by: Philipp Rossak <embed3d@gmail.com> >> --- >> drivers/iio/adc/sun4i-gpadc-iio.c | 9 --------- >> include/linux/mfd/sun4i-gpadc.h | 9 +++++++++ >> 2 files changed, 9 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c >> index d95dd0fde2a6..666329940e1e 100644 >> --- a/drivers/iio/adc/sun4i-gpadc-iio.c >> +++ b/drivers/iio/adc/sun4i-gpadc-iio.c >> @@ -109,15 +109,6 @@ struct sun4i_gpadc_iio { >> struct device *sensor_device; >> }; >> >> -#define SUN4I_GPADC_ADC_CHANNEL(_channel, _name) { \ >> - .type = IIO_VOLTAGE, \ >> - .indexed = 1, \ >> - .channel = _channel, \ >> - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ >> - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ >> - .datasheet_name = _name, \ >> -} >> - >> static struct iio_map sun4i_gpadc_hwmon_maps[] = { >> { >> .adc_channel_label = "temp_adc", >> diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h >> index 139872c2e0fe..54c7c9375c1b 100644 >> --- a/include/linux/mfd/sun4i-gpadc.h >> +++ b/include/linux/mfd/sun4i-gpadc.h >> @@ -90,6 +90,15 @@ >> /* 10s delay before suspending the IP */ >> #define SUN4I_GPADC_AUTOSUSPEND_DELAY 10000 >> >> +#define SUN4I_GPADC_ADC_CHANNEL(_channel, _name) { \ >> + .type = IIO_VOLTAGE, \ >> + .indexed = 1, \ >> + .channel = _channel, \ >> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ >> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ >> + .datasheet_name = _name, \ >> +} >> + >> struct sun4i_gpadc_dev { >> struct device *dev; >> struct regmap *regmap; >
On Mon, 3 Sep 2018 16:24:32 +0200 Philipp Rossak <embed3d@gmail.com> wrote: > On 02.09.2018 22:01, Jonathan Cameron wrote: > > On Thu, 30 Aug 2018 17:44:53 +0200 > > Philipp Rossak <embed3d@gmail.com> wrote: > > > >> We are moving the SUN4I_GPADC_CHANNEL define to the header file. > > Maxime has raised this point in other patches... > > > > Why? Obvious what but I have no idea why you are doing this. > > > > Thanks, > > > > Jonathan > There are two reasons: > 1. Personal taste: I like to have the #define stuff in the header file. > 2. When I started the rework I had to get some better overview, so I > moved it... > > Since those two reasons are no good reasons to submit a patch I will > drop it and keep it in the *.c file. Don't move it. For a 'utility' type define like this that is just about cutting down on code repetition it is nice to be able to see what it does near to where it is used. Also, as a general rule kernel style is to not put things in a header unless they are needed in multiple files. There are exceptions, but it is generally felt keeping things local to where they are used leads to easier to review code. Jonathan > > Philipp > > >> > >> Signed-off-by: Philipp Rossak <embed3d@gmail.com> > >> --- > >> drivers/iio/adc/sun4i-gpadc-iio.c | 9 --------- > >> include/linux/mfd/sun4i-gpadc.h | 9 +++++++++ > >> 2 files changed, 9 insertions(+), 9 deletions(-) > >> > >> diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c > >> index d95dd0fde2a6..666329940e1e 100644 > >> --- a/drivers/iio/adc/sun4i-gpadc-iio.c > >> +++ b/drivers/iio/adc/sun4i-gpadc-iio.c > >> @@ -109,15 +109,6 @@ struct sun4i_gpadc_iio { > >> struct device *sensor_device; > >> }; > >> > >> -#define SUN4I_GPADC_ADC_CHANNEL(_channel, _name) { \ > >> - .type = IIO_VOLTAGE, \ > >> - .indexed = 1, \ > >> - .channel = _channel, \ > >> - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > >> - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > >> - .datasheet_name = _name, \ > >> -} > >> - > >> static struct iio_map sun4i_gpadc_hwmon_maps[] = { > >> { > >> .adc_channel_label = "temp_adc", > >> diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h > >> index 139872c2e0fe..54c7c9375c1b 100644 > >> --- a/include/linux/mfd/sun4i-gpadc.h > >> +++ b/include/linux/mfd/sun4i-gpadc.h > >> @@ -90,6 +90,15 @@ > >> /* 10s delay before suspending the IP */ > >> #define SUN4I_GPADC_AUTOSUSPEND_DELAY 10000 > >> > >> +#define SUN4I_GPADC_ADC_CHANNEL(_channel, _name) { \ > >> + .type = IIO_VOLTAGE, \ > >> + .indexed = 1, \ > >> + .channel = _channel, \ > >> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > >> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > >> + .datasheet_name = _name, \ > >> +} > >> + > >> struct sun4i_gpadc_dev { > >> struct device *dev; > >> struct regmap *regmap; > >
On Mon, 3 Sep 2018 15:58:30 +0200 Philipp Rossak <embed3d@gmail.com> wrote: > On 02.09.2018 22:11, Jonathan Cameron wrote: > > This feels like a good place to factor out the code into a utility > > function that just does one of them. That should hopefully > > reduce the indenting etc enough to make the code easier to read. > > > >> + info->tzds[i].info = info; > >> + info->tzds[i].sensor_id = i; > >> + > >> + info->tzds[i].tzd = thermal_zone_of_sensor_register( > >> + info->sensor_device, > >> + i, &info->tzds[i], &sun4i_ts_tz_ops); > >> + /* > >> + * Do not fail driver probing when failing to register in > >> + * thermal because no thermal DT node is found. > >> + */ > >> + if (IS_ERR(info->tzds[i].tzd) && \ > >> + PTR_ERR(info->tzds[i].tzd) != -ENODEV) { > >> + dev_err(&pdev->dev, > >> + "could not register thermal sensor: %ld\n", > >> + PTR_ERR(info->tzds[i].tzd)); > >> + return PTR_ERR(info->tzds[i].tzd); > >> + } > > So this code above should be placed in a separate function and called by > the for loop? > Did I understand that right? Yes - exactly. > > Philipp
Hi Philipp, On Thu, 30 Aug 2018 17:45:15 +0200 Philipp Rossak <embed3d@gmail.com> wrote: > The H3 SID is supported by the kernel so we can add a NVMEM Data cell, > that contains the calibration data. > > On the H3 the eFuses are located at the offset 0x200. The thermal data > itself has an offset of 0x34 from the eFuse base. So we end on an offset > of 0x234. > > 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 1866aec69ec1..0fc447f0c02a 100644 > --- a/arch/arm/boot/dts/sun8i-h3.dtsi > +++ b/arch/arm/boot/dts/sun8i-h3.dtsi > @@ -106,8 +106,15 @@ > > soc { > sid: eeprom@1c14000 { > + #address-cells = <1>; > + #size-cells = <1>; > compatible = "allwinner,sun8i-h3-sid"; > reg = <0x01c14000 0x400>; > + > + /* Data cells */ > + thermal_calibration: calib@234 { > + reg = <0x234 0x8>; > + }; You are declaring 8 bytes of calibration data but to my knowledge it's only 2 bytes per sensor, so 2 bytes for H3. Am I missing something ? Thanks, > }; > }; > > @@ -227,4 +234,6 @@ > &ths { > compatible = "allwinner,sun8i-h3-ths"; > #thermal-sensor-cells = <0>; > + nvmem-cells = <&thermal_calibration>; > + nvmem-cell-names = "calibration"; > }; > -- > 2.11.0 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, Sep 03, 2018 at 07:01:47PM +0800, Icenowy Zheng wrote: > 于 2018年9月3日 GMT+08:00 下午6:20:22, Maxime Ripard <maxime.ripard@bootlin.com> 写到: > >On Fri, Aug 31, 2018 at 05:51:41PM +0800, Icenowy Zheng wrote: > >> Personally I suggest to leave out all SID or calibration related > >> patches here. > >> > >> Currently we seems to be wrongly converting SID to big endian, > >however, > >> the orgnization of the THS calibration data on H6 shows that it's > >> surely little endian: > >> > >> It consists a temperature value in 1/10 celsuis as unit, and some > >> thermal register readout values, which are the values read out at the > >> given temperature, and every value here (the temperature and the > >> readout) are all half word length. > >> > >> Let the temperature value be AABB, the two readout values be XXYY and > >> ZZWW, the oragnization is: > >> BB AA YY XX WW ZZ ** ** . > >> > >> When converting the SID to big endian, it becomes: > >> XX YY AA BB ** ** ZZ WW , > >> which is non-sense, and not able to do sub-word cell addressing. > >> > >> Maxime, should I drop the LE2BE conversion in SID driver? (I doubt > >> whether it will break compatibility.) > > > >This is exposed to the userspace, so no. > > Please note the LE2BE totally breaks the SID addressing. On which SoCs? > Without it dropped all cells must be referenced with 4 byte word as > unit, and half word addressing of SID is thus not possible. The > driver will also need then to split the half words if needed. > > I think this is kind of hardware misbehavior, and not a simple > UAPI change, so the UAPI stability shouldn't affect this change. How would you feel having your files on your filesystem suddenly not in the same endianness and having all the bytes reordered? This is definitely about the userspace ABI. Maxime
Hi Philipp, On Thu, Aug 30, 2018 at 05:45:18PM +0200, Philipp Rossak wrote: > Since we have now thermal trotteling enabeled we can now add the full > range of the OPP table. > That's not the reason why they were not added. Please see commit 2db639d8c1663d7543c9ab5323383d94c8a76c63[1]. Basically, you only want the OPPs which can work below or at the default voltage of the CPU supply, because the CPU supply is specific to each board. If you set your CPU to work at a given frequency and the voltage isn't updated (saying opp-microvolt = <x>; in DT isn't enough, you need cpu-supply to be provided and functional), the CPU might just crash. Without cpu-supply property, underclocking isn't effective in term of thermal cooling or power saving. Overclocking is very, very, very likely to make the CPU crash. It's not a very difficult thing to do to test if a given frequency work well but it needs a specific test environment and it's a lengthy test, you can have a look at those tools here[3] if you like. It's not because it works in a given test case that'll work on the long term under heavy load and constant frequency changes. For A83T, I already did it and the outcome is the patch in [1]. Same for A33. So, if you want to use these three higher OPPs, you need to define them in your board DTS and add the cpu-supply property. See what's done for the A33 and more specifically the Sinlinx SinA33[2] as an example. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2db639d8c1663d7543c9ab5323383d94c8a76c63 [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun8i-a33-sinlinx-sina33.dts [3] http://linux-sunxi.org/Hardware_Reliability_Tests#CPU Quentin > The operating points were found in Allwinner BSP and fex files. > > Signed-off-by: Philipp Rossak <embed3d@gmail.com> > --- > arch/arm/boot/dts/sun8i-a83t.dtsi | 32 ++++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/arch/arm/boot/dts/sun8i-a83t.dtsi b/arch/arm/boot/dts/sun8i-a83t.dtsi > index 78aa448e869f..ddcf404f9c80 100644 > --- a/arch/arm/boot/dts/sun8i-a83t.dtsi > +++ b/arch/arm/boot/dts/sun8i-a83t.dtsi > @@ -250,6 +250,22 @@ > opp-microvolt = <840000>; > clock-latency-ns = <244144>; /* 8 32k periods */ > }; > + > + opp-1608000000 { > + opp-hz = /bits/ 64 <1608000000>; > + opp-microvolt = <920000>; > + clock-latency-ns = <244144>; /* 8 32k periods */ > + }; > + opp-1800000000 { /* BOOT FREQ */ > + opp-hz = /bits/ 64 <1800000000>; > + opp-microvolt = <1000000>; > + clock-latency-ns = <244144>; /* 8 32k periods */ > + }; > + opp-2016000000 { > + opp-hz = /bits/ 64 <2016000000>; > + opp-microvolt = <1080000>; > + clock-latency-ns = <244144>; /* 8 32k periods */ > + }; > }; > > cpu1_opp_table: opp_table1 { > @@ -303,6 +319,22 @@ > opp-microvolt = <840000>; > clock-latency-ns = <244144>; /* 8 32k periods */ > }; > + > + opp-1608000000 { > + opp-hz = /bits/ 64 <1608000000>; > + opp-microvolt = <920000>; > + clock-latency-ns = <244144>; /* 8 32k periods */ > + }; > + opp-1800000000 { /* BOOT FREQ */ > + opp-hz = /bits/ 64 <1800000000>; > + opp-microvolt = <1000000>; > + clock-latency-ns = <244144>; /* 8 32k periods */ > + }; > + opp-2016000000 { > + opp-hz = /bits/ 64 <2016000000>; > + opp-microvolt = <1080000>; > + clock-latency-ns = <244144>; /* 8 32k periods */ > + }; > }; > > soc { > -- > 2.11.0 >
On 06.09.2018 09:24, Quentin Schulz wrote: > Hi Philipp, > > On Thu, Aug 30, 2018 at 05:45:18PM +0200, Philipp Rossak wrote: >> Since we have now thermal trotteling enabeled we can now add the full >> range of the OPP table. >> > That's not the reason why they were not added. > > Please see commit 2db639d8c1663d7543c9ab5323383d94c8a76c63[1]. > > Basically, you only want the OPPs which can work below or at the default > voltage of the CPU supply, because the CPU supply is specific to each > board. > > If you set your CPU to work at a given frequency and the voltage isn't > updated (saying opp-microvolt = <x>; in DT isn't enough, you need > cpu-supply to be provided and functional), the CPU might just crash. > > Without cpu-supply property, underclocking isn't effective in term of > thermal cooling or power saving. Overclocking is very, very, very likely > to make the CPU crash. > > It's not a very difficult thing to do to test if a given frequency work > well but it needs a specific test environment and it's a lengthy test, > you can have a look at those tools here[3] if you like. It's not because > it works in a given test case that'll work on the long term under heavy > load and constant frequency changes. > > For A83T, I already did it and the outcome is the patch in [1]. Same for > A33. > > So, if you want to use these three higher OPPs, you need to define them > in your board DTS and add the cpu-supply property. See what's done for > the A33 and more specifically the Sinlinx SinA33[2] as an example. > > [1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2db639d8c1663d7543c9ab5323383d94c8a76c63 > [2]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun8i-a33-sinlinx-sina33.dts > [3]http://linux-sunxi.org/Hardware_Reliability_Tests#CPU > > Quentin > Hey Quentin, thanks for your feedback! Sounds like we will never be able to run the A83T on its maximum frequency in mainline. I will do some testing, during the next weeks/months when I have time. With the old Allwinner kernel I was able to run the A83T with its maximum frequency without any problems since my board is very good cooled. For now I will drop this patch. Philipp
On Thu, Sep 06, 2018 at 01:39:43PM +0200, Philipp Rossak wrote: > On 06.09.2018 09:24, Quentin Schulz wrote: > > Hi Philipp, > > > > On Thu, Aug 30, 2018 at 05:45:18PM +0200, Philipp Rossak wrote: > > > Since we have now thermal trotteling enabeled we can now add the full > > > range of the OPP table. > > > > > That's not the reason why they were not added. > > > > Please see commit 2db639d8c1663d7543c9ab5323383d94c8a76c63[1]. > > > > Basically, you only want the OPPs which can work below or at the default > > voltage of the CPU supply, because the CPU supply is specific to each > > board. > > > > If you set your CPU to work at a given frequency and the voltage isn't > > updated (saying opp-microvolt = <x>; in DT isn't enough, you need > > cpu-supply to be provided and functional), the CPU might just crash. > > > > Without cpu-supply property, underclocking isn't effective in term of > > thermal cooling or power saving. Overclocking is very, very, very likely > > to make the CPU crash. > > > > It's not a very difficult thing to do to test if a given frequency work > > well but it needs a specific test environment and it's a lengthy test, > > you can have a look at those tools here[3] if you like. It's not because > > it works in a given test case that'll work on the long term under heavy > > load and constant frequency changes. > > > > For A83T, I already did it and the outcome is the patch in [1]. Same for > > A33. > > > > So, if you want to use these three higher OPPs, you need to define them > > in your board DTS and add the cpu-supply property. See what's done for > > the A33 and more specifically the Sinlinx SinA33[2] as an example. > > > > [1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2db639d8c1663d7543c9ab5323383d94c8a76c63 > > [2]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun8i-a33-sinlinx-sina33.dts > > [3]http://linux-sunxi.org/Hardware_Reliability_Tests#CPU > > > > Quentin > > > > Hey Quentin, > > thanks for your feedback! > > Sounds like we will never be able to run the A83T on its maximum frequency > in mainline. > > I will do some testing, during the next weeks/months when I have time. > With the old Allwinner kernel I was able to run the A83T with its maximum > frequency without any problems since my board is very good cooled. You definitely can, but I think Quentin's point was to do it on a per-board basis, not for all of the A83t boards at once. Maxime
On 04.09.2018 18:46, Emmanuel Vadot wrote: >> + /* Data cells */ >> + thermal_calibration: calib@234 { >> + reg = <0x234 0x8>; >> + }; > You are declaring 8 bytes of calibration data but to my knowledge it's > only 2 bytes per sensor, so 2 bytes for H3. > Am I missing something ? > > Thanks, Emmanuel you are right, it is 2 bytes per Sensor and should be 2 bytes for H3, but the thermal calibration data field is on all chips 64 bit wide - so 8 bytes. So I'm reading here the complete calibration data field. Philipp
On Thu, Sep 06, 2018 at 01:47:47PM +0200, Philipp Rossak wrote: > On 04.09.2018 18:46, Emmanuel Vadot wrote: > > > + /* Data cells */ > > > + thermal_calibration: calib@234 { > > > + reg = <0x234 0x8>; > > > + }; > > You are declaring 8 bytes of calibration data but to my knowledge it's > > only 2 bytes per sensor, so 2 bytes for H3. > > Am I missing something ? > > > > Thanks, > > Emmanuel you are right, it is 2 bytes per Sensor and should be 2 bytes for > H3, but the thermal calibration data field is on all chips 64 bit wide - so > 8 bytes. So I'm reading here the complete calibration data field. Having one cell per channel would make more sense I guess. Maxime
于 2018年9月6日 GMT+08:00 下午7:51:15, Maxime Ripard <maxime.ripard@bootlin.com> 写到: >On Thu, Sep 06, 2018 at 01:47:47PM +0200, Philipp Rossak wrote: >> On 04.09.2018 18:46, Emmanuel Vadot wrote: >> > > + /* Data cells */ >> > > + thermal_calibration: calib@234 { >> > > + reg = <0x234 0x8>; >> > > + }; >> > You are declaring 8 bytes of calibration data but to my knowledge >it's >> > only 2 bytes per sensor, so 2 bytes for H3. >> > Am I missing something ? >> > >> > Thanks, >> >> Emmanuel you are right, it is 2 bytes per Sensor and should be 2 >bytes for >> H3, but the thermal calibration data field is on all chips 64 bit >wide - so >> 8 bytes. So I'm reading here the complete calibration data field. > >Having one cell per channel would make more sense I guess. I have mentioned that this is impossible because of wrong addressing caused by LE2BE in SID driver. > >Maxime
Hi Maxime, Philipp, On Thu, Sep 06, 2018 at 01:42:41PM +0200, Maxime Ripard wrote: > On Thu, Sep 06, 2018 at 01:39:43PM +0200, Philipp Rossak wrote: > > On 06.09.2018 09:24, Quentin Schulz wrote: > > > Hi Philipp, > > > > > > On Thu, Aug 30, 2018 at 05:45:18PM +0200, Philipp Rossak wrote: > > > > Since we have now thermal trotteling enabeled we can now add the full > > > > range of the OPP table. > > > > > > > That's not the reason why they were not added. > > > > > > Please see commit 2db639d8c1663d7543c9ab5323383d94c8a76c63[1]. > > > > > > Basically, you only want the OPPs which can work below or at the default > > > voltage of the CPU supply, because the CPU supply is specific to each > > > board. > > > > > > If you set your CPU to work at a given frequency and the voltage isn't > > > updated (saying opp-microvolt = <x>; in DT isn't enough, you need > > > cpu-supply to be provided and functional), the CPU might just crash. > > > > > > Without cpu-supply property, underclocking isn't effective in term of > > > thermal cooling or power saving. Overclocking is very, very, very likely > > > to make the CPU crash. > > > > > > It's not a very difficult thing to do to test if a given frequency work > > > well but it needs a specific test environment and it's a lengthy test, > > > you can have a look at those tools here[3] if you like. It's not because > > > it works in a given test case that'll work on the long term under heavy > > > load and constant frequency changes. > > > > > > For A83T, I already did it and the outcome is the patch in [1]. Same for > > > A33. > > > > > > So, if you want to use these three higher OPPs, you need to define them > > > in your board DTS and add the cpu-supply property. See what's done for > > > the A33 and more specifically the Sinlinx SinA33[2] as an example. > > > > > > [1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2db639d8c1663d7543c9ab5323383d94c8a76c63 > > > [2]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun8i-a33-sinlinx-sina33.dts > > > [3]http://linux-sunxi.org/Hardware_Reliability_Tests#CPU > > > > > > Quentin > > > > > > > Hey Quentin, > > > > thanks for your feedback! > > > > Sounds like we will never be able to run the A83T on its maximum frequency > > in mainline. > > > > I will do some testing, during the next weeks/months when I have time. > > With the old Allwinner kernel I was able to run the A83T with its maximum > > frequency without any problems since my board is very good cooled. > > You definitely can, but I think Quentin's point was to do it on a > per-board basis, not for all of the A83t boards at once. > Yes, exactly. That's what we did for the Sinlinx SinA33. Just take inspiration from it. You can run the CPU at max frequency with the old Allwinner kernel because it most likely updates the CPU voltage. Let's say overheating is not an issue, you still need to have a way to overclock AND overvolt to the values in the OPP you're targetting. Your CPU won't be stable with just overclocking without overvolting it, trust me, I've been there. It can even be possible that the Allwinner BSP is only stable in your use case. The CPU voltage is specific to every board (cpu-supply property). So we do not declare the CPU frequencies that require overvolting since we cannot be sure a board will define the cpu-supply property and thus, be able to overvolt the CPU. So, though thermal throttling is an important feature because overheating the CPU too much will shut it down on the hardware level (among other consequences), it does not make the higher OPPs magically work without a valid and working cpu-supply. The link I gave you is an important piece of software to test the stability of a system under heavy load and a lot of frequency changes. It's important to know that because it works for you in your use case doesn't make it work in any use case. Testing with these pieces of software helps to cover more hardcore use cases but isn't perfect of course. That's a start though. Quentin
On 06.09.2018 14:04, Icenowy Zheng wrote: > > > 于 2018年9月6日 GMT+08:00 下午7:51:15, Maxime Ripard <maxime.ripard@bootlin.com> 写到: >> On Thu, Sep 06, 2018 at 01:47:47PM +0200, Philipp Rossak wrote: >>> On 04.09.2018 18:46, Emmanuel Vadot wrote: >>>>> + /* Data cells */ >>>>> + thermal_calibration: calib@234 { >>>>> + reg = <0x234 0x8>; >>>>> + }; >>>> You are declaring 8 bytes of calibration data but to my knowledge >> it's >>>> only 2 bytes per sensor, so 2 bytes for H3. >>>> Am I missing something ? >>>> >>>> Thanks, >>> >>> Emmanuel you are right, it is 2 bytes per Sensor and should be 2 >> bytes for >>> H3, but the thermal calibration data field is on all chips 64 bit >> wide - so >>> 8 bytes. So I'm reading here the complete calibration data field. >> >> Having one cell per channel would make more sense I guess. Ok I will change this. > > I have mentioned that this is impossible because of wrong > addressing caused by LE2BE in SID driver. > I know! But I would like to prepare patches for it, that they can be merged when this is fixed. Philipp
Sorry for resurrecting an old discussion, but since someone posted patches for H5 and H6, I thought we should resolve this. I'm working on patches to fix / replace the big-endian issue. On Thu, Sep 6, 2018 at 7:51 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > On Thu, Sep 06, 2018 at 01:47:47PM +0200, Philipp Rossak wrote: > > On 04.09.2018 18:46, Emmanuel Vadot wrote: > > > > + /* Data cells */ > > > > + thermal_calibration: calib@234 { > > > > + reg = <0x234 0x8>; > > > > + }; > > > You are declaring 8 bytes of calibration data but to my knowledge it's > > > only 2 bytes per sensor, so 2 bytes for H3. > > > Am I missing something ? > > > > > > Thanks, > > > > Emmanuel you are right, it is 2 bytes per Sensor and should be 2 bytes for > > H3, but the thermal calibration data field is on all chips 64 bit wide - so > > 8 bytes. So I'm reading here the complete calibration data field. > > Having one cell per channel would make more sense I guess. Would it? The 2 32-bit words directly map onto the registers 0x74 / 0x78 in the THS. As far as the SID is concerned, their is just one consumer for this data, the thermal sensor. How the thermal sensor uses that data is really not its concern. And the thermal sensor is really just copying the data from the e-fuses into its registers. Nothing more. Furthermore, with the register access interface, the e-fuses are read/write 32 bits at a time. Seems to me it would make more sense to enforce a 32-bit word size, so cells should be multiples of 32 bits. Regards ChenYu
On Tue, Feb 19, 2019 at 03:54:00PM +0800, Chen-Yu Tsai wrote: > Sorry for resurrecting an old discussion, but since someone posted patches > for H5 and H6, I thought we should resolve this. I'm working on patches to > fix / replace the big-endian issue. > > On Thu, Sep 6, 2018 at 7:51 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > On Thu, Sep 06, 2018 at 01:47:47PM +0200, Philipp Rossak wrote: > > > On 04.09.2018 18:46, Emmanuel Vadot wrote: > > > > > + /* Data cells */ > > > > > + thermal_calibration: calib@234 { > > > > > + reg = <0x234 0x8>; > > > > > + }; > > > > You are declaring 8 bytes of calibration data but to my knowledge it's > > > > only 2 bytes per sensor, so 2 bytes for H3. > > > > Am I missing something ? > > > > > > > > Thanks, > > > > > > Emmanuel you are right, it is 2 bytes per Sensor and should be 2 bytes for > > > H3, but the thermal calibration data field is on all chips 64 bit wide - so > > > 8 bytes. So I'm reading here the complete calibration data field. > > > > Having one cell per channel would make more sense I guess. > > Would it? The 2 32-bit words directly map onto the registers 0x74 / 0x78 in > the THS. As far as the SID is concerned, their is just one consumer for this > data, the thermal sensor. How the thermal sensor uses that data is really not > its concern. And the thermal sensor is really just copying the data from the > e-fuses into its registers. Nothing more. > > Furthermore, with the register access interface, the e-fuses are read/write > 32 bits at a time. Seems to me it would make more sense to enforce a 32-bit > word size, so cells should be multiples of 32 bits. I guess you convinced me :) Maxime
On Tue, 19 Feb 2019 15:54:00 +0800 Chen-Yu Tsai <wens@csie.org> wrote: > Sorry for resurrecting an old discussion, but since someone posted patches > for H5 and H6, I thought we should resolve this. I'm working on patches to > fix / replace the big-endian issue. > > On Thu, Sep 6, 2018 at 7:51 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > On Thu, Sep 06, 2018 at 01:47:47PM +0200, Philipp Rossak wrote: > > > On 04.09.2018 18:46, Emmanuel Vadot wrote: > > > > > + /* Data cells */ > > > > > + thermal_calibration: calib@234 { > > > > > + reg = <0x234 0x8>; > > > > > + }; > > > > You are declaring 8 bytes of calibration data but to my knowledge it's > > > > only 2 bytes per sensor, so 2 bytes for H3. > > > > Am I missing something ? > > > > > > > > Thanks, > > > > > > Emmanuel you are right, it is 2 bytes per Sensor and should be 2 bytes for > > > H3, but the thermal calibration data field is on all chips 64 bit wide - so > > > 8 bytes. So I'm reading here the complete calibration data field. > > > > Having one cell per channel would make more sense I guess. > > Would it? The 2 32-bit words directly map onto the registers 0x74 / 0x78 in > the THS. As far as the SID is concerned, their is just one consumer for this > data, the thermal sensor. How the thermal sensor uses that data is really not > its concern. And the thermal sensor is really just copying the data from the > e-fuses into its registers. Nothing more. If you see this at the controller lever and not at the sensor level it might make sense to use the full 32bits data even some bits are ignored I guess. > Furthermore, with the register access interface, the e-fuses are read/write > 32 bits at a time. Seems to me it would make more sense to enforce a 32-bit > word size, so cells should be multiples of 32 bits. Ok, honestly at this point I don't really care what choice is done as long as the dts is updated, I'll deal with the diff for FreeBSD and ping the others BSD (Net and Open who also have a driver) for this change. > Regards > ChenYu Cheers,
On 19.02.19 08:54, Chen-Yu Tsai wrote: > Sorry for resurrecting an old discussion, but since someone posted patches > for H5 and H6, I thought we should resolve this. I'm working on patches to > fix / replace the big-endian issue. > > On Thu, Sep 6, 2018 at 7:51 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: >> >> On Thu, Sep 06, 2018 at 01:47:47PM +0200, Philipp Rossak wrote: >>> On 04.09.2018 18:46, Emmanuel Vadot wrote: >>>>> + /* Data cells */ >>>>> + thermal_calibration: calib@234 { >>>>> + reg = <0x234 0x8>; >>>>> + }; >>>> You are declaring 8 bytes of calibration data but to my knowledge it's >>>> only 2 bytes per sensor, so 2 bytes for H3. >>>> Am I missing something ? >>>> >>>> Thanks, >>> >>> Emmanuel you are right, it is 2 bytes per Sensor and should be 2 bytes for >>> H3, but the thermal calibration data field is on all chips 64 bit wide - so >>> 8 bytes. So I'm reading here the complete calibration data field. >> >> Having one cell per channel would make more sense I guess. > > Would it? The 2 32-bit words directly map onto the registers 0x74 / 0x78 in > the THS. As far as the SID is concerned, their is just one consumer for this > data, the thermal sensor. How the thermal sensor uses that data is really not > its concern. And the thermal sensor is really just copying the data from the > e-fuses into its registers. Nothing more. Using 2 32-bit words for the THS would be also ok (from my perspective). > > Furthermore, with the register access interface, the e-fuses are read/write > 32 bits at a time. Seems to me it would make more sense to enforce a 32-bit > word size, so cells should be multiples of 32 bits. > For THS I'm ok with that. > Regards > ChenYu > Regards, Philipp
Philipp Rossak <embed3d@gmail.com> writes: > 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 > > This Patchtseries was in the beginning based on Icenowy Zengs v4 patchseries [1]. Since we decided to merge the mfd driver into the GPADC this changed. So only one patch could be reused. > > Patches that adds support for H5, A64, A80 and H6 SoCs are allready prepared, > and will be upstreamed if this patchseries is applied and the testing is done. > > Sorry for delaying this. > > Regards, > Philipp > > changes since v2: > * mfd driver is now merged into the gpadc driver > * complete rework > > 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 (29): > mfd: Makefile: Remove build option for MFD:sun4i-gpadc > mfd: Kconfig: Remove MFD_SUN4I_GPADC config option > iio: adc: Remove ID table > iio: adc: Kconfig: Update Kconfig to new build options > iio: adc: move SUN4I_GPADC_CHANNEL define to header file > iio: adc: remove ofnode options > iio: adc: remove mfd_probe & sunwi_irq_init function > iio: adc: remove hwmon structure > iio: adc: Threat A33 as thermal sensor and remove non thermal sun4i > channel > iio: adc: rework irq and adc_channel handling > iio: adc: add new compatibles > mfd: Remove old mfd driver & Move sun4i-gpadc.h to iio/adc/ > arm: config: Enable SUN4I_GPADC in defconfig > dt-bindings: update the Allwinner GPADC device tree binding for H3 & > A83T > iio: adc: sun4i-gpadc-iio: rework: readout temp_data > 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: device specific suspend & resume > 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: h3: use calibration for ths > ARM: dts: sun8i: a83t: add support for the thermal sensor in A83T > ARM: dts: sun8i: a83t: add thermal zone to A83T > ARM: sun8i: a83t: full range OPP tables and CPUfreq > > .../devicetree/bindings/iio/adc/sun4i-gpadc.txt | 41 +- > arch/arm/boot/dts/sun8i-a83t.dtsi | 143 +++++ > arch/arm/boot/dts/sun8i-h3.dtsi | 52 ++ > arch/arm/boot/dts/sunxi-h3-h5.dtsi | 10 + > arch/arm/configs/sunxi_defconfig | 1 + > drivers/iio/adc/Kconfig | 11 +- > drivers/iio/adc/sun4i-gpadc-iio.c | 617 +++++++++++++-------- > drivers/mfd/Kconfig | 17 - > drivers/mfd/Makefile | 1 - > drivers/mfd/sun4i-gpadc.c | 181 ------ > include/linux/{mfd => iio/adc}/sun4i-gpadc.h | 47 +- > 11 files changed, 681 insertions(+), 440 deletions(-) > delete mode 100644 drivers/mfd/sun4i-gpadc.c > rename include/linux/{mfd => iio/adc}/sun4i-gpadc.h (72%) What became of these patches? I can't seem to find any follow-ups. Was the approach abandoned in favour of something else?
On Tue, Mar 19, 2019 at 12:30:25PM +0000, Måns Rullgård wrote: > Philipp Rossak <embed3d@gmail.com> writes: > > > 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 > > > > This Patchtseries was in the beginning based on Icenowy Zengs v4 patchseries [1]. Since we decided to merge the mfd driver into the GPADC this changed. So only one patch could be reused. > > > > Patches that adds support for H5, A64, A80 and H6 SoCs are allready prepared, > > and will be upstreamed if this patchseries is applied and the testing is done. > > > > Sorry for delaying this. > > > > Regards, > > Philipp > > > > changes since v2: > > * mfd driver is now merged into the gpadc driver > > * complete rework > > > > 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 (29): > > mfd: Makefile: Remove build option for MFD:sun4i-gpadc > > mfd: Kconfig: Remove MFD_SUN4I_GPADC config option > > iio: adc: Remove ID table > > iio: adc: Kconfig: Update Kconfig to new build options > > iio: adc: move SUN4I_GPADC_CHANNEL define to header file > > iio: adc: remove ofnode options > > iio: adc: remove mfd_probe & sunwi_irq_init function > > iio: adc: remove hwmon structure > > iio: adc: Threat A33 as thermal sensor and remove non thermal sun4i > > channel > > iio: adc: rework irq and adc_channel handling > > iio: adc: add new compatibles > > mfd: Remove old mfd driver & Move sun4i-gpadc.h to iio/adc/ > > arm: config: Enable SUN4I_GPADC in defconfig > > dt-bindings: update the Allwinner GPADC device tree binding for H3 & > > A83T > > iio: adc: sun4i-gpadc-iio: rework: readout temp_data > > 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: device specific suspend & resume > > 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: h3: use calibration for ths > > ARM: dts: sun8i: a83t: add support for the thermal sensor in A83T > > ARM: dts: sun8i: a83t: add thermal zone to A83T > > ARM: sun8i: a83t: full range OPP tables and CPUfreq > > > > .../devicetree/bindings/iio/adc/sun4i-gpadc.txt | 41 +- > > arch/arm/boot/dts/sun8i-a83t.dtsi | 143 +++++ > > arch/arm/boot/dts/sun8i-h3.dtsi | 52 ++ > > arch/arm/boot/dts/sunxi-h3-h5.dtsi | 10 + > > arch/arm/configs/sunxi_defconfig | 1 + > > drivers/iio/adc/Kconfig | 11 +- > > drivers/iio/adc/sun4i-gpadc-iio.c | 617 +++++++++++++-------- > > drivers/mfd/Kconfig | 17 - > > drivers/mfd/Makefile | 1 - > > drivers/mfd/sun4i-gpadc.c | 181 ------ > > include/linux/{mfd => iio/adc}/sun4i-gpadc.h | 47 +- > > 11 files changed, 681 insertions(+), 440 deletions(-) > > delete mode 100644 drivers/mfd/sun4i-gpadc.c > > rename include/linux/{mfd => iio/adc}/sun4i-gpadc.h (72%) > > What became of these patches? I can't seem to find any follow-ups. Was > the approach abandoned in favour of something else? It didn't, I don't think we ever got any follow-up to that series so that's the main reason it's been held back. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Tue, Mar 19, 2019 at 8:37 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > On Tue, Mar 19, 2019 at 12:30:25PM +0000, Måns Rullgård wrote: > > Philipp Rossak <embed3d@gmail.com> writes: > > > > > 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 > > > > > > This Patchtseries was in the beginning based on Icenowy Zengs v4 patchseries [1]. Since we decided to merge the mfd driver into the GPADC this changed. So only one patch could be reused. > > > > > > Patches that adds support for H5, A64, A80 and H6 SoCs are allready prepared, > > > and will be upstreamed if this patchseries is applied and the testing is done. > > > > > > Sorry for delaying this. > > > > > > Regards, > > > Philipp > > > > > > changes since v2: > > > * mfd driver is now merged into the gpadc driver > > > * complete rework > > > > > > 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 (29): > > > mfd: Makefile: Remove build option for MFD:sun4i-gpadc > > > mfd: Kconfig: Remove MFD_SUN4I_GPADC config option > > > iio: adc: Remove ID table > > > iio: adc: Kconfig: Update Kconfig to new build options > > > iio: adc: move SUN4I_GPADC_CHANNEL define to header file > > > iio: adc: remove ofnode options > > > iio: adc: remove mfd_probe & sunwi_irq_init function > > > iio: adc: remove hwmon structure > > > iio: adc: Threat A33 as thermal sensor and remove non thermal sun4i > > > channel > > > iio: adc: rework irq and adc_channel handling > > > iio: adc: add new compatibles > > > mfd: Remove old mfd driver & Move sun4i-gpadc.h to iio/adc/ > > > arm: config: Enable SUN4I_GPADC in defconfig > > > dt-bindings: update the Allwinner GPADC device tree binding for H3 & > > > A83T > > > iio: adc: sun4i-gpadc-iio: rework: readout temp_data > > > 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: device specific suspend & resume > > > 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: h3: use calibration for ths > > > ARM: dts: sun8i: a83t: add support for the thermal sensor in A83T > > > ARM: dts: sun8i: a83t: add thermal zone to A83T > > > ARM: sun8i: a83t: full range OPP tables and CPUfreq > > > > > > .../devicetree/bindings/iio/adc/sun4i-gpadc.txt | 41 +- > > > arch/arm/boot/dts/sun8i-a83t.dtsi | 143 +++++ > > > arch/arm/boot/dts/sun8i-h3.dtsi | 52 ++ > > > arch/arm/boot/dts/sunxi-h3-h5.dtsi | 10 + > > > arch/arm/configs/sunxi_defconfig | 1 + > > > drivers/iio/adc/Kconfig | 11 +- > > > drivers/iio/adc/sun4i-gpadc-iio.c | 617 +++++++++++++-------- > > > drivers/mfd/Kconfig | 17 - > > > drivers/mfd/Makefile | 1 - > > > drivers/mfd/sun4i-gpadc.c | 181 ------ > > > include/linux/{mfd => iio/adc}/sun4i-gpadc.h | 47 +- > > > 11 files changed, 681 insertions(+), 440 deletions(-) > > > delete mode 100644 drivers/mfd/sun4i-gpadc.c > > > rename include/linux/{mfd => iio/adc}/sun4i-gpadc.h (72%) > > > > What became of these patches? I can't seem to find any follow-ups. Was > > the approach abandoned in favour of something else? > > It didn't, I don't think we ever got any follow-up to that series so > that's the main reason it's been held back. IIRC it was blocked by a disagreement on how to handle the calibration data in the SID. Hopefully this is now resolved. ChenYu
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 This Patchtseries was in the beginning based on Icenowy Zengs v4 patchseries [1]. Since we decided to merge the mfd driver into the GPADC this changed. So only one patch could be reused. Patches that adds support for H5, A64, A80 and H6 SoCs are allready prepared, and will be upstreamed if this patchseries is applied and the testing is done. Sorry for delaying this. Regards, Philipp changes since v2: * mfd driver is now merged into the gpadc driver * complete rework 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 (29): mfd: Makefile: Remove build option for MFD:sun4i-gpadc mfd: Kconfig: Remove MFD_SUN4I_GPADC config option iio: adc: Remove ID table iio: adc: Kconfig: Update Kconfig to new build options iio: adc: move SUN4I_GPADC_CHANNEL define to header file iio: adc: remove ofnode options iio: adc: remove mfd_probe & sunwi_irq_init function iio: adc: remove hwmon structure iio: adc: Threat A33 as thermal sensor and remove non thermal sun4i channel iio: adc: rework irq and adc_channel handling iio: adc: add new compatibles mfd: Remove old mfd driver & Move sun4i-gpadc.h to iio/adc/ arm: config: Enable SUN4I_GPADC in defconfig dt-bindings: update the Allwinner GPADC device tree binding for H3 & A83T iio: adc: sun4i-gpadc-iio: rework: readout temp_data 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: device specific suspend & resume 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: h3: use calibration for ths ARM: dts: sun8i: a83t: add support for the thermal sensor in A83T ARM: dts: sun8i: a83t: add thermal zone to A83T ARM: sun8i: a83t: full range OPP tables and CPUfreq .../devicetree/bindings/iio/adc/sun4i-gpadc.txt | 41 +- arch/arm/boot/dts/sun8i-a83t.dtsi | 143 +++++ arch/arm/boot/dts/sun8i-h3.dtsi | 52 ++ arch/arm/boot/dts/sunxi-h3-h5.dtsi | 10 + arch/arm/configs/sunxi_defconfig | 1 + drivers/iio/adc/Kconfig | 11 +- drivers/iio/adc/sun4i-gpadc-iio.c | 617 +++++++++++++-------- drivers/mfd/Kconfig | 17 - drivers/mfd/Makefile | 1 - drivers/mfd/sun4i-gpadc.c | 181 ------ include/linux/{mfd => iio/adc}/sun4i-gpadc.h | 47 +- 11 files changed, 681 insertions(+), 440 deletions(-) delete mode 100644 drivers/mfd/sun4i-gpadc.c rename include/linux/{mfd => iio/adc}/sun4i-gpadc.h (72%)