[PATCH 0/3] [VIN-Tests] minor updates

[PATCH 0/3] [VIN-Tests] minor updates

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

Hi Niklas,

Some minor updates to the 8camera-status tool, and a speed up fix for the
vin-tests core function 'mc_get_dev()'.

Kieran Bingham (3):
  tools: 8camera-status: Fix write failure mis-allocations
  tools: 8camera-status: Expand to 9 cameras
  vin-tests: Refactor mc_get_dev

 scripts/vin-tests.sh | 14 +++-----------
 tools/8camera-status | 12 ++++++------
 2 files changed, 9 insertions(+), 17 deletions(-)

-- 
2.7.4

[PATCH 1/3] tools: 8camera-status: Fix write failure mis-allocations

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Debug prints of the max9271_write call are appearing in the failure counts.

Fix the 'catcher' so that it is more specific to the failure

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 tools/8camera-status | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/8camera-status b/tools/8camera-status
index 234b8a0cab2b..cdfe76ef286f 100755
--- a/tools/8camera-status
+++ b/tools/8camera-status
@@ -21,7 +21,7 @@ BEGIN {
 	print
 }
 
-/max9271_write/ {
+/max9271_write.*write failed/ {
 	max9271_write_fail++
 	print
 }
-- 
2.7.4

[PATCH 2/3] tools: 8camera-status: Expand to 9 cameras

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

The 8th camera has an address conflict on the Salvator-X.
As such it has been moved to the right by one address,
Include this '9th' camera in the reporting status

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 tools/8camera-status | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/8camera-status b/tools/8camera-status
index cdfe76ef286f..846673578f09 100755
--- a/tools/8camera-status
+++ b/tools/8camera-status
@@ -1,7 +1,7 @@
 #!/usr/bin/awk -f
 
 BEGIN {
-	for (x = 0; x <= 8; x++)
+	for (x = 0; x <= 9; x++)
 		cameras[x] = "-"
 }
 
@@ -67,15 +67,15 @@ function ParseProbe ( line )
 
 function PrintCameras()
 {
-	print "===================================="
-	print "| 1 | 2 | 3 | 4 |  | 5 | 6 | 7 | 8 |"
-	for (x=1; x <= 8; x++) {
+	print "========================================"
+	print "| 1 | 2 | 3 | 4 |  | 5 | 6 | 7 | 8 | 9 |"
+	for (x=1; x <= 9; x++) {
 		printf "| " cameras[x] " "
 		if (x == 4)
 			printf("|  ")
 	}
 	printf "|\n"
-	print "===================================="
+	print "======================================="
 }
 
 function Summarise() {
-- 
2.7.4

[PATCH 3/3] vin-tests: Refactor mc_get_dev

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Rather that using shell parsing of each file when looking for a device
node, use a combination of grep and sed to identify the device.

This is a remarkable speed optimisation for this code segment.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 scripts/vin-tests.sh | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/scripts/vin-tests.sh b/scripts/vin-tests.sh
index 7c81aa51c1c5..2e6214bc95e6 100644
--- a/scripts/vin-tests.sh
+++ b/scripts/vin-tests.sh
@@ -99,17 +99,9 @@ mc_get_mdev() {
 }
 
 mc_get_dev() {
-    name=$1
-    mdev=$(mc_get_mdev)
-
-    for dev in  /sys/class/video4linux/*; do
-        if [[ "$(cat $dev/name)" == "$name" ]]; then
-            basename $dev
-            return 0
-        fi
-    done
-
-    error "Can't find device"
+    name="$1"
+    grep -l "$name" /sys/class/video4linux/video*/name | \
+	    sed 's#.*video4linux\(.*\)/name#/dev\1#g'
 }
 
 mc_log() {
-- 
2.7.4

Re: [PATCH 1/3] tools: 8camera-status: Fix write failure mis-allocations

[Niklas Söderlund]

Hi Kieran,

Thanks for your patch, applied.

On 2017-11-15 14:38:29 +0000, Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> Debug prints of the max9271_write call are appearing in the failure counts.
> 
> Fix the 'catcher' so that it is more specific to the failure
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  tools/8camera-status | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/8camera-status b/tools/8camera-status
> index 234b8a0cab2b..cdfe76ef286f 100755
> --- a/tools/8camera-status
> +++ b/tools/8camera-status
> @@ -21,7 +21,7 @@ BEGIN {
>  	print
>  }
>  
> -/max9271_write/ {
> +/max9271_write.*write failed/ {
>  	max9271_write_fail++
>  	print
>  }
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCH 2/3] tools: 8camera-status: Expand to 9 cameras

[Niklas Söderlund]

Hi Kieran,

Thanks for your patch, applied.

On 2017-11-15 14:38:30 +0000, Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> The 8th camera has an address conflict on the Salvator-X.
> As such it has been moved to the right by one address,
> Include this '9th' camera in the reporting status
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  tools/8camera-status | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/8camera-status b/tools/8camera-status
> index cdfe76ef286f..846673578f09 100755
> --- a/tools/8camera-status
> +++ b/tools/8camera-status
> @@ -1,7 +1,7 @@
>  #!/usr/bin/awk -f
>  
>  BEGIN {
> -	for (x = 0; x <= 8; x++)
> +	for (x = 0; x <= 9; x++)
>  		cameras[x] = "-"
>  }
>  
> @@ -67,15 +67,15 @@ function ParseProbe ( line )
>  
>  function PrintCameras()
>  {
> -	print "===================================="
> -	print "| 1 | 2 | 3 | 4 |  | 5 | 6 | 7 | 8 |"
> -	for (x=1; x <= 8; x++) {
> +	print "========================================"
> +	print "| 1 | 2 | 3 | 4 |  | 5 | 6 | 7 | 8 | 9 |"
> +	for (x=1; x <= 9; x++) {
>  		printf "| " cameras[x] " "
>  		if (x == 4)
>  			printf("|  ")
>  	}
>  	printf "|\n"
> -	print "===================================="
> +	print "======================================="
>  }
>  
>  function Summarise() {
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCH 3/3] vin-tests: Refactor mc_get_dev

[Niklas Söderlund]

Hi Kieran,

Thanks for your patch.

Unfortunately I experience some problems with this patch.

On 2017-11-15 14:38:31 +0000, Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> Rather that using shell parsing of each file when looking for a device
> node, use a combination of grep and sed to identify the device.
> 
> This is a remarkable speed optimisation for this code segment.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  scripts/vin-tests.sh | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/scripts/vin-tests.sh b/scripts/vin-tests.sh
> index 7c81aa51c1c5..2e6214bc95e6 100644
> --- a/scripts/vin-tests.sh
> +++ b/scripts/vin-tests.sh
> @@ -99,17 +99,9 @@ mc_get_mdev() {
>  }
>  
>  mc_get_dev() {
> -    name=$1
> -    mdev=$(mc_get_mdev)
> -
> -    for dev in  /sys/class/video4linux/*; do
> -        if [[ "$(cat $dev/name)" == "$name" ]]; then
> -            basename $dev
> -            return 0
> -        fi
> -    done
> -
> -    error "Can't find device"
> +    name="$1"
> +    grep -l "$name" /sys/class/video4linux/video*/name | \
> +	    sed 's#.*video4linux\(.*\)/name#/dev\1#g'

The only user of mc_get_dev() is the set-edid utility, which uses it to 
find the adv748x HDMI subdevice (adv748x 4-0070 hdmi) so it can program 
the EDID. I had to change search path above to 
/sys/class/video4linux/*/name to also searches in v4l-subdevX 
directories to find thatt device.

Also the return value changes from 'v4l-subdev42' to /dev/v4l-subdev42' 
so the set-edid tool needed a small update to handle that :-)

I have applied this patch as-is, and then a followup which takes care of 
the above. Out of curiosity, are you working on any new tests which uses 
mc_get_dev()?

>  }
>  
>  mc_log() {
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCH 3/3] vin-tests: Refactor mc_get_dev

[Kieran Bingham]

Hi Niklas,

On 15/11/17 16:51, Niklas Söderlund wrote:
> Hi Kieran,
> 
> Thanks for your patch.
> 
> Unfortunately I experience some problems with this patch.

My apologies, I thought this was working when I used it, it was left in my local
branch and hadn't been sent (along with the other two) and I thought they were
all in use on my board. It looks like somehow I've maybe saved my earlier
development patch, perhaps instead of the one that was working on the board and
it's not correct. I'm sorry for that.


> On 2017-11-15 14:38:31 +0000, Kieran Bingham wrote:
>> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>
>> Rather that using shell parsing of each file when looking for a device
>> node, use a combination of grep and sed to identify the device.
>>
>> This is a remarkable speed optimisation for this code segment.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>> ---
>>  scripts/vin-tests.sh | 14 +++-----------
>>  1 file changed, 3 insertions(+), 11 deletions(-)
>>
>> diff --git a/scripts/vin-tests.sh b/scripts/vin-tests.sh
>> index 7c81aa51c1c5..2e6214bc95e6 100644
>> --- a/scripts/vin-tests.sh
>> +++ b/scripts/vin-tests.sh
>> @@ -99,17 +99,9 @@ mc_get_mdev() {
>>  }
>>  
>>  mc_get_dev() {
>> -    name=$1
>> -    mdev=$(mc_get_mdev)
>> -
>> -    for dev in  /sys/class/video4linux/*; do
>> -        if [[ "$(cat $dev/name)" == "$name" ]]; then
>> -            basename $dev
>> -            return 0
>> -        fi
>> -    done
>> -
>> -    error "Can't find device"
>> +    name="$1"
>> +    grep -l "$name" /sys/class/video4linux/video*/name | \
>> +	    sed 's#.*video4linux\(.*\)/name#/dev\1#g'
> 
> The only user of mc_get_dev() is the set-edid utility, which uses it to 
> find the adv748x HDMI subdevice (adv748x 4-0070 hdmi) so it can program 
> the EDID. I had to change search path above to 
> /sys/class/video4linux/*/name to also searches in v4l-subdevX 
> directories to find thatt device.

Yes, that is a change in input parameters and output parameters in my patch as I
submitted. Which was clearly just a bit wrong :)

> 
> Also the return value changes from 'v4l-subdev42' to /dev/v4l-subdev42' 
> so the set-edid tool needed a small update to handle that :-)

Yes, this should have been more like:

grep "$name" /sys/class/video4linux/*/name | \
	sed 's#.*video4linux/\(.*\)/name.*#\1#g'

> I have applied this patch as-is, and then a followup which takes care of 
> the above. Out of curiosity, are you working on any new tests which uses 
> mc_get_dev()?

This was a patch I mentioned to you back when I had written it but hadn't
submitted. Clearly it looks like I was doing something different as I had indeed
changed the output return value. Which looks quite intentional, in the above.

For the moment - I'm going to put this down to baby-brain ... and pretend it
didn't happen ... then try to make sure I don't do something equally as silly again.

Sorry for the noise, and thank you for fixing it up! :-)
--
Kieran