On Thu, Feb 13, 2025 at 01:04:01PM -0800, Anjelique Melendez wrote:
> Currently multiple if/else statements are used in functions to decipher
> between SPMI temp alarm Gen 1, Gen 2 and Gen 2 Rev 1 functionality. Instead
> refactor the driver so that SPMI temp alarm chips will have reference to a
> spmi_temp_alarm_data struct which defines data and function callbacks
> based on the HW subtype.
>
> Signed-off-by: Anjelique Melendez <anjelique.melendez@xxxxxxxxxxxxxxxx>
> ---
> drivers/thermal/qcom/qcom-spmi-temp-alarm.c | 143 +++++++++++++-------
> 1 file changed, 95 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/thermal/qcom/qcom-spmi-temp-alarm.c b/drivers/thermal/qcom/qcom-spmi-temp-alarm.c
> index b2077ff9fe73..af71d4238340 100644
> --- a/drivers/thermal/qcom/qcom-spmi-temp-alarm.c
> +++ b/drivers/thermal/qcom/qcom-spmi-temp-alarm.c
> @@ -31,7 +31,6 @@
>
> #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_OVERRIDE_S2 BIT(6)
> #define SHUTDOWN_CTRL1_THRESHOLD_MASK GENMASK(1, 0)
> @@ -68,10 +67,20 @@ static const long temp_map_gen2_v1[THRESH_COUNT][STAGE_COUNT] = {
> /* Temperature in Milli Celsius reported during stage 0 if no ADC is present */
> #define DEFAULT_TEMP 37000
>
> +struct qpnp_tm_chip;
> +
> +struct spmi_temp_alarm_data {
> + const struct thermal_zone_device_ops *ops;
> + const long (*temp_map)[THRESH_COUNT][STAGE_COUNT];
> + int (*get_temp_stage)(struct qpnp_tm_chip *chip);
> + int (*configure_trip_temps)(struct qpnp_tm_chip *chip);
> +};
> +
> struct qpnp_tm_chip {
> struct regmap *map;
> struct device *dev;
> struct thermal_zone_device *tz_dev;
> + const struct spmi_temp_alarm_data *data;
> unsigned int subtype;
> unsigned int dig_revision;
> long temp;
> @@ -82,14 +91,11 @@ struct qpnp_tm_chip {
> struct mutex lock;
> bool initialized;
> bool require_s2_shutdown;
> + long temp_thresh_map[STAGE_COUNT];
>
> struct iio_channel *adc;
> - const long (*temp_map)[THRESH_COUNT][STAGE_COUNT];
> };
>
> -/* 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};
> -
Don't move the code / data without a need, it complicates review.
> static int qpnp_tm_read(struct qpnp_tm_chip *chip, u16 addr, u8 *data)
> {
> unsigned int val;
> @@ -118,34 +124,51 @@ static int qpnp_tm_write(struct qpnp_tm_chip *chip, u16 addr, u8 data)
> */
> static long qpnp_tm_decode_temp(struct qpnp_tm_chip *chip, unsigned int stage)
> {
> - if (!chip->temp_map || chip->thresh >= THRESH_COUNT || stage == 0 ||
> - stage > STAGE_COUNT)
> + if (stage == 0 || stage > STAGE_COUNT)
> return 0;
>
> - return (*chip->temp_map)[chip->thresh][stage - 1];
> + return chip->temp_thresh_map[stage - 1];
> }
>
> /**
> * 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.
> + * Return: stage on success, or errno on failure.
> */
> static int qpnp_tm_get_temp_stage(struct qpnp_tm_chip *chip)
> {
> + u8 reg = 0;
> int ret;
> +
> + ret = qpnp_tm_read(chip, QPNP_TM_REG_STATUS, ®);
> + if (ret < 0)
> + return ret;
> +
> + return FIELD_GET(STATUS_GEN1_STAGE_MASK, reg);
> +}
> +
> +/* 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};
> +
> +/**
> + * qpnp_tm_get_gen2_temp_stage() - return over-temperature stage
> + * @chip: Pointer to the qpnp_tm chip
> + *
> + * Return: stage on success, or errno on failure.
> + */
> +static int qpnp_tm_gen2_get_temp_stage(struct qpnp_tm_chip *chip)
> +{
> u8 reg = 0;
> + int ret;
>
> ret = qpnp_tm_read(chip, QPNP_TM_REG_STATUS, ®);
> 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;
> + ret = FIELD_GET(STATUS_GEN2_STATE_MASK, reg);
>
> - return ret;
> + return alarm_state_map[ret];
> }
>
> /*
> @@ -154,23 +177,16 @@ static int qpnp_tm_get_temp_stage(struct qpnp_tm_chip *chip)
> */
> static int qpnp_tm_update_temp_no_adc(struct qpnp_tm_chip *chip)
> {
> - unsigned int stage, stage_new, stage_old;
> + unsigned int stage_new, stage_old;
> int ret;
>
> WARN_ON(!mutex_is_locked(&chip->lock));
>
> - ret = qpnp_tm_get_temp_stage(chip);
> + ret = chip->data->get_temp_stage(chip);
> if (ret < 0)
> return ret;
> - stage = ret;
> -
> - 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];
> - }
> + stage_new = ret;
> + stage_old = chip->stage;
>
> if (stage_new > stage_old) {
> /* increasing stage, use lower bound */
> @@ -182,7 +198,7 @@ static int qpnp_tm_update_temp_no_adc(struct qpnp_tm_chip *chip)
> - TEMP_STAGE_HYSTERESIS;
> }
>
> - chip->stage = stage;
> + chip->stage = stage_new;
>
> return 0;
> }
> @@ -222,8 +238,8 @@ static int qpnp_tm_get_temp(struct thermal_zone_device *tz, int *temp)
> static int qpnp_tm_update_critical_trip_temp(struct qpnp_tm_chip *chip,
> int temp)
> {
> - long stage2_threshold_min = (*chip->temp_map)[THRESH_MIN][1];
> - long stage2_threshold_max = (*chip->temp_map)[THRESH_MAX][1];
> + long stage2_threshold_min = (*chip->data->temp_map)[THRESH_MIN][1];
> + long stage2_threshold_max = (*chip->data->temp_map)[THRESH_MAX][1];
> bool disable_s2_shutdown = false;
> u8 reg;
>
> @@ -258,6 +274,8 @@ static int qpnp_tm_update_critical_trip_temp(struct qpnp_tm_chip *chip,
> }
>
> skip:
> + memcpy(chip->temp_thresh_map, chip->data->temp_map[chip->thresh],
> + sizeof(chip->temp_thresh_map));
> reg |= chip->thresh;
> if (disable_s2_shutdown && !chip->require_s2_shutdown)
> reg |= SHUTDOWN_CTRL1_OVERRIDE_S2;
> @@ -295,6 +313,42 @@ static irqreturn_t qpnp_tm_isr(int irq, void *data)
> return IRQ_HANDLED;
> }
>
> +static int qpnp_tm_configure_trip_temp(struct qpnp_tm_chip *chip)
> +{
> + int crit_temp, ret;
> +
> + mutex_unlock(&chip->lock);
> +
> + ret = thermal_zone_get_crit_temp(chip->tz_dev, &crit_temp);
> + if (ret)
> + crit_temp = THERMAL_TEMP_INVALID;
> +
> + mutex_lock(&chip->lock);
> +
> + return qpnp_tm_update_critical_trip_temp(chip, crit_temp);
> +}
> +
> +static const struct spmi_temp_alarm_data spmi_temp_alarm_data = {
> + .ops = &qpnp_tm_sensor_ops,
> + .temp_map = &temp_map_gen1,
> + .configure_trip_temps = qpnp_tm_configure_trip_temp,
> + .get_temp_stage = qpnp_tm_get_temp_stage,
> +};
> +
> +static const struct spmi_temp_alarm_data spmi_temp_alarm_gen2_data = {
> + .ops = &qpnp_tm_sensor_ops,
> + .temp_map = &temp_map_gen1,
> + .configure_trip_temps = qpnp_tm_configure_trip_temp,
> + .get_temp_stage = qpnp_tm_gen2_get_temp_stage,
> +};
> +
> +static const struct spmi_temp_alarm_data spmi_temp_alarm_gen2_rev1_data = {
> + .ops = &qpnp_tm_sensor_ops,
All three data structures use &qpnp_tm_sensor_ops and
qpnp_tm_configure_trip_temp(). Plese use them directly. In case it is
necessary for the next patch, please perform this refactoring separately
with a proper explanation ("In preparation to .... peform foo and bar").
> + .temp_map = &temp_map_gen2_v1,
> + .configure_trip_temps = qpnp_tm_configure_trip_temp,
> + .get_temp_stage = qpnp_tm_gen2_get_temp_stage,
> +};
> +
> /*
> * This function initializes the internal temp value based on only the
> * current thermal stage and threshold. Setup threshold control and
> @@ -305,7 +359,6 @@ static int qpnp_tm_init(struct qpnp_tm_chip *chip)
> unsigned int stage;
> int ret;
> u8 reg = 0;
> - int crit_temp;
>
> mutex_lock(&chip->lock);
>
> @@ -316,26 +369,15 @@ static int qpnp_tm_init(struct qpnp_tm_chip *chip)
> chip->thresh = reg & SHUTDOWN_CTRL1_THRESHOLD_MASK;
> chip->temp = DEFAULT_TEMP;
>
> - ret = qpnp_tm_get_temp_stage(chip);
> - if (ret < 0)
> + stage = chip->data->get_temp_stage(chip);
> + if (stage < 0)
> goto out;
> - chip->stage = ret;
> -
> - stage = chip->subtype == QPNP_TM_SUBTYPE_GEN1
> - ? chip->stage : alarm_state_map[chip->stage];
> + chip->stage = stage;
>
> if (stage)
> chip->temp = qpnp_tm_decode_temp(chip, stage);
>
> - mutex_unlock(&chip->lock);
> -
> - ret = thermal_zone_get_crit_temp(chip->tz_dev, &crit_temp);
> - if (ret)
> - crit_temp = THERMAL_TEMP_INVALID;
> -
> - mutex_lock(&chip->lock);
> -
> - ret = qpnp_tm_update_critical_trip_temp(chip, crit_temp);
> + ret = chip->data->configure_trip_temps(chip);
> if (ret < 0)
> goto out;
>
> @@ -439,10 +481,15 @@ static int qpnp_tm_probe(struct platform_device *pdev)
> }
>
> chip->subtype = subtype;
> - if (subtype == QPNP_TM_SUBTYPE_GEN2 && dig_major >= 1)
> - chip->temp_map = &temp_map_gen2_v1;
> +
> + if (subtype == QPNP_TM_SUBTYPE_GEN1)
> + chip->data = &spmi_temp_alarm_data;
> + else if (subtype == QPNP_TM_SUBTYPE_GEN2 && dig_major >= 1)
> + chip->data = &spmi_temp_alarm_gen2_rev1_data;
> + else if (subtype == QPNP_TM_SUBTYPE_GEN2)
> + chip->data = &spmi_temp_alarm_gen2_data;
> else
> - chip->temp_map = &temp_map_gen1;
> + return -ENODEV;
>
> /*
> * Register the sensor before initializing the hardware to be able to
> @@ -450,7 +497,7 @@ static int qpnp_tm_probe(struct platform_device *pdev)
> * before the hardware initialization is completed.
> */
> chip->tz_dev = devm_thermal_of_zone_register(
> - &pdev->dev, 0, chip, &qpnp_tm_sensor_ops);
> + &pdev->dev, 0, chip, chip->data->ops);
> if (IS_ERR(chip->tz_dev))
> return dev_err_probe(&pdev->dev, PTR_ERR(chip->tz_dev),
> "failed to register sensor\n");
> --
> 2.34.1
>
--
With best wishes
Dmitry
