[U-Boot-Users] Status LEDs status?

[U-Boot-Users] Status LEDs status?

[Pantelis Antoniou]

Hi

I face the following problem. In a new version of my
board with a mpc850 the location of the status leds
moved to an external register, instead of being on
a CPU port.

However the status LED code assumes that the LEDs
are on a port.

As I scanned the other cpu directories I saw that
the status led code is copied in each of them.

I dont want to special case the status_led code with
my board, so I'm inclined to make a new status led
interface in the common or drivers directory.

IMHO the duplication of the status led code and it's
placement in the cpu directory is not very intuitive.

Your thoughts on that?

Regards

Pantelis

[U-Boot-Users] Status LEDs status?

[Wolfgang Denk]

Hi,

in message <3EE452B3.8000603@intracom.gr> you wrote:
> 
> I face the following problem. In a new version of my
> board with a mpc850 the location of the status leds
> moved to an external register, instead of being on
> a CPU port.
> 
> However the status LED code assumes that the LEDs
> are on a port.

That's because so far all boards where support for the status LED has
been enabled fall intot his group. Feel free to add your own version.


> As I scanned the other cpu directories I saw that
> the status led code is copied in each of them.

This is not correct. There is a "status_led.c" file only in  the  CPU
directories  for  5xx, 8xx and 82xx CPUs. These are pretty similar as
far as the status LED is affected.

> I dont want to special case the status_led code with
> my board, so I'm inclined to make a new status led
> interface in the common or drivers directory.

I think this should go to drivers/ .

> IMHO the duplication of the status led code and it's
> placement in the cpu directory is not very intuitive.

But the current implementation is CPU dependend.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-4596-87  Fax: (+49)-8142-4596-88  Email: wd at denx.de
Love sometimes expresses itself in sacrifice.
	-- Kirk, "Metamorphosis", stardate 3220.3

[U-Boot-Users] Status LEDs status?

[Pantelis Antoniou]

Hi Wolfgang

Well the first thing we can do is removing
the redundant code.

With the following patch I moved the status LED code
from the cpu directories and placed it in the drivers
directory.

The code for mpc8xx is the superset of both
the 8260 and 5xx code, so this is the base for it.

Patch applies cleanly againt CVS and is tested to
build in 8xx/8260/5xx configs that use LEDs.

What do you think?

Regards

Pantelis

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: status_led.patch
Url: http://lists.denx.de/pipermail/u-boot/attachments/20030609/3d5c56af/attachment.txt 

[U-Boot-Users] Status LEDs status?

[Wolfgang Denk]

In message <3EE46FBE.3020803@intracom.gr> you wrote:
>
> Well the first thing we can do is removing
> the redundant code.
> 
> With the following patch I moved the status LED code
> from the cpu directories and placed it in the drivers
> directory.
> 
> The code for mpc8xx is the superset of both
> the 8260 and 5xx code, so this is the base for it.
> 
> Patch applies cleanly againt CVS and is tested to
> build in 8xx/8260/5xx configs that use LEDs.
> 
> What do you think?

This is exactly what I do NOT want.

I do not want tosee processor specific code  in  common  directories.
The  drivers/ directory is intended for general code that applies for
all CPUs.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-4596-87  Fax: (+49)-8142-4596-88  Email: wd at denx.de
Premature optimization is the root of all evil.         -- D.E. Knuth

[U-Boot-Users] Status LEDs status?

[Pantelis Antoniou]

Wolfgang Denk wrote:

>In message <3EE46FBE.3020803@intracom.gr> you wrote:
>  
>
>>Well the first thing we can do is removing
>>the redundant code.
>>
>>With the following patch I moved the status LED code
>>from the cpu directories and placed it in the drivers
>>directory.
>>
>>The code for mpc8xx is the superset of both
>>the 8260 and 5xx code, so this is the base for it.
>>
>>Patch applies cleanly againt CVS and is tested to
>>build in 8xx/8260/5xx configs that use LEDs.
>>
>>What do you think?
>>    
>>
>
>This is exactly what I do NOT want.
>
>I do not want tosee processor specific code  in  common  directories.
>The  drivers/ directory is intended for general code that applies for
>all CPUs.
>
>Best regards,
>
>Wolfgang Denk
>
>
Whoa, hold on.

First of all when you have three copies of the the same thing in different
directories there's bound to be trouble.
For example the 8260 & 5xx copies are already slightly out of sync.

Secondly LEDs are board specific and not cpu specific.
It was a coincidence that up to now the LEDs happened to be in cpu ports
and could be groupped under the cpu directories.

I think it is trivial to abstract the led operations into their respective
cpu include files for the common case. The existing board config headers
shouldn't be modified, while allowing other boards to override them.

In this case the status led code would be generic for all configurations
that support them.

Regards

Pantelis

[U-Boot-Users] Status LEDs status?

[Wolfgang Denk]

In message <3EE47B1C.3090601@intracom.gr> you wrote:
> 
> First of all when you have three copies of the the same thing in different
> directories there's bound to be trouble.

These are three similar files for similar processors, not necessarily
identical copies.

> For example the 8260 & 5xx copies are already slightly out of sync.

Probably because the 5xx is slightly different than the 8260?

> Secondly LEDs are board specific and not cpu specific.

OK, you are right in a way.  In  the  same  way,  the  ethernet  port
configuration  is  board  dependend.  BUt  nevertheless  the ethernet
drivers are specific to  the  ethernet  controllers  as  for  example
provided  by  the MPC8xx or MPC8260 chips, sothe ethernet drivers for
these are in the 8xx resp. 8260 CPU directories.

> It was a coincidence that up to now the LEDs happened to be in cpu ports
> and could be groupped under the cpu directories.

It is not a coincidence.  The  common  fact  is  that  they  all  use
(processor specific) port pins.

> I think it is trivial to abstract the led operations into their respective
> cpu include files for the common case. The existing board config headers
> shouldn't be modified, while allowing other boards to override them.
> 
> In this case the status led code would be generic for all configurations
> that support them.

If you have such a patch, please submit it. But please  note  that  I
don't want to add any CPU specific code in the drivers/ directory.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-4596-87  Fax: (+49)-8142-4596-88  Email: wd at denx.de
We call our dog Egypt, because in every room he leaves a pyramid.

[U-Boot-Users] Status LEDs status?

[Pantelis Antoniou]

Wolfgang Denk wrote:

>In message <3EE47B1C.3090601@intracom.gr> you wrote:
>
snip

>
>If you have such a patch, please submit it. But please  note  that  I
>don't want to add any CPU specific code in the drivers/ directory.
>
>Best regards,
>
>Wolfgang Denk
>
>
OK

How about this patch.

The main idea is to do all the hardware specific stuff
in a header while the file in the drivers directory
is very simple and common to all the configurations.

The key is the new include file mpc_led.h which
implements the current behaviour for the 8xx/8260/5xx.

If your board has LEDs that don't fall into
the current scheme then in the board include file
you define CONFIG_BOARD_SPECIFIC_LED and provide
the __led_init __led_toggle and __led_set inline functions.

IMHO this cleans up the current scheme, allows boards
that have non standard LED configurations to work, and
I think makes it easier for arches other that ppc to
support LEDs.

Regards

Pantelis

[U-Boot-Users] Status LEDs status?

[Wolfgang Denk]

In message <3EE57F28.1020208@intracom.gr> you wrote:
>
> How about this patch.
> 
> The main idea is to do all the hardware specific stuff
> in a header while the file in the drivers directory
> is very simple and common to all the configurations.

Added (after fixing the bugs).

Will show up on CVS soon.

Thanks.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-4596-87  Fax: (+49)-8142-4596-88  Email: wd at denx.de
Mistakes are often the stepping stones to utter failure.

[U-Boot-Users] Status LEDs status?

[Pantelis Antoniou]

Oops, sorry here is the patch attached.

Regards

Pantelis

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: status_led.patch
Url: http://lists.denx.de/pipermail/u-boot/attachments/20030610/83b75704/attachment.txt