Resending (because I didn't send it in PlainText mode, so the MLs blocked the email). Sorry. On Tue, Feb 11, 2020 at 11:00 AM Prashant Malani <pmalani@xxxxxxxxxxxx> wrote: > > Hi Enric, > > Thanks as always for reviewing the patch. Kindly see my responses inline: > > On Tue, Feb 11, 2020 at 2:28 AM Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx> wrote: >> >> Hi Prashant, >> >> On 7/2/20 21:37, Prashant Malani wrote: >> > Some Chrome OS devices with Embedded Controllers (EC) can read and >> > modify Type C port state. >> > >> > Add an entry in the DT Bindings documentation that lists out the logical >> > device and describes the relevant port information, to be used by the >> > corresponding driver. >> > >> > Signed-off-by: Prashant Malani <pmalani@xxxxxxxxxxxx> >> > --- >> > >> > Changes in v2: >> > - No changes. Patch first introduced in v2 of series. >> > >> > .../bindings/chrome/google,cros-ec-typec.yaml | 77 +++++++++++++++++++ >> > 1 file changed, 77 insertions(+) >> > create mode 100644 Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml >> > >> > diff --git a/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml >> > new file mode 100644 >> > index 00000000000000..46ebcbe76db3c2 >> > --- /dev/null >> > +++ b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml >> > @@ -0,0 +1,77 @@ >> > +# SPDX-License-Identifier: GPL-2.0 >> >> I think that Google is fine with the dual licensing here. Would be good if this >> can be (GPL-2.0-only OR BSD-2-Clause) > > > Thanks for catching this. I will update it in the next version. >> >> >> > +%YAML 1.2 >> > +--- >> > +$id: http://devicetree.org/schemas/chrome/google,cros-ec-typec.yaml# >> > +$schema: http://devicetree.org/meta-schemas/core.yaml# >> > + >> > +title: Google Chrome OS EC(Embedded Controller) Type C port driver. >> > + >> > +maintainers: >> > + - Benson Leung <bleung@xxxxxxxxxxxx> >> > + - Prashant Malani <pmalani@xxxxxxxxxxxx> >> > + >> > +description: >> > + Chrome OS devices have an Embedded Controller(EC) which has access to >> > + Type C port state. This node is intended to allow the host to read and >> > + control the Type C ports. The node for this device should be under a >> > + cros-ec node like google,cros-ec-spi. >> > + >> > +properties: >> > + compatible: >> > + const: google,cros-ec-typec >> > + >> > + port: >> > + description: A node that represents a physical Type C port on the >> > + device. >> > + type: object >> > + properties: >> > + port-number: >> > + description: The number used by the Chrome OS EC to identify >> > + this type C port. >> > + $ref: /schemas/types.yaml#/definitions/uint32 >> >> Any range of values allowed? 0 is okay? > > > 0 is acceptable. Looks like Chrome OS EC numbers the ports as 0 to (num_ports - 1). > Actually, looking at it more closely, the port info EC command struct uses uint8_t (https://elixir.bootlin.com/linux/latest/source/include/linux/platform_data/cros_ec_commands.h#L4879) > Also, num_ports cannot be larger than: https://elixir.bootlin.com/linux/latest/ident/EC_USB_PD_MAX_PORTS > > So perhaps this should be updated to say it can be any value from 0 - EC_USB_PD_MAX_PORTS ? (Not sure if I can reference #defines from header files in the DT bindings file....) >> >> >> > + power-role: >> >> Sorry if this question is silly, aren't this and below properties the same as >> provided by usb-connector? Can't this be usb-c-connector? >> >> Documentation/devicetree/bindings/connector/usb-connector.txt > > > That's correct, it is the same. I think there is a slight difference between the properties of usb-connector (what properties are defined as optional and required) so I don't know if we can re-use usb-connector. > TBH I wasn't sure about this myself. I am also not sure whether there will be an issue with usb-c-connector being "claimed" by another driver. I think Heikki could perhaps guide us here. >> >> >> > + description: Determines the power role that the Type C port will >> > + adopt. >> > + oneOf: >> > + - items: >> > + - const: sink >> > + - const: source >> > + - const: dual >> > + data-role: >> > + description: Determines the data role that the Type C port will >> > + adopt. >> > + oneOf: >> > + - items: >> > + - const: host >> > + - const: device >> > + - const: dual >> > + try-power-role: >> > + description: Determines the preferred power role of the Type C port. >> > + oneOf: >> > + - items: >> > + - const: sink >> > + - const: source >> > + - const: dual >> > + >> > + required: >> > + - port-number >> > + - power-role >> > + - data-role >> > + - try-power-role >> > + >> > +required: >> > + - compatible >> > + - port >> > + >> > +examples: >> > + - |+ >> >> Rob can confirm, but I think is a good practice add the parent node, so add the >> cros-ec-spi node here? > > Done. Will add it in the next version. >> >> >> > + typec { >> > + compatible = "google,cros-ec-typec"; >> > + >> > + port@0 { >> >> You can run: >> >> make dt_binding_check DT_SCHEMA_FILES=<...>/chrome/google,cros-ec-typec.yaml >> >> And you'll get an error: >> >> typec: 'port' is a required property > > Noted. I will run this check before pushing next time and fix the error. >> >> >> > + port-number = <0>; >> > + power-role = "dual"; >> > + data-role = "dual"; >> > + try-power-role = "source"; >> > + }; >> > + }; >> > >> Thanks, >> >> Enric > > > > Thanks, > > -Prashant
