| Message ID | 1338816766-7089-2-git-send-email-federico.vaga@gmail.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On 06/04/2012 03:32 PM, Federico Vaga wrote: > Signed-off-by: Federico Vaga <federico.vaga@gmail.com> > Acked-by: Giancarlo Asnaghi <giancarlo.asnaghi@st.com> > Cc: Alan Cox <alan@linux.intel.com> Please port you driver to the recent c_can changes. Use the c_can branch of the linux-can-next repo[1] as base for your work. You have to rework the register access function. Please have a look if there are devm_ variants for the registration/mapping of the pci and clock. [1] https://gitorious.org/linux-can/linux-can-next More comments inline. Marc > --- > drivers/net/can/c_can/Kconfig | 11 +- > drivers/net/can/c_can/Makefile | 1 + > drivers/net/can/c_can/c_can_pci.c | 221 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 230 insertions(+), 3 deletions(-) > create mode 100644 drivers/net/can/c_can/c_can_pci.c > > diff --git a/drivers/net/can/c_can/Kconfig b/drivers/net/can/c_can/Kconfig > index ffb9773..74ef97d 100644 > --- a/drivers/net/can/c_can/Kconfig > +++ b/drivers/net/can/c_can/Kconfig > @@ -2,14 +2,19 @@ menuconfig CAN_C_CAN > tristate "Bosch C_CAN devices" > depends on CAN_DEV && HAS_IOMEM > > -if CAN_C_CAN please keep the if CAN_C_CAN... > - > config CAN_C_CAN_PLATFORM > tristate "Generic Platform Bus based C_CAN driver" > + depends on CAN_C_CAN ...then you don't have to add the depends on here. > ---help--- > This driver adds support for the C_CAN chips connected to > the "platform bus" (Linux abstraction for directly to the > processor attached devices) which can be found on various > boards from ST Microelectronics (http://www.st.com) > like the SPEAr1310 and SPEAr320 evaluation boards. > -endif ... Just move you pci driver inside the if...endif block... > + > +config CAN_C_CAN_PCI > + tristate "Generic PCI Bus based C_CAN driver" > + depends on CAN_C_CAN ...and remove the depends on CAN_C_CAN. You probably have to add a depends on PCI. > + ---help--- > + This driver adds support for the C_CAN chips connected to > + the PCI bus. > diff --git a/drivers/net/can/c_can/Makefile b/drivers/net/can/c_can/Makefile > index 9273f6d..ad1cc84 100644 > --- a/drivers/net/can/c_can/Makefile > +++ b/drivers/net/can/c_can/Makefile > @@ -4,5 +4,6 @@ > > obj-$(CONFIG_CAN_C_CAN) += c_can.o > obj-$(CONFIG_CAN_C_CAN_PLATFORM) += c_can_platform.o > +obj-$(CONFIG_CAN_C_CAN_PCI) += c_can_pci.o > > ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG > diff --git a/drivers/net/can/c_can/c_can_pci.c b/drivers/net/can/c_can/c_can_pci.c > new file mode 100644 > index 0000000..b635375 > --- /dev/null > +++ b/drivers/net/can/c_can/c_can_pci.c > @@ -0,0 +1,221 @@ > +/* > + * Platform CAN bus driver for Bosch C_CAN controller > + * > + * Copyright (C) 2012 Federico Vaga <federico.vaga@gmail.com> > + * ^^^ double space :) > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + */ > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/netdevice.h> > +#include <linux/clk.h> > +#include <linux/pci.h> > +#include <linux/can/dev.h> > + > +#include "c_can.h" > + > +enum c_can_pci_reg_align { > + C_CAN_REG_ALIGN_16, > + C_CAN_REG_ALIGN_32, > +}; > + > +struct c_can_pci_data { > + unsigned int reg_align; /* Set the register alignment in the memory */ ^^^^^^^^^^^^ use the enum you defined above. > + unsigned int freq; /* Set the frequency if clk is not usable */ > +}; > + > +/* > + * 16-bit c_can registers can be arranged differently in the memory > + * architecture of different implementations. For example: 16-bit > + * registers can be aligned to a 16-bit boundary or 32-bit boundary etc. > + * Handle the same by providing a common read/write interface. > + */ > +static u16 c_can_pci_read_reg_aligned_to_16bit(struct c_can_priv *priv, > + void *reg) > +{ > + return readw(reg); > +} > + > +static void c_can_pci_write_reg_aligned_to_16bit(struct c_can_priv *priv, > + void *reg, u16 val) > +{ > + writew(val, reg); > +} > + > +static u16 c_can_pci_read_reg_aligned_to_32bit(struct c_can_priv *priv, > + void *reg) > +{ > + return readw(reg + (long)reg - (long)priv->regs); > +} > + > +static void c_can_pci_write_reg_aligned_to_32bit(struct c_can_priv *priv, > + void *reg, u16 val) > +{ > + writew(val, reg + (long)reg - (long)priv->regs); > +} > + > +static int __devinit c_can_pci_probe(struct pci_dev *pdev, > + const struct pci_device_id *ent) > +{ > + struct c_can_pci_data *c_can_pci_data = (void *)ent->driver_data; > + struct c_can_priv *priv; > + struct net_device *dev; > + void __iomem *addr; > + struct clk *clk; > + int ret; > + > + ret = pci_enable_device(pdev); > + if (ret) { > + dev_err(&pdev->dev, "pci_enable_device FAILED\n"); > + goto out; > + } > + > + ret = pci_request_regions(pdev, KBUILD_MODNAME); > + if (ret) { > + dev_err(&pdev->dev, "pci_request_regions FAILED\n"); > + goto out_disable_device; > + } > + > + pci_set_master(pdev); > + pci_enable_msi(pdev); > + > + addr = pci_iomap(pdev, 0, pci_resource_len(pdev, 0)); > + if (!addr) { > + dev_err(&pdev->dev, > + "device has no PCI memory resources, " > + "failing adapter\n"); > + ret = -ENOMEM; > + goto out_release_regions; > + } > + > + /* allocate the c_can device */ > + dev = alloc_c_can_dev(); > + if (!dev) { > + ret = -ENOMEM; > + goto out_iounmap; > + } > + > + priv = netdev_priv(dev); > + pci_set_drvdata(pdev, dev); > + SET_NETDEV_DEV(dev, &pdev->dev); > + > + dev->irq = pdev->irq; > + priv->regs = addr; > + > + if (!c_can_pci_data->freq) { > + /* get the appropriate clk */ > + clk = clk_get(&pdev->dev, NULL); > + if (IS_ERR(clk)) { > + dev_err(&pdev->dev, "no clock defined\n"); > + ret = -ENODEV; > + goto out_free_c_can; > + } > + priv->can.clock.freq = clk_get_rate(clk); > + priv->priv = clk; > + } else { > + priv->can.clock.freq = c_can_pci_data->freq; > + priv->priv = NULL; > + } > + > + switch (c_can_pci_data->reg_align) { > + case C_CAN_REG_ALIGN_32: > + priv->read_reg = c_can_pci_read_reg_aligned_to_32bit; > + priv->write_reg = c_can_pci_write_reg_aligned_to_32bit; > + break; > + case C_CAN_REG_ALIGN_16: > + default: > + priv->read_reg = c_can_pci_read_reg_aligned_to_16bit; > + priv->write_reg = c_can_pci_write_reg_aligned_to_16bit; > + break; > + } > + > + ret = register_c_can_dev(dev); > + if (ret) { > + dev_err(&pdev->dev, "registering %s failed (err=%d)\n", > + KBUILD_MODNAME, ret); > + goto out_free_clock; > + } > + > + dev_info(&pdev->dev, "%s device registered (regs=%p, irq=%d)\n", > + KBUILD_MODNAME, priv->regs, dev->irq); > + > + return 0; > + > +out_free_clock: > + if (!priv->priv) ^^^ looks fishy > + clk_put(priv->priv); > +out_free_c_can: > + pci_set_drvdata(pdev, NULL); > + free_c_can_dev(dev); > +out_iounmap: > + pci_iounmap(pdev, addr); > +out_release_regions: > + pci_disable_msi(pdev); > + pci_clear_master(pdev); > + pci_release_regions(pdev); > +out_disable_device: > + /* > + * do not call pci_disable_device on sta2x11 because it > + * break all other Bus masters on this EP > + */ > + if(pdev->vendor == PCI_VENDOR_ID_STMICRO && > + pdev->device == PCI_DEVICE_ID_STMICRO_CAN) > + goto out; > + pci_disable_device(pdev); > +out: > + return ret; > +} > + > +static void __devexit c_can_pci_remove(struct pci_dev *pdev) > +{ > + struct net_device *dev = pci_get_drvdata(pdev); > + struct c_can_priv *priv = netdev_priv(dev); > + > + pci_set_drvdata(pdev, NULL); > + free_c_can_dev(dev); > + if (!priv->priv) dito > + clk_put(priv->priv); > + pci_iounmap(pdev, priv->regs); > + pci_disable_msi(pdev); > + pci_clear_master(pdev); > + pci_release_regions(pdev); > + /* > + * do not call pci_disable_device on sta2x11 because it > + * break all other Bus masters on this EP > + */ > + if(pdev->vendor == PCI_VENDOR_ID_STMICRO && > + pdev->device == PCI_DEVICE_ID_STMICRO_CAN) > + return; > + pci_disable_device(pdev); > +} > + > +static struct c_can_pci_data c_can_sta2x11= { > + .reg_align = C_CAN_REG_ALIGN_32, > + .freq = 52000000, /* 52 Mhz */ > +}; > + > +#define C_CAN_ID(_vend, _dev, _driverdata) { \ > + PCI_DEVICE(_vend, _dev), \ > + .driver_data = (unsigned long)&_driverdata, \ > +} > +DEFINE_PCI_DEVICE_TABLE(c_can_pci_tbl) = { ^^^^ static? > + C_CAN_ID(PCI_VENDOR_ID_STMICRO, PCI_DEVICE_ID_STMICRO_CAN, > + c_can_sta2x11), > + {}, > +}; > +static struct pci_driver sta2x11_pci_driver = { > + .name = KBUILD_MODNAME, > + .id_table = c_can_pci_tbl, > + .probe = c_can_pci_probe, > + .remove = __devexit_p(c_can_pci_remove), > +}; > + > +module_pci_driver(sta2x11_pci_driver); > + > +MODULE_AUTHOR("Federico Vaga <federico.vaga@gmail.com>"); > +MODULE_LICENSE("GPL V2"); IIRC, the correct case is "GPL v2" > +MODULE_DESCRIPTION("PCI CAN bus driver for Bosch C_CAN controller"); > +MODULE_DEVICE_TABLE(pci, c_can_pci_tbl);
> +enum c_can_pci_reg_align { > + C_CAN_REG_ALIGN_16, > + C_CAN_REG_ALIGN_32, > +}; Anythign wrong with bool aligned32; > + > +struct c_can_pci_data { > + unsigned int reg_align; /* Set the register alignment in the memory */ Not the enum .. indeed > +static u16 c_can_pci_read_reg_aligned_to_16bit(struct c_can_priv *priv, > + void *reg) I'm a bit worried this function name might be too short ;) > + dev_info(&pdev->dev, "%s device registered (regs=%p, irq=%d)\n", > + KBUILD_MODNAME, priv->regs, dev->irq); dev_dbg > + * do not call pci_disable_device on sta2x11 because it > + * break all other Bus masters on this EP > + */ > + if(pdev->vendor == PCI_VENDOR_ID_STMICRO && > + pdev->device == PCI_DEVICE_ID_STMICRO_CAN) > + goto out; Is that the disabling or the dropping it into D3. We have a PCI quirk flag for the latter. See "quirk_no_ata_d3". That will also avoid any accidents elsewhere. Right now the quirk has "ata" in the name but the ata is just historically because we had to quirk various disk controllers. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Anythign wrong with > > bool aligned32; I personally think booleans are evil. But both this and the other thing: >> +static u16 c_can_pci_read_reg_aligned_to_16bit(struct c_can_priv *priv, >> + void *reg) > > I'm a bit worried this function name might be too short ;) come from the platform driver this is based on (I already blamed federico offlist for not preserving authorship of the original file). So, this file is mostly copied from the platform driver, which is a duplication of code. A mandated duplication, given how the thing is currently laid out: the c_can core driver exports functions that the other two files are using (the platform and the new pci driver). In my opinion, it would be much better to have one less layer and no exports at all. The core driver should be a platform driver, and the pci driver would just build platform data and register the platform device. Sure this isn't up to federico, who has the pci device but cannot access any boards where the previous driver is used. What do the maintainers think? I (or federico :) may propose a reshaping, if the idea makes sense. /alessandro, another user of the sta2x11 where c_can_pci lives -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > +static u16 c_can_pci_read_reg_aligned_to_16bit(struct c_can_priv > > *priv, + void *reg) > > I'm a bit worried this function name might be too short ;) I know :) I was inspired by the same function in c_can_platform.c About these function I suggest to move them into c_can.c because they are the same for c_can_platform.c and c_can_pci.c Then add a new field c_can_priv->offset which can be used to shift the register offset coherently with the memory alignment. Finally, remove c_can_priv- >read_reg and c_can_priv->write_reg and use internal c_can.c function to read and write registers. static u16 c_can_read_reg(struct c_can_priv *priv, enum reg index) { return readw(priv->base + (priv->regs[index] << priv->offset)); } static void c_can_write_reg(struct c_can_priv *priv, enum reg index, u16 val) { writew(val, priv->base + (priv->regs[index] << priv->offset)); } If it's ok, I can made a patch for this in the next days. > > + * do not call pci_disable_device on sta2x11 because it > > + * break all other Bus masters on this EP > > + */ > > + if(pdev->vendor == PCI_VENDOR_ID_STMICRO && > > + pdev->device == PCI_DEVICE_ID_STMICRO_CAN) > > + goto out; > > Is that the disabling or the dropping it into D3. We have a PCI quirk > flag for the latter. See "quirk_no_ata_d3". That will also avoid any > accidents elsewhere. Right now the quirk has "ata" in the name but the > ata is just historically because we had to quirk various disk > controllers. We are investigating if this is still necessary on the current version of the board.
> -----Original Message----- > From: linux-can-owner@vger.kernel.org [mailto:linux-can- > owner@vger.kernel.org] On Behalf Of Federico Vaga > Sent: Monday, June 04, 2012 10:16 PM > To: Alan Cox > Cc: Wolfgang Grandegger; Marc Kleine-Budde; Giancarlo ASNAGHI; Alan > Cox; Alessandro Rubini; linux-can@vger.kernel.org; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI > > > > +static u16 c_can_pci_read_reg_aligned_to_16bit(struct c_can_priv > > > *priv, + void *reg) > > > > I'm a bit worried this function name might be too short ;) > > I know :) I was inspired by the same function in c_can_platform.c There was a purpose to keeping these long function names when I wrote the c_can_platform driver initially. These were kept to support the SoCs (even the flaky ones) which I could trace at that time and used C_CAN controllers (e.g. Hynix, ST's SPEAr eMPUs, etc..) and had different register bank layouts. In some of these SoC's the C_CAN registers which are essentially 16-bit or 32-bit registers are aligned always to a 32-bit boundary (i.e. even a 16-bit register is aligned to 32-bit boundary). So, I had to implement two variants of the read/write reg routines. I am not sure your SoC implementation needs them. If it does, I will categorize it as flaky as well :) > About these function I suggest to move them into c_can.c because they > are the same for c_can_platform.c and c_can_pci.c Then add a new field > c_can_priv->offset which can be used to shift the register offset > coherently with the memory alignment. Finally, remove c_can_priv- > >read_reg and c_can_priv->write_reg and use internal c_can.c function > to > read and write registers. See above. There was a reason for keeping these routines in c_can_platform.c Simply put, every platform having a Bosch C_CAN module can have it's own implementation of the bus (for example you use PCI) and register bank layout (16-bit or 32-bit aligned). I would suggest to keep the same arrangement. > static u16 c_can_read_reg(struct c_can_priv *priv, enum reg index) > { > return readw(priv->base + (priv->regs[index] << priv->offset)); > } > static void c_can_write_reg(struct c_can_priv *priv, enum reg index, > u16 val) > { > writew(val, priv->base + (priv->regs[index] << priv->offset)); > } > > > If it's ok, I can made a patch for this in the next days. > [snip..] Regards, Bhupesh -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> In some of these SoC's the C_CAN registers which are essentially > 16-bit or 32-bit registers are aligned always to a 32-bit boundary > (i.e. even a 16-bit register is aligned to 32-bit boundary). > > So, I had to implement two variants of the read/write reg routines. I > am not sure your SoC implementation needs them. If it does, I will > categorize it as flaky as well :) My implementation is align to 32, but I'm trying to make a generic PCI wrapper (some other could be aligned to 16) > See above. There was a reason for keeping these routines in > c_can_platform.c Simply put, every platform having a Bosch C_CAN > module can have it's own implementation of the bus (for example you > use PCI) and register bank layout (16-bit or 32-bit aligned). I don't understand the reason to keep these functions in c_can_platform.c . Two generic read/write functions could be written into c_can.c by using a shift value (0 if aligned to 16, 1 if aligned to 32) as I showed in the previous mail: > > static u16 c_can_read_reg(struct c_can_priv *priv, enum reg index) > > { > > > > return readw(priv->base + (priv->regs[index] << priv->offset)); > > > > } > > static void c_can_write_reg(struct c_can_priv *priv, enum reg index, > > > > u16 val) > > > > { > > > > writew(val, priv->base + (priv->regs[index] << priv->offset)); > > > > } Every platform having a Bosch C_CAN/D_CAN can specify its shift value (0 or 1) and it's done.
> -----Original Message----- > From: Federico Vaga [mailto:federico.vaga@gmail.com] > Sent: Tuesday, June 05, 2012 4:49 PM > To: Bhupesh SHARMA > Cc: Alan Cox; Wolfgang Grandegger; Marc Kleine-Budde; Giancarlo > ASNAGHI; Alan Cox; Alessandro Rubini; linux-can@vger.kernel.org; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI > > > In some of these SoC's the C_CAN registers which are essentially > > 16-bit or 32-bit registers are aligned always to a 32-bit boundary > > (i.e. even a 16-bit register is aligned to 32-bit boundary). > > > > So, I had to implement two variants of the read/write reg routines. I > > am not sure your SoC implementation needs them. If it does, I will > > categorize it as flaky as well :) > > My implementation is align to 32, but I'm trying to make a generic PCI > wrapper (some other could be aligned to 16) So it means your implementation is also flaky and you are probably wasting HW memory space while integrating the Bosch C_CAN module in your SoC :) > > See above. There was a reason for keeping these routines in > > c_can_platform.c Simply put, every platform having a Bosch C_CAN > > module can have it's own implementation of the bus (for example you > > use PCI) and register bank layout (16-bit or 32-bit aligned). > > I don't understand the reason to keep these functions in > c_can_platform.c . Two generic read/write functions could be written > into c_can.c by using a shift value (0 if aligned to 16, 1 if aligned > to > 32) as I showed in the previous mail: > > > > static u16 c_can_read_reg(struct c_can_priv *priv, enum reg index) > > > { > > > > > > return readw(priv->base + (priv->regs[index] << priv->offset)); > > > > > > } > > > static void c_can_write_reg(struct c_can_priv *priv, enum reg > index, > > > > > > u16 val) > > > > > > { > > > > > > writew(val, priv->base + (priv->regs[index] << priv->offset)); > > > > > > } > > Every platform having a Bosch C_CAN/D_CAN can specify its shift value > (0 > or 1) and it's done. > I am not a big fan of adding platform specific flakes in any core file, that why we keep the platform file separate from the core ones. But I will left Marc and Wolfgang to further comment on the same. Regards, Bhupesh -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> My implementation is align to 32, but I'm trying to make a generic PCI >> wrapper (some other could be aligned to 16) > So it means your implementation is also flaky and you are probably > wasting HW memory space while integrating the Bosch C_CAN module in > your SoC :) Then I may say _your_ implementation is flaky because it wastes one bit in the address decoder and a lot of logic gates in the data bus. It's normal to align registers at 32 bits, as it's simpler and faster. Most SoCs have only 32-bit aligned registers, for a reason. > I am not a big fan of adding platform specific flakes in any core > file, that why we keep the platform file separate from the core > ones. A number of other drivers have a shift parameter, because it's very common for the hardware integrator to feel free to choose the easiest wiring for the device. The choice to keep the platform driver separate from the core driver only adds complication in my opinion: you need to export 4 symbols and yhen every user must duplicate code (like federico is replicating theplatform driver in the pci driver). I'd really prefer to have the core driver be a platform driver, and the others just add platform data to describe how it is wired. That's actually the reason why the platform bus exists. > But I will left Marc and Wolfgang to further comment on the same. I agree: let them decide. /alessandro -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/05/2012 03:13 PM, Alessandro Rubini wrote: >>> My implementation is align to 32, but I'm trying to make a generic PCI >>> wrapper (some other could be aligned to 16) > >> So it means your implementation is also flaky and you are probably >> wasting HW memory space while integrating the Bosch C_CAN module in >> your SoC :) > > Then I may say _your_ implementation is flaky because it wastes one > bit in the address decoder and a lot of logic gates in the data > bus. It's normal to align registers at 32 bits, as it's simpler and > faster. Most SoCs have only 32-bit aligned registers, for a reason. > >> I am not a big fan of adding platform specific flakes in any core >> file, that why we keep the platform file separate from the core >> ones. > > A number of other drivers have a shift parameter, because it's very > common for the hardware integrator to feel free to choose the easiest > wiring for the device. The choice to keep the platform driver > separate from the core driver only adds complication in my opinion: > you need to export 4 symbols and yhen every user must duplicate code > (like federico is replicating theplatform driver in the pci driver). > > I'd really prefer to have the core driver be a platform driver, and > the others just add platform data to describe how it is wired. That's > actually the reason why the platform bus exists. > >> But I will left Marc and Wolfgang to further comment on the same. > > I agree: let them decide. I personally like the "pci device sets up a platform device" idea. My question is, is this considered being a good practise? Marc
> -----Original Message----- > From: rubini@gnudd.com [mailto:rubini@gnudd.com] > Sent: Tuesday, June 05, 2012 6:44 PM > To: Bhupesh SHARMA > Cc: federico.vaga@gmail.com; alan@lxorguk.ukuu.org.uk; > wg@grandegger.com; mkl@pengutronix.de; Giancarlo ASNAGHI; > alan@linux.intel.com; linux-can@vger.kernel.org; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI > > >> My implementation is align to 32, but I'm trying to make a generic > PCI > >> wrapper (some other could be aligned to 16) > > > So it means your implementation is also flaky and you are probably > > wasting HW memory space while integrating the Bosch C_CAN module in > > your SoC :) > > Then I may say _your_ implementation is flaky because it wastes one > bit in the address decoder and a lot of logic gates in the data > bus. It's normal to align registers at 32 bits, as it's simpler and > faster. Most SoCs have only 32-bit aligned registers, for a reason. You missed my original point. I mentioned in my first mail itself, that I studied a few SoCs integrating the C_CAN module from Bosch before writing the driver. Not all have aligned their register space to a 32-bit boundary. My platform driver still supports them. This _does_ not imply that our SoC has/may have the same problem :) Each SoC designer can have his/her own different view on this sort of implementation. The platform driver was written to support both the implementations (SW is supposed to support all sort of HW design constraints :) ). > > I am not a big fan of adding platform specific flakes in any core > > file, that why we keep the platform file separate from the core > > ones. > > A number of other drivers have a shift parameter, because it's very > common for the hardware integrator to feel free to choose the easiest > wiring for the device. The choice to keep the platform driver > separate from the core driver only adds complication in my opinion: > you need to export 4 symbols and yhen every user must duplicate code > (like federico is replicating theplatform driver in the pci driver). > > I'd really prefer to have the core driver be a platform driver, and > the others just add platform data to describe how it is wired. That's > actually the reason why the platform bus exists. > > > But I will left Marc and Wolfgang to further comment on the same. > > I agree: let them decide. Sure.. Regards, Bhupesh -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> I personally like the "pci device sets up a platform device" idea. Good. Than me or federico will submit a proposal. > My question is, is this considered being a good practise? I don't think there are many pci bridges around, but platform drivers exists just for that reason: to be instantiated when you know how the wiring ("platform") details. I.e., somebody registers the platform device associated to the driver. Sometimes the platform device is compiled in, sometimes it comes from the device tree. I think it can come from PCI as well. thanks /alessandro, apologizing with Bhupesh Sharma for his tone in the previous mail -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Alessandro/Federico, On Tue, Jun 05, 2012 at 19:00:13, Alessandro Rubini wrote: > > I personally like the "pci device sets up a platform device" idea. > > Good. Than me or federico will submit a proposal. > I am late to the discussion, is there any specific reason to maintain a separate platform file (c_can_pci.c). 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. You can look at D_CAN integration to C_CAN driver. [1] https://gitorious.org/linux-can/linux-can-next Regards AnilKumar-- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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. thanks /alessandro -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, > -----Original Message----- > From: rubini@gnudd.com [mailto:rubini@gnudd.com] > Sent: Tuesday, June 05, 2012 10:20 PM > To: anilkumar@ti.com > Cc: mkl@pengutronix.de; Bhupesh SHARMA; federico.vaga@gmail.com; > alan@lxorguk.ukuu.org.uk; wg@grandegger.com; Giancarlo ASNAGHI; > alan@linux.intel.com; linux-can@vger.kernel.org; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org > 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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
How we proceed? I submit my c_can_pci.c as a separated module, we create a c_can_platform_common.c, or we are thinking about a generic c_can.c as plaftorm driver?
On 06/04/2012 06:45 PM, Alessandro Rubini wrote: >> Anythign wrong with >> >> bool aligned32; > > I personally think booleans are evil. But both this and the other > thing: > >>> +static u16 c_can_pci_read_reg_aligned_to_16bit(struct c_can_priv *priv, >>> + void *reg) >> >> I'm a bit worried this function name might be too short ;) > > come from the platform driver this is based on (I already blamed > federico offlist for not preserving authorship of the original file). > > So, this file is mostly copied from the platform driver, which is a > duplication of code. A mandated duplication, given how the thing > is currently laid out: the c_can core driver exports functions that > the other two files are using (the platform and the new pci driver). > > In my opinion, it would be much better to have one less layer and no > exports at all. The core driver should be a platform driver, and the > pci driver would just build platform data and register the platform > device. Do you have examples for that approach? Not sure yet if it really saves code and makes it more readable. > Sure this isn't up to federico, who has the pci device but cannot > access any boards where the previous driver is used. What do the > maintainers think? I (or federico :) may propose a reshaping, if > the idea makes sense. I would suggest to provide the c_can_pci driver using the *current* API, even if it's not optimal. Federicos patch then already looks quite good. It should use the new register access methods introduced by the D_CAN support patch, though. Any further improvements to the device abstraction and a more consistent handling of the platform data are welcome. Wolfgang. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Federico, here comes my late review. Mark and others have already commented and I will focus on further improvements... On 06/04/2012 03:32 PM, Federico Vaga wrote: A few more words would be nice here. > Signed-off-by: Federico Vaga <federico.vaga@gmail.com> > Acked-by: Giancarlo Asnaghi <giancarlo.asnaghi@st.com> > Cc: Alan Cox <alan@linux.intel.com> > --- > drivers/net/can/c_can/Kconfig | 11 +- > drivers/net/can/c_can/Makefile | 1 + > drivers/net/can/c_can/c_can_pci.c | 221 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 230 insertions(+), 3 deletions(-) > create mode 100644 drivers/net/can/c_can/c_can_pci.c > > diff --git a/drivers/net/can/c_can/Kconfig b/drivers/net/can/c_can/Kconfig > index ffb9773..74ef97d 100644 > --- a/drivers/net/can/c_can/Kconfig > +++ b/drivers/net/can/c_can/Kconfig > @@ -2,14 +2,19 @@ menuconfig CAN_C_CAN > tristate "Bosch C_CAN devices" > depends on CAN_DEV && HAS_IOMEM > > -if CAN_C_CAN > - Please don't change unrelated things. It's done that way also in other CAN subdirectories. > config CAN_C_CAN_PLATFORM > tristate "Generic Platform Bus based C_CAN driver" > + depends on CAN_C_CAN > ---help--- > This driver adds support for the C_CAN chips connected to > the "platform bus" (Linux abstraction for directly to the > processor attached devices) which can be found on various > boards from ST Microelectronics (http://www.st.com) > like the SPEAr1310 and SPEAr320 evaluation boards. > -endif > + > +config CAN_C_CAN_PCI > + tristate "Generic PCI Bus based C_CAN driver" > + depends on CAN_C_CAN > + ---help--- > + This driver adds support for the C_CAN chips connected to > + the PCI bus. > diff --git a/drivers/net/can/c_can/Makefile b/drivers/net/can/c_can/Makefile > index 9273f6d..ad1cc84 100644 > --- a/drivers/net/can/c_can/Makefile > +++ b/drivers/net/can/c_can/Makefile > @@ -4,5 +4,6 @@ > > obj-$(CONFIG_CAN_C_CAN) += c_can.o > obj-$(CONFIG_CAN_C_CAN_PLATFORM) += c_can_platform.o > +obj-$(CONFIG_CAN_C_CAN_PCI) += c_can_pci.o > > ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG > diff --git a/drivers/net/can/c_can/c_can_pci.c b/drivers/net/can/c_can/c_can_pci.c > new file mode 100644 > index 0000000..b635375 > --- /dev/null > +++ b/drivers/net/can/c_can/c_can_pci.c > @@ -0,0 +1,221 @@ > +/* > + * Platform CAN bus driver for Bosch C_CAN controller s /Platform/PCI/ ? > + * > + * Copyright (C) 2012 Federico Vaga <federico.vaga@gmail.com> > + * > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + */ > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/netdevice.h> > +#include <linux/clk.h> > +#include <linux/pci.h> > +#include <linux/can/dev.h> > + > +#include "c_can.h" > + > +enum c_can_pci_reg_align { > + C_CAN_REG_ALIGN_16, > + C_CAN_REG_ALIGN_32, > +}; > + > +struct c_can_pci_data { > + unsigned int reg_align; /* Set the register alignment in the memory */ Should be "enum c_can_pci_reg_align" here. > + unsigned int freq; /* Set the frequency if clk is not usable */ > +}; > + > +/* > + * 16-bit c_can registers can be arranged differently in the memory > + * architecture of different implementations. For example: 16-bit > + * registers can be aligned to a 16-bit boundary or 32-bit boundary etc. > + * Handle the same by providing a common read/write interface. > + */ > +static u16 c_can_pci_read_reg_aligned_to_16bit(struct c_can_priv *priv, > + void *reg) > +{ > + return readw(reg); > +} > + > +static void c_can_pci_write_reg_aligned_to_16bit(struct c_can_priv *priv, > + void *reg, u16 val) > +{ > + writew(val, reg); > +} > + > +static u16 c_can_pci_read_reg_aligned_to_32bit(struct c_can_priv *priv, > + void *reg) > +{ > + return readw(reg + (long)reg - (long)priv->regs); > +} > + > +static void c_can_pci_write_reg_aligned_to_32bit(struct c_can_priv *priv, > + void *reg, u16 val) > +{ > + writew(val, reg + (long)reg - (long)priv->regs); > +} This will look better with the new register access methods. > +static int __devinit c_can_pci_probe(struct pci_dev *pdev, > + const struct pci_device_id *ent) > +{ > + struct c_can_pci_data *c_can_pci_data = (void *)ent->driver_data; > + struct c_can_priv *priv; > + struct net_device *dev; > + void __iomem *addr; > + struct clk *clk; > + int ret; > + > + ret = pci_enable_device(pdev); > + if (ret) { > + dev_err(&pdev->dev, "pci_enable_device FAILED\n"); > + goto out; > + } > + > + ret = pci_request_regions(pdev, KBUILD_MODNAME); > + if (ret) { > + dev_err(&pdev->dev, "pci_request_regions FAILED\n"); > + goto out_disable_device; > + } > + > + pci_set_master(pdev); > + pci_enable_msi(pdev); > + > + addr = pci_iomap(pdev, 0, pci_resource_len(pdev, 0)); > + if (!addr) { > + dev_err(&pdev->dev, > + "device has no PCI memory resources, " > + "failing adapter\n"); > + ret = -ENOMEM; > + goto out_release_regions; > + } > + > + /* allocate the c_can device */ > + dev = alloc_c_can_dev(); > + if (!dev) { > + ret = -ENOMEM; > + goto out_iounmap; > + } > + > + priv = netdev_priv(dev); > + pci_set_drvdata(pdev, dev); > + SET_NETDEV_DEV(dev, &pdev->dev); > + > + dev->irq = pdev->irq; > + priv->regs = addr; > + > + if (!c_can_pci_data->freq) { > + /* get the appropriate clk */ > + clk = clk_get(&pdev->dev, NULL); > + if (IS_ERR(clk)) { > + dev_err(&pdev->dev, "no clock defined\n"); > + ret = -ENODEV; > + goto out_free_c_can; > + } > + priv->can.clock.freq = clk_get_rate(clk); > + priv->priv = clk; > + } else { > + priv->can.clock.freq = c_can_pci_data->freq; > + priv->priv = NULL; > + } > + > + switch (c_can_pci_data->reg_align) { > + case C_CAN_REG_ALIGN_32: > + priv->read_reg = c_can_pci_read_reg_aligned_to_32bit; > + priv->write_reg = c_can_pci_write_reg_aligned_to_32bit; > + break; > + case C_CAN_REG_ALIGN_16: > + default: > + priv->read_reg = c_can_pci_read_reg_aligned_to_16bit; > + priv->write_reg = c_can_pci_write_reg_aligned_to_16bit; > + break; > + } > + > + ret = register_c_can_dev(dev); > + if (ret) { > + dev_err(&pdev->dev, "registering %s failed (err=%d)\n", > + KBUILD_MODNAME, ret); > + goto out_free_clock; > + } > + > + dev_info(&pdev->dev, "%s device registered (regs=%p, irq=%d)\n", > + KBUILD_MODNAME, priv->regs, dev->irq); > + > + return 0; > + > +out_free_clock: > + if (!priv->priv) > + clk_put(priv->priv); > +out_free_c_can: > + pci_set_drvdata(pdev, NULL); > + free_c_can_dev(dev); > +out_iounmap: > + pci_iounmap(pdev, addr); > +out_release_regions: > + pci_disable_msi(pdev); > + pci_clear_master(pdev); > + pci_release_regions(pdev); > +out_disable_device: > + /* > + * do not call pci_disable_device on sta2x11 because it > + * break all other Bus masters on this EP > + */ Puh! > + if(pdev->vendor == PCI_VENDOR_ID_STMICRO && > + pdev->device == PCI_DEVICE_ID_STMICRO_CAN) > + goto out; > + pci_disable_device(pdev); > +out: > + return ret; > +} > + > +static void __devexit c_can_pci_remove(struct pci_dev *pdev) > +{ > + struct net_device *dev = pci_get_drvdata(pdev); > + struct c_can_priv *priv = netdev_priv(dev); > + > + pci_set_drvdata(pdev, NULL); > + free_c_can_dev(dev); Should be moved a few line down. > + if (!priv->priv) > + clk_put(priv->priv); > + pci_iounmap(pdev, priv->regs); > + pci_disable_msi(pdev); > + pci_clear_master(pdev); > + pci_release_regions(pdev); > + /* > + * do not call pci_disable_device on sta2x11 because it > + * break all other Bus masters on this EP > + */ > + if(pdev->vendor == PCI_VENDOR_ID_STMICRO && > + pdev->device == PCI_DEVICE_ID_STMICRO_CAN) > + return; > + pci_disable_device(pdev); > +} > + > +static struct c_can_pci_data c_can_sta2x11= { > + .reg_align = C_CAN_REG_ALIGN_32, > + .freq = 52000000, /* 52 Mhz */ > +}; > + > +#define C_CAN_ID(_vend, _dev, _driverdata) { \ > + PCI_DEVICE(_vend, _dev), \ > + .driver_data = (unsigned long)&_driverdata, \ > +} > +DEFINE_PCI_DEVICE_TABLE(c_can_pci_tbl) = { > + C_CAN_ID(PCI_VENDOR_ID_STMICRO, PCI_DEVICE_ID_STMICRO_CAN, > + c_can_sta2x11), > + {}, > +}; > +static struct pci_driver sta2x11_pci_driver = { Why do you not use a generic name here? > + .name = KBUILD_MODNAME, > + .id_table = c_can_pci_tbl, > + .probe = c_can_pci_probe, > + .remove = __devexit_p(c_can_pci_remove), > +}; > + > +module_pci_driver(sta2x11_pci_driver); > + > +MODULE_AUTHOR("Federico Vaga <federico.vaga@gmail.com>"); > +MODULE_LICENSE("GPL V2"); > +MODULE_DESCRIPTION("PCI CAN bus driver for Bosch C_CAN controller"); > +MODULE_DEVICE_TABLE(pci, c_can_pci_tbl); Thanks for your contribution. Wolfgang. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/11/2012 03:18 PM, Federico Vaga wrote: > How we proceed? > I submit my c_can_pci.c as a separated module, we create a > c_can_platform_common.c, > or we are thinking about a generic c_can.c as plaftorm driver? I would accept your patch with the remaining fixes especially the new register access methods introduced by the D_CAN support patch recently. Any further improvements to the device abstraction and a more consistent handling of the platform data or register access should be addressed by sub-sequent patches. Wolfgang. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> In my opinion, it would be much better to have one less layer and no >> exports at all. The core driver should be a platform driver, and the >> pci driver would just build platform data and register the platform >> device. > > Do you have examples for that approach? Not sure yet if it really saves > code and makes it more readable. Maybe the physmap mtd driver is a good example. Everybody's using it (but not from PCI). I found drivers/pcmcia/bcm63xx_pcmcia.c that registers a platform driver from a pci probe function, but I'm sure there are other ones. OTOH, I have another example of how not to do stuff, but I won't point fingers now (it's not a CAN thing). I just think the platform bus is there just for this reason: to provide data to a generic driver, without module dependencies and such stuff. > I would suggest to provide the c_can_pci driver using the *current* API, > even if it's not optimal. Federicos patch then already looks quite good. > It should use the new register access methods introduced by the D_CAN > support patch, though. Great. When it's in I'll show my proposal as an RFC patch, as time permits, so we'll see if it's better or not. > Any further improvements to the device abstraction and a more consistent > handling of the platform data are welcome. Good to know, thanks a lot /alessandro -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > +out_free_clock: > > + if (!priv->priv) > > ^^^ > > looks fishy Also c_can_platform.c use priv->priv when it needs to get clk. I can add a comment to specify what the statement do.
On 06/12/2012 04:25 PM, Federico Vaga wrote: >>> +out_free_clock: >>> + if (!priv->priv) >> >> ^^^ >> >> looks fishy > > Also c_can_platform.c use priv->priv when it needs to get clk. I can add > a comment to specify what the statement do. > +out_free_clock: > + if (!priv->priv) > + clk_put(priv->priv); Why do you call clk_put on priv->priv, if priv->priv is NULL? Marc
> > +out_free_clock: > > + if (!priv->priv) > > + clk_put(priv->priv); > > Why do you call clk_put on priv->priv, if priv->priv is NULL? Right! if(priv->priv) clk_put(priv->priv);
diff --git a/drivers/net/can/c_can/Kconfig b/drivers/net/can/c_can/Kconfig index ffb9773..74ef97d 100644 --- a/drivers/net/can/c_can/Kconfig +++ b/drivers/net/can/c_can/Kconfig @@ -2,14 +2,19 @@ menuconfig CAN_C_CAN tristate "Bosch C_CAN devices" depends on CAN_DEV && HAS_IOMEM -if CAN_C_CAN - config CAN_C_CAN_PLATFORM tristate "Generic Platform Bus based C_CAN driver" + depends on CAN_C_CAN ---help--- This driver adds support for the C_CAN chips connected to the "platform bus" (Linux abstraction for directly to the processor attached devices) which can be found on various boards from ST Microelectronics (http://www.st.com) like the SPEAr1310 and SPEAr320 evaluation boards. -endif + +config CAN_C_CAN_PCI + tristate "Generic PCI Bus based C_CAN driver" + depends on CAN_C_CAN + ---help--- + This driver adds support for the C_CAN chips connected to + the PCI bus. diff --git a/drivers/net/can/c_can/Makefile b/drivers/net/can/c_can/Makefile index 9273f6d..ad1cc84 100644 --- a/drivers/net/can/c_can/Makefile +++ b/drivers/net/can/c_can/Makefile @@ -4,5 +4,6 @@ obj-$(CONFIG_CAN_C_CAN) += c_can.o obj-$(CONFIG_CAN_C_CAN_PLATFORM) += c_can_platform.o +obj-$(CONFIG_CAN_C_CAN_PCI) += c_can_pci.o ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG diff --git a/drivers/net/can/c_can/c_can_pci.c b/drivers/net/can/c_can/c_can_pci.c new file mode 100644 index 0000000..b635375 --- /dev/null +++ b/drivers/net/can/c_can/c_can_pci.c @@ -0,0 +1,221 @@ +/* + * Platform CAN bus driver for Bosch C_CAN controller + * + * Copyright (C) 2012 Federico Vaga <federico.vaga@gmail.com> + * + * This file is licensed under the terms of the GNU General Public + * License version 2. This program is licensed "as is" without any + * warranty of any kind, whether express or implied. + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/netdevice.h> +#include <linux/clk.h> +#include <linux/pci.h> +#include <linux/can/dev.h> + +#include "c_can.h" + +enum c_can_pci_reg_align { + C_CAN_REG_ALIGN_16, + C_CAN_REG_ALIGN_32, +}; + +struct c_can_pci_data { + unsigned int reg_align; /* Set the register alignment in the memory */ + unsigned int freq; /* Set the frequency if clk is not usable */ +}; + +/* + * 16-bit c_can registers can be arranged differently in the memory + * architecture of different implementations. For example: 16-bit + * registers can be aligned to a 16-bit boundary or 32-bit boundary etc. + * Handle the same by providing a common read/write interface. + */ +static u16 c_can_pci_read_reg_aligned_to_16bit(struct c_can_priv *priv, + void *reg) +{ + return readw(reg); +} + +static void c_can_pci_write_reg_aligned_to_16bit(struct c_can_priv *priv, + void *reg, u16 val) +{ + writew(val, reg); +} + +static u16 c_can_pci_read_reg_aligned_to_32bit(struct c_can_priv *priv, + void *reg) +{ + return readw(reg + (long)reg - (long)priv->regs); +} + +static void c_can_pci_write_reg_aligned_to_32bit(struct c_can_priv *priv, + void *reg, u16 val) +{ + writew(val, reg + (long)reg - (long)priv->regs); +} + +static int __devinit c_can_pci_probe(struct pci_dev *pdev, + const struct pci_device_id *ent) +{ + struct c_can_pci_data *c_can_pci_data = (void *)ent->driver_data; + struct c_can_priv *priv; + struct net_device *dev; + void __iomem *addr; + struct clk *clk; + int ret; + + ret = pci_enable_device(pdev); + if (ret) { + dev_err(&pdev->dev, "pci_enable_device FAILED\n"); + goto out; + } + + ret = pci_request_regions(pdev, KBUILD_MODNAME); + if (ret) { + dev_err(&pdev->dev, "pci_request_regions FAILED\n"); + goto out_disable_device; + } + + pci_set_master(pdev); + pci_enable_msi(pdev); + + addr = pci_iomap(pdev, 0, pci_resource_len(pdev, 0)); + if (!addr) { + dev_err(&pdev->dev, + "device has no PCI memory resources, " + "failing adapter\n"); + ret = -ENOMEM; + goto out_release_regions; + } + + /* allocate the c_can device */ + dev = alloc_c_can_dev(); + if (!dev) { + ret = -ENOMEM; + goto out_iounmap; + } + + priv = netdev_priv(dev); + pci_set_drvdata(pdev, dev); + SET_NETDEV_DEV(dev, &pdev->dev); + + dev->irq = pdev->irq; + priv->regs = addr; + + if (!c_can_pci_data->freq) { + /* get the appropriate clk */ + clk = clk_get(&pdev->dev, NULL); + if (IS_ERR(clk)) { + dev_err(&pdev->dev, "no clock defined\n"); + ret = -ENODEV; + goto out_free_c_can; + } + priv->can.clock.freq = clk_get_rate(clk); + priv->priv = clk; + } else { + priv->can.clock.freq = c_can_pci_data->freq; + priv->priv = NULL; + } + + switch (c_can_pci_data->reg_align) { + case C_CAN_REG_ALIGN_32: + priv->read_reg = c_can_pci_read_reg_aligned_to_32bit; + priv->write_reg = c_can_pci_write_reg_aligned_to_32bit; + break; + case C_CAN_REG_ALIGN_16: + default: + priv->read_reg = c_can_pci_read_reg_aligned_to_16bit; + priv->write_reg = c_can_pci_write_reg_aligned_to_16bit; + break; + } + + ret = register_c_can_dev(dev); + if (ret) { + dev_err(&pdev->dev, "registering %s failed (err=%d)\n", + KBUILD_MODNAME, ret); + goto out_free_clock; + } + + dev_info(&pdev->dev, "%s device registered (regs=%p, irq=%d)\n", + KBUILD_MODNAME, priv->regs, dev->irq); + + return 0; + +out_free_clock: + if (!priv->priv) + clk_put(priv->priv); +out_free_c_can: + pci_set_drvdata(pdev, NULL); + free_c_can_dev(dev); +out_iounmap: + pci_iounmap(pdev, addr); +out_release_regions: + pci_disable_msi(pdev); + pci_clear_master(pdev); + pci_release_regions(pdev); +out_disable_device: + /* + * do not call pci_disable_device on sta2x11 because it + * break all other Bus masters on this EP + */ + if(pdev->vendor == PCI_VENDOR_ID_STMICRO && + pdev->device == PCI_DEVICE_ID_STMICRO_CAN) + goto out; + pci_disable_device(pdev); +out: + return ret; +} + +static void __devexit c_can_pci_remove(struct pci_dev *pdev) +{ + struct net_device *dev = pci_get_drvdata(pdev); + struct c_can_priv *priv = netdev_priv(dev); + + pci_set_drvdata(pdev, NULL); + free_c_can_dev(dev); + if (!priv->priv) + clk_put(priv->priv); + pci_iounmap(pdev, priv->regs); + pci_disable_msi(pdev); + pci_clear_master(pdev); + pci_release_regions(pdev); + /* + * do not call pci_disable_device on sta2x11 because it + * break all other Bus masters on this EP + */ + if(pdev->vendor == PCI_VENDOR_ID_STMICRO && + pdev->device == PCI_DEVICE_ID_STMICRO_CAN) + return; + pci_disable_device(pdev); +} + +static struct c_can_pci_data c_can_sta2x11= { + .reg_align = C_CAN_REG_ALIGN_32, + .freq = 52000000, /* 52 Mhz */ +}; + +#define C_CAN_ID(_vend, _dev, _driverdata) { \ + PCI_DEVICE(_vend, _dev), \ + .driver_data = (unsigned long)&_driverdata, \ +} +DEFINE_PCI_DEVICE_TABLE(c_can_pci_tbl) = { + C_CAN_ID(PCI_VENDOR_ID_STMICRO, PCI_DEVICE_ID_STMICRO_CAN, + c_can_sta2x11), + {}, +}; +static struct pci_driver sta2x11_pci_driver = { + .name = KBUILD_MODNAME, + .id_table = c_can_pci_tbl, + .probe = c_can_pci_probe, + .remove = __devexit_p(c_can_pci_remove), +}; + +module_pci_driver(sta2x11_pci_driver); + +MODULE_AUTHOR("Federico Vaga <federico.vaga@gmail.com>"); +MODULE_LICENSE("GPL V2"); +MODULE_DESCRIPTION("PCI CAN bus driver for Bosch C_CAN controller"); +MODULE_DEVICE_TABLE(pci, c_can_pci_tbl);