LinuxTV Patchwork [v5,03/13] media: v4l2-fwnode: add initial connector parsing support

login
register
mail settings
Submitter Marco Felsch
Date April 5, 2019, 6:03 a.m.
Message ID <20190405060317.11240-4-m.felsch@pengutronix.de>
Download mbox | patch
Permalink /patch/55466/
State New
Headers show

Comments

Marco Felsch - April 5, 2019, 6:03 a.m.
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/

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 | 113 ++++++++++++++++++++++++++
 include/media/v4l2-fwnode.h           |  16 ++++
 2 files changed, 129 insertions(+)
jacopo@jmondi.org - April 5, 2019, 1:06 p.m.
Hi Marco,
   thanks for the patch

On Fri, Apr 05, 2019 at 08:03:07AM +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/
>
> 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 | 113 ++++++++++++++++++++++++++
>  include/media/v4l2-fwnode.h           |  16 ++++
>  2 files changed, 129 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 20571846e636..a6bbe42ca518 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -592,6 +592,119 @@ 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 *name;
> +} connectors[] = {
> +	{
> +		.type = V4L2_CON_COMPOSITE,
> +		.name = "composite-video-connector",
> +	}, {
> +		.type = V4L2_CON_SVIDEO,
> +		.name = "svideo-connector",
> +	}, {
> +		.type = V4L2_CON_VGA,
> +		.name = "vga-connector",
> +	}, {
> +		.type = V4L2_CON_DVI,
> +		.name = "dvi-connector",
> +	}, {
> +		.type = V4L2_CON_HDMI,
> +		.name = "hdmi-connector"
> +	}
> +};
> +
> +static enum v4l2_connector_type
> +v4l2_fwnode_string_to_connector_type(const char *con_str)
> +{
> +	int i;

unsigned int

> +
> +	for (i = 0; i < ARRAY_SIZE(connectors); i++)
> +		if (!strcmp(con_str, connectors[i].name))
> +			return connectors[i].type;
> +
> +	/* no valid connector found */
> +	return V4L2_CON_UNKNOWN;
> +}
> +
> +static int
> +v4l2_fwnode_connector_parse_analog(struct fwnode_handle *fwnode,
> +				   struct v4l2_fwnode_connector *vc)
> +{
> +	u32 tvnorms;
> +	int ret;
> +
> +	ret = fwnode_property_read_u32(fwnode, "tvnorms", &tvnorms);
> +
> +	/* set it to V4L2_STD_ALL in case of not specified tvnorms */

Is this comment needed?

> +	vc->connector.analog.supported_tvnorms = ret ? V4L2_STD_ALL : tvnorms;

empty line before return?

> +	return 0;
> +}
> +
> +int v4l2_fwnode_parse_connector(struct fwnode_handle *__fwnode,
> +				struct v4l2_fwnode_connector *connector)
> +{
> +	struct fwnode_handle *fwnode;
> +	struct fwnode_endpoint __ep;
> +	const char *c_type_str, *label;
> +	int ret;
> +
> +	memset(connector, 0, sizeof(*connector));
> +
> +	fwnode = fwnode_graph_get_remote_port_parent(__fwnode);

You should fwnode_handle_put(fwnode) when done and in the error path

> +
> +	/* 1st parse all common properties */
> +	/* connector-type is stored within the compatible string */
> +	ret = fwnode_property_read_string(fwnode, "compatible", &c_type_str);
> +	if (ret)
> +		return -EINVAL;
> +
> +	connector->type = v4l2_fwnode_string_to_connector_type(c_type_str);
> +
> +	fwnode_graph_parse_endpoint(__fwnode, &__ep);
> +	connector->remote_port = __ep.port;
> +	connector->remote_id = __ep.id;
> +
> +	ret = fwnode_property_read_string(fwnode, "label", &label);
> +	if (!ret) {
> +		/* ensure label doesn't exceed V4L2_CONNECTOR_MAX_LABEL size */
> +		strscpy(connector->label, label, V4L2_CONNECTOR_MAX_LABEL);
> +	} else {
> +		/*
> +		 * labels are optional, if no one is given create one:

s/no one/none.

> +		 * <connector-type-string>@port<endpoint_port>/ep<endpoint_id>
> +		 */
> +		snprintf(connector->label, V4L2_CONNECTOR_MAX_LABEL,
> +			 "%s@port%u/ep%u", c_type_str, connector->remote_port,
> +			 connector->remote_id);
> +	}
> +
> +	/* now parse the connector specific properties */
> +	switch (connector->type) {
> +		/* fall through */

Could you move the /* fall through */ comment below case?

> +	case V4L2_CON_COMPOSITE:
here
> +	case V4L2_CON_SVIDEO:
> +		ret = v4l2_fwnode_connector_parse_analog(fwnode, connector);
> +		break;
> +		/* fall through */
> +	case V4L2_CON_VGA:
here
> +	case V4L2_CON_DVI:
here
> +	case V4L2_CON_HDMI:
> +		pr_warn("Connector specific parsing is currently not supported for %s\n",
> +			c_type_str);
> +		ret = 0;
> +		break;
> +		/* fall through */
> +	case V4L2_CON_UNKNOWN:
and here
> +	default:
> +		pr_err("Unknown connector type\n");
> +		ret = -EINVAL;
> +	};
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_fwnode_parse_connector);
> +
>  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 04344b71656f..aa8bbef8589b 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -269,6 +269,22 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
>   */
>  void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
>
> +/**
> + * v4l2_fwnode_parse_connector() - parse the connector on endpoint
> + * @fwnode: pointer to the endpoint's fwnode handle where the connector is
> + *          connected to.

No full stop please (even if this is highly unconsistent in this
header, some lines have it, some don't, but it seems to me the most of
them do not)

> + * @connector: pointer to the V4L2 fwnode connector data structure
> + *
> + * Fill the connector data structure with the connector type, label and the
> + * endpoint id and port where the connector belongs to. If no label is present
> + * a unique one will be created. Labels with more than 40 characters are cut.
> + *
> + * Return: %0 on success or a negative error code on failure:
> + *	   %-EINVAL on parsing failure

This matches the other function's documentation style. Nice!

> + */
> +int v4l2_fwnode_parse_connector(struct fwnode_handle *fwnode,
> +				struct v4l2_fwnode_connector *connector);
> +

All minor comments:
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

>  /**
>   * typedef parse_endpoint_func - Driver's callback function to be called on
>   *	each V4L2 fwnode endpoint.
> --
> 2.20.1
>
Marco Felsch - April 10, 2019, 10:31 a.m.
Hi Jacopo,

On 19-04-05 15:06, Jacopo Mondi wrote:
> Hi Marco,
>    thanks for the patch

thanks for the fast response =)

> On Fri, Apr 05, 2019 at 08:03:07AM +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/
> >
> > 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 | 113 ++++++++++++++++++++++++++
> >  include/media/v4l2-fwnode.h           |  16 ++++
> >  2 files changed, 129 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> > index 20571846e636..a6bbe42ca518 100644
> > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > @@ -592,6 +592,119 @@ 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 *name;
> > +} connectors[] = {
> > +	{
> > +		.type = V4L2_CON_COMPOSITE,
> > +		.name = "composite-video-connector",
> > +	}, {
> > +		.type = V4L2_CON_SVIDEO,
> > +		.name = "svideo-connector",
> > +	}, {
> > +		.type = V4L2_CON_VGA,
> > +		.name = "vga-connector",
> > +	}, {
> > +		.type = V4L2_CON_DVI,
> > +		.name = "dvi-connector",
> > +	}, {
> > +		.type = V4L2_CON_HDMI,
> > +		.name = "hdmi-connector"
> > +	}
> > +};
> > +
> > +static enum v4l2_connector_type
> > +v4l2_fwnode_string_to_connector_type(const char *con_str)
> > +{
> > +	int i;
> 
> unsigned int

Okay.

> > +
> > +	for (i = 0; i < ARRAY_SIZE(connectors); i++)
> > +		if (!strcmp(con_str, connectors[i].name))
> > +			return connectors[i].type;
> > +
> > +	/* no valid connector found */
> > +	return V4L2_CON_UNKNOWN;
> > +}
> > +
> > +static int
> > +v4l2_fwnode_connector_parse_analog(struct fwnode_handle *fwnode,
> > +				   struct v4l2_fwnode_connector *vc)
> > +{
> > +	u32 tvnorms;
> > +	int ret;
> > +
> > +	ret = fwnode_property_read_u32(fwnode, "tvnorms", &tvnorms);
> > +
> > +	/* set it to V4L2_STD_ALL in case of not specified tvnorms */
> 
> Is this comment needed?

Nope, let me reword it to: "tvnorms is optional"

> > +	vc->connector.analog.supported_tvnorms = ret ? V4L2_STD_ALL : tvnorms;
> 
> empty line before return?

Yes.

> > +	return 0;
> > +}
> > +
> > +int v4l2_fwnode_parse_connector(struct fwnode_handle *__fwnode,
> > +				struct v4l2_fwnode_connector *connector)
> > +{
> > +	struct fwnode_handle *fwnode;
> > +	struct fwnode_endpoint __ep;
> > +	const char *c_type_str, *label;
> > +	int ret;
> > +
> > +	memset(connector, 0, sizeof(*connector));
> > +
> > +	fwnode = fwnode_graph_get_remote_port_parent(__fwnode);
> 
> You should fwnode_handle_put(fwnode) when done and in the error path

Of course.

> > +
> > +	/* 1st parse all common properties */
> > +	/* connector-type is stored within the compatible string */
> > +	ret = fwnode_property_read_string(fwnode, "compatible", &c_type_str);
> > +	if (ret)
> > +		return -EINVAL;
> > +
> > +	connector->type = v4l2_fwnode_string_to_connector_type(c_type_str);
> > +
> > +	fwnode_graph_parse_endpoint(__fwnode, &__ep);
> > +	connector->remote_port = __ep.port;
> > +	connector->remote_id = __ep.id;
> > +
> > +	ret = fwnode_property_read_string(fwnode, "label", &label);
> > +	if (!ret) {
> > +		/* ensure label doesn't exceed V4L2_CONNECTOR_MAX_LABEL size */
> > +		strscpy(connector->label, label, V4L2_CONNECTOR_MAX_LABEL);
> > +	} else {
> > +		/*
> > +		 * labels are optional, if no one is given create one:
> 
> s/no one/none.

Okay.

> > +		 * <connector-type-string>@port<endpoint_port>/ep<endpoint_id>
> > +		 */
> > +		snprintf(connector->label, V4L2_CONNECTOR_MAX_LABEL,
> > +			 "%s@port%u/ep%u", c_type_str, connector->remote_port,
> > +			 connector->remote_id);
> > +	}
> > +
> > +	/* now parse the connector specific properties */
> > +	switch (connector->type) {
> > +		/* fall through */
> 
> Could you move the /* fall through */ comment below case?

Yes.

> > +	case V4L2_CON_COMPOSITE:
> here
> > +	case V4L2_CON_SVIDEO:
> > +		ret = v4l2_fwnode_connector_parse_analog(fwnode, connector);
> > +		break;
> > +		/* fall through */
> > +	case V4L2_CON_VGA:
> here
> > +	case V4L2_CON_DVI:
> here
> > +	case V4L2_CON_HDMI:
> > +		pr_warn("Connector specific parsing is currently not supported for %s\n",
> > +			c_type_str);
> > +		ret = 0;
> > +		break;
> > +		/* fall through */
> > +	case V4L2_CON_UNKNOWN:
> and here
> > +	default:
> > +		pr_err("Unknown connector type\n");
> > +		ret = -EINVAL;
> > +	};
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(v4l2_fwnode_parse_connector);
> > +
> >  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 04344b71656f..aa8bbef8589b 100644
> > --- a/include/media/v4l2-fwnode.h
> > +++ b/include/media/v4l2-fwnode.h
> > @@ -269,6 +269,22 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
> >   */
> >  void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
> >
> > +/**
> > + * v4l2_fwnode_parse_connector() - parse the connector on endpoint
> > + * @fwnode: pointer to the endpoint's fwnode handle where the connector is
> > + *          connected to.
> 
> No full stop please (even if this is highly unconsistent in this
> header, some lines have it, some don't, but it seems to me the most of
> them do not)

Yes I see it.. damn.

> > + * @connector: pointer to the V4L2 fwnode connector data structure
> > + *
> > + * Fill the connector data structure with the connector type, label and the
> > + * endpoint id and port where the connector belongs to. If no label is present
> > + * a unique one will be created. Labels with more than 40 characters are cut.
> > + *
> > + * Return: %0 on success or a negative error code on failure:
> > + *	   %-EINVAL on parsing failure
> 
> This matches the other function's documentation style. Nice!
> 
> > + */
> > +int v4l2_fwnode_parse_connector(struct fwnode_handle *fwnode,
> > +				struct v4l2_fwnode_connector *connector);
> > +
> 
> All minor comments:
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks for the review, I will add it in the next version.

> Thanks
>   j
> 

Regards,
  Marco

> >  /**
> >   * typedef parse_endpoint_func - Driver's callback function to be called on
> >   *	each V4L2 fwnode endpoint.
> > --
> > 2.20.1
> >

Patch

diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index 20571846e636..a6bbe42ca518 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -592,6 +592,119 @@  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 *name;
+} connectors[] = {
+	{
+		.type = V4L2_CON_COMPOSITE,
+		.name = "composite-video-connector",
+	}, {
+		.type = V4L2_CON_SVIDEO,
+		.name = "svideo-connector",
+	}, {
+		.type = V4L2_CON_VGA,
+		.name = "vga-connector",
+	}, {
+		.type = V4L2_CON_DVI,
+		.name = "dvi-connector",
+	}, {
+		.type = V4L2_CON_HDMI,
+		.name = "hdmi-connector"
+	}
+};
+
+static enum v4l2_connector_type
+v4l2_fwnode_string_to_connector_type(const char *con_str)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(connectors); i++)
+		if (!strcmp(con_str, connectors[i].name))
+			return connectors[i].type;
+
+	/* no valid connector found */
+	return V4L2_CON_UNKNOWN;
+}
+
+static int
+v4l2_fwnode_connector_parse_analog(struct fwnode_handle *fwnode,
+				   struct v4l2_fwnode_connector *vc)
+{
+	u32 tvnorms;
+	int ret;
+
+	ret = fwnode_property_read_u32(fwnode, "tvnorms", &tvnorms);
+
+	/* set it to V4L2_STD_ALL in case of not specified tvnorms */
+	vc->connector.analog.supported_tvnorms = ret ? V4L2_STD_ALL : tvnorms;
+	return 0;
+}
+
+int v4l2_fwnode_parse_connector(struct fwnode_handle *__fwnode,
+				struct v4l2_fwnode_connector *connector)
+{
+	struct fwnode_handle *fwnode;
+	struct fwnode_endpoint __ep;
+	const char *c_type_str, *label;
+	int ret;
+
+	memset(connector, 0, sizeof(*connector));
+
+	fwnode = fwnode_graph_get_remote_port_parent(__fwnode);
+
+	/* 1st parse all common properties */
+	/* connector-type is stored within the compatible string */
+	ret = fwnode_property_read_string(fwnode, "compatible", &c_type_str);
+	if (ret)
+		return -EINVAL;
+
+	connector->type = v4l2_fwnode_string_to_connector_type(c_type_str);
+
+	fwnode_graph_parse_endpoint(__fwnode, &__ep);
+	connector->remote_port = __ep.port;
+	connector->remote_id = __ep.id;
+
+	ret = fwnode_property_read_string(fwnode, "label", &label);
+	if (!ret) {
+		/* ensure label doesn't exceed V4L2_CONNECTOR_MAX_LABEL size */
+		strscpy(connector->label, label, V4L2_CONNECTOR_MAX_LABEL);
+	} else {
+		/*
+		 * labels are optional, if no one is given create one:
+		 * <connector-type-string>@port<endpoint_port>/ep<endpoint_id>
+		 */
+		snprintf(connector->label, V4L2_CONNECTOR_MAX_LABEL,
+			 "%s@port%u/ep%u", c_type_str, connector->remote_port,
+			 connector->remote_id);
+	}
+
+	/* now parse the connector specific properties */
+	switch (connector->type) {
+		/* fall through */
+	case V4L2_CON_COMPOSITE:
+	case V4L2_CON_SVIDEO:
+		ret = v4l2_fwnode_connector_parse_analog(fwnode, connector);
+		break;
+		/* fall through */
+	case V4L2_CON_VGA:
+	case V4L2_CON_DVI:
+	case V4L2_CON_HDMI:
+		pr_warn("Connector specific parsing is currently not supported for %s\n",
+			c_type_str);
+		ret = 0;
+		break;
+		/* fall through */
+	case V4L2_CON_UNKNOWN:
+	default:
+		pr_err("Unknown connector type\n");
+		ret = -EINVAL;
+	};
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_fwnode_parse_connector);
+
 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 04344b71656f..aa8bbef8589b 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -269,6 +269,22 @@  int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
  */
 void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
 
+/**
+ * v4l2_fwnode_parse_connector() - parse the connector on endpoint
+ * @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 and the
+ * endpoint id and port where the connector belongs to. If no label is present
+ * a unique one will be created. Labels with more than 40 characters are cut.
+ *
+ * Return: %0 on success or a negative error code on failure:
+ *	   %-EINVAL on parsing failure
+ */
+int v4l2_fwnode_parse_connector(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.

Privacy Policy