Re: [PATCH v3] peak_pci: add support for PEAK-System PCIe/PCIeC/miniPCI boards

[Wolfgang Grandegger <wg@grandegger.com>]

On 02/01/2012 01:29 PM, Stephane Grosjean wrote:
> Le 01/02/2012 13:02, Wolfgang Grandegger a écrit :
>> On 02/01/2012 12:02 PM, Stephane Grosjean wrote:
>>> This patch adds the support for the following 3x sja1000 based PCI
>>> boards
>>> from PEAK-System Technik (www.peak-system.com):
>>>
>>> PCAN-PCI Express (1 or 2 channels)
>>> PCAN-ExpressCard (1 or 2 channels)
>>> PCAN-miniPCI (1 or 2 channels)
>>>
>>> This version also updates Kconfig.
>>>
>>> Signed-off-by: Stephane Grosjean<s.grosjean@peak-system.com>
>>> ---
>>>   drivers/net/can/sja1000/Kconfig    |    7 +-
>>>   drivers/net/can/sja1000/peak_pci.c |  510
>>> +++++++++++++++++++++++++++++++++--
>>>   2 files changed, 484 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/drivers/net/can/sja1000/Kconfig
>>> b/drivers/net/can/sja1000/Kconfig
>>> index 36e9d59..d178e01 100644
>>> --- a/drivers/net/can/sja1000/Kconfig
>>> +++ b/drivers/net/can/sja1000/Kconfig
>>> @@ -44,11 +44,12 @@ config CAN_EMS_PCI
>>>         (http://www.ems-wuensche.de).
>>>
>>>   config CAN_PEAK_PCI
>>> -    tristate "PEAK PCAN PCI/PCIe Cards"
>>> +    tristate "PEAK PCAN-PCI/PCIe/PCIeC/miniPCI Cards"
>>>       depends on PCI
>> It now also depends on I2C.

To be more precise, it depends on "CONFIG_I2C_ALGOBIT".

> Yes you're right...but if I added such a dependency, I2C would be
> mandatory, even for the other PEAK PCI boards

Yes, and I was already thinking if it's a good idea...

> 
> I would prefer:
> 
> 1 - Changing Kconfig : like that
> 
> config CAN_PEAK_PCI
>         tristate "PEAK PCAN-PCI/PCIe/miniPCI Cards"
>         depends on PCI
>         ---help---
>           This driver is for the PCAN-PCI/PCIe/miniPCI cards
>           (1, 2, 3 or 4 channels) from PEAK-System Technik
>           (http://www.peak-system.com).
> 
>           The I2C driver must be selected to fully support the
>           PCAN-ExpressCard card LEDs.

Does the PCAN-ExpressCard card work without I2C? If yes, I would be
enough to use CONFIG_I2C_ALGOBIT directly around the relevant code.

> config CAN_PEAK_PCIEC
>         bool "PEAK PCAN-ExpressCard Card"
>         depends on CAN_PEAK_PCI && I2C
>         ---help---
>           Add full support of the PCAN-ExpressCard card
>           (1, 2, 3 or 4 channels) from PEAK-System Technik
>           (http://www.peak-system.com).
> 
> 
> 2 - adding some #ifdef CONFIG_PEAK_PCIEC/ #endif in peak_pci.c around
> all what concerns leds management.

Not only LED management but the *complete* support for the
PCAN-ExpressCard card, in case I2C is mandatory. Using a separate file,
e.g. peak_pciec.c would be nice, but I think it does not make the code
more readable. Therefore, I think a few #ifdef are tolerable.

Wolfgang.