Messages in this thread | | | Date | Mon, 7 Mar 2022 06:48:25 -0800 | From | Bjorn Andersson <> | Subject | Re: [PATCH v4 7/7] usb: typec: mux: Add On Semi fsa4480 driver |
| |
On Mon 07 Mar 02:16 PST 2022, Andy Shevchenko wrote:
> On Sun, Mar 06, 2022 at 07:40:40PM -0800, Bjorn Andersson wrote: > > The ON Semiconductor FSA4480 is a USB Type-C port multimedia switch with > > support for analog audio headsets. It allows sharing a common USB Type-C > > port to pass USB2.0 signal, analog audio, sideband use wires and analog > > microphone signal. > > > > Due to lacking upstream audio support for testing, the audio muxing is > > left untouched, but implementation of muxing the SBU lines is provided > > as a pair of Type-C mux and switch devices. This provides the necessary > > support for enabling the DisplayPort altmode on devices with this > > circuit. > > ... > > > +static const struct regmap_config fsa4480_regmap_config = { > > + .reg_bits = 8, > > + .val_bits = 8, > > + .max_register = FSA4480_MAX_REGISTER, > > +}; > > You are using mutex for accessing hardware. Do you still need a regmap lock? > If so, add a comment to explain why. >
I've not considered that before, but you're right, there doesn't seem to be any reason to keep the locking in the regmap.
> ... > > > + /* 15us to allow the SBU switch to turn off */ > > + usleep_range(15, 1000); > > This is quite unusual range. > > If you are fine with the long delay, why to stress the system on it? > Otherwise the use of 1000 is unclear. > > That said, I would expect one of the below: > > usleep_range(15, 30); > usleep_range(500, 1000); >
Glad you asked about that, as you say the typical form is to keep the range within 2x of the lower value, or perhaps lower + 5.
But if the purpose is to specify a minimum time and then give a max to give the system some flexibility in it's decision of when to wake up. And in situations such as this, we're talking about someone connecting a cable, so we're in "no rush" and I picked the completely arbitrary 1ms as the max.
Do you see any drawback of this much higher number? (Other than it looking "wrong")
> ... > > > + sw_desc.fwnode = dev->fwnode; > > Please, don't dereference for fwnode explicitly. Use dev_fwnode(). >
Okay, will update accordingly.
Thanks, Bjorn
> ... > > > + mux_desc.fwnode = dev->fwnode; > > Ditto. > > -- > With Best Regards, > Andy Shevchenko > >
| |