Hi,
This patch series contains a bug fix for the existing platforms and then
paves the way for adding support for Broadcom STB's latest chips in 16nm
processes, and finally updates the driver with pecularities introduced
with the 16nm, like the lack of interrupt notification from the HW.
Please queue up the first patch for -stable if you want, thanks!
Florian Fainelli (6):
thermal: brcmstb_thermal: Do not use DT coefficients
thermal: brcmstb_thermal: Prepare to support a different process
dt-bindings: thermal: Define BCM7216 thermal sensor compatible
thermal: brcmstb_thermal: Add 16nm process thermal parameters
thermal: brcmstb_thermal: Restructure interrupt registration
thermal: brcmstb_thermal: Register different ops per process
.../bindings/thermal/brcm,avs-tmon.txt | 8 +-
drivers/thermal/broadcom/brcmstb_thermal.c | 108 ++++++++++--------
2 files changed, 67 insertions(+), 49 deletions(-)
--
2.17.1
At the time the brcmstb_thermal driver and its binding were merged, the
DT binding did not make the coefficients properties a mandatory one,
therefore all users of the brcmstb_thermal driver out there have a non
functional implementation with zero coefficients. Even if these
properties were provided, the formula used for computation is incorrect.
The coefficients are entirely process specific (right now, only 28nm is
supported) and not board or SoC specific, it is therefore appropriate to
hard code them in the driver given the compatibility string we are
probed with which has to be updated whenever a new process is
introduced.
We remove the existing coefficients definition since subsequent patches
are going to add support for a new process and will introduce new
coefficients as well.
Fixes: 9e03cf1b2dd5 ("thermal: add brcmstb AVS TMON driver")
Signed-off-by: Florian Fainelli <[email protected]>
---
drivers/thermal/broadcom/brcmstb_thermal.c | 37 ++++------------------
1 file changed, 6 insertions(+), 31 deletions(-)
diff --git a/drivers/thermal/broadcom/brcmstb_thermal.c b/drivers/thermal/broadcom/brcmstb_thermal.c
index 5825ac581f56..42482af0422e 100644
--- a/drivers/thermal/broadcom/brcmstb_thermal.c
+++ b/drivers/thermal/broadcom/brcmstb_thermal.c
@@ -48,15 +48,6 @@
#define AVS_TMON_TEMP_INT_CODE 0x1c
#define AVS_TMON_TP_TEST_ENABLE 0x20
-/* Default coefficients */
-#define AVS_TMON_TEMP_SLOPE -487
-#define AVS_TMON_TEMP_OFFSET 410040
-
-/* HW related temperature constants */
-#define AVS_TMON_TEMP_MAX 0x3ff
-#define AVS_TMON_TEMP_MIN -88161
-#define AVS_TMON_TEMP_MASK AVS_TMON_TEMP_MAX
-
enum avs_tmon_trip_type {
TMON_TRIP_TYPE_LOW = 0,
TMON_TRIP_TYPE_HIGH,
@@ -108,23 +99,11 @@ struct brcmstb_thermal_priv {
struct thermal_zone_device *thermal;
};
-static void avs_tmon_get_coeffs(struct thermal_zone_device *tz, int *slope,
- int *offset)
-{
- *slope = thermal_zone_get_slope(tz);
- *offset = thermal_zone_get_offset(tz);
-}
-
/* Convert a HW code to a temperature reading (millidegree celsius) */
static inline int avs_tmon_code_to_temp(struct thermal_zone_device *tz,
u32 code)
{
- const int val = code & AVS_TMON_TEMP_MASK;
- int slope, offset;
-
- avs_tmon_get_coeffs(tz, &slope, &offset);
-
- return slope * val + offset;
+ return (410040 - (int)((code & 0x3FF) * 487));
}
/*
@@ -136,20 +115,16 @@ static inline int avs_tmon_code_to_temp(struct thermal_zone_device *tz,
static inline u32 avs_tmon_temp_to_code(struct thermal_zone_device *tz,
int temp, bool low)
{
- int slope, offset;
-
- if (temp < AVS_TMON_TEMP_MIN)
- return AVS_TMON_TEMP_MAX; /* Maximum code value */
-
- avs_tmon_get_coeffs(tz, &slope, &offset);
+ if (temp < -88161)
+ return 0x3FF; /* Maximum code value */
- if (temp >= offset)
+ if (temp >= 410040)
return 0; /* Minimum code value */
if (low)
- return (u32)(DIV_ROUND_UP(offset - temp, abs(slope)));
+ return (u32)(DIV_ROUND_UP(410040 - temp, 487));
else
- return (u32)((offset - temp) / abs(slope));
+ return (u32)((410040 - temp) / 487);
}
static int brcmstb_get_temp(void *data, int *temp)
--
2.17.1
Since we do not have interrupts on BCM7216, we cannot have trip point
crossing, the thermal subsystem expects us to provide a NULL set_trips
operation in that case, so make it possible to provide per-process
thermal_zone_of_device_ops
Signed-off-by: Florian Fainelli <[email protected]>
---
drivers/thermal/broadcom/brcmstb_thermal.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/thermal/broadcom/brcmstb_thermal.c b/drivers/thermal/broadcom/brcmstb_thermal.c
index 64f715053ce9..a75a335d1bb3 100644
--- a/drivers/thermal/broadcom/brcmstb_thermal.c
+++ b/drivers/thermal/broadcom/brcmstb_thermal.c
@@ -96,6 +96,7 @@ static struct avs_tmon_trip avs_tmon_trips[] = {
struct brcmstb_thermal_params {
unsigned int offset;
unsigned int mult;
+ const struct thermal_zone_of_device_ops *of_ops;
};
struct brcmstb_thermal_priv {
@@ -278,19 +279,25 @@ static int brcmstb_set_trips(void *data, int low, int high)
return 0;
}
-static const struct thermal_zone_of_device_ops of_ops = {
+static const struct thermal_zone_of_device_ops brcmstb_16nm_of_ops = {
.get_temp = brcmstb_get_temp,
- .set_trips = brcmstb_set_trips,
};
static const struct brcmstb_thermal_params brcmstb_16nm_params = {
.offset = 457829,
.mult = 557,
+ .of_ops = &brcmstb_16nm_of_ops,
+};
+
+static const struct thermal_zone_of_device_ops brcmstb_28nm_of_ops = {
+ .get_temp = brcmstb_get_temp,
+ .set_trips = brcmstb_set_trips,
};
static const struct brcmstb_thermal_params brcmstb_28nm_params = {
.offset = 410040,
.mult = 487,
+ .of_ops = &brcmstb_28nm_of_ops,
};
static const struct of_device_id brcmstb_thermal_id_table[] = {
@@ -329,7 +336,7 @@ static int brcmstb_thermal_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, priv);
thermal = devm_thermal_zone_of_sensor_register(&pdev->dev, 0, priv,
- &of_ops);
+ priv->temp_params.of_ops);
if (IS_ERR(thermal)) {
ret = PTR_ERR(thermal);
dev_err(&pdev->dev, "could not register sensor: %d\n", ret);
--
2.17.1
Match the 7216 compatible string in order to derive the correct 16nm
process thermal parameters to obtain correct readings.
Signed-off-by: Florian Fainelli <[email protected]>
---
drivers/thermal/broadcom/brcmstb_thermal.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/thermal/broadcom/brcmstb_thermal.c b/drivers/thermal/broadcom/brcmstb_thermal.c
index 2ad4eeb79f9c..41d4a142707c 100644
--- a/drivers/thermal/broadcom/brcmstb_thermal.c
+++ b/drivers/thermal/broadcom/brcmstb_thermal.c
@@ -283,12 +283,18 @@ static const struct thermal_zone_of_device_ops of_ops = {
.set_trips = brcmstb_set_trips,
};
+static const struct brcmstb_thermal_params brcmstb_16nm_params = {
+ .offset = 457829,
+ .mult = 557,
+};
+
static const struct brcmstb_thermal_params brcmstb_28nm_params = {
.offset = 410040,
.mult = 487,
};
static const struct of_device_id brcmstb_thermal_id_table[] = {
+ { .compatible = "brcm,avs-tmon-bcm7216", .data = &brcmstb_16nm_params },
{ .compatible = "brcm,avs-tmon", .data = &brcmstb_28nm_params },
{},
};
--
2.17.1
On 10/30/2019 11:21 AM, Florian Fainelli wrote:
> Hi,
>
> This patch series contains a bug fix for the existing platforms and then
> paves the way for adding support for Broadcom STB's latest chips in 16nm
> processes, and finally updates the driver with pecularities introduced
> with the 16nm, like the lack of interrupt notification from the HW.
>
> Please queue up the first patch for -stable if you want, thanks!
Ping?
>
> Florian Fainelli (6):
> thermal: brcmstb_thermal: Do not use DT coefficients
> thermal: brcmstb_thermal: Prepare to support a different process
> dt-bindings: thermal: Define BCM7216 thermal sensor compatible
> thermal: brcmstb_thermal: Add 16nm process thermal parameters
> thermal: brcmstb_thermal: Restructure interrupt registration
> thermal: brcmstb_thermal: Register different ops per process
>
> .../bindings/thermal/brcm,avs-tmon.txt | 8 +-
> drivers/thermal/broadcom/brcmstb_thermal.c | 108 ++++++++++--------
> 2 files changed, 67 insertions(+), 49 deletions(-)
>
--
Florian
On 11/20/2019 10:43 AM, Florian Fainelli wrote:
>
>
> On 10/30/2019 11:21 AM, Florian Fainelli wrote:
>> Hi,
>>
>> This patch series contains a bug fix for the existing platforms and then
>> paves the way for adding support for Broadcom STB's latest chips in 16nm
>> processes, and finally updates the driver with pecularities introduced
>> with the 16nm, like the lack of interrupt notification from the HW.
>>
>> Please queue up the first patch for -stable if you want, thanks!
>
> Ping?
Rui, anyone?
>
>>
>> Florian Fainelli (6):
>> thermal: brcmstb_thermal: Do not use DT coefficients
>> thermal: brcmstb_thermal: Prepare to support a different process
>> dt-bindings: thermal: Define BCM7216 thermal sensor compatible
>> thermal: brcmstb_thermal: Add 16nm process thermal parameters
>> thermal: brcmstb_thermal: Restructure interrupt registration
>> thermal: brcmstb_thermal: Register different ops per process
>>
>> .../bindings/thermal/brcm,avs-tmon.txt | 8 +-
>> drivers/thermal/broadcom/brcmstb_thermal.c | 108 ++++++++++--------
>> 2 files changed, 67 insertions(+), 49 deletions(-)
>>
>
--
Florian
On Wed, Oct 30, 2019 at 11:51 PM Florian Fainelli <[email protected]> wrote:
>
> At the time the brcmstb_thermal driver and its binding were merged, the
> DT binding did not make the coefficients properties a mandatory one,
> therefore all users of the brcmstb_thermal driver out there have a non
> functional implementation with zero coefficients. Even if these
> properties were provided, the formula used for computation is incorrect.
>
> The coefficients are entirely process specific (right now, only 28nm is
> supported) and not board or SoC specific, it is therefore appropriate to
> hard code them in the driver given the compatibility string we are
> probed with which has to be updated whenever a new process is
> introduced.
>
> We remove the existing coefficients definition since subsequent patches
> are going to add support for a new process and will introduce new
> coefficients as well.
>
> Fixes: 9e03cf1b2dd5 ("thermal: add brcmstb AVS TMON driver")
I think you should fix the computation formula as the first patch and
then merge the rest of this patch into your second patch.
I don't think the intermediate state of converting named constants to
magic numbers is needed just to convert it over to another set of
parameters.
Regards,
Amit
> Signed-off-by: Florian Fainelli <[email protected]>
> ---
> drivers/thermal/broadcom/brcmstb_thermal.c | 37 ++++------------------
> 1 file changed, 6 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/thermal/broadcom/brcmstb_thermal.c b/drivers/thermal/broadcom/brcmstb_thermal.c
> index 5825ac581f56..42482af0422e 100644
> --- a/drivers/thermal/broadcom/brcmstb_thermal.c
> +++ b/drivers/thermal/broadcom/brcmstb_thermal.c
> @@ -48,15 +48,6 @@
> #define AVS_TMON_TEMP_INT_CODE 0x1c
> #define AVS_TMON_TP_TEST_ENABLE 0x20
>
> -/* Default coefficients */
> -#define AVS_TMON_TEMP_SLOPE -487
> -#define AVS_TMON_TEMP_OFFSET 410040
> -
> -/* HW related temperature constants */
> -#define AVS_TMON_TEMP_MAX 0x3ff
> -#define AVS_TMON_TEMP_MIN -88161
> -#define AVS_TMON_TEMP_MASK AVS_TMON_TEMP_MAX
> -
> enum avs_tmon_trip_type {
> TMON_TRIP_TYPE_LOW = 0,
> TMON_TRIP_TYPE_HIGH,
> @@ -108,23 +99,11 @@ struct brcmstb_thermal_priv {
> struct thermal_zone_device *thermal;
> };
>
> -static void avs_tmon_get_coeffs(struct thermal_zone_device *tz, int *slope,
> - int *offset)
> -{
> - *slope = thermal_zone_get_slope(tz);
> - *offset = thermal_zone_get_offset(tz);
> -}
> -
> /* Convert a HW code to a temperature reading (millidegree celsius) */
> static inline int avs_tmon_code_to_temp(struct thermal_zone_device *tz,
> u32 code)
> {
> - const int val = code & AVS_TMON_TEMP_MASK;
> - int slope, offset;
> -
> - avs_tmon_get_coeffs(tz, &slope, &offset);
> -
> - return slope * val + offset;
> + return (410040 - (int)((code & 0x3FF) * 487));
> }
>
> /*
> @@ -136,20 +115,16 @@ static inline int avs_tmon_code_to_temp(struct thermal_zone_device *tz,
> static inline u32 avs_tmon_temp_to_code(struct thermal_zone_device *tz,
> int temp, bool low)
> {
> - int slope, offset;
> -
> - if (temp < AVS_TMON_TEMP_MIN)
> - return AVS_TMON_TEMP_MAX; /* Maximum code value */
> -
> - avs_tmon_get_coeffs(tz, &slope, &offset);
> + if (temp < -88161)
> + return 0x3FF; /* Maximum code value */
>
> - if (temp >= offset)
> + if (temp >= 410040)
> return 0; /* Minimum code value */
>
> if (low)
> - return (u32)(DIV_ROUND_UP(offset - temp, abs(slope)));
> + return (u32)(DIV_ROUND_UP(410040 - temp, 487));
> else
> - return (u32)((offset - temp) / abs(slope));
> + return (u32)((410040 - temp) / 487);
> }
>
> static int brcmstb_get_temp(void *data, int *temp)
> --
> 2.17.1
>
On Wed, Oct 30, 2019 at 11:52 PM Florian Fainelli <[email protected]> wrote:
>
> Match the 7216 compatible string in order to derive the correct 16nm
> process thermal parameters to obtain correct readings.
>
> Signed-off-by: Florian Fainelli <[email protected]>
Reviewed-by: Amit Kucheria <[email protected]>
> ---
> drivers/thermal/broadcom/brcmstb_thermal.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/thermal/broadcom/brcmstb_thermal.c b/drivers/thermal/broadcom/brcmstb_thermal.c
> index 2ad4eeb79f9c..41d4a142707c 100644
> --- a/drivers/thermal/broadcom/brcmstb_thermal.c
> +++ b/drivers/thermal/broadcom/brcmstb_thermal.c
> @@ -283,12 +283,18 @@ static const struct thermal_zone_of_device_ops of_ops = {
> .set_trips = brcmstb_set_trips,
> };
>
> +static const struct brcmstb_thermal_params brcmstb_16nm_params = {
> + .offset = 457829,
> + .mult = 557,
> +};
> +
> static const struct brcmstb_thermal_params brcmstb_28nm_params = {
> .offset = 410040,
> .mult = 487,
> };
>
> static const struct of_device_id brcmstb_thermal_id_table[] = {
> + { .compatible = "brcm,avs-tmon-bcm7216", .data = &brcmstb_16nm_params },
> { .compatible = "brcm,avs-tmon", .data = &brcmstb_28nm_params },
> {},
> };
> --
> 2.17.1
>
On Wed, Oct 30, 2019 at 11:52 PM Florian Fainelli <[email protected]> wrote:
>
> Since we do not have interrupts on BCM7216, we cannot have trip point
> crossing, the thermal subsystem expects us to provide a NULL set_trips
> operation in that case, so make it possible to provide per-process
> thermal_zone_of_device_ops
>
> Signed-off-by: Florian Fainelli <[email protected]>
Reviewed-by: Amit Kucheria <[email protected]>
> ---
> drivers/thermal/broadcom/brcmstb_thermal.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/thermal/broadcom/brcmstb_thermal.c b/drivers/thermal/broadcom/brcmstb_thermal.c
> index 64f715053ce9..a75a335d1bb3 100644
> --- a/drivers/thermal/broadcom/brcmstb_thermal.c
> +++ b/drivers/thermal/broadcom/brcmstb_thermal.c
> @@ -96,6 +96,7 @@ static struct avs_tmon_trip avs_tmon_trips[] = {
> struct brcmstb_thermal_params {
> unsigned int offset;
> unsigned int mult;
> + const struct thermal_zone_of_device_ops *of_ops;
> };
>
> struct brcmstb_thermal_priv {
> @@ -278,19 +279,25 @@ static int brcmstb_set_trips(void *data, int low, int high)
> return 0;
> }
>
> -static const struct thermal_zone_of_device_ops of_ops = {
> +static const struct thermal_zone_of_device_ops brcmstb_16nm_of_ops = {
> .get_temp = brcmstb_get_temp,
> - .set_trips = brcmstb_set_trips,
> };
>
> static const struct brcmstb_thermal_params brcmstb_16nm_params = {
> .offset = 457829,
> .mult = 557,
> + .of_ops = &brcmstb_16nm_of_ops,
> +};
> +
> +static const struct thermal_zone_of_device_ops brcmstb_28nm_of_ops = {
> + .get_temp = brcmstb_get_temp,
> + .set_trips = brcmstb_set_trips,
> };
>
> static const struct brcmstb_thermal_params brcmstb_28nm_params = {
> .offset = 410040,
> .mult = 487,
> + .of_ops = &brcmstb_28nm_of_ops,
> };
>
> static const struct of_device_id brcmstb_thermal_id_table[] = {
> @@ -329,7 +336,7 @@ static int brcmstb_thermal_probe(struct platform_device *pdev)
> platform_set_drvdata(pdev, priv);
>
> thermal = devm_thermal_zone_of_sensor_register(&pdev->dev, 0, priv,
> - &of_ops);
> + priv->temp_params.of_ops);
> if (IS_ERR(thermal)) {
> ret = PTR_ERR(thermal);
> dev_err(&pdev->dev, "could not register sensor: %d\n", ret);
> --
> 2.17.1
>
On 12/4/19 12:41 AM, Amit Kucheria wrote:
> On Wed, Oct 30, 2019 at 11:51 PM Florian Fainelli <[email protected]> wrote:
>>
>> At the time the brcmstb_thermal driver and its binding were merged, the
>> DT binding did not make the coefficients properties a mandatory one,
>> therefore all users of the brcmstb_thermal driver out there have a non
>> functional implementation with zero coefficients. Even if these
>> properties were provided, the formula used for computation is incorrect.
>>
>> The coefficients are entirely process specific (right now, only 28nm is
>> supported) and not board or SoC specific, it is therefore appropriate to
>> hard code them in the driver given the compatibility string we are
>> probed with which has to be updated whenever a new process is
>> introduced.
>>
>> We remove the existing coefficients definition since subsequent patches
>> are going to add support for a new process and will introduce new
>> coefficients as well.
>>
>> Fixes: 9e03cf1b2dd5 ("thermal: add brcmstb AVS TMON driver")
>
> I think you should fix the computation formula as the first patch and
> then merge the rest of this patch into your second patch.
>
> I don't think the intermediate state of converting named constants to
> magic numbers is needed just to convert it over to another set of
> parameters.
ok, so you would rather see this patch using the defined constants while
fixing the formula, and in a subsequent patch getting rid of the
constant names since they are going to be added to a per-process table,
does that sound reasonable?
>
> Regards,
> Amit
>
>> Signed-off-by: Florian Fainelli <[email protected]>
>> ---
>> drivers/thermal/broadcom/brcmstb_thermal.c | 37 ++++------------------
>> 1 file changed, 6 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/thermal/broadcom/brcmstb_thermal.c b/drivers/thermal/broadcom/brcmstb_thermal.c
>> index 5825ac581f56..42482af0422e 100644
>> --- a/drivers/thermal/broadcom/brcmstb_thermal.c
>> +++ b/drivers/thermal/broadcom/brcmstb_thermal.c
>> @@ -48,15 +48,6 @@
>> #define AVS_TMON_TEMP_INT_CODE 0x1c
>> #define AVS_TMON_TP_TEST_ENABLE 0x20
>>
>> -/* Default coefficients */
>> -#define AVS_TMON_TEMP_SLOPE -487
>> -#define AVS_TMON_TEMP_OFFSET 410040
>> -
>> -/* HW related temperature constants */
>> -#define AVS_TMON_TEMP_MAX 0x3ff
>> -#define AVS_TMON_TEMP_MIN -88161
>> -#define AVS_TMON_TEMP_MASK AVS_TMON_TEMP_MAX
>> -
>> enum avs_tmon_trip_type {
>> TMON_TRIP_TYPE_LOW = 0,
>> TMON_TRIP_TYPE_HIGH,
>> @@ -108,23 +99,11 @@ struct brcmstb_thermal_priv {
>> struct thermal_zone_device *thermal;
>> };
>>
>> -static void avs_tmon_get_coeffs(struct thermal_zone_device *tz, int *slope,
>> - int *offset)
>> -{
>> - *slope = thermal_zone_get_slope(tz);
>> - *offset = thermal_zone_get_offset(tz);
>> -}
>> -
>> /* Convert a HW code to a temperature reading (millidegree celsius) */
>> static inline int avs_tmon_code_to_temp(struct thermal_zone_device *tz,
>> u32 code)
>> {
>> - const int val = code & AVS_TMON_TEMP_MASK;
>> - int slope, offset;
>> -
>> - avs_tmon_get_coeffs(tz, &slope, &offset);
>> -
>> - return slope * val + offset;
>> + return (410040 - (int)((code & 0x3FF) * 487));
>> }
>>
>> /*
>> @@ -136,20 +115,16 @@ static inline int avs_tmon_code_to_temp(struct thermal_zone_device *tz,
>> static inline u32 avs_tmon_temp_to_code(struct thermal_zone_device *tz,
>> int temp, bool low)
>> {
>> - int slope, offset;
>> -
>> - if (temp < AVS_TMON_TEMP_MIN)
>> - return AVS_TMON_TEMP_MAX; /* Maximum code value */
>> -
>> - avs_tmon_get_coeffs(tz, &slope, &offset);
>> + if (temp < -88161)
>> + return 0x3FF; /* Maximum code value */
>>
>> - if (temp >= offset)
>> + if (temp >= 410040)
>> return 0; /* Minimum code value */
>>
>> if (low)
>> - return (u32)(DIV_ROUND_UP(offset - temp, abs(slope)));
>> + return (u32)(DIV_ROUND_UP(410040 - temp, 487));
>> else
>> - return (u32)((offset - temp) / abs(slope));
>> + return (u32)((410040 - temp) / 487);
>> }
>>
>> static int brcmstb_get_temp(void *data, int *temp)
>> --
>> 2.17.1
>>
--
Florian
On Wed, Dec 4, 2019 at 11:12 PM Florian Fainelli <[email protected]> wrote:
>
> On 12/4/19 12:41 AM, Amit Kucheria wrote:
> > On Wed, Oct 30, 2019 at 11:51 PM Florian Fainelli <[email protected]> wrote:
> >>
> >> At the time the brcmstb_thermal driver and its binding were merged, the
> >> DT binding did not make the coefficients properties a mandatory one,
> >> therefore all users of the brcmstb_thermal driver out there have a non
> >> functional implementation with zero coefficients. Even if these
> >> properties were provided, the formula used for computation is incorrect.
> >>
> >> The coefficients are entirely process specific (right now, only 28nm is
> >> supported) and not board or SoC specific, it is therefore appropriate to
> >> hard code them in the driver given the compatibility string we are
> >> probed with which has to be updated whenever a new process is
> >> introduced.
> >>
> >> We remove the existing coefficients definition since subsequent patches
> >> are going to add support for a new process and will introduce new
> >> coefficients as well.
> >>
> >> Fixes: 9e03cf1b2dd5 ("thermal: add brcmstb AVS TMON driver")
> >
> > I think you should fix the computation formula as the first patch and
> > then merge the rest of this patch into your second patch.
> >
> > I don't think the intermediate state of converting named constants to
> > magic numbers is needed just to convert it over to another set of
> > parameters.
>
> ok, so you would rather see this patch using the defined constants while
> fixing the formula, and in a subsequent patch getting rid of the
> constant names since they are going to be added to a per-process table,
> does that sound reasonable?
Yes, that way the first patch would fix a bug (and with a Fixes tag
potentially gets backported to stable), the second would convert over
to the per-process table.
Regards,
Amit
> >
> > Regards,
> > Amit
> >
> >> Signed-off-by: Florian Fainelli <[email protected]>
> >> ---
> >> drivers/thermal/broadcom/brcmstb_thermal.c | 37 ++++------------------
> >> 1 file changed, 6 insertions(+), 31 deletions(-)
> >>
> >> diff --git a/drivers/thermal/broadcom/brcmstb_thermal.c b/drivers/thermal/broadcom/brcmstb_thermal.c
> >> index 5825ac581f56..42482af0422e 100644
> >> --- a/drivers/thermal/broadcom/brcmstb_thermal.c
> >> +++ b/drivers/thermal/broadcom/brcmstb_thermal.c
> >> @@ -48,15 +48,6 @@
> >> #define AVS_TMON_TEMP_INT_CODE 0x1c
> >> #define AVS_TMON_TP_TEST_ENABLE 0x20
> >>
> >> -/* Default coefficients */
> >> -#define AVS_TMON_TEMP_SLOPE -487
> >> -#define AVS_TMON_TEMP_OFFSET 410040
> >> -
> >> -/* HW related temperature constants */
> >> -#define AVS_TMON_TEMP_MAX 0x3ff
> >> -#define AVS_TMON_TEMP_MIN -88161
> >> -#define AVS_TMON_TEMP_MASK AVS_TMON_TEMP_MAX
> >> -
> >> enum avs_tmon_trip_type {
> >> TMON_TRIP_TYPE_LOW = 0,
> >> TMON_TRIP_TYPE_HIGH,
> >> @@ -108,23 +99,11 @@ struct brcmstb_thermal_priv {
> >> struct thermal_zone_device *thermal;
> >> };
> >>
> >> -static void avs_tmon_get_coeffs(struct thermal_zone_device *tz, int *slope,
> >> - int *offset)
> >> -{
> >> - *slope = thermal_zone_get_slope(tz);
> >> - *offset = thermal_zone_get_offset(tz);
> >> -}
> >> -
> >> /* Convert a HW code to a temperature reading (millidegree celsius) */
> >> static inline int avs_tmon_code_to_temp(struct thermal_zone_device *tz,
> >> u32 code)
> >> {
> >> - const int val = code & AVS_TMON_TEMP_MASK;
> >> - int slope, offset;
> >> -
> >> - avs_tmon_get_coeffs(tz, &slope, &offset);
> >> -
> >> - return slope * val + offset;
> >> + return (410040 - (int)((code & 0x3FF) * 487));
> >> }
> >>l
> >> /*
> >> @@ -136,20 +115,16 @@ static inline int avs_tmon_code_to_temp(struct thermal_zone_device *tz,
> >> static inline u32 avs_tmon_temp_to_code(struct thermal_zone_device *tz,
> >> int temp, bool low)
> >> {
> >> - int slope, offset;
> >> -
> >> - if (temp < AVS_TMON_TEMP_MIN)
> >> - return AVS_TMON_TEMP_MAX; /* Maximum code value */
> >> -
> >> - avs_tmon_get_coeffs(tz, &slope, &offset);
> >> + if (temp < -88161)
> >> + return 0x3FF; /* Maximum code value */
> >>
> >> - if (temp >= offset)
> >> + if (temp >= 410040)
> >> return 0; /* Minimum code value */
> >>
> >> if (low)
> >> - return (u32)(DIV_ROUND_UP(offset - temp, abs(slope)));
> >> + return (u32)(DIV_ROUND_UP(410040 - temp, 487));
> >> else
> >> - return (u32)((offset - temp) / abs(slope));
> >> + return (u32)((410040 - temp) / 487);
> >> }
> >>
> >> static int brcmstb_get_temp(void *data, int *temp)
> >> --
> >> 2.17.1
> >>
>
>
> --
> Florian
Hi Florian,
On 04/12/2019 05:50, Florian Fainelli wrote:
>
>
> On 11/20/2019 10:43 AM, Florian Fainelli wrote:
>>
>>
>> On 10/30/2019 11:21 AM, Florian Fainelli wrote:
>>> Hi,
>>>
>>> This patch series contains a bug fix for the existing platforms and then
>>> paves the way for adding support for Broadcom STB's latest chips in 16nm
>>> processes, and finally updates the driver with pecularities introduced
>>> with the 16nm, like the lack of interrupt notification from the HW.
>>>
>>> Please queue up the first patch for -stable if you want, thanks!
>>
>> Ping?
>
> Rui, anyone?
Given comments on patch 1, I'm waiting for the V2.
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On 12/9/19 1:04 AM, Daniel Lezcano wrote:
>
> Hi Florian,
>
> On 04/12/2019 05:50, Florian Fainelli wrote:
>>
>>
>> On 11/20/2019 10:43 AM, Florian Fainelli wrote:
>>>
>>>
>>> On 10/30/2019 11:21 AM, Florian Fainelli wrote:
>>>> Hi,
>>>>
>>>> This patch series contains a bug fix for the existing platforms and then
>>>> paves the way for adding support for Broadcom STB's latest chips in 16nm
>>>> processes, and finally updates the driver with pecularities introduced
>>>> with the 16nm, like the lack of interrupt notification from the HW.
>>>>
>>>> Please queue up the first patch for -stable if you want, thanks!
>>>
>>> Ping?
>>
>> Rui, anyone?
>
> Given comments on patch 1, I'm waiting for the V2.
Yes, I will follow up with a v2 later this week. Thanks
--
Florian