Differential D20630

ACPI support for USB , mainly hub.
ClosedPublic
Actions

Authored by takawata on Jun 13 2019, 3:41 PM.

Details

Reviewers

• hselasky
jhb
imp
hrs

Commits

rS349161: Add ACPI support for USB driver.

Summary

ACPI supports USB related features.
This patch will do

detects USB root hub if host controller has ACPI handle.
Enumulate ACPI handles of USB ports and attach to device driver structure.
Evaluate _UPC and _PLD objects under the ACPI objects of USB ports and parse them.

I think this patch is premature for merge, but I want to hear your opinion.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

takawata created this revision.Jun 13 2019, 3:41 PM

Harbormaster completed remote builds in B24839: Diff 58589.Jun 13 2019, 3:41 PM

I've not looked deeply into this, but it seems to my limited understanding in the right direction, though I'll defer to hps@ should his opinion differ.
Just noted a couple of nits, plus there's some minor style issues with some of the code that we can get into once other comments are made to the source.

sys/dev/usb/usb_hub_acpi.c
8 ↗	(On Diff #58589)	The project template now omits "all rights reserved," and it's preferred that new code not have that unless the author really wants it. It is your choice, though.

#!/bin/sh

#
# This script should be run on every USB C- and H- file before
# commit to enforce correct style.
#

[ -z "$1" ] && (echo "Please specify a filename.") && exit

#
# The "-ta" option is an extension to indent.
# Contact hselasky@freebsd.org to receive a
# patch that adds support for this option to
# indent !
#

#
# sed -E 's/&\(([^, \)]*)\)/\&\1/' |
#

for F in $*
do

echo "Now styling $F"

(cat $F | indent -Toss_mixerinfo -TFILE -Tu_char -Tu_int -Tu_long \
 -TTAILQ_HEAD -TLIST_HEAD -TTAILQ_ENTRY -TLIST_ENTRY \
 -TSTAILQ_HEAD -TSTAILQ_ENTRY \
 -Tu_short -Tfd_set -ta -st -bad -bap -nbbb -nbc -br -nbs \
 -c41 -cd41 -cdb -ce -ci4 -cli0 -d0 -di8 -ndj -ei -nfc1 \
 -nfcb -i8 -ip8 -l79 -lc77 -ldi0 -nlp -npcs -psl -sc \
 -nsob -nv | 
 sed -e "s/_HEAD [(]/_HEAD(/g" |
 sed -e "s/_ENTRY [(]/_ENTRY(/g" |
 sed -e "s/     __packed/ __packed/g" | 
 sed -e "s/     __aligned/ __aligned/g" |
 sed -e "s/^#define /#define    /g") > temp

(diff temp $F > /dev/null) || (cp temp $F)

done

Can you pass the new files through the style script above?

^^^ only .c and .h files

• hselasky added inline comments.Jun 13 2019, 4:22 PM

sys/dev/usb/usb_hub_acpi.c
110 ↗	(On Diff #58589)	remove this commented line
124 ↗	(On Diff #58589)	/* Found */
153 ↗	(On Diff #58589)	Spelling error Propritary

Resolve comments.

Harbormaster completed remote builds in B24845: Diff 58609.Jun 14 2019, 3:35 AM

• hselasky added inline comments.Jun 14 2019, 9:55 AM

sys/dev/usb/usb_hub_acpi.c
79 ↗	(On Diff #58609)	Remove one empty line.
84 ↗	(On Diff #58609)	Insert an empty line before static UINT32 ...
104 ↗	(On Diff #58609)	uint8_t nports;
131 ↗	(On Diff #58609)	Add empty line before static int
145 ↗	(On Diff #58609)	Indent this line properly.
204 ↗	(On Diff #58609)	Wrap long line.
252 ↗	(On Diff #58609)	Can you move the curly bracket at the end of the line to the next line.
261 ↗	(On Diff #58609)	Address is 64-bit? Use cast when comparing. devinfo->Address <= (uint64_t)sc->nports)
262 ↗	(On Diff #58609)	Looks off-by-one: sc->porthandle[devinfo->Address - 1] = ah;
293 ↗	(On Diff #58609)	/* success prior than non - acpi hub */
318 ↗	(On Diff #58609)	Use device_printf() ??
347 ↗	(On Diff #58609)	This print does not look needed.
414 ↗	(On Diff #58609)	Remove this empty line.
425 ↗	(On Diff #58609)	Remove this empty line.
446 ↗	(On Diff #58609)	Remove one empty line here.
sys/dev/usb/usb_hub_private.h
37 ↗	(On Diff #58609)	Insert empty line before UHUB_INTR_INTERVAL. Also use TAB after #define, before UHUB_INTR_INTERVAL.
sys/modules/usb/usb/Makefile
42 ↗	(On Diff #58609)	Will this build for all platforms? ARM, RISC, MIPS, SPARC ...