Re: [PATCH v4] flexcan: add err_irq handler for flexcan

[Marc Kleine-Budde <mkl@pengutronix.de>]

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

On 07/02/2014 10:32 AM, qiang.zhao@freescale.com wrote:
> On 07/02/2014 03:04 PM, Marc Kleine-Budde wrote:
>> -----Original Message-----
>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
>> Sent: Wednesday, July 02, 2014 3:04 PM
>> To: Zhao Qiang-B45475; linuxppc-dev@lists.ozlabs.org; wg@grandegger.com;
>> linux-can@vger.kernel.org; Wood Scott-B07421
>> Subject: Re: [PATCH v4] flexcan: add err_irq handler for flexcan
>>
>> On 07/02/2014 04:00 AM, qiang.zhao@freescale.com wrote:
>>>>> +static irqreturn_t flexcan_err_irq(int irq, void *dev_id) {
>>>>> +	struct net_device *dev = dev_id;
>>>>> +	struct flexcan_priv *priv = netdev_priv(dev);
>>>>> +	struct flexcan_regs __iomem *regs = priv->base;
>>>>> +	u32 reg_ctrl, reg_esr;
>>>>> +
>>>>> +	reg_esr = flexcan_read(&regs->esr);
>>>>> +	reg_ctrl = flexcan_read(&regs->ctrl);
>>>>> +
>>>>> +	if (reg_esr & FLEXCAN_ESR_ALL_INT) {
>>>>> +		if (reg_esr & FLEXCAN_ESR_ERR_INT)
>>>>> +			flexcan_write(reg_ctrl & ~FLEXCAN_CTRL_ERR_MSK,
>>>>> +				      &regs->ctrl);
>>>>> +		flexcan_irq(irq, dev);
>>>>
>>>> I still don't understand why you need a special flexcan_err_irq()
>>>> function. Why don't you just call:
>>>>
>>>> request_irq(priv->err_irq, flexcan_irq, IRQF_SHARED, dev->name, dev);
>>>>
>>>> instead?
>>>
>>> Flexcan_irq is for flexcan normal interrupt(such as Message Buffer,
>> Wake up and so on).
>>> And it will return IRQ_HANDLED if flexcan_irq is triggered.
>>> But err_irq is shared with other devices, it should return IRQ_HANDLED
>>> when the interrupt is triggered by flexcan device, if not return
>> IRQ_NONE.
>>
>> What about fixing flexcan_irq() first and the make use of it?
> 
> Err_irq is a shared interrupt with other device, 
> I hope that its handler is independent.
> However, if you persist in your opinion, I will do it as you said.

There is another option, you can move all of the error interrupt
handling code from flexcan_irq() to flexcan_irq_err(). To keep the ARM
SoCs supported, you need to call flexcan_irq_err() form the
flexcan_irq() handler.

What I don't want is a) code duplication and b) no fishy wrapper
functions around flexcan_irq() that work around flexcan_irq() returning
IRQ_HANDLED unconditionally.

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: 242 bytes --]