Re: About the radio-si470x driver for I2C interface

2009-04-13 Thread Joonyoung Shim 
On 4/14/2009 3:35 AM, Tobias Lorenz wrote:
> Hi Joonyoung,
> 
>> I have some problem. There is codes for usb in radio-si470x-common.c file.
> 
>> Hrm, if it cannot be removed, maybe it seems to seperate using ifdef.
> 
>> What do you think about this?
> 
> I moved some more functions from radio-si470x-common.c file to
> radio-si470x-usb.c:
> 
> - si470x_start
> 
> - si470x_stop
> 
> - si470x_fops_open
> 
> - si470x_fops_release
> 
> Now the common file should be free of any USB specific components.
> 
> Please have a look at
> 
> http://linuxtv.org/hg/~tlorenz/v4l-dvb/
> 
> Maybe we can move something back later for optimization. But for now, it
> should be okay.
> 
> Bye,
> 
> Toby
> 

Hi Tobias,

I have some questions.

The i2c device is connected always unlike the usb device, so it doesn't
use disconnected and disconnect_lock of struct si470x_device, these are
usb specific.

Unlike you say, si470x_start and si470x_stop functions exist yet in
radio-si470x-common.c. 

I know from datasheet that software should wait for the powerup time(110ms)
after power up, and i verified it at the si4709 device using i2c interface,
but there is not at your implementation.

I wonder above things, and i send you the patch to fix make warning and build
errors, and for easy work.

=
Fix compile warnings and errors of radio-si470x-i2c

Signed-off-by: Joonyoung Shim 
---
 linux/drivers/media/radio/si470x/Kconfig   |2 +-
 linux/drivers/media/radio/si470x/Makefile  |4 +-
 .../drivers/media/radio/si470x/radio-si470x-i2c.c  |  153 +---
 linux/drivers/media/radio/si470x/radio-si470x.h|5 +-
 4 files changed, 74 insertions(+), 90 deletions(-)

diff --git a/linux/drivers/media/radio/si470x/Kconfig 
b/linux/drivers/media/radio/si470x/Kconfig
index 3172e1a..07ac2d3 100644
--- a/linux/drivers/media/radio/si470x/Kconfig
+++ b/linux/drivers/media/radio/si470x/Kconfig
@@ -34,4 +34,4 @@ config I2C_SI470X
  computer's I2C port.
 
  To compile this driver as a module, choose M here: the
- module will be called radio-si470x-i2c.
+ module will be called radio-i2c-si470x.
diff --git a/linux/drivers/media/radio/si470x/Makefile 
b/linux/drivers/media/radio/si470x/Makefile
index 945e7b1..a4bba94 100644
--- a/linux/drivers/media/radio/si470x/Makefile
+++ b/linux/drivers/media/radio/si470x/Makefile
@@ -3,7 +3,7 @@
 #
 
 radio-si470x-objs  := radio-si470x-usb.o radio-si470x-common.o
-radio-si470x-i2c-objs  := radio-si470x-i2c.o radio-si470x-common.o
+radio-i2c-si470x-objs  := radio-si470x-i2c.o radio-si470x-common.o
 
 obj-$(CONFIG_USB_SI470X) += radio-si470x.o
-obj-$(CONFIG_I2C_SI470X) += radio-si470x-i2c.o
+obj-$(CONFIG_I2C_SI470X) += radio-i2c-si470x.o
diff --git a/linux/drivers/media/radio/si470x/radio-si470x-i2c.c 
b/linux/drivers/media/radio/si470x/radio-si470x-i2c.c
index 7058b84..27a0691 100644
--- a/linux/drivers/media/radio/si470x/radio-si470x-i2c.c
+++ b/linux/drivers/media/radio/si470x/radio-si470x-i2c.c
@@ -57,14 +57,9 @@ MODULE_PARM_DESC(radio_nr, "Radio Nr");
  */
 int si470x_get_register(struct si470x_device *radio, int regnr)
 {
-   int retval;
+   /* TODO */
 
-   retval = 0; /* TODO */
-
-   if (retval >= 0)
-   radio->registers[regnr] = 0; /* TODO */
-
-   return (retval < 0) ? -EINVAL : 0;
+   return 0;
 }
 
 
@@ -73,13 +68,9 @@ int si470x_get_register(struct si470x_device *radio, int 
regnr)
  */
 int si470x_set_register(struct si470x_device *radio, int regnr)
 {
-   int retval;
-
-   radio->registers[regnr] = 0; /* TODO */
+   /* TODO */
 
-   retval = 0; /* TODO */
-
-   return (retval < 0) ? -EINVAL : 0;
+   return 0;
 }
 
 
@@ -88,16 +79,9 @@ int si470x_set_register(struct si470x_device *radio, int 
regnr)
  */
 int si470x_get_all_registers(struct si470x_device *radio)
 {
-   int retval;
-   unsigned char regnr;
-
-   retval = 0; /* TODO */
+   /* TODO */
 
-   if (retval >= 0)
-   for (regnr = 0; regnr < RADIO_REGISTER_NUM; regnr++)
-   radio->registers[regnr] = 0; /* TODO */
-
-   return (retval < 0) ? -EINVAL : 0;
+   return 0;
 }
 
 
@@ -106,17 +90,9 @@ int si470x_get_all_registers(struct si470x_device *radio)
  */
 int si470x_get_rds_registers(struct si470x_device *radio)
 {
-   int retval;
-   int size;
-   unsigned char regnr;
-
-   retval = /* TODO */
+   /* TODO */
 
-   if (retval >= 0)
-   for (regnr = 0; regnr < RDS_REGISTER_NUM; regnr++)
-   radio->registers[STATUSRSSI + regnr] = 0; /* TODO */
-
-   return (retval < 0) ? -EINVAL : 0;
+   return 0;
 }
 
 
@@ -131,26 +107,15 @@ int si470x_get_rds_registers(struct si470x_device *radio)
 int si470x_fops_open(struct file *file)
 {
struct si470x_device *radio = video_drvdata(file);
-   int retval;
+   int retval = 0

Re: About the radio-si470x driver for I2C interface

2009-04-13 Thread Hans Verkuil 
On Monday 13 April 2009 03:14:13 Alexey Klimov wrote:
> Hello, Tobias
>
> On Mon, Apr 13, 2009 at 12:56 AM, Tobias Lorenz  
wrote:
> > Hi Joonyoung,
> >
> > Hi Alexey,
> >
> > I've split the driver into a couple of segments:
> >
> > - radio-si470x-common.c is for common functions
> >
> > - radio-si470x-usb.c are the usb support functions
> >
> > - radio-si470x-i2c.c is an untested prototyped file for your i2c
> > support functions
> >
> > - radio-si470x.h is a header file with everything required by the
> > c-files
> >
> > I hope this is a basis we can start on with i2c support. What do you
> > think?
> >
> > The URL is:
> >
> > http://linuxtv.org/hg/~tlorenz/v4l-dvb
> >
> > Bye,
> >
> > Toby
>
> Great! It's always interesting to see big changes.
> I understand i2c interface not so good and have only general questions.
>
> Many (most?) drivers in v4l tree were converted to use new v4l2-
> framework. For example, dsbr100 was converted to v4l2_device
> http://linuxtv.org/hg/v4l-dvb/rev/77f37ad5dd0c and em28xx was
> converted to v4l2_subdev
> http://linuxtv.org/hg/v4l-dvb/rev/00525b115901
> As i remember, Hans Verkuil said that all v4l drivers should be
> converted to new framework. Is it time to switch to this new interface
> ? Probably, there are a lot of code examples in current tree that can
> help..

Any new v4l2 i2c driver must use v4l2_subdev in order to be reusable by v4l2 
drivers. All 'legacy' i2c drivers are now converted and only soc-camera and 
v4l2-int-device.h based drivers remain unconverted, although I expect that 
those will also be moved to v4l2_subdev in 2.6.31.

What complicates matters here is that the si470x is a radio tuner, and as 
such should be implemented as a tuner device (common/tuners) which have 
their own framework. But if memory serves the si470x can also do RDS for 
which there is no support in the tuner framework.

I'm leaning towards implementing this i2c driver as a normal v4l2_subdev 
driver, rather than making it part of the tuner driver/framework.

Actually, I suspect that the tuner driver/framework can be substantially 
simplified with this new framework: much of the complexity there is related 
to the autoprobing crap, and that no longer applies. This is an interesting 
future research topic :-)

> And second question. About lock/unlock_kernel in open functions.
> Please, take a look at
> http://www.spinics.net/lists/linux-media/msg04057.html
> Well, is it time to do something with this?

Yes, please remove/replace these calls.

Regards,

Hans

> Well, my questions about improving functionality, not about mistakes or
> bugs :)



-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG
--
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

Re: About the radio-si470x driver for I2C interface

2009-04-13 Thread Joonyoung Shim 
On 4/13/2009 7:31 PM, Joonyoung Shim wrote:
>> I'm not sure about the consequences in case of renaming the radio-si470x
>> module. But it would be consequent to add the appendix -usb and -i2c to
>> the current name.
>>
>> I applied the patch as follows:
> 
> Okay, your patch is better.
> Thanks.
> 
> I will post the i2c part soon after testing.

I have some problem. There is codes for usb in radio-si470x-common.c file.
Hrm, if it cannot be removed, maybe it seems to seperate using ifdef.
What do you think about this?

> --
> 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

Re: About the radio-si470x driver for I2C interface

2009-04-13 Thread Joonyoung Shim 
> I'm not sure about the consequences in case of renaming the radio-si470x
> module. But it would be consequent to add the appendix -usb and -i2c to
> the current name.
> 
> I applied the patch as follows:

Okay, your patch is better.
Thanks.

I will post the i2c part soon after testing.
--
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

Re: About the radio-si470x driver for I2C interface

2009-04-12 Thread Joonyoung Shim 
On 4/13/2009 10:46 AM, Joonyoung Shim wrote:
> On 4/13/2009 5:56 AM, Tobias Lorenz wrote:
>> Hi Joonyoung,
>>
>> Hi Alexey,
>>
>> I've split the driver into a couple of segments:
>>
>> - radio-si470x-common.c is for common functions
>>
>> - radio-si470x-usb.c are the usb support functions
>>
>> - radio-si470x-i2c.c is an untested prototyped file for your i2c support
>> functions
>>
>> - radio-si470x.h is a header file with everything required by the c-files
>>
>> I hope this is a basis we can start on with i2c support. What do you think?
>>
>> The URL is:
>>
>> http://linuxtv.org/hg/~tlorenz/v4l-dvb
> 
> It looks good, i will test with implementing the i2c functions.

I compiled getting your source from above URL, but i could not compile because
of supporting only usb compilation at Makefile.
I suggest to modify at Kconfig and Makefile like following patch.
What do you think?


diff -r 43d455adb02c linux/drivers/media/radio/Makefile
--- a/linux/drivers/media/radio/MakefileSun Apr 12 22:51:40 2009 +0200
+++ b/linux/drivers/media/radio/MakefileMon Apr 13 14:31:05 2009 +0900
@@ -17,7 +17,7 @@
 obj-$(CONFIG_RADIO_TRUST) += radio-trust.o
 obj-$(CONFIG_RADIO_MAESTRO) += radio-maestro.o
 obj-$(CONFIG_USB_DSBR) += dsbr100.o
-obj-$(CONFIG_USB_SI470X) += si470x/
+obj-$(CONFIG_RADIO_SI470X) += si470x/
 obj-$(CONFIG_USB_MR800) += radio-mr800.o
 obj-$(CONFIG_RADIO_TEA5764) += radio-tea5764.o
 
diff -r 43d455adb02c linux/drivers/media/radio/si470x/Kconfig
--- a/linux/drivers/media/radio/si470x/Kconfig  Sun Apr 12 22:51:40 2009 +0200
+++ b/linux/drivers/media/radio/si470x/Kconfig  Mon Apr 13 14:31:05 2009 +0900
@@ -1,6 +1,10 @@
+config RADIO_SI470X
+   tristate "Silicon Labs Si470x FM Radio Receiver support"
+   depends on VIDEO_V4L2
+
 config USB_SI470X
tristate "Silicon Labs Si470x FM Radio Receiver support with USB"
-   depends on USB && VIDEO_V4L2
+   depends on USB && RADIO_SI470X
---help---
  This is a driver for USB devices with the Silicon Labs SI470x
  chip. Currently these devices are known to work:
@@ -25,7 +29,7 @@
 
 config I2C_SI470X
tristate "Silicon Labs Si470x FM Radio Receiver support with I2C"
-   depends on I2C && VIDEO_V4L2
+   depends on I2C && RADIO_SI470X
---help---
  This is a driver for I2C devices with the Silicon Labs SI470x
  chip.
diff -r 43d455adb02c linux/drivers/media/radio/si470x/Makefile
--- a/linux/drivers/media/radio/si470x/Makefile Sun Apr 12 22:51:40 2009 +0200
+++ b/linux/drivers/media/radio/si470x/Makefile Mon Apr 13 14:31:05 2009 +0900
@@ -2,8 +2,8 @@
 # Makefile for radios with Silicon Labs Si470x FM Radio Receivers
 #
 
-radio-si470x-objs  := radio-si470x-usb.o radio-si470x-common.o
-radio-si470x-i2c-objs  := radio-si470x-i2c.o radio-si470x-common.o
+si470x-usb-objs:= radio-si470x-usb.o radio-si470x-common.o
+si470x-i2c-objs:= radio-si470x-i2c.o radio-si470x-common.o
 
-obj-$(CONFIG_USB_SI470X) += radio-si470x.o
-obj-$(CONFIG_I2C_SI470X) += radio-si470x-i2c.o
+obj-$(CONFIG_USB_SI470X) += si470x-usb.o
+obj-$(CONFIG_I2C_SI470X) += si470x-i2c.o


> 
> Thanks.
> --
> 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

Re: About the radio-si470x driver for I2C interface

2009-04-12 Thread Joonyoung Shim 
On 4/13/2009 5:56 AM, Tobias Lorenz wrote:
> Hi Joonyoung,
> 
> Hi Alexey,
> 
> I've split the driver into a couple of segments:
> 
> - radio-si470x-common.c is for common functions
> 
> - radio-si470x-usb.c are the usb support functions
> 
> - radio-si470x-i2c.c is an untested prototyped file for your i2c support
> functions
> 
> - radio-si470x.h is a header file with everything required by the c-files
> 
> I hope this is a basis we can start on with i2c support. What do you think?
> 
> The URL is:
> 
> http://linuxtv.org/hg/~tlorenz/v4l-dvb

It looks good, i will test with implementing the i2c functions.

Thanks.
--
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

Re: About the radio-si470x driver for I2C interface

2009-04-12 Thread Alexey Klimov 
Hello, Tobias

On Mon, Apr 13, 2009 at 12:56 AM, Tobias Lorenz  wrote:
> Hi Joonyoung,
>
> Hi Alexey,
>
> I've split the driver into a couple of segments:
>
> - radio-si470x-common.c is for common functions
>
> - radio-si470x-usb.c are the usb support functions
>
> - radio-si470x-i2c.c is an untested prototyped file for your i2c support
> functions
>
> - radio-si470x.h is a header file with everything required by the c-files
>
> I hope this is a basis we can start on with i2c support. What do you think?
>
> The URL is:
>
> http://linuxtv.org/hg/~tlorenz/v4l-dvb
>
> Bye,
>
> Toby

Great! It's always interesting to see big changes.
I understand i2c interface not so good and have only general questions.

Many (most?) drivers in v4l tree were converted to use new v4l2-
framework. For example, dsbr100 was converted to v4l2_device
http://linuxtv.org/hg/v4l-dvb/rev/77f37ad5dd0c and em28xx was
converted to v4l2_subdev
http://linuxtv.org/hg/v4l-dvb/rev/00525b115901
As i remember, Hans Verkuil said that all v4l drivers should be
converted to new framework. Is it time to switch to this new interface
? Probably, there are a lot of code examples in current tree that can
help..

And second question. About lock/unlock_kernel in open functions.
Please, take a look at
http://www.spinics.net/lists/linux-media/msg04057.html
Well, is it time to do something with this?

Well, my questions about improving functionality, not about mistakes or bugs :)

-- 
Best regards, Klimov Alexey
--
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

Re: About the radio-si470x driver for I2C interface

2009-04-01 Thread Joonyoung Shim 
> Hello
> 
> On Tue, Mar 10, 2009 at 3:20 AM, Mauro Carvalho Chehab
>  wrote:
>> On Mon, 9 Mar 2009 23:33:38 +0100
>> Tobias Lorenz  wrote:
>>
>>> Hi,
>>>
 The proper way is to break radio-si470x into two parts:

 - A i2c adapter driver (similar to what we have on cx88-i2c, for
   example, plus the radio part of cx88-video);
 - A radio driver (similar to tea5767.c).

 This way, the i2c driver can be used on designs that use a different i2c 
 adapter.
>>> yes, this is why I already capsulated most of the USB functionality into 
>>> own functions. I awaited that somewhen the si470x is used in the "usual" 
>>> way by i2c.
>>>
>>> I'm not sure, if we should split the driver into three files 
>>> (generic/common, usb, i2c) or just implement the new functionality within 
>>> the same file using macros/defines.
>> It is better to split. It will provide more flexibility.
> 
> Tobias, Joonyoung
> 
> Is there any success on this ?

Hi,

I have tried, but could not do it yet.

Like radio-tea5764.c, radio-si470x driver for I2C interface also was
implemented as the I2C driver, but it has many redundant functions with
the existing radio-si470x for USB interface, so I tried to join two
interfaces but it was a difficult work. I have never seen the linux device
driver probing more than one interface optionally.

-- Joonyoung Shim

> 
> --
> Best regards, Klimov Alexey
> 
> 
> 

--
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

Re: About the radio-si470x driver for I2C interface

2009-03-31 Thread Alexey Klimov 
Hello

On Tue, Mar 10, 2009 at 3:20 AM, Mauro Carvalho Chehab
 wrote:
> On Mon, 9 Mar 2009 23:33:38 +0100
> Tobias Lorenz  wrote:
>
>> Hi,
>>
>> > The proper way is to break radio-si470x into two parts:
>> >
>> >     - A i2c adapter driver (similar to what we have on cx88-i2c, for
>> >       example, plus the radio part of cx88-video);
>> >     - A radio driver (similar to tea5767.c).
>> >
>> > This way, the i2c driver can be used on designs that use a different i2c 
>> > adapter.
>>
>> yes, this is why I already capsulated most of the USB functionality into own 
>> functions. I awaited that somewhen the si470x is used in the "usual" way by 
>> i2c.
>>
>> I'm not sure, if we should split the driver into three files 
>> (generic/common, usb, i2c) or just implement the new functionality within 
>> the same file using macros/defines.
>
> It is better to split. It will provide more flexibility.

Tobias, Joonyoung

Is there any success on this ?

-- 
Best regards, Klimov Alexey
--
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

Re: About the radio-si470x driver for I2C interface

2009-03-09 Thread Mauro Carvalho Chehab 
On Mon, 9 Mar 2009 23:33:38 +0100
Tobias Lorenz  wrote:

> Hi,
> 
> > The proper way is to break radio-si470x into two parts:
> > 
> > - A i2c adapter driver (similar to what we have on cx88-i2c, for
> >   example, plus the radio part of cx88-video);
> > - A radio driver (similar to tea5767.c).
> > 
> > This way, the i2c driver can be used on designs that use a different i2c 
> > adapter.
> 
> yes, this is why I already capsulated most of the USB functionality into own 
> functions. I awaited that somewhen the si470x is used in the "usual" way by 
> i2c.
> 
> I'm not sure, if we should split the driver into three files (generic/common, 
> usb, i2c) or just implement the new functionality within the same file using 
> macros/defines.

It is better to split. It will provide more flexibility.

Cheers,
Mauro
--
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

Re: About the radio-si470x driver for I2C interface

2009-03-06 Thread Mauro Carvalho Chehab 
On Fri, 6 Mar 2009 12:13:05 +0900
Joonyoung Shim  wrote:

> Hi all,
> 
> I have worked with Silicon Labs Si4709 chip using the I2C interface.
> There is the radio-si470x driver in linux-kernel, but it uses usb interface.
> 
> First, i made a new file based on radio-si470x.c in driver/media/radio/ for
> si4709 i2c driver and modified it to use i2c interface instead of usb
> interface and could listen to FM radio station.
> 
> I think it can be to join two things together to one file because there isn't
> the difference between the two except for the interface.
> I am considering how to integrate them.
> 
> Please send your opinion.
> 

The proper way is to break radio-si470x into two parts:

- A i2c adapter driver (similar to what we have on cx88-i2c, for
  example, plus the radio part of cx88-video);
- A radio driver (similar to tea5767.c).

This way, the i2c driver can be used on designs that use a different i2c 
adapter.

Cheers,
Mauro
--
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

About the radio-si470x driver for I2C interface

2009-03-05 Thread Joonyoung Shim 
Hi all,

I have worked with Silicon Labs Si4709 chip using the I2C interface.
There is the radio-si470x driver in linux-kernel, but it uses usb interface.

First, i made a new file based on radio-si470x.c in driver/media/radio/ for
si4709 i2c driver and modified it to use i2c interface instead of usb
interface and could listen to FM radio station.

I think it can be to join two things together to one file because there isn't
the difference between the two except for the interface.
I am considering how to integrate them.

Please send your opinion.


-- 
- Joonyoung Shim
--
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