On Tue, 17 May 2011, Jan Andersson wrote:
> >> + /* Probe to see if we have a big or little endian controller. Bit 7
> >> + * of PORTSC is always 1 and 15:13 are always zero, so we have:
> >> + *
> >> + * 1------- 000----- -------- 000----- => Little endian mode HC
> >> + * 000----- 1------- 000----- -------- => Big endian mode HC
> >> + *
> >> + * We also need to take into account that readl will byte swap.
> >> + *
> >> + * If we detect a big endian register interface we also assume that
> >> + * the controller uses big endian descriptors.
> >> + */
> >> + if (!(uhci_readl(uhci, USBPORTSC1) & 0x80)) {
> >> + uhci->big_endian_mmio = 1;
> >> + uhci->big_endian_desc = 1;
> >> + }
> >
> > Sorry for not noticing this earlier. This is a 16-bit register, so
> > shouldn't you be accessing it with uhci_readw() instead of
> > uhci_readl()?
>
> The hardware allows reading both registers in one go with a 32-bit
> access so the code above works. I used a 32-bit access since I had used
> the same technique for a debug monitor where I was limited to 32-bit
> accesses.
It still seems like a strange thing to do, and a bad precedent. If
some other hardware that does not permit a 32-bit access comes along,
people won't be able to copy your code.
> > Then the comment could be shortened too, since only the first two byte
> > values on each line would be needed and the line about readl could go
> > away.
>
> You are right that the comment could be shortened, but readw also
> byteswaps so the remark about readl would just be changed to readw :-)
Not really. Changing the remark to refer to readw makes no sense,
because the whole point of that code is to test whether the 0x80 bit
ends up being set, in order to see whether or not the bytes were
swapped.
In fact, I would have written the whole comment differently.
Something like this:
/*
* Probe to determine the endianness of the controller.
* We know that bit 7 of the PORTSC1 register is always set
* and bit 15 is always clear. If uhci_readw() yields a value
* with bit 7 (0x80) turned on then the current little-endian
* setting is correct. Otherwise we assume the value was
* byte-swapped; hence the register interface and presumably
* also the descriptors are big-endian.
*/
> > Apart from this minor detail, the patch series is fine with me. You
> > can resubmit this last patch if you want to fix it up, and Greg can add
> >
> > Acked-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> >
> > to patches 1/5, 3/5, and 5/5.
> >
>
> Thank you. I want to test if I change to readw so that I don't make a
> mistake with all the swaps that take place. I will resubmit this after
> testing tomorrow - unless you can accept the use of readl() above.
It's entirely up to you. Since the code works okay either way, I don't
care very much.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
