Re: [REVIEW PATCH v3 1/2] media: Change media device link_notify behaviour

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

On Monday 10 June 2013 12:47:02 Sylwester Nawrocki wrote:
> On 06/10/2013 12:10 AM, Sakari Ailus wrote:
> > Sylwester Nawrocki wrote:
> > ...
> > 
> >> diff --git a/drivers/media/platform/omap3isp/isp.c
> >> b/drivers/media/platform/omap3isp/isp.c index 1d7dbd5..1a2d25c 100644
> >> --- a/drivers/media/platform/omap3isp/isp.c
> >> +++ b/drivers/media/platform/omap3isp/isp.c
> >> @@ -792,9 +792,9 @@ int omap3isp_pipeline_pm_use(struct media_entity
> >> *entity, int use)>> 
> >>   /*
> >>   
> >>    * isp_pipeline_link_notify - Link management notification callback
> >> 
> >> - * @source: Pad at the start of the link
> >> - * @sink: Pad at the end of the link
> >> + * @link: The link
> >> 
> >>    * @flags: New link flags that will be applied
> >> 
> >> + * @notification: The link's state change notification type
> >> (MEDIA_DEV_NOTIFY_*)>> 
> >>    *
> >>    * React to link management on powered pipelines by updating the use
> >>    count of * all entities in the source and sink sides of the link.
> >>    Entities are powered>> 
> >> @@ -804,29 +804,38 @@ int omap3isp_pipeline_pm_use(struct media_entity
> >> *entity, int use)>> 
> >>    * off is assumed to never fail. This function will not fail for
> >>    disconnection * events.
> >>    */
> >> 
> >> -static int isp_pipeline_link_notify(struct media_pad *source,
> >> -				    struct media_pad *sink, u32 flags)
> >> +static int isp_pipeline_link_notify(struct media_link *link, u32 flags,
> >> +				    unsigned int notification)
> >> 
> >>   {
> >> 
> >> -	int source_use = isp_pipeline_pm_use_count(source->entity);
> >> -	int sink_use = isp_pipeline_pm_use_count(sink->entity);
> >> +	struct media_entity *source = link->source->entity;
> >> +	struct media_entity *sink = link->sink->entity;
> >> +	int source_use = isp_pipeline_pm_use_count(source);
> >> +	int sink_use = isp_pipeline_pm_use_count(sink);
> >> 
> >>   	int ret;
> >> 
> >> -	if (!(flags & MEDIA_LNK_FL_ENABLED)) {
> >> +	if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH &&
> >> +	    !(link->flags & MEDIA_LNK_FL_ENABLED)) {
> >> 
> >>   		/* Powering off entities is assumed to never fail. */
> >> 
> >> -		isp_pipeline_pm_power(source->entity, -sink_use);
> >> -		isp_pipeline_pm_power(sink->entity, -source_use);
> >> +		isp_pipeline_pm_power(source, -sink_use);
> >> +		isp_pipeline_pm_power(sink, -source_use);
> >> 
> >>   		return 0;
> >>   	
> >>   	}
> >> 
> >> -	ret = isp_pipeline_pm_power(source->entity, sink_use);
> >> -	if (ret < 0)
> >> -		return ret;
> >> +	if (notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH &&
> >> +		(flags & MEDIA_LNK_FL_ENABLED)) {
> > 
> > You could return zero here if the opposite was true, and unindent the
> > rest. Up to you --- the patch is fine.

I would personally keep the code as-is, to keep the symmetry, but I'm fine 
with both :-)

> All right, thanks for the Ack. An updated patch to follow.
> 
> > Acked-by: Sakari Ailus <sakari.ailus@iki.fi>
> > 
> >> -	ret = isp_pipeline_pm_power(sink->entity, source_use);
> >> -	if (ret < 0)
> >> -		isp_pipeline_pm_power(source->entity, -sink_use);
> >> +		ret = isp_pipeline_pm_power(source, sink_use);
> >> +		if (ret < 0)
> >> +			return ret;
> >> 
> >> -	return ret;
> >> +		ret = isp_pipeline_pm_power(sink, source_use);
> >> +		if (ret < 0)
> >> +			isp_pipeline_pm_power(source, -sink_use);
> >> +
> >> +		return ret;
> >> +	}
> >> +
> >> +	return 0;
> >> 
> >>   }

-- 
Regards,

Laurent Pinchart