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

From: Jonathan Cameron
Date: Sat Sep 16 2017 - 18:16:40 EST


On Sat, 16 Sep 2017 11:45:47 +0200
Quentin Schulz <quentin.schulz@xxxxxxxxxxxxxxxxxx> wrote:

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

Agreed. Would act as better 'documentation'.

Jonathan
>
> > #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