Hi Mauro,
Thank you for the patch.
Could you please CC me on patches touching the media controller core ? I can
send a MAINTAINERS patch to make sure that gets handled automatically.
On Wednesday, 1 August 2018 18:55:04 EEST Mauro Carvalho Chehab wrote:
> Consumer devices are provided with a wide diferent range of types
> supported by the same driver, allowing different configutations.
>
> In order to make easier to setup media controller links, "taint"
> pads with the signal type it carries.
>
> While here, get rid of DEMOD_PAD_VBI_OUT, as the signal it carries
> is actually the same as the normal video output.
>
> The difference happens at the video/VBI interface:
> - for VBI, only the hidden lines are streamed;
> - for video, the stream is usually cropped to hide the
> vbi lines.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@xxxxxxxxxx>
> ---
> drivers/media/dvb-frontends/au8522_decoder.c | 3 ++
> drivers/media/i2c/msp3400-driver.c | 2 ++
> drivers/media/i2c/saa7115.c | 2 ++
> drivers/media/i2c/tvp5150.c | 2 ++
> drivers/media/pci/saa7134/saa7134-core.c | 2 ++
> drivers/media/tuners/si2157.c | 3 ++
> drivers/media/usb/dvb-usb-v2/mxl111sf.c | 2 ++
> drivers/media/v4l2-core/tuner-core.c | 5 +++
> include/media/media-entity.h | 35 ++++++++++++++++++++
> 9 files changed, 56 insertions(+)
[snip]
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index 3aa3d58d1d58..8bfbe6b59fa9 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -155,6 +155,40 @@ struct media_link {
> bool is_backlink;
> };
>
> +/**
> + * struct media_pad_signal_type - type of the signal inside a media pad
I'd say "carried by a media pad" instead of "inside a media pad".
> + *
> + * @PAD_SIGNAL_DEFAULT
Shouldn't we use a MEDIA_PAD_ prefix ?
> + * Default signal. Use this when all inputs or all outputs are
> + * uniquely identified by the pad number.
How about "Use this when the pad can carry a single signal type" ? I
understand your formulation as meaning that all pads of the entity have to be
of the default type, or none can be.
> + * @PAD_SIGNAL_ANALOG
This isn't a very good name given that PAD_SIGNAL_TV_CARRIERS is also analog.
> + * The pad contains an analog signa. It can be Radio Frequency,
s/signa/signal/
> + * Intermediate Frequency or baseband signal.
> + * Tuner inputs, composite and s-video signals should use it.
> + * On tuner sources, this is used for digital TV demodulators and for
> + * IF-PLL demodulator like tda9887.
> + * @PAD_SIGNAL_TV_CARRIERS
> + * The pad contains analog signals carrying either a digital or an analog
> + * modulated (or baseband) signal.
As above, maybe "The pad carries either ...".
> This is provided by tuner source
> + * pads and used by analog TV standard decoders and by digital TV demods.
> + * @PAD_SIGNAL_DV
> + * Contains a digital video signal, with can be a bitstream of samples
> + * taken from an analog TV video source. On such case, it usually
> + * contains the VBI data on it.
> + * @PAD_SIGNAL_AUDIO
> + * Contains an Intermediate Frequency analog signal from an audio
> + * sub-carrier or an audio bitstream. IF signals are provided by tuners
> + * and consumed by audio AM/FM decoders. Bitstream audio is provided by
s/ / /
> + * an audio decoder.
Generally speaking the types you propose here seem quite ad-hoc, without much
coherency. For instance you split analog and digital video, but group all
audio under a single type. It's also not very clear from the description how
to handle analog video, as it could match both PAD_SIGNAL_ANALOG and
PAD_SIGNAL_TV_CARRIERS. Both of those types also accept analog baseband
signals. I think this should be reworked, it doesn't sound very usable except
for the specific use case that this series tries to address.
> + */
> +enum media_pad_signal_type {
> + PAD_SIGNAL_DEFAULT = 0,
> + PAD_SIGNAL_ANALOG,
> + PAD_SIGNAL_TV_CARRIERS,
> + PAD_SIGNAL_DV,
> + PAD_SIGNAL_AUDIO,
> +};
> +
> /**
> * struct media_pad - A media pad graph object.
> *
> @@ -169,6 +203,7 @@ struct media_pad {
> struct media_gobj graph_obj; /* must be first field in struct */
> struct media_entity *entity;
> u16 index;
> + enum media_pad_signal_type sig_type;
Missing kerneldoc and Documentation/ update ? It's important to document the
use cases, and in particular whether the type should be static or can vary (as
in whether a pad can carry different types over time).
> unsigned long flags;
> };
--
Regards,
Laurent Pinchart
