linux-kernel - Re: [PATCH v2 2/2] usb: typec: fusb302: add support of supported_pd

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e2aec7c6-2d73-6446-89c9-797850f9402f@roeck-us.net>
Date:   Wed, 9 Feb 2022 09:50:49 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Potin Lai <potin.lai@...ntatw.com>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        Patrick Williams <patrick@...cx.xyz>
Subject: Re: [PATCH v2 2/2] usb: typec: fusb302: add support of
 supported_pd_rev

On 2/8/22 19:34, Potin Lai wrote:
> 
> Guenter Roeck 於 2022/2/8 下午 11:22 寫道:
>> On 2/8/22 03:22, Potin Lai wrote:
>>> Add support for passing supported PD rev to TCPM.
>>> If "supported-pd-rev" property exist, then return supported_pd_rev as
>>> defined value in DTS, otherwise return PD_MAX_REV
>>>
>>> Example of DTS:
>>>
>>> fusb302: typec-portc@22 {
>>>      compatible = "fcs,fusb302";
>>>      reg = <0x22>;
>>>      ...
>>>      supported-pd-rev=<1>; // PD_REV20
>>>      ...
>>> };
>>>
>>
>> The new property needs to be documented. However, I am not entirely sure
>> I understand why it is needed. Wouldn't the supported PD revision
>> be a chip (fusb302) specific constant ? If so, why does it need a
>> devicetree property ? I think that needs some additional explanation.
>>
>> Thanks,
>> Guenter
>>
> 
> My initially intend was adding flexibility for developer to decided which PD revision
> they want to use for negotiation.
> 

I may be missing something, but I don't think that "flexibility for developer
to decide which PD revision they want to use for negotiation" is entirely appropriate.
This should be a chip property, not something a developer should decide. As written,
the code does accept PC version 3 for FUSB302 by default, which seems odd and unusual.
If the chip supports PD version 3, why artificially limit it to earlier PD revisions ?

I think this requires a detailed description of the use case. Is fusb302 indeed not able
to support a specific version of the power delivery specification ? If yes,
what is the limitation, and why would it need to be configurable with a devicetree
property ?

Thanks,
Guenter

> I saw your reply in another patch,  I agree with you, it will be simple and consistent if
> move "supported-pd-rev" to tcpm fwnode as other capabilities.
> 
> Just want to double confirm, is "usb-connector.yaml" right place to put documentation
> if adding "supported-pd-rev" in fwnode?
> 
> Thanks,
> Potin
> 
>>> Signed-off-by: Potin Lai <potin.lai@...ntatw.com>
>>> ---
>>>   drivers/usb/typec/tcpm/fusb302.c | 20 ++++++++++++++++++++
>>>   1 file changed, 20 insertions(+)
>>>
>>> diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
>>> index 72f9001b0792..8cff92d58b96 100644
>>> --- a/drivers/usb/typec/tcpm/fusb302.c
>>> +++ b/drivers/usb/typec/tcpm/fusb302.c
>>> @@ -109,6 +109,9 @@ struct fusb302_chip {
>>>       enum typec_cc_status cc2;
>>>       u32 snk_pdo[PDO_MAX_OBJECTS];
>>>   +    /* supported pd rev */
>>> +    u32 supported_pd_rev;
>>> +
>>>   #ifdef CONFIG_DEBUG_FS
>>>       struct dentry *dentry;
>>>       /* lock for log buffer access */
>>> @@ -1056,6 +1059,13 @@ static int tcpm_pd_transmit(struct tcpc_dev *dev, enum tcpm_transmit_type type,
>>>       return ret;
>>>   }
>>>   +static u32 tcpm_supported_pd_rev(struct tcpc_dev *dev)
>>> +{
>>> +    struct fusb302_chip *chip = container_of(dev, struct fusb302_chip,
>>> +                         tcpc_dev);
>>> +    return chip->supported_pd_rev;
>>> +}
>>> +
>>>   static enum typec_cc_status fusb302_bc_lvl_to_cc(u8 bc_lvl)
>>>   {
>>>       if (bc_lvl == FUSB_REG_STATUS0_BC_LVL_1230_MAX)
>>> @@ -1129,6 +1139,7 @@ static void init_tcpc_dev(struct tcpc_dev *fusb302_tcpc_dev)
>>>       fusb302_tcpc_dev->set_roles = tcpm_set_roles;
>>>       fusb302_tcpc_dev->start_toggling = tcpm_start_toggling;
>>>       fusb302_tcpc_dev->pd_transmit = tcpm_pd_transmit;
>>> +    fusb302_tcpc_dev->supported_pd_rev = tcpm_supported_pd_rev;
>>>   }
>>>     static const char * const cc_polarity_name[] = {
>>> @@ -1683,6 +1694,7 @@ static int fusb302_probe(struct i2c_client *client,
>>>       struct fusb302_chip *chip;
>>>       struct i2c_adapter *adapter = client->adapter;
>>>       struct device *dev = &client->dev;
>>> +    struct device_node *np = dev->of_node;
>>>       const char *name;
>>>       int ret = 0;
>>>   @@ -1756,6 +1768,14 @@ static int fusb302_probe(struct i2c_client *client,
>>>           dev_err(dev, "cannot request IRQ for GPIO Int_N, ret=%d", ret);
>>>           goto tcpm_unregister_port;
>>>       }
>>> +
>>> +    if (of_property_read_u32(np, "supported-pd-rev",
>>> +                &chip->supported_pd_rev) < 0) {
>>> +        chip->supported_pd_rev = PD_MAX_REV;
>>> +    } else if (chip->supported_pd_rev > PD_MAX_REV) {
>>> +        chip->supported_pd_rev = PD_MAX_REV;
>>> +    }
>>
>> The else part is also checked in the tcpm code and thus seems
>> to be redundant.
>>
>>> +
>>>       enable_irq_wake(chip->gpio_int_n_irq);
>>>       i2c_set_clientdata(client, chip);
>>