Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Guenter Roeck <groeck7@gmail.com>
Message-ID: <1acc1733-da53-6255-5cad-15f79850f44f@roeck-us.net>
Date:   Tue, 8 Feb 2022 07:22:57 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Content-Language: en-US
To:     Potin Lai <potin.lai@quantatw.com>,
        Heikki Krogerus <heikki.krogerus@linux.intel.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        Patrick Williams <patrick@stwcx.xyz>
References: <20220208082026.4855-1-potin.lai@quantatw.com>
 <20220208112226.9108-1-potin.lai@quantatw.com>
 <20220208112226.9108-3-potin.lai@quantatw.com>
From:   Guenter Roeck <linux@roeck-us.net>
Subject: Re: [PATCH v2 2/2] usb: typec: fusb302: add support of
 supported_pd_rev
In-Reply-To: <20220208112226.9108-3-potin.lai@quantatw.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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

> Signed-off-by: Potin Lai <potin.lai@quantatw.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);
>