Re: [PATCH] cbus-retu-wdt: Store all data in device struct

2011-03-07 Thread Michael Büsch 
On Mon, 2011-03-07 at 09:43 +0200, Felipe Balbi wrote: 
  -   struct miscdevice   retu_wdt_miscdev;
  +   struct miscdevice   miscdev;
 
 this rename is not part of $SUBJECT

Well, that element is used in several new container_of() instances.
retu_wdt_miscdev is needlessly long and just increases the line-length
pain on the container_of() lines.
The retu_wdt prefix is useless, because it's an element of the retu_wdt
data structure.

  struct delayed_work ping_work;
  +   struct mutexmutex;
 
 checkpatch.pl --strict will complain about this mutex not having a
 comment explaining it. Can you add something ?

Is that really required? I mean, it's obvious what the mutex is used
for and what it is protecting (the whole struct).

  -static int _retu_modify_counter(unsigned int new)
  +static inline void _retu_modify_counter(struct retu_wdt_dev *wdev,
  +   unsigned int new)
 
 let the compiler inline it, maybe ?!?

Ok, it doesn't change the object code either way.

-- 
Greetings Michael.

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] cbus-retu-wdt: Store all data in device struct

2011-03-06 Thread Felipe Balbi 
On Fri, Mar 04, 2011 at 04:32:59PM +0100, Michael Buesch wrote:
 Get rid of all static global variables and store all
 information in the device structure. This cleans up
 the random mixup of information storage (struct vs static).
 
 Signed-off-by: Michael Buesch m...@bu3sch.de
 
 ---
 
 Index: linux-2.6.38-rc6/drivers/cbus/retu-wdt.c
 ===
 --- linux-2.6.38-rc6.orig/drivers/cbus/retu-wdt.c 2011-03-04 
 16:22:03.524019824 +0100
 +++ linux-2.6.38-rc6/drivers/cbus/retu-wdt.c  2011-03-04 16:22:31.186357952 
 +0100
 @@ -7,6 +7,8 @@
   *
   * Written by Amit Kucheria amit.kuche...@nokia.com
   *
 + * Cleanups by Michael Buesch m...@bu3sch.de (C) 2011
 + *
   * This file is subject to the terms and conditions of the GNU General
   * Public License. See the file COPYING in the main directory of this
   * archive for more details.
 @@ -48,37 +50,31 @@
  #define RETU_WDT_DEFAULT_TIMER 32
  #define RETU_WDT_MAX_TIMER 63
  
 -static DEFINE_MUTEX(retu_wdt_mutex);
 -
 -/* Current period of watchdog */
 -static unsigned int period_val = RETU_WDT_DEFAULT_TIMER;
 -
  struct retu_wdt_dev {
   struct device   *dev;
 + unsigned intperiod_val; /* Current period of watchdog */
   unsigned long   users;
 - struct miscdevice   retu_wdt_miscdev;
 + struct miscdevice   miscdev;

this rename is not part of $SUBJECT

   struct delayed_work ping_work;
 + struct mutexmutex;

checkpatch.pl --strict will complain about this mutex not having a
comment explaining it. Can you add something ?

  };
  
 -static struct retu_wdt_dev *retu_wdt;
  
 -static int _retu_modify_counter(unsigned int new)
 +static inline void _retu_modify_counter(struct retu_wdt_dev *wdev,
 + unsigned int new)

let the compiler inline it, maybe ?!?

-- 
balbi
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html