Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Wed, 16 Feb 2022 11:05:28 +0200
From:   Heikki Krogerus <heikki.krogerus@linux.intel.com>
To:     Samuel Holland <samuel@sholland.org>
Cc:     Guenter Roeck <linux@roeck-us.net>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org
Subject: Re: [PATCH v3 3/4] usb: typec: Factor out non-PD fwnode properties
Message-ID: <Ygy+WOhXNLKjZQfA@kuha.fi.intel.com>
References: <20220214050118.61015-1-samuel@sholland.org>
 <20220214050118.61015-4-samuel@sholland.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220214050118.61015-4-samuel@sholland.org>
Precedence: bulk

On Sun, Feb 13, 2022 at 11:01:16PM -0600, Samuel Holland wrote:
> Basic programmable non-PD Type-C port controllers do not need the full
> TCPM library, but they share the same devicetree binding and the same
> typec_capability structure. Factor out a helper for parsing those
> properties which map to fields in struct typec_capability, so the code
> can be shared between TCPM and basic non-TCPM drivers.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
> 
> Changes in v3:
>  - Leave the call to fw_devlink_purge_absent_suppliers in its original
>    place in the TCPM code. It is not needed by the new driver.
>  - Remove unused variable from TCPM function.
> 
> Changes in v2:
>  - Always put the return values from typec_find_* in a signed variable
>    for error checking.
> 
>  drivers/usb/typec/class.c     | 43 +++++++++++++++++++++++++++++++++++
>  drivers/usb/typec/tcpm/tcpm.c | 24 +------------------
>  include/linux/usb/typec.h     |  3 +++
>  3 files changed, 47 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index 45a6f0c807cb..ee0e520707dd 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -1894,6 +1894,49 @@ void *typec_get_drvdata(struct typec_port *port)
>  }
>  EXPORT_SYMBOL_GPL(typec_get_drvdata);
>  
> +int typec_get_fw_cap(struct typec_capability *cap,
> +		     struct fwnode_handle *fwnode)
> +{
> +	const char *cap_str;
> +	int ret;
> +
> +	cap->fwnode = fwnode;
> +
> +	ret = fwnode_property_read_string(fwnode, "power-role", &cap_str);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = typec_find_port_power_role(cap_str);
> +	if (ret < 0)
> +		return ret;
> +	cap->type = ret;
> +
> +	/* USB data support is optional */
> +	ret = fwnode_property_read_string(fwnode, "data-role", &cap_str);
> +	if (ret == 0) {
> +		ret = typec_find_port_data_role(cap_str);
> +		if (ret < 0)
> +			return ret;
> +		cap->data = ret;
> +	}
> +
> +	/* Get the preferred power role for a DRP */
> +	if (cap->type == TYPEC_PORT_DRP) {
> +		cap->prefer_role = TYPEC_NO_PREFERRED_ROLE;
> +
> +		ret = fwnode_property_read_string(fwnode, "try-power-role", &cap_str);
> +		if (ret == 0) {
> +			ret = typec_find_power_role(cap_str);
> +			if (ret < 0)
> +				return ret;
> +			cap->prefer_role = ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(typec_get_fw_cap);
> +
>  /**
>   * typec_port_register_altmode - Register USB Type-C Port Alternate Mode
>   * @port: USB Type-C Port that supports the alternate mode
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 5fce795b69c7..3bc2f4ebd1fe 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -5928,7 +5928,6 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
>  			    struct fwnode_handle *fwnode)
>  {
>  	const char *opmode_str;
> -	const char *cap_str;
>  	int ret;
>  	u32 mw, frs_current;
>  
> @@ -5944,23 +5943,10 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
>  	 */
>  	fw_devlink_purge_absent_suppliers(fwnode);
>  
> -	/* USB data support is optional */
> -	ret = fwnode_property_read_string(fwnode, "data-role", &cap_str);
> -	if (ret == 0) {
> -		ret = typec_find_port_data_role(cap_str);
> -		if (ret < 0)
> -			return ret;
> -		port->typec_caps.data = ret;
> -	}
> -
> -	ret = fwnode_property_read_string(fwnode, "power-role", &cap_str);
> +	ret = typec_get_fw_cap(&port->typec_caps, fwnode);
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = typec_find_port_power_role(cap_str);
> -	if (ret < 0)
> -		return ret;
> -	port->typec_caps.type = ret;
>  	port->port_type = port->typec_caps.type;
>  	port->pd_supported = !fwnode_property_read_bool(fwnode, "pd-disable");
>  
> @@ -5997,14 +5983,6 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
>  	if (port->port_type == TYPEC_PORT_SRC)
>  		return 0;
>  
> -	/* Get the preferred power role for DRP */
> -	ret = fwnode_property_read_string(fwnode, "try-power-role", &cap_str);
> -	if (ret < 0)
> -		return ret;
> -
> -	port->typec_caps.prefer_role = typec_find_power_role(cap_str);
> -	if (port->typec_caps.prefer_role < 0)
> -		return -EINVAL;
>  sink:
>  	port->self_powered = fwnode_property_read_bool(fwnode, "self-powered");
>  
> diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
> index 7ba45a97eeae..fdf737d48b3b 100644
> --- a/include/linux/usb/typec.h
> +++ b/include/linux/usb/typec.h
> @@ -295,6 +295,9 @@ int typec_set_mode(struct typec_port *port, int mode);
>  
>  void *typec_get_drvdata(struct typec_port *port);
>  
> +int typec_get_fw_cap(struct typec_capability *cap,
> +		     struct fwnode_handle *fwnode);
> +
>  int typec_find_pwr_opmode(const char *name);
>  int typec_find_orientation(const char *name);
>  int typec_find_port_power_role(const char *name);
> -- 
> 2.33.1

thanks,

-- 
heikki