lkml.org 
[lkml]   [2017]   [Sep]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v4 4/6] iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor
在 2017-09-16 17:45,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@aosc.io>
>> ---
>> 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

The definition of this register is the same as the CTRL3.

Maybe this name is better, but the similarity between them still needs
to be documented, as the SUN4I_GPADC_CTRL3_XXX macros will be used to
populate this register.

>
> 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?

I will then rework it to use IRQs, but not now.

Maybe I should add it when I use them?

>
> 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?

Nope, it's another problem -- the runtime resume function is even not
called before the first sample, and the first sample will happen when
the THS is still suspended.

>
> 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?

Interesting... It's meaningful to disable the mod clock during suspend.

>
> Thanks,
> Quentin

\
 
 \ /
  Last update: 2017-09-16 12:14    [W:0.345 / U:2.804 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site