Re: [PATCH v9,1/7] thermal: mediatek: Relocate driver to mediatek folder

[Balsam CHIHI <bchihi@baylibre.com>]

Hi Angelo,

Nice to hear from you again.
Understood. The changes you asked for will be implemented in the v10
of this series.
It will be sent very soon, I'm still working on Daniel's review too.

Thank you for the review!

Best regards,
Balsam

On Wed, Sep 14, 2022 at 1:58 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 17/08/22 10:07, bchihi@baylibre.com ha scritto:
> > From: Michael Kao <michael.kao@mediatek.com>
> >
> > Add MediaTek proprietary folder to upstream more thermal zone and cooler
> > drivers, relocate the original thermal controller driver to it, and rename it
> > as "auxadc_thermal.c" to show its purpose more clearly.
> >
> > Signed-off-by: Michael Kao <michael.kao@mediatek.com>
> > Co-developed-by: Ben Tseng <ben.tseng@mediatek.com>
> > Signed-off-by: Ben Tseng <ben.tseng@mediatek.com>
> > Co-developed-by: Balsam CHIHI <bchihi@baylibre.com>
> > Signed-off-by: Balsam CHIHI <bchihi@baylibre.com>
> > ---
> >   drivers/thermal/Kconfig                       | 14 ++++--------
> >   drivers/thermal/Makefile                      |  2 +-
> >   drivers/thermal/mediatek/Kconfig              | 22 +++++++++++++++++++
> >   drivers/thermal/mediatek/Makefile             |  1 +
> >   .../auxadc_thermal.c}                         |  2 +-
> >   5 files changed, 29 insertions(+), 12 deletions(-)
> >   create mode 100644 drivers/thermal/mediatek/Kconfig
> >   create mode 100644 drivers/thermal/mediatek/Makefile
> >   rename drivers/thermal/{mtk_thermal.c => mediatek/auxadc_thermal.c} (99%)
> >
> > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> > index e052dae614eb..d35f63daca3b 100644
> > --- a/drivers/thermal/Kconfig
> > +++ b/drivers/thermal/Kconfig
> > @@ -412,16 +412,10 @@ config DA9062_THERMAL
> >         zone.
> >         Compatible with the DA9062 and DA9061 PMICs.
> >
> > -config MTK_THERMAL
> > -     tristate "Temperature sensor driver for mediatek SoCs"
> > -     depends on ARCH_MEDIATEK || COMPILE_TEST
> > -     depends on HAS_IOMEM
> > -     depends on NVMEM || NVMEM=n
> > -     depends on RESET_CONTROLLER
> > -     default y
> > -     help
> > -       Enable this option if you want to have support for thermal management
> > -       controller present in Mediatek SoCs
> > +menu "Mediatek thermal drivers"
> > +depends on ARCH_MEDIATEK || COMPILE_TEST
> > +source "drivers/thermal/mediatek/Kconfig"
> > +endmenu
> >
> >   config AMLOGIC_THERMAL
> >       tristate "Amlogic Thermal Support"
> > diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> > index def8e1a0399c..f7daeb8056c4 100644
> > --- a/drivers/thermal/Makefile
> > +++ b/drivers/thermal/Makefile
> > @@ -55,7 +55,7 @@ obj-y                               += st/
> >   obj-$(CONFIG_QCOM_TSENS)    += qcom/
> >   obj-y                               += tegra/
> >   obj-$(CONFIG_HISI_THERMAL)     += hisi_thermal.o
> > -obj-$(CONFIG_MTK_THERMAL)    += mtk_thermal.o
> > +obj-y                                += mediatek/
> >   obj-$(CONFIG_GENERIC_ADC_THERMAL)   += thermal-generic-adc.o
> >   obj-$(CONFIG_UNIPHIER_THERMAL)      += uniphier_thermal.o
> >   obj-$(CONFIG_AMLOGIC_THERMAL)     += amlogic_thermal.o
> > diff --git a/drivers/thermal/mediatek/Kconfig b/drivers/thermal/mediatek/Kconfig
> > new file mode 100644
> > index 000000000000..8ff32370b591
> > --- /dev/null
> > +++ b/drivers/thermal/mediatek/Kconfig
> > @@ -0,0 +1,22 @@
> > +config MTK_THERMAL
> > +     tristate "MediaTek thermal drivers"
> > +     depends on THERMAL_OF
> > +     help
> > +       This is the option for MediaTek thermal software solutions.
> > +       Please enable corresponding options to get temperature
> > +       information from thermal sensors or turn on throttle
> > +       mechaisms for thermal mitigation.
> > +
> > +if MTK_THERMAL
> > +
> > +config MTK_SOC_THERMAL
> > +     tristate "AUXADC temperature sensor driver for MediaTek SoCs"
> > +     depends on HAS_IOMEM
> > +     depends on NVMEM
>
> That's not true. This driver doesn't depend on NVMEM, as in function
> mtk_thermal_get_calibration_data(), the calibration params are initialized
> with default values and if no "calibration-data" cell was found, it will
> return 0 and simply go on with probing... and this is because not all SoCs
> need calibration data from eFuses.
> If you check carefully, the original entry says "NVMEM || NVMEM=n".
>
> Please drop the NVMEM dependency from this config entirely.
>
> > +     depends on RESET_CONTROLLER
>
> While at it, can you please also drop the dependency for RESET_CONTROLLER?
> I know it was present on the original config entry, but that's wrong, as
> the driver doesn't strictly require resets, as it uses device_reset_optional().
>
>
> Regards,
> Angelo
>