Wolfgang On 3/4/19 12:13 PM, Wolfgang Grandegger wrote: > > > Am 04.03.19 um 18:22 schrieb Dan Murphy: >> Wolfgang >> >> On 3/4/19 10:56 AM, Wolfgang Grandegger wrote: >>> Hello Dan, >>> >>> the series already looks quite good. I still realized a few (minor) >>> issues while browsing the patch/code... >>> >> >> Thanks for the review. It is getting there. >> >>> Am 01.03.19 um 19:50 schrieb Dan Murphy: >>>> Create a m_can platform framework that peripherial >>>> devices can register to and use common code and register sets. >>>> The peripherial devices may provide read/write and configuration >>>> support of the IP. >>>> >>>> Signed-off-by: Dan Murphy <dmurphy@xxxxxx> >>>> --- >>>> >>>> v6 - Squashed platform patch to this patch for bissectablity, fixed coding style >>>> issues, updated Kconfig help, placed mcan reg offsets back into c file, renamed >>>> priv->skb to priv->tx_skb and cleared perp interrupts at ISR start - >>>> Patch 1 comments - https://lore.kernel.org/patchwork/patch/1042446/ >>>> Patch 2 comments - https://lore.kernel.org/patchwork/patch/1042442/ >>>> >>>> drivers/net/can/m_can/Kconfig | 13 +- >>>> drivers/net/can/m_can/Makefile | 1 + >>>> drivers/net/can/m_can/m_can.c | 702 +++++++++++++------------ >>>> drivers/net/can/m_can/m_can.h | 110 ++++ >>>> drivers/net/can/m_can/m_can_platform.c | 198 +++++++ >>>> 5 files changed, 681 insertions(+), 343 deletions(-) >>>> create mode 100644 drivers/net/can/m_can/m_can.h >>>> create mode 100644 drivers/net/can/m_can/m_can_platform.c >>>> >>>> diff --git a/drivers/net/can/m_can/Kconfig b/drivers/net/can/m_can/Kconfig >>>> index 04f20dd39007..f7119fd72df4 100644 >>>> --- a/drivers/net/can/m_can/Kconfig >>>> +++ b/drivers/net/can/m_can/Kconfig >>>> @@ -1,5 +1,14 @@ >>>> config CAN_M_CAN >>>> + tristate "Bosch M_CAN support" >>>> + ---help--- >>>> + Say Y here if you want support for Bosch M_CAN controller framework. >>>> + This is common support for devices that embed the Bosch M_CAN IP. >>>> + >>>> +config CAN_M_CAN_PLATFORM >>>> + tristate "Bosch M_CAN support for io-mapped devices" >>>> depends on HAS_IOMEM >>>> - tristate "Bosch M_CAN devices" >>>> + depends on CAN_M_CAN >>>> ---help--- >>>> - Say Y here if you want to support for Bosch M_CAN controller. >>>> + Say Y here if you want support for IO Mapped Bosch M_CAN controller. >>>> + This support is for devices that have the Bosch M_CAN controller >>>> + IP embedded into the device and the IP is IO Mapped to the processor. >>>> diff --git a/drivers/net/can/m_can/Makefile b/drivers/net/can/m_can/Makefile >>>> index 8bbd7f24f5be..057bbcdb3c74 100644 >>>> --- a/drivers/net/can/m_can/Makefile >>>> +++ b/drivers/net/can/m_can/Makefile >>>> @@ -3,3 +3,4 @@ >>>> # >>>> >>>> obj-$(CONFIG_CAN_M_CAN) += m_can.o >>>> +obj-$(CONFIG_CAN_M_CAN_PLATFORM) += m_can_platform.o >>>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c >>>> index 9b449400376b..b37d0886f9cb 100644 >>>> --- a/drivers/net/can/m_can/m_can.c >>>> +++ b/drivers/net/can/m_can/m_can.c > > ...snip... > >>>> @@ -1451,7 +1459,7 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb, >>>> netif_stop_queue(dev); >>>> netdev_warn(dev, >>>> "TX queue active although FIFO is full."); >>>> - return NETDEV_TX_BUSY; >>>> + return; >>> >>> m_can_start_xmit() doesn't return NETDEV_TX_BUSY but NETDEV_TX_OK and >>> the queue is stopped! Also the skb is not freed! The code states >>> "/* This shouldn't happen */" but then it just prints a warning. Did >>> you see that message? >>> >> >> No I have not seen this warning but I will re-check to be sure. > > If we don't return NETDEV_TX_BUSY but NETDEV_TX_OK, we must handle it > differently. > OK. I see the diff between the perp and io mapped. I will update the code appropriately. > ...snip... > >>>> +struct m_can_priv; >>>> +struct m_can_ops { >>>> + /* Device specific call backs */ >>>> + int (*clr_dev_interrupts)(struct m_can_priv *m_can_class); >>> >>> Why not just "clear_interrupt"... to be consistant with the names below. >> >> I wanted to be clear in the M_CAN code that these are device interrupts and not M_CAN interrupts. >> >> I can change it to clear_interrupt if you think it makes more sense. > > Well, like for "read_reg" etc, I think it's clear that it's a > device-specific function/ops: > > cdev->read_reg > cdev->clear_interrupt > OK >>>> + u32 (*read_reg)(struct m_can_priv *m_can_class, int reg); >>>> + int (*write_reg)(struct m_can_priv *m_can_class, int reg, int val); >>>> + u32 (*read_fifo)(struct m_can_priv *m_can_class, int addr_offset); >>>> + int (*write_fifo)(struct m_can_priv *m_can_class, int addr_offset, >>>> + int val); >>>> + int (*device_init)(struct m_can_priv *m_can_class); > > And the same here: > > cdev->init > OK >>>> + int pm_clock_support; >>> >>> A "bool" would be more appropriate, I think. >> >> >> I was abiding by this checkpatch warning I got on the is_peripherial. >> >> CHECK: Avoid using bool structure members because of possible alignment issues - see: https://lkml.org/lkml/2017/11/21/384 >> #94: FILE: drivers/net/can/m_can/m_can.h:94: >> + bool is_peripherial; >> > > Ah, right! I was also surprised to get that warning. The kernel is full > of bool's, but well, we should make "checkpatch" happy (and Linus). > > Wolfgang > -- ------------------ Dan Murphy
