linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Guenter Roeck <linux@roeck-us.net>
To: Kyle Tso <kyletso@google.com>,
	heikki.krogerus@linux.intel.com, gregkh@linuxfoundation.org
Cc: badhri@google.com, linux-usb@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] usb: typec: tcpm: Support non-PD mode
Date: Fri, 23 Jul 2021 12:07:32 -0700	[thread overview]
Message-ID: <66b76646-3c40-fb12-6678-6542c10caaa9@roeck-us.net> (raw)
In-Reply-To: <CAGZ6i=3PJ+aRzM7=c6f9oCaCjvdQ7GqtCn+dv7H0yC8WMoe+KA@mail.gmail.com>

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
>>


  reply	other threads:[~2021-07-23 19:07 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-16  9:39 Kyle Tso
2021-07-23 17:18 ` Kyle Tso
2021-07-23 19:07   ` Guenter Roeck [this message]
2021-07-26  9:42     ` Kyle Tso

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=66b76646-3c40-fb12-6678-6542c10caaa9@roeck-us.net \
    --to=linux@roeck-us.net \
    --cc=badhri@google.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=heikki.krogerus@linux.intel.com \
    --cc=kyletso@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --subject='Re: [PATCH] usb: typec: tcpm: Support non-PD mode' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).