Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Tue, Feb 9, 2016 at 7:01 PM, Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
> USB Type-C Connector System Software Interface (UCSI) is a
> specification that defines registers and data structures
> used to interface with the USB Type-C connectors on a system.
>
> The specification is public and available at:
> http://www.intel.com/content/www/us/en/io/universal-serial-bus/usb-type-c-ucsi-spec.html

+#define to_ucsi_connector(_port_) container_of(_port_->cap,
         \
+                                              struct ucsi_connector,          \
+                                              typec_cap)

Perhaps
static inline ...

+
+#define cci_to_connector(_ucsi_, cci) (_ucsi_->connector +                    \
+                                       UCSI_CCI_CONNECTOR_CHANGE(cci) - 1)
+

If you start you macro on the next line it will look better.

> +static int
> +ucsi_connect(struct ucsi_connector *con, struct ucsi_connector_status *constat)
> +{
> +       struct typec_port *port = con->port;
> +
> +       port->connected = true;
> +
> +       if (constat->partner_flags & UCSI_CONSTAT_PARTNER_FLAG_ALT_MODE)
> +               port->partner_type = TYPEC_PARTNER_ALTMODE;
> +       else
> +               port->partner_type = TYPEC_PARTNER_USB;
> +
> +       switch (constat->partner_type) {
> +       case UCSI_CONSTAT_PARTNER_TYPE_CABLE_NO_UFP:
> +               /* REVISIT: We don't care about just the cable for now */
> +               return 0;
> +       case UCSI_CONSTAT_PARTNER_TYPE_DFP:
> +       case UCSI_CONSTAT_PARTNER_TYPE_CABLE_AND_UFP:
> +               port->pwr_role = TYPEC_PWR_SINK;
> +               port->data_role = TYPEC_DEVICE;
> +               break;
> +       case UCSI_CONSTAT_PARTNER_TYPE_UFP:
> +               port->pwr_role = TYPEC_PWR_SOURCE;
> +               port->data_role = TYPEC_HOST;
> +               break;
> +       case UCSI_CONSTAT_PARTNER_TYPE_DEBUG:
> +               port->partner_type = TYPEC_PARTNER_DEBUG;
> +               goto out;
> +       case UCSI_CONSTAT_PARTNER_TYPE_AUDIO:
> +               port->partner_type = TYPEC_PARTNER_AUDIO;
> +               goto out;
> +       }
> +
> +       switch (constat->pwr_op_mode) {
> +       case UCSI_CONSTAT_PWR_OPMODE_NONE:
> +       case UCSI_CONSTAT_PWR_OPMODE_DEFAULT:
> +               port->pwr_opmode = TYPEC_PWR_MODE_USB;
> +               break;
> +       case UCSI_CONSTAT_PWR_OPMODE_BC:
> +               port->partner_type = TYPEC_PARTNER_CHARGER;
> +               port->pwr_opmode = TYPEC_PWR_MODE_BC1_2;
> +               break;
> +       case UCSI_CONSTAT_PWR_OPMODE_PD:
> +               port->pwr_opmode = TYPEC_PWR_MODE_PD;
> +               break;
> +       case UCSI_CONSTAT_PWR_OPMODE_TYPEC1_3:
> +               port->pwr_opmode = TYPEC_PWR_MODE_1_5A;
> +               break;
> +       case UCSI_CONSTAT_PWR_OPMODE_TYPEC3_0:
> +               port->pwr_opmode = TYPEC_PWR_MODE_3_0A;
> +               break;
> +       default:
> +               break;
> +       }
> +out:

CodingStyle suggests to do a better label naming.

> +       return typec_connect(port);
> +}

> +static void ucsi_connector_change(struct work_struct *work)
> +{
> +       struct ucsi_connector *con = container_of(work, struct ucsi_connector,
> +                                                 work);

> +}


> +int ucsi_init(struct ucsi *ucsi)
> +{
> +       struct ucsi_control *ctrl = (void *)&ucsi->ppm->data->control;
> +       struct ucsi_connector *con;
> +       int ret;

> +       int i;

> +       for (i = 0, con = ucsi->connector; i < ucsi->cap.num_connectors;
> +            i++, con++) {

> +err:
> +       if (i > 0)
> +               for (; i >= 0; i--, con--)
> +                       typec_unregister_port(con->port);

Perhaps

while (--i >= 0) {
 ...
}

But...

> +
> +       kfree(ucsi->connector);
> +       return ret;
> +}

> +struct ucsi *ucsi_register_ppm(struct device *dev, struct ucsi_ppm *ppm)
> +{
> +       struct ucsi *ucsi;
> +
> +       ucsi = kzalloc(sizeof(*ucsi), GFP_KERNEL);
> +       if (!ucsi)
> +               return ERR_PTR(-ENOMEM);
> +
> +       init_completion(&ucsi->complete);
> +       ucsi->dev = dev;
> +       ucsi->ppm = ppm;
> +
> +       return ucsi;
> +}
> +EXPORT_SYMBOL_GPL(ucsi_register_ppm);
> +
> +/**
> + * ucsi_unregister_ppm - Unregister UCSI PPM Interface
> + * @ucsi: struct ucsi associated with the PPM
> + *
> + * Unregister an UCSI PPM that was created with ucsi_register().
> + */
> +void ucsi_unregister_ppm(struct ucsi *ucsi)
> +{
> +       struct ucsi_connector *con;
> +       int i;
> +
> +       /* Disable all notifications */
> +       ucsi->ppm->data->control = UCSI_SET_NOTIFICATION_ENABLE;
> +       ucsi->ppm->cmd(ucsi->ppm);
> +

> +       for (i = 0, con = ucsi->connector; i < ucsi->cap.num_connectors;
> +            i++, con++)
> +               typec_unregister_port(con->port);

...looks a code duplication.

> +
> +       kfree(ucsi->connector);
> +       kfree(ucsi);
> +}
> +EXPORT_SYMBOL_GPL(ucsi_unregister_ppm);

> +#include <linux/types.h>
> +
> +/* -------------------------------------------------------------------------- */
> +
> +struct ucsi_data {
> +       __u16 version;
> +       __u16 RESERVED;

Small letters?

> +       __u32 cci;
> +       __u64 control;
> +       __u32 message_in[4];
> +       __u32 message_out[4];
> +} __packed;

> +/* Bits for SET_NOTIFICATION_ENABLE command */
> +#define UCSI_ENABLE_NTFY_CMD_COMPLETE          BIT(0)
> +#define UCSI_ENABLE_NTFY_EXT_PWR_SRC_CHANGE    BIT(1)
> +#define UCSI_ENABLE_NTFY_PWR_OPMODE_CHANGE     BIT(2)
> +#define UCSI_ENABLE_NTFY_CAP_CHANGE            BIT(5)
> +#define UCSI_ENABLE_NTFY_PWR_LEVEL_CHANGE      BIT(6)
> +#define UCSI_ENABLE_NTFY_PD_RESET_COMPLETE     BIT(7)
> +#define UCSI_ENABLE_NTFY_CAM_CHANGE            BIT(8)
> +#define UCSI_ENABLE_NTFY_BAT_STATUS_CHANGE     BIT(9)
> +#define UCSI_ENABLE_NTFY_PARTNER_CHANGE                BIT(11)
> +#define UCSI_ENABLE_NTFY_PWR_DIR_CHANGE                BIT(12)
> +#define UCSI_ENABLE_NTFY_CONNECTOR_CHANGE      BIT(14)
> +#define UCSI_ENABLE_NTFY_ERROR                 BIT(15)

> +#define UCSI_ENABLE_NTFY_ALL                   0xdbf3

Hmm... I would add the decode part of this, i.e. what kind of
notifications are excluded (and maybe why), e.g. BIT(2).


> +       __u8 num_alt_modes;
> +       __u8 RESERVED;

Small letters?

> +       __u16 bc_version;
> +       __u16 pd_version;
> +       __u16 typec_version;
> +} __packed;

> +       __u8 mode_support:1;
> +       __u8 RESERVED_2:2;
> +       __u8 latency:4;
> +       __u8 RESERVED_4:4;
> +} __packed;

Ditto.

-- 
With Best Regards,
Andy Shevchenko