Hello Hans, thank you for the comments.
On Fri, 2010-04-23 at 11:16 +0200, ext Hans Verkuil wrote: > On Tuesday 20 April 2010 17:20:04 Matti J. Aaltonen wrote: > > Hi. > > > > This is the initial version of my driver for Texas Instruments > > WL1273 FM receiver transmitter. The driver is divided into three parts: > > the MFD core which handles the communication with the chip and also > > keeps the chip state, ASoC codec takes care of the digital audio part and > > the V4L2 control part with some private IOCTLs. > > > > This is my first up-streaming effort so all comments are welcome. > > OK, I did a quick review and the main things that you need to look at are > the RDS receiver API as defined in the spec > (http://www.linuxtv.org/downloads/v4l-dvb-apis/ch04s11.html) and the FM and > RDS transmitter controls: > http://www.linuxtv.org/downloads/v4l-dvb-apis/ch01s09.html#fm-tx-controls. OK, I'll do that. > > Any private controls that you think you need should be discussed first. We > may need to standardize them. OK, I didn't realize that. But it makes sense, so let's discuss... > The other thing you have to do in the V4L2 driver is to use struct > v4l2_device. > See also Documentation/video4linux/v4l2-framework.txt. OK. > I also noticed some FM and RDS things in the alsa driver. It is not clear to > me why these are there since this is pretty much V4L2 specific. Yes, I kind of new that those controls weren't a good idea. But I implemented those when I didn't know better. Ande I left them there to see if they would get accepted. But I'll remove them as the functionality is duplicated in the V4L2 module. > Regarding hardcoding regions: isn't this more for the application? Are there > any legal requirements for region handling? I thought about that a lot. And I know that the regions are a policy thing that doesn't belong into the driver/kernel. But those two regions are directly supported by the hardware so I thought that it would be OK make them available. > Most radio tuners just accept the whole frequency range that they support and > leave it to the application to restrict it if needed depending on the region. I understand... see above... > Those disabled controls like bass, treble etc. should be removed. Workarounds > for plainly broken applications is not something we want in our drivers. > Instead make a patch for that app and send it to the maintainer. If it is > unmaintained, then let us know: we can move unmaintained but frequently used > apps to our own repository. Fine... I did that just to copy the functionality of some existing driver when I started. B.R. Matti > > Regards, > > Hans > > > > > Cheers, > > Matti > > > > Matti J. Aaltonen (3): > > MFD: WL1273 FM Radio: MFD driver for the FM radio. > > ASoC: WL1273 FM Radio: Digital audio codec. > > V4L2: WL1273 FM Radio: Controls for the FM radio. > > > > drivers/media/radio/Kconfig | 15 + > > drivers/media/radio/Makefile | 1 + > > drivers/media/radio/radio-wl1273.c | 805 ++++++++++++++++ > > drivers/mfd/Kconfig | 6 + > > drivers/mfd/Makefile | 2 + > > drivers/mfd/wl1273-core.c | 1825 > > ++++++++++++++++++++++++++++++++++++ > > include/linux/mfd/wl1273-core.h | 265 ++++++ > > sound/soc/codecs/Kconfig | 6 + > > sound/soc/codecs/Makefile | 2 + > > sound/soc/codecs/wl1273.c | 708 ++++++++++++++ > > sound/soc/codecs/wl1273.h | 49 + > > 11 files changed, 3684 insertions(+), 0 deletions(-) > > create mode 100644 drivers/media/radio/radio-wl1273.c > > create mode 100644 drivers/mfd/wl1273-core.c > > create mode 100644 include/linux/mfd/wl1273-core.h > > create mode 100644 sound/soc/codecs/wl1273.c > > create mode 100644 sound/soc/codecs/wl1273.h > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-media" in > > the body of a message to majord...@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
