linux-kernel - Re: [PATCH RFC v2] radio-sf16fmr2: convert to generic TEA575x interface

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <201105172133.14835.hverkuil@xs4all.nl>
Date:	Tue, 17 May 2011 21:33:14 +0200
From:	Hans Verkuil <hverkuil@...all.nl>
To:	Ondrej Zary <linux@...nbow-software.org>
Cc:	linux-media@...r.kernel.org, alsa-devel@...a-project.org,
	"Kernel development list" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC v2] radio-sf16fmr2: convert to generic TEA575x interface

Hi Ondrej!

On Sunday, May 15, 2011 23:26:33 Hans Verkuil wrote:
> On Sunday, May 15, 2011 22:18:21 Ondrej Zary wrote:
> > Thanks, it's much simpler with the new control framework.
> > Do the negative volume control values make sense? The TC9154A chip can
> > attenuate the volume from 0 to -68dB in 2dB steps.
> 
> It does make sense, but I think I would offset the values so they start at 0.
> Mostly because there might be some old apps that set the volume to 0 when they
> want to mute, which in this case is full volume.
> 
> I am not aware of any driver where a volume of 0 isn't the same as the lowest
> volume possible, so in this particular case I would apply an offset.
> 
> I will have to do a closer review tomorrow or the day after. I think there are
> a few subtleties that I need to look at. Ping me if you haven't heard from me
> by Wednesday. I would really like to get these drivers up to spec now that I
> have someone who can test them, and once that's done I hope that I never have
> to look at them again :-) (Unlikely, but one can dream...)

OK, I looked at it a bit more and it needs to be changed a little bit. The
problem is that the VOLUME control is added after snd_tea575x_init, i.e.
after the video_register_device call. The video_register_device call should
be the last thing done before the init sequence returns. There may be applications
(dbus/hal) that open devices as soon as they appear, so doing more initialization
after the video node is registered is not a good idea (many older V4L drivers
make this mistake).

Perhaps creating a snd_tea575x_register function doing just the registration
may be a good idea. Or a callback before doing the video_register_device.

Another thing: the tea->mute field shouldn't be needed anymore. And the
'mute on init' bit in snd_tea575x_init can be removed as well since that
is automatically performed by v4l2_ctrl_handler_setup.

In addition, the .ioctl field in tea575x_fops can be replaced by .unlocked_ioctl.
The whole exclusive open stuff and the in_use field can be removed. The only
thing needed is a struct mutex in struct snd_tea575x, initialize it and set
tea575x_radio_inst->lock to the mutex. This will serialize all access safely.

To do this really right you should add struct v4l2_device to struct snd_tea575x
(the radio-sf16fmr2 driver has one, so you can use that as an example). With
that in place you can also add support for 'priority' handling. I'd say see
what you can do, and if it takes too much time then mail me the tea575x code
and the radio-sf16frm2 code and I'll finish it.

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/