Re: [PATCH v2 1/4] dt-bindings: interrupt-controller: add Mobileye EyeQ5 PCIe MSI controller

[Gregory CLEMENT <gregory.clement@bootlin.com>]

Hello Krzysztof,

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] dt-bindings: interrupt-controller: eyeq5-msi: drop bogus #interrupt-cells
> - [Low] dt-bindings: interrupt-controller: eyeq5-msi: drop software label property

You do not need to review this version. The remarks above from Sashiko
are correct. I will send a new revision soon. I hope that it will be the
correct one.

Gregory

> --
>
> commit 9aaa3be0c94345e12c3b93b0a3ada8acf3fb0a25
> Author: Gregory CLEMENT <gregory.clement@bootlin.com>
>
> dt-bindings: interrupt-controller: add Mobileye EyeQ5 PCIe MSI controller
>
> This commit adds the devicetree binding schema for the Mobileye EyeQ5
> PCIe MSI controller.
>
>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/mobileye,eyeq5-msi.yaml b/Documentation/devicetree/bindings/interrupt-controller/mobileye,eyeq5-msi.yaml
>> new file mode 100644
>> index 0000000000000..b86698e4f82c7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/mobileye,eyeq5-msi.yaml
>> @@ -0,0 +1,90 @@
>
> [ ... ]
>
>> +properties:
>> +  compatible:
>> +    const: mobileye,eyeq5-msi
>> +
>> +  reg:
>> +    items:
>> +      - description: GCSR block of the PCIe port
>> +      - description: MSI vector control block
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  interrupt-controller: true
>> +
>> +  '#interrupt-cells':
>> +    const: 1
>
> [Severity: Medium]
> Should interrupt-controller and '#interrupt-cells' be dropped from
> properties and required in mobileye,eyeq5-msi.yaml?
>
> According to the description, this hardware block only snoops message
> signaled interrupt write transactions from a peripheral port and signals
> its parent interrupt controller when a vector is pending. Consumers
> reference this node exclusively via msi-parent with '#msi-cells' set to 0,
> and it has no wired interrupt inputs.
>
> Consistently, the driver in drivers/irqchip/irq-eyeq5-msi.c only registers
> a parent domain via msi_create_parent_irq_domain() using
> eyeq_msi_domain_ops (.alloc, .free, and .select =
> msi_lib_irq_domain_select) and implements no .translate or .xlate callback
> for wired interrupt specifiers.
>
> Requiring interrupt-controller and '#interrupt-cells' set to 1 exposes an
> undocumented 1-cell interrupt specifier for a controller that cannot serve
> as an interrupt-parent, whereas chaining to a parent interrupt is an
> operating system driver detail (compare with altr,msi-controller.yaml,
> fsl,ls-msi.yaml, and fsl,mu-msi.yaml).
>
>> +
>> +  msi-controller: true
>> +
>> +  '#msi-cells':
>> +    const: 0
>> +
>> +  label:
>> +    $ref: /schemas/types.yaml#/definitions/string
>> +    description: Name of the MSI domain. If omitted, the node name is used.
>
> [Severity: Low]
> Is the label property appropriate for mobileye,eyeq5-msi.yaml?
>
> In devicetree bindings, label is intended for human-identifiable physical
> labels on a board or enclosure (such as external ports, leds, or buttons),
> whereas an irqdomain name and driver fallback behavior are kernel software
> concepts. In drivers/irqchip/irq-eyeq5-msi.c, this property is only used
> to print a boot message in pr_info(), and no other binding under
> Documentation/devicetree/bindings/interrupt-controller/ defines label.
>
> Additionally, if label were ever kept, its string type is already defined
> by core dtschema common properties, making the explicit $ref to
> /schemas/types.yaml#/definitions/string redundant.
>
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261102-eyeq5_msi-v2-0-4aa3d3608064@bootlin.com?part=1

-- 
Grégory CLEMENT, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com