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.
