On Sat, Mar 08, 2008 at 09:08:26PM +0100, matthieu castet wrote:
quoted text
> Hi Matthew,
>=20
> thanks for your comments
>=20
> Matthew Dharm wrote:
> >Why are you using an initializer instead of a new protocol code?
> Because using a new protocol code means I need to patch all the place=20
> where there is a comparison between us->subclass and US_SC_SCSI.
> After all I am US_SC_SCSI with a special case for ATA12 & ATA16 commands.
> I don't translate all scsi to atacb (that's what does US_SC_ISD200).

Yet, you call invoke_transport directly, just like any other protocol
handler.

The proper way to do this is as a separate protocol handler.  If you want
to make it clear that you are only intercepting a couple of command types,
then don't call invoke_transport() directly, call the transparent scsi
protocol handler (which, of course, does the same thing but provides
clearer layering).

Oh, and you should add some "unlikely" tags to these if() conditionals.

quoted text
> >Actually, why do you even have a separate 'dispatcher' function?  Why not
> >just one protocol handler function which checks the command at the top a=
nd
quoted text
> >calls invoke_transport there?
> What do you means by having a separate 'dispatcher' function?
> You means why I have 2 functions emulate_pass_thru_with_atacb and=20
> usb_stor_transparent_scsi_command_atacb ?
> I did 2 functions for having a code more clean.
>=20
> You suggest something like
> void usb_stor_transparent_scsi_command_atacb(struct scsi_cmnd *srb,
>                        struct us_data *us)
> {
>     if (srb->cmnd[0] !=3D ATA_16 && srb->cmnd[0] !=3D ATA_12) {
>         usb_stor_invoke_transport(srb, us);
>         return;
>     }
>     copy emulate_pass_thru_with_atacb code here
> }

Yes, modulo my comment above about calling the transparent scsi protocol
handler instead of invoke_transport directly.

quoted text
> >Also, unless ATACB is a new standard (and I don't think it is, as the
> >Cypress datasheet uses the term 'vendor specific'), then your functions
> >need renaming.  Instead of 'emulate_pass_thru_with_atacb', how about
> >something like 'cypress_atacb' --  since it's already a protocol handler,
> >everyone already knows it's for passing commands.
> But 'emulate_pass_thru_with_atacb' only handle ATA pass_thru scsi=20
> commands. It doesn't translate all scsi commands to atacb like=20
> 'cypress_atacb' could suggest.
> That's why I put 'usb_stor_transparent_scsi_command_atacb' saying it is=
=20
quoted text
> transparent_scsi_command + atacb support.

Yes, but your name suggests that ATACB is a new industry standard which is
implemented by more than a few chips from one specific vendor.  That's not
acceptable.

Try 'cypress_atacb_passthrough' instead?

Matt

--=20
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.=
net=20
Maintainer, Linux USB Mass Storage Driver

P:  Nine more messages in admin.policy.
M: I know, I'm typing as fast as I can!
					-- Pitr and Mike
User Friendly, 11/27/97