Re: Add support for PEAK PCMCIA PCAN-PC card

[Grosjean Stephane <s.grosjean@peak-system.com>]

Le 10/01/2012 16:05, Oliver Hartkopp a écrit :
> On 10.01.2012 14:41, Wolfgang Grandegger wrote:
>
>> Nice, I have such a pcmcia card...
>
> Yes. Me too :-)

Happy to please you :-))

>
> +config CAN_PEAK_PCMCIA
> +	tristate "PEAK PCAN PC-CARD Card"
>
> tristate "PEAK PCAN PC-Card (PCMCIA)"
>
> ??

Please confirm: you'd like me to add the "(PCMCIA)" string at the end of 
the menu text, wouldn't you?

> Should we probably put all PCMCIA cards into a separate
>
> linux/drivers/net/can/sja1000/pcmcia
>
> directory - like we have it for the USB devices?

If my Humble Opinion may help you: not sure this will be lots of other 
pcmcia (can) adapters in the future...

>>> +	depends on PCMCIA
>>> +	---help---
>>> +	  This driver is for the PCAN PC-CARD PCMCIA card with 1 or 2 channels
>>> +	  from PEAK Systems (http://www.peak-system.com).
>>> +
>>>   config CAN_PEAK_PCI
>>>   	tristate "PEAK PCAN PCI/PCIe Cards"
>>>   	depends on PCI
>>> diff --git a/drivers/net/can/sja1000/Makefile b/drivers/net/can/sja1000/Makefile
>>> index 0604f24..b3d05cb 100644
>>> --- a/drivers/net/can/sja1000/Makefile
>>> +++ b/drivers/net/can/sja1000/Makefile
>>> @@ -9,6 +9,7 @@ obj-$(CONFIG_CAN_SJA1000_OF_PLATFORM) += sja1000_of_platform.o
>>>   obj-$(CONFIG_CAN_EMS_PCMCIA) += ems_pcmcia.o
>>>   obj-$(CONFIG_CAN_EMS_PCI) += ems_pci.o
>>>   obj-$(CONFIG_CAN_KVASER_PCI) += kvaser_pci.o
>>> +obj-$(CONFIG_CAN_PEAK_PCMCIA) += peak_pcmcia.o
>
> This should be the module/driver name too.
>>> +
>>> +/* PEAK-System PCMCIA driver name */
>>> +#define DRV_NAME "peak_pccard"
>> I find the mix of "peak_pcmcia" and "peak_pccard" and "pcan_pccard" for
>> the same hardware confusing. Please use just one name and prefix.
>
> Yep - peak_pcmcia

We just finished to talk about that and, as far as the PCAN-PC Card is a 
PC-CARD (16bit), we'd like to use the "peak_pccard" text for the 
module/module file/source file names instead of "peak_pcmcia".
Related question: I suppose I'll have to change the 
"CONFIG_CAN_PEAK_PCMCIA" into "CONFIG_CAN_PEAK_PCCARD" too.
Please confirm.

> +
> +#define DRV_CHAN_MAX 2
> +
> +#define DRV_CAN_CLOCK (16000000 / 2)
>> Hm, the prefix DRV_ here and below does not really make sense. But well,
>> it's a minor issue.
>
> ack. Remove DRV_ or rename it so someting like to PCC_

Ok, changed to "PCC_" prefix.

> No more nitpicks in additions to Wolfgangs remarks.

Just one last question: FYI, I built the peak_pcmcia.c starting from 
peak_pci.c, so I suppose that the DRV_ prefix should be changed into 
that file too.

> Tnx&  best regards,
> Oliver

Many thanks for all!

Best regards,

Stéphane