| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <286633b2-43d2-655e-b3f1-54bf5c7a4a21@linaro.org>
Date: Tue, 28 Jun 2022 12:28:17 +0200
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Fabrice Gasnier <fabrice.gasnier@...s.st.com>, robh+dt@...nel.org,
heikki.krogerus@...ux.intel.com, gregkh@...uxfoundation.org
Cc: krzysztof.kozlowski+dt@...aro.org, devicetree@...r.kernel.org,
linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-stm32@...md-mailman.stormreply.com,
amelie.delaunay@...s.st.com, alexandre.torgue@...s.st.com
Subject: Re: [PATCH 1/4] dt-bindings: usb: typec: add bindings for stm32g0
controller
On 27/06/2022 16:21, Fabrice Gasnier wrote:
> On 6/24/22 18:16, Krzysztof Kozlowski wrote:
>> On 24/06/2022 17:54, Fabrice Gasnier wrote:
>>> This patch adds DT schema documentation for the STM32G0 Type-C controller.
>>
>> No "This patch"
>
> Hi Krzysztof,
>
> ack,
>
>>
>> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
>>
>>> STM32G0 provides an integrated USB Type-C and power delivery interface.
>>> It can be programmed with a firmware to handle UCSI protocol over I2C
>>> interface. A GPIO is used as an interrupt line.
>>> It may be used as a wakeup source, so use optional "wakeup-source" and
>>> "power-domains" properties to support wakeup.
>>>
>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@...s.st.com>
>>> ---
>>> .../bindings/usb/st,typec-stm32g0.yaml | 83 +++++++++++++++++++
>>> 1 file changed, 83 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml b/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml
>>> new file mode 100644
>>> index 0000000000000..b2729bd015a1a
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml
>>> @@ -0,0 +1,83 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: "http://devicetree.org/schemas/usb/st,typec-stm32g0.yaml#"
>>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
>>
>> No quotes.
>
> ack,
>
>>
>>> +
>>> +title: STMicroelectronics STM32G0 Type-C controller bindings
>>
>> s/bindings//
>
> ack,
>
>>
>>> +
>>> +description: |
>>> + The STM32G0 MCU can be programmed to control Type-C connector(s) through I2C
>>> + typically using the UCSI protocol over I2C, with a dedicated alert
>>> + (interrupt) pin.
>>> +
>>> +maintainers:
>>> + - Fabrice Gasnier <fabrice.gasnier@...s.st.com>
>>> +
>>> +properties:
>>> + compatible:
>>> + const: st,stm32g0-typec
>>> +
>>> + reg:
>>> + maxItems: 1
>>> +
>>> + interrupts:
>>> + maxItems: 1
>>> +
>>> + connector:
>>> + type: object> + allOf:
>>> + - $ref: ../connector/usb-connector.yaml#
>>
>> Full path, so /schemas/connector/...
>>
>> unevaluatedProperties: false
>
> ack,
>
>>
>>> +
>>> + firmware-name:
>>> + description: |
>>> + Should contain the name of the default firmware image
>>> + file located on the firmware search path
>>> +
>>> + wakeup-source: true
>>> + power-domains: true
>>
>> maxItems
>
> Do you mean maxItems regarding the "power-domains" property ?
Yes.
> This will depend on the user platform, where it's used as an I2C device.
> So I'm not sure this can / should be specified here.
> Could please you clarify ?
Then maybe this property is not valid here. Power domains usually are
used for blocks of a SoC, having common power source and power gating.
In your case it looks much more like a regulator supply.
>
>>
>>> +
>>> +required:
>>> + - compatible
>>> + - reg
>>> + - interrupts
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> + - |
>>> + #include <dt-bindings/interrupt-controller/irq.h>
>>> + i2c5 {
>>
>> Just "i2c"
>
> ack,
>
>>
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> +
>>> + stm32g0@53 {
>>
>> Generic node name describing class of the device.
>
>
> I wasn't aware of generic node name for an I2C device (not talking of
> the controller). I may have missed it.
>
> Could you please clarify ?
The class of a device is not a I2C device. I2C is just a bus. For
example the generic name for Power Management IC connected over I2C
(quite common case) is "pmic".
For USB HCD controllers the generic name is "usb". For USB
ports/connectors this is "connector". So what is your hardware?
"interface" is a bit too unspecific to figure it out.
>
>>
>>> + compatible = "st,stm32g0-typec";
>>> + reg = <0x53>;
>>> + /* Alert pin on GPIO PE12 */
>>> + interrupts = <12 IRQ_TYPE_EDGE_FALLING>;
>>> + interrupt-parent = <&gpioe>;
>>> +
>>> + /* Example with one type-C connector */
>>> + connector {
>>> + compatible = "usb-c-connector";
>>> + label = "USB-C";
>>> +
>>> + port {
>>
>> This does not look like proper schema of connector.yaml.
>
> This refers to graph.yaml [1], where similar example is seen [2].
>
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/graph.yaml#L79
>
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/graph.yaml#L207
Just look at the usb-conector schema. It's different. You miss ports.
Maybe other properties as well.
>
> device-1 {
> port {
> device_1_output: endpoint {
> remote-endpoint = <&device_2_input>;
> };
> };
> };
> device-2 {
> port {
> device_2_input: endpoint {
> remote-endpoint = <&device_1_output>;
> };
> };
> };
>
>
> Could you please clarify this point too ?
>
>>
>>> + con_usb_c_ep: endpoint {
>>> + remote-endpoint = <&usbotg_hs_ep>;
>>> + };
>>> + };
>>> + };
>>> + };
>>> + };
>>> +
>>> + usbotg_hs {
>>
>> Generic node names, no underscores in node names.
>
> ack, I guess you'd recommend "usb" here. I'll update it.
Yes, looks like usb.
Best regards,
Krzysztof
Powered by blists - more mailing lists