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@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']:
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
