Status LEDs status?
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
Hi,
Quoted 8 lines · in message <[email protected]> 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.
Quoted 2 lines
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.
Quoted 3 lines
I dont want to special case the
status_ledcode 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/ .
Quoted 2 lines
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
Signature · 4 lines
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
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
Quoted 8 lines · In message <[email protected]> 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.
Quoted 7 lines
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
Signature · 3 lines
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
Quoted 22 lines · Wolfgang Denk
Quoted 7 lines · In message <[email protected]> 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.
Quoted 9 lines
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.
Quoted 9 lines
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
Quoted 3 lines · In message <[email protected]> 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.
Quoted 1 line
For example the 8260 & 5xx copies are already slightly out of sync.
Probably because the 5xx is slightly different than the 8260?
Quoted 1 line
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.
Quoted 2 lines
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.
Quoted 6 lines
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
Signature · 3 lines
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.
Quoted 2 lines · Wolfgang Denk
In message <[email protected]> you wrote:
snip
Quoted 9 lines
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
Quoted 6 lines · In message <[email protected]> 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
Signature · 3 lines
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.
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