From: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public•gmane.org>
To: Bhupesh SHARMA <bhupesh.sharma-qxv4g6HH51o@public•gmane.org>
Cc: "Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public•gmane.org"
<Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public•gmane.org>,
"netdev-u79uwXL29TY76Z2rM5mHXA@public•gmane.org"
<netdev-u79uwXL29TY76Z2rM5mHXA@public•gmane.org>,
Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public•gmane.org>,
David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public•gmane.org>,
Oliver Hartkopp
<socketcan-fJ+pQTUTwRTk1uMJSBkQmQ@public•gmane.org>
Subject: Re: [PATCH net-next-2.6 v3 1/1] can: c_can: Added support for Bosch C_CAN controller
Date: Tue, 11 Jan 2011 09:29:50 +0100 [thread overview]
Message-ID: <4D2C14FE.7080903@grandegger.com> (raw)
In-Reply-To: <D5ECB3C7A6F99444980976A8C6D896384DEAFA2D9D-8vAmw3ZAcdzhJTuQ9jeba9BPR1lH4CV8@public.gmane.org>
Hi Bhupesh,
On 01/11/2011 05:50 AM, Bhupesh SHARMA wrote:
> Hi Wolfgang,
>
> Thanks for your review.
> Please see my comments inline.
>
>> -----Original Message-----
>> From: Wolfgang Grandegger [mailto:wg-5Yr1BZd7O62+XT7JhA+gdA@public•gmane.org]
...
>>> + * Bosch C_CAN controller is compliant to CAN protocol version 2.0
>> part A and B.
>>> + * Bosch C_CAN user manual can be obtained from:
>>> + * http://www.semiconductors.bosch.de/pdf/Users_Manual_C_CAN.pdf
>>
>> Unfortunately, this link is not valid any more.
>
> Oops..
> It seems they have shifted to:
> http://www.semiconductors.bosch.de/media/en/pdf/ipmodules_1/c_can/users_manual_c_can.pdf
Ah, nice. I was not aware of that new link.
...
>>> +int c_can_write_msg_object(struct net_device *dev,
>>> + int iface, struct can_frame *frame, int objno)
>>> +{
>>> + int i;
>>> + u16 flags = 0;
>>> + unsigned int id;
>>> + struct c_can_priv *priv = netdev_priv(dev);
>>> +
>>> + if (frame->can_id & CAN_EFF_FLAG) {
>>> + id = frame->can_id & CAN_EFF_MASK;
>>> + flags |= IF_ARB_MSGXTD;
>>> + } else
>>> + id = ((frame->can_id & CAN_SFF_MASK) << 18);
I just realize that the {} for the else block is missing.
>>> + /*
>>> + * check for 'last error code' which tells us the
>>> + * type of the last error to occur on the CAN bus
>>> + */
>>> + switch (lec_type) {
>>> + /* common for all type of bus errors */
>>> + priv->can.can_stats.bus_error++;
>>> + stats->rx_errors++;
>>> + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
>>> + cf->data[2] |= CAN_ERR_PROT_UNSPEC;
>>
>> Are you sure that this part is ever executed? I wonder why the compile
>> does
>> not complain.
>
> I did not get any compilation error.
I know.
> But I will check if the program flow enters this part or not.
Good. It was *not* executed in my little user space test program.
...
>>> +static int __devexit c_can_plat_remove(struct platform_device *pdev)
>>> +{
>>> + struct net_device *dev = platform_get_drvdata(pdev);
>>> + struct c_can_priv *priv = netdev_priv(dev);
>>> + struct resource *mem;
>>> +
>>> + /* disable all interrupts */
>>> + c_can_enable_all_interrupts(priv, DISABLE_ALL_INTERRUPTS);
>>
>> To avoid exportign that function, couldn't it be done at the beginning
>> of
>> unregister_c_can_dev()?
>
> Yes this can be done.
> But, IMHO *disabling* interrupts seems more logical as part of __devexit
> Code.
I think the interrupts are already disabled when you unload the module
because the device must be closed (c_can_stop has been called). Anyway,
I think the above c_can_enable_all_interrupts() call is well placed in
the unregister function. I would avoid the export the function.
Wolfgang.
next prev parent reply other threads:[~2011-01-11 8:29 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-01-04 9:59 [PATCH net-next-2.6 v3 1/1] can: c_can: Added support for Bosch C_CAN controller Bhupesh Sharma
[not found] ` <1294135195-9448-1-git-send-email-bhupesh.sharma-qxv4g6HH51o@public.gmane.org>
2011-01-06 19:33 ` David Miller
[not found] ` <20110106.113356.102556872.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
2011-01-06 19:40 ` Wolfgang Grandegger
[not found] ` <4D261AAA.5030005-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
2011-01-06 19:44 ` Marc Kleine-Budde
[not found] ` <4D261BA4.2020003-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2011-01-06 20:08 ` Wolfgang Grandegger
[not found] ` <4D262158.4030301-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
2011-01-09 11:01 ` Oliver Hartkopp
[not found] ` <4D299583.10909-fJ+pQTUTwRTk1uMJSBkQmQ@public.gmane.org>
2011-01-09 14:40 ` Wolfgang Grandegger
[not found] ` <4D29C8F6.5030806-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
2011-01-11 4:13 ` Bhupesh SHARMA
[not found] ` <D5ECB3C7A6F99444980976A8C6D896384DEAFA2D56-8vAmw3ZAcdzhJTuQ9jeba9BPR1lH4CV8@public.gmane.org>
2011-01-11 8:31 ` Wolfgang Grandegger
2011-01-11 21:01 ` Oliver Hartkopp
2011-01-08 9:09 ` Wolfgang Grandegger
[not found] ` <4D2829C5.5020206-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
2011-01-11 4:50 ` Bhupesh SHARMA
[not found] ` <D5ECB3C7A6F99444980976A8C6D896384DEAFA2D9D-8vAmw3ZAcdzhJTuQ9jeba9BPR1lH4CV8@public.gmane.org>
2011-01-11 8:29 ` Wolfgang Grandegger [this message]
[not found] ` <4D2C14FE.7080903-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
2011-01-11 18:25 ` Wolfgang Grandegger
[not found] ` <4D2CA0AA.6080505-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
2011-01-11 22:01 ` Marc Kleine-Budde
[not found] ` <4D2CD34B.8000609-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2011-01-11 22:38 ` Wolfram Sang
[not found] ` <20110111223843.GB18762-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2011-01-11 22:48 ` Marc Kleine-Budde
[not found] ` <4D2CDE49.3030302-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2011-01-11 23:05 ` Wolfram Sang
2011-01-12 8:47 ` Wolfgang Grandegger
[not found] ` <4D2D6ABE.7000405-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
2011-01-12 8:53 ` Marc Kleine-Budde
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=4D2C14FE.7080903@grandegger.com \
--to=wg-5yr1bzd7o62+xt7jha+gda@public$(echo .)gmane.org \
--cc=Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public$(echo .)gmane.org \
--cc=bhupesh.sharma-qxv4g6HH51o@public$(echo .)gmane.org \
--cc=davem-fT/PcQaiUtIeIZ0/mPfg9Q@public$(echo .)gmane.org \
--cc=mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public$(echo .)gmane.org \
--cc=netdev-u79uwXL29TY76Z2rM5mHXA@public$(echo .)gmane.org \
--cc=socketcan-fJ+pQTUTwRTk1uMJSBkQmQ@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