Re: [PATCH V3 5/5] net: can: ifi: Add IFI CANFD IP support - Marc Kleine-Budde

From: Marc Kleine-Budde <mkl@pengutronix.de>
To: Marek Vasut <marex@denx.de>
Cc: netdev@vger.kernel.org, Oliver Hartkopp <socketcan@hartkopp.net>,
	Mark Rutland <mark.rutland@arm.com>,
	Wolfgang Grandegger <wg@grandegger.com>
Subject: Re: [PATCH V3 5/5] net: can: ifi: Add IFI CANFD IP support
Date: Wed, 20 Jan 2016 15:46:58 +0100	[thread overview]
Message-ID: <569F9DE2.10405@pengutronix.de> (raw)
In-Reply-To: <201601201541.53246.marex@denx.de>

[-- Attachment #1: Type: text/plain, Size: 3083 bytes --]

On 01/20/2016 03:41 PM, Marek Vasut wrote:
> On Wednesday, January 20, 2016 at 03:39:51 PM, Marc Kleine-Budde wrote:
>> On 01/20/2016 03:33 PM, Marek Vasut wrote:
>>> The patch adds support for IFI CAN/FD controller [1]. This driver
>>> currently supports sending and receiving both standard CAN and new
>>> CAN/FD frames. Both ISO and BOSCH variant of CAN/FD is supported.
>>>
>>> [1] http://www.ifi-pld.de/IP/CANFD/canfd.html
>>>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Cc: Oliver Hartkopp <socketcan@hartkopp.net>
>>> Cc: Wolfgang Grandegger <wg@grandegger.com>
>>> ---
>>> V2: - Move request_irq()/free_irq() into
>>> ifi_canfd_open()/ifi_canfd_close()
>>>
>>>       just like other drivers do it to prevent crash when reloading
>>>       module.
>>>     
>>>     - Fix Woverflow complains on x86_64 and itanium, exactly the same way
>>>     
>>>       as in commit dec23dca5a9ca4b9eb2fb66926f567889028b904 .
>>>
>>> V3: - Hopefully fix all problems with BIT(31) by adding more u32 casts.
>>>
>>>     - Drop struct device from struct ifi_canfd_priv .
>>>
>>> NOTE: The driver is surprisingly similar to m_can, but the register
>>>
>>>       layout of the IFI core is completely different, so it's clear
>>>       that those are two different IP cores.
>>>
>>> ---
> 
> [...]
> 
> Hi,
> 
>>> +static irqreturn_t ifi_canfd_isr(int irq, void *dev_id)
>>> +{
>>> +	struct net_device *ndev = (struct net_device *)dev_id;
>>> +	struct ifi_canfd_priv *priv = netdev_priv(ndev);
>>> +	struct net_device_stats *stats = &ndev->stats;
>>> +	const u32 rx_irq_mask = IFI_CANFD_INTERRUPT_RXFIFO_NEMPTY |
>>> +				IFI_CANFD_INTERRUPT_RXFIFO_NEMPTY_PER;
>>> +	const u32 tx_irq_mask = IFI_CANFD_INTERRUPT_TXFIFO_EMPTY |
>>> +				IFI_CANFD_INTERRUPT_TXFIFO_REMOVE;
>>> +	const u32 clr_irq_mask = (u32)(~(IFI_CANFD_INTERRUPT_SET_IRQ |
>>> +					 IFI_CANFD_INTERRUPT_ERROR_WARNING));
>>
>> I've squashed:
>>>> -       const u32 clr_irq_mask = (u32)(~(IFI_CANFD_INTERRUPT_SET_IRQ |
>>>> -                                       
>>>> IFI_CANFD_INTERRUPT_ERROR_WARNING)); +       const u32 clr_irq_mask =
>>>> ~(IFI_CANFD_INTERRUPT_SET_IRQ | +                                 
>>>> IFI_CANFD_INTERRUPT_ERROR_WARNING);
>>
>> and the driver compiles without warnings.
> 
> It doesn't , try with x86_64_defconfig, it will complain with -Woverflow
> on gcc 4.9 or newer. That's what the kernel robot complained about in V1
> of the patch too.

Doh! Right, let's try this:

>> -#define IFI_CANFD_INTERRUPT_ERROR_WARNING      BIT(1)
>> +#define IFI_CANFD_INTERRUPT_ERROR_WARNING      ((u32)BIT(1))

> I'd be happy to be proven wrong though.

/me too

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]