On 07-10-2013 10:52, Eduardo Valentin wrote: > On 04-10-2013 18:17, Wendy Ng wrote: >> Hello Eduardo, >> > > Hello Wendy, > >> I have rebased my work on the top of yours and tried out the new way of >> registering the thermal zone. The new method is certainly making >> things easier for me to create a new thermal zone device driver. But I >> have some questions and I hope to hear back from you before I upload my >> new patch set that is based on your changes. > > Great that you gave it a shot. Thanks for that. > >> >> 1. I found that the hwmon sysfs interface is no longer created when I >> call thermal_zone_of_sensor_register() to register my thermal sensor. I >> believe it is because you purposely hard-coded 'no_hwmon' to true. >> Would it be possible to let the users to control this option as part of >> the sensor register function? > > Hmmm.. I also thought of allowing the thermal to hwmon interface > configurable. But the interface gets created in different path from the > sensor registration, which makes things a bit harder. The zones are > probed while parsing the DT file. While the sensors get linked, > thermal_zone_of_sensor_register(), typically when sensors are probed. > > Do you have a real use case for using the thermal to hwmon interface? > Durgadoss (original author) and Rui were considering deprecating and > finally removing it. If yes, it is time to rise your voice here. > >> >> 2. The of_thermal_get_temp() output parameter 'temp' is an unsigned >> value in order to conforms with the thermal framework 'get_temp' >> function prototype. But the new sensor interface 'get_temp' callback >> function (defined in __thermal_zone struct) expects 'temp' to be >> signed. So technically, a negative value can be returned from the >> underlying sensor function. Should some checks be added to >> of_thermal_get_temp() to ensure that a negative value (i.e. a very large >> unsigned value) won't be passed back to the thermal framework? Or >> should the 'get_temp' function prototype in __thermal_zone struct be >> changed with 'unsigned' temperature value and it will be the >> responsibility of the underlying function to do the check? >> > > I could for sure, add a warning inside of_thermal_get_temp(). But in > general, I believe the thermal framework has to cover for signed > temperature values, even if typical usage is on positive temperature > range. Mainly for correct temperature representation. > > >> 3. Your documentation (thermal.txt) seems to suggest that the >> 'cooling-attachments' are one of the required properties for >> 'thermal-zones' node in the device tree binding files. However, I don't > > Hmm... That was not the intention. > >> have any cooling device ready in my current patch series yet and >> therefore I tried to leave out the 'cooling-attachments' node from the >> 'thermal-zones' node. It seems to be working just find and I can >> register the sensor and read out the temperature once the target >> platform has been booted up. Is this your expectation that people can >> leave the 'cooling-attachments' node out? I want to make sure I can >> still submit a new patch set for this series with just the functionality >> of reading out the temperature from the thermal sensor. > > So, let me try to clarify a couple of things. First thing is that the > cooling-attachments has been renamed to cooling-maps, just a new naming > convention. Also, there are thermal zones that do not have > cooling-maps, they just describe thermal shutdown by software, thermal > zones with only CRITICAL trip points, for instance. For this reason, > thermal zones must be probed properly without cooling-maps. I need to > double check the documentation in this case, I believe.
Just to clarify a bit, this does not mean we should allow having thermal zones only for monitoring. If you are generating a thermal zone, it means you want to cover thermal limits. The example I gave was a thermal zone that does not have cooling-maps, but it does have a thermal constraint, a trip point which is CRITICAL, meaning, the silicon state is not reliable anymore. > >> >> Thanks, >> -Wendy >> >> On 9/23/2013 4:46 PM, Eduardo Valentin wrote: >>> On 23-09-2013 18:43, Wendy Ng wrote: >>>> On 9/23/2013 11:19 AM, Eduardo Valentin wrote: >>>>> On 23-09-2013 13:51, Wendy Ng wrote: >>>>>> This adds the support for reading out temperature from Broadcom >>>>>> bcm281xx >>>>>> SoCs. >>>>>> >>>>>> Signed-off-by: Wendy Ng <wendy...@broadcom.com> >>>>>> Reviewed-by: Markus Mayer <mma...@broadcom.com> >>>>>> Reviewed-by: Christian Daudt <c...@broadcom.com> >>>>>> --- >>>>>> .../bindings/thermal/bcm-kona-thermal.txt | 18 +++ >>>>>> drivers/thermal/Kconfig | 10 ++ >>>>>> drivers/thermal/Makefile | 1 + >>>>>> drivers/thermal/bcm_thermal.c | 170 >>>>>> ++++++++++++++++++++ >>>>>> 4 files changed, 199 insertions(+) >>>>>> create mode 100644 >>>>>> Documentation/devicetree/bindings/thermal/bcm-kona-thermal.txt >>>>>> create mode 100644 drivers/thermal/bcm_thermal.c >>>>>> >>>>>> diff --git >>>>>> a/Documentation/devicetree/bindings/thermal/bcm-kona-thermal.txt >>>>>> b/Documentation/devicetree/bindings/thermal/bcm-kona-thermal.txt >>>>>> new file mode 100644 >>>>>> index 0000000..acca99e >>>>>> --- /dev/null >>>>>> +++ b/Documentation/devicetree/bindings/thermal/bcm-kona-thermal.txt >>>>>> @@ -0,0 +1,18 @@ >>>>>> +* Broadcom Kona Thermal Management Unit >>>>>> + >>>>>> +This version is for the BCM281xx family of SoCs. >>>>>> + >>>>>> +Required properties: >>>>>> +- compatible : "brcm,bcm11351-thermal", "brcm,kona-thermal" >>>>>> +- reg : Address range of the thermal register >>>>>> +- thermal-name: this entry must be specified and it will be passed >>>>>> into >>>>>> +thermal_zone_device_register(). This name will also be reported >>>>>> under Hwmon >>>>>> +sysfs 'name' attribute. >>>>>> + >>>>>> +Example: >>>>>> + thermal@34008000 { >>>>>> + compatible = "brcm,bcm11351-thermal", "brcm,kona-thermal"; >>>>>> + reg = <0x34008000 0x0024>; >>>>>> + thermal-name = "bcm_kona_therm"; >>>>>> + status = "disabled"; >>>>>> + }; >>>>>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig >>>>>> index dbfc390..7f823f0 100644 >>>>>> --- a/drivers/thermal/Kconfig >>>>>> +++ b/drivers/thermal/Kconfig >>>>>> @@ -134,6 +134,16 @@ config KIRKWOOD_THERMAL >>>>>> Support for the Kirkwood thermal sensor driver into the Linux >>>>>> thermal >>>>>> framework. Only kirkwood 88F6282 and 88F6283 have this >>>>>> sensor. >>>>>> +config BCM_THERMAL >>>>>> + tristate "Temperature sensor on Broadcom BCM281xx family of SoCs" >>>>>> + depends on ARCH_BCM >>>>>> + default y >>>>>> + help >>>>>> + If you say yes here you get support for TMU (Thermal Management >>>>>> + Unit) on Broadcom BCM281xx family of SoCs. This provides >>>>>> thermal >>>>>> + monitoring of CPU clusters, graphics, and SoC glue, but does >>>>>> not >>>>>> + include monitoring of charger temperature. >>>>>> + >>>>>> config DOVE_THERMAL >>>>>> tristate "Temperature sensor on Marvell Dove SoCs" >>>>>> depends on ARCH_DOVE >>>>>> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile >>>>>> index 584b363..3ea8c1c 100644 >>>>>> --- a/drivers/thermal/Makefile >>>>>> +++ b/drivers/thermal/Makefile >>>>>> @@ -21,6 +21,7 @@ obj-$(CONFIG_SPEAR_THERMAL) += spear_thermal.o >>>>>> obj-$(CONFIG_RCAR_THERMAL) += rcar_thermal.o >>>>>> obj-$(CONFIG_KIRKWOOD_THERMAL) += kirkwood_thermal.o >>>>>> obj-y += samsung/ >>>>>> +obj-$(CONFIG_BCM_THERMAL) += bcm_thermal.o >>>>>> obj-$(CONFIG_DOVE_THERMAL) += dove_thermal.o >>>>>> obj-$(CONFIG_DB8500_THERMAL) += db8500_thermal.o >>>>>> obj-$(CONFIG_ARMADA_THERMAL) += armada_thermal.o >>>>>> diff --git a/drivers/thermal/bcm_thermal.c >>>>>> b/drivers/thermal/bcm_thermal.c >>>>>> new file mode 100644 >>>>>> index 0000000..131d3c4 >>>>>> --- /dev/null >>>>>> +++ b/drivers/thermal/bcm_thermal.c >>>>>> @@ -0,0 +1,170 @@ >>>>>> +/* >>>>>> + * Copyright 2013 Broadcom Corporation. >>>>>> + * >>>>>> + * This program is free software; you can redistribute it and/or >>>>>> modify >>>>>> + * it under the terms of the GNU General Public License, version 2, >>>>>> + * as published by the Free Software Foundation (the "GPL"). >>>>>> + * >>>>>> + * 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. >>>>>> + * >>>>>> + * A copy of the GPL is available at >>>>>> http://www.broadcom.com/licenses/GPLv2.php, >>>>>> + * or by writing to the Free Software Foundation, Inc., >>>>>> + * 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. >>>>>> + */ >>>>>> + >>>>>> +/** >>>>>> +* Broadcom Thermal Management Unit - bcm_tmu >>>>>> +*/ >>>>>> +#include <linux/module.h> >>>>>> +#include <linux/err.h> >>>>>> +#include <linux/kernel.h> >>>>>> +#include <linux/platform_device.h> >>>>>> +#include <linux/io.h> >>>>>> +#include <linux/thermal.h> >>>>>> +#include <linux/of.h> >>>>>> + >>>>>> +/* From TMON Register Database */ >>>>>> +#define TMON_TEMP_VAL_OFFSET 0x0000001c >>>>>> +#define TMON_TEMP_VAL_TEMP_VAL_SHIFT 0 >>>>>> +#define TMON_TEMP_VAL_TEMP_VAL_MASK 0x000003ff >>>>>> + >>>>>> +/* Broadcom Thermal Zone Device Structure */ >>>>>> +struct bcm_thermal_zone_priv { >>>>>> + char name[THERMAL_NAME_LENGTH]; >>>>>> + void __iomem *base; >>>>>> +}; >>>>>> + >>>>>> +/* Temperature conversion function for TMON block */ >>>>>> +static long raw_to_mcelsius(u32 raw) >>>>>> +{ >>>>>> + /* >>>>>> + * According to Broadcom internal Analog Module Specification >>>>>> + * the formula for converting TMON block output to temperature in >>>>>> + * degree Celsius is: >>>>>> + * T = 428 - (0.561 * raw) >>>>>> + * Note: the valid operating range for the TMON block is -40C to >>>>>> 125C >>>>>> + */ >>>>>> + return 428000 - (561 * (long)raw); >>>>>> +} >>>>>> + >>>>>> +/* Get temperature callback function for thermal zone */ >>>>>> +static int bcm_get_temp(struct thermal_zone_device *thermal, >>>>>> + unsigned long *temp) >>>>>> +{ >>>>>> + u32 raw; >>>>>> + long mcelsius; >>>>>> + struct bcm_thermal_zone_priv *priv = thermal->devdata; >>>>>> + >>>>>> + if (!priv) { >>>>>> + pr_err("%s: thermal zone number %d devdata not >>>>>> initialized.\n", >>>>>> + __func__, thermal->id); >>>>>> + return -EINVAL; >>>>>> + } >>>>>> + >>>>>> + raw = (readl(priv->base + TMON_TEMP_VAL_OFFSET) >>>>>> + & TMON_TEMP_VAL_TEMP_VAL_MASK) >> >>>>>> TMON_TEMP_VAL_TEMP_VAL_SHIFT; >>>>>> + >>>>>> + pr_debug("%s: thermal zone number %d raw temp 0x%x\n", __func__, >>>>>> + thermal->id, raw); >>>>>> + >>>>>> + mcelsius = raw_to_mcelsius(raw); >>>>>> + >>>>>> + /* >>>>>> + * Since 'mcelsius' might be negative, we need to limit it to >>>>>> smallest >>>>>> + * unsigned value before returning it to thermal framework. >>>>>> + */ >>>>>> + if (mcelsius < 0) >>>>>> + *temp = 0; >>>>>> + else >>>>>> + *temp = mcelsius; >>>>>> + >>>>>> + pr_debug("%s: thermal zone number %d final temp %d\n", __func__, >>>>>> + thermal->id, (int) *temp); >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +/* Operation callback functions for thermal zone */ >>>>>> +static struct thermal_zone_device_ops bcm_dev_ops = { >>>>>> + .get_temp = bcm_get_temp, >>>>>> +}; >>>>>> + >>>>>> +static const struct of_device_id bcm_tmu_match_table[] = { >>>>>> + { .compatible = "brcm,kona-thermal" }, >>>>>> + {}, >>>>>> +}; >>>>>> +MODULE_DEVICE_TABLE(of, bcm_tmu_match_table); >>>>>> + >>>>>> +static int bcm_tmu_probe(struct platform_device *pdev) >>>>>> +{ >>>>>> + struct thermal_zone_device *thermal = NULL; >>>>>> + struct bcm_thermal_zone_priv *priv; >>>>>> + struct resource *res; >>>>>> + const char *str; >>>>>> + >>>>>> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); >>>>>> + if (!priv) { >>>>>> + dev_err(&pdev->dev, "Failed to malloc priv.\n"); >>>>>> + return -ENOMEM; >>>>>> + } >>>>>> + >>>>>> + /* Obtain the tmu name from device tree file */ >>>>>> + if (of_property_read_string(pdev->dev.of_node, "thermal-name", >>>>> Hello Wendy, >>>>> >>>>> I would prefer we wait until the work for thermal data [1] gets >>>>> accepted >>>>> before accepting this driver, specially because you are adding device >>>>> specific DT entry to help in your registration with the thermal >>>>> framework. With the mentioned work you wont need it at all. >>>>> >>>>> All best, >>>>> >>>>> [1] - http://lkml.org/lkml/2013/9/15/122 >>>> Hello Eduardo, >>>> >>>> Since your changes are quite extensive, would you recommend me to get >>>> your 16 patches into my local workspace and try to integrate the code >>>> from my patches on the top of your patches at this time? Or should I >>>> wait until your patches get accepted. >>> Hello again Wendy, >>> >>> The only part I am concerned is the device tree entry you are creating >>> for your device/driver. That could be obsolete in near future with the >>> work I am doing currently. >>> >>> The work is, as you mentioned, quite extensive, but should make things >>> easier for device driver writer. If you want, of course, it would be >>> great if you could rebase your work on top of mine and try it out on >>> your side and let me know the outcome. >>> >>> Comments on the proposed binding and documentation is also welcome. Let >>> me know if there is something that it is not clear. >>> >>>> Thanks, >>>> -Wendy >>>>>> + &str) == 0) { >>>>>> + strlcpy(priv->name, str, sizeof(priv->name)); >>>>>> + } else { >>>>>> + dev_err(&pdev->dev, "Failed to get thermal-name from DT.\n"); >>>>>> + return -EINVAL; >>>>>> + } >>>>>> + >>>>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>>>> + priv->base = devm_ioremap_resource(&pdev->dev, res); >>>>>> + if (IS_ERR(priv->base)) >>>>>> + return PTR_ERR(priv->base); >>>>>> + >>>>>> + thermal = thermal_zone_device_register(priv->name, 0, 0, priv, >>>>>> + &bcm_dev_ops, NULL, 0, 0); >>>>>> + if (IS_ERR(thermal)) { >>>>>> + dev_err(&pdev->dev, >>>>>> + "Failed to register Broadcom thermal zone device.\n"); >>>>>> + return PTR_ERR(thermal); >>>>>> + } >>>>>> + >>>>>> + platform_set_drvdata(pdev, thermal); >>>>>> + >>>>>> + dev_info(&pdev->dev, "Broadcom Thermal Monitor Initialized.\n"); >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static int bcm_tmu_remove(struct platform_device *pdev) >>>>>> +{ >>>>>> + struct thermal_zone_device *broadcom_thermal = >>>>>> + platform_get_drvdata(pdev); >>>>>> + >>>>>> + thermal_zone_device_unregister(broadcom_thermal); >>>>>> + >>>>>> + dev_info(&pdev->dev, "Broadcom Thermal Monitor >>>>>> Uninitialized.\n"); >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static struct platform_driver bcm_tmu_driver = { >>>>>> + .driver = { >>>>>> + .name = "bcm-thermal", >>>>>> + .owner = THIS_MODULE, >>>>>> + .of_match_table = bcm_tmu_match_table, >>>>>> + }, >>>>>> + .probe = bcm_tmu_probe, >>>>>> + .remove = bcm_tmu_remove, >>>>>> +}; >>>>>> + >>>>>> +module_platform_driver(bcm_tmu_driver); >>>>>> + >>>>>> +MODULE_DESCRIPTION("Broadcom Thermal Driver"); >>>>>> +MODULE_AUTHOR("Broadcom"); >>>>>> +MODULE_LICENSE("GPL v2"); >>>>>> +MODULE_ALIAS("platform:bcm-thermal"); >>>>>> >>> >> > > -- You have got to be excited about what you are doing. (L. Lamport) Eduardo Valentin
signature.asc
Description: OpenPGP digital signature