[oe] [PATCH 10/10] xserver-kdrive-common: xilinx virtex5 support

[Paul Menzel]

Am Freitag, den 30.07.2010, 10:42 -0500 schrieb Adrian Alonso:
> * Add Xserver support for virtex5 board
> * Update module id for Xilinx platforms, xserver
>   arguments are setup at runtime by getting
>   module id info from /proc/cpuinfo

Please split those two changes up into separate patches.

This should be worded differently. Maybe, »On Xilinx platforms there is
no line `Hardware`. Instead it is stored in the line starting with
`platform`.

> * Increase PR
> 
> Signed-off-by: Adrian Alonso <aalonso00 at gmail.com>

NAK.

> ---
>  .../xserver-kdrive-common/Xserver                  |    9 ++++++++-
>  .../xserver-kdrive-common_0.1.bb                   |    2 +-
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/recipes/xserver-kdrive-common/xserver-kdrive-common/Xserver b/recipes/xserver-kdrive-common/xserver-kdrive-common/Xserver
> index 347b005..375b787 100644
> --- a/recipes/xserver-kdrive-common/xserver-kdrive-common/Xserver
> +++ b/recipes/xserver-kdrive-common/xserver-kdrive-common/Xserver
> @@ -34,7 +34,12 @@ module_id() {
>      # grep "Module ID" /proc/hal/assets | sed "s/.*://"
>      ## used to read from /proc/hal/model, but that is removed in 2.6
>      # echo ' iPAQ' `cat /proc/hal/model`
> -    awk 'BEGIN { FS=": " } /Hardware/ { print $2 } ' </proc/cpuinfo
> +    id=`awk 'BEGIN { FS=": " } /Hardware/ { print $2 } ' </proc/cpuinfo`
> +	if [ -n "$id" ]; then
> +		echo -n "${id}"
> +    else
> +        awk 'BEGIN { FS=": " } /platform/ { print $2 } ' </proc/cpuinfo
> +	fi
>  }

1. Please use spaces instead of tabulators.
2. It took me some time to figure out, why you use a variable here. I
think it would be nicer the following way.

	id=`awk 'BEGIN { FS=": " } /Hardware/ { print $2 } ' </proc/cpuinfo`
	if [ ! -n "$id" ]; then
		id=`awk 'BEGIN { FS=": " } /platform/ { print $2 } ' </proc/cpuinfo`
	echo -n "${id}"

Why are the curly brackets needed?

>  export USER=root
> @@ -171,6 +176,8 @@ case `module_id` in
>                   if [ "$XSERVER" = "/usr/bin/Xorg" ];then
>                        ARGS=""
>                   fi;; #TODO: handle kdrive
> +		 "Xilinx Virtex"*)
> +				 ARGS="$ARGS $PPM" ;;
>          *)
>                  # It is a device we do not know about, in which case we force
>                  # kdrive to use the current framebuffer geometry -- otherwise

Please use spaces instead of tabulators.

[…]


Thanks,

Paul


PS: Please remember to update the status of your patches in Patchwork
[1][2].


[1] http://patchwork.openembedded.org/
[2] http://wiki.openembedded.net/index.php/Patchwork (I need to update
this patch on how to use `pwclient` to update the states of the patches.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <http://lists.openembedded.org/pipermail/openembedded-devel/attachments/20100731/ee4c43e4/attachment-0002.sig>