Re: [PATCH v2 1/3] dt-bindings: mfd: st,stmpe: add deprecated properties

[sashiko-bot@kernel.org]

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

New issues:
- [Low] The commit message description is not capitalized, violating the MFD subsystem naming conventions.

Pre-existing issues:
- [High] Incorrect teardown order in the MFD core driver causes use-after-disable and illegal IRQ domain removal.
- [High] Interrupt is requested before hardware is initialized in the touchscreen probe.
- [High] Missing manual `input_unregister_device()` in `stmpe_ts_remove()` causes the driver to access disabled hardware during input device unregistration.
--

commit 2b78324bb3bbfce531df8a18d620cea4c9246016
Author: Frank Li <Frank.Li@nxp.com>

dt-bindings: mfd: st,stmpe: add deprecated properties

This commit adds deprecated properties st,sample-time, st,mod-12b,
st,ref-sel, and st,adc-freq to the st,stmpe-ts child node. This resolves
CHECK_DTBS warnings for devicetrees that place these properties under
the sample_ts node instead of the parent MFD node.

> dt-bindings: mfd: st,stmpe: add deprecated properties

[Severity: Low]
Does the description "add deprecated properties" follow the MFD subsystem
naming conventions? The guidelines specify that the description after the
subsystem prefix should always be capitalized.

> diff --git a/Documentation/devicetree/bindings/mfd/st,stmpe.yaml b/Documentation/devicetree/bindings/mfd/st,stmpe.yaml
> index ddb27ae64ba37..592e8f17659f3 100644
> --- a/Documentation/devicetree/bindings/mfd/st,stmpe.yaml
> +++ b/Documentation/devicetree/bindings/mfd/st,stmpe.yaml
> @@ -141,6 +141,30 @@ properties:
>        compatible:
>          const: st,stmpe-ts
>  
> +      st,sample-time:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [ 0, 1, 2, 3, 4, 5, 6 ]
> +        deprecated: true
> +        description: See top layer st,sample-time.
> +
> +      st,mod-12b:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [ 0, 1 ]
> +        deprecated: true
> +        description: See top layer st,mod-12b
> +
> +      st,ref-sel:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [ 0, 1 ]
> +        deprecated: true
> +        description: See top layer st,ref-sel
> +
> +      st,adc-freq:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [ 0, 1, 2, 3 ]
> +        deprecated: true
> +        description: See top layer st,adc-freq
> +
>        st,ave-ctrl:
>          $ref: /schemas/types.yaml#/definitions/uint32
>          enum: [ 0, 1, 2, 3 ]

While reviewing this patch, I noticed a few pre-existing issues in the related
C drivers that might warrant a look.

drivers/mfd/stmpe.c:stmpe_remove() {
	if (stmpe->domain)
		irq_domain_remove(stmpe->domain);

	if (!IS_ERR(stmpe->vio) && regulator_is_enabled(stmpe->vio))
		regulator_disable(stmpe->vio);
	...
	__stmpe_disable(stmpe, STMPE_BLOCK_ADC);

	mfd_remove_devices(stmpe->dev);
}

[Severity: High]
This isn't a bug introduced by this patch, but does this teardown sequence
cause a use-after-disable or illegal IRQ domain removal? Since the core
resources, IRQ domains, and power are disabled before mfd_remove_devices()
is called, won't the child drivers attempt to manage IRQs and communicate
with unpowered hardware when their remove() callbacks execute?


drivers/input/touchscreen/stmpe-ts.c:stmpe_input_probe() {
	error = devm_request_threaded_irq(&pdev->dev, ts_irq,
					  NULL, stmpe_ts_handler,
					  IRQF_ONESHOT, STMPE_TS_NAME, ts);
	if (error) {
		...
	}

	error = stmpe_init_hw(ts);
}

[Severity: High]
This is a pre-existing issue, but is it safe to request the IRQ before
the hardware is fully initialized? Could a spurious or pending interrupt
trigger the handler on uninitialized or disabled hardware, potentially
leading to bus errors or hangs when the handler attempts to read registers?


drivers/input/touchscreen/stmpe-ts.c:stmpe_ts_remove() {
	stmpe_disable(ts->stmpe, STMPE_BLOCK_TOUCHSCREEN);
}

[Severity: High]
This isn't a bug introduced by this patch, but does manually disabling the
touchscreen block here bypass the devres lifecycle dependency order? 

Because the managed input device and IRQ are cleaned up later by devres,
does this cause the input device's close callback to execute after the
hardware is disabled?

drivers/input/touchscreen/stmpe-ts.c:stmpe_ts_close() {
	stmpe_set_bits(ts->stmpe, STMPE_REG_TSC_CTRL,
			STMPE_TSC_CTRL_TSC_EN, 0);
}

Could this out-of-order cleanup lead to illegal I2C writes to the disabled
hardware block?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831-dts-stmpe-v2-0-f93d5f080443@nxp.com?part=1