Re: [PATCH v5 1/2] dt-bindings: usb: add analogix,anx7688.yaml

Re: [PATCH v5 1/2] dt-bindings: usb: add analogix,anx7688.yaml

[Dafna Hirschfeld]

Hi

On 05.03.21 15:34, Laurent Pinchart wrote:
> Hi Dafna,
> 
> Thank you for the patch.
> 
> On Fri, Mar 05, 2021 at 01:43:50PM +0100, Dafna Hirschfeld wrote:
>> ANX7688 is a USB Type-C port controller with a MUX. It converts HDMI 2.0 to
>> DisplayPort 1.3 Ultra-HDi (4096x2160p60).
>> The integrated crosspoint switch (the MUX) supports USB 3.1 data transfer
>> along with the DisplayPort Alternate Mode signaling over USB Type-C.
>> Additionally, an on-chip microcontroller (OCM) is available to manage the
>> signal switching, Channel Configuration (CC) detection, USB Power
>> Delivery (USB-PD), Vendor Defined Message (VDM) protocol support and other
>> functions as defined in the USB TypeC and USB Power Delivery
>> specifications.
>>
>> ANX7688 is found on Acer Chromebook R13 (elm) and on
>> Pine64 PinePhone.
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> ---
>>   .../bindings/usb/analogix,anx7688.yaml        | 177 ++++++++++++++++++
>>   1 file changed, 177 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/usb/analogix,anx7688.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/usb/analogix,anx7688.yaml b/Documentation/devicetree/bindings/usb/analogix,anx7688.yaml
>> new file mode 100644
>> index 000000000000..6c4dd6b4b28b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/usb/analogix,anx7688.yaml
>> @@ -0,0 +1,177 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/usb/analogix,anx7688.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Analogix ANX7688 Type-C Port Controller with HDMI to DP conversion
>> +
>> +maintainers:
>> +  - Nicolas Boichat <drinkcat@chromium.org>
>> +  - Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> +
>> +description: |
>> +  ANX7688 is a USB Type-C port controller with a MUX. It converts HDMI 2.0 to
>> +  DisplayPort 1.3 Ultra-HDi (4096x2160p60).
>> +  The integrated crosspoint switch (the MUX) supports USB 3.1 data transfer along with
>> +  the DisplayPort Alternate Mode signaling over USB Type-C. Additionally,
>> +  an on-chip microcontroller (OCM) is available to manage the signal switching,
>> +  Channel Configuration (CC) detection, USB Power Delivery (USB-PD), Vendor
>> +  Defined Message (VDM) protocol support and other functions as defined in the
>> +  USB TypeC and USB Power Delivery specifications.
>> +
>> +
> 
> Extra blank line ?
> 
>> +properties:
>> +  compatible:
>> +    const: analogix,anx7688
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  avdd33-supply:
>> +    description: 3.3V Analog core supply voltage.
>> +
>> +  dvdd18-supply:
>> +    description: 1.8V Digital I/O supply voltage.
>> +
>> +  avdd18-supply:
>> +    description: 1.8V Analog core power supply voltage.
>> +
>> +  avdd10-supply:
>> +    description: 1.0V Analog core power supply voltage.
>> +
>> +  dvdd10-supply:
>> +    description: 1.0V Digital core supply voltage.
>> +
> 
> That's lots of supplies. If there's a reasonable chance that some of
> them will always be driven by the same regulator (especially if the
> ANX7688 documentation requires that), then they could be grouped. For
> instance dvdd18-supply and avdd18-supply could be grouped into
> vdd18-supply. It would still allow us to extend the bindings in a
> backward compatible way later if a system uses different regulators. You
> have more information about the hardware than I do, so it's your call.
> 
>> +  hdmi5v-supply:
>> +    description: 5V power supply for the HDMI.
>> +
>> +  hdmi_vt-supply:
>> +    description: Termination voltage for HDMI input.
> 
> Maybe hdmi-vt-supply ?
> 
>> +
>> +  clocks:
>> +    description: The input clock specifier.
>> +    maxItems: 1
> 
> How about
> 
>      items:
>        - description: The input clock specifier.
> 
>> +
>> +  clock-names:
>> +    items:
>> +      - const: xtal
>> +
>> +  hpd-gpios:
>> +    description: |
>> +      In USB Type-C applications, DP_HPD has no use. In standard DisplayPort
>> +      applications, DP_HPD is used as DP hot-plug.
>> +    maxItems: 1
>> +
>> +  enable-gpios:
>> +    description: Chip power down control. No internal pull-down or pull-up resistor.
>> +    maxItems: 1
>> +
>> +  reset-gpios:
>> +    description: Reset input signal. Active low.
>> +    maxItems: 1
>> +
>> +  vbus-det-gpios:
>> +    description: |
>> +      An input gpio for VBUS detection and high voltage detection,
>> +      external resistance divide VBUS voltage to 1/8.
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    description: |
>> +      The interrupt notifies 4 possible events - TCPC ALERT int, PD int, DP int, HDMI int.
>> +    maxItems: 1
>> +
>> +  cabledet-gpios:
>> +    description: An output gpio, indicates by the device that a cable is plugged.
>> +    maxItems: 1
>> +
>> +  vbus-ctrl-gpios:
>> +    description:
>> +      External VBUS power path. Enable VBUS source and disable VBUS sink or vice versa.
>> +    maxItems: 1
>> +
>> +  vconn-en1-gpios:
>> +    description: Controls the VCONN switch on the CC1 pin.
>> +    maxItems: 1
>> +
>> +  vconn-en2-gpios:
>> +    description: Controls the VCONN switch on the CC2 pin.
>> +    maxItems: 1
>> +
>> +  ports:
>> +    $ref: /schemas/graph.yaml#/properties/ports
>> +
>> +    properties:
>> +      port@0:
>> +        $ref: /schemas/graph.yaml#/properties/port
>> +        description: Video port for HDMI input.
>> +
>> +      port@1:
>> +        $ref: /schemas/graph.yaml#/properties/port
>> +        description: USB port for the USB3 input.
>> +
>> +      port@2:
>> +        $ref: /schemas/graph.yaml#/properties/port
>> +        description: USB Type-c connector, see connector/usb-connector.yaml.
>> +
>> +    required:
>> +      - port@0
> 
> As all the ports exist at the hardware level, should they always be
> present ? The endpoints are optional of course, in case a port isn't
> connected on a particular system.
> 
>> +
>> +required:
>> +  - compatible
>> +  - reg
> 
> Shouldn't clocks and regulators be also required ?

hmm, theoretically yes. Practically, in the case of Acer R13 (mediatek elm) device,
the ANX7688 is powered up and controlled by the Embedded Controller. The kernel only
needs to read few registers from that device for the drm bridge driver.
(also mentioned that in the cover letter).

Thanks,
Dafna

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/gpio/gpio.h>
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +
>> +    i2c0 {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        anx7688: anx7688@2c {
>> +            compatible = "analogix,anx7688";
>> +            reg = <0x2c>;
>> +            avdd33-supply = <&reg_dcdc1>;
>> +            dvdd18-supply = <&reg_ldo_io1>;
>> +            avdd18-supply = <&reg_ldo_io1>;
>> +            avdd10-supply = <&reg_anx1v0>;
>> +            dvdd10-supply = <&reg_anx1v0>;
>> +            hdmi_vt-supply = <&reg_dldo1>;
>> +            enable-gpios = <&pio 3 10 GPIO_ACTIVE_LOW>; /* PD10 */
>> +            reset-gpios = <&pio 3 6 GPIO_ACTIVE_HIGH>; /* PD6 */
>> +            interrupt-parent = <&r_pio>;
>> +            interrupts = <0 11 IRQ_TYPE_EDGE_FALLING>; /* PL11 */
>> +            cabledet-gpios = <&r_pio 0 8 GPIO_ACTIVE_HIGH>; /* PL8 */
>> +            vconn-en1-gpios = <&pio 3 9 GPIO_ACTIVE_LOW>; /* PD9 */
>> +            vconn-en2-gpios = <&pio 3 9 GPIO_ACTIVE_LOW>; /* PD9 */
>> +            ports {
>> +                #address-cells = <1>;
>> +                #size-cells = <0>;
>> +
>> +                port@0 {
>> +                    reg = <0>;
>> +                    anx7688_in0: endpoint {
>> +                        remote-endpoint = <&hdmi0_out>;
>> +                    };
>> +                };
>> +
>> +                port@1 {
>> +                    reg = <1>;
>> +                    anx7688_in1: endpoint {
>> +                        remote-endpoint = <&usbdrd_phy_ss>;
>> +                    };
>> +                };
>> +                port@2 {
>> +                    reg = <2>;
>> +                    anx7688_out: endpoint {
>> +                        remote-endpoint = <&typec_connector>;
>> +                    };
>> +                };
>> +            };
>> +        };
>> +    };
> 

Re: [PATCH v5 2/2] drm/bridge: anx7688: Add ANX7688 bridge driver support

[Dafna Hirschfeld]

Adding megous@megous.com and linux-usb@vger.kernel.org to the list

Thanks,
Dafna

On 05.03.21 13:43, Dafna Hirschfeld wrote:
> From: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> 
> This driver adds support for the ANX7688 HDMI to DP converter block of the
> ANX7688 device.
> 
> For our use case, the only reason the Linux kernel driver is necessary is
> to reject resolutions that require more bandwidth than what is available
> on the DP side. DP bandwidth and lane count are reported by the bridge via
> 2 registers and, as far as we know, only chips that have a firmware
> version greater than 0.85 support these two registers.
> 
> Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> [The driver is OF only so should depends on CONFIG_OF]
> Reported-by: kbuild test robot <lkp@intel.com>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> [convert the driver to be a i2c driver]
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>   drivers/gpu/drm/bridge/analogix/Kconfig       |  11 ++
>   drivers/gpu/drm/bridge/analogix/Makefile      |   1 +
>   .../drm/bridge/analogix/analogix-anx7688.c    | 186 ++++++++++++++++++
>   3 files changed, 198 insertions(+)
>   create mode 100644 drivers/gpu/drm/bridge/analogix/analogix-anx7688.c
> 
> diff --git a/drivers/gpu/drm/bridge/analogix/Kconfig b/drivers/gpu/drm/bridge/analogix/Kconfig
> index 024ea2a570e7..323327aabc38 100644
> --- a/drivers/gpu/drm/bridge/analogix/Kconfig
> +++ b/drivers/gpu/drm/bridge/analogix/Kconfig
> @@ -11,6 +11,17 @@ config DRM_ANALOGIX_ANX6345
>   	  ANX6345 transforms the LVTTL RGB output of an
>   	  application processor to eDP or DisplayPort.
>   
> +config DRM_ANALOGIX_ANX7688
> +	tristate "Analogix ANX7688 bridge"
> +	depends on OF
> +	select DRM_KMS_HELPER
> +	select REGMAP_I2C
> +	help
> +	  ANX7688 is an ultra-low power 4K Ultra-HD (4096x2160p60)
> +	  mobile HD transmitter designed for portable
> +	  devices. The ANX7688 converts HDMI 2.0 to
> +	  DisplayPort 1.3 Ultra-HD.
> +
>   config DRM_ANALOGIX_ANX78XX
>   	tristate "Analogix ANX78XX bridge"
>   	select DRM_ANALOGIX_DP
> diff --git a/drivers/gpu/drm/bridge/analogix/Makefile b/drivers/gpu/drm/bridge/analogix/Makefile
> index 44da392bb9f9..8f2272b8b732 100644
> --- a/drivers/gpu/drm/bridge/analogix/Makefile
> +++ b/drivers/gpu/drm/bridge/analogix/Makefile
> @@ -2,5 +2,6 @@
>   analogix_dp-objs := analogix_dp_core.o analogix_dp_reg.o analogix-i2c-dptx.o
>   obj-$(CONFIG_DRM_ANALOGIX_ANX6345) += analogix-anx6345.o
>   obj-$(CONFIG_DRM_ANALOGIX_ANX7625) += anx7625.o
> +obj-$(CONFIG_DRM_ANALOGIX_ANX7688) += analogix-anx7688.o
>   obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
>   obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix_dp.o
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx7688.c b/drivers/gpu/drm/bridge/analogix/analogix-anx7688.c
> new file mode 100644
> index 000000000000..85a4b1a23035
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/analogix/analogix-anx7688.c
> @@ -0,0 +1,186 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * ANX7688 HDMI->DP bridge driver
> + *
> + * Copyright 2020 Google LLC
> + */
> +
> +#include <linux/types.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_print.h>
> +
> +/* Register addresses */
> +#define ANX7688_VENDOR_ID_REG		0x00
> +#define ANX7688_DEVICE_ID_REG		0x02
> +
> +#define ANX7688_FW_VERSION_REG		0x80
> +
> +#define ANX7688_DP_BANDWIDTH_REG	0x85
> +#define ANX7688_DP_LANE_COUNT_REG	0x86
> +
> +#define ANX7688_VENDOR_ID		0x1f29
> +#define ANX7688_DEVICE_ID		0x7688
> +
> +/* First supported firmware version (0.85) */
> +#define ANX7688_MINIMUM_FW_VERSION	0x0085
> +
> +static const struct regmap_config anx7688_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +};
> +
> +struct anx7688 {
> +	struct i2c_client *client;
> +	struct regmap *regmap;
> +	struct drm_bridge bridge;
> +	bool filter;
> +};
> +
> +static inline struct anx7688 *
> +bridge_to_anx7688(struct drm_bridge *bridge)
> +{
> +	return container_of(bridge, struct anx7688, bridge);
> +}
> +
> +static bool anx7688_bridge_mode_fixup(struct drm_bridge *bridge,
> +				      const struct drm_display_mode *mode,
> +				      struct drm_display_mode *adjusted_mode)
> +{
> +	struct anx7688 *anx = bridge_to_anx7688(bridge);
> +	int totalbw, requiredbw;
> +	u8 dpbw, lanecount;
> +	u8 regs[2];
> +	int ret;
> +
> +	if (!anx->filter)
> +		return true;
> +
> +	/* Read both regs 0x85 (bandwidth) and 0x86 (lane count). */
> +	ret = regmap_bulk_read(anx->regmap, ANX7688_DP_BANDWIDTH_REG, regs, 2);
> +	if (ret < 0) {
> +		DRM_ERROR("Failed to read bandwidth/lane count\n");
> +		return false;
> +	}
> +	dpbw = regs[0];
> +	lanecount = regs[1];
> +
> +	/* Maximum 0x19 bandwidth (6.75 Gbps Turbo mode), 2 lanes */
> +	if (dpbw > 0x19 || lanecount > 2) {
> +		DRM_ERROR("Invalid bandwidth/lane count (%02x/%d)\n", dpbw,
> +			  lanecount);
> +		return false;
> +	}
> +
> +	/* Compute available bandwidth (kHz) */
> +	totalbw = dpbw * lanecount * 270000 * 8 / 10;
> +
> +	/* Required bandwidth (8 bpc, kHz) */
> +	requiredbw = mode->clock * 8 * 3;
> +
> +	DRM_DEBUG_KMS("DP bandwidth: %d kHz (%02x/%d); mode requires %d Khz\n",
> +		      totalbw, dpbw, lanecount, requiredbw);
> +
> +	if (totalbw == 0) {
> +		DRM_ERROR("Bandwidth/lane count are 0, not rejecting modes\n");
> +		return true;
> +	}
> +
> +	return totalbw >= requiredbw;
> +}
> +
> +static const struct drm_bridge_funcs anx7688_bridge_funcs = {
> +	.mode_fixup = anx7688_bridge_mode_fixup,
> +};
> +
> +static int anx7688_bridge_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct anx7688 *anx7688;
> +	u16 vendor, device, fw_version;
> +	u8 buffer[4];
> +	int ret;
> +
> +	anx7688 = devm_kzalloc(dev, sizeof(*anx7688), GFP_KERNEL);
> +	if (!anx7688)
> +		return -ENOMEM;
> +
> +	anx7688->client = client;
> +	i2c_set_clientdata(client, anx7688);
> +
> +	anx7688->regmap = devm_regmap_init_i2c(client, &anx7688_regmap_config);
> +
> +	/* Read both vendor and device id (4 bytes). */
> +	ret = regmap_bulk_read(anx7688->regmap, ANX7688_VENDOR_ID_REG,
> +			       buffer, 4);
> +	if (ret) {
> +		dev_err(dev, "Failed to read chip vendor/device id\n");
> +		return ret;
> +	}
> +
> +	vendor = (u16)buffer[1] << 8 | buffer[0];
> +	device = (u16)buffer[3] << 8 | buffer[2];
> +	if (vendor != ANX7688_VENDOR_ID || device != ANX7688_DEVICE_ID) {
> +		dev_err(dev, "Invalid vendor/device id %04x/%04x\n",
> +			vendor, device);
> +		return -ENODEV;
> +	}
> +
> +	ret = regmap_bulk_read(anx7688->regmap, ANX7688_FW_VERSION_REG,
> +			       buffer, 2);
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to read firmware version\n");
> +		return ret;
> +	}
> +
> +	fw_version = (u16)buffer[0] << 8 | buffer[1];
> +	dev_info(dev, "ANX7688 firmware version 0x%04x\n", fw_version);
> +
> +	anx7688->bridge.of_node = dev->of_node;
> +
> +	/* FW version >= 0.85 supports bandwidth/lane count registers */
> +	if (fw_version >= ANX7688_MINIMUM_FW_VERSION)
> +		anx7688->filter = true;
> +	else
> +		/* Warn, but not fail, for backwards compatibility */
> +		DRM_WARN("Old ANX7688 FW version (0x%04x), not filtering\n",
> +			 fw_version);
> +
> +	anx7688->bridge.funcs = &anx7688_bridge_funcs;
> +	drm_bridge_add(&anx7688->bridge);
> +
> +	return 0;
> +}
> +
> +static int anx7688_bridge_remove(struct i2c_client *client)
> +{
> +	struct anx7688 *anx7688 = i2c_get_clientdata(client);
> +
> +	drm_bridge_remove(&anx7688->bridge);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id anx7688_bridge_match_table[] = {
> +	{ .compatible = "analogix,anx7688", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, anx7688_bridge_match_table);
> +
> +static struct i2c_driver anx7688_bridge_driver = {
> +	.probe_new = anx7688_bridge_probe,
> +	.remove = anx7688_bridge_remove,
> +	.driver = {
> +		.name = "anx7688-bridge",
> +		.of_match_table = anx7688_bridge_match_table,
> +	},
> +};
> +
> +module_i2c_driver(anx7688_bridge_driver);
> +
> +MODULE_DESCRIPTION("ANX7688 HDMI->DP bridge driver");
> +MODULE_AUTHOR("Nicolas Boichat <drinkcat@chromium.org>");
> +MODULE_AUTHOR("Enric Balletbo i Serra <enric.balletbo@collabora.com>");
> +MODULE_LICENSE("GPL");
> 

Re: [PATCH v5 1/2] dt-bindings: usb: add analogix,anx7688.yaml

[Laurent Pinchart]

Hi Dafna,

On Fri, Mar 05, 2021 at 04:14:03PM +0100, Dafna Hirschfeld wrote:
> On 05.03.21 15:34, Laurent Pinchart wrote:
> > On Fri, Mar 05, 2021 at 01:43:50PM +0100, Dafna Hirschfeld wrote:
> >> ANX7688 is a USB Type-C port controller with a MUX. It converts HDMI 2.0 to
> >> DisplayPort 1.3 Ultra-HDi (4096x2160p60).
> >> The integrated crosspoint switch (the MUX) supports USB 3.1 data transfer
> >> along with the DisplayPort Alternate Mode signaling over USB Type-C.
> >> Additionally, an on-chip microcontroller (OCM) is available to manage the
> >> signal switching, Channel Configuration (CC) detection, USB Power
> >> Delivery (USB-PD), Vendor Defined Message (VDM) protocol support and other
> >> functions as defined in the USB TypeC and USB Power Delivery
> >> specifications.
> >>
> >> ANX7688 is found on Acer Chromebook R13 (elm) and on
> >> Pine64 PinePhone.
> >>
> >> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> >> ---
> >>   .../bindings/usb/analogix,anx7688.yaml        | 177 ++++++++++++++++++
> >>   1 file changed, 177 insertions(+)
> >>   create mode 100644 Documentation/devicetree/bindings/usb/analogix,anx7688.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/usb/analogix,anx7688.yaml b/Documentation/devicetree/bindings/usb/analogix,anx7688.yaml
> >> new file mode 100644
> >> index 000000000000..6c4dd6b4b28b
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/usb/analogix,anx7688.yaml
> >> @@ -0,0 +1,177 @@
> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/usb/analogix,anx7688.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Analogix ANX7688 Type-C Port Controller with HDMI to DP conversion
> >> +
> >> +maintainers:
> >> +  - Nicolas Boichat <drinkcat@chromium.org>
> >> +  - Enric Balletbo i Serra <enric.balletbo@collabora.com>
> >> +
> >> +description: |
> >> +  ANX7688 is a USB Type-C port controller with a MUX. It converts HDMI 2.0 to
> >> +  DisplayPort 1.3 Ultra-HDi (4096x2160p60).
> >> +  The integrated crosspoint switch (the MUX) supports USB 3.1 data transfer along with
> >> +  the DisplayPort Alternate Mode signaling over USB Type-C. Additionally,
> >> +  an on-chip microcontroller (OCM) is available to manage the signal switching,
> >> +  Channel Configuration (CC) detection, USB Power Delivery (USB-PD), Vendor
> >> +  Defined Message (VDM) protocol support and other functions as defined in the
> >> +  USB TypeC and USB Power Delivery specifications.
> >> +
> >> +
> > 
> > Extra blank line ?
> > 
> >> +properties:
> >> +  compatible:
> >> +    const: analogix,anx7688
> >> +
> >> +  reg:
> >> +    maxItems: 1
> >> +
> >> +  avdd33-supply:
> >> +    description: 3.3V Analog core supply voltage.
> >> +
> >> +  dvdd18-supply:
> >> +    description: 1.8V Digital I/O supply voltage.
> >> +
> >> +  avdd18-supply:
> >> +    description: 1.8V Analog core power supply voltage.
> >> +
> >> +  avdd10-supply:
> >> +    description: 1.0V Analog core power supply voltage.
> >> +
> >> +  dvdd10-supply:
> >> +    description: 1.0V Digital core supply voltage.
> >> +
> > 
> > That's lots of supplies. If there's a reasonable chance that some of
> > them will always be driven by the same regulator (especially if the
> > ANX7688 documentation requires that), then they could be grouped. For
> > instance dvdd18-supply and avdd18-supply could be grouped into
> > vdd18-supply. It would still allow us to extend the bindings in a
> > backward compatible way later if a system uses different regulators. You
> > have more information about the hardware than I do, so it's your call.
> > 
> >> +  hdmi5v-supply:
> >> +    description: 5V power supply for the HDMI.
> >> +
> >> +  hdmi_vt-supply:
> >> +    description: Termination voltage for HDMI input.
> > 
> > Maybe hdmi-vt-supply ?
> > 
> >> +
> >> +  clocks:
> >> +    description: The input clock specifier.
> >> +    maxItems: 1
> > 
> > How about
> > 
> >      items:
> >        - description: The input clock specifier.
> > 
> >> +
> >> +  clock-names:
> >> +    items:
> >> +      - const: xtal
> >> +
> >> +  hpd-gpios:
> >> +    description: |
> >> +      In USB Type-C applications, DP_HPD has no use. In standard DisplayPort
> >> +      applications, DP_HPD is used as DP hot-plug.
> >> +    maxItems: 1
> >> +
> >> +  enable-gpios:
> >> +    description: Chip power down control. No internal pull-down or pull-up resistor.
> >> +    maxItems: 1
> >> +
> >> +  reset-gpios:
> >> +    description: Reset input signal. Active low.
> >> +    maxItems: 1
> >> +
> >> +  vbus-det-gpios:
> >> +    description: |
> >> +      An input gpio for VBUS detection and high voltage detection,
> >> +      external resistance divide VBUS voltage to 1/8.
> >> +    maxItems: 1
> >> +
> >> +  interrupts:
> >> +    description: |
> >> +      The interrupt notifies 4 possible events - TCPC ALERT int, PD int, DP int, HDMI int.
> >> +    maxItems: 1
> >> +
> >> +  cabledet-gpios:
> >> +    description: An output gpio, indicates by the device that a cable is plugged.
> >> +    maxItems: 1
> >> +
> >> +  vbus-ctrl-gpios:
> >> +    description:
> >> +      External VBUS power path. Enable VBUS source and disable VBUS sink or vice versa.
> >> +    maxItems: 1
> >> +
> >> +  vconn-en1-gpios:
> >> +    description: Controls the VCONN switch on the CC1 pin.
> >> +    maxItems: 1
> >> +
> >> +  vconn-en2-gpios:
> >> +    description: Controls the VCONN switch on the CC2 pin.
> >> +    maxItems: 1
> >> +
> >> +  ports:
> >> +    $ref: /schemas/graph.yaml#/properties/ports
> >> +
> >> +    properties:
> >> +      port@0:
> >> +        $ref: /schemas/graph.yaml#/properties/port
> >> +        description: Video port for HDMI input.
> >> +
> >> +      port@1:
> >> +        $ref: /schemas/graph.yaml#/properties/port
> >> +        description: USB port for the USB3 input.
> >> +
> >> +      port@2:
> >> +        $ref: /schemas/graph.yaml#/properties/port
> >> +        description: USB Type-c connector, see connector/usb-connector.yaml.
> >> +
> >> +    required:
> >> +      - port@0
> > 
> > As all the ports exist at the hardware level, should they always be
> > present ? The endpoints are optional of course, in case a port isn't
> > connected on a particular system.
> > 
> >> +
> >> +required:
> >> +  - compatible
> >> +  - reg
> > 
> > Shouldn't clocks and regulators be also required ?
> 
> hmm, theoretically yes. Practically, in the case of Acer R13 (mediatek elm) device,
> the ANX7688 is powered up and controlled by the Embedded Controller. The kernel only
> needs to read few registers from that device for the drm bridge driver.
> (also mentioned that in the cover letter).

This may not be a popular opinion, but if the ANX7688 is fully
controlled by the EC, I wonder if we shouldn't have an "EC DRM bridge"
driver that would interrogate the EC instead :-)

Is there a risk of bus conflicts if the EC and the main SoC try to
access the ANX7688 over I2C at the same time ?

> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> >> +
> >> +additionalProperties: false
> >> +
> >> +examples:
> >> +  - |
> >> +    #include <dt-bindings/gpio/gpio.h>
> >> +    #include <dt-bindings/interrupt-controller/irq.h>
> >> +
> >> +    i2c0 {
> >> +        #address-cells = <1>;
> >> +        #size-cells = <0>;
> >> +
> >> +        anx7688: anx7688@2c {
> >> +            compatible = "analogix,anx7688";
> >> +            reg = <0x2c>;
> >> +            avdd33-supply = <&reg_dcdc1>;
> >> +            dvdd18-supply = <&reg_ldo_io1>;
> >> +            avdd18-supply = <&reg_ldo_io1>;
> >> +            avdd10-supply = <&reg_anx1v0>;
> >> +            dvdd10-supply = <&reg_anx1v0>;
> >> +            hdmi_vt-supply = <&reg_dldo1>;
> >> +            enable-gpios = <&pio 3 10 GPIO_ACTIVE_LOW>; /* PD10 */
> >> +            reset-gpios = <&pio 3 6 GPIO_ACTIVE_HIGH>; /* PD6 */
> >> +            interrupt-parent = <&r_pio>;
> >> +            interrupts = <0 11 IRQ_TYPE_EDGE_FALLING>; /* PL11 */
> >> +            cabledet-gpios = <&r_pio 0 8 GPIO_ACTIVE_HIGH>; /* PL8 */
> >> +            vconn-en1-gpios = <&pio 3 9 GPIO_ACTIVE_LOW>; /* PD9 */
> >> +            vconn-en2-gpios = <&pio 3 9 GPIO_ACTIVE_LOW>; /* PD9 */
> >> +            ports {
> >> +                #address-cells = <1>;
> >> +                #size-cells = <0>;
> >> +
> >> +                port@0 {
> >> +                    reg = <0>;
> >> +                    anx7688_in0: endpoint {
> >> +                        remote-endpoint = <&hdmi0_out>;
> >> +                    };
> >> +                };
> >> +
> >> +                port@1 {
> >> +                    reg = <1>;
> >> +                    anx7688_in1: endpoint {
> >> +                        remote-endpoint = <&usbdrd_phy_ss>;
> >> +                    };
> >> +                };
> >> +                port@2 {
> >> +                    reg = <2>;
> >> +                    anx7688_out: endpoint {
> >> +                        remote-endpoint = <&typec_connector>;
> >> +                    };
> >> +                };
> >> +            };
> >> +        };
> >> +    };

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 1/2] dt-bindings: usb: add analogix,anx7688.yaml

[Ondřej Jirman]

Hello Dafna,

On Fri, Mar 05, 2021 at 04:14:03PM +0100, Dafna Hirschfeld wrote:
> Hi
> 
> On 05.03.21 15:34, Laurent Pinchart wrote:
> > Hi Dafna,
> > 
> > Thank you for the patch.
> > 
> > On Fri, Mar 05, 2021 at 01:43:50PM +0100, Dafna Hirschfeld wrote:
> > > ANX7688 is a USB Type-C port controller with a MUX. It converts HDMI 2.0 to
> > > DisplayPort 1.3 Ultra-HDi (4096x2160p60).
> > > The integrated crosspoint switch (the MUX) supports USB 3.1 data transfer
> > > along with the DisplayPort Alternate Mode signaling over USB Type-C.
> > > Additionally, an on-chip microcontroller (OCM) is available to manage the
> > > signal switching, Channel Configuration (CC) detection, USB Power
> > > Delivery (USB-PD), Vendor Defined Message (VDM) protocol support and other
> > > functions as defined in the USB TypeC and USB Power Delivery
> > > specifications.
> > > 
> > > ANX7688 is found on Acer Chromebook R13 (elm) and on
> > > Pine64 PinePhone.

Thanks for your work on the bindings. :) It would be great to find something
acceptable for mainlining.

A few comments based on my experience implementing the USB-PD part for PinePhone
are bellow.

> > > +properties:
> > > +  compatible:
> > > +    const: analogix,anx7688
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  avdd33-supply:
> > > +    description: 3.3V Analog core supply voltage.
> > > +
> > > +  dvdd18-supply:
> > > +    description: 1.8V Digital I/O supply voltage.
> > > +
> > > +  avdd18-supply:
> > > +    description: 1.8V Analog core power supply voltage.
> > > +
> > > +  avdd10-supply:
> > > +    description: 1.0V Analog core power supply voltage.
> > > +
> > > +  dvdd10-supply:
> > > +    description: 1.0V Digital core supply voltage.
> > > +
> > 
> > That's lots of supplies. If there's a reasonable chance that some of
> > them will always be driven by the same regulator (especially if the
> > ANX7688 documentation requires that), then they could be grouped. For
> > instance dvdd18-supply and avdd18-supply could be grouped into
> > vdd18-supply. It would still allow us to extend the bindings in a
> > backward compatible way later if a system uses different regulators. You
> > have more information about the hardware than I do, so it's your call.

On PinePhone, AVDD and DVDD for 1.0V are just separately fitlered outputs
from the same regulator. So it would work there to just use vdd18 and
vdd10. The same is true for reference design, so it's probably safe
to assume this can be simplified.

> > > +  hdmi5v-supply:
> > > +    description: 5V power supply for the HDMI.
> > > +
> > > +  hdmi_vt-supply:
> > > +    description: Termination voltage for HDMI input.
> > 
> > Maybe hdmi-vt-supply ?
> > 
> > > +
> > > +  clocks:
> > > +    description: The input clock specifier.
> > > +    maxItems: 1
> > 
> > How about
> > 
> >      items:
> >        - description: The input clock specifier.
> > 
> > > +
> > > +  clock-names:
> > > +    items:
> > > +      - const: xtal
> > > +
> > > +  hpd-gpios:
> > > +    description: |
> > > +      In USB Type-C applications, DP_HPD has no use. In standard DisplayPort
> > > +      applications, DP_HPD is used as DP hot-plug.
> > > +    maxItems: 1

On PinePhone this is wired to a HDMI port on the SoC, and HPD is handled by the
sun4i HDMI DRM driver. Seems like HPD will be handled by HDMI controller on
many/all? other platforms too. Why have it here?

> > > +  enable-gpios:
> > > +    description: Chip power down control. No internal pull-down or pull-up resistor.
> > > +    maxItems: 1
> > > +
> > > +  reset-gpios:
> > > +    description: Reset input signal. Active low.
> > > +    maxItems: 1
> > > +
> > > +  vbus-det-gpios:
> > > +    description: |
> > > +      An input gpio for VBUS detection and high voltage detection,
> > > +      external resistance divide VBUS voltage to 1/8.
> > > +    maxItems: 1

Why have this in the bindings? It seems that this is handled internally by the
ANX7688 via OCM firmware. And it's not really gpio either, it's an analog input
with AD converter hooked to it internally.

> > > +  interrupts:
> > > +    description: |
> > > +      The interrupt notifies 4 possible events - TCPC ALERT int, PD int, DP int, HDMI int.
> > > +    maxItems: 1
> > > +
> > > +  cabledet-gpios:
> > > +    description: An output gpio, indicates by the device that a cable is plugged.
> > > +    maxItems: 1
> > > +
> > > +  vbus-ctrl-gpios:
> > > +    description:
> > > +      External VBUS power path. Enable VBUS source and disable VBUS sink or vice versa.
> > > +    maxItems: 1

VBUS control seems to be already modelled by the usb-connector bindings. Why
have this here?

> > > +  vconn-en1-gpios:
> > > +    description: Controls the VCONN switch on the CC1 pin.
> > > +    maxItems: 1
> > > +
> > > +  vconn-en2-gpios:
> > > +    description: Controls the VCONN switch on the CC2 pin.
> > > +    maxItems: 1

VCONN is a voltage regulator that can be enabled either on CC1 or CC2
pin, or disabled completely. This control is *partially* performed in reference
design directly by the OCM. OCM only decides which CC pin to switch
the VCONN regulator to, and informs the SoC when the base VCONN regulator
for the switches needs to be enabled.

So vconn-en1/2 gpios are irrelevant to the driver, but the driver needs
to control VCONN power supply somehow and defer control over it to the OCM.

I don't know how to express it in the bindings. Maybe a vconn-supply here
on the anx7688 node?

It may also be part of the usb-connector binding, but this is really when it
comes to anx7688 a parent regulator for the switches, and switches are not
controlled by this driver, just their parent regulator is.

Any ideas?

kind regards,
	o.

> > > +  ports:
> > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > +
> > > +    properties:
> > > +      port@0:
> > > +        $ref: /schemas/graph.yaml#/properties/port
> > > +        description: Video port for HDMI input.
> > > +
> > > +      port@1:
> > > +        $ref: /schemas/graph.yaml#/properties/port
> > > +        description: USB port for the USB3 input.
> > > +
> > > +      port@2:
> > > +        $ref: /schemas/graph.yaml#/properties/port
> > > +        description: USB Type-c connector, see connector/usb-connector.yaml.
> > > +
> > > +    required:
> > > +      - port@0
> > 
> > As all the ports exist at the hardware level, should they always be
> > present ? The endpoints are optional of course, in case a port isn't
> > connected on a particular system.
> > 
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > 
> > Shouldn't clocks and regulators be also required ?
> 
> hmm, theoretically yes. Practically, in the case of Acer R13 (mediatek elm) device,
> the ANX7688 is powered up and controlled by the Embedded Controller. The kernel only
> needs to read few registers from that device for the drm bridge driver.
> (also mentioned that in the cover letter).
> 
> Thanks,
> Dafna

Re: [PATCH v5 1/2] dt-bindings: usb: add analogix,anx7688.yaml

[Dafna Hirschfeld]

Hi,

On 05.03.21 16:19, Laurent Pinchart wrote:
> Hi Dafna,
> 
> On Fri, Mar 05, 2021 at 04:14:03PM +0100, Dafna Hirschfeld wrote:
>> On 05.03.21 15:34, Laurent Pinchart wrote:
>>> On Fri, Mar 05, 2021 at 01:43:50PM +0100, Dafna Hirschfeld wrote:
>>>> ANX7688 is a USB Type-C port controller with a MUX. It converts HDMI 2.0 to
>>>> DisplayPort 1.3 Ultra-HDi (4096x2160p60).
>>>> The integrated crosspoint switch (the MUX) supports USB 3.1 data transfer
>>>> along with the DisplayPort Alternate Mode signaling over USB Type-C.
>>>> Additionally, an on-chip microcontroller (OCM) is available to manage the
>>>> signal switching, Channel Configuration (CC) detection, USB Power
>>>> Delivery (USB-PD), Vendor Defined Message (VDM) protocol support and other
>>>> functions as defined in the USB TypeC and USB Power Delivery
>>>> specifications.
>>>>
>>>> ANX7688 is found on Acer Chromebook R13 (elm) and on
>>>> Pine64 PinePhone.
>>>>
>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>>> ---
>>>>    .../bindings/usb/analogix,anx7688.yaml        | 177 ++++++++++++++++++
>>>>    1 file changed, 177 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/usb/analogix,anx7688.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/usb/analogix,anx7688.yaml b/Documentation/devicetree/bindings/usb/analogix,anx7688.yaml
>>>> new file mode 100644
>>>> index 000000000000..6c4dd6b4b28b
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/usb/analogix,anx7688.yaml
>>>> @@ -0,0 +1,177 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/usb/analogix,anx7688.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Analogix ANX7688 Type-C Port Controller with HDMI to DP conversion
>>>> +
>>>> +maintainers:
>>>> +  - Nicolas Boichat <drinkcat@chromium.org>
>>>> +  - Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>>> +
>>>> +description: |
>>>> +  ANX7688 is a USB Type-C port controller with a MUX. It converts HDMI 2.0 to
>>>> +  DisplayPort 1.3 Ultra-HDi (4096x2160p60).
>>>> +  The integrated crosspoint switch (the MUX) supports USB 3.1 data transfer along with
>>>> +  the DisplayPort Alternate Mode signaling over USB Type-C. Additionally,
>>>> +  an on-chip microcontroller (OCM) is available to manage the signal switching,
>>>> +  Channel Configuration (CC) detection, USB Power Delivery (USB-PD), Vendor
>>>> +  Defined Message (VDM) protocol support and other functions as defined in the
>>>> +  USB TypeC and USB Power Delivery specifications.
>>>> +
>>>> +
>>>
>>> Extra blank line ?
>>>
>>>> +properties:
>>>> +  compatible:
>>>> +    const: analogix,anx7688
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  avdd33-supply:
>>>> +    description: 3.3V Analog core supply voltage.
>>>> +
>>>> +  dvdd18-supply:
>>>> +    description: 1.8V Digital I/O supply voltage.
>>>> +
>>>> +  avdd18-supply:
>>>> +    description: 1.8V Analog core power supply voltage.
>>>> +
>>>> +  avdd10-supply:
>>>> +    description: 1.0V Analog core power supply voltage.
>>>> +
>>>> +  dvdd10-supply:
>>>> +    description: 1.0V Digital core supply voltage.
>>>> +
>>>
>>> That's lots of supplies. If there's a reasonable chance that some of
>>> them will always be driven by the same regulator (especially if the
>>> ANX7688 documentation requires that), then they could be grouped. For
>>> instance dvdd18-supply and avdd18-supply could be grouped into
>>> vdd18-supply. It would still allow us to extend the bindings in a
>>> backward compatible way later if a system uses different regulators. You
>>> have more information about the hardware than I do, so it's your call.

Can you explain what do you mean by 'grouped' ?
Do you mean that instead of having two properties dvdd18-supply and avdd18-supply
I have only one property vdd18-supply?

>>>
>>>> +  hdmi5v-supply:
>>>> +    description: 5V power supply for the HDMI.
>>>> +
>>>> +  hdmi_vt-supply:
>>>> +    description: Termination voltage for HDMI input.
>>>
>>> Maybe hdmi-vt-supply ?
>>>
>>>> +
>>>> +  clocks:
>>>> +    description: The input clock specifier.
>>>> +    maxItems: 1
>>>
>>> How about
>>>
>>>       items:
>>>         - description: The input clock specifier.
>>>
>>>> +
>>>> +  clock-names:
>>>> +    items:
>>>> +      - const: xtal
>>>> +
>>>> +  hpd-gpios:
>>>> +    description: |
>>>> +      In USB Type-C applications, DP_HPD has no use. In standard DisplayPort
>>>> +      applications, DP_HPD is used as DP hot-plug.
>>>> +    maxItems: 1
>>>> +
>>>> +  enable-gpios:
>>>> +    description: Chip power down control. No internal pull-down or pull-up resistor.
>>>> +    maxItems: 1
>>>> +
>>>> +  reset-gpios:
>>>> +    description: Reset input signal. Active low.
>>>> +    maxItems: 1
>>>> +
>>>> +  vbus-det-gpios:
>>>> +    description: |
>>>> +      An input gpio for VBUS detection and high voltage detection,
>>>> +      external resistance divide VBUS voltage to 1/8.
>>>> +    maxItems: 1
>>>> +
>>>> +  interrupts:
>>>> +    description: |
>>>> +      The interrupt notifies 4 possible events - TCPC ALERT int, PD int, DP int, HDMI int.
>>>> +    maxItems: 1
>>>> +
>>>> +  cabledet-gpios:
>>>> +    description: An output gpio, indicates by the device that a cable is plugged.
>>>> +    maxItems: 1
>>>> +
>>>> +  vbus-ctrl-gpios:
>>>> +    description:
>>>> +      External VBUS power path. Enable VBUS source and disable VBUS sink or vice versa.
>>>> +    maxItems: 1
>>>> +
>>>> +  vconn-en1-gpios:
>>>> +    description: Controls the VCONN switch on the CC1 pin.
>>>> +    maxItems: 1
>>>> +
>>>> +  vconn-en2-gpios:
>>>> +    description: Controls the VCONN switch on the CC2 pin.
>>>> +    maxItems: 1
>>>> +
>>>> +  ports:
>>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>>> +
>>>> +    properties:
>>>> +      port@0:
>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>> +        description: Video port for HDMI input.
>>>> +
>>>> +      port@1:
>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>> +        description: USB port for the USB3 input.
>>>> +
>>>> +      port@2:
>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>> +        description: USB Type-c connector, see connector/usb-connector.yaml.
>>>> +
>>>> +    required:
>>>> +      - port@0
>>>
>>> As all the ports exist at the hardware level, should they always be
>>> present ? The endpoints are optional of course, in case a port isn't
>>> connected on a particular system.
>>>
>>>> +
>>>> +required:
>>>> +  - compatible
>>>> +  - reg
>>>
>>> Shouldn't clocks and regulators be also required ?
>>
>> hmm, theoretically yes. Practically, in the case of Acer R13 (mediatek elm) device,
>> the ANX7688 is powered up and controlled by the Embedded Controller. The kernel only
>> needs to read few registers from that device for the drm bridge driver.
>> (also mentioned that in the cover letter).
> 
> This may not be a popular opinion, but if the ANX7688 is fully
> controlled by the EC, I wonder if we shouldn't have an "EC DRM bridge"
> driver that would interrogate the EC instead :-)

But the driver in patch 2/2 do access the anx7688 device with I2C.
It does interrogate the EC.

> 
> Is there a risk of bus conflicts if the EC and the main SoC try to
> access the ANX7688 over I2C at the same time ?

The SoC access the anx7688 though something called 'i2c-tunnel' (see google,cros-ec-i2c-tunnel.yaml)
So the I2C tunnels though the EC (I don't know how this is really implemented to be honest).


Thanks,
Dafna

> 
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>
>>>> +
>>>> +additionalProperties: false
>>>> +
>>>> +examples:
>>>> +  - |
>>>> +    #include <dt-bindings/gpio/gpio.h>
>>>> +    #include <dt-bindings/interrupt-controller/irq.h>
>>>> +
>>>> +    i2c0 {
>>>> +        #address-cells = <1>;
>>>> +        #size-cells = <0>;
>>>> +
>>>> +        anx7688: anx7688@2c {
>>>> +            compatible = "analogix,anx7688";
>>>> +            reg = <0x2c>;
>>>> +            avdd33-supply = <&reg_dcdc1>;
>>>> +            dvdd18-supply = <&reg_ldo_io1>;
>>>> +            avdd18-supply = <&reg_ldo_io1>;
>>>> +            avdd10-supply = <&reg_anx1v0>;
>>>> +            dvdd10-supply = <&reg_anx1v0>;
>>>> +            hdmi_vt-supply = <&reg_dldo1>;
>>>> +            enable-gpios = <&pio 3 10 GPIO_ACTIVE_LOW>; /* PD10 */
>>>> +            reset-gpios = <&pio 3 6 GPIO_ACTIVE_HIGH>; /* PD6 */
>>>> +            interrupt-parent = <&r_pio>;
>>>> +            interrupts = <0 11 IRQ_TYPE_EDGE_FALLING>; /* PL11 */
>>>> +            cabledet-gpios = <&r_pio 0 8 GPIO_ACTIVE_HIGH>; /* PL8 */
>>>> +            vconn-en1-gpios = <&pio 3 9 GPIO_ACTIVE_LOW>; /* PD9 */
>>>> +            vconn-en2-gpios = <&pio 3 9 GPIO_ACTIVE_LOW>; /* PD9 */
>>>> +            ports {
>>>> +                #address-cells = <1>;
>>>> +                #size-cells = <0>;
>>>> +
>>>> +                port@0 {
>>>> +                    reg = <0>;
>>>> +                    anx7688_in0: endpoint {
>>>> +                        remote-endpoint = <&hdmi0_out>;
>>>> +                    };
>>>> +                };
>>>> +
>>>> +                port@1 {
>>>> +                    reg = <1>;
>>>> +                    anx7688_in1: endpoint {
>>>> +                        remote-endpoint = <&usbdrd_phy_ss>;
>>>> +                    };
>>>> +                };
>>>> +                port@2 {
>>>> +                    reg = <2>;
>>>> +                    anx7688_out: endpoint {
>>>> +                        remote-endpoint = <&typec_connector>;
>>>> +                    };
>>>> +                };
>>>> +            };
>>>> +        };
>>>> +    };
> 

Re: [PATCH v5 1/2] dt-bindings: usb: add analogix,anx7688.yaml

[Enric Balletbo i Serra]

Hi Dafna,

Many thanks to take this challenge and work on this.

On 30/3/21 15:35, Dafna Hirschfeld wrote:
> Hi,
> 
> On 05.03.21 16:19, Laurent Pinchart wrote:
>> Hi Dafna,
>>
>> On Fri, Mar 05, 2021 at 04:14:03PM +0100, Dafna Hirschfeld wrote:
>>> On 05.03.21 15:34, Laurent Pinchart wrote:
>>>> On Fri, Mar 05, 2021 at 01:43:50PM +0100, Dafna Hirschfeld wrote:
>>>>> ANX7688 is a USB Type-C port controller with a MUX. It converts HDMI 2.0 to
>>>>> DisplayPort 1.3 Ultra-HDi (4096x2160p60).
>>>>> The integrated crosspoint switch (the MUX) supports USB 3.1 data transfer
>>>>> along with the DisplayPort Alternate Mode signaling over USB Type-C.
>>>>> Additionally, an on-chip microcontroller (OCM) is available to manage the
>>>>> signal switching, Channel Configuration (CC) detection, USB Power
>>>>> Delivery (USB-PD), Vendor Defined Message (VDM) protocol support and other
>>>>> functions as defined in the USB TypeC and USB Power Delivery
>>>>> specifications.
>>>>>
>>>>> ANX7688 is found on Acer Chromebook R13 (elm) and on
>>>>> Pine64 PinePhone.
>>>>>
>>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>>>> ---
>>>>>    .../bindings/usb/analogix,anx7688.yaml        | 177 ++++++++++++++++++
>>>>>    1 file changed, 177 insertions(+)
>>>>>    create mode 100644
>>>>> Documentation/devicetree/bindings/usb/analogix,anx7688.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/usb/analogix,anx7688.yaml
>>>>> b/Documentation/devicetree/bindings/usb/analogix,anx7688.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..6c4dd6b4b28b
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/usb/analogix,anx7688.yaml
>>>>> @@ -0,0 +1,177 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/usb/analogix,anx7688.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Analogix ANX7688 Type-C Port Controller with HDMI to DP conversion
>>>>> +
>>>>> +maintainers:
>>>>> +  - Nicolas Boichat <drinkcat@chromium.org>
>>>>> +  - Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>>>> +
>>>>> +description: |
>>>>> +  ANX7688 is a USB Type-C port controller with a MUX. It converts HDMI 2.0 to
>>>>> +  DisplayPort 1.3 Ultra-HDi (4096x2160p60).
>>>>> +  The integrated crosspoint switch (the MUX) supports USB 3.1 data
>>>>> transfer along with
>>>>> +  the DisplayPort Alternate Mode signaling over USB Type-C. Additionally,
>>>>> +  an on-chip microcontroller (OCM) is available to manage the signal
>>>>> switching,
>>>>> +  Channel Configuration (CC) detection, USB Power Delivery (USB-PD), Vendor
>>>>> +  Defined Message (VDM) protocol support and other functions as defined in
>>>>> the
>>>>> +  USB TypeC and USB Power Delivery specifications.
>>>>> +
>>>>> +
>>>>
>>>> Extra blank line ?
>>>>
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: analogix,anx7688
>>>>> +
>>>>> +  reg:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  avdd33-supply:
>>>>> +    description: 3.3V Analog core supply voltage.
>>>>> +
>>>>> +  dvdd18-supply:
>>>>> +    description: 1.8V Digital I/O supply voltage.
>>>>> +
>>>>> +  avdd18-supply:
>>>>> +    description: 1.8V Analog core power supply voltage.
>>>>> +
>>>>> +  avdd10-supply:
>>>>> +    description: 1.0V Analog core power supply voltage.
>>>>> +
>>>>> +  dvdd10-supply:
>>>>> +    description: 1.0V Digital core supply voltage.
>>>>> +
>>>>
>>>> That's lots of supplies. If there's a reasonable chance that some of
>>>> them will always be driven by the same regulator (especially if the
>>>> ANX7688 documentation requires that), then they could be grouped. For
>>>> instance dvdd18-supply and avdd18-supply could be grouped into
>>>> vdd18-supply. It would still allow us to extend the bindings in a
>>>> backward compatible way later if a system uses different regulators. You
>>>> have more information about the hardware than I do, so it's your call.
> 
> Can you explain what do you mean by 'grouped' ?
> Do you mean that instead of having two properties dvdd18-supply and avdd18-supply
> I have only one property vdd18-supply?
> 

You can simplify all this with vdd33, vdd18 vdd10. For the Chromebook case all
the analogic and digital part are the same regulator just filtered. That's a
common configuration and if there is some hardware that needs it we can extend
later.

>>>>
>>>>> +  hdmi5v-supply:
>>>>> +    description: 5V power supply for the HDMI.
>>>>> +
>>>>> +  hdmi_vt-supply:
>>>>> +    description: Termination voltage for HDMI input.
>>>>
>>>> Maybe hdmi-vt-supply ?
>>>>
>>>>> +
>>>>> +  clocks:
>>>>> +    description: The input clock specifier.
>>>>> +    maxItems: 1
>>>>
>>>> How about
>>>>
>>>>       items:
>>>>         - description: The input clock specifier.
>>>>
>>>>> +
>>>>> +  clock-names:
>>>>> +    items:
>>>>> +      - const: xtal
>>>>> +
>>>>> +  hpd-gpios:
>>>>> +    description: |
>>>>> +      In USB Type-C applications, DP_HPD has no use. In standard DisplayPort
>>>>> +      applications, DP_HPD is used as DP hot-plug.
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  enable-gpios:
>>>>> +    description: Chip power down control. No internal pull-down or pull-up
>>>>> resistor.
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  reset-gpios:
>>>>> +    description: Reset input signal. Active low.
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  vbus-det-gpios:
>>>>> +    description: |
>>>>> +      An input gpio for VBUS detection and high voltage detection,
>>>>> +      external resistance divide VBUS voltage to 1/8.
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  interrupts:
>>>>> +    description: |
>>>>> +      The interrupt notifies 4 possible events - TCPC ALERT int, PD int,
>>>>> DP int, HDMI int.
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  cabledet-gpios:
>>>>> +    description: An output gpio, indicates by the device that a cable is
>>>>> plugged.
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  vbus-ctrl-gpios:
>>>>> +    description:
>>>>> +      External VBUS power path. Enable VBUS source and disable VBUS sink
>>>>> or vice versa.
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  vconn-en1-gpios:
>>>>> +    description: Controls the VCONN switch on the CC1 pin.
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  vconn-en2-gpios:
>>>>> +    description: Controls the VCONN switch on the CC2 pin.
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  ports:
>>>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>>>> +
>>>>> +    properties:
>>>>> +      port@0:
>>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>>> +        description: Video port for HDMI input.
>>>>> +
>>>>> +      port@1:
>>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>>> +        description: USB port for the USB3 input.
>>>>> +
>>>>> +      port@2:
>>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>>> +        description: USB Type-c connector, see connector/usb-connector.yaml.
>>>>> +
>>>>> +    required:
>>>>> +      - port@0
>>>>
>>>> As all the ports exist at the hardware level, should they always be
>>>> present ? The endpoints are optional of course, in case a port isn't
>>>> connected on a particular system.
>>>>
>>>>> +
>>>>> +required:
>>>>> +  - compatible
>>>>> +  - reg
>>>>
>>>> Shouldn't clocks and regulators be also required ?
>>>
>>> hmm, theoretically yes. Practically, in the case of Acer R13 (mediatek elm)
>>> device,
>>> the ANX7688 is powered up and controlled by the Embedded Controller. The
>>> kernel only
>>> needs to read few registers from that device for the drm bridge driver.
>>> (also mentioned that in the cover letter).

I think that for the Chromebook you can assume that these supplies are a
fixed-regulator that are always on.

>>
>> This may not be a popular opinion, but if the ANX7688 is fully
>> controlled by the EC, I wonder if we shouldn't have an "EC DRM bridge"
>> driver that would interrogate the EC instead :-)
> 

We can do this, and tbh will be more easy for us, but we were already asked to
do it generic, so ...


> But the driver in patch 2/2 do access the anx7688 device with I2C.
> It does interrogate the EC.
> 
>>
>> Is there a risk of bus conflicts if the EC and the main SoC try to
>> access the ANX7688 over I2C at the same time ?
> 

No, there is a kind of i2c tunnel but you don't talk directly with the ANX7688.
The EC exposes the anx bus control to the AP.


> The SoC access the anx7688 though something called 'i2c-tunnel' (see
> google,cros-ec-i2c-tunnel.yaml)
> So the I2C tunnels though the EC (I don't know how this is really implemented to
> be honest).
> 
> 
> Thanks,
> Dafna
> 
>>
>>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>
>>>>> +
>>>>> +additionalProperties: false
>>>>> +
>>>>> +examples:
>>>>> +  - |
>>>>> +    #include <dt-bindings/gpio/gpio.h>
>>>>> +    #include <dt-bindings/interrupt-controller/irq.h>
>>>>> +
>>>>> +    i2c0 {
>>>>> +        #address-cells = <1>;
>>>>> +        #size-cells = <0>;
>>>>> +
>>>>> +        anx7688: anx7688@2c {
>>>>> +            compatible = "analogix,anx7688";
>>>>> +            reg = <0x2c>;
>>>>> +            avdd33-supply = <&reg_dcdc1>;
>>>>> +            dvdd18-supply = <&reg_ldo_io1>;
>>>>> +            avdd18-supply = <&reg_ldo_io1>;
>>>>> +            avdd10-supply = <&reg_anx1v0>;
>>>>> +            dvdd10-supply = <&reg_anx1v0>;
>>>>> +            hdmi_vt-supply = <&reg_dldo1>;
>>>>> +            enable-gpios = <&pio 3 10 GPIO_ACTIVE_LOW>; /* PD10 */
>>>>> +            reset-gpios = <&pio 3 6 GPIO_ACTIVE_HIGH>; /* PD6 */
>>>>> +            interrupt-parent = <&r_pio>;
>>>>> +            interrupts = <0 11 IRQ_TYPE_EDGE_FALLING>; /* PL11 */
>>>>> +            cabledet-gpios = <&r_pio 0 8 GPIO_ACTIVE_HIGH>; /* PL8 */
>>>>> +            vconn-en1-gpios = <&pio 3 9 GPIO_ACTIVE_LOW>; /* PD9 */
>>>>> +            vconn-en2-gpios = <&pio 3 9 GPIO_ACTIVE_LOW>; /* PD9 */
>>>>> +            ports {
>>>>> +                #address-cells = <1>;
>>>>> +                #size-cells = <0>;
>>>>> +
>>>>> +                port@0 {
>>>>> +                    reg = <0>;
>>>>> +                    anx7688_in0: endpoint {
>>>>> +                        remote-endpoint = <&hdmi0_out>;
>>>>> +                    };
>>>>> +                };
>>>>> +
>>>>> +                port@1 {
>>>>> +                    reg = <1>;
>>>>> +                    anx7688_in1: endpoint {
>>>>> +                        remote-endpoint = <&usbdrd_phy_ss>;
>>>>> +                    };
>>>>> +                };
>>>>> +                port@2 {
>>>>> +                    reg = <2>;
>>>>> +                    anx7688_out: endpoint {
>>>>> +                        remote-endpoint = <&typec_connector>;
>>>>> +                    };
>>>>> +                };
>>>>> +            };
>>>>> +        };
>>>>> +    };
>>

Re: [PATCH v5 1/2] dt-bindings: usb: add analogix,anx7688.yaml

[Dafna Hirschfeld]

Hi,

On 05.03.21 18:24, Ondřej Jirman wrote:
> Hello Dafna,
> 
> On Fri, Mar 05, 2021 at 04:14:03PM +0100, Dafna Hirschfeld wrote:
>> Hi
>>
>> On 05.03.21 15:34, Laurent Pinchart wrote:
>>> Hi Dafna,
>>>
>>> Thank you for the patch.
>>>
>>> On Fri, Mar 05, 2021 at 01:43:50PM +0100, Dafna Hirschfeld wrote:
>>>> ANX7688 is a USB Type-C port controller with a MUX. It converts HDMI 2.0 to
>>>> DisplayPort 1.3 Ultra-HDi (4096x2160p60).
>>>> The integrated crosspoint switch (the MUX) supports USB 3.1 data transfer
>>>> along with the DisplayPort Alternate Mode signaling over USB Type-C.
>>>> Additionally, an on-chip microcontroller (OCM) is available to manage the
>>>> signal switching, Channel Configuration (CC) detection, USB Power
>>>> Delivery (USB-PD), Vendor Defined Message (VDM) protocol support and other
>>>> functions as defined in the USB TypeC and USB Power Delivery
>>>> specifications.
>>>>
>>>> ANX7688 is found on Acer Chromebook R13 (elm) and on
>>>> Pine64 PinePhone.
> 
> Thanks for your work on the bindings. :) It would be great to find something
> acceptable for mainlining.
> 
> A few comments based on my experience implementing the USB-PD part for PinePhone
> are bellow.
> 
>>>> +properties:
>>>> +  compatible:
>>>> +    const: analogix,anx7688
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  avdd33-supply:
>>>> +    description: 3.3V Analog core supply voltage.
>>>> +
>>>> +  dvdd18-supply:
>>>> +    description: 1.8V Digital I/O supply voltage.
>>>> +
>>>> +  avdd18-supply:
>>>> +    description: 1.8V Analog core power supply voltage.
>>>> +
>>>> +  avdd10-supply:
>>>> +    description: 1.0V Analog core power supply voltage.
>>>> +
>>>> +  dvdd10-supply:
>>>> +    description: 1.0V Digital core supply voltage.
>>>> +
>>>
>>> That's lots of supplies. If there's a reasonable chance that some of
>>> them will always be driven by the same regulator (especially if the
>>> ANX7688 documentation requires that), then they could be grouped. For
>>> instance dvdd18-supply and avdd18-supply could be grouped into
>>> vdd18-supply. It would still allow us to extend the bindings in a
>>> backward compatible way later if a system uses different regulators. You
>>> have more information about the hardware than I do, so it's your call.
> 
> On PinePhone, AVDD and DVDD for 1.0V are just separately fitlered outputs
> from the same regulator. So it would work there to just use vdd18 and
> vdd10. The same is true for reference design, so it's probably safe
> to assume this can be simplified.
> 
>>>> +  hdmi5v-supply:
>>>> +    description: 5V power supply for the HDMI.
>>>> +
>>>> +  hdmi_vt-supply:
>>>> +    description: Termination voltage for HDMI input.
>>>
>>> Maybe hdmi-vt-supply ?
>>>
>>>> +
>>>> +  clocks:
>>>> +    description: The input clock specifier.
>>>> +    maxItems: 1
>>>
>>> How about
>>>
>>>       items:
>>>         - description: The input clock specifier.
>>>
>>>> +
>>>> +  clock-names:
>>>> +    items:
>>>> +      - const: xtal
>>>> +
>>>> +  hpd-gpios:
>>>> +    description: |
>>>> +      In USB Type-C applications, DP_HPD has no use. In standard DisplayPort
>>>> +      applications, DP_HPD is used as DP hot-plug.
>>>> +    maxItems: 1
> 
> On PinePhone this is wired to a HDMI port on the SoC, and HPD is handled by the
> sun4i HDMI DRM driver. Seems like HPD will be handled by HDMI controller on
> many/all? other platforms too. Why have it here?

Right, I didn't have the full picture when listing all the pins of the anx7688.
I was not sure what should be listed.
I will remove that.

> 
>>>> +  enable-gpios:
>>>> +    description: Chip power down control. No internal pull-down or pull-up resistor.
>>>> +    maxItems: 1
>>>> +
>>>> +  reset-gpios:
>>>> +    description: Reset input signal. Active low.
>>>> +    maxItems: 1
>>>> +
>>>> +  vbus-det-gpios:
>>>> +    description: |
>>>> +      An input gpio for VBUS detection and high voltage detection,
>>>> +      external resistance divide VBUS voltage to 1/8.
>>>> +    maxItems: 1
> 
> Why have this in the bindings? It seems that this is handled internally by the
> ANX7688 via OCM firmware. And it's not really gpio either, it's an analog input
> with AD converter hooked to it internally.

I will remove that.

> 
>>>> +  interrupts:
>>>> +    description: |
>>>> +      The interrupt notifies 4 possible events - TCPC ALERT int, PD int, DP int, HDMI int.
>>>> +    maxItems: 1
>>>> +
>>>> +  cabledet-gpios:
>>>> +    description: An output gpio, indicates by the device that a cable is plugged.
>>>> +    maxItems: 1
>>>> +
>>>> +  vbus-ctrl-gpios:
>>>> +    description:
>>>> +      External VBUS power path. Enable VBUS source and disable VBUS sink or vice versa.
>>>> +    maxItems: 1
> 
> VBUS control seems to be already modelled by the usb-connector bindings. Why
> have this here?

dito
> 
>>>> +  vconn-en1-gpios:
>>>> +    description: Controls the VCONN switch on the CC1 pin.
>>>> +    maxItems: 1
>>>> +
>>>> +  vconn-en2-gpios:
>>>> +    description: Controls the VCONN switch on the CC2 pin.
>>>> +    maxItems: 1
> 
> VCONN is a voltage regulator that can be enabled either on CC1 or CC2
> pin, or disabled completely. This control is *partially* performed in reference
> design directly by the OCM. OCM only decides which CC pin to switch
> the VCONN regulator to, and informs the SoC when the base VCONN regulator
> for the switches needs to be enabled.
> 
> So vconn-en1/2 gpios are irrelevant to the driver, but the driver needs
> to control VCONN power supply somehow and defer control over it to the OCM.
> 
> I don't know how to express it in the bindings. Maybe a vconn-supply here
> on the anx7688 node?
> 
> It may also be part of the usb-connector binding, but this is really when it
> comes to anx7688 a parent regulator for the switches, and switches are not
> controlled by this driver, just their parent regulator is.
> 
> Any ideas?

Looking at the diagram in the schematics, I see that both vbus-supply and vconn-supply
come directly from the PMIC so similarly to the vbus-supply, the vconn-supply
can be part of the usb-connector. Then a driver can access the vconn-supply from the remote usb
connector. Is there a problem with that?

Thanks a lot for the review, I am not very familiar with usb and type-c, so your review helps a lot.
I will send v6 soon.

Thanks,
Dafna

> 
> kind regards,
> 	o.
> 
>>>> +  ports:
>>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>>> +
>>>> +    properties:
>>>> +      port@0:
>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>> +        description: Video port for HDMI input.
>>>> +
>>>> +      port@1:
>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>> +        description: USB port for the USB3 input.
>>>> +
>>>> +      port@2:
>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>> +        description: USB Type-c connector, see connector/usb-connector.yaml.
>>>> +
>>>> +    required:
>>>> +      - port@0
>>>
>>> As all the ports exist at the hardware level, should they always be
>>> present ? The endpoints are optional of course, in case a port isn't
>>> connected on a particular system.
>>>
>>>> +
>>>> +required:
>>>> +  - compatible
>>>> +  - reg
>>>
>>> Shouldn't clocks and regulators be also required ?
>>
>> hmm, theoretically yes. Practically, in the case of Acer R13 (mediatek elm) device,
>> the ANX7688 is powered up and controlled by the Embedded Controller. The kernel only
>> needs to read few registers from that device for the drm bridge driver.
>> (also mentioned that in the cover letter).
>>
>> Thanks,
>> Dafna

Re: [PATCH v5 1/2] dt-bindings: usb: add analogix,anx7688.yaml

[Laurent Pinchart]

On Tue, Mar 30, 2021 at 05:14:44PM +0200, Enric Balletbo i Serra wrote:
> On 30/3/21 15:35, Dafna Hirschfeld wrote:
> > On 05.03.21 16:19, Laurent Pinchart wrote:
> >> On Fri, Mar 05, 2021 at 04:14:03PM +0100, Dafna Hirschfeld wrote:
> >>> On 05.03.21 15:34, Laurent Pinchart wrote:
> >>>> On Fri, Mar 05, 2021 at 01:43:50PM +0100, Dafna Hirschfeld wrote:
> >>>>> ANX7688 is a USB Type-C port controller with a MUX. It converts HDMI 2.0 to
> >>>>> DisplayPort 1.3 Ultra-HDi (4096x2160p60).
> >>>>> The integrated crosspoint switch (the MUX) supports USB 3.1 data transfer
> >>>>> along with the DisplayPort Alternate Mode signaling over USB Type-C.
> >>>>> Additionally, an on-chip microcontroller (OCM) is available to manage the
> >>>>> signal switching, Channel Configuration (CC) detection, USB Power
> >>>>> Delivery (USB-PD), Vendor Defined Message (VDM) protocol support and other
> >>>>> functions as defined in the USB TypeC and USB Power Delivery
> >>>>> specifications.
> >>>>>
> >>>>> ANX7688 is found on Acer Chromebook R13 (elm) and on
> >>>>> Pine64 PinePhone.
> >>>>>
> >>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> >>>>> ---
> >>>>>    .../bindings/usb/analogix,anx7688.yaml        | 177 ++++++++++++++++++
> >>>>>    1 file changed, 177 insertions(+)
> >>>>>    create mode 100644
> >>>>> Documentation/devicetree/bindings/usb/analogix,anx7688.yaml
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/usb/analogix,anx7688.yaml
> >>>>> b/Documentation/devicetree/bindings/usb/analogix,anx7688.yaml
> >>>>> new file mode 100644
> >>>>> index 000000000000..6c4dd6b4b28b
> >>>>> --- /dev/null
> >>>>> +++ b/Documentation/devicetree/bindings/usb/analogix,anx7688.yaml
> >>>>> @@ -0,0 +1,177 @@
> >>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>>>> +%YAML 1.2
> >>>>> +---
> >>>>> +$id: http://devicetree.org/schemas/usb/analogix,anx7688.yaml#
> >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>> +
> >>>>> +title: Analogix ANX7688 Type-C Port Controller with HDMI to DP conversion
> >>>>> +
> >>>>> +maintainers:
> >>>>> +  - Nicolas Boichat <drinkcat@chromium.org>
> >>>>> +  - Enric Balletbo i Serra <enric.balletbo@collabora.com>
> >>>>> +
> >>>>> +description: |
> >>>>> +  ANX7688 is a USB Type-C port controller with a MUX. It converts HDMI 2.0 to
> >>>>> +  DisplayPort 1.3 Ultra-HDi (4096x2160p60).
> >>>>> +  The integrated crosspoint switch (the MUX) supports USB 3.1 data
> >>>>> transfer along with
> >>>>> +  the DisplayPort Alternate Mode signaling over USB Type-C. Additionally,
> >>>>> +  an on-chip microcontroller (OCM) is available to manage the signal
> >>>>> switching,
> >>>>> +  Channel Configuration (CC) detection, USB Power Delivery (USB-PD), Vendor
> >>>>> +  Defined Message (VDM) protocol support and other functions as defined in
> >>>>> the
> >>>>> +  USB TypeC and USB Power Delivery specifications.
> >>>>> +
> >>>>> +
> >>>>
> >>>> Extra blank line ?
> >>>>
> >>>>> +properties:
> >>>>> +  compatible:
> >>>>> +    const: analogix,anx7688
> >>>>> +
> >>>>> +  reg:
> >>>>> +    maxItems: 1
> >>>>> +
> >>>>> +  avdd33-supply:
> >>>>> +    description: 3.3V Analog core supply voltage.
> >>>>> +
> >>>>> +  dvdd18-supply:
> >>>>> +    description: 1.8V Digital I/O supply voltage.
> >>>>> +
> >>>>> +  avdd18-supply:
> >>>>> +    description: 1.8V Analog core power supply voltage.
> >>>>> +
> >>>>> +  avdd10-supply:
> >>>>> +    description: 1.0V Analog core power supply voltage.
> >>>>> +
> >>>>> +  dvdd10-supply:
> >>>>> +    description: 1.0V Digital core supply voltage.
> >>>>> +
> >>>>
> >>>> That's lots of supplies. If there's a reasonable chance that some of
> >>>> them will always be driven by the same regulator (especially if the
> >>>> ANX7688 documentation requires that), then they could be grouped. For
> >>>> instance dvdd18-supply and avdd18-supply could be grouped into
> >>>> vdd18-supply. It would still allow us to extend the bindings in a
> >>>> backward compatible way later if a system uses different regulators. You
> >>>> have more information about the hardware than I do, so it's your call.
> > 
> > Can you explain what do you mean by 'grouped' ?
> > Do you mean that instead of having two properties dvdd18-supply and avdd18-supply
> > I have only one property vdd18-supply?
> 
> You can simplify all this with vdd33, vdd18 vdd10. For the Chromebook case all
> the analogic and digital part are the same regulator just filtered. That's a
> common configuration and if there is some hardware that needs it we can extend
> later.

That's the idea, yes. If in a typical use case multiple supplies are
provided by a single regulator (for some devices that datasheet strongly
recommends that, or event mandates it), then it makes sense to group
those supplies in a single DT supply property. It can always be extended
later indeed, without any backward compatibility issue.

> >>>>> +  hdmi5v-supply:
> >>>>> +    description: 5V power supply for the HDMI.
> >>>>> +
> >>>>> +  hdmi_vt-supply:
> >>>>> +    description: Termination voltage for HDMI input.
> >>>>
> >>>> Maybe hdmi-vt-supply ?
> >>>>
> >>>>> +
> >>>>> +  clocks:
> >>>>> +    description: The input clock specifier.
> >>>>> +    maxItems: 1
> >>>>
> >>>> How about
> >>>>
> >>>>       items:
> >>>>         - description: The input clock specifier.
> >>>>
> >>>>> +
> >>>>> +  clock-names:
> >>>>> +    items:
> >>>>> +      - const: xtal
> >>>>> +
> >>>>> +  hpd-gpios:
> >>>>> +    description: |
> >>>>> +      In USB Type-C applications, DP_HPD has no use. In standard DisplayPort
> >>>>> +      applications, DP_HPD is used as DP hot-plug.
> >>>>> +    maxItems: 1
> >>>>> +
> >>>>> +  enable-gpios:
> >>>>> +    description: Chip power down control. No internal pull-down or pull-up
> >>>>> resistor.
> >>>>> +    maxItems: 1
> >>>>> +
> >>>>> +  reset-gpios:
> >>>>> +    description: Reset input signal. Active low.
> >>>>> +    maxItems: 1
> >>>>> +
> >>>>> +  vbus-det-gpios:
> >>>>> +    description: |
> >>>>> +      An input gpio for VBUS detection and high voltage detection,
> >>>>> +      external resistance divide VBUS voltage to 1/8.
> >>>>> +    maxItems: 1
> >>>>> +
> >>>>> +  interrupts:
> >>>>> +    description: |
> >>>>> +      The interrupt notifies 4 possible events - TCPC ALERT int, PD int,
> >>>>> DP int, HDMI int.
> >>>>> +    maxItems: 1
> >>>>> +
> >>>>> +  cabledet-gpios:
> >>>>> +    description: An output gpio, indicates by the device that a cable is
> >>>>> plugged.
> >>>>> +    maxItems: 1
> >>>>> +
> >>>>> +  vbus-ctrl-gpios:
> >>>>> +    description:
> >>>>> +      External VBUS power path. Enable VBUS source and disable VBUS sink
> >>>>> or vice versa.
> >>>>> +    maxItems: 1
> >>>>> +
> >>>>> +  vconn-en1-gpios:
> >>>>> +    description: Controls the VCONN switch on the CC1 pin.
> >>>>> +    maxItems: 1
> >>>>> +
> >>>>> +  vconn-en2-gpios:
> >>>>> +    description: Controls the VCONN switch on the CC2 pin.
> >>>>> +    maxItems: 1
> >>>>> +
> >>>>> +  ports:
> >>>>> +    $ref: /schemas/graph.yaml#/properties/ports
> >>>>> +
> >>>>> +    properties:
> >>>>> +      port@0:
> >>>>> +        $ref: /schemas/graph.yaml#/properties/port
> >>>>> +        description: Video port for HDMI input.
> >>>>> +
> >>>>> +      port@1:
> >>>>> +        $ref: /schemas/graph.yaml#/properties/port
> >>>>> +        description: USB port for the USB3 input.
> >>>>> +
> >>>>> +      port@2:
> >>>>> +        $ref: /schemas/graph.yaml#/properties/port
> >>>>> +        description: USB Type-c connector, see connector/usb-connector.yaml.
> >>>>> +
> >>>>> +    required:
> >>>>> +      - port@0
> >>>>
> >>>> As all the ports exist at the hardware level, should they always be
> >>>> present ? The endpoints are optional of course, in case a port isn't
> >>>> connected on a particular system.
> >>>>
> >>>>> +
> >>>>> +required:
> >>>>> +  - compatible
> >>>>> +  - reg
> >>>>
> >>>> Shouldn't clocks and regulators be also required ?
> >>>
> >>> hmm, theoretically yes. Practically, in the case of Acer R13 (mediatek elm) device,
> >>> the ANX7688 is powered up and controlled by the Embedded Controller. The kernel only
> >>> needs to read few registers from that device for the drm bridge driver.
> >>> (also mentioned that in the cover letter).
> 
> I think that for the Chromebook you can assume that these supplies are a
> fixed-regulator that are always on.
> 
> >> This may not be a popular opinion, but if the ANX7688 is fully
> >> controlled by the EC, I wonder if we shouldn't have an "EC DRM bridge"
> >> driver that would interrogate the EC instead :-)
> 
> We can do this, and tbh will be more easy for us, but we were already asked to
> do it generic, so ...

It's hardly generic if the "generic" driver assumes that there's an EC
controlling the device, isn't it ?

> > But the driver in patch 2/2 do access the anx7688 device with I2C.
> > It does interrogate the EC.
> > 
> >> Is there a risk of bus conflicts if the EC and the main SoC try to
> >> access the ANX7688 over I2C at the same time ?
> 
> No, there is a kind of i2c tunnel but you don't talk directly with the ANX7688.
> The EC exposes the anx bus control to the AP.

Maybe an additional reason to talk to the EC directly instead ? Won't
upstreaming an ANX7688 driver that hardcodes assumptions about an EC
being present cause issues in the future, when someone will want to
extend the driver for a standalone ANX7688 ? The driver will start
programming the ANX7688, and the EC won't like it. We would have to add
a "this is a real ANX7688" DT property later, which would really not be
nice.

> > The SoC access the anx7688 though something called 'i2c-tunnel' (see
> > google,cros-ec-i2c-tunnel.yaml)
> > So the I2C tunnels though the EC (I don't know how this is really implemented to
> > be honest).
> > 
> >>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>>
> >>>>> +
> >>>>> +additionalProperties: false
> >>>>> +
> >>>>> +examples:
> >>>>> +  - |
> >>>>> +    #include <dt-bindings/gpio/gpio.h>
> >>>>> +    #include <dt-bindings/interrupt-controller/irq.h>
> >>>>> +
> >>>>> +    i2c0 {
> >>>>> +        #address-cells = <1>;
> >>>>> +        #size-cells = <0>;
> >>>>> +
> >>>>> +        anx7688: anx7688@2c {
> >>>>> +            compatible = "analogix,anx7688";
> >>>>> +            reg = <0x2c>;
> >>>>> +            avdd33-supply = <&reg_dcdc1>;
> >>>>> +            dvdd18-supply = <&reg_ldo_io1>;
> >>>>> +            avdd18-supply = <&reg_ldo_io1>;
> >>>>> +            avdd10-supply = <&reg_anx1v0>;
> >>>>> +            dvdd10-supply = <&reg_anx1v0>;
> >>>>> +            hdmi_vt-supply = <&reg_dldo1>;
> >>>>> +            enable-gpios = <&pio 3 10 GPIO_ACTIVE_LOW>; /* PD10 */
> >>>>> +            reset-gpios = <&pio 3 6 GPIO_ACTIVE_HIGH>; /* PD6 */
> >>>>> +            interrupt-parent = <&r_pio>;
> >>>>> +            interrupts = <0 11 IRQ_TYPE_EDGE_FALLING>; /* PL11 */
> >>>>> +            cabledet-gpios = <&r_pio 0 8 GPIO_ACTIVE_HIGH>; /* PL8 */
> >>>>> +            vconn-en1-gpios = <&pio 3 9 GPIO_ACTIVE_LOW>; /* PD9 */
> >>>>> +            vconn-en2-gpios = <&pio 3 9 GPIO_ACTIVE_LOW>; /* PD9 */
> >>>>> +            ports {
> >>>>> +                #address-cells = <1>;
> >>>>> +                #size-cells = <0>;
> >>>>> +
> >>>>> +                port@0 {
> >>>>> +                    reg = <0>;
> >>>>> +                    anx7688_in0: endpoint {
> >>>>> +                        remote-endpoint = <&hdmi0_out>;
> >>>>> +                    };
> >>>>> +                };
> >>>>> +
> >>>>> +                port@1 {
> >>>>> +                    reg = <1>;
> >>>>> +                    anx7688_in1: endpoint {
> >>>>> +                        remote-endpoint = <&usbdrd_phy_ss>;
> >>>>> +                    };
> >>>>> +                };
> >>>>> +                port@2 {
> >>>>> +                    reg = <2>;
> >>>>> +                    anx7688_out: endpoint {
> >>>>> +                        remote-endpoint = <&typec_connector>;
> >>>>> +                    };
> >>>>> +                };
> >>>>> +            };
> >>>>> +        };
> >>>>> +    };

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 1/2] dt-bindings: usb: add analogix,anx7688.yaml

[Enric Balletbo i Serra]

Hi Laurent and Dafna,

On 31/3/21 22:40, Laurent Pinchart wrote:
> On Tue, Mar 30, 2021 at 05:14:44PM +0200, Enric Balletbo i Serra wrote:
>> On 30/3/21 15:35, Dafna Hirschfeld wrote:
>>> On 05.03.21 16:19, Laurent Pinchart wrote:
>>>> On Fri, Mar 05, 2021 at 04:14:03PM +0100, Dafna Hirschfeld wrote:
>>>>> On 05.03.21 15:34, Laurent Pinchart wrote:
>>>>>> On Fri, Mar 05, 2021 at 01:43:50PM +0100, Dafna Hirschfeld wrote:
>>>>>>> ANX7688 is a USB Type-C port controller with a MUX. It converts HDMI 2.0 to
>>>>>>> DisplayPort 1.3 Ultra-HDi (4096x2160p60).
>>>>>>> The integrated crosspoint switch (the MUX) supports USB 3.1 data transfer
>>>>>>> along with the DisplayPort Alternate Mode signaling over USB Type-C.
>>>>>>> Additionally, an on-chip microcontroller (OCM) is available to manage the
>>>>>>> signal switching, Channel Configuration (CC) detection, USB Power
>>>>>>> Delivery (USB-PD), Vendor Defined Message (VDM) protocol support and other
>>>>>>> functions as defined in the USB TypeC and USB Power Delivery
>>>>>>> specifications.
>>>>>>>
>>>>>>> ANX7688 is found on Acer Chromebook R13 (elm) and on
>>>>>>> Pine64 PinePhone.
>>>>>>>
>>>>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>>>>>> ---
>>>>>>>    .../bindings/usb/analogix,anx7688.yaml        | 177 ++++++++++++++++++
>>>>>>>    1 file changed, 177 insertions(+)
>>>>>>>    create mode 100644
>>>>>>> Documentation/devicetree/bindings/usb/analogix,anx7688.yaml
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/usb/analogix,anx7688.yaml
>>>>>>> b/Documentation/devicetree/bindings/usb/analogix,anx7688.yaml
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..6c4dd6b4b28b
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/usb/analogix,anx7688.yaml
>>>>>>> @@ -0,0 +1,177 @@
>>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>>> +%YAML 1.2
>>>>>>> +---
>>>>>>> +$id: http://devicetree.org/schemas/usb/analogix,anx7688.yaml#
>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>> +
>>>>>>> +title: Analogix ANX7688 Type-C Port Controller with HDMI to DP conversion
>>>>>>> +
>>>>>>> +maintainers:
>>>>>>> +  - Nicolas Boichat <drinkcat@chromium.org>
>>>>>>> +  - Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>>>>>> +
>>>>>>> +description: |
>>>>>>> +  ANX7688 is a USB Type-C port controller with a MUX. It converts HDMI 2.0 to
>>>>>>> +  DisplayPort 1.3 Ultra-HDi (4096x2160p60).
>>>>>>> +  The integrated crosspoint switch (the MUX) supports USB 3.1 data
>>>>>>> transfer along with
>>>>>>> +  the DisplayPort Alternate Mode signaling over USB Type-C. Additionally,
>>>>>>> +  an on-chip microcontroller (OCM) is available to manage the signal
>>>>>>> switching,
>>>>>>> +  Channel Configuration (CC) detection, USB Power Delivery (USB-PD), Vendor
>>>>>>> +  Defined Message (VDM) protocol support and other functions as defined in
>>>>>>> the
>>>>>>> +  USB TypeC and USB Power Delivery specifications.
>>>>>>> +
>>>>>>> +
>>>>>>
>>>>>> Extra blank line ?
>>>>>>
>>>>>>> +properties:
>>>>>>> +  compatible:
>>>>>>> +    const: analogix,anx7688
>>>>>>> +
>>>>>>> +  reg:
>>>>>>> +    maxItems: 1
>>>>>>> +
>>>>>>> +  avdd33-supply:
>>>>>>> +    description: 3.3V Analog core supply voltage.
>>>>>>> +
>>>>>>> +  dvdd18-supply:
>>>>>>> +    description: 1.8V Digital I/O supply voltage.
>>>>>>> +
>>>>>>> +  avdd18-supply:
>>>>>>> +    description: 1.8V Analog core power supply voltage.
>>>>>>> +
>>>>>>> +  avdd10-supply:
>>>>>>> +    description: 1.0V Analog core power supply voltage.
>>>>>>> +
>>>>>>> +  dvdd10-supply:
>>>>>>> +    description: 1.0V Digital core supply voltage.
>>>>>>> +
>>>>>>
>>>>>> That's lots of supplies. If there's a reasonable chance that some of
>>>>>> them will always be driven by the same regulator (especially if the
>>>>>> ANX7688 documentation requires that), then they could be grouped. For
>>>>>> instance dvdd18-supply and avdd18-supply could be grouped into
>>>>>> vdd18-supply. It would still allow us to extend the bindings in a
>>>>>> backward compatible way later if a system uses different regulators. You
>>>>>> have more information about the hardware than I do, so it's your call.
>>>
>>> Can you explain what do you mean by 'grouped' ?
>>> Do you mean that instead of having two properties dvdd18-supply and avdd18-supply
>>> I have only one property vdd18-supply?
>>
>> You can simplify all this with vdd33, vdd18 vdd10. For the Chromebook case all
>> the analogic and digital part are the same regulator just filtered. That's a
>> common configuration and if there is some hardware that needs it we can extend
>> later.
> 
> That's the idea, yes. If in a typical use case multiple supplies are
> provided by a single regulator (for some devices that datasheet strongly
> recommends that, or event mandates it), then it makes sense to group
> those supplies in a single DT supply property. It can always be extended
> later indeed, without any backward compatibility issue.
> 
>>>>>>> +  hdmi5v-supply:
>>>>>>> +    description: 5V power supply for the HDMI.
>>>>>>> +
>>>>>>> +  hdmi_vt-supply:
>>>>>>> +    description: Termination voltage for HDMI input.
>>>>>>
>>>>>> Maybe hdmi-vt-supply ?
>>>>>>
>>>>>>> +
>>>>>>> +  clocks:
>>>>>>> +    description: The input clock specifier.
>>>>>>> +    maxItems: 1
>>>>>>
>>>>>> How about
>>>>>>
>>>>>>       items:
>>>>>>         - description: The input clock specifier.
>>>>>>
>>>>>>> +
>>>>>>> +  clock-names:
>>>>>>> +    items:
>>>>>>> +      - const: xtal
>>>>>>> +
>>>>>>> +  hpd-gpios:
>>>>>>> +    description: |
>>>>>>> +      In USB Type-C applications, DP_HPD has no use. In standard DisplayPort
>>>>>>> +      applications, DP_HPD is used as DP hot-plug.
>>>>>>> +    maxItems: 1
>>>>>>> +
>>>>>>> +  enable-gpios:
>>>>>>> +    description: Chip power down control. No internal pull-down or pull-up
>>>>>>> resistor.
>>>>>>> +    maxItems: 1
>>>>>>> +
>>>>>>> +  reset-gpios:
>>>>>>> +    description: Reset input signal. Active low.
>>>>>>> +    maxItems: 1
>>>>>>> +
>>>>>>> +  vbus-det-gpios:
>>>>>>> +    description: |
>>>>>>> +      An input gpio for VBUS detection and high voltage detection,
>>>>>>> +      external resistance divide VBUS voltage to 1/8.
>>>>>>> +    maxItems: 1
>>>>>>> +
>>>>>>> +  interrupts:
>>>>>>> +    description: |
>>>>>>> +      The interrupt notifies 4 possible events - TCPC ALERT int, PD int,
>>>>>>> DP int, HDMI int.
>>>>>>> +    maxItems: 1
>>>>>>> +
>>>>>>> +  cabledet-gpios:
>>>>>>> +    description: An output gpio, indicates by the device that a cable is
>>>>>>> plugged.
>>>>>>> +    maxItems: 1
>>>>>>> +
>>>>>>> +  vbus-ctrl-gpios:
>>>>>>> +    description:
>>>>>>> +      External VBUS power path. Enable VBUS source and disable VBUS sink
>>>>>>> or vice versa.
>>>>>>> +    maxItems: 1
>>>>>>> +
>>>>>>> +  vconn-en1-gpios:
>>>>>>> +    description: Controls the VCONN switch on the CC1 pin.
>>>>>>> +    maxItems: 1
>>>>>>> +
>>>>>>> +  vconn-en2-gpios:
>>>>>>> +    description: Controls the VCONN switch on the CC2 pin.
>>>>>>> +    maxItems: 1
>>>>>>> +
>>>>>>> +  ports:
>>>>>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>>>>>> +
>>>>>>> +    properties:
>>>>>>> +      port@0:
>>>>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>>>>> +        description: Video port for HDMI input.
>>>>>>> +
>>>>>>> +      port@1:
>>>>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>>>>> +        description: USB port for the USB3 input.
>>>>>>> +
>>>>>>> +      port@2:
>>>>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>>>>> +        description: USB Type-c connector, see connector/usb-connector.yaml.
>>>>>>> +
>>>>>>> +    required:
>>>>>>> +      - port@0
>>>>>>
>>>>>> As all the ports exist at the hardware level, should they always be
>>>>>> present ? The endpoints are optional of course, in case a port isn't
>>>>>> connected on a particular system.
>>>>>>
>>>>>>> +
>>>>>>> +required:
>>>>>>> +  - compatible
>>>>>>> +  - reg
>>>>>>
>>>>>> Shouldn't clocks and regulators be also required ?
>>>>>
>>>>> hmm, theoretically yes. Practically, in the case of Acer R13 (mediatek elm) device,
>>>>> the ANX7688 is powered up and controlled by the Embedded Controller. The kernel only
>>>>> needs to read few registers from that device for the drm bridge driver.
>>>>> (also mentioned that in the cover letter).
>>
>> I think that for the Chromebook you can assume that these supplies are a
>> fixed-regulator that are always on.
>>
>>>> This may not be a popular opinion, but if the ANX7688 is fully
>>>> controlled by the EC, I wonder if we shouldn't have an "EC DRM bridge"
>>>> driver that would interrogate the EC instead :-)
>>
>> We can do this, and tbh will be more easy for us, but we were already asked to
>> do it generic, so ...
> 
> It's hardly generic if the "generic" driver assumes that there's an EC
> controlling the device, isn't it ?
> 
>>> But the driver in patch 2/2 do access the anx7688 device with I2C.
>>> It does interrogate the EC.
>>>
>>>> Is there a risk of bus conflicts if the EC and the main SoC try to
>>>> access the ANX7688 over I2C at the same time ?
>>
>> No, there is a kind of i2c tunnel but you don't talk directly with the ANX7688.
>> The EC exposes the anx bus control to the AP.
> 
> Maybe an additional reason to talk to the EC directly instead ? Won't
> upstreaming an ANX7688 driver that hardcodes assumptions about an EC
> being present cause issues in the future, when someone will want to
> extend the driver for a standalone ANX7688 ? The driver will start
> programming the ANX7688, and the EC won't like it. We would have to add
> a "this is a real ANX7688" DT property later, which would really not be
> nice.
> 

Well, we try to not assume that there is an EC controlling the device, but I
agree that is difficult for us create a "generic" driver when our test setup is
not generic at all.

The ANX7688 has two typical applications, one is only a HDMI to DisplayPort
bridge, which might be compatible with our setup. And another one, is a
Full-Featured USB-C device. Our thought was that, if the DT (see the binding)
had only the port0 the "generic" driver should act as a bridge only driver, if
port1 and port2 is also defined, the "generic" driver should act a bridge+type-c
controller.

Unfortunately we don't have the setup to test the driver as a full-featured
USB-C device, so I agree that it could be easy to make assumptions that might be
wrong in the future (not very nice, right).

I'd propose then, go with the original idea and do a cros_ec_anx7688 specific
driver that assumes is behind and EC. And if at some point someone with a full
setup sends a "generic" driver for the anx7688 we can check if we can make it
work with our setup or not.

Thanks,
  Enric


>>> The SoC access the anx7688 though something called 'i2c-tunnel' (see
>>> google,cros-ec-i2c-tunnel.yaml)
>>> So the I2C tunnels though the EC (I don't know how this is really implemented to
>>> be honest).
>>>
>>>>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>>>
>>>>>>> +
>>>>>>> +additionalProperties: false
>>>>>>> +
>>>>>>> +examples:
>>>>>>> +  - |
>>>>>>> +    #include <dt-bindings/gpio/gpio.h>
>>>>>>> +    #include <dt-bindings/interrupt-controller/irq.h>
>>>>>>> +
>>>>>>> +    i2c0 {
>>>>>>> +        #address-cells = <1>;
>>>>>>> +        #size-cells = <0>;
>>>>>>> +
>>>>>>> +        anx7688: anx7688@2c {
>>>>>>> +            compatible = "analogix,anx7688";
>>>>>>> +            reg = <0x2c>;
>>>>>>> +            avdd33-supply = <&reg_dcdc1>;
>>>>>>> +            dvdd18-supply = <&reg_ldo_io1>;
>>>>>>> +            avdd18-supply = <&reg_ldo_io1>;
>>>>>>> +            avdd10-supply = <&reg_anx1v0>;
>>>>>>> +            dvdd10-supply = <&reg_anx1v0>;
>>>>>>> +            hdmi_vt-supply = <&reg_dldo1>;
>>>>>>> +            enable-gpios = <&pio 3 10 GPIO_ACTIVE_LOW>; /* PD10 */
>>>>>>> +            reset-gpios = <&pio 3 6 GPIO_ACTIVE_HIGH>; /* PD6 */
>>>>>>> +            interrupt-parent = <&r_pio>;
>>>>>>> +            interrupts = <0 11 IRQ_TYPE_EDGE_FALLING>; /* PL11 */
>>>>>>> +            cabledet-gpios = <&r_pio 0 8 GPIO_ACTIVE_HIGH>; /* PL8 */
>>>>>>> +            vconn-en1-gpios = <&pio 3 9 GPIO_ACTIVE_LOW>; /* PD9 */
>>>>>>> +            vconn-en2-gpios = <&pio 3 9 GPIO_ACTIVE_LOW>; /* PD9 */
>>>>>>> +            ports {
>>>>>>> +                #address-cells = <1>;
>>>>>>> +                #size-cells = <0>;
>>>>>>> +
>>>>>>> +                port@0 {
>>>>>>> +                    reg = <0>;
>>>>>>> +                    anx7688_in0: endpoint {
>>>>>>> +                        remote-endpoint = <&hdmi0_out>;
>>>>>>> +                    };
>>>>>>> +                };
>>>>>>> +
>>>>>>> +                port@1 {
>>>>>>> +                    reg = <1>;
>>>>>>> +                    anx7688_in1: endpoint {
>>>>>>> +                        remote-endpoint = <&usbdrd_phy_ss>;
>>>>>>> +                    };
>>>>>>> +                };
>>>>>>> +                port@2 {
>>>>>>> +                    reg = <2>;
>>>>>>> +                    anx7688_out: endpoint {
>>>>>>> +                        remote-endpoint = <&typec_connector>;
>>>>>>> +                    };
>>>>>>> +                };
>>>>>>> +            };
>>>>>>> +        };
>>>>>>> +    };
> 

Re: [PATCH v5 1/2] dt-bindings: usb: add analogix,anx7688.yaml

[Ondřej Jirman]

On Wed, Mar 31, 2021 at 07:16:40PM +0200, Dafna Hirschfeld wrote:
> Hi,
> 
> On 05.03.21 18:24, Ondřej Jirman wrote:
> > Hello Dafna,
> > 

[...]

> > > > > +  vconn-en1-gpios:
> > > > > +    description: Controls the VCONN switch on the CC1 pin.
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  vconn-en2-gpios:
> > > > > +    description: Controls the VCONN switch on the CC2 pin.
> > > > > +    maxItems: 1
> > 
> > VCONN is a voltage regulator that can be enabled either on CC1 or CC2
> > pin, or disabled completely. This control is *partially* performed in reference
> > design directly by the OCM. OCM only decides which CC pin to switch
> > the VCONN regulator to, and informs the SoC when the base VCONN regulator
> > for the switches needs to be enabled.
> > 
> > So vconn-en1/2 gpios are irrelevant to the driver, but the driver needs
> > to control VCONN power supply somehow and defer control over it to the OCM.
> > 
> > I don't know how to express it in the bindings. Maybe a vconn-supply here
> > on the anx7688 node?
> > 
> > It may also be part of the usb-connector binding, but this is really when it
> > comes to anx7688 a parent regulator for the switches, and switches are not
> > controlled by this driver, just their parent regulator is.
> > 
> > Any ideas?
> 
> Looking at the diagram in the schematics, I see that both vbus-supply and vconn-supply
> come directly from the PMIC so similarly to the vbus-supply, the vconn-supply
> can be part of the usb-connector. Then a driver can access the vconn-supply from the remote usb
> connector. Is there a problem with that?

No problem with that. usb-c-connector binding would just have to be extended.

I just don't see any need for these `vconn-en*-gpios`, because the control
is performed directly by the OCM firmware via GPIOs in the ANX7688 chip,
outside of the control of the Linux driver, and the driver doesn't even care
about the status of these pins. And if it will ever care, the status can be
read via I2C from the ANX7688 chip directly.

kind regards,
	o.

> Thanks a lot for the review, I am not very familiar with usb and type-c, so your review helps a lot.
> I will send v6 soon.
> 
> Thanks,
> Dafna