[PATCH 1/3] Add a common struct clk

[u.kleine-koenig@pengutronix.de (Uwe Kleine-König)]

Hello Jeremy,

On Wed, Dec 08, 2010 at 09:02:37AM +0800, Jeremy Kerr wrote:
> > I assume the initial feedback should be provided from someone internal
> > to Canonical or Linaro?  Can you give an estimate when you can post it,
> > I really thing that's the way to go for simplifying the clock code on
> > imx which is on my todo list.
> 
> No, I was waiting on feedback from the ST-E platform folks, who will need the 
> atomic clocks. However, I've been out of action for a couple of weeks, hence 
> the delay.
> 
> I'll get the next revision posted this week.
Great.
 
> > While reading quickly over the patch I wondered if there isn't a better
> > way to get that spinlock/mutex thingy implemented.
> > 
> > You currently have:
> > 
> > 	struct clk {
> > 	       const struct clk_ops    *ops;
> > 	       unsigned int            enable_count;
> > 	       int                     flags;
> > 	       union {
> > 	               struct mutex    mutex;
> > 	               spinlock_t      spinlock;
> > 	       } lock;
> > 	};
> > 
> > What about using this one instead?:
> > 
> > 	struct clk_base {
> > 		/* merge that with ops?  Probably not */
> > 		const struct clk_lock_ops *lock_ops;
> > 		const struct clk_ops *ops;
> > 		unsigned int enable_count;
> > 	};
> > 
> > 	struct clk {
> > 		struct clk_base base;
> > 		struct mutex lock;
> > 	};
> > 
> > 	struct clk_atomic {
> > 		struct clk_base base;
> > 		spinlock_t lock;
> > 	};
> 
> This means we'll need a separate API (clk_get_rate, etc) for the atomic 
> clocks, or change the API to take a clk_base (and then fix up all the users of 
> the API).
Ah, that's true.  As I said, I didnt' thought it to an end, just seemed
to be clearer to me.
 
> Regardless, I'd prefer to keep the separation to just the lock itself, rather 
> than percolating down to other interfaces.
> 
> 
> > This way and when I prefer to use the sleeping variant only I don't need
> > to bother with spinlocks at all.
> 
> How do you mean? You shouldn't need to deal with spinlocks with the current 
> code if you're just using non-atomic clocks.
Of course I can ignore them, this is more that I don't like having
members in structs or unions that are unused.  (As a mutex contains a
spinlock anyhow this is admittedly a bit strange when thinking again.
:-)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |