From: Eduardo Valentin <eduardo.valentin@ti.com>
To: Amit Daniel Kachhap <amit.daniel@samsung.com>
Cc: linux-pm@vger.kernel.org, Zhang Rui <rui.zhang@intel.com>,
linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org,
amit.kachhap@gmail.com, Kukjin Kim <kgene.kim@samsung.com>,
Eduardo Valentin <eduardo.valentin@ti.com>
Subject: Re: [PATCH V3 05/21] thermal: exynos: Bifurcate exynos tmu driver and configuration data
Date: Thu, 9 May 2013 09:39:18 -0400 [thread overview]
Message-ID: <518BA706.2050202@ti.com> (raw)
In-Reply-To: <1367931671-3906-6-git-send-email-amit.daniel@samsung.com>
[-- Attachment #1: Type: text/plain, Size: 12126 bytes --]
Hey Amit,
On 07-05-2013 09:00, Amit Daniel Kachhap wrote:
> This code splits the exynos tmu driver code into SOC specific data parts.
> This will simplify adding new SOC specific data to the same TMU controller.
I like the idea of spliting the code from data. The driver goes into a
self contained direction. Thanks for doing this.Couple of minor comments
follows.
>
> Acked-by: Kukjin Kim <kgene.kim@samsung.com>
> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
> ---
> drivers/thermal/samsung/Kconfig | 13 ++++-
> drivers/thermal/samsung/Makefile | 1 +
> drivers/thermal/samsung/exynos_thermal_common.h | 3 +-
> drivers/thermal/samsung/exynos_tmu.c | 67 +------------------
> drivers/thermal/samsung/exynos_tmu_data.c | 78 +++++++++++++++++++++++
> drivers/thermal/samsung/exynos_tmu_data.h | 41 ++++++++++++
> 6 files changed, 137 insertions(+), 66 deletions(-)
> create mode 100644 drivers/thermal/samsung/exynos_tmu_data.c
> create mode 100644 drivers/thermal/samsung/exynos_tmu_data.h
>
> diff --git a/drivers/thermal/samsung/Kconfig b/drivers/thermal/samsung/Kconfig
> index 7857e20..f23f533 100644
> --- a/drivers/thermal/samsung/Kconfig
> +++ b/drivers/thermal/samsung/Kconfig
> @@ -5,7 +5,8 @@ config EXYNOS_THERMAL
> If you say yes here you get support for the exynos thermal driver
> for exynos4 and exynos5 soc. This driver initialises the TMU, reports
> temperature and handles cooling action if defined. This driver uses
> - core thermal API's.
> + core thermal API's and TMU configuration data from the supported
> + soc's.
>
> config EXYNOS_THERMAL_CORE
> bool "Core thermal framework support for EXYNOS SOC's"
> @@ -15,3 +16,13 @@ config EXYNOS_THERMAL_CORE
> (Thermal Management Unit) common registration/unregistration
> functions to the core thermal layer and also to use the generic
> cpu cooling API's.
> +
> +config EXYNOS_THERMAL_DATA
> + bool "Temperature sensor congiguration data for EXYNOS series SOC"
> + depends on (CPU_EXYNOS4210 || SOC_EXYNOS4212 || SOC_EXYNOS4412 || SOC_EXYNOS5250)
> + depends on EXYNOS_THERMAL
> + help
> + If you say yes here you can enable TMU (Thermal Management Unit) on
> + SAMSUNG EXYNOS 4210, 4412, 4414 and 5250 series of SoC. This option
> + enables/prepares the configuration, trip and cooling data for the TMU
> + driver.
What is your strategy here? Looks like you want to provide all required
data (for all supported SoCs) based on 1 config entry
(EXYNOS_THERMAL_DATA). What do you think of having one per SoC supported?
> diff --git a/drivers/thermal/samsung/Makefile b/drivers/thermal/samsung/Makefile
> index 22528d6..c0446e8 100644
> --- a/drivers/thermal/samsung/Makefile
> +++ b/drivers/thermal/samsung/Makefile
> @@ -4,3 +4,4 @@
> obj-$(CONFIG_EXYNOS_THERMAL) += exynos_thermal.o
> exynos_thermal-y := exynos_tmu.o
> exynos_thermal-$(CONFIG_EXYNOS_THERMAL_CORE) += exynos_thermal_common.o
> +exynos_thermal-$(CONFIG_EXYNOS_THERMAL_DATA) += exynos_tmu_data.o
> diff --git a/drivers/thermal/samsung/exynos_thermal_common.h b/drivers/thermal/samsung/exynos_thermal_common.h
> index 1c3f8b7..3b85858 100644
> --- a/drivers/thermal/samsung/exynos_thermal_common.h
> +++ b/drivers/thermal/samsung/exynos_thermal_common.h
> @@ -62,7 +62,7 @@ struct freq_clip_table {
> struct thermal_trip_point_conf {
> int trip_val[MAX_TRIP_COUNT];
> int trip_count;
> - u8 trigger_falling;
> + unsigned char trigger_falling;
> };
Why do you need this change to bifurcate data / code? (Sounds like you
probably want to send this in a separate patch, as it seams to be a
separated issue).
>
> struct thermal_cooling_conf {
> @@ -93,6 +93,5 @@ exynos_register_thermal(struct thermal_sensor_conf *sensor_conf) { return 0; }
>
> static inline void
> exynos_report_trigger(void) { return; }
> -
I dont think this is realted to this patch.
> #endif /* CONFIG_EXYNOS_COMMON */
> #endif /* _LINUX_EXYNOS_THERMAL_COMMON_H */
> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> index a02f2c9..05b5068 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -34,6 +34,7 @@
> #include <linux/workqueue.h>
> #include "exynos_thermal_common.h"
> #include "exynos_tmu.h"
> +#include "exynos_tmu_data.h"
>
> /* Exynos generic registers */
> #define EXYNOS_TMU_REG_TRIMINFO 0x0
> @@ -385,66 +386,6 @@ static struct thermal_sensor_conf exynos_sensor_conf = {
> .write_emul_temp = exynos_tmu_set_emulation,
> };
>
> -#if defined(CONFIG_CPU_EXYNOS4210)
> -static struct exynos_tmu_platform_data const exynos4210_default_tmu_data = {
> - .threshold = 80,
> - .trigger_levels[0] = 5,
> - .trigger_levels[1] = 20,
> - .trigger_levels[2] = 30,
> - .trigger_level0_en = 1,
> - .trigger_level1_en = 1,
> - .trigger_level2_en = 1,
> - .trigger_level3_en = 0,
> - .gain = 15,
> - .reference_voltage = 7,
> - .cal_type = TYPE_ONE_POINT_TRIMMING,
> - .freq_tab[0] = {
> - .freq_clip_max = 800 * 1000,
> - .temp_level = 85,
> - },
> - .freq_tab[1] = {
> - .freq_clip_max = 200 * 1000,
> - .temp_level = 100,
> - },
> - .freq_tab_count = 2,
> - .type = SOC_ARCH_EXYNOS4210,
> -};
> -#define EXYNOS4210_TMU_DRV_DATA (&exynos4210_default_tmu_data)
> -#else
> -#define EXYNOS4210_TMU_DRV_DATA (NULL)
> -#endif
> -
> -#if defined(CONFIG_SOC_EXYNOS5250) || defined(CONFIG_SOC_EXYNOS4412)
> -static struct exynos_tmu_platform_data const exynos_default_tmu_data = {
> - .threshold_falling = 10,
> - .trigger_levels[0] = 85,
> - .trigger_levels[1] = 103,
> - .trigger_levels[2] = 110,
> - .trigger_level0_en = 1,
> - .trigger_level1_en = 1,
> - .trigger_level2_en = 1,
> - .trigger_level3_en = 0,
> - .gain = 8,
> - .reference_voltage = 16,
> - .noise_cancel_mode = 4,
> - .cal_type = TYPE_ONE_POINT_TRIMMING,
> - .efuse_value = 55,
> - .freq_tab[0] = {
> - .freq_clip_max = 800 * 1000,
> - .temp_level = 85,
> - },
> - .freq_tab[1] = {
> - .freq_clip_max = 200 * 1000,
> - .temp_level = 103,
> - },
> - .freq_tab_count = 2,
> - .type = SOC_ARCH_EXYNOS,
> -};
> -#define EXYNOS_TMU_DRV_DATA (&exynos_default_tmu_data)
> -#else
> -#define EXYNOS_TMU_DRV_DATA (NULL)
> -#endif
> -
> #ifdef CONFIG_OF
> static const struct of_device_id exynos_tmu_match[] = {
> {
> @@ -453,11 +394,11 @@ static const struct of_device_id exynos_tmu_match[] = {
> },
> {
> .compatible = "samsung,exynos4412-tmu",
> - .data = (void *)EXYNOS_TMU_DRV_DATA,
> + .data = (void *)EXYNOS5250_TMU_DRV_DATA,
> },
> {
> .compatible = "samsung,exynos5250-tmu",
> - .data = (void *)EXYNOS_TMU_DRV_DATA,
> + .data = (void *)EXYNOS5250_TMU_DRV_DATA,
> },
> {},
> };
> @@ -471,7 +412,7 @@ static struct platform_device_id exynos_tmu_driver_ids[] = {
> },
> {
> .name = "exynos5250-tmu",
> - .driver_data = (kernel_ulong_t)EXYNOS_TMU_DRV_DATA,
> + .driver_data = (kernel_ulong_t)EXYNOS5250_TMU_DRV_DATA,
> },
> { },
> };
> diff --git a/drivers/thermal/samsung/exynos_tmu_data.c b/drivers/thermal/samsung/exynos_tmu_data.c
> new file mode 100644
> index 0000000..13a60ca
> --- /dev/null
> +++ b/drivers/thermal/samsung/exynos_tmu_data.c
> @@ -0,0 +1,78 @@
> +/*
> + * exynos_tmu_data.c - Samsung EXYNOS tmu data file
> + *
> + * Copyright (C) 2013 Samsung Electronics
> + * Amit Daniel Kachhap <amit.daniel@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + */
> +
> +#include "exynos_thermal_common.h"
> +#include "exynos_tmu.h"
> +
> +#if defined(CONFIG_CPU_EXYNOS4210)
> +struct exynos_tmu_platform_data const exynos4210_default_tmu_data = {
> + .threshold = 80,
> + .trigger_levels[0] = 5,
> + .trigger_levels[1] = 20,
> + .trigger_levels[2] = 30,
> + .trigger_level0_en = 1,
> + .trigger_level1_en = 1,
> + .trigger_level2_en = 1,
> + .trigger_level3_en = 0,
> + .gain = 15,
> + .reference_voltage = 7,
> + .cal_type = TYPE_ONE_POINT_TRIMMING,
> + .freq_tab[0] = {
> + .freq_clip_max = 800 * 1000,
> + .temp_level = 85,
> + },
> + .freq_tab[1] = {
> + .freq_clip_max = 200 * 1000,
> + .temp_level = 100,
> + },
> + .freq_tab_count = 2,
> + .type = SOC_ARCH_EXYNOS4210,
> +};
> +#endif
> +
> +#if defined(CONFIG_SOC_EXYNOS5250) || defined(CONFIG_SOC_EXYNOS4412)
> +struct exynos_tmu_platform_data const exynos5250_default_tmu_data = {
> + .threshold_falling = 10,
> + .trigger_levels[0] = 85,
> + .trigger_levels[1] = 103,
> + .trigger_levels[2] = 110,
> + .trigger_level0_en = 1,
> + .trigger_level1_en = 1,
> + .trigger_level2_en = 1,
> + .trigger_level3_en = 0,
> + .gain = 8,
> + .reference_voltage = 16,
> + .noise_cancel_mode = 4,
> + .cal_type = TYPE_ONE_POINT_TRIMMING,
> + .efuse_value = 55,
> + .freq_tab[0] = {
> + .freq_clip_max = 800 * 1000,
> + .temp_level = 85,
> + },
> + .freq_tab[1] = {
> + .freq_clip_max = 200 * 1000,
> + .temp_level = 103,
> + },
> + .freq_tab_count = 2,
> + .type = SOC_ARCH_EXYNOS,
> +};
> +#endif
If you define config entries per SoC data, you could have a file per SoC
data and remove the #ifdefery by solving the config in the Makefile.
What do you think?
> diff --git a/drivers/thermal/samsung/exynos_tmu_data.h b/drivers/thermal/samsung/exynos_tmu_data.h
> new file mode 100644
> index 0000000..5b69711
> --- /dev/null
> +++ b/drivers/thermal/samsung/exynos_tmu_data.h
> @@ -0,0 +1,41 @@
> +/*
> + * exynos_tmu_data.h - Samsung EXYNOS tmu data header file
> + *
> + * Copyright (C) 2013 Samsung Electronics
> + * Amit Daniel Kachhap <amit.daniel@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + */
> +
> +#ifndef _LINUX_EXYNOS_TMU_DATA_H
> +#define _LINUX_EXYNOS_TMU_DATA_H
> +
how about _EXYNOS_TMU_DATA_H ?
> +#if defined(CONFIG_CPU_EXYNOS4210) && defined(CONFIG_EXYNOS_THERMAL_DATA)
> +extern struct exynos_tmu_platform_data const exynos4210_default_tmu_data;
> +#define EXYNOS4210_TMU_DRV_DATA (&exynos4210_default_tmu_data)
> +#else
> +#define EXYNOS4210_TMU_DRV_DATA (NULL)
> +#endif
> +
> +#if (defined(CONFIG_SOC_EXYNOS5250) || defined(CONFIG_SOC_EXYNOS4412)) && \
> +defined(CONFIG_EXYNOS_THERMAL_DATA)
> +extern struct exynos_tmu_platform_data const exynos5250_default_tmu_data;
> +#define EXYNOS5250_TMU_DRV_DATA (&exynos5250_default_tmu_data)
> +#else
> +#define EXYNOS5250_TMU_DRV_DATA (NULL)
> +#endif
> +
> +#endif /*_LINUX_EXYNOS_TMU_DATA_H*/
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 295 bytes --]
next prev parent reply other threads:[~2013-05-09 13:39 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-05-07 13:00 [PATCH V3 0/20] thermal: exynos: Add thermal driver for exynos5440 Amit Daniel Kachhap
2013-05-07 13:00 ` [PATCH V3 01/21] thermal: exynos: Moving exynos thermal files into samsung directory Amit Daniel Kachhap
2013-05-07 13:07 ` [RESEND PATCH " Amit Daniel Kachhap
2013-05-08 15:49 ` Eduardo Valentin
2013-05-09 2:15 ` amit daniel kachhap
2013-05-07 13:00 ` [PATCH V3 02/21] thermal: exynos: Bifurcate exynos thermal common and tmu controller code Amit Daniel Kachhap
2013-05-07 13:41 ` Zhang Rui
2013-05-08 3:49 ` amit daniel kachhap
2013-05-08 16:01 ` Eduardo Valentin
2013-05-09 5:15 ` amit daniel kachhap
2013-05-07 13:00 ` [PATCH V3 03/21] thermal: exynos: Rename exynos_thermal.c to exynos_tmu.c Amit Daniel Kachhap
2013-05-10 15:12 ` Eduardo Valentin
2013-05-07 13:00 ` [PATCH V3 04/21] thermal: exynos: Move exynos_thermal.h from include/* to driver/* folder Amit Daniel Kachhap
2013-05-09 13:28 ` Eduardo Valentin
2013-05-10 1:53 ` amit daniel kachhap
2013-05-07 13:00 ` [PATCH V3 05/21] thermal: exynos: Bifurcate exynos tmu driver and configuration data Amit Daniel Kachhap
2013-05-09 13:39 ` Eduardo Valentin [this message]
2013-05-10 2:05 ` amit daniel kachhap
2013-05-07 13:00 ` [PATCH V3 06/21] thermal: exynos: Add missing definations and code cleanup Amit Daniel Kachhap
2013-05-09 13:52 ` Eduardo Valentin
2013-05-10 2:12 ` amit daniel kachhap
2013-05-07 13:00 ` [PATCH V3 07/21] thermal: exynos: Add extra entries in the tmu platform data Amit Daniel Kachhap
2013-05-07 13:00 ` [PATCH V3 08/21] thermal: exynos: Support thermal tripping Amit Daniel Kachhap
2013-05-07 13:00 ` [PATCH V3 09/21] thermal: exynos: Move register definitions from driver file to data file Amit Daniel Kachhap
2013-05-09 14:32 ` Eduardo Valentin
2013-05-10 2:17 ` amit daniel kachhap
2013-05-10 14:14 ` Eduardo Valentin
2013-05-13 1:35 ` amit daniel kachhap
2013-05-07 13:01 ` [PATCH V3 10/21] thermal: exynos: Fix to clear only the generated interrupts Amit Daniel Kachhap
2013-05-09 14:33 ` Eduardo Valentin
2013-05-10 2:22 ` amit daniel kachhap
2013-05-10 15:40 ` Eduardo Valentin
2013-05-13 1:36 ` amit daniel kachhap
2013-05-07 13:01 ` [PATCH V3 11/21] thermal: exynos: Add support for instance based register/unregister Amit Daniel Kachhap
2013-05-07 13:01 ` [PATCH V3 12/21] thermal: exynos: Modify private_data to appropriate name driver_data Amit Daniel Kachhap
2013-05-07 13:01 ` [PATCH V3 13/21] thermal: exynos: Return success even if no cooling data supplied Amit Daniel Kachhap
2013-05-07 13:01 ` [PATCH V3 14/21] thermal: exynos: Make the zone handling dependent on trip count Amit Daniel Kachhap
2013-05-07 13:01 ` [PATCH V3 15/21] thermal: exynos: Add support to handle many instances of TMU Amit Daniel Kachhap
2013-05-09 14:53 ` Eduardo Valentin
2013-05-10 2:32 ` amit daniel kachhap
2013-05-07 13:01 ` [PATCH V3 16/21] thermal: exynos: Add features to check instead of using SOC type Amit Daniel Kachhap
2013-05-07 13:01 ` [PATCH V3 17/21] ARM: dts: thermal: exynos4: Add documentation for Exynos SoC thermal bindings Amit Daniel Kachhap
2013-05-07 13:01 ` [PATCH V3 18/21] thermal: exynos: Add support for exynos5440 TMU sensor Amit Daniel Kachhap
2013-05-09 14:47 ` Eduardo Valentin
2013-05-10 2:31 ` amit daniel kachhap
2013-05-10 14:14 ` Eduardo Valentin
2013-05-07 13:01 ` [PATCH V3 19/21] Documentation: thermal: Explain the exynos thermal driver model Amit Daniel Kachhap
2013-05-07 13:01 ` [PATCH V3 20/21] thermal: exynos: Support for TMU regulator defined at device tree Amit Daniel Kachhap
2013-05-09 14:44 ` Eduardo Valentin
2013-05-10 2:28 ` amit daniel kachhap
2013-05-10 16:05 ` Eduardo Valentin
2013-05-07 13:01 ` [PATCH V3 21/21] ARM: dts: Add device tree node for exynos5440 TMU controller Amit Daniel Kachhap
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=518BA706.2050202@ti.com \
--to=eduardo.valentin@ti.com \
--cc=amit.daniel@samsung.com \
--cc=amit.kachhap@gmail.com \
--cc=kgene.kim@samsung.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=linux-samsung-soc@vger.kernel.org \
--cc=rui.zhang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox