mbox series

[v3,0/7] Thunderbolt and DP altmode support for cros-ec-typec

Message ID 20241107193021.2690050-1-abhishekpandit@chromium.org
Headers show
Series Thunderbolt and DP altmode support for cros-ec-typec | expand

Message

Abhishek Pandit-Subedi Nov. 7, 2024, 7:29 p.m. UTC
Hi Heikki, Tzung-Bi et al,

This patch series adds support for alternate mode entry for the
cros-ec-typec driver for Displayport and Thunderbolt.

Thunderbolt support is added by adapting an RFC Heikki had posted
previously:

https://lore.kernel.org/linux-usb/20191230152857.43917-1-heikki.krogerus@linux.intel.com/

A few comments on the series:

* The cros-ec interface will not accept any VDOs/VDMs so we simply
  ignore any configurations we are passed (i.e. DPConfigure). This means
  the sysfs control of DP lanes won't work.
* ChromeOS has two modes of operation for alt-modes: entirely EC driven
  or AP-driven from userspace (via the typec daemon). Thus, we don't
  expect the kernel alt-mode drivers to auto-enter modes in all cases.
  This series allows auto-enter for displayport but disables it for TBT
  for this reason.

This was tested with a ChromeOS Brya device using kernel 6.6 and built
with allmodconfig for linux-usb.

Thanks,
Abhishek

Changes in v3:
- Removed mode from altmode device ids
- Updated modalias for typecd bus to remove mode
- Re-ordered to start of series
- Revert rename of TYPEC_TBT_MODE
- Remove mode from typec_device_id
- Use port.active instead of introducing auto-enter field
- Introduce inactive field to typec_altmode_desc to set default value
  for active.
- Always make port 'active' field writable
- Refactored typec_altmode_dp_data per review request
- Removed unused vdm operations during altmode registration
- Fix usage of TBT sid and mode.
- Removed unused vdm operations during altmode registration
- Set port.inactive = true instead of auto-enter.

Changes in v2:
- Update altmode_match to ignore mode entirely
- Also apply the same behavior to typec_match
- Use <linux/usb/typec_tbt.h> and add missing TBT_CABLE_ROUNDED
- Pass struct typec_thunderbolt_data to typec_altmode_notify
- Rename TYPEC_TBT_MODE to USB_TYPEC_TBT_MODE
- Use USB_TYPEC_TBT_SID and USB_TYPEC_TBT_MODE for device id
- Change module license to GPL due to checkpatch warning
- Refactored displayport into cros_typec_altmode.c to extract common
  implementation between altmodes
- Refactored thunderbolt support into cros_typec_altmode.c
- Only disable auto-enter for Thunderbolt
- Update commit message to clearly indicate the need for userspace
  intervention to enter TBT mode

Abhishek Pandit-Subedi (6):
  usb: typec: Only use SVID for matching altmodes
  usb: typec: Check port is active before enter mode on probe
  platform/chrome: cros_ec_typec: Update partner altmode active
  platform/chrome: cros_ec_typec: Displayport support
  platform/chrome: cros_ec_typec: Thunderbolt support
  platform/chrome: cros_ec_typec: Disable tbt on port

Heikki Krogerus (1):
  usb: typec: Add driver for Thunderbolt 3 Alternate Mode

 MAINTAINERS                                  |   3 +
 drivers/platform/chrome/Makefile             |   7 +
 drivers/platform/chrome/cros_ec_typec.c      |  47 ++-
 drivers/platform/chrome/cros_ec_typec.h      |   1 +
 drivers/platform/chrome/cros_typec_altmode.c | 360 +++++++++++++++++++
 drivers/platform/chrome/cros_typec_altmode.h |  48 +++
 drivers/usb/typec/altmodes/Kconfig           |   9 +
 drivers/usb/typec/altmodes/Makefile          |   2 +
 drivers/usb/typec/altmodes/displayport.c     |   9 +-
 drivers/usb/typec/altmodes/nvidia.c          |   2 +-
 drivers/usb/typec/altmodes/thunderbolt.c     | 312 ++++++++++++++++
 drivers/usb/typec/bus.c                      |   6 +-
 drivers/usb/typec/class.c                    |   9 +-
 include/linux/usb/typec.h                    |   2 +
 include/linux/usb/typec_tbt.h                |   1 +
 scripts/mod/devicetable-offsets.c            |   1 -
 scripts/mod/file2alias.c                     |   4 +-
 17 files changed, 793 insertions(+), 30 deletions(-)
 create mode 100644 drivers/platform/chrome/cros_typec_altmode.c
 create mode 100644 drivers/platform/chrome/cros_typec_altmode.h
 create mode 100644 drivers/usb/typec/altmodes/thunderbolt.c

Comments

Heikki Krogerus Nov. 8, 2024, 11:41 a.m. UTC | #1
On Thu, Nov 07, 2024 at 11:29:54AM -0800, Abhishek Pandit-Subedi wrote:
> Mode in struct typec_altmode is used to indicate the index of the
> altmode on a port, partner or plug. It is used in enter mode VDMs but
> doesn't make much sense for matching against altmode drivers or for
> matching partner to port altmodes.
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
> 
> Changes in v3:
> - Removed mode from altmode device ids
> - Updated modalias for typecd bus to remove mode
> - Re-ordered to start of series
> 
> Changes in v2:
> - Update altmode_match to ignore mode entirely
> - Also apply the same behavior to typec_match
> 
>  drivers/usb/typec/altmodes/displayport.c | 2 +-
>  drivers/usb/typec/altmodes/nvidia.c      | 2 +-
>  drivers/usb/typec/bus.c                  | 6 ++----
>  drivers/usb/typec/class.c                | 4 ++--
>  scripts/mod/devicetable-offsets.c        | 1 -
>  scripts/mod/file2alias.c                 | 4 +---
>  6 files changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
> index 2f03190a9873..3245e03d59e6 100644
> --- a/drivers/usb/typec/altmodes/displayport.c
> +++ b/drivers/usb/typec/altmodes/displayport.c
> @@ -791,7 +791,7 @@ void dp_altmode_remove(struct typec_altmode *alt)
>  EXPORT_SYMBOL_GPL(dp_altmode_remove);
>  
>  static const struct typec_device_id dp_typec_id[] = {
> -	{ USB_TYPEC_DP_SID, USB_TYPEC_DP_MODE },
> +	{ USB_TYPEC_DP_SID },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(typec, dp_typec_id);
> diff --git a/drivers/usb/typec/altmodes/nvidia.c b/drivers/usb/typec/altmodes/nvidia.c
> index fe70b36f078f..2b77d931e494 100644
> --- a/drivers/usb/typec/altmodes/nvidia.c
> +++ b/drivers/usb/typec/altmodes/nvidia.c
> @@ -24,7 +24,7 @@ static void nvidia_altmode_remove(struct typec_altmode *alt)
>  }
>  
>  static const struct typec_device_id nvidia_typec_id[] = {
> -	{ USB_TYPEC_NVIDIA_VLINK_SID, TYPEC_ANY_MODE },
> +	{ USB_TYPEC_NVIDIA_VLINK_SID },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(typec, nvidia_typec_id);
> diff --git a/drivers/usb/typec/bus.c b/drivers/usb/typec/bus.c
> index aa879253d3b8..ae90688d23e4 100644
> --- a/drivers/usb/typec/bus.c
> +++ b/drivers/usb/typec/bus.c
> @@ -454,8 +454,7 @@ static int typec_match(struct device *dev, const struct device_driver *driver)
>  	const struct typec_device_id *id;
>  
>  	for (id = drv->id_table; id->svid; id++)
> -		if (id->svid == altmode->svid &&
> -		    (id->mode == TYPEC_ANY_MODE || id->mode == altmode->mode))
> +		if (id->svid == altmode->svid)
>  			return 1;
>  	return 0;
>  }
> @@ -470,8 +469,7 @@ static int typec_uevent(const struct device *dev, struct kobj_uevent_env *env)
>  	if (add_uevent_var(env, "MODE=%u", altmode->mode))
>  		return -ENOMEM;
>  
> -	return add_uevent_var(env, "MODALIAS=typec:id%04Xm%02X",
> -			      altmode->svid, altmode->mode);
> +	return add_uevent_var(env, "MODALIAS=typec:id%04X", altmode->svid);
>  }
>  
>  static int typec_altmode_create_links(struct altmode *alt)
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index 4b3047e055a3..febe453b96be 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -237,13 +237,13 @@ static int altmode_match(struct device *dev, void *data)
>  	if (!is_typec_altmode(dev))
>  		return 0;
>  
> -	return ((adev->svid == id->svid) && (adev->mode == id->mode));
> +	return (adev->svid == id->svid);
>  }
>  
>  static void typec_altmode_set_partner(struct altmode *altmode)
>  {
>  	struct typec_altmode *adev = &altmode->adev;
> -	struct typec_device_id id = { adev->svid, adev->mode, };
> +	struct typec_device_id id = { adev->svid };
>  	struct typec_port *port = typec_altmode2port(adev);
>  	struct altmode *partner;
>  	struct device *dev;
> diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
> index 9c7b404defbd..d3d00e85edf7 100644
> --- a/scripts/mod/devicetable-offsets.c
> +++ b/scripts/mod/devicetable-offsets.c
> @@ -237,7 +237,6 @@ int main(void)
>  
>  	DEVID(typec_device_id);
>  	DEVID_FIELD(typec_device_id, svid);
> -	DEVID_FIELD(typec_device_id, mode);
>  
>  	DEVID(tee_client_device_id);
>  	DEVID_FIELD(tee_client_device_id, uuid);
> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> index c4cc11aa558f..218ccb7150bf 100644
> --- a/scripts/mod/file2alias.c
> +++ b/scripts/mod/file2alias.c
> @@ -1343,14 +1343,12 @@ static int do_tbsvc_entry(const char *filename, void *symval, char *alias)
>  	return 1;
>  }
>  
> -/* Looks like: typec:idNmN */
> +/* Looks like: typec:idN */
>  static int do_typec_entry(const char *filename, void *symval, char *alias)
>  {
>  	DEF_FIELD(symval, typec_device_id, svid);
> -	DEF_FIELD(symval, typec_device_id, mode);
>  
>  	sprintf(alias, "typec:id%04X", svid);
> -	ADD(alias, "m", mode != TYPEC_ANY_MODE, mode);
>  
>  	return 1;
>  }
> -- 
> 2.47.0.277.g8800431eea-goog
Heikki Krogerus Nov. 8, 2024, 12:21 p.m. UTC | #2
On Thu, Nov 07, 2024 at 11:29:56AM -0800, Abhishek Pandit-Subedi wrote:
> Enforce the same requirement as when we attempt to activate altmode via
> sysfs (do not enter partner mode if port mode is not active). In order
> to set a default value for this field, also introduce the inactive field
> in struct typec_altmode_desc.
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
> 
> Changes in v3:
> - Use port.active instead of introducing auto-enter field
> - Introduce inactive field to typec_altmode_desc to set default value
>   for active.
> - Always make port 'active' field writable
> 
>  drivers/usb/typec/altmodes/displayport.c | 7 +++++--
>  drivers/usb/typec/altmodes/thunderbolt.c | 6 +++++-
>  drivers/usb/typec/class.c                | 5 +++--
>  include/linux/usb/typec.h                | 2 ++
>  4 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
> index 3245e03d59e6..f4116e96f6a1 100644
> --- a/drivers/usb/typec/altmodes/displayport.c
> +++ b/drivers/usb/typec/altmodes/displayport.c
> @@ -767,8 +767,11 @@ int dp_altmode_probe(struct typec_altmode *alt)
>  	if (plug)
>  		typec_altmode_set_drvdata(plug, dp);
>  
> -	dp->state = plug ? DP_STATE_ENTER_PRIME : DP_STATE_ENTER;
> -	schedule_work(&dp->work);
> +	/* Only attempt to enter altmode if port is active. */
> +	if (port->active) {
> +		dp->state = plug ? DP_STATE_ENTER_PRIME : DP_STATE_ENTER;
> +		schedule_work(&dp->work);
> +	}
>  
>  	return 0;
>  }
> diff --git a/drivers/usb/typec/altmodes/thunderbolt.c b/drivers/usb/typec/altmodes/thunderbolt.c
> index a945b9d35a1d..45567abc3bb8 100644
> --- a/drivers/usb/typec/altmodes/thunderbolt.c
> +++ b/drivers/usb/typec/altmodes/thunderbolt.c
> @@ -212,6 +212,7 @@ static const struct typec_altmode_ops tbt_altmode_ops = {
>  
>  static int tbt_altmode_probe(struct typec_altmode *alt)
>  {
> +	const struct typec_altmode *port = typec_altmode_get_partner(alt);
>  	struct tbt_altmode *tbt;
>  
>  	tbt = devm_kzalloc(&alt->dev, sizeof(*tbt), GFP_KERNEL);
> @@ -226,7 +227,10 @@ static int tbt_altmode_probe(struct typec_altmode *alt)
>  	typec_altmode_set_drvdata(alt, tbt);
>  	typec_altmode_set_ops(alt, &tbt_altmode_ops);
>  
> -	if (tbt_ready(alt)) {
> +	/* Only attempt to enter altmode if port is active and cable/plug
> +	 * information is ready.
> +	 */
> +	if (port->active && tbt_ready(alt)) {
>  		if (tbt->plug[TYPEC_PLUG_SOP_PP])
>  			tbt->state = TBT_STATE_SOP_PP_ENTER;
>  		else if (tbt->plug[TYPEC_PLUG_SOP_P])
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index febe453b96be..b5e67a57762c 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -458,7 +458,8 @@ static umode_t typec_altmode_attr_is_visible(struct kobject *kobj,
>  	struct typec_altmode *adev = to_typec_altmode(kobj_to_dev(kobj));
>  
>  	if (attr == &dev_attr_active.attr)
> -		if (!adev->ops || !adev->ops->activate)
> +		if (!is_typec_port(adev->dev.parent) &&
> +		    (!adev->ops || !adev->ops->activate))
>  			return 0444;
>  
>  	return attr->mode;
> @@ -563,7 +564,7 @@ typec_register_altmode(struct device *parent,
>  
>  	if (is_port) {
>  		alt->attrs[3] = &dev_attr_supported_roles.attr;
> -		alt->adev.active = true; /* Enabled by default */
> +		alt->adev.active = !desc->inactive; /* Enabled by default */
>  	}
>  
>  	sprintf(alt->group_name, "mode%d", desc->mode);
> diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
> index d616b8807000..56c01771c190 100644
> --- a/include/linux/usb/typec.h
> +++ b/include/linux/usb/typec.h
> @@ -140,6 +140,7 @@ int typec_cable_set_identity(struct typec_cable *cable);
>   * @mode: Index of the Mode
>   * @vdo: VDO returned by Discover Modes USB PD command
>   * @roles: Only for ports. DRP if the mode is available in both roles
> + * @inactive: Only for ports. Make this port inactive (default is active).
>   *
>   * Description of an Alternate Mode which a connector, cable plug or partner
>   * supports.
> @@ -150,6 +151,7 @@ struct typec_altmode_desc {
>  	u32			vdo;
>  	/* Only used with ports */
>  	enum typec_port_data	roles;
> +	bool			inactive : 1;
>  };
>  
>  void typec_partner_set_pd_revision(struct typec_partner *partner, u16 pd_revision);
> -- 
> 2.47.0.277.g8800431eea-goog
Greg Kroah-Hartman Nov. 8, 2024, 1:17 p.m. UTC | #3
On Thu, Nov 07, 2024 at 11:29:56AM -0800, Abhishek Pandit-Subedi wrote:
> Enforce the same requirement as when we attempt to activate altmode via
> sysfs (do not enter partner mode if port mode is not active). In order
> to set a default value for this field, also introduce the inactive field
> in struct typec_altmode_desc.
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
> 
> Changes in v3:
> - Use port.active instead of introducing auto-enter field
> - Introduce inactive field to typec_altmode_desc to set default value
>   for active.
> - Always make port 'active' field writable
> 
>  drivers/usb/typec/altmodes/displayport.c | 7 +++++--
>  drivers/usb/typec/altmodes/thunderbolt.c | 6 +++++-
>  drivers/usb/typec/class.c                | 5 +++--
>  include/linux/usb/typec.h                | 2 ++
>  4 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
> index 3245e03d59e6..f4116e96f6a1 100644
> --- a/drivers/usb/typec/altmodes/displayport.c
> +++ b/drivers/usb/typec/altmodes/displayport.c
> @@ -767,8 +767,11 @@ int dp_altmode_probe(struct typec_altmode *alt)
>  	if (plug)
>  		typec_altmode_set_drvdata(plug, dp);
>  
> -	dp->state = plug ? DP_STATE_ENTER_PRIME : DP_STATE_ENTER;
> -	schedule_work(&dp->work);
> +	/* Only attempt to enter altmode if port is active. */
> +	if (port->active) {
> +		dp->state = plug ? DP_STATE_ENTER_PRIME : DP_STATE_ENTER;
> +		schedule_work(&dp->work);
> +	}

What prevents active from changing right after you checked it?

> @@ -150,6 +151,7 @@ struct typec_altmode_desc {
>  	u32			vdo;
>  	/* Only used with ports */
>  	enum typec_port_data	roles;
> +	bool			inactive : 1;

A boolean bitfield?  That's not needed, please just do this properly.

thanks,

greg k-h
Abhishek Pandit-Subedi Nov. 8, 2024, 6:28 p.m. UTC | #4
On Fri, Nov 8, 2024 at 5:17 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Nov 07, 2024 at 11:29:56AM -0800, Abhishek Pandit-Subedi wrote:
> > Enforce the same requirement as when we attempt to activate altmode via
> > sysfs (do not enter partner mode if port mode is not active). In order
> > to set a default value for this field, also introduce the inactive field
> > in struct typec_altmode_desc.
> >
> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > ---
> >
> > Changes in v3:
> > - Use port.active instead of introducing auto-enter field
> > - Introduce inactive field to typec_altmode_desc to set default value
> >   for active.
> > - Always make port 'active' field writable
> >
> >  drivers/usb/typec/altmodes/displayport.c | 7 +++++--
> >  drivers/usb/typec/altmodes/thunderbolt.c | 6 +++++-
> >  drivers/usb/typec/class.c                | 5 +++--
> >  include/linux/usb/typec.h                | 2 ++
> >  4 files changed, 15 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
> > index 3245e03d59e6..f4116e96f6a1 100644
> > --- a/drivers/usb/typec/altmodes/displayport.c
> > +++ b/drivers/usb/typec/altmodes/displayport.c
> > @@ -767,8 +767,11 @@ int dp_altmode_probe(struct typec_altmode *alt)
> >       if (plug)
> >               typec_altmode_set_drvdata(plug, dp);
> >
> > -     dp->state = plug ? DP_STATE_ENTER_PRIME : DP_STATE_ENTER;
> > -     schedule_work(&dp->work);
> > +     /* Only attempt to enter altmode if port is active. */
> > +     if (port->active) {
> > +             dp->state = plug ? DP_STATE_ENTER_PRIME : DP_STATE_ENTER;
> > +             schedule_work(&dp->work);
> > +     }
>
> What prevents active from changing right after you checked it?

There's another check in `typec_altmode_enter` for the port itself:
https://github.com/torvalds/linux/blob/master/drivers/usb/typec/bus.c#L138

If I leave this out, it will just fail in `dp_altmode_work` when it
calls `typec_altmode_enter` and will leave a dev_err("failed to enter
mode"). This might be more user friendly since it's visible to the
user that the partner supports the mode but the port blocked entry.
I'll update the message on mode entry to also print the return value
(-EPERM) in the next patch.

>
> > @@ -150,6 +151,7 @@ struct typec_altmode_desc {
> >       u32                     vdo;
> >       /* Only used with ports */
> >       enum typec_port_data    roles;
> > +     bool                    inactive : 1;
>
> A boolean bitfield?  That's not needed, please just do this properly.

Ack - will remove the bitfield. `struct typec_altmode` also does this
-- I'll remove that in a cleanup patch too.

>
> thanks,
>
> greg k-h
Dmitry Baryshkov Nov. 9, 2024, 6:38 a.m. UTC | #5
On Thu, Nov 07, 2024 at 11:29:58AM -0800, Abhishek Pandit-Subedi wrote:
> Add support for entering and exiting displayport alt-mode on systems
> using AP driven alt-mode.
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
> 
> Changes in v3:
> - Refactored typec_altmode_dp_data per review request
> - Removed unused vdm operations during altmode registration
> 
> Changes in v2:
> - Refactored displayport into cros_typec_altmode.c to extract common
>   implementation between altmodes
> 
>  MAINTAINERS                                  |   3 +
>  drivers/platform/chrome/Makefile             |   4 +
>  drivers/platform/chrome/cros_ec_typec.c      |  12 +-
>  drivers/platform/chrome/cros_ec_typec.h      |   1 +
>  drivers/platform/chrome/cros_typec_altmode.c | 275 +++++++++++++++++++
>  drivers/platform/chrome/cros_typec_altmode.h |  34 +++
>  6 files changed, 326 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/platform/chrome/cros_typec_altmode.c
>  create mode 100644 drivers/platform/chrome/cros_typec_altmode.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index cd6aa609deba..5f9d8b8f1cb3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5369,9 +5369,12 @@ F:	include/linux/platform_data/cros_usbpd_notify.h
>  
>  CHROMEOS EC USB TYPE-C DRIVER
>  M:	Prashant Malani <pmalani@chromium.org>
> +M:	Benson Leung <bleung@chromium.org>
> +M:	Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
>  L:	chrome-platform@lists.linux.dev
>  S:	Maintained
>  F:	drivers/platform/chrome/cros_ec_typec.*
> +F:	drivers/platform/chrome/cros_typec_altmode.*
>  F:	drivers/platform/chrome/cros_typec_switch.c
>  F:	drivers/platform/chrome/cros_typec_vdm.*
>  
> diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
> index 2dcc6ccc2302..2f90d4db8099 100644
> --- a/drivers/platform/chrome/Makefile
> +++ b/drivers/platform/chrome/Makefile
> @@ -18,7 +18,11 @@ obj-$(CONFIG_CROS_EC_SPI)		+= cros_ec_spi.o
>  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
> +ifneq ($(CONFIG_TYPEC_DP_ALTMODE),)
> +	cros-ec-typec-objs		+= cros_typec_altmode.o
> +endif
>  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
> index e3eabe5e42ac..3a6f5f2717b9 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -18,6 +18,7 @@
>  
>  #include "cros_ec_typec.h"
>  #include "cros_typec_vdm.h"
> +#include "cros_typec_altmode.h"
>  
>  #define DRV_NAME "cros-ec-typec"
>  
> @@ -293,12 +294,11 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec,
>  	desc.svid = USB_TYPEC_DP_SID;
>  	desc.mode = USB_TYPEC_DP_MODE;
>  	desc.vdo = DP_PORT_VDO;
> -	amode = typec_port_register_altmode(port->port, &desc);
> +	amode = cros_typec_register_displayport(port, &desc,
> +						typec->ap_driven_altmode);
>  	if (IS_ERR(amode))
>  		return PTR_ERR(amode);
>  	port->port_altmode[CROS_EC_ALTMODE_DP] = amode;
> -	typec_altmode_set_drvdata(amode, port);
> -	amode->ops = &port_amode_ops;
>  
>  	/*
>  	 * Register TBT compatibility alt mode. The EC will not enter the mode
> @@ -575,6 +575,10 @@ static int cros_typec_enable_dp(struct cros_typec_data *typec,
>  	if (!ret)
>  		ret = typec_mux_set(port->mux, &port->state);
>  
> +	if (!ret)
> +		cros_typec_displayport_status_update(port->state.alt,
> +						     port->state.data);
> +
>  	return ret;
>  }
>  
> @@ -1254,6 +1258,8 @@ static int cros_typec_probe(struct platform_device *pdev)
>  
>  	typec->typec_cmd_supported = cros_ec_check_features(ec_dev, EC_FEATURE_TYPEC_CMD);
>  	typec->needs_mux_ack = cros_ec_check_features(ec_dev, EC_FEATURE_TYPEC_MUX_REQUIRE_AP_ACK);
> +	typec->ap_driven_altmode = cros_ec_check_features(
> +		ec_dev, EC_FEATURE_TYPEC_REQUIRE_AP_MODE_ENTRY);
>  
>  	ret = cros_ec_cmd(typec->ec, 0, EC_CMD_USB_PD_PORTS, NULL, 0,
>  			  &resp, sizeof(resp));
> diff --git a/drivers/platform/chrome/cros_ec_typec.h b/drivers/platform/chrome/cros_ec_typec.h
> index deda180a646f..9fd5342bb0ad 100644
> --- a/drivers/platform/chrome/cros_ec_typec.h
> +++ b/drivers/platform/chrome/cros_ec_typec.h
> @@ -39,6 +39,7 @@ struct cros_typec_data {
>  	struct work_struct port_work;
>  	bool typec_cmd_supported;
>  	bool needs_mux_ack;
> +	bool ap_driven_altmode;
>  };
>  
>  /* Per port data. */
> diff --git a/drivers/platform/chrome/cros_typec_altmode.c b/drivers/platform/chrome/cros_typec_altmode.c
> new file mode 100644
> index 000000000000..3598b8a6ceee
> --- /dev/null
> +++ b/drivers/platform/chrome/cros_typec_altmode.c
> @@ -0,0 +1,275 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Alt-mode implementation on ChromeOS EC.
> + *
> + * Copyright 2024 Google LLC
> + * Author: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> + */
> +#include "cros_ec_typec.h"
> +
> +#include <linux/usb/typec_dp.h>
> +#include <linux/usb/pd_vdo.h>
> +
> +#include "cros_typec_altmode.h"
> +
> +struct cros_typec_altmode_data {
> +	struct work_struct work;
> +	struct cros_typec_port *port;
> +	struct typec_altmode *alt;
> +	bool ap_mode_entry;
> +
> +	u32 header;
> +	u32 *vdo_data;
> +	u8 vdo_size;
> +
> +	u16 sid;
> +	u8 mode;
> +};
> +
> +struct cros_typec_dp_data {
> +	struct cros_typec_altmode_data adata;
> +	struct typec_displayport_data data;
> +	bool configured;
> +	bool pending_status_update;
> +};
> +
> +static void cros_typec_altmode_work(struct work_struct *work)
> +{
> +	struct cros_typec_altmode_data *data =
> +		container_of(work, struct cros_typec_altmode_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;

What protects data->header / vdo_data / vdo_size from concurrent
modification?

> +}
> +
> +static int cros_typec_altmode_enter(struct typec_altmode *alt, u32 *vdo)
> +{
> +	struct cros_typec_altmode_data *data = typec_altmode_get_drvdata(alt);
> +	struct ec_params_typec_control req = {
> +		.port = data->port->port_num,
> +		.command = TYPEC_CONTROL_COMMAND_ENTER_MODE,
> +	};
> +	int svdm_version;
> +	int ret;
> +
> +	if (!data->ap_mode_entry) {
> +		const struct typec_altmode *partner =
> +			typec_altmode_get_partner(alt);
> +		dev_warn(&partner->dev,
> +			 "EC does not support ap driven mode entry\n");

Nit: AP, not ap

> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (data->sid == USB_TYPEC_DP_SID)
> +		req.mode_to_enter = CROS_EC_ALTMODE_DP;
> +	else
> +		return -EOPNOTSUPP;
> +
> +	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(data->sid, 1, svdm_version, CMD_ENTER_MODE);
> +	data->header |= VDO_OPOS(data->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_altmode_exit(struct typec_altmode *alt)
> +{
> +	struct cros_typec_altmode_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;
> +
> +	if (!data->ap_mode_entry) {
> +		const struct typec_altmode *partner =
> +			typec_altmode_get_partner(alt);
> +		dev_warn(&partner->dev,
> +			 "EC does not support ap driven mode entry\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	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(data->sid, 1, svdm_version, CMD_EXIT_MODE);
> +	data->header |= VDO_OPOS(data->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_displayport_vdm(struct typec_altmode *alt, u32 header,
> +				      const u32 *data, int count)
> +{
> +	struct cros_typec_dp_data *dp_data = typec_altmode_get_drvdata(alt);
> +	struct cros_typec_altmode_data *adata = &dp_data->adata;
> +
> +
> +	int cmd_type = PD_VDO_CMDT(header);
> +	int cmd = PD_VDO_CMD(header);
> +	int svdm_version;
> +
> +	if (!adata->ap_mode_entry) {
> +		const struct typec_altmode *partner =
> +			typec_altmode_get_partner(alt);
> +		dev_warn(&partner->dev,
> +			 "EC does not support ap driven mode entry\n");
> +		return -EOPNOTSUPP;
> +	}

Move the ckeck to cros_typec_altmode_vdm() ?

But this makes me wonder, should the driver use different ops in such a
case? Can't we just use a stubbed version of ops if
cros_typec_register_displayport() gets ap_mode_entry = false ?

> +
> +	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(adata->port->partner,
> +						       PD_VDO_SVDM_VER(header));
> +			svdm_version = PD_VDO_SVDM_VER(header);
> +		}
> +
> +		adata->header = VDO(adata->sid, 1, svdm_version, cmd);
> +		adata->header |= VDO_OPOS(adata->mode);
> +
> +		/*
> +		 * DP_CMD_CONFIGURE: We can't actually do anything with the
> +		 * provided VDO yet so just send back an ACK.
> +		 *
> +		 * DP_CMD_STATUS_UPDATE: We wait for Mux changes to send
> +		 * DPStatus Acks.
> +		 */
> +		switch (cmd) {
> +		case DP_CMD_CONFIGURE:
> +			dp_data->data.conf = *data;
> +			adata->header |= VDO_CMDT(CMDT_RSP_ACK);
> +			dp_data->configured = true;
> +			schedule_work(&adata->work);
> +			break;
> +		case DP_CMD_STATUS_UPDATE:
> +			dp_data->pending_status_update = true;
> +			break;
> +		default:
> +			adata->header |= VDO_CMDT(CMDT_RSP_ACK);
> +			schedule_work(&adata->work);
> +			break;
> +		}
> +
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cros_typec_altmode_vdm(struct typec_altmode *alt, u32 header,
> +				      const u32 *data, int count)
> +{
> +	struct cros_typec_altmode_data *adata = typec_altmode_get_drvdata(alt);
> +
> +	if (adata->sid == USB_TYPEC_DP_SID)
> +		return cros_typec_displayport_vdm(alt, header, data, count);
> +
> +	return -EINVAL;
> +}
> +
> +static const struct typec_altmode_ops cros_typec_altmode_ops = {
> +	.enter = cros_typec_altmode_enter,
> +	.exit = cros_typec_altmode_exit,
> +	.vdm = cros_typec_altmode_vdm,
> +};
> +
> +#if IS_ENABLED(CONFIG_TYPEC_DP_ALTMODE)
> +int cros_typec_displayport_status_update(struct typec_altmode *altmode,
> +					 struct typec_displayport_data *data)
> +{
> +	struct cros_typec_dp_data *dp_data =
> +		typec_altmode_get_drvdata(altmode);
> +	struct cros_typec_altmode_data *adata = &dp_data->adata;
> +
> +	if (!dp_data->pending_status_update) {
> +		dev_dbg(&altmode->dev,
> +			"Got DPStatus without a pending request");
> +		return 0;
> +	}
> +
> +	if (dp_data->configured && dp_data->data.conf != data->conf)
> +		dev_dbg(&altmode->dev,
> +			"DP Conf doesn't match. Requested 0x%04x, Actual 0x%04x",
> +			dp_data->data.conf, data->conf);
> +
> +	dp_data->data = *data;
> +	dp_data->pending_status_update = false;
> +	adata->header |= VDO_CMDT(CMDT_RSP_ACK);
> +	adata->vdo_data = &dp_data->data.status;
> +	adata->vdo_size = 2;
> +
> +	schedule_work(&adata->work);
> +	return 0;
> +}
> +
> +struct typec_altmode *
> +cros_typec_register_displayport(struct cros_typec_port *port,
> +				struct typec_altmode_desc *desc,
> +				bool ap_mode_entry)
> +{
> +	struct typec_altmode *alt;
> +	struct cros_typec_altmode_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_altmode_work);
> +	data->alt = alt;
> +	data->port = port;
> +	data->ap_mode_entry = ap_mode_entry;
> +	data->sid = desc->svid;
> +	data->mode = desc->mode;
> +
> +	typec_altmode_set_ops(alt, &cros_typec_altmode_ops);
> +	typec_altmode_set_drvdata(alt, data);
> +
> +	return alt;
> +}
> +#endif
> diff --git a/drivers/platform/chrome/cros_typec_altmode.h b/drivers/platform/chrome/cros_typec_altmode.h
> new file mode 100644
> index 000000000000..c6f8fb02c99c
> --- /dev/null
> +++ b/drivers/platform/chrome/cros_typec_altmode.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __CROS_TYPEC_ALTMODE_H__
> +#define __CROS_TYPEC_ALTMODE_H__
> +
> +struct cros_typec_port;
> +struct typec_altmode;
> +struct typec_altmode_desc;
> +struct typec_displayport_data;
> +
> +#if IS_ENABLED(CONFIG_TYPEC_DP_ALTMODE)
> +struct typec_altmode *
> +cros_typec_register_displayport(struct cros_typec_port *port,
> +				struct typec_altmode_desc *desc,
> +				bool ap_mode_entry);
> +
> +int cros_typec_displayport_status_update(struct typec_altmode *altmode,
> +					 struct typec_displayport_data *data);
> +#else
> +static inline struct typec_altmode *
> +cros_typec_register_displayport(struct cros_typec_port *port,
> +				struct typec_altmode_desc *desc,
> +				bool ap_mode_entry)
> +{
> +	return typec_port_register_altmode(port->port, desc);
> +}
> +
> +static inline int cros_typec_displayport_status_update(struct typec_altmode *altmode,
> +					 struct typec_displayport_data *data)
> +{
> +	return 0;
> +}
> +#endif
> +#endif /* __CROS_TYPEC_ALTMODE_H__ */
> -- 
> 2.47.0.277.g8800431eea-goog
>