From: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public•gmane.org>
To: Masayuki Ohtake <masa-korg-ECg8zkTtlr0C6LszWs/t0g@public•gmane.org>
Cc: andrew.chih.howe.khor-ral2JQCrhuEAvxtiuMwx3w@public•gmane.org,
qi.wang-ral2JQCrhuEAvxtiuMwx3w@public•gmane.org,
margie.foster-ral2JQCrhuEAvxtiuMwx3w@public•gmane.org,
netdev-u79uwXL29TY76Z2rM5mHXA@public•gmane.org,
yong.y.wang-ral2JQCrhuEAvxtiuMwx3w@public•gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public•gmane.org,
socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public•gmane.org,
kok.howg.ewe-ral2JQCrhuEAvxtiuMwx3w@public•gmane.org,
Christian Pellegrin <chripell-VaTbYqLCNhc@public•gmane.org>,
Tomoya MORINAGA
<morinaga526-ECg8zkTtlr0C6LszWs/t0g@public•gmane.org>,
"David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public•gmane.org>,
joel.clark-ral2JQCrhuEAvxtiuMwx3w@public•gmane.org,
Samuel Ortiz <sameo-VuQAYsv1563Yd54FQh9/CA@public•gmane.org>
Subject: Re: [MeeGo-Dev][PATCH v3] Topcliff: Update PCH_CAN driver to 2.6.35
Date: Wed, 13 Oct 2010 13:08:14 +0200 [thread overview]
Message-ID: <4CB5931E.8030103@grandegger.com> (raw)
In-Reply-To: <004a01cb6abe$c41a9cc0$66f8800a-a06+6cuVnkTSQfdrb5gaxUEOCMrvLtNR@public.gmane.org>
On 10/13/2010 12:09 PM, Masayuki Ohtake wrote:
> On Thursday, September 30, 2010 6:10 PM, Wolfgang Grandegger wrote:
>
>>
>> + iowrite32(num, &(priv->regs)->if2_creq);
>> + while (counter) {
>>> + if2_creq = (ioread32(&(priv->regs)->if2_creq)) &
>>> + CAN_IF_CREQ_BUSY;
>>> + if (!if2_creq)
>>> + break;
>>> + counter--;
>>> + }
>>> + if (!counter)
>>> + dev_err(&priv->ndev->dev, "IF2 BUSY Flag is set forever.\n");
>>> +}
>>
>> Duplicated code!
>
> No.
> These are not the same.
Of course they are not the same. The only difference is the register
offset (of if1 or if2). A common function with a pointer to the if
register as argument makes sense.
> Though it is possible to integrate to one function by adding parameter,
> I think, current two function method is more easily to read.
I disagree.
>>
>>
>>
>>> + if (status & PCH_STUF_ERR)
>>> + cf->data[2] |= CAN_ERR_PROT_STUFF;
>>> +
>>> + if (status & PCH_FORM_ERR)
>>> + cf->data[2] |= CAN_ERR_PROT_FORM;
>> +
>> + if (status & PCH_ACK_ERR)
>> + cf->data[2] |= CAN_ERR_PROT_LOC_ACK | CAN_ERR_PROT_LOC_ACK_DEL;
>> +
>> + if ((status & PCH_BIT1_ERR) || (status & PCH_BIT0_ERR))
>> + cf->data[2] |= CAN_ERR_PROT_BIT;
>> +
>> + if (status & PCH_CRC_ERR)
>> + cf->data[2] |= CAN_ERR_PROT_LOC_CRC_SEQ |
>> + CAN_ERR_PROT_LOC_CRC_DEL;
>> +
>> + if (status & PCH_LEC_ALL)
>> + iowrite32(status | PCH_LEC_ALL,
>> + &(priv->regs)->stat);
Well, if status==7 (PCH_LEC_ALL), all of the above conditions are true
as well... convinced now?
>> A bit-wise test of the above values is wrong, I believe. Please use the
>> switch statement instead.
>
> The above conditions are not only one time.
> I think "switch" is not suitable for the above.
> Thus, current "if" processing is better.
I don't understand! The Last Error Code (LEC) can have values from 0 to
7. A "switch" statement is therefore the right choice. Or have I missed
something.
>>
>>
>> + u32 brp;
>> +
>> + pch_can_get_run_mode(priv, &curr_mode);
>> + if (curr_mode == PCH_CAN_RUN)
>> + pch_can_set_run_mode(priv, PCH_CAN_STOP);
>>
>> The device is stopped when this function is called. Please remove.
>
> No.
> The above is necessary.
Yes, because you started the device *too early* in pch_can_open() called
by pch_open(). See my other related comments of my previous mail.
> Because this is our HW specification.
> Before setting bitrate, run-mode must be "STOP".
I think it can be avoided easily.
>>
>>
>> +static netdev_tx_t pch_xmit(struct sk_buff *skb, struct net_device *ndev)
>> +{
>> + canid_t id;
>> + u32 id1 = 0;
>> + u32 id2 = 0;
>>
>> Need these values to be preset?
>
> These values are not essential.
> But these help a engineer to read this code.
I disagree.
>> + /* Enable CAN Interrupts */
>> + pch_can_set_int_custom(priv);
>> +
>> + /* Restore Run Mode */
>> + pch_can_set_run_mode(priv, PCH_CAN_RUN);
>> +
>> + return retval;
>> +}
>>
>> Are the suspend and resume functions tested?
>>
> Yes, we tested before.
>
> =========================================
>
> Except the above, we are modifying for your indications.
>
> I will resubmit soon.
Thanks,
Wolfgang.
next prev parent reply other threads:[~2010-10-13 11:08 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <4C9C7C6F.1000003@dsn.okisemi.com>
[not found] ` <4C9C7C6F.1000003-ECg8zkTtlr0C6LszWs/t0g@public.gmane.org>
2010-09-27 8:38 ` [MeeGo-Dev][PATCH v3] Topcliff: Update PCH_CAN driver to 2.6.35 Wolfgang Grandegger
2010-09-30 9:10 ` Wolfgang Grandegger
[not found] ` <4CA4541F.5040804-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
2010-09-30 18:50 ` David Miller
2010-10-01 10:02 ` Masayuki Ohtake
[not found] ` <000401cb614f$c932c750$66f8800a-a06+6cuVnkTSQfdrb5gaxUEOCMrvLtNR@public.gmane.org>
2010-10-01 12:40 ` Wolfgang Grandegger
2010-10-05 10:21 ` Masayuki Ohtake
2010-10-05 11:08 ` Marc Kleine-Budde
2010-10-05 12:09 ` Masayuki Ohtake
[not found] ` <002e01cb6486$2ed72cc0$66f8800a-a06+6cuVnkTSQfdrb5gaxUEOCMrvLtNR@public.gmane.org>
2010-10-05 18:45 ` David Miller
2010-10-06 3:07 ` Masayuki Ohtake
[not found] ` <000b01cb6503$962bc7f0$66f8800a-a06+6cuVnkTSQfdrb5gaxUEOCMrvLtNR@public.gmane.org>
2010-10-06 3:09 ` David Miller
[not found] ` <20101005.200904.71120150.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
2010-10-06 9:12 ` Wolfgang Grandegger
2010-10-12 7:09 ` Masayuki Ohtake
[not found] ` <000c01cb69dc$5d2aaab0$66f8800a-a06+6cuVnkTSQfdrb5gaxUEOCMrvLtNR@public.gmane.org>
2010-10-12 7:42 ` Wolfgang Grandegger
2010-10-12 7:56 ` Marc Kleine-Budde
2010-10-13 4:23 ` Masayuki Ohtake
[not found] ` <000701cb6a8e$60de6900$66f8800a-a06+6cuVnkTSQfdrb5gaxUEOCMrvLtNR@public.gmane.org>
2010-10-13 7:38 ` Marc Kleine-Budde
2010-10-13 10:09 ` Masayuki Ohtake
[not found] ` <004a01cb6abe$c41a9cc0$66f8800a-a06+6cuVnkTSQfdrb5gaxUEOCMrvLtNR@public.gmane.org>
2010-10-13 11:08 ` Wolfgang Grandegger [this message]
2010-10-13 12:05 ` Masayuki Ohtake
2010-09-24 10:24 Masayuki Ohtak
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4CB5931E.8030103@grandegger.com \
--to=wg-5yr1bzd7o62+xt7jha+gda@public$(echo .)gmane.org \
--cc=andrew.chih.howe.khor-ral2JQCrhuEAvxtiuMwx3w@public$(echo .)gmane.org \
--cc=chripell-VaTbYqLCNhc@public$(echo .)gmane.org \
--cc=davem-fT/PcQaiUtIeIZ0/mPfg9Q@public$(echo .)gmane.org \
--cc=joel.clark-ral2JQCrhuEAvxtiuMwx3w@public$(echo .)gmane.org \
--cc=kok.howg.ewe-ral2JQCrhuEAvxtiuMwx3w@public$(echo .)gmane.org \
--cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public$(echo .)gmane.org \
--cc=margie.foster-ral2JQCrhuEAvxtiuMwx3w@public$(echo .)gmane.org \
--cc=masa-korg-ECg8zkTtlr0C6LszWs/t0g@public$(echo .)gmane.org \
--cc=morinaga526-ECg8zkTtlr0C6LszWs/t0g@public$(echo .)gmane.org \
--cc=netdev-u79uwXL29TY76Z2rM5mHXA@public$(echo .)gmane.org \
--cc=qi.wang-ral2JQCrhuEAvxtiuMwx3w@public$(echo .)gmane.org \
--cc=sameo-VuQAYsv1563Yd54FQh9/CA@public$(echo .)gmane.org \
--cc=socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public$(echo .)gmane.org \
--cc=yong.y.wang-ral2JQCrhuEAvxtiuMwx3w@public$(echo .)gmane.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox