Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface
From: Andy Shevchenko
Date: Wed Feb 10 2016 - 06:56:10 EST
On Tue, Feb 9, 2016 at 7:01 PM, Heikki Krogerus
<heikki.krogerus@xxxxxxxxxxxxxxx> 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