Re: [PATCH v11 11/15] media: tvp5150: add s_power callback

Re: [PATCH v11 11/15] media: tvp5150: add s_power callback

[Sakari Ailus]

Hi Marco,

On Mon, Sep 30, 2019 at 11:38:56AM +0200, Marco Felsch wrote:
> Don't en-/disable the interrupts during s_stream because someone can
> disable the stream but wants to get informed if the stream is locked
> again. So keep the interrupts enabled the whole time the pipeline is
> opened.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/media/i2c/tvp5150.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> index dda9f0a2995f..4afe2093b950 100644
> --- a/drivers/media/i2c/tvp5150.c
> +++ b/drivers/media/i2c/tvp5150.c
> @@ -1356,11 +1356,26 @@ static const struct media_entity_operations tvp5150_sd_media_ops = {
>  /****************************************************************************
>  			I2C Command
>   ****************************************************************************/
> +static int tvp5150_s_power(struct  v4l2_subdev *sd, int on)
> +{
> +	struct tvp5150 *decoder = to_tvp5150(sd);
> +	unsigned int val = 0;
> +
> +	if (on)
> +		val = TVP5150_INT_A_LOCK;
> +
> +	if (decoder->irq)
> +		/* Enable / Disable lock interrupt */
> +		regmap_update_bits(decoder->regmap, TVP5150_INT_ENABLE_REG_A,
> +				   TVP5150_INT_A_LOCK, val);

Could you use runtime PM to do this instead?

> +
> +	return 0;
> +}
>  
>  static int tvp5150_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct tvp5150 *decoder = to_tvp5150(sd);
> -	unsigned int mask, val = 0, int_val = 0;
> +	unsigned int mask, val = 0;
>  
>  	mask = TVP5150_MISC_CTL_YCBCR_OE | TVP5150_MISC_CTL_SYNC_OE |
>  	       TVP5150_MISC_CTL_CLOCK_OE;
> @@ -1373,15 +1388,10 @@ static int tvp5150_s_stream(struct v4l2_subdev *sd, int enable)
>  			val = decoder->lock ? decoder->oe : 0;
>  		else
>  			val = decoder->oe;
> -		int_val = TVP5150_INT_A_LOCK;
>  		v4l2_subdev_notify_event(&decoder->sd, &tvp5150_ev_fmt);
>  	}
>  
>  	regmap_update_bits(decoder->regmap, TVP5150_MISC_CTL, mask, val);
> -	if (decoder->irq)
> -		/* Enable / Disable lock interrupt */
> -		regmap_update_bits(decoder->regmap, TVP5150_INT_ENABLE_REG_A,
> -				   TVP5150_INT_A_LOCK, int_val);
>  
>  	return 0;
>  }
> @@ -1580,6 +1590,7 @@ static const struct v4l2_subdev_core_ops tvp5150_core_ops = {
>  	.g_register = tvp5150_g_register,
>  	.s_register = tvp5150_s_register,
>  #endif
> +	.s_power = tvp5150_s_power,
>  };
>  
>  static const struct v4l2_subdev_tuner_ops tvp5150_tuner_ops = {
> -- 
> 2.20.1
> 

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v11 11/15] media: tvp5150: add s_power callback

[Marco Felsch]

Hi Sakari,

On 19-10-24 14:59, Sakari Ailus wrote:
> Hi Marco,
> 
> On Mon, Sep 30, 2019 at 11:38:56AM +0200, Marco Felsch wrote:
> > Don't en-/disable the interrupts during s_stream because someone can
> > disable the stream but wants to get informed if the stream is locked
> > again. So keep the interrupts enabled the whole time the pipeline is
> > opened.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  drivers/media/i2c/tvp5150.c | 23 +++++++++++++++++------
> >  1 file changed, 17 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> > index dda9f0a2995f..4afe2093b950 100644
> > --- a/drivers/media/i2c/tvp5150.c
> > +++ b/drivers/media/i2c/tvp5150.c
> > @@ -1356,11 +1356,26 @@ static const struct media_entity_operations tvp5150_sd_media_ops = {
> >  /****************************************************************************
> >  			I2C Command
> >   ****************************************************************************/
> > +static int tvp5150_s_power(struct  v4l2_subdev *sd, int on)
> > +{
> > +	struct tvp5150 *decoder = to_tvp5150(sd);
> > +	unsigned int val = 0;
> > +
> > +	if (on)
> > +		val = TVP5150_INT_A_LOCK;
> > +
> > +	if (decoder->irq)
> > +		/* Enable / Disable lock interrupt */
> > +		regmap_update_bits(decoder->regmap, TVP5150_INT_ENABLE_REG_A,
> > +				   TVP5150_INT_A_LOCK, val);
> 
> Could you use runtime PM to do this instead?

You mean I should add a simple runtime_resume/suspend() which is called
during v4l2_subdev_internal_ops.open/close()? Of course I can do that
but why?

Regards,
  Marco

> > +
> > +	return 0;
> > +}
> >  
> >  static int tvp5150_s_stream(struct v4l2_subdev *sd, int enable)
> >  {
> >  	struct tvp5150 *decoder = to_tvp5150(sd);
> > -	unsigned int mask, val = 0, int_val = 0;
> > +	unsigned int mask, val = 0;
> >  
> >  	mask = TVP5150_MISC_CTL_YCBCR_OE | TVP5150_MISC_CTL_SYNC_OE |
> >  	       TVP5150_MISC_CTL_CLOCK_OE;
> > @@ -1373,15 +1388,10 @@ static int tvp5150_s_stream(struct v4l2_subdev *sd, int enable)
> >  			val = decoder->lock ? decoder->oe : 0;
> >  		else
> >  			val = decoder->oe;
> > -		int_val = TVP5150_INT_A_LOCK;
> >  		v4l2_subdev_notify_event(&decoder->sd, &tvp5150_ev_fmt);
> >  	}
> >  
> >  	regmap_update_bits(decoder->regmap, TVP5150_MISC_CTL, mask, val);
> > -	if (decoder->irq)
> > -		/* Enable / Disable lock interrupt */
> > -		regmap_update_bits(decoder->regmap, TVP5150_INT_ENABLE_REG_A,
> > -				   TVP5150_INT_A_LOCK, int_val);
> >  
> >  	return 0;
> >  }
> > @@ -1580,6 +1590,7 @@ static const struct v4l2_subdev_core_ops tvp5150_core_ops = {
> >  	.g_register = tvp5150_g_register,
> >  	.s_register = tvp5150_s_register,
> >  #endif
> > +	.s_power = tvp5150_s_power,
> >  };
> >  
> >  static const struct v4l2_subdev_tuner_ops tvp5150_tuner_ops = {
> > -- 
> > 2.20.1
> > 
> 
> -- 
> Sakari Ailus
> sakari.ailus@linux.intel.com
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v11 11/15] media: tvp5150: add s_power callback

[Sakari Ailus]

Hi Marco,

On Fri, Nov 08, 2019 at 11:25:02AM +0100, Marco Felsch wrote:
> Hi Sakari,
> 
> On 19-10-24 14:59, Sakari Ailus wrote:
> > Hi Marco,
> > 
> > On Mon, Sep 30, 2019 at 11:38:56AM +0200, Marco Felsch wrote:
> > > Don't en-/disable the interrupts during s_stream because someone can
> > > disable the stream but wants to get informed if the stream is locked
> > > again. So keep the interrupts enabled the whole time the pipeline is
> > > opened.
> > > 
> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > ---
> > >  drivers/media/i2c/tvp5150.c | 23 +++++++++++++++++------
> > >  1 file changed, 17 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> > > index dda9f0a2995f..4afe2093b950 100644
> > > --- a/drivers/media/i2c/tvp5150.c
> > > +++ b/drivers/media/i2c/tvp5150.c
> > > @@ -1356,11 +1356,26 @@ static const struct media_entity_operations tvp5150_sd_media_ops = {
> > >  /****************************************************************************
> > >  			I2C Command
> > >   ****************************************************************************/
> > > +static int tvp5150_s_power(struct  v4l2_subdev *sd, int on)
> > > +{
> > > +	struct tvp5150 *decoder = to_tvp5150(sd);
> > > +	unsigned int val = 0;
> > > +
> > > +	if (on)
> > > +		val = TVP5150_INT_A_LOCK;
> > > +
> > > +	if (decoder->irq)
> > > +		/* Enable / Disable lock interrupt */
> > > +		regmap_update_bits(decoder->regmap, TVP5150_INT_ENABLE_REG_A,
> > > +				   TVP5150_INT_A_LOCK, val);
> > 
> > Could you use runtime PM to do this instead?
> 
> You mean I should add a simple runtime_resume/suspend() which is called
> during v4l2_subdev_internal_ops.open/close()? Of course I can do that
> but why?

There's no reason to use generic mechanisms that have been around for ten
or so years. Eventually the s_power() op will be dropped.

-- 
Sakari Ailus

Re: [PATCH v11 04/15] media: v4l2-fwnode: add initial connector parsing support

[Sakari Ailus]

Hi Marco,

On Mon, Sep 30, 2019 at 11:38:49AM +0200, Marco Felsch wrote:
> The patch adds the initial connector parsing code, so we can move from a
> driver specific parsing code to a generic one. Currently only the
> generic fields and the analog-connector specific fields are parsed. Parsing
> the other connector specific fields can be added by a simple callbacks.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> [1] https://patchwork.kernel.org/cover/10794703/
> 
> v10:
> - drop V4L2_CONN_HDMI support
> - adapt pr_err msg to reflect new state (-> connector is unkown)
> 
> v9:
> - Fix leading semicolon found by kbuild semicolon.cocci
> 
> v8:
> - V4L2_CON_* -> V4L2_CONN_*
> - tvnorms -> sdtv-standards
> - adapt to new v4l2_fwnode_connector_analog member
> - return error in case of V4L2_CONN_HDMI
> 
> v7:
> @Jacopo: I dropped your r b tag becuase of the amount of changes I
> made..
> 
> - drop unnecessary comments
> - fix commet style
> - s/v4l2_fwnode_connector_conv.name/v4l2_fwnode_connector_conv.compatible/
> - make label size variable and drop V4L2_CONNECTOR_MAX_LABEL usage
> - do not assign a default label in case of no label was specified
> - remove useless /* fall through */ comments
> - add support for N connector links
> - rename local variables to be more meaningful
> - adjust kernedoc
> - add v4l2_fwnode_connector_free()
> - improve error handling (use different error values)
> - make use of pr_warn_once()
> 
> v6:
> - use unsigned count var
> - fix comment and style issues
> - place '/* fall through */' to correct places
> - fix error handling and cleanup by releasing fwnode
> - drop vga and dvi parsing support as those connectors are rarely used
>   these days
> 
> v5:
> - s/strlcpy/strscpy/
> 
> v2-v4:
> - nothing since the patch was squashed from series [1] into this
>   series.
> ---
>  drivers/media/v4l2-core/v4l2-fwnode.c | 129 ++++++++++++++++++++++++++
>  include/media/v4l2-fwnode.h           |  38 ++++++++
>  2 files changed, 167 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 3bd1888787eb..0bfa7cbf78df 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -595,6 +595,135 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
>  }
>  EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
>  
> +static const struct v4l2_fwnode_connector_conv {
> +	enum v4l2_connector_type type;
> +	const char *compatible;
> +} connectors[] = {
> +	{
> +		.type = V4L2_CONN_COMPOSITE,
> +		.compatible = "composite-video-connector",
> +	}, {
> +		.type = V4L2_CONN_SVIDEO,
> +		.compatible = "svideo-connector",
> +	},
> +};
> +
> +static enum v4l2_connector_type
> +v4l2_fwnode_string_to_connector_type(const char *con_str)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(connectors); i++)
> +		if (!strcmp(con_str, connectors[i].compatible))
> +			return connectors[i].type;
> +
> +	return V4L2_CONN_UNKNOWN;
> +}
> +
> +static int
> +v4l2_fwnode_connector_parse_analog(struct fwnode_handle *fwnode,
> +				   struct v4l2_fwnode_connector *vc)
> +{
> +	u32 stds;
> +	int ret;
> +
> +	ret = fwnode_property_read_u32(fwnode, "sdtv-standards", &stds);
> +
> +	/* The property is optional. */
> +	vc->connector.analog.sdtv_stds = ret ? V4L2_STD_ALL : stds;
> +
> +	return 0;
> +}
> +
> +void v4l2_fwnode_connector_free(struct v4l2_fwnode_connector *connector)
> +{
> +	unsigned int i;
> +
> +	if (IS_ERR_OR_NULL(connector))
> +		return;
> +
> +	for (i = 0; i < connector->nr_of_links; i++)
> +		v4l2_fwnode_put_link(&connector->links[i]);
> +	kfree(connector->links);

Please do set connector->links NULL here. That avoids accidentally double
kfree on the pointer.

> +}
> +EXPORT_SYMBOL_GPL(v4l2_fwnode_connector_free);
> +
> +int v4l2_fwnode_connector_alloc_parse(struct fwnode_handle *fwnode,
> +				      struct v4l2_fwnode_connector *connector)
> +{
> +	struct fwnode_handle *remote_pp, *remote_ep;
> +	const char *type_name;
> +	unsigned int i = 0, ep_num = 0;
> +	int err;
> +
> +	memset(connector, 0, sizeof(*connector));
> +
> +	remote_pp = fwnode_graph_get_remote_port_parent(fwnode);
> +	if (!remote_pp)
> +		return -ENOLINK;
> +
> +	/* Parse all common properties first. */
> +	fwnode_graph_for_each_endpoint(remote_pp, remote_ep)
> +		ep_num++;

If there are no endpoints, ep_num will be zero and kmalloc_array() will
return NULL? There should be a way there are no endpoints, rather than
returning -ENOMEM.

> +
> +	connector->nr_of_links = ep_num;
> +	connector->links = kmalloc_array(ep_num, sizeof(*connector->links),
> +					 GFP_KERNEL);
> +	if (!connector->links) {
> +		err = -ENOMEM;
> +		goto err_put_fwnode;
> +	}
> +
> +	fwnode_graph_for_each_endpoint(remote_pp, remote_ep) {
> +		err = v4l2_fwnode_parse_link(remote_ep, &connector->links[i]);

If you start parsing a connector starting from another device connected to
it, nothing seems to prevent parsing the same links twice, in case the
connector is connected to more than one sub-device.

Or do I miss something crucial here?

> +		if (err) {
> +			fwnode_handle_put(remote_ep);
> +			goto err_free_links;
> +		}
> +		i++;
> +	}
> +
> +	/*
> +	 * Links reference counting guarantees access -> no duplication needed
> +	 */
> +	fwnode_property_read_string(remote_pp, "label", &connector->label);
> +
> +	/* The connector-type is stored within the compatible string. */
> +	err = fwnode_property_read_string(remote_pp, "compatible", &type_name);
> +	if (err) {
> +		err = -EINVAL;
> +		goto err_free_links;
> +	}
> +	connector->type = v4l2_fwnode_string_to_connector_type(type_name);
> +
> +	/* Now parse the connector specific properties. */
> +	switch (connector->type) {
> +	case V4L2_CONN_COMPOSITE:
> +	case V4L2_CONN_SVIDEO:
> +		err = v4l2_fwnode_connector_parse_analog(remote_pp, connector);
> +		break;
> +	case V4L2_CONN_UNKNOWN:
> +	default:
> +		pr_err("Unknown connector type\n");
> +		err = -EINVAL;
> +		goto err_free_links;
> +	}
> +
> +	fwnode_handle_put(remote_pp);
> +
> +	return err;
> +
> +err_free_links:
> +	for (i = 0; i < ep_num; i++)
> +		v4l2_fwnode_put_link(&connector->links[i]);
> +	kfree(connector->links);
> +err_put_fwnode:
> +	fwnode_handle_put(remote_pp);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_fwnode_connector_alloc_parse);
> +
>  static int
>  v4l2_async_notifier_fwnode_parse_endpoint(struct device *dev,
>  					  struct v4l2_async_notifier *notifier,
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index 65da646b579e..800302aa84d8 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -276,6 +276,44 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
>   */
>  void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
>  
> +/**
> + * v4l2_fwnode_connector_free() - free the V4L2 connector acquired by
> + * v4l2_fwnode_parse_connector()

This would be nicer aligned with the text after the dash.

> + * @connector: the V4L2 connector the resources of which are to be released
> + *
> + * Drop references to the connection link partners and free the memory allocated
> + * by v4l2_fwnode_parse_connector() call.
> + *
> + * It is safe to call this function with NULL argument or on a V4L2 connector
> + * the parsing of which failed.
> + */
> +void v4l2_fwnode_connector_free(struct v4l2_fwnode_connector *connector);
> +
> +/**
> + * v4l2_fwnode_parse_connector() - parse the connector on endpoint

The name is different from the actual function.

> + * @fwnode: pointer to the endpoint's fwnode handle where the connector is
> + *          connected to
> + * @connector: pointer to the V4L2 fwnode connector data structure
> + *
> + * Fill the connector data structure with the connector type, label, all found
> + * links between the host and the connector as well as connector specific data.
> + * Since the label is optional it can be %NULL if no one was found.
> + *
> + * A reference is taken to both the connector and the connector destination
> + * where the connector belongs to. This must be dropped when no longer needed.
> + * Also the memory it has allocated to store the variable size data must be
> + * freed. Both dropping the references and freeing the memory is done by
> + * v4l2_fwnode_connector_free().
> + *
> + * Return:
> + * * %0 on success or a negative error code on failure:
> + * * %-ENOMEM if memory allocation failed
> + * * %-ENOLINK if the connector can't be found
> + * * %-EINVAL on parsing failure

Could this error code tell the endpoint is not connected to a connector?

I think the perfectly suitable error code for this would be -ENOTCONN. :-D

> + */
> +int v4l2_fwnode_connector_alloc_parse(struct fwnode_handle *fwnode,
> +				      struct v4l2_fwnode_connector *connector);
> +
>  /**
>   * typedef parse_endpoint_func - Driver's callback function to be called on
>   *	each V4L2 fwnode endpoint.
> 

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v11 04/15] media: v4l2-fwnode: add initial connector parsing support

[Marco Felsch]

Hi Sakari,

On 19-11-16 01:34, Sakari Ailus wrote:
> Hi Marco,
> 
> On Mon, Sep 30, 2019 at 11:38:49AM +0200, Marco Felsch wrote:
> > The patch adds the initial connector parsing code, so we can move from a
> > driver specific parsing code to a generic one. Currently only the
> > generic fields and the analog-connector specific fields are parsed. Parsing
> > the other connector specific fields can be added by a simple callbacks.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> > [1] https://patchwork.kernel.org/cover/10794703/
> > 
> > v10:
> > - drop V4L2_CONN_HDMI support
> > - adapt pr_err msg to reflect new state (-> connector is unkown)
> > 
> > v9:
> > - Fix leading semicolon found by kbuild semicolon.cocci
> > 
> > v8:
> > - V4L2_CON_* -> V4L2_CONN_*
> > - tvnorms -> sdtv-standards
> > - adapt to new v4l2_fwnode_connector_analog member
> > - return error in case of V4L2_CONN_HDMI
> > 
> > v7:
> > @Jacopo: I dropped your r b tag becuase of the amount of changes I
> > made..
> > 
> > - drop unnecessary comments
> > - fix commet style
> > - s/v4l2_fwnode_connector_conv.name/v4l2_fwnode_connector_conv.compatible/
> > - make label size variable and drop V4L2_CONNECTOR_MAX_LABEL usage
> > - do not assign a default label in case of no label was specified
> > - remove useless /* fall through */ comments
> > - add support for N connector links
> > - rename local variables to be more meaningful
> > - adjust kernedoc
> > - add v4l2_fwnode_connector_free()
> > - improve error handling (use different error values)
> > - make use of pr_warn_once()
> > 
> > v6:
> > - use unsigned count var
> > - fix comment and style issues
> > - place '/* fall through */' to correct places
> > - fix error handling and cleanup by releasing fwnode
> > - drop vga and dvi parsing support as those connectors are rarely used
> >   these days
> > 
> > v5:
> > - s/strlcpy/strscpy/
> > 
> > v2-v4:
> > - nothing since the patch was squashed from series [1] into this
> >   series.
> > ---
> >  drivers/media/v4l2-core/v4l2-fwnode.c | 129 ++++++++++++++++++++++++++
> >  include/media/v4l2-fwnode.h           |  38 ++++++++
> >  2 files changed, 167 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> > index 3bd1888787eb..0bfa7cbf78df 100644
> > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > @@ -595,6 +595,135 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
> >  }
> >  EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
> >  
> > +static const struct v4l2_fwnode_connector_conv {
> > +	enum v4l2_connector_type type;
> > +	const char *compatible;
> > +} connectors[] = {
> > +	{
> > +		.type = V4L2_CONN_COMPOSITE,
> > +		.compatible = "composite-video-connector",
> > +	}, {
> > +		.type = V4L2_CONN_SVIDEO,
> > +		.compatible = "svideo-connector",
> > +	},
> > +};
> > +
> > +static enum v4l2_connector_type
> > +v4l2_fwnode_string_to_connector_type(const char *con_str)
> > +{
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(connectors); i++)
> > +		if (!strcmp(con_str, connectors[i].compatible))
> > +			return connectors[i].type;
> > +
> > +	return V4L2_CONN_UNKNOWN;
> > +}
> > +
> > +static int
> > +v4l2_fwnode_connector_parse_analog(struct fwnode_handle *fwnode,
> > +				   struct v4l2_fwnode_connector *vc)
> > +{
> > +	u32 stds;
> > +	int ret;
> > +
> > +	ret = fwnode_property_read_u32(fwnode, "sdtv-standards", &stds);
> > +
> > +	/* The property is optional. */
> > +	vc->connector.analog.sdtv_stds = ret ? V4L2_STD_ALL : stds;
> > +
> > +	return 0;
> > +}
> > +
> > +void v4l2_fwnode_connector_free(struct v4l2_fwnode_connector *connector)
> > +{
> > +	unsigned int i;
> > +
> > +	if (IS_ERR_OR_NULL(connector))
> > +		return;
> > +
> > +	for (i = 0; i < connector->nr_of_links; i++)
> > +		v4l2_fwnode_put_link(&connector->links[i]);
> > +	kfree(connector->links);
> 
> Please do set connector->links NULL here. That avoids accidentally double
> kfree on the pointer.

Okay, I will do that.

> > +}
> > +EXPORT_SYMBOL_GPL(v4l2_fwnode_connector_free);
> > +
> > +int v4l2_fwnode_connector_alloc_parse(struct fwnode_handle *fwnode,
> > +				      struct v4l2_fwnode_connector *connector)
> > +{
> > +	struct fwnode_handle *remote_pp, *remote_ep;
> > +	const char *type_name;
> > +	unsigned int i = 0, ep_num = 0;
> > +	int err;
> > +
> > +	memset(connector, 0, sizeof(*connector));
> > +
> > +	remote_pp = fwnode_graph_get_remote_port_parent(fwnode);
> > +	if (!remote_pp)
> > +		return -ENOLINK;

Should I drop this logic here and force the caller to pass the connector
endpoint?

> > +
> > +	/* Parse all common properties first. */
> > +	fwnode_graph_for_each_endpoint(remote_pp, remote_ep)
> > +		ep_num++;
> 
> If there are no endpoints, ep_num will be zero and kmalloc_array() will
> return NULL? There should be a way there are no endpoints, rather than
> returning -ENOMEM.

Hm.. using the current implementation (please see my above comment)
ensures that there is always one link but if I change that I need to
check this, good point.

> > +
> > +	connector->nr_of_links = ep_num;
> > +	connector->links = kmalloc_array(ep_num, sizeof(*connector->links),
> > +					 GFP_KERNEL);
> > +	if (!connector->links) {
> > +		err = -ENOMEM;
> > +		goto err_put_fwnode;
> > +	}
> > +
> > +	fwnode_graph_for_each_endpoint(remote_pp, remote_ep) {
> > +		err = v4l2_fwnode_parse_link(remote_ep, &connector->links[i]);
> 
> If you start parsing a connector starting from another device connected to
> it, nothing seems to prevent parsing the same links twice, in case the
> connector is connected to more than one sub-device.
> 
> Or do I miss something crucial here?

That is true because currently each sub-device implements their own
connector parsing/handling stuff.. What i wanted to address with this
code is that one sub-device can have multiple links to one connector
e.g. s-video connectors (1xluma 1xchroma). I don't know how to handle
this without a simple connector device.

Regards,
  Marco

> > +		if (err) {
> > +			fwnode_handle_put(remote_ep);
> > +			goto err_free_links;
> > +		}
> > +		i++;
> > +	}
> > +
> > +	/*
> > +	 * Links reference counting guarantees access -> no duplication needed
> > +	 */
> > +	fwnode_property_read_string(remote_pp, "label", &connector->label);
> > +
> > +	/* The connector-type is stored within the compatible string. */
> > +	err = fwnode_property_read_string(remote_pp, "compatible", &type_name);
> > +	if (err) {
> > +		err = -EINVAL;
> > +		goto err_free_links;
> > +	}
> > +	connector->type = v4l2_fwnode_string_to_connector_type(type_name);
> > +
> > +	/* Now parse the connector specific properties. */
> > +	switch (connector->type) {
> > +	case V4L2_CONN_COMPOSITE:
> > +	case V4L2_CONN_SVIDEO:
> > +		err = v4l2_fwnode_connector_parse_analog(remote_pp, connector);
> > +		break;
> > +	case V4L2_CONN_UNKNOWN:
> > +	default:
> > +		pr_err("Unknown connector type\n");
> > +		err = -EINVAL;
> > +		goto err_free_links;
> > +	}
> > +
> > +	fwnode_handle_put(remote_pp);
> > +
> > +	return err;
> > +
> > +err_free_links:
> > +	for (i = 0; i < ep_num; i++)
> > +		v4l2_fwnode_put_link(&connector->links[i]);
> > +	kfree(connector->links);
> > +err_put_fwnode:
> > +	fwnode_handle_put(remote_pp);
> > +
> > +	return err;
> > +}
> > +EXPORT_SYMBOL_GPL(v4l2_fwnode_connector_alloc_parse);
> > +
> >  static int
> >  v4l2_async_notifier_fwnode_parse_endpoint(struct device *dev,
> >  					  struct v4l2_async_notifier *notifier,
> > diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> > index 65da646b579e..800302aa84d8 100644
> > --- a/include/media/v4l2-fwnode.h
> > +++ b/include/media/v4l2-fwnode.h
> > @@ -276,6 +276,44 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
> >   */
> >  void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
> >  
> > +/**
> > + * v4l2_fwnode_connector_free() - free the V4L2 connector acquired by
> > + * v4l2_fwnode_parse_connector()
> 
> This would be nicer aligned with the text after the dash.
> 
> > + * @connector: the V4L2 connector the resources of which are to be released
> > + *
> > + * Drop references to the connection link partners and free the memory allocated
> > + * by v4l2_fwnode_parse_connector() call.
> > + *
> > + * It is safe to call this function with NULL argument or on a V4L2 connector
> > + * the parsing of which failed.
> > + */
> > +void v4l2_fwnode_connector_free(struct v4l2_fwnode_connector *connector);
> > +
> > +/**
> > + * v4l2_fwnode_parse_connector() - parse the connector on endpoint
> 
> The name is different from the actual function.
> 
> > + * @fwnode: pointer to the endpoint's fwnode handle where the connector is
> > + *          connected to
> > + * @connector: pointer to the V4L2 fwnode connector data structure
> > + *
> > + * Fill the connector data structure with the connector type, label, all found
> > + * links between the host and the connector as well as connector specific data.
> > + * Since the label is optional it can be %NULL if no one was found.
> > + *
> > + * A reference is taken to both the connector and the connector destination
> > + * where the connector belongs to. This must be dropped when no longer needed.
> > + * Also the memory it has allocated to store the variable size data must be
> > + * freed. Both dropping the references and freeing the memory is done by
> > + * v4l2_fwnode_connector_free().
> > + *
> > + * Return:
> > + * * %0 on success or a negative error code on failure:
> > + * * %-ENOMEM if memory allocation failed
> > + * * %-ENOLINK if the connector can't be found
> > + * * %-EINVAL on parsing failure
> 
> Could this error code tell the endpoint is not connected to a connector?
> 
> I think the perfectly suitable error code for this would be -ENOTCONN. :-D
> 
> > + */
> > +int v4l2_fwnode_connector_alloc_parse(struct fwnode_handle *fwnode,
> > +				      struct v4l2_fwnode_connector *connector);
> > +
> >  /**
> >   * typedef parse_endpoint_func - Driver's callback function to be called on
> >   *	each V4L2 fwnode endpoint.
> > 
> 
> -- 
> Regards,
> 
> Sakari Ailus
> sakari.ailus@linux.intel.com
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v11 04/15] media: v4l2-fwnode: add initial connector parsing support

[Marco Felsch]

Hi Sakari,

gentle ping.

Regards,
  Marco

On 19-11-19 12:37, Marco Felsch wrote:
> Hi Sakari,
> 
> On 19-11-16 01:34, Sakari Ailus wrote:
> > Hi Marco,
> > 
> > On Mon, Sep 30, 2019 at 11:38:49AM +0200, Marco Felsch wrote:
> > > The patch adds the initial connector parsing code, so we can move from a
> > > driver specific parsing code to a generic one. Currently only the
> > > generic fields and the analog-connector specific fields are parsed. Parsing
> > > the other connector specific fields can be added by a simple callbacks.
> > > 
> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > ---
> > > [1] https://patchwork.kernel.org/cover/10794703/
> > > 
> > > v10:
> > > - drop V4L2_CONN_HDMI support
> > > - adapt pr_err msg to reflect new state (-> connector is unkown)
> > > 
> > > v9:
> > > - Fix leading semicolon found by kbuild semicolon.cocci
> > > 
> > > v8:
> > > - V4L2_CON_* -> V4L2_CONN_*
> > > - tvnorms -> sdtv-standards
> > > - adapt to new v4l2_fwnode_connector_analog member
> > > - return error in case of V4L2_CONN_HDMI
> > > 
> > > v7:
> > > @Jacopo: I dropped your r b tag becuase of the amount of changes I
> > > made..
> > > 
> > > - drop unnecessary comments
> > > - fix commet style
> > > - s/v4l2_fwnode_connector_conv.name/v4l2_fwnode_connector_conv.compatible/
> > > - make label size variable and drop V4L2_CONNECTOR_MAX_LABEL usage
> > > - do not assign a default label in case of no label was specified
> > > - remove useless /* fall through */ comments
> > > - add support for N connector links
> > > - rename local variables to be more meaningful
> > > - adjust kernedoc
> > > - add v4l2_fwnode_connector_free()
> > > - improve error handling (use different error values)
> > > - make use of pr_warn_once()
> > > 
> > > v6:
> > > - use unsigned count var
> > > - fix comment and style issues
> > > - place '/* fall through */' to correct places
> > > - fix error handling and cleanup by releasing fwnode
> > > - drop vga and dvi parsing support as those connectors are rarely used
> > >   these days
> > > 
> > > v5:
> > > - s/strlcpy/strscpy/
> > > 
> > > v2-v4:
> > > - nothing since the patch was squashed from series [1] into this
> > >   series.
> > > ---
> > >  drivers/media/v4l2-core/v4l2-fwnode.c | 129 ++++++++++++++++++++++++++
> > >  include/media/v4l2-fwnode.h           |  38 ++++++++
> > >  2 files changed, 167 insertions(+)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> > > index 3bd1888787eb..0bfa7cbf78df 100644
> > > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > > @@ -595,6 +595,135 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
> > >  }
> > >  EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
> > >  
> > > +static const struct v4l2_fwnode_connector_conv {
> > > +	enum v4l2_connector_type type;
> > > +	const char *compatible;
> > > +} connectors[] = {
> > > +	{
> > > +		.type = V4L2_CONN_COMPOSITE,
> > > +		.compatible = "composite-video-connector",
> > > +	}, {
> > > +		.type = V4L2_CONN_SVIDEO,
> > > +		.compatible = "svideo-connector",
> > > +	},
> > > +};
> > > +
> > > +static enum v4l2_connector_type
> > > +v4l2_fwnode_string_to_connector_type(const char *con_str)
> > > +{
> > > +	unsigned int i;
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(connectors); i++)
> > > +		if (!strcmp(con_str, connectors[i].compatible))
> > > +			return connectors[i].type;
> > > +
> > > +	return V4L2_CONN_UNKNOWN;
> > > +}
> > > +
> > > +static int
> > > +v4l2_fwnode_connector_parse_analog(struct fwnode_handle *fwnode,
> > > +				   struct v4l2_fwnode_connector *vc)
> > > +{
> > > +	u32 stds;
> > > +	int ret;
> > > +
> > > +	ret = fwnode_property_read_u32(fwnode, "sdtv-standards", &stds);
> > > +
> > > +	/* The property is optional. */
> > > +	vc->connector.analog.sdtv_stds = ret ? V4L2_STD_ALL : stds;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +void v4l2_fwnode_connector_free(struct v4l2_fwnode_connector *connector)
> > > +{
> > > +	unsigned int i;
> > > +
> > > +	if (IS_ERR_OR_NULL(connector))
> > > +		return;
> > > +
> > > +	for (i = 0; i < connector->nr_of_links; i++)
> > > +		v4l2_fwnode_put_link(&connector->links[i]);
> > > +	kfree(connector->links);
> > 
> > Please do set connector->links NULL here. That avoids accidentally double
> > kfree on the pointer.
> 
> Okay, I will do that.
> 
> > > +}
> > > +EXPORT_SYMBOL_GPL(v4l2_fwnode_connector_free);
> > > +
> > > +int v4l2_fwnode_connector_alloc_parse(struct fwnode_handle *fwnode,
> > > +				      struct v4l2_fwnode_connector *connector)
> > > +{
> > > +	struct fwnode_handle *remote_pp, *remote_ep;
> > > +	const char *type_name;
> > > +	unsigned int i = 0, ep_num = 0;
> > > +	int err;
> > > +
> > > +	memset(connector, 0, sizeof(*connector));
> > > +
> > > +	remote_pp = fwnode_graph_get_remote_port_parent(fwnode);
> > > +	if (!remote_pp)
> > > +		return -ENOLINK;
> 
> Should I drop this logic here and force the caller to pass the connector
> endpoint?
> 
> > > +
> > > +	/* Parse all common properties first. */
> > > +	fwnode_graph_for_each_endpoint(remote_pp, remote_ep)
> > > +		ep_num++;
> > 
> > If there are no endpoints, ep_num will be zero and kmalloc_array() will
> > return NULL? There should be a way there are no endpoints, rather than
> > returning -ENOMEM.
> 
> Hm.. using the current implementation (please see my above comment)
> ensures that there is always one link but if I change that I need to
> check this, good point.
> 
> > > +
> > > +	connector->nr_of_links = ep_num;
> > > +	connector->links = kmalloc_array(ep_num, sizeof(*connector->links),
> > > +					 GFP_KERNEL);
> > > +	if (!connector->links) {
> > > +		err = -ENOMEM;
> > > +		goto err_put_fwnode;
> > > +	}
> > > +
> > > +	fwnode_graph_for_each_endpoint(remote_pp, remote_ep) {
> > > +		err = v4l2_fwnode_parse_link(remote_ep, &connector->links[i]);
> > 
> > If you start parsing a connector starting from another device connected to
> > it, nothing seems to prevent parsing the same links twice, in case the
> > connector is connected to more than one sub-device.
> > 
> > Or do I miss something crucial here?
> 
> That is true because currently each sub-device implements their own
> connector parsing/handling stuff.. What i wanted to address with this
> code is that one sub-device can have multiple links to one connector
> e.g. s-video connectors (1xluma 1xchroma). I don't know how to handle
> this without a simple connector device.
> 
> Regards,
>   Marco
> 
> > > +		if (err) {
> > > +			fwnode_handle_put(remote_ep);
> > > +			goto err_free_links;
> > > +		}
> > > +		i++;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Links reference counting guarantees access -> no duplication needed
> > > +	 */
> > > +	fwnode_property_read_string(remote_pp, "label", &connector->label);
> > > +
> > > +	/* The connector-type is stored within the compatible string. */
> > > +	err = fwnode_property_read_string(remote_pp, "compatible", &type_name);
> > > +	if (err) {
> > > +		err = -EINVAL;
> > > +		goto err_free_links;
> > > +	}
> > > +	connector->type = v4l2_fwnode_string_to_connector_type(type_name);
> > > +
> > > +	/* Now parse the connector specific properties. */
> > > +	switch (connector->type) {
> > > +	case V4L2_CONN_COMPOSITE:
> > > +	case V4L2_CONN_SVIDEO:
> > > +		err = v4l2_fwnode_connector_parse_analog(remote_pp, connector);
> > > +		break;
> > > +	case V4L2_CONN_UNKNOWN:
> > > +	default:
> > > +		pr_err("Unknown connector type\n");
> > > +		err = -EINVAL;
> > > +		goto err_free_links;
> > > +	}
> > > +
> > > +	fwnode_handle_put(remote_pp);
> > > +
> > > +	return err;
> > > +
> > > +err_free_links:
> > > +	for (i = 0; i < ep_num; i++)
> > > +		v4l2_fwnode_put_link(&connector->links[i]);
> > > +	kfree(connector->links);
> > > +err_put_fwnode:
> > > +	fwnode_handle_put(remote_pp);
> > > +
> > > +	return err;
> > > +}
> > > +EXPORT_SYMBOL_GPL(v4l2_fwnode_connector_alloc_parse);
> > > +
> > >  static int
> > >  v4l2_async_notifier_fwnode_parse_endpoint(struct device *dev,
> > >  					  struct v4l2_async_notifier *notifier,
> > > diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> > > index 65da646b579e..800302aa84d8 100644
> > > --- a/include/media/v4l2-fwnode.h
> > > +++ b/include/media/v4l2-fwnode.h
> > > @@ -276,6 +276,44 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
> > >   */
> > >  void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
> > >  
> > > +/**
> > > + * v4l2_fwnode_connector_free() - free the V4L2 connector acquired by
> > > + * v4l2_fwnode_parse_connector()
> > 
> > This would be nicer aligned with the text after the dash.
> > 
> > > + * @connector: the V4L2 connector the resources of which are to be released
> > > + *
> > > + * Drop references to the connection link partners and free the memory allocated
> > > + * by v4l2_fwnode_parse_connector() call.
> > > + *
> > > + * It is safe to call this function with NULL argument or on a V4L2 connector
> > > + * the parsing of which failed.
> > > + */
> > > +void v4l2_fwnode_connector_free(struct v4l2_fwnode_connector *connector);
> > > +
> > > +/**
> > > + * v4l2_fwnode_parse_connector() - parse the connector on endpoint
> > 
> > The name is different from the actual function.
> > 
> > > + * @fwnode: pointer to the endpoint's fwnode handle where the connector is
> > > + *          connected to
> > > + * @connector: pointer to the V4L2 fwnode connector data structure
> > > + *
> > > + * Fill the connector data structure with the connector type, label, all found
> > > + * links between the host and the connector as well as connector specific data.
> > > + * Since the label is optional it can be %NULL if no one was found.
> > > + *
> > > + * A reference is taken to both the connector and the connector destination
> > > + * where the connector belongs to. This must be dropped when no longer needed.
> > > + * Also the memory it has allocated to store the variable size data must be
> > > + * freed. Both dropping the references and freeing the memory is done by
> > > + * v4l2_fwnode_connector_free().
> > > + *
> > > + * Return:
> > > + * * %0 on success or a negative error code on failure:
> > > + * * %-ENOMEM if memory allocation failed
> > > + * * %-ENOLINK if the connector can't be found
> > > + * * %-EINVAL on parsing failure
> > 
> > Could this error code tell the endpoint is not connected to a connector?
> > 
> > I think the perfectly suitable error code for this would be -ENOTCONN. :-D
> > 
> > > + */
> > > +int v4l2_fwnode_connector_alloc_parse(struct fwnode_handle *fwnode,
> > > +				      struct v4l2_fwnode_connector *connector);
> > > +
> > >  /**
> > >   * typedef parse_endpoint_func - Driver's callback function to be called on
> > >   *	each V4L2 fwnode endpoint.
> > > 
> > 
> > -- 
> > Regards,
> > 
> > Sakari Ailus
> > sakari.ailus@linux.intel.com
> > 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v11 04/15] media: v4l2-fwnode: add initial connector parsing support

[Marco Felsch]

Hi Sakari,

On 19-11-16 01:34, Sakari Ailus wrote:
> Hi Marco,
> 
> On Mon, Sep 30, 2019 at 11:38:49AM +0200, Marco Felsch wrote:

...

> > +int v4l2_fwnode_connector_alloc_parse(struct fwnode_handle *fwnode,
> > +				      struct v4l2_fwnode_connector *connector)
> > +{
> > +	struct fwnode_handle *remote_pp, *remote_ep;
> > +	const char *type_name;
> > +	unsigned int i = 0, ep_num = 0;
> > +	int err;
> > +
> > +	memset(connector, 0, sizeof(*connector));
> > +
> > +	remote_pp = fwnode_graph_get_remote_port_parent(fwnode);
> > +	if (!remote_pp)
> > +		return -ENOLINK;

I will align the API with the v4l2_fwnode_endpoint_alloc_parse()
function so the caller need to pass the connector fwnode handle.

> > +
> > +	/* Parse all common properties first. */
> > +	fwnode_graph_for_each_endpoint(remote_pp, remote_ep)
> > +		ep_num++;
> 
> If there are no endpoints, ep_num will be zero and kmalloc_array() will
> return NULL? There should be a way there are no endpoints, rather than
> returning -ENOMEM.
> 
> > +
> > +	connector->nr_of_links = ep_num;
> > +	connector->links = kmalloc_array(ep_num, sizeof(*connector->links),
> > +					 GFP_KERNEL);
> > +	if (!connector->links) {
> > +		err = -ENOMEM;
> > +		goto err_put_fwnode;
> > +	}
> > +
> > +	fwnode_graph_for_each_endpoint(remote_pp, remote_ep) {
> > +		err = v4l2_fwnode_parse_link(remote_ep, &connector->links[i]);
> 
> If you start parsing a connector starting from another device connected to
> it, nothing seems to prevent parsing the same links twice, in case the
> connector is connected to more than one sub-device.
> 
> Or do I miss something crucial here?

Yes thats right but it seems that sharing connectors isn't supported at
all. All bridge drivers using connectors implementing the connector
handling by their self. Anyway, I will add a function parameter to check
that we parse only the endpoints connected to the calling sub-dev.

Regards,
  Marco

> > +		if (err) {
> > +			fwnode_handle_put(remote_ep);
> > +			goto err_free_links;
> > +		}
> > +		i++;
> > +	}
> > +
> > +	/*
> > +	 * Links reference counting guarantees access -> no duplication needed
> > +	 */
> > +	fwnode_property_read_string(remote_pp, "label", &connector->label);
> > +
> > +	/* The connector-type is stored within the compatible string. */
> > +	err = fwnode_property_read_string(remote_pp, "compatible", &type_name);
> > +	if (err) {
> > +		err = -EINVAL;
> > +		goto err_free_links;
> > +	}
> > +	connector->type = v4l2_fwnode_string_to_connector_type(type_name);
> > +
> > +	/* Now parse the connector specific properties. */
> > +	switch (connector->type) {
> > +	case V4L2_CONN_COMPOSITE:
> > +	case V4L2_CONN_SVIDEO:
> > +		err = v4l2_fwnode_connector_parse_analog(remote_pp, connector);
> > +		break;
> > +	case V4L2_CONN_UNKNOWN:
> > +	default:
> > +		pr_err("Unknown connector type\n");
> > +		err = -EINVAL;
> > +		goto err_free_links;
> > +	}
> > +
> > +	fwnode_handle_put(remote_pp);
> > +
> > +	return err;
> > +
> > +err_free_links:
> > +	for (i = 0; i < ep_num; i++)
> > +		v4l2_fwnode_put_link(&connector->links[i]);
> > +	kfree(connector->links);
> > +err_put_fwnode:
> > +	fwnode_handle_put(remote_pp);
> > +
> > +	return err;
> > +}
> > +EXPORT_SYMBOL_GPL(v4l2_fwnode_connector_alloc_parse);
> > +

Re: [PATCH v11 04/15] media: v4l2-fwnode: add initial connector parsing support

[Marco Felsch]

Hi Sakari,

just a few questions about the below helper.

On 20-01-08 16:36, Marco Felsch wrote:
> Hi Sakari,
> 
> On 19-11-16 01:34, Sakari Ailus wrote:
> > Hi Marco,
> > 
> > On Mon, Sep 30, 2019 at 11:38:49AM +0200, Marco Felsch wrote:
> 
> ...
> 
> > > +int v4l2_fwnode_connector_alloc_parse(struct fwnode_handle *fwnode,
> > > +				      struct v4l2_fwnode_connector *connector)
> > > +{
> > > +	struct fwnode_handle *remote_pp, *remote_ep;
> > > +	const char *type_name;
> > > +	unsigned int i = 0, ep_num = 0;
> > > +	int err;
> > > +
> > > +	memset(connector, 0, sizeof(*connector));
> > > +
> > > +	remote_pp = fwnode_graph_get_remote_port_parent(fwnode);
> > > +	if (!remote_pp)
> > > +		return -ENOLINK;
> 
> I will align the API with the v4l2_fwnode_endpoint_alloc_parse()
> function so the caller need to pass the connector fwnode handle.

Should we really align this with v4l2_fwnode_endpoint_alloc_parse()? I
have some concerns because this moves the step to the user. Using the
current approach makes it easier for the user.

Regards,
  Marco

> > > +
> > > +	/* Parse all common properties first. */
> > > +	fwnode_graph_for_each_endpoint(remote_pp, remote_ep)
> > > +		ep_num++;
> > 
> > If there are no endpoints, ep_num will be zero and kmalloc_array() will
> > return NULL? There should be a way there are no endpoints, rather than
> > returning -ENOMEM.
> > 
> > > +
> > > +	connector->nr_of_links = ep_num;
> > > +	connector->links = kmalloc_array(ep_num, sizeof(*connector->links),
> > > +					 GFP_KERNEL);
> > > +	if (!connector->links) {
> > > +		err = -ENOMEM;
> > > +		goto err_put_fwnode;
> > > +	}
> > > +
> > > +	fwnode_graph_for_each_endpoint(remote_pp, remote_ep) {
> > > +		err = v4l2_fwnode_parse_link(remote_ep, &connector->links[i]);
> > 
> > If you start parsing a connector starting from another device connected to
> > it, nothing seems to prevent parsing the same links twice, in case the
> > connector is connected to more than one sub-device.
> > 
> > Or do I miss something crucial here?
> 
> Yes thats right but it seems that sharing connectors isn't supported at
> all. All bridge drivers using connectors implementing the connector
> handling by their self. Anyway, I will add a function parameter to check
> that we parse only the endpoints connected to the calling sub-dev.
> 
> Regards,
>   Marco
> 
> > > +		if (err) {
> > > +			fwnode_handle_put(remote_ep);
> > > +			goto err_free_links;
> > > +		}
> > > +		i++;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Links reference counting guarantees access -> no duplication needed
> > > +	 */
> > > +	fwnode_property_read_string(remote_pp, "label", &connector->label);
> > > +
> > > +	/* The connector-type is stored within the compatible string. */
> > > +	err = fwnode_property_read_string(remote_pp, "compatible", &type_name);
> > > +	if (err) {
> > > +		err = -EINVAL;
> > > +		goto err_free_links;
> > > +	}
> > > +	connector->type = v4l2_fwnode_string_to_connector_type(type_name);
> > > +
> > > +	/* Now parse the connector specific properties. */
> > > +	switch (connector->type) {
> > > +	case V4L2_CONN_COMPOSITE:
> > > +	case V4L2_CONN_SVIDEO:
> > > +		err = v4l2_fwnode_connector_parse_analog(remote_pp, connector);
> > > +		break;
> > > +	case V4L2_CONN_UNKNOWN:
> > > +	default:
> > > +		pr_err("Unknown connector type\n");
> > > +		err = -EINVAL;
> > > +		goto err_free_links;
> > > +	}
> > > +
> > > +	fwnode_handle_put(remote_pp);
> > > +
> > > +	return err;
> > > +
> > > +err_free_links:
> > > +	for (i = 0; i < ep_num; i++)
> > > +		v4l2_fwnode_put_link(&connector->links[i]);
> > > +	kfree(connector->links);
> > > +err_put_fwnode:
> > > +	fwnode_handle_put(remote_pp);
> > > +
> > > +	return err;
> > > +}
> > > +EXPORT_SYMBOL_GPL(v4l2_fwnode_connector_alloc_parse);
> > > +
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v11 04/15] media: v4l2-fwnode: add initial connector parsing support

[Marco Felsch]

Hi Sakari,

On 19-11-16 01:34, Sakari Ailus wrote:
> Hi Marco,
> 
> On Mon, Sep 30, 2019 at 11:38:49AM +0200, Marco Felsch wrote:

...

Let me sum up our irc discussion about that kAPI.

Our starting point is a fwnode based subdev which has connectors in
front of there pins. Connectors are used to limit the subdev to some
device limits e.g. if the device support only PAL-Input streams and the
subdev has an buggy autodetect mechanism. In that case the connector can
be used by the subdev to set the possible TV-Norms to PAL. Currently the
tvp5150 is the only fwnode based subdev implementing connectors.

Connectors have common and connector specific properties. All current
provided connectors can be found here:
Documentation/devicetree/bindings/display/connector/ .

Parsing the properties is common to all _upcoming_ fwnode based subdevs
so this should be done within the core. So lets move on to the parsing
helper.

> > +int v4l2_fwnode_connector_alloc_parse(struct fwnode_handle *fwnode,
> > +				      struct v4l2_fwnode_connector *connector)
> > +{

This kAPI seems to fit all current use cases. The API is not responsible
to alloc the 'struct v4l2_fwnode_connector' instead it is only used to
fill this struct. The given 'struct fwnode_handle' should be the subdev
local ep-fwnode because the user already has a reference to this ep.

This helper has two use-cases:
  1) Parsing the connector properties and add the initial (1st) link.
  2) Add further n-links upon n-calls to an already parsed connector.

Going this way we need to ensure that the caller init the 'struct
v4l2_fwnode_connector' to '0' before call this helper. This can be
documented within the kAPI doc.

Regards,
  Marco

Re: [PATCH v11 04/15] media: v4l2-fwnode: add initial connector parsing support

[Sakari Ailus]

Hi Marco,

On Wed, Jan 15, 2020 at 06:06:21PM +0100, Marco Felsch wrote:
> Hi Sakari,
> 
> On 19-11-16 01:34, Sakari Ailus wrote:
> > Hi Marco,
> > 
> > On Mon, Sep 30, 2019 at 11:38:49AM +0200, Marco Felsch wrote:
> 
> ...
> 
> Let me sum up our irc discussion about that kAPI.
> 
> Our starting point is a fwnode based subdev which has connectors in
> front of there pins. Connectors are used to limit the subdev to some
> device limits e.g. if the device support only PAL-Input streams and the
> subdev has an buggy autodetect mechanism. In that case the connector can
> be used by the subdev to set the possible TV-Norms to PAL. Currently the
> tvp5150 is the only fwnode based subdev implementing connectors.
> 
> Connectors have common and connector specific properties. All current
> provided connectors can be found here:
> Documentation/devicetree/bindings/display/connector/ .
> 
> Parsing the properties is common to all _upcoming_ fwnode based subdevs
> so this should be done within the core. So lets move on to the parsing
> helper.
> 
> > > +int v4l2_fwnode_connector_alloc_parse(struct fwnode_handle *fwnode,
> > > +				      struct v4l2_fwnode_connector *connector)
> > > +{
> 
> This kAPI seems to fit all current use cases. The API is not responsible
> to alloc the 'struct v4l2_fwnode_connector' instead it is only used to
> fill this struct. The given 'struct fwnode_handle' should be the subdev
> local ep-fwnode because the user already has a reference to this ep.
> 
> This helper has two use-cases:
>   1) Parsing the connector properties and add the initial (1st) link.
>   2) Add further n-links upon n-calls to an already parsed connector.
> 
> Going this way we need to ensure that the caller init the 'struct
> v4l2_fwnode_connector' to '0' before call this helper. This can be
> documented within the kAPI doc.

The problem with the above is that although the API does not prevent
sharing connectors as such, it does not support it either: parsing a
connector on another sub-device ends up looking like a new connector,
independently of whether one (or more) entities representing the same
connector already exist.

Either way, I discussed this with Hans, and we concluded it's fine for now.
We've dealt with similar changes before, so when someone needs sharing
connectors, he'll need to implement it, also changing the kAPI drivers use.

Could you address the other comments I've given on the set?

-- 
Kind regards,

Sakari Ailus