Re: [PATCH 2/2] platform/chrome: cros_ec_typec: Use existing feature check

[Enric Balletbo i Serra <enric.balletbo@collabora.com>]

Hi Prashant,

Thank you for your patch.

On 2/8/21 20:47, Prashant Malani wrote:
> Replace the cros_typec_feature_supported() function with the
> pre-existing cros_ec_check_features() function which does the same
> thing.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
>  drivers/platform/chrome/cros_ec_typec.c | 33 +++++++++----------------
>  1 file changed, 11 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index 27c068c4c38d..f96af8aa31b5 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -1054,24 +1054,6 @@ static int cros_typec_get_cmd_version(struct cros_typec_data *typec)
>  	return 0;
>  }
>  
> -/* Check the EC feature flags to see if TYPEC_* features are supported. */
> -static int cros_typec_feature_supported(struct cros_typec_data *typec, enum ec_feature_code feature)
> -{
> -	struct ec_response_get_features resp = {};
> -	int ret;
> -
> -	ret = cros_typec_ec_command(typec, 0, EC_CMD_GET_FEATURES, NULL, 0,
> -				    &resp, sizeof(resp));
> -	if (ret < 0) {
> -		dev_warn(typec->dev,
> -			 "Failed to get features, assuming typec feature=%d unsupported.\n",
> -			 feature);
> -		return 0;
> -	}
> -
> -	return resp.flags[feature / 32] & EC_FEATURE_MASK_1(feature);
> -}
> -
>  static void cros_typec_port_work(struct work_struct *work)
>  {
>  	struct cros_typec_data *typec = container_of(work, struct cros_typec_data, port_work);
> @@ -1113,6 +1095,7 @@ MODULE_DEVICE_TABLE(of, cros_typec_of_match);
>  
>  static int cros_typec_probe(struct platform_device *pdev)
>  {
> +	struct cros_ec_dev *ec_dev = NULL;
>  	struct device *dev = &pdev->dev;
>  	struct cros_typec_data *typec;
>  	struct ec_response_usb_pd_ports resp;
> @@ -1132,10 +1115,16 @@ static int cros_typec_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	typec->typec_cmd_supported = !!cros_typec_feature_supported(typec,
> -					EC_FEATURE_TYPEC_CMD);
> -	typec->needs_mux_ack = !!cros_typec_feature_supported(typec,
> -					EC_FEATURE_TYPEC_MUX_REQUIRE_AP_ACK);
> +	if (typec->ec->ec)

Is this check really needed. Can typec->ec->ec be NULL at this point?

> +		ec_dev = dev_get_drvdata(&typec->ec->ec->dev);
> +
> +	if (ec_dev) {

and this?

> +		typec->typec_cmd_supported = !!cros_ec_check_features(ec_dev, EC_FEATURE_TYPEC_CMD);
> +		typec->needs_mux_ack = !!cros_ec_check_features(ec_dev,
> +							EC_FEATURE_TYPEC_MUX_REQUIRE_AP_ACK);
> +	} else {

and this?

> +		dev_warn(dev, "Invalid cros_ec_dev pointer; feature flags not checked.\n");

Can't just be

		typec->typec_cmd_supported = !!cros_ec_check_features(ec_dev,
EC_FEATURE_TYPEC_CMD);
		typec->needs_mux_ack = !!cros_ec_check_features(ec_dev,
EC_FEATURE_TYPEC_MUX_REQUIRE_AP_ACK);

Thanks,
 Enric

> +	}
>  
>  	ret = cros_typec_ec_command(typec, 0, EC_CMD_USB_PD_PORTS, NULL, 0,
>  				    &resp, sizeof(resp));
>