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

[Jon Hunter]

On 28/10/2022 13:31, Thierry Reding wrote:
> 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@xxxxxxxxxx>
> > > ---
> > >    .../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@xxxxxxxxxx>
> > > +
> > > +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@xxxxxxxxxx/
> >
> > 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.

I am actually wondering if this is actually embedded in any images 
because I see it populated by the i2c-nvidia-gpu.c driver [0]. So I am 
wondering if we can use PROPERTY_ENTRY_STRING() for this driver instead 
and have a more descriptive name such as 'nvidia,rtx'?

Jon

[0] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/busses/i2c-nvidia-gpu.c#n261
--
nvpublic