[RFC PATCH v2 0/2] usb: typec: Add driver for Microchip sama7g5 tcpc

[RFC PATCH v2 0/2] usb: typec: Add driver for Microchip sama7g5 tcpc

[cristian.birsan]

From: Cristian Birsan <cristian.birsan@microchip.com>

This patch set adds initial driver support for Microchip USB Type-C Port
Controller (TCPC) embedded in sama7g5 SoC.

The controller does not implement power delivery and the driver uses dummy
functions to register the port with TCPM. The current silicon version is
not able to trigger interrupts so the driver will poll for changes on
CC1/CC2 lines.

Support for sink is implemented and tested with an USB device. The plan is
to extend the driver and add source support.

Changes in v2:
- fix DT bindings yamllint warnings/errors
- fix compilation error reported by: kernel test robot <lkp@intel.com>

Cristian Birsan (2):
  dt-bindings: usb: Add DT bindings for Microchip sama7g5 tcpc
  usb: typec: sama7g5_tcpc: add driver for Microchip sama7g5 tcpc

 .../bindings/usb/microchip,sama7g5-tcpc.yaml  |  90 +++
 drivers/usb/typec/tcpm/Kconfig                |   8 +
 drivers/usb/typec/tcpm/Makefile               |   1 +
 drivers/usb/typec/tcpm/sama7g5_tcpc.c         | 610 ++++++++++++++++++
 4 files changed, 709 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/microchip,sama7g5-tcpc.yaml
 create mode 100644 drivers/usb/typec/tcpm/sama7g5_tcpc.c

-- 
2.25.1

[RFC PATCH v2 1/2] dt-bindings: usb: Add DT bindings for Microchip sama7g5 tcpc

[cristian.birsan]

From: Cristian Birsan <cristian.birsan@microchip.com>

This patch adds DT bindings for the new Microchip USB Type-C Port
Controller (TCPC) embedded in sama7g5 SoC.

Signed-off-by: Cristian Birsan <cristian.birsan@microchip.com>
---
 .../bindings/usb/microchip,sama7g5-tcpc.yaml  | 90 +++++++++++++++++++
 1 file changed, 90 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/microchip,sama7g5-tcpc.yaml

diff --git a/Documentation/devicetree/bindings/usb/microchip,sama7g5-tcpc.yaml b/Documentation/devicetree/bindings/usb/microchip,sama7g5-tcpc.yaml
new file mode 100644
index 000000000000..2d3289dd9ac9
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/microchip,sama7g5-tcpc.yaml
@@ -0,0 +1,90 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/usb/microchip,sama7g5-tcpc.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Microchip SAMA7G5 Type-C port controller bindings
+
+maintainers:
+  - Cristian Birsan <cristian.birsan@microchip.com>
+
+properties:
+  compatible:
+    enum:
+      - microchip,sama7g5-tcpc
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: Peripheral Bus Clock
+      - description: Generic Clock
+
+  clock-names:
+    items:
+      - const: pclk
+      - const: gclk
+
+  vbus-supply:
+    description: vbus power supply (5V) phandle
+
+  microchip,vbus-gpio:
+    description: vbus detect gpio
+
+  connector:
+    type: object
+
+    allOf:
+      - $ref: ../connector/usb-connector.yaml
+
+    properties:
+      compatible:
+        const: usb-c-connector
+
+      power-role: true
+
+    required:
+      - compatible
+
+required:
+  - compatible
+  - reg
+  - connector
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/at91.h>
+    #include <dt-bindings/usb/pd.h>
+    tcpcb: tcpcb@e0844000 {
+      compatible = "microchip,sama7g5-tcpc";
+      reg = <0xe0844000 0x3fff>;
+      clocks = <&pmc PMC_TYPE_PERIPHERAL 95>, <&pmc PMC_TYPE_GCK 95>;
+      clock-names = "pclk", "gclk";
+
+      usb_con: connector {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        compatible = "usb-c-connector";
+        label = "USB-C";
+        data-role = "device";
+        power-role = "sink";
+        try-power-role = "sink";
+        source-pdos = <PDO_FIXED(5000, 500,
+            PDO_FIXED_USB_COMM)>;
+        sink-pdos = <PDO_FIXED(5000, 500,
+            PDO_FIXED_USB_COMM)>;
+        op-sink-microwatt = <2500000>;
+
+        port@1 {
+          reg = <1>;
+          usb_con_hs: endpoint {
+            remote-endpoint = <&usb_phy1>;
+          };
+        };
+      };
+    };
+...
-- 
2.25.1

[RFC PATCH v2 2/2] usb: typec: sama7g5_tcpc: add driver for Microchip sama7g5 tcpc

[cristian.birsan]

From: Cristian Birsan <cristian.birsan@microchip.com>

This patch adds initial driver support for the new Microchip USB
Type-C Port Controller (TCPC) embedded in sama7g5 SoC.

Signed-off-by: Cristian Birsan <cristian.birsan@microchip.com>
---
 drivers/usb/typec/tcpm/Kconfig        |   8 +
 drivers/usb/typec/tcpm/Makefile       |   1 +
 drivers/usb/typec/tcpm/sama7g5_tcpc.c | 610 ++++++++++++++++++++++++++
 3 files changed, 619 insertions(+)
 create mode 100644 drivers/usb/typec/tcpm/sama7g5_tcpc.c

diff --git a/drivers/usb/typec/tcpm/Kconfig b/drivers/usb/typec/tcpm/Kconfig
index 557f392fe24d..8ba0fd85741f 100644
--- a/drivers/usb/typec/tcpm/Kconfig
+++ b/drivers/usb/typec/tcpm/Kconfig
@@ -52,6 +52,14 @@ config TYPEC_FUSB302
 	  Type-C Port Controller Manager to provide USB PD and USB
 	  Type-C functionalities.
 
+config TYPEC_SAMA7G5
+	tristate "Microchip SAMA7G5 Type-C Port Controller driver"
+	select REGMAP_MMIO
+	help
+	  Say Y or M here if your system has SAMA7G5 TCPC controller.
+	  It works with Type-C Port Controller Manager to provide USB
+	  Type-C functionalities.
+
 config TYPEC_WCOVE
 	tristate "Intel WhiskeyCove PMIC USB Type-C PHY driver"
 	depends on ACPI
diff --git a/drivers/usb/typec/tcpm/Makefile b/drivers/usb/typec/tcpm/Makefile
index 7d499f3569fd..9abe8a7ae1cc 100644
--- a/drivers/usb/typec/tcpm/Makefile
+++ b/drivers/usb/typec/tcpm/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_TYPEC_TCPM)		+= tcpm.o
 obj-$(CONFIG_TYPEC_FUSB302)		+= fusb302.o
+obj-$(CONFIG_TYPEC_SAMA7G5)		+= sama7g5_tcpc.o
 obj-$(CONFIG_TYPEC_WCOVE)		+= typec_wcove.o
 typec_wcove-y				:= wcove.o
 obj-$(CONFIG_TYPEC_TCPCI)		+= tcpci.o
diff --git a/drivers/usb/typec/tcpm/sama7g5_tcpc.c b/drivers/usb/typec/tcpm/sama7g5_tcpc.c
new file mode 100644
index 000000000000..2986c0fcc8a3
--- /dev/null
+++ b/drivers/usb/typec/tcpm/sama7g5_tcpc.c
@@ -0,0 +1,610 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Microchip SAMA7G5 Type-C Port Controller Driver
+ *
+ * Copyright (C) 2021 Microchip Technology, Inc. and its subsidiaries
+ */
+
+#include <linux/clk.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/usb/pd.h>
+#include <linux/usb/tcpm.h>
+#include <linux/usb/typec.h>
+
+#define SAMA7G5_TCPC_GCLK				32000
+
+/* TCPC registers offsets */
+#define TCPC_CR			0x80		/* TCPC Control Register */
+#define TCPC_UPC		0xA0		/* TCPC PHY Control Register */
+#define TCPC_UPS		0xA4		/* TCPC PHY Status Register */
+
+#define TCPC_CR_RESET		0x54434301	/* Magic value */
+
+/* TCPC PHY Control Register */
+#define TCPC_UPC_BCDETE		BIT(29)
+#define TCPC_UPC_BCVSRCE	BIT(28)
+#define	TCPC_UPC_BCDETSEL	BIT(27)
+#define	TCPC_UPC_BCIDPSRCE	BIT(26)
+#define TCPC_UPC_DMPDFE		BIT(25)
+#define TCPC_UPC_DMPDFD		BIT(24)
+#define TCPC_UPC_IP_OFF		(0 << 12)
+#define TCPC_UPC_IP_0P5		(1 << 12)
+#define TCPC_UPC_IP_1P5		(2 << 12)
+#define TCPC_UPC_IP_3P0		(3 << 12)
+#define TCPC_UPC_THRESHOLD0	(0 << 8)
+#define TCPC_UPC_THRESHOLD2	(2 << 8)
+#define TCPC_UPC_THRESHOLD4	(4 << 8)
+#define TCPC_UPC_THRESHOLD6	(6 << 8)
+
+/* TCPC PHY  Status Register */
+#define TCPC_UPS_CC2RDT		BIT(4)
+#define TCPC_UPS_CC1ID		BIT(3)
+#define TCPC_UPS_CC_MASK	GENMASK(4, 3)
+#define TCPC_UPS_CHGDCP		BIT(2)
+#define TCPC_UPS_DM		BIT(1)
+#define TCPC_UPS_DP		BIT(0)
+
+#define TCPC_VERSION		0xFC
+
+/* USB Type-C measurement timings */
+#define T_CC_MEASURE		100 /* 100 ms */
+
+#define SAMA7G5_TCPC_VBUS_IRQFLAGS (IRQF_ONESHOT \
+			   | IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING)
+
+struct sama7g5_tcpc {
+	struct device *dev;
+
+	struct workqueue_struct *wq;
+	struct delayed_work measure_work;
+
+	struct regmap *regmap;
+	void __iomem *base;
+
+	struct clk *pclk;
+	struct clk *gclk;
+
+	struct gpio_desc *vbus_pin;
+	struct regulator *vbus;
+
+	/* lock for sharing states */
+	struct mutex lock;
+
+	/* port status */
+	enum typec_cc_polarity cc_polarity;
+	enum typec_cc_status cc1_status;
+	enum typec_cc_status cc2_status;
+	enum typec_cc_status cc1_status_prev;
+	enum typec_cc_status cc2_status_prev;
+
+	/* mutex used for VBUS detection */
+	struct mutex vbus_mutex;
+	int vbus_present;
+	int vbus_present_prev;
+
+	unsigned int phy_status;
+	unsigned int phy_status_old;
+
+	struct tcpc_dev tcpc;
+	struct tcpm_port *tcpm;
+};
+
+#define tcpc_to_sama7g5_tcpc(_tcpc_) \
+		container_of(_tcpc_, struct sama7g5_tcpc, tcpc)
+
+static bool sama7g5_tcpc_readable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case TCPC_CR:
+	case TCPC_UPC:
+	case TCPC_UPS:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool sama7g5_tcpc_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case TCPC_CR:
+	case TCPC_UPC:
+	case TCPC_UPS:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct regmap_config sama7g5_tcpc_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.max_register = TCPC_VERSION,
+	.readable_reg = sama7g5_tcpc_readable_reg,
+	.writeable_reg = sama7g5_tcpc_writeable_reg,
+};
+
+static int sama7g5_tcpc_get_vbus(struct tcpc_dev *tcpc)
+{
+	struct sama7g5_tcpc *sama7g5_tcpc = tcpc_to_sama7g5_tcpc(tcpc);
+	int ret;
+
+	mutex_lock(&sama7g5_tcpc->vbus_mutex);
+	ret = sama7g5_tcpc->vbus_present ? 1 : 0;
+	mutex_unlock(&sama7g5_tcpc->vbus_mutex);
+
+	return ret;
+}
+
+static int sama7g5_tcpc_set_vbus(struct tcpc_dev *tcpc, bool on, bool sink)
+{
+	struct sama7g5_tcpc *sama7g5_tcpc = tcpc_to_sama7g5_tcpc(tcpc);
+	int ret;
+
+	mutex_lock(&sama7g5_tcpc->vbus_mutex);
+	if (on)
+		ret = regulator_enable(sama7g5_tcpc->vbus);
+	else
+		ret = regulator_disable(sama7g5_tcpc->vbus);
+	mutex_unlock(&sama7g5_tcpc->vbus_mutex);
+
+	return ret;
+}
+
+static int sama7g5_tcpc_set_vconn(struct tcpc_dev *tcpc, bool on)
+{
+	/* VCONN is not supported */
+	return -EPERM;
+}
+
+static int sama7g5_tcpc_get_cc(struct tcpc_dev *tcpc, enum typec_cc_status *cc1,
+			       enum typec_cc_status *cc2)
+{
+	struct sama7g5_tcpc *sama7g5_tcpc = tcpc_to_sama7g5_tcpc(tcpc);
+
+	mutex_lock(&sama7g5_tcpc->lock);
+	*cc1 = sama7g5_tcpc->cc1_status;
+	*cc2 = sama7g5_tcpc->cc2_status;
+	mutex_unlock(&sama7g5_tcpc->lock);
+
+	return 0;
+}
+
+static int sama7g5_tcpc_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status cc)
+{
+	struct sama7g5_tcpc *sama7g5_tcpc = tcpc_to_sama7g5_tcpc(tcpc);
+	unsigned int ctrl;
+	int ret = 0;
+
+	mutex_lock(&sama7g5_tcpc->lock);
+	switch (cc) {
+	case TYPEC_CC_RD:
+		ctrl = TCPC_UPC_IP_OFF;
+		break;
+	case TYPEC_CC_RP_DEF:
+		ctrl = TCPC_UPC_IP_0P5;
+		break;
+	default:
+		ret =  -EINVAL;
+		goto done;
+	}
+	ret = regmap_write(sama7g5_tcpc->regmap, TCPC_UPC, ctrl);
+done:
+	mutex_unlock(&sama7g5_tcpc->lock);
+	return ret;
+}
+
+static int sama7g5_tcpc_set_polarity(struct tcpc_dev *tcpc,
+				     enum typec_cc_polarity pol)
+{
+	return 0;
+}
+
+static int sama7g5_tcpc_set_roles(struct tcpc_dev *tcpc, bool attached,
+			   enum typec_role role, enum typec_data_role data)
+{
+	return 0;
+}
+
+static int sama7g5_tcpc_set_pd_rx(struct tcpc_dev *tcpc, bool on)
+{
+	return -EPERM;
+}
+
+static int sama7g5_tcpc_pd_transmit(struct tcpc_dev *tcpc,
+				    enum tcpm_transmit_type type,
+				    const struct pd_message *msg,
+				    unsigned int negotiated_rev)
+{
+	return -EPERM;
+}
+
+static int sama7g5_tcpc_start_toggling(struct tcpc_dev *tcpc,
+				       enum typec_port_type port_type,
+				       enum typec_cc_status cc)
+{
+	return -EOPNOTSUPP;
+}
+
+static void _sama7g5_tcpc_measure_snk(struct sama7g5_tcpc *sama7g5_tcpc)
+{
+	struct device *dev = sama7g5_tcpc->dev;
+	int ret;
+
+	/* Save previous CC1/CC2 state */
+	sama7g5_tcpc->cc1_status_prev = sama7g5_tcpc->cc1_status;
+	sama7g5_tcpc->cc2_status_prev = sama7g5_tcpc->cc2_status;
+
+	/* Comparator Threshold 2 */
+	ret = regmap_write(sama7g5_tcpc->regmap, TCPC_UPC, TCPC_UPC_IP_OFF |
+			   TCPC_UPC_THRESHOLD2);
+	if (ret) {
+		dev_err(dev, "failed to wite register: %d\n", ret);
+		return;
+	}
+
+	usleep_range(560, 1000);
+
+	ret = regmap_read(sama7g5_tcpc->regmap, TCPC_UPS,
+			  &sama7g5_tcpc->phy_status);
+	if (ret) {
+		dev_err(dev, "failed to read register: %d\n", ret);
+		return;
+	}
+
+	if (!(sama7g5_tcpc->phy_status & TCPC_UPS_CC_MASK)) {
+		/* VRa*/
+		sama7g5_tcpc->cc1_status = TYPEC_CC_OPEN;
+		sama7g5_tcpc->cc2_status = TYPEC_CC_OPEN;
+		return;
+	}
+
+	/* CC1 or CC2 is connected wait for PD messages to end ~ 30ms */
+	usleep_range(30000, 35000);
+
+	/* Comparator Threshold 4 */
+	sama7g5_tcpc->phy_status_old = sama7g5_tcpc->phy_status;
+
+	ret = regmap_write(sama7g5_tcpc->regmap, TCPC_UPC, TCPC_UPC_IP_OFF |
+			   TCPC_UPC_THRESHOLD4);
+	if (ret) {
+		dev_err(dev, "failed to wite register: %d\n", ret);
+		return;
+	}
+
+	usleep_range(560, 1000);
+	ret = regmap_read(sama7g5_tcpc->regmap, TCPC_UPS,
+			  &sama7g5_tcpc->phy_status);
+	if (ret) {
+		dev_err(dev, "failed to read register: %d\n", ret);
+		return;
+	}
+
+	if ((sama7g5_tcpc->phy_status_old & TCPC_UPS_CC1ID) &&
+	    (!(sama7g5_tcpc->phy_status & TCPC_UPS_CC1ID))) {
+		sama7g5_tcpc->cc1_status = TYPEC_CC_RP_DEF;
+		sama7g5_tcpc->cc2_status = TYPEC_CC_OPEN;
+		return;
+	}
+
+	if ((sama7g5_tcpc->phy_status_old & TCPC_UPS_CC2RDT) &&
+	    (!(sama7g5_tcpc->phy_status & TCPC_UPS_CC2RDT))) {
+		sama7g5_tcpc->cc1_status = TYPEC_CC_OPEN;
+		sama7g5_tcpc->cc2_status = TYPEC_CC_RP_DEF;
+		return;
+	}
+
+	/* Comparator Threshold 6 */
+	sama7g5_tcpc->phy_status_old = sama7g5_tcpc->phy_status;
+
+	ret = regmap_write(sama7g5_tcpc->regmap, TCPC_UPC, TCPC_UPC_IP_OFF |
+			   TCPC_UPC_THRESHOLD6);
+	if (ret) {
+		dev_err(dev, "failed to wite register: %d\n", ret);
+		return;
+	}
+
+	usleep_range(560, 1000);
+	ret = regmap_read(sama7g5_tcpc->regmap, TCPC_UPS,
+			  &sama7g5_tcpc->phy_status);
+	if (ret) {
+		dev_err(dev, "failed to read register: %d\n", ret);
+		return;
+	}
+
+	if ((sama7g5_tcpc->phy_status_old & TCPC_UPS_CC1ID) &&
+	    (!(sama7g5_tcpc->phy_status & TCPC_UPS_CC1ID))) {
+		sama7g5_tcpc->cc1_status = TYPEC_CC_RP_1_5;
+		sama7g5_tcpc->cc2_status = TYPEC_CC_OPEN;
+		return;
+	}
+
+	if ((sama7g5_tcpc->phy_status_old & TCPC_UPS_CC1ID) &&
+	    ((sama7g5_tcpc->phy_status & TCPC_UPS_CC1ID))) {
+		sama7g5_tcpc->cc1_status = TYPEC_CC_RP_3_0;
+		sama7g5_tcpc->cc2_status = TYPEC_CC_OPEN;
+		return;
+	}
+
+	if ((sama7g5_tcpc->phy_status_old & TCPC_UPS_CC2RDT) &&
+	    (!(sama7g5_tcpc->phy_status & TCPC_UPS_CC2RDT))) {
+		sama7g5_tcpc->cc1_status = TYPEC_CC_OPEN;
+		sama7g5_tcpc->cc2_status = TYPEC_CC_RP_1_5;
+		return;
+	}
+
+	if ((sama7g5_tcpc->phy_status_old & TCPC_UPS_CC2RDT) &&
+	    ((sama7g5_tcpc->phy_status & TCPC_UPS_CC2RDT))) {
+		sama7g5_tcpc->cc1_status = TYPEC_CC_OPEN;
+		sama7g5_tcpc->cc2_status = TYPEC_CC_RP_3_0;
+		return;
+	}
+}
+
+static void sama7g5_tcpc_measure_work(struct work_struct *work)
+{
+	struct sama7g5_tcpc *port = container_of(work, struct sama7g5_tcpc,
+						 measure_work.work);
+
+	mutex_lock(&port->lock);
+
+	_sama7g5_tcpc_measure_snk(port);
+
+	/* Check if the state has changed and notify TCPM */
+	if (port->cc1_status != port->cc1_status_prev ||
+	    port->cc2_status != port->cc2_status_prev)
+		tcpm_cc_change(port->tcpm);
+
+	mod_delayed_work(port->wq, &port->measure_work,
+			 msecs_to_jiffies(T_CC_MEASURE));
+
+	mutex_unlock(&port->lock);
+}
+
+static int sama7g5_tcpc_init(struct tcpc_dev *tcpc)
+{
+	struct sama7g5_tcpc *sama7g5_tcpc = tcpc_to_sama7g5_tcpc(tcpc);
+	int ret;
+
+	ret = regmap_write(sama7g5_tcpc->regmap, TCPC_CR, TCPC_CR_RESET);
+	if (ret)
+		return ret;
+
+	sama7g5_tcpc->wq =
+		create_singlethread_workqueue(dev_name(sama7g5_tcpc->dev));
+	if (!sama7g5_tcpc->wq)
+		return -ENOMEM;
+
+	INIT_DELAYED_WORK(&sama7g5_tcpc->measure_work,
+			  sama7g5_tcpc_measure_work);
+
+	sama7g5_tcpc->cc1_status = TYPEC_CC_OPEN;
+	sama7g5_tcpc->cc2_status = TYPEC_CC_OPEN;
+	sama7g5_tcpc->cc1_status_prev = TYPEC_CC_OPEN;
+	sama7g5_tcpc->cc2_status_prev = TYPEC_CC_OPEN;
+	sama7g5_tcpc->cc_polarity = TYPEC_POLARITY_CC1;
+
+	/* We do not have an interrupt so polling only */
+	mod_delayed_work(sama7g5_tcpc->wq, &sama7g5_tcpc->measure_work,
+			 msecs_to_jiffies(T_CC_MEASURE));
+
+	/* Enable VBUS detection */
+	if (sama7g5_tcpc->vbus_pin)
+		enable_irq(gpiod_to_irq(sama7g5_tcpc->vbus_pin));
+
+	return 0;
+}
+
+static int vbus_is_present(struct sama7g5_tcpc *sama7g5_tcpc)
+{
+	if (sama7g5_tcpc->vbus_pin)
+		return gpiod_get_value(sama7g5_tcpc->vbus_pin);
+
+	/* No Vbus detection: Assume always present */
+	return 1;
+}
+
+static irqreturn_t sama7g5_vbus_irq_thread(int irq, void *devid)
+{
+	struct sama7g5_tcpc *sama7g5_tcpc = devid;
+
+	/* debounce */
+	udelay(10);
+
+	mutex_lock(&sama7g5_tcpc->vbus_mutex);
+
+	sama7g5_tcpc->vbus_present = vbus_is_present(sama7g5_tcpc);
+	if (sama7g5_tcpc->vbus_present != sama7g5_tcpc->vbus_present_prev) {
+		/* VBUS changed, notify TCPM */
+		tcpm_vbus_change(sama7g5_tcpc->tcpm);
+		sama7g5_tcpc->vbus_present_prev = sama7g5_tcpc->vbus_present;
+	}
+
+	mutex_unlock(&sama7g5_tcpc->vbus_mutex);
+	return IRQ_HANDLED;
+}
+
+static int sama7g5_tcpc_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct sama7g5_tcpc *sama7g5_tcpc;
+
+	struct resource *mem;
+	void __iomem *base;
+
+	sama7g5_tcpc = devm_kzalloc(&pdev->dev, sizeof(*sama7g5_tcpc),
+				    GFP_KERNEL);
+	if (!sama7g5_tcpc)
+		return -ENOMEM;
+
+	mem =  platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(&pdev->dev, mem);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+	sama7g5_tcpc->base = base;
+
+	sama7g5_tcpc->regmap =  devm_regmap_init_mmio(&pdev->dev, base,
+						&sama7g5_tcpc_regmap_config);
+	if (IS_ERR(sama7g5_tcpc->regmap)) {
+		dev_err(&pdev->dev, "Regmap init failed\n");
+		return PTR_ERR(sama7g5_tcpc->regmap);
+	}
+
+	/* Get the peripheral clock */
+	sama7g5_tcpc->pclk = devm_clk_get(&pdev->dev, "pclk");
+	if (IS_ERR(sama7g5_tcpc->pclk)) {
+		ret = PTR_ERR(sama7g5_tcpc->pclk);
+		dev_err(&pdev->dev,
+			"failed to get the peripheral clock: %d\n", ret);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(sama7g5_tcpc->pclk);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"failed to enable the peripheral clock: %d\n", ret);
+		return ret;
+	}
+
+	/* Get the generic clock */
+	sama7g5_tcpc->gclk = devm_clk_get(&pdev->dev, "gclk");
+	if (IS_ERR(sama7g5_tcpc->gclk)) {
+		ret = PTR_ERR(sama7g5_tcpc->gclk);
+		dev_err(&pdev->dev,
+			"failed to get the PMC generic clock: %d\n", ret);
+		return ret;
+	}
+
+	ret = clk_set_rate(sama7g5_tcpc->gclk, SAMA7G5_TCPC_GCLK);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"unable to change gclk rate to: %u\n",
+			SAMA7G5_TCPC_GCLK);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(sama7g5_tcpc->gclk);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"failed to enable the generic clock: %d\n", ret);
+		return ret;
+	}
+
+	mutex_init(&sama7g5_tcpc->lock);
+	mutex_init(&sama7g5_tcpc->vbus_mutex);
+
+	sama7g5_tcpc->vbus_pin = devm_gpiod_get_optional(&pdev->dev,
+				"microchip,vbus", GPIOD_IN);
+
+	if (IS_ERR(sama7g5_tcpc->vbus_pin)) {
+		ret = PTR_ERR(sama7g5_tcpc->vbus_pin);
+		dev_err(&pdev->dev, "unable to claim vbus-gpio: %d\n", ret);
+	}
+
+	sama7g5_tcpc->vbus = devm_regulator_get_optional(&pdev->dev, "vbus");
+
+	if (IS_ERR(sama7g5_tcpc->vbus)) {
+		ret = PTR_ERR(sama7g5_tcpc->vbus);
+		dev_err(&pdev->dev, "unable to claim vbus-supply: %d\n", ret);
+	}
+
+	if (sama7g5_tcpc->vbus_pin) {
+		irq_set_status_flags(gpiod_to_irq(sama7g5_tcpc->vbus_pin),
+				     IRQ_NOAUTOEN);
+		ret = devm_request_threaded_irq(&pdev->dev,
+				gpiod_to_irq(sama7g5_tcpc->vbus_pin), NULL,
+				sama7g5_vbus_irq_thread,
+				SAMA7G5_TCPC_VBUS_IRQFLAGS,
+				"sama7g5_tcpc", sama7g5_tcpc);
+		if (ret) {
+			sama7g5_tcpc->vbus_pin = NULL;
+			dev_warn(&pdev->dev,
+				 "failed to request vbus irq; "
+				 "assuming always on\n");
+		}
+	}
+
+	sama7g5_tcpc->dev = &pdev->dev;
+	platform_set_drvdata(pdev, sama7g5_tcpc);
+
+	sama7g5_tcpc->tcpc.init = sama7g5_tcpc_init;
+	sama7g5_tcpc->tcpc.get_vbus = sama7g5_tcpc_get_vbus;
+	sama7g5_tcpc->tcpc.set_vbus = sama7g5_tcpc_set_vbus;
+	sama7g5_tcpc->tcpc.set_cc = sama7g5_tcpc_set_cc;
+	sama7g5_tcpc->tcpc.get_cc = sama7g5_tcpc_get_cc;
+	sama7g5_tcpc->tcpc.set_polarity = sama7g5_tcpc_set_polarity;
+	sama7g5_tcpc->tcpc.set_vconn = sama7g5_tcpc_set_vconn;
+	sama7g5_tcpc->tcpc.start_toggling = sama7g5_tcpc_start_toggling;
+	sama7g5_tcpc->tcpc.set_pd_rx = sama7g5_tcpc_set_pd_rx;
+	sama7g5_tcpc->tcpc.set_roles = sama7g5_tcpc_set_roles;
+	sama7g5_tcpc->tcpc.pd_transmit = sama7g5_tcpc_pd_transmit;
+
+	sama7g5_tcpc->tcpc.fwnode = device_get_named_child_node(&pdev->dev,
+								"connector");
+	if (!sama7g5_tcpc->tcpc.fwnode) {
+		dev_err(&pdev->dev, "Can't find connector node.\n");
+		return -EINVAL;
+	}
+
+	sama7g5_tcpc->tcpm = tcpm_register_port(sama7g5_tcpc->dev,
+						&sama7g5_tcpc->tcpc);
+	if (IS_ERR(sama7g5_tcpc->tcpm)) {
+		fwnode_remove_software_node(sama7g5_tcpc->tcpc.fwnode);
+		return PTR_ERR(sama7g5_tcpc->tcpm);
+	}
+
+	return 0;
+}
+
+static int sama7g5_tcpc_remove(struct platform_device *pdev)
+{
+	struct sama7g5_tcpc *sama7g5_tcpc;
+
+	sama7g5_tcpc = platform_get_drvdata(pdev);
+
+	/* Mask everything */
+	if (sama7g5_tcpc->vbus_pin)
+		disable_irq(gpiod_to_irq(sama7g5_tcpc->vbus_pin));
+
+
+	if (!IS_ERR_OR_NULL(sama7g5_tcpc->tcpm))
+		tcpm_unregister_port(sama7g5_tcpc->tcpm);
+
+	destroy_workqueue(sama7g5_tcpc->wq);
+
+	clk_disable_unprepare(sama7g5_tcpc->gclk);
+	clk_disable_unprepare(sama7g5_tcpc->pclk);
+
+	return 0;
+}
+
+static const struct of_device_id sama7g5_tcpc_dt_ids[] = {
+	{
+		.compatible = "microchip,sama7g5-tcpc",
+	},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, sama7g5_tcpc_dt_ids);
+
+static struct platform_driver sama7g5_tcpc_driver = {
+	.probe	= sama7g5_tcpc_probe,
+	.remove = sama7g5_tcpc_remove,
+	.driver = {
+		.name	= "microchip,sama7g5-tcpc",
+		.of_match_table	= sama7g5_tcpc_dt_ids,
+	},
+};
+module_platform_driver(sama7g5_tcpc_driver);
+
+MODULE_AUTHOR("Cristian Birsan <cristian.birsan@microchip.com>");
+MODULE_DESCRIPTION("Microchip SAMA7G5 Type-C Port Controller Driver");
+MODULE_LICENSE("GPL");
-- 
2.25.1

Re: [RFC PATCH v2 0/2] usb: typec: Add driver for Microchip sama7g5 tcpc

[Greg KH]

On Tue, Mar 30, 2021 at 11:54:40PM +0300, cristian.birsan@microchip.com wrote:
> From: Cristian Birsan <cristian.birsan@microchip.com>
> 
> This patch set adds initial driver support for Microchip USB Type-C Port
> Controller (TCPC) embedded in sama7g5 SoC.
> 
> The controller does not implement power delivery and the driver uses dummy
> functions to register the port with TCPM. The current silicon version is
> not able to trigger interrupts so the driver will poll for changes on
> CC1/CC2 lines.
> 
> Support for sink is implemented and tested with an USB device. The plan is
> to extend the driver and add source support.

Why are these marked "RFC"?

Do you really not think they should be accepted?  Why not, what is left
to do with them?

I do not normally review "RFC" patches as the authors do not think they
should be merged, and we have plenty of patches that are being asked to
be merged already :)

thanks,

greg k-h

Re: [RFC PATCH v2 2/2] usb: typec: sama7g5_tcpc: add driver for Microchip sama7g5 tcpc

[Heikki Krogerus]

Hi Cristian,

On Tue, Mar 30, 2021 at 11:54:42PM +0300, cristian.birsan@microchip.com wrote:
> From: Cristian Birsan <cristian.birsan@microchip.com>
> 
> This patch adds initial driver support for the new Microchip USB
> Type-C Port Controller (TCPC) embedded in sama7g5 SoC.
> 
> Signed-off-by: Cristian Birsan <cristian.birsan@microchip.com>
> ---
>  drivers/usb/typec/tcpm/Kconfig        |   8 +
>  drivers/usb/typec/tcpm/Makefile       |   1 +
>  drivers/usb/typec/tcpm/sama7g5_tcpc.c | 610 ++++++++++++++++++++++++++
>  3 files changed, 619 insertions(+)
>  create mode 100644 drivers/usb/typec/tcpm/sama7g5_tcpc.c
> 
> diff --git a/drivers/usb/typec/tcpm/Kconfig b/drivers/usb/typec/tcpm/Kconfig
> index 557f392fe24d..8ba0fd85741f 100644
> --- a/drivers/usb/typec/tcpm/Kconfig
> +++ b/drivers/usb/typec/tcpm/Kconfig
> @@ -52,6 +52,14 @@ config TYPEC_FUSB302
>  	  Type-C Port Controller Manager to provide USB PD and USB
>  	  Type-C functionalities.
>  
> +config TYPEC_SAMA7G5
> +	tristate "Microchip SAMA7G5 Type-C Port Controller driver"
> +	select REGMAP_MMIO
> +	help
> +	  Say Y or M here if your system has SAMA7G5 TCPC controller.
> +	  It works with Type-C Port Controller Manager to provide USB
> +	  Type-C functionalities.
> +
>  config TYPEC_WCOVE
>  	tristate "Intel WhiskeyCove PMIC USB Type-C PHY driver"
>  	depends on ACPI
> diff --git a/drivers/usb/typec/tcpm/Makefile b/drivers/usb/typec/tcpm/Makefile
> index 7d499f3569fd..9abe8a7ae1cc 100644
> --- a/drivers/usb/typec/tcpm/Makefile
> +++ b/drivers/usb/typec/tcpm/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_TYPEC_TCPM)		+= tcpm.o
>  obj-$(CONFIG_TYPEC_FUSB302)		+= fusb302.o
> +obj-$(CONFIG_TYPEC_SAMA7G5)		+= sama7g5_tcpc.o
>  obj-$(CONFIG_TYPEC_WCOVE)		+= typec_wcove.o
>  typec_wcove-y				:= wcove.o
>  obj-$(CONFIG_TYPEC_TCPCI)		+= tcpci.o
> diff --git a/drivers/usb/typec/tcpm/sama7g5_tcpc.c b/drivers/usb/typec/tcpm/sama7g5_tcpc.c
> new file mode 100644
> index 000000000000..2986c0fcc8a3
> --- /dev/null
> +++ b/drivers/usb/typec/tcpm/sama7g5_tcpc.c
> @@ -0,0 +1,610 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Microchip SAMA7G5 Type-C Port Controller Driver
> + *
> + * Copyright (C) 2021 Microchip Technology, Inc. and its subsidiaries
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/usb/pd.h>
> +#include <linux/usb/tcpm.h>
> +#include <linux/usb/typec.h>
> +
> +#define SAMA7G5_TCPC_GCLK				32000
> +
> +/* TCPC registers offsets */
> +#define TCPC_CR			0x80		/* TCPC Control Register */
> +#define TCPC_UPC		0xA0		/* TCPC PHY Control Register */
> +#define TCPC_UPS		0xA4		/* TCPC PHY Status Register */
> +
> +#define TCPC_CR_RESET		0x54434301	/* Magic value */
> +
> +/* TCPC PHY Control Register */
> +#define TCPC_UPC_BCDETE		BIT(29)
> +#define TCPC_UPC_BCVSRCE	BIT(28)
> +#define	TCPC_UPC_BCDETSEL	BIT(27)

Why do you have a tab right after "#define" above?

> +#define	TCPC_UPC_BCIDPSRCE	BIT(26)

And here?

> +#define TCPC_UPC_DMPDFE		BIT(25)
> +#define TCPC_UPC_DMPDFD		BIT(24)
> +#define TCPC_UPC_IP_OFF		(0 << 12)
> +#define TCPC_UPC_IP_0P5		(1 << 12)
> +#define TCPC_UPC_IP_1P5		(2 << 12)
> +#define TCPC_UPC_IP_3P0		(3 << 12)
> +#define TCPC_UPC_THRESHOLD0	(0 << 8)
> +#define TCPC_UPC_THRESHOLD2	(2 << 8)
> +#define TCPC_UPC_THRESHOLD4	(4 << 8)
> +#define TCPC_UPC_THRESHOLD6	(6 << 8)
> +
> +/* TCPC PHY  Status Register */
> +#define TCPC_UPS_CC2RDT		BIT(4)
> +#define TCPC_UPS_CC1ID		BIT(3)
> +#define TCPC_UPS_CC_MASK	GENMASK(4, 3)
> +#define TCPC_UPS_CHGDCP		BIT(2)
> +#define TCPC_UPS_DM		BIT(1)
> +#define TCPC_UPS_DP		BIT(0)
> +
> +#define TCPC_VERSION		0xFC
> +
> +/* USB Type-C measurement timings */
> +#define T_CC_MEASURE		100 /* 100 ms */
> +
> +#define SAMA7G5_TCPC_VBUS_IRQFLAGS (IRQF_ONESHOT \
> +			   | IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING)
> +
> +struct sama7g5_tcpc {
> +	struct device *dev;
> +
> +	struct workqueue_struct *wq;
> +	struct delayed_work measure_work;
> +
> +	struct regmap *regmap;
> +	void __iomem *base;
> +
> +	struct clk *pclk;
> +	struct clk *gclk;
> +
> +	struct gpio_desc *vbus_pin;
> +	struct regulator *vbus;
> +
> +	/* lock for sharing states */
> +	struct mutex lock;
> +
> +	/* port status */
> +	enum typec_cc_polarity cc_polarity;
> +	enum typec_cc_status cc1_status;
> +	enum typec_cc_status cc2_status;
> +	enum typec_cc_status cc1_status_prev;
> +	enum typec_cc_status cc2_status_prev;
> +
> +	/* mutex used for VBUS detection */
> +	struct mutex vbus_mutex;
> +	int vbus_present;
> +	int vbus_present_prev;
> +
> +	unsigned int phy_status;
> +	unsigned int phy_status_old;
> +
> +	struct tcpc_dev tcpc;
> +	struct tcpm_port *tcpm;
> +};
> +
> +#define tcpc_to_sama7g5_tcpc(_tcpc_) \
> +		container_of(_tcpc_, struct sama7g5_tcpc, tcpc)
> +
> +static bool sama7g5_tcpc_readable_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case TCPC_CR:
> +	case TCPC_UPC:
> +	case TCPC_UPS:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static bool sama7g5_tcpc_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case TCPC_CR:
> +	case TCPC_UPC:
> +	case TCPC_UPS:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static const struct regmap_config sama7g5_tcpc_regmap_config = {
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 32,
> +	.max_register = TCPC_VERSION,
> +	.readable_reg = sama7g5_tcpc_readable_reg,
> +	.writeable_reg = sama7g5_tcpc_writeable_reg,
> +};
> +
> +static int sama7g5_tcpc_get_vbus(struct tcpc_dev *tcpc)
> +{
> +	struct sama7g5_tcpc *sama7g5_tcpc = tcpc_to_sama7g5_tcpc(tcpc);
> +	int ret;
> +
> +	mutex_lock(&sama7g5_tcpc->vbus_mutex);
> +	ret = sama7g5_tcpc->vbus_present ? 1 : 0;
> +	mutex_unlock(&sama7g5_tcpc->vbus_mutex);
> +
> +	return ret;
> +}
> +
> +static int sama7g5_tcpc_set_vbus(struct tcpc_dev *tcpc, bool on, bool sink)
> +{
> +	struct sama7g5_tcpc *sama7g5_tcpc = tcpc_to_sama7g5_tcpc(tcpc);
> +	int ret;
> +
> +	mutex_lock(&sama7g5_tcpc->vbus_mutex);
> +	if (on)
> +		ret = regulator_enable(sama7g5_tcpc->vbus);
> +	else
> +		ret = regulator_disable(sama7g5_tcpc->vbus);
> +	mutex_unlock(&sama7g5_tcpc->vbus_mutex);
> +
> +	return ret;
> +}
> +
> +static int sama7g5_tcpc_set_vconn(struct tcpc_dev *tcpc, bool on)
> +{
> +	/* VCONN is not supported */
> +	return -EPERM;
> +}
> +
> +static int sama7g5_tcpc_get_cc(struct tcpc_dev *tcpc, enum typec_cc_status *cc1,
> +			       enum typec_cc_status *cc2)
> +{
> +	struct sama7g5_tcpc *sama7g5_tcpc = tcpc_to_sama7g5_tcpc(tcpc);
> +
> +	mutex_lock(&sama7g5_tcpc->lock);
> +	*cc1 = sama7g5_tcpc->cc1_status;
> +	*cc2 = sama7g5_tcpc->cc2_status;
> +	mutex_unlock(&sama7g5_tcpc->lock);
> +
> +	return 0;
> +}
> +
> +static int sama7g5_tcpc_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status cc)
> +{
> +	struct sama7g5_tcpc *sama7g5_tcpc = tcpc_to_sama7g5_tcpc(tcpc);
> +	unsigned int ctrl;
> +	int ret = 0;
> +
> +	mutex_lock(&sama7g5_tcpc->lock);
> +	switch (cc) {
> +	case TYPEC_CC_RD:
> +		ctrl = TCPC_UPC_IP_OFF;
> +		break;
> +	case TYPEC_CC_RP_DEF:
> +		ctrl = TCPC_UPC_IP_0P5;
> +		break;
> +	default:
> +		ret =  -EINVAL;
> +		goto done;
> +	}
> +	ret = regmap_write(sama7g5_tcpc->regmap, TCPC_UPC, ctrl);
> +done:
> +	mutex_unlock(&sama7g5_tcpc->lock);
> +	return ret;
> +}
> +
> +static int sama7g5_tcpc_set_polarity(struct tcpc_dev *tcpc,
> +				     enum typec_cc_polarity pol)
> +{
> +	return 0;
> +}
> +
> +static int sama7g5_tcpc_set_roles(struct tcpc_dev *tcpc, bool attached,
> +			   enum typec_role role, enum typec_data_role data)
> +{
> +	return 0;
> +}
> +
> +static int sama7g5_tcpc_set_pd_rx(struct tcpc_dev *tcpc, bool on)
> +{
> +	return -EPERM;
> +}
> +
> +static int sama7g5_tcpc_pd_transmit(struct tcpc_dev *tcpc,
> +				    enum tcpm_transmit_type type,
> +				    const struct pd_message *msg,
> +				    unsigned int negotiated_rev)
> +{
> +	return -EPERM;
> +}
> +
> +static int sama7g5_tcpc_start_toggling(struct tcpc_dev *tcpc,
> +				       enum typec_port_type port_type,
> +				       enum typec_cc_status cc)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static void _sama7g5_tcpc_measure_snk(struct sama7g5_tcpc *sama7g5_tcpc)
> +{
> +	struct device *dev = sama7g5_tcpc->dev;
> +	int ret;
> +
> +	/* Save previous CC1/CC2 state */
> +	sama7g5_tcpc->cc1_status_prev = sama7g5_tcpc->cc1_status;
> +	sama7g5_tcpc->cc2_status_prev = sama7g5_tcpc->cc2_status;
> +
> +	/* Comparator Threshold 2 */
> +	ret = regmap_write(sama7g5_tcpc->regmap, TCPC_UPC, TCPC_UPC_IP_OFF |
> +			   TCPC_UPC_THRESHOLD2);

How about:

	ret = regmap_write(sama7g5_tcpc->regmap, TCPC_UPC,
                           TCPC_UPC_IP_OFF | TCPC_UPC_THRESHOLD2);

just to make it a bit easier to read?

> +	if (ret) {
> +		dev_err(dev, "failed to wite register: %d\n", ret);
> +		return;
> +	}
> +
> +	usleep_range(560, 1000);
> +
> +	ret = regmap_read(sama7g5_tcpc->regmap, TCPC_UPS,
> +			  &sama7g5_tcpc->phy_status);
> +	if (ret) {
> +		dev_err(dev, "failed to read register: %d\n", ret);
> +		return;
> +	}
> +
> +	if (!(sama7g5_tcpc->phy_status & TCPC_UPS_CC_MASK)) {
> +		/* VRa*/
> +		sama7g5_tcpc->cc1_status = TYPEC_CC_OPEN;
> +		sama7g5_tcpc->cc2_status = TYPEC_CC_OPEN;
> +		return;
> +	}
> +
> +	/* CC1 or CC2 is connected wait for PD messages to end ~ 30ms */
> +	usleep_range(30000, 35000);
> +
> +	/* Comparator Threshold 4 */
> +	sama7g5_tcpc->phy_status_old = sama7g5_tcpc->phy_status;
> +
> +	ret = regmap_write(sama7g5_tcpc->regmap, TCPC_UPC, TCPC_UPC_IP_OFF |
> +			   TCPC_UPC_THRESHOLD4);

And here:

	ret = regmap_write(sama7g5_tcpc->regmap, TCPC_UPC,
                           TCPC_UPC_IP_OFF | TCPC_UPC_THRESHOLD4);

> +	if (ret) {
> +		dev_err(dev, "failed to wite register: %d\n", ret);
> +		return;
> +	}
> +
> +	usleep_range(560, 1000);
> +	ret = regmap_read(sama7g5_tcpc->regmap, TCPC_UPS,
> +			  &sama7g5_tcpc->phy_status);
> +	if (ret) {
> +		dev_err(dev, "failed to read register: %d\n", ret);
> +		return;
> +	}
> +
> +	if ((sama7g5_tcpc->phy_status_old & TCPC_UPS_CC1ID) &&
> +	    (!(sama7g5_tcpc->phy_status & TCPC_UPS_CC1ID))) {
> +		sama7g5_tcpc->cc1_status = TYPEC_CC_RP_DEF;
> +		sama7g5_tcpc->cc2_status = TYPEC_CC_OPEN;
> +		return;
> +	}
> +
> +	if ((sama7g5_tcpc->phy_status_old & TCPC_UPS_CC2RDT) &&
> +	    (!(sama7g5_tcpc->phy_status & TCPC_UPS_CC2RDT))) {
> +		sama7g5_tcpc->cc1_status = TYPEC_CC_OPEN;
> +		sama7g5_tcpc->cc2_status = TYPEC_CC_RP_DEF;
> +		return;
> +	}
> +
> +	/* Comparator Threshold 6 */
> +	sama7g5_tcpc->phy_status_old = sama7g5_tcpc->phy_status;
> +
> +	ret = regmap_write(sama7g5_tcpc->regmap, TCPC_UPC, TCPC_UPC_IP_OFF |
> +			   TCPC_UPC_THRESHOLD6);

Ditto?

> +	if (ret) {
> +		dev_err(dev, "failed to wite register: %d\n", ret);
> +		return;
> +	}
> +
> +	usleep_range(560, 1000);
> +	ret = regmap_read(sama7g5_tcpc->regmap, TCPC_UPS,
> +			  &sama7g5_tcpc->phy_status);
> +	if (ret) {
> +		dev_err(dev, "failed to read register: %d\n", ret);
> +		return;
> +	}
> +
> +	if ((sama7g5_tcpc->phy_status_old & TCPC_UPS_CC1ID) &&
> +	    (!(sama7g5_tcpc->phy_status & TCPC_UPS_CC1ID))) {
> +		sama7g5_tcpc->cc1_status = TYPEC_CC_RP_1_5;
> +		sama7g5_tcpc->cc2_status = TYPEC_CC_OPEN;
> +		return;
> +	}
> +
> +	if ((sama7g5_tcpc->phy_status_old & TCPC_UPS_CC1ID) &&
> +	    ((sama7g5_tcpc->phy_status & TCPC_UPS_CC1ID))) {
> +		sama7g5_tcpc->cc1_status = TYPEC_CC_RP_3_0;
> +		sama7g5_tcpc->cc2_status = TYPEC_CC_OPEN;
> +		return;
> +	}
> +
> +	if ((sama7g5_tcpc->phy_status_old & TCPC_UPS_CC2RDT) &&
> +	    (!(sama7g5_tcpc->phy_status & TCPC_UPS_CC2RDT))) {
> +		sama7g5_tcpc->cc1_status = TYPEC_CC_OPEN;
> +		sama7g5_tcpc->cc2_status = TYPEC_CC_RP_1_5;
> +		return;
> +	}
> +
> +	if ((sama7g5_tcpc->phy_status_old & TCPC_UPS_CC2RDT) &&
> +	    ((sama7g5_tcpc->phy_status & TCPC_UPS_CC2RDT))) {
> +		sama7g5_tcpc->cc1_status = TYPEC_CC_OPEN;
> +		sama7g5_tcpc->cc2_status = TYPEC_CC_RP_3_0;
> +		return;
> +	}

It looks like you only use that phy_status_old member in this
function, so you could make it a local variable, no?

> +}
> +
> +static void sama7g5_tcpc_measure_work(struct work_struct *work)
> +{
> +	struct sama7g5_tcpc *port = container_of(work, struct sama7g5_tcpc,
> +						 measure_work.work);
> +
> +	mutex_lock(&port->lock);
> +
> +	_sama7g5_tcpc_measure_snk(port);
> +
> +	/* Check if the state has changed and notify TCPM */
> +	if (port->cc1_status != port->cc1_status_prev ||
> +	    port->cc2_status != port->cc2_status_prev)
> +		tcpm_cc_change(port->tcpm);

And those ccx_status_prev you are only using here, so you probable
don't need those members either.

> +	mod_delayed_work(port->wq, &port->measure_work,
> +			 msecs_to_jiffies(T_CC_MEASURE));
> +
> +	mutex_unlock(&port->lock);
> +}
> +
> +static int sama7g5_tcpc_init(struct tcpc_dev *tcpc)
> +{
> +	struct sama7g5_tcpc *sama7g5_tcpc = tcpc_to_sama7g5_tcpc(tcpc);
> +	int ret;
> +
> +	ret = regmap_write(sama7g5_tcpc->regmap, TCPC_CR, TCPC_CR_RESET);
> +	if (ret)
> +		return ret;
> +
> +	sama7g5_tcpc->wq =
> +		create_singlethread_workqueue(dev_name(sama7g5_tcpc->dev));

One line is enough for the above.

> +	if (!sama7g5_tcpc->wq)
> +		return -ENOMEM;
> +
> +	INIT_DELAYED_WORK(&sama7g5_tcpc->measure_work,
> +			  sama7g5_tcpc_measure_work);
> +
> +	sama7g5_tcpc->cc1_status = TYPEC_CC_OPEN;
> +	sama7g5_tcpc->cc2_status = TYPEC_CC_OPEN;
> +	sama7g5_tcpc->cc1_status_prev = TYPEC_CC_OPEN;
> +	sama7g5_tcpc->cc2_status_prev = TYPEC_CC_OPEN;
> +	sama7g5_tcpc->cc_polarity = TYPEC_POLARITY_CC1;
> +
> +	/* We do not have an interrupt so polling only */
> +	mod_delayed_work(sama7g5_tcpc->wq, &sama7g5_tcpc->measure_work,
> +			 msecs_to_jiffies(T_CC_MEASURE));
> +
> +	/* Enable VBUS detection */
> +	if (sama7g5_tcpc->vbus_pin)
> +		enable_irq(gpiod_to_irq(sama7g5_tcpc->vbus_pin));
> +
> +	return 0;
> +}
> +
> +static int vbus_is_present(struct sama7g5_tcpc *sama7g5_tcpc)
> +{
> +	if (sama7g5_tcpc->vbus_pin)
> +		return gpiod_get_value(sama7g5_tcpc->vbus_pin);
> +
> +	/* No Vbus detection: Assume always present */
> +	return 1;
> +}
> +
> +static irqreturn_t sama7g5_vbus_irq_thread(int irq, void *devid)
> +{
> +	struct sama7g5_tcpc *sama7g5_tcpc = devid;
> +
> +	/* debounce */
> +	udelay(10);
> +
> +	mutex_lock(&sama7g5_tcpc->vbus_mutex);
> +
> +	sama7g5_tcpc->vbus_present = vbus_is_present(sama7g5_tcpc);
> +	if (sama7g5_tcpc->vbus_present != sama7g5_tcpc->vbus_present_prev) {
> +		/* VBUS changed, notify TCPM */
> +		tcpm_vbus_change(sama7g5_tcpc->tcpm);
> +		sama7g5_tcpc->vbus_present_prev = sama7g5_tcpc->vbus_present;
> +	}

The vbus_present_prev looks like it could also be a local variable.

> +	mutex_unlock(&sama7g5_tcpc->vbus_mutex);
> +	return IRQ_HANDLED;
> +}
> +
> +static int sama7g5_tcpc_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct sama7g5_tcpc *sama7g5_tcpc;
> +
> +	struct resource *mem;
> +	void __iomem *base;
> +
> +	sama7g5_tcpc = devm_kzalloc(&pdev->dev, sizeof(*sama7g5_tcpc),
> +				    GFP_KERNEL);
> +	if (!sama7g5_tcpc)
> +		return -ENOMEM;
> +
> +	mem =  platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(&pdev->dev, mem);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +	sama7g5_tcpc->base = base;

        sama7g5_tcpc->base = devm_platform_ioremap_resourse(pdev, 0);
        if (IS_ERR(sama7g5_tcpc->base))
                return PTR_ERR(sama7g5_tcpc->base);

> +	sama7g5_tcpc->regmap =  devm_regmap_init_mmio(&pdev->dev, base,
> +						&sama7g5_tcpc_regmap_config);

You can align that properly too:

	sama7g5_tcpc->regmap =  devm_regmap_init_mmio(&pdev->dev, sama7g5_tcpc->base,
                                                      &sama7g5_tcpc_regmap_config);

> +	if (IS_ERR(sama7g5_tcpc->regmap)) {
> +		dev_err(&pdev->dev, "Regmap init failed\n");
> +		return PTR_ERR(sama7g5_tcpc->regmap);
> +	}
> +
> +	/* Get the peripheral clock */
> +	sama7g5_tcpc->pclk = devm_clk_get(&pdev->dev, "pclk");
> +	if (IS_ERR(sama7g5_tcpc->pclk)) {
> +		ret = PTR_ERR(sama7g5_tcpc->pclk);
> +		dev_err(&pdev->dev,
> +			"failed to get the peripheral clock: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(sama7g5_tcpc->pclk);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"failed to enable the peripheral clock: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Get the generic clock */
> +	sama7g5_tcpc->gclk = devm_clk_get(&pdev->dev, "gclk");
> +	if (IS_ERR(sama7g5_tcpc->gclk)) {
> +		ret = PTR_ERR(sama7g5_tcpc->gclk);
> +		dev_err(&pdev->dev,
> +			"failed to get the PMC generic clock: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = clk_set_rate(sama7g5_tcpc->gclk, SAMA7G5_TCPC_GCLK);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"unable to change gclk rate to: %u\n",
> +			SAMA7G5_TCPC_GCLK);
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(sama7g5_tcpc->gclk);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"failed to enable the generic clock: %d\n", ret);
> +		return ret;
> +	}
> +
> +	mutex_init(&sama7g5_tcpc->lock);
> +	mutex_init(&sama7g5_tcpc->vbus_mutex);
> +
> +	sama7g5_tcpc->vbus_pin = devm_gpiod_get_optional(&pdev->dev,
> +				"microchip,vbus", GPIOD_IN);

You can align that properly.

> +	if (IS_ERR(sama7g5_tcpc->vbus_pin)) {
> +		ret = PTR_ERR(sama7g5_tcpc->vbus_pin);
> +		dev_err(&pdev->dev, "unable to claim vbus-gpio: %d\n", ret);
> +	}
> +
> +	sama7g5_tcpc->vbus = devm_regulator_get_optional(&pdev->dev, "vbus");
> +
> +	if (IS_ERR(sama7g5_tcpc->vbus)) {
> +		ret = PTR_ERR(sama7g5_tcpc->vbus);
> +		dev_err(&pdev->dev, "unable to claim vbus-supply: %d\n", ret);
> +	}
> +
> +	if (sama7g5_tcpc->vbus_pin) {
> +		irq_set_status_flags(gpiod_to_irq(sama7g5_tcpc->vbus_pin),
> +				     IRQ_NOAUTOEN);
> +		ret = devm_request_threaded_irq(&pdev->dev,
> +				gpiod_to_irq(sama7g5_tcpc->vbus_pin), NULL,
> +				sama7g5_vbus_irq_thread,
> +				SAMA7G5_TCPC_VBUS_IRQFLAGS,
> +				"sama7g5_tcpc", sama7g5_tcpc);
> +		if (ret) {
> +			sama7g5_tcpc->vbus_pin = NULL;
> +			dev_warn(&pdev->dev,
> +				 "failed to request vbus irq; "
> +				 "assuming always on\n");
> +		}
> +	}
> +
> +	sama7g5_tcpc->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, sama7g5_tcpc);
> +
> +	sama7g5_tcpc->tcpc.init = sama7g5_tcpc_init;
> +	sama7g5_tcpc->tcpc.get_vbus = sama7g5_tcpc_get_vbus;
> +	sama7g5_tcpc->tcpc.set_vbus = sama7g5_tcpc_set_vbus;
> +	sama7g5_tcpc->tcpc.set_cc = sama7g5_tcpc_set_cc;
> +	sama7g5_tcpc->tcpc.get_cc = sama7g5_tcpc_get_cc;
> +	sama7g5_tcpc->tcpc.set_polarity = sama7g5_tcpc_set_polarity;
> +	sama7g5_tcpc->tcpc.set_vconn = sama7g5_tcpc_set_vconn;
> +	sama7g5_tcpc->tcpc.start_toggling = sama7g5_tcpc_start_toggling;
> +	sama7g5_tcpc->tcpc.set_pd_rx = sama7g5_tcpc_set_pd_rx;
> +	sama7g5_tcpc->tcpc.set_roles = sama7g5_tcpc_set_roles;
> +	sama7g5_tcpc->tcpc.pd_transmit = sama7g5_tcpc_pd_transmit;
> +
> +	sama7g5_tcpc->tcpc.fwnode = device_get_named_child_node(&pdev->dev,
> +								"connector");
> +	if (!sama7g5_tcpc->tcpc.fwnode) {
> +		dev_err(&pdev->dev, "Can't find connector node.\n");
> +		return -EINVAL;
> +	}
> +
> +	sama7g5_tcpc->tcpm = tcpm_register_port(sama7g5_tcpc->dev,
> +						&sama7g5_tcpc->tcpc);
> +	if (IS_ERR(sama7g5_tcpc->tcpm)) {
> +		fwnode_remove_software_node(sama7g5_tcpc->tcpc.fwnode);
> +		return PTR_ERR(sama7g5_tcpc->tcpm);
> +	}
> +
> +	return 0;
> +}
> +
> +static int sama7g5_tcpc_remove(struct platform_device *pdev)
> +{
> +	struct sama7g5_tcpc *sama7g5_tcpc;
> +
> +	sama7g5_tcpc = platform_get_drvdata(pdev);
> +
> +	/* Mask everything */
> +	if (sama7g5_tcpc->vbus_pin)
> +		disable_irq(gpiod_to_irq(sama7g5_tcpc->vbus_pin));
> +
> +
> +	if (!IS_ERR_OR_NULL(sama7g5_tcpc->tcpm))
> +		tcpm_unregister_port(sama7g5_tcpc->tcpm);
> +
> +	destroy_workqueue(sama7g5_tcpc->wq);
> +
> +	clk_disable_unprepare(sama7g5_tcpc->gclk);
> +	clk_disable_unprepare(sama7g5_tcpc->pclk);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id sama7g5_tcpc_dt_ids[] = {
> +	{
> +		.compatible = "microchip,sama7g5-tcpc",
> +	},
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, sama7g5_tcpc_dt_ids);
> +
> +static struct platform_driver sama7g5_tcpc_driver = {
> +	.probe	= sama7g5_tcpc_probe,
> +	.remove = sama7g5_tcpc_remove,
> +	.driver = {
> +		.name	= "microchip,sama7g5-tcpc",
> +		.of_match_table	= sama7g5_tcpc_dt_ids,
> +	},
> +};
> +module_platform_driver(sama7g5_tcpc_driver);
> +
> +MODULE_AUTHOR("Cristian Birsan <cristian.birsan@microchip.com>");
> +MODULE_DESCRIPTION("Microchip SAMA7G5 Type-C Port Controller Driver");
> +MODULE_LICENSE("GPL");

thanks,

-- 
heikki

Re: [RFC PATCH v2 0/2] usb: typec: Add driver for Microchip sama7g5 tcpc

[Cristian.Birsan]

On 3/31/21 9:21 AM, Greg KH wrote:
> 
> On Tue, Mar 30, 2021 at 11:54:40PM +0300, cristian.birsan@microchip.com wrote:
>> From: Cristian Birsan <cristian.birsan@microchip.com>
>>
>> This patch set adds initial driver support for Microchip USB Type-C Port
>> Controller (TCPC) embedded in sama7g5 SoC.
>>
>> The controller does not implement power delivery and the driver uses dummy
>> functions to register the port with TCPM. The current silicon version is
>> not able to trigger interrupts so the driver will poll for changes on
>> CC1/CC2 lines.
>>
>> Support for sink is implemented and tested with an USB device. The plan is
>> to extend the driver and add source support.
> 
> Why are these marked "RFC"?

I sent the patch as RFC because I wanted to gather feedback on it and on how
it fits under Type-C/TCPM subsystem.

> 
> Do you really not think they should be accepted?  Why not, what is left
> to do with them?
> 

I think the driver can be merged after I address the review received on the
mailing list. At the same time, I plan to enhance it with the following:

- add sink detection at power source and check it with USB Host.

- add a way to notify VBUS presence back to USB gadget.  As TCPC needs to
detect VBUS and report it to TCPM, I moved the VBUS detection gpio from the
USB gadget device tree node to TCPC. For now, the gadget always assumes that
VBUS is on. I saw that some USB PHY drivers use a notifier chain. I can
implement something similar.
  
> I do not normally review "RFC" patches as the authors do not think they
> should be merged, and we have plenty of patches that are being asked to
> be merged already :)

Until now I received a review from Heikki Krogerus. I will let this version
of the patch on the mailing list for some time in case there is additional
feedback.

> 
> thanks,
> 
> greg k-h
> 

Regards,
Cristian

Re: [RFC PATCH v2 1/2] dt-bindings: usb: Add DT bindings for Microchip sama7g5 tcpc

[Rob Herring]

On Tue, Mar 30, 2021 at 11:54:41PM +0300, cristian.birsan@microchip.com wrote:
> From: Cristian Birsan <cristian.birsan@microchip.com>
> 
> This patch adds DT bindings for the new Microchip USB Type-C Port
> Controller (TCPC) embedded in sama7g5 SoC.
> 
> Signed-off-by: Cristian Birsan <cristian.birsan@microchip.com>
> ---
>  .../bindings/usb/microchip,sama7g5-tcpc.yaml  | 90 +++++++++++++++++++
>  1 file changed, 90 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/microchip,sama7g5-tcpc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/usb/microchip,sama7g5-tcpc.yaml b/Documentation/devicetree/bindings/usb/microchip,sama7g5-tcpc.yaml
> new file mode 100644
> index 000000000000..2d3289dd9ac9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/microchip,sama7g5-tcpc.yaml
> @@ -0,0 +1,90 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/usb/microchip,sama7g5-tcpc.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Microchip SAMA7G5 Type-C port controller bindings
> +
> +maintainers:
> +  - Cristian Birsan <cristian.birsan@microchip.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - microchip,sama7g5-tcpc
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: Peripheral Bus Clock
> +      - description: Generic Clock
> +
> +  clock-names:
> +    items:
> +      - const: pclk
> +      - const: gclk
> +
> +  vbus-supply:
> +    description: vbus power supply (5V) phandle

This belongs in the connector node unless this chip uses Vbus as a 
supply.

> +
> +  microchip,vbus-gpio:
> +    description: vbus detect gpio

We have standard property for this in the connector node.

> +
> +  connector:
> +    type: object
> +
> +    allOf:
> +      - $ref: ../connector/usb-connector.yaml

You can drop 'allOf'. And add:

       unevaluatedProperties: false

> +
> +    properties:
> +      compatible:
> +        const: usb-c-connector
> +
> +      power-role: true
> +
> +    required:
> +      - compatible
> +
> +required:
> +  - compatible
> +  - reg
> +  - connector
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/at91.h>
> +    #include <dt-bindings/usb/pd.h>
> +    tcpcb: tcpcb@e0844000 {
> +      compatible = "microchip,sama7g5-tcpc";
> +      reg = <0xe0844000 0x3fff>;
> +      clocks = <&pmc PMC_TYPE_PERIPHERAL 95>, <&pmc PMC_TYPE_GCK 95>;
> +      clock-names = "pclk", "gclk";
> +
> +      usb_con: connector {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        compatible = "usb-c-connector";
> +        label = "USB-C";
> +        data-role = "device";
> +        power-role = "sink";
> +        try-power-role = "sink";
> +        source-pdos = <PDO_FIXED(5000, 500,
> +            PDO_FIXED_USB_COMM)>;
> +        sink-pdos = <PDO_FIXED(5000, 500,
> +            PDO_FIXED_USB_COMM)>;
> +        op-sink-microwatt = <2500000>;
> +
> +        port@1 {
> +          reg = <1>;
> +          usb_con_hs: endpoint {
> +            remote-endpoint = <&usb_phy1>;
> +          };
> +        };
> +      };
> +    };
> +...
> -- 
> 2.25.1
>