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

Pavel Machek
2020-02-19T08:12:37Z

[-- 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 --]