Re: [PATCH v6 1/7] media: V4L2: add temporary clock helpers

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

Hi Guennadi,

On Tuesday 19 March 2013 11:27:56 Guennadi Liakhovetski wrote:
> On Tue, 19 Mar 2013, Sylwester Nawrocki wrote:
> > >>> +	if (!IS_ERR(clk)&&  !try_module_get(clk->ops->owner))
> > >>> +		clk = ERR_PTR(-ENODEV);
> > >>> +	mutex_unlock(&clk_lock);
> > >>> +
> > >>> +	if (!IS_ERR(clk)) {
> > >>> +		clk->subdev = sd;
> > >> 
> > >> Why is this needed ? It seems a strange addition that might potentially
> > >> make transition to the common clocks API more difficult.
> > > 
> > > We got rid of the v4l2_clk_bind() function and the .bind() callback. Now
> > > I need a pointer to subdevice _before_ v4l2_clk_register() (former
> > > v4l2_clk_bound()), that's why I have to store it here.
> > 
> > Hmm, sorry, I'm not following. How can we store a subdev pointer in the
> > clock data structure that has not been registered yet and thus cannot be
> > found with v4l2_clk_find() ?
> 
> sorry, I meant v4l2_async_subdev_register(), not v4l2_clk_register(), my
> mistake. And I meant v4l2_async_subdev_bind(), v4l2_async_subdev_unbind().
> Before we had in the subdev driver (see imx074 example)
> 
> 	/* Tell the bridge the subdevice is about to bind */
> 	v4l2_async_subdev_bind();
> 
> 	/* get a clock */
> 	clk = v4l2_clk_get();
> 	if (IS_ERR(clk))
> 		return -EPROBE_DEFER;
> 
> 	/*
> 	 * enable the clock - this needs a subdev pointer, that we passed
> 	 * to the bridge with v4l2_async_subdev_bind() above
> 	 */
> 	v4l2_clk_enable(clk);
> 	do_probe();
> 	v4l2_clk_disable(clk);
> 
> 	/* inform the bridge: binding successful */
> 	v4l2_async_subdev_bound();
> 
> Now we have just
> 
> 	/* get a clock */
> 	clk = v4l2_clk_get();
> 	if (IS_ERR(clk))
> 		return -EPROBE_DEFER;
> 
> 	/*
> 	 * enable the clock - this needs a subdev pointer, that we stored
> 	 * in the clock object for the bridge driver to use with
> 	 * v4l2_clk_get() above
> 	 */
> 	v4l2_clk_enable(clk);
> 	do_probe();
> 	v4l2_clk_disable(clk);

I'm sorry, but I still don't understand why you need a pointer to the subdev 
in the clock provider implementation of v4l2_clk_enable/disable() :-)

> 	/* inform the bridge: binding successful */
> 	v4l2_async_subdev_bound();

-- 
Regards,

Laurent Pinchart