lkml.org 
[lkml]   [2016]   [Feb]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface
    From
    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

    \
     
     \ /
      Last update: 2016-02-10 13:21    [W:4.181 / U:0.052 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site