From: "Masayuki Ohtake" <masa-korg-ECg8zkTtlr0C6LszWs/t0g@public•gmane.org>
To: "Wolfgang Grandegger" <wg-5Yr1BZd7O62+XT7JhA+gdA@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 19:09:42 +0900 [thread overview]
Message-ID: <004a01cb6abe$c41a9cc0$66f8800a@maildom.okisemi.com> (raw)
In-Reply-To: 4CA4541F.5040804@grandegger.com
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.
Though it is possible to integrate to one function by adding parameter,
I think, current two function method is more easily to read.
>
>
>
>> + 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);
>
>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.
>
>
> + 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.
Because this is our HW specification.
Before setting bitrate, run-mode must be "STOP".
>
>
> +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.
>
>
> + /* 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, Ohtake(OKISemi)
next prev parent reply other threads:[~2010-10-13 10:09 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 [this message]
[not found] ` <004a01cb6abe$c41a9cc0$66f8800a-a06+6cuVnkTSQfdrb5gaxUEOCMrvLtNR@public.gmane.org>
2010-10-13 11:08 ` Wolfgang Grandegger
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='004a01cb6abe$c41a9cc0$66f8800a@maildom.okisemi.com' \
--to=masa-korg-ecg8zkttlr0c6lszws/t0g@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=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=wg-5Yr1BZd7O62+XT7JhA+gdA@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