* [PATCH] usb: typec: tcpm: Support non-PD mode
@ 2021-07-16 9:39 Kyle Tso
2021-07-23 17:18 ` Kyle Tso
0 siblings, 1 reply; 4+ messages in thread
From: Kyle Tso @ 2021-07-16 9:39 UTC (permalink / raw)
To: linux, heikki.krogerus, gregkh; +Cc: badhri, linux-usb, linux-kernel, Kyle Tso
tcpm.c could work well without PD capabilities. Do not block the probe
if capabilities are not defined in fwnode and skip the PD power
negotiation in the state machine.
Signed-off-by: Kyle Tso <kyletso@google.com>
---
drivers/usb/typec/tcpm/tcpm.c | 50 ++++++++++++++++++++---------------
1 file changed, 29 insertions(+), 21 deletions(-)
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 5b22a1c931a9..a42de5e17d24 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -3914,6 +3914,8 @@ static void run_state_machine(struct tcpm_port *port)
if (port->ams == POWER_ROLE_SWAP ||
port->ams == FAST_ROLE_SWAP)
tcpm_ams_finish(port);
+ if (!port->nr_src_pdo)
+ tcpm_set_state(port, SRC_READY, 0);
port->upcoming_state = SRC_SEND_CAPABILITIES;
tcpm_ams_start(port, POWER_NEGOTIATION);
break;
@@ -4161,7 +4163,10 @@ static void run_state_machine(struct tcpm_port *port)
current_lim = PD_P_SNK_STDBY_MW / 5;
tcpm_set_current_limit(port, current_lim, 5000);
tcpm_set_charge(port, true);
- tcpm_set_state(port, SNK_WAIT_CAPABILITIES, 0);
+ if (!port->nr_snk_pdo)
+ tcpm_set_state(port, SNK_READY, 0);
+ else
+ tcpm_set_state(port, SNK_WAIT_CAPABILITIES, 0);
break;
}
/*
@@ -5939,15 +5944,17 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
/* Get source pdos */
ret = fwnode_property_count_u32(fwnode, "source-pdos");
- if (ret <= 0)
- return -EINVAL;
+ if (ret < 0)
+ ret = 0;
port->nr_src_pdo = min(ret, PDO_MAX_OBJECTS);
- ret = fwnode_property_read_u32_array(fwnode, "source-pdos",
- port->src_pdo, port->nr_src_pdo);
- if ((ret < 0) || tcpm_validate_caps(port, port->src_pdo,
- port->nr_src_pdo))
- return -EINVAL;
+ if (port->nr_src_pdo) {
+ ret = fwnode_property_read_u32_array(fwnode, "source-pdos",
+ port->src_pdo, port->nr_src_pdo);
+ if ((ret < 0) || tcpm_validate_caps(port, port->src_pdo,
+ port->nr_src_pdo))
+ return -EINVAL;
+ }
if (port->port_type == TYPEC_PORT_SRC)
return 0;
@@ -5963,19 +5970,21 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
sink:
/* Get sink pdos */
ret = fwnode_property_count_u32(fwnode, "sink-pdos");
- if (ret <= 0)
- return -EINVAL;
+ if (ret < 0)
+ ret = 0;
port->nr_snk_pdo = min(ret, PDO_MAX_OBJECTS);
- ret = fwnode_property_read_u32_array(fwnode, "sink-pdos",
- port->snk_pdo, port->nr_snk_pdo);
- if ((ret < 0) || tcpm_validate_caps(port, port->snk_pdo,
- port->nr_snk_pdo))
- return -EINVAL;
+ if (port->nr_snk_pdo) {
+ ret = fwnode_property_read_u32_array(fwnode, "sink-pdos",
+ port->snk_pdo, port->nr_snk_pdo);
+ if ((ret < 0) || tcpm_validate_caps(port, port->snk_pdo,
+ port->nr_snk_pdo))
+ return -EINVAL;
- if (fwnode_property_read_u32(fwnode, "op-sink-microwatt", &mw) < 0)
- return -EINVAL;
- port->operating_snk_mw = mw / 1000;
+ if (fwnode_property_read_u32(fwnode, "op-sink-microwatt", &mw) < 0)
+ return -EINVAL;
+ port->operating_snk_mw = mw / 1000;
+ }
port->self_powered = fwnode_property_read_bool(fwnode, "self-powered");
@@ -6283,9 +6292,8 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
int err;
if (!dev || !tcpc ||
- !tcpc->get_vbus || !tcpc->set_cc || !tcpc->get_cc ||
- !tcpc->set_polarity || !tcpc->set_vconn || !tcpc->set_vbus ||
- !tcpc->set_pd_rx || !tcpc->set_roles || !tcpc->pd_transmit)
+ !tcpc->get_vbus || !tcpc->set_cc || !tcpc->get_cc || !tcpc->set_polarity ||
+ !tcpc->set_vconn || !tcpc->set_vbus || !tcpc->set_roles)
return ERR_PTR(-EINVAL);
port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
--
2.32.0.402.g57bb445576-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] usb: typec: tcpm: Support non-PD mode
2021-07-16 9:39 [PATCH] usb: typec: tcpm: Support non-PD mode Kyle Tso
@ 2021-07-23 17:18 ` Kyle Tso
2021-07-23 19:07 ` Guenter Roeck
0 siblings, 1 reply; 4+ messages in thread
From: Kyle Tso @ 2021-07-23 17:18 UTC (permalink / raw)
To: linux, heikki.krogerus, gregkh; +Cc: badhri, linux-usb, linux-kernel
On Fri, Jul 16, 2021 at 5:39 PM Kyle Tso <kyletso@google.com> wrote:
>
> tcpm.c could work well without PD capabilities. Do not block the probe
> if capabilities are not defined in fwnode and skip the PD power
> negotiation in the state machine.
>
> Signed-off-by: Kyle Tso <kyletso@google.com>
> ---
Hi, any comments about this patch?
thanks,
Kyle
> drivers/usb/typec/tcpm/tcpm.c | 50 ++++++++++++++++++++---------------
> 1 file changed, 29 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 5b22a1c931a9..a42de5e17d24 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -3914,6 +3914,8 @@ static void run_state_machine(struct tcpm_port *port)
> if (port->ams == POWER_ROLE_SWAP ||
> port->ams == FAST_ROLE_SWAP)
> tcpm_ams_finish(port);
> + if (!port->nr_src_pdo)
> + tcpm_set_state(port, SRC_READY, 0);
> port->upcoming_state = SRC_SEND_CAPABILITIES;
> tcpm_ams_start(port, POWER_NEGOTIATION);
> break;
> @@ -4161,7 +4163,10 @@ static void run_state_machine(struct tcpm_port *port)
> current_lim = PD_P_SNK_STDBY_MW / 5;
> tcpm_set_current_limit(port, current_lim, 5000);
> tcpm_set_charge(port, true);
> - tcpm_set_state(port, SNK_WAIT_CAPABILITIES, 0);
> + if (!port->nr_snk_pdo)
> + tcpm_set_state(port, SNK_READY, 0);
> + else
> + tcpm_set_state(port, SNK_WAIT_CAPABILITIES, 0);
> break;
> }
> /*
> @@ -5939,15 +5944,17 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
>
> /* Get source pdos */
> ret = fwnode_property_count_u32(fwnode, "source-pdos");
> - if (ret <= 0)
> - return -EINVAL;
> + if (ret < 0)
> + ret = 0;
>
> port->nr_src_pdo = min(ret, PDO_MAX_OBJECTS);
> - ret = fwnode_property_read_u32_array(fwnode, "source-pdos",
> - port->src_pdo, port->nr_src_pdo);
> - if ((ret < 0) || tcpm_validate_caps(port, port->src_pdo,
> - port->nr_src_pdo))
> - return -EINVAL;
> + if (port->nr_src_pdo) {
> + ret = fwnode_property_read_u32_array(fwnode, "source-pdos",
> + port->src_pdo, port->nr_src_pdo);
> + if ((ret < 0) || tcpm_validate_caps(port, port->src_pdo,
> + port->nr_src_pdo))
> + return -EINVAL;
> + }
>
> if (port->port_type == TYPEC_PORT_SRC)
> return 0;
> @@ -5963,19 +5970,21 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
> sink:
> /* Get sink pdos */
> ret = fwnode_property_count_u32(fwnode, "sink-pdos");
> - if (ret <= 0)
> - return -EINVAL;
> + if (ret < 0)
> + ret = 0;
>
> port->nr_snk_pdo = min(ret, PDO_MAX_OBJECTS);
> - ret = fwnode_property_read_u32_array(fwnode, "sink-pdos",
> - port->snk_pdo, port->nr_snk_pdo);
> - if ((ret < 0) || tcpm_validate_caps(port, port->snk_pdo,
> - port->nr_snk_pdo))
> - return -EINVAL;
> + if (port->nr_snk_pdo) {
> + ret = fwnode_property_read_u32_array(fwnode, "sink-pdos",
> + port->snk_pdo, port->nr_snk_pdo);
> + if ((ret < 0) || tcpm_validate_caps(port, port->snk_pdo,
> + port->nr_snk_pdo))
> + return -EINVAL;
>
> - if (fwnode_property_read_u32(fwnode, "op-sink-microwatt", &mw) < 0)
> - return -EINVAL;
> - port->operating_snk_mw = mw / 1000;
> + if (fwnode_property_read_u32(fwnode, "op-sink-microwatt", &mw) < 0)
> + return -EINVAL;
> + port->operating_snk_mw = mw / 1000;
> + }
>
> port->self_powered = fwnode_property_read_bool(fwnode, "self-powered");
>
> @@ -6283,9 +6292,8 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
> int err;
>
> if (!dev || !tcpc ||
> - !tcpc->get_vbus || !tcpc->set_cc || !tcpc->get_cc ||
> - !tcpc->set_polarity || !tcpc->set_vconn || !tcpc->set_vbus ||
> - !tcpc->set_pd_rx || !tcpc->set_roles || !tcpc->pd_transmit)
> + !tcpc->get_vbus || !tcpc->set_cc || !tcpc->get_cc || !tcpc->set_polarity ||
> + !tcpc->set_vconn || !tcpc->set_vbus || !tcpc->set_roles)
> return ERR_PTR(-EINVAL);
>
> port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> --
> 2.32.0.402.g57bb445576-goog
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] usb: typec: tcpm: Support non-PD mode
2021-07-23 17:18 ` Kyle Tso
@ 2021-07-23 19:07 ` Guenter Roeck
2021-07-26 9:42 ` Kyle Tso
0 siblings, 1 reply; 4+ messages in thread
From: Guenter Roeck @ 2021-07-23 19:07 UTC (permalink / raw)
To: Kyle Tso, heikki.krogerus, gregkh; +Cc: badhri, linux-usb, linux-kernel
On 7/23/21 10:18 AM, Kyle Tso wrote:
> On Fri, Jul 16, 2021 at 5:39 PM Kyle Tso <kyletso@google.com> wrote:
>>
>> tcpm.c could work well without PD capabilities. Do not block the probe
"could" is a bit vague. What is the use case ?
>> if capabilities are not defined in fwnode and skip the PD power
>> negotiation in the state machine.
>>
>> Signed-off-by: Kyle Tso <kyletso@google.com>
>> ---
>
> Hi, any comments about this patch?
>
First question would be if this is documented/standardized. More comments below.
> thanks,
> Kyle
>
>> drivers/usb/typec/tcpm/tcpm.c | 50 ++++++++++++++++++++---------------
>> 1 file changed, 29 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>> index 5b22a1c931a9..a42de5e17d24 100644
>> --- a/drivers/usb/typec/tcpm/tcpm.c
>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>> @@ -3914,6 +3914,8 @@ static void run_state_machine(struct tcpm_port *port)
>> if (port->ams == POWER_ROLE_SWAP ||
>> port->ams == FAST_ROLE_SWAP)
>> tcpm_ams_finish(port);
>> + if (!port->nr_src_pdo)
>> + tcpm_set_state(port, SRC_READY, 0);
>> port->upcoming_state = SRC_SEND_CAPABILITIES;
>> tcpm_ams_start(port, POWER_NEGOTIATION);
>> break;
>> @@ -4161,7 +4163,10 @@ static void run_state_machine(struct tcpm_port *port)
>> current_lim = PD_P_SNK_STDBY_MW / 5;
>> tcpm_set_current_limit(port, current_lim, 5000);
>> tcpm_set_charge(port, true);
>> - tcpm_set_state(port, SNK_WAIT_CAPABILITIES, 0);
>> + if (!port->nr_snk_pdo)
>> + tcpm_set_state(port, SNK_READY, 0);
>> + else
>> + tcpm_set_state(port, SNK_WAIT_CAPABILITIES, 0);
>> break;
>> }
>> /*
>> @@ -5939,15 +5944,17 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
>>
>> /* Get source pdos */
>> ret = fwnode_property_count_u32(fwnode, "source-pdos");
>> - if (ret <= 0)
>> - return -EINVAL;
>> + if (ret < 0)
>> + ret = 0;
>>
This makes the capability properties optional. I think that would have
to be documented.
>> port->nr_src_pdo = min(ret, PDO_MAX_OBJECTS);
>> - ret = fwnode_property_read_u32_array(fwnode, "source-pdos",
>> - port->src_pdo, port->nr_src_pdo);
>> - if ((ret < 0) || tcpm_validate_caps(port, port->src_pdo,
>> - port->nr_src_pdo))
>> - return -EINVAL;
>> + if (port->nr_src_pdo) {
>> + ret = fwnode_property_read_u32_array(fwnode, "source-pdos",
>> + port->src_pdo, port->nr_src_pdo);
>> + if ((ret < 0) || tcpm_validate_caps(port, port->src_pdo,
>> + port->nr_src_pdo))
>> + return -EINVAL;
>> + }
>>
>> if (port->port_type == TYPEC_PORT_SRC)
>> return 0;
>> @@ -5963,19 +5970,21 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
>> sink:
>> /* Get sink pdos */
>> ret = fwnode_property_count_u32(fwnode, "sink-pdos");
>> - if (ret <= 0)
>> - return -EINVAL;
>> + if (ret < 0)
>> + ret = 0;
>>
>> port->nr_snk_pdo = min(ret, PDO_MAX_OBJECTS);
>> - ret = fwnode_property_read_u32_array(fwnode, "sink-pdos",
>> - port->snk_pdo, port->nr_snk_pdo);
>> - if ((ret < 0) || tcpm_validate_caps(port, port->snk_pdo,
>> - port->nr_snk_pdo))
>> - return -EINVAL;
>> + if (port->nr_snk_pdo) {
>> + ret = fwnode_property_read_u32_array(fwnode, "sink-pdos",
>> + port->snk_pdo, port->nr_snk_pdo);
>> + if ((ret < 0) || tcpm_validate_caps(port, port->snk_pdo,
>> + port->nr_snk_pdo))
>> + return -EINVAL;
>>
>> - if (fwnode_property_read_u32(fwnode, "op-sink-microwatt", &mw) < 0)
>> - return -EINVAL;
>> - port->operating_snk_mw = mw / 1000;
>> + if (fwnode_property_read_u32(fwnode, "op-sink-microwatt", &mw) < 0)
>> + return -EINVAL;
>> + port->operating_snk_mw = mw / 1000;
>> + }
>>
>> port->self_powered = fwnode_property_read_bool(fwnode, "self-powered");
>>
>> @@ -6283,9 +6292,8 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>> int err;
>>
>> if (!dev || !tcpc ||
>> - !tcpc->get_vbus || !tcpc->set_cc || !tcpc->get_cc ||
>> - !tcpc->set_polarity || !tcpc->set_vconn || !tcpc->set_vbus ||
>> - !tcpc->set_pd_rx || !tcpc->set_roles || !tcpc->pd_transmit)
>> + !tcpc->get_vbus || !tcpc->set_cc || !tcpc->get_cc || !tcpc->set_polarity ||
>> + !tcpc->set_vconn || !tcpc->set_vbus || !tcpc->set_roles)
With this change, if a driver does not define the necessary pd callbacks
(set_pd_rx, pd_transmit), but its devicetree data does provide pdo properties,
we'll get a nice crash.
On top of that, I am quite sure that the set_pd_rx() callback is still called
from various places even if neither sink-pdos nor source-pdos properties
are defined.
I think this really tries to handle two conditions: A low level driver that
doesn't support PD, and a system where the low level driver does support PD
but the system itself doesn't. And then there is the odd case where the system
only supports either source or sink PD while claiming to support the other.
Maybe it would make sense to separate both conditions, for example by introducing
a new flag such as pd_supported to indicate that the system doesn't support the
PD protocol.
Guenter
>> return ERR_PTR(-EINVAL);
>>
>> port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
>> --
>> 2.32.0.402.g57bb445576-goog
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] usb: typec: tcpm: Support non-PD mode
2021-07-23 19:07 ` Guenter Roeck
@ 2021-07-26 9:42 ` Kyle Tso
0 siblings, 0 replies; 4+ messages in thread
From: Kyle Tso @ 2021-07-26 9:42 UTC (permalink / raw)
To: Guenter Roeck; +Cc: heikki.krogerus, gregkh, badhri, linux-usb, linux-kernel
On Sat, Jul 24, 2021 at 3:07 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 7/23/21 10:18 AM, Kyle Tso wrote:
> > On Fri, Jul 16, 2021 at 5:39 PM Kyle Tso <kyletso@google.com> wrote:
> >>
> >> tcpm.c could work well without PD capabilities. Do not block the probe
>
> "could" is a bit vague. What is the use case ?
>
Just to enable the "capability" of a type-c port without PD support. I
will rephrase the message to be more specific.
> >> if capabilities are not defined in fwnode and skip the PD power
> >> negotiation in the state machine.
> >>
> >> Signed-off-by: Kyle Tso <kyletso@google.com>
> >> ---
> >
> > Hi, any comments about this patch?
> >
>
> First question would be if this is documented/standardized. More comments below.
>
> > thanks,
> > Kyle
> >
> >> drivers/usb/typec/tcpm/tcpm.c | 50 ++++++++++++++++++++---------------
> >> 1 file changed, 29 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> >> index 5b22a1c931a9..a42de5e17d24 100644
> >> --- a/drivers/usb/typec/tcpm/tcpm.c
> >> +++ b/drivers/usb/typec/tcpm/tcpm.c
> >> @@ -3914,6 +3914,8 @@ static void run_state_machine(struct tcpm_port *port)
> >> if (port->ams == POWER_ROLE_SWAP ||
> >> port->ams == FAST_ROLE_SWAP)
> >> tcpm_ams_finish(port);
> >> + if (!port->nr_src_pdo)
> >> + tcpm_set_state(port, SRC_READY, 0);
> >> port->upcoming_state = SRC_SEND_CAPABILITIES;
> >> tcpm_ams_start(port, POWER_NEGOTIATION);
> >> break;
> >> @@ -4161,7 +4163,10 @@ static void run_state_machine(struct tcpm_port *port)
> >> current_lim = PD_P_SNK_STDBY_MW / 5;
> >> tcpm_set_current_limit(port, current_lim, 5000);
> >> tcpm_set_charge(port, true);
> >> - tcpm_set_state(port, SNK_WAIT_CAPABILITIES, 0);
> >> + if (!port->nr_snk_pdo)
> >> + tcpm_set_state(port, SNK_READY, 0);
> >> + else
> >> + tcpm_set_state(port, SNK_WAIT_CAPABILITIES, 0);
> >> break;
> >> }
> >> /*
> >> @@ -5939,15 +5944,17 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
> >>
> >> /* Get source pdos */
> >> ret = fwnode_property_count_u32(fwnode, "source-pdos");
> >> - if (ret <= 0)
> >> - return -EINVAL;
> >> + if (ret < 0)
> >> + ret = 0;
> >>
>
> This makes the capability properties optional. I think that would have
> to be documented.
>
Do you mean the documentation in dt-bindings? I think they have
already been optional?
> >> port->nr_src_pdo = min(ret, PDO_MAX_OBJECTS);
> >> - ret = fwnode_property_read_u32_array(fwnode, "source-pdos",
> >> - port->src_pdo, port->nr_src_pdo);
> >> - if ((ret < 0) || tcpm_validate_caps(port, port->src_pdo,
> >> - port->nr_src_pdo))
> >> - return -EINVAL;
> >> + if (port->nr_src_pdo) {
> >> + ret = fwnode_property_read_u32_array(fwnode, "source-pdos",
> >> + port->src_pdo, port->nr_src_pdo);
> >> + if ((ret < 0) || tcpm_validate_caps(port, port->src_pdo,
> >> + port->nr_src_pdo))
> >> + return -EINVAL;
> >> + }
> >>
> >> if (port->port_type == TYPEC_PORT_SRC)
> >> return 0;
> >> @@ -5963,19 +5970,21 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
> >> sink:
> >> /* Get sink pdos */
> >> ret = fwnode_property_count_u32(fwnode, "sink-pdos");
> >> - if (ret <= 0)
> >> - return -EINVAL;
> >> + if (ret < 0)
> >> + ret = 0;
> >>
> >> port->nr_snk_pdo = min(ret, PDO_MAX_OBJECTS);
> >> - ret = fwnode_property_read_u32_array(fwnode, "sink-pdos",
> >> - port->snk_pdo, port->nr_snk_pdo);
> >> - if ((ret < 0) || tcpm_validate_caps(port, port->snk_pdo,
> >> - port->nr_snk_pdo))
> >> - return -EINVAL;
> >> + if (port->nr_snk_pdo) {
> >> + ret = fwnode_property_read_u32_array(fwnode, "sink-pdos",
> >> + port->snk_pdo, port->nr_snk_pdo);
> >> + if ((ret < 0) || tcpm_validate_caps(port, port->snk_pdo,
> >> + port->nr_snk_pdo))
> >> + return -EINVAL;
> >>
> >> - if (fwnode_property_read_u32(fwnode, "op-sink-microwatt", &mw) < 0)
> >> - return -EINVAL;
> >> - port->operating_snk_mw = mw / 1000;
> >> + if (fwnode_property_read_u32(fwnode, "op-sink-microwatt", &mw) < 0)
> >> + return -EINVAL;
> >> + port->operating_snk_mw = mw / 1000;
> >> + }
> >>
> >> port->self_powered = fwnode_property_read_bool(fwnode, "self-powered");
> >>
> >> @@ -6283,9 +6292,8 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
> >> int err;
> >>
> >> if (!dev || !tcpc ||
> >> - !tcpc->get_vbus || !tcpc->set_cc || !tcpc->get_cc ||
> >> - !tcpc->set_polarity || !tcpc->set_vconn || !tcpc->set_vbus ||
> >> - !tcpc->set_pd_rx || !tcpc->set_roles || !tcpc->pd_transmit)
> >> + !tcpc->get_vbus || !tcpc->set_cc || !tcpc->get_cc || !tcpc->set_polarity ||
> >> + !tcpc->set_vconn || !tcpc->set_vbus || !tcpc->set_roles)
>
> With this change, if a driver does not define the necessary pd callbacks
> (set_pd_rx, pd_transmit), but its devicetree data does provide pdo properties,
> we'll get a nice crash.
>
> On top of that, I am quite sure that the set_pd_rx() callback is still called
> from various places even if neither sink-pdos nor source-pdos properties
> are defined.
>
> I think this really tries to handle two conditions: A low level driver that
> doesn't support PD, and a system where the low level driver does support PD
> but the system itself doesn't. And then there is the odd case where the system
> only supports either source or sink PD while claiming to support the other.
> Maybe it would make sense to separate both conditions, for example by introducing
> a new flag such as pd_supported to indicate that the system doesn't support the
> PD protocol.
>
> Guenter
>
Yes, your concern is correct and reasonable. I will send v2 to fix these.
thanks,
Kyle
> >> return ERR_PTR(-EINVAL);
> >>
> >> port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> >> --
> >> 2.32.0.402.g57bb445576-goog
> >>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-07-26 9:43 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-16 9:39 [PATCH] usb: typec: tcpm: Support non-PD mode Kyle Tso
2021-07-23 17:18 ` Kyle Tso
2021-07-23 19:07 ` Guenter Roeck
2021-07-26 9:42 ` Kyle Tso
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.