Re: [PATCH 4/5] thermal: qcom-spmi-temp-alarm: add support for GEN2 rev 2 PMIC peripherals

[Dmitry Baryshkov]

On Wed, 31 Jul 2024 at 01:44, Anjelique Melendez
<quic_amelende@xxxxxxxxxxx> wrote:
>
> >>
> >> +/* Configure TEMP_DAC registers based on DT thermal_zone trips */
> >> +static int qpnp_tm_gen2_rev2_update_trip_temps(struct qpnp_tm_chip *chip)
> >> +{
> >> +    struct thermal_trip trip = {0};
> >> +    int ret, ntrips, i;
> >> +
> >> +    ntrips = thermal_zone_get_num_trips(chip->tz_dev);
> >> +    /* Keep hardware defaults if no DT trips are defined. */
> >> +    if (ntrips <= 0)
> >> +            return 0;
> >> +
> >> +    for (i = 0; i < ntrips; i++) {
> >> +            ret = thermal_zone_get_trip(chip->tz_dev, i, &trip);
> >> +            if (ret < 0)
> >> +                    return ret;
> >> +
> >> +            ret = qpnp_tm_gen2_rev2_set_temp_thresh(chip, i, trip.temperature);
> >> +            if (ret < 0)
> >> +                    return ret;
> >> +    }
> >> +
> >> +    /* Verify that trips are strictly increasing. */
> >
> > There is no such requirement in the DT bindings. Please don't invent
> > artificial restrictions, especially if they are undocumented.
> >
>
> This is not an entirely new restirction. Currently the temp alarm driver
> has hardcoded temperature thresholds options which are "strictly increasing"
> (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thermal/qcom/qcom-spmi-temp-alarm.c?h=v6.11-rc1#n44).
> The threshold values are initially configured based on the stage 2 critical trip
> temperature.
> For newer PMICs, we have individual temperature registers for stage 1, 2, and 3,
> so we instead configure each threshold temperature as defined in DT. In general
> since stage 1 = warning, stage 2 = system should shut down, stage 3 = emergency shutdown,
> we would expect for temperature thresholds to increase for each stage
> (https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/thermal?h=v5.4.281&id=f1599f9e4cd6f1dd0cad202853fb830854f4e944).
>
> I agree that we are missing some documentation but since the trips are defined in the
> thermal_zone node what is the best way to mention this requirement? Will adding a
> few sentences to qcom,spmi-temp-alarm.yaml description be enough? Do we need
> to make changes to thermal_zone.yaml so that dt_binding_check catches this requirement?

In my opinion the driver needs to be fixed to compile the state after
looking at all trip points, but Daniel / Amit / Thara can have a more
qualified opinion.

>
> >> +    for (i = 1; i < STAGE_COUNT; i++) {
> >> +            if (chip->temp_dac_map[i] <= chip->temp_dac_map[i - 1]) {
> >> +                    dev_err(chip->dev, "Threshold %d=%ld <= threshold %d=%ld\n",
> >> +                            i, chip->temp_dac_map[i], i - 1,
> >> +                            chip->temp_dac_map[i - 1]);
> >> +                    return -EINVAL;
> >> +            }
> >> +    }
> >> +
> >> +    return 0;
> Thanks,
> Anjelique



-- 
With best wishes
Dmitry