From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757706AbcBJMEt (ORCPT ); Wed, 10 Feb 2016 07:04:49 -0500 Received: from mga04.intel.com ([192.55.52.120]:51045 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753843AbcBJMEs (ORCPT ); Wed, 10 Feb 2016 07:04:48 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,425,1449561600"; d="scan'208";a="912256034" Date: Wed, 10 Feb 2016 14:04:44 +0200 From: Heikki Krogerus To: Oliver Neukum Cc: Greg KH , Felipe Balbi , Mathias Nyman , linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org Subject: Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Message-ID: <20160210120444.GH5270@kuha.fi.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> <1455103193.8878.10.camel@suse.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1455103193.8878.10.camel@suse.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Feb 10, 2016 at 12:19:53PM +0100, Oliver Neukum wrote: > > > +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. OK, I'll fix those. > > + 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. If there is a real risk of getting connector change events before we have enabled them, I think the only thing we can do is add checks to ucsi_interrupt() to make sure that we don't try to handle events from connectors that are not yet initialized. Thanks, -- heikki