Re: [RFC PATCH v2] ARM: dts: Add TS-7553-V2 support

[Kris Bahnsen]

On Thu, 2022-07-14 at 10:34 +0200, Krzysztof Kozlowski wrote:
> On 14/07/2022 00:12, Kris Bahnsen wrote:
> > Add initial support of the i.MX6UL based TS-7553-V2 platform.
> 
> Use subject prefix matching the subsystem. git log --oneline --

Can you please elaborate? The subject prefix is "ARM: dts:", I'm not
sure what is missing. Should it be something like
"ARM: dts: imx6ul-ts7553v2:" in this case?

> 
> > 
> > Signed-off-by: Kris Bahnsen <kris@xxxxxxxxxxxxxx>
> > ---
> > 
> > V1->V2: Implement changes recommended by Rob Herring and dtbs_check
> > 
> > RFC only, not yet ready to merge, more testing needed and we're working on
> > SPI LCD support for this platform.
> > 
> > Specifically, I have a few questions on some paradigms and dtbs_check output:
> > 
> > imx6ul-ts7553v2.dtb: /: i2c-gpio: {'compatible': ... \
> > 'magnetometer@c': {'compatible': ['asahi-kasei,ak8975'], 'reg': [[12]]}}}} \
> > is not of type 'array'
> >   I'm not sure what this error is referring to as I've copied the example in
> >   invensense,mpu6050.yaml almost verbatim. Is this an issue with our patch
> >   or a false positive from dtbs_check?
> 
> You would need to paste entire error, maybe with checker flags -v.

Here is the verbose output. I'm not familiar enough yet with the schema and its
validation code to catch what is wrong and would appreciate any insight.

Check:  arch/arm/boot/dts/imx6ul-ts7553v2.dtb
/work/arch/arm/boot/dts/imx6ul-ts7553v2.dtb: /: i2c-gpio: {'compatible': ['i2c-gpio'], \
'#address-cells': [[1]], '#size-cells': [[0]], 'pinctrl-names': ['default'], \
'pinctrl-0': [[58]], 'sda-gpios': [[11, 5, 6]], 'scl-gpios': [[11, 4, 6]], \
'imu@68': {'compatible': ['invensense,mpu9250'], 'reg': [[104]], \
'interrupt-parent': [[55]], 'interrupts': [[1, 1]], 'i2c-gate': {'#address-cells': [[1]], \
'#size-cells': [[0]], 'magnetometer@c': {'compatible': ['asahi-kasei,ak8975'], \
'reg': [[12]]}}}} is not of type 'array'

Failed validating 'type' in schema['patternProperties']['(?<!,nr)-gpios?$']:
    {'items': {'additionalItems': {'$ref': '#/definitions/cell'},
               'items': [{'oneOf': [{'maximum': 4294967295,
                                     'minimum': 1,
                                     'phandle': True,
                                     'type': 'integer'},
                                    {'const': 0, 'type': 'integer'}]}],
               'minItems': 1,
               'type': 'array'},
     'minItems': 1,
     'type': 'array'}

On instance['i2c-gpio']:
    {'#address-cells': [[1]],
     '#size-cells': [[0]],
     'compatible': ['i2c-gpio'],
     'imu@68': {'compatible': ['invensense,mpu9250'],
                'i2c-gate': {'#address-cells': [[1]],
                             '#size-cells': [[0]],
                             'magnetometer@c': {'compatible': ['asahi-kasei,ak8975'],
                                                'reg': [[12]]}},
                'interrupt-parent': [[55]],
                'interrupts': [[1, 1]],
                'reg': [[104]]},
     'pinctrl-0': [[58]],
     'pinctrl-names': ['default'],
     'scl-gpios': [[11, 4, 6]],
     'sda-gpios': [[11, 5, 6]]}
        From schema: /usr/local/lib/python3.9/dist-packages/dtschema/schemas/gpio/gpio-consumer.yaml

> 
> > 
> > 
> > imx6ul-ts7553v2.dtb: spi@2010000: spidev@1: 'compatible' is a required property
> >   Many of our devices have open-ended I2C and SPI ports that may or may not be
> >   used in customer applications. With "spidev" compatible string no longer
> >   supported, there is no easy way we know of to leave a placeholder or
> >   indication that the interface is present, usable, and either needs specific
> >   support enabled in kernel or userspace access via /dev/. We would love
> >   feedback on how to handle this situation when submitting platforms upstream.
> 
> No empty devices, especially for spidev in DTS. There is really no
> single need to add fake spidev... really, why? The customer cannot read
> hardware manual and cannot see the header on the board? You can give him
> a tutorial/howto guide, but don't embed dead or non-real code in DTS.

We ship devices as bootable out of the box. A number of our customers end up
attaching SPI devices that do not have existing kernel drivers and talk to them
from userspace without having to touch a kernel build. The loss of spidev
directly has increased support requests we receive on the matter.

> 
> > 
> > 
> > imx6ul-ts7553v2.dtb: wifi@0: compatible:0: 'microchip,wilc1000' was expected
> > imx6ul-ts7553v2.dtb: wifi@0: compatible: ['microchip,wilc3000'...] is too long
> > imx6ul-ts7553v2.dtb: wifi@0: 'chip_en-gpios' does not match any of the \
> > regexes: pinctrl-[0-9]+'
> >   As noted in the comments in the dts, the WILC1000 in-kernel driver doesn't
> >   support the BLE features of the WILC3000. We maintain an external module
> >   tree that lets us build Microchip's official driver with WILC3000 support.
> >   Would the extraneous compatible string and property be accepted upstream
> >   in light of this?
> 
> No. No undocumented comaptibles with some wrong properties. chip_en is
> clearly wrong, so it cannot go to DTS. Upstream driver or remove the node.

Unfortunate, but, understood.

> 
> > 
> > 
> >  Documentation/devicetree/bindings/arm/fsl.yaml |    1 +
> 
> This is a separate patch.

Makes sense.

> 
> >  arch/arm/boot/dts/Makefile                     |    1 +
> >  arch/arm/boot/dts/imx6ul-ts7553v2.dts          |  693 ++++++++++
> >  arch/arm/configs/ts7970_defconfig              | 1627 ++++++++++++++++++++++++
> >  arch/arm/configs/tsimx6ul_defconfig            |  967 ++++++++++++++
> 
> This as well (and won't be accepted - no new defconfigs).

The defconfigs being included were an oversight and absolutely sloppy on my
part. I sincerely apologize for that.

> 
> > 
> > +
> > +	leds {
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&pinctrl_gpio_leds>;
> > +		compatible = "gpio-leds";
> > +
> > +		green-led {
> 
> led-0
> 
> > +			label = "green-led";
> 
> Rather use color and function, then labels.

Fixed, thank you. I was unaware of this newer set of properties and I've
found where they are clearly spelled out.

> 
> > +
> > +	gpio-keys {
> > +		compatible = "gpio-keys";
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&pinctrl_gpio_keys>;
> > +
> > +		left {
> 
> This fails on dtbs_check. Generic node names, so "key-0" or "key-left"

For reference, as of commit b047602d579b4fb028128a525f056bbdc890e7f0, there
are no errors/warnings from dtbs_check or checkpatch.pl regarding node
names being "key-..." and the example in gpio-keys.yaml uses "up" "left" etc.

I've also changed the node name to just "keys" per devicetree specifications
doc.

> 
> > +	i2c_gpio: i2c-gpio {
> 
> Generic node name, so "i2c"

Understood.

Are there any guidelines/restrictions on label use/schema 
throughout a dts file? The devicetree-specification document only defines
valid characters for a label and I've been unable to find any other docs.

> 
> > +		compatible = "i2c-gpio";
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&pinctrl_i2cgpio>;
> > +		sda-gpios = <&gpio5 5 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
> > +		scl-gpios = <&gpio5 4 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +		status = "okay";
> 
> Why do you add status? Isn't this a new node?

That was my mistake, Rob pointed it out in v1 and I forgot to remove it.

> 
> > +
> > +	pinctrl_i2cgpio: i2cgrpgpio {
> 
> Name not matching schema, as they must end with grp. Derive your board
> from something new, not ancient...
> > +
> > +	pinctrl_usdhc1_100mhz: usdhc1grp100mhz {
> 
> Same.
> 
> > 
> > +
> > +	pinctrl_usdhc1_200mhz: usdhc1grp200mhz {
> 
> No really...
> 

Thanks for pointing this out, I was unable to find any specific docs on the
pinctrl node name schema and dtbs_check gave no errors on it.

> > 
> > +};
> > diff --git a/arch/arm/configs/ts7970_defconfig b/arch/arm/configs/ts7970_defconfig
> > new file mode 100644
> > index 000000000000..a96831752449
> 
> Rest is not accepted as not explained/justified.
> 
> 
> Best regards,
> Krzysztof

Many thanks for the review.