On Thu, Oct 01, 2020 at 11:20:08AM -0800, Mychaela Falconia wrote: > Hello esteemed linux-usb folks, > > While I am trying to get a little quirk for my custom hardware device > added to the ftdi_sio driver, I noticed an inconsistency between use > of interface->altsetting vs. interface->cur_altsetting in the existing > driver code. Specifically: > > * ftdi_determine_type() function uses this construct to get the number > of the interface it is operating on: > > inter = serial->interface->altsetting->desc.bInterfaceNumber; > > * ftdi_set_max_packet_size() uses interface->cur_altsetting to get to > desc.bNumEndpoints and endpoint[i].desc > > * The JTAG cleanup patch which Johan just applied uses > intf->cur_altsetting->desc.bInterfaceNumber Yeah, I noticed that too. > So which is the right one to use, intf->altsetting or intf->cur_altsetting? > The comments in include/linux/usb.h say that the altsetting member > points to an array of struct usb_host_interface stored in no particular > order, so using that pointer in ftdi_determine_type() seems wrong - > but then I am a total novice in this area, hence I am hoping that > someone more knowledgeable could confirm. As long as you only access bNumInterfaces, which by definition is identical for all altsettings, it's not wrong per se. But in general, drivers should use cur_altsetting to not have to worry about such subtleties and as a safe guard in case further fields are ever needed. > The most recent version of my DUART28C quirk adding patch uses > serial->interface->altsetting, copied from ftdi_determine_type() - > should I change my proposed patch to use cur_altsetting instead? Yes, please use that for new code. > Should I also make a patch to ftdi_determine_type() changing it to use > cur_altsetting as well? Sure; it's not needed for correctness, but let's do it for consistency. Johan
