LinuxLists.cc - [PATCH v2 0/6] can: sja1000: cleanups and new OF property

[permalink] [raw]

Subject: [PATCH v2 1/6] can: sja1000: remove unused defines

Remove unused defines for the OF platform.

Signed-off-by: Florian Vaussard <[email protected]>
---
drivers/net/can/sja1000/sja1000_of_platform.c | 3 ---
1 file changed, 3 deletions(-)

diff --git a/drivers/net/can/sja1000/sja1000_of_platform.c b/drivers/net/can/sja1000/sja1000_of_platform.c
index 047accd..2f29eb9 100644
--- a/drivers/net/can/sja1000/sja1000_of_platform.c
+++ b/drivers/net/can/sja1000/sja1000_of_platform.c
@@ -55,9 +55,6 @@ MODULE_LICENSE("GPL v2");

#define SJA1000_OFP_CAN_CLOCK (16000000 / 2)

-#define SJA1000_OFP_OCR OCR_TX0_PULLDOWN
-#define SJA1000_OFP_CDR (CDR_CBP | CDR_CLK_OFF)
-
static u8 sja1000_ofp_read_reg(const struct sja1000_priv *priv, int reg)
{
return ioread8(priv->reg_base + reg);
--
1.8.1.2

2014-01-31 10:35:37

[permalink] [raw]

Subject: [PATCH v2 4/6] can: sja1000: fuse of_platform into platform

The OpenFirmware probe can be merged into the standard platform
probe to leverage common code.

Signed-off-by: Florian Vaussard <[email protected]>
---
drivers/net/can/sja1000/Kconfig | 13 +-
drivers/net/can/sja1000/Makefile | 1 -
drivers/net/can/sja1000/sja1000_of_platform.c | 218 --------------------------
drivers/net/can/sja1000/sja1000_platform.c | 141 +++++++++++++----
4 files changed, 116 insertions(+), 257 deletions(-)
delete mode 100644 drivers/net/can/sja1000/sja1000_of_platform.c

diff --git a/drivers/net/can/sja1000/Kconfig b/drivers/net/can/sja1000/Kconfig
index ff2ba86..4b18b87 100644
--- a/drivers/net/can/sja1000/Kconfig
+++ b/drivers/net/can/sja1000/Kconfig
@@ -17,16 +17,9 @@ config CAN_SJA1000_PLATFORM
the "platform bus" (Linux abstraction for directly to the
processor attached devices). Which can be found on various
boards from Phytec (http://www.phytec.de) like the PCM027,
- PCM038.
-
-config CAN_SJA1000_OF_PLATFORM
- tristate "Generic OF Platform Bus based SJA1000 driver"
- depends on OF
- ---help---
- This driver adds support for the SJA1000 chips connected to
- the OpenFirmware "platform bus" found on embedded systems with
- OpenFirmware bindings, e.g. if you have a PowerPC based system
- you may want to enable this option.
+ PCM038. It also provides the OpenFirmware "platform bus" found
+ on embedded systems with OpenFirmware bindings, e.g. if you
+ have a PowerPC based system you may want to enable this option.

config CAN_EMS_PCMCIA
tristate "EMS CPC-CARD Card"
diff --git a/drivers/net/can/sja1000/Makefile b/drivers/net/can/sja1000/Makefile
index b3d05cb..531d5fc 100644
--- a/drivers/net/can/sja1000/Makefile
+++ b/drivers/net/can/sja1000/Makefile
@@ -5,7 +5,6 @@
obj-$(CONFIG_CAN_SJA1000) += sja1000.o
obj-$(CONFIG_CAN_SJA1000_ISA) += sja1000_isa.o
obj-$(CONFIG_CAN_SJA1000_PLATFORM) += sja1000_platform.o
-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
diff --git a/drivers/net/can/sja1000/sja1000_of_platform.c b/drivers/net/can/sja1000/sja1000_of_platform.c
deleted file mode 100644
index 2f29eb9..0000000
--- a/drivers/net/can/sja1000/sja1000_of_platform.c
+++ /dev/null
@@ -1,218 +0,0 @@
-/*
- * Driver for SJA1000 CAN controllers on the OpenFirmware platform bus
- *
- * Copyright (C) 2008-2009 Wolfgang Grandegger <[email protected]>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the version 2 of the GNU General Public License
- * as published by the Free Software Foundation
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software Foundation,
- * Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
- */
-
-/* This is a generic driver for SJA1000 chips on the OpenFirmware platform
- * bus found on embedded PowerPC systems. You need a SJA1000 CAN node
- * definition in your flattened device tree source (DTS) file similar to:
- *
- * can@3,100 {
- * compatible = "nxp,sja1000";
- * reg = <3 0x100 0x80>;
- * interrupts = <2 0>;
- * interrupt-parent = <&mpic>;
- * nxp,external-clock-frequency = <16000000>;
- * };
- *
- * See "Documentation/devicetree/bindings/net/can/sja1000.txt" for further
- * information.
- */
-
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include <linux/interrupt.h>
-#include <linux/netdevice.h>
-#include <linux/delay.h>
-#include <linux/io.h>
-#include <linux/can/dev.h>
-
-#include <linux/of_platform.h>
-#include <linux/of_address.h>
-#include <linux/of_irq.h>
-
-#include "sja1000.h"
-
-#define DRV_NAME "sja1000_of_platform"
-
-MODULE_AUTHOR("Wolfgang Grandegger <[email protected]>");
-MODULE_DESCRIPTION("Socket-CAN driver for SJA1000 on the OF platform bus");
-MODULE_LICENSE("GPL v2");
-
-#define SJA1000_OFP_CAN_CLOCK (16000000 / 2)
-
-static u8 sja1000_ofp_read_reg(const struct sja1000_priv *priv, int reg)
-{
- return ioread8(priv->reg_base + reg);
-}
-
-static void sja1000_ofp_write_reg(const struct sja1000_priv *priv,
- int reg, u8 val)
-{
- iowrite8(val, priv->reg_base + reg);
-}
-
-static int sja1000_ofp_remove(struct platform_device *ofdev)
-{
- struct net_device *dev = platform_get_drvdata(ofdev);
- struct sja1000_priv *priv = netdev_priv(dev);
- struct device_node *np = ofdev->dev.of_node;
- struct resource res;
-
- unregister_sja1000dev(dev);
- free_sja1000dev(dev);
- iounmap(priv->reg_base);
- irq_dispose_mapping(dev->irq);
-
- of_address_to_resource(np, 0, &res);
- release_mem_region(res.start, resource_size(&res));
-
- return 0;
-}
-
-static int sja1000_ofp_probe(struct platform_device *ofdev)
-{
- struct device_node *np = ofdev->dev.of_node;
- struct net_device *dev;
- struct sja1000_priv *priv;
- struct resource res;
- u32 prop;
- int err, irq, res_size;
- void __iomem *base;
-
- err = of_address_to_resource(np, 0, &res);
- if (err) {
- dev_err(&ofdev->dev, "invalid address\n");
- return err;
- }
-
- res_size = resource_size(&res);
-
- if (!request_mem_region(res.start, res_size, DRV_NAME)) {
- dev_err(&ofdev->dev, "couldn't request %pR\n", &res);
- return -EBUSY;
- }
-
- base = ioremap_nocache(res.start, res_size);
- if (!base) {
- dev_err(&ofdev->dev, "couldn't ioremap %pR\n", &res);
- err = -ENOMEM;
- goto exit_release_mem;
- }
-
- irq = irq_of_parse_and_map(np, 0);
- if (irq == 0) {
- dev_err(&ofdev->dev, "no irq found\n");
- err = -ENODEV;
- goto exit_unmap_mem;
- }
-
- dev = alloc_sja1000dev(0);
- if (!dev) {
- err = -ENOMEM;
- goto exit_dispose_irq;
- }
-
- priv = netdev_priv(dev);
-
- priv->read_reg = sja1000_ofp_read_reg;
- priv->write_reg = sja1000_ofp_write_reg;
-
- err = of_property_read_u32(np, "nxp,external-clock-frequency", &prop);
- if (!err)
- priv->can.clock.freq = prop / 2;
- else
- priv->can.clock.freq = SJA1000_OFP_CAN_CLOCK; /* default */
-
- err = of_property_read_u32(np, "nxp,tx-output-mode", &prop);
- if (!err)
- priv->ocr |= prop & OCR_MODE_MASK;
- else
- priv->ocr |= OCR_MODE_NORMAL; /* default */
-
- err = of_property_read_u32(np, "nxp,tx-output-config", &prop);
- if (!err)
- priv->ocr |= (prop << OCR_TX_SHIFT) & OCR_TX_MASK;
- else
- priv->ocr |= OCR_TX0_PULLDOWN; /* default */
-
- err = of_property_read_u32(np, "nxp,clock-out-frequency", &prop);
- if (!err && prop) {
- u32 divider = priv->can.clock.freq * 2 / prop;
-
- if (divider > 1)
- priv->cdr |= divider / 2 - 1;
- else
- priv->cdr |= CDR_CLKOUT_MASK;
- } else {
- priv->cdr |= CDR_CLK_OFF; /* default */
- }
-
- if (!of_property_read_bool(np, "nxp,no-comparator-bypass"))
- priv->cdr |= CDR_CBP; /* default */
-
- priv->irq_flags = IRQF_SHARED;
- priv->reg_base = base;
-
- dev->irq = irq;
-
- dev_info(&ofdev->dev,
- "reg_base=0x%p irq=%d clock=%d ocr=0x%02x cdr=0x%02x\n",
- priv->reg_base, dev->irq, priv->can.clock.freq,
- priv->ocr, priv->cdr);
-
- platform_set_drvdata(ofdev, dev);
- SET_NETDEV_DEV(dev, &ofdev->dev);
-
- err = register_sja1000dev(dev);
- if (err) {
- dev_err(&ofdev->dev, "registering %s failed (err=%d)\n",
- DRV_NAME, err);
- goto exit_free_sja1000;
- }
-
- return 0;
-
-exit_free_sja1000:
- free_sja1000dev(dev);
-exit_dispose_irq:
- irq_dispose_mapping(irq);
-exit_unmap_mem:
- iounmap(base);
-exit_release_mem:
- release_mem_region(res.start, res_size);
-
- return err;
-}
-
-static struct of_device_id sja1000_ofp_table[] = {
- {.compatible = "nxp,sja1000"},
- {},
-};
-MODULE_DEVICE_TABLE(of, sja1000_ofp_table);
-
-static struct platform_driver sja1000_ofp_driver = {
- .driver = {
- .owner = THIS_MODULE,
- .name = DRV_NAME,
- .of_match_table = sja1000_ofp_table,
- },
- .probe = sja1000_ofp_probe,
- .remove = sja1000_ofp_remove,
-};
-
-module_platform_driver(sja1000_ofp_driver);
diff --git a/drivers/net/can/sja1000/sja1000_platform.c b/drivers/net/can/sja1000/sja1000_platform.c
index 779679a..96a92a1 100644
--- a/drivers/net/can/sja1000/sja1000_platform.c
+++ b/drivers/net/can/sja1000/sja1000_platform.c
@@ -27,12 +27,16 @@
#include <linux/can/dev.h>
#include <linux/can/platform/sja1000.h>
#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>

#include "sja1000.h"

#define DRV_NAME "sja1000_platform"
+#define SJA1000_OFP_CAN_CLOCK (16000000 / 2)

MODULE_AUTHOR("Sascha Hauer <[email protected]>");
+MODULE_AUTHOR("Wolfgang Grandegger <[email protected]>");
MODULE_DESCRIPTION("Socket-CAN driver for SJA1000 on the platform bus");
MODULE_ALIAS("platform:" DRV_NAME);
MODULE_LICENSE("GPL v2");
@@ -67,24 +71,98 @@ static void sp_write_reg32(const struct sja1000_priv *priv, int reg, u8 val)
iowrite8(val, priv->reg_base + reg * 4);
}

-static int sp_probe(struct platform_device *pdev)
+static void sp_populate(struct sja1000_priv *priv,
+ struct sja1000_platform_data *pdata,
+ unsigned long resource_mem_flags)
+{
+ /* The CAN clock frequency is half the oscillator clock frequency */
+ priv->can.clock.freq = pdata->osc_freq / 2;
+ priv->ocr = pdata->ocr;
+ priv->cdr = pdata->cdr;
+
+ switch (resource_mem_flags & IORESOURCE_MEM_TYPE_MASK) {
+ case IORESOURCE_MEM_32BIT:
+ priv->read_reg = sp_read_reg32;
+ priv->write_reg = sp_write_reg32;
+ break;
+ case IORESOURCE_MEM_16BIT:
+ priv->read_reg = sp_read_reg16;
+ priv->write_reg = sp_write_reg16;
+ break;
+ case IORESOURCE_MEM_8BIT:
+ default:
+ priv->read_reg = sp_read_reg8;
+ priv->write_reg = sp_write_reg8;
+ break;
+ }
+}
+
+#if defined(CONFIG_OF)
+static void sp_populate_of(struct sja1000_priv *priv, struct device_node *of)
{
int err;
+ u32 prop;
+
+ priv->read_reg = sp_read_reg8;
+ priv->write_reg = sp_write_reg8;
+
+ err = of_property_read_u32(of, "nxp,external-clock-frequency", &prop);
+ if (!err)
+ priv->can.clock.freq = prop / 2;
+ else
+ priv->can.clock.freq = SJA1000_OFP_CAN_CLOCK; /* default */
+
+ err = of_property_read_u32(of, "nxp,tx-output-mode", &prop);
+ if (!err)
+ priv->ocr |= prop & OCR_MODE_MASK;
+ else
+ priv->ocr |= OCR_MODE_NORMAL; /* default */
+
+ err = of_property_read_u32(of, "nxp,tx-output-config", &prop);
+ if (!err)
+ priv->ocr |= (prop << OCR_TX_SHIFT) & OCR_TX_MASK;
+ else
+ priv->ocr |= OCR_TX0_PULLDOWN; /* default */
+
+ err = of_property_read_u32(of, "nxp,clock-out-frequency", &prop);
+ if (!err && prop) {
+ u32 divider = priv->can.clock.freq * 2 / prop;
+
+ if (divider > 1)
+ priv->cdr |= divider / 2 - 1;
+ else
+ priv->cdr |= CDR_CLKOUT_MASK;
+ } else {
+ priv->cdr |= CDR_CLK_OFF; /* default */
+ }
+
+ if (!of_property_read_bool(of, "nxp,no-comparator-bypass"))
+ priv->cdr |= CDR_CBP; /* default */
+}
+#else
+static void sp_populate_of(struct sja1000_priv *priv, device_node *of)
+{
+}
+#endif
+
+static int sp_probe(struct platform_device *pdev)
+{
+ int err, irq = 0;
void __iomem *addr;
struct net_device *dev;
struct sja1000_priv *priv;
- struct resource *res_mem, *res_irq;
+ struct resource *res_mem, *res_irq = 0;
struct sja1000_platform_data *pdata;
+ struct device_node *of = pdev->dev.of_node;

pdata = dev_get_platdata(&pdev->dev);
- if (!pdata) {
+ if (!pdata && !of) {
dev_err(&pdev->dev, "No platform data provided!\n");
return -ENODEV;
}

res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
- if (!res_mem || !res_irq)
+ if (!res_mem)
return -ENODEV;

if (!devm_request_mem_region(&pdev->dev, res_mem->start,
@@ -96,36 +174,34 @@ static int sp_probe(struct platform_device *pdev)
if (!addr)
return -ENOMEM;

+ if (of)
+ irq = irq_of_parse_and_map(of, 0);
+ else
+ res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+
+ if (!irq && !res_irq)
+ return -ENODEV;
+
dev = alloc_sja1000dev(0);
if (!dev)
return -ENOMEM;
priv = netdev_priv(dev);

- dev->irq = res_irq->start;
- priv->irq_flags = res_irq->flags & IRQF_TRIGGER_MASK;
- if (res_irq->flags & IORESOURCE_IRQ_SHAREABLE)
- priv->irq_flags |= IRQF_SHARED;
+ if (res_irq) {
+ irq = res_irq->start;
+ priv->irq_flags = res_irq->flags & IRQF_TRIGGER_MASK;
+ if (res_irq->flags & IORESOURCE_IRQ_SHAREABLE)
+ priv->irq_flags |= IRQF_SHARED;
+ } else
+ priv->irq_flags = IRQF_SHARED;
+
+ dev->irq = irq;
priv->reg_base = addr;
- /* The CAN clock frequency is half the oscillator clock frequency */
- priv->can.clock.freq = pdata->osc_freq / 2;
- priv->ocr = pdata->ocr;
- priv->cdr = pdata->cdr;

- switch (res_mem->flags & IORESOURCE_MEM_TYPE_MASK) {
- case IORESOURCE_MEM_32BIT:
- priv->read_reg = sp_read_reg32;
- priv->write_reg = sp_write_reg32;
- break;
- case IORESOURCE_MEM_16BIT:
- priv->read_reg = sp_read_reg16;
- priv->write_reg = sp_write_reg16;
- break;
- case IORESOURCE_MEM_8BIT:
- default:
- priv->read_reg = sp_read_reg8;
- priv->write_reg = sp_write_reg8;
- break;
- }
+ if (of)
+ sp_populate_of(priv, of);
+ else
+ sp_populate(priv, pdata, res_mem->flags);

platform_set_drvdata(pdev, dev);
SET_NETDEV_DEV(dev, &pdev->dev);
@@ -156,12 +232,21 @@ static int sp_remove(struct platform_device *pdev)
return 0;
}

+#if defined(CONFIG_OF)
+static struct of_device_id sja1000_ofp_table[] = {
+ {.compatible = "nxp,sja1000"},
+ {},
+};
+MODULE_DEVICE_TABLE(of, sja1000_ofp_table);
+#endif
+
static struct platform_driver sp_driver = {
.probe = sp_probe,
.remove = sp_remove,
.driver = {
.name = DRV_NAME,
.owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(sja1000_ofp_table),
},
};

--
1.8.1.2

2014-01-31 10:35:36

[permalink] [raw]

Subject: [PATCH v2 6/6] can: sja1000: of: add reg-io-width property for 8, 16 and 32-bit register access

Add the 'reg-io-width' property for 8, 16 and 32-bit access, like
what is currently done with IORESOURCE_MEM_{8,16,32}BIT for non-OF
boot.

Signed-off-by: Florian Vaussard <[email protected]>
---
drivers/net/can/sja1000/sja1000_platform.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/sja1000/sja1000_platform.c b/drivers/net/can/sja1000/sja1000_platform.c
index 96a92a1..62ebc3d 100644
--- a/drivers/net/can/sja1000/sja1000_platform.c
+++ b/drivers/net/can/sja1000/sja1000_platform.c
@@ -103,8 +103,17 @@ static void sp_populate_of(struct sja1000_priv *priv, struct device_node *of)
int err;
u32 prop;

- priv->read_reg = sp_read_reg8;
- priv->write_reg = sp_write_reg8;
+ of_property_read_u32(of, "reg-io-width", &prop);
+ if (prop == 4) {
+ priv->read_reg = sp_read_reg32;
+ priv->write_reg = sp_write_reg32;
+ } else if (prop == 2) {
+ priv->read_reg = sp_read_reg16;
+ priv->write_reg = sp_write_reg16;
+ } else {
+ priv->read_reg = sp_read_reg8;
+ priv->write_reg = sp_write_reg8;
+ }

err = of_property_read_u32(of, "nxp,external-clock-frequency", &prop);
if (!err)
--
1.8.1.2

2014-01-31 10:35:35

[permalink] [raw]

Subject: [PATCH v2 5/6] Documentation: devicetree: sja1000: add reg-io-width binding

Add the reg-io-width property to describe the width of the memory
accesses.

Cc: Grant Likely <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Pawel Moll <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Ian Campbell <[email protected]>
Cc: Kumar Gala <[email protected]>
Cc: [email protected]
Acked-by: Rob Herring <[email protected]>
Signed-off-by: Florian Vaussard <[email protected]>
---
Documentation/devicetree/bindings/net/can/sja1000.txt | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/can/sja1000.txt b/Documentation/devicetree/bindings/net/can/sja1000.txt
index f2105a4..b4a6d53 100644
--- a/Documentation/devicetree/bindings/net/can/sja1000.txt
+++ b/Documentation/devicetree/bindings/net/can/sja1000.txt
@@ -12,6 +12,10 @@ Required properties:

Optional properties:

+- reg-io-width : Specify the size (in bytes) of the IO accesses that
+ should be performed on the device. Valid value is 1, 2 or 4.
+ Default to 1 (8 bits).
+
- nxp,external-clock-frequency : Frequency of the external oscillator
clock in Hz. Note that the internal clock frequency used by the
SJA1000 is half of that value. If not specified, a default value
--
1.8.1.2

2014-01-31 10:36:44

[permalink] [raw]

Subject: [PATCH v2 3/6] can: sja1000: platform: use devm_* APIs

Simplify probe and remove functions by converting most of the resources
to use devm_* APIs.

Signed-off-by: Florian Vaussard <[email protected]>
---
drivers/net/can/sja1000/sja1000_platform.c | 46 ++++++++----------------------
1 file changed, 12 insertions(+), 34 deletions(-)

diff --git a/drivers/net/can/sja1000/sja1000_platform.c b/drivers/net/can/sja1000/sja1000_platform.c
index 29f9b63..779679a 100644
--- a/drivers/net/can/sja1000/sja1000_platform.c
+++ b/drivers/net/can/sja1000/sja1000_platform.c
@@ -79,34 +79,26 @@ static int sp_probe(struct platform_device *pdev)
pdata = dev_get_platdata(&pdev->dev);
if (!pdata) {
dev_err(&pdev->dev, "No platform data provided!\n");
- err = -ENODEV;
- goto exit;
+ return -ENODEV;
}

res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
- if (!res_mem || !res_irq) {
- err = -ENODEV;
- goto exit;
- }
+ if (!res_mem || !res_irq)
+ return -ENODEV;

- if (!request_mem_region(res_mem->start, resource_size(res_mem),
- DRV_NAME)) {
- err = -EBUSY;
- goto exit;
- }
+ if (!devm_request_mem_region(&pdev->dev, res_mem->start,
+ resource_size(res_mem), DRV_NAME))
+ return -EBUSY;

- addr = ioremap_nocache(res_mem->start, resource_size(res_mem));
- if (!addr) {
- err = -ENOMEM;
- goto exit_release;
- }
+ addr = devm_ioremap_nocache(&pdev->dev, res_mem->start,
+ resource_size(res_mem));
+ if (!addr)
+ return -ENOMEM;

dev = alloc_sja1000dev(0);
- if (!dev) {
- err = -ENOMEM;
- goto exit_iounmap;
- }
+ if (!dev)
+ return -ENOMEM;
priv = netdev_priv(dev);

dev->irq = res_irq->start;
@@ -151,28 +143,14 @@ static int sp_probe(struct platform_device *pdev)

exit_free:
free_sja1000dev(dev);
- exit_iounmap:
- iounmap(addr);
- exit_release:
- release_mem_region(res_mem->start, resource_size(res_mem));
- exit:
return err;
}

static int sp_remove(struct platform_device *pdev)
{
struct net_device *dev = platform_get_drvdata(pdev);
- struct sja1000_priv *priv = netdev_priv(dev);
- struct resource *res;

unregister_sja1000dev(dev);
-
- if (priv->reg_base)
- iounmap(priv->reg_base);
-
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- release_mem_region(res->start, resource_size(res));
-
free_sja1000dev(dev);

return 0;
--
1.8.1.2

2014-01-31 10:37:21

[permalink] [raw]

Subject: [PATCH v2 2/6] can: sja1000: convert printk to use netdev API

Use netdev_* where applicable.

Signed-off-by: Florian Vaussard <[email protected]>
---
drivers/net/can/sja1000/sja1000.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
index f17c301..55cce47 100644
--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -106,8 +106,7 @@ static int sja1000_probe_chip(struct net_device *dev)
struct sja1000_priv *priv = netdev_priv(dev);

if (priv->reg_base && sja1000_is_absent(priv)) {
- printk(KERN_INFO "%s: probing @0x%lX failed\n",
- DRV_NAME, dev->base_addr);
+ netdev_err(dev, "probing failed\n");
return 0;
}
return -1;
--
1.8.1.2

2014-01-31 12:28:50

[permalink] [raw]

Subject: Re: [PATCH v2 6/6] can: sja1000: of: add reg-io-width property for 8, 16 and 32-bit register access

On 01/31/2014 11:35 AM, Florian Vaussard wrote:
> Add the 'reg-io-width' property for 8, 16 and 32-bit access, like
> what is currently done with IORESOURCE_MEM_{8,16,32}BIT for non-OF
> boot.
>
> Signed-off-by: Florian Vaussard <[email protected]>
> ---
> drivers/net/can/sja1000/sja1000_platform.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/can/sja1000/sja1000_platform.c b/drivers/net/can/sja1000/sja1000_platform.c
> index 96a92a1..62ebc3d 100644
> --- a/drivers/net/can/sja1000/sja1000_platform.c
> +++ b/drivers/net/can/sja1000/sja1000_platform.c
> @@ -103,8 +103,17 @@ static void sp_populate_of(struct sja1000_priv *priv, struct device_node *of)
> int err;
> u32 prop;
>
> - priv->read_reg = sp_read_reg8;
> - priv->write_reg = sp_write_reg8;
> + of_property_read_u32(of, "reg-io-width", &prop);

The new property might not be available in the DT, please check the
return value of of_property_read_u32(). Use 8 bit wide as default.

> + if (prop == 4) {
> + priv->read_reg = sp_read_reg32;
> + priv->write_reg = sp_write_reg32;
> + } else if (prop == 2) {
> + priv->read_reg = sp_read_reg16;
> + priv->write_reg = sp_write_reg16;
> + } else {
> + priv->read_reg = sp_read_reg8;
> + priv->write_reg = sp_write_reg8;
> + }
>
> err = of_property_read_u32(of, "nxp,external-clock-frequency", &prop);
> if (!err)
>

Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |

Attachments:

2014-01-31 12:29:44

[permalink] [raw]

Subject: Re: [PATCH v2 5/6] Documentation: devicetree: sja1000: add reg-io-width binding

On 01/31/2014 11:35 AM, Florian Vaussard wrote:
> Add the reg-io-width property to describe the width of the memory
> accesses.
>
> Cc: Grant Likely <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Pawel Moll <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: Ian Campbell <[email protected]>
> Cc: Kumar Gala <[email protected]>
> Cc: [email protected]
> Acked-by: Rob Herring <[email protected]>
> Signed-off-by: Florian Vaussard <[email protected]>

I think it makes sense to squash into patch 6.

Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |

Attachments:

2014-01-31 12:33:45

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] can: sja1000: fuse of_platform into platform

On 01/31/2014 11:35 AM, Florian Vaussard wrote:
> The OpenFirmware probe can be merged into the standard platform
> probe to leverage common code.

Good work, as we want to replace the existing driver, I'm quite picky on
this patch, see more comments inline.

Please don't delete of of_platform driver, yet.

> Signed-off-by: Florian Vaussard <[email protected]>
> ---
> drivers/net/can/sja1000/Kconfig | 13 +-
> drivers/net/can/sja1000/Makefile | 1 -
> drivers/net/can/sja1000/sja1000_of_platform.c | 218 --------------------------
> drivers/net/can/sja1000/sja1000_platform.c | 141 +++++++++++++----
> 4 files changed, 116 insertions(+), 257 deletions(-)
> delete mode 100644 drivers/net/can/sja1000/sja1000_of_platform.c
[...]

> diff --git a/drivers/net/can/sja1000/sja1000_platform.c b/drivers/net/can/sja1000/sja1000_platform.c
> index 779679a..96a92a1 100644
> --- a/drivers/net/can/sja1000/sja1000_platform.c
> +++ b/drivers/net/can/sja1000/sja1000_platform.c
> @@ -27,12 +27,16 @@
> #include <linux/can/dev.h>
> #include <linux/can/platform/sja1000.h>
> #include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
>
> #include "sja1000.h"
>
> #define DRV_NAME "sja1000_platform"
> +#define SJA1000_OFP_CAN_CLOCK (16000000 / 2)

This driver uses "SP" short for sja100_platform as namespace, please
convert.

>
> MODULE_AUTHOR("Sascha Hauer <[email protected]>");
> +MODULE_AUTHOR("Wolfgang Grandegger <[email protected]>");
> MODULE_DESCRIPTION("Socket-CAN driver for SJA1000 on the platform bus");
> MODULE_ALIAS("platform:" DRV_NAME);
> MODULE_LICENSE("GPL v2");
> @@ -67,24 +71,98 @@ static void sp_write_reg32(const struct sja1000_priv *priv, int reg, u8 val)
> iowrite8(val, priv->reg_base + reg * 4);
> }
>
> -static int sp_probe(struct platform_device *pdev)
> +static void sp_populate(struct sja1000_priv *priv,
> + struct sja1000_platform_data *pdata,
> + unsigned long resource_mem_flags)
> +{
> + /* The CAN clock frequency is half the oscillator clock frequency */
> + priv->can.clock.freq = pdata->osc_freq / 2;
> + priv->ocr = pdata->ocr;
> + priv->cdr = pdata->cdr;
> +
> + switch (resource_mem_flags & IORESOURCE_MEM_TYPE_MASK) {
> + case IORESOURCE_MEM_32BIT:
> + priv->read_reg = sp_read_reg32;
> + priv->write_reg = sp_write_reg32;
> + break;
> + case IORESOURCE_MEM_16BIT:
> + priv->read_reg = sp_read_reg16;
> + priv->write_reg = sp_write_reg16;
> + break;
> + case IORESOURCE_MEM_8BIT:
> + default:
> + priv->read_reg = sp_read_reg8;
> + priv->write_reg = sp_write_reg8;
> + break;
> + }
> +}
> +
> +#if defined(CONFIG_OF)

The driver also compiles on systems without CONFIG_OF without these
defines. Please remove.

> +static void sp_populate_of(struct sja1000_priv *priv, struct device_node *of)
> {
> int err;
> + u32 prop;
> +
> + priv->read_reg = sp_read_reg8;
> + priv->write_reg = sp_write_reg8;
> +
> + err = of_property_read_u32(of, "nxp,external-clock-frequency", &prop);
> + if (!err)
> + priv->can.clock.freq = prop / 2;
> + else
> + priv->can.clock.freq = SJA1000_OFP_CAN_CLOCK; /* default */
> +
> + err = of_property_read_u32(of, "nxp,tx-output-mode", &prop);
> + if (!err)
> + priv->ocr |= prop & OCR_MODE_MASK;
> + else
> + priv->ocr |= OCR_MODE_NORMAL; /* default */
> +
> + err = of_property_read_u32(of, "nxp,tx-output-config", &prop);
> + if (!err)
> + priv->ocr |= (prop << OCR_TX_SHIFT) & OCR_TX_MASK;
> + else
> + priv->ocr |= OCR_TX0_PULLDOWN; /* default */
> +
> + err = of_property_read_u32(of, "nxp,clock-out-frequency", &prop);
> + if (!err && prop) {
> + u32 divider = priv->can.clock.freq * 2 / prop;
> +
> + if (divider > 1)
> + priv->cdr |= divider / 2 - 1;
> + else
> + priv->cdr |= CDR_CLKOUT_MASK;
> + } else {
> + priv->cdr |= CDR_CLK_OFF; /* default */
> + }
> +
> + if (!of_property_read_bool(of, "nxp,no-comparator-bypass"))
> + priv->cdr |= CDR_CBP; /* default */
> +}
> +#else
> +static void sp_populate_of(struct sja1000_priv *priv, device_node *of)
> +{
> +}
> +#endif
> +
> +static int sp_probe(struct platform_device *pdev)
> +{
> + int err, irq = 0;
> void __iomem *addr;
> struct net_device *dev;
> struct sja1000_priv *priv;
> - struct resource *res_mem, *res_irq;
> + struct resource *res_mem, *res_irq = 0;

Please use NULL to initialize pointers.

> struct sja1000_platform_data *pdata;
> + struct device_node *of = pdev->dev.of_node;
>
> pdata = dev_get_platdata(&pdev->dev);
> - if (!pdata) {
> + if (!pdata && !of) {
> dev_err(&pdev->dev, "No platform data provided!\n");
> return -ENODEV;
> }
>
> res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> - if (!res_mem || !res_irq)
> + if (!res_mem)
> return -ENODEV;
>
> if (!devm_request_mem_region(&pdev->dev, res_mem->start,
> @@ -96,36 +174,34 @@ static int sp_probe(struct platform_device *pdev)
> if (!addr)
> return -ENOMEM;
>
> + if (of)
> + irq = irq_of_parse_and_map(of, 0);
> + else
> + res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +
> + if (!irq && !res_irq)
> + return -ENODEV;
> +
> dev = alloc_sja1000dev(0);
> if (!dev)
> return -ENOMEM;
> priv = netdev_priv(dev);
>
> - dev->irq = res_irq->start;
> - priv->irq_flags = res_irq->flags & IRQF_TRIGGER_MASK;
> - if (res_irq->flags & IORESOURCE_IRQ_SHAREABLE)
> - priv->irq_flags |= IRQF_SHARED;
> + if (res_irq) {
> + irq = res_irq->start;
> + priv->irq_flags = res_irq->flags & IRQF_TRIGGER_MASK;
> + if (res_irq->flags & IORESOURCE_IRQ_SHAREABLE)
> + priv->irq_flags |= IRQF_SHARED;
> + } else
> + priv->irq_flags = IRQF_SHARED;

Please add { } for else, too.

> +
> + dev->irq = irq;
> priv->reg_base = addr;
> - /* The CAN clock frequency is half the oscillator clock frequency */
> - priv->can.clock.freq = pdata->osc_freq / 2;
> - priv->ocr = pdata->ocr;
> - priv->cdr = pdata->cdr;
>
> - switch (res_mem->flags & IORESOURCE_MEM_TYPE_MASK) {
> - case IORESOURCE_MEM_32BIT:
> - priv->read_reg = sp_read_reg32;
> - priv->write_reg = sp_write_reg32;
> - break;
> - case IORESOURCE_MEM_16BIT:
> - priv->read_reg = sp_read_reg16;
> - priv->write_reg = sp_write_reg16;
> - break;
> - case IORESOURCE_MEM_8BIT:
> - default:
> - priv->read_reg = sp_read_reg8;
> - priv->write_reg = sp_write_reg8;
> - break;
> - }
> + if (of)
> + sp_populate_of(priv, of);
> + else
> + sp_populate(priv, pdata, res_mem->flags);
>
> platform_set_drvdata(pdev, dev);
> SET_NETDEV_DEV(dev, &pdev->dev);
> @@ -156,12 +232,21 @@ static int sp_remove(struct platform_device *pdev)
> return 0;
> }
>
> +#if defined(CONFIG_OF)

Please remove the ifdef.

> +static struct of_device_id sja1000_ofp_table[] = {

Please rename into sp_of_table or something similar.

> + {.compatible = "nxp,sja1000"},
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, sja1000_ofp_table);
> +#endif
> +
> static struct platform_driver sp_driver = {
> .probe = sp_probe,
> .remove = sp_remove,
> .driver = {
> .name = DRV_NAME,
> .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(sja1000_ofp_table),

The of_match_ptr is not needed anymore, please remove.

> },
> };
>
>

Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |

Attachments:

2014-01-31 12:34:07

[permalink] [raw]

Subject: Re: [PATCH v2 3/6] can: sja1000: platform: use devm_* APIs

On 01/31/2014 11:35 AM, Florian Vaussard wrote:
> Simplify probe and remove functions by converting most of the resources
> to use devm_* APIs.
>
> Signed-off-by: Florian Vaussard <[email protected]>

Looks good.

Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |

Attachments:

2014-01-31 12:34:34

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] can: sja1000: convert printk to use netdev API

On 01/31/2014 11:35 AM, Florian Vaussard wrote:
> Use netdev_* where applicable.
>
> Signed-off-by: Florian Vaussard <[email protected]>

Loks good.

Tnx,
Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |

Attachments:

2014-01-31 12:35:38

[permalink] [raw]

Subject: Re: [PATCH v2 1/6] can: sja1000: remove unused defines

On 01/31/2014 11:35 AM, Florian Vaussard wrote:
> Remove unused defines for the OF platform.
>
> Signed-off-by: Florian Vaussard <[email protected]>

I've overlooked in my first review that this patch is touching the
of_platform driver. Please remove the patch, as we want to remove the
driver sooner or later.

Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |

Attachments:

2014-01-31 12:37:33

[permalink] [raw]

Subject: Re: [PATCH v2 0/6] can: sja1000: cleanups and new OF property

On 01/31/2014 11:35 AM, Florian Vaussard wrote:
> Hello,
>
> Changes sinces v1:
> - Merge sja1000_of_platform.c into sja1000_platform.c (patch 4)
>
> The first part of this series performs serveral small cleanups
> (patches 1 to 3).
>
> Patch 4 merges sja1000_of_platform.c into sja1000_platform.c.
> Changes are pretty conservatives (mostly copy/paste/move). IRQ
> is treated differently in the OF and non-OF versions, thus this
> is where the fused version differs the most.
>
> The final part introduces the 'reg-io-width' binding (already used
> by some other drivers) to perform a similar job as what was done
> with IORESOURCE_MEM_XXBIT. This is needed on my system to correctly
> take into account the aliasing of the address bus.

Good work, please keep compatibility for DTs without the 'reg-io-width'
binding. The rest of my comments are only nitpicks. :)

Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |

Attachments:

2014-01-31 12:41:42

[permalink] [raw]

Subject: Re: [PATCH v2 0/6] can: sja1000: cleanups and new OF property

Hello Marc,

On 01/31/2014 01:37 PM, Marc Kleine-Budde wrote:
> On 01/31/2014 11:35 AM, Florian Vaussard wrote:
>> Hello,
>>
>> Changes sinces v1:
>> - Merge sja1000_of_platform.c into sja1000_platform.c (patch 4)
>>
>> The first part of this series performs serveral small cleanups
>> (patches 1 to 3).
>>
>> Patch 4 merges sja1000_of_platform.c into sja1000_platform.c.
>> Changes are pretty conservatives (mostly copy/paste/move). IRQ
>> is treated differently in the OF and non-OF versions, thus this
>> is where the fused version differs the most.
>>
>> The final part introduces the 'reg-io-width' binding (already used
>> by some other drivers) to perform a similar job as what was done
>> with IORESOURCE_MEM_XXBIT. This is needed on my system to correctly
>> take into account the aliasing of the address bus.
>
> Good work, please keep compatibility for DTs without the 'reg-io-width'
> binding. The rest of my comments are only nitpicks. :)
>

Good points. I will send a v3.

Regards,

Florian

2014-01-31 12:45:37

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] can: sja1000: fuse of_platform into platform

Hello Marc,

On 01/31/2014 01:33 PM, Marc Kleine-Budde wrote:
> On 01/31/2014 11:35 AM, Florian Vaussard wrote:
>> The OpenFirmware probe can be merged into the standard platform
>> probe to leverage common code.
>
> Good work, as we want to replace the existing driver, I'm quite picky on
> this patch, see more comments inline.
>
> Please don't delete of of_platform driver, yet.
>

Do you have any reason for not deleting of_platform? After this patch,
we will have duplicated functionalities, this may be misleading for
other people.

Regards,
Florian

>> Signed-off-by: Florian Vaussard <[email protected]>
>> ---
>> drivers/net/can/sja1000/Kconfig | 13 +-
>> drivers/net/can/sja1000/Makefile | 1 -
>> drivers/net/can/sja1000/sja1000_of_platform.c | 218 --------------------------
>> drivers/net/can/sja1000/sja1000_platform.c | 141 +++++++++++++----
>> 4 files changed, 116 insertions(+), 257 deletions(-)
>> delete mode 100644 drivers/net/can/sja1000/sja1000_of_platform.c
> [...]
>
>> diff --git a/drivers/net/can/sja1000/sja1000_platform.c b/drivers/net/can/sja1000/sja1000_platform.c
>> index 779679a..96a92a1 100644
>> --- a/drivers/net/can/sja1000/sja1000_platform.c
>> +++ b/drivers/net/can/sja1000/sja1000_platform.c
>> @@ -27,12 +27,16 @@
>> #include <linux/can/dev.h>
>> #include <linux/can/platform/sja1000.h>
>> #include <linux/io.h>
>> +#include <linux/of.h>
>> +#include <linux/of_irq.h>
>>
>> #include "sja1000.h"
>>
>> #define DRV_NAME "sja1000_platform"
>> +#define SJA1000_OFP_CAN_CLOCK (16000000 / 2)
>
> This driver uses "SP" short for sja100_platform as namespace, please
> convert.
>
>>
>> MODULE_AUTHOR("Sascha Hauer <[email protected]>");
>> +MODULE_AUTHOR("Wolfgang Grandegger <[email protected]>");
>> MODULE_DESCRIPTION("Socket-CAN driver for SJA1000 on the platform bus");
>> MODULE_ALIAS("platform:" DRV_NAME);
>> MODULE_LICENSE("GPL v2");
>> @@ -67,24 +71,98 @@ static void sp_write_reg32(const struct sja1000_priv *priv, int reg, u8 val)
>> iowrite8(val, priv->reg_base + reg * 4);
>> }
>>
>> -static int sp_probe(struct platform_device *pdev)
>> +static void sp_populate(struct sja1000_priv *priv,
>> + struct sja1000_platform_data *pdata,
>> + unsigned long resource_mem_flags)
>> +{
>> + /* The CAN clock frequency is half the oscillator clock frequency */
>> + priv->can.clock.freq = pdata->osc_freq / 2;
>> + priv->ocr = pdata->ocr;
>> + priv->cdr = pdata->cdr;
>> +
>> + switch (resource_mem_flags & IORESOURCE_MEM_TYPE_MASK) {
>> + case IORESOURCE_MEM_32BIT:
>> + priv->read_reg = sp_read_reg32;
>> + priv->write_reg = sp_write_reg32;
>> + break;
>> + case IORESOURCE_MEM_16BIT:
>> + priv->read_reg = sp_read_reg16;
>> + priv->write_reg = sp_write_reg16;
>> + break;
>> + case IORESOURCE_MEM_8BIT:
>> + default:
>> + priv->read_reg = sp_read_reg8;
>> + priv->write_reg = sp_write_reg8;
>> + break;
>> + }
>> +}
>> +
>> +#if defined(CONFIG_OF)
>
> The driver also compiles on systems without CONFIG_OF without these
> defines. Please remove.
>
>> +static void sp_populate_of(struct sja1000_priv *priv, struct device_node *of)
>> {
>> int err;
>> + u32 prop;
>> +
>> + priv->read_reg = sp_read_reg8;
>> + priv->write_reg = sp_write_reg8;
>> +
>> + err = of_property_read_u32(of, "nxp,external-clock-frequency", &prop);
>> + if (!err)
>> + priv->can.clock.freq = prop / 2;
>> + else
>> + priv->can.clock.freq = SJA1000_OFP_CAN_CLOCK; /* default */
>> +
>> + err = of_property_read_u32(of, "nxp,tx-output-mode", &prop);
>> + if (!err)
>> + priv->ocr |= prop & OCR_MODE_MASK;
>> + else
>> + priv->ocr |= OCR_MODE_NORMAL; /* default */
>> +
>> + err = of_property_read_u32(of, "nxp,tx-output-config", &prop);
>> + if (!err)
>> + priv->ocr |= (prop << OCR_TX_SHIFT) & OCR_TX_MASK;
>> + else
>> + priv->ocr |= OCR_TX0_PULLDOWN; /* default */
>> +
>> + err = of_property_read_u32(of, "nxp,clock-out-frequency", &prop);
>> + if (!err && prop) {
>> + u32 divider = priv->can.clock.freq * 2 / prop;
>> +
>> + if (divider > 1)
>> + priv->cdr |= divider / 2 - 1;
>> + else
>> + priv->cdr |= CDR_CLKOUT_MASK;
>> + } else {
>> + priv->cdr |= CDR_CLK_OFF; /* default */
>> + }
>> +
>> + if (!of_property_read_bool(of, "nxp,no-comparator-bypass"))
>> + priv->cdr |= CDR_CBP; /* default */
>> +}
>> +#else
>> +static void sp_populate_of(struct sja1000_priv *priv, device_node *of)
>> +{
>> +}
>> +#endif
>> +
>> +static int sp_probe(struct platform_device *pdev)
>> +{
>> + int err, irq = 0;
>> void __iomem *addr;
>> struct net_device *dev;
>> struct sja1000_priv *priv;
>> - struct resource *res_mem, *res_irq;
>> + struct resource *res_mem, *res_irq = 0;
>
> Please use NULL to initialize pointers.
>
>> struct sja1000_platform_data *pdata;
>> + struct device_node *of = pdev->dev.of_node;
>>
>> pdata = dev_get_platdata(&pdev->dev);
>> - if (!pdata) {
>> + if (!pdata && !of) {
>> dev_err(&pdev->dev, "No platform data provided!\n");
>> return -ENODEV;
>> }
>>
>> res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> - res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>> - if (!res_mem || !res_irq)
>> + if (!res_mem)
>> return -ENODEV;
>>
>> if (!devm_request_mem_region(&pdev->dev, res_mem->start,
>> @@ -96,36 +174,34 @@ static int sp_probe(struct platform_device *pdev)
>> if (!addr)
>> return -ENOMEM;
>>
>> + if (of)
>> + irq = irq_of_parse_and_map(of, 0);
>> + else
>> + res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>> +
>> + if (!irq && !res_irq)
>> + return -ENODEV;
>> +
>> dev = alloc_sja1000dev(0);
>> if (!dev)
>> return -ENOMEM;
>> priv = netdev_priv(dev);
>>
>> - dev->irq = res_irq->start;
>> - priv->irq_flags = res_irq->flags & IRQF_TRIGGER_MASK;
>> - if (res_irq->flags & IORESOURCE_IRQ_SHAREABLE)
>> - priv->irq_flags |= IRQF_SHARED;
>> + if (res_irq) {
>> + irq = res_irq->start;
>> + priv->irq_flags = res_irq->flags & IRQF_TRIGGER_MASK;
>> + if (res_irq->flags & IORESOURCE_IRQ_SHAREABLE)
>> + priv->irq_flags |= IRQF_SHARED;
>> + } else
>> + priv->irq_flags = IRQF_SHARED;
>
> Please add { } for else, too.
>
>> +
>> + dev->irq = irq;
>> priv->reg_base = addr;
>> - /* The CAN clock frequency is half the oscillator clock frequency */
>> - priv->can.clock.freq = pdata->osc_freq / 2;
>> - priv->ocr = pdata->ocr;
>> - priv->cdr = pdata->cdr;
>>
>> - switch (res_mem->flags & IORESOURCE_MEM_TYPE_MASK) {
>> - case IORESOURCE_MEM_32BIT:
>> - priv->read_reg = sp_read_reg32;
>> - priv->write_reg = sp_write_reg32;
>> - break;
>> - case IORESOURCE_MEM_16BIT:
>> - priv->read_reg = sp_read_reg16;
>> - priv->write_reg = sp_write_reg16;
>> - break;
>> - case IORESOURCE_MEM_8BIT:
>> - default:
>> - priv->read_reg = sp_read_reg8;
>> - priv->write_reg = sp_write_reg8;
>> - break;
>> - }
>> + if (of)
>> + sp_populate_of(priv, of);
>> + else
>> + sp_populate(priv, pdata, res_mem->flags);
>>
>> platform_set_drvdata(pdev, dev);
>> SET_NETDEV_DEV(dev, &pdev->dev);
>> @@ -156,12 +232,21 @@ static int sp_remove(struct platform_device *pdev)
>> return 0;
>> }
>>
>> +#if defined(CONFIG_OF)
>
> Please remove the ifdef.
>
>> +static struct of_device_id sja1000_ofp_table[] = {
>
> Please rename into sp_of_table or something similar.
>
>> + {.compatible = "nxp,sja1000"},
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, sja1000_ofp_table);
>> +#endif
>> +
>> static struct platform_driver sp_driver = {
>> .probe = sp_probe,
>> .remove = sp_remove,
>> .driver = {
>> .name = DRV_NAME,
>> .owner = THIS_MODULE,
>> + .of_match_table = of_match_ptr(sja1000_ofp_table),
>
> The of_match_ptr is not needed anymore, please remove.
>
>> },
>> };
>>
>>
>
> Marc
>

2014-01-31 12:49:49

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] can: sja1000: fuse of_platform into platform

On 01/31/2014 01:45 PM, Florian Vaussard wrote:
> Hello Marc,
>
> On 01/31/2014 01:33 PM, Marc Kleine-Budde wrote:
>> On 01/31/2014 11:35 AM, Florian Vaussard wrote:
>>> The OpenFirmware probe can be merged into the standard platform
>>> probe to leverage common code.
>>
>> Good work, as we want to replace the existing driver, I'm quite picky on
>> this patch, see more comments inline.
>>
>> Please don't delete of of_platform driver, yet.
>>
>
> Do you have any reason for not deleting of_platform? After this patch,
> we will have duplicated functionalities, this may be misleading for
> other people.

Okay.
IIRC there _was_ a problem on sparc, it _should_ be fixed now, better
get a Tested-by from Andreas (Cc'ed).

Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |

Attachments:

2014-01-31 12:51:25

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] can: sja1000: fuse of_platform into platform

On 01/31/2014 01:49 PM, Marc Kleine-Budde wrote:
> On 01/31/2014 01:45 PM, Florian Vaussard wrote:
>> Hello Marc,
>>
>> On 01/31/2014 01:33 PM, Marc Kleine-Budde wrote:
>>> On 01/31/2014 11:35 AM, Florian Vaussard wrote:
>>>> The OpenFirmware probe can be merged into the standard platform
>>>> probe to leverage common code.
>>>
>>> Good work, as we want to replace the existing driver, I'm quite picky on
>>> this patch, see more comments inline.
>>>
>>> Please don't delete of of_platform driver, yet.
>>>
>>
>> Do you have any reason for not deleting of_platform? After this patch,
>> we will have duplicated functionalities, this may be misleading for
>> other people.
>
> Okay.
> IIRC there _was_ a problem on sparc, it _should_ be fixed now, better
> get a Tested-by from Andreas (Cc'ed).

You have to use irq_of_parse_and_map() on sparc, but your patches does this.

Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |

Attachments:

2014-01-31 13:00:11

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] can: sja1000: fuse of_platform into platform

On 01/31/2014 01:51 PM, Marc Kleine-Budde wrote:
> On 01/31/2014 01:49 PM, Marc Kleine-Budde wrote:
>> On 01/31/2014 01:45 PM, Florian Vaussard wrote:
>>> Hello Marc,
>>>
>>> On 01/31/2014 01:33 PM, Marc Kleine-Budde wrote:
>>>> On 01/31/2014 11:35 AM, Florian Vaussard wrote:
>>>>> The OpenFirmware probe can be merged into the standard platform
>>>>> probe to leverage common code.
>>>>
>>>> Good work, as we want to replace the existing driver, I'm quite picky on
>>>> this patch, see more comments inline.
>>>>
>>>> Please don't delete of of_platform driver, yet.
>>>>
>>>
>>> Do you have any reason for not deleting of_platform? After this patch,
>>> we will have duplicated functionalities, this may be misleading for
>>> other people.
>>
>> Okay.
>> IIRC there _was_ a problem on sparc, it _should_ be fixed now, better
>> get a Tested-by from Andreas (Cc'ed).
>

So it is ok for you if I remove of_platform, right?

> You have to use irq_of_parse_and_map() on sparc, but your patches does this.
>

My first implementation was not using irq_of_parse_and_map(), as I do
not need it on ARM, but I suspected that other people might need it.
This is why the final code is slightly more complicated to handle both
cases.

Regards,
Florian

2014-01-31 13:14:22

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] can: sja1000: fuse of_platform into platform

On 01/31/2014 02:00 PM, Florian Vaussard wrote:
>
>
> On 01/31/2014 01:51 PM, Marc Kleine-Budde wrote:
>> On 01/31/2014 01:49 PM, Marc Kleine-Budde wrote:
>>> On 01/31/2014 01:45 PM, Florian Vaussard wrote:
>>>> Hello Marc,
>>>>
>>>> On 01/31/2014 01:33 PM, Marc Kleine-Budde wrote:
>>>>> On 01/31/2014 11:35 AM, Florian Vaussard wrote:
>>>>>> The OpenFirmware probe can be merged into the standard platform
>>>>>> probe to leverage common code.
>>>>>
>>>>> Good work, as we want to replace the existing driver, I'm quite picky on
>>>>> this patch, see more comments inline.
>>>>>
>>>>> Please don't delete of of_platform driver, yet.
>>>>>
>>>>
>>>> Do you have any reason for not deleting of_platform? After this patch,
>>>> we will have duplicated functionalities, this may be misleading for
>>>> other people.
>>>
>>> Okay.
>>> IIRC there _was_ a problem on sparc, it _should_ be fixed now, better
>>> get a Tested-by from Andreas (Cc'ed).
>>
>
> So it is ok for you if I remove of_platform, right?

If you get a Tested-by for this patch by a sparc user.

Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |

Attachments:

2014-02-03 08:11:45