From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from smtp-vbr5.xs4all.nl ([194.109.24.25]:4627 "EHLO smtp-vbr5.xs4all.nl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754522AbZELHD1 (ORCPT ); Tue, 12 May 2009 03:03:27 -0400 From: Hans Verkuil To: Eduardo Valentin Subject: Re: [PATCH v2 0/7] [RFC] FM Transmitter (si4713) and another changes Date: Tue, 12 May 2009 09:03:18 +0200 Cc: "linux-media@vger.kernel.org" , "Nurkkala Eero.An (EXT-Offcode/Oulu)" References: <1242034309-13448-1-git-send-email-eduardo.valentin@nokia.com> In-Reply-To: <1242034309-13448-1-git-send-email-eduardo.valentin@nokia.com> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200905120903.18775.hverkuil@xs4all.nl> Sender: linux-media-owner@vger.kernel.org List-ID: On Monday 11 May 2009 11:31:42 Eduardo Valentin wrote: > Hello all, > > It took a few but I'm resending the FM transmitter driver again. > Sorry for this delay, but I had another things to give attention. > > Anyway, after reading the API and re-writing the code I came up > with the following 7 patches. Three of them are in the v4l2 API. > The other 4 are for the si4713 device. > > It is because of the first 3 patches that I'm sending this as a RFC. > > The first and second patches, as suggested before, are creating another > v4l2 extended controls class, the V4L2_CTRL_CLASS_FMTX. At this > first interaction, I've put all si4713 device extra properties there. > But I think that some of the can be moved to private class > (V4L2_CID_PRIVATE_BASE). That's the case of the region related things. > Comments are wellcome. > > The third patch came *maybe* because I've misunderstood something. But > I realized that the v4l2-subdev helper functions for I2C devices assumes > that the bridge device will create an I2C adaptor. And in that case, only > I2C address and its type are suffient. But in this case, makes no sense > to me to create an adaptor for the si4713 platform device driver. This is > the case where the device (si4713) is connected to an existing adaptor. > That's why I've realized that currently there is no way to pass I2C board > info using the current v4l2 I2C helper functions. Other info like irq > line and platform data are not passed to subdevices. So, that's why I've > created that patch. I've made several changes to the v4l2-subdev helpers: you now pass the i2c adapter directly. I've also fixed the unregister code to properly unregister any i2c client so you can safely rmmod and modprobe the i2c module. What sort of platform data do you need to pass to the i2c driver? I have yet to see a valid use case for this that can't be handled in a different way. Remember that devices like this are not limited to fixed platforms, but can also be used in USB or PCI devices. Regards, Hans > The remaining patches are the si4713 device driver itself. As suggested, > I've splited the driver into i2c driver and v4l2 radio driver. The first > one is exporting it self as a v4l2 subdev as well. Now it is composed by > the si4713.c and si4713-subdev.c. But in the future versions I think I'll > merge both and remove the si4713.c (by reducing lots of things), because > it was mainly designed to be used by the sysfs interface. I've also > keeped the sysfs interface (besides the extended control interface). The > v4l2 radio driver became a platform driver which is mainly a wrapper to > the I2C subdevice. Again here I've found some problem with the device > remove. Because, as the I2C helper function assumes the bridge device > will create an adaptor, then when the bridge removes the adaptor, its > devices will be removed as well. So, when re-inserting the driver, > registration will be good. However, if we use an existing adaptor, then > we need to remove the i2c client manually. Otherwise it will fail when > re-inserting the device. > > As I said before, comments are wellcome. I'm mostly to be > misunderstanding something from the API. > > BR, > > Eduardo Valentin (7): > v4l2: video device: Add V4L2_CTRL_CLASS_FMTX controls > v4l2: video device: Add FMTX controls default configurations > v4l2_subdev i2c: Add i2c board info to v4l2_i2c_new_subdev > FMTx: si4713: Add files to handle si4713 i2c device > FMTx: si4713: Add files to add radio interface for si4713 > FMTx: si4713: Add Kconfig and Makefile entries > FMTx: si4713: Add document file > > Documentation/video4linux/si4713.txt | 132 ++ > drivers/media/radio/Kconfig | 22 + > drivers/media/radio/Makefile | 3 + > drivers/media/radio/radio-si4713.c | 345 ++++++ > drivers/media/radio/radio-si4713.h | 48 + > drivers/media/radio/si4713-subdev.c | 1045 ++++++++++++++++ > drivers/media/radio/si4713.c | 2250 > ++++++++++++++++++++++++++++++++++ drivers/media/radio/si4713.h | > 295 +++++ > drivers/media/video/v4l2-common.c | 99 ++- > include/linux/videodev2.h | 45 + > include/media/v4l2-common.h | 6 + > 11 files changed, 4284 insertions(+), 6 deletions(-) > create mode 100644 Documentation/video4linux/si4713.txt > create mode 100644 drivers/media/radio/radio-si4713.c > create mode 100644 drivers/media/radio/radio-si4713.h > create mode 100644 drivers/media/radio/si4713-subdev.c > create mode 100644 drivers/media/radio/si4713.c > create mode 100644 drivers/media/radio/si4713.h > > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom