Re: [PATCH V4 2/4] thermal: Add BCM2711 thermal driver

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 11/01/2020 17:15, Stefan Wahren wrote:
> This adds the thermal sensor driver for the Broadcom BCM2711 SoC,
> which is placed on the Raspberry Pi 4. The driver only provides
> SoC temperature reading so far.
> 
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> Reviewed-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> Tested-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> ---
>  drivers/thermal/broadcom/Kconfig           |   7 ++
>  drivers/thermal/broadcom/Makefile          |   1 +
>  drivers/thermal/broadcom/bcm2711_thermal.c | 129 +++++++++++++++++++++++++++++
>  3 files changed, 137 insertions(+)
>  create mode 100644 drivers/thermal/broadcom/bcm2711_thermal.c
> 
> diff --git a/drivers/thermal/broadcom/Kconfig b/drivers/thermal/broadcom/Kconfig
> index cf43e15..061f1db 100644
> --- a/drivers/thermal/broadcom/Kconfig
> +++ b/drivers/thermal/broadcom/Kconfig
> @@ -1,4 +1,11 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> +config BCM2711_THERMAL
> +	tristate "Broadcom AVS RO thermal sensor driver"
> +	depends on ARCH_BCM2835 || COMPILE_TEST
> +	depends on THERMAL_OF && MFD_SYSCON
> +	help
> +	  Support for thermal sensors on Broadcom BCM2711 SoCs.
> +
>  config BCM2835_THERMAL
>  	tristate "Thermal sensors on bcm2835 SoC"
>  	depends on ARCH_BCM2835 || COMPILE_TEST
> diff --git a/drivers/thermal/broadcom/Makefile b/drivers/thermal/broadcom/Makefile
> index 490ab1f..c917b24 100644
> --- a/drivers/thermal/broadcom/Makefile
> +++ b/drivers/thermal/broadcom/Makefile
> @@ -1,4 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> +obj-$(CONFIG_BCM2711_THERMAL)		+= bcm2711_thermal.o
>  obj-$(CONFIG_BCM2835_THERMAL)		+= bcm2835_thermal.o
>  obj-$(CONFIG_BRCMSTB_THERMAL)		+= brcmstb_thermal.o
>  obj-$(CONFIG_BCM_NS_THERMAL)		+= ns-thermal.o
> diff --git a/drivers/thermal/broadcom/bcm2711_thermal.c b/drivers/thermal/broadcom/bcm2711_thermal.c
> new file mode 100644
> index 0000000..b1d3c4d
> --- /dev/null
> +++ b/drivers/thermal/broadcom/bcm2711_thermal.c
> @@ -0,0 +1,129 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Broadcom AVS RO thermal sensor driver
> + *
> + * based on brcmstb_thermal
> + *
> + * Copyright (C) 2020 Stefan Wahren
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +#include <linux/thermal.h>
> +
> +#include "../thermal_hwmon.h"
> +
> +#define AVS_RO_TEMP_STATUS		0x200
> + #define AVS_RO_TEMP_STATUS_valid_msk	(BIT(16) | BIT(10))
> + #define AVS_RO_TEMP_STATUS_data_msk	GENMASK(9, 0)

extra spaces above and please keep uppercase for the macro.

> +struct bcm2711_thermal_priv {
> +	struct regmap *regmap;
> +	struct device *dev;

There is no gain of adding this pointer for the sake of adding a simple
trace in get_temp. Moreover, if the reading fails, the log will be
flooded with the error. Returning an error is enough, up to the caller
to handle the error and print something or not in this case.

> +	struct thermal_zone_device *thermal;
> +};
> +
> +static int bcm2711_get_temp(void *data, int *temp)
> +{
> +	struct bcm2711_thermal_priv *priv = data;
> +	int slope = thermal_zone_get_slope(priv->thermal);
> +	int offset = thermal_zone_get_offset(priv->thermal);
> +	u32 val;
> +	int ret;
> +	long t;
> +
> +	ret = regmap_read(priv->regmap, AVS_RO_TEMP_STATUS, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (!(val & AVS_RO_TEMP_STATUS_valid_msk)) {
> +		dev_err(priv->dev, "reading not valid\n");
> +		return -EIO;
> +	}
> +
> +	val &= AVS_RO_TEMP_STATUS_data_msk;
> +
> +	/* Convert a HW code to a temperature reading (millidegree celsius) */
> +	t = slope * val + offset;
> +	if (t < 0)
> +		*temp = 0;
> +	else
> +		*temp = t;

	*temp = t < 0 ? 0 : t;

> +	return 0;
> +}
> +
> +static const struct thermal_zone_of_device_ops bcm2711_thermal_of_ops = {
> +	.get_temp	= bcm2711_get_temp,
> +};
> +
> +static const struct of_device_id bcm2711_thermal_id_table[] = {
> +	{ .compatible = "brcm,bcm2711-thermal" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, bcm2711_thermal_id_table);
> +
> +static int bcm2711_thermal_probe(struct platform_device *pdev)
> +{
> +	struct thermal_zone_device *thermal;
> +	struct bcm2711_thermal_priv *priv;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *parent;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	/* get regmap from syscon node */
> +	parent = of_get_parent(dev->of_node); /* parent should be syscon node */
> +	regmap = syscon_node_to_regmap(parent);
> +	of_node_put(parent);
> +	if (IS_ERR(regmap)) {
> +		ret = PTR_ERR(regmap);
> +		dev_err(dev, "failed to get regmap: %d\n", ret);
> +		return ret;
> +	}
> +	priv->regmap = regmap;
> +	priv->dev = dev;
> +
> +	thermal = devm_thermal_zone_of_sensor_register(dev, 0, priv,
> +						       &bcm2711_thermal_of_ops);
> +	if (IS_ERR(thermal)) {
> +		ret = PTR_ERR(thermal);
> +		dev_err(dev, "could not register sensor: %d\n", ret);
> +		return ret;
> +	}
> +
> +	priv->thermal = thermal;
> +
> +	thermal->tzp->no_hwmon = false;
> +	ret = thermal_add_hwmon_sysfs(thermal);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static struct platform_driver bcm2711_thermal_driver = {
> +	.probe = bcm2711_thermal_probe,
> +	.driver = {
> +		.name = "bcm2711_thermal",
> +		.of_match_table = bcm2711_thermal_id_table,
> +	},
> +};
> +module_platform_driver(bcm2711_thermal_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Stefan Wahren");
> +MODULE_DESCRIPTION("Broadcom AVS RO thermal sensor driver");
> 


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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel