Re: Add support for PEAK PCMCIA PCAN-PC card

[Wolfgang Grandegger <wg@grandegger.com>]

Hi Stephane,

On 01/11/2012 03:11 PM, Grosjean Stephane wrote:
> 
> 
> 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?

I think "PEAK PCAN PC-Card" is already pretty clear.

>> 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...

I agree. Let's wait and see.


>>>> +    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.

Yes, #defines, file names and prefixes should be consistently used.

>> +
>> +#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.

Well, it's me who pushed that driver to mainline ;-). Anyway, the prefix
is bad but not worth to prepare or reject a patch.

Wolfgang.