RE: [PATCH RFC] c_can_pci: generic module for c_can on PCI

[Bhupesh SHARMA]

Hi,

> -----Original Message-----
> From: rubini@xxxxxxxxx [mailto:rubini@xxxxxxxxx]
> Sent: Tuesday, June 05, 2012 10:20 PM
> To: anilkumar@xxxxxx
> Cc: mkl@xxxxxxxxxxxxxx; Bhupesh SHARMA; federico.vaga@xxxxxxxxx;
> alan@xxxxxxxxxxxxxxxxxxx; wg@xxxxxxxxxxxxxx; Giancarlo ASNAGHI;
> alan@xxxxxxxxxxxxxxx; linux-can@xxxxxxxxxxxxxxx;
> netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI
> 
> > I am late to the discussion, is there any specific reason to maintain
> a
> > separate platform file (c_can_pci.c).
> 
> Because it depends on pci and ifdef is bad.
> 
> > I think 90% of the code is copied from c_can_paltform.c, code
> > changes will be less if you merge to existing c_can platform driver.
> 
> Yes, but then we need to ifdef around, which merges two bad files
> into a single but worse file.
> 
> But since the only current user of c_can is the platform device, why
> not merging the platform with the core and having pci just register a
> platform device?  The only problem I see is that we need cooperation,
> because neither me nor federico have a c_can equipped board besides
> the pci one.
> 

I can see examples of where different platform files are present for SJA CAN controller
as well depending on the underlying bus being used: OpenFirmware, ISA, PCI, etc..,
whilst there is a single core file there as well 'sja1000.c'

[1] Kvaser PCI platform driver, using services exposed by sja1000 core:
	http://lxr.linux.no/linux+v3.4.1/drivers/net/can/sja1000/kvaser_pci.c

[2] EMS PCI platform driver, using services exposed by sja1000 core:
	http://lxr.linux.no/linux+v3.4.1/drivers/net/can/sja1000/ems_pci.c

[3] SJA1000 core:
	http://lxr.linux.no/linux+v3.4.1/drivers/net/can/sja1000/sja1000.c

Here each platform driver has its own version of register read/write routine implementation.
The C_CAN approach is similar to that used by SJA1000. Instead of merging the "platform with the core",
I would instead suggest to have two separate platform drivers (for each bus type) and invoke common
routines kept in say another file 'c_can_platform_common.c', thus insuring that there is no code
duplicity and we have a clean hierarchical structure as well. So we can have:
	- Core file, c_can.c
	- Common platform file, c_can_platform_common.c
	- Platform file, c_can_platform.c, c_can_pci.c, etc..

This ensures that nothing breaks at the end of the existing C_CAN users and we have a clean
file structure as well.

Ofcourse, Wolfgang has a better idea of this structure, as he defined the same for SJA1000 and I 
consulted with him on this, while he was reviewing my initial C_CAN patch set. I will let him and Marc
comment further on my proposal. Your comments are also most welcome :)

Regards,
Bhupesh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/