can: m_can: add device tree binding documentation
Subsystems (3)
PATCH v3 1/2 can: m_can: add device tree binding documentation
add M_CAN device tree binding documentation
--- .../devicetree/bindings/net/can/m_can.txt | 65 ++++++++++++++++++++ 1 files changed, 65 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/net/can/m_can.txt
Documentation/devicetree/bindings/net/can/m_can.txt new file +65 -0
@@ -0,0 +1,65 @@ +Bosch MCAN controller Device Tree Bindings +------------------------------------------------- + +Required properties: +- compatible : Should be "bosch,m_can" for M_CAN controllers +- reg : physical base address and size of the M_CAN + registers map and Message RAM +- reg-names : Should be "m_can" and "message_ram" +- interrupts : Should be the interrupt number of M_CAN interrupt + line 0 and line 1, could be same if sharing + the same interrupt. +- interrupt-names : Should contain "int0" and "int1" +- clocks : Clocks used by controller, should be host clock + and CAN clock. +- clock-names : Should contain "hclk" and "cclk" +- pinctrl-<n> : Pinctrl states as described in bindings/pinctrl/pinctrl-bindings.txt +- pinctrl-names : Names corresponding to the numbered pinctrl states +- mram-cfg : Message RAM configuration data. + Multiple M_CAN instances can share the same Message RAM and each element(e.g + Rx FIFO or Tx Buffer and etc) number in Message RAM is also configurable, + so this property is telling driver how the shared or private Message RAM + are used by this M_CAN controller. + + The format should be as follows: + <offset sidf_elems xidf_elems rxf0_elems rxf1_elems rxb_elems + txe_elems txb_elems> + The 'offset' is an address offset of the Message RAM where the following + elements start from. This is usually set to 0x0 if you're using a private + Message RAM. The remain cells are used to specify how many elements are used + for each FIFO/Buffer. + +M_CAN includes the following elements according to user manual: +11-bit Filter 0-128 elements / 0-128 words +29-bit Filter 0-64 elements / 0-128 words +Rx FIFO 0 0-64 elements / 0-1152 words +Rx FIFO 1 0-64 elements / 0-1152 words +Rx Buffers 0-64 elements / 0-1152 words +Tx Event FIFO 0-32 elements / 0-64 words +Tx Buffers 0-32 elements / 0-576 words + +Please refer to 2.4.1 Message RAM Configuration in Bosch M_CAN user manual +for details. + +Example: +SoC dtsi: +m_can1: can at 020e8000 { + compatible = "bosch,m_can"; + reg = <0x020e8000 0x4000>, <0x02298000 0x4000>; + reg-names = "m_can", "message_ram"; + interrupts = <0 114 0x04>, + <0 114 0x04>; + interrupt-names = "int0", "int1"; + clocks = <&clks IMX6SX_CLK_CANFD>, + <&clks IMX6SX_CLK_CANFD>; + clock-names = "hclk", "cclk"; + mram-cfg = <0x0 0 0 32 0 0 0 1>; + status = "disabled"; +}; + +Board dtsi: +&m_can1 { + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_m_can1>; + status = "enabled"; +};
Signature · 1 line
1.7.8
PATCH v3 2/2 can: m_can: add Bosch M_CAN controller support
The patch adds the basic CAN TX/RX function support for Bosch M_CAN controller. For TX, only one dedicated tx buffer is used for sending data.
For RX, RXFIFO 0 is used for receiving data to avoid overflow.
Rx FIFO 1 and Rx Buffers are not used currently, as well as Tx Event FIFO.
Due to the message ram can be shared by multi m_can instances
and the fifo element is configurable which is SoC dependant,
the design is to parse the message ram related configuration data from device tree rather than hardcode define it in driver which can make the message ram sharing fully transparent to M_CAN controller driver,
then we can gain better driver maintainability and future features upgrade.
M_CAN also supports CANFD protocol features like data payload up to 64 bytes and bitrate switch at runtime, however, this patch still does not add the support for these features.
---
Changes since v2:
- fix checkpatch warnings
- handle CAN_CTRLMODE_BERR_REPORTING properly that disable LEC error interrupts (Bus error) if no CAN_CTRLMODE_BERR_REPORTING.
- introduce struct mram_cfg array to store message ram configuration data for each fifo
- fix txb offset calculate wrong issue(should not plus mram_off again)
- check return for clk_prepare_enable
- split doc part into a separate patch
Changes since v1:
Addressed all comments from Mark Rutland, Hartkopp and Marc Kleine-Budde
- merge three patches into one
- create directory drivers/net/can/m_can
- improve binding doc
- make sure using valid pointer before netif_receive_skb(skb)
- remove debug info a bit
- let the stats are updated even if alloc_can_err_skb() fails
- other small fixes
--- drivers/net/can/Kconfig | 2 + drivers/net/can/Makefile | 1 + drivers/net/can/m_can/Kconfig | 4 + drivers/net/can/m_can/Makefile | 7 + drivers/net/can/m_can/m_can.c | 1189 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 1203 insertions(+), 0 deletions(-) create mode 100644 drivers/net/can/m_can/Kconfig create mode 100644 drivers/net/can/m_can/Makefile create mode 100644 drivers/net/can/m_can/m_can.c
drivers/net/can/Kconfig +2 -0
@@ -143,6 +143,8 @@ source "drivers/net/can/sja1000/Kconfig" source "drivers/net/can/c_can/Kconfig" +source "drivers/net/can/m_can/Kconfig" + source "drivers/net/can/cc770/Kconfig" source "drivers/net/can/spi/Kconfig"
drivers/net/can/Makefile +1 -0
@@ -17,6 +17,7 @@ obj-y += softing/ obj-$(CONFIG_CAN_SJA1000) += sja1000/ obj-$(CONFIG_CAN_MSCAN) += mscan/ obj-$(CONFIG_CAN_C_CAN) += c_can/ +obj-$(CONFIG_CAN_M_CAN) += m_can/ obj-$(CONFIG_CAN_CC770) += cc770/ obj-$(CONFIG_CAN_AT91) += at91_can.o obj-$(CONFIG_CAN_TI_HECC) += ti_hecc.o
drivers/net/can/m_can/Kconfig new file +4 -0
@@ -0,0 +1,4 @@ +config CAN_M_CAN + tristate "Bosch M_CAN devices" + ---help--- + Say Y here if you want to support for Bosch M_CAN controller.
drivers/net/can/m_can/Makefile new file +7 -0
@@ -0,0 +1,7 @@ +# +# Makefile for the Bosch M_CAN controller drivers. +# + +obj-$(CONFIG_CAN_M_CAN) += m_can.o + +ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
drivers/net/can/m_can/m_can.c new file +1189 -0
1,189 lines not on this page · raw message
Signature · 1 line
1.7.8
PATCH v3 2/2 can: m_can: add Bosch M_CAN controller support
On 07/11/2014 03:59 PM, Dong Aisheng wrote:
(...)
Quoted 6 lines
drivers/net/can/m_can/m_can.c new file +6
-0+/* m_can private data structure */ +struct m_can_priv { +struct can_priv can;/* must be the first member */ +struct napi_struct napi; +struct net_device *dev; +struct device *device;
drivers/net/can/m_can/m_can.c line 255
Quoted 10 lines
drivers/net/can/m_can/m_can.c new file +10
-0+struct clk *hclk; +struct clk *cclk; +void __iomem *base; +u32 irqstatus; + +/* message ram configuration */ +void __iomem *mram_base; +struct mram_cfg mcfg[MRAM_CFG_NUM ]; +}; +
It will be good if we write the comments for the driver private structure
Quoted 5 lines
drivers/net/can/m_can/m_can.c new file +5
-0+static inline u32 m_can_read (const struct m_can_priv *priv,enum m_can_reg reg) +{ +return readl (priv->base + reg); +} +
(...)
drivers/net/can/m_can/m_can.c line 840
Quoted 5 lines
drivers/net/can/m_can/m_can.c new file +5
-0+static void free_m_can_dev (struct net_device *dev) +{ +free_candev (dev); +} +
Why do we need a separate function which calls a single function... :-)
drivers/net/can/m_can/m_can.c line 846
Quoted 6 lines
drivers/net/can/m_can/m_can.c new file +6
-0+static struct net_device *alloc_m_can_dev (void ) +{ +struct net_device *dev; +struct m_can_priv *priv; + + dev =alloc_candev (sizeof (struct m_can_priv ),1 );
sizeof(*priv)...?
drivers/net/can/m_can/m_can.c line 848
Quoted 2 lines
drivers/net/can/m_can/m_can.c new file +2
-0+if (!dev) +return NULL ;
Return value -ENOMEM ?
Quoted 28 lines
drivers/net/can/m_can/m_can.c new file +28
-0+ + priv =netdev_priv (dev); +netif_napi_add (dev, &priv->napi, m_can_poll,M_CAN_NAPI_WEIGHT ); + + priv->dev = dev; + priv->can.bittiming_const = &m_can_bittiming_const; + priv->can.do_set_mode = m_can_set_mode; + priv->can.do_get_berr_counter = m_can_get_berr_counter; + priv->can.ctrlmode_supported =CAN_CTRLMODE_LOOPBACK | +CAN_CTRLMODE_LISTENONLY | +CAN_CTRLMODE_BERR_REPORTING ; + +return dev; +} + +static int m_can_open (struct net_device *dev) +{ +struct m_can_priv *priv =netdev_priv (dev); +int err; + + err =clk_prepare_enable (priv->hclk); +if (err) +return err; + + err =clk_prepare_enable (priv->cclk); +if (err) + goto exit_disable_hclk; +
drivers/net/can/m_can/m_can.c line 886
Quoted 10 lines
drivers/net/can/m_can/m_can.c new file +10
-0+/* open the can device */ + err =open_candev (dev); +if (err) { +netdev_err (dev,"failed to open can device\n" ); + goto exit_disable_cclk; + } + +/* register interrupt handler */ + err =request_irq (dev->irq, m_can_isr,IRQF_SHARED , dev->name, + dev);
why don't we use devm_request_irq()...? If you use this no need to worry about freeing the irq
Quoted 35 lines
drivers/net/can/m_can/m_can.c new file +35
-0+if (err <0 ) { +netdev_err (dev,"failed to request interrupt\n" ); + goto exit_irq_fail; + } + +/* start the m_can controller */ +m_can_start (dev); + +can_led_event (dev,CAN_LED_EVENT_OPEN ); +napi_enable (&priv->napi); +netif_start_queue (dev); + +return 0 ; + +exit_irq_fail: +close_candev (dev); +exit_disable_cclk: +clk_disable_unprepare (priv->cclk); +exit_disable_hclk: +clk_disable_unprepare (priv->hclk); +return err; +} + +static void m_can_stop (struct net_device *dev) +{ +struct m_can_priv *priv =netdev_priv (dev); + +/* disable all interrupts */ +m_can_disable_all_interrupts (priv); + +clk_disable_unprepare (priv->hclk); +clk_disable_unprepare (priv->cclk); + +/* set the state as STOPPED */ + priv->can.state =CAN_STATE_STOPPED ;
drivers/net/can/m_can/m_can.c line 931
Quoted 10 lines
drivers/net/can/m_can/m_can.c new file +10
-0+} + +static int m_can_close (struct net_device *dev) +{ +struct m_can_priv *priv =netdev_priv (dev); + +netif_stop_queue (dev); +napi_disable (&priv->napi); +m_can_stop (dev); +free_irq (dev->irq, dev);
not required when you use devm_request_irq()
Quoted 6 lines
drivers/net/can/m_can/m_can.c new file +6
-0+close_candev (dev); +can_led_event (dev,CAN_LED_EVENT_STOP ); + +return 0 ; +} +
(...)
drivers/net/can/m_can/m_can.c line 992
Quoted 3 lines
drivers/net/can/m_can/m_can.c new file +3
-0+ +static const struct of_device_id m_can_of_table[] = { + { .compatible ="bosch,m_can" , .data =NULL },
we can simply give '0' . No need of .data = NULL. Things should be simple right.... :-)
Quoted 11 lines
drivers/net/can/m_can/m_can.c new file +11
-0+ { /* sentinel */ }, +}; +MODULE_DEVICE_TABLE (of, m_can_of_table); + +static int m_can_of_parse_mram (struct platform_device *pdev, +struct m_can_priv *priv) +{ +struct device_node *np = pdev->dev.of_node; +struct resource *res; +void __iomem *addr; +u32 out_val[MRAM_CFG_LEN ];
drivers/net/can/m_can/m_can.c line 1013
Quoted 10 lines
drivers/net/can/m_can/m_can.c new file +10
-0+int ret; + +/* message ram could be shared */ + res =platform_get_resource_byname (pdev,IORESOURCE_MEM ,"message_ram" ); +if (!res) +return -ENODEV ; + + addr =devm_ioremap (&pdev->dev, res->start,resource_size (res)); +if (!addr) +return -ENODEV ;
Is this err return is appropriate ... ?
drivers/net/can/m_can/m_can.c line 1022
Quoted 9 lines
drivers/net/can/m_can/m_can.c new file +9
-0+ +/* get message ram configuration */ + ret =of_property_read_u32_array (np,"mram-cfg" , + out_val,sizeof (out_val) /4 ); +if (ret) { +dev_err (&pdev->dev,"can not get message ram configuration\n" ); +return -ENODEV ; + } +
Is this err return is appropriate ... ?
Quoted 22 lines
drivers/net/can/m_can/m_can.c new file +22
-0+ priv->mram_base = addr; + priv->mcfg[MRAM_SIDF ].off = out_val[0 ]; + priv->mcfg[MRAM_SIDF ].num = out_val[1 ]; + priv->mcfg[MRAM_XIDF ].off = priv->mcfg[MRAM_SIDF ].off + + priv->mcfg[MRAM_SIDF ].num *SIDF_ELEMENT_SIZE ; + priv->mcfg[MRAM_XIDF ].num = out_val[2 ]; + priv->mcfg[MRAM_RXF0 ].off = priv->mcfg[MRAM_XIDF ].off + + priv->mcfg[MRAM_XIDF ].num *XIDF_ELEMENT_SIZE ; + priv->mcfg[MRAM_RXF0 ].num = out_val[3 ] &RXFC_FS_MASK ; + priv->mcfg[MRAM_RXF1 ].off = priv->mcfg[MRAM_RXF0 ].off + + priv->mcfg[MRAM_RXF0 ].num *RXF0_ELEMENT_SIZE ; + priv->mcfg[MRAM_RXF1 ].num = out_val[4 ] &RXFC_FS_MASK ; + priv->mcfg[MRAM_RXB ].off = priv->mcfg[MRAM_RXF1 ].off + + priv->mcfg[MRAM_RXF1 ].num *RXF1_ELEMENT_SIZE ; + priv->mcfg[MRAM_RXB ].num = out_val[5 ]; + priv->mcfg[MRAM_TXE ].off = priv->mcfg[MRAM_RXB ].off + + priv->mcfg[MRAM_RXB ].num *RXB_ELEMENT_SIZE ; + priv->mcfg[MRAM_TXE ].num = out_val[6 ]; + priv->mcfg[MRAM_TXB ].off = priv->mcfg[MRAM_TXE ].off + + priv->mcfg[MRAM_TXE ].num *TXE_ELEMENT_SIZE ; + priv->mcfg[MRAM_TXB ].num = out_val[7 ] &TXBC_NDTB_MASK ; +
drivers/net/can/m_can/m_can.c line 1054
Quoted 10 lines
drivers/net/can/m_can/m_can.c new file +10
-0+dev_dbg (&pdev->dev,"mram_base %p sidf 0x%x %d xidf 0x%x %d rxf0 0x%x %d rxf1 0x%x %d rxb 0x%x %d txe 0x%x %d txb 0x%x %d\n" , + priv->mram_base, + priv->mcfg[MRAM_SIDF ].off, priv->mcfg[MRAM_SIDF ].num, + priv->mcfg[MRAM_XIDF ].off, priv->mcfg[MRAM_XIDF ].num, + priv->mcfg[MRAM_RXF0 ].off, priv->mcfg[MRAM_RXF0 ].num, + priv->mcfg[MRAM_RXF1 ].off, priv->mcfg[MRAM_RXF1 ].num, + priv->mcfg[MRAM_RXB ].off, priv->mcfg[MRAM_RXB ].num, + priv->mcfg[MRAM_TXE ].off, priv->mcfg[MRAM_TXE ].num, + priv->mcfg[MRAM_TXB ].off, priv->mcfg[MRAM_TXB ].num); +
dev_dbg() will insert the new lines in b/w. It wont print the values as you expected. Check this by enabling debug ...
Quoted 3 lines
drivers/net/can/m_can/m_can.c new file +3
-0+return 0 ; +} +
(...)
drivers/net/can/m_can/m_can.c line 1157
Quoted 6 lines
drivers/net/can/m_can/m_can.c new file +6
-0+ +static void unregister_m_can_dev (struct net_device *dev) +{ +unregister_candev (dev); +} +
again a function which calls a single func.
Quoted 10 lines
drivers/net/can/m_can/m_can.c new file +10
-0+static int m_can_plat_remove (struct platform_device *pdev) +{ +struct net_device *dev =platform_get_drvdata (pdev); + +unregister_m_can_dev (dev); +platform_set_drvdata (pdev,NULL ); + +free_m_can_dev (dev); + +return 0 ;
drivers/net/can/m_can/m_can.c line 1177
Quoted 10 lines
drivers/net/can/m_can/m_can.c new file +10
-0+} + +static const struct dev_pm_ops m_can_pmops = { +SET_SYSTEM_SLEEP_PM_OPS (m_can_suspend, m_can_resume) +}; + +static struct platform_driver m_can_plat_driver = { + .driver = { + .name =KBUILD_MODNAME , + .owner =THIS_MODULE ,
No need to update .owner. module_platform_driver() will do for you. see:http://lxr.free-electrons.com/source/include/linux/platform_device.h#L190
Quoted 12 lines
drivers/net/can/m_can/m_can.c new file +12
-0+ .of_match_table = of_match_ptr(m_can_of_table), + .pm = &m_can_pmops, + }, + .probe = m_can_plat_probe, + .remove = m_can_plat_remove, +}; + +module_platform_driver (m_can_plat_driver); + +MODULE_AUTHOR ("Dong Aisheng <[email protected]>" ); +MODULE_LICENSE ("GPL v2" ); +MODULE_DESCRIPTION ("CAN bus driver for Bosch M_CAN controller" );
Signature · 2 lines
Regards, Varka Bhadram.
Marc Kleine-Budde
maintainerPATCH v3 2/2 can: m_can: add Bosch M_CAN controller support
Quoted 32 lines · Varka Bhadram
On 07/11/2014 03:59 PM, Dong Aisheng wrote:
(...)
Quoted 6 lines
drivers/net/can/m_can/m_can.c new file +6
-0+/* m_can private data structure */ +struct m_can_priv { +struct can_priv can;/* must be the first member */ +struct napi_struct napi; +struct net_device *dev; +struct device *device;drivers/net/can/m_can/m_can.c line 255
Quoted 10 lines
drivers/net/can/m_can/m_can.c new file +10
-0+struct clk *hclk; +struct clk *cclk; +void __iomem *base; +u32 irqstatus; + +/* message ram configuration */ +void __iomem *mram_base; +struct mram_cfg mcfg[MRAM_CFG_NUM ]; +}; +It will be good if we write the comments for the driver private structure
Quoted 6 lines
+static inline u32
m_can_read(const struct m_can_priv *priv, enum m_can_reg reg)drivers/net/can/m_can/m_can.c new file +4
-0+{ +return readl (priv->base + reg); +} +(...)
Quoted 7 lines
drivers/net/can/m_can/m_can.c line 840
Quoted 5 lines
drivers/net/can/m_can/m_can.c new file +5
-0+static void free_m_can_dev (struct net_device *dev) +{ +free_candev (dev); +} +Why do we need a separate function which calls a single function... :-)
To be symetric with alloc_m_can_dev()
Quoted 10 lines
drivers/net/can/m_can/m_can.c line 846
Quoted 7 lines
drivers/net/can/m_can/m_can.c new file +6
-0+static struct net_device *alloc_m_can_dev (void ) +{ +struct net_device *dev; +struct m_can_priv *priv; + + dev =alloc_candev (sizeof (struct m_can_priv ),1 );
sizeof(*priv)...?
Quoted 4 lines
drivers/net/can/m_can/m_can.c line 848
Quoted 2 lines
drivers/net/can/m_can/m_can.c new file +2
-0+if (!dev) +return NULL ;Return value
-ENOMEM?
I'm okay with NULL, however if we want to return an arror value, it must be ERR_PTR() wrapped.
Quoted 34 lines
Quoted 34 lines
drivers/net/can/m_can/m_can.c new file +33
-0+ + priv =netdev_priv (dev); +netif_napi_add (dev, &priv->napi, m_can_poll,M_CAN_NAPI_WEIGHT ); + + priv->dev = dev; + priv->can.bittiming_const = &m_can_bittiming_const; + priv->can.do_set_mode = m_can_set_mode; + priv->can.do_get_berr_counter = m_can_get_berr_counter; + priv->can.ctrlmode_supported =CAN_CTRLMODE_LOOPBACK | +CAN_CTRLMODE_LISTENONLY | +CAN_CTRLMODE_BERR_REPORTING ; + +return dev; +} + +static int m_can_open (struct net_device *dev) +{ +struct m_can_priv *priv =netdev_priv (dev); +int err; + + err =clk_prepare_enable (priv->hclk); +if (err) +return err; + + err =clk_prepare_enable (priv->cclk); +if (err) + goto exit_disable_hclk; + +/* open the can device */ + err =open_candev (dev); +if (err) { +netdev_err (dev,"failed to open can device\n" ); + goto exit_disable_cclk;
Quoted 8 lines
drivers/net/can/m_can/m_can.c line 886
Quoted 5 lines
drivers/net/can/m_can/m_can.c new file +5
-0+ } + +/* register interrupt handler */ + err =request_irq (dev->irq, m_can_isr,IRQF_SHARED , dev->name, + dev);why don't we use
devm_request_irq()...? If you use this no need to worry about freeing the irq
...because the IRQ is allocated during ifup and released during ifdown.
Quoted 40 lines
Quoted 40 lines
drivers/net/can/m_can/m_can.c new file +39
-0+if (err <0 ) { +netdev_err (dev,"failed to request interrupt\n" ); + goto exit_irq_fail; + } + +/* start the m_can controller */ +m_can_start (dev); + +can_led_event (dev,CAN_LED_EVENT_OPEN ); +napi_enable (&priv->napi); +netif_start_queue (dev); + +return 0 ; + +exit_irq_fail: +close_candev (dev); +exit_disable_cclk: +clk_disable_unprepare (priv->cclk); +exit_disable_hclk: +clk_disable_unprepare (priv->hclk); +return err; +} + +static void m_can_stop (struct net_device *dev) +{ +struct m_can_priv *priv =netdev_priv (dev); + +/* disable all interrupts */ +m_can_disable_all_interrupts (priv); + +clk_disable_unprepare (priv->hclk); +clk_disable_unprepare (priv->cclk); + +/* set the state as STOPPED */ + priv->can.state =CAN_STATE_STOPPED ; +} + +static int m_can_close (struct net_device *dev) +{
Quoted 8 lines
drivers/net/can/m_can/m_can.c line 931
Quoted 6 lines
drivers/net/can/m_can/m_can.c new file +6
-0+struct m_can_priv *priv =netdev_priv (dev); + +netif_stop_queue (dev); +napi_disable (&priv->napi); +m_can_stop (dev); +free_irq (dev->irq, dev);not required when you use
devm_request_irq()
No....see above.
Quoted 10 lines
Quoted 7 lines
drivers/net/can/m_can/m_can.c new file +6
-0+close_candev (dev); +can_led_event (dev,CAN_LED_EVENT_STOP ); + +return 0 ; +} +(...)
Quoted 6 lines
drivers/net/can/m_can/m_can.c line 992
Quoted 3 lines
drivers/net/can/m_can/m_can.c new file +3
-0+ +static const struct of_device_id m_can_of_table[] = { + { .compatible ="bosch,m_can" , .data =NULL },we can simply give '0' . No need of .data = NULL. Things should be simple right.... :-)
.data should be a pointer, while "0" isn't. (Although 0 is valid C, we don't want a integer 0 to initialize a pointer.) However, you can omit .data = NULL completely. When initialzing via C99, any omited members of the struct will automatically be initialized with 0x0. I like to see the .data = NULL because it documents that there isn't any data (yet), once another compatible is added, we need the .data anyways.
Quoted 17 lines
Quoted 17 lines
drivers/net/can/m_can/m_can.c new file +15
-0+ { /* sentinel */ }, +}; +MODULE_DEVICE_TABLE (of, m_can_of_table); + +static int m_can_of_parse_mram (struct platform_device *pdev, +struct m_can_priv *priv) +{ +struct device_node *np = pdev->dev.of_node; +struct resource *res; +void __iomem *addr; +u32 out_val[MRAM_CFG_LEN ]; +int ret; + +/* message ram could be shared */ + res = platform_get_resource_byname(pdev, IORESOURCE_MEM,"
message_ram");
Quoted 8 lines
drivers/net/can/m_can/m_can.c line 1013
Quoted 6 lines
drivers/net/can/m_can/m_can.c new file +6
-0+if (!res) +return -ENODEV ; + + addr =devm_ioremap (&pdev->dev, res->start,resource_size (res)); +if (!addr) +return -ENODEV ;Is this err return is appropriate ... ?
-ENOMEM seems to be more commonly used.
Quoted 12 lines
drivers/net/can/m_can/m_can.c line 1022
Quoted 10 lines
drivers/net/can/m_can/m_can.c new file +9
-0+ +/* get message ram configuration */ + ret =of_property_read_u32_array (np,"mram-cfg" , + out_val,sizeof (out_val) /4 ); +if (ret) { +dev_err (&pdev->dev,"can not get message ram configuration\n" ); +return -ENODEV ; + } +Is this err return is appropriate ... ?
Whay do you suggest?
Quoted 30 lines
Quoted 30 lines
drivers/net/can/m_can/m_can.c new file +23
-0+ priv->mram_base = addr; + priv->mcfg[MRAM_SIDF ].off = out_val[0 ]; + priv->mcfg[MRAM_SIDF ].num = out_val[1 ]; + priv->mcfg[MRAM_XIDF ].off = priv->mcfg[MRAM_SIDF ].off + + priv->mcfg[MRAM_SIDF ].num *SIDF_ELEMENT_SIZE ; + priv->mcfg[MRAM_XIDF ].num = out_val[2 ]; + priv->mcfg[MRAM_RXF0 ].off = priv->mcfg[MRAM_XIDF ].off + + priv->mcfg[MRAM_XIDF ].num *XIDF_ELEMENT_SIZE ; + priv->mcfg[MRAM_RXF0 ].num = out_val[3 ] &RXFC_FS_MASK ; + priv->mcfg[MRAM_RXF1 ].off = priv->mcfg[MRAM_RXF0 ].off + + priv->mcfg[MRAM_RXF0 ].num *RXF0_ELEMENT_SIZE ; + priv->mcfg[MRAM_RXF1 ].num = out_val[4 ] &RXFC_FS_MASK ; + priv->mcfg[MRAM_RXB ].off = priv->mcfg[MRAM_RXF1 ].off + + priv->mcfg[MRAM_RXF1 ].num *RXF1_ELEMENT_SIZE ; + priv->mcfg[MRAM_RXB ].num = out_val[5 ]; + priv->mcfg[MRAM_TXE ].off = priv->mcfg[MRAM_RXB ].off + + priv->mcfg[MRAM_RXB ].num *RXB_ELEMENT_SIZE ; + priv->mcfg[MRAM_TXE ].num = out_val[6 ]; + priv->mcfg[MRAM_TXB ].off = priv->mcfg[MRAM_TXE ].off + + priv->mcfg[MRAM_TXE ].num *TXE_ELEMENT_SIZE ; + priv->mcfg[MRAM_TXB ].num = out_val[7 ] &TXBC_NDTB_MASK ; + + dev_dbg(&pdev->dev, "mram_base %p sidf 0x%x %d xidf 0x%x %d rxf00x%x %d rxf1 0x%x %d rxb 0x%x %d txe 0x%x %d txb 0x%x %d\n",
drivers/net/can/m_can/m_can.c new file +5
-0+ priv->mram_base, + priv->mcfg[MRAM_SIDF ].off, priv->mcfg[MRAM_SIDF ].num, + priv->mcfg[MRAM_XIDF ].off, priv->mcfg[MRAM_XIDF ].num, + priv->mcfg[MRAM_RXF0 ].off, priv->mcfg[MRAM_RXF0 ].num, + priv->mcfg[MRAM_RXF1].off, priv->mcfg[MRAM_RXF1].num,
Quoted 8 lines
drivers/net/can/m_can/m_can.c line 1054
Quoted 4 lines
drivers/net/can/m_can/m_can.c new file +4
-0+ priv->mcfg[MRAM_RXB ].off, priv->mcfg[MRAM_RXB ].num, + priv->mcfg[MRAM_TXE ].off, priv->mcfg[MRAM_TXE ].num, + priv->mcfg[MRAM_TXB].off, priv->mcfg[MRAM_TXB].num); +
dev_dbg()will insert the new lines in b/w. It wont print the values as you expected.
Check this by enabling debug ...
What do you mean by b/w?
Quoted 31 lines
Quoted 4 lines
drivers/net/can/m_can/m_can.c new file +3
-0+return 0 ; +} +(...)
drivers/net/can/m_can/m_can.c line 1157
Quoted 6 lines
drivers/net/can/m_can/m_can.c new file +6
-0+ +static void unregister_m_can_dev (struct net_device *dev) +{ +unregister_candev (dev); +} +again a function which calls a single func.
Quoted 15 lines
drivers/net/can/m_can/m_can.c new file +15
-0+static int m_can_plat_remove (struct platform_device *pdev) +{ +struct net_device *dev =platform_get_drvdata (pdev); + +unregister_m_can_dev (dev); +platform_set_drvdata (pdev,NULL ); + +free_m_can_dev (dev); + +return 0 ; +} + +static const struct dev_pm_ops m_can_pmops = { +SET_SYSTEM_SLEEP_PM_OPS (m_can_suspend, m_can_resume) +};
Quoted 8 lines
drivers/net/can/m_can/m_can.c line 1177
Quoted 5 lines
drivers/net/can/m_can/m_can.c new file +5
-0+ +static struct platform_driver m_can_plat_driver = { + .driver = { + .name =KBUILD_MODNAME , + .owner =THIS_MODULE ,No need to update .owner.
module_platform_driver()will do for you. see:http://lxr.free-electrons.com/source/include/linux/platform_device.h#L190
Oh, right.
Quoted 4 lines
Quoted 4 lines
drivers/net/can/m_can/m_can.c new file +4
-0+ .of_match_table = of_match_ptr(m_can_of_table), + .pm = &m_can_pmops, + }, + .probe = m_can_plat_probe,
Quoted 10 lines
drivers/net/can/m_can/m_can.c line 1189
Quoted 8 lines
drivers/net/can/m_can/m_can.c new file +8
-0+ .remove = m_can_plat_remove, +}; + +module_platform_driver (m_can_plat_driver); + +MODULE_AUTHOR ("Dong Aisheng <[email protected]>" ); +MODULE_LICENSE ("GPL v2" ); +MODULE_DESCRIPTION ("CAN bus driver for Bosch M_CAN controller" );
Marc
Signature · 12 lines
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 | -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 242 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140711/4d33bd33/attachment.sig>
PATCH v3 2/2 can: m_can: add Bosch M_CAN controller support
Quoted 156 lines · Marc Kleine-Budde
Quoted 30 lines · Varka Bhadram
On 07/11/2014 03:59 PM, Dong Aisheng wrote:
(...)
Quoted 6 lines
drivers/net/can/m_can/m_can.c new file +6
-0+/* m_can private data structure */ +struct m_can_priv { +struct can_priv can;/* must be the first member */ +struct napi_struct napi; +struct net_device *dev; +struct device *device;drivers/net/can/m_can/m_can.c line 255
Quoted 10 lines
drivers/net/can/m_can/m_can.c new file +10
-0+struct clk *hclk; +struct clk *cclk; +void __iomem *base; +u32 irqstatus; + +/* message ram configuration */ +void __iomem *mram_base; +struct mram_cfg mcfg[MRAM_CFG_NUM ]; +}; +It will be good if we write the comments for the driver private structure
Quoted 6 lines
+static inline u32
m_can_read(const struct m_can_priv *priv, enum m_can_reg reg)drivers/net/can/m_can/m_can.c new file +4
-0+{ +return readl (priv->base + reg); +} +(...)
Quoted 6 lines
drivers/net/can/m_can/m_can.c line 840
Quoted 5 lines
drivers/net/can/m_can/m_can.c new file +5
-0+static void free_m_can_dev (struct net_device *dev) +{ +free_candev (dev); +} +Why do we need a separate function which calls a single function... :-)
To be symetric with
alloc_m_can_dev()Quoted 11 lines
drivers/net/can/m_can/m_can.c line 846
Quoted 6 lines
drivers/net/can/m_can/m_can.c new file +6
-0+static struct net_device *alloc_m_can_dev (void ) +{ +struct net_device *dev; +struct m_can_priv *priv; + + dev =alloc_candev (sizeof (struct m_can_priv ),1 );
sizeof(*priv)...?drivers/net/can/m_can/m_can.c line 848
Quoted 2 lines
drivers/net/can/m_can/m_can.c new file +2
-0+if (!dev) +return NULL ;Return value
-ENOMEM?I'm okay with NULL, however if we want to return an arror value, it must be
ERR_PTR()wrapped.Quoted 32 lines
Quoted 32 lines
drivers/net/can/m_can/m_can.c new file +32
-0+ + priv =netdev_priv (dev); +netif_napi_add (dev, &priv->napi, m_can_poll,M_CAN_NAPI_WEIGHT ); + + priv->dev = dev; + priv->can.bittiming_const = &m_can_bittiming_const; + priv->can.do_set_mode = m_can_set_mode; + priv->can.do_get_berr_counter = m_can_get_berr_counter; + priv->can.ctrlmode_supported =CAN_CTRLMODE_LOOPBACK | +CAN_CTRLMODE_LISTENONLY | +CAN_CTRLMODE_BERR_REPORTING ; + +return dev; +} + +static int m_can_open (struct net_device *dev) +{ +struct m_can_priv *priv =netdev_priv (dev); +int err; + + err =clk_prepare_enable (priv->hclk); +if (err) +return err; + + err =clk_prepare_enable (priv->cclk); +if (err) + goto exit_disable_hclk; + +/* open the can device */ + err =open_candev (dev); +if (err) { +netdev_err (dev,"failed to open can device\n" );Quoted 8 lines
drivers/net/can/m_can/m_can.c line 886
Quoted 6 lines
drivers/net/can/m_can/m_can.c new file +6
-0+ goto exit_disable_cclk; + } + +/* register interrupt handler */ + err =request_irq (dev->irq, m_can_isr,IRQF_SHARED , dev->name, + dev);why don't we use
devm_request_irq()...? If you use this no need to worry about freeing the irq...because the IRQ is allocated during ifup and released during ifdown.
Quoted 38 lines
Quoted 38 lines
drivers/net/can/m_can/m_can.c new file +38
-0+if (err <0 ) { +netdev_err (dev,"failed to request interrupt\n" ); + goto exit_irq_fail; + } + +/* start the m_can controller */ +m_can_start (dev); + +can_led_event (dev,CAN_LED_EVENT_OPEN ); +napi_enable (&priv->napi); +netif_start_queue (dev); + +return 0 ; + +exit_irq_fail: +close_candev (dev); +exit_disable_cclk: +clk_disable_unprepare (priv->cclk); +exit_disable_hclk: +clk_disable_unprepare (priv->hclk); +return err; +} + +static void m_can_stop (struct net_device *dev) +{ +struct m_can_priv *priv =netdev_priv (dev); + +/* disable all interrupts */ +m_can_disable_all_interrupts (priv); + +clk_disable_unprepare (priv->hclk); +clk_disable_unprepare (priv->cclk); + +/* set the state as STOPPED */ + priv->can.state =CAN_STATE_STOPPED ; +} + +static int m_can_close (struct net_device *dev)Quoted 8 lines
drivers/net/can/m_can/m_can.c line 931
Quoted 7 lines
drivers/net/can/m_can/m_can.c new file +7
-0+{ +struct m_can_priv *priv =netdev_priv (dev); + +netif_stop_queue (dev); +napi_disable (&priv->napi); +m_can_stop (dev); +free_irq (dev->irq, dev);not required when you use
devm_request_irq()No....see above.
Quoted 8 lines
Quoted 6 lines
drivers/net/can/m_can/m_can.c new file +6
-0+close_candev (dev); +can_led_event (dev,CAN_LED_EVENT_STOP ); + +return 0 ; +} +(...)
Quoted 5 lines
drivers/net/can/m_can/m_can.c line 992
Quoted 3 lines
drivers/net/can/m_can/m_can.c new file +3
-0+ +static const struct of_device_id m_can_of_table[] = { + { .compatible ="bosch,m_can" , .data =NULL },we can simply give '0' . No need of .data = NULL. Things should be simple right.... :-)
Quoted 6 lines
.data should be a pointer, while "0" isn't. (Although 0 is valid C, we don't want a integer 0 to initialize a pointer.) However, you can omit .data = NULL completely. When initialzing via C99, any omited members of the struct will automatically be initialized with
0x0. I like to see the .data = NULL because it documents that there isn't any data (yet), once another compatible is added, we need the .data anyways.
static const struct of_device_id m_can_of_table[] = {
{ .compatible = "bosch,m_can", },
};This is enough... right ?
Quoted 73 lines
Quoted 15 lines
Quoted 15 lines
drivers/net/can/m_can/m_can.c new file +15
-0+ { /* sentinel */ }, +}; +MODULE_DEVICE_TABLE (of, m_can_of_table); + +static int m_can_of_parse_mram (struct platform_device *pdev, +struct m_can_priv *priv) +{ +struct device_node *np = pdev->dev.of_node; +struct resource *res; +void __iomem *addr; +u32 out_val[MRAM_CFG_LEN ]; +int ret; + +/* message ram could be shared */ + res = platform_get_resource_byname(pdev, IORESOURCE_MEM,Quoted 8 lines
drivers/net/can/m_can/m_can.c line 1013
Quoted 7 lines
"
message_ram");drivers/net/can/m_can/m_can.c new file +6
-0+if (!res) +return -ENODEV ; + + addr =devm_ioremap (&pdev->dev, res->start,resource_size (res)); +if (!addr) +return -ENODEV ;Is this err return is appropriate ... ?
-ENOMEMseems to be more commonly used.Quoted 10 lines
drivers/net/can/m_can/m_can.c line 1022
Quoted 9 lines
drivers/net/can/m_can/m_can.c new file +9
-0+ +/* get message ram configuration */ + ret =of_property_read_u32_array (np,"mram-cfg" , + out_val,sizeof (out_val) /4 ); +if (ret) { +dev_err (&pdev->dev,"can not get message ram configuration\n" ); +return -ENODEV ; + } +Is this err return is appropriate ... ?
Whay do you suggest?
Quoted 28 lines
Quoted 28 lines
drivers/net/can/m_can/m_can.c new file +23
-0+ priv->mram_base = addr; + priv->mcfg[MRAM_SIDF ].off = out_val[0 ]; + priv->mcfg[MRAM_SIDF ].num = out_val[1 ]; + priv->mcfg[MRAM_XIDF ].off = priv->mcfg[MRAM_SIDF ].off + + priv->mcfg[MRAM_SIDF ].num *SIDF_ELEMENT_SIZE ; + priv->mcfg[MRAM_XIDF ].num = out_val[2 ]; + priv->mcfg[MRAM_RXF0 ].off = priv->mcfg[MRAM_XIDF ].off + + priv->mcfg[MRAM_XIDF ].num *XIDF_ELEMENT_SIZE ; + priv->mcfg[MRAM_RXF0 ].num = out_val[3 ] &RXFC_FS_MASK ; + priv->mcfg[MRAM_RXF1 ].off = priv->mcfg[MRAM_RXF0 ].off + + priv->mcfg[MRAM_RXF0 ].num *RXF0_ELEMENT_SIZE ; + priv->mcfg[MRAM_RXF1 ].num = out_val[4 ] &RXFC_FS_MASK ; + priv->mcfg[MRAM_RXB ].off = priv->mcfg[MRAM_RXF1 ].off + + priv->mcfg[MRAM_RXF1 ].num *RXF1_ELEMENT_SIZE ; + priv->mcfg[MRAM_RXB ].num = out_val[5 ]; + priv->mcfg[MRAM_TXE ].off = priv->mcfg[MRAM_RXB ].off + + priv->mcfg[MRAM_RXB ].num *RXB_ELEMENT_SIZE ; + priv->mcfg[MRAM_TXE ].num = out_val[6 ]; + priv->mcfg[MRAM_TXB ].off = priv->mcfg[MRAM_TXE ].off + + priv->mcfg[MRAM_TXE ].num *TXE_ELEMENT_SIZE ; + priv->mcfg[MRAM_TXB ].num = out_val[7 ] &TXBC_NDTB_MASK ; + + dev_dbg(&pdev->dev, "mram_base %p sidf 0x%x %d xidf 0x%x %d rxf00x%x %d rxf1 0x%x %d rxb 0x%x %d txe 0x%x %d txb 0x%x %d\n",
drivers/net/can/m_can/m_can.c new file +4
-0+ priv->mram_base, + priv->mcfg[MRAM_SIDF ].off, priv->mcfg[MRAM_SIDF ].num, + priv->mcfg[MRAM_XIDF ].off, priv->mcfg[MRAM_XIDF ].num, + priv->mcfg[MRAM_RXF0].off, priv->mcfg[MRAM_RXF0].num,Quoted 8 lines
drivers/net/can/m_can/m_can.c line 1054
Quoted 5 lines
drivers/net/can/m_can/m_can.c new file +5
-0+ priv->mcfg[MRAM_RXF1 ].off, priv->mcfg[MRAM_RXF1 ].num, + priv->mcfg[MRAM_RXB ].off, priv->mcfg[MRAM_RXB ].num, + priv->mcfg[MRAM_TXE ].off, priv->mcfg[MRAM_TXE ].num, + priv->mcfg[MRAM_TXB].off, priv->mcfg[MRAM_TXB].num); +
dev_dbg()will insert the new lines in b/w. It wont print the values as you expected.
Check this by enabling debug ...
Quoted 1 line
What do you mean by b/w?
You are expecting the data to be print in format like:
pdev->dev/name: mram_base %p sidf 0x%x %d xidf 0x%x %d rxf0 0x%x %d rxf1 0x%x %d rxb 0x%x %d txe 0x%x %d txb 0x%x %d
But when we use the dev_dbg()/pr_debug()... It will put data like:
pdev->dev/name: mram_base %p sidf 0x%x
0x%x %d rxf0 0x%x
rxf1 0x%x %d rxb
....
check this by enable DEBUG...
Quoted 52 lines
Quoted 27 lines
Quoted 3 lines
drivers/net/can/m_can/m_can.c new file +3
-0+return 0 ; +} +(...)
drivers/net/can/m_can/m_can.c line 1157
Quoted 6 lines
drivers/net/can/m_can/m_can.c new file +6
-0+ +static void unregister_m_can_dev (struct net_device *dev) +{ +unregister_candev (dev); +} +again a function which calls a single func.
Quoted 14 lines
drivers/net/can/m_can/m_can.c new file +14
-0+static int m_can_plat_remove (struct platform_device *pdev) +{ +struct net_device *dev =platform_get_drvdata (pdev); + +unregister_m_can_dev (dev); +platform_set_drvdata (pdev,NULL ); + +free_m_can_dev (dev); + +return 0 ; +} + +static const struct dev_pm_ops m_can_pmops = { +SET_SYSTEM_SLEEP_PM_OPS (m_can_suspend, m_can_resume)Quoted 8 lines
drivers/net/can/m_can/m_can.c line 1177
Quoted 6 lines
drivers/net/can/m_can/m_can.c new file +6
-0+}; + +static struct platform_driver m_can_plat_driver = { + .driver = { + .name =KBUILD_MODNAME , + .owner =THIS_MODULE ,No need to update .owner.
module_platform_driver()will do for you. see:http://lxr.free-electrons.com/source/include/linux/platform_device.h#L190Oh, right.
Quoted 4 lines
Quoted 4 lines
drivers/net/can/m_can/m_can.c new file +4
-0+ .of_match_table = of_match_ptr(m_can_of_table), + .pm = &m_can_pmops, + }, + .probe = m_can_plat_probe,Quoted 9 lines
drivers/net/can/m_can/m_can.c line 1189
Quoted 8 lines
drivers/net/can/m_can/m_can.c new file +8
-0+ .remove = m_can_plat_remove, +}; + +module_platform_driver (m_can_plat_driver); + +MODULE_AUTHOR ("Dong Aisheng <[email protected]>" ); +MODULE_LICENSE ("GPL v2" ); +MODULE_DESCRIPTION ("CAN bus driver for Bosch M_CAN controller" );Marc
Signature · 2 lines
Regards, Varka Bhadram.
Marc Kleine-Budde
maintainerPATCH v3 2/2 can: m_can: add Bosch M_CAN controller support
On 07/11/2014 02:13 PM, Varka Bhadram wrote:
[...]
Quoted 11 lines
Quoted 10 lines
Quoted 4 lines
drivers/net/can/m_can/m_can.c line 992
Quoted 2 lines
drivers/net/can/m_can/m_can.c new file +2
-0+static const struct of_device_id m_can_of_table[] = { + { .compatible ="bosch,m_can" , .data =NULL },we can simply give '0' . No need of .data = NULL. Things should be simple right.... :-)
.data should be a pointer, while "0" isn't. (Although 0 is valid C, we don't want a integer 0 to initialize a pointer.) However, you can omit .data = NULL completely. When initialzing via C99, any omited members of the struct will automatically be initialized with
0x0. I like to see the .data = NULL because it documents that there isn't any data (yet), once another compatible is added, we need the .data anyways.
Quoted 5 lines
static const struct of_device_id m_can_of_table[] = { { .compatible = "bosch,m_can", }, };This is enough... right ?
Yes....
Not having .data = NULL is correct, but I'd like to see it, just to
document: "there is no data needed, nobody forgot it"
...but...
Just must have a NULL-element terminating that list:
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
+ { /* sentinel */ },
^^^^^^^^^^^^^^^^^^
Quoted 74 lines
Quoted 74 lines
Quoted 15 lines
Quoted 15 lines
drivers/net/can/m_can/m_can.c new file +14
-0+}; +MODULE_DEVICE_TABLE (of, m_can_of_table); + +static int m_can_of_parse_mram (struct platform_device *pdev, +struct m_can_priv *priv) +{ +struct device_node *np = pdev->dev.of_node; +struct resource *res; +void __iomem *addr; +u32 out_val[MRAM_CFG_LEN ]; +int ret; + +/* message ram could be shared */ + res = platform_get_resource_byname(pdev, IORESOURCE_MEM,Quoted 8 lines
drivers/net/can/m_can/m_can.c line 1013
Quoted 7 lines
"
message_ram");drivers/net/can/m_can/m_can.c new file +6
-0+if (!res) +return -ENODEV ; + + addr =devm_ioremap (&pdev->dev, res->start,resource_size (res)); +if (!addr) +return -ENODEV ;Is this err return is appropriate ... ?
-ENOMEMseems to be more commonly used.Quoted 11 lines
drivers/net/can/m_can/m_can.c line 1022
Quoted 10 lines
drivers/net/can/m_can/m_can.c new file +6
-0+ +/* get message ram configuration */ + ret =of_property_read_u32_array (np,"mram-cfg" , + out_val,sizeof (out_val) /4 ); +if (ret) { + dev_err(&pdev->dev, "can not get message ramconfiguration\n");
drivers/net/can/m_can/m_can.c new file +3
-0+return -ENODEV ; + } +Is this err return is appropriate ... ?
Whay do you suggest?
Quoted 36 lines
Quoted 24 lines
drivers/net/can/m_can/m_can.c new file +23
-0+ priv->mram_base = addr; + priv->mcfg[MRAM_SIDF ].off = out_val[0 ]; + priv->mcfg[MRAM_SIDF ].num = out_val[1 ]; + priv->mcfg[MRAM_XIDF ].off = priv->mcfg[MRAM_SIDF ].off + + priv->mcfg[MRAM_SIDF ].num *SIDF_ELEMENT_SIZE ; + priv->mcfg[MRAM_XIDF ].num = out_val[2 ]; + priv->mcfg[MRAM_RXF0 ].off = priv->mcfg[MRAM_XIDF ].off + + priv->mcfg[MRAM_XIDF ].num *XIDF_ELEMENT_SIZE ; + priv->mcfg[MRAM_RXF0 ].num = out_val[3 ] &RXFC_FS_MASK ; + priv->mcfg[MRAM_RXF1 ].off = priv->mcfg[MRAM_RXF0 ].off + + priv->mcfg[MRAM_RXF0 ].num *RXF0_ELEMENT_SIZE ; + priv->mcfg[MRAM_RXF1 ].num = out_val[4 ] &RXFC_FS_MASK ; + priv->mcfg[MRAM_RXB ].off = priv->mcfg[MRAM_RXF1 ].off + + priv->mcfg[MRAM_RXF1 ].num *RXF1_ELEMENT_SIZE ; + priv->mcfg[MRAM_RXB ].num = out_val[5 ]; + priv->mcfg[MRAM_TXE ].off = priv->mcfg[MRAM_RXB ].off + + priv->mcfg[MRAM_RXB ].num *RXB_ELEMENT_SIZE ; + priv->mcfg[MRAM_TXE ].num = out_val[6 ]; + priv->mcfg[MRAM_TXB ].off = priv->mcfg[MRAM_TXE ].off + + priv->mcfg[MRAM_TXE ].num *TXE_ELEMENT_SIZE ; + priv->mcfg[MRAM_TXB ].num = out_val[7 ] &TXBC_NDTB_MASK ; + + dev_dbg(&pdev->dev, "mram_base %p sidf 0x%x %d xidf 0x%x %d rxf00x%x %d rxf1 0x%x %d rxb 0x%x %d txe 0x%x %d txb 0x%x %d\n",
drivers/net/can/m_can/m_can.c line 1054
Quoted 9 lines
drivers/net/can/m_can/m_can.c new file +9
-0+ priv->mram_base, + priv->mcfg[MRAM_SIDF ].off, priv->mcfg[MRAM_SIDF ].num, + priv->mcfg[MRAM_XIDF ].off, priv->mcfg[MRAM_XIDF ].num, + priv->mcfg[MRAM_RXF0 ].off, priv->mcfg[MRAM_RXF0 ].num, + priv->mcfg[MRAM_RXF1 ].off, priv->mcfg[MRAM_RXF1 ].num, + priv->mcfg[MRAM_RXB ].off, priv->mcfg[MRAM_RXB ].num, + priv->mcfg[MRAM_TXE ].off, priv->mcfg[MRAM_TXE ].num, + priv->mcfg[MRAM_TXB].off, priv->mcfg[MRAM_TXB].num); +
dev_dbg()will insert the new lines in b/w. It wont print the values as you expected.
Check this by enabling debug ...
Quoted 1 line
Quoted 1 line
What do you mean by b/w?
b/w == between ?
here: b/w != black/white :)
Quoted 11 lines
You are expecting the data to be print in format like:
pdev->dev/name:mram_base%p sidf 0x%x %d xidf 0x%x %d rxf0 0x%x %d rxf1 0x%x %d rxb 0x%x %d txe 0x%x %d txb 0x%x %dBut when we use the dev_dbg()/pr_debug()... It will put data like:
pdev->dev/name:mram_base%p sidf 0x%x
0x%x %d rxf0 0x%x
rxf1 0x%x %d rxb
....check this by enable DEBUG...
Okay, thanks for pointing out.
Marc
Signature · 12 lines
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 | -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 242 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140711/af371ea0/attachment.sig>
PATCH v3 2/2 can: m_can: add Bosch M_CAN controller support
In reply to Varka BhadramQuoted 248 lines · Varka Bhadram
Quoted 156 lines · Marc Kleine-Budde
Quoted 30 lines · Varka Bhadram
On 07/11/2014 03:59 PM, Dong Aisheng wrote:
(...)
Quoted 6 lines
drivers/net/can/m_can/m_can.c new file +6
-0+/* m_can private data structure */ +struct m_can_priv { +struct can_priv can;/* must be the first member */ +struct napi_struct napi; +struct net_device *dev; +struct device *device;drivers/net/can/m_can/m_can.c line 255
Quoted 10 lines
drivers/net/can/m_can/m_can.c new file +10
-0+struct clk *hclk; +struct clk *cclk; +void __iomem *base; +u32 irqstatus; + +/* message ram configuration */ +void __iomem *mram_base; +struct mram_cfg mcfg[MRAM_CFG_NUM ]; +}; +It will be good if we write the comments for the driver private structure
Quoted 6 lines
+static inline u32
m_can_read(const struct m_can_priv *priv, enum m_can_reg reg)drivers/net/can/m_can/m_can.c new file +4
-0+{ +return readl (priv->base + reg); +} +(...)
Quoted 6 lines
drivers/net/can/m_can/m_can.c line 840
Quoted 5 lines
drivers/net/can/m_can/m_can.c new file +5
-0+static void free_m_can_dev (struct net_device *dev) +{ +free_candev (dev); +} +Why do we need a separate function which calls a single function... :-)
To be symetric with
alloc_m_can_dev()Quoted 11 lines
drivers/net/can/m_can/m_can.c line 846
Quoted 6 lines
drivers/net/can/m_can/m_can.c new file +6
-0+static struct net_device *alloc_m_can_dev (void ) +{ +struct net_device *dev; +struct m_can_priv *priv; + + dev =alloc_candev (sizeof (struct m_can_priv ),1 );
sizeof(*priv)...?drivers/net/can/m_can/m_can.c line 848
Quoted 2 lines
drivers/net/can/m_can/m_can.c new file +2
-0+if (!dev) +return NULL ;Return value
-ENOMEM?I'm okay with NULL, however if we want to return an arror value, it must be
ERR_PTR()wrapped.Quoted 32 lines
Quoted 32 lines
drivers/net/can/m_can/m_can.c new file +32
-0+ + priv =netdev_priv (dev); +netif_napi_add (dev, &priv->napi, m_can_poll,M_CAN_NAPI_WEIGHT ); + + priv->dev = dev; + priv->can.bittiming_const = &m_can_bittiming_const; + priv->can.do_set_mode = m_can_set_mode; + priv->can.do_get_berr_counter = m_can_get_berr_counter; + priv->can.ctrlmode_supported =CAN_CTRLMODE_LOOPBACK | +CAN_CTRLMODE_LISTENONLY | +CAN_CTRLMODE_BERR_REPORTING ; + +return dev; +} + +static int m_can_open (struct net_device *dev) +{ +struct m_can_priv *priv =netdev_priv (dev); +int err; + + err =clk_prepare_enable (priv->hclk); +if (err) +return err; + + err =clk_prepare_enable (priv->cclk); +if (err) + goto exit_disable_hclk; + +/* open the can device */ + err =open_candev (dev); +if (err) { +netdev_err (dev,"failed to open can device\n" );Quoted 8 lines
drivers/net/can/m_can/m_can.c line 886
Quoted 6 lines
drivers/net/can/m_can/m_can.c new file +6
-0+ goto exit_disable_cclk; + } + +/* register interrupt handler */ + err =request_irq (dev->irq, m_can_isr,IRQF_SHARED , dev->name, + dev);why don't we use
devm_request_irq()...? If you use this no need to worry about freeing the irq...because the IRQ is allocated during ifup and released during ifdown.
Quoted 38 lines
Quoted 38 lines
drivers/net/can/m_can/m_can.c new file +38
-0+if (err <0 ) { +netdev_err (dev,"failed to request interrupt\n" ); + goto exit_irq_fail; + } + +/* start the m_can controller */ +m_can_start (dev); + +can_led_event (dev,CAN_LED_EVENT_OPEN ); +napi_enable (&priv->napi); +netif_start_queue (dev); + +return 0 ; + +exit_irq_fail: +close_candev (dev); +exit_disable_cclk: +clk_disable_unprepare (priv->cclk); +exit_disable_hclk: +clk_disable_unprepare (priv->hclk); +return err; +} + +static void m_can_stop (struct net_device *dev) +{ +struct m_can_priv *priv =netdev_priv (dev); + +/* disable all interrupts */ +m_can_disable_all_interrupts (priv); + +clk_disable_unprepare (priv->hclk); +clk_disable_unprepare (priv->cclk); + +/* set the state as STOPPED */ + priv->can.state =CAN_STATE_STOPPED ; +} + +static int m_can_close (struct net_device *dev)Quoted 8 lines
drivers/net/can/m_can/m_can.c line 931
Quoted 7 lines
drivers/net/can/m_can/m_can.c new file +7
-0+{ +struct m_can_priv *priv =netdev_priv (dev); + +netif_stop_queue (dev); +napi_disable (&priv->napi); +m_can_stop (dev); +free_irq (dev->irq, dev);not required when you use
devm_request_irq()No....see above.
Quoted 8 lines
Quoted 6 lines
drivers/net/can/m_can/m_can.c new file +6
-0+close_candev (dev); +can_led_event (dev,CAN_LED_EVENT_STOP ); + +return 0 ; +} +(...)
Quoted 5 lines
drivers/net/can/m_can/m_can.c line 992
Quoted 3 lines
drivers/net/can/m_can/m_can.c new file +3
-0+ +static const struct of_device_id m_can_of_table[] = { + { .compatible ="bosch,m_can" , .data =NULL },we can simply give '0' . No need of .data = NULL. Things should be simple right.... :-)
Quoted 6 lines
.data should be a pointer, while "0" isn't. (Although 0 is valid C, we don't want a integer 0 to initialize a pointer.) However, you can omit .data = NULL completely. When initialzing via C99, any omited members of the struct will automatically be initialized with
0x0. I like to see the .data = NULL because it documents that there isn't any data (yet), once another compatible is added, we need the .data anyways.static const struct of_device_id m_can_of_table[] = { { .compatible = "bosch,m_can", }, };This is enough... right ?
Quoted 73 lines
Quoted 15 lines
Quoted 15 lines
drivers/net/can/m_can/m_can.c new file +15
-0+ { /* sentinel */ }, +}; +MODULE_DEVICE_TABLE (of, m_can_of_table); + +static int m_can_of_parse_mram (struct platform_device *pdev, +struct m_can_priv *priv) +{ +struct device_node *np = pdev->dev.of_node; +struct resource *res; +void __iomem *addr; +u32 out_val[MRAM_CFG_LEN ]; +int ret; + +/* message ram could be shared */ + res = platform_get_resource_byname(pdev, IORESOURCE_MEM,Quoted 8 lines
drivers/net/can/m_can/m_can.c line 1013
Quoted 7 lines
"
message_ram");drivers/net/can/m_can/m_can.c new file +6
-0+if (!res) +return -ENODEV ; + + addr =devm_ioremap (&pdev->dev, res->start,resource_size (res)); +if (!addr) +return -ENODEV ;Is this err return is appropriate ... ?
-ENOMEMseems to be more commonly used.Quoted 10 lines
drivers/net/can/m_can/m_can.c line 1022
Quoted 9 lines
drivers/net/can/m_can/m_can.c new file +9
-0+ +/* get message ram configuration */ + ret =of_property_read_u32_array (np,"mram-cfg" , + out_val,sizeof (out_val) /4 ); +if (ret) { +dev_err (&pdev->dev,"can not get message ram configuration\n" ); +return -ENODEV ; + } +Is this err return is appropriate ... ?
Whay do you suggest?
Quoted 28 lines
Quoted 28 lines
drivers/net/can/m_can/m_can.c new file +23
-0+ priv->mram_base = addr; + priv->mcfg[MRAM_SIDF ].off = out_val[0 ]; + priv->mcfg[MRAM_SIDF ].num = out_val[1 ]; + priv->mcfg[MRAM_XIDF ].off = priv->mcfg[MRAM_SIDF ].off + + priv->mcfg[MRAM_SIDF ].num *SIDF_ELEMENT_SIZE ; + priv->mcfg[MRAM_XIDF ].num = out_val[2 ]; + priv->mcfg[MRAM_RXF0 ].off = priv->mcfg[MRAM_XIDF ].off + + priv->mcfg[MRAM_XIDF ].num *XIDF_ELEMENT_SIZE ; + priv->mcfg[MRAM_RXF0 ].num = out_val[3 ] &RXFC_FS_MASK ; + priv->mcfg[MRAM_RXF1 ].off = priv->mcfg[MRAM_RXF0 ].off + + priv->mcfg[MRAM_RXF0 ].num *RXF0_ELEMENT_SIZE ; + priv->mcfg[MRAM_RXF1 ].num = out_val[4 ] &RXFC_FS_MASK ; + priv->mcfg[MRAM_RXB ].off = priv->mcfg[MRAM_RXF1 ].off + + priv->mcfg[MRAM_RXF1 ].num *RXF1_ELEMENT_SIZE ; + priv->mcfg[MRAM_RXB ].num = out_val[5 ]; + priv->mcfg[MRAM_TXE ].off = priv->mcfg[MRAM_RXB ].off + + priv->mcfg[MRAM_RXB ].num *RXB_ELEMENT_SIZE ; + priv->mcfg[MRAM_TXE ].num = out_val[6 ]; + priv->mcfg[MRAM_TXB ].off = priv->mcfg[MRAM_TXE ].off + + priv->mcfg[MRAM_TXE ].num *TXE_ELEMENT_SIZE ; + priv->mcfg[MRAM_TXB ].num = out_val[7 ] &TXBC_NDTB_MASK ; + + dev_dbg(&pdev->dev, "mram_base %p sidf 0x%x %d xidf 0x%x %d rxf00x%x %d rxf1 0x%x %d rxb 0x%x %d txe 0x%x %d txb 0x%x %d\n",
drivers/net/can/m_can/m_can.c new file +4
-0+ priv->mram_base, + priv->mcfg[MRAM_SIDF ].off, priv->mcfg[MRAM_SIDF ].num, + priv->mcfg[MRAM_XIDF ].off, priv->mcfg[MRAM_XIDF ].num, + priv->mcfg[MRAM_RXF0].off, priv->mcfg[MRAM_RXF0].num,Quoted 8 lines
drivers/net/can/m_can/m_can.c line 1054
Quoted 5 lines
drivers/net/can/m_can/m_can.c new file +5
-0+ priv->mcfg[MRAM_RXF1 ].off, priv->mcfg[MRAM_RXF1 ].num, + priv->mcfg[MRAM_RXB ].off, priv->mcfg[MRAM_RXB ].num, + priv->mcfg[MRAM_TXE ].off, priv->mcfg[MRAM_TXE ].num, + priv->mcfg[MRAM_TXB].off, priv->mcfg[MRAM_TXB].num); +
dev_dbg()will insert the new lines in b/w. It wont print the values as you expected.
Check this by enabling debug ...Quoted 1 line
What do you mean by b/w?
You are expecting the data to be print in format like:
pdev->dev/name:mram_base%p sidf 0x%x %d xidf 0x%x %d rxf0 0x%x %d rxf1 0x%x %d rxb 0x%x %d txe 0x%x %d txb 0x%x %d
Quoted 8 lines
But when we use the dev_dbg()/pr_debug()... It will put data like:
pdev->dev/name:mram_base%p sidf 0x%x
0x%x %d rxf0 0x%x
rxf1 0x%x %d rxb
....check this by enable DEBUG...
My test showed the format is:
root at imx6qdlsolo:~# uname -a
Linux imx6qdlsolo 3.16.0-rc2-next-20140627-00006-gd55dd62-dirty #373 SMP Fri Jul 11 18:12:31 CST 2014 armv7l GNU/Linux root at imx6qdlsolo:~# dmesg | grep m_canm_can 20e8000.can: mram_base c0990000 sidf 0x0 0 xidf 0x0 0 rxf0 0x0 32 rxf1 0x200 0 rxb 0x200 0 txe 0x200 0 txb 0x200 1 m_can 20e8000.can: m_can device registered (regs=c0988000, irq=146)m_can 20f0000.can: mram_base c09a0000 sidf 0x400 0 xidf 0x400 0 rxf0 0x400 32 rxf1 0x600 0 rxb 0x600 0 txe 0x600 0 txb 0x600 1 m_can 20f0000.can: m_can device registered (regs=c0998000, irq=147)
It looks ok.
I'm not sure about the issue you said.
I just enabled the DEBUG by adding #define DEBUG line on the top of m_can.c.
Anyting i miss understood?
Regards
Dong Aisheng
Quoted 58 lines
Quoted 52 lines
Quoted 27 lines
Quoted 3 lines
drivers/net/can/m_can/m_can.c new file +3
-0+return 0 ; +} +(...)
drivers/net/can/m_can/m_can.c line 1157
Quoted 6 lines
drivers/net/can/m_can/m_can.c new file +6
-0+ +static void unregister_m_can_dev (struct net_device *dev) +{ +unregister_candev (dev); +} +again a function which calls a single func.
Quoted 14 lines
drivers/net/can/m_can/m_can.c new file +14
-0+static int m_can_plat_remove (struct platform_device *pdev) +{ +struct net_device *dev =platform_get_drvdata (pdev); + +unregister_m_can_dev (dev); +platform_set_drvdata (pdev,NULL ); + +free_m_can_dev (dev); + +return 0 ; +} + +static const struct dev_pm_ops m_can_pmops = { +SET_SYSTEM_SLEEP_PM_OPS (m_can_suspend, m_can_resume)Quoted 8 lines
drivers/net/can/m_can/m_can.c line 1177
Quoted 6 lines
drivers/net/can/m_can/m_can.c new file +6
-0+}; + +static struct platform_driver m_can_plat_driver = { + .driver = { + .name =KBUILD_MODNAME , + .owner =THIS_MODULE ,No need to update .owner.
module_platform_driver()will do for you. see:http://lxr.free-electrons.com/source/include/linux/platform_device.h#L190Oh, right.
Quoted 4 lines
Quoted 4 lines
drivers/net/can/m_can/m_can.c new file +4
-0+ .of_match_table = of_match_ptr(m_can_of_table), + .pm = &m_can_pmops, + }, + .probe = m_can_plat_probe,Quoted 9 lines
drivers/net/can/m_can/m_can.c line 1189
Quoted 8 lines
drivers/net/can/m_can/m_can.c new file +8
-0+ .remove = m_can_plat_remove, +}; + +module_platform_driver (m_can_plat_driver); + +MODULE_AUTHOR ("Dong Aisheng <[email protected]>" ); +MODULE_LICENSE ("GPL v2" ); +MODULE_DESCRIPTION ("CAN bus driver for Bosch M_CAN controller" );Marc
Signature · 2 lines
Regards, Varka Bhadram.
PATCH v3 2/2 can: m_can: add Bosch M_CAN controller support
Quoted 27 lines · Dong Aisheng
+ dev_dbg(&pdev->dev, "
mram_base%p sidf 0x%x %d xidf 0x%x %d rxf0
0x%x %d rxf1 0x%x %d rxb 0x%x %d txe 0x%x %d txb 0x%x %d\n",drivers/net/can/m_can/m_can.c new file +9
-0+ priv->mram_base, + priv->mcfg[MRAM_SIDF ].off, priv->mcfg[MRAM_SIDF ].num, + priv->mcfg[MRAM_XIDF ].off, priv->mcfg[MRAM_XIDF ].num, + priv->mcfg[MRAM_RXF0 ].off, priv->mcfg[MRAM_RXF0 ].num, + priv->mcfg[MRAM_RXF1 ].off, priv->mcfg[MRAM_RXF1 ].num, + priv->mcfg[MRAM_RXB ].off, priv->mcfg[MRAM_RXB ].num, + priv->mcfg[MRAM_TXE ].off, priv->mcfg[MRAM_TXE ].num, + priv->mcfg[MRAM_TXB].off, priv->mcfg[MRAM_TXB].num); +Quoted 7 lines
Quoted 4 lines
Quoted 3 lines
dev_dbg()will insert the new lines in b/w. It wont print the values as you expected.
Check this by enabling debug ...What do you mean by b/w?
You are expecting the data to be print in format like:
pdev->dev/name:mram_base%p sidf 0x%x %d xidf 0x%x %d rxf0 0x%x %d rxf1 0x%x %d rxb 0x%x %d txe 0x%x %d txb 0x%x %dQuoted 8 lines
But when we use the dev_dbg()/pr_debug()... It will put data like:
pdev->dev/name:mram_base%p sidf 0x%x
0x%x %d rxf0 0x%x
rxf1 0x%x %d rxb
....check this by enable DEBUG...
Quoted 9 lines
My test showed the format is:
root at imx6qdlsolo:~# uname -a
Linux imx6qdlsolo 3.16.0-rc2-next-20140627-00006-gd55dd62-dirty #373 SMP Fri Jul 11 18:12:31 CST 2014 armv7l GNU/Linux root at imx6qdlsolo:~# dmesg | grepm_canm_can20e8000.can:mram_basec0990000sidf0x00 xidf0x00 rxf00x032 rxf10x2000 rxb0x2000 txe0x2000 txb0x2001m_can20e8000.can:m_candevice registered (regs=c0988000, irq=146)m_can20f0000.can:mram_basec09a0000sidf0x4000 xidf0x4000 rxf00x40032 rxf10x6000 rxb0x6000 txe0x6000 txb0x6001m_can20f0000.can:m_candevice registered (regs=c0998000, irq=147)
I think you got the expected result.
I faced the above problem when i use pr_debug(). Can you check with this....?
This is good. Thanks for clarification.
Quoted 4 lines
Regards
Dong Aisheng
Signature · 2 lines
Regards, Varka Bhadram.
PATCH v3 2/2 can: m_can: add Bosch M_CAN controller support
Quoted 37 lines · Varka Bhadram
Quoted 27 lines · Dong Aisheng
+ dev_dbg(&pdev->dev, "
mram_base%p sidf 0x%x %d xidf 0x%x %d rxf0
0x%x %d rxf1 0x%x %d rxb 0x%x %d txe 0x%x %d txb 0x%x %d\n",drivers/net/can/m_can/m_can.c new file +9
-0+ priv->mram_base, + priv->mcfg[MRAM_SIDF ].off, priv->mcfg[MRAM_SIDF ].num, + priv->mcfg[MRAM_XIDF ].off, priv->mcfg[MRAM_XIDF ].num, + priv->mcfg[MRAM_RXF0 ].off, priv->mcfg[MRAM_RXF0 ].num, + priv->mcfg[MRAM_RXF1 ].off, priv->mcfg[MRAM_RXF1 ].num, + priv->mcfg[MRAM_RXB ].off, priv->mcfg[MRAM_RXB ].num, + priv->mcfg[MRAM_TXE ].off, priv->mcfg[MRAM_TXE ].num, + priv->mcfg[MRAM_TXB].off, priv->mcfg[MRAM_TXB].num); +Quoted 7 lines
Quoted 4 lines
Quoted 3 lines
dev_dbg()will insert the new lines in b/w. It wont print the values as you expected.
Check this by enabling debug ...What do you mean by b/w?
You are expecting the data to be print in format like:
pdev->dev/name:mram_base%p sidf 0x%x %d xidf 0x%x %d rxf0 0x%x %d rxf1 0x%x %d rxb 0x%x %d txe 0x%x %d txb 0x%x %dQuoted 8 lines
But when we use the dev_dbg()/pr_debug()... It will put data like:
pdev->dev/name:mram_base%p sidf 0x%x
0x%x %d rxf0 0x%x
rxf1 0x%x %d rxb
....check this by enable DEBUG...
Quoted 9 lines
My test showed the format is:
root at imx6qdlsolo:~# uname -a
Linux imx6qdlsolo 3.16.0-rc2-next-20140627-00006-gd55dd62-dirty #373 SMP Fri Jul 11 18:12:31 CST 2014 armv7l GNU/Linux root at imx6qdlsolo:~# dmesg | grepm_canm_can20e8000.can:mram_basec0990000sidf0x00 xidf0x00 rxf00x032 rxf10x2000 rxb0x2000 txe0x2000 txb0x2001m_can20e8000.can:m_candevice registered (regs=c0988000, irq=146)m_can20f0000.can:mram_basec09a0000sidf0x4000 xidf0x4000 rxf00x40032 rxf10x6000 rxb0x6000 txe0x6000 txb0x6001m_can20f0000.can:m_candevice registered (regs=c0998000, irq=147)
Quoted 4 lines
I think you got the expected result.
I faced the above problem when i use
pr_debug(). Can you check with this....?
It works too.
root at imx6qdlsolo:~# dmesg | grep mrammram_base c0990000 sidf 0x0 0 xidf 0x0 0 rxf0 0x0 32 rxf1 0x200 0 rxb 0x200 0 txe 0x200 0 txb 0x200 1 mram_base c09a0000 sidf 0x400 0 xidf 0x400 0 rxf0 0x400 32 rxf1 0x600 0 rxb 0x600 0 txe 0x600 0 txb 0x600 1
Quoted 10 lines
This is good. Thanks for clarification.
Quoted 4 lines
Regards
Dong AishengSignature · 2 lines
Regards, Varka Bhadram.
Regards
Dong Aisheng
PATCH v3 2/2 can: m_can: add Bosch M_CAN controller support
Quoted 37 lines · Dong Aisheng
Quoted 36 lines · Varka Bhadram
Quoted 35 lines · Dong Aisheng
+ dev_dbg(&pdev->dev, "
mram_base%p sidf 0x%x %d xidf 0x%x %d rxf0
0x%x %d rxf1 0x%x %d rxb 0x%x %d txe 0x%x %d txb 0x%x %d\n",(quoted diff) +9
-0+ priv->mram_base, + priv->mcfg[MRAM_SIDF].off, priv->mcfg[MRAM_SIDF].num, + priv->mcfg[MRAM_XIDF].off, priv->mcfg[MRAM_XIDF].num, + priv->mcfg[MRAM_RXF0].off, priv->mcfg[MRAM_RXF0].num, + priv->mcfg[MRAM_RXF1].off, priv->mcfg[MRAM_RXF1].num, + priv->mcfg[MRAM_RXB].off, priv->mcfg[MRAM_RXB].num, + priv->mcfg[MRAM_TXE].off, priv->mcfg[MRAM_TXE].num, + priv->mcfg[MRAM_TXB].off, priv->mcfg[MRAM_TXB].num); +Quoted 7 lines
Quoted 4 lines
Quoted 3 lines
dev_dbg()will insert the new lines in b/w. It wont print the values as you expected.
Check this by enabling debug ...What do you mean by b/w?
You are expecting the data to be print in format like:
pdev->dev/name:mram_base%p sidf 0x%x %d xidf 0x%x %d rxf0 0x%x %d rxf1 0x%x %d rxb 0x%x %d txe 0x%x %d txb 0x%x %dQuoted 8 lines
But when we use the dev_dbg()/pr_debug()... It will put data like:
pdev->dev/name:mram_base%p sidf 0x%x
0x%x %d rxf0 0x%x
rxf1 0x%x %d rxb
....check this by enable DEBUG...
My test showed the format is:
root at imx6qdlsolo:~# uname -a
Linux imx6qdlsolo 3.16.0-rc2-next-20140627-00006-gd55dd62-dirty #373 SMP Fri Jul 11 18:12:31 CST 2014 armv7l GNU/Linux root at imx6qdlsolo:~# dmesg | grepm_canm_can20e8000.can:mram_basec0990000sidf0x00 xidf0x00 rxf00x032 rxf10x2000 rxb0x2000 txe0x2000 txb0x2001m_can20e8000.can:m_candevice registered (regs=c0988000, irq=146)m_can20f0000.can:mram_basec09a0000sidf0x4000 xidf0x4000 rxf00x40032 rxf10x6000 rxb0x6000 txe0x6000 txb0x6001m_can20f0000.can:m_candevice registered (regs=c0998000, irq=147)
Quoted 9 lines
Quoted 4 lines
I think you got the expected result.
I faced the above problem when i use
pr_debug(). Can you check with this....?It works too.
root at imx6qdlsolo:~# dmesg | grep mrammram_basec0990000sidf0x00 xidf0x00 rxf00x032 rxf10x2000 rxb0x2000 txe0x2000 txb0x2001mram_basec09a0000sidf0x4000 xidf0x4000 rxf00x40032 rxf10x6000 rxb0x6000 txe0x6000 txb0x6001
Ohooooo... I got the problem because i used pr_debug in a 'for' loop.
Any way thanks.
Quoted 11 lines
Quoted 9 lines
This is good. Thanks for clarification.
Quoted 3 lines
Regards
Dong AishengSignature · 2 lines
Regards, Varka Bhadram.Regards
Dong Aisheng
Signature · 2 lines
Regards, Varka Bhadram.
PATCH v3 1/2 can: m_can: add device tree binding documentation
In reply to Dong AishengQuoted 25 lines · Dong Aisheng
add
M_CANdevice tree binding documentationCc Wolfgang Grandegger <[email protected]>Cc Marc Kleine-Budde <[email protected]>Cc Mark Rutland <[email protected]>Cc Oliver Hartkopp <[email protected]>Cc Varka Bhadram <[email protected]>Signed-off-by Dong Aisheng <[email protected]>--- .../devicetree/bindings/net/can/m_can.txt | 65 ++++++++++++++++++++ 1 files changed, 65 insertions(+), 0 deletions(-)create mode 100644
Documentation/devicetree/bindings/net/can/m_can.txtDocumentation/devicetree/bindings/net/can/m_can.txt new file +6
-0@@ -0,0 +1,65 @@ +Bosch MCAN controller Device Tree Bindings +------------------------------------------------- + +Required properties: +- compatible : Should be "bosch,m_can" for M_CAN controllers +- reg : physical base address and size of the M_CAN
Documentation/devicetree/bindings/net/can/m_can.txt line 16
Quoted 10 lines
Documentation/devicetree/bindings/net/can/m_can.txt new file +10
-0+ registers map and Message RAM +- reg-names : Should be "m_can" and "message_ram" +- interrupts : Should be the interrupt number of M_CAN interrupt + line 0 and line 1, could be same if sharing + the same interrupt. +- interrupt-names : Should contain "int0" and "int1" +- clocks : Clocks used by controller, should be host clock + and CAN clock. +- clock-names : Should contain "hclk" and "cclk" +- pinctrl-<n> : Pinctrl states as described in bindings/pinctrl/pinctrl-bindings.txt
I think this should be pinctrl-0
Documentation/devicetree/bindings/net/can/m_can.txt line 17
Quoted 1 line
Documentation/devicetree/bindings/net/can/m_can.txt new file +1
-0+- pinctrl-names : Names corresponding to the numbered pinctrl states
remove 1 tab space before :
Documentation/devicetree/bindings/net/can/m_can.txt line 23
Quoted 6 lines
Documentation/devicetree/bindings/net/can/m_can.txt new file +6
-0+- mram-cfg : Message RAM configuration data. + Multiple M_CAN instances can share the same Message RAM and each element(e.g + Rx FIFO or Tx Buffer and etc) number in Message RAM is also configurable, + so this property is telling driver how the shared or private Message RAM + are used by this M_CAN controller. +
It may written like:
mram-cfg : Message RAM configuration data
Multiple M_CAN instances can share the same Message RAM and each element (e.g Rx FIFO or Tx Buffer and etc) number in Message RAM is also configurable,
...
Quoted 42 lines
Documentation/devicetree/bindings/net/can/m_can.txt new file +42
-0+ The format should be as follows: + <offset sidf_elems xidf_elems rxf0_elems rxf1_elems rxb_elems + txe_elems txb_elems> + The 'offset' is an address offset of the Message RAM where the following + elements start from. This is usually set to 0x0 if you're using a private + Message RAM. The remain cells are used to specify how many elements are used + for each FIFO/Buffer. + +M_CAN includes the following elements according to user manual: +11-bit Filter 0-128 elements / 0-128 words +29-bit Filter 0-64 elements / 0-128 words +Rx FIFO 0 0-64 elements / 0-1152 words +Rx FIFO 1 0-64 elements / 0-1152 words +Rx Buffers 0-64 elements / 0-1152 words +Tx Event FIFO 0-32 elements / 0-64 words +Tx Buffers 0-32 elements / 0-576 words + +Please refer to 2.4.1 Message RAM Configuration in Bosch M_CAN user manual +for details. + +Example: +SoC dtsi: +m_can1: can at 020e8000 { + compatible = "bosch,m_can"; + reg = <0x020e8000 0x4000>, <0x02298000 0x4000>; + reg-names = "m_can", "message_ram"; + interrupts = <0 114 0x04>, + <0 114 0x04>; + interrupt-names = "int0", "int1"; + clocks = <&clks IMX6SX_CLK_CANFD>, + <&clks IMX6SX_CLK_CANFD>; + clock-names = "hclk", "cclk"; + mram-cfg = <0x0 0 0 32 0 0 0 1>; + status = "disabled"; +}; + +Board dtsi: +&m_can1 { + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_m_can1>; + status = "enabled"; +};
Signature · 2 lines
Regards, Varka Bhadram.
PATCH v3 1/2 can: m_can: add device tree binding documentation
Quoted 30 lines · Varka Bhadram
Quoted 29 lines · Dong Aisheng
add
M_CANdevice tree binding documentationCc Wolfgang Grandegger <[email protected]>Cc Marc Kleine-Budde <[email protected]>Cc Mark Rutland <[email protected]>Cc Oliver Hartkopp <[email protected]>Cc Varka Bhadram <[email protected]>Signed-off-by Dong Aisheng <[email protected]>--- .../devicetree/bindings/net/can/m_can.txt | 65 ++++++++++++++++++++ 1 files changed, 65 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/net/can/m_can.txtDocumentation/devicetree/bindings/net/can/m_can.txt new file +10
-0@@ -0,0 +1,65 @@ +Bosch MCAN controller Device Tree Bindings +------------------------------------------------- + +Required properties: +- compatible : Should be "bosch,m_can" for M_CAN controllers +- reg : physical base address and size of the M_CAN + registers map and Message RAM +- reg-names : Should be "m_can" and "message_ram" +- interrupts : Should be the interrupt number of M_CAN interrupt + line 0 and line 1, could be same if sharing
Quoted 9 lines
Documentation/devicetree/bindings/net/can/m_can.txt line 16
Quoted 6 lines
Documentation/devicetree/bindings/net/can/m_can.txt new file +6
-0+ the same interrupt. +- interrupt-names : Should contain "int0" and "int1" +- clocks : Clocks used by controller, should be host clock + and CAN clock. +- clock-names : Should contain "hclk" and "cclk" +- pinctrl-<n> : Pinctrl states as described in bindings/pinctrl/pinctrl-bindings.txtI think this should be pinctrl-0
First, this part is defined by pinctrl binding doc.
Second i think it may be possible someone wants to add other pinctrl states when implement low power state in the future.
So i just keep it as pinctrl-<n>.
Quoted 4 lines
Documentation/devicetree/bindings/net/can/m_can.txt line 17
Quoted 1 line
Documentation/devicetree/bindings/net/can/m_can.txt new file +1
-0+- pinctrl-names : Names corresponding to the numbered pinctrl statesremove 1 tab space before :
It's a bit strange.
Other line like pinctrl-<n> is also two tabs.
And the code looks fine and already aligned.
- pinctrl-<n> : Pinctrl states as described in bindings/pinctrl/pinctrl-bindings.txt
- pinctrl-names : Names corresponding to the numbered pinctrl states
Do you mean change line of pinctrl-names from two tabs to one space and a tab before :?
Quoted 7 lines
Documentation/devicetree/bindings/net/can/m_can.txt line 23
Quoted 6 lines
Documentation/devicetree/bindings/net/can/m_can.txt new file +6
-0+- mram-cfg : Message RAM configuration data. + Multiple M_CAN instances can share the same Message RAM and each element(e.g + Rx FIFO or Tx Buffer and etc) number in Message RAM is also configurable, + so this property is telling driver how the shared or private Message RAM + are used by this M_CAN controller. +
Quoted 6 lines
It may written like:
mram-cfg : Message RAM configuration data
MultipleM_CANinstances can share the same Message RAM and each element (e.g Rx FIFO or Tx Buffer and etc) number in Message RAM is also configurable,
...
I'm fine with that.
The question is it's easy to over 80 columns if writing like that, is it ok?
Regards
Dong Aisheng
Quoted 48 lines
Quoted 42 lines
Documentation/devicetree/bindings/net/can/m_can.txt new file +42
-0+ The format should be as follows: + <offset sidf_elems xidf_elems rxf0_elems rxf1_elems rxb_elems + txe_elems txb_elems> + The 'offset' is an address offset of the Message RAM where the following + elements start from. This is usually set to 0x0 if you're using a private + Message RAM. The remain cells are used to specify how many elements are used + for each FIFO/Buffer. + +M_CAN includes the following elements according to user manual: +11-bit Filter 0-128 elements / 0-128 words +29-bit Filter 0-64 elements / 0-128 words +Rx FIFO 0 0-64 elements / 0-1152 words +Rx FIFO 1 0-64 elements / 0-1152 words +Rx Buffers 0-64 elements / 0-1152 words +Tx Event FIFO 0-32 elements / 0-64 words +Tx Buffers 0-32 elements / 0-576 words + +Please refer to 2.4.1 Message RAM Configuration in Bosch M_CAN user manual +for details. + +Example: +SoC dtsi: +m_can1: can at 020e8000 { + compatible = "bosch,m_can"; + reg = <0x020e8000 0x4000>, <0x02298000 0x4000>; + reg-names = "m_can", "message_ram"; + interrupts = <0 114 0x04>, + <0 114 0x04>; + interrupt-names = "int0", "int1"; + clocks = <&clks IMX6SX_CLK_CANFD>, + <&clks IMX6SX_CLK_CANFD>; + clock-names = "hclk", "cclk"; + mram-cfg = <0x0 0 0 32 0 0 0 1>; + status = "disabled"; +}; + +Board dtsi: +&m_can1 { + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_m_can1>; + status = "enabled"; +};Signature · 2 lines
Regards, Varka Bhadram.
PATCH v3 1/2 can: m_can: add device tree binding documentation
Quoted 39 lines · Dong Aisheng
Quoted 29 lines · Varka Bhadram
Quoted 28 lines · Dong Aisheng
add
M_CANdevice tree binding documentationCc Wolfgang Grandegger <[email protected]>Cc Marc Kleine-Budde <[email protected]>Cc Mark Rutland <[email protected]>Cc Oliver Hartkopp <[email protected]>Cc Varka Bhadram <[email protected]>Signed-off-by Dong Aisheng <[email protected]>--- .../devicetree/bindings/net/can/m_can.txt | 65 ++++++++++++++++++++ 1 files changed, 65 insertions(+), 0 deletions(-)create mode 100644
Documentation/devicetree/bindings/net/can/m_can.txtDocumentation/devicetree/bindings/net/can/m_can.txt new file +9
-0@@ -0,0 +1,65 @@ +Bosch MCAN controller Device Tree Bindings +------------------------------------------------- + +Required properties: +- compatible : Should be "bosch,m_can" for M_CAN controllers +- reg : physical base address and size of the M_CAN + registers map and Message RAM +- reg-names : Should be "m_can" and "message_ram" +- interrupts : Should be the interrupt number of M_CAN interruptQuoted 9 lines
Documentation/devicetree/bindings/net/can/m_can.txt line 16
Quoted 7 lines
Documentation/devicetree/bindings/net/can/m_can.txt new file +7
-0+ line 0 and line 1, could be same if sharing + the same interrupt. +- interrupt-names : Should contain "int0" and "int1" +- clocks : Clocks used by controller, should be host clock + and CAN clock. +- clock-names : Should contain "hclk" and "cclk" +- pinctrl-<n> : Pinctrl states as described in bindings/pinctrl/pinctrl-bindings.txtI think this should be pinctrl-0
Quoted 4 lines
First, this part is defined by pinctrl binding doc.
Second i think it may be possible someone wants to add other pinctrl states when implement low power state in the future.
So i just keep it as pinctrl-<n>.
Normally we will use pinctrl-0 for mentioning the pinctrl bindings.
If somebody going to add something to the pinctrl bindings they will add separately with pinctrl-1: bla bla bla...
Quoted 10 lines
Quoted 3 lines
Documentation/devicetree/bindings/net/can/m_can.txt line 17
Quoted 1 line
Documentation/devicetree/bindings/net/can/m_can.txt new file +1
-0+- pinctrl-names : Names corresponding to the numbered pinctrl statesremove 1 tab space before :
It's a bit strange.
Other line like pinctrl-<n> is also two tabs.
And the code looks fine and already aligned.
- pinctrl-<n> : Pinctrl states as described inbindings/pinctrl/pinctrl-bindings.txt
- pinctrl-names : Names corresponding to the numbered pinctrl states
Do you mean change line of pinctrl-names from two tabs to one space and a tab before :?
When i see in the patch the alignment is like this
pinctrl-<n> :
pinctrl-name :
Quoted 12 lines
Quoted 6 lines
Documentation/devicetree/bindings/net/can/m_can.txt line 23
Quoted 6 lines
Documentation/devicetree/bindings/net/can/m_can.txt new file +6
-0+- mram-cfg : Message RAM configuration data. + Multiple M_CAN instances can share the same Message RAM and each element(e.g + Rx FIFO or Tx Buffer and etc) number in Message RAM is also configurable, + so this property is telling driver how the shared or private Message RAM + are used by this M_CAN controller. +Quoted 6 lines
It may written like:
mram-cfg : Message RAM configuration data
MultipleM_CANinstances can share the same Message RAM and each element (e.g Rx FIFO or Tx Buffer and etc) number in Message RAM is also configurable,
...
Quoted 3 lines
I'm fine with that.
The question is it's easy to over 80 columns if writing like that, is it ok?
When we follow the above format it would be more readable.
Quoted 2 lines
Regards
Dong Aisheng
Signature · 2 lines
Regards, Varka Bhadram.
PATCH v3 1/2 can: m_can: add device tree binding documentation
Quoted 45 lines · Varka Bhadram
Quoted 39 lines · Dong Aisheng
Quoted 29 lines · Varka Bhadram
Quoted 28 lines · Dong Aisheng
add
M_CANdevice tree binding documentationCc Wolfgang Grandegger <[email protected]>Cc Marc Kleine-Budde <[email protected]>Cc Mark Rutland <[email protected]>Cc Oliver Hartkopp <[email protected]>Cc Varka Bhadram <[email protected]>Signed-off-by Dong Aisheng <[email protected]>--- .../devicetree/bindings/net/can/m_can.txt | 65 ++++++++++++++++++++ 1 files changed, 65 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/net/can/m_can.txtDocumentation/devicetree/bindings/net/can/m_can.txt new file +9
-0@@ -0,0 +1,65 @@ +Bosch MCAN controller Device Tree Bindings +------------------------------------------------- + +Required properties: +- compatible : Should be "bosch,m_can" for M_CAN controllers +- reg : physical base address and size of the M_CAN + registers map and Message RAM +- reg-names : Should be "m_can" and "message_ram" +- interrupts : Should be the interrupt number of M_CAN interruptQuoted 9 lines
Documentation/devicetree/bindings/net/can/m_can.txt line 16
Quoted 7 lines
Documentation/devicetree/bindings/net/can/m_can.txt new file +7
-0+ line 0 and line 1, could be same if sharing + the same interrupt. +- interrupt-names : Should contain "int0" and "int1" +- clocks : Clocks used by controller, should be host clock + and CAN clock. +- clock-names : Should contain "hclk" and "cclk" +- pinctrl-<n> : Pinctrl states as described in bindings/pinctrl/pinctrl-bindings.txtI think this should be pinctrl-0
Quoted 4 lines
First, this part is defined by pinctrl binding doc.
Second i think it may be possible someone wants to add other pinctrl states when implement low power state in the future.
So i just keep it as pinctrl-<n>.
Quoted 5 lines
Normally we will use pinctrl-0 for mentioning the pinctrl bindings.
If somebody going to add something to the pinctrl bindings they will add separately with pinctrl-1: bla bla bla...
Will it cause misleading that it only supports one state? And if we only define pinctrl-0 here, how do we describe pinctrl-names? It should be "default"? It looks not accurate enough to me.
Per my understanding, I think it's better to leave it as standard pinctrl-binding doc states since anyhow people should read pinctrl-binding doc.
Quoted 10 lines
Quoted 10 lines
Quoted 3 lines
Documentation/devicetree/bindings/net/can/m_can.txt line 17
Quoted 1 line
Documentation/devicetree/bindings/net/can/m_can.txt new file +1
-0+- pinctrl-names : Names corresponding to the numbered pinctrl statesremove 1 tab space before :
It's a bit strange.
Other line like pinctrl-<n> is also two tabs.
And the code looks fine and already aligned.
- pinctrl-<n> : Pinctrl states as described inbindings/pinctrl/pinctrl-bindings.txt
- pinctrl-names : Names corresponding to the numbered pinctrl states
Do you mean change line of pinctrl-names from two tabs to one space and a tab before :?
Quoted 4 lines
When i see in the patch the alignment is like this
pinctrl-<n> :
pinctrl-name :
Yes,in the original patch it's like:
+- pinctrl-<n> : Pinctrl states as described in bindings/pinctrl/pinctrl-bindings.txt +- pinctrl-names : Names corresponding to the numbered pinctrl states
If remove one tab:
+- pinctrl-<n> : Pinctrl states as described in bindings/pinctrl/pinctrl-bindings.txt +- pinctrl-names : Names corresponding to the numbered pinctrl states
But in vim reading the code, it's like:
- pinctrl-<n> : Pinctrl states as described in bindings/pinctrl/pinctrl-bindings.txt
- pinctrl-names : Names corresponding to the numbered pinctrl states I don't know why.
What should we do about this case?
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Documentation/devicetree/bindings/net/can/m_can.txt line 23
Quoted 6 lines
Documentation/devicetree/bindings/net/can/m_can.txt new file +6
-0+- mram-cfg : Message RAM configuration data. + Multiple M_CAN instances can share the same Message RAM and each element(e.g + Rx FIFO or Tx Buffer and etc) number in Message RAM is also configurable, + so this property is telling driver how the shared or private Message RAM + are used by this M_CAN controller. +It may written like:
mram-cfg : Message RAM configuration data
MultipleM_CANinstances can share the same Message RAM and each element (e.g Rx FIFO or Tx Buffer and etc) number in Message RAM is also configurable,
...
Quoted 6 lines
Quoted 3 lines
I'm fine with that.
The question is it's easy to over 80 columns if writing like that, is it ok?When we follow the above format it would be more readable.
Okay.
Quoted 8 lines
Quoted 2 lines
Regards
Dong AishengSignature · 2 lines
Regards, Varka Bhadram.
Regards
Dong Aisheng
PATCH v3 1/2 can: m_can: add device tree binding documentation
Quoted 76 lines · Dong Aisheng
Quoted 40 lines · Varka Bhadram
Quoted 39 lines · Dong Aisheng
Quoted 38 lines · Varka Bhadram
Quoted 25 lines · Dong Aisheng
add
M_CANdevice tree binding documentationCc Wolfgang Grandegger<[email protected]>Cc Marc Kleine-Budde<[email protected]>Cc Mark Rutland<[email protected]>Cc Oliver Hartkopp<[email protected]>Cc Varka Bhadram<[email protected]>Signed-off-by Dong Aisheng<[email protected]>--- .../devicetree/bindings/net/can/m_can.txt | 65 ++++++++++++++++++++ 1 files changed, 65 insertions(+), 0 deletions(-)create mode 100644
Documentation/devicetree/bindings/net/can/m_can.txtDocumentation/devicetree/bindings/net/can/m_can.txt new file +6
-0@@ -0,0 +1,65 @@ +Bosch MCAN controller Device Tree Bindings +------------------------------------------------- + +Required properties: +- compatible : Should be "bosch,m_can" for M_CAN controllers +- reg : physical base address and size of the M_CANDocumentation/devicetree/bindings/net/can/m_can.txt line 16
Quoted 10 lines
Documentation/devicetree/bindings/net/can/m_can.txt new file +10
-0+ registers map and Message RAM +- reg-names : Should be "m_can" and "message_ram" +- interrupts : Should be the interrupt number of M_CAN interrupt + line 0 and line 1, could be same if sharing + the same interrupt. +- interrupt-names : Should contain "int0" and "int1" +- clocks : Clocks used by controller, should be host clock + and CAN clock. +- clock-names : Should contain "hclk" and "cclk" +- pinctrl-<n> : Pinctrl states as described in bindings/pinctrl/pinctrl-bindings.txtI think this should be pinctrl-0
Quoted 9 lines
Quoted 4 lines
First, this part is defined by pinctrl binding doc.
Second i think it may be possible someone wants to add other pinctrl states when implement low power state in the future.
So i just keep it as pinctrl-<n>.Normally we will use pinctrl-0 for mentioning the pinctrl bindings.
If somebody going to add something to the pinctrl bindings they will add separately with pinctrl-1: bla bla bla...
Will it cause misleading that it only supports one state? And if we only define pinctrl-0 here, how do we describe pinctrl-names? It should be "default"? It looks not accurate enough to me.
Per my understanding, I think it's better to leave it as standard pinctrl-binding doc states since anyhow people should read pinctrl-binding doc.
Quoted 10 lines
Quoted 10 lines
Quoted 3 lines
Documentation/devicetree/bindings/net/can/m_can.txt line 17
Quoted 1 line
Documentation/devicetree/bindings/net/can/m_can.txt new file +1
-0+- pinctrl-names : Names corresponding to the numbered pinctrl statesremove 1 tab space before :
It's a bit strange.
Other line like pinctrl-<n> is also two tabs.
And the code looks fine and already aligned.
- pinctrl-<n> : Pinctrl states as described inbindings/pinctrl/pinctrl-bindings.txt
- pinctrl-names : Names corresponding to the numbered pinctrl states
Do you mean change line of pinctrl-names from two tabs to one space and a tab before :?Quoted 4 lines
When i see in the patch the alignment is like this
pinctrl-<n> :
pinctrl-name :Yes,in the original patch it's like:
+- pinctrl-<n> : Pinctrl states as described in bindings/pinctrl/pinctrl-bindings.txt +- pinctrl-names : Names corresponding to the numbered pinctrl statesIf remove one tab:
Quoted 8 lines
+- pinctrl-<n> : Pinctrl states as described in bindings/pinctrl/pinctrl-bindings.txt +- pinctrl-names : Names corresponding to the numbered pinctrl statesBut in vim reading the code, it's like:
- pinctrl-<n> : Pinctrl states as described inbindings/pinctrl/pinctrl-bindings.txt
- pinctrl-names : Names corresponding to the numbered pinctrl states I don't know why.
What should we do about this case?
It seems to me that the style followed in bindings/pinctrl/pinctrl-bindings.txt looks good to me.
pinctrl-<n>: Pinctrl states as described in
bindings/pinctrl/pinctrl-bindings.txt
pinctrl-names: Names corresponding to the
numbered pinctrl states
Quoted 29 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Documentation/devicetree/bindings/net/can/m_can.txt line 23
Quoted 6 lines
Documentation/devicetree/bindings/net/can/m_can.txt new file +6
-0+- mram-cfg : Message RAM configuration data. + Multiple M_CAN instances can share the same Message RAM and each element(e.g + Rx FIFO or Tx Buffer and etc) number in Message RAM is also configurable, + so this property is telling driver how the shared or private Message RAM + are used by this M_CAN controller. +It may written like:
mram-cfg : Message RAM configuration data
MultipleM_CANinstances can share the same Message RAM and each element (e.g Rx FIFO or Tx Buffer and etc) number in Message RAM is also configurable,
...Quoted 5 lines
Quoted 3 lines
I'm fine with that.
The question is it's easy to over 80 columns if writing like that, is it ok?When we follow the above format it would be more readable.
Okay.
Quoted 6 lines
Quoted 2 lines
Regards
Dong AishengSignature · 2 lines
Regards, Varka Bhadram.Regards
Dong Aisheng
Signature · 2 lines
Regards, Varka Bhadram.
Marc Kleine-Budde
maintainerPATCH v3 1/2 can: m_can: add device tree binding documentation
In reply to Dong AishengQuoted 31 lines · Dong Aisheng
add
M_CANdevice tree binding documentationCc Wolfgang Grandegger <[email protected]>Cc Marc Kleine-Budde <[email protected]>Cc Mark Rutland <[email protected]>Cc Oliver Hartkopp <[email protected]>Cc Varka Bhadram <[email protected]>Signed-off-by Dong Aisheng <[email protected]>--- .../devicetree/bindings/net/can/m_can.txt | 65 ++++++++++++++++++++ 1 files changed, 65 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/net/can/m_can.txtDocumentation/devicetree/bindings/net/can/m_can.txt new file +12
-0@@ -0,0 +1,65 @@ +Bosch MCAN controller Device Tree Bindings +------------------------------------------------- + +Required properties: +- compatible : Should be "bosch,m_can" for M_CAN controllers +- reg : physical base address and size of the M_CAN + registers map and Message RAM +- reg-names : Should be "m_can" and "message_ram" +- interrupts : Should be the interrupt number of M_CAN interrupt + line 0 and line 1, could be same if sharing + the same interrupt. +- interrupt-names : Should contain "int0" and "int1"
Documentation/devicetree/bindings/net/can/m_can.txt line 22
Quoted 10 lines
Documentation/devicetree/bindings/net/can/m_can.txt new file +10
-0+- clocks : Clocks used by controller, should be host clock + and CAN clock. +- clock-names : Should contain "hclk" and "cclk" +- pinctrl-<n> : Pinctrl states as described in bindings/pinctrl/pinctrl-bindings.txt +- pinctrl-names : Names corresponding to the numbered pinctrl states +- mram-cfg : Message RAM configuration data. + Multiple M_CAN instances can share the same Message RAM and each element(e.g + Rx FIFO or Tx Buffer and etc) number in Message RAM is also configurable, + so this property is telling driver how the shared or private Message RAM + are used by this M_CAN controller.
Will you have to modify the binding, espeically the mram-cfg, if you add canfd support to the driver?
I think mram-cfg should have a vendor prefix, e.g.: bosch,mram-cfg
Quoted 34 lines
Documentation/devicetree/bindings/net/can/m_can.txt new file +33
-0+ + The format should be as follows: + <offset sidf_elems xidf_elems rxf0_elems rxf1_elems rxb_elems + txe_elems txb_elems> + The 'offset' is an address offset of the Message RAM where the following + elements start from. This is usually set to 0x0 if you're using a private + Message RAM. The remain cells are used to specify how many elements are used + for each FIFO/Buffer. + +M_CAN includes the following elements according to user manual: +11-bit Filter 0-128 elements / 0-128 words +29-bit Filter 0-64 elements / 0-128 words +Rx FIFO 0 0-64 elements / 0-1152 words +Rx FIFO 1 0-64 elements / 0-1152 words +Rx Buffers 0-64 elements / 0-1152 words +Tx Event FIFO 0-32 elements / 0-64 words +Tx Buffers 0-32 elements / 0-576 words + +Please refer to 2.4.1 Message RAM Configuration in Bosch M_CAN user manual +for details. + +Example: +SoC dtsi: +m_can1: can at 020e8000 { + compatible = "bosch,m_can"; + reg = <0x020e8000 0x4000>, <0x02298000 0x4000>; + reg-names = "m_can", "message_ram"; + interrupts = <0 114 0x04>, + <0 114 0x04>; + interrupt-names = "int0", "int1"; + clocks = <&clks IMX6SX_CLK_CANFD>, + <&clks IMX6SX_CLK_CANFD>; + clock-names = "hclk", "cclk";
Documentation/devicetree/bindings/net/can/m_can.txt line 65
Quoted 10 lines
Documentation/devicetree/bindings/net/can/m_can.txt new file +10
-0+ mram-cfg = <0x0 0 0 32 0 0 0 1>; + status = "disabled"; +}; + +Board dtsi: +&m_can1 { + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_m_can1>; + status = "enabled"; +};
Marc
Signature · 12 lines
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 | -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 242 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140711/8b533762/attachment.sig>
PATCH v3 1/2 can: m_can: add device tree binding documentation
Quoted 37 lines · Marc Kleine-Budde
Quoted 36 lines · Dong Aisheng
add
M_CANdevice tree binding documentationCc Wolfgang Grandegger <[email protected]>Cc Marc Kleine-Budde <[email protected]>Cc Mark Rutland <[email protected]>Cc Oliver Hartkopp <[email protected]>Cc Varka Bhadram <[email protected]>Signed-off-by Dong Aisheng <[email protected]>--- .../devicetree/bindings/net/can/m_can.txt | 65 ++++++++++++++++++++ 1 files changed, 65 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/net/can/m_can.txtDocumentation/devicetree/bindings/net/can/m_can.txt new file +17
-0@@ -0,0 +1,65 @@ +Bosch MCAN controller Device Tree Bindings +------------------------------------------------- + +Required properties: +- compatible : Should be "bosch,m_can" for M_CAN controllers +- reg : physical base address and size of the M_CAN + registers map and Message RAM +- reg-names : Should be "m_can" and "message_ram" +- interrupts : Should be the interrupt number of M_CAN interrupt + line 0 and line 1, could be same if sharing + the same interrupt. +- interrupt-names : Should contain "int0" and "int1" +- clocks : Clocks used by controller, should be host clock + and CAN clock. +- clock-names : Should contain "hclk" and "cclk" +- pinctrl-<n> : Pinctrl states as described in bindings/pinctrl/pinctrl-bindings.txt +- pinctrl-names : Names corresponding to the numbered pinctrl states
Quoted 9 lines
Documentation/devicetree/bindings/net/can/m_can.txt line 22
Quoted 5 lines
Documentation/devicetree/bindings/net/can/m_can.txt new file +5
-0+- mram-cfg : Message RAM configuration data. + Multiple M_CAN instances can share the same Message RAM and each element(e.g + Rx FIFO or Tx Buffer and etc) number in Message RAM is also configurable, + so this property is telling driver how the shared or private Message RAM + are used by this M_CAN controller.Will you have to modify the binding, espeically the mram-cfg, if you add canfd support to the driver?
No need, CANFD in i.MX6SX is using the same IP of Bosch M_CAN.
Quoted 2 lines
I think mram-cfg should have a vendor prefix, e.g.: bosch,mram-cfg
It's a good idea.
Will update it.
Quoted 38 lines
Quoted 38 lines
Documentation/devicetree/bindings/net/can/m_can.txt new file +38
-0+ + The format should be as follows: + <offset sidf_elems xidf_elems rxf0_elems rxf1_elems rxb_elems + txe_elems txb_elems> + The 'offset' is an address offset of the Message RAM where the following + elements start from. This is usually set to 0x0 if you're using a private + Message RAM. The remain cells are used to specify how many elements are used + for each FIFO/Buffer. + +M_CAN includes the following elements according to user manual: +11-bit Filter 0-128 elements / 0-128 words +29-bit Filter 0-64 elements / 0-128 words +Rx FIFO 0 0-64 elements / 0-1152 words +Rx FIFO 1 0-64 elements / 0-1152 words +Rx Buffers 0-64 elements / 0-1152 words +Tx Event FIFO 0-32 elements / 0-64 words +Tx Buffers 0-32 elements / 0-576 words + +Please refer to 2.4.1 Message RAM Configuration in Bosch M_CAN user manual +for details. + +Example: +SoC dtsi: +m_can1: can at 020e8000 { + compatible = "bosch,m_can"; + reg = <0x020e8000 0x4000>, <0x02298000 0x4000>; + reg-names = "m_can", "message_ram"; + interrupts = <0 114 0x04>, + <0 114 0x04>; + interrupt-names = "int0", "int1"; + clocks = <&clks IMX6SX_CLK_CANFD>, + <&clks IMX6SX_CLK_CANFD>; + clock-names = "hclk", "cclk"; + mram-cfg = <0x0 0 0 32 0 0 0 1>; + status = "disabled"; +}; + +Board dtsi:
Quoted 14 lines
Documentation/devicetree/bindings/net/can/m_can.txt line 65
Quoted 6 lines
Documentation/devicetree/bindings/net/can/m_can.txt new file +5
-0+&m_can1 { + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_m_can1>; + status = "enabled"; +};Marc
Signature · 4 lines
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 |
Regards
Dong Aisheng