Re: [PATCH 1/5] usb: typec: ucsi: add callback for connector status updates

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

On Mon, Apr 08, 2024 at 07:30:49AM +0300, Dmitry Baryshkov wrote:
> Allow UCSI glue driver to perform addtional work to update connector
> status. For example, it might check the cable orientation.  This call is
> performed after reading new connector statatus, so the platform driver
> can peek at new connection status bits.
> 
> The callback is called both when registering the port and when the
> connector change event is being handled.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/usb/typec/ucsi/ucsi.c | 6 ++++++
>  drivers/usb/typec/ucsi/ucsi.h | 3 +++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 3106e69050cd..7ad544c968e4 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -1199,6 +1199,9 @@ static void ucsi_handle_connector_change(struct work_struct *work)
>  
>  	trace_ucsi_connector_change(con->num, &con->status);
>  
> +	if (ucsi->ops->connector_status)
> +		ucsi->ops->connector_status(con);
> +
>  	role = !!(con->status.flags & UCSI_CONSTAT_PWR_DIR);
>  
>  	if (con->status.change & UCSI_CONSTAT_POWER_DIR_CHANGE) {
> @@ -1588,6 +1591,9 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con)
>  	}
>  	ret = 0; /* ucsi_send_command() returns length on success */
>  
> +	if (ucsi->ops->connector_status)
> +		ucsi->ops->connector_status(con);
> +
>  	switch (UCSI_CONSTAT_PARTNER_TYPE(con->status.flags)) {
>  	case UCSI_CONSTAT_PARTNER_TYPE_UFP:
>  	case UCSI_CONSTAT_PARTNER_TYPE_CABLE_AND_UFP:
> diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> index 2caf2969668c..6599fbd09bee 100644
> --- a/drivers/usb/typec/ucsi/ucsi.h
> +++ b/drivers/usb/typec/ucsi/ucsi.h
> @@ -53,12 +53,14 @@ struct dentry;
>  #define UCSI_CCI_ERROR			BIT(30)
>  #define UCSI_CCI_COMMAND_COMPLETE	BIT(31)
>  
> +struct ucsi_connector;

Let's keep the forward declarations in one place. Please move that to
the beginning of the file where the other forward declarations are.

thanks,

>  /**
>   * 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
> + * @connector_status: Updates connector status, called holding connector lock
>   *
>   * 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
> @@ -73,6 +75,7 @@ struct ucsi_operations {
>  			   const void *val, size_t val_len);
>  	bool (*update_altmodes)(struct ucsi *ucsi, struct ucsi_altmode *orig,
>  				struct ucsi_altmode *updated);
> +	void (*connector_status)(struct ucsi_connector *con);
>  };
>  
>  struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops);

-- 
heikki