The following series introduces a Type C port driver for Chrome OS devices
that have an EC (Embedded Controller). It derives port information from
ACPI or DT entries. This patch series adds basic support, including
registering ports, and setting certain basic attributes.
Prashant Malani (3):
platform/chrome: Add Type C connector class driver
platform/chrome: typec: Get PD_CONTROL cmd version
platform/chrome: typec: Update port info from EC
drivers/platform/chrome/Kconfig | 11 +
drivers/platform/chrome/Makefile | 1 +
drivers/platform/chrome/cros_ec_typec.c | 346 ++++++++++++++++++++++++
3 files changed, 358 insertions(+)
create mode 100644 drivers/platform/chrome/cros_ec_typec.c
--
2.25.0.341.g760bfbb309-goog
Add a driver to implement the Type C connector class for Chrome OS
devices with ECs (Embedded Controllers).
The driver relies on firmware device specifications for various port
attributes. On ACPI platforms, this is specified using the logical
device with HID GOOG0014. On DT platforms, this is specified using the
DT node with compatible string "google,cros-ec-typec".
This patch reads the device FW node and uses the port attributes to
register the typec ports with the Type C connector class framework, but
doesn't do much else.
Subsequent patches will add more functionality to the driver, including
obtaining current port information (polarity, vconn role, current power
role etc.) after querying the EC.
Signed-off-by: Prashant Malani <[email protected]>
---
drivers/platform/chrome/Kconfig | 11 ++
drivers/platform/chrome/Makefile | 1 +
drivers/platform/chrome/cros_ec_typec.c | 228 ++++++++++++++++++++++++
3 files changed, 240 insertions(+)
create mode 100644 drivers/platform/chrome/cros_ec_typec.c
diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
index 5f57282a28da00..1370dfd1ca1565 100644
--- a/drivers/platform/chrome/Kconfig
+++ b/drivers/platform/chrome/Kconfig
@@ -214,6 +214,17 @@ config CROS_EC_SYSFS
To compile this driver as a module, choose M here: the
module will be called cros_ec_sysfs.
+config CROS_EC_TYPEC
+ tristate "ChromeOS EC Type-C Connector Control"
+ depends on MFD_CROS_EC_DEV && TYPEC
+ default n
+ help
+ If you say Y here, you get support for accessing Type C connector
+ information from the Chrome OS EC.
+
+ To compile this driver as a module, choose M here: the module will be
+ called cros_ec_typec.
+
config CROS_USBPD_LOGGER
tristate "Logging driver for USB PD charger"
depends on CHARGER_CROS_USBPD
diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
index aacd5920d8a180..caf2a9cdb5e6d1 100644
--- a/drivers/platform/chrome/Makefile
+++ b/drivers/platform/chrome/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_CROS_EC_ISHTP) += cros_ec_ishtp.o
obj-$(CONFIG_CROS_EC_RPMSG) += cros_ec_rpmsg.o
obj-$(CONFIG_CROS_EC_SPI) += cros_ec_spi.o
cros_ec_lpcs-objs := cros_ec_lpc.o cros_ec_lpc_mec.o
+obj-$(CONFIG_CROS_EC_TYPEC) += cros_ec_typec.o
obj-$(CONFIG_CROS_EC_LPC) += cros_ec_lpcs.o
obj-$(CONFIG_CROS_EC_PROTO) += cros_ec_proto.o cros_ec_trace.o
obj-$(CONFIG_CROS_KBD_LED_BACKLIGHT) += cros_kbd_led_backlight.o
diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
new file mode 100644
index 00000000000000..fe5659171c2a85
--- /dev/null
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -0,0 +1,228 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2020 Google LLC
+ *
+ * This driver provides the ability to view and manage Type C ports through the
+ * Chrome OS EC.
+ */
+
+#include <linux/acpi.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_data/cros_ec_commands.h>
+#include <linux/platform_data/cros_ec_proto.h>
+#include <linux/platform_device.h>
+#include <linux/usb/typec.h>
+
+#define DRV_NAME "cros-ec-typec"
+
+/* Platform-specific data for the Chrome OS EC Type C controller. */
+struct cros_typec_data {
+ struct device *dev;
+ struct cros_ec_device *ec;
+ int num_ports;
+ /* Array of ports, indexed by port number. */
+ struct typec_port *ports[EC_USB_PD_MAX_PORTS];
+};
+
+int cros_typec_parse_port_props(struct typec_capability *cap,
+ struct fwnode_handle *fwnode,
+ struct device *dev)
+{
+ const char *buf;
+ int ret;
+
+ memset(cap, 0, sizeof(*cap));
+ ret = fwnode_property_read_string(fwnode, "power-role", &buf);
+ if (ret) {
+ dev_err(dev, "power-role not found: %d\n", ret);
+ return ret;
+ }
+
+ ret = typec_find_port_power_role(buf);
+ if (ret < 0)
+ return ret;
+ cap->type = ret;
+
+ ret = fwnode_property_read_string(fwnode, "data-role", &buf);
+ if (ret) {
+ dev_err(dev, "data-role not found: %d\n", ret);
+ return ret;
+ }
+
+ ret = typec_find_port_data_role(buf);
+ if (ret < 0)
+ return ret;
+ cap->data = ret;
+
+ ret = fwnode_property_read_string(fwnode, "try-power-role", &buf);
+ if (ret) {
+ dev_err(dev, "try-power-role not found: %d\n", ret);
+ return ret;
+ }
+
+ ret = typec_find_power_role(buf);
+ if (ret < 0)
+ return ret;
+ cap->prefer_role = ret;
+
+ cap->fwnode = fwnode;
+
+ return 0;
+}
+
+static int cros_typec_init_ports(struct cros_typec_data *typec)
+{
+ struct device *dev = typec->dev;
+ struct typec_capability cap;
+ struct fwnode_handle *fwnode;
+ int ret;
+ int i;
+ int nports;
+ u32 port_num;
+
+ nports = device_get_child_node_count(dev);
+ if (nports == 0) {
+ dev_err(dev, "No port entries found.\n");
+ return -ENODEV;
+ }
+
+ device_for_each_child_node(dev, fwnode) {
+ if (fwnode_property_read_u32(fwnode, "port-number",
+ &port_num)) {
+ dev_err(dev, "No port-number for port, skipping.\n");
+ ret = -EINVAL;
+ goto unregister_ports;
+ }
+
+ if (port_num >= typec->num_ports) {
+ dev_err(dev, "Invalid port number.\n");
+ ret = -EINVAL;
+ goto unregister_ports;
+ }
+
+ dev_dbg(dev, "Registering port %d\n", port_num);
+ ret = cros_typec_parse_port_props(&cap, fwnode, dev);
+ if (ret < 0)
+ goto unregister_ports;
+ typec->ports[port_num] = typec_register_port(dev, &cap);
+ if (IS_ERR(typec->ports[port_num])) {
+ dev_err(dev, "Failed to register port %d\n", port_num);
+ ret = PTR_ERR(typec->ports[port_num]);
+ goto unregister_ports;
+ }
+ }
+
+ return 0;
+
+unregister_ports:
+ for (i = 0; i < typec->num_ports; i++)
+ typec_unregister_port(typec->ports[i]);
+ return ret;
+}
+
+static int cros_typec_ec_command(struct cros_typec_data *typec,
+ unsigned int version,
+ unsigned int command,
+ void *outdata,
+ unsigned int outsize,
+ void *indata,
+ unsigned int insize)
+{
+ struct cros_ec_command *msg;
+ int ret;
+
+ msg = kzalloc(sizeof(*msg) + max(outsize, insize), GFP_KERNEL);
+ if (!msg)
+ return -ENOMEM;
+
+ msg->version = version;
+ msg->command = command;
+ msg->outsize = outsize;
+ msg->insize = insize;
+
+ if (outsize)
+ memcpy(msg->data, outdata, outsize);
+
+ ret = cros_ec_cmd_xfer_status(typec->ec, msg);
+ if (ret >= 0 && insize)
+ memcpy(indata, msg->data, insize);
+
+ kfree(msg);
+ return ret;
+}
+
+
+static int cros_typec_get_num_ports(struct cros_typec_data *typec)
+{
+ struct ec_response_usb_pd_ports resp;
+ int ret;
+
+ ret = cros_typec_ec_command(typec, 0, EC_CMD_USB_PD_PORTS, NULL, 0,
+ &resp, sizeof(resp));
+ if (ret < 0)
+ return ret;
+
+ typec->num_ports = resp.num_ports;
+ if (typec->num_ports > EC_USB_PD_MAX_PORTS) {
+ dev_warn(typec->dev,
+ "Too many ports reported: %d, limiting to max: %d\n",
+ typec->num_ports, EC_USB_PD_MAX_PORTS);
+ }
+
+ return 0;
+}
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id cros_typec_acpi_id[] = {
+ { "GOOG0014", 0 },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(acpi, cros_typec_acpi_id);
+#endif
+
+#ifdef CONFIG_OF
+static const struct of_device_id cros_typec_of_match[] = {
+ { .compatible = "google,cros-ec-typec", },
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, cros_typec_of_match);
+#endif
+
+static int cros_typec_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct cros_typec_data *typec;
+ int ret;
+
+ typec = devm_kzalloc(dev, sizeof(*typec), GFP_KERNEL);
+ if (!typec)
+ return -ENOMEM;
+ typec->dev = dev;
+ typec->ec = dev_get_drvdata(pdev->dev.parent);
+ platform_set_drvdata(pdev, typec);
+
+ ret = cros_typec_get_num_ports(typec);
+ if (ret < 0)
+ return ret;
+
+ ret = cros_typec_init_ports(typec);
+ if (!ret)
+ return ret;
+
+ return 0;
+}
+
+static struct platform_driver cros_typec_driver = {
+ .driver = {
+ .name = DRV_NAME,
+ .acpi_match_table = ACPI_PTR(cros_typec_acpi_id),
+ .of_match_table = of_match_ptr(cros_typec_of_match),
+ },
+ .probe = cros_typec_probe,
+};
+
+module_platform_driver(cros_typec_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Chrome OS EC Type C control");
--
2.25.0.341.g760bfbb309-goog
Query the EC to determine the version number of the PD_CONTROL
command which is supported by the EC. Also store this value in the Type
C data struct since it will be used to determine how to parse the
response to queries for port information from the EC.
Signed-off-by: Prashant Malani <[email protected]>
---
drivers/platform/chrome/cros_ec_typec.c | 34 ++++++++++++++++++++++++-
1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index fe5659171c2a85..b8edcfaa8a5297 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -21,6 +21,7 @@ struct cros_typec_data {
struct device *dev;
struct cros_ec_device *ec;
int num_ports;
+ unsigned int cmd_ver;
/* Array of ports, indexed by port number. */
struct typec_port *ports[EC_USB_PD_MAX_PORTS];
};
@@ -173,6 +174,31 @@ static int cros_typec_get_num_ports(struct cros_typec_data *typec)
return 0;
}
+static int cros_typec_get_cmd_version(struct cros_typec_data *typec)
+{
+ struct ec_params_get_cmd_versions_v1 req_v1;
+ struct ec_response_get_cmd_versions resp;
+ int ret;
+
+ /* We're interested in the PD control command version. */
+ req_v1.cmd = EC_CMD_USB_PD_CONTROL;
+ ret = cros_typec_ec_command(typec, 1, EC_CMD_GET_CMD_VERSIONS,
+ &req_v1, sizeof(req_v1), &resp,
+ sizeof(resp));
+ if (ret < 0)
+ return ret;
+
+ if (resp.version_mask & EC_VER_MASK(1))
+ typec->cmd_ver = 1;
+ else
+ typec->cmd_ver = 0;
+
+ dev_dbg(typec->dev, "PD Control has version mask 0x%hhx\n",
+ typec->cmd_ver);
+
+ return 0;
+}
+
#ifdef CONFIG_ACPI
static const struct acpi_device_id cros_typec_acpi_id[] = {
{ "GOOG0014", 0 },
@@ -206,8 +232,14 @@ static int cros_typec_probe(struct platform_device *pdev)
if (ret < 0)
return ret;
+ ret = cros_typec_get_cmd_version(typec);
+ if (ret < 0) {
+ dev_err(dev, "failed to get PD command version info\n");
+ return ret;
+ }
+
ret = cros_typec_init_ports(typec);
- if (!ret)
+ if (ret < 0)
return ret;
return 0;
--
2.25.0.341.g760bfbb309-goog
After registering the ports at probe, get the current port information
from EC and update the Type C connector class ports accordingly.
Co-developed-by: Jon Flatley <[email protected]>
Signed-off-by: Prashant Malani <[email protected]>
---
drivers/platform/chrome/cros_ec_typec.c | 88 ++++++++++++++++++++++++-
1 file changed, 87 insertions(+), 1 deletion(-)
diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index b8edcfaa8a5297..f6aeba5bdc4972 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -153,6 +153,80 @@ static int cros_typec_ec_command(struct cros_typec_data *typec,
return ret;
}
+static void cros_typec_set_port_params_v0(struct cros_typec_data *typec,
+ int port_num, struct ec_response_usb_pd_control *resp)
+{
+ struct typec_port *port = typec->ports[port_num];
+ enum typec_orientation polarity;
+
+ if (!resp->enabled)
+ polarity = TYPEC_ORIENTATION_NONE;
+ else if (!resp->polarity)
+ polarity = TYPEC_ORIENTATION_NORMAL;
+ else
+ polarity = TYPEC_ORIENTATION_REVERSE;
+
+ typec_set_pwr_role(port, resp->role ? TYPEC_SOURCE : TYPEC_SINK);
+ typec_set_orientation(port, polarity);
+}
+
+static void cros_typec_set_port_params_v1(struct cros_typec_data *typec,
+ int port_num, struct ec_response_usb_pd_control_v1 *resp)
+{
+ struct typec_port *port = typec->ports[port_num];
+ enum typec_orientation polarity;
+
+ if (!(resp->enabled & PD_CTRL_RESP_ENABLED_CONNECTED))
+ polarity = TYPEC_ORIENTATION_NONE;
+ else if (!resp->polarity)
+ polarity = TYPEC_ORIENTATION_NORMAL;
+ else
+ polarity = TYPEC_ORIENTATION_REVERSE;
+ typec_set_orientation(port, polarity);
+ typec_set_data_role(port, resp->role & PD_CTRL_RESP_ROLE_DATA ?
+ TYPEC_HOST : TYPEC_DEVICE);
+ typec_set_pwr_role(port, resp->role & PD_CTRL_RESP_ROLE_POWER ?
+ TYPEC_SOURCE : TYPEC_SINK);
+ typec_set_vconn_role(port, resp->role & PD_CTRL_RESP_ROLE_VCONN ?
+ TYPEC_SOURCE : TYPEC_SINK);
+}
+
+static int cros_typec_port_update(struct cros_typec_data *typec, int port_num)
+{
+ struct ec_params_usb_pd_control req;
+ struct ec_response_usb_pd_control_v1 resp;
+ int ret;
+
+ if (port_num < 0 || port_num >= typec->num_ports) {
+ dev_err(typec->dev, "cannot get status for invalid port %d\n",
+ port_num);
+ return -EINVAL;
+ }
+
+ req.port = port_num;
+ req.role = USB_PD_CTRL_ROLE_NO_CHANGE;
+ req.mux = USB_PD_CTRL_MUX_NO_CHANGE;
+ req.swap = USB_PD_CTRL_SWAP_NONE;
+
+ ret = cros_typec_ec_command(typec, typec->cmd_ver,
+ EC_CMD_USB_PD_CONTROL, &req, sizeof(req),
+ &resp, sizeof(resp));
+ if (ret < 0)
+ return ret;
+
+ dev_dbg(typec->dev, "Enabled %d: 0x%hhx\n", port_num, resp.enabled);
+ dev_dbg(typec->dev, "Role %d: 0x%hhx\n", port_num, resp.role);
+ dev_dbg(typec->dev, "Polarity %d: 0x%hhx\n", port_num, resp.polarity);
+ dev_dbg(typec->dev, "State %d: %s\n", port_num, resp.state);
+
+ if (typec->cmd_ver == 1)
+ cros_typec_set_port_params_v1(typec, port_num, &resp);
+ else
+ cros_typec_set_port_params_v0(typec, port_num,
+ (struct ec_response_usb_pd_control *) &resp);
+
+ return 0;
+}
static int cros_typec_get_num_ports(struct cros_typec_data *typec)
{
@@ -219,7 +293,7 @@ static int cros_typec_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct cros_typec_data *typec;
- int ret;
+ int ret, i;
typec = devm_kzalloc(dev, sizeof(*typec), GFP_KERNEL);
if (!typec)
@@ -242,7 +316,19 @@ static int cros_typec_probe(struct platform_device *pdev)
if (ret < 0)
return ret;
+ for (i = 0; i < typec->num_ports; i++) {
+ ret = cros_typec_port_update(typec, i);
+ if (ret < 0)
+ goto unregister_ports;
+ }
+
return 0;
+
+unregister_ports:
+ for (i = 0; i < typec->num_ports; i++)
+ if (typec->ports[i])
+ typec_unregister_port(typec->ports[i]);
+ return ret;
}
static struct platform_driver cros_typec_driver = {
--
2.25.0.341.g760bfbb309-goog
Hi Prashant,
On 5/2/20 21:59, Prashant Malani wrote:
> Add a driver to implement the Type C connector class for Chrome OS
> devices with ECs (Embedded Controllers).
>
> The driver relies on firmware device specifications for various port
> attributes. On ACPI platforms, this is specified using the logical
> device with HID GOOG0014. On DT platforms, this is specified using the
> DT node with compatible string "google,cros-ec-typec".
>
If that's the case you should document this in a binding file. Will be this
driver a replacement of the cros-ec-extcon driver or is a different thing?
There is a device where I can test this?
> This patch reads the device FW node and uses the port attributes to
> register the typec ports with the Type C connector class framework, but
> doesn't do much else.
>
> Subsequent patches will add more functionality to the driver, including
> obtaining current port information (polarity, vconn role, current power
> role etc.) after querying the EC.
>
> Signed-off-by: Prashant Malani <[email protected]>
> ---
> drivers/platform/chrome/Kconfig | 11 ++
> drivers/platform/chrome/Makefile | 1 +
> drivers/platform/chrome/cros_ec_typec.c | 228 ++++++++++++++++++++++++
> 3 files changed, 240 insertions(+)
> create mode 100644 drivers/platform/chrome/cros_ec_typec.c
>
> diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
> index 5f57282a28da00..1370dfd1ca1565 100644
> --- a/drivers/platform/chrome/Kconfig
> +++ b/drivers/platform/chrome/Kconfig
> @@ -214,6 +214,17 @@ config CROS_EC_SYSFS
> To compile this driver as a module, choose M here: the
> module will be called cros_ec_sysfs.
>
> +config CROS_EC_TYPEC
> + tristate "ChromeOS EC Type-C Connector Control"
> + depends on MFD_CROS_EC_DEV && TYPEC
> + default n
Default value is already n, so you don't need to put it here. But I'd say that
we might be interested on have default MFD_CROS_EC_DEV like the other drivers.
> + help
> + If you say Y here, you get support for accessing Type C connector
> + information from the Chrome OS EC.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called cros_ec_typec.
> +
> config CROS_USBPD_LOGGER
> tristate "Logging driver for USB PD charger"
> depends on CHARGER_CROS_USBPD
> diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
> index aacd5920d8a180..caf2a9cdb5e6d1 100644
> --- a/drivers/platform/chrome/Makefile
> +++ b/drivers/platform/chrome/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_CROS_EC_ISHTP) += cros_ec_ishtp.o
> obj-$(CONFIG_CROS_EC_RPMSG) += cros_ec_rpmsg.o
> obj-$(CONFIG_CROS_EC_SPI) += cros_ec_spi.o
> cros_ec_lpcs-objs := cros_ec_lpc.o cros_ec_lpc_mec.o
> +obj-$(CONFIG_CROS_EC_TYPEC) += cros_ec_typec.o
> obj-$(CONFIG_CROS_EC_LPC) += cros_ec_lpcs.o
> obj-$(CONFIG_CROS_EC_PROTO) += cros_ec_proto.o cros_ec_trace.o
> obj-$(CONFIG_CROS_KBD_LED_BACKLIGHT) += cros_kbd_led_backlight.o
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> new file mode 100644
> index 00000000000000..fe5659171c2a85
> --- /dev/null
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -0,0 +1,228 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2020 Google LLC
> + *
> + * This driver provides the ability to view and manage Type C ports through the
> + * Chrome OS EC.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> +#include <linux/platform_device.h>
> +#include <linux/usb/typec.h>
> +
> +#define DRV_NAME "cros-ec-typec"
> +
> +/* Platform-specific data for the Chrome OS EC Type C controller. */
> +struct cros_typec_data {
> + struct device *dev;
> + struct cros_ec_device *ec;
> + int num_ports;
> + /* Array of ports, indexed by port number. */
> + struct typec_port *ports[EC_USB_PD_MAX_PORTS];
> +};
> +
> +int cros_typec_parse_port_props(struct typec_capability *cap,
static int
> + struct fwnode_handle *fwnode,
> + struct device *dev)
> +{
> + const char *buf;
> + int ret;
> +
> + memset(cap, 0, sizeof(*cap));
> + ret = fwnode_property_read_string(fwnode, "power-role", &buf);
> + if (ret) {
> + dev_err(dev, "power-role not found: %d\n", ret);
> + return ret;
> + }
> +
> + ret = typec_find_port_power_role(buf);
> + if (ret < 0)
> + return ret;
> + cap->type = ret;
> +
> + ret = fwnode_property_read_string(fwnode, "data-role", &buf);
> + if (ret) {
> + dev_err(dev, "data-role not found: %d\n", ret);
> + return ret;
> + }
> +
> + ret = typec_find_port_data_role(buf);
> + if (ret < 0)
> + return ret;
> + cap->data = ret;
> +
> + ret = fwnode_property_read_string(fwnode, "try-power-role", &buf);
> + if (ret) {
> + dev_err(dev, "try-power-role not found: %d\n", ret);
> + return ret;
> + }
> +
> + ret = typec_find_power_role(buf);
> + if (ret < 0)
> + return ret;
> + cap->prefer_role = ret;
> +
> + cap->fwnode = fwnode;
> +
> + return 0;
> +}
> +
> +static int cros_typec_init_ports(struct cros_typec_data *typec)
> +{
> + struct device *dev = typec->dev;
> + struct typec_capability cap;
> + struct fwnode_handle *fwnode;
> + int ret;
> + int i;
> + int nports;
> + u32 port_num;
> +
> + nports = device_get_child_node_count(dev);
> + if (nports == 0) {
> + dev_err(dev, "No port entries found.\n");
> + return -ENODEV;
> + }
> +
> + device_for_each_child_node(dev, fwnode) {
> + if (fwnode_property_read_u32(fwnode, "port-number",
> + &port_num)) {
> + dev_err(dev, "No port-number for port, skipping.\n");
> + ret = -EINVAL;
> + goto unregister_ports;
> + }
> +
> + if (port_num >= typec->num_ports) {
> + dev_err(dev, "Invalid port number.\n");
> + ret = -EINVAL;
> + goto unregister_ports;
> + }
> +
> + dev_dbg(dev, "Registering port %d\n", port_num);
> + ret = cros_typec_parse_port_props(&cap, fwnode, dev);
> + if (ret < 0)
> + goto unregister_ports;
> + typec->ports[port_num] = typec_register_port(dev, &cap);
> + if (IS_ERR(typec->ports[port_num])) {
> + dev_err(dev, "Failed to register port %d\n", port_num);
> + ret = PTR_ERR(typec->ports[port_num]);
> + goto unregister_ports;
> + }
> + }
> +
> + return 0;
> +
> +unregister_ports:
> + for (i = 0; i < typec->num_ports; i++)
> + typec_unregister_port(typec->ports[i]);
> + return ret;
> +}
> +
> +static int cros_typec_ec_command(struct cros_typec_data *typec,
> + unsigned int version,
> + unsigned int command,
> + void *outdata,
> + unsigned int outsize,
> + void *indata,
> + unsigned int insize)
> +{
> + struct cros_ec_command *msg;
> + int ret;
> +
> + msg = kzalloc(sizeof(*msg) + max(outsize, insize), GFP_KERNEL);
> + if (!msg)
> + return -ENOMEM;
> +
> + msg->version = version;
> + msg->command = command;
> + msg->outsize = outsize;
> + msg->insize = insize;
> +
> + if (outsize)
> + memcpy(msg->data, outdata, outsize);
> +
> + ret = cros_ec_cmd_xfer_status(typec->ec, msg);
> + if (ret >= 0 && insize)
> + memcpy(indata, msg->data, insize);
> +
> + kfree(msg);
> + return ret;
> +}
> +
> +
> +static int cros_typec_get_num_ports(struct cros_typec_data *typec)
> +{
This functions is only called once at probe, you can just put the code there. I
had some readibility problems trying to follow de code.
> + struct ec_response_usb_pd_ports resp;
> + int ret;
> +
> + ret = cros_typec_ec_command(typec, 0, EC_CMD_USB_PD_PORTS, NULL, 0,
> + &resp, sizeof(resp));
> + if (ret < 0)
> + return ret;
> +
> + typec->num_ports = resp.num_ports;
> + if (typec->num_ports > EC_USB_PD_MAX_PORTS) {
> + dev_warn(typec->dev,
> + "Too many ports reported: %d, limiting to max: %d\n",
> + typec->num_ports, EC_USB_PD_MAX_PORTS);
You say that you are limiting the number of ports to max but typec->num_ports is
still resp.num_ports, is that correct?
> + }
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id cros_typec_acpi_id[] = {
> + { "GOOG0014", 0 },
> + { /* sentinel */ }
No need to add /* sentinel */ here, is obvious.
> +};
> +MODULE_DEVICE_TABLE(acpi, cros_typec_acpi_id);
> +#endif
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id cros_typec_of_match[] = {
> + { .compatible = "google,cros-ec-typec", },
> + { /* sentinel */ },
No need to add /* sentinel */ here, is obvious. And no need for the colon at the
end.
> +};
> +MODULE_DEVICE_TABLE(of, cros_typec_of_match);
> +#endif
> +
> +static int cros_typec_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct cros_typec_data *typec;
> + int ret;
> +
> + typec = devm_kzalloc(dev, sizeof(*typec), GFP_KERNEL);
> + if (!typec)
> + return -ENOMEM;
> + typec->dev = dev;
> + typec->ec = dev_get_drvdata(pdev->dev.parent);
> + platform_set_drvdata(pdev, typec);
> +
> + ret = cros_typec_get_num_ports(typec);
> + if (ret < 0)
> + return ret;
> +
> + ret = cros_typec_init_ports(typec);
both, cros_typec_get_num_ports and cros_typec_init_ports are only called once,
as I said I had some problems of readibility because typec->num_ports is set in
one function and unregister in another function when fails.
I'd just remove those two functions and put the code directly here.
> + if (!ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static struct platform_driver cros_typec_driver = {
> + .driver = {
> + .name = DRV_NAME,
no tab, just space after .-name
> + .acpi_match_table = ACPI_PTR(cros_typec_acpi_id),
> + .of_match_table = of_match_ptr(cros_typec_of_match),
> + },
> + .probe = cros_typec_probe,
no tab, just space after .probe
> +};
> +
> +module_platform_driver(cros_typec_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Chrome OS EC Type C control");
>
You probably want to add the MODULE_AUTHOR here
Hi Enric,
Thanks for taking a look at the patch. Please see my responses inline
(I will defer sending the next version till the one pending question I
had is resolved):
On Thu, Feb 6, 2020 at 8:19 AM Enric Balletbo i Serra
<[email protected]> wrote:
>
> Hi Prashant,
>
> On 5/2/20 21:59, Prashant Malani wrote:
> > Add a driver to implement the Type C connector class for Chrome OS
> > devices with ECs (Embedded Controllers).
> >
> > The driver relies on firmware device specifications for various port
> > attributes. On ACPI platforms, this is specified using the logical
> > device with HID GOOG0014. On DT platforms, this is specified using the
> > DT node with compatible string "google,cros-ec-typec".
> >
>
> If that's the case you should document this in a binding file.
Done. I will add this in the next version of the series.
> driver a replacement of the cros-ec-extcon driver or is a different thing?
Currently it is distinct. We're hoping to plug in to type C connector
class work which Heikki is working on (relating to Type C Mux agent).
Hopefully, we will gradually transition the extcon functionality to
the Type C port driver.
>
> There is a device where I can test this?
I think you can try it on kevin if you add the DT node for it (haven't
tried it myself on kevin). An example will be present in the DT
Documentation I provide in the next version.
>
> > This patch reads the device FW node and uses the port attributes to
> > register the typec ports with the Type C connector class framework, but
> > doesn't do much else.
> >
> > Subsequent patches will add more functionality to the driver, including
> > obtaining current port information (polarity, vconn role, current power
> > role etc.) after querying the EC.
> >
> > Signed-off-by: Prashant Malani <[email protected]>
> > ---
> > drivers/platform/chrome/Kconfig | 11 ++
> > drivers/platform/chrome/Makefile | 1 +
> > drivers/platform/chrome/cros_ec_typec.c | 228 ++++++++++++++++++++++++
> > 3 files changed, 240 insertions(+)
> > create mode 100644 drivers/platform/chrome/cros_ec_typec.c
> >
> > diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
> > index 5f57282a28da00..1370dfd1ca1565 100644
> > --- a/drivers/platform/chrome/Kconfig
> > +++ b/drivers/platform/chrome/Kconfig
> > @@ -214,6 +214,17 @@ config CROS_EC_SYSFS
> > To compile this driver as a module, choose M here: the
> > module will be called cros_ec_sysfs.
> >
> > +config CROS_EC_TYPEC
> > + tristate "ChromeOS EC Type-C Connector Control"
> > + depends on MFD_CROS_EC_DEV && TYPEC
> > + default n
>
> Default value is already n, so you don't need to put it here. But I'd say that
> we might be interested on have default MFD_CROS_EC_DEV like the other drivers.
Done.
>
> > + help
> > + If you say Y here, you get support for accessing Type C connector
> > + information from the Chrome OS EC.
> > +
> > + To compile this driver as a module, choose M here: the module will be
> > + called cros_ec_typec.
> > +
> > config CROS_USBPD_LOGGER
> > tristate "Logging driver for USB PD charger"
> > depends on CHARGER_CROS_USBPD
> > diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
> > index aacd5920d8a180..caf2a9cdb5e6d1 100644
> > --- a/drivers/platform/chrome/Makefile
> > +++ b/drivers/platform/chrome/Makefile
> > @@ -12,6 +12,7 @@ obj-$(CONFIG_CROS_EC_ISHTP) += cros_ec_ishtp.o
> > obj-$(CONFIG_CROS_EC_RPMSG) += cros_ec_rpmsg.o
> > obj-$(CONFIG_CROS_EC_SPI) += cros_ec_spi.o
> > cros_ec_lpcs-objs := cros_ec_lpc.o cros_ec_lpc_mec.o
> > +obj-$(CONFIG_CROS_EC_TYPEC) += cros_ec_typec.o
> > obj-$(CONFIG_CROS_EC_LPC) += cros_ec_lpcs.o
> > obj-$(CONFIG_CROS_EC_PROTO) += cros_ec_proto.o cros_ec_trace.o
> > obj-$(CONFIG_CROS_KBD_LED_BACKLIGHT) += cros_kbd_led_backlight.o
> > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> > new file mode 100644
> > index 00000000000000..fe5659171c2a85
> > --- /dev/null
> > +++ b/drivers/platform/chrome/cros_ec_typec.c
> > @@ -0,0 +1,228 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright 2020 Google LLC
> > + *
> > + * This driver provides the ability to view and manage Type C ports through the
> > + * Chrome OS EC.
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_data/cros_ec_commands.h>
> > +#include <linux/platform_data/cros_ec_proto.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/usb/typec.h>
> > +
> > +#define DRV_NAME "cros-ec-typec"
> > +
> > +/* Platform-specific data for the Chrome OS EC Type C controller. */
> > +struct cros_typec_data {
> > + struct device *dev;
> > + struct cros_ec_device *ec;
> > + int num_ports;
> > + /* Array of ports, indexed by port number. */
> > + struct typec_port *ports[EC_USB_PD_MAX_PORTS];
> > +};
> > +
> > +int cros_typec_parse_port_props(struct typec_capability *cap,
>
> static int
Sorry, done.
>
> > + struct fwnode_handle *fwnode,
> > + struct device *dev)
> > +{
> > + const char *buf;
> > + int ret;
> > +
> > + memset(cap, 0, sizeof(*cap));
> > + ret = fwnode_property_read_string(fwnode, "power-role", &buf);
> > + if (ret) {
> > + dev_err(dev, "power-role not found: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + ret = typec_find_port_power_role(buf);
> > + if (ret < 0)
> > + return ret;
> > + cap->type = ret;
> > +
> > + ret = fwnode_property_read_string(fwnode, "data-role", &buf);
> > + if (ret) {
> > + dev_err(dev, "data-role not found: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + ret = typec_find_port_data_role(buf);
> > + if (ret < 0)
> > + return ret;
> > + cap->data = ret;
> > +
> > + ret = fwnode_property_read_string(fwnode, "try-power-role", &buf);
> > + if (ret) {
> > + dev_err(dev, "try-power-role not found: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + ret = typec_find_power_role(buf);
> > + if (ret < 0)
> > + return ret;
> > + cap->prefer_role = ret;
> > +
> > + cap->fwnode = fwnode;
> > +
> > + return 0;
> > +}
> > +
> > +static int cros_typec_init_ports(struct cros_typec_data *typec)
> > +{
> > + struct device *dev = typec->dev;
> > + struct typec_capability cap;
> > + struct fwnode_handle *fwnode;
> > + int ret;
> > + int i;
> > + int nports;
> > + u32 port_num;
> > +
> > + nports = device_get_child_node_count(dev);
> > + if (nports == 0) {
> > + dev_err(dev, "No port entries found.\n");
> > + return -ENODEV;
> > + }
> > +
> > + device_for_each_child_node(dev, fwnode) {
> > + if (fwnode_property_read_u32(fwnode, "port-number",
> > + &port_num)) {
> > + dev_err(dev, "No port-number for port, skipping.\n");
> > + ret = -EINVAL;
> > + goto unregister_ports;
> > + }
> > +
> > + if (port_num >= typec->num_ports) {
> > + dev_err(dev, "Invalid port number.\n");
> > + ret = -EINVAL;
> > + goto unregister_ports;
> > + }
> > +
> > + dev_dbg(dev, "Registering port %d\n", port_num);
> > + ret = cros_typec_parse_port_props(&cap, fwnode, dev);
> > + if (ret < 0)
> > + goto unregister_ports;
> > + typec->ports[port_num] = typec_register_port(dev, &cap);
> > + if (IS_ERR(typec->ports[port_num])) {
> > + dev_err(dev, "Failed to register port %d\n", port_num);
> > + ret = PTR_ERR(typec->ports[port_num]);
> > + goto unregister_ports;
> > + }
> > + }
> > +
> > + return 0;
> > +
> > +unregister_ports:
> > + for (i = 0; i < typec->num_ports; i++)
> > + typec_unregister_port(typec->ports[i]);
> > + return ret;
> > +}
> > +
> > +static int cros_typec_ec_command(struct cros_typec_data *typec,
> > + unsigned int version,
> > + unsigned int command,
> > + void *outdata,
> > + unsigned int outsize,
> > + void *indata,
> > + unsigned int insize)
> > +{
> > + struct cros_ec_command *msg;
> > + int ret;
> > +
> > + msg = kzalloc(sizeof(*msg) + max(outsize, insize), GFP_KERNEL);
> > + if (!msg)
> > + return -ENOMEM;
> > +
> > + msg->version = version;
> > + msg->command = command;
> > + msg->outsize = outsize;
> > + msg->insize = insize;
> > +
> > + if (outsize)
> > + memcpy(msg->data, outdata, outsize);
> > +
> > + ret = cros_ec_cmd_xfer_status(typec->ec, msg);
> > + if (ret >= 0 && insize)
> > + memcpy(indata, msg->data, insize);
> > +
> > + kfree(msg);
> > + return ret;
> > +}
> > +
> > +
> > +static int cros_typec_get_num_ports(struct cros_typec_data *typec)
> > +{
>
> This functions is only called once at probe, you can just put the code there. I
> had some readibility problems trying to follow de code.
Done.
>
> > + struct ec_response_usb_pd_ports resp;
> > + int ret;
> > +
> > + ret = cros_typec_ec_command(typec, 0, EC_CMD_USB_PD_PORTS, NULL, 0,
> > + &resp, sizeof(resp));
> > + if (ret < 0)
> > + return ret;
> > +
> > + typec->num_ports = resp.num_ports;
> > + if (typec->num_ports > EC_USB_PD_MAX_PORTS) {
> > + dev_warn(typec->dev,
> > + "Too many ports reported: %d, limiting to max: %d\n",
> > + typec->num_ports, EC_USB_PD_MAX_PORTS);
>
> You say that you are limiting the number of ports to max but typec->num_ports is
> still resp.num_ports, is that correct?
>
Sorry, thanks for catching this, it should be set to
EC_USB_PD_MAX_PORTS. I will fix this in the next version.
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +#ifdef CONFIG_ACPI
> > +static const struct acpi_device_id cros_typec_acpi_id[] = {
> > + { "GOOG0014", 0 },
> > + { /* sentinel */ }
>
> No need to add /* sentinel */ here, is obvious.
Done.
>
> > +};
> > +MODULE_DEVICE_TABLE(acpi, cros_typec_acpi_id);
> > +#endif
> > +
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id cros_typec_of_match[] = {
> > + { .compatible = "google,cros-ec-typec", },
> > + { /* sentinel */ },
>
> No need to add /* sentinel */ here, is obvious. And no need for the colon at the
> end.
Done.
>
> > +};
> > +MODULE_DEVICE_TABLE(of, cros_typec_of_match);
> > +#endif
> > +
> > +static int cros_typec_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct cros_typec_data *typec;
> > + int ret;
> > +
> > + typec = devm_kzalloc(dev, sizeof(*typec), GFP_KERNEL);
> > + if (!typec)
> > + return -ENOMEM;
> > + typec->dev = dev;
> > + typec->ec = dev_get_drvdata(pdev->dev.parent);
> > + platform_set_drvdata(pdev, typec);
> > +
> > + ret = cros_typec_get_num_ports(typec);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = cros_typec_init_ports(typec);
>
> both, cros_typec_get_num_ports and cros_typec_init_ports are only called once,
> as I said I had some problems of readibility because typec->num_ports is set in
> one function and unregister in another function when fails.
Can we keep cros_typec_init_ports() as a separate function? I was
hoping to avoid cluttering the _probe() with FW node parsing logic.
cros_typec_init_ports() registers the ports (and unregisters on
failure), so seems fairly self-contained.
Of course, happy to place everything in probe if you still prefer that.
>
> I'd just remove those two functions and put the code directly here.
>
> > + if (!ret)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > +static struct platform_driver cros_typec_driver = {
> > + .driver = {
> > + .name = DRV_NAME,
>
> no tab, just space after .-name
Done.
>
> > + .acpi_match_table = ACPI_PTR(cros_typec_acpi_id),
> > + .of_match_table = of_match_ptr(cros_typec_of_match),
> > + },
> > + .probe = cros_typec_probe,
>
> no tab, just space after .probe
Done.
>
> > +};
> > +
> > +module_platform_driver(cros_typec_driver);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_DESCRIPTION("Chrome OS EC Type C control");
> >
>
> You probably want to add the MODULE_AUTHOR here
Done.
Best regards,
Fixes: 2adbeaafa10e ("platform/chrome: Add Type C connector class driver")
Signed-off-by: kbuild test robot <[email protected]>
---
cros_ec_typec.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index fe5659171c2a85..9b83aa4d5456d4 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -25,9 +25,9 @@ struct cros_typec_data {
struct typec_port *ports[EC_USB_PD_MAX_PORTS];
};
-int cros_typec_parse_port_props(struct typec_capability *cap,
- struct fwnode_handle *fwnode,
- struct device *dev)
+static int cros_typec_parse_port_props(struct typec_capability *cap,
+ struct fwnode_handle *fwnode,
+ struct device *dev)
{
const char *buf;
int ret;
Hi Prashant,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on chrome-platform-linux/for-next]
[also build test WARNING on linux/master linus/master v5.5 next-20200207]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Prashant-Malani/platform-chrome-Add-Type-C-connector-class-driver/20200206-225838
base: https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git for-next
reproduce:
# apt-get install sparse
# sparse version: v0.6.1-159-g100509c0-dirty
make ARCH=x86_64 allmodconfig
make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>
sparse warnings: (new ones prefixed by >>)
>> drivers/platform/chrome/cros_ec_typec.c:28:5: sparse: sparse: symbol 'cros_typec_parse_port_props' was not declared. Should it be static?
Please review and possibly fold the followup patch.
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Prashant,
On 7/2/20 0:30, Prashant Malani wrote:
> Hi Enric,
>
> Thanks for taking a look at the patch. Please see my responses inline
> (I will defer sending the next version till the one pending question I
> had is resolved):
>
> On Thu, Feb 6, 2020 at 8:19 AM Enric Balletbo i Serra
> <[email protected]> wrote:
>>
>> Hi Prashant,
>>
>> On 5/2/20 21:59, Prashant Malani wrote:
>>> Add a driver to implement the Type C connector class for Chrome OS
>>> devices with ECs (Embedded Controllers).
>>>
>>> The driver relies on firmware device specifications for various port
>>> attributes. On ACPI platforms, this is specified using the logical
>>> device with HID GOOG0014. On DT platforms, this is specified using the
>>> DT node with compatible string "google,cros-ec-typec".
>>>
>>
>> If that's the case you should document this in a binding file.
>
> Done. I will add this in the next version of the series.
>
>> driver a replacement of the cros-ec-extcon driver or is a different thing?
>
> Currently it is distinct. We're hoping to plug in to type C connector
> class work which Heikki is working on (relating to Type C Mux agent).
> Hopefully, we will gradually transition the extcon functionality to
> the Type C port driver.
>
>>
>> There is a device where I can test this?
>
> I think you can try it on kevin if you add the DT node for it (haven't
> tried it myself on kevin). An example will be present in the DT
> Documentation I provide in the next version.
>
>>
>>> This patch reads the device FW node and uses the port attributes to
>>> register the typec ports with the Type C connector class framework, but
>>> doesn't do much else.
>>>
>>> Subsequent patches will add more functionality to the driver, including
>>> obtaining current port information (polarity, vconn role, current power
>>> role etc.) after querying the EC.
>>>
>>> Signed-off-by: Prashant Malani <[email protected]>
>>> ---
>>> drivers/platform/chrome/Kconfig | 11 ++
>>> drivers/platform/chrome/Makefile | 1 +
>>> drivers/platform/chrome/cros_ec_typec.c | 228 ++++++++++++++++++++++++
>>> 3 files changed, 240 insertions(+)
>>> create mode 100644 drivers/platform/chrome/cros_ec_typec.c
>>>
>>> diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
>>> index 5f57282a28da00..1370dfd1ca1565 100644
>>> --- a/drivers/platform/chrome/Kconfig
>>> +++ b/drivers/platform/chrome/Kconfig
>>> @@ -214,6 +214,17 @@ config CROS_EC_SYSFS
>>> To compile this driver as a module, choose M here: the
>>> module will be called cros_ec_sysfs.
>>>
>>> +config CROS_EC_TYPEC
>>> + tristate "ChromeOS EC Type-C Connector Control"
>>> + depends on MFD_CROS_EC_DEV && TYPEC
>>> + default n
>>
>> Default value is already n, so you don't need to put it here. But I'd say that
>> we might be interested on have default MFD_CROS_EC_DEV like the other drivers.
>
> Done.
>
>>
>>> + help
>>> + If you say Y here, you get support for accessing Type C connector
>>> + information from the Chrome OS EC.
>>> +
>>> + To compile this driver as a module, choose M here: the module will be
>>> + called cros_ec_typec.
>>> +
>>> config CROS_USBPD_LOGGER
>>> tristate "Logging driver for USB PD charger"
>>> depends on CHARGER_CROS_USBPD
>>> diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
>>> index aacd5920d8a180..caf2a9cdb5e6d1 100644
>>> --- a/drivers/platform/chrome/Makefile
>>> +++ b/drivers/platform/chrome/Makefile
>>> @@ -12,6 +12,7 @@ obj-$(CONFIG_CROS_EC_ISHTP) += cros_ec_ishtp.o
>>> obj-$(CONFIG_CROS_EC_RPMSG) += cros_ec_rpmsg.o
>>> obj-$(CONFIG_CROS_EC_SPI) += cros_ec_spi.o
>>> cros_ec_lpcs-objs := cros_ec_lpc.o cros_ec_lpc_mec.o
>>> +obj-$(CONFIG_CROS_EC_TYPEC) += cros_ec_typec.o
>>> obj-$(CONFIG_CROS_EC_LPC) += cros_ec_lpcs.o
>>> obj-$(CONFIG_CROS_EC_PROTO) += cros_ec_proto.o cros_ec_trace.o
>>> obj-$(CONFIG_CROS_KBD_LED_BACKLIGHT) += cros_kbd_led_backlight.o
>>> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
>>> new file mode 100644
>>> index 00000000000000..fe5659171c2a85
>>> --- /dev/null
>>> +++ b/drivers/platform/chrome/cros_ec_typec.c
>>> @@ -0,0 +1,228 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * Copyright 2020 Google LLC
>>> + *
>>> + * This driver provides the ability to view and manage Type C ports through the
>>> + * Chrome OS EC.
>>> + */
>>> +
>>> +#include <linux/acpi.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/platform_data/cros_ec_commands.h>
>>> +#include <linux/platform_data/cros_ec_proto.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/usb/typec.h>
>>> +
>>> +#define DRV_NAME "cros-ec-typec"
>>> +
>>> +/* Platform-specific data for the Chrome OS EC Type C controller. */
>>> +struct cros_typec_data {
>>> + struct device *dev;
>>> + struct cros_ec_device *ec;
>>> + int num_ports;
>>> + /* Array of ports, indexed by port number. */
>>> + struct typec_port *ports[EC_USB_PD_MAX_PORTS];
>>> +};
>>> +
>>> +int cros_typec_parse_port_props(struct typec_capability *cap,
>>
>> static int
>
> Sorry, done.
>
>>
>>> + struct fwnode_handle *fwnode,
>>> + struct device *dev)
>>> +{
>>> + const char *buf;
>>> + int ret;
>>> +
>>> + memset(cap, 0, sizeof(*cap));
>>> + ret = fwnode_property_read_string(fwnode, "power-role", &buf);
>>> + if (ret) {
>>> + dev_err(dev, "power-role not found: %d\n", ret);
>>> + return ret;
>>> + }
>>> +
>>> + ret = typec_find_port_power_role(buf);
>>> + if (ret < 0)
>>> + return ret;
>>> + cap->type = ret;
>>> +
>>> + ret = fwnode_property_read_string(fwnode, "data-role", &buf);
>>> + if (ret) {
>>> + dev_err(dev, "data-role not found: %d\n", ret);
>>> + return ret;
>>> + }
>>> +
>>> + ret = typec_find_port_data_role(buf);
>>> + if (ret < 0)
>>> + return ret;
>>> + cap->data = ret;
>>> +
>>> + ret = fwnode_property_read_string(fwnode, "try-power-role", &buf);
>>> + if (ret) {
>>> + dev_err(dev, "try-power-role not found: %d\n", ret);
>>> + return ret;
>>> + }
>>> +
>>> + ret = typec_find_power_role(buf);
>>> + if (ret < 0)
>>> + return ret;
>>> + cap->prefer_role = ret;
>>> +
>>> + cap->fwnode = fwnode;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int cros_typec_init_ports(struct cros_typec_data *typec)
>>> +{
>>> + struct device *dev = typec->dev;
>>> + struct typec_capability cap;
>>> + struct fwnode_handle *fwnode;
>>> + int ret;
>>> + int i;
>>> + int nports;
>>> + u32 port_num;
>>> +
>>> + nports = device_get_child_node_count(dev);
>>> + if (nports == 0) {
>>> + dev_err(dev, "No port entries found.\n");
>>> + return -ENODEV;
>>> + }
>>> +
>>> + device_for_each_child_node(dev, fwnode) {
>>> + if (fwnode_property_read_u32(fwnode, "port-number",
>>> + &port_num)) {
>>> + dev_err(dev, "No port-number for port, skipping.\n");
>>> + ret = -EINVAL;
>>> + goto unregister_ports;
>>> + }
>>> +
>>> + if (port_num >= typec->num_ports) {
>>> + dev_err(dev, "Invalid port number.\n");
>>> + ret = -EINVAL;
>>> + goto unregister_ports;
>>> + }
>>> +
>>> + dev_dbg(dev, "Registering port %d\n", port_num);
>>> + ret = cros_typec_parse_port_props(&cap, fwnode, dev);
>>> + if (ret < 0)
>>> + goto unregister_ports;
>>> + typec->ports[port_num] = typec_register_port(dev, &cap);
>>> + if (IS_ERR(typec->ports[port_num])) {
>>> + dev_err(dev, "Failed to register port %d\n", port_num);
>>> + ret = PTR_ERR(typec->ports[port_num]);
>>> + goto unregister_ports;
>>> + }
>>> + }
>>> +
>>> + return 0;
>>> +
>>> +unregister_ports:
>>> + for (i = 0; i < typec->num_ports; i++)
>>> + typec_unregister_port(typec->ports[i]);
>>> + return ret;
>>> +}
>>> +
>>> +static int cros_typec_ec_command(struct cros_typec_data *typec,
>>> + unsigned int version,
>>> + unsigned int command,
>>> + void *outdata,
>>> + unsigned int outsize,
>>> + void *indata,
>>> + unsigned int insize)
>>> +{
>>> + struct cros_ec_command *msg;
>>> + int ret;
>>> +
>>> + msg = kzalloc(sizeof(*msg) + max(outsize, insize), GFP_KERNEL);
>>> + if (!msg)
>>> + return -ENOMEM;
>>> +
>>> + msg->version = version;
>>> + msg->command = command;
>>> + msg->outsize = outsize;
>>> + msg->insize = insize;
>>> +
>>> + if (outsize)
>>> + memcpy(msg->data, outdata, outsize);
>>> +
>>> + ret = cros_ec_cmd_xfer_status(typec->ec, msg);
>>> + if (ret >= 0 && insize)
>>> + memcpy(indata, msg->data, insize);
>>> +
>>> + kfree(msg);
>>> + return ret;
>>> +}
>>> +
>>> +
>>> +static int cros_typec_get_num_ports(struct cros_typec_data *typec)
>>> +{
>>
>> This functions is only called once at probe, you can just put the code there. I
>> had some readibility problems trying to follow de code.
>
> Done.
>
>>
>>> + struct ec_response_usb_pd_ports resp;
>>> + int ret;
>>> +
>>> + ret = cros_typec_ec_command(typec, 0, EC_CMD_USB_PD_PORTS, NULL, 0,
>>> + &resp, sizeof(resp));
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + typec->num_ports = resp.num_ports;
>>> + if (typec->num_ports > EC_USB_PD_MAX_PORTS) {
>>> + dev_warn(typec->dev,
>>> + "Too many ports reported: %d, limiting to max: %d\n",
>>> + typec->num_ports, EC_USB_PD_MAX_PORTS);
>>
>> You say that you are limiting the number of ports to max but typec->num_ports is
>> still resp.num_ports, is that correct?
>>
>
> Sorry, thanks for catching this, it should be set to
> EC_USB_PD_MAX_PORTS. I will fix this in the next version.
>
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +#ifdef CONFIG_ACPI
>>> +static const struct acpi_device_id cros_typec_acpi_id[] = {
>>> + { "GOOG0014", 0 },
>>> + { /* sentinel */ }
>>
>> No need to add /* sentinel */ here, is obvious.
>
> Done.
>
>>
>>> +};
>>> +MODULE_DEVICE_TABLE(acpi, cros_typec_acpi_id);
>>> +#endif
>>> +
>>> +#ifdef CONFIG_OF
>>> +static const struct of_device_id cros_typec_of_match[] = {
>>> + { .compatible = "google,cros-ec-typec", },
>>> + { /* sentinel */ },
>>
>> No need to add /* sentinel */ here, is obvious. And no need for the colon at the
>> end.
>
> Done.
>>
>>> +};
>>> +MODULE_DEVICE_TABLE(of, cros_typec_of_match);
>>> +#endif
>>> +
>>> +static int cros_typec_probe(struct platform_device *pdev)
>>> +{
>>> + struct device *dev = &pdev->dev;
>>> + struct cros_typec_data *typec;
>>> + int ret;
>>> +
>>> + typec = devm_kzalloc(dev, sizeof(*typec), GFP_KERNEL);
>>> + if (!typec)
>>> + return -ENOMEM;
>>> + typec->dev = dev;
>>> + typec->ec = dev_get_drvdata(pdev->dev.parent);
>>> + platform_set_drvdata(pdev, typec);
>>> +
>>> + ret = cros_typec_get_num_ports(typec);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + ret = cros_typec_init_ports(typec);
>>
>> both, cros_typec_get_num_ports and cros_typec_init_ports are only called once,
>> as I said I had some problems of readibility because typec->num_ports is set in
>> one function and unregister in another function when fails.
>
> Can we keep cros_typec_init_ports() as a separate function? I was
> hoping to avoid cluttering the _probe() with FW node parsing logic.
> cros_typec_init_ports() registers the ports (and unregisters on
> failure), so seems fairly self-contained.
> Of course, happy to place everything in probe if you still prefer that.
>
I don't mind clutter probe if is more readable but try it, and lets see how
looks like. You are also registering and unregistering the typec ports there,
not only parsing. Usually what we have is parsing the firmware or dt and then
register and unregister the typec ports, all mixed. I'm more in favour of have
it separated, IMHO.
>>
>> I'd just remove those two functions and put the code directly here.
>>
>>> + if (!ret)
>>> + return ret;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static struct platform_driver cros_typec_driver = {
>>> + .driver = {
>>> + .name = DRV_NAME,
>>
>> no tab, just space after .-name
>
> Done.
>>
>>> + .acpi_match_table = ACPI_PTR(cros_typec_acpi_id),
>>> + .of_match_table = of_match_ptr(cros_typec_of_match),
>>> + },
>>> + .probe = cros_typec_probe,
>>
>> no tab, just space after .probe
>
> Done.
>
>>
>>> +};
>>> +
>>> +module_platform_driver(cros_typec_driver);
>>> +
>>> +MODULE_LICENSE("GPL");
>>> +MODULE_DESCRIPTION("Chrome OS EC Type C control");
>>>
>>
>> You probably want to add the MODULE_AUTHOR here
>
> Done.
>
> Best regards,
>
On Fri, Feb 7, 2020 at 9:00 AM Enric Balletbo i Serra
<[email protected]> wrote:
>
> Hi Prashant,
>
> On 7/2/20 0:30, Prashant Malani wrote:
> > Hi Enric,
> >
> > Thanks for taking a look at the patch. Please see my responses inline
> > (I will defer sending the next version till the one pending question I
> > had is resolved):
> >
> > On Thu, Feb 6, 2020 at 8:19 AM Enric Balletbo i Serra
> > <[email protected]> wrote:
> >>
> >> Hi Prashant,
> >>
> >> On 5/2/20 21:59, Prashant Malani wrote:
> >>> Add a driver to implement the Type C connector class for Chrome OS
> >>> devices with ECs (Embedded Controllers).
> >>>
> >>> The driver relies on firmware device specifications for various port
> >>> attributes. On ACPI platforms, this is specified using the logical
> >>> device with HID GOOG0014. On DT platforms, this is specified using the
> >>> DT node with compatible string "google,cros-ec-typec".
> >>>
> >>
> >> If that's the case you should document this in a binding file.
> >
> > Done. I will add this in the next version of the series.
> >
> >> driver a replacement of the cros-ec-extcon driver or is a different thing?
> >
> > Currently it is distinct. We're hoping to plug in to type C connector
> > class work which Heikki is working on (relating to Type C Mux agent).
> > Hopefully, we will gradually transition the extcon functionality to
> > the Type C port driver.
> >
> >>
> >> There is a device where I can test this?
> >
> > I think you can try it on kevin if you add the DT node for it (haven't
> > tried it myself on kevin). An example will be present in the DT
> > Documentation I provide in the next version.
> >
> >>
> >>> This patch reads the device FW node and uses the port attributes to
> >>> register the typec ports with the Type C connector class framework, but
> >>> doesn't do much else.
> >>>
> >>> Subsequent patches will add more functionality to the driver, including
> >>> obtaining current port information (polarity, vconn role, current power
> >>> role etc.) after querying the EC.
> >>>
> >>> Signed-off-by: Prashant Malani <[email protected]>
> >>> ---
> >>> drivers/platform/chrome/Kconfig | 11 ++
> >>> drivers/platform/chrome/Makefile | 1 +
> >>> drivers/platform/chrome/cros_ec_typec.c | 228 ++++++++++++++++++++++++
> >>> 3 files changed, 240 insertions(+)
> >>> create mode 100644 drivers/platform/chrome/cros_ec_typec.c
> >>>
> >>> diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
> >>> index 5f57282a28da00..1370dfd1ca1565 100644
> >>> --- a/drivers/platform/chrome/Kconfig
> >>> +++ b/drivers/platform/chrome/Kconfig
> >>> @@ -214,6 +214,17 @@ config CROS_EC_SYSFS
> >>> To compile this driver as a module, choose M here: the
> >>> module will be called cros_ec_sysfs.
> >>>
> >>> +config CROS_EC_TYPEC
> >>> + tristate "ChromeOS EC Type-C Connector Control"
> >>> + depends on MFD_CROS_EC_DEV && TYPEC
> >>> + default n
> >>
> >> Default value is already n, so you don't need to put it here. But I'd say that
> >> we might be interested on have default MFD_CROS_EC_DEV like the other drivers.
> >
> > Done.
> >
> >>
> >>> + help
> >>> + If you say Y here, you get support for accessing Type C connector
> >>> + information from the Chrome OS EC.
> >>> +
> >>> + To compile this driver as a module, choose M here: the module will be
> >>> + called cros_ec_typec.
> >>> +
> >>> config CROS_USBPD_LOGGER
> >>> tristate "Logging driver for USB PD charger"
> >>> depends on CHARGER_CROS_USBPD
> >>> diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
> >>> index aacd5920d8a180..caf2a9cdb5e6d1 100644
> >>> --- a/drivers/platform/chrome/Makefile
> >>> +++ b/drivers/platform/chrome/Makefile
> >>> @@ -12,6 +12,7 @@ obj-$(CONFIG_CROS_EC_ISHTP) += cros_ec_ishtp.o
> >>> obj-$(CONFIG_CROS_EC_RPMSG) += cros_ec_rpmsg.o
> >>> obj-$(CONFIG_CROS_EC_SPI) += cros_ec_spi.o
> >>> cros_ec_lpcs-objs := cros_ec_lpc.o cros_ec_lpc_mec.o
> >>> +obj-$(CONFIG_CROS_EC_TYPEC) += cros_ec_typec.o
> >>> obj-$(CONFIG_CROS_EC_LPC) += cros_ec_lpcs.o
> >>> obj-$(CONFIG_CROS_EC_PROTO) += cros_ec_proto.o cros_ec_trace.o
> >>> obj-$(CONFIG_CROS_KBD_LED_BACKLIGHT) += cros_kbd_led_backlight.o
> >>> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> >>> new file mode 100644
> >>> index 00000000000000..fe5659171c2a85
> >>> --- /dev/null
> >>> +++ b/drivers/platform/chrome/cros_ec_typec.c
> >>> @@ -0,0 +1,228 @@
> >>> +// SPDX-License-Identifier: GPL-2.0-only
> >>> +/*
> >>> + * Copyright 2020 Google LLC
> >>> + *
> >>> + * This driver provides the ability to view and manage Type C ports through the
> >>> + * Chrome OS EC.
> >>> + */
> >>> +
> >>> +#include <linux/acpi.h>
> >>> +#include <linux/module.h>
> >>> +#include <linux/of.h>
> >>> +#include <linux/platform_data/cros_ec_commands.h>
> >>> +#include <linux/platform_data/cros_ec_proto.h>
> >>> +#include <linux/platform_device.h>
> >>> +#include <linux/usb/typec.h>
> >>> +
> >>> +#define DRV_NAME "cros-ec-typec"
> >>> +
> >>> +/* Platform-specific data for the Chrome OS EC Type C controller. */
> >>> +struct cros_typec_data {
> >>> + struct device *dev;
> >>> + struct cros_ec_device *ec;
> >>> + int num_ports;
> >>> + /* Array of ports, indexed by port number. */
> >>> + struct typec_port *ports[EC_USB_PD_MAX_PORTS];
> >>> +};
> >>> +
> >>> +int cros_typec_parse_port_props(struct typec_capability *cap,
> >>
> >> static int
> >
> > Sorry, done.
> >
> >>
> >>> + struct fwnode_handle *fwnode,
> >>> + struct device *dev)
> >>> +{
> >>> + const char *buf;
> >>> + int ret;
> >>> +
> >>> + memset(cap, 0, sizeof(*cap));
> >>> + ret = fwnode_property_read_string(fwnode, "power-role", &buf);
> >>> + if (ret) {
> >>> + dev_err(dev, "power-role not found: %d\n", ret);
> >>> + return ret;
> >>> + }
> >>> +
> >>> + ret = typec_find_port_power_role(buf);
> >>> + if (ret < 0)
> >>> + return ret;
> >>> + cap->type = ret;
> >>> +
> >>> + ret = fwnode_property_read_string(fwnode, "data-role", &buf);
> >>> + if (ret) {
> >>> + dev_err(dev, "data-role not found: %d\n", ret);
> >>> + return ret;
> >>> + }
> >>> +
> >>> + ret = typec_find_port_data_role(buf);
> >>> + if (ret < 0)
> >>> + return ret;
> >>> + cap->data = ret;
> >>> +
> >>> + ret = fwnode_property_read_string(fwnode, "try-power-role", &buf);
> >>> + if (ret) {
> >>> + dev_err(dev, "try-power-role not found: %d\n", ret);
> >>> + return ret;
> >>> + }
> >>> +
> >>> + ret = typec_find_power_role(buf);
> >>> + if (ret < 0)
> >>> + return ret;
> >>> + cap->prefer_role = ret;
> >>> +
> >>> + cap->fwnode = fwnode;
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int cros_typec_init_ports(struct cros_typec_data *typec)
> >>> +{
> >>> + struct device *dev = typec->dev;
> >>> + struct typec_capability cap;
> >>> + struct fwnode_handle *fwnode;
> >>> + int ret;
> >>> + int i;
> >>> + int nports;
> >>> + u32 port_num;
> >>> +
> >>> + nports = device_get_child_node_count(dev);
> >>> + if (nports == 0) {
> >>> + dev_err(dev, "No port entries found.\n");
> >>> + return -ENODEV;
> >>> + }
> >>> +
> >>> + device_for_each_child_node(dev, fwnode) {
> >>> + if (fwnode_property_read_u32(fwnode, "port-number",
> >>> + &port_num)) {
> >>> + dev_err(dev, "No port-number for port, skipping.\n");
> >>> + ret = -EINVAL;
> >>> + goto unregister_ports;
> >>> + }
> >>> +
> >>> + if (port_num >= typec->num_ports) {
> >>> + dev_err(dev, "Invalid port number.\n");
> >>> + ret = -EINVAL;
> >>> + goto unregister_ports;
> >>> + }
> >>> +
> >>> + dev_dbg(dev, "Registering port %d\n", port_num);
> >>> + ret = cros_typec_parse_port_props(&cap, fwnode, dev);
> >>> + if (ret < 0)
> >>> + goto unregister_ports;
> >>> + typec->ports[port_num] = typec_register_port(dev, &cap);
> >>> + if (IS_ERR(typec->ports[port_num])) {
> >>> + dev_err(dev, "Failed to register port %d\n", port_num);
> >>> + ret = PTR_ERR(typec->ports[port_num]);
> >>> + goto unregister_ports;
> >>> + }
> >>> + }
> >>> +
> >>> + return 0;
> >>> +
> >>> +unregister_ports:
> >>> + for (i = 0; i < typec->num_ports; i++)
> >>> + typec_unregister_port(typec->ports[i]);
> >>> + return ret;
> >>> +}
> >>> +
> >>> +static int cros_typec_ec_command(struct cros_typec_data *typec,
> >>> + unsigned int version,
> >>> + unsigned int command,
> >>> + void *outdata,
> >>> + unsigned int outsize,
> >>> + void *indata,
> >>> + unsigned int insize)
> >>> +{
> >>> + struct cros_ec_command *msg;
> >>> + int ret;
> >>> +
> >>> + msg = kzalloc(sizeof(*msg) + max(outsize, insize), GFP_KERNEL);
> >>> + if (!msg)
> >>> + return -ENOMEM;
> >>> +
> >>> + msg->version = version;
> >>> + msg->command = command;
> >>> + msg->outsize = outsize;
> >>> + msg->insize = insize;
> >>> +
> >>> + if (outsize)
> >>> + memcpy(msg->data, outdata, outsize);
> >>> +
> >>> + ret = cros_ec_cmd_xfer_status(typec->ec, msg);
> >>> + if (ret >= 0 && insize)
> >>> + memcpy(indata, msg->data, insize);
> >>> +
> >>> + kfree(msg);
> >>> + return ret;
> >>> +}
> >>> +
> >>> +
> >>> +static int cros_typec_get_num_ports(struct cros_typec_data *typec)
> >>> +{
> >>
> >> This functions is only called once at probe, you can just put the code there. I
> >> had some readibility problems trying to follow de code.
> >
> > Done.
> >
> >>
> >>> + struct ec_response_usb_pd_ports resp;
> >>> + int ret;
> >>> +
> >>> + ret = cros_typec_ec_command(typec, 0, EC_CMD_USB_PD_PORTS, NULL, 0,
> >>> + &resp, sizeof(resp));
> >>> + if (ret < 0)
> >>> + return ret;
> >>> +
> >>> + typec->num_ports = resp.num_ports;
> >>> + if (typec->num_ports > EC_USB_PD_MAX_PORTS) {
> >>> + dev_warn(typec->dev,
> >>> + "Too many ports reported: %d, limiting to max: %d\n",
> >>> + typec->num_ports, EC_USB_PD_MAX_PORTS);
> >>
> >> You say that you are limiting the number of ports to max but typec->num_ports is
> >> still resp.num_ports, is that correct?
> >>
> >
> > Sorry, thanks for catching this, it should be set to
> > EC_USB_PD_MAX_PORTS. I will fix this in the next version.
> >
> >>> + }
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +#ifdef CONFIG_ACPI
> >>> +static const struct acpi_device_id cros_typec_acpi_id[] = {
> >>> + { "GOOG0014", 0 },
> >>> + { /* sentinel */ }
> >>
> >> No need to add /* sentinel */ here, is obvious.
> >
> > Done.
> >
> >>
> >>> +};
> >>> +MODULE_DEVICE_TABLE(acpi, cros_typec_acpi_id);
> >>> +#endif
> >>> +
> >>> +#ifdef CONFIG_OF
> >>> +static const struct of_device_id cros_typec_of_match[] = {
> >>> + { .compatible = "google,cros-ec-typec", },
> >>> + { /* sentinel */ },
> >>
> >> No need to add /* sentinel */ here, is obvious. And no need for the colon at the
> >> end.
> >
> > Done.
> >>
> >>> +};
> >>> +MODULE_DEVICE_TABLE(of, cros_typec_of_match);
> >>> +#endif
> >>> +
> >>> +static int cros_typec_probe(struct platform_device *pdev)
> >>> +{
> >>> + struct device *dev = &pdev->dev;
> >>> + struct cros_typec_data *typec;
> >>> + int ret;
> >>> +
> >>> + typec = devm_kzalloc(dev, sizeof(*typec), GFP_KERNEL);
> >>> + if (!typec)
> >>> + return -ENOMEM;
> >>> + typec->dev = dev;
> >>> + typec->ec = dev_get_drvdata(pdev->dev.parent);
> >>> + platform_set_drvdata(pdev, typec);
> >>> +
> >>> + ret = cros_typec_get_num_ports(typec);
> >>> + if (ret < 0)
> >>> + return ret;
> >>> +
> >>> + ret = cros_typec_init_ports(typec);
> >>
> >> both, cros_typec_get_num_ports and cros_typec_init_ports are only called once,
> >> as I said I had some problems of readibility because typec->num_ports is set in
> >> one function and unregister in another function when fails.
> >
> > Can we keep cros_typec_init_ports() as a separate function? I was
> > hoping to avoid cluttering the _probe() with FW node parsing logic.
> > cros_typec_init_ports() registers the ports (and unregisters on
> > failure), so seems fairly self-contained.
> > Of course, happy to place everything in probe if you still prefer that.
> >
>
> I don't mind clutter probe if is more readable but try it, and lets see how
> looks like. You are also registering and unregistering the typec ports there,
> not only parsing. Usually what we have is parsing the firmware or dt and then
> register and unregister the typec ports, all mixed. I'm more in favour of have
> it separated, IMHO.
Sounds good, I'll push another version with cros_typec_init_ports()
distinct and we can check how it looks.
If it's still not readable, I'll push version with the code directly in probe()
>
> >>
> >> I'd just remove those two functions and put the code directly here.
> >>
> >>> + if (!ret)
> >>> + return ret;
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static struct platform_driver cros_typec_driver = {
> >>> + .driver = {
> >>> + .name = DRV_NAME,
> >>
> >> no tab, just space after .-name
> >
> > Done.
> >>
> >>> + .acpi_match_table = ACPI_PTR(cros_typec_acpi_id),
> >>> + .of_match_table = of_match_ptr(cros_typec_of_match),
> >>> + },
> >>> + .probe = cros_typec_probe,
> >>
> >> no tab, just space after .probe
> >
> > Done.
> >
> >>
> >>> +};
> >>> +
> >>> +module_platform_driver(cros_typec_driver);
> >>> +
> >>> +MODULE_LICENSE("GPL");
> >>> +MODULE_DESCRIPTION("Chrome OS EC Type C control");
> >>>
> >>
> >> You probably want to add the MODULE_AUTHOR here
> >
> > Done.
> >
> > Best regards,
> >