linux-kernel - Re: [PATCH 03/11] dt-bindings: usb: Add binding for Cypress cypd4226 I2C driver

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y1vLoT+/dgOgrxjD@orome>
Date:   Fri, 28 Oct 2022 14:31:29 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Jon Hunter <jonathanh@...dia.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>
Cc:     Wayne Chang <waynec@...dia.com>, gregkh@...uxfoundation.org,
        treding@...dia.com, heikki.krogerus@...ux.intel.com,
        ajayg@...dia.com, kishon@...com, vkoul@...nel.org,
        p.zabel@...gutronix.de, balbi@...nel.org, mathias.nyman@...el.com,
        jckuo@...dia.com, linux-usb@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        singhanc@...dia.com, linux-i2c@...r.kernel.org,
        linux-phy@...ts.infradead.org, linux-tegra@...r.kernel.org
Subject: Re: [PATCH 03/11] dt-bindings: usb: Add binding for Cypress cypd4226
 I2C driver

On Wed, Oct 26, 2022 at 08:13:57AM +0100, Jon Hunter wrote:
> 
> On 24/10/2022 08:41, Wayne Chang wrote:
> > add device-tree binding documentation for Cypress cypd4226 type-C
> > controller's I2C interface. It is a standard i2c slave with GPIO
> > input as IRQ interface.
> > 
> > Signed-off-by: Wayne Chang <waynec@...dia.com>
> > ---
> >   .../bindings/usb/cypress,cypd4226.yaml        | 86 +++++++++++++++++++
> >   1 file changed, 86 insertions(+)
> >   create mode 100644 Documentation/devicetree/bindings/usb/cypress,cypd4226.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/cypress,cypd4226.yaml b/Documentation/devicetree/bindings/usb/cypress,cypd4226.yaml
> > new file mode 100644
> > index 000000000000..5ac28ab4e7a1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/cypress,cypd4226.yaml
> > @@ -0,0 +1,86 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/usb/cypress,cypd4226.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Cypress cypd4226 UCSI I2C Type-C Controller
> > +
> > +maintainers:
> > +  - Wayne Chang <waynec@...dia.com>
> > +
> > +description: |
> > +  The Cypress cypd4226 UCSI I2C type-C controller is a I2C interface type-C
> > +  controller.
> > +
> > +properties:
> > +  compatible:
> > +    const: cypress,cypd4226
> > +
> > +  '#address-cells':
> > +    const: 1
> > +
> > +  '#size-cells':
> > +    const: 0
> > +
> > +  reg:
> > +    const: 0x08
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  cypress,firmware-build:
> > +    enum:
> > +      - nv
> > +      - gn
> > +    description: |
> > +      the name of the CCGx firmware built for product series.
> > +      should be set one of following:
> > +      - "nv" for the RTX product series
> 
> Please add 'NVIDIA' so that it is 'for the NVIDIA RTX product series'
> 
> > +      - "gn" for the Jetson product series
> 
> Same here please add 'NVIDIA' so that it is 'for the NVIDIA Jetson product
> series'.
> 
> Rob, any concerns about this property in general? Unfortunately, ACPI choose
> a 16-bit type for this and used 'nv' for the RTX product. I don't find 'gn'
> for Jetson very descriptive but we need a way to differentiate from RTX.
> 
> This is needed in the Cypress CCGX driver for the following ...
> 
> https://lore.kernel.org/lkml/20220928150840.3804313-1-waynec@nvidia.com/
> 
> Ideally, this should have been included in this series but was sent before.
> We can always re-work/update the above patch even though it has been queued
> up now.

The driver seems to use this 16-bit value only to compare with a
corresponding field in the firmware headers. How exactly we obtain this
value is therefore not important. However, since this 16-bit value is
embedded in firmware images, we also cannot substitute them with
something more sensible.

However, I'm also a little confused as to the meaning of the property.
Looking at the driver, the fw_build field is used to check for
"supported vendors". "nv" and "gn" are clearly the same vendor (NVIDIA),
so that's at least not 100% accurate. The DT bindings say that this
denotes the product series, which seems to be more in line with how the
driver uses it.

The driver also uses it to implement changes in behavior across
different variants, which is something that we would typically describe
using compatible strings. So I wonder if we should, at least for device
tree, switch to using different compatible strings rather than this
separate matching mechanism. We could then associate a "product series"
with the compatible string rather than having this extra field.

There's also an argument to be made for keeping the interface the same
between ACPI and DT.

Rob, Krzysztof?

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)