Re: [patch, attach, RFC] usb-serial: ti_usb removing firmware

[Al Borchers <alborchers@steinerpoint.com>]

Oleg --

Quoting Oleg Verych <olecom@gmail.com>:

> On 12/16/06, Al Borchers <alborchers@steinerpoint.com> wrote:
> > * We can't remove the compiled in firmware--that would break things
> >   until users get and install the firmware images.
> >
> > * What I did in my patch at
> www.brimson.com/downloads/ti_usb_multitech-1.1.tgz
> >   is to use request_firmware for new devices, and, if the firmware files
> are
> >   present, for the existing 3410 and 5052 devices.  If the firmware files
> are
> >   not present for 3410 and 5052, then it uses the compiled in firmware.
>
> Please, see this as policy, not as the need ("this" here is no binary
> blobs in the linux
> sources && symlink your windowz firmware files in "/lib/firmware".
> Please see my last
> e-mail.) I can understand "all cases handling" in application
> programming, but not here.

I agree in principle.  That is why when I added new support for new
devices in the patch mentioned above, I did not put the firmware
images for those devices in the linux driver.

But the ti_usb_3410_5052 and several other usb-serial drivers have
had firmware images compiled in for years.  If we remove those compiled
in firmware images, and a user updates to the new kernel without installing
the firmware image files, their usb-serial converters will stop working.
That will cause a lot of problems.

>
> [...]
> > * The whitespace, 80 cols, dbg cleanup is great--maybe put that in a
> separate
> >   patch.
>
> emacs, and this is problem with linux/Makefile (my first ever
> code-patch for linux ;)

Emacs did the right thing--just separate the whitspace changes and 80
column changes into a separate patch and submit that separately.

>
> > * I would not remove the comments about the hotplug script until we
> >   can use the 2.6.19 usb_driver_set_configuration function.
>
> Please see my last e-mail [1]. And many useless code must be copies
> for every ID.

The script in the comments at the top of the file is needed to make
the driver work.  It must be installed as a udev script or hotplug
script.  It needs to be documented somewhere--maybe in
Documentation/usb/usb-serial.txt.

> > * -               status = -ENODEV;
> >   +               status = 0xFAC; /* greater than zero -- device is going
> down
> > */
> >   -               status = -ENODEV;
> >   +               status = 0x0FF;
> >
> >   Why 0xFAC and 0x0FF?  I will have to investigate and test this.  I think
> >   we want negative values.
>
> Please see my last e-mail [1]. Serial-converter device _is_ made to be
> that way, and
> configuration steps must be good steps. With error codes you did that
> as error, _two_
> I/O errors is very much for ordinary setup of the plugged-in device,
> do you see my point?

Yes.  I don't like the error messages that are generated, but when I
developed the driver this was necessary to get it to automatically switch
configurations with the hotplug script.  I will test positive return
values and see how they work now.

>
> > * +               dev_info(&dev->dev, "%s - Choose configuration #2,
> please.\n",
> >   +                        ti_usb_driver.name);
> >   +
> >
> >   This is what the hotplug script does automatically.  Drop this info
> message.
>
> Yes. (in my prev. e-mail [1]) I said, that most people can use bash
> and not udev.

Neither is ideal, and changing the config by hand is a good backup,
but a udev/hotplug script is automatic and I think that is better than
asking users to do it by hand each time they plug in the device.

> > * +#define TI_3410_EZ430_ID               0xF430  /* TI ez430 development
> tool
> > */
> >
> >   Where do you use this?
>
> Shiny new device from TI, don't you know? You've lost nothing MSP430
> Design Contest
> is over ;D www.designmsp430.com

Looks like a fun device--I'll have to get one.

Put this in the ti_id_table_3410 and ti_id_table_combined so the
device is recognized.

Thanks,
-- Al

>
> > Thanks,
> > -- Al
>
> Thank you very much, Al.
> But if debian guys do not against, i think it can be in the Etch,
> there isn't any crap in patch.
>
> [1] <http://article.gmane.org/gmane.linux.usb.devel/49186>
>
>  --
>  -o--=O`C
>    #oo'L O
>  <___=E M
>