On 09/13/2012 09:24 AM, AnilKumar, Chimata wrote: > Marc, > > On Wed, Sep 12, 2012 at 18:32:53, Marc Kleine-Budde wrote: >> On 09/12/2012 02:48 PM, AnilKumar, Chimata wrote: >>> Hi Marc, >>> >>> On Tue, Sep 04, 2012 at 12:57:18, Marc Kleine-Budde wrote: >>>> On 09/04/2012 08:14 AM, AnilKumar, Chimata wrote: >>>>> Marc, >>>>> >>>>> Thanks for the comments, >>>>> >>>>> On Tue, Sep 04, 2012 at 01:31:35, Marc Kleine-Budde wrote: >>>>>> On 09/03/2012 01:52 PM, AnilKumar Ch wrote: >>>>>>> Adds suspend resume support to DCAN driver which enables >>>>>>> DCAN power down mode bit (PDR). Then DCAN will ack the local >>>>>>> power-down mode by setting PDA bit in STATUS register. >>>>>>> >>>>>>> Also adds a status flag to know the status of DCAN module, >>>>>>> whether it is opened or not. >>>>>> >>>>>> Use "ndev->flags & IFF_UP" for that. Have a look at the at91_can driver >>>>>> [1]. I'm not sure if you need locking here. >>>>>> >>>>> >>>>> Then I can use this to check the status, lock is not >>>>> required. >>>>> >>>>>> [1] http://lxr.free-electrons.com/source/drivers/net/can/at91_can.c#L1198 >>>>>> >>>>>>> Signed-off-by: AnilKumar Ch <anilkumar@xxxxxx> >>>>>>> --- >>>>>>> drivers/net/can/c_can/c_can.c | 78 ++++++++++++++++++++++++++++++++ >>>>>>> drivers/net/can/c_can/c_can.h | 5 ++ >>>>>>> drivers/net/can/c_can/c_can_platform.c | 70 ++++++++++++++++++++++++++-- >>>>>>> 3 files changed, 150 insertions(+), 3 deletions(-) >>> >>> [snip] >>> >>>>>>> +#ifdef CONFIG_PM >>>>>>> +int c_can_power_down(struct net_device *dev) >>>>>>> +{ >>>>>>> + unsigned long time_out; >>>>>>> + struct c_can_priv *priv = netdev_priv(dev); >>>>>>> + >>>>>>> + if (!priv->is_opened) >>>>>>> + return 0; >>>>>> >>>>>> Should we add a BUG_ON(id->driver_data != BOSCH_D_CAN)? >>>>> >>>>> These APIs are called from platform driver where device type >>>>> device type is verified. I think we have to add BUG_ON() in >>>>> platform driver. >>>> >>>> The platform driver returns if not on D_CAN and will not call this >>>> function. But this functions are exported, so can be called by someone >>>> else. So if the caller is not D_CAN, it's a bug. >>>> >>> >>> I agree with you, but I have some concern here. >>> I think we should do "return 0;" instead of BUG_ON(). With >>> BUG_ON() system will hang and ultimately user lost his/her >>> contents. >> >> Good point, better safe then sorry. >> But this should not happen. What about WARN_ON()? >> > > Either would be fine printing a warning message or WARN_ON() I'm for a WARN_ON() 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:
signature.asc
Description: OpenPGP digital signature
