| Message ID | 20241212-mbg-v2-support-v2-0-3249a4339b6e@quicinc.com |
|---|---|
| Headers | show |
| Series | Add support for MBG Thermal monitoring device | expand |
On Thu, 12 Dec 2024 21:41:20 +0530, Satya Priya Kakitapalli wrote: > Add PM8775 ADC5 GEN3 Channel info and bindings for the MBG Temp > alarm peripheral found on PM8775 pmic. > > Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@quicinc.com> > --- > .../bindings/thermal/qcom-spmi-mbg-tm.yaml | 86 ++++++++++++++++++++++ > .../iio/adc/qcom,spmi-adc5-gen3-pm8775.h | 41 +++++++++++ > 2 files changed, 127 insertions(+) > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: dtschema/dtc warnings/errors: In file included from Documentation/devicetree/bindings/thermal/qcom-spmi-mbg-tm.example.dts:25: ./scripts/dtc/include-prefixes/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8775.h:9:10: fatal error: dt-bindings/iio/adc/qcom,spmi-vadc.h: No such file or directory 9 | #include <dt-bindings/iio/adc/qcom,spmi-vadc.h> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ compilation terminated. make[2]: *** [scripts/Makefile.dtbs:131: Documentation/devicetree/bindings/thermal/qcom-spmi-mbg-tm.example.dtb] Error 1 make[2]: *** Waiting for unfinished jobs.... make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1506: dt_binding_check] Error 2 make: *** [Makefile:251: __sub-make] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241212-mbg-v2-support-v2-1-3249a4339b6e@quicinc.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On Thu, Dec 12, 2024 at 09:41:20PM +0530, Satya Priya Kakitapalli wrote: > + > +required: > + - compatible > + - reg > + - interrupts > + - io-channels > + - io-channel-names Binding looks ok, but this wasn't tested due to unneeded dependency. Please decouple from dependency, so automation can properly test it. > + > +unevaluatedProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/irq.h> > + #include <dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8775.h> > + > + pmic { > + #address-cells = <1>; > + #size-cells = <0>; > + > + pmm8654au_0_tz: temperature-sensor@d700 { Drop label. > + compatible = "qcom,spmi-pm8775-mbg-tm"; > + reg = <0xd700>; > + interrupts = <0x1 0xd7 0x0 IRQ_TYPE_EDGE_RISING>; > + io-channels = <&pm8775_1_adc PM8775_ADC5_GEN3_DIE_TEMP(1)>; > + io-channel-names = "thermal"; > + #thermal-sensor-cells = <0>; > + }; > + }; > + > + thermal-zones { > + pm8775-mbg0-thermal { Drop the nodes, not related. Best regards, Krzysztof
On Thu, Dec 12, 2024 at 09:41:19PM +0530, Satya Priya Kakitapalli wrote: > Add bindings, driver and DT for the Qualcomm's MBG thermal > monitoring device. > > Please note that this series is dependent on [1] which adds > ADC5-GEN3 support. Where is the changelog? What happened here? Why this is RFC? Limited review follows (as this is not yet ready). Best regards, Krzysztof
On 12/13/2024 2:08 PM, Krzysztof Kozlowski wrote: > On Thu, Dec 12, 2024 at 09:41:19PM +0530, Satya Priya Kakitapalli wrote: >> Add bindings, driver and DT for the Qualcomm's MBG thermal >> monitoring device. >> >> Please note that this series is dependent on [1] which adds >> ADC5-GEN3 support. > Where is the changelog? What happened here? Sorry for missing the change log, please find the summary below: Changes from v1 to v2: - Squash the ADC IIO channel bindings(header) and yaml into single patch - Add new patch [2/5] to add reference to qcom-spmi-mbg-tm.yaml in mfd/qcom,spmi-pmic.yaml bindings. - Use scoped_guard mutex in driver, remove the helper APIs and directly use regmap_set/clear_bits() APIs - Corrected the isr logic to notify the thermal only for lvl1 upper threshold violation. Earlier logic was not handling this fully. - Update the dt node names and remove the polling delay as it is 0 by default. - Link to v1: https://lore.kernel.org/linux-arm-msm/54c3fdd2-f701-4a06-bb3f-41f5a431687a@quicinc.com/ > Why this is RFC? On v1 I was suggested to mark this as RFC since the dependent changes are not yet accepted. > Limited review follows (as this is not yet ready). > > Best regards, > Krzysztof >
On 12.12.2024 5:11 PM, Satya Priya Kakitapalli wrote: > Add driver for the MBG thermal monitoring device. It monitors > the die temperature, and when there is a level 1 upper threshold > violation, it receives an interrupt over spmi. The driver reads > the fault status register and notifies thermal accordingly. > > Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@quicinc.com> > --- [...] > +static const struct mbg_map_table map_table[] = { Is this peripheral/pmic-specific? > + /* minT vtemp0 tc */ > + { -60000, 4337, 1967 }, > + { -40000, 4731, 1964 }, > + { -20000, 5124, 1957 }, > + { 0, 5515, 1949 }, > + { 20000, 5905, 1940 }, > + { 40000, 6293, 1930 }, > + { 60000, 6679, 1921 }, > + { 80000, 7064, 1910 }, > + { 100000, 7446, 1896 }, > + { 120000, 7825, 1878 }, > + { 140000, 8201, 1859 }, > +}; > + > +static int mbg_tm_get_temp(struct thermal_zone_device *tz, int *temp) > +{ > + struct mbg_tm_chip *chip = thermal_zone_device_priv(tz); > + int ret, milli_celsius; > + > + if (!temp) > + return -EINVAL; > + > + if (chip->last_thres_crossed) { > + pr_debug("last_temp: %d\n", chip->last_temp); Use dev_dbg for consistency with the other debug prints > + chip->last_thres_crossed = false; > + *temp = chip->last_temp; > + return 0; > + } > + > + ret = iio_read_channel_processed(chip->adc, &milli_celsius); > + if (ret < 0) { > + dev_err(chip->dev, "failed to read iio channel %d\n", ret); > + return ret; > + } > + > + *temp = milli_celsius; > + > + return 0; > +} > + > +static int temp_to_vtemp(int temp) > +{ > + > + int idx, vtemp, tc = 0, t0 = 0, vtemp0 = 0; > + > + for (idx = 0; idx < ARRAY_SIZE(map_table); idx++) > + if (temp >= map_table[idx].min_temp && > + temp < (map_table[idx].min_temp + 20000)) { Please align the two lines, tab width is 8 for kernel code > + tc = map_table[idx].tc; > + t0 = map_table[idx].min_temp; > + vtemp0 = map_table[idx].vtemp0; > + break; > + } > + > + /* > + * Formula to calculate vtemp(mV) from a given temp > + * vtemp = (temp - minT) * tc + vtemp0 > + * tc, t0 and vtemp0 values are mentioned in the map_table array. > + */ > + vtemp = ((temp - t0) * tc + vtemp0 * 100000) / 1000000; So you say vtemp = ... and the func is called temp_to_vtemp > + return abs(vtemp - MBG_TEMP_DEFAULT_TEMP_MV) / MBG_TEMP_STEP_MV; But you end up returning a scaled version of it.. Please clarify that in the code > +} > + > +static int mbg_tm_set_trip_temp(struct thermal_zone_device *tz, int low_temp, > + int temp) > +{ > + struct mbg_tm_chip *chip = thermal_zone_device_priv(tz); > + int ret = 0; > + > + guard(mutex)(&chip->lock); > + > + /* The HW has a limitation that the trip set must be above 25C */ > + if (temp > MBG_MIN_TRIP_TEMP && temp < MBG_MAX_SUPPORTED_TEMP) { > + regmap_set_bits(chip->map, > + chip->base + MBG_TEMP_MON2_MISC_CFG, MON2_UP_THRESH_EN); > + ret = regmap_write(chip->map, chip->base + MON2_LVL1_UP_THRESH, > + temp_to_vtemp(temp)); > + if (ret < 0) > + return ret; > + } else { > + dev_dbg(chip->dev, "Set trip b/w 25C and 160C\n"); > + regmap_clear_bits(chip->map, > + chip->base + MBG_TEMP_MON2_MISC_CFG, MON2_UP_THRESH_EN); > + } > + > + /* > + * Configure the last_temp one degree higher, to ensure the > + * violated temp is returned to thermal framework when it reads > + * temperature for the first time after the violation happens. > + * This is needed to account for the inaccuracy in the conversion > + * formula used which leads to the thermal framework setting back > + * the same thresholds in case the temperature it reads does not > + * show violation. > + */ > + chip->last_temp = temp + MBG_TEMP_CONSTANT; > + > + return ret; > +} > + > +static const struct thermal_zone_device_ops mbg_tm_ops = { > + .get_temp = mbg_tm_get_temp, > + .set_trips = mbg_tm_set_trip_temp, > +}; > + > +static irqreturn_t mbg_tm_isr(int irq, void *data) > +{ > + struct mbg_tm_chip *chip = data; > + int ret, val; > + > + scoped_guard(mutex, &chip->lock) { > + ret = regmap_read(chip->map, > + chip->base + MBG_TEMP_MON2_FAULT_STATUS, &val); > + if (ret < 0) > + return IRQ_HANDLED; > + } > + > + if ((val & MON_FAULT_STATUS_MASK) & MON_FAULT_STATUS_LVL1) { > + if ((val & MON_POLARITY_STATUS_MASK) & MON_POLARITY_STATUS_UPR) { Just checking the last argument to AND in both lines is enough, as they're both parts of the bitfield [...] > + ret = device_property_read_u32(chip->dev, "reg", &res); > + if (ret < 0) > + return ret; return dev_err_probe(dev, ret, "Couldn't read reg property"\n); > + > + chip->base = res; > + > + chip->irq = platform_get_irq(pdev, 0); > + if (chip->irq < 0) > + return chip->irq; Similarly here > + > + chip->adc = devm_iio_channel_get(&pdev->dev, "thermal"); > + if (IS_ERR(chip->adc)) > + return dev_err_probe(&pdev->dev, PTR_ERR(chip->adc), > + "failed to get adc channel\n"); > + > + chip->tz_dev = devm_thermal_of_zone_register(&pdev->dev, 0, > + chip, &mbg_tm_ops); > + if (IS_ERR(chip->tz_dev)) > + return dev_err_probe(&pdev->dev, PTR_ERR(chip->tz_dev), > + "failed to register sensor\n"); Please also make the error messages start with an uppercase letter > + > + return devm_request_threaded_irq(&pdev->dev, chip->irq, NULL, > + mbg_tm_isr, IRQF_ONESHOT, node->name, chip); > +} > + > +static const struct of_device_id mbg_tm_match_table[] = { > + { .compatible = "qcom,spmi-pm8775-mbg-tm" }, I don't think the 'spmi' bit belongs here Konrad
On 13/12/2024 10:20, Satya Priya Kakitapalli wrote: >> Why this is RFC? > > > On v1 I was suggested to mark this as RFC since the dependent changes > are not yet accepted. > Cover letter should explain this, IOW, explain your intentions. Best regards, Krzysztof
Add bindings, driver and DT for the Qualcomm's MBG thermal monitoring device. Please note that this series is dependent on [1] which adds ADC5-GEN3 support. [1] https://lore.kernel.org/linux-arm-msm/c4ca0a4c-e421-4cf6-b073-8e9019400f4c@quicinc.com/ Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@quicinc.com> --- Satya Priya Kakitapalli (5): dt-bindings: thermal: Add MBG thermal monitor support dt-bindings: mfd: qcom,spmi-pmic: Add MBG thermal monitor ref thermal: qcom: Add support for MBG thermal monitoring arm64: dts: qcom: sa8775p-pmics: Add vadc support on SA8775P arm64: dts: qcom: sa8775p-pmics: Add support for MBG TM .../devicetree/bindings/mfd/qcom,spmi-pmic.yaml | 4 + .../bindings/thermal/qcom-spmi-mbg-tm.yaml | 86 ++++++++ arch/arm64/boot/dts/qcom/sa8775p-pmics.dtsi | 210 ++++++++++++++++++ drivers/thermal/qcom/Kconfig | 11 + drivers/thermal/qcom/Makefile | 1 + drivers/thermal/qcom/qcom-spmi-mbg-tm.c | 245 +++++++++++++++++++++ .../iio/adc/qcom,spmi-adc5-gen3-pm8775.h | 41 ++++ 7 files changed, 598 insertions(+) --- base-commit: b4ba0e91c6f0de7bebe1b9d209bebe9bd56daa42 change-id: 20241209-mbg-v2-support-63df41a40d53 prerequisite-message-id: <c4ca0a4c-e421-4cf6-b073-8e9019400f4c@quicinc.com> prerequisite-patch-id: b4aee2e3887f478bdb7ef86519d4e2233a7e8600 prerequisite-patch-id: f1a62cf8dff80669a7b2cac238a033e92853ad38 prerequisite-patch-id: bc6d5a0b2448ed92c77ba0b13fb211ee605b58c7 prerequisite-patch-id: 80bfefa75a9ee0440cc6ad195e424e71bf3c91e9 Best regards,