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

Marian-Cristian Rotariu
2020-02-20T11:20:39Z

Hi!

> -----Original Message-----
> From: Pavel Machek <pavel@denx.de>
> Sent: 19 February 2020 08:13
> To: Marian-Cristian Rotariu <marian-cristian.rotariu.rb@bp.renesas.com>
> Cc: cip-dev@lists.cip-project.org
> Subject: Re: [cip-dev] [PATCH 4.19.y-cip 15/23] usb: typec: driver for TI
> HD3SS3220 USB Type-C DRP port controller
>
> 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?

I think Biju followed the overall indications from:
Documentation/timers-howto.txt (.rst)

According to the documentation the inflection point is at 10us, so this is kind
of a grey area. Just to put it in context, the RZ/G2E has 2x1.2Ghz ARM Cortex A53.
Therefore this is kind of a high-performance embedded platform. At least, we
like to advertise it like that 😊

So, I am inclined to keep this as it is if you agree.

Cheers,
Marian.
_______________________________________________
cip-dev mailing list
cip-dev@lists.cip-project.org