From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754341AbcBJLW3 (ORCPT ); Wed, 10 Feb 2016 06:22:29 -0500 Received: from mx2.suse.de ([195.135.220.15]:58526 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753981AbcBJLW1 (ORCPT ); Wed, 10 Feb 2016 06:22:27 -0500 Message-ID: <1455103193.8878.10.camel@suse.com> Subject: Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface From: Oliver Neukum To: Heikki Krogerus Cc: Greg KH , Felipe Balbi , Mathias Nyman , linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org Date: Wed, 10 Feb 2016 12:19:53 +0100 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> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.12.11 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > +static int ucsi_run_cmd(struct ucsi *ucsi, void *data, size_t size) > +{ > + int status; > + int ret; > + > + dev_vdbg(ucsi->dev, "%s control 0x%llx\n", __func__, > + ucsi->ppm->data->control); > + > + ret = ucsi->ppm->cmd(ucsi->ppm); > + if (ret) > + return ret; > + > + /* REVISIT: We may need to set UCSI_CCI_CMD_COMPLETE flag here */ > + wait_for_completion(&ucsi->complete); > + > + status = ucsi->status; > + if (status != UCSI_ERROR && size) > + memcpy(data, ucsi->ppm->data->message_in, size); > + > + ret = ucsi_ack(ucsi, UCSI_ACK_CMD); > + if (ret) > + goto out; > + > + if (status == UCSI_ERROR) { > + u16 error; > + > + ucsi->ppm->data->control = UCSI_GET_ERROR_STATUS; > + ret = ucsi->ppm->cmd(ucsi->ppm); > + if (ret) > + goto out; > + > + wait_for_completion(&ucsi->complete); > + > + /* Something has really gone wrong */ > + if (ucsi->status == UCSI_ERROR) { > + ret = -ENODEV; > + goto out; > + } > + > + memcpy(&error, ucsi->ppm->data->message_in, sizeof(error)); > + > + ret = ucsi_ack(ucsi, UCSI_ACK_CMD); > + if (ret) > + goto out; > + > + switch (error) { > + case UCSI_ERROR_INVALID_CON_NUM: > + ret = -ENXIO; > + break; > + case UCSI_ERROR_INCOMPATIBLE_PARTNER: > + case UCSI_ERROR_CC_COMMUNICATION_ERR: > + case UCSI_ERROR_CONTRACT_NEGOTIATION_FAIL: > + ret = -EIO; > + break; This looks like you mapped all the interesting error condition on a single error code and gave out other error codes to conditions of lesser importance. > + case UCSI_ERROR_DEAD_BATTERY: > + dev_warn(ucsi->dev, "Dead Battery Condition!\n"); > + ret = -EPERM; > + break; > + case UCSI_ERROR_UNREGONIZED_CMD: > + case UCSI_ERROR_INVALID_CMD_ARGUMENT: > + default: > + ret = -EINVAL; > + break; > + } > + } > +out: > + ucsi->ppm->data->control = 0; > + return ret; > +} [..] > +/** > + * ucsi_init - Initialize an UCSI Interface > + * @ucsi: The UCSI Interface > + * > + * Registers all the USB Type-C ports governed by the PPM of @ucsi and enables > + * all the notifications from the PPM. > + */ > +int ucsi_init(struct ucsi *ucsi) > +{ > + struct ucsi_control *ctrl = (void *)&ucsi->ppm->data->control; > + struct ucsi_connector *con; > + int ret; > + int i; > + > + /* Enable basic notifications */ > + ctrl->cmd = UCSI_SET_NOTIFICATION_ENABLE; > + ctrl->data = UCSI_ENABLE_NTFY_CMD_COMPLETE | UCSI_ENABLE_NTFY_ERROR; > + ret = ucsi_run_cmd(ucsi, NULL, 0); > + if (ret) > + return ret; > + > + /* Get PPM capabilities */ > + ctrl->cmd = UCSI_GET_CAPABILITY; > + ret = ucsi_run_cmd(ucsi, &ucsi->cap, sizeof(ucsi->cap)); > + if (ret) > + return ret; > + > + ucsi->connector = kcalloc(ucsi->cap.num_connectors, > + sizeof(struct ucsi_connector), GFP_KERNEL); > + if (!ucsi->connector) > + return -ENOMEM; > + > + for (i = 0, con = ucsi->connector; i < ucsi->cap.num_connectors; > + i++, con++) { > + struct typec_capability *cap = &con->typec_cap; > + struct ucsi_connector_status constat; > + > + /* Get connector capability */ > + ctrl->cmd = UCSI_GET_CONNECTOR_CAPABILITY; > + ctrl->data = i + 1; > + ret = ucsi_run_cmd(ucsi, &con->cap, sizeof(con->cap)); > + if (ret) > + goto err; > + > + /* Register the connector */ > + > + if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DRP) > + cap->type = TYPEC_PORT_DRP; > + else if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DFP) > + cap->type = TYPEC_PORT_DFP; > + else if (con->cap.op_mode & UCSI_CONCAP_OPMODE_UFP) > + cap->type = TYPEC_PORT_UFP; > + > + cap->usb_pd = !!(ucsi->cap.attributes & > + UCSI_CAP_ATTR_USB_PD); > + cap->audio_accessory = !!(con->cap.op_mode & > + UCSI_CONCAP_OPMODE_AUDIO_ACCESSORY); > + cap->debug_accessory = !!(con->cap.op_mode & > + UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY); > + > + /* TODO: Alt modes */ > + > + cap->dr_swap = ucsi_dr_swap; > + cap->pr_swap = ucsi_pr_swap; > + > + con->port = typec_register_port(ucsi->dev, cap); > + if (IS_ERR(con->port)) { > + ret = PTR_ERR(con->port); > + goto err; > + } > + > + con->num = i + 1; > + con->ucsi = ucsi; > + INIT_WORK(&con->work, ucsi_connector_change); You init the work later. Does this depend on the firmware generating no events before they are enabled? Depending on firmware is a bad idea. > + > + /* Check if the connector is connected */ > + if (WARN_ON(ucsi_get_constat(con, &constat) != 0)) > + continue; > + > + if (constat.connected) > + ucsi_connect(con, &constat); > + } > + > + /* Enable all notifications */ > + ctrl->cmd = UCSI_SET_NOTIFICATION_ENABLE; > + ctrl->data = UCSI_ENABLE_NTFY_ALL; > + ret = ucsi_run_cmd(ucsi, NULL, 0); > + if (ret) > + goto err; > + > + return 0; > +err: > + if (i > 0) > + for (; i >= 0; i--, con--) > + typec_unregister_port(con->port); > + > + kfree(ucsi->connector); > + return ret; > +} > +EXPORT_SYMBOL(ucsi_init); > +