* Re: [PATCH v2 0/7] [RFC] FM Transmitter (si4713) and another changes
@ 2009-05-12 7:50 Hans Verkuil
2009-05-12 7:55 ` Eduardo Valentin
0 siblings, 1 reply; 7+ messages in thread
From: Hans Verkuil @ 2009-05-12 7:50 UTC (permalink / raw)
To: eduardo.valentin; +Cc: linux-media, Nurkkala Eero.An
> On Tue, May 12, 2009 at 09:03:18AM +0200, ext Hans Verkuil wrote:
>> 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.
>
> Right. I will check those.
>
>>
>> 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.
>
> Yes, sure. Well, a simple "set_power" procedure is an example of things
> that
> I see as platform specific. Which may be passed by platform data
> structures.
> In some platform a set power / reset line may be done by a simple gpio.
> but
> in others it may be a different procedure.
The v4l2_device struct has a notify callback: you can use that one
instead. That way the subdev driver doesn't have to have any knowledge
about the platform it is used in.
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 0/7] [RFC] FM Transmitter (si4713) and another changes
2009-05-12 7:50 [PATCH v2 0/7] [RFC] FM Transmitter (si4713) and another changes Hans Verkuil
@ 2009-05-12 7:55 ` Eduardo Valentin
2009-05-13 5:59 ` Eduardo Valentin
0 siblings, 1 reply; 7+ messages in thread
From: Eduardo Valentin @ 2009-05-12 7:55 UTC (permalink / raw)
To: ext Hans Verkuil
Cc: Valentin Eduardo (Nokia-D/Helsinki),
linux-media, Nurkkala Eero.An (EXT-Offcode/Oulu)
On Tue, May 12, 2009 at 09:50:22AM +0200, ext Hans Verkuil wrote:
>
> > On Tue, May 12, 2009 at 09:03:18AM +0200, ext Hans Verkuil wrote:
> >> 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.
> >
> > Right. I will check those.
> >
> >>
> >> 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.
> >
> > Yes, sure. Well, a simple "set_power" procedure is an example of things
> > that
> > I see as platform specific. Which may be passed by platform data
> > structures.
> > In some platform a set power / reset line may be done by a simple gpio.
> > but
> > in others it may be a different procedure.
>
> The v4l2_device struct has a notify callback: you can use that one
> instead. That way the subdev driver doesn't have to have any knowledge
> about the platform it is used in.
Right. I see. But in that case the brigde driver would be bound to
a board specific? For instance of this very driver, I wrote a platform
driver just to make its radio interface. But I don't think it is a good
idea to bound the platform driver to a board specific creating a notification
handler just to set the power of the i2c chip. I see this procedure as a board
specific thing. As well as the IRQ line for instance. That configuration is
also board specific. Which is usually passed using i2c board info. Correct me
if I'm wrong.
>
> Regards,
>
> Hans
>
> --
> Hans Verkuil - video4linux developer - sponsored by TANDBERG
--
Eduardo Valentin
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 0/7] [RFC] FM Transmitter (si4713) and another changes
2009-05-12 7:55 ` Eduardo Valentin
@ 2009-05-13 5:59 ` Eduardo Valentin
0 siblings, 0 replies; 7+ messages in thread
From: Eduardo Valentin @ 2009-05-13 5:59 UTC (permalink / raw)
To: Valentin Eduardo (Nokia-D/Helsinki)
Cc: ext Hans Verkuil, linux-media, Nurkkala Eero.An (EXT-Offcode/Oulu)
On Tue, May 12, 2009 at 09:55:19AM +0200, Valentin Eduardo (Nokia-D/Helsinki) wrote:
> > >>
> > >> 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.
> > >
> > > Yes, sure. Well, a simple "set_power" procedure is an example of things
> > > that
> > > I see as platform specific. Which may be passed by platform data
> > > structures.
> > > In some platform a set power / reset line may be done by a simple gpio.
> > > but
> > > in others it may be a different procedure.
> >
> > The v4l2_device struct has a notify callback: you can use that one
> > instead. That way the subdev driver doesn't have to have any knowledge
> > about the platform it is used in.
>
> Right. I see. But in that case the brigde driver would be bound to
> a board specific? For instance of this very driver, I wrote a platform
> driver just to make its radio interface. But I don't think it is a good
> idea to bound the platform driver to a board specific creating a notification
> handler just to set the power of the i2c chip. I see this procedure as a board
> specific thing. As well as the IRQ line for instance. That configuration is
> also board specific. Which is usually passed using i2c board info. Correct me
> if I'm wrong.
Even though I still can pass board specific to the platform driver using
its platform data and then use the notify callback of v4l2_device, I still
will miss the configuration for IRQ line. I don't see how to pass that to
the i2c subdev by using the current helper functions.
>
> >
> > Regards,
> >
> > Hans
> >
> > --
> > Hans Verkuil - video4linux developer - sponsored by TANDBERG
>
> --
> Eduardo Valentin
--
Eduardo Valentin
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 0/7] [RFC] FM Transmitter (si4713) and another changes
@ 2009-05-11 9:31 Eduardo Valentin
2009-05-12 7:03 ` Hans Verkuil
0 siblings, 1 reply; 7+ messages in thread
From: Eduardo Valentin @ 2009-05-11 9:31 UTC (permalink / raw)
To: Hans Verkuil
Cc: linux-media, Nurkkala Eero.An (EXT-Offcode/Oulu), Eduardo Valentin
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.
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 0/7] [RFC] FM Transmitter (si4713) and another changes
2009-05-11 9:31 Eduardo Valentin
@ 2009-05-12 7:03 ` Hans Verkuil
2009-05-12 7:33 ` Eduardo Valentin
0 siblings, 1 reply; 7+ messages in thread
From: Hans Verkuil @ 2009-05-12 7:03 UTC (permalink / raw)
To: Eduardo Valentin; +Cc: linux-media, Nurkkala Eero.An (EXT-Offcode/Oulu)
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 0/7] [RFC] FM Transmitter (si4713) and another changes
2009-05-12 7:03 ` Hans Verkuil
@ 2009-05-12 7:33 ` Eduardo Valentin
2009-05-12 7:35 ` Eduardo Valentin
0 siblings, 1 reply; 7+ messages in thread
From: Eduardo Valentin @ 2009-05-12 7:33 UTC (permalink / raw)
To: ext Hans Verkuil
Cc: Valentin Eduardo (Nokia-D/Helsinki),
linux-media, Nurkkala Eero.An (EXT-Offcode/Oulu)
On Tue, May 12, 2009 at 09:03:18AM +0200, ext Hans Verkuil wrote:
> 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.
Right. I will check those.
>
> 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.
Yes, sure. Well, a simple "set_power" procedure is an example of things that
I see as platform specific. Which may be passed by platform data structures.
In some platform a set power / reset line may be done by a simple gpio. but
in others it may be a different procedure.
>
> 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
--
Eduardo Valentin
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 0/7] [RFC] FM Transmitter (si4713) and another changes
2009-05-12 7:33 ` Eduardo Valentin
@ 2009-05-12 7:35 ` Eduardo Valentin
0 siblings, 0 replies; 7+ messages in thread
From: Eduardo Valentin @ 2009-05-12 7:35 UTC (permalink / raw)
To: Valentin Eduardo (Nokia-D/Helsinki)
Cc: ext Hans Verkuil, linux-media, Nurkkala Eero.An (EXT-Offcode/Oulu)
On Tue, May 12, 2009 at 09:33:31AM +0200, Valentin Eduardo (Nokia-D/Helsinki) wrote:
> On Tue, May 12, 2009 at 09:03:18AM +0200, ext Hans Verkuil wrote:
> > 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.
>
> Right. I will check those.
>
> >
> > 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.
>
> Yes, sure. Well, a simple "set_power" procedure is an example of things that
> I see as platform specific. Which may be passed by platform data structures.
> In some platform a set power / reset line may be done by a simple gpio. but
> in others it may be a different procedure.
Also, the IRQ line value can also be passed to device driver using the i2c
board info. That's another way of use of this i2c board info.
>
> >
> > 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
>
> --
> Eduardo Valentin
--
Eduardo Valentin
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2009-05-13 6:04 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-05-12 7:50 [PATCH v2 0/7] [RFC] FM Transmitter (si4713) and another changes Hans Verkuil
2009-05-12 7:55 ` Eduardo Valentin
2009-05-13 5:59 ` Eduardo Valentin
-- strict thread matches above, loose matches on Subject: below --
2009-05-11 9:31 Eduardo Valentin
2009-05-12 7:03 ` Hans Verkuil
2009-05-12 7:33 ` Eduardo Valentin
2009-05-12 7:35 ` Eduardo Valentin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.