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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 14/07/2022 23:26, Kris Bahnsen wrote:
> 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?

Run the command, you will see.

> 
>>
>>>
>>> Signed-off-by: Kris Bahnsen <kris@embeddedTS.com>
>>> ---
>>>
>>> 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']:

Because you use "i2c-gpio", it seems... Fix it and check if error goes away.

>     {'#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.

Unfortunately this is an argument like - our customers always wanted
dead code in DTS, so we embed here such. Feel free to add a comment
about placeholder etc, but empty node not. Another issue is that empty
node without compatible also does not help your customers...

> 
(...)

> 
>>
>>> +
>>> +	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 know, I changed it. It's in linux-next.

> 
> 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.

For label - use underscores and that's it. Only the node names should be
generic.

> 

Best regards,
Krzysztof