From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> To: kris@embeddedTS.com, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> Cc: Shawn Guo <shawnguo@kernel.org>, Sascha Hauer <s.hauer@pengutronix.de>, Pengutronix Kernel Team <kernel@pengutronix.de>, Fabio Estevam <festevam@gmail.com>, NXP Linux Team <linux-imx@nxp.com>, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Mark Featherston <mark@embeddedTS.com> Subject: Re: [RFC PATCH v2] ARM: dts: Add TS-7553-V2 support Date: Fri, 15 Jul 2022 08:42:15 +0200 [thread overview] Message-ID: <eb993f8d-e72f-aba3-e7a4-1bbd2ac00f6c@linaro.org> (raw) In-Reply-To: <1657833995.2979.1.camel@embeddedTS.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> To: kris@embeddedTS.com, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> Cc: Shawn Guo <shawnguo@kernel.org>, Sascha Hauer <s.hauer@pengutronix.de>, Pengutronix Kernel Team <kernel@pengutronix.de>, Fabio Estevam <festevam@gmail.com>, NXP Linux Team <linux-imx@nxp.com>, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Mark Featherston <mark@embeddedTS.com> Subject: Re: [RFC PATCH v2] ARM: dts: Add TS-7553-V2 support Date: Fri, 15 Jul 2022 08:42:15 +0200 [thread overview] Message-ID: <eb993f8d-e72f-aba3-e7a4-1bbd2ac00f6c@linaro.org> (raw) In-Reply-To: <1657833995.2979.1.camel@embeddedTS.com> 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-07-15 6:42 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-07-13 22:12 [RFC PATCH v2] ARM: dts: Add TS-7553-V2 support Kris Bahnsen 2022-07-13 22:12 ` Kris Bahnsen 2022-07-14 8:34 ` Krzysztof Kozlowski 2022-07-14 8:34 ` Krzysztof Kozlowski 2022-07-14 21:26 ` Kris Bahnsen 2022-07-14 21:26 ` Kris Bahnsen 2022-07-15 6:42 ` Krzysztof Kozlowski [this message] 2022-07-15 6:42 ` Krzysztof Kozlowski 2022-07-15 17:54 ` Kris Bahnsen 2022-07-15 17:54 ` Kris Bahnsen 2022-07-18 12:49 ` Krzysztof Kozlowski 2022-07-18 12:49 ` Krzysztof Kozlowski 2022-07-19 17:39 ` Kris Bahnsen 2022-07-19 17:39 ` Kris Bahnsen 2022-07-19 19:06 ` Krzysztof Kozlowski 2022-07-19 19:06 ` Krzysztof Kozlowski
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=eb993f8d-e72f-aba3-e7a4-1bbd2ac00f6c@linaro.org \ --to=krzysztof.kozlowski@linaro.org \ --cc=devicetree@vger.kernel.org \ --cc=festevam@gmail.com \ --cc=kernel@pengutronix.de \ --cc=kris@embeddedTS.com \ --cc=krzysztof.kozlowski+dt@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-imx@nxp.com \ --cc=linux-kernel@vger.kernel.org \ --cc=mark@embeddedTS.com \ --cc=robh+dt@kernel.org \ --cc=s.hauer@pengutronix.de \ --cc=shawnguo@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.