Re: [PATCH v3 2/2] usb: phy: Add USB3 PHY support for Intel LGM SoC

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Andy,

Thank you very much for the review comments and your time...

On 12/6/2020 9:18 pm, Andy Shevchenko wrote:
On Fri, Jun 12, 2020 at 10:59:41AM +0800, Ramuthevar,Vadivel MuruganX wrote:
From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@xxxxxxxxxxxxxxx>

Add support for USB PHY on Intel LGM SoC.

Thank you for an update, looks pretty much good, my comments below.
Sure, will update.

...

+static int get_flipped(struct tca_apb *ta, bool *flipped)
+{
+	union extcon_property_value property;
+	int ret;
+
+	ret = extcon_get_property(ta->phy.edev, EXTCON_USB_HOST,
+				  EXTCON_PROP_USB_TYPEC_POLARITY, &property);
+	if (ret) {

+		dev_err(ta->phy.dev, "no polarity property from extcon\n");

(1)
Noted.

+		return ret;
+	}
+
+	*flipped = property.intval;
+
+	return ret;
+}

...

+	ret = get_flipped(ta, &flipped);
+	if (ret)
+		dev_err(ta->phy.dev, "no polarity property from extcon\n");

You already has a message in (1). You should decide which one to leave.

But note, if it's a fatal error, you have to return here, otherwise, if you
decide to leave message here, it should be not on error level.
Agreed!, let double and update as per your suggestion if it is fatal error.

+	connected = extcon_get_state(ta->phy.edev, EXTCON_USB_HOST);
+	if (connected == ta->connected)
+		return;
+
+	ta->connected = connected;
+	if (connected) {
+		val = TCPC_VALID | FIELD_PREP(TCPC_MUX_CTL, MUX_USB);
+		if (flipped)
+			val |= TCPC_FLIPPED;
+		dev_info(ta->phy.dev, "connected%s\n", flipped ? " flipped" : "");
+	} else {
+		val = TCPC_DISCONN;
+		dev_info(ta->phy.dev, "disconnected\n");
+	}
+
+	writel(val, ta->phy.io_priv + TCPC_OFFSET);
+

+	if (ta->phy.set_vbus(&ta->phy, connected))
+		dev_err(ta->phy.dev, "failed to set VBUS\n");

Please, split it to
	ret = ...;
Noted, will update.
	if (ret)

+}

...

+static int vbus_notifier(struct notifier_block *nb,
+			 unsigned long event, void *ptr)

Consider to put it on one line (you can also shrink the names of unused
parameters.
Sure, will try to reduce the length of the variable names.

Regards
Vadivel

+{
+	return NOTIFY_DONE;
+}




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux