Mailing List Archive

[PATCH v2 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 <pholla@chromium.org>
---
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
Re: [PATCH v2 3/3] platform/chrome: cros_ec_ucsi: Implement UCSI PDC driver [ In reply to ]
On 26/03/2024 00:42, 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);

Nothing improved.

One patchset per 24h. Allow people to actually review your code.

Best regards,
Krzysztof
Re: [PATCH v2 3/3] platform/chrome: cros_ec_ucsi: Implement UCSI PDC driver [ In reply to ]
Hi Krzysztof,

Thanks for the review.

On Tue, Mar 26, 2024 at 1:47?AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> Nothing improved.

Yes. I only added maintainers of drivers/platform/chrome in v2. I am
still investigating why MODULE_ALIAS() is required.

> One patchset per 24h. Allow people to actually review your code.

Thanks for letting me know. I will throttle future patch uploads.

Best regards,
Pavan
Re: [PATCH v2 3/3] platform/chrome: cros_ec_ucsi: Implement UCSI PDC driver [ In reply to ]
On 27/03/2024 04:39, Pavan Holla wrote:
> Hi Krzysztof,
>
> Thanks for the review.
>
> On Tue, Mar 26, 2024 at 1:47?AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> Nothing improved.
>
> Yes. I only added maintainers of drivers/platform/chrome in v2. I am
> still investigating why MODULE_ALIAS() is required.

Heh, I wrote why. You miss ID table.

Best regards,
Krzysztof
Re: [PATCH v2 3/3] platform/chrome: cros_ec_ucsi: Implement UCSI PDC driver [ In reply to ]
On Tue, Mar 26, 2024 at 9:59?PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 27/03/2024 04:39, Pavan Holla wrote:
> > Hi Krzysztof,
> >
> > Thanks for the review.
> >
> > On Tue, Mar 26, 2024 at 1:47?AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >> Nothing improved.
> >
> > Yes. I only added maintainers of drivers/platform/chrome in v2. I am
> > still investigating why MODULE_ALIAS() is required.
>
> Heh, I wrote why. You miss ID table.

This driver is going to be used by the cros_ec_dev.c MFD. The UCSI device doesn’t
have an ACPI or OF entry, so I am not sure how I can use MODULE_DEVICE_TABLE
here. If I don’t use MODULE_ALIAS(“platform:” DRV_NAME),
https://elixir.bootlin.com/linux/latest/source/drivers/mfd/cros_ec_dev.c#L206
isn’t able to automatically associate the driver with the device at boot.
I haven’t upstreamed the change in cros_ec_dev.c yet, but the code is similar to
existing code for drivers/platform/chrome/cros_usbpd_logger.c. There are many
other occurrences of the same MODULE_ALIAS pattern:

~/v6.6$ grep -r 'MODULE_ALIAS("platform:" DRV_NAME)' *  | wc -l
93

I am still trying to figure out why the “platform:” string allows cros_ec_dev.c to
load the driver. I suspect it might be due to
https://elixir.bootlin.com/linux/latest/source/drivers/base/platform.c#L1257.

Thanks,
Pavan
Re: [PATCH v2 3/3] platform/chrome: cros_ec_ucsi: Implement UCSI PDC driver [ In reply to ]
On 28/03/2024 03:32, Pavan Holla wrote:
> On Tue, Mar 26, 2024 at 9:59?PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 27/03/2024 04:39, Pavan Holla wrote:
>>> Hi Krzysztof,
>>>
>>> Thanks for the review.
>>>
>>> On Tue, Mar 26, 2024 at 1:47?AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>> Nothing improved.
>>>
>>> Yes. I only added maintainers of drivers/platform/chrome in v2. I am
>>> still investigating why MODULE_ALIAS() is required.
>>
>> Heh, I wrote why. You miss ID table.
>
> This driver is going to be used by the cros_ec_dev.c MFD. The UCSI device doesn’t
> have an ACPI or OF entry, so I am not sure how I can use MODULE_DEVICE_TABLE
> here. If I don’t use MODULE_ALIAS(“platform:” DRV_NAME),
> https://elixir.bootlin.com/linux/latest/source/drivers/mfd/cros_ec_dev.c#L206
> isn’t able to automatically associate the driver with the device at boot.
> I haven’t upstreamed the change in cros_ec_dev.c yet, but the code is similar to
> existing code for drivers/platform/chrome/cros_usbpd_logger.c. There are many
> other occurrences of the same MODULE_ALIAS pattern:

Just open other platform drivers and look how it is done there. Or ask
colleagues. There is absolutely no one in entire Chromium/google who
ever wrote platform_driver? platform_driver has ID table for matching.

Otherwise how do you expect this to be matched? How your driver is being
matched and device bound? By fallback, right? So what is the primary method?

Best regards,
Krzysztof
Re: [PATCH v2 3/3] platform/chrome: cros_ec_ucsi: Implement UCSI PDC driver [ In reply to ]
On Thu, Mar 28, 2024 at 09:36:54AM +0100, Krzysztof Kozlowski wrote:
> On 28/03/2024 03:32, Pavan Holla wrote:
> > On Tue, Mar 26, 2024 at 9:59?PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>
> >> On 27/03/2024 04:39, Pavan Holla wrote:
> >>> Hi Krzysztof,
> >>>
> >>> Thanks for the review.
> >>>
> >>> On Tue, Mar 26, 2024 at 1:47?AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>>> Nothing improved.
> >>>
> >>> Yes. I only added maintainers of drivers/platform/chrome in v2. I am
> >>> still investigating why MODULE_ALIAS() is required.
> >>
> >> Heh, I wrote why. You miss ID table.
> >
> > This driver is going to be used by the cros_ec_dev.c MFD. The UCSI device doesn’t
> > have an ACPI or OF entry, so I am not sure how I can use MODULE_DEVICE_TABLE
> > here. If I don’t use MODULE_ALIAS(“platform:” DRV_NAME),
> > https://elixir.bootlin.com/linux/latest/source/drivers/mfd/cros_ec_dev.c#L206
> > isn’t able to automatically associate the driver with the device at boot.
> > I haven’t upstreamed the change in cros_ec_dev.c yet, but the code is similar to
> > existing code for drivers/platform/chrome/cros_usbpd_logger.c. There are many
> > other occurrences of the same MODULE_ALIAS pattern:
>
> Just open other platform drivers and look how it is done there. Or ask
> colleagues. There is absolutely no one in entire Chromium/google who
> ever wrote platform_driver? platform_driver has ID table for matching.
>
> Otherwise how do you expect this to be matched? How your driver is being
> matched and device bound? By fallback, right? So what is the primary method?

Those platform devices are adding in drivers/mfd/cros_ec_dev.c via
mfd_add_hotplug_devices().

By looking other use cases of mfd_add_hotplug_devices():
$ grep -R --files-with-matches mfd_add_hotplug_devices drivers/mfd/
drivers/mfd/dln2.c
drivers/mfd/cros_ec_dev.c
drivers/mfd/viperboard.c

They also have no ID tables and need MODULE_ALIAS().
- drivers/gpio/gpio-dln2.c
- drivers/i2c/busses/i2c-dln2.c
- drivers/spi/spi-dln2.c
- drivers/iio/adc/dln2-adc.c
- drivers/gpio/gpio-viperboard.c
- drivers/i2c/busses/i2c-viperboard.c
- drivers/iio/adc/viperboard_adc.c

I'm not sure whether using the path results in:
- Lack of device ID table.
- Need MODULE_ALIAS().
in the platform device drivers. And perhaps it relies on the fallback match?
Re: [PATCH v2 3/3] platform/chrome: cros_ec_ucsi: Implement UCSI PDC driver [ In reply to ]
On 28/03/2024 10:57, Tzung-Bi Shih wrote:
> On Thu, Mar 28, 2024 at 09:36:54AM +0100, Krzysztof Kozlowski wrote:
>> On 28/03/2024 03:32, Pavan Holla wrote:
>>> On Tue, Mar 26, 2024 at 9:59?PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>>
>>>> On 27/03/2024 04:39, Pavan Holla wrote:
>>>>> Hi Krzysztof,
>>>>>
>>>>> Thanks for the review.
>>>>>
>>>>> On Tue, Mar 26, 2024 at 1:47?AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>>>> Nothing improved.
>>>>>
>>>>> Yes. I only added maintainers of drivers/platform/chrome in v2. I am
>>>>> still investigating why MODULE_ALIAS() is required.
>>>>
>>>> Heh, I wrote why. You miss ID table.
>>>
>>> This driver is going to be used by the cros_ec_dev.c MFD. The UCSI device doesn’t
>>> have an ACPI or OF entry, so I am not sure how I can use MODULE_DEVICE_TABLE
>>> here. If I don’t use MODULE_ALIAS(“platform:” DRV_NAME),
>>> https://elixir.bootlin.com/linux/latest/source/drivers/mfd/cros_ec_dev.c#L206
>>> isn’t able to automatically associate the driver with the device at boot.
>>> I haven’t upstreamed the change in cros_ec_dev.c yet, but the code is similar to
>>> existing code for drivers/platform/chrome/cros_usbpd_logger.c. There are many
>>> other occurrences of the same MODULE_ALIAS pattern:
>>
>> Just open other platform drivers and look how it is done there. Or ask
>> colleagues. There is absolutely no one in entire Chromium/google who
>> ever wrote platform_driver? platform_driver has ID table for matching.
>>
>> Otherwise how do you expect this to be matched? How your driver is being
>> matched and device bound? By fallback, right? So what is the primary method?
>
> Those platform devices are adding in drivers/mfd/cros_ec_dev.c via
> mfd_add_hotplug_devices().

This is not matching.

>
> By looking other use cases of mfd_add_hotplug_devices():
> $ grep -R --files-with-matches mfd_add_hotplug_devices drivers/mfd/
> drivers/mfd/dln2.c
> drivers/mfd/cros_ec_dev.c
> drivers/mfd/viperboard.c
>
> They also have no ID tables and need MODULE_ALIAS().
> - drivers/gpio/gpio-dln2.c
> - drivers/i2c/busses/i2c-dln2.c
> - drivers/spi/spi-dln2.c
> - drivers/iio/adc/dln2-adc.c
> - drivers/gpio/gpio-viperboard.c
> - drivers/i2c/busses/i2c-viperboard.c
> - drivers/iio/adc/viperboard_adc.c

So if there is a bug in some driver, you are allowed to add it? :) There
is plenty of poor examples, so what I was suggesting to look for good
examples. I agree that itself might be a tricky task.

> I'm not sure whether using the path results in:
> - Lack of device ID table.
> - Need MODULE_ALIAS().
> in the platform device drivers. And perhaps it relies on the fallback match?

Guys, think for a sec. If you are adding module alias being equivalent
to platform ID table entry, then why you are not using the platform ID
table entry in the first? That's the entire point.

So to repeat myself:
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.

Best regards,
Krzysztof
Re: [PATCH v2 3/3] platform/chrome: cros_ec_ucsi: Implement UCSI PDC driver [ In reply to ]
On 26/03/2024 01:42, Pavan Holla wrote:
> Implementation of transport driver for UCSI. This driver will be used
> if the ChromeOS EC implements a PPM.
>
> Signed-off-by: Pavan Holla <pholla@chromium.org>
> ---
> 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 ++

While it's fine to use platform/chrome for platform drivers, please
place drivers which have a subsystem into the subsystem dir. I think we
don't want to hunt UCSI implementations all over the codebase. Please
use drivers/usb/typec/ucsi/ location for your driver. This also removes
a need for a global header.

--
With best wishes
Dmitry
Re: [PATCH v2 3/3] platform/chrome: cros_ec_ucsi: Implement UCSI PDC driver [ In reply to ]
On Thu, Mar 28, 2024 at 01:16:36PM +0100, Krzysztof Kozlowski wrote:
> On 28/03/2024 10:57, Tzung-Bi Shih wrote:
> > By looking other use cases of mfd_add_hotplug_devices():
> > $ grep -R --files-with-matches mfd_add_hotplug_devices drivers/mfd/
> > drivers/mfd/dln2.c
> > drivers/mfd/cros_ec_dev.c
> > drivers/mfd/viperboard.c
> >
> > They also have no ID tables and need MODULE_ALIAS().
> > - drivers/gpio/gpio-dln2.c
> > - drivers/i2c/busses/i2c-dln2.c
> > - drivers/spi/spi-dln2.c
> > - drivers/iio/adc/dln2-adc.c
> > - drivers/gpio/gpio-viperboard.c
> > - drivers/i2c/busses/i2c-viperboard.c
> > - drivers/iio/adc/viperboard_adc.c
>
> So if there is a bug in some driver, you are allowed to add it? :) There
> is plenty of poor examples, so what I was suggesting to look for good
> examples. I agree that itself might be a tricky task.
>
> > I'm not sure whether using the path results in:
> > - Lack of device ID table.
> > - Need MODULE_ALIAS().
> > in the platform device drivers. And perhaps it relies on the fallback match?
>
> Guys, think for a sec. If you are adding module alias being equivalent
> to platform ID table entry, then why you are not using the platform ID
> table entry in the first? That's the entire point.
>
> So to repeat myself:
> 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.

Thanks. I see.

Yet another example in: drivers/mfd/max8997.c and drivers/rtc/rtc-max8997.c.

We should be able to use a platform_device_id, MODULE_DEVICE_TABLE(), and
`.id_table` for matching.

If that works, I think we need to turn most MODULE_ALIAS() use cases in
drivers/platform/chrome/ to this way afterward.
Re: [PATCH v2 3/3] platform/chrome: cros_ec_ucsi: Implement UCSI PDC driver [ In reply to ]
Hi Dmitry,

Thanks for the review.

On Thu, Mar 28, 2024 at 8:32?AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
> While it's fine to use platform/chrome for platform drivers, please
> place drivers which have a subsystem into the subsystem dir. I think we
> don't want to hunt UCSI implementations all over the codebase. Please
> use drivers/usb/typec/ucsi/ location for your driver. This also removes
> a need for a global header.

I agree with your assessment that drivers/usb/typec/ucsi/ is a good
location for the driver currently. However, the driver is still in
early stages of development. We may have to account for PDC/ EC quirks
(we have multiple vendors), add FW update functionality outside the
UCSI spec, or add PDC logging functionality. While I'd like to write
separate drivers for those, some of this functionality will need to
acquire a lock over communication to the PDC. Even if I were to write
separate drivers for new functionality related to the PDC, maybe it's
better for all ChromeOS PDC related drivers to reside in the same
location. I am not sure what form this driver will take in the near
future, thus I would prefer to keep it in platform/chrome. Maybe
cros_ec_ucsi isn't the best name here, cros_ec_pdc probably conveys
the intention better.

Thanks,
Pavan
Re: [PATCH v2 3/3] platform/chrome: cros_ec_ucsi: Implement UCSI PDC driver [ In reply to ]
On Fri, 29 Mar 2024 at 17:09, Pavan Holla <pholla@chromium.org> wrote:
>
> Hi Dmitry,
>
> Thanks for the review.
>
> On Thu, Mar 28, 2024 at 8:32?AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> > While it's fine to use platform/chrome for platform drivers, please
> > place drivers which have a subsystem into the subsystem dir. I think we
> > don't want to hunt UCSI implementations all over the codebase. Please
> > use drivers/usb/typec/ucsi/ location for your driver. This also removes
> > a need for a global header.
>
> I agree with your assessment that drivers/usb/typec/ucsi/ is a good
> location for the driver currently. However, the driver is still in
> early stages of development. We may have to account for PDC/ EC quirks
> (we have multiple vendors), add FW update functionality outside the
> UCSI spec, or add PDC logging functionality. While I'd like to write
> separate drivers for those, some of this functionality will need to
> acquire a lock over communication to the PDC. Even if I were to write
> separate drivers for new functionality related to the PDC, maybe it's
> better for all ChromeOS PDC related drivers to reside in the same
> location. I am not sure what form this driver will take in the near
> future, thus I would prefer to keep it in platform/chrome. Maybe
> cros_ec_ucsi isn't the best name here, cros_ec_pdc probably conveys
> the intention better.

In such a case please consider using auxilliary device bus or MFD
cells to describe pdc / ucsi relationship. See how this is handled for
pmic-glink / ucsi_glink.
The drivers/platform should really be limited to simple drivers, which
don't have a better place. Otherwise it becomes a nightmare to handle
driver changes (this applies not only to the UCSI but also to other
drivers which have their own subsystem tree).

--
With best wishes
Dmitry
Re: [PATCH v2 3/3] platform/chrome: cros_ec_ucsi: Implement UCSI PDC driver [ In reply to ]
On Fri, Mar 29, 2024 at 8:13?AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Fri, 29 Mar 2024 at 17:09, Pavan Holla <pholla@chromium.org> wrote:
> >
> > Hi Dmitry,
> >
> > Thanks for the review.
> >
> > On Thu, Mar 28, 2024 at 8:32?AM Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> > > While it's fine to use platform/chrome for platform drivers, please
> > > place drivers which have a subsystem into the subsystem dir. I think we
> > > don't want to hunt UCSI implementations all over the codebase. Please
> > > use drivers/usb/typec/ucsi/ location for your driver. This also removes
> > > a need for a global header.
> >
> > I agree with your assessment that drivers/usb/typec/ucsi/ is a good
> > location for the driver currently. However, the driver is still in
> > early stages of development. We may have to account for PDC/ EC quirks
> > (we have multiple vendors), add FW update functionality outside the
> > UCSI spec, or add PDC logging functionality. While I'd like to write
> > separate drivers for those, some of this functionality will need to
> > acquire a lock over communication to the PDC. Even if I were to write
> > separate drivers for new functionality related to the PDC, maybe it's
> > better for all ChromeOS PDC related drivers to reside in the same
> > location. I am not sure what form this driver will take in the near
> > future, thus I would prefer to keep it in platform/chrome. Maybe
> > cros_ec_ucsi isn't the best name here, cros_ec_pdc probably conveys
> > the intention better.
>
> In such a case please consider using auxilliary device bus or MFD
> cells to describe pdc / ucsi relationship. See how this is handled for
> pmic-glink / ucsi_glink.
> The drivers/platform should really be limited to simple drivers, which
> don't have a better place. Otherwise it becomes a nightmare to handle
> driver changes (this applies not only to the UCSI but also to other
> drivers which have their own subsystem tree).

Thanks for the pointers. I will move the driver to drivers/usb/typec/ucsi/