Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 3 Aug 2021 10:29:43 -0700
From:   Prashant Malani <pmalani@chromium.org>
To:     Enric Balletbo i Serra <enric.balletbo@collabora.com>
Cc:     linux-kernel@vger.kernel.org, Benson Leung <bleung@chromium.org>,
        Guenter Roeck <groeck@chromium.org>
Subject: Re: [PATCH 2/2] platform/chrome: cros_ec_typec: Use existing feature
 check
Message-ID: <YQl9B2FKb6rKHq3Z@google.com>
References: <20210802184711.3872372-1-pmalani@chromium.org>
 <20210802184711.3872372-2-pmalani@chromium.org>
 <81610a2b-aa3f-f8d7-5214-e59a7ce839d6@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <81610a2b-aa3f-f8d7-5214-e59a7ce839d6@collabora.com>
Precedence: bulk

Hi Enric,

Thanks for reviewing the patch.

On Tue, Aug 03, 2021 at 12:09:47PM +0200, Enric Balletbo i Serra wrote:
> 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?

Looking at it closely, it looks like it can't be NULL
(cros_ec_register() fails if the platform device registration fails).

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

I haven't been able to prove this solely by looking at the code, hence
wanted to be defensive here. That said, in the ARM and x86 platforms I
tested this change on, it wasn't NULL.

> 
> > +		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);

Sure; I'll push another version with the NULL checks dropped.

Thanks,

-Prashant