Some development questions

[Christian Gmeiner-4]

Hi all,

I have spend some time in the sources of the driver now, that I got a
good understanding of whats going on.
I have started to make /usr/src/linux/scripts/checkpatch.pl more
happier about the current source. I want
to "fix" most of the style issues which are left. You can have a first
look at my work here:
http://freehg.org/u/austriancoder/em8300-cgmeiner/
Hope such patches will find their way into the main repository :)

The next thing on my clean up list is to get rid of these "Register
access macros". If you look closely
at the source you will find references to write_register and to writel
- for writing values into registers.
After a quick research I found this one:
http://lwn.net/Articles/102232/ (usable > 2.6.9).
I hope you share my opinion to do it everywhere in the source the
same... so I want go for writel.

What is your opinion?

After this, I will try to update some known registers as I saw two or
more times the same sequence like

        writel(0x2, &em->mem[0x2000]);
        writel(0x0, &em->mem[0x2000]);

or

        /* Release reset */
        write_register(0x2000, 0x1);

So it seems that register 0x2000 has something to do with risc core reset.

cheers,
Christian

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Dxr3-devel mailing list
Dxr3-devel@...
https://lists.sourceforge.net/lists/listinfo/dxr3-devel

[Nicolas Boullis]

Hi,

On Tue, Aug 26, 2008 at 09:12:54PM +0200, Christian Gmeiner wrote:

> Hi all,
>
> I have spend some time in the sources of the driver now, that I got a
> good understanding of whats going on.
> I have started to make /usr/src/linux/scripts/checkpatch.pl more
> happier about the current source. I want
> to "fix" most of the style issues which are left. You can have a first
> look at my work here:
> http://freehg.org/u/austriancoder/em8300-cgmeiner/
> Hope such patches will find their way into the main repository :)

Some of them will certainly find their way. ;-)

Thanks a lot.


Some comments anyway:
 1) It would be nice if you could start your development branches on top
    on the latest changeset from the main repository (that was
    9328b32dd801 while you used 8bfd80c74fa2). That should make the
    possible future merge easier, although there should be no problem
    here.
 2) It would be nice if you could make all your changesets "atomic"
    (http://freehg.org/u/austriancoder/em8300-cgmeiner/rev/1073fc66a4ea 
    is not really, although it's not really a problem for this one) and
    use the first line of the description as a single-line short
    description for the whole change.
 3) Here are (roughly) the rules I try to follow to be compatible with a
    broad range of kernel versions:
     - whenever possible, don't use kernel versions at all (think
       CONFIG_*)
     - if it is necessary to support several different API, whenever
       possible, use the more recent one in the code and use a macro as
       a wrapper for older kernels
     - when 2 API overlap and must be supported, use the more recent one
       as early as possible
    http://freehg.org/u/austriancoder/em8300-cgmeiner/rev/8073355cea68 
    does not follow there rules; I'll rewrite it.

By the way, I don't know what /usr/src/linux/scripts/checkpatch.pl
really is, but I think there's no way the em8300 drivers can be merged
into the kernel until our adv717x and bt865 drivers are merged with the
corresponding drivers in the kernel... :-/

> The next thing on my clean up list is to get rid of these "Register
> access macros". If you look closely
> at the source you will find references to write_register and to writel
> - for writing values into registers.
> After a quick research I found this one:
> http://lwn.net/Articles/102232/ (usable > 2.6.9).
> I hope you share my opinion to do it everywhere in the source the
> same... so I want go for writel.
>
> What is your opinion?

Hmmm... Using 2 different syntaxes to deal with the same register
certainly is very ugly!
But I'd say that "write_register(0x2000, 0x1);" is somewhat easier to
read than "writel(0x0, &em->mem[0x2000]);"...


Cheers,

Nicolas

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Dxr3-devel mailing list
Dxr3-devel@...
https://lists.sourceforge.net/lists/listinfo/dxr3-devel

[Christian Gmeiner-4]

Haudi,

2008/9/9 Nicolas Boullis <nboullis@...>:

> Hi,
>
> On Tue, Aug 26, 2008 at 09:12:54PM +0200, Christian Gmeiner wrote:
>> Hi all,
>>
>> I have spend some time in the sources of the driver now, that I got a
>> good understanding of whats going on.
>> I have started to make /usr/src/linux/scripts/checkpatch.pl more
>> happier about the current source. I want
>> to "fix" most of the style issues which are left. You can have a first
>> look at my work here:
>> http://freehg.org/u/austriancoder/em8300-cgmeiner/
>> Hope such patches will find their way into the main repository :)
>
> Some of them will certainly find their way. ;-)
>
> Thanks a lot.

Fine... more stuff is coming...

>
>
> Some comments anyway:
>  1) It would be nice if you could start your development branches on top
>    on the latest changeset from the main repository (that was
>    9328b32dd801 while you used 8bfd80c74fa2). That should make the
>    possible future merge easier, although there should be no problem
>    here.
>  2) It would be nice if you could make all your changesets "atomic"
>    (http://freehg.org/u/austriancoder/em8300-cgmeiner/rev/1073fc66a4ea
>    is not really, although it's not really a problem for this one) and
>    use the first line of the description as a single-line short
>    description for the whole change.

Okay.. I will try to care about it :)

>  3) Here are (roughly) the rules I try to follow to be compatible with a
>    broad range of kernel versions:
>     - whenever possible, don't use kernel versions at all (think
>       CONFIG_*)
>     - if it is necessary to support several different API, whenever
>       possible, use the more recent one in the code and use a macro as
>       a wrapper for older kernels
>     - when 2 API overlap and must be supported, use the more recent one
>       as early as possible
>    http://freehg.org/u/austriancoder/em8300-cgmeiner/rev/8073355cea68
>    does not follow there rules; I'll rewrite it.

I am waiting for your solution to be commited.

>
> By the way, I don't know what /usr/src/linux/scripts/checkpatch.pl
> really is, but I think there's no way the em8300 drivers can be merged
> into the kernel until our adv717x and bt865 drivers are merged with the
> corresponding drivers in the kernel... :-/

As I have a lot of free time at the moment I will look in this topic.
Maybe I will start
an other development repository, where I will have working code only
for the current
development release. If the adv717x and bt865 driver in the kernel are ready, we
should kick our interface (ioctls, etc) and switch to v4l2 as it has
support for video
output devices. Also I think the em9010 stuff should go into an own module.

To be honest, I have started some work on v4l2 and em8300 already. But
I have some
other stuff on my todo:

* register macros
* cleanup i2c code
* better way to load fw
* coding style fixes
* adv717x and bt865 modlues

I hope you can do merges of my tree more often.

>
>
>> The next thing on my clean up list is to get rid of these "Register
>> access macros". If you look closely
>> at the source you will find references to write_register and to writel
>> - for writing values into registers.
>> After a quick research I found this one:
>> http://lwn.net/Articles/102232/ (usable > 2.6.9).
>> I hope you share my opinion to do it everywhere in the source the
>> same... so I want go for writel.
>>
>> What is your opinion?
>
> Hmmm... Using 2 different syntaxes to deal with the same register
> certainly is very ugly!
> But I'd say that "write_register(0x2000, 0x1);" is somewhat easier to
> read than "writel(0x0, &em->mem[0x2000]);"...
>

Fine.. will do the changes.

greets,
--
BSc, Christian Gmeiner

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Dxr3-devel mailing list
Dxr3-devel@...
https://lists.sourceforge.net/lists/listinfo/dxr3-devel

[Nicolas Boullis]

Hi,

On Wed, Sep 10, 2008 at 08:31:49AM +0000, Christian Gmeiner wrote:
>
> Fine... more stuff is coming...

I just did some more merges.
Note that the patches are not rejected, I just have not had time yet to
review them (especially
http://freehg.org/u/austriancoder/em8300-cgmeiner/rev/452d407a9ddc which
is non-trivial).

> >  3) Here are (roughly) the rules I try to follow to be compatible with a
> >    broad range of kernel versions:
> >     - whenever possible, don't use kernel versions at all (think
> >       CONFIG_*)
> >     - if it is necessary to support several different API, whenever
> >       possible, use the more recent one in the code and use a macro as
> >       a wrapper for older kernels
> >     - when 2 API overlap and must be supported, use the more recent one
> >       as early as possible
> >    http://freehg.org/u/austriancoder/em8300-cgmeiner/rev/8073355cea68
> >    does not follow there rules; I'll rewrite it.
>
> I am waiting for your solution to be commited.

Done: http://dxr3.sourceforge.net/hg/em8300-nboullis/rev/74b8fc170464

> > By the way, I don't know what /usr/src/linux/scripts/checkpatch.pl
> > really is, but I think there's no way the em8300 drivers can be merged
> > into the kernel until our adv717x and bt865 drivers are merged with the
> > corresponding drivers in the kernel... :-/
>
> As I have a lot of free time at the moment I will look in this topic.
> Maybe I will start
> an other development repository, where I will have working code only
> for the current
> development release. If the adv717x and bt865 driver in the kernel are ready, we

I don't think the adv7170, adv7175 and bt865 drivers in the kernel are
featurefull enough for use with H+/DXR3 boards... I fear the features
have to be merged, which will be somewhat non-trivial.


> should kick our interface (ioctls, etc) and switch to v4l2 as it has
> support for video output devices.

That'd be nice, but compatibility also matters. People don't wan't to
throw their old code. (Look at the dxr3 plugin for dvr that never
switched to ALSA...)


> Also I think the em9010 stuff should go into an own module.

I agree. By the way, I found a datasheet for the em9010 that tells that
it's an i2c chip. The current code does not address it in an
i2c-compatible way, so I think one should check if it works when
adressed in an i2c-compatible way, and then switch to using i2c
functions.

> To be honest, I have started some work on v4l2 and em8300 already. But
> I have some
> other stuff on my todo:
>
> * register macros
> * cleanup i2c code
> * better way to load fw
> * coding style fixes
> * adv717x and bt865 modlues
>
> I hope you can do merges of my tree more often.

Does it really matter? If your repository is more advanced and
better-maintained than mine, yours naturally becomes the "central"
repository.


Cheers,

Nicolas

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Dxr3-devel mailing list
Dxr3-devel@...
https://lists.sourceforge.net/lists/listinfo/dxr3-devel

[Nicolas Boullis]

Hi Christian,

On Fri, Sep 12, 2008 at 02:14:56AM +0200, Nicolas Boullis wrote:

> Hi,
>
> On Wed, Sep 10, 2008 at 08:31:49AM +0000, Christian Gmeiner wrote:
> >
> > Fine... more stuff is coming...
>
> I just did some more merges.
> Note that the patches are not rejected, I just have not had time yet to
> review them (especially
> http://freehg.org/u/austriancoder/em8300-cgmeiner/rev/452d407a9ddc which
> is non-trivial).

I just had a more thorough look at 452d407a9ddc and I fal to understand
why you do all this stuff around the pci_set_master call (check for
PCI_COMMAND_MASTER both before and after).
As far as U can see, most drivers that use pci_set_master don't do such
things (although at least drivers/ide/setup-pci.c and
drivers/media/video/ivtv/ivtv-driver.c do).
Do you have any explanation?


Cheers,

Nicolas

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Dxr3-devel mailing list
Dxr3-devel@...
https://lists.sourceforge.net/lists/listinfo/dxr3-devel

[Christian Gmeiner-4]

Hi Nicolas,

2008/9/14 Nicolas Boullis <nboullis@...>:

> Hi Christian,
>
> On Fri, Sep 12, 2008 at 02:14:56AM +0200, Nicolas Boullis wrote:
>> Hi,
>>
>> On Wed, Sep 10, 2008 at 08:31:49AM +0000, Christian Gmeiner wrote:
>> >
>> > Fine... more stuff is coming...
>>
>> I just did some more merges.
>> Note that the patches are not rejected, I just have not had time yet to
>> review them (especially
>> http://freehg.org/u/austriancoder/em8300-cgmeiner/rev/452d407a9ddc which
>> is non-trivial).
>
> I just had a more thorough look at 452d407a9ddc and I fal to understand
> why you do all this stuff around the pci_set_master call (check for
> PCI_COMMAND_MASTER both before and after).
> As far as U can see, most drivers that use pci_set_master don't do such
> things (although at least drivers/ide/setup-pci.c and
> drivers/media/video/ivtv/ivtv-driver.c do).
> Do you have any explanation?
>

If you want I will explain it to you, but I think my linux driver
programming book is too old :( I
had a quick look at drivers/pci/pci.c and found out, that all those
check, if our device supports
busmastering are not needed, as pci_set_master does all the hard work.
So.. I have ordered
a fresh book.

So please change the whole block to:


static int em8300_pci_setup(struct pci_dev *dev)
{
        struct em8300_s *em = pci_get_drvdata(dev);
        unsigned char revision;
        int rc = 0;
        u16 cmd;

        rc = pci_enable_device(dev);
        if (rc < 0) {
                printk(KERN_ERR "em8300: Unable to enable PCI device\n");
                return rc;
        }

        pci_set_master(dev);

        em->adr = pci_resource_start(dev, 0);
        em->memsize = pci_resource_len(dev, 0);

        pci_read_config_byte(dev, PCI_CLASS_REVISION, &revision);
        em->pci_revision = revision;

        return 0;
}

I am sorry for this trouble, but I have learned much from it and hope
such situations will
never happen again, as I will have a look at the sources of the kernel
if I am not sure.

By the way... which kernel version do we support?

thanks
--
Christian Gmeiner, B.Sc.

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Dxr3-devel mailing list
Dxr3-devel@...
https://lists.sourceforge.net/lists/listinfo/dxr3-devel

[Nicolas Boullis]

Hi,

On Mon, Sep 15, 2008 at 06:00:14AM +0000, Christian Gmeiner wrote:

>
> If you want I will explain it to you, but I think my linux driver
> programming book is too old :( I
> had a quick look at drivers/pci/pci.c and found out, that all those
> check, if our device supports
> busmastering are not needed, as pci_set_master does all the hard work.
> So.. I have ordered
> a fresh book.
>
> So please change the whole block to:
>
>
> static int em8300_pci_setup(struct pci_dev *dev)
> {
> struct em8300_s *em = pci_get_drvdata(dev);
> unsigned char revision;
> int rc = 0;
> u16 cmd;
>
> rc = pci_enable_device(dev);
> if (rc < 0) {
> printk(KERN_ERR "em8300: Unable to enable PCI device\n");
> return rc;
> }
>
> pci_set_master(dev);
>
> em->adr = pci_resource_start(dev, 0);
> em->memsize = pci_resource_len(dev, 0);
>
> pci_read_config_byte(dev, PCI_CLASS_REVISION, &revision);
> em->pci_revision = revision;
>
> return 0;
> }

I did it and merged your other patches.
(Edit: I've just seen you have new ones...)


> I am sorry for this trouble, but I have learned much from it and hope
> such situations will
> never happen again, as I will have a look at the sources of the kernel
> if I am not sure.

It's no problem.


> By the way... which kernel version do we support?

As far as I am concerned, I'm willing to support all 2.4 and 2.6
kernels.
I know most developers don't care about 2.4 any more, but:
 * I know at least one person who uses the em8300 drivers with a 2.4
   kernel;
 * I hate the way the 2.6 kernel is managed (unstable API, invasive
   changes, ...), so I understand why one may prefer to keep the
   rock-solid 2.4 branch.

It'd be ok for me, if needed, to drop support for old 2.4 and old 2.6
kernels.


Cheers,

Nicolas

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Dxr3-devel mailing list
Dxr3-devel@...
https://lists.sourceforge.net/lists/listinfo/dxr3-devel

[Gabor Z. Papp]

* Nicolas Boullis <nboullis@...>:

| I know most developers don't care about 2.4 any more, but:
|  * I know at least one person who uses the em8300 drivers with a 2.4
|    kernel;

/me yelling :-)

Thanks Nicolas.

|  * I hate the way the 2.6 kernel is managed (unstable API, invasive
|    changes, ...), so I understand why one may prefer to keep the
|    rock-solid 2.4 branch.

My dxr3 system works like a charm since many years with 2.4, using P3
and sata disks, lirc (note the current lirc madness on 2.6...)
2.6.25.x releases were the first ones that ran mostly stable on the
same hardware where 2.4 not fails. So probably I could switch, but why...

| It'd be ok for me, if needed, to drop support for old 2.4 and old 2.6
| kernels.

Please no, at least for 2.4. If you guys send me patches for testing
or ask me to test the svn/cvs/anything, I would report
success/problems on 2.4. I'm silently following the current
development here and using always the latest release/rc.

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Dxr3-devel mailing list
Dxr3-devel@...
https://lists.sourceforge.net/lists/listinfo/dxr3-devel

[Christian Gmeiner-4]

Hi

2008/9/17 Gabor Z. Papp <gzp@...>:

> * Nicolas Boullis <nboullis@...>:
>
> | I know most developers don't care about 2.4 any more, but:
> |  * I know at least one person who uses the em8300 drivers with a 2.4
> |    kernel;
>
> /me yelling :-)
>
> Thanks Nicolas.
>
> |  * I hate the way the 2.6 kernel is managed (unstable API, invasive
> |    changes, ...), so I understand why one may prefer to keep the
> |    rock-solid 2.4 branch.
>
> My dxr3 system works like a charm since many years with 2.4, using P3
> and sata disks, lirc (note the current lirc madness on 2.6...)
> 2.6.25.x releases were the first ones that ran mostly stable on the
> same hardware where 2.4 not fails. So probably I could switch, but why...

Never change a running system :)

>
> | It'd be ok for me, if needed, to drop support for old 2.4 and old 2.6
> | kernels.
>
> Please no, at least for 2.4. If you guys send me patches for testing
> or ask me to test the svn/cvs/anything, I would report
> success/problems on 2.4. I'm silently following the current
> development here and using always the latest release/rc.

That is fine to hear that someone is testing the driver with a 2.4 kernel. I
want to say that in the main tree, I will not remove 2.4 and old 2.6 stuff,
but around the end of this year I will start a repository for kernel inclusion.
So in this new repository 2.4 and old 2.6 support will gets removed, but I
will announce this step here at the ml.

have a nice day
--
Christian Gmeiner, B.Sc.

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Dxr3-devel mailing list
Dxr3-devel@...
https://lists.sourceforge.net/lists/listinfo/dxr3-devel

[Christian Gmeiner-4]

Hello,

>
> I did it and merged your other patches.
> (Edit: I've just seen you have new ones...)
>

fine to see... yes there are some new patches which want to get reviewed :)
Here the interesting facts:
* reworked firmware loading
* rework device memory allocation
* some other cleanups

Oh.. an I have a mistake in on commit summary: fix compile error
should be fix compile warning

>
>> By the way... which kernel version do we support?
>
> As far as I am concerned, I'm willing to support all 2.4 and 2.6
> kernels.
> I know most developers don't care about 2.4 any more, but:
>  * I know at least one person who uses the em8300 drivers with a 2.4
>   kernel;
>  * I hate the way the 2.6 kernel is managed (unstable API, invasive
>   changes, ...), so I understand why one may prefer to keep the
>   rock-solid 2.4 branch.
>
> It'd be ok for me, if needed, to drop support for old 2.4 and old 2.6
> kernels.
>

I will drop support for 2.4 and old 2.6 if v4l people - looking at
Hans - have done some
needed changes. And then I will start a new repository for it.

Btw, whats about your http://freehg.org/u/nboullis/em8300-scale-n-crop/ ?

have a nice day
--
Christian Gmeiner, B.Sc.

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Dxr3-devel mailing list
Dxr3-devel@...
https://lists.sourceforge.net/lists/listinfo/dxr3-devel

[Nicolas Boullis]

On Wed, Sep 17, 2008 at 09:54:14AM +0200, Gabor Z. Papp wrote:
>
> | It'd be ok for me, if needed, to drop support for old 2.4 and old 2.6
> | kernels.
>
> Please no, at least for 2.4.

Sorry, I meant something like "dropping support for pre-2.4.26 2.4
kernels and for pre-2.6.18 2.6 kernels", certainly not dropping support
for all 2.4 kernels. Would you object this?


Cheers,

Nicolas

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Dxr3-devel mailing list
Dxr3-devel@...
https://lists.sourceforge.net/lists/listinfo/dxr3-devel

[Nicolas Boullis]

Hi,

On Wed, Sep 17, 2008 at 09:02:03AM +0000, Christian Gmeiner wrote:

> Hello,
>
> >
> > I did it and merged your other patches.
> > (Edit: I've just seen you have new ones...)
> >
>
> fine to see... yes there are some new patches which want to get reviewed :)
> Here the interesting facts:
> * reworked firmware loading
> * rework device memory allocation
> * some other cleanups

Some comments already, after a quick look:

 - I think 455d3227a288 ("rework firmware loading") is plain wrong. On
   systems without CONFIG_FW_LOADER, em8300_require_ucode is no-op, so I
   think the driver would fail to setup the EM8300-based cards. I'd
   rather not stop supporting systems without CONFIG_FW_LOADER (which
   includes all pre-2.4.23 kernels).
   Moreover, if mplayer is broken, we don't have to fix it here.

 - As for ef34058b80f3 ("add a useful error message"), *if I remember
   correctly* flags==0 marks the end of the microcode and is expected,
   so I guess the error message is wrong.

 - As for 6059be5ad914 ("use a template for i2c_algo_bit_data"), I think
   em8300_i2c_algo_template should be made const. I guess I would also
   use simple assignment of the structure rather than memcpy (easier to
   read).

 - As for ec05b2763b8d ("fix compile error"), I think I'd use
   KERN_WARNING rather than KERN_ERR.

 - As for 09b3e93eac9f ("rework device memory allocation"), how do you
   get those figures?


> Oh.. an I have a mistake in on commit summary: fix compile error
> should be fix compile warning

Can't you fix it?
By the way, I think you have removed some changesets on freehg.org. How
did you do this?


> Btw, whats about your http://freehg.org/u/nboullis/em8300-scale-n-crop/ ?

 1) I've had very little feedback of people with the corresponding
    hardware (ADV7170-based card + TV set that understands WSS).

 2) It is still somewhat broken because the EM8300 chip sometimes
    switches itself to 16:9 using the "old" way. I have not yet found a
    way to prevent it.


Cheers,

Nicolas

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Dxr3-devel mailing list
Dxr3-devel@...
https://lists.sourceforge.net/lists/listinfo/dxr3-devel

[Christian Gmeiner-4]

Hi,

2008/9/17 Nicolas Boullis <nboullis@...>:

> Hi,
>
> On Wed, Sep 17, 2008 at 09:02:03AM +0000, Christian Gmeiner wrote:
>> Hello,
>>
>> >
>> > I did it and merged your other patches.
>> > (Edit: I've just seen you have new ones...)
>> >
>>
>> fine to see... yes there are some new patches which want to get reviewed :)
>> Here the interesting facts:
>> * reworked firmware loading
>> * rework device memory allocation
>> * some other cleanups
>
> Some comments already, after a quick look:
>
>  - I think 455d3227a288 ("rework firmware loading") is plain wrong. On
>   systems without CONFIG_FW_LOADER, em8300_require_ucode is no-op, so I
>   think the driver would fail to setup the EM8300-based cards. I'd
>   rather not stop supporting systems without CONFIG_FW_LOADER (which
>   includes all pre-2.4.23 kernels).
>   Moreover, if mplayer is broken, we don't have to fix it here.

I think you have misunderstood me. Image following situation:
Kernel 2.6.26 with hg em8300 drivers. You have forgotten to add the
firmware to the correct place. You load your driver, and start vdr or mplayer.
Now... nothing is working, but in your log you find lot of lines saying your
firmware is missing and vdr/mplayer tries to write data or is working
with ioctls.

And this is the point of this rework. If we are on a CONFIG_FW_LOADER system,
require the fw at module loading time, and not if we do an ioctl or
something else which
triggers the loading.
As you can see in the diff, I have added no code which was not used
before. I have moved the

 case _IOC_NR(EM8300_IOCTL_WRITEREG):
     em8300_require_ucode(em);

     if (!em->ucodeloaded) {
         return -ENOTTY;
     }

and all others, only into one em8300_probe function block.

 em8300_require_ucode(em);
 if (!em->ucodeloaded) {
     printk(KERN_ERR "em8300: Failed to load firmware\n");
     result = -ENODEV;
     goto firmware_error;
 }


Keep in mind, that this does _only_ affect CONFIG_FW_LOADER based systems.
Hope you get the point now... if you do not trust me, try it by your
own and rename
microcode file and see what is happen.

>
>  - As for ef34058b80f3 ("add a useful error message"), *if I remember
>   correctly* flags==0 marks the end of the microcode and is expected,
>   so I guess the error message is wrong.

I have created a little test program to see what is going on at fw
loading, and as far as I
can see, flags == 0 does _not_ mark the end of the microcode. See
attachment and do
some own tests.

>  - As for 6059be5ad914 ("use a template for i2c_algo_bit_data"), I think
>   em8300_i2c_algo_template should be made const. I guess I would also
>   use simple assignment of the structure rather than memcpy (easier to
>   read).
ack


>  - As for ec05b2763b8d ("fix compile error"), I think I'd use
>   KERN_WARNING rather than KERN_ERR.
ack

>  - As for 09b3e93eac9f ("rework device memory allocation"), how do you
>   get those figures?
>

size em8300.ko

>
>> Oh.. an I have a mistake in on commit summary: fix compile error
>> should be fix compile warning
>
> Can't you fix it?

I will have a look at the mercurial docs..

> By the way, I think you have removed some changesets on freehg.org. How
> did you do this?

I am not sure... but after each round of merge I reset my repository
and start at the same
level as the main repository.

>
>> Btw, whats about your http://freehg.org/u/nboullis/em8300-scale-n-crop/ ?
>
>  1) I've had very little feedback of people with the corresponding
>    hardware (ADV7170-based card + TV set that understands WSS).

me has only adv7175 based cards..

>  2) It is still somewhat broken because the EM8300 chip sometimes
>    switches itself to 16:9 using the "old" way. I have not yet found a
>    way to prevent it.

What is the "old" way?

greets
--
Christian Gmeiner, B.Sc.


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Dxr3-devel mailing list
Dxr3-devel@...
https://lists.sourceforge.net/lists/listinfo/dxr3-devel

[Gabor Z. Papp]

* Nicolas Boullis <nboullis@...>:

| > | It'd be ok for me, if needed, to drop support for old 2.4 and old 2.6
| > | kernels.
| >
| > Please no, at least for 2.4.

| Sorry, I meant something like "dropping support for pre-2.4.26 2.4
| kernels and for pre-2.6.18 2.6 kernels", certainly not dropping support
| for all 2.4 kernels. Would you object this?

I'm using always the latest 2.4

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Dxr3-devel mailing list
Dxr3-devel@...
https://lists.sourceforge.net/lists/listinfo/dxr3-devel

[Nicolas Boullis]

Hi,

On Thu, Sep 18, 2008 at 06:03:41AM +0000, Christian Gmeiner wrote:

>
> I think you have misunderstood me. Image following situation:
> Kernel 2.6.26 with hg em8300 drivers. You have forgotten to add the
> firmware to the correct place. You load your driver, and start vdr or mplayer.
> Now... nothing is working, but in your log you find lot of lines saying your
> firmware is missing and vdr/mplayer tries to write data or is working
> with ioctls.
>
> And this is the point of this rework. If we are on a CONFIG_FW_LOADER system,
> require the fw at module loading time, and not if we do an ioctl or
> something else which
> triggers the loading.
> As you can see in the diff, I have added no code which was not used
> before. I have moved the
>
>  case _IOC_NR(EM8300_IOCTL_WRITEREG):
>      em8300_require_ucode(em);
>
>      if (!em->ucodeloaded) {
>          return -ENOTTY;
>      }
>
> and all others, only into one em8300_probe function block.
>
>  em8300_require_ucode(em);
>  if (!em->ucodeloaded) {
>      printk(KERN_ERR "em8300: Failed to load firmware\n");
>      result = -ENODEV;
>      goto firmware_error;
>  }
>
>
> Keep in mind, that this does _only_ affect CONFIG_FW_LOADER based systems.
> Hope you get the point now... if you do not trust me, try it by your
> own and rename
> microcode file and see what is happen.

Sorry, but I still disagree quite strongly.

Here is what happens, before your change, on a system without
CONFIG_FW_LOADER:
 1. the driver is loaded and the card is half-enabled;
 2. the firmware is loaded with the em8300setup userspace application
    (or any other userspace application, as I think the dxr3 plugin for
    vdr loads it as well) as the corresponding ioctl is allowed even
    without a firmware;
 3. the card is fully-enabled and everything's fine.

Now, with your patch, on such a system, em8300_require_ucode is no-op.
Then em8300_probe fails because nothing sets em->ucodeloaded, and the
user is given no chance to load the firmware.

Unless you prove me wrong, I won't accept such a change.

I can see several solutions:
 1. Officially drop support for systems without CONFIG_FW_LOADER, but do
    it cleanly. I guess that means that the build should fail on such a
    system. This means drop support for pre-2.4.23 kernels. (This is not
    my prefered solution.)
 2. Only require the firmware loading in em8300_probe on systems with
    CONFIG_FW_LOADER, but do not break anything on systems without it.
 3. Leave things unchanged.


As for mplayer (and possibly others): why should a sane application
insist if it fails to open /dev/em8300_{ma,mv,sp}? If it does, it's not
the driver's fault!


> >  - As for ef34058b80f3 ("add a useful error message"), *if I remember
> >   correctly* flags==0 marks the end of the microcode and is expected,
> >   so I guess the error message is wrong.
>
> I have created a little test program to see what is going on at fw
> loading, and as far as I
> can see, flags == 0 does _not_ mark the end of the microcode. See
> attachment and do
> some own tests.

OK, I'll check again and more thoroughly.


> >  - As for 09b3e93eac9f ("rework device memory allocation"), how do you
> >   get those figures?
>
> size em8300.ko

OK, thanks.


> >> Oh.. an I have a mistake in on commit summary: fix compile error
> >> should be fix compile warning
> >
> > Can't you fix it?
>
> I will have a look at the mercurial docs..

You can export a changeset, modify it, and reimport it. Note that it
means it makes a new branch. Then do the same export/import game for the
following changes (or use transplant).


> > By the way, I think you have removed some changesets on freehg.org. How
> > did you do this?
>
> I am not sure... but after each round of merge I reset my repository
> and start at the same
> level as the main repository.

Hmmm... I did not know it was possible to reset a repository on
freehg.org; how do you do that?


> >  2) It is still somewhat broken because the EM8300 chip sometimes
> >    switches itself to 16:9 using the "old" way. I have not yet found a
> >    way to prevent it.
>
> What is the "old" way?

Set (or clear) bit 4 of DICOM_TvOut to enable/disable "hardware"
letterbox.
When the EM8300 chip (or its firmware) sees an aspect ratio change, it
enables or disables "hardware" letterbox mode.
The new code makes no use of this feature; "hardware" letterbox mode is
supposed to be always disabled, while letterbox and pillarbox are
implemented in software by playing with
DICOM_{Frame,Visible}{Top,Bottom,Left,Right}.
Unfortunately, the hardware sometimes enables "hardware" letterbox mode
anyway, which adds to the scaling implemented in software, and leads to
en image vertically compressed.

Note that even if you don't have an adv7170-based card, it might still
be worth testing if you have a 16:9 TV set (to have pillarbox for 4:3
broadcasts). But you won't of course have WSS.


Cheers,

Nicolas

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Dxr3-devel mailing list
Dxr3-devel@...
https://lists.sourceforge.net/lists/listinfo/dxr3-devel

[Christian Gmeiner-4]

Hi,

2008/9/19 Nicolas Boullis <nboullis@...>:

> Hi,
>
> On Thu, Sep 18, 2008 at 06:03:41AM +0000, Christian Gmeiner wrote:
>>
>> I think you have misunderstood me. Image following situation:
>> Kernel 2.6.26 with hg em8300 drivers. You have forgotten to add the
>> firmware to the correct place. You load your driver, and start vdr or mplayer.
>> Now... nothing is working, but in your log you find lot of lines saying your
>> firmware is missing and vdr/mplayer tries to write data or is working
>> with ioctls.
>>
>> And this is the point of this rework. If we are on a CONFIG_FW_LOADER system,
>> require the fw at module loading time, and not if we do an ioctl or
>> something else which
>> triggers the loading.
>> As you can see in the diff, I have added no code which was not used
>> before. I have moved the
>>
>>  case _IOC_NR(EM8300_IOCTL_WRITEREG):
>>      em8300_require_ucode(em);
>>
>>      if (!em->ucodeloaded) {
>>          return -ENOTTY;
>>      }
>>
>> and all others, only into one em8300_probe function block.
>>
>>  em8300_require_ucode(em);
>>  if (!em->ucodeloaded) {
>>      printk(KERN_ERR "em8300: Failed to load firmware\n");
>>      result = -ENODEV;
>>      goto firmware_error;
>>  }
>>
>>
>> Keep in mind, that this does _only_ affect CONFIG_FW_LOADER based systems.
>> Hope you get the point now... if you do not trust me, try it by your
>> own and rename
>> microcode file and see what is happen.
>
> Sorry, but I still disagree quite strongly.
>
> Here is what happens, before your change, on a system without
> CONFIG_FW_LOADER:
>  1. the driver is loaded and the card is half-enabled;
>  2. the firmware is loaded with the em8300setup userspace application
>    (or any other userspace application, as I think the dxr3 plugin for
>    vdr loads it as well) as the corresponding ioctl is allowed even
>    without a firmware;
>  3. the card is fully-enabled and everything's fine.
>
> Now, with your patch, on such a system, em8300_require_ucode is no-op.
> Then em8300_probe fails because nothing sets em->ucodeloaded, and the
> user is given no chance to load the firmware.
>
> Unless you prove me wrong, I won't accept such a change.

Ahhh... now I got it... shame on me.

>
> I can see several solutions:
>  1. Officially drop support for systems without CONFIG_FW_LOADER, but do
>    it cleanly. I guess that means that the build should fail on such a
>    system. This means drop support for pre-2.4.23 kernels. (This is not
>    my prefered solution.)
>  2. Only require the firmware loading in em8300_probe on systems with
>    CONFIG_FW_LOADER, but do not break anything on systems without it.
>  3. Leave things unchanged.
>

I would go for 3... I am fine with it.

>
>> >  - As for ef34058b80f3 ("add a useful error message"), *if I remember
>> >   correctly* flags==0 marks the end of the microcode and is expected,
>> >   so I guess the error message is wrong.
>>
>> I have created a little test program to see what is going on at fw
>> loading, and as far as I
>> can see, flags == 0 does _not_ mark the end of the microcode. See
>> attachment and do
>> some own tests.
>
> OK, I'll check again and more thoroughly.
>
>
>> >  - As for 09b3e93eac9f ("rework device memory allocation"), how do you
>> >   get those figures?
>>
>> size em8300.ko
>
> OK, thanks.
>
>
>> >> Oh.. an I have a mistake in on commit summary: fix compile error
>> >> should be fix compile warning
>> >
>> > Can't you fix it?
>>
>> I will have a look at the mercurial docs..
>
> You can export a changeset, modify it, and reimport it. Note that it
> means it makes a new branch. Then do the same export/import game for the
> following changes (or use transplant).
>

Thanks, will try it.

>
>> > By the way, I think you have removed some changesets on freehg.org. How
>> > did you do this?
>>
>> I am not sure... but after each round of merge I reset my repository
>> and start at the same
>> level as the main repository.
>
> Hmmm... I did not know it was possible to reset a repository on
> freehg.org; how do you do that?
>

Its not like a reset, but is more like.. delete repository... create a
new one with
the same name and push your em8300 repository to it.

>> >  2) It is still somewhat broken because the EM8300 chip sometimes
>> >    switches itself to 16:9 using the "old" way. I have not yet found a
>> >    way to prevent it.
>>
>> What is the "old" way?
>
> Set (or clear) bit 4 of DICOM_TvOut to enable/disable "hardware"
> letterbox.
> When the EM8300 chip (or its firmware) sees an aspect ratio change, it
> enables or disables "hardware" letterbox mode.
> The new code makes no use of this feature; "hardware" letterbox mode is
> supposed to be always disabled, while letterbox and pillarbox are
> implemented in software by playing with
> DICOM_{Frame,Visible}{Top,Bottom,Left,Right}.
> Unfortunately, the hardware sometimes enables "hardware" letterbox mode
> anyway, which adds to the scaling implemented in software, and leads to
> en image vertically compressed.
>
> Note that even if you don't have an adv7170-based card, it might still
> be worth testing if you have a 16:9 TV set (to have pillarbox for 4:3
> broadcasts). But you won't of course have WSS.
>

I only own a 4:3 TV set... :(

Oh.. and the main repository seems to be broken:

Traceback (most recent call last):
  File "/home/groups/d/dx/dxr3/cgi-bin/hgwebdir.cgi", line 21, in ?
    from mercurial.hgweb.hgwebdir_mod import hgwebdir
  File "/home/groups/d/dx/dxr3/lib/hg-0.9.4/mercurial/hgweb/__init__.py",
line 9, in ?
    import hgweb_mod, hgwebdir_mod
  File "/home/groups/d/dx/dxr3/lib/hg-0.9.4/mercurial/hgweb/hgweb_mod.py",
line 9, in ?
    import os, mimetypes, re, zlib, mimetools, cStringIO, sys
  File "/usr/lib64/python2.4/mimetools.py", line 6, in ?
    import tempfile
  File "/usr/lib64/python2.4/tempfile.py", line 33, in ?
    from random import Random as _Random
  File "/usr/lib64/python2.4/random.py", line 47, in ?
    from binascii import hexlify as _hexlify
ImportError: /usr/lib64/python2.4/lib-dynload/binascii.so: failed to
map segment from shared object: Cannot allocate memory


cheers
--
Christian Gmeiner, B.Sc.

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Dxr3-devel mailing list
Dxr3-devel@...
https://lists.sourceforge.net/lists/listinfo/dxr3-devel

[Nicolas Boullis]

Hi,

On Sat, Sep 20, 2008 at 03:35:43PM +0000, Christian Gmeiner wrote:

>
> Oh.. and the main repository seems to be broken:
>
> Traceback (most recent call last):
>   File "/home/groups/d/dx/dxr3/cgi-bin/hgwebdir.cgi", line 21, in ?
>     from mercurial.hgweb.hgwebdir_mod import hgwebdir
>   File "/home/groups/d/dx/dxr3/lib/hg-0.9.4/mercurial/hgweb/__init__.py",
> line 9, in ?
>     import hgweb_mod, hgwebdir_mod
>   File "/home/groups/d/dx/dxr3/lib/hg-0.9.4/mercurial/hgweb/hgweb_mod.py",
> line 9, in ?
>     import os, mimetypes, re, zlib, mimetools, cStringIO, sys
>   File "/usr/lib64/python2.4/mimetools.py", line 6, in ?
>     import tempfile
>   File "/usr/lib64/python2.4/tempfile.py", line 33, in ?
>     from random import Random as _Random
>   File "/usr/lib64/python2.4/random.py", line 47, in ?
>     from binascii import hexlify as _hexlify
> ImportError: /usr/lib64/python2.4/lib-dynload/binascii.so: failed to
> map segment from shared object: Cannot allocate memory

Hmmm, that's bad. I hope SourceForge did not break it on purpose.
Anyway, until I can fix it, I just set up a new public copy of my main
hg repository:
  http://freehg.org/u/nboullis/em8300/


Cheers,

Nicolas

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Dxr3-devel mailing list
Dxr3-devel@...
https://lists.sourceforge.net/lists/listinfo/dxr3-devel

[Christian Gmeiner-4]

Hi Nicolas,

how should we proceed? Fix all issues (remove wrong fw loading stuff,
do the i2c stuff in an
other way) and then you pull or shall I "reset" my repository and do
better.... or you want
to merge some of my patches?

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Dxr3-devel mailing list
Dxr3-devel@...
https://lists.sourceforge.net/lists/listinfo/dxr3-devel

[Nicolas Boullis]

Hi,

On Thu, Sep 25, 2008 at 08:22:49AM +0000, Christian Gmeiner wrote:
>
> how should we proceed? Fix all issues (remove wrong fw loading stuff,
> do the i2c stuff in an
> other way) and then you pull or shall I "reset" my repository and do
> better.... or you want
> to merge some of my patches?

Sorry for the long silence.
I just merged most of your changes.
As for 455d3227a288 ("rework firmware loading"), we already talked about
it.
As for ef34058b80f3 ("add a useful error message"), you are right that
this apparently is not a normal end condition (what I thought). But I
think it's quite useless to test this since there are *many* other
"strange" conditions where we could think the firmware is corrupted.
I also changed the description ec05b2763b8d ("fix compile error" -> "fix
compile warning").
And since I skipped the first change, I transplanted the remaining ones,
so their IDs changed.


Cheers,

Nicolas

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Dxr3-devel mailing list
Dxr3-devel@...
https://lists.sourceforge.net/lists/listinfo/dxr3-devel