Skip to contentSkip to conversation

thermal/drivers: airoha: Add support for AN7583

PATCH Superseded Christian Marangi 4 participants 18 messages May 20 – Jun 27

Subsystems (2)
Device tree bindings Thermal

v5 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Christian Marangi

author
#0 lore raw

v5 cover thermal/drivers: airoha: Add support for AN7583

This small series implement all the required changes to support Airoha AN7583 Thermal Sensor.

The SoC dropped the Monitor subsystem and only provide temperature reading. Some generalization was required to at least save the common code with also the help of reg field API.

Changes v5:
- Fix alpabetical order in Documentation patch
- Address suggested change by AI Bot
Changes v4:
- Rebase on top of linux-next
Changes v3:
- Property use chip-scu for thermal sensor Changes v2:
- Update DT schema patch to implement dedicated schema

Christian Marangi (7):
thermal/drivers: airoha: fix copy paste error on clamp_t low temp thermal/drivers: airoha: fix copy paste error for sen internal thermal/drivers: airoha: Convert to regmap API thermal/drivers: airoha: Generalize probe function thermal/drivers: airoha: Generalize get_thermal_ADC and set_mux
function
dt-bindings: arm: airoha: Add the chip-scu node for AN7583 SoC thermal/drivers: airoha: Add support for AN7583 Thermal Sensor

 .../bindings/arm/airoha,en7581-chip-scu.yaml  |  14 +
 drivers/thermal/airoha_thermal.c              | 380 +++++++++++++++---
 2 files changed, 327 insertions(+), 67 deletions(-)
Signature · 1 line
2.53.0
author
#1 lore raw

PATCH v5 1/7 thermal/drivers: airoha: fix copy paste error on clamp_t low temp

In airoha_thermal_set_trips, there is a copy paste error on clamping the value for the low trip temp point. Fix it to the correct value and actually clamp for the low variable.

Fixes 42de37f40e1b ("thermal/drivers: Add support for Airoha EN7581 thermal sensor")
Signed-off-by Christian Marangi <[email protected]>
---
 drivers/thermal/airoha_thermal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/thermal/airoha_thermal.c +1 -1
index b9fd6bfc88e5..439aa011b75c 100644
@@ -273,7 +273,7 @@ static int airoha_thermal_set_trips(struct thermal_zone_device *tz, int low,
 
 	if (low != -INT_MAX) {
 		/* Validate low and clamp it to a supported value */
-		low = clamp_t(int, high, RAW_TO_TEMP(priv, 0),
+		low = clamp_t(int, low, RAW_TO_TEMP(priv, 0),
 			      RAW_TO_TEMP(priv, FIELD_MAX(EN7581_DOUT_TADC_MASK)));
 
 		/* We offset the low temp of 1°C to trigger correct event */
Signature · 1 line
2.53.0
AI review
#2 lore raw

Re: [PATCH v5 1/7] thermal/drivers: airoha: fix copy paste error on clamp_t low temp

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] The driver fails to clear old thresholds or disable the thermal monitor when trip points are disabled, potentially causing an interrupt storm.
- [High] The interrupt handler uses a switch statement on a bitmask, silently dropping events if both hot and cold interrupts are asserted simultaneously.
- [High] The thermal interrupt is requested before the thermal zone is registered, which can lead to a NULL pointer dereference. --

commit e9247122d90d0d1f57f8075a6a171215a771b2d6
Author: Christian Marangi <[email protected]>

thermal/drivers: airoha: fix copy paste error on clamp_t low temp

In airoha_thermal_set_trips, there is a copy paste error on clamping the value for the low trip temp point. Fix it to the correct value and actually clamp for the low variable.

drivers/thermal/airoha_thermal.c static int airoha_thermal_set_trips(struct thermal_zone_device *tz, int low, · line 279

Quoted 13 lines
drivers/thermal/airoha_thermal.c +1 -1
index b9fd6bfc88e5e..439aa011b75c7 100644
@@ -273,7 +273,7 @@ static int airoha_thermal_set_trips(struct thermal_zone_device *tz, int low,
 
 	if (low != -INT_MAX) {
 		/* Validate low and clamp it to a supported value */
-		low = clamp_t(int, high, RAW_TO_TEMP(priv, 0),
+		low = clamp_t(int, low, RAW_TO_TEMP(priv, 0),
 			      RAW_TO_TEMP(priv, FIELD_MAX(EN7581_DOUT_TADC_MASK)));
 
 		/* We offset the low temp of 1°C to trigger correct event */

[Severity: High]
This is a pre-existing issue, but does this function properly handle the case where trip points are disabled?

If a trip point boundary is disabled (e.g., passing INT_MAX for the high trip), its corresponding threshold register (EN7581_TEMPOFFSETH) is not updated. If both boundaries are disabled, enable_monitor is set to false, and the monitor is never explicitly disabled via EN7581_TEMPMONCTL0.

This leaves stale thresholds active in hardware. If the temperature crosses these stale thresholds, the hardware will continue asserting interrupts. Because the trip is disabled in software, the thermal core will not take action to adjust the boundaries, which could lead to an unhandled interrupt storm that can render the system unusable.

[Severity: High]
This is another pre-existing issue, but could the interrupt handler silently drop events if both hot and cold interrupts assert simultaneously?

Looking at airoha_thermal_irq():

drivers/thermal/airoha_thermal.c:airoha_thermal_irq() {
	...
	status = readl(priv->base + EN7581_TEMPMONINTSTS);
	switch (status & (EN7581_HOFSINTSTS0 | EN7581_LOFSINTSTS0)) {
	case EN7581_HOFSINTSTS0:
		event = THERMAL_TRIP_VIOLATED;
		update = true;
		break;
	case EN7581_LOFSINTSTS0:
		event = THERMAL_EVENT_UNSPECIFIED;
		update = true;
		break;
	default:
		/* Should be impossible as we enable only these Interrupt */
		break;
	}

	/* Reset Interrupt */
	writel(status, priv->base + EN7581_TEMPMONINTSTS);

	if (update)
		thermal_zone_device_update(priv->tz, event);
	...
}

If the hardware asserts both the hot and cold interrupt bits simultaneously (e.g., due to a delayed IRQ handler combined with temperature fluctuations, or a hardware glitch), the bitmask value matches neither case.

It will fall through to default, leaving update = false, and clear both interrupts in hardware without notifying the thermal core.

[Severity: High]
This is also a pre-existing issue, but could requesting the interrupt before registering the thermal zone lead to a NULL pointer dereference?

Looking at airoha_thermal_probe():

drivers/thermal/airoha_thermal.c:airoha_thermal_probe() {
	...
	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
					airoha_thermal_irq, IRQF_ONESHOT,
					pdev->name, priv);
	...
	priv->tz = devm_thermal_of_zone_register(dev, 0, priv, &thdev_ops);
	...
}

The driver does not explicitly disable or clear hardware interrupts prior to requesting the IRQ.

If a bootloader or previous kernel left the thermal monitor running and interrupts enabled, an interrupt can fire immediately upon request. The handler airoha_thermal_irq() will execute and call thermal_zone_device_update(priv->tz, event).

Because priv->tz is not yet populated, this would cause a NULL pointer dereference during initialization.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
author
#4 lore raw

PATCH v5 2/7 thermal/drivers: airoha: fix copy paste error for sen internal

In reply to Christian Marangi

In airoha_thermal_setup_monitor there is a copy paste error on configuring the internval for temp monitor. Fix the error and use the correct mask for the sen interval for the EN7581_TEMPMONCTL2 register.

Fixes 42de37f40e1b ("thermal/drivers: Add support for Airoha EN7581 thermal sensor")
Signed-off-by Christian Marangi <[email protected]>
---
 drivers/thermal/airoha_thermal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/thermal/airoha_thermal.c +1 -1
index 439aa011b75c..829a7327fc40 100644
@@ -403,7 +403,7 @@ static void airoha_thermal_setup_monitor(struct airoha_thermal_priv *priv)
 	 * sen interval is 379 * 52.715us = 19.97ms
 	 */
 	writel(FIELD_PREP(EN7581_FILT_INTERVAL, 1) |
-	       FIELD_PREP(EN7581_FILT_INTERVAL, 379),
+	       FIELD_PREP(EN7581_SEN_INTERVAL, 379),
 	       priv->base + EN7581_TEMPMONCTL2);
 
 	/* AHB poll is set to 146 * 68.64 = 10.02us */
Signature · 1 line
2.53.0
author
#6 lore raw

PATCH v5 3/7 thermal/drivers: airoha: Convert to regmap API

In reply to Christian Marangi

In preparation for support of Airoha AN7583, convert the driver to regmap API. This is needed as Airoha AN7583 will be based on syscon regmap.

Signed-off-by Christian Marangi <[email protected]>
---
 drivers/thermal/airoha_thermal.c | 77 +++++++++++++++++++-------------
 1 file changed, 45 insertions(+), 32 deletions(-)
drivers/thermal/airoha_thermal.c +45 -32
index 829a7327fc40..b63893a8997a 100644
@@ -194,7 +194,7 @@
 #define AIROHA_MAX_SAMPLES			6
 
 struct airoha_thermal_priv {
-	void __iomem *base;
+	struct regmap *map;
 	struct regmap *chip_scu;
 	struct resource scu_adc_res;
 

@@ -265,8 +265,8 @@ static int airoha_thermal_set_trips(struct thermal_zone_device *tz, int low,
 			       RAW_TO_TEMP(priv, FIELD_MAX(EN7581_DOUT_TADC_MASK)));
 
 		/* We offset the high temp of 1°C to trigger correct event */
-		writel(TEMP_TO_RAW(priv, high) >> 4,
-		       priv->base + EN7581_TEMPOFFSETH);
+		regmap_write(priv->map, EN7581_TEMPOFFSETH,
+			     TEMP_TO_RAW(priv, high) >> 4);
 
 		enable_monitor = true;
 	}

@@ -277,15 +277,15 @@ static int airoha_thermal_set_trips(struct thermal_zone_device *tz, int low,
 			      RAW_TO_TEMP(priv, FIELD_MAX(EN7581_DOUT_TADC_MASK)));
 
 		/* We offset the low temp of 1°C to trigger correct event */
-		writel(TEMP_TO_RAW(priv, low) >> 4,
-		       priv->base + EN7581_TEMPOFFSETL);
+		regmap_write(priv->map, EN7581_TEMPOFFSETL,
+			     TEMP_TO_RAW(priv, low) >> 4);
 
 		enable_monitor = true;
 	}
 
 	/* Enable sensor 0 monitor after trip are set */
 	if (enable_monitor)
-		writel(EN7581_SENSE0_EN, priv->base + EN7581_TEMPMONCTL0);
+		regmap_write(priv->map, EN7581_TEMPMONCTL0, EN7581_SENSE0_EN);
 
 	return 0;
 }

@@ -300,9 +300,9 @@ static irqreturn_t airoha_thermal_irq(int irq, void *data)
 	struct airoha_thermal_priv *priv = data;
 	enum thermal_notify_event event;
 	bool update = false;
-	u32 status;
+	u32 status = 0;
 
-	status = readl(priv->base + EN7581_TEMPMONINTSTS);
+	regmap_read(priv->map, EN7581_TEMPMONINTSTS, &status);
 	switch (status & (EN7581_HOFSINTSTS0 | EN7581_LOFSINTSTS0)) {
 	case EN7581_HOFSINTSTS0:
 		event = THERMAL_TRIP_VIOLATED;

@@ -318,7 +318,7 @@ static irqreturn_t airoha_thermal_irq(int irq, void *data)
 	}
 
 	/* Reset Interrupt */
-	writel(status, priv->base + EN7581_TEMPMONINTSTS);
+	regmap_write(priv->map, EN7581_TEMPMONINTSTS, status);
 
 	if (update)
 		thermal_zone_device_update(priv->tz, event);

@@ -329,18 +329,19 @@ static irqreturn_t airoha_thermal_irq(int irq, void *data)
 static void airoha_thermal_setup_adc_val(struct device *dev,
 					 struct airoha_thermal_priv *priv)
 {
-	u32 efuse_calib_info, cpu_sensor;
+	u32 efuse_calib_info = 0;
+	u32 cpu_sensor = 0;
 
 	/* Setup thermal sensor to ADC mode and setup the mux to DIODE1 */
 	airoha_init_thermal_ADC_mode(priv);
 	/* sleep 10 ms for ADC to enable */
 	usleep_range(10 * USEC_PER_MSEC, 11 * USEC_PER_MSEC);
 
-	efuse_calib_info = readl(priv->base + EN7581_EFUSE_TEMP_OFFSET_REG);
+	regmap_read(priv->map, EN7581_EFUSE_TEMP_OFFSET_REG, &efuse_calib_info);
 	if (efuse_calib_info) {
 		priv->default_offset = FIELD_GET(EN7581_EFUSE_TEMP_OFFSET, efuse_calib_info);
 		/* Different slope are applied if the sensor is used for CPU or for package */
-		cpu_sensor = readl(priv->base + EN7581_EFUSE_TEMP_CPU_SENSOR_REG);
+		regmap_read(priv->map, EN7581_EFUSE_TEMP_CPU_SENSOR_REG, &cpu_sensor);
 		if (cpu_sensor) {
 			priv->default_slope = EN7581_SLOPE_X100_DIO_DEFAULT;
 			priv->init_temp = EN7581_INIT_TEMP_FTK_X10;

@@ -359,8 +360,8 @@ static void airoha_thermal_setup_adc_val(struct device *dev,
 static void airoha_thermal_setup_monitor(struct airoha_thermal_priv *priv)
 {
 	/* Set measure mode */
-	writel(FIELD_PREP(EN7581_MSRCTL0, EN7581_MSRCTL_6SAMPLE_MAX_MIX_AVG4),
-	       priv->base + EN7581_TEMPMSRCTL0);
+	regmap_write(priv->map, EN7581_TEMPMSRCTL0,
+		     FIELD_PREP(EN7581_MSRCTL0, EN7581_MSRCTL_6SAMPLE_MAX_MIX_AVG4));
 
 	/*
 	 * Configure ADC valid reading addr

@@ -375,15 +376,15 @@ static void airoha_thermal_setup_monitor(struct airoha_thermal_priv *priv)
 	 * We set valid instead of volt as we don't enable valid/volt
 	 * split reading and AHB read valid addr in such case.
 	 */
-	writel(priv->scu_adc_res.start + EN7581_DOUT_TADC,
-	       priv->base + EN7581_TEMPADCVALIDADDR);
+	regmap_write(priv->map, EN7581_TEMPADCVALIDADDR,
+		     priv->scu_adc_res.start + EN7581_DOUT_TADC);
 
 	/*
 	 * Configure valid bit on a fake value of bit 16. The ADC outputs
 	 * max of 2 bytes for voltage.
 	 */
-	writel(FIELD_PREP(EN7581_ADV_RD_VALID_POS, 16),
-	       priv->base + EN7581_TEMPADCVALIDMASK);
+	regmap_write(priv->map, EN7581_TEMPADCVALIDMASK,
+		     FIELD_PREP(EN7581_ADV_RD_VALID_POS, 16));
 
 	/*
 	 * AHB supports max 12 bytes for ADC voltage. Shift the read

@@ -391,40 +392,52 @@ static void airoha_thermal_setup_monitor(struct airoha_thermal_priv *priv)
 	 * in the order of half a °C and is acceptable in the context
 	 * of triggering interrupt in critical condition.
 	 */
-	writel(FIELD_PREP(EN7581_ADC_VOLTAGE_SHIFT, 4),
-	       priv->base + EN7581_TEMPADCVOLTAGESHIFT);
+	regmap_write(priv->map, EN7581_TEMPADCVOLTAGESHIFT,
+		     FIELD_PREP(EN7581_ADC_VOLTAGE_SHIFT, 4));
 
 	/* BUS clock is 300MHz counting unit is 3 * 68.64 * 256 = 52.715us */
-	writel(FIELD_PREP(EN7581_PERIOD_UNIT, 3),
-	       priv->base + EN7581_TEMPMONCTL1);
+	regmap_write(priv->map, EN7581_TEMPMONCTL1,
+		     FIELD_PREP(EN7581_PERIOD_UNIT, 3));
 
 	/*
 	 * filt interval is 1 * 52.715us = 52.715us,
 	 * sen interval is 379 * 52.715us = 19.97ms
 	 */
-	writel(FIELD_PREP(EN7581_FILT_INTERVAL, 1) |
-	       FIELD_PREP(EN7581_SEN_INTERVAL, 379),
-	       priv->base + EN7581_TEMPMONCTL2);
+	regmap_write(priv->map, EN7581_TEMPMONCTL2,
+		     FIELD_PREP(EN7581_FILT_INTERVAL, 1) |
+		     FIELD_PREP(EN7581_SEN_INTERVAL, 379));
 
 	/* AHB poll is set to 146 * 68.64 = 10.02us */
-	writel(FIELD_PREP(EN7581_ADC_POLL_INTVL, 146),
-	       priv->base + EN7581_TEMPAHBPOLL);
+	regmap_write(priv->map, EN7581_TEMPAHBPOLL,
+		     FIELD_PREP(EN7581_ADC_POLL_INTVL, 146));
 }
 
+static const struct regmap_config airoha_thermal_regmap_config = {
+	.reg_bits		= 32,
+	.reg_stride		= 4,
+	.val_bits		= 32,
+};
+
 static int airoha_thermal_probe(struct platform_device *pdev)
 {
 	struct airoha_thermal_priv *priv;
 	struct device_node *chip_scu_np;
 	struct device *dev = &pdev->dev;
+	void __iomem *base;
 	int irq, ret;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
-	priv->base = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(priv->base))
-		return PTR_ERR(priv->base);
+	base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	priv->map = devm_regmap_init_mmio(dev, base,
+					  &airoha_thermal_regmap_config);
+	if (IS_ERR(priv->map))
+		return PTR_ERR(priv->map);
 
 	chip_scu_np = of_parse_phandle(dev->of_node, "airoha,chip-scu", 0);
 	if (!chip_scu_np)

@@ -462,8 +475,8 @@ static int airoha_thermal_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, priv);
 
 	/* Enable LOW and HIGH interrupt */
-	writel(EN7581_HOFSINTEN0 | EN7581_LOFSINTEN0,
-	       priv->base + EN7581_TEMPMONINT);
+	regmap_write(priv->map, EN7581_TEMPMONINT,
+		     EN7581_HOFSINTEN0 | EN7581_LOFSINTEN0);
 
 	return 0;
 }
Signature · 1 line
2.53.0
AI review
#7 lore raw

Re: [PATCH v5 3/7] thermal/drivers: airoha: Convert to regmap API

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Simultaneous temperature trip interrupts are dropped instead of being handled.
- [High] The IRQ is requested before the thermal zone is registered, allowing a potential NULL pointer dereference in the IRQ handler.
- [High] Non-atomic read-modify-restore sequence on a shared syscon regmap without external locking.
--

commit 952c01597265eea8ee25e21cc2c92bef3e5ca97e
Author: Christian Marangi <[email protected]>

thermal/drivers: airoha: Convert to regmap API

In preparation for support of Airoha AN7583, convert the driver to regmap API. This is needed as Airoha AN7583 will be based on syscon regmap.

Quoted 4 lines
drivers/thermal/airoha_thermal.c +0 -0
index 829a7327fc403..b63893a8997a3 100644

[ ... ]

drivers/thermal/airoha_thermal.c static irqreturn_t airoha_thermal_irq(int irq, void *data) · line 308

Quoted 12 lines
drivers/thermal/airoha_thermal.c +2 -2
@@ -300,9 +300,9 @@ static irqreturn_t airoha_thermal_irq(int irq, void *data)
 	struct airoha_thermal_priv *priv = data;
 	enum thermal_notify_event event;
 	bool update = false;
-	u32 status;
+	u32 status = 0;
 
-	status = readl(priv->base + EN7581_TEMPMONINTSTS);
+	regmap_read(priv->map, EN7581_TEMPMONINTSTS, &status);
 	switch (status & (EN7581_HOFSINTSTS0 | EN7581_LOFSINTSTS0)) {
 	case EN7581_HOFSINTSTS0:
 		event = THERMAL_TRIP_VIOLATED;

[Severity: High]
This is a pre-existing issue, but what happens if both high and low temperature interrupt status bits are set simultaneously?

If both EN7581_HOFSINTSTS0 and EN7581_LOFSINTSTS0 are set, the bitwise AND will yield a combination of both bits. Since there are only case statements for the individual bits, the code will fall through to the default case.

The status will be written back to clear the interrupts, but the events will be silently dropped without notifying the thermal core.

Would it be safer to replace this switch statement with independent if checks for each interrupt flag?

drivers/thermal/airoha_thermal.c static irqreturn_t airoha_thermal_irq(int irq, void *data) · line 336

Quoted 10 lines
drivers/thermal/airoha_thermal.c +2 -1
@@ -329,18 +329,19 @@ static irqreturn_t airoha_thermal_irq(int irq, void *data)
 static void airoha_thermal_setup_adc_val(struct device *dev,
 					 struct airoha_thermal_priv *priv)
 {
-	u32 efuse_calib_info, cpu_sensor;
+	u32 efuse_calib_info = 0;
+	u32 cpu_sensor = 0;
 
 	/* Setup thermal sensor to ADC mode and setup the mux to DIODE1 */
 	airoha_init_thermal_ADC_mode(priv);

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for the chip_scu regmap sequence to race with other drivers?

Looking at airoha_init_thermal_ADC_mode(), it performs a read-modify-restore sequence on a shared syscon regmap:

static void airoha_init_thermal_ADC_mode(struct airoha_thermal_priv *priv)

{
    ...
    regmap_read(priv->chip_scu, EN7581_PLLRG_PROTECT, &pllrg);
    regmap_write(priv->chip_scu, EN7581_PLLRG_PROTECT,
                 EN7581_SCU_THERMAL_PROTECT_KEY);
    ...
    regmap_write(priv->chip_scu, EN7581_PLLRG_PROTECT, pllrg);
}

Because the internal regmap lock is only held during individual register accesses, could another driver (like a clock or pinctrl driver) accessing the SCU concurrently cause the protect key to be incorrectly overwritten or prematurely removed?

[ ... ]

drivers/thermal/airoha_thermal.c static int airoha_thermal_probe(struct platform_device *pdev) · line 482

Quoted 11 lines
drivers/thermal/airoha_thermal.c +2 -2
@@ -462,8 +475,8 @@ static int airoha_thermal_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, priv);
 
 	/* Enable LOW and HIGH interrupt */
-	writel(EN7581_HOFSINTEN0 | EN7581_LOFSINTEN0,
-	       priv->base + EN7581_TEMPMONINT);
+	regmap_write(priv->map, EN7581_TEMPMONINT,
+		     EN7581_HOFSINTEN0 | EN7581_LOFSINTEN0);
 
 	return 0;
 }

[Severity: High]
This is a pre-existing issue, but could we encounter a NULL pointer dereference if an interrupt fires before the thermal zone is registered?

Looking earlier in airoha_thermal_probe():

    ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
                                    airoha_thermal_irq, IRQF_ONESHOT,
                                    pdev->name, priv);

    ...

    priv->tz = devm_thermal_of_zone_register(dev, 0, priv, &thdev_ops);

If an interrupt is already pending (for example, left active by the bootloader) when the IRQ is requested, the handler will run immediately.

If an event is detected, airoha_thermal_irq() will call thermal_zone_device_update(priv->tz, event). Since priv->tz is still NULL at that point, it will crash.

Should the IRQ be requested after the thermal zone registration is complete?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
author
#8 lore raw

PATCH v5 4/7 thermal/drivers: airoha: Generalize probe function

In reply to Christian Marangi

In preparation for support of Airoha AN7583, generalize the probe function to address for the 2 SoC differece.

Implement a match_data struct where it's possible to define a more specific probe and post_probe function and specific thermal ops and pllrg protect value.

Signed-off-by Christian Marangi <[email protected]>
---
 drivers/thermal/airoha_thermal.c | 102 +++++++++++++++++++++++--------
 1 file changed, 75 insertions(+), 27 deletions(-)
drivers/thermal/airoha_thermal.c +75 -27
index b63893a8997a..ebb47ae5f2ce 100644
@@ -198,12 +198,23 @@ struct airoha_thermal_priv {
 	struct regmap *chip_scu;
 	struct resource scu_adc_res;
 
+	u32 pllrg_protect;
+
 	struct thermal_zone_device *tz;
 	int init_temp;
 	int default_slope;
 	int default_offset;
 };
 
+struct airoha_thermal_soc_data {
+	u32 pllrg_protect;
+
+	const struct thermal_zone_device_ops *thdev_ops;
+	int (*probe)(struct platform_device *pdev,
+		     struct airoha_thermal_priv *priv);
+	int (*post_probe)(struct platform_device *pdev);
+};
+
 static int airoha_get_thermal_ADC(struct airoha_thermal_priv *priv)
 {
 	u32 val;

@@ -220,7 +231,8 @@ static void airoha_init_thermal_ADC_mode(struct airoha_thermal_priv *priv)
 	regmap_read(priv->chip_scu, EN7581_PLLRG_PROTECT, &pllrg);
 
 	/* Give access to thermal regs */
-	regmap_write(priv->chip_scu, EN7581_PLLRG_PROTECT, EN7581_SCU_THERMAL_PROTECT_KEY);
+	regmap_write(priv->chip_scu, EN7581_PLLRG_PROTECT,
+		     priv->pllrg_protect);
 	adc_mux = FIELD_PREP(EN7581_MUX_TADC, EN7581_SCU_THERMAL_MUX_DIODE1);
 	regmap_write(priv->chip_scu, EN7581_PWD_TADC, adc_mux);
 

@@ -228,7 +240,7 @@ static void airoha_init_thermal_ADC_mode(struct airoha_thermal_priv *priv)
 	regmap_write(priv->chip_scu, EN7581_PLLRG_PROTECT, pllrg);
 }
 
-static int airoha_thermal_get_temp(struct thermal_zone_device *tz, int *temp)
+static int en7581_thermal_get_temp(struct thermal_zone_device *tz, int *temp)
 {
 	struct airoha_thermal_priv *priv = thermal_zone_device_priv(tz);
 	int min_value, max_value, avg_value, value;

@@ -253,7 +265,7 @@ static int airoha_thermal_get_temp(struct thermal_zone_device *tz, int *temp)
 	return 0;
 }
 
-static int airoha_thermal_set_trips(struct thermal_zone_device *tz, int low,
+static int en7581_thermal_set_trips(struct thermal_zone_device *tz, int low,
 				    int high)
 {
 	struct airoha_thermal_priv *priv = thermal_zone_device_priv(tz);

@@ -290,12 +302,12 @@ static int airoha_thermal_set_trips(struct thermal_zone_device *tz, int low,
 	return 0;
 }
 
-static const struct thermal_zone_device_ops thdev_ops = {
-	.get_temp = airoha_thermal_get_temp,
-	.set_trips = airoha_thermal_set_trips,
+static const struct thermal_zone_device_ops en7581_thdev_ops = {
+	.get_temp = en7581_thermal_get_temp,
+	.set_trips = en7581_thermal_set_trips,
 };
 
-static irqreturn_t airoha_thermal_irq(int irq, void *data)
+static irqreturn_t en7581_thermal_irq(int irq, void *data)
 {
 	struct airoha_thermal_priv *priv = data;
 	enum thermal_notify_event event;

@@ -326,7 +338,7 @@ static irqreturn_t airoha_thermal_irq(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static void airoha_thermal_setup_adc_val(struct device *dev,
+static void en7581_thermal_setup_adc_val(struct device *dev,
 					 struct airoha_thermal_priv *priv)
 {
 	u32 efuse_calib_info = 0;

@@ -357,7 +369,7 @@ static void airoha_thermal_setup_adc_val(struct device *dev,
 	}
 }
 
-static void airoha_thermal_setup_monitor(struct airoha_thermal_priv *priv)
+static void en7581_thermal_setup_monitor(struct airoha_thermal_priv *priv)
 {
 	/* Set measure mode */
 	regmap_write(priv->map, EN7581_TEMPMSRCTL0,

@@ -412,30 +424,26 @@ static void airoha_thermal_setup_monitor(struct airoha_thermal_priv *priv)
 		     FIELD_PREP(EN7581_ADC_POLL_INTVL, 146));
 }
 
-static const struct regmap_config airoha_thermal_regmap_config = {
+static const struct regmap_config en7581_thermal_regmap_config = {
 	.reg_bits		= 32,
 	.reg_stride		= 4,
 	.val_bits		= 32,
 };
 
-static int airoha_thermal_probe(struct platform_device *pdev)
+static int en7581_thermal_probe(struct platform_device *pdev,
+				struct airoha_thermal_priv *priv)
 {
-	struct airoha_thermal_priv *priv;
 	struct device_node *chip_scu_np;
 	struct device *dev = &pdev->dev;
 	void __iomem *base;
 	int irq, ret;
 
-	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
-	if (!priv)
-		return -ENOMEM;
-
 	base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(base))
 		return PTR_ERR(base);
 
 	priv->map = devm_regmap_init_mmio(dev, base,
-					  &airoha_thermal_regmap_config);
+					  &en7581_thermal_regmap_config);
 	if (IS_ERR(priv->map))
 		return PTR_ERR(priv->map);
 

@@ -455,18 +463,55 @@ static int airoha_thermal_probe(struct platform_device *pdev)
 		return irq;
 
 	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
-					airoha_thermal_irq, IRQF_ONESHOT,
+					en7581_thermal_irq, IRQF_ONESHOT,
 					pdev->name, priv);
 	if (ret) {
 		dev_err(dev, "Can't get interrupt working.\n");
 		return ret;
 	}
 
-	airoha_thermal_setup_monitor(priv);
-	airoha_thermal_setup_adc_val(dev, priv);
+	en7581_thermal_setup_monitor(priv);
+	en7581_thermal_setup_adc_val(dev, priv);
+
+	return 0;
+}
+
+static int en7581_thermal_post_probe(struct platform_device *pdev)
+{
+	struct airoha_thermal_priv *priv = platform_get_drvdata(pdev);
+
+	/* Enable LOW and HIGH interrupt (if supported) */
+	regmap_write(priv->map, EN7581_TEMPMONINT,
+		     EN7581_HOFSINTEN0 | EN7581_LOFSINTEN0);
+
+	return 0;
+}
+
+static int airoha_thermal_probe(struct platform_device *pdev)
+{
+	const struct airoha_thermal_soc_data *soc_data;
+	struct airoha_thermal_priv *priv;
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	soc_data = device_get_match_data(dev);
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->pllrg_protect = soc_data->pllrg_protect;
+
+	if (!soc_data->probe)
+		return -EINVAL;
+
+	ret = soc_data->probe(pdev, priv);
+	if (ret)
+		return ret;
 
 	/* register of thermal sensor and get info from DT */
-	priv->tz = devm_thermal_of_zone_register(dev, 0, priv, &thdev_ops);
+	priv->tz = devm_thermal_of_zone_register(dev, 0, priv,
+						 soc_data->thdev_ops);
 	if (IS_ERR(priv->tz)) {
 		dev_err(dev, "register thermal zone sensor failed\n");
 		return PTR_ERR(priv->tz);

@@ -474,15 +519,18 @@ static int airoha_thermal_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, priv);
 
-	/* Enable LOW and HIGH interrupt */
-	regmap_write(priv->map, EN7581_TEMPMONINT,
-		     EN7581_HOFSINTEN0 | EN7581_LOFSINTEN0);
-
-	return 0;
+	return soc_data->post_probe ? soc_data->post_probe(pdev) : 0;
 }
 
+static const struct airoha_thermal_soc_data en7581_data = {
+	.pllrg_protect = EN7581_SCU_THERMAL_PROTECT_KEY,
+	.thdev_ops = &en7581_thdev_ops,
+	.probe = &en7581_thermal_probe,
+	.post_probe = &en7581_thermal_post_probe,
+};
+
 static const struct of_device_id airoha_thermal_match[] = {
-	{ .compatible = "airoha,en7581-thermal" },
+	{ .compatible = "airoha,en7581-thermal", .data = &en7581_data },
 	{},
 };
 MODULE_DEVICE_TABLE(of, airoha_thermal_match);
Signature · 1 line
2.53.0
AI review
#9 lore raw

Re: [PATCH v5 4/7] thermal/drivers: airoha: Generalize probe function

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Unchecked return value from device_get_match_data() leads to a NULL pointer dereference.

Pre-existing issues:
- [High] IRQ registered before thermal zone initialization is complete, causing a race condition and devm teardown use-after-free. --

commit f9520e831723d0643c683a7acaaf6600c5487d0a
Author: Christian Marangi <[email protected]>

thermal/drivers: airoha: Generalize probe function

This commit generalizes the Airoha thermal probe function to prepare for supporting the Airoha AN7583 SoC. It introduces a match_data struct to define SoC-specific probe and post_probe functions, thermal ops, and PLLRG protect values.

Quoted 4 lines
drivers/thermal/airoha_thermal.c +0 -0
index b63893a8997a3..ebb47ae5f2cee 100644

[ ... ]

drivers/thermal/airoha_thermal.c static int airoha_thermal_probe(struct platform_device *pdev) · line 467

Quoted 7 lines
drivers/thermal/airoha_thermal.c +1 -1
@@ -455,18 +463,55 @@ static int airoha_thermal_probe(struct platform_device *pdev)
 		return irq;
 
 	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
-					airoha_thermal_irq, IRQF_ONESHOT,
+					en7581_thermal_irq, IRQF_ONESHOT,
 					pdev->name, priv);

[Severity: High]
This is a pre-existing issue, but does requesting the interrupt here before the thermal zone is registered create a race condition?

If an interrupt fires early (e.g., from a pending bootloader interrupt), en7581_thermal_irq() will run and call thermal_zone_device_update() before priv->tz is initialized, leading to a NULL pointer dereference.

Additionally, because devres teardown executes in reverse allocation order, the thermal zone is destroyed before the IRQ is freed during module removal. Could this lead to a use-after-free if an interrupt fires during teardown and accesses the freed priv->tz?

[ ... ]

Quoted 4 lines
drivers/thermal/airoha_thermal.c +4 -0
+static int airoha_thermal_probe(struct platform_device *pdev)
+{
+	const struct airoha_thermal_soc_data *soc_data;
+	struct airoha_thermal_priv *priv;

drivers/thermal/airoha_thermal.c static int airoha_thermal_probe(struct platform_device *pdev) · line 503

Quoted 10 lines
drivers/thermal/airoha_thermal.c +10 -0
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	soc_data = device_get_match_data(dev);
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->pllrg_protect = soc_data->pllrg_protect;

[Severity: High]
Does this code lead to a NULL pointer dereference if the driver is bound manually via sysfs?

device_get_match_data() returns NULL when the driver is bound manually rather than through Device Tree matching. If soc_data is NULL, accessing soc_data->pllrg_protect will trigger a kernel panic. Should soc_data be checked for NULL before being dereferenced?

Quoted 7 lines
drivers/thermal/airoha_thermal.c +7 -0
+
+	if (!soc_data->probe)
+		return -EINVAL;
+
+	ret = soc_data->probe(pdev, priv);
+	if (ret)
+		return ret;

[ ... ]

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
author
#10 lore raw

PATCH v5 5/7 thermal/drivers: airoha: Generalize get_thermal_ADC and set_mux function

In reply to Christian Marangi

In preparation for support of Airoha AN7583, generalize get_thermal_ADC() and set_thermal_mux() with the use of reg_field API.

This is to account the same logic between the current supported SoC and the new one but with different register address.

While at it also further improve some comments and move sleep inside the set_thermal_mux function.

Signed-off-by Christian Marangi <[email protected]>
---
 drivers/thermal/airoha_thermal.c | 56 +++++++++++++++++++++++++-------
 1 file changed, 44 insertions(+), 12 deletions(-)
drivers/thermal/airoha_thermal.c +44 -12
index ebb47ae5f2ce..249abbbd46bc 100644
@@ -193,9 +193,18 @@
 
 #define AIROHA_MAX_SAMPLES			6
 
+enum airoha_thermal_chip_scu_field {
+	AIROHA_THERMAL_DOUT_TADC,
+	AIROHA_THERMAL_MUX_TADC,
+
+	/* keep last */
+	AIROHA_THERMAL_FIELD_MAX,
+};
+
 struct airoha_thermal_priv {
 	struct regmap *map;
 	struct regmap *chip_scu;
+	struct regmap_field *chip_scu_fields[AIROHA_THERMAL_FIELD_MAX];
 	struct resource scu_adc_res;
 
 	u32 pllrg_protect;

@@ -219,25 +228,32 @@ static int airoha_get_thermal_ADC(struct airoha_thermal_priv *priv)
 {
 	u32 val;
 
-	regmap_read(priv->chip_scu, EN7581_DOUT_TADC, &val);
-	return FIELD_GET(EN7581_DOUT_TADC_MASK, val);
+	regmap_field_read(priv->chip_scu_fields[AIROHA_THERMAL_DOUT_TADC],
+			  &val);
+	return val;
 }
 
-static void airoha_init_thermal_ADC_mode(struct airoha_thermal_priv *priv)
+static void airoha_set_thermal_mux(struct airoha_thermal_priv *priv,
+				   int tdac_idx)
 {
-	u32 adc_mux, pllrg;
+	u32 pllrg;
 
 	/* Save PLLRG current value */
 	regmap_read(priv->chip_scu, EN7581_PLLRG_PROTECT, &pllrg);
 
-	/* Give access to thermal regs */
+	/* Give access to Thermal regs */
 	regmap_write(priv->chip_scu, EN7581_PLLRG_PROTECT,
 		     priv->pllrg_protect);
-	adc_mux = FIELD_PREP(EN7581_MUX_TADC, EN7581_SCU_THERMAL_MUX_DIODE1);
-	regmap_write(priv->chip_scu, EN7581_PWD_TADC, adc_mux);
+
+	/* Configure Thermal ADC mux to tdac_idx */
+	regmap_field_write(priv->chip_scu_fields[AIROHA_THERMAL_MUX_TADC],
+			   tdac_idx);
 
 	/* Restore PLLRG value on exit */
 	regmap_write(priv->chip_scu, EN7581_PLLRG_PROTECT, pllrg);
+
+	/* Sleep 10 ms for Thermal ADC to enable */
+	usleep_range(10 * USEC_PER_MSEC, 11 * USEC_PER_MSEC);
 }
 
 static int en7581_thermal_get_temp(struct thermal_zone_device *tz, int *temp)

@@ -344,10 +360,8 @@ static void en7581_thermal_setup_adc_val(struct device *dev,
 	u32 efuse_calib_info = 0;
 	u32 cpu_sensor = 0;
 
-	/* Setup thermal sensor to ADC mode and setup the mux to DIODE1 */
-	airoha_init_thermal_ADC_mode(priv);
-	/* sleep 10 ms for ADC to enable */
-	usleep_range(10 * USEC_PER_MSEC, 11 * USEC_PER_MSEC);
+	/* Setup Thermal Sensor to ADC mode and setup the mux to DIODE1 */
+	airoha_set_thermal_mux(priv, EN7581_SCU_THERMAL_MUX_DIODE1);
 
 	regmap_read(priv->map, EN7581_EFUSE_TEMP_OFFSET_REG, &efuse_calib_info);
 	if (efuse_calib_info) {

@@ -430,13 +444,18 @@ static const struct regmap_config en7581_thermal_regmap_config = {
 	.val_bits		= 32,
 };
 
+static const struct reg_field en7581_chip_scu_fields[AIROHA_THERMAL_FIELD_MAX] = {
+	[AIROHA_THERMAL_DOUT_TADC] = REG_FIELD(EN7581_DOUT_TADC, 0, 15),
+	[AIROHA_THERMAL_MUX_TADC] = REG_FIELD(EN7581_PWD_TADC, 1, 3),
+};
+
 static int en7581_thermal_probe(struct platform_device *pdev,
 				struct airoha_thermal_priv *priv)
 {
 	struct device_node *chip_scu_np;
 	struct device *dev = &pdev->dev;
 	void __iomem *base;
-	int irq, ret;
+	int i, irq, ret;
 
 	base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(base))

@@ -455,6 +474,19 @@ static int en7581_thermal_probe(struct platform_device *pdev,
 	if (IS_ERR(priv->chip_scu))
 		return PTR_ERR(priv->chip_scu);
 
+	for (i = 0; i < AIROHA_THERMAL_FIELD_MAX; i++) {
+		struct regmap_field *field;
+
+		field = devm_regmap_field_alloc(dev, priv->chip_scu,
+						en7581_chip_scu_fields[i]);
+		if (IS_ERR(field)) {
+			of_node_put(chip_scu_np);
+			return PTR_ERR(field);
+		}
+
+		priv->chip_scu_fields[i] = field;
+	}
+
 	of_address_to_resource(chip_scu_np, 0, &priv->scu_adc_res);
 	of_node_put(chip_scu_np);
Signature · 1 line
2.53.0
AI review
#11 lore raw

Re: [PATCH v5 5/7] thermal/drivers: airoha: Generalize get_thermal_ADC and set_mux function

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Replacing `regmap_write()` with `regmap_field_write()` implicitly changes a full register…

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Replacing regmap_write() with regmap_field_write() implicitly changes a full register overwrite to a read-modify-write, potentially failing to clear other bits in the register. --

commit 156e68047b2cd59d81061c97136f7825e128ca91
Author: Christian Marangi <[email protected]>

thermal/drivers: airoha: Generalize get_thermal_ADC and set_mux function

This commit introduces the use of the regmap field API for get_thermal_ADC and set_thermal_mux in preparation for Airoha AN7583 support. It also improves comments and moves a sleep call into the mux function.

Quoted 4 lines
drivers/thermal/airoha_thermal.c +0 -0
index ebb47ae5f2cee..249abbbd46bcd 100644

[ ... ]

Quoted 11 lines
drivers/thermal/airoha_thermal.c +3 -2
@@ -219,25 +228,32 @@ static int airoha_get_thermal_ADC(struct airoha_thermal_priv *priv)
-static void airoha_init_thermal_ADC_mode(struct airoha_thermal_priv *priv)
+static void airoha_set_thermal_mux(struct airoha_thermal_priv *priv,
+				   int tdac_idx)
 {
-	u32 adc_mux, pllrg;
+	u32 pllrg;
 
 	/* Save PLLRG current value */
 	regmap_read(priv->chip_scu, EN7581_PLLRG_PROTECT, &pllrg);

drivers/thermal/airoha_thermal.c static int airoha_get_thermal_ADC(struct airoha_thermal_priv *priv) · line 242

Quoted 10 lines
drivers/thermal/airoha_thermal.c +5 -3
-	/* Give access to thermal regs */
+	/* Give access to Thermal regs */
 	regmap_write(priv->chip_scu, EN7581_PLLRG_PROTECT,
 		     priv->pllrg_protect);
-	adc_mux = FIELD_PREP(EN7581_MUX_TADC, EN7581_SCU_THERMAL_MUX_DIODE1);
-	regmap_write(priv->chip_scu, EN7581_PWD_TADC, adc_mux);
+
+	/* Configure Thermal ADC mux to tdac_idx */
+	regmap_field_write(priv->chip_scu_fields[AIROHA_THERMAL_MUX_TADC],
+			   tdac_idx);

[Severity: High]
Does this unintentionally change a full register write into a read-modify-write?

Before this patch, airoha_init_thermal_ADC_mode used regmap_write to write the mux value directly to EN7581_PWD_TADC. Because adc_mux only populated bits 1-3, this implicitly cleared all other bits to 0.

By switching to regmap_field_write, the operation becomes a read-modify-write on bits 1-3. Given the register name EN7581_PWD_TADC, if there is a power-down bit that defaults to 1 at reset, the new code will preserve it as 1 instead of clearing it to 0.

Could this regression leave the ADC permanently powered down or in an invalid state?

Quoted 3 lines
drivers/thermal/airoha_thermal.c +0 -0
 	/* Restore PLLRG value on exit */
 	regmap_write(priv->chip_scu, EN7581_PLLRG_PROTECT, pllrg);
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
author
#12 lore raw

PATCH v5 6/7 dt-bindings: arm: airoha: Add the chip-scu node for AN7583 SoC

In reply to Christian Marangi

Document support for Airoha AN7583 chip-scu node.

This is similar to Airoha EN7581 with the addition of the presence of thermal sensor in addition to controlling HW PIN and other miscellaneous pheriperals.

Signed-off-by Christian Marangi <[email protected]>
---
 .../bindings/arm/airoha,en7581-chip-scu.yaml       | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
Documentation/devicetree/bindings/arm/airoha,en7581-chip-scu.yaml +14 -0
index 67c449d804c2..cc564dc7b414 100644
@@ -19,15 +19,29 @@ properties:
     items:
       - enum:
           - airoha,en7581-chip-scu
+          - airoha,an7583-chip-scu
       - const: syscon
 
   reg:
     maxItems: 1
 
+  '#thermal-sensor-cells':
+    const: 0
+
 required:
   - compatible
   - reg
 
+if:
+  properties:
+    compatible:
+      contains:
+        const: airoha,en7581-chip-scu
+
+then:
+  properties:
+    '#thermal-sensor-cells': false
+
 additionalProperties: false
 
 examples:
Signature · 1 line
2.53.0
AI review
#13 lore raw

Re: [PATCH v5 6/7] dt-bindings: arm: airoha: Add the chip-scu node for AN7583 SoC

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] The `#thermal-sensor-cells` property is not marked as required for the new…

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The #thermal-sensor-cells property is not marked as required for the new airoha,an7583-chip-scu variant, allowing incomplete device tree nodes to silently pass schema validation. --

commit 87c7f9a301f3391f2461e89c9c85f755bb4ba049
Author: Christian Marangi <[email protected]>

dt-bindings: arm: airoha: Add the chip-scu node for AN7583 SoC

Document support for the Airoha AN7583 chip-scu node. This variant is similar to the EN7581 but adds a thermal sensor capability in addition to controlling HW PIN and other peripherals.

Quoted 3 lines
Documentation/devicetree/bindings/arm/airoha,en7581-chip-scu.yaml +0 -0

[ ... ]

Quoted 4 lines

required:
- compatible
- reg

Documentation/devicetree/bindings/arm/airoha,en7581-chip-scu.yaml properties: · line 44

Quoted 10 lines
Documentation/devicetree/bindings/arm/airoha,en7581-chip-scu.yaml +10 -0
+if:
+  properties:
+    compatible:
+      contains:
+        const: airoha,en7581-chip-scu
+
+then:
+  properties:
+    '#thermal-sensor-cells': false
+

[Severity: Medium]
Should #thermal-sensor-cells be explicitly added to the required list for the airoha,an7583-chip-scu variant?

When a hardware variant adds a new provider capability, the corresponding standard properties typically need to be explicitly required. Without it, incomplete device tree nodes missing the thermal sensor cells property might silently pass schema validation.

Could an else block be added here to enforce this requirement?

else:
required:
- '#thermal-sensor-cells'

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
Reviewed-by on v5 6/7
#14 lore raw

Re: [PATCH v5 6/7] dt-bindings: arm: airoha: Add the chip-scu node for AN7583 SoC

In reply to Christian Marangi
Quoted 10 lines · Christian Marangi

Document support for Airoha AN7583 chip-scu node.

This is similar to Airoha EN7581 with the addition of the presence of thermal sensor in addition to controlling HW PIN and other miscellaneous pheriperals.

Signed-off-by Christian Marangi <[email protected]>
---
 .../bindings/arm/airoha,en7581-chip-scu.yaml       | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
Reviewed-by Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof

author
#15 lore raw

PATCH v5 7/7 thermal/drivers: airoha: Add support for AN7583 Thermal Sensor

In reply to Christian Marangi

Add support for Airoha AN7583 Thermal driver. This apply similar logic on how to read the temperature but totally drop support for the PTP_THERMAL subsystem. PTP_THERMAL subsystem was a way to trigger trip point from hardware by configuring how to read the temperature internally.

This subsystem has been totally removed from Airoha AN7583 permitting only to read the temperature.

The SoC support up to 3 sensor but the original driver always read the BGA sensor hence it's currently implemented reading only this specific sensor. Reference and values for the other 2 sensor are defined for further implementation if confirmed working.

set_thermal_mux() is extended to also address muxing the sensor as AN7583 use a different way to read the temperature from 3 different diode. The EN7581 code is updated to account for these changes.

Signed-off-by Christian Marangi <[email protected]>
---
 drivers/thermal/airoha_thermal.c | 161 ++++++++++++++++++++++++++++++-
 1 file changed, 157 insertions(+), 4 deletions(-)
drivers/thermal/airoha_thermal.c +157 -4
index 249abbbd46bc..c84b5c36e880 100644
@@ -18,6 +18,12 @@
 #define EN7581_DOUT_TADC			0x2f8
 #define   EN7581_DOUT_TADC_MASK			GENMASK(15, 0)
 
+#define AN7583_MUX_SENSOR			0x2a0
+#define   AN7583_LOAD_ADJ			GENMASK(3, 2)
+#define AN7583_MUX_TADC				0x2e4
+#define   AN7583_MUX_TADC_MASK			GENMASK(3, 1)
+#define AN7583_DOUT_TADC			0x2f0
+
 /* PTP_THERMAL regs */
 #define EN7581_TEMPMONCTL0			0x800
 #define   EN7581_SENSE3_EN			BIT(3)

@@ -181,6 +187,11 @@
 #define EN7581_SCU_THERMAL_PROTECT_KEY		0x12
 #define EN7581_SCU_THERMAL_MUX_DIODE1		0x7
 
+#define AN7583_SCU_THERMAL_PROTECT_KEY		0x80
+#define AN7583_NUM_SENSOR			3
+
+#define AIROHA_THERMAL_NO_MUX_SENSOR		-1
+
 /* Convert temp to raw value as read from ADC	((((temp / 100) - init) * slope) / 1000) + offset */
 #define TEMP_TO_RAW(priv, temp)			((((((temp) / 100) - (priv)->init_temp) * \
 						  (priv)->default_slope) / 1000) + \

@@ -193,8 +204,39 @@
 
 #define AIROHA_MAX_SAMPLES			6
 
+/*
+ * AN7583 supports all these ADC mux but the original driver
+ * always checked temp with the AN7583_BGP_TEMP_SENSOR.
+ * Assume using the other sensor temperature is invalid and
+ * always read from AN7583_BGP_TEMP_SENSOR.
+ *
+ * On top of this it's defined that AN7583 supports 3
+ * sensor: AN7583_BGP_TEMP_SENSOR, AN7583_GBE_TEMP_SENSOR,
+ * AN7583_CPU_TEMP_SENSOR.
+ *
+ * Provide the ADC mux for reference.
+ */
+enum an7583_thermal_adc_mux {
+	AN7583_BGP_TEMP_SENSOR,
+	AN7583_PAD_AVS,
+	AN7583_CORE_POWER,
+	AN7583_AVSDAC_OUT,
+	AN7583_VCM,
+	AN7583_GBE_TEMP_SENSOR,
+	AN7583_CPU_TEMP_SENSOR,
+
+	AN7583_ADC_MUX_MAX,
+};
+
+enum an7583_thermal_diode_mux {
+	AN7583_D0_TADC,
+	AN7583_ZERO_TADC,
+	AN7583_D1_TADC,
+};
+
 enum airoha_thermal_chip_scu_field {
 	AIROHA_THERMAL_DOUT_TADC,
+	AIROHA_THERMAL_MUX_SENSOR,
 	AIROHA_THERMAL_MUX_TADC,
 
 	/* keep last */

@@ -208,6 +250,7 @@ struct airoha_thermal_priv {
 	struct resource scu_adc_res;
 
 	u32 pllrg_protect;
+	int current_adc;
 
 	struct thermal_zone_device *tz;
 	int init_temp;

@@ -224,6 +267,24 @@ struct airoha_thermal_soc_data {
 	int (*post_probe)(struct platform_device *pdev);
 };
 
+static const unsigned int an7583_thermal_coeff[AN7583_ADC_MUX_MAX] = {
+	[AN7583_BGP_TEMP_SENSOR] = 973,
+	[AN7583_GBE_TEMP_SENSOR] = 995,
+	[AN7583_CPU_TEMP_SENSOR] = 1035,
+};
+
+static const unsigned int an7583_thermal_slope[AN7583_ADC_MUX_MAX] = {
+	[AN7583_BGP_TEMP_SENSOR] = 7440,
+	[AN7583_GBE_TEMP_SENSOR] = 7620,
+	[AN7583_CPU_TEMP_SENSOR] = 8390,
+};
+
+static const unsigned int an7583_thermal_offset[AN7583_ADC_MUX_MAX] = {
+	[AN7583_BGP_TEMP_SENSOR] = 294,
+	[AN7583_GBE_TEMP_SENSOR] = 298,
+	[AN7583_CPU_TEMP_SENSOR] = 344,
+};
+
 static int airoha_get_thermal_ADC(struct airoha_thermal_priv *priv)
 {
 	u32 val;

@@ -234,7 +295,7 @@ static int airoha_get_thermal_ADC(struct airoha_thermal_priv *priv)
 }
 
 static void airoha_set_thermal_mux(struct airoha_thermal_priv *priv,
-				   int tdac_idx)
+				   int tdac_idx, int sensor_idx)
 {
 	u32 pllrg;
 

@@ -245,9 +306,20 @@ static void airoha_set_thermal_mux(struct airoha_thermal_priv *priv,
 	regmap_write(priv->chip_scu, EN7581_PLLRG_PROTECT,
 		     priv->pllrg_protect);
 
+	/*
+	 * Configure Thermal Sensor mux to sensor_idx.
+	 * (if not supported, sensor_idx is AIROHA_THERMAL_NO_MUX_SENSOR)
+	 */
+	if (sensor_idx != AIROHA_THERMAL_NO_MUX_SENSOR)
+		regmap_field_write(priv->chip_scu_fields[AIROHA_THERMAL_MUX_SENSOR],
+				   sensor_idx);
+
 	/* Configure Thermal ADC mux to tdac_idx */
-	regmap_field_write(priv->chip_scu_fields[AIROHA_THERMAL_MUX_TADC],
-			   tdac_idx);
+	if (priv->current_adc != tdac_idx) {
+		regmap_field_write(priv->chip_scu_fields[AIROHA_THERMAL_MUX_TADC],
+				   tdac_idx);
+		priv->current_adc = tdac_idx;
+	}
 
 	/* Restore PLLRG value on exit */
 	regmap_write(priv->chip_scu, EN7581_PLLRG_PROTECT, pllrg);

@@ -361,7 +433,8 @@ static void en7581_thermal_setup_adc_val(struct device *dev,
 	u32 cpu_sensor = 0;
 
 	/* Setup Thermal Sensor to ADC mode and setup the mux to DIODE1 */
-	airoha_set_thermal_mux(priv, EN7581_SCU_THERMAL_MUX_DIODE1);
+	airoha_set_thermal_mux(priv, EN7581_SCU_THERMAL_MUX_DIODE1,
+			       AIROHA_THERMAL_NO_MUX_SENSOR);
 
 	regmap_read(priv->map, EN7581_EFUSE_TEMP_OFFSET_REG, &efuse_calib_info);
 	if (efuse_calib_info) {

@@ -477,6 +550,10 @@ static int en7581_thermal_probe(struct platform_device *pdev,
 	for (i = 0; i < AIROHA_THERMAL_FIELD_MAX; i++) {
 		struct regmap_field *field;
 
+		/* Skip registering MUX_SENSOR field as not supported */
+		if (i == AIROHA_THERMAL_MUX_SENSOR)
+			continue;
+
 		field = devm_regmap_field_alloc(dev, priv->chip_scu,
 						en7581_chip_scu_fields[i]);
 		if (IS_ERR(field)) {

@@ -519,6 +596,74 @@ static int en7581_thermal_post_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static int an7583_thermal_get_temp(struct thermal_zone_device *tz, int *temp)
+{
+	struct airoha_thermal_priv *priv = thermal_zone_device_priv(tz);
+	int sensor_idx;
+	int delta_diode, delta_gain;
+	int coeff, slope, offset;
+
+	int diode_zero, diode_d0, diode_d1;
+
+	/* Always read sensor AN7583_BGP_TEMP_SENSOR */
+	sensor_idx = AN7583_BGP_TEMP_SENSOR;
+
+	coeff = an7583_thermal_coeff[sensor_idx];
+	slope = an7583_thermal_slope[sensor_idx];
+	offset = an7583_thermal_offset[sensor_idx];
+
+	airoha_set_thermal_mux(priv, AN7583_ZERO_TADC, sensor_idx);
+	diode_zero = airoha_get_thermal_ADC(priv);
+	airoha_set_thermal_mux(priv, AN7583_D0_TADC, sensor_idx);
+	diode_d0 = airoha_get_thermal_ADC(priv);
+	airoha_set_thermal_mux(priv, AN7583_D1_TADC, sensor_idx);
+	diode_d1 = airoha_get_thermal_ADC(priv);
+
+	delta_diode = diode_d1 - diode_d0;
+	delta_gain = (delta_diode * coeff) / 100 + (diode_zero - diode_d1);
+	if (!delta_gain)
+		return -EINVAL;
+
+	*temp = (slope * delta_diode * 10) / delta_gain - offset * 10;
+	*temp *= 100;
+
+	return 0;
+}
+
+static const struct thermal_zone_device_ops an7583_tz_ops = {
+	.get_temp = an7583_thermal_get_temp,
+};
+
+static const struct reg_field an7583_chip_scu_fields[AIROHA_THERMAL_FIELD_MAX] = {
+	[AIROHA_THERMAL_DOUT_TADC] = REG_FIELD(AN7583_DOUT_TADC, 0, 31),
+	[AIROHA_THERMAL_MUX_TADC] = REG_FIELD(AN7583_MUX_TADC, 1, 3),
+	[AIROHA_THERMAL_MUX_SENSOR] = REG_FIELD(AN7583_MUX_SENSOR, 2, 3),
+};
+
+static int an7583_thermal_probe(struct platform_device *pdev,
+				struct airoha_thermal_priv *priv)
+{
+	struct device *dev = &pdev->dev;
+	int i;
+
+	priv->chip_scu = device_node_to_regmap(dev->of_node);
+	if (IS_ERR(priv->chip_scu))
+		return PTR_ERR(priv->chip_scu);
+
+	for (i = 0; i < AIROHA_THERMAL_FIELD_MAX; i++) {
+		struct regmap_field *field;
+
+		field = devm_regmap_field_alloc(dev, priv->chip_scu,
+						an7583_chip_scu_fields[i]);
+		if (IS_ERR(field))
+			return PTR_ERR(field);
+
+		priv->chip_scu_fields[i] = field;
+	}
+
+	return 0;
+}
+
 static int airoha_thermal_probe(struct platform_device *pdev)
 {
 	const struct airoha_thermal_soc_data *soc_data;

@@ -533,6 +678,7 @@ static int airoha_thermal_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	priv->pllrg_protect = soc_data->pllrg_protect;
+	priv->current_adc = -1;
 
 	if (!soc_data->probe)
 		return -EINVAL;

@@ -561,8 +707,15 @@ static const struct airoha_thermal_soc_data en7581_data = {
 	.post_probe = &en7581_thermal_post_probe,
 };
 
+static const struct airoha_thermal_soc_data an7583_data = {
+	.pllrg_protect = AN7583_SCU_THERMAL_PROTECT_KEY,
+	.thdev_ops = &an7583_tz_ops,
+	.probe = &an7583_thermal_probe,
+};
+
 static const struct of_device_id airoha_thermal_match[] = {
 	{ .compatible = "airoha,en7581-thermal", .data = &en7581_data },
+	{ .compatible = "airoha,an7583-chip-scu", .data = &an7583_data },
 	{},
 };
 MODULE_DEVICE_TABLE(of, airoha_thermal_match);
Signature · 1 line
2.53.0
AI review
#16 lore raw

Re: [PATCH v5 7/7] thermal/drivers: airoha: Add support for AN7583 Thermal Sensor

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:

New issues:
- [High] Potential 32-bit signed integer overflow in temperature calculation.
- [Medium] The MUX_SENSOR register field width is too narrow to hold all defined sensor indices, leading to silent truncation.

Pre-existing issues:
- [Critical] Missing remove callback causes devres Use-After-Free of thermal zone.
- [High] Unprotected read-modify-write sequence on a shared syscon register creates a race condition.
- [High] Unchecked regmap read operations lead to use of uninitialized stack variables.
- [High] IRQ requested before thermal zone pointer is initialized leads to potential NULL pointer dereference.
- [High] Missing NULL check on device_get_match_data() leads to kernel panic on sysfs bind. --

commit 57a01e256a4324da608f512eaa2939419b755b71
Author: Christian Marangi <[email protected]>

thermal/drivers: airoha: Add support for AN7583 Thermal Sensor

Add support for Airoha AN7583 Thermal driver. This apply similar logic on how to read the temperature but totally drop support for the PTP_THERMAL subsystem.

Quoted 4 lines
drivers/thermal/airoha_thermal.c +0 -0
index 249abbbd46bcd..c84b5c36e8803 100644

[ ... ]

Quoted 6 lines

static void airoha_set_thermal_mux(struct airoha_thermal_priv *priv,

drivers/thermal/airoha_thermal.c +1 -1
-				   int tdac_idx)
+				   int tdac_idx, int sensor_idx)
 {
 	u32 pllrg;

[ ... ]

drivers/thermal/airoha_thermal.c static void airoha_set_thermal_mux(struct airoha_thermal_priv *priv, · line 325

Quoted 11 lines
drivers/thermal/airoha_thermal.c +5 -2
 	/* Configure Thermal ADC mux to tdac_idx */
-	regmap_field_write(priv->chip_scu_fields[AIROHA_THERMAL_MUX_TADC],
-			   tdac_idx);
+	if (priv->current_adc != tdac_idx) {
+		regmap_field_write(priv->chip_scu_fields[AIROHA_THERMAL_MUX_TADC],
+				   tdac_idx);
+		priv->current_adc = tdac_idx;
+	}
 
 	/* Restore PLLRG value on exit */
 	regmap_write(priv->chip_scu, EN7581_PLLRG_PROTECT, pllrg);

[Severity: High]
This is a pre-existing issue, but since this patch significantly increases the frequency of this function for runtime polling, is there a race condition with the read-modify-write sequence on the shared syscon register?

The regmap lock only protects individual reads and writes, making the unlock-modify-lock sequence on EN7581_PLLRG_PROTECT non-atomic. If another driver sharing the SCU performs operations concurrently, could they race and leave the protection register permanently unlocked or restore an incorrect state?

[Severity: High]
This is a pre-existing issue, but does the unchecked regmap read operation lead to using an uninitialized stack variable?

In airoha_set_thermal_mux(), if the regmap_read() for EN7581_PLLRG_PROTECT fails, the pllrg variable remains uninitialized and is later written back to the hardware protection register.

Similarly, in airoha_get_thermal_ADC(), an unchecked regmap_field_read() leaves the stack variable uninitialized before being returned and used in temperature calculations.

[ ... ]

Quoted 9 lines
drivers/thermal/airoha_thermal.c +9 -0
+static int an7583_thermal_get_temp(struct thermal_zone_device *tz, int *temp)
+{
+	struct airoha_thermal_priv *priv = thermal_zone_device_priv(tz);
+	int sensor_idx;
+	int delta_diode, delta_gain;
+	int coeff, slope, offset;
+
+	int diode_zero, diode_d0, diode_d1;
+

[ ... ]

drivers/thermal/airoha_thermal.c static int en7581_thermal_post_probe(struct platform_device *pdev) · line 631

Quoted 10 lines
drivers/thermal/airoha_thermal.c +10 -0
+	delta_diode = diode_d1 - diode_d0;
+	delta_gain = (delta_diode * coeff) / 100 + (diode_zero - diode_d1);
+	if (!delta_gain)
+		return -EINVAL;
+
+	*temp = (slope * delta_diode * 10) / delta_gain - offset * 10;
+	*temp *= 100;
+
+	return 0;
+}

[Severity: High]
Can the multiplication of slope and delta_diode overflow a 32-bit signed integer?

The slope variable can be up to 8390, and delta_diode is derived from the difference of two 32-bit ADC readings. A delta_diode value exceeding 25,595 would cause the (slope * delta_diode * 10) calculation to overflow the signed 32-bit maximum limit (2,147,483,647), leading to undefined behavior and wildly incorrect temperature readings.

[ ... ]

drivers/thermal/airoha_thermal.c static int en7581_thermal_post_probe(struct platform_device *pdev) · line 641

Quoted 9 lines
drivers/thermal/airoha_thermal.c +9 -0
+static const struct thermal_zone_device_ops an7583_tz_ops = {
+	.get_temp = an7583_thermal_get_temp,
+};
+
+static const struct reg_field an7583_chip_scu_fields[AIROHA_THERMAL_FIELD_MAX] = {
+	[AIROHA_THERMAL_DOUT_TADC] = REG_FIELD(AN7583_DOUT_TADC, 0, 31),
+	[AIROHA_THERMAL_MUX_TADC] = REG_FIELD(AN7583_MUX_TADC, 1, 3),
+	[AIROHA_THERMAL_MUX_SENSOR] = REG_FIELD(AN7583_MUX_SENSOR, 2, 3),
+};

[Severity: Medium]
Is the AIROHA_THERMAL_MUX_SENSOR register field width too narrow to hold all defined sensor indices?

The field is defined as a 2-bit field (bits 2 to 3), which can hold a maximum value of 3. However, the driver enumerates sensor indices up to 6 (AN7583_CPU_TEMP_SENSOR = 6). If the driver is extended to use these other sensors, calling regmap_field_write() with values larger than 3 will silently truncate the values, causing the hardware to sample the wrong sensor.

[ ... ]

Quoted 4 lines

static int airoha_thermal_probe(struct platform_device *pdev) {

drivers/thermal/airoha_thermal.c +0 -0
 	const struct airoha_thermal_soc_data *soc_data;
 	struct airoha_thermal_priv *priv;

drivers/thermal/airoha_thermal.c

Quoted 10 lines
drivers/thermal/airoha_thermal.c +0 -0
 	struct device *dev = &pdev->dev;
 	int ret;
 
 	soc_data = device_get_match_data(dev);
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
 	priv->pllrg_protect = soc_data->pllrg_protect;

[Severity: High]
This is a pre-existing issue, but could device_get_match_data() return NULL and cause a kernel panic here?

If the driver is manually bound via sysfs to a device that lacks an associated device tree node, device_get_match_data() returns NULL, leading to a NULL pointer dereference when accessing soc_data->pllrg_protect.

drivers/thermal/airoha_thermal.c

Quoted 12 lines
drivers/thermal/airoha_thermal.c +1 -0
+	priv->current_adc = -1;
 
 	if (!soc_data->probe)
 		return -EINVAL;
 
 	ret = soc_data->probe(pdev, priv);
 	if (ret)
 		return ret;
 
 	/* register of thermal sensor and get info from DT */
 	priv->tz = devm_thermal_of_zone_register(dev, 0, priv,
 						 soc_data->thdev_ops);

[Severity: High]
This is a pre-existing issue, but is the IRQ requested before the thermal zone pointer is initialized?

In en7581_thermal_probe(), which is called by soc_data->probe(), the hardware interrupt is registered via devm_request_threaded_irq() before priv->tz is assigned here. If an interrupt fires immediately, en7581_thermal_irq() will execute and pass the uninitialized NULL priv->tz to thermal_zone_device_update(), causing a kernel panic.

[Severity: Critical]
This is a pre-existing issue, but does the lack of a remove callback cause a use-after-free of the thermal zone?

The driver enables hardware interrupts but does not provide a remove function to disable them. On module unload or manual unbind, devres cleans up resources in reverse order: the thermal zone priv->tz is unregistered and freed before the threaded IRQ handler is freed. If the interrupt fires during this unbind window, en7581_thermal_irq() executes and accesses the freed thermal zone.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7
author
#17 lore raw
In reply to Christian Marangi

Il giorno mer 20 mag 2026 alle ore 17:55 Christian Marangi <[email protected]> ha scritto:

Quoted 29 lines

This small series implement all the required changes to support Airoha AN7583 Thermal Sensor.

The SoC dropped the Monitor subsystem and only provide temperature reading. Some generalization was required to at least save the common code with also the help of reg field API.

Changes v5:
- Fix alpabetical order in Documentation patch
- Address suggested change by AI Bot
Changes v4:
- Rebase on top of linux-next
Changes v3:
- Property use chip-scu for thermal sensor Changes v2:
- Update DT schema patch to implement dedicated schema

Christian Marangi (7):
thermal/drivers: airoha: fix copy paste error on clamp_t low temp thermal/drivers: airoha: fix copy paste error for sen internal thermal/drivers: airoha: Convert to regmap API thermal/drivers: airoha: Generalize probe function thermal/drivers: airoha: Generalize get_thermal_ADC and set_mux
function
dt-bindings: arm: airoha: Add the chip-scu node for AN7583 SoC thermal/drivers: airoha: Add support for AN7583 Thermal Sensor

Quoted 4 lines
 .../bindings/arm/airoha,en7581-chip-scu.yaml  |  14 +
 drivers/thermal/airoha_thermal.c              | 380 +++++++++++++++---
 2 files changed, 327 insertions(+), 67 deletions(-)

Any news for this series? Can someone check it? Every patch have a tag.