[PATCH 7/8] platform/chrome: cros_ec_typec: Thunderbolt support

Abhishek Pandit-Subedi posted 8 patches 2 months ago
There is a newer version of this series
[PATCH 7/8] platform/chrome: cros_ec_typec: Thunderbolt support
Posted by Abhishek Pandit-Subedi 2 months ago
Add support for entering and exiting Thunderbolt alt-mode using AP
driven alt-mode.

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---

 drivers/platform/chrome/Makefile              |   1 +
 drivers/platform/chrome/cros_ec_typec.c       |  29 +--
 drivers/platform/chrome/cros_typec_altmode.h  |  14 ++
 .../platform/chrome/cros_typec_thunderbolt.c  | 184 ++++++++++++++++++
 4 files changed, 216 insertions(+), 12 deletions(-)
 create mode 100644 drivers/platform/chrome/cros_typec_thunderbolt.c

diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
index fe6c5234ac27..da7a44738171 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
 cros-ec-typec-$(CONFIG_TYPEC_DP_ALTMODE) += cros_typec_displayport.o
+cros-ec-typec-$(CONFIG_TYPEC_TBT_ALTMODE) += cros_typec_thunderbolt.o
 obj-$(CONFIG_CROS_EC_TYPEC)		+= cros-ec-typec.o
 
 obj-$(CONFIG_CROS_EC_LPC)		+= cros_ec_lpcs.o
diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index f9221d0d95f5..ec13d84d11b8 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -304,21 +304,26 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec,
 	typec_altmode_set_drvdata(amode, port);
 	amode->ops = &port_amode_ops;
 #endif
-
 	/*
 	 * Register TBT compatibility alt mode. The EC will not enter the mode
-	 * if it doesn't support it, so it's safe to register it unconditionally
-	 * here for now.
+	 * if it doesn't support it and it will not enter automatically by
+	 * design so we can use the |ap_driven_altmode| feature to check if we
+	 * should register it.
 	 */
-	memset(&desc, 0, sizeof(desc));
-	desc.svid = USB_TYPEC_TBT_SID;
-	desc.mode = TYPEC_ANY_MODE;
-	amode = typec_port_register_altmode(port->port, &desc);
-	if (IS_ERR(amode))
-		return PTR_ERR(amode);
-	port->port_altmode[CROS_EC_ALTMODE_TBT] = amode;
-	typec_altmode_set_drvdata(amode, port);
-	amode->ops = &port_amode_ops;
+	if (typec->ap_driven_altmode) {
+		memset(&desc, 0, sizeof(desc));
+		desc.svid = USB_TYPEC_TBT_SID;
+		desc.mode = TYPEC_ANY_MODE;
+		amode = cros_typec_register_thunderbolt(port, &desc);
+		if (IS_ERR(amode))
+			return PTR_ERR(amode);
+		port->port_altmode[CROS_EC_ALTMODE_TBT] = amode;
+
+#if !IS_ENABLED(CONFIG_TYPEC_TBT_ALTMODE)
+		typec_altmode_set_drvdata(amode, port);
+		amode->ops = &port_amode_ops;
+#endif
+	}
 
 	port->state.alt = NULL;
 	port->state.mode = TYPEC_STATE_USB;
diff --git a/drivers/platform/chrome/cros_typec_altmode.h b/drivers/platform/chrome/cros_typec_altmode.h
index a8b37a18c83a..24e766189211 100644
--- a/drivers/platform/chrome/cros_typec_altmode.h
+++ b/drivers/platform/chrome/cros_typec_altmode.h
@@ -31,4 +31,18 @@ int cros_typec_displayport_status_update(struct typec_altmode *altmode,
 	return 0;
 }
 #endif
+
+#if IS_ENABLED(CONFIG_TYPEC_TBT_ALTMODE)
+struct typec_altmode *
+cros_typec_register_thunderbolt(struct cros_typec_port *port,
+				struct typec_altmode_desc *desc);
+#else
+struct typec_altmode *
+cros_typec_register_thunderbolt(struct cros_typec_port *port,
+				struct typec_altmode_desc *desc)
+{
+	return typec_port_register_altmode(port->port, desc);
+}
+#endif
+
 #endif /* __CROS_TYPEC_ALTMODE_H__ */
diff --git a/drivers/platform/chrome/cros_typec_thunderbolt.c b/drivers/platform/chrome/cros_typec_thunderbolt.c
new file mode 100644
index 000000000000..b399237b773f
--- /dev/null
+++ b/drivers/platform/chrome/cros_typec_thunderbolt.c
@@ -0,0 +1,184 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Alt-mode implementation for Thunderbolt on ChromeOS EC.
+ *
+ * Copyright 2024 Google LLC
+ * Author: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
+ */
+#include "cros_ec_typec.h"
+
+#include <linux/usb/typec_tbt.h>
+#include <linux/usb/pd_vdo.h>
+
+#include "cros_typec_altmode.h"
+
+struct typec_tbt_data {
+	struct work_struct work;
+
+	struct cros_typec_port *port;
+	struct typec_altmode *alt;
+
+	u32 header;
+	u32 *vdo_data;
+	u8 vdo_size;
+};
+
+static void cros_typec_thunderbolt_work(struct work_struct *work)
+{
+	struct typec_tbt_data *data =
+		container_of(work, struct typec_tbt_data, work);
+
+	if (typec_altmode_vdm(data->alt, data->header, data->vdo_data,
+			      data->vdo_size))
+		dev_err(&data->alt->dev, "VDM 0x%x failed", data->header);
+
+	data->header = 0;
+	data->vdo_data = NULL;
+	data->vdo_size = 0;
+}
+
+static int cros_typec_thunderbolt_enter(struct typec_altmode *alt, u32 *vdo)
+{
+	struct typec_tbt_data *data = typec_altmode_get_drvdata(alt);
+	struct ec_params_typec_control req = {
+		.port = data->port->port_num,
+		.command = TYPEC_CONTROL_COMMAND_ENTER_MODE,
+		.mode_to_enter = CROS_EC_ALTMODE_TBT,
+	};
+	int svdm_version;
+	int ret;
+
+	ret = cros_ec_cmd(data->port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL,
+			  &req, sizeof(req), NULL, 0);
+	if (ret < 0)
+		return ret;
+
+	svdm_version = typec_altmode_get_svdm_version(alt);
+	if (svdm_version < 0)
+		return svdm_version;
+
+	data->header = VDO(USB_TYPEC_TBT_SID, 1, svdm_version, CMD_ENTER_MODE);
+	data->header |= VDO_OPOS(TYPEC_TBT_MODE);
+	data->header |= VDO_CMDT(CMDT_RSP_ACK);
+
+	data->vdo_data = NULL;
+	data->vdo_size = 1;
+
+	schedule_work(&data->work);
+
+	return ret;
+}
+
+static int cros_typec_thunderbolt_exit(struct typec_altmode *alt)
+{
+	struct typec_tbt_data *data = typec_altmode_get_drvdata(alt);
+	struct ec_params_typec_control req = {
+		.port = data->port->port_num,
+		.command = TYPEC_CONTROL_COMMAND_EXIT_MODES,
+	};
+	int svdm_version;
+	int ret;
+
+	ret = cros_ec_cmd(data->port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL,
+			  &req, sizeof(req), NULL, 0);
+
+	if (ret < 0)
+		return ret;
+
+	svdm_version = typec_altmode_get_svdm_version(alt);
+	if (svdm_version < 0)
+		return svdm_version;
+
+	data->header = VDO(USB_TYPEC_TBT_SID, 1, svdm_version, CMD_EXIT_MODE);
+	data->header |= VDO_OPOS(TYPEC_TBT_MODE);
+	data->header |= VDO_CMDT(CMDT_RSP_ACK);
+
+	data->vdo_data = NULL;
+	data->vdo_size = 1;
+
+	schedule_work(&data->work);
+
+	return ret;
+}
+
+static int cros_typec_thunderbolt_vdm(struct typec_altmode *alt, u32 header,
+				      const u32 *data, int count)
+{
+	struct typec_tbt_data *tbt_data = typec_altmode_get_drvdata(alt);
+
+	int cmd_type = PD_VDO_CMDT(header);
+	int cmd = PD_VDO_CMD(header);
+	int svdm_version;
+
+	svdm_version = typec_altmode_get_svdm_version(alt);
+	if (svdm_version < 0)
+		return svdm_version;
+
+	switch (cmd_type) {
+	case CMDT_INIT:
+		if (PD_VDO_SVDM_VER(header) < svdm_version) {
+			typec_partner_set_svdm_version(tbt_data->port->partner,
+						       PD_VDO_SVDM_VER(header));
+			svdm_version = PD_VDO_SVDM_VER(header);
+		}
+
+		tbt_data->header = VDO(USB_TYPEC_TBT_SID, 1, svdm_version, cmd);
+		tbt_data->header |= VDO_OPOS(TYPEC_TBT_MODE);
+
+		/*
+		 * TODO - Just always reply to the VDMs that we are done.
+		 */
+		switch (cmd) {
+		case CMD_ENTER_MODE:
+			/* Don't respond to the enter mode vdm because it
+			 * triggers mux configuration. This is handled directly
+			 * by the cros_ec_typec driver so the Thunderbolt driver
+			 * doesn't need to be involved.
+			 */
+			break;
+		default:
+			tbt_data->header |= VDO_CMDT(CMDT_RSP_ACK);
+			schedule_work(&tbt_data->work);
+			break;
+		}
+
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static const struct typec_altmode_ops cros_typec_thunderbolt_ops = {
+	.enter = cros_typec_thunderbolt_enter,
+	.exit = cros_typec_thunderbolt_exit,
+	.vdm = cros_typec_thunderbolt_vdm,
+};
+
+struct typec_altmode *
+cros_typec_register_thunderbolt(struct cros_typec_port *port,
+				struct typec_altmode_desc *desc)
+{
+	struct typec_altmode *alt;
+	struct typec_tbt_data *data;
+
+	alt = typec_port_register_altmode(port->port, desc);
+	if (IS_ERR(alt))
+		return alt;
+
+	data = devm_kzalloc(&alt->dev, sizeof(*data), GFP_KERNEL);
+	if (!data) {
+		typec_unregister_altmode(alt);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	INIT_WORK(&data->work, cros_typec_thunderbolt_work);
+	data->alt = alt;
+	data->port = port;
+
+	typec_altmode_set_ops(alt, &cros_typec_thunderbolt_ops);
+	typec_altmode_set_drvdata(alt, data);
+
+	return alt;
+}
-- 
2.46.0.792.g87dc391469-goog
Re: [PATCH 7/8] platform/chrome: cros_ec_typec: Thunderbolt support
Posted by kernel test robot 2 months ago
Hi Abhishek,

kernel test robot noticed the following build errors:

[auto build test ERROR on chrome-platform/for-next]
[also build test ERROR on chrome-platform/for-firmware-next usb/usb-testing usb/usb-next usb/usb-linus linus/master v6.11 next-20240927]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Abhishek-Pandit-Subedi/usb-typec-Add-driver-for-Thunderbolt-3-Alternate-Mode/20240926-002931
base:   https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git for-next
patch link:    https://lore.kernel.org/r/20240925092505.7.Ic61ced3cdfb5d6776435356061f12307da719829%40changeid
patch subject: [PATCH 7/8] platform/chrome: cros_ec_typec: Thunderbolt support
config: i386-randconfig-r133-20240927 (https://download.01.org/0day-ci/archive/20240927/202409271548.taoANDmm-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240927/202409271548.taoANDmm-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409271548.taoANDmm-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: drivers/platform/chrome/cros_typec_thunderbolt.o: in function `cros_typec_register_displayport':
>> drivers/platform/chrome/cros_typec_altmode.h:24: multiple definition of `cros_typec_register_displayport'; drivers/platform/chrome/cros_ec_typec.o:drivers/platform/chrome/cros_typec_altmode.h:24: first defined here
   ld: drivers/platform/chrome/cros_typec_thunderbolt.o: in function `cros_typec_displayport_status_update':
>> drivers/platform/chrome/cros_typec_altmode.h:30: multiple definition of `cros_typec_displayport_status_update'; drivers/platform/chrome/cros_ec_typec.o:drivers/platform/chrome/cros_typec_altmode.h:30: first defined here


vim +24 drivers/platform/chrome/cros_typec_altmode.h

a57f16462c7621 Abhishek Pandit-Subedi 2024-09-25  10  
a57f16462c7621 Abhishek Pandit-Subedi 2024-09-25  11  #if IS_ENABLED(CONFIG_TYPEC_DP_ALTMODE)
a57f16462c7621 Abhishek Pandit-Subedi 2024-09-25  12  struct typec_altmode *
a57f16462c7621 Abhishek Pandit-Subedi 2024-09-25  13  cros_typec_register_displayport(struct cros_typec_port *port,
a57f16462c7621 Abhishek Pandit-Subedi 2024-09-25  14  				struct typec_altmode_desc *desc,
a57f16462c7621 Abhishek Pandit-Subedi 2024-09-25  15  				bool ap_mode_entry);
a57f16462c7621 Abhishek Pandit-Subedi 2024-09-25  16  
a57f16462c7621 Abhishek Pandit-Subedi 2024-09-25  17  int cros_typec_displayport_status_update(struct typec_altmode *altmode,
a57f16462c7621 Abhishek Pandit-Subedi 2024-09-25  18  					 struct typec_displayport_data *data);
a57f16462c7621 Abhishek Pandit-Subedi 2024-09-25  19  #else
a57f16462c7621 Abhishek Pandit-Subedi 2024-09-25  20  struct typec_altmode *
a57f16462c7621 Abhishek Pandit-Subedi 2024-09-25  21  cros_typec_register_displayport(struct cros_typec_port *port,
a57f16462c7621 Abhishek Pandit-Subedi 2024-09-25  22  				struct typec_altmode_desc *desc,
a57f16462c7621 Abhishek Pandit-Subedi 2024-09-25  23  				bool ap_mode_entry)
a57f16462c7621 Abhishek Pandit-Subedi 2024-09-25 @24  {
a57f16462c7621 Abhishek Pandit-Subedi 2024-09-25  25  	return typec_port_register_altmode(port->port, desc);
a57f16462c7621 Abhishek Pandit-Subedi 2024-09-25  26  }
a57f16462c7621 Abhishek Pandit-Subedi 2024-09-25  27  
a57f16462c7621 Abhishek Pandit-Subedi 2024-09-25  28  int cros_typec_displayport_status_update(struct typec_altmode *altmode,
a57f16462c7621 Abhishek Pandit-Subedi 2024-09-25  29  					 struct typec_displayport_data *data)
a57f16462c7621 Abhishek Pandit-Subedi 2024-09-25 @30  {
a57f16462c7621 Abhishek Pandit-Subedi 2024-09-25  31  	return 0;
a57f16462c7621 Abhishek Pandit-Subedi 2024-09-25  32  }
a57f16462c7621 Abhishek Pandit-Subedi 2024-09-25  33  #endif
4db96705bb611f Abhishek Pandit-Subedi 2024-09-25  34  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH 7/8] platform/chrome: cros_ec_typec: Thunderbolt support
Posted by kernel test robot 2 months ago
Hi Abhishek,

kernel test robot noticed the following build errors:

[auto build test ERROR on chrome-platform/for-next]
[also build test ERROR on chrome-platform/for-firmware-next usb/usb-testing usb/usb-next usb/usb-linus linus/master v6.11 next-20240926]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Abhishek-Pandit-Subedi/usb-typec-Add-driver-for-Thunderbolt-3-Alternate-Mode/20240926-002931
base:   https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git for-next
patch link:    https://lore.kernel.org/r/20240925092505.7.Ic61ced3cdfb5d6776435356061f12307da719829%40changeid
patch subject: [PATCH 7/8] platform/chrome: cros_ec_typec: Thunderbolt support
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20240926/202409262259.lXP7G1FE-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240926/202409262259.lXP7G1FE-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409262259.lXP7G1FE-lkp@intel.com/

All errors (new ones prefixed by >>):

   sh4-linux-ld: drivers/platform/chrome/cros_typec_thunderbolt.o: in function `cros_typec_register_displayport':
>> (.text+0x34c): multiple definition of `cros_typec_register_displayport'; drivers/platform/chrome/cros_ec_typec.o:(.text+0x2118): first defined here
   sh4-linux-ld: drivers/platform/chrome/cros_typec_thunderbolt.o: in function `cros_typec_displayport_status_update':
>> (.text+0x37c): multiple definition of `cros_typec_displayport_status_update'; drivers/platform/chrome/cros_ec_typec.o:(.text+0x2148): first defined here

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH 7/8] platform/chrome: cros_ec_typec: Thunderbolt support
Posted by Dmitry Baryshkov 2 months ago
On Wed, Sep 25, 2024 at 09:25:08AM GMT, Abhishek Pandit-Subedi wrote:
> Add support for entering and exiting Thunderbolt alt-mode using AP
> driven alt-mode.
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
> 
>  drivers/platform/chrome/Makefile              |   1 +
>  drivers/platform/chrome/cros_ec_typec.c       |  29 +--
>  drivers/platform/chrome/cros_typec_altmode.h  |  14 ++
>  .../platform/chrome/cros_typec_thunderbolt.c  | 184 ++++++++++++++++++
>  4 files changed, 216 insertions(+), 12 deletions(-)
>  create mode 100644 drivers/platform/chrome/cros_typec_thunderbolt.c

This patch looks like nearly exact 1:1 copy of the previous one. Please
merge both altmode implementations in the same way as tcpm.c does.

-- 
With best wishes
Dmitry
Re: [PATCH 7/8] platform/chrome: cros_ec_typec: Thunderbolt support
Posted by Abhishek Pandit-Subedi 2 months ago
On Wed, Sep 25, 2024 at 10:13 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Wed, Sep 25, 2024 at 09:25:08AM GMT, Abhishek Pandit-Subedi wrote:
> > Add support for entering and exiting Thunderbolt alt-mode using AP
> > driven alt-mode.
> >
> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > ---
> >
> >  drivers/platform/chrome/Makefile              |   1 +
> >  drivers/platform/chrome/cros_ec_typec.c       |  29 +--
> >  drivers/platform/chrome/cros_typec_altmode.h  |  14 ++
> >  .../platform/chrome/cros_typec_thunderbolt.c  | 184 ++++++++++++++++++
> >  4 files changed, 216 insertions(+), 12 deletions(-)
> >  create mode 100644 drivers/platform/chrome/cros_typec_thunderbolt.c
>
> This patch looks like nearly exact 1:1 copy of the previous one. Please
> merge both altmode implementations in the same way as tcpm.c does.

It's easier for tcpm.c to have a merged implementation because it
simply forwards VDMs to the internal state machine to handle without
doing anything with them. Our implementation is closer to
ucsi/displayport.c which needs to maintain an internal state machine
for DP and TBT VDMs and respond differently.

I can merge the two but I'd like to understand intent (reduce code
duplication? reduce the number of files?). As it is, keeping the files
separate makes it easier to understand how each alt-mode operates in
my opinion.

>
> --
> With best wishes
> Dmitry
Re: [PATCH 7/8] platform/chrome: cros_ec_typec: Thunderbolt support
Posted by Dmitry Baryshkov 2 months ago
On Wed, Sep 25, 2024 at 11:42:46AM GMT, Abhishek Pandit-Subedi wrote:
> On Wed, Sep 25, 2024 at 10:13 AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Wed, Sep 25, 2024 at 09:25:08AM GMT, Abhishek Pandit-Subedi wrote:
> > > Add support for entering and exiting Thunderbolt alt-mode using AP
> > > driven alt-mode.
> > >
> > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > ---
> > >
> > >  drivers/platform/chrome/Makefile              |   1 +
> > >  drivers/platform/chrome/cros_ec_typec.c       |  29 +--
> > >  drivers/platform/chrome/cros_typec_altmode.h  |  14 ++
> > >  .../platform/chrome/cros_typec_thunderbolt.c  | 184 ++++++++++++++++++
> > >  4 files changed, 216 insertions(+), 12 deletions(-)
> > >  create mode 100644 drivers/platform/chrome/cros_typec_thunderbolt.c
> >
> > This patch looks like nearly exact 1:1 copy of the previous one. Please
> > merge both altmode implementations in the same way as tcpm.c does.
> 
> It's easier for tcpm.c to have a merged implementation because it
> simply forwards VDMs to the internal state machine to handle without
> doing anything with them. Our implementation is closer to
> ucsi/displayport.c which needs to maintain an internal state machine
> for DP and TBT VDMs and respond differently.
> 
> I can merge the two but I'd like to understand intent (reduce code
> duplication? reduce the number of files?). As it is, keeping the files
> separate makes it easier to understand how each alt-mode operates in
> my opinion.

Separate common code and AltMode-specific code. This way we reduce a
risk of errors fixed in only one of two drivers and at the same time the
driver clearly separates common vs specific code paths (e.g. VDM
handling is mode-specific, while the rest of the code is common).

> 
> >
> > --
> > With best wishes
> > Dmitry

-- 
With best wishes
Dmitry
Re: [PATCH 7/8] platform/chrome: cros_ec_typec: Thunderbolt support
Posted by Abhishek Pandit-Subedi 2 months ago
On Wed, Sep 25, 2024 at 2:20 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Wed, Sep 25, 2024 at 11:42:46AM GMT, Abhishek Pandit-Subedi wrote:
> > On Wed, Sep 25, 2024 at 10:13 AM Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> > >
> > > On Wed, Sep 25, 2024 at 09:25:08AM GMT, Abhishek Pandit-Subedi wrote:
> > > > Add support for entering and exiting Thunderbolt alt-mode using AP
> > > > driven alt-mode.
> > > >
> > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > > ---
> > > >
> > > >  drivers/platform/chrome/Makefile              |   1 +
> > > >  drivers/platform/chrome/cros_ec_typec.c       |  29 +--
> > > >  drivers/platform/chrome/cros_typec_altmode.h  |  14 ++
> > > >  .../platform/chrome/cros_typec_thunderbolt.c  | 184 ++++++++++++++++++
> > > >  4 files changed, 216 insertions(+), 12 deletions(-)
> > > >  create mode 100644 drivers/platform/chrome/cros_typec_thunderbolt.c
> > >
> > > This patch looks like nearly exact 1:1 copy of the previous one. Please
> > > merge both altmode implementations in the same way as tcpm.c does.
> >
> > It's easier for tcpm.c to have a merged implementation because it
> > simply forwards VDMs to the internal state machine to handle without
> > doing anything with them. Our implementation is closer to
> > ucsi/displayport.c which needs to maintain an internal state machine
> > for DP and TBT VDMs and respond differently.
> >
> > I can merge the two but I'd like to understand intent (reduce code
> > duplication? reduce the number of files?). As it is, keeping the files
> > separate makes it easier to understand how each alt-mode operates in
> > my opinion.
>
> Separate common code and AltMode-specific code. This way we reduce a
> risk of errors fixed in only one of two drivers and at the same time the
> driver clearly separates common vs specific code paths (e.g. VDM
> handling is mode-specific, while the rest of the code is common).

Ack -- I'll provide a merged solution in the next version of this patch series.

>
> >
> > >
> > > --
> > > With best wishes
> > > Dmitry
>
> --
> With best wishes
> Dmitry