Re: [PATCH v2] dt-bindings: pwm: st,pwm: convert to DT schema

[sashiko-bot@kernel.org]

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

New issues:
- [High] The YAML binding strictly validates against `st,pwm`, but the hardware driver and actual device trees use `st,sti-pwm`.
- [Medium] The schema drops `pinctrl-0` from the required list but keeps `pinctrl-names` as required, creating an inconsistent configuration requirement.

Pre-existing issues:
- [High] The driver requests the IRQ before fully initializing the regmap fields used by the interrupt handler, creating a race condition that can lead to a NULL pointer dereference.
--

commit e38e89bd06d5e023d198bb6e9fb8add957cdf8e9
Author: Charan Pedumuru <charan.pedumuru@gmail.com>

dt-bindings: pwm: st,pwm: convert to DT schema

This commit converts the STMicroelectronics STiH41x PWM/Capture controller
text binding to DT schema. It updates the required list by adding
'interrupts' and dropping 'pinctrl-0'.

> diff --git a/Documentation/devicetree/bindings/pwm/st,pwm.yaml b/Documentation/devicetree/bindings/pwm/st,pwm.yaml
> new file mode 100644
> index 0000000000000..a045502e0e704
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/st,pwm.yaml
> @@ -0,0 +1,93 @@
[ ... ]
> +properties:
> +  compatible:
> +    const: st,pwm

[Severity: High]
Does this compatible string match the hardware driver and actual device trees?

If existing in-kernel device trees and the matching driver
(drivers/pwm/pwm-sti.c) use "st,sti-pwm" instead of "st,pwm", will
make dtbs_check silently skip validation for them?

[ ... ]
> +  st,capture-num-chan:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Number of available input capture channels.
> +    minimum: 0
> +    maximum: 4
> +    default: 0
> +
> +required:
> +  - reg
> +  - compatible
> +  - "#pwm-cells"
> +  - pinctrl-names

[Severity: Medium]
Since 'pinctrl-0' was dropped from the required list, should 'pinctrl-names'
also be removed?

Retaining 'pinctrl-names' in the required list while making 'pinctrl-0'
optional forces device trees that do not define any pinctrl configuration
to include a meaningless pinctrl-names = "default"; just to pass schema
validation.

> +  - clock-names
> +  - clocks
> +  - interrupts
[ ... ]
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    pwm@fe510000 {
> +        compatible = "st,pwm";

[Severity: High]
Does this example propagate an incorrect compatible string?

If the hardware actually uses "st,sti-pwm", using "st,pwm" here might mislead
future device tree authors.

[ ... ]

[Severity: High]
This is a pre-existing issue, but can the interrupt fire before the regmap
fields are fully initialized?

Looking at drivers/pwm/pwm-sti.c:sti_pwm_probe(), the IRQ is requested early:

	ret = devm_request_irq(&pdev->dev, irq, sti_pwm_interrupt, 0, ...);

Later, the regmap fields crucial for the IRQ handler are initialized:

	ret = sti_pwm_probe_regmap(pc);

If a pending interrupt from the bootloader fires immediately after
devm_request_irq(), sti_pwm_interrupt() will execute and call
regmap_field_read(pc->pwm_cpt_int_stat, ...). Since pc->pwm_cpt_int_stat
is NULL before sti_pwm_probe_regmap() initializes it, would this cause a
kernel panic due to a NULL pointer dereference?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618-st-pwm-v2-1-c792d5795ce2@gmail.com?part=1