* RE: [PATCH 7/7] CCISS: run through Lindent
2006-06-15 15:42 ` [PATCH 7/7] CCISS: run through Lindent Bjorn Helgaas
@ 2006-06-15 16:01 ` Cameron, Steve
2006-06-15 16:28 ` Miller, Mike (OS Dev)
1 sibling, 0 replies; 4+ messages in thread
From: Cameron, Steve @ 2006-06-15 16:01 UTC (permalink / raw)
To: Helgaas, Bjorn, Miller, Mike (OS Dev)
Cc: ISS StorageDev, linux-kernel, Andrew Morton
Sometimes there are new devices that need device specific
handling (e.g. max number of commands, no. of SG elements, etc
are bigger or smaller) and an old driver on a new board
might do bad things. Currently, you can run an old driver
on a new board by explictly telling it to make the attempt
via a command line option, iirc, but having it do it always
and without user prodding it to do so is less safe.
-- steve
-----Original Message-----
From: Helgaas, Bjorn
Sent: Thu 6/15/2006 10:42 AM
To: Miller, Mike (OS Dev)
Cc: ISS StorageDev; linux-kernel@vger.kernel.org; Andrew Morton
Subject: Re: [PATCH 7/7] CCISS: run through Lindent
On Thursday 15 June 2006 08:49, Miller, Mike (OS Dev) wrote
> > -----Original Message-----
> > From: Helgaas, Bjorn
> > Sent: Wednesday, June 14, 2006 6:14 PM
> > To: Miller, Mike (OS Dev)
> > Cc: ISS StorageDev; linux-kernel@vger.kernel.org; Andrew Morton
> > Subject: [PATCH 7/7] CCISS: run through Lindent
> >
> > cciss is full of inconsistent style ("for (" vs. "for(", lines that
> > end with whitespace, lines beginning with a mix of spaces &
> > tabs, etc).
> >
> > This patch changes only whitespace.
> >
> > Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
>
> I agree that we had lots of whitespace garbage and inconsistent styles
> in the driver but I'm not sure I like all the indentation being removed
> from the product table. It makes it a bit harder to read, IMO.
If you are OK with the patch other than the product table, how about
if you apply the patch as-is, and I post a follow-on patch to fix the
indentation?
I'm contemplating more than just a white-space change, and it'd
probably be better to keep the "indent" patch to be white-space
changes only.
The cciss_pci_device_id[] table lists a bunch of subvendor and
subdevice IDs. Is there any reason to check those sub-IDs
explicitly? In other words, we currently list:
{PCI_VENDOR_ID_COMPAQ, PCI_DEVICE_ID_COMPAQ_CISS,
0x0E11, 0x4070, 0, 0, 0},
Could we replace that with:
{PCI_VENDOR_ID_COMPAQ, PCI_DEVICE_ID_COMPAQ_CISS,
PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
This would potentially make the driver claim additional devices.
But do COMPAQ/CISS devices with sub-IDs other than the listed ones
really exist anyway?
Bjorn
^ permalink raw reply [flat|nested] 4+ messages in thread* RE: [PATCH 7/7] CCISS: run through Lindent
2006-06-15 15:42 ` [PATCH 7/7] CCISS: run through Lindent Bjorn Helgaas
2006-06-15 16:01 ` Cameron, Steve
@ 2006-06-15 16:28 ` Miller, Mike (OS Dev)
1 sibling, 0 replies; 4+ messages in thread
From: Miller, Mike (OS Dev) @ 2006-06-15 16:28 UTC (permalink / raw)
To: Helgaas, Bjorn; +Cc: ISS StorageDev, linux-kernel, Andrew Morton
> -----Original Message-----
> From: Helgaas, Bjorn
> Sent: Thursday, June 15, 2006 10:43 AM
> To: Miller, Mike (OS Dev)
> Cc: ISS StorageDev; linux-kernel@vger.kernel.org; Andrew Morton
> Subject: Re: [PATCH 7/7] CCISS: run through Lindent
>
> On Thursday 15 June 2006 08:49, Miller, Mike (OS Dev) wrote
> > > -----Original Message-----
> > > From: Helgaas, Bjorn
> > > Sent: Wednesday, June 14, 2006 6:14 PM
> > > To: Miller, Mike (OS Dev)
> > > Cc: ISS StorageDev; linux-kernel@vger.kernel.org; Andrew Morton
> > > Subject: [PATCH 7/7] CCISS: run through Lindent
> > >
> > > cciss is full of inconsistent style ("for (" vs. "for(",
> lines that
> > > end with whitespace, lines beginning with a mix of spaces & tabs,
> > > etc).
> > >
> > > This patch changes only whitespace.
> > >
> > > Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> >
> > I agree that we had lots of whitespace garbage and
> inconsistent styles
> > in the driver but I'm not sure I like all the indentation being
> > removed from the product table. It makes it a bit harder to
> read, IMO.
>
> If you are OK with the patch other than the product table,
> how about if you apply the patch as-is, and I post a
> follow-on patch to fix the indentation?
>
> I'm contemplating more than just a white-space change, and
> it'd probably be better to keep the "indent" patch to be
> white-space changes only.
Yes, please submit a follow-on for the indentation.
>
> The cciss_pci_device_id[] table lists a bunch of subvendor
> and subdevice IDs. Is there any reason to check those
> sub-IDs explicitly? In other words, we currently list:
>
> {PCI_VENDOR_ID_COMPAQ, PCI_DEVICE_ID_COMPAQ_CISS,
> 0x0E11, 0x4070, 0, 0, 0},
>
> Could we replace that with:
>
> {PCI_VENDOR_ID_COMPAQ, PCI_DEVICE_ID_COMPAQ_CISS,
> PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
>
> This would potentially make the driver claim additional devices.
> But do COMPAQ/CISS devices with sub-IDs other than the listed
> ones really exist anyway?
Please note that we have several pci ids:
{PCI_VENDOR_ID_COMPAQ, PCI_DEVICE_ID_COMPAQ_CISS[ABCD],
0x0E11, 0x4070, 0, 0, 0},
The pci id identifies the controller family such as 53xx, 64xx, etc. We
use the subsystem pci id to identify the particular controller like 641,
642, etc.
I had added this to the driver we release on hp.com:
{ PCI_VENDOR_ID_HP, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
PCI_CLASS_STORAGE_RAID << 8, 0xffff << 8, 0},
to bind to Smart Array controllers for which I don't have an id. Seems
to work okay but then I need another method for displaying product name,
etc. There are also additional fields in the products struct on which we
rely. Things like number of s/g's and number of outstanding commands,
for instance. These are supposed to be in the firmware's config table
but we find they lie to us most of the time. :)
Lots of words, not much meaning.
mikem
>
> Bjorn
>
^ permalink raw reply [flat|nested] 4+ messages in thread