From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757386AbcBJL4G (ORCPT ); Wed, 10 Feb 2016 06:56:06 -0500 Received: from mail-yw0-f193.google.com ([209.85.161.193]:34139 "EHLO mail-yw0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753239AbcBJL4E (ORCPT ); Wed, 10 Feb 2016 06:56:04 -0500 MIME-Version: 1.0 In-Reply-To: <1455037283-106479-3-git-send-email-heikki.krogerus@linux.intel.com> References: <1455037283-106479-1-git-send-email-heikki.krogerus@linux.intel.com> <1455037283-106479-3-git-send-email-heikki.krogerus@linux.intel.com> Date: Wed, 10 Feb 2016 13:56:03 +0200 Message-ID: Subject: Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface From: Andy Shevchenko To: Heikki Krogerus Cc: Greg KH , USB , "linux-kernel@vger.kernel.org" , Mathias Nyman , Felipe Balbi Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Feb 9, 2016 at 7:01 PM, Heikki Krogerus 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 > + > +/* -------------------------------------------------------------------------- */ > + > +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