On 22 March 2018 04:03, Guenter Roeck wrote: > > static enum pdo_err tcpm_caps_err(struct tcpm_port *port, const u32 *pdo, > > @@ -1308,6 +1347,26 @@ static enum pdo_err tcpm_caps_err(struct tcpm_port > *port, const u32 *pdo, > > pdo_min_voltage(pdo[i - 1]))) > > return PDO_ERR_DUPE_PDO; > > break; > > + /* > > + * The Programmable Power Supply APDOs, if present, > > + * shall be sent in Maximum Voltage order; > > + * lowest to highest. > > + */ > > + case PDO_TYPE_APDO: > > + if (pdo_apdo_type(pdo[i]) != APDO_TYPE_PPS) > > + break; > > + > > + if (pdo_pps_apdo_max_current(pdo[i]) < > > + pdo_pps_apdo_max_current(pdo[i - 1])) > > + return > PDO_ERR_PPS_APDO_NOT_SORTED; > > + else if ((pdo_pps_apdo_min_voltage(pdo[i]) == > > + pdo_pps_apdo_min_voltage(pdo[i - 1])) > && > > + (pdo_pps_apdo_max_voltage(pdo[i]) == > > + pdo_pps_apdo_max_voltage(pdo[i - 1])) > && > > + (pdo_pps_apdo_max_current(pdo[i]) == > > + pdo_pps_apdo_max_current(pdo[i - 1]))) > > Unnecessary ( ) I have to say I think it's neater/clearer with than without but if that's something you really don't like then I'll remove them. > > +static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 op_curr) > > +{ > > + unsigned int target_mw; > > + int ret = 0; > > + > > Unnecessary initialization. Ok, will remove. > > +static int tcpm_pps_set_out_volt(struct tcpm_port *port, u16 out_volt) > > +{ > > + unsigned int target_mw; > > + int ret = 0; > > + > Unnecessary initialization. Ditto > > + mutex_lock(&port->swap_lock); > > + mutex_lock(&port->lock); > > + > > + if (!port->pps_data.active) { > > + ret = -EOPNOTSUPP; > > + goto port_unlock; > > + } > > + > > + if (port->state != SNK_READY) { > > + ret = -EAGAIN; > > + goto port_unlock; > > + } > > + > > + if ((out_volt < port->pps_data.min_volt) || > > + (out_volt > port->pps_data.max_volt)) { > > Unnecessary ( ) Ok. > > + ret = -EINVAL; > > + goto port_unlock; > > + } > > + > > + target_mw = (port->pps_data.op_curr * out_volt) / 1000; > > + if (target_mw < port->operating_snk_mw) { > > + ret = -EINVAL; > > + goto port_unlock; > > + } > > + > > + reinit_completion(&port->pps_complete); > > + port->pps_data.out_volt = out_volt; > > + port->pps_status = 0; > > + port->pps_pending = true; > > + tcpm_set_state(port, SNK_NEGOTIATE_PPS_CAPABILITIES, 0); > > + mutex_unlock(&port->lock); > > + > > + if (!wait_for_completion_timeout(&port->pps_complete, > > + msecs_to_jiffies(PD_STATE_MACHINE_TIMEOUT))) > > + ret = -ETIMEDOUT; > > + else > > + ret = port->pps_status; > > + > > + goto swap_unlock; > > + > > +port_unlock: > > + mutex_unlock(&port->lock); > > +swap_unlock: > > + mutex_unlock(&port->swap_lock); > > + > > + return ret; > > +} > > + > > +static int tcpm_pps_activate(struct tcpm_port *port, bool activate) > > +{ > > + int ret = 0; > > + > > + mutex_lock(&port->swap_lock); > > + mutex_lock(&port->lock); > > + > > + if (!port->pps_data.supported) { > > + ret = -EOPNOTSUPP; > > + goto port_unlock; > > + } > > + > > + /* Trying to deactivate PPS when already deactivated so just bail */ > > + if ((!port->pps_data.active) && (!activate)) > > Unnecessary ( ) Actually agree on this one :) ��.n��������+%������w��{.n�����{���)��jg��������ݢj����G�������j:+v���w�m������w�������h�����٥