Re: [PATCH v2 1/3] usb: typec: ucsi: return CCI and message from sync_control callback

[Łukasz Bartosik]

On Mon, Jan 20, 2025 at 10:16 AM Dmitry Baryshkov
<dmitry.baryshkov@xxxxxxxxxx> wrote:
>
> Some of the drivers emulate or handle some of the commands in the
> platform-specific way. The code ends up being split between several
> callbacks, which complicates emulation.
>
> In preparation to reworking such drivers, move read_cci() and
> read_message_in() calls into ucsi_sync_control_common().
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> ---
>  drivers/usb/typec/ucsi/cros_ec_ucsi.c |  5 +++--

Reviewed-by: Łukasz Bartosik <ukaszb@xxxxxxxxxxxx>

Thanks,
Łukasz

>  drivers/usb/typec/ucsi/ucsi.c         | 19 +++++++++++--------
>  drivers/usb/typec/ucsi/ucsi.h         |  6 ++++--
>  drivers/usb/typec/ucsi/ucsi_acpi.c    |  5 +++--
>  drivers/usb/typec/ucsi/ucsi_ccg.c     |  5 +++--
>  5 files changed, 24 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/usb/typec/ucsi/cros_ec_ucsi.c b/drivers/usb/typec/ucsi/cros_ec_ucsi.c
> index c605c8616726874434d5812731420fbc6f12790e..744f0709a40eddeed17f23e986f60e9f985381fd 100644
> --- a/drivers/usb/typec/ucsi/cros_ec_ucsi.c
> +++ b/drivers/usb/typec/ucsi/cros_ec_ucsi.c
> @@ -105,12 +105,13 @@ static int cros_ucsi_async_control(struct ucsi *ucsi, u64 cmd)
>         return 0;
>  }
>
> -static int cros_ucsi_sync_control(struct ucsi *ucsi, u64 cmd)
> +static int cros_ucsi_sync_control(struct ucsi *ucsi, u64 cmd, u32 *cci,
> +                                 void *data, size_t size)
>  {
>         struct cros_ucsi_data *udata = ucsi_get_drvdata(ucsi);
>         int ret;
>
> -       ret = ucsi_sync_control_common(ucsi, cmd);
> +       ret = ucsi_sync_control_common(ucsi, cmd, cci, data, size);
>         switch (ret) {
>         case -EBUSY:
>                 /* EC may return -EBUSY if CCI.busy is set.
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index fcf499cc9458c0d12015a7e36e5f1ac448c3a431..559390a07a4e427c9b520dffaac905277d071638 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -55,7 +55,8 @@ void ucsi_notify_common(struct ucsi *ucsi, u32 cci)
>  }
>  EXPORT_SYMBOL_GPL(ucsi_notify_common);
>
> -int ucsi_sync_control_common(struct ucsi *ucsi, u64 command)
> +int ucsi_sync_control_common(struct ucsi *ucsi, u64 command, u32 *cci,
> +                            void *data, size_t size)
>  {
>         bool ack = UCSI_COMMAND(command) == UCSI_ACK_CC_CI;
>         int ret;
> @@ -80,6 +81,13 @@ int ucsi_sync_control_common(struct ucsi *ucsi, u64 command)
>         else
>                 clear_bit(COMMAND_PENDING, &ucsi->flags);
>
> +       if (!ret && cci)
> +               ret = ucsi->ops->read_cci(ucsi, cci);
> +
> +       if (!ret && data &&
> +           (*cci & UCSI_CCI_COMMAND_COMPLETE))
> +               ret = ucsi->ops->read_message_in(ucsi, data, size);
> +
>         return ret;
>  }
>  EXPORT_SYMBOL_GPL(ucsi_sync_control_common);
> @@ -95,7 +103,7 @@ static int ucsi_acknowledge(struct ucsi *ucsi, bool conn_ack)
>                 ctrl |= UCSI_ACK_CONNECTOR_CHANGE;
>         }
>
> -       return ucsi->ops->sync_control(ucsi, ctrl);
> +       return ucsi->ops->sync_control(ucsi, ctrl, NULL, NULL, 0);
>  }
>
>  static int ucsi_run_command(struct ucsi *ucsi, u64 command, u32 *cci,
> @@ -108,9 +116,7 @@ static int ucsi_run_command(struct ucsi *ucsi, u64 command, u32 *cci,
>         if (size > UCSI_MAX_DATA_LENGTH(ucsi))
>                 return -EINVAL;
>
> -       ret = ucsi->ops->sync_control(ucsi, command);
> -       if (ucsi->ops->read_cci(ucsi, cci))
> -               return -EIO;
> +       ret = ucsi->ops->sync_control(ucsi, command, cci, data, size);
>
>         if (*cci & UCSI_CCI_BUSY)
>                 return ucsi_run_command(ucsi, UCSI_CANCEL, cci, NULL, 0, false) ?: -EBUSY;
> @@ -127,9 +133,6 @@ static int ucsi_run_command(struct ucsi *ucsi, u64 command, u32 *cci,
>         else
>                 err = 0;
>
> -       if (!err && data && UCSI_CCI_LENGTH(*cci))
> -               err = ucsi->ops->read_message_in(ucsi, data, size);
> -
>         /*
>          * Don't ACK connection change if there was an error.
>          */
> diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> index 5ff369c24a2fc445e8559201d563c31d83b2c876..9ffc8debd7d77d118db042f2749cf429a848f8df 100644
> --- a/drivers/usb/typec/ucsi/ucsi.h
> +++ b/drivers/usb/typec/ucsi/ucsi.h
> @@ -76,7 +76,8 @@ struct ucsi_operations {
>         int (*read_version)(struct ucsi *ucsi, u16 *version);
>         int (*read_cci)(struct ucsi *ucsi, u32 *cci);
>         int (*read_message_in)(struct ucsi *ucsi, void *val, size_t val_len);
> -       int (*sync_control)(struct ucsi *ucsi, u64 command);
> +       int (*sync_control)(struct ucsi *ucsi, u64 command, u32 *cci,
> +                           void *data, size_t size);
>         int (*async_control)(struct ucsi *ucsi, u64 command);
>         bool (*update_altmodes)(struct ucsi *ucsi, struct ucsi_altmode *orig,
>                                 struct ucsi_altmode *updated);
> @@ -528,7 +529,8 @@ void ucsi_altmode_update_active(struct ucsi_connector *con);
>  int ucsi_resume(struct ucsi *ucsi);
>
>  void ucsi_notify_common(struct ucsi *ucsi, u32 cci);
> -int ucsi_sync_control_common(struct ucsi *ucsi, u64 command);
> +int ucsi_sync_control_common(struct ucsi *ucsi, u64 command, u32 *cci,
> +                            void *data, size_t size);
>
>  #if IS_ENABLED(CONFIG_POWER_SUPPLY)
>  int ucsi_register_port_psy(struct ucsi_connector *con);
> diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
> index 5c55155519634d95b6e544632f869c2867093617..8b02082201ec5b85031472563b8b8d1eea6134de 100644
> --- a/drivers/usb/typec/ucsi/ucsi_acpi.c
> +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
> @@ -122,12 +122,13 @@ static int ucsi_gram_read_message_in(struct ucsi *ucsi, void *val, size_t val_le
>         return ret;
>  }
>
> -static int ucsi_gram_sync_control(struct ucsi *ucsi, u64 command)
> +static int ucsi_gram_sync_control(struct ucsi *ucsi, u64 command, u32 *cci,
> +                                 void *data, size_t size)
>  {
>         struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
>         int ret;
>
> -       ret = ucsi_sync_control_common(ucsi, command);
> +       ret = ucsi_sync_control_common(ucsi, command, cci, data, size);
>         if (ret < 0)
>                 return ret;
>
> diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
> index 740171f24ef9fae9abdb52d7995abe692e0a7623..02ac04a52239327475a3590734b77f3ac74bb589 100644
> --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
> +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> @@ -628,7 +628,8 @@ static int ucsi_ccg_async_control(struct ucsi *ucsi, u64 command)
>         return ccg_write(uc, reg, (u8 *)&command, sizeof(command));
>  }
>
> -static int ucsi_ccg_sync_control(struct ucsi *ucsi, u64 command)
> +static int ucsi_ccg_sync_control(struct ucsi *ucsi, u64 command, u32 *cci,
> +                                void *data, size_t size)
>  {
>         struct ucsi_ccg *uc = ucsi_get_drvdata(ucsi);
>         struct ucsi_connector *con;
> @@ -652,7 +653,7 @@ static int ucsi_ccg_sync_control(struct ucsi *ucsi, u64 command)
>                 ucsi_ccg_update_set_new_cam_cmd(uc, con, &command);
>         }
>
> -       ret = ucsi_sync_control_common(ucsi, command);
> +       ret = ucsi_sync_control_common(ucsi, command, cci, data, size);
>
>  err_put:
>         pm_runtime_put_sync(uc->dev);
>
> --
> 2.39.5
>