Re: [PATCH 4.19.y-cip 15/23] usb: typec: driver for TI HD3SS3220 USB Type-C DRP port controller

From: Pavel Machek <pavel@denx.de>
To: Marian-Cristian Rotariu <marian-cristian.rotariu.rb@bp.renesas.com>
Cc: cip-dev@lists.cip-project.org
Subject: Re: [PATCH 4.19.y-cip 15/23] usb: typec: driver for TI HD3SS3220 USB Type-C DRP port controller
Date: Wed, 19 Feb 2020 09:12:34 +0100	[thread overview]
Message-ID: <20200219081234.GD31996@amd> (raw)
In-Reply-To: <1582034720-5249-16-git-send-email-marian-cristian.rotariu.rb@bp.renesas.com>

[-- Attachment #1: Type: text/plain, Size: 1270 bytes --]

Hi!

> From: Biju Das <biju.das@bp.renesas.com>
> 
> commit 1c48c759ef4bb9031b3347277f04484e07e27d97 upstream.
> 
> Driver for TI HD3SS3220 USB Type-C DRP port controller.
> 
> The driver currently registers the port and supports data role
> swapping.

Ok, so this has few problems that are fixed by subsequent patches, but
I believe I see some more:

> +static enum usb_role hd3ss3220_get_attached_state(struct hd3ss3220 *hd3ss3220)
> +{
> +	unsigned int reg_val;
> +	enum usb_role attached_state;
> +	int ret;
> +
> +	ret = regmap_read(hd3ss3220->regmap, HD3SS3220_REG_CN_STAT_CTRL,
> +			  &reg_val);
> +	if (ret < 0)
> +		return ret;

This function claims to return "enum usb_role", but here it returns
errno from regmap_read.

> +static int hd3ss3220_dr_set(const struct typec_capability *cap,
> +			    enum typec_data_role role)
> +{
...
> +	ret = hd3ss3220_set_source_pref(hd3ss3220, pref);
> +	usleep_range(10, 100);

Would udelay() make more sense here? Are your CPUs / timer subsystem
so fast that sleeping for 10usec is possible and reasonable to do?

Best regards,
							Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]