Re: [PATCH v4 4/6] iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor

[Quentin Schulz]

Hi Icenowy,

On 14/09/2017 16:52, Icenowy Zheng wrote:
> This adds support for the Allwinner H3 thermal sensor.
> 
> Allwinner H3 has a thermal sensor like the one in A33, but have its
> registers nearly all re-arranged, sample clock moved to CCU and a pair
> of bus clock and reset added. It's also the base of newer SoCs' thermal
> sensors.
> 
> The thermal sensors on A64 and H5 is like the one on H3, but with of
> course different formula factors.
> 
> Signed-off-by: Icenowy Zheng <icenowy@xxxxxxx>
> ---
> Changes in v4:
> - Splitted out some code refactors.
> - Code sequence changed back. (The gpadc_data went back to the start of
>   the source file)
> 
>  drivers/iio/adc/sun4i-gpadc-iio.c | 48 +++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/sun4i-gpadc.h   | 27 ++++++++++++++++++++++
>  2 files changed, 75 insertions(+)
[...]
> diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h
> index 78d31984a222..5c2a12101052 100644
> --- a/include/linux/mfd/sun4i-gpadc.h
> +++ b/include/linux/mfd/sun4i-gpadc.h
[...]
> +#define SUN8I_H3_GPADC_CTRL2_T_ACQ1(x)			((GENMASK(15, 0) * (x)) << 16)
> +

You want to replace * by &.

((GENMASK(15, 0) & (x)) << 16)

Would ((GENMASK(31, 16) & ((x) << 16)) make the bits you set even more
obvious?

>  #define SUN4I_GPADC_CTRL3				0x0c
> +/*
> + * This register is named "Average filter Control Register" in H3 Datasheet,
> + * but the register's definition is the same as the old CTRL3 register.
> + */
> +#define SUN8I_H3_GPADC_CTRL3				0x70
>  

I would name it as it is in the documentation:
SUN8I_H3_THS_FILTER

No need for comments then.

>  #define SUN4I_GPADC_CTRL3_FILTER_EN			BIT(2)
>  #define SUN4I_GPADC_CTRL3_FILTER_TYPE(x)		(GENMASK(1, 0) & (x))
> @@ -71,6 +84,13 @@
>  #define SUN4I_GPADC_INT_FIFOC_TP_UP_IRQ_EN		BIT(1)
>  #define SUN4I_GPADC_INT_FIFOC_TP_DOWN_IRQ_EN		BIT(0)
>  
> +#define SUN8I_H3_GPADC_INTC				0x44
> +
> +#define SUN8I_H3_GPADC_INTC_TEMP_PERIOD(x)		((GENMASK(19, 0) & (x)) << 12)
> +#define SUN8I_H3_GPADC_INTC_TEMP_DATA			BIT(8)
> +#define SUN8I_H3_GPADC_INTC_TEMP_SHUT			BIT(4)
> +#define SUN8I_H3_GPADC_INTC_TEMP_ALARM			BIT(0)
> +

Since it isn't an ADC anymore but rather just a THS, why don't you use
SUN8I_H3_THS instead of SUN8I_H3_GPADC? That way, it also matches the
datasheet.

>  #define SUN4I_GPADC_INT_FIFOS				0x14
>  
>  #define SUN4I_GPADC_INT_FIFOS_TEMP_DATA_PENDING		BIT(18)
> @@ -80,9 +100,16 @@
>  #define SUN4I_GPADC_INT_FIFOS_TP_UP_PENDING		BIT(1)
>  #define SUN4I_GPADC_INT_FIFOS_TP_DOWN_PENDING		BIT(0)
>  
> +#define SUN8I_H3_GPADC_INTS				0x44

0x48

[...]

1) You're not using irqs, why would you define registers that will never
be used?

2) Why aren't you using irqs? I remember we discussed on IRC that you
had some problems with the H3 when resuming or when probing the driver.
The register would have a zero in it until you have a first sample that
arrived (i.e. after the sample rate you set with T_ACQ) that would make
the thermal framework panic since the thermal sensor would return
something way too hot and shutdown your board?

The H3 apparently supports IRQs, why do you not support them for the
temperature? They might be broken as it is on A33 but then it might be a
good idea to write it down in a comment in the driver (and not adding
the unused registers in the header file) or at least in the commit log.

3) Now that you have support for clocks, wouldn't it be a good idea to
disable them during suspend?

Thanks,
Quentin
-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com