Re: [PATCHv4 00/25] dvb core: add basic support for the media controller

[Mauro Carvalho Chehab <mchehab@osg.samsung.com>]

Em Sat, 14 Feb 2015 10:32:21 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 02/13/2015 11:57 PM, Mauro Carvalho Chehab wrote:
> > This patch series adds basic support for the media controller at the
> > DVB core: it creates one media entity per DVB devnode, if the media
> > device is passed as an argument to the DVB structures.
> > 
> > The cx231xx driver was modified to pass such argument for DVB NET,
> > DVB frontend and DVB demux.
> > 
> > -
> > 
> > version 4:
> > 
> > - Addressed the issues pointed via e-mail
> 
> No, you didn't. Especially with regards to the alsa node definition. I'm
> pretty sure you need at least the subdevice information which is now removed.

Well, back on Jan, 26 I answered your issues about that at:
	http://www.spinics.net/lists/linux-media/msg85857.html

As you didn't reply back in a reasonable amount of time, I assumed that
you're happy with that.

In any case, the definitions are still there, as nothing got dropped
from the external header.

So, when ALSA media controller support will be added at the Kernel, we
can decide if it will use major/minor or card/device/subdevice or both.

As I said back in Jan, 26, IMO, the best would be to use both:

struct media_entity_desc {
	...
	union {
		struct {
			u32 major;
			u32 minor;
		} dev;
		/* deprecated fields */
		...
	}
	union {
		struct {
			u32 card;
			u32 device;
			u32 subdevice;
		} alsa_props;
		__u8 raw[172];
	}
}

(additional and deprecated fields removed just to simplify its
 representation above)

Even for ALSA, it is a way easier for libmediactl.c to keep using
major/minor to get the device node name via both udev/sysfs than
using anything else, as I don't think that udev has any method to
find the associated name without major,minor information. Ok, there
are indirect methods using the ALSA API to get such association, but
it is just easier to fill everything at the struct than to add the
extra complexity for the media control clients to convert between
major/minor into card/device/subdevice.

What I'm saying is that the card/device/subdevice really seems to be
an extra property for this specific type of devnode, and not a
replacement.

In any case, I think we should take the decision on how to properly
map the ALSA specific bits when we merge ALSA media controller patches,
and not before.

> I also do *not* like the fact that you posted a v4 and immediately applied
> these patches to the master without leaving any time for more discussions.
> 
> These patches change the kernel API and need to go to proper review and need
> a bunch of Acks, Laurent's at the very minimum since he's MC maintainer.
>
> Please revert the whole patch series from master, then we can discuss this
> more.
> 
> For the record, for patch 02/25:
> 
> Nacked-by: Hans Verkuil <hans.verkuil@cisco.com>
> 
> I do *not* agree with this API change.
> 
> We can discuss this more on Monday.

This hole series is for discussions for a long time (since the beginning of
January), without rejection, and its now starting to receive patches from
other authors. Keeping it OOT just makes harder to discuss and for people to
test. It is time to move on.

As I said on IRC, as I opted to merge it for 3.21, we'll have 2 entire
Kernel cycles to make it mature before being merged upstream. During that
period, we can fix any issues on it.

Regards,
Mauro