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

Pavel Machek
2020-02-20T15:46:54Z

[-- Attachment #1.1: Type: text/plain, Size: 1617 bytes --]

Hi!

> > > +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.

######### HERE

> > > +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.

It is not a big deal to keep it. What about the "returning -errno but
return type is enum" issue above? (Marked with "HERE").

Best regards,
Pavel

--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
cip-dev mailing list
cip-dev@lists.cip-project.org