2024-03-25 23:37:50

by Pavan Holla

[permalink] [raw]
Subject: [PATCH 0/3] usb: typec: Implement UCSI driver for ChromeOS

We are developing a UCSI ChromeOS EC transport driver. The ChromeOS EC
implements a UCSI PPM. This driver is being developed in
drivers/platform/chrome since

1) Most other drivers which depend on ChromeOS EC reside there.

2) Our architecture might undergo a few revisions rapidly, so
platform/chrome seems like a good place while we finalize our
design.

This patch series creates a public include/usb/ucsi.h that can be used
by transport drivers outside drivers/usb/typec/ucsi. Then, we use this
interface and ChromeOS EC host commands to send UCSI commands in
drivers/platform/chrome/cros_ec_ucsi.c.

Signed-off-by: Pavan Holla <[email protected]>
---
Pavan Holla (3):
usb: typec: ucsi: Provide interface for UCSI transport
usb: typec: ucsi: Import interface for UCSI transport
platform/chrome: cros_ec_ucsi: Implement UCSI PDC driver

MAINTAINERS | 1 +
drivers/platform/chrome/Kconfig | 14 ++
drivers/platform/chrome/Makefile | 1 +
drivers/platform/chrome/cros_ec_ucsi.c | 247 +++++++++++++++++++++++++
drivers/usb/typec/ucsi/ucsi.h | 54 +-----
include/linux/platform_data/cros_ec_commands.h | 19 ++
include/linux/usb/ucsi.h | 66 +++++++
7 files changed, 349 insertions(+), 53 deletions(-)
---
base-commit: 4cece764965020c22cff7665b18a012006359095
change-id: 20240325-public-ucsi-h-3ecee4106a58

Best regards,
--
Pavan Holla <[email protected]>



2024-03-25 23:38:01

by Pavan Holla

[permalink] [raw]
Subject: [PATCH 2/3] usb: typec: ucsi: Import interface for UCSI transport

Import include/linux/usb/ucsi.h and remove any duplicate declarations.

Signed-off-by: Pavan Holla <[email protected]>
---
drivers/usb/typec/ucsi/ucsi.h | 54 +------------------------------------------
1 file changed, 1 insertion(+), 53 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index 32daf5f58650..167951538030 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -10,22 +10,13 @@
#include <linux/usb/typec.h>
#include <linux/usb/pd.h>
#include <linux/usb/role.h>
+#include <linux/usb/ucsi.h>
#include <asm/unaligned.h>

/* -------------------------------------------------------------------------- */

-struct ucsi;
-struct ucsi_altmode;
struct dentry;

-/* UCSI offsets (Bytes) */
-#define UCSI_VERSION 0
-#define UCSI_CCI 4
-#define UCSI_CONTROL 8
-#define UCSI_MESSAGE_IN 16
-#define UCSI_MESSAGE_OUT 32
-#define UCSIv2_MESSAGE_OUT 272
-
/* UCSI versions */
#define UCSI_VERSION_1_2 0x0120
#define UCSI_VERSION_2_0 0x0200
@@ -42,48 +33,6 @@ struct dentry;
*/
#define UCSI_SPEC_REVISION_TO_BCD(_v_) (((_v_) + 1) << 8)

-/* Command Status and Connector Change Indication (CCI) bits */
-#define UCSI_CCI_CONNECTOR(_c_) (((_c_) & GENMASK(7, 1)) >> 1)
-#define UCSI_CCI_LENGTH(_c_) (((_c_) & GENMASK(15, 8)) >> 8)
-#define UCSI_CCI_NOT_SUPPORTED BIT(25)
-#define UCSI_CCI_CANCEL_COMPLETE BIT(26)
-#define UCSI_CCI_RESET_COMPLETE BIT(27)
-#define UCSI_CCI_BUSY BIT(28)
-#define UCSI_CCI_ACK_COMPLETE BIT(29)
-#define UCSI_CCI_ERROR BIT(30)
-#define UCSI_CCI_COMMAND_COMPLETE BIT(31)
-
-/**
- * struct ucsi_operations - UCSI I/O operations
- * @read: Read operation
- * @sync_write: Blocking write operation
- * @async_write: Non-blocking write operation
- * @update_altmodes: Squashes duplicate DP altmodes
- *
- * Read and write routines for UCSI interface. @sync_write must wait for the
- * Command Completion Event from the PPM before returning, and @async_write must
- * return immediately after sending the data to the PPM.
- */
-struct ucsi_operations {
- int (*read)(struct ucsi *ucsi, unsigned int offset,
- void *val, size_t val_len);
- int (*sync_write)(struct ucsi *ucsi, unsigned int offset,
- const void *val, size_t val_len);
- int (*async_write)(struct ucsi *ucsi, unsigned int offset,
- const void *val, size_t val_len);
- bool (*update_altmodes)(struct ucsi *ucsi, struct ucsi_altmode *orig,
- struct ucsi_altmode *updated);
-};
-
-struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops);
-void ucsi_destroy(struct ucsi *ucsi);
-int ucsi_register(struct ucsi *ucsi);
-void ucsi_unregister(struct ucsi *ucsi);
-void *ucsi_get_drvdata(struct ucsi *ucsi);
-void ucsi_set_drvdata(struct ucsi *ucsi, void *data);
-
-void ucsi_connector_change(struct ucsi *ucsi, u8 num);
-
/* -------------------------------------------------------------------------- */

/* Commands */
@@ -465,7 +414,6 @@ int ucsi_send_command(struct ucsi *ucsi, u64 command,
void *retval, size_t size);

void ucsi_altmode_update_active(struct ucsi_connector *con);
-int ucsi_resume(struct ucsi *ucsi);

#if IS_ENABLED(CONFIG_POWER_SUPPLY)
int ucsi_register_port_psy(struct ucsi_connector *con);

--
2.44.0.396.g6e790dbe36-goog


2024-03-25 23:38:23

by Pavan Holla

[permalink] [raw]
Subject: [PATCH 3/3] platform/chrome: cros_ec_ucsi: Implement UCSI PDC driver

Implementation of transport driver for UCSI. This driver will be used
if the ChromeOS EC implements a PPM.

Signed-off-by: Pavan Holla <[email protected]>
---
drivers/platform/chrome/Kconfig | 14 ++
drivers/platform/chrome/Makefile | 1 +
drivers/platform/chrome/cros_ec_ucsi.c | 247 +++++++++++++++++++++++++
include/linux/platform_data/cros_ec_commands.h | 19 ++
4 files changed, 281 insertions(+)

diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
index 7a83346bfa53..67ab79a6815b 100644
--- a/drivers/platform/chrome/Kconfig
+++ b/drivers/platform/chrome/Kconfig
@@ -238,6 +238,20 @@ config CROS_EC_TYPEC
To compile this driver as a module, choose M here: the module will be
called cros-ec-typec.

+config CROS_EC_UCSI
+ tristate "UCSI Driver for ChromeOS EC"
+ depends on MFD_CROS_EC_DEV
+ depends on CROS_USBPD_NOTIFY
+ depends on TYPEC_UCSI
+ depends on !EXTCON_TCSS_CROS_EC
+ default MFD_CROS_EC_DEV
+ help
+ This driver enables UCSI support for a ChromeOS EC. The EC is
+ expected to implement a PPM.
+
+ To compile the driver as a module, choose M here: the module
+ will be called cros_ec_ucsi.
+
config CROS_HPS_I2C
tristate "ChromeOS HPS device"
depends on HID && I2C && PM
diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
index 2dcc6ccc2302..85964e165a70 100644
--- a/drivers/platform/chrome/Makefile
+++ b/drivers/platform/chrome/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_CROS_EC_UART) += cros_ec_uart.o
cros_ec_lpcs-objs := cros_ec_lpc.o cros_ec_lpc_mec.o
cros-ec-typec-objs := cros_ec_typec.o cros_typec_vdm.o
obj-$(CONFIG_CROS_EC_TYPEC) += cros-ec-typec.o
+obj-$(CONFIG_CROS_EC_UCSI) += cros_ec_ucsi.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_ucsi.c b/drivers/platform/chrome/cros_ec_ucsi.c
new file mode 100644
index 000000000000..9fee300a3d93
--- /dev/null
+++ b/drivers/platform/chrome/cros_ec_ucsi.c
@@ -0,0 +1,247 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * UCSI driver for ChromeOS EC
+ *
+ * Copyright 2024 Google LLC.
+ */
+
+#include <linux/container_of.h>
+#include <linux/dev_printk.h>
+#include <linux/module.h>
+#include <linux/platform_data/cros_ec_commands.h>
+#include <linux/platform_data/cros_usbpd_notify.h>
+#include <linux/platform_data/cros_ec_proto.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/usb/ucsi.h>
+#include <linux/wait.h>
+
+#define DRV_NAME "cros-ec-ucsi"
+
+#define MAX_EC_DATA_SIZE 256
+#define WRITE_TMO_MS 500
+
+#define COMMAND_PENDING 1
+#define ACK_PENDING 2
+
+#define UCSI_COMMAND(_cmd_) ((_cmd_) & 0xff)
+#define UCSI_ACK_CC_CI 0x04
+
+struct cros_ucsi_data {
+ struct device *dev;
+ struct ucsi *ucsi;
+
+ struct cros_ec_device *ec;
+ struct notifier_block nb;
+ struct work_struct work;
+
+ struct completion complete;
+ unsigned long flags;
+};
+
+static int cros_ucsi_read(struct ucsi *ucsi, unsigned int offset, void *val,
+ size_t val_len)
+{
+ struct cros_ucsi_data *udata = ucsi_get_drvdata(ucsi);
+ struct ec_params_ucsi_ppm_get req = {
+ .offset = offset,
+ .size = val_len,
+ };
+ int ret;
+
+ if (val_len > MAX_EC_DATA_SIZE) {
+ dev_err(udata->dev, "Can't read %zu bytes. Too big.", val_len);
+ return -EINVAL;
+ }
+
+ ret = cros_ec_cmd(udata->ec, 0, EC_CMD_UCSI_PPM_GET,
+ &req, sizeof(req), val, val_len);
+ if (ret < 0) {
+ dev_warn(udata->dev, "Failed to send EC message UCSI_PPM_GET: error=%d", ret);
+ return ret;
+ }
+ return 0;
+}
+
+static int cros_ucsi_async_write(struct ucsi *ucsi, unsigned int offset,
+ const void *val, size_t val_len)
+{
+ struct cros_ucsi_data *udata = ucsi_get_drvdata(ucsi);
+ uint8_t ec_buffer[MAX_EC_DATA_SIZE + sizeof(struct ec_params_ucsi_ppm_set)];
+ struct ec_params_ucsi_ppm_set *req = (struct ec_params_ucsi_ppm_set *)ec_buffer;
+ int ret = 0;
+
+ if (val_len > MAX_EC_DATA_SIZE) {
+ dev_err(udata->dev, "Can't write %zu bytes. Too big.", val_len);
+ return -EINVAL;
+ }
+
+ memset(req, 0, sizeof(ec_buffer));
+ req->offset = offset;
+ memcpy(req->data, val, val_len);
+ ret = cros_ec_cmd(udata->ec, 0, EC_CMD_UCSI_PPM_SET,
+ req, sizeof(struct ec_params_ucsi_ppm_set) + val_len, NULL, 0);
+
+ if (ret < 0) {
+ dev_warn(udata->dev, "Failed to send EC message UCSI_PPM_SET: error=%d", ret);
+ return ret;
+ }
+ return 0;
+}
+
+static int cros_ucsi_sync_write(struct ucsi *ucsi, unsigned int offset,
+ const void *val, size_t val_len)
+{
+ struct cros_ucsi_data *udata = ucsi_get_drvdata(ucsi);
+ bool ack = UCSI_COMMAND(*(u64 *)val) == UCSI_ACK_CC_CI;
+ int ret;
+
+ if (ack)
+ set_bit(ACK_PENDING, &udata->flags);
+ else
+ set_bit(COMMAND_PENDING, &udata->flags);
+
+ ret = cros_ucsi_async_write(ucsi, offset, val, val_len);
+ if (ret)
+ goto out;
+
+ if (!wait_for_completion_timeout(&udata->complete, WRITE_TMO_MS))
+ ret = -ETIMEDOUT;
+
+out:
+ if (ack)
+ clear_bit(ACK_PENDING, &udata->flags);
+ else
+ clear_bit(COMMAND_PENDING, &udata->flags);
+ return ret;
+}
+
+struct ucsi_operations cros_ucsi_ops = {
+ .read = cros_ucsi_read,
+ .async_write = cros_ucsi_async_write,
+ .sync_write = cros_ucsi_sync_write,
+};
+
+static void cros_ucsi_work(struct work_struct *work)
+{
+ struct cros_ucsi_data *udata = container_of(work, struct cros_ucsi_data, work);
+ u32 cci;
+ int ret;
+
+ ret = cros_ucsi_read(udata->ucsi, UCSI_CCI, &cci, sizeof(cci));
+ if (ret)
+ return;
+
+ if (UCSI_CCI_CONNECTOR(cci))
+ ucsi_connector_change(udata->ucsi, UCSI_CCI_CONNECTOR(cci));
+
+ if (cci & UCSI_CCI_ACK_COMPLETE && test_bit(ACK_PENDING, &udata->flags))
+ complete(&udata->complete);
+ if (cci & UCSI_CCI_COMMAND_COMPLETE &&
+ test_bit(COMMAND_PENDING, &udata->flags))
+ complete(&udata->complete);
+}
+
+static int cros_ucsi_event(struct notifier_block *nb,
+ unsigned long host_event, void *_notify)
+{
+ struct cros_ucsi_data *udata = container_of(nb, struct cros_ucsi_data, nb);
+
+ if (!(host_event & PD_EVENT_PPM))
+ return NOTIFY_OK;
+
+ dev_dbg(udata->dev, "UCSI notification received");
+ flush_work(&udata->work);
+ schedule_work(&udata->work);
+
+ return NOTIFY_OK;
+}
+
+static int cros_ucsi_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct cros_ucsi_data *udata;
+ int ret;
+
+ udata = devm_kzalloc(dev, sizeof(*udata), GFP_KERNEL);
+ if (!udata)
+ return -ENOMEM;
+
+ udata->dev = dev;
+
+ udata->ec = ((struct cros_ec_dev *) dev_get_drvdata(pdev->dev.parent))->ec_dev;
+ if (!udata->ec) {
+ dev_err(dev, "couldn't find parent EC device\n");
+ return -ENODEV;
+ }
+
+ platform_set_drvdata(pdev, udata);
+
+ INIT_WORK(&udata->work, cros_ucsi_work);
+ init_completion(&udata->complete);
+
+ udata->ucsi = ucsi_create(udata->dev, &cros_ucsi_ops);
+ if (IS_ERR(udata->ucsi)) {
+ dev_err(dev, "failed to allocate UCSI instance\n");
+ return PTR_ERR(udata->ucsi);
+ }
+
+ ucsi_set_drvdata(udata->ucsi, udata);
+
+ ret = ucsi_register(udata->ucsi);
+ if (ret) {
+ ucsi_destroy(udata->ucsi);
+ udata->ucsi = NULL;
+ return ret;
+ }
+
+ udata->nb.notifier_call = cros_ucsi_event;
+ ret = cros_usbpd_register_notify(&udata->nb);
+ return ret;
+}
+
+static int cros_ucsi_remove(struct platform_device *dev)
+{
+ struct cros_ucsi_data *udata = platform_get_drvdata(dev);
+
+ if (udata->ucsi) {
+ ucsi_unregister(udata->ucsi);
+ ucsi_destroy(udata->ucsi);
+ udata->ucsi = NULL;
+ }
+ return 0;
+}
+
+static int cros_ucsi_suspend(struct device *dev)
+{
+ struct cros_ucsi_data *udata = dev_get_drvdata(dev);
+
+ cancel_work_sync(&udata->work);
+
+ return 0;
+}
+
+static int cros_ucsi_resume(struct device *dev)
+{
+ struct cros_ucsi_data *udata = dev_get_drvdata(dev);
+
+ return ucsi_resume(udata->ucsi);
+}
+
+static SIMPLE_DEV_PM_OPS(cros_ucsi_pm_ops, cros_ucsi_suspend,
+ cros_ucsi_resume);
+
+static struct platform_driver cros_ucsi_driver = {
+ .driver = {
+ .name = DRV_NAME,
+ .pm = &cros_ucsi_pm_ops,
+ },
+ .probe = cros_ucsi_probe,
+ .remove = cros_ucsi_remove,
+};
+
+module_platform_driver(cros_ucsi_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("UCSI driver for ChromeOS EC.");
+MODULE_ALIAS("platform:" DRV_NAME);
diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
index ecc47d5fe239..39f6f54bbe4c 100644
--- a/include/linux/platform_data/cros_ec_commands.h
+++ b/include/linux/platform_data/cros_ec_commands.h
@@ -4933,6 +4933,7 @@ struct ec_response_pd_status {
#define PD_EVENT_POWER_CHANGE BIT(1)
#define PD_EVENT_IDENTITY_RECEIVED BIT(2)
#define PD_EVENT_DATA_SWAP BIT(3)
+#define PD_EVENT_PPM BIT(5)
struct ec_response_host_event_status {
uint32_t status; /* PD MCU host event status */
} __ec_align4;
@@ -5994,6 +5995,24 @@ struct ec_response_typec_vdm_response {

#undef VDO_MAX_SIZE

+/*
+ * Read/write interface for UCSI OPM <-> PPM communication.
+ */
+#define EC_CMD_UCSI_PPM_SET 0x0140
+
+/* The data size is stored in the host command protocol header. */
+struct ec_params_ucsi_ppm_set {
+ uint16_t offset;
+ uint8_t data[];
+} __ec_align2;
+
+#define EC_CMD_UCSI_PPM_GET 0x0141
+
+struct ec_params_ucsi_ppm_get {
+ uint16_t offset;
+ uint8_t size;
+} __ec_align2;
+
/*****************************************************************************/
/* The command range 0x200-0x2FF is reserved for Rotor. */


--
2.44.0.396.g6e790dbe36-goog


2024-03-26 01:30:04

by Pavan Holla

[permalink] [raw]
Subject: [PATCH 1/3] usb: typec: ucsi: Provide interface for UCSI transport

The ucsi.h include can be used by driver implementations that
provide transport for UCSI commands.

Signed-off-by: Pavan Holla <[email protected]>
---
MAINTAINERS | 1 +
include/linux/usb/ucsi.h | 66 ++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 67 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index aa3b947fb080..e799d67a8fa5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22987,6 +22987,7 @@ F: Documentation/ABI/testing/sysfs-class-typec
F: Documentation/driver-api/usb/typec.rst
F: drivers/usb/typec/
F: include/linux/usb/typec.h
+F: include/linux/usb/ucsi.h

USB TYPEC INTEL PMC MUX DRIVER
M: Heikki Krogerus <[email protected]>
diff --git a/include/linux/usb/ucsi.h b/include/linux/usb/ucsi.h
new file mode 100644
index 000000000000..3ec1db968070
--- /dev/null
+++ b/include/linux/usb/ucsi.h
@@ -0,0 +1,66 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __LINUX_USB_UCSI_H
+#define __LINUX_USB_UCSI_H
+
+#include <linux/bitops.h>
+#include <linux/device.h>
+#include <linux/types.h>
+
+/* -------------------------------------------------------------------------- */
+
+struct ucsi;
+struct ucsi_altmode;
+
+/* UCSI offsets (Bytes) */
+#define UCSI_VERSION 0
+#define UCSI_CCI 4
+#define UCSI_CONTROL 8
+#define UCSI_MESSAGE_IN 16
+#define UCSI_MESSAGE_OUT 32
+#define UCSIv2_MESSAGE_OUT 272
+
+/* Command Status and Connector Change Indication (CCI) bits */
+#define UCSI_CCI_CONNECTOR(_c_) (((_c_) & GENMASK(7, 1)) >> 1)
+#define UCSI_CCI_LENGTH(_c_) (((_c_) & GENMASK(15, 8)) >> 8)
+#define UCSI_CCI_NOT_SUPPORTED BIT(25)
+#define UCSI_CCI_CANCEL_COMPLETE BIT(26)
+#define UCSI_CCI_RESET_COMPLETE BIT(27)
+#define UCSI_CCI_BUSY BIT(28)
+#define UCSI_CCI_ACK_COMPLETE BIT(29)
+#define UCSI_CCI_ERROR BIT(30)
+#define UCSI_CCI_COMMAND_COMPLETE BIT(31)
+
+/**
+ * struct ucsi_operations - UCSI I/O operations
+ * @read: Read operation
+ * @sync_write: Blocking write operation
+ * @async_write: Non-blocking write operation
+ * @update_altmodes: Squashes duplicate DP altmodes
+ *
+ * Read and write routines for UCSI interface. @sync_write must wait for the
+ * Command Completion Event from the PPM before returning, and @async_write must
+ * return immediately after sending the data to the PPM.
+ */
+struct ucsi_operations {
+ int (*read)(struct ucsi *ucsi, unsigned int offset,
+ void *val, size_t val_len);
+ int (*sync_write)(struct ucsi *ucsi, unsigned int offset,
+ const void *val, size_t val_len);
+ int (*async_write)(struct ucsi *ucsi, unsigned int offset,
+ const void *val, size_t val_len);
+ bool (*update_altmodes)(struct ucsi *ucsi, struct ucsi_altmode *orig,
+ struct ucsi_altmode *updated);
+};
+
+struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops);
+void ucsi_destroy(struct ucsi *ucsi);
+int ucsi_register(struct ucsi *ucsi);
+void ucsi_unregister(struct ucsi *ucsi);
+int ucsi_resume(struct ucsi *ucsi);
+void *ucsi_get_drvdata(struct ucsi *ucsi);
+void ucsi_set_drvdata(struct ucsi *ucsi, void *data);
+
+void ucsi_connector_change(struct ucsi *ucsi, u8 num);
+
+#endif /* __LINUX_USB_UCSI_H */

--
2.44.0.396.g6e790dbe36-goog


2024-03-26 02:11:41

by Prashant Malani

[permalink] [raw]
Subject: Re: [PATCH 2/3] usb: typec: ucsi: Import interface for UCSI transport

Hi Pavan,

On Mon, Mar 25, 2024 at 4:37 PM Pavan Holla <[email protected]> wrote:
>
> Import include/linux/usb/ucsi.h and remove any duplicate declarations.
>
> Signed-off-by: Pavan Holla <[email protected]>

Can you squash Patch 1/3 and this Patch (2/3)? It sounds like an
atomic change that doesn't need two patches.

BR,

-Prashant

2024-03-26 08:46:45

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 3/3] platform/chrome: cros_ec_ucsi: Implement UCSI PDC driver

On 26/03/2024 00:37, Pavan Holla wrote:
> Implementation of transport driver for UCSI. This driver will be used
> if the ChromeOS EC implements a PPM.
>


> +static struct platform_driver cros_ucsi_driver = {
> + .driver = {
> + .name = DRV_NAME,
> + .pm = &cros_ucsi_pm_ops,
> + },
> + .probe = cros_ucsi_probe,
> + .remove = cros_ucsi_remove,
> +};
> +
> +module_platform_driver(cros_ucsi_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("UCSI driver for ChromeOS EC.");
> +MODULE_ALIAS("platform:" DRV_NAME);

You should not need MODULE_ALIAS() in normal cases. If you need it,
usually it means your device ID table is wrong (e.g. misses either
entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
for incomplete ID table.

Using DRV_NAME here brings even more confusion...

Best regards,
Krzysztof


2024-03-26 09:13:19

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH 1/3] usb: typec: ucsi: Provide interface for UCSI transport

On Mon, Mar 25, 2024 at 11:37:28PM +0000, Pavan Holla wrote:
> The ucsi.h include can be used by driver implementations that
> provide transport for UCSI commands.
>
> Signed-off-by: Pavan Holla <[email protected]>
> ---
> MAINTAINERS | 1 +
> include/linux/usb/ucsi.h | 66 ++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 67 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index aa3b947fb080..e799d67a8fa5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -22987,6 +22987,7 @@ F: Documentation/ABI/testing/sysfs-class-typec
> F: Documentation/driver-api/usb/typec.rst
> F: drivers/usb/typec/
> F: include/linux/usb/typec.h
> +F: include/linux/usb/ucsi.h
>
> USB TYPEC INTEL PMC MUX DRIVER
> M: Heikki Krogerus <[email protected]>
> diff --git a/include/linux/usb/ucsi.h b/include/linux/usb/ucsi.h
> new file mode 100644
> index 000000000000..3ec1db968070
> --- /dev/null
> +++ b/include/linux/usb/ucsi.h
> @@ -0,0 +1,66 @@
> +/* SPDX-License-Identifier: GPL-2.0 */

No copyright line? I think your employer would not like it if they saw
that :(

Please get review from some internal-to-google developers before
submitting your next version of this series.

thanks,

greg k-h

2024-03-26 23:48:20

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH 3/3] platform/chrome: cros_ec_ucsi: Implement UCSI PDC driver

Hi Pavan,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 4cece764965020c22cff7665b18a012006359095]

url: https://github.com/intel-lab-lkp/linux/commits/Pavan-Holla/usb-typec-ucsi-Provide-interface-for-UCSI-transport/20240326-074003
base: 4cece764965020c22cff7665b18a012006359095
patch link: https://lore.kernel.org/r/20240325-public-ucsi-h-v1-3-7c7e888edc0a%40chromium.org
patch subject: [PATCH 3/3] platform/chrome: cros_ec_ucsi: Implement UCSI PDC driver
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20240327/[email protected]/config)
compiler: alpha-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240327/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All warnings (new ones prefixed by >>):

>> drivers/platform/chrome/cros_ec_ucsi.c:224:12: warning: 'cros_ucsi_resume' defined but not used [-Wunused-function]
224 | static int cros_ucsi_resume(struct device *dev)
| ^~~~~~~~~~~~~~~~
>> drivers/platform/chrome/cros_ec_ucsi.c:215:12: warning: 'cros_ucsi_suspend' defined but not used [-Wunused-function]
215 | static int cros_ucsi_suspend(struct device *dev)
| ^~~~~~~~~~~~~~~~~


vim +/cros_ucsi_resume +224 drivers/platform/chrome/cros_ec_ucsi.c

214
> 215 static int cros_ucsi_suspend(struct device *dev)
216 {
217 struct cros_ucsi_data *udata = dev_get_drvdata(dev);
218
219 cancel_work_sync(&udata->work);
220
221 return 0;
222 }
223
> 224 static int cros_ucsi_resume(struct device *dev)
225 {
226 struct cros_ucsi_data *udata = dev_get_drvdata(dev);
227
228 return ucsi_resume(udata->ucsi);
229 }
230

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

2024-03-26 23:49:29

by Pavan Holla

[permalink] [raw]
Subject: Re: [PATCH 1/3] usb: typec: ucsi: Provide interface for UCSI transport

Hi Greg,

Thanks for the review.

On Tue, Mar 26, 2024 at 2:12 AM Greg Kroah-Hartman
<[email protected]> wrote:
> No copyright line? I think your employer would not like it if they saw
> that :(

My employer ( Google ) does not want me to insert a copyright notice
if third party code
was copied verbatim. I plan to squash commit 1 and commit 2 in my next
version so that
the move becomes clearer. I will keep your comment in mind for future CL's.

> Please get review from some internal-to-google developers before
> submitting your next version of this series.

Acknowledged.

Best Regards,
Pavan

2024-03-27 03:47:24

by Pavan Holla

[permalink] [raw]
Subject: Re: [PATCH 2/3] usb: typec: ucsi: Import interface for UCSI transport

On Mon, Mar 25, 2024 at 7:11 PM Prashant Malani <[email protected]> wrote:
> Can you squash Patch 1/3 and this Patch (2/3)? It sounds like an
> atomic change that doesn't need two patches.

Acknowledged. Will do it in the next version.

Thanks,
Pavan

2024-03-28 09:06:12

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH 3/3] platform/chrome: cros_ec_ucsi: Implement UCSI PDC driver

Hi Pavan,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 4cece764965020c22cff7665b18a012006359095]

url: https://github.com/intel-lab-lkp/linux/commits/Pavan-Holla/usb-typec-ucsi-Provide-interface-for-UCSI-transport/20240326-074003
base: 4cece764965020c22cff7665b18a012006359095
patch link: https://lore.kernel.org/r/20240325-public-ucsi-h-v1-3-7c7e888edc0a%40chromium.org
patch subject: [PATCH 3/3] platform/chrome: cros_ec_ucsi: Implement UCSI PDC driver
config: x86_64-randconfig-123-20240328 (https://download.01.org/0day-ci/archive/20240328/[email protected]/config)
compiler: gcc-12 (Ubuntu 12.3.0-9ubuntu2) 12.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240328/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

sparse warnings: (new ones prefixed by >>)
>> drivers/platform/chrome/cros_ec_ucsi.c:119:24: sparse: sparse: symbol 'cros_ucsi_ops' was not declared. Should it be static?

vim +/cros_ucsi_ops +119 drivers/platform/chrome/cros_ec_ucsi.c

118
> 119 struct ucsi_operations cros_ucsi_ops = {
120 .read = cros_ucsi_read,
121 .async_write = cros_ucsi_async_write,
122 .sync_write = cros_ucsi_sync_write,
123 };
124

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki