Re: [PATCH v4] thermal: Add QPNP PMIC temperature alarm driver

[Eduardo Valentin <edubezval@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 14896 bytes --]

Ivan,

On Mon, Feb 02, 2015 at 05:19:30PM +0200, Ivan T. Ivanov wrote:
> Add support for the temperature alarm peripheral found inside
> Qualcomm plug-and-play (QPNP) PMIC chips. The temperature alarm
> peripheral outputs a pulse on an interrupt line whenever the
> thermal over temperature stage value changes.
> 
> Register a thermal sensor. The temperature reported by this thermal
> sensor device should reflect the actual PMIC die temperature if an
> ADC is present on the given PMIC. If no ADC is present, then the
> reported temperature should be estimated from the over temperature
> stage value.
> 
> Cc: David Collins <collinsd@codeaurora.org>
> Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
> ---
> 
> Changes since v3:
> 
> - Driver register thermal sensor instead thermal zone. Device thermal zone
>   should be properly described in DT files according thermal.txt document.

Thanks a lot for keeping this up. I believe the driver looks smaller and
cleaner now, don't you agree?

> - Dropped support for software override PMIC shutdown sequence and related
>   bit definitions. If software did not take action for clean device shutdown,
>   until critical temperature is reached, PMIC chip will execute internal
>   pre-programed shutdown sequence.
> 

OK. Let me know if this functionality is crucial and needs further
discussion.

I have two very minor comments as follows.


> v3: http://comments.gmane.org/gmane.linux.ports.arm.msm/10071
> 
>  .../bindings/thermal/qcom-spmi-temp-alarm.txt      |  57 ++++
>  drivers/thermal/Kconfig                            |  11 +
>  drivers/thermal/Makefile                           |   1 +
>  drivers/thermal/qcom-spmi-temp-alarm.c             | 311 +++++++++++++++++++++
>  4 files changed, 380 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/thermal/qcom-spmi-temp-alarm.txt
>  create mode 100644 drivers/thermal/qcom-spmi-temp-alarm.c
> 
> diff --git a/Documentation/devicetree/bindings/thermal/qcom-spmi-temp-alarm.txt b/Documentation/devicetree/bindings/thermal/qcom-spmi-temp-alarm.txt
> new file mode 100644
> index 0000000..290ec06
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/thermal/qcom-spmi-temp-alarm.txt
> @@ -0,0 +1,57 @@
> +Qualcomm QPNP PMIC Temperature Alarm
> +
> +QPNP temperature alarm peripherals are found inside of Qualcomm PMIC chips
> +that utilize the Qualcomm SPMI implementation. These peripherals provide an
> +interrupt signal and status register to identify high PMIC die temperature.
> +
> +Required properties:
> +- compatible:      Should contain "qcom,spmi-temp-alarm".
> +- reg:             Specifies the SPMI address and length of the controller's
> +                   registers.
> +- interrupts:      PMIC temperature alarm interrupt.
> +- #thermal-sensor-cells: Should be 0. See thermal.txt for a description.
> +
> +Optional properties:
> +- io-channels:     Should contain IIO channel specifier for the ADC channel,
> +                   which report chip die temperature.
> +- io-channel-names: Should contain "thermal".
> +
> +Example:
> +
> +	pm8941_temp: thermal-alarm@2400 {
> +		compatible = "qcom,spmi-temp-alarm";
> +		reg = <0x2400 0x100>;
> +		interrupts = <0 0x24 0 IRQ_TYPE_EDGE_RISING>;
> +		#thermal-sensor-cells = <0>;
> +
> +		io-channels = <&pm8941_vadc VADC_DIE_TEMP>;
> +		io-channel-names = "thermal";
> +	};
> +
> +	thermal-zones {
> +		pm8941 {
> +			polling-delay-passive = <250>;
> +			polling-delay = <1000>;
> +
> +			thermal-sensors = <&pm8941_temp>;
> +
> +			trips {
> +				passive {
> +					temperature = <1050000>;
> +					hysteresis = <2000>;
> +					type = "passive";
> +				};
> +				alert {
> +					temperature = <125000>;
> +					hysteresis = <2000>;
> +					type = "hot";
> +				};
> +				crit {
> +					temperature = <145000>;
> +					hysteresis = <2000>;
> +					type = "critical";
> +				};
> +			};
> +		};
> +	};

Do you have the appropriate DT changes under architecture code too?

I mean, I am fine picking these changes, but should this series include
also a minor inclusion under arch/arm/boot/dts too, given that you
already did the hard part?

> +
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index af40db0..30aee81 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -299,4 +299,15 @@ depends on ARCH_STI && OF
>  source "drivers/thermal/st/Kconfig"
>  endmenu
> 
> +config QCOM_SPMI_TEMP_ALARM
> +	tristate "Qualcomm SPMI PMIC Temperature Alarm"
> +	depends on OF && SPMI && IIO
> +	select REGMAP_SPMI
> +	help
> +	  This enables a thermal sysfs driver for Qualcomm plug-and-play (QPNP)
> +	  PMIC devices. It shows up in sysfs as a thermal sensor with multiple
> +	  trip points. The temperature reported by the thermal sensor reflects the
> +	  real time die temperature if an ADC is present or an estimate of the
> +	  temperature based upon the over temperature stage value.
> +
>  endif
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index fa0dc48..1fe8665 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -22,6 +22,7 @@ thermal_sys-$(CONFIG_CPU_THERMAL)	+= cpu_cooling.o
>  thermal_sys-$(CONFIG_CLOCK_THERMAL)	+= clock_cooling.o
> 
>  # platform thermal drivers
> +obj-$(CONFIG_QCOM_SPMI_TEMP_ALARM)	+= qcom-spmi-temp-alarm.o
>  obj-$(CONFIG_SPEAR_THERMAL)	+= spear_thermal.o
>  obj-$(CONFIG_ROCKCHIP_THERMAL)	+= rockchip_thermal.o
>  obj-$(CONFIG_RCAR_THERMAL)	+= rcar_thermal.o
> diff --git a/drivers/thermal/qcom-spmi-temp-alarm.c b/drivers/thermal/qcom-spmi-temp-alarm.c
> new file mode 100644
> index 0000000..7215ec7
> --- /dev/null
> +++ b/drivers/thermal/qcom-spmi-temp-alarm.c
> @@ -0,0 +1,311 @@
> +/*
> + * Copyright (c) 2011-2015, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * 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.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/iio/consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/thermal.h>
> +
> +#define QPNP_TM_REG_TYPE		0x04
> +#define QPNP_TM_REG_SUBTYPE		0x05
> +#define QPNP_TM_REG_STATUS		0x08
> +#define QPNP_TM_REG_SHUTDOWN_CTRL1	0x40
> +#define QPNP_TM_REG_ALARM_CTRL		0x46
> +
> +#define QPNP_TM_TYPE			0x09
> +#define QPNP_TM_SUBTYPE			0x08
> +
> +#define STATUS_STAGE_MASK		0x03
> +
> +#define SHUTDOWN_CTRL1_THRESHOLD_MASK	0x03
> +
> +#define ALARM_CTRL_FORCE_ENABLE		0x80
> +
> +/*
> + * Trip point values based on threshold control
> + * 0 = {105 C, 125 C, 145 C}
> + * 1 = {110 C, 130 C, 150 C}
> + * 2 = {115 C, 135 C, 155 C}
> + * 3 = {120 C, 140 C, 160 C}
> +*/
> +#define TEMP_STAGE_STEP			20000	/* Stage step: 20.000 C */
> +#define TEMP_STAGE_HYSTERESIS		2000
> +
> +#define TEMP_THRESH_MIN			105000	/* Threshold Min: 105 C */
> +#define TEMP_THRESH_STEP		5000	/* Threshold step: 5 C */
> +
> +#define THRESH_MIN			0
> +
> +/* Temperature in Milli Celsius reported during stage 0 if no ADC is present */
> +#define DEFAULT_TEMP			37000
> +
> +struct qpnp_tm_chip {
> +	struct regmap			*map;
> +	struct thermal_zone_device	*tz_dev;
> +	long				temp;
> +	unsigned int			thresh;
> +	unsigned int			stage;
> +	unsigned int			prev_stage;
> +	unsigned int			base;
> +	struct iio_channel		*adc;
> +};
> +
> +static int qpnp_tm_read(struct qpnp_tm_chip *chip, u16 addr, u8 *data)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(chip->map, chip->base + addr, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	*data = val;
> +	return 0;
> +}
> +
> +static int qpnp_tm_write(struct qpnp_tm_chip *chip, u16 addr, u8 data)
> +{
> +	return regmap_write(chip->map, chip->base + addr, data);
> +}
> +
> +/*
> + * This function updates the internal temp value based on the
> + * current thermal stage and threshold as well as the previous stage
> + */
> +static int qpnp_tm_update_temp_no_adc(struct qpnp_tm_chip *chip)
> +{
> +	unsigned int stage;
> +	int ret;
> +	u8 reg = 0;
> +
> +	ret = qpnp_tm_read(chip, QPNP_TM_REG_STATUS, &reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	stage = reg & STATUS_STAGE_MASK;
> +
> +	if (stage > chip->stage) {
> +		/* increasing stage, use lower bound */
> +		chip->temp = (stage - 1) * TEMP_STAGE_STEP +
> +			     chip->thresh * TEMP_THRESH_STEP +
> +			     TEMP_STAGE_HYSTERESIS + TEMP_THRESH_MIN;
> +	} else if (stage < chip->stage) {
> +		/* decreasing stage, use upper bound */
> +		chip->temp = stage * TEMP_STAGE_STEP +
> +			     chip->thresh * TEMP_THRESH_STEP -
> +			     TEMP_STAGE_HYSTERESIS + TEMP_THRESH_MIN;
> +	}

For my own edification, no change in state means no change in
temperature too, right?

> +
> +	chip->stage = stage;
> +
> +	return 0;
> +}
> +
> +static int qpnp_tm_get_temp(void *data, long *temp)
> +{
> +	struct qpnp_tm_chip *chip = data;
> +	int ret, mili_celsius;
> +
> +	if (!temp)
> +		return -EINVAL;
> +
> +	if (IS_ERR(chip->adc)) {
> +		ret = qpnp_tm_update_temp_no_adc(chip);
> +		if (ret < 0)
> +			return ret;
> +	} else {
> +		ret = iio_read_channel_processed(chip->adc, &mili_celsius);
> +		if (ret < 0)
> +			return ret;
> +
> +		chip->temp = mili_celsius;
> +	}
> +
> +	*temp = chip->temp < 0 ? 0 : chip->temp;
> +
> +	return 0;
> +}
> +
> +static const struct thermal_zone_of_device_ops qpnp_tm_sensor_ops = {
> +	.get_temp = qpnp_tm_get_temp,
> +};
> +
> +static irqreturn_t qpnp_tm_isr(int irq, void *data)
> +{
> +	struct qpnp_tm_chip *chip = data;
> +
> +	thermal_zone_device_update(chip->tz_dev);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +/*
> + * This function initializes the internal temp value based on only the
> + * current thermal stage and threshold. Setup threshold control and
> + * disable shutdown override.
> + */
> +static int qpnp_tm_init(struct qpnp_tm_chip *chip)
> +{
> +	int ret;
> +	u8 reg;
> +
> +	chip->thresh = THRESH_MIN;
> +	chip->temp = DEFAULT_TEMP;
> +
> +	ret = qpnp_tm_read(chip, QPNP_TM_REG_STATUS, &reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	chip->stage = reg & STATUS_STAGE_MASK;
> +
> +	if (chip->stage)
> +		chip->temp = chip->thresh * TEMP_THRESH_STEP +
> +			     (chip->stage - 1) * TEMP_STAGE_STEP +
> +			     TEMP_THRESH_MIN;
> +
> +	/*
> +	 * Set threshold and disable software override of stage 2 and 3
> +	 * shutdowns.
> +	 */
> +	reg = chip->thresh & SHUTDOWN_CTRL1_THRESHOLD_MASK;
> +	ret = qpnp_tm_write(chip, QPNP_TM_REG_SHUTDOWN_CTRL1, reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Enable the thermal alarm PMIC module in always-on mode. */
> +	reg = ALARM_CTRL_FORCE_ENABLE;
> +	ret = qpnp_tm_write(chip, QPNP_TM_REG_ALARM_CTRL, reg);
> +
> +	return ret;
> +}
> +
> +static int qpnp_tm_probe(struct platform_device *pdev)
> +{
> +	struct qpnp_tm_chip *chip;
> +	struct device_node *node;
> +	u8 type, subtype;
> +	u32 res[2];
> +	int ret, irq;
> +
> +	node = pdev->dev.of_node;
> +
> +	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(&pdev->dev, chip);
> +
> +	chip->map = dev_get_regmap(pdev->dev.parent, NULL);
> +	if (!chip->map)
> +		return -ENXIO;
> +
> +	ret = of_property_read_u32_array(node, "reg", res, 2);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* ADC based measurements are optional */
> +	chip->adc = iio_channel_get(&pdev->dev, "thermal");
> +	if (PTR_ERR(chip->adc) == -EPROBE_DEFER)
> +		return PTR_ERR(chip->adc);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
> +
> +	chip->base = res[0];
> +
> +	ret = qpnp_tm_read(chip, QPNP_TM_REG_TYPE, &type);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "could not read type\n");
> +		goto fail;
> +	}
> +
> +	ret = qpnp_tm_read(chip, QPNP_TM_REG_SUBTYPE, &subtype);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "could not read subtype\n");
> +		goto fail;
> +	}
> +
> +	if (type != QPNP_TM_TYPE || subtype != QPNP_TM_SUBTYPE) {
> +		dev_err(&pdev->dev, "invalid type 0x%02x or subtype 0x%02x\n",
> +			type, subtype);
> +		ret = -ENODEV;
> +		goto fail;
> +	}
> +
> +	ret = qpnp_tm_init(chip);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "init failed\n");
> +		goto fail;
> +	}
> +
> +	chip->tz_dev = thermal_zone_of_sensor_register(&pdev->dev, 0, chip,
> +							&qpnp_tm_sensor_ops);
> +	if (IS_ERR(chip->tz_dev)) {
> +		dev_err(&pdev->dev, "failed to register sensor\n");
> +		ret = PTR_ERR(chip->tz_dev);
> +		goto fail;
> +	}
> +
> +	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, qpnp_tm_isr,
> +					IRQF_ONESHOT, node->name, chip);

What if we request this IRQ before registering the of thermal zone
sensor? I believe it makes more sense conceptually, as you mean, you
register into upper layers once your driver is fully ready to do so.

Any objections changing the order?

> +	if (ret < 0)
> +		goto unreg;
> +
> +	return 0;
> +
> +unreg:
> +	thermal_zone_of_sensor_unregister(&pdev->dev, chip->tz_dev);

As mentioned by Stanimir Varban, you may also remove the above, since
the irq is devm.

> +fail:
> +	if (!IS_ERR(chip->adc))
> +		iio_channel_release(chip->adc);
> +
> +	return ret;
> +}
> +
> +static int qpnp_tm_remove(struct platform_device *pdev)
> +{
> +	struct qpnp_tm_chip *chip = dev_get_drvdata(&pdev->dev);
> +
> +	thermal_zone_of_sensor_unregister(&pdev->dev, chip->tz_dev);
> +	if (!IS_ERR(chip->adc))
> +		iio_channel_release(chip->adc);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id qpnp_tm_match_table[] = {
> +	{ .compatible = "qcom,spmi-temp-alarm" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, qpnp_tm_match_table);
> +
> +static struct platform_driver qpnp_tm_driver = {
> +	.driver = {
> +		.name = "spmi-temp-alarm",
> +		.of_match_table = qpnp_tm_match_table,
> +	},
> +	.probe  = qpnp_tm_probe,
> +	.remove = qpnp_tm_remove,
> +};
> +module_platform_driver(qpnp_tm_driver);
> +
> +MODULE_ALIAS("platform:spmi-temp-alarm");
> +MODULE_DESCRIPTION("QPNP PMIC Temperature Alarm driver");
> +MODULE_LICENSE("GPL v2");
> --
> 1.9.1
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]