LinuxLists.cc - [PATCH 1/2] staging: typec: tcpm: Validate source and sink caps

[permalink] [raw]

Subject: [PATCH 2/2] staging: typec: tcpm: Only request matching pdos

At present, TCPM code assumes that local device supports
variable/batt pdos and always selects the pdo with highest
possible power within the board limit. This assumption
might not hold good for all devices. To overcome this,
this patch makes TCPM only accept a source_pdo when there is
a matching sink pdo.

For Fixed pdos: The voltage should match between the
incoming source_cap and the registered snk_pdo
For Variable/Batt pdos: The incoming source_cap voltage
range should fall within the registered snk_pdo's voltage
range.

Also, when the cap_mismatch bit is set, the max_power/current
should be set to the max_current/power of the sink_pdo.
This is according to:

"If the Capability Mismatch bit is set to one
The Maximum Operating Current/Power field may contain a value
larger than the maximum current/power offered in the Source
Capabilities message’s PDO as referenced by the Object position field.
This enables the Sink to indicate that it requires more current/power
than is being offered. If the Sink requires a different voltage this
will be indicated by its Sink Capabilities message.

Signed-off-by: Badhri Jagan Sridharan <[email protected]>
---
drivers/staging/typec/tcpm.c | 130 +++++++++++++++++++++++++++++++++----------
1 file changed, 100 insertions(+), 30 deletions(-)

diff --git a/drivers/staging/typec/tcpm.c b/drivers/staging/typec/tcpm.c
index 58a2c279f7d1..df0986d9f756 100644
--- a/drivers/staging/typec/tcpm.c
+++ b/drivers/staging/typec/tcpm.c
@@ -262,6 +262,9 @@ struct tcpm_port {
unsigned int nr_src_pdo;
u32 snk_pdo[PDO_MAX_OBJECTS];
unsigned int nr_snk_pdo;
+ unsigned int nr_snk_fixed_pdo;
+ unsigned int nr_snk_var_pdo;
+ unsigned int nr_snk_batt_pdo;
u32 snk_vdo[VDO_MAX_OBJECTS];
unsigned int nr_snk_vdo;

@@ -1780,37 +1783,77 @@ static int tcpm_pd_check_request(struct tcpm_port *port)
return 0;
}

-static int tcpm_pd_select_pdo(struct tcpm_port *port)
+static int tcpm_pd_select_pdo(struct tcpm_port *port, int *sink_pdo)
{
- unsigned int i, max_mw = 0, max_mv = 0;
+ unsigned int i, j, max_mw = 0, max_mv = 0;
int ret = -EINVAL;

/*
- * Select the source PDO providing the most power while staying within
- * the board's voltage limits. Prefer PDO providing exp
+ * Select the source PDO providing the most power which has a
+ * matchig sink cap. Prefer PDO providing exp
*/
for (i = 0; i < port->nr_source_caps; i++) {
u32 pdo = port->source_caps[i];
enum pd_pdo_type type = pdo_type(pdo);
- unsigned int mv, ma, mw;
-
- if (type == PDO_TYPE_FIXED)
- mv = pdo_fixed_voltage(pdo);
- else
- mv = pdo_min_voltage(pdo);
-
- if (type == PDO_TYPE_BATT) {
- mw = pdo_max_power(pdo);
- } else {
- ma = min(pdo_max_current(pdo),
- port->max_snk_ma);
- mw = ma * mv / 1000;
+ unsigned int mv = 0, ma = 0, mw = 0, selected = 0;
+
+ if (type == PDO_TYPE_FIXED) {
+ for (j = 0; j < port->nr_snk_fixed_pdo; j++) {
+ if (pdo_fixed_voltage(pdo) ==
+ pdo_fixed_voltage(port->snk_pdo[j])) {
+ mv = pdo_fixed_voltage(pdo);
+ selected = j;
+ break;
+ }
+ }
+ } else if (type == PDO_TYPE_BATT && port->nr_snk_batt_pdo) {
+ for (j = port->nr_snk_fixed_pdo;
+ j < port->nr_snk_fixed_pdo +
+ port->nr_snk_batt_pdo;
+ j++) {
+ if ((pdo_min_voltage(pdo) >=
+ pdo_min_voltage(port->snk_pdo[j])) &&
+ pdo_max_voltage(pdo) <=
+ pdo_max_voltage(port->snk_pdo[j])) {
+ mv = pdo_min_voltage(pdo);
+ selected = j;
+ break;
+ }
+ }
+ } else if (type == PDO_TYPE_VAR && port->nr_snk_var_pdo) {
+ for (j = port->nr_snk_fixed_pdo +
+ port->nr_snk_batt_pdo;
+ j < port->nr_snk_fixed_pdo +
+ port->nr_snk_batt_pdo +
+ port->nr_snk_var_pdo;
+ j++) {
+ if ((pdo_min_voltage(pdo) >=
+ pdo_min_voltage(port->snk_pdo[j])) &&
+ pdo_max_voltage(pdo) <=
+ pdo_max_voltage(port->snk_pdo[j])) {
+ mv = pdo_min_voltage(pdo);
+ selected = j;
+ break;
+ }
+ }
}

+ if (mv != 0) {
+ if (type == PDO_TYPE_BATT) {
+ mw = min(pdo_max_power(pdo),
+ pdo_max_power(port->snk_pdo[selected]
+ ));
+ } else {
+ ma = min(pdo_max_current(pdo),
+ pdo_max_current(
+ port->snk_pdo[selected]));
+ mw = ma * mv / 1000;
+ }
+ }
/* Perfer higher voltages if available */
- if ((mw > max_mw || (mw == max_mw && mv > max_mv)) &&
- mv <= port->max_snk_mv) {
+ if (mw > max_mw || (mw == max_mw && mv > max_mv)) {
ret = i;
+ *sink_pdo = selected;
max_mw = mw;
max_mv = mv;
}
@@ -1824,13 +1867,14 @@ static int tcpm_pd_build_request(struct tcpm_port *port, u32 *rdo)
unsigned int mv, ma, mw, flags;
unsigned int max_ma, max_mw;
enum pd_pdo_type type;
- int index;
- u32 pdo;
+ int index, snk_pdo_index;
+ u32 pdo, matching_snk_pdo;

- index = tcpm_pd_select_pdo(port);
+ index = tcpm_pd_select_pdo(port, &snk_pdo_index);
if (index < 0)
return -EINVAL;
pdo = port->source_caps[index];
+ matching_snk_pdo = port->snk_pdo[snk_pdo_index];
type = pdo_type(pdo);

if (type == PDO_TYPE_FIXED)
@@ -1838,26 +1882,32 @@ static int tcpm_pd_build_request(struct tcpm_port *port, u32 *rdo)
else
mv = pdo_min_voltage(pdo);

- /* Select maximum available current within the board's power limit */
+ /* Select maximum available current within the sink pdo's limit */
if (type == PDO_TYPE_BATT) {
- mw = pdo_max_power(pdo);
- ma = 1000 * min(mw, port->max_snk_mw) / mv;
+ mw = min(pdo_max_power(pdo), pdo_max_power(matching_snk_pdo));
+ ma = 1000 * mw / mv;
} else {
ma = min(pdo_max_current(pdo),
- 1000 * port->max_snk_mw / mv);
+ pdo_max_current(matching_snk_pdo));
+ mw = ma * mv / 1000;
}
- ma = min(ma, port->max_snk_ma);

flags = RDO_USB_COMM | RDO_NO_SUSPEND;

/* Set mismatch bit if offered power is less than operating power */
- mw = ma * mv / 1000;
max_ma = ma;
max_mw = mw;
if (mw < port->operating_snk_mw) {
flags |= RDO_CAP_MISMATCH;
- max_mw = port->operating_snk_mw;
- max_ma = max_mw * 1000 / mv;
+ if (type == PDO_TYPE_BATT) {
+ if (pdo_max_power(matching_snk_pdo) >
+ pdo_max_power(pdo))
+ max_mw = pdo_max_power(matching_snk_pdo);
+ } else {
+ if (pdo_max_current(matching_snk_pdo) >
+ pdo_max_current(pdo))
+ max_ma = pdo_max_current(matching_snk_pdo);
+ }
}

tcpm_log(port, "cc=%d cc1=%d cc2=%d vbus=%d vconn=%s polarity=%d",
@@ -3609,6 +3659,17 @@ int tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo,
}
EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities);

+static int tcpm_get_nr_type_pdos(const u32 *pdo, unsigned int nr_pdo,
+ enum pd_pdo_type type)
+{
+ unsigned int i = 0;
+
+ for (i = 0; i < nr_pdo; i++)
+ if (pdo_type(pdo[i] == type))
+ i++;
+ return i;
+}
+
struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
{
struct tcpm_port *port;
@@ -3655,6 +3716,15 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
tcpc->config->nr_src_pdo);
port->nr_snk_pdo = tcpm_copy_pdos(port->snk_pdo, tcpc->config->snk_pdo,
tcpc->config->nr_snk_pdo);
+ port->nr_snk_fixed_pdo = tcpm_get_nr_type_pdos(port->snk_pdo,
+ port->nr_snk_pdo,
+ PDO_TYPE_FIXED);
+ port->nr_snk_var_pdo = tcpm_get_nr_type_pdos(port->snk_pdo,
+ port->nr_snk_pdo,
+ PDO_TYPE_VAR);
+ port->nr_snk_batt_pdo = tcpm_get_nr_type_pdos(port->snk_pdo,
+ port->nr_snk_pdo,
+ PDO_TYPE_BATT);
port->nr_snk_vdo = tcpm_copy_vdos(port->snk_vdo, tcpc->config->snk_vdo,
tcpc->config->nr_snk_vdo);

--
2.14.1.581.gf28d330327-goog

2017-09-08 09:36:08

[permalink] [raw]

Subject: Re: [PATCH 1/2] staging: typec: tcpm: Validate source and sink caps

On Thu, Sep 07, 2017 at 06:22:13PM -0700, Badhri Jagan Sridharan wrote:
> +static int tcpm_validate_caps(struct tcpm_port *port, const u32 *pdo,
> + unsigned int nr_pdo)
> +{
> + unsigned int i;
> +
> + /* Should at least contain vSafe5v */
> + if (nr_pdo < 1) {
> + tcpm_log_force(port,
> + " err: source/sink caps should atleast have vSafe5V");
> + return -EINVAL;
> + }
> +
> + /* The vSafe5V Fixed Supply Object Shall always be the first object */
> + if (pdo_type(pdo[0]) != PDO_TYPE_FIXED ||
> + pdo_fixed_voltage(pdo[0]) != VSAFE5V) {
> + tcpm_log_force(port,
> + " err: vSafe5V Fixed Supply Object Shall always be the first object");
> + return -EINVAL;
> + }
> +
> + for (i = 1; i < nr_pdo; i++) {
> + if (pdo_type(pdo[i]) < pdo_type(pdo[i - 1])) {
> + tcpm_log_force(port,
> + " err:PDOs should be in the following order: Fixed; Battery; Variable. pdo index:%u"
> + , i);
> + return -EINVAL;
> + } else if (pdo_type(pdo[i]) == pdo_type(pdo[i - 1])) {

The else statement isn't needed. Pull this in one indent level.

> + enum pd_pdo_type type = pdo_type(pdo[i]);
> +
> + switch (type) {
> + /*
> + * The remaining Fixed Supply Objects, if
> + * present, shall be sent in voltage order;
> + * lowest to highest.
> + */
> + case PDO_TYPE_FIXED:
> + if (pdo_fixed_voltage(pdo[i]) <=
> + pdo_fixed_voltage(pdo[i - 1])) {

We can't have 2 fixed voltages which are the same? The <= is < in the
section below and the spec just says lowest to highest for everything.
(I am similar to John Snow in that I know nothing).

> + tcpm_log_force(port,
> + " err: Fixed supply pdos should be in increasing order, pdo index:%u"
> + , i);
> + return -EINVAL;
> + }
> + break;
> + /*
> + * The Battery Supply Objects and Variable
> + * supply, if present shall be sent in Minimum
> + * Voltage order; lowest to highest.
> + */
> + case PDO_TYPE_VAR:
> + case PDO_TYPE_BATT:
> + if (pdo_min_voltage(pdo[i]) <
> + pdo_min_voltage(pdo[i - 1])) {
> + tcpm_log_force(port,
> + " err: Variable supply pdos should be in increasing order, pdo index:%u"
> + , i);
> + return -EINVAL;
> + }
> + break;
> + default:
> + tcpm_log_force(port, " Unknown pdo type");
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> /*
> * PD (data, control) command handling functions
> */
> @@ -1308,6 +1376,9 @@ static void tcpm_pd_data_request(struct tcpm_port *port,
>
> tcpm_log_source_caps(port);
>
> + tcpm_validate_caps(port, port->source_caps,
> + port->nr_source_caps);

It's strange that this isn't checked. We just want to print errors?
Can this ever be true here because it feels like we won't load if it's
invalid. Is this really the right place to check?

> +
> /*
> * This message may be received even if VBUS is not
> * present. This is quite unexpected; see USB PD
> @@ -3475,9 +3546,13 @@ static int tcpm_copy_vdos(u32 *dest_vdo, const u32 *src_vdo,
> return nr_vdo;
> }
>
> -void tcpm_update_source_capabilities(struct tcpm_port *port, const u32 *pdo,
> - unsigned int nr_pdo)
> +int tcpm_update_source_capabilities(struct tcpm_port *port, const u32 *pdo,
> + unsigned int nr_pdo)
> {
> + if (tcpm_validate_caps(port, pdo, nr_pdo)) {
> + tcpm_log_force(port, "Invalid source caps");
> + return -EINVAL;
> + }
> mutex_lock(&port->lock);
> port->nr_src_pdo = tcpm_copy_pdos(port->src_pdo, pdo, nr_pdo);
> switch (port->state) {
> @@ -3497,16 +3572,21 @@ void tcpm_update_source_capabilities(struct tcpm_port *port, const u32 *pdo,
> break;
> }
> mutex_unlock(&port->lock);
> + return 0;
> }
> EXPORT_SYMBOL_GPL(tcpm_update_source_capabilities);
>
> -void tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo,
> - unsigned int nr_pdo,
> - unsigned int max_snk_mv,
> - unsigned int max_snk_ma,
> - unsigned int max_snk_mw,
> - unsigned int operating_snk_mw)
> +int tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo,
> + unsigned int nr_pdo,
> + unsigned int max_snk_mv,
> + unsigned int max_snk_ma,
> + unsigned int max_snk_mw,
> + unsigned int operating_snk_mw)
> {
> + if (tcpm_validate_caps(port, pdo, nr_pdo)) {
> + tcpm_log_force(port, "Invalid source caps");
> + return -EINVAL;
> + }
> mutex_lock(&port->lock);
> port->nr_snk_pdo = tcpm_copy_pdos(port->snk_pdo, pdo, nr_pdo);
> port->max_snk_mv = max_snk_mv;
> @@ -3525,6 +3605,7 @@ void tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo,
> break;
> }
> mutex_unlock(&port->lock);
> + return 0;
> }
> EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities);
>
> @@ -3560,7 +3641,16 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>
> init_completion(&port->tx_complete);
> init_completion(&port->swap_complete);
> + tcpm_debugfs_init(port);

Why are we moving debugfs init for this patch? I'm too lazy to figure
out how this relates to the patch. :/

>
> + if (tcpm_validate_caps(port, tcpc->config->src_pdo,
> + tcpc->config->nr_src_pdo) ||
> + tcpm_validate_caps(port,
> + tcpc->config->snk_pdo,
> + tcpc->config->nr_snk_pdo)) {

This is indented too far. It should be:

if (tcpm_validate_caps(port, tcpc->config->src_pdo,
tcpc->config->nr_src_pdo) ||
tcpm_validate_caps(port, tcpc->config->snk_pdo,
tcpc->config->nr_snk_pdo)) {

> + err = -EINVAL;
> + goto out_destroy_wq;
> + }
> port->nr_src_pdo = tcpm_copy_pdos(port->src_pdo, tcpc->config->src_pdo,
> tcpc->config->nr_src_pdo);
> port->nr_snk_pdo = tcpm_copy_pdos(port->snk_pdo, tcpc->config->snk_pdo,
> @@ -3620,7 +3710,6 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
> }
> }
>
> - tcpm_debugfs_init(port);
> mutex_lock(&port->lock);
> tcpm_init(port);
> mutex_unlock(&port->lock);

regards,
dan carpenter

2017-09-08 09:37:57

[permalink] [raw]

Subject: Re: [PATCH 2/2] staging: typec: tcpm: Only request matching pdos

On Thu, Sep 07, 2017 at 06:22:14PM -0700, Badhri Jagan Sridharan wrote:
> diff --git a/drivers/staging/typec/tcpm.c b/drivers/staging/typec/tcpm.c
> index 58a2c279f7d1..df0986d9f756 100644
> --- a/drivers/staging/typec/tcpm.c
> +++ b/drivers/staging/typec/tcpm.c
> @@ -262,6 +262,9 @@ struct tcpm_port {
> unsigned int nr_src_pdo;
> u32 snk_pdo[PDO_MAX_OBJECTS];
> unsigned int nr_snk_pdo;
> + unsigned int nr_snk_fixed_pdo;
> + unsigned int nr_snk_var_pdo;
> + unsigned int nr_snk_batt_pdo;

These names are too long. The extra words obscure the parts of the
variable names which have information. It hurts readability. Do this:

unsigned int nr_fixed; /* number of fixed sink PDOs */
unsigned int nr_var; /* number of variable sink PDOs */
unsigned int nr_batt; /* number of battery sink PDOs */

> u32 snk_vdo[VDO_MAX_OBJECTS];
> unsigned int nr_snk_vdo;
>
> @@ -1780,37 +1783,77 @@ static int tcpm_pd_check_request(struct tcpm_port *port)
> return 0;
> }
>
> -static int tcpm_pd_select_pdo(struct tcpm_port *port)
> +static int tcpm_pd_select_pdo(struct tcpm_port *port, int *sink_pdo)
> {
> - unsigned int i, max_mw = 0, max_mv = 0;
> + unsigned int i, j, max_mw = 0, max_mv = 0;
> int ret = -EINVAL;
>
> /*
> - * Select the source PDO providing the most power while staying within
> - * the board's voltage limits. Prefer PDO providing exp
> + * Select the source PDO providing the most power which has a
> + * matchig sink cap. Prefer PDO providing exp
^^^^^^^ ^^^^^^^^^^^^^
"matching". What does "providing exp" mean?

> */
> for (i = 0; i < port->nr_source_caps; i++) {
> u32 pdo = port->source_caps[i];
> enum pd_pdo_type type = pdo_type(pdo);
> - unsigned int mv, ma, mw;
> -
> - if (type == PDO_TYPE_FIXED)
> - mv = pdo_fixed_voltage(pdo);
> - else
> - mv = pdo_min_voltage(pdo);
> -
> - if (type == PDO_TYPE_BATT) {
> - mw = pdo_max_power(pdo);
> - } else {
> - ma = min(pdo_max_current(pdo),
> - port->max_snk_ma);
> - mw = ma * mv / 1000;
> + unsigned int mv = 0, ma = 0, mw = 0, selected = 0;
> +
> + if (type == PDO_TYPE_FIXED) {
> + for (j = 0; j < port->nr_snk_fixed_pdo; j++) {
> + if (pdo_fixed_voltage(pdo) ==
> + pdo_fixed_voltage(port->snk_pdo[j])) {
> + mv = pdo_fixed_voltage(pdo);
> + selected = j;
> + break;
> + }
> + }
> + } else if (type == PDO_TYPE_BATT && port->nr_snk_batt_pdo) {

The test for nr_snk_batt_pdo isn't required. If it's zero then the for
loop is just a no-op. Remove it.

> + for (j = port->nr_snk_fixed_pdo;
> + j < port->nr_snk_fixed_pdo +
> + port->nr_snk_batt_pdo;

This should be aligned like this:

for (j = port->nr_snk_fixed_pdo;
j < port->nr_snk_fixed_pdo +
port->nr_snk_batt_pdo;

> + j++) {
> + if ((pdo_min_voltage(pdo) >=
> + pdo_min_voltage(port->snk_pdo[j])) &&
> + pdo_max_voltage(pdo) <=
> + pdo_max_voltage(port->snk_pdo[j])) {

No need for the extra parentheses around the first part of the &&. The
condition isn't indented right either because the last two lines are
indented one space more than they should be. Just do:

if (pdo_min_voltage(pdo) >=
pdo_min_voltage(port->snk_pdo[j]) &&
pdo_max_voltage(pdo) <=
pdo_max_voltage(port->snk_pdo[j])) {

> + mv = pdo_min_voltage(pdo);
> + selected = j;
> + break;

We always select the first valid voltage?

> + }
> + }
> + } else if (type == PDO_TYPE_VAR && port->nr_snk_var_pdo) {
> + for (j = port->nr_snk_fixed_pdo +
> + port->nr_snk_batt_pdo;
> + j < port->nr_snk_fixed_pdo +
> + port->nr_snk_batt_pdo +
> + port->nr_snk_var_pdo;
> + j++) {
> + if ((pdo_min_voltage(pdo) >=
> + pdo_min_voltage(port->snk_pdo[j])) &&
> + pdo_max_voltage(pdo) <=
> + pdo_max_voltage(port->snk_pdo[j])) {
> + mv = pdo_min_voltage(pdo);
> + selected = j;
> + break;
> + }
> + }

Same stuff.

> }
>
> + if (mv != 0) {

Flip this condition around.

if (mv == 0)
continue;

> + if (type == PDO_TYPE_BATT) {
> + mw = min(pdo_max_power(pdo),
> + pdo_max_power(port->snk_pdo[selected]
> + ));
> + } else {
> + ma = min(pdo_max_current(pdo),
> + pdo_max_current(
> + port->snk_pdo[selected]));
> + mw = ma * mv / 1000;
> + }

Then pull this code in one indent level and it looks nicer.

> + }
> /* Perfer higher voltages if available */
> - if ((mw > max_mw || (mw == max_mw && mv > max_mv)) &&
> - mv <= port->max_snk_mv) {
> + if (mw > max_mw || (mw == max_mw && mv > max_mv)) {
> ret = i;
> + *sink_pdo = selected;
> max_mw = mw;
> max_mv = mv;
> }

[ snip ]

> @@ -3609,6 +3659,17 @@ int tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo,
> }
> EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities);
>
> +static int tcpm_get_nr_type_pdos(const u32 *pdo, unsigned int nr_pdo,

This function name is awkward. It's quite long and that means we keep
bumping into the 80 character limit. Often times "get_" functions need
to be followed by a "put_" but so the name is a little misleading. It's
static so we don't really need the tcpm_ prefix. Just call it
nr_type_pdos().

> + enum pd_pdo_type type)
> +{
> + unsigned int i = 0;
> +
> + for (i = 0; i < nr_pdo; i++)
> + if (pdo_type(pdo[i] == type))

Parentheses are in the wrong place so this is always false. It should
say:
if (pdo_type(pdo[i]) == type)

> + i++;

The "i" variable is the iterator. We should be saying "count++"; This
function always returns nr_pdo. Write it like this:

static int nr_type_pdos(const u32 *pdo, unsigned int nr_pdo,
enum pd_pdo_type type)
{
int count = 0;
int i;

for (i = 0; i < nr_pdo; i++) {
if (pdo_type(pdo[i]) == type)
count++;
}
return count;
}

regards,
dan carpenter

2017-09-08 09:46:03

[permalink] [raw]

Subject: Re: [PATCH 1/2] staging: typec: tcpm: Validate source and sink caps

On Thu, Sep 07, 2017 at 06:22:13PM -0700, Badhri Jagan Sridharan wrote:
> The source and sink caps should follow the following rules.
> This patch validates whether the src_caps/snk_caps adheres
> to it.
>
> 6.4.1 Capabilities Message
> A Capabilities message (Source Capabilities message or Sink
> Capabilities message) shall have at least one Power
> Data Object for vSafe5V. The Capabilities message shall also
> contain the sending Port’s information followed by up to
> 6 additional Power Data Objects. Power Data Objects in a
> Capabilities message shall be sent in the following order:
>
> 1. The vSafe5V Fixed Supply Object shall always be the first object.
> 2. The remaining Fixed Supply Objects, if present, shall be sent
> in voltage order; lowest to highest.
> 3. The Battery Supply Objects, if present shall be sent in Minimum
> Voltage order; lowest to highest.
> 4. The Variable Supply (non-battery) Objects, if present, shall be
> sent in Minimum Voltage order; lowest to highest.
>
> Errors in source/sink_caps of the local port will prevent
> the port registration. Whereas, errors in source caps of partner
> device would only log them.
>
> Signed-off-by: Badhri Jagan Sridharan <[email protected]>
> ---
> drivers/staging/typec/pd.h | 2 +
> drivers/staging/typec/tcpm.c | 107 +++++++++++++++++++++++++++++++++++++++----
> drivers/staging/typec/tcpm.h | 16 +++----
> 3 files changed, 108 insertions(+), 17 deletions(-)

Before you add more stuff to this driver, what is needed to get it out
of staging? That would be more useful to do now, right?

thanks,

greg k-h

2017-09-08 17:28:10

[permalink] [raw]

Subject: Re: [PATCH 1/2] staging: typec: tcpm: Validate source and sink caps

Thanks for the comments. Replies inline.

On Fri, Sep 8, 2017 at 2:34 AM, Dan Carpenter <[email protected]> wrote:
> On Thu, Sep 07, 2017 at 06:22:13PM -0700, Badhri Jagan Sridharan wrote:
>> +static int tcpm_validate_caps(struct tcpm_port *port, const u32 *pdo,
>> + unsigned int nr_pdo)
>> +{
>> + unsigned int i;
>> +
>> + /* Should at least contain vSafe5v */
>> + if (nr_pdo < 1) {
>> + tcpm_log_force(port,
>> + " err: source/sink caps should atleast have vSafe5V");
>> + return -EINVAL;
>> + }
>> +
>> + /* The vSafe5V Fixed Supply Object Shall always be the first object */
>> + if (pdo_type(pdo[0]) != PDO_TYPE_FIXED ||
>> + pdo_fixed_voltage(pdo[0]) != VSAFE5V) {
>> + tcpm_log_force(port,
>> + " err: vSafe5V Fixed Supply Object Shall always be the first object");
>> + return -EINVAL;
>> + }
>> +
>> + for (i = 1; i < nr_pdo; i++) {
>> + if (pdo_type(pdo[i]) < pdo_type(pdo[i - 1])) {
>> + tcpm_log_force(port,
>> + " err:PDOs should be in the following order: Fixed; Battery; Variable. pdo index:%u"
>> + , i);
>> + return -EINVAL;
>> + } else if (pdo_type(pdo[i]) == pdo_type(pdo[i - 1])) {
>
> The else statement isn't needed. Pull this in one indent level.

Just to clarify, you are suggesting,
"if (pdo_type(pdo[i]) == pdo_type(pdo[i - 1]))"
instead of "else if (pdo_type(pdo[i]) == pdo_type(pdo[i - 1]))"
right ?

>
>> + enum pd_pdo_type type = pdo_type(pdo[i]);
>> +
>> + switch (type) {
>> + /*
>> + * The remaining Fixed Supply Objects, if
>> + * present, shall be sent in voltage order;
>> + * lowest to highest.
>> + */
>> + case PDO_TYPE_FIXED:
>> + if (pdo_fixed_voltage(pdo[i]) <=
>> + pdo_fixed_voltage(pdo[i - 1])) {
>
> We can't have 2 fixed voltages which are the same? The <= is < in the
> section below and the spec just says lowest to highest for everything.
> (I am similar to John Snow in that I know nothing).

The PD3.0 spec says the following:
A Source Shall Not offer multiple Power Data Objects of
the same type (fixed, variable, Battery) and the same voltage
but Shall instead offer one Power Data Object with the
highest available current for that Source capability and voltage.

I forgot to add the duplicate check(same min/max voltage for VAR/BATT.
Will add it in the next version.

>
>> + tcpm_log_force(port,
>> + " err: Fixed supply pdos should be in increasing order, pdo index:%u"
>> + , i);
>> + return -EINVAL;
>> + }
>> + break;
>> + /*
>> + * The Battery Supply Objects and Variable
>> + * supply, if present shall be sent in Minimum
>> + * Voltage order; lowest to highest.
>> + */
>> + case PDO_TYPE_VAR:
>> + case PDO_TYPE_BATT:
>> + if (pdo_min_voltage(pdo[i]) <
>> + pdo_min_voltage(pdo[i - 1])) {
>> + tcpm_log_force(port,
>> + " err: Variable supply pdos should be in increasing order, pdo index:%u"
>> + , i);
>> + return -EINVAL;
>> + }
>> + break;
>> + default:
>> + tcpm_log_force(port, " Unknown pdo type");
>> + }
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> /*
>> * PD (data, control) command handling functions
>> */
>> @@ -1308,6 +1376,9 @@ static void tcpm_pd_data_request(struct tcpm_port *port,
>>
>> tcpm_log_source_caps(port);
>>
>> + tcpm_validate_caps(port, port->source_caps,
>> + port->nr_source_caps);
>
> It's strange that this isn't checked. We just want to print errors?
> Can this ever be true here because it feels like we won't load if it's
> invalid. Is this really the right place to check?

The source_caps here is from the port partner which might be running
a different codebase. Most of the chargers run their custom
code which implies that we should atleast be aware if any of the charger
does not comply to the spec. The TCPM code would still manage to
negotiate the contract without the strict ordering from the charger
side. So, just printing errors should be suffice. Not negotiating a
contract might be little too extreme.

>
>> +
>> /*
>> * This message may be received even if VBUS is not
>> * present. This is quite unexpected; see USB PD
>> @@ -3475,9 +3546,13 @@ static int tcpm_copy_vdos(u32 *dest_vdo, const u32 *src_vdo,
>> return nr_vdo;
>> }
>>
>> -void tcpm_update_source_capabilities(struct tcpm_port *port, const u32 *pdo,
>> - unsigned int nr_pdo)
>> +int tcpm_update_source_capabilities(struct tcpm_port *port, const u32 *pdo,
>> + unsigned int nr_pdo)
>> {
>> + if (tcpm_validate_caps(port, pdo, nr_pdo)) {
>> + tcpm_log_force(port, "Invalid source caps");
>> + return -EINVAL;
>> + }
>> mutex_lock(&port->lock);
>> port->nr_src_pdo = tcpm_copy_pdos(port->src_pdo, pdo, nr_pdo);
>> switch (port->state) {
>> @@ -3497,16 +3572,21 @@ void tcpm_update_source_capabilities(struct tcpm_port *port, const u32 *pdo,
>> break;
>> }
>> mutex_unlock(&port->lock);
>> + return 0;
>> }
>> EXPORT_SYMBOL_GPL(tcpm_update_source_capabilities);
>>
>> -void tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo,
>> - unsigned int nr_pdo,
>> - unsigned int max_snk_mv,
>> - unsigned int max_snk_ma,
>> - unsigned int max_snk_mw,
>> - unsigned int operating_snk_mw)
>> +int tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo,
>> + unsigned int nr_pdo,
>> + unsigned int max_snk_mv,
>> + unsigned int max_snk_ma,
>> + unsigned int max_snk_mw,
>> + unsigned int operating_snk_mw)
>> {
>> + if (tcpm_validate_caps(port, pdo, nr_pdo)) {
>> + tcpm_log_force(port, "Invalid source caps");
>> + return -EINVAL;
>> + }
>> mutex_lock(&port->lock);
>> port->nr_snk_pdo = tcpm_copy_pdos(port->snk_pdo, pdo, nr_pdo);
>> port->max_snk_mv = max_snk_mv;
>> @@ -3525,6 +3605,7 @@ void tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo,
>> break;
>> }
>> mutex_unlock(&port->lock);
>> + return 0;
>> }
>> EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities);
>>
>> @@ -3560,7 +3641,16 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>>
>> init_completion(&port->tx_complete);
>> init_completion(&port->swap_complete);
>> + tcpm_debugfs_init(port);
>
> Why are we moving debugfs init for this patch? I'm too lazy to figure
> out how this relates to the patch. :/

The tcpm_validate_caps prints error messages into the logbuffer.
There is already an error in the TOT code, where the tcpm_log(line 3613) is
called before tcpm_debugfs_init is called. This would make the kernel
come down due to null pointer deference if the execution takes this
path as logbuffer_lock is not yet initialized. I am also not removing the
debugfs dir if tcpm_register_port fails to help retrieve
the error messages in the logbuffer. I could dump the error message into
kernel dmesg but didnt take that path to keep it in the same logbuffer.
>
>>
>> + if (tcpm_validate_caps(port, tcpc->config->src_pdo,
>> + tcpc->config->nr_src_pdo) ||
>> + tcpm_validate_caps(port,
>> + tcpc->config->snk_pdo,
>> + tcpc->config->nr_snk_pdo)) {
>
> This is indented too far. It should be:
>
> if (tcpm_validate_caps(port, tcpc->config->src_pdo,
> tcpc->config->nr_src_pdo) ||
> tcpm_validate_caps(port, tcpc->config->snk_pdo,
> tcpc->config->nr_snk_pdo)) {
>
>
>
>
>> + err = -EINVAL;
>> + goto out_destroy_wq;
>> + }
>> port->nr_src_pdo = tcpm_copy_pdos(port->src_pdo, tcpc->config->src_pdo,
>> tcpc->config->nr_src_pdo);
>> port->nr_snk_pdo = tcpm_copy_pdos(port->snk_pdo, tcpc->config->snk_pdo,
>> @@ -3620,7 +3710,6 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>> }
>> }
>>
>> - tcpm_debugfs_init(port);
>> mutex_lock(&port->lock);
>> tcpm_init(port);
>> mutex_unlock(&port->lock);
>
> regards,
> dan carpenter
>

2017-09-08 17:30:35

[permalink] [raw]

Subject: Re: [PATCH 1/2] staging: typec: tcpm: Validate source and sink caps

On Fri, Sep 8, 2017 at 2:45 AM, Greg Kroah-Hartman
<[email protected]> wrote:
> On Thu, Sep 07, 2017 at 06:22:13PM -0700, Badhri Jagan Sridharan wrote:
>> The source and sink caps should follow the following rules.
>> This patch validates whether the src_caps/snk_caps adheres
>> to it.
>>
>> 6.4.1 Capabilities Message
>> A Capabilities message (Source Capabilities message or Sink
>> Capabilities message) shall have at least one Power
>> Data Object for vSafe5V. The Capabilities message shall also
>> contain the sending Port’s information followed by up to
>> 6 additional Power Data Objects. Power Data Objects in a
>> Capabilities message shall be sent in the following order:
>>
>> 1. The vSafe5V Fixed Supply Object shall always be the first object.
>> 2. The remaining Fixed Supply Objects, if present, shall be sent
>> in voltage order; lowest to highest.
>> 3. The Battery Supply Objects, if present shall be sent in Minimum
>> Voltage order; lowest to highest.
>> 4. The Variable Supply (non-battery) Objects, if present, shall be
>> sent in Minimum Voltage order; lowest to highest.
>>
>> Errors in source/sink_caps of the local port will prevent
>> the port registration. Whereas, errors in source caps of partner
>> device would only log them.
>>
>> Signed-off-by: Badhri Jagan Sridharan <[email protected]>
>> ---
>> drivers/staging/typec/pd.h | 2 +
>> drivers/staging/typec/tcpm.c | 107 +++++++++++++++++++++++++++++++++++++++----
>> drivers/staging/typec/tcpm.h | 16 +++----
>> 3 files changed, 108 insertions(+), 17 deletions(-)
>
> Before you add more stuff to this driver, what is needed to get it out
> of staging? That would be more useful to do now, right?

Actually not adding more features here. There was a bug where
the phone wasnt charging at the highest possible power output,
came up with the these patches while debugging that issue.

>
> thanks,
>
> greg k-h

2017-09-08 17:44:24

[permalink] [raw]

Subject: Re: [PATCH 2/2] staging: typec: tcpm: Only request matching pdos

On Fri, Sep 8, 2017 at 2:36 AM, Dan Carpenter <[email protected]> wrote:
> On Thu, Sep 07, 2017 at 06:22:14PM -0700, Badhri Jagan Sridharan wrote:
>> diff --git a/drivers/staging/typec/tcpm.c b/drivers/staging/typec/tcpm.c
>> index 58a2c279f7d1..df0986d9f756 100644
>> --- a/drivers/staging/typec/tcpm.c
>> +++ b/drivers/staging/typec/tcpm.c
>> @@ -262,6 +262,9 @@ struct tcpm_port {
>> unsigned int nr_src_pdo;
>> u32 snk_pdo[PDO_MAX_OBJECTS];
>> unsigned int nr_snk_pdo;
>> + unsigned int nr_snk_fixed_pdo;
>> + unsigned int nr_snk_var_pdo;
>> + unsigned int nr_snk_batt_pdo;
>
> These names are too long. The extra words obscure the parts of the
> variable names which have information. It hurts readability. Do this:
>
> unsigned int nr_fixed; /* number of fixed sink PDOs */
> unsigned int nr_var; /* number of variable sink PDOs */
> unsigned int nr_batt; /* number of battery sink PDOs */
>
>> u32 snk_vdo[VDO_MAX_OBJECTS];
>> unsigned int nr_snk_vdo;
>>
>> @@ -1780,37 +1783,77 @@ static int tcpm_pd_check_request(struct tcpm_port *port)
>> return 0;
>> }
>>
>> -static int tcpm_pd_select_pdo(struct tcpm_port *port)
>> +static int tcpm_pd_select_pdo(struct tcpm_port *port, int *sink_pdo)
>> {
>> - unsigned int i, max_mw = 0, max_mv = 0;
>> + unsigned int i, j, max_mw = 0, max_mv = 0;
>> int ret = -EINVAL;
>>
>> /*
>> - * Select the source PDO providing the most power while staying within
>> - * the board's voltage limits. Prefer PDO providing exp
>> + * Select the source PDO providing the most power which has a
>> + * matchig sink cap. Prefer PDO providing exp
> ^^^^^^^ ^^^^^^^^^^^^^
> "matching". What does "providing exp" mean?

Actually that was moved forward from the existing code.
So not sure what that means ? I can remove it, if needed.

>
>> */
>> for (i = 0; i < port->nr_source_caps; i++) {
>> u32 pdo = port->source_caps[i];
>> enum pd_pdo_type type = pdo_type(pdo);
>> - unsigned int mv, ma, mw;
>> -
>> - if (type == PDO_TYPE_FIXED)
>> - mv = pdo_fixed_voltage(pdo);
>> - else
>> - mv = pdo_min_voltage(pdo);
>> -
>> - if (type == PDO_TYPE_BATT) {
>> - mw = pdo_max_power(pdo);
>> - } else {
>> - ma = min(pdo_max_current(pdo),
>> - port->max_snk_ma);
>> - mw = ma * mv / 1000;
>> + unsigned int mv = 0, ma = 0, mw = 0, selected = 0;
>> +
>> + if (type == PDO_TYPE_FIXED) {
>> + for (j = 0; j < port->nr_snk_fixed_pdo; j++) {
>> + if (pdo_fixed_voltage(pdo) ==
>> + pdo_fixed_voltage(port->snk_pdo[j])) {
>> + mv = pdo_fixed_voltage(pdo);
>> + selected = j;
>> + break;
>> + }
>> + }
>> + } else if (type == PDO_TYPE_BATT && port->nr_snk_batt_pdo) {
>
> The test for nr_snk_batt_pdo isn't required. If it's zero then the for
> loop is just a no-op. Remove it.
>
>> + for (j = port->nr_snk_fixed_pdo;
>> + j < port->nr_snk_fixed_pdo +
>> + port->nr_snk_batt_pdo;
>
> This should be aligned like this:
>
> for (j = port->nr_snk_fixed_pdo;
> j < port->nr_snk_fixed_pdo +
> port->nr_snk_batt_pdo;
>
>> + j++) {
>> + if ((pdo_min_voltage(pdo) >=
>> + pdo_min_voltage(port->snk_pdo[j])) &&
>> + pdo_max_voltage(pdo) <=
>> + pdo_max_voltage(port->snk_pdo[j])) {
>
> No need for the extra parentheses around the first part of the &&. The
> condition isn't indented right either because the last two lines are
> indented one space more than they should be. Just do:
>
> if (pdo_min_voltage(pdo) >=
> pdo_min_voltage(port->snk_pdo[j]) &&
> pdo_max_voltage(pdo) <=
> pdo_max_voltage(port->snk_pdo[j])) {
>
>
>> + mv = pdo_min_voltage(pdo);
>> + selected = j;
>> + break;
>
> We always select the first valid voltage?

Good catch ! Should be evaluating against all matching sink_caps
not just the first one which matches. Will make amends in the next
version.

>
>> + }
>> + }
>> + } else if (type == PDO_TYPE_VAR && port->nr_snk_var_pdo) {
>> + for (j = port->nr_snk_fixed_pdo +
>> + port->nr_snk_batt_pdo;
>> + j < port->nr_snk_fixed_pdo +
>> + port->nr_snk_batt_pdo +
>> + port->nr_snk_var_pdo;
>> + j++) {
>> + if ((pdo_min_voltage(pdo) >=
>> + pdo_min_voltage(port->snk_pdo[j])) &&
>> + pdo_max_voltage(pdo) <=
>> + pdo_max_voltage(port->snk_pdo[j])) {
>> + mv = pdo_min_voltage(pdo);
>> + selected = j;
>> + break;
>> + }
>> + }
>
> Same stuff.
>
>> }
>>
>> + if (mv != 0) {
>
> Flip this condition around.
>
> if (mv == 0)
> continue;
>
>> + if (type == PDO_TYPE_BATT) {
>> + mw = min(pdo_max_power(pdo),
>> + pdo_max_power(port->snk_pdo[selected]
>> + ));
>> + } else {
>> + ma = min(pdo_max_current(pdo),
>> + pdo_max_current(
>> + port->snk_pdo[selected]));
>> + mw = ma * mv / 1000;
>> + }
>
> Then pull this code in one indent level and it looks nicer.
>
>> + }
>> /* Perfer higher voltages if available */
>> - if ((mw > max_mw || (mw == max_mw && mv > max_mv)) &&
>> - mv <= port->max_snk_mv) {
>> + if (mw > max_mw || (mw == max_mw && mv > max_mv)) {
>> ret = i;
>> + *sink_pdo = selected;
>> max_mw = mw;
>> max_mv = mv;
>> }
>
> [ snip ]
>
>> @@ -3609,6 +3659,17 @@ int tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo,
>> }
>> EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities);
>>
>> +static int tcpm_get_nr_type_pdos(const u32 *pdo, unsigned int nr_pdo,
>
> This function name is awkward. It's quite long and that means we keep
> bumping into the 80 character limit. Often times "get_" functions need
> to be followed by a "put_" but so the name is a little misleading. It's
> static so we don't really need the tcpm_ prefix. Just call it
> nr_type_pdos().
>
>> + enum pd_pdo_type type)
>> +{
>> + unsigned int i = 0;
>> +
>> + for (i = 0; i < nr_pdo; i++)
>> + if (pdo_type(pdo[i] == type))
>
> Parentheses are in the wrong place so this is always false. It should
> say:
> if (pdo_type(pdo[i]) == type)
>
>> + i++;
>
> The "i" variable is the iterator. We should be saying "count++"; This
> function always returns nr_pdo. Write it like this:
>
> static int nr_type_pdos(const u32 *pdo, unsigned int nr_pdo,
> enum pd_pdo_type type)
> {
> int count = 0;
> int i;
>
> for (i = 0; i < nr_pdo; i++) {
> if (pdo_type(pdo[i]) == type)
> count++;
> }
> return count;
> }

Sure. Will make the about amends and all other readability changes
in the next version.

>
> regards,
> dan carpenter
>

2017-09-08 19:13:46

[permalink] [raw]

Subject: Re: [PATCH 1/2] staging: typec: tcpm: Validate source and sink caps

On Fri, Sep 08, 2017 at 10:29:52AM -0700, Badhri Jagan Sridharan wrote:
> On Fri, Sep 8, 2017 at 2:45 AM, Greg Kroah-Hartman
> <[email protected]> wrote:
> > On Thu, Sep 07, 2017 at 06:22:13PM -0700, Badhri Jagan Sridharan wrote:
> >> The source and sink caps should follow the following rules.
> >> This patch validates whether the src_caps/snk_caps adheres
> >> to it.
> >>
> >> 6.4.1 Capabilities Message
> >> A Capabilities message (Source Capabilities message or Sink
> >> Capabilities message) shall have at least one Power
> >> Data Object for vSafe5V. The Capabilities message shall also
> >> contain the sending Port’s information followed by up to
> >> 6 additional Power Data Objects. Power Data Objects in a
> >> Capabilities message shall be sent in the following order:
> >>
> >> 1. The vSafe5V Fixed Supply Object shall always be the first object.
> >> 2. The remaining Fixed Supply Objects, if present, shall be sent
> >> in voltage order; lowest to highest.
> >> 3. The Battery Supply Objects, if present shall be sent in Minimum
> >> Voltage order; lowest to highest.
> >> 4. The Variable Supply (non-battery) Objects, if present, shall be
> >> sent in Minimum Voltage order; lowest to highest.
> >>
> >> Errors in source/sink_caps of the local port will prevent
> >> the port registration. Whereas, errors in source caps of partner
> >> device would only log them.
> >>
> >> Signed-off-by: Badhri Jagan Sridharan <[email protected]>
> >> ---
> >> drivers/staging/typec/pd.h | 2 +
> >> drivers/staging/typec/tcpm.c | 107 +++++++++++++++++++++++++++++++++++++++----
> >> drivers/staging/typec/tcpm.h | 16 +++----
> >> 3 files changed, 108 insertions(+), 17 deletions(-)
> >
> > Before you add more stuff to this driver, what is needed to get it out
> > of staging? That would be more useful to do now, right?
>
> Actually not adding more features here. There was a bug where
> the phone wasnt charging at the highest possible power output,
> came up with the these patches while debugging that issue.

Ok, but again, when is this going to get out of staging?

2017-09-08 21:56:17

[permalink] [raw]

Subject: Re: [PATCH 1/2] staging: typec: tcpm: Validate source and sink caps

On Fri, Sep 08, 2017 at 10:27:27AM -0700, Badhri Jagan Sridharan wrote:
> >> +
> >> + for (i = 1; i < nr_pdo; i++) {
> >> + if (pdo_type(pdo[i]) < pdo_type(pdo[i - 1])) {
> >> + tcpm_log_force(port,
> >> + " err:PDOs should be in the following order: Fixed; Battery; Variable. pdo index:%u"
> >> + , i);
> >> + return -EINVAL;
> >> + } else if (pdo_type(pdo[i]) == pdo_type(pdo[i - 1])) {
> >
> > The else statement isn't needed. Pull this in one indent level.
>
> Just to clarify, you are suggesting,
> "if (pdo_type(pdo[i]) == pdo_type(pdo[i - 1]))"
> instead of "else if (pdo_type(pdo[i]) == pdo_type(pdo[i - 1]))"
> right ?
>

Sorry. My bad. We can't pull it in an indent level.

Thanks for answering the rest of my questions as well. It's clear to
me now.

regards,
dan carpenter

2017-09-09 08:49:53

[permalink] [raw]

Subject: Re: [PATCH 2/2] staging: typec: tcpm: Only request matching pdos

On Fri, Sep 08, 2017 at 10:43:40AM -0700, Badhri Jagan Sridharan wrote:
> >> /*
> >> - * Select the source PDO providing the most power while staying within
> >> - * the board's voltage limits. Prefer PDO providing exp
> >> + * Select the source PDO providing the most power which has a
> >> + * matchig sink cap. Prefer PDO providing exp
> > ^^^^^^^ ^^^^^^^^^^^^^
> > "matching". What does "providing exp" mean?
>
> Actually that was moved forward from the existing code.
> So not sure what that means ? I can remove it, if needed.
>

If I can't understand it, it probably means that I don't have any
background knowledge, but if you can't understand it then probably no
one can. :P Feel free to delete.

regards,
dan carpenter

2017-09-09 14:01:14

[permalink] [raw]

Subject: Re: [PATCH 2/2] staging: typec: tcpm: Only request matching pdos

On Sat, Sep 09, 2017 at 11:48:48AM +0300, Dan Carpenter wrote:
> On Fri, Sep 08, 2017 at 10:43:40AM -0700, Badhri Jagan Sridharan wrote:
> > >> /*
> > >> - * Select the source PDO providing the most power while staying within
> > >> - * the board's voltage limits. Prefer PDO providing exp
> > >> + * Select the source PDO providing the most power which has a
> > >> + * matchig sink cap. Prefer PDO providing exp
> > > ^^^^^^^ ^^^^^^^^^^^^^
> > > "matching". What does "providing exp" mean?
> >
> > Actually that was moved forward from the existing code.
> > So not sure what that means ? I can remove it, if needed.
> >
>
> If I can't understand it, it probably means that I don't have any
> background knowledge, but if you can't understand it then probably no
> one can. :P Feel free to delete.
>
Agreed. I don't recall what it means either ;-). Most likely something
got lost.

Guenter

2017-09-09 14:03:13

[permalink] [raw]

Subject: Re: [PATCH 1/2] staging: typec: tcpm: Validate source and sink caps

On Fri, Sep 08, 2017 at 09:13:25PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Sep 08, 2017 at 10:29:52AM -0700, Badhri Jagan Sridharan wrote:
> > On Fri, Sep 8, 2017 at 2:45 AM, Greg Kroah-Hartman
> > <[email protected]> wrote:
> > > On Thu, Sep 07, 2017 at 06:22:13PM -0700, Badhri Jagan Sridharan wrote:
> > >> The source and sink caps should follow the following rules.
> > >> This patch validates whether the src_caps/snk_caps adheres
> > >> to it.
> > >>
> > >> 6.4.1 Capabilities Message
> > >> A Capabilities message (Source Capabilities message or Sink
> > >> Capabilities message) shall have at least one Power
> > >> Data Object for vSafe5V. The Capabilities message shall also
> > >> contain the sending Port’s information followed by up to
> > >> 6 additional Power Data Objects. Power Data Objects in a
> > >> Capabilities message shall be sent in the following order:
> > >>
> > >> 1. The vSafe5V Fixed Supply Object shall always be the first object.
> > >> 2. The remaining Fixed Supply Objects, if present, shall be sent
> > >> in voltage order; lowest to highest.
> > >> 3. The Battery Supply Objects, if present shall be sent in Minimum
> > >> Voltage order; lowest to highest.
> > >> 4. The Variable Supply (non-battery) Objects, if present, shall be
> > >> sent in Minimum Voltage order; lowest to highest.
> > >>
> > >> Errors in source/sink_caps of the local port will prevent
> > >> the port registration. Whereas, errors in source caps of partner
> > >> device would only log them.
> > >>
> > >> Signed-off-by: Badhri Jagan Sridharan <[email protected]>
> > >> ---
> > >> drivers/staging/typec/pd.h | 2 +
> > >> drivers/staging/typec/tcpm.c | 107 +++++++++++++++++++++++++++++++++++++++----
> > >> drivers/staging/typec/tcpm.h | 16 +++----
> > >> 3 files changed, 108 insertions(+), 17 deletions(-)
> > >
> > > Before you add more stuff to this driver, what is needed to get it out
> > > of staging? That would be more useful to do now, right?
> >
> > Actually not adding more features here. There was a bug where
> > the phone wasnt charging at the highest possible power output,
> > came up with the these patches while debugging that issue.
>
> Ok, but again, when is this going to get out of staging?

Code wise nothing. Add documentation to data structures, eliminate dead code,
drop and left over FIXME / TODO (those are not substantial enough to retain).

Guenter

2017-09-09 16:54:47

[permalink] [raw]

Subject: Re: [PATCH 1/2] staging: typec: tcpm: Validate source and sink caps

On 09/08/2017 12:13 PM, Greg Kroah-Hartman wrote:
> On Fri, Sep 08, 2017 at 10:29:52AM -0700, Badhri Jagan Sridharan wrote:
>> On Fri, Sep 8, 2017 at 2:45 AM, Greg Kroah-Hartman
>> <[email protected]> wrote:
>>> On Thu, Sep 07, 2017 at 06:22:13PM -0700, Badhri Jagan Sridharan wrote:
>>>> The source and sink caps should follow the following rules.
>>>> This patch validates whether the src_caps/snk_caps adheres
>>>> to it.
>>>>
>>>> 6.4.1 Capabilities Message
>>>> A Capabilities message (Source Capabilities message or Sink
>>>> Capabilities message) shall have at least one Power
>>>> Data Object for vSafe5V. The Capabilities message shall also
>>>> contain the sending Port’s information followed by up to
>>>> 6 additional Power Data Objects. Power Data Objects in a
>>>> Capabilities message shall be sent in the following order:
>>>>
>>>> 1. The vSafe5V Fixed Supply Object shall always be the first object.
>>>> 2. The remaining Fixed Supply Objects, if present, shall be sent
>>>> in voltage order; lowest to highest.
>>>> 3. The Battery Supply Objects, if present shall be sent in Minimum
>>>> Voltage order; lowest to highest.
>>>> 4. The Variable Supply (non-battery) Objects, if present, shall be
>>>> sent in Minimum Voltage order; lowest to highest.
>>>>
>>>> Errors in source/sink_caps of the local port will prevent
>>>> the port registration. Whereas, errors in source caps of partner
>>>> device would only log them.
>>>>
>>>> Signed-off-by: Badhri Jagan Sridharan <[email protected]>
>>>> ---
>>>> drivers/staging/typec/pd.h | 2 +
>>>> drivers/staging/typec/tcpm.c | 107 +++++++++++++++++++++++++++++++++++++++----
>>>> drivers/staging/typec/tcpm.h | 16 +++----
>>>> 3 files changed, 108 insertions(+), 17 deletions(-)
>>>
>>> Before you add more stuff to this driver, what is needed to get it out
>>> of staging? That would be more useful to do now, right?
>>
>> Actually not adding more features here. There was a bug where
>> the phone wasnt charging at the highest possible power output,
>> came up with the these patches while debugging that issue.
>
> Ok, but again, when is this going to get out of staging?
>
I have a set of patches ready to do just that. It only moves tcpm - tcpci
is still untested as far as I know, and fusb302 has unapproved devicetree
properties.

Want me to send it out now or after -rc1 ?

Guenter

2017-09-09 16:58:32

[permalink] [raw]

Subject: Re: [PATCH 1/2] staging: typec: tcpm: Validate source and sink caps

On 09/09/2017 09:54 AM, Guenter Roeck wrote:
> On 09/08/2017 12:13 PM, Greg Kroah-Hartman wrote:
>> On Fri, Sep 08, 2017 at 10:29:52AM -0700, Badhri Jagan Sridharan wrote:
>>> On Fri, Sep 8, 2017 at 2:45 AM, Greg Kroah-Hartman
>>> <[email protected]> wrote:
>>>> On Thu, Sep 07, 2017 at 06:22:13PM -0700, Badhri Jagan Sridharan wrote:
>>>>> The source and sink caps should follow the following rules.
>>>>> This patch validates whether the src_caps/snk_caps adheres
>>>>> to it.
>>>>>
>>>>> 6.4.1 Capabilities Message
>>>>> A Capabilities message (Source Capabilities message or Sink
>>>>> Capabilities message) shall have at least one Power
>>>>> Data Object for vSafe5V. The Capabilities message shall also
>>>>> contain the sending Port’s information followed by up to
>>>>> 6 additional Power Data Objects. Power Data Objects in a
>>>>> Capabilities message shall be sent in the following order:
>>>>>
>>>>> 1. The vSafe5V Fixed Supply Object shall always be the first object.
>>>>> 2. The remaining Fixed Supply Objects, if present, shall be sent
>>>>> in voltage order; lowest to highest.
>>>>> 3. The Battery Supply Objects, if present shall be sent in Minimum
>>>>> Voltage order; lowest to highest.
>>>>> 4. The Variable Supply (non-battery) Objects, if present, shall be
>>>>> sent in Minimum Voltage order; lowest to highest.
>>>>>
>>>>> Errors in source/sink_caps of the local port will prevent
>>>>> the port registration. Whereas, errors in source caps of partner
>>>>> device would only log them.
>>>>>
>>>>> Signed-off-by: Badhri Jagan Sridharan <[email protected]>
>>>>> ---
>>>>> drivers/staging/typec/pd.h | 2 +
>>>>> drivers/staging/typec/tcpm.c | 107 +++++++++++++++++++++++++++++++++++++++----
>>>>> drivers/staging/typec/tcpm.h | 16 +++----
>>>>> 3 files changed, 108 insertions(+), 17 deletions(-)
>>>>
>>>> Before you add more stuff to this driver, what is needed to get it out
>>>> of staging? That would be more useful to do now, right?
>>>
>>> Actually not adding more features here. There was a bug where
>>> the phone wasnt charging at the highest possible power output,
>>> came up with the these patches while debugging that issue.
>>
>> Ok, but again, when is this going to get out of staging?
>>
> I have a set of patches ready to do just that. It only moves tcpm - tcpci
> is still untested as far as I know, and fusb302 has unapproved devicetree
> properties.
>

Actually, the dt properties are Acked by Rob, so we can move that driver as well
(TODO is a bit misleading there). I'll add that to the patch series.

Guenter

2017-09-10 15:21:57

[permalink] [raw]

Subject: Re: [PATCH 1/2] staging: typec: tcpm: Validate source and sink caps

On Sat, Sep 09, 2017 at 09:54:42AM -0700, Guenter Roeck wrote:
> On 09/08/2017 12:13 PM, Greg Kroah-Hartman wrote:
> > On Fri, Sep 08, 2017 at 10:29:52AM -0700, Badhri Jagan Sridharan wrote:
> > > On Fri, Sep 8, 2017 at 2:45 AM, Greg Kroah-Hartman
> > > <[email protected]> wrote:
> > > > On Thu, Sep 07, 2017 at 06:22:13PM -0700, Badhri Jagan Sridharan wrote:
> > > > > The source and sink caps should follow the following rules.
> > > > > This patch validates whether the src_caps/snk_caps adheres
> > > > > to it.
> > > > >
> > > > > 6.4.1 Capabilities Message
> > > > > A Capabilities message (Source Capabilities message or Sink
> > > > > Capabilities message) shall have at least one Power
> > > > > Data Object for vSafe5V. The Capabilities message shall also
> > > > > contain the sending Port’s information followed by up to
> > > > > 6 additional Power Data Objects. Power Data Objects in a
> > > > > Capabilities message shall be sent in the following order:
> > > > >
> > > > > 1. The vSafe5V Fixed Supply Object shall always be the first object.
> > > > > 2. The remaining Fixed Supply Objects, if present, shall be sent
> > > > > in voltage order; lowest to highest.
> > > > > 3. The Battery Supply Objects, if present shall be sent in Minimum
> > > > > Voltage order; lowest to highest.
> > > > > 4. The Variable Supply (non-battery) Objects, if present, shall be
> > > > > sent in Minimum Voltage order; lowest to highest.
> > > > >
> > > > > Errors in source/sink_caps of the local port will prevent
> > > > > the port registration. Whereas, errors in source caps of partner
> > > > > device would only log them.
> > > > >
> > > > > Signed-off-by: Badhri Jagan Sridharan <[email protected]>
> > > > > ---
> > > > > drivers/staging/typec/pd.h | 2 +
> > > > > drivers/staging/typec/tcpm.c | 107 +++++++++++++++++++++++++++++++++++++++----
> > > > > drivers/staging/typec/tcpm.h | 16 +++----
> > > > > 3 files changed, 108 insertions(+), 17 deletions(-)
> > > >
> > > > Before you add more stuff to this driver, what is needed to get it out
> > > > of staging? That would be more useful to do now, right?
> > >
> > > Actually not adding more features here. There was a bug where
> > > the phone wasnt charging at the highest possible power output,
> > > came up with the these patches while debugging that issue.
> >
> > Ok, but again, when is this going to get out of staging?
> >
> I have a set of patches ready to do just that. It only moves tcpm - tcpci
> is still untested as far as I know, and fusb302 has unapproved devicetree
> properties.
>
> Want me to send it out now or after -rc1 ?

Now is good, I have been queueing up staging patches in my
staging-testing tree, so as to not get hit with a ton of work to do once
-rc1 comes out.

thanks,

greg k-h

2017-09-18 10:20:14