Re: [PATCH V1] thermal: qcom-spmi-temp-alarm: add support for GEN2 PMIC peripherals

[Zhang Rui]

On Thu, 2017-07-13 at 17:39 +0530, Kiran Gunda wrote:
> From: David Collins <collinsd@xxxxxxxxxxxxxx>
> 
> Add support for the TEMP_ALARM GEN2 PMIC peripheral subtype.ÂÂThe
> GEN2 subtype defines an over temperature state with hysteresis
> instead of stage in the status register.ÂÂThere are two GEN2
> states corresponding to stages 1 and 2.
> 
> Signed-off-by: David Collins <collinsd@xxxxxxxxxxxxxx>
> Signed-off-by: Kiran Gunda <kgunda@xxxxxxxxxxxxxx>

Ivan,

can you please review this patch and let me know your opinion?

thanks,
rui
> ---
> Âdrivers/thermal/qcom-spmi-temp-alarm.c | 92
> ++++++++++++++++++++++++++--------
> Â1 file changed, 71 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/thermal/qcom-spmi-temp-alarm.c
> b/drivers/thermal/qcom-spmi-temp-alarm.c
> index f502419..a5e17ba 100644
> --- a/drivers/thermal/qcom-spmi-temp-alarm.c
> +++ b/drivers/thermal/qcom-spmi-temp-alarm.c
> @@ -1,5 +1,5 @@
> Â/*
> - * Copyright (c) 2011-2015, The Linux Foundation. All rights
> reserved.
> + * Copyright (c) 2011-2015, 2017, 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
> @@ -11,6 +11,7 @@
> Â * GNU General Public License for more details.
> Â */
> Â
> +#include <linux/bitops.h>
> Â#include <linux/delay.h>
> Â#include <linux/err.h>
> Â#include <linux/iio/consumer.h>
> @@ -29,13 +30,17 @@
> Â#define QPNP_TM_REG_ALARM_CTRL		0x46
> Â
> Â#define QPNP_TM_TYPE			0x09
> -#define QPNP_TM_SUBTYPE			0x08
> +#define QPNP_TM_SUBTYPE_GEN1		0x08
> +#define QPNP_TM_SUBTYPE_GEN2		0x09
> Â
> -#define STATUS_STAGE_MASK		0x03
> +#define STATUS_GEN1_STAGE_MASK		GENMASK(1, 0)
> +#define STATUS_GEN2_STATE_MASK		GENMASK(6, 4)
> +#define STATUS_GEN2_STATE_SHIFT		4
> Â
> -#define SHUTDOWN_CTRL1_THRESHOLD_MASK	0x03
> +#define SHUTDOWN_CTRL1_OVERRIDE_MASK	GENMASK(7, 6)
> +#define SHUTDOWN_CTRL1_THRESHOLD_MASK	GENMASK(1, 0)
> Â
> -#define ALARM_CTRL_FORCE_ENABLE		0x80
> +#define ALARM_CTRL_FORCE_ENABLE		BIT(7)
> Â
> Â/*
> Â * Trip point values based on threshold control
> @@ -58,6 +63,7 @@
> Âstruct qpnp_tm_chip {
> Â	struct regmap			*map;
> Â	struct thermal_zone_device	*tz_dev;
> +	unsigned int			subtype;
> Â	long				temp;
> Â	unsigned int			thresh;
> Â	unsigned int			stage;
> @@ -66,6 +72,9 @@ struct qpnp_tm_chip {
> Â	struct iio_channel		*adc;
> Â};
> Â
> +/* This array maps from GEN2 alarm state to GEN1 alarm stage */
> +static const unsigned int alarm_state_map[8] = {0, 1, 1, 2, 2, 3, 3,
> 3};
> +
> Âstatic int qpnp_tm_read(struct qpnp_tm_chip *chip, u16 addr, u8
> *data)
> Â{
> Â	unsigned int val;
> @@ -84,30 +93,59 @@ static int qpnp_tm_write(struct qpnp_tm_chip
> *chip, u16 addr, u8 data)
> Â	return regmap_write(chip->map, chip->base + addr, data);
> Â}
> Â
> +/**
> + * qpnp_tm_get_temp_stage() - return over-temperature stage
> + * @chip:		Pointer to the qpnp_tm chip
> + *
> + * Return: stage (GEN1) or state (GEN2) on success, or errno on
> failure.
> + */
> +static int qpnp_tm_get_temp_stage(struct qpnp_tm_chip *chip)
> +{
> +	int ret;
> +	u8 reg = 0;
> +
> +	ret = qpnp_tm_read(chip, QPNP_TM_REG_STATUS, &reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (chip->subtype == QPNP_TM_SUBTYPE_GEN1)
> +		ret = reg & STATUS_GEN1_STAGE_MASK;
> +	else
> +		ret = (reg & STATUS_GEN2_STATE_MASK) >>
> STATUS_GEN2_STATE_SHIFT;
> +
> +	return ret;
> +}
> +
> Â/*
> Â * 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;
> +	unsigned int stage, stage_new, stage_old;
> Â	int ret;
> -	u8 reg = 0;
> Â
> -	ret = qpnp_tm_read(chip, QPNP_TM_REG_STATUS, &reg);
> +	ret = qpnp_tm_get_temp_stage(chip);
> Â	if (ret < 0)
> Â		return ret;
> +	stage = ret;
> Â
> -	stage = reg & STATUS_STAGE_MASK;
> +	if (chip->subtype == QPNP_TM_SUBTYPE_GEN1) {
> +		stage_new = stage;
> +		stage_old = chip->stage;
> +	} else {
> +		stage_new = alarm_state_map[stage];
> +		stage_old = alarm_state_map[chip->stage];
> +	}
> Â
> -	if (stage > chip->stage) {
> +	if (stage_new > stage_old) {
> Â		/* increasing stage, use lower bound */
> -		chip->temp = (stage - 1) * TEMP_STAGE_STEP +
> +		chip->temp = (stage_new - 1) * TEMP_STAGE_STEP +
> Â			ÂÂÂÂÂchip->thresh * TEMP_THRESH_STEP +
> Â			ÂÂÂÂÂTEMP_STAGE_HYSTERESIS +
> TEMP_THRESH_MIN;
> -	} else if (stage < chip->stage) {
> +	} else if (stage_new < stage_old) {
> Â		/* decreasing stage, use upper bound */
> -		chip->temp = stage * TEMP_STAGE_STEP +
> +		chip->temp = stage_new * TEMP_STAGE_STEP +
> Â			ÂÂÂÂÂchip->thresh * TEMP_THRESH_STEP -
> Â			ÂÂÂÂÂTEMP_STAGE_HYSTERESIS +
> TEMP_THRESH_MIN;
> Â	}
> @@ -162,28 +200,37 @@ static irqreturn_t qpnp_tm_isr(int irq, void
> *data)
> Â */
> Âstatic int qpnp_tm_init(struct qpnp_tm_chip *chip)
> Â{
> +	unsigned int stage;
> Â	int ret;
> -	u8 reg;
> +	u8 reg = 0;
> Â
> -	chip->thresh = THRESH_MIN;
> +	ret = qpnp_tm_read(chip, QPNP_TM_REG_SHUTDOWN_CTRL1, &reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	chip->thresh = reg & SHUTDOWN_CTRL1_THRESHOLD_MASK;
> Â	chip->temp = DEFAULT_TEMP;
> Â
> -	ret = qpnp_tm_read(chip, QPNP_TM_REG_STATUS, &reg);
> +	ret = qpnp_tm_get_temp_stage(chip);
> Â	if (ret < 0)
> Â		return ret;
> +	chip->stage = ret;
> Â
> -	chip->stage = reg & STATUS_STAGE_MASK;
> +	stage = chip->subtype == QPNP_TM_SUBTYPE_GEN1
> +		? chip->stage : alarm_state_map[chip->stage];
> Â
> -	if (chip->stage)
> +	if (stage)
> Â		chip->temp = chip->thresh * TEMP_THRESH_STEP +
> -			ÂÂÂÂÂ(chip->stage - 1) * TEMP_STAGE_STEP +
> +			ÂÂÂÂÂ(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;
> +	chip->thresh = THRESH_MIN;
> +	reg &= ~(SHUTDOWN_CTRL1_OVERRIDE_MASK |
> SHUTDOWN_CTRL1_THRESHOLD_MASK);
> +	reg |= chip->thresh & SHUTDOWN_CTRL1_THRESHOLD_MASK;
> Â	ret = qpnp_tm_write(chip, QPNP_TM_REG_SHUTDOWN_CTRL1, reg);
> Â	if (ret < 0)
> Â		return ret;
> @@ -242,13 +289,16 @@ static int qpnp_tm_probe(struct platform_device
> *pdev)
> Â		goto fail;
> Â	}
> Â
> -	if (type != QPNP_TM_TYPE || subtype != QPNP_TM_SUBTYPE) {
> +	if (type != QPNP_TM_TYPE || (subtype != QPNP_TM_SUBTYPE_GEN1
> +				ÂÂÂÂÂ&& subtype !=
> QPNP_TM_SUBTYPE_GEN2)) {
> Â		dev_err(&pdev->dev, "invalid type 0x%02x or subtype
> 0x%02x\n",
> Â			type, subtype);
> Â		ret = -ENODEV;
> Â		goto fail;
> Â	}
> Â
> +	chip->subtype = subtype;
> +
> Â	ret = qpnp_tm_init(chip);
> Â	if (ret < 0) {
> Â		dev_err(&pdev->dev, "init failed\n");