Re: [PATCH 1/4] dt-bindings: arm: add cznic,turris-omnia-mcu binding

["Marek Behún" <kabel@kernel.org> (by way of Marek Behún <kabel@kernel.org>)]

Hello Krzysztof,

thanks for the review.

On Thu, 24 Aug 2023 09:37:23 +0200
Krzysztof Kozlowski <krzk@kernel.org> wrote:

> On 23/08/2023 18:10, Marek Behún wrote:
> > Add binding for cznic,turris-omnia-mcu, the device-tree node
> > representing the system-controller features provided by the MCU on the
> > Turris Omnia router.
> > 
> > Signed-off-by: Marek Behún <kabel@kernel.org>  
> 
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC (and consider --no-git-fallback argument). It might
> happen, that command when run on an older kernel, gives you outdated
> entries. Therefore please be sure you base your patches on recent Linux
> kernel.

I shall do that.

> > ---
> >  .../bindings/arm/cznic,turris-omnia-mcu.yaml  | 72 +++++++++++++++++++
> >  MAINTAINERS                                   |  1 +
> >  2 files changed, 73 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/arm/cznic,turris-omnia-mcu.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/cznic,turris-omnia-mcu.yaml b/Documentation/devicetree/bindings/arm/cznic,turris-omnia-mcu.yaml
> > new file mode 100644
> > index 000000000000..055485847e71
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/cznic,turris-omnia-mcu.yaml  
> 
> ARM directory is only for top-level bindings, so this should go to soc.

Hmm. The board uses a marvell SoC, but the board is from CZ.NIC (who
does not create its own SoCs). So should this go into soc/marvell or
soc/cznic?

> > @@ -0,0 +1,72 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/arm/cznic,turris-omnia-mcu.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: CZ.NIC's Turris Omnia MCU
> > +
> > +maintainers:
> > +  - Marek Behún <kabel@kernel.org>
> > +
> > +description:
> > +  The MCU on Turris Omnia acts as a system controller providing additional
> > +  GPIOs, interrupts, watchdog, system power off and wakeup configuration.
> > +
> > +properties:
> > +  compatible:
> > +    const: cznic,turris-omnia-mcu
> > +
> > +  reg:
> > +    description: MCU I2C slave address
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  interrupt-controller: true
> > +
> > +  '#interrupt-cells':
> > +    const: 2
> > +
> > +  gpio-controller: true
> > +
> > +  '#gpio-cells':
> > +    const: 2
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - interrupt-controller
> > +  - gpio-controller
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +
> > +    ic: interrupt-controller {
> > +        interrupt-controller;
> > +        #interrupt-cells = <2>;
> > +    };  
> 
> Drop this node, not relevant.

Will the binding example compile without the ic node if the
system-controller below uses it?

> > +
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        system-controller@2a {
> > +            compatible = "cznic,turris-omnia-mcu";
> > +            reg = <0x2a>;
> > +
> > +            interrupt-parent = <&ic>;
> > +            interrupts = <11 IRQ_TYPE_NONE>;  
> 
> Are you sure interrupt is type none?

The interrupt type is either LEVEL_LOW or EDGE_FALLING, depending on
the version of the MCU firmware, so this has to be selected by the
driver.

I tried setting LEVEL_LOW, since that is the one that is used by the
newest MCU firmware. But then if the firmware is old and I want to
select EDGE_FALLING in the driver when requesting the IRQ, it fails
with message
  type mismatch, failed to map hwirq-%lu for %s!
from
  kernel/irq/irqdomain.c function irq_create_fwspec_mapping

I guess I can use irqd_set_trigger_type() before requesting the IRQ to
avoid this error.

Should I use use LEVEL_LOW in the binding example and device-tree?

Thank you for the review.

Marek