Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Marco Felsch <m.felsch@pengutronix.de>
Cc: mchehab@kernel.org, sakari.ailus@linux.intel.com,
	hans.verkuil@cisco.com, jacopo+renesas@jmondi.org,
	robh+dt@kernel.org, linux-media@vger.kernel.org,
	devicetree@vger.kernel.org, kernel@pengutronix.de,
	Jacopo Mondi <jacopo@jmondi.org>
Subject: Re: [PATCH v6 02/13] media: v4l2-fwnode: add v4l2_fwnode_connector
Date: Thu, 15 Aug 2019 15:38:10 +0300
Message-ID: <20190815123810.GC13823@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20190809075536.pukp444dmb7haoxj@pengutronix.de>

Hi Marco,

On Fri, Aug 09, 2019 at 09:55:36AM +0200, Marco Felsch wrote:
> On 19-05-16 19:36, Laurent Pinchart wrote:
> > On Mon, Apr 15, 2019 at 02:44:02PM +0200, Marco Felsch wrote:
> > > Currently every driver needs to parse the connector endpoints by it self.
> > 
> > s/it self/itself/
> > 
> > > This is the initial work to make this generic. The generic connector has
> > > some common fields and some connector specific parts. The generic one
> > > includes:
> > >   - type
> > >   - label
> > >   - remote_port (the port where the connector is connected to)
> > >   - remote_id   (the endpoint where the connector is connected to)
> > 
> > This assumes a single connection between a connector and a remote port,
> > and a single port on the connector side. Is this guaranteed ? For the
> > mini-DIN-4 connectors (often used for S-Video) for instance, I recall
> > from the extensive discussions we had in the past that they should be
> > modeled with two pins, one for the Y component and one for C components.
> > The rationale for this is to support systems where such a connector
> > could be used to carry S-Video, but also two composite video signals
> > (usually through an external adapter from 2 RCA female connectors to one
> > S-Video male connector) that would be routed to two separate video
> > decoders (or two different inputs of the same video decoder). Other
> > topologies may be possible too.
> 
> I got your concerns and I also remember the tvp5150 port bindings
> myself in the past. Do you know how often such a setup you described
> above happens these days? I would rather add more documentation to the
> bindings [1] and add a check to v4l2_fwnode_parse_connector() to
> guarantee that one port has only one endpoint. Because I don't think
> that analog connectors has a bright future these days.
> 
> [1] Documentation/devicetree/bindings/display/connector/ \
>     analog-tv-connector.txt

I have seen it on older hardware, I don't know about more recent
systems. For the S-Video case at least, you need to support two DT
ports, even if you don't support connecting them to two different
devices yet.

In any case, I'm fine if those topologies are not supported yet, but it
should be possible to support them in a backward-compatible way. In
particular, in this case, we should make sure the DT bindings will allow
such topologies, and the DT parsing API should make it possible to
support them without fugure changes to drivers that use the API from
this patch for "simple" topologies.

> > > The specific fields are within a union, since only one of them can be
> > > available at the time. Since this is the initial support the patch adds
> > > only the analog-connector specific ones.
> > > 
> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > > [1] https://patchwork.kernel.org/cover/10794703/
> > > 
> > > v6:
> > > - fix some spelling and style issues
> > > - rm unnecessary comments
> > > - drop vga and dvi connector
> > > 
> > > v2-v4:
> > > - nothing since the patch was squashed from series [1] into this
> > >   series.
> > > 
> > >  include/media/v4l2-connector.h | 30 ++++++++++++++++++++++++++++++
> > >  include/media/v4l2-fwnode.h    | 33 +++++++++++++++++++++++++++++++++
> > >  2 files changed, 63 insertions(+)
> > >  create mode 100644 include/media/v4l2-connector.h
> > > 
> > > diff --git a/include/media/v4l2-connector.h b/include/media/v4l2-connector.h
> > > new file mode 100644
> > > index 000000000000..3a951c54f50e
> > > --- /dev/null
> > > +++ b/include/media/v4l2-connector.h
> > > @@ -0,0 +1,30 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > +/*
> > > + * v4l2-connector.h
> > > + *
> > > + * V4L2 connector types.
> > > + *
> > > + * Copyright 2019 Pengutronix, Marco Felsch <kernel@pengutronix.de>
> > > + */
> > > +
> > > +#ifndef V4L2_CONNECTOR_H
> > > +#define V4L2_CONNECTOR_H
> > > +
> > > +#define V4L2_CONNECTOR_MAX_LABEL 41
> > 
> > Hans pointed out this was a weird number. Should you turn the label
> > field into a pointer to make this more generic (with a
> > v4l2_fwnode_connector_cleanup() function then) ?
> 
> Yes, that would be the better approach. I will change that.
> 
> > > +
> > > +/**
> > > + * enum v4l2_connector_type - connector type
> > > + * @V4L2_CON_UNKNOWN:   unknown connector type, no V4L2 connetor configuration
> > > + * @V4L2_CON_COMPOSITE: analog composite connector
> > > + * @V4L2_CON_SVIDEO:    analog svideo connector
> > > + * @V4L2_CON_HDMI:      digital hdmi connector
> > > + */
> > > +enum v4l2_connector_type {
> > > +	V4L2_CON_UNKNOWN,
> > > +	V4L2_CON_COMPOSITE,
> > > +	V4L2_CON_SVIDEO,
> > > +	V4L2_CON_HDMI,
> > > +};
> > > +
> > > +#endif /* V4L2_CONNECTOR_H */
> > > +
> > > diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> > > index 6c07825e18b9..f4df1b95c5ef 100644
> > > --- a/include/media/v4l2-fwnode.h
> > > +++ b/include/media/v4l2-fwnode.h
> > > @@ -22,6 +22,7 @@
> > >  #include <linux/list.h>
> > >  #include <linux/types.h>
> > >  
> > > +#include <media/v4l2-connector.h>
> > >  #include <media/v4l2-mediabus.h>
> > >  #include <media/v4l2-subdev.h>
> > >  
> > > @@ -126,6 +127,38 @@ struct v4l2_fwnode_link {
> > >  	unsigned int remote_port;
> > >  };
> > >  
> > > +/**
> > > + * struct v4l2_fwnode_connector_analog - analog connector data structure
> > > + * @supported_tvnorms: tv norms this connector supports, set to V4L2_STD_ALL
> > > + *                     if no restrictions are specified.
> > > + */
> > > +struct v4l2_fwnode_connector_analog {
> > > +	v4l2_std_id supported_tvnorms;
> > > +};
> > > +
> > > +/**
> > > + * struct v4l2_fwnode_connector - the connector data structure
> > > + * @remote_port: identifier of the remote endpoint port the connector connects
> > > + *		 to
> > > + * @remote_id: identifier of the remote endpoint the connector connects to
> > > + * @label: connetor label
> > > + * @type: connector type
> > > + * @connector: connector configuration
> > > + * @connector.analog: analog connector configuration
> > > + *                    &struct v4l2_fwnode_connector_analog
> > > + */
> > > +struct v4l2_fwnode_connector {
> > > +	unsigned int remote_port;
> > > +	unsigned int remote_id;
> > > +	char label[V4L2_CONNECTOR_MAX_LABEL];
> > > +	enum v4l2_connector_type type;
> > > +
> > > +	union {
> > > +		struct v4l2_fwnode_connector_analog analog;
> > > +		/* future connectors */
> > > +	} connector;
> > > +};
> > > +
> > >  /**
> > >   * v4l2_fwnode_endpoint_parse() - parse all fwnode node properties
> > >   * @fwnode: pointer to the endpoint's fwnode handle

-- 
Regards,

Laurent Pinchart

  reply index

Thread overview: 70+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-15 12:44 [PATCH v6 00/13] TVP5150 new features Marco Felsch
2019-04-15 12:44 ` [PATCH v6 01/13] dt-bindings: connector: analog: add tv norms property Marco Felsch
2019-05-06 10:01   ` Hans Verkuil
2019-05-06 10:06     ` Hans Verkuil
2019-05-14 18:11     ` Mauro Carvalho Chehab
2019-08-09  6:00       ` Marco Felsch
2019-05-16 16:27   ` Laurent Pinchart
2019-08-09  5:58     ` Marco Felsch
2019-08-15 12:33       ` Laurent Pinchart
2019-08-15 12:50         ` Marco Felsch
2019-08-15 13:02           ` Laurent Pinchart
2019-08-15 13:35             ` Marco Felsch
2019-04-15 12:44 ` [PATCH v6 02/13] media: v4l2-fwnode: add v4l2_fwnode_connector Marco Felsch
2019-05-06  9:50   ` Hans Verkuil
2019-05-14 18:17     ` Mauro Carvalho Chehab
2019-08-09  7:20     ` Marco Felsch
2019-05-16 16:36   ` Laurent Pinchart
2019-08-09  7:55     ` Marco Felsch
2019-08-15 12:38       ` Laurent Pinchart [this message]
2019-08-15 13:04         ` Marco Felsch
2019-08-15 13:10           ` Laurent Pinchart
2019-08-15 13:37             ` Marco Felsch
2019-04-15 12:44 ` [PATCH v6 03/13] media: v4l2-fwnode: add initial connector parsing support Marco Felsch
2019-05-06 10:10   ` Hans Verkuil
2019-05-14 18:20     ` Mauro Carvalho Chehab
2019-05-16 16:51       ` Laurent Pinchart
2019-08-09 12:16         ` Marco Felsch
2019-08-15 12:48           ` Laurent Pinchart
2019-08-15 13:14             ` Marco Felsch
2019-08-09  8:59       ` Marco Felsch
2019-04-15 12:44 ` [PATCH v6 04/13] partial revert of "[media] tvp5150: add HW input connectors support" Marco Felsch
2019-04-15 12:44 ` [PATCH v6 05/13] media: tvp5150: add input source selection of_graph support Marco Felsch
2019-05-06 10:09   ` Jacopo Mondi
2019-05-14 18:25   ` Mauro Carvalho Chehab
2019-05-16 18:03     ` Laurent Pinchart
2019-08-13  8:54       ` Marco Felsch
2019-08-15 12:51         ` Laurent Pinchart
2019-08-15 13:22           ` Marco Felsch
2019-08-15 13:26             ` Laurent Pinchart
2019-04-15 12:44 ` [PATCH v6 06/13] media: dt-bindings: tvp5150: Add input port connectors DT bindings Marco Felsch
2019-05-14 18:27   ` Mauro Carvalho Chehab
2019-05-16 18:05   ` Laurent Pinchart
2019-08-13  8:56     ` Marco Felsch
2019-04-15 12:44 ` [PATCH v6 07/13] media: tvp5150: add FORMAT_TRY support for get/set selection handlers Marco Felsch
2019-05-06 13:36   ` Jacopo Mondi
2019-08-09  5:33     ` Marco Felsch
2019-05-14 18:48   ` Mauro Carvalho Chehab
2019-08-09  5:34     ` Marco Felsch
2019-04-15 12:44 ` [PATCH v6 08/13] media: tvp5150: initialize subdev before parsing device tree Marco Felsch
2019-05-14 20:20   ` Mauro Carvalho Chehab
2019-08-09  5:42     ` Marco Felsch
2019-04-15 12:44 ` [PATCH v6 09/13] media: tvp5150: add s_power callback Marco Felsch
2019-05-14 20:13   ` Mauro Carvalho Chehab
2019-08-09  5:39     ` Marco Felsch
2019-04-15 12:44 ` [PATCH v6 10/13] media: dt-bindings: tvp5150: cleanup bindings stlye Marco Felsch
2019-04-15 12:44 ` [PATCH v6 11/13] media: dt-bindings: tvp5150: add optional tvnorms documentation Marco Felsch
2019-04-15 12:44 ` [PATCH v6 12/13] media: tvp5150: add support to limit tv norms on connector Marco Felsch
2019-05-16 18:07   ` Laurent Pinchart
2019-08-13  9:10     ` Marco Felsch
2019-08-15 12:53       ` Laurent Pinchart
2019-08-15 13:26         ` Marco Felsch
2019-04-15 12:44 ` [PATCH v6 13/13] media: tvp5150: make debug output more readable Marco Felsch
2019-05-06 13:39   ` Jacopo Mondi
2019-05-14 20:18     ` Mauro Carvalho Chehab
2019-08-09  5:42       ` Marco Felsch
2019-05-06  5:47 ` [PATCH v6 00/13] TVP5150 new features Marco Felsch
2019-05-14 17:18   ` Mauro Carvalho Chehab
2019-05-14 20:20     ` Mauro Carvalho Chehab
2019-05-14 20:58       ` Marco Felsch
2019-05-14 23:41         ` Mauro Carvalho Chehab

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190815123810.GC13823@pendragon.ideasonboard.com \
    --to=laurent.pinchart@ideasonboard.com \
    --cc=devicetree@vger.kernel.org \
    --cc=hans.verkuil@cisco.com \
    --cc=jacopo+renesas@jmondi.org \
    --cc=jacopo@jmondi.org \
    --cc=kernel@pengutronix.de \
    --cc=linux-media@vger.kernel.org \
    --cc=m.felsch@pengutronix.de \
    --cc=mchehab@kernel.org \
    --cc=robh+dt@kernel.org \
    --cc=sakari.ailus@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org linux-media@archiver.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/ public-inbox