Re: [PATCH v2] media: V4L2: add temporary clock helpers

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Sakari,

On Tuesday 13 November 2012 01:37:51 Sakari Ailus wrote:
> On Mon, Nov 12, 2012 at 12:06:50PM +0100, Laurent Pinchart wrote:
> > On Monday 12 November 2012 00:33:17 Sakari Ailus wrote:
> > > On Wed, Oct 31, 2012 at 02:02:54PM +0100, Guennadi Liakhovetski wrote:
> > > > On Wed, 31 Oct 2012, Laurent Pinchart wrote:
> > > ...
> > > 
> > > > > > +#include <linux/atomic.h>
> > > > > > +#include <linux/errno.h>
> > > > > > +#include <linux/list.h>
> > > > > > +#include <linux/module.h>
> > > > > > +#include <linux/mutex.h>
> > > > > > +#include <linux/string.h>
> > > > > > +
> > > > > > +#include <media/v4l2-clk.h>
> > > > > > +#include <media/v4l2-subdev.h>
> > > > > > +
> > > > > > +static DEFINE_MUTEX(clk_lock);
> > > > > > +static LIST_HEAD(v4l2_clk);
> > > > > 
> > > > > As Sylwester mentioned, what about s/v4l2_clk/v4l2_clks/ ?
> > > > 
> > > > Don't you think naming of a static variable isn't important enough?
> > > > ;-) I think code authors should have enough freedom to at least pick
> > > > up single vs. plural form:-) "clks" is too many consonants for my
> > > > taste, if it were anything important I'd rather agree to "clk_head" or
> > > > "clk_list" or something similar.
> > > 
> > > clk_list makes sense IMO since the clk_ prefis is the same.
> > > 
> > > ...
> > > 
> > > > > > +void v4l2_clk_put(struct v4l2_clk *clk)
> > > > > > +{
> > > > > > +	if (!IS_ERR(clk))
> > > > > > +		module_put(clk->ops->owner);
> > > > > > +}
> > > > > > +EXPORT_SYMBOL(v4l2_clk_put);
> > > > > > +
> > > > > > +int v4l2_clk_enable(struct v4l2_clk *clk)
> > > > > > +{
> > > > > > +	if (atomic_inc_return(&clk->enable) = 1 && clk->ops->enable) {
> > > > > > +		int ret = clk->ops->enable(clk);
> > > > > > +		if (ret < 0)
> > > > > > +			atomic_dec(&clk->enable);
> > > > > > +		return ret;
> > > > > > +	}
> > > > > 
> > > > > I think you need a spinlock here instead of atomic operations. You
> > > > > could get preempted after atomic_inc_return() and before
> > > > > clk->ops->enable() by another process that would call
> > > > > v4l2_clk_enable(). The function would return with enabling the
> > > > > clock.
> > > > 
> > > > Sorry, what's the problem then? "Our" instance will succeed and call
> > > > ->enable() and the preempting instance will see the enable count > 1
> > > > and just return.
> > > 
> > > The clock is guaranteed to be enabled only after the call has returned.
> > > The second caller of v4lw_clk_enable() thus may proceed without the
> > > clock being enabled.
> > > 
> > > In principle enable() might also want to sleep, so how about using a
> > > mutex for the purpose instead of a spinlock?
> > 
> > If enable() needs to sleep we should split the enable call into prepare
> > and enable, like the common clock framework did.
> 
> I'm pretty sure we won't need to toggle this from interrupt context which is
> what the clock framework does, AFAIU. Accessing i2c subdevs mandates
> sleeping already.
> 
> We might not need to have a mutex either if no driver needs to sleep for
> this, still I guess this is more likely. I'm ok with both; just thought to
> mention this.

Right, I'm fine with a mutex for now, we'll split enable into enable and 
prepare later if needed.

-- 
Regards,

Laurent Pinchart