PS3 ps3av_set_video_mode() make id signed

PATCH In review Roel Kluin 3 participants 5 messages Jan 17, 2009 – Feb 12, 2009

Subsystems (2)

PowerPC PS3 platform support

lore

Share & tools

lore

Roel Kluin

author

2009-01-17 20:54 UTC #0 lore raw

PATCH 1/1 PS3 ps3av_set_video_mode() make id signed

vi drivers/video/ps3fb.c +618
static int ps3fb_set_par(struct fb_info *info)

{
        struct ps3fb_par *par = info->par;
... [ and at line 660 ] ...
	if (ps3av_set_video_mode(par->new_mode_id))

now new_mode_id is an int

vi drivers/video/ps3fb.c +132
struct ps3fb_par {
...

        int new_mode_id;

...
};

vi drivers/ps3/ps3av.c +844
int ps3av_set_video_mode(u32 id)

-------------------------^^^

{
...

	if (... || id < 0) {

--------------------^^^

		dev_dbg(&ps3av->dev->core, "%s: error id :%d\n", __func__, id);
                return -EINVAL;
        }
...
		id = ps3av_auto_videomode(&ps3av->av_hw_conf);
                if (id < 1) {
---------------------^^^
                        printk(KERN_ERR "%s: invalid id :%d\n", __func__, id);
                        return -EINVAL;
                }
...
	ps3av->ps3av_mode = id;

vi drivers/ps3/ps3av.c +763
static int ps3av_auto_videomode()

-------^^^

+42
static struct ps3av {
...

        int ps3av_mode;

...
};

--------------------->8---------------8<------------------- make id signed so a negative id will get noticed

Signed-off-by Roel Kluin <[email protected]>

---

drivers/ps3/ps3av.c +1 -1

index 5324978..7aa6d41 100644
@@ -838,7 +838,7 @@ static int ps3av_get_hw_conf(struct ps3av *ps3av)
 }
 
 /* set mode using id */
-int ps3av_set_video_mode(u32 id)
+int ps3av_set_video_mode(int id)
 {
 	int size;
 	u32 option;

Geert Uytterhoeven

2009-01-20 09:21 UTC #1 lore raw

Quoted 48 lines · Roel Kluin

vi drivers/video/ps3fb.c +618
static int ps3fb_set_par(struct fb_info *info)
{
        struct ps3fb_par *par = info->par;
... [ and at line 660 ] ...
	if (ps3av_set_video_mode(par->new_mode_id))
now new_mode_id is an int
vi drivers/video/ps3fb.c +132
struct ps3fb_par {
...
        int new_mode_id;
...
};
vi drivers/ps3/ps3av.c +844
int ps3av_set_video_mode(u32 id)
-------------------------^^^
{
...
	if (... || id < 0) {
--------------------^^^
		dev_dbg(&ps3av->dev->core, "%s: error id :%d\n", __func__, id);
                return -EINVAL;
        }
...
		id = ps3av_auto_videomode(&ps3av->av_hw_conf);
                if (id < 1) {
---------------------^^^
                        printk(KERN_ERR "%s: invalid id :%d\n", __func__, id);
                        return -EINVAL;
                }
...
	ps3av->ps3av_mode = id;
vi drivers/ps3/ps3av.c +763
static int ps3av_auto_videomode()
-------^^^
+42
static struct ps3av {
...

Quoted 6 lines

        int ps3av_mode;
...
};
--------------------->8---------------8<------------------- make id signed so a negative id will get noticed

Thanks for noticing! It looks OK, except...

Quoted 7 lines

Signed-off-by Roel Kluin <[email protected]>
---
drivers/ps3/ps3av.c +0 -0
index 5324978..7aa6d41 100644

drivers/ps3/ps3av.c static int ps3av_get_hw_conf(struct ps3av *ps3av) · line 844

Quoted 8 lines

drivers/ps3/ps3av.c +1 -1

@@ -838,7 +838,7 @@ static int ps3av_get_hw_conf(struct ps3av *ps3av)
 }
 
 /* set mode using id */
-int ps3av_set_video_mode(u32 id)
+int ps3av_set_video_mode(int id)
 {
 	int size;
 	u32 option;

1. You forgot to update the forward declaration of ps3av_set_video_mode() in arch/powerpc/include/asm/ps3av.h (did you compile this succesfully?)

2. You forgot to change `u32 id' in ps3av_probe().

Can you please make these changes, too? Thx again!

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   [email protected]
Internet: http://www.sony-europe.com/

A division of Sony Europe (Belgium) N.V. VAT BE 0413.825.160 · RPR Brussels Fortis · BIC GEBABEBB · IBAN BE41293037680010

Roel Kluin

author

2009-01-20 15:57 UTC #2 lore raw

Quoted 7 lines

Quoted 2 lines
make id signed so a negative id will get noticed
Thanks for noticing! It looks OK, except...
1. You forgot to update the forward declaration of ps3av_set_video_mode() in arch/powerpc/include/asm/ps3av.h (did you compile this succesfully?)

fixed that (and no, sorry, I didn't compile test it, I have to focus a bit on a biotechnology exam, maybe later).

Quoted 1 line

2. You forgot to change `u32 id' in ps3av_probe().

ok, changed it. but I am not sure whether the last two hunks are ok. Should we error out like this or just let the negative value be assigned to ps3av->ps3av_mode? If not, is there more cleanup required?

Quoted 1 line

Can you please make these changes, too? Thx again!

No problem.

--------------------->8-------------------------8<------------------------ Make id signed so a negative id will get noticed. Error out if ps3av_auto_videomode() fails.

Signed-off-by Roel Kluin <[email protected]>

---
 arch/powerpc/include/asm/ps3av.h |    2 +-
 drivers/ps3/ps3av.c              |   16 ++++++++++++----
 2 files changed, 13 insertions(+), 5 deletions(-)

arch/powerpc/include/asm/ps3av.h +1 -1

index cd24ac1..0427b0b 100644
@@ -730,7 +730,7 @@ extern int ps3av_cmd_av_get_hw_conf(struct ps3av_pkt_av_get_hw_conf *);
 extern int ps3av_cmd_video_get_monitor_info(struct ps3av_pkt_av_get_monitor_info *,
 					    u32);
 
-extern int ps3av_set_video_mode(u32);
+extern int ps3av_set_video_mode(int);
 extern int ps3av_set_audio_mode(u32, u32, u32, u32, u32);
 extern int ps3av_get_auto_mode(void);
 extern int ps3av_get_mode(void);

drivers/ps3/ps3av.c +12 -4

index 5324978..235e87f 100644
@@ -838,7 +838,7 @@ static int ps3av_get_hw_conf(struct ps3av *ps3av)
 }
 
 /* set mode using id */
-int ps3av_set_video_mode(u32 id)
+int ps3av_set_video_mode(int id)
 {
 	int size;
 	u32 option;

@@ -940,7 +940,7 @@ EXPORT_SYMBOL_GPL(ps3av_audio_mute);
 static int ps3av_probe(struct ps3_system_bus_device *dev)
 {
 	int res;
-	u32 id;
+	int id;
 
 	dev_dbg(&dev->core, " -> %s:%d\n", __func__, __LINE__);
 	dev_dbg(&dev->core, "  timeout=%d\n", timeout);

@@ -962,8 +962,10 @@ static int ps3av_probe(struct ps3_system_bus_device *dev)
 	init_completion(&ps3av->done);
 	complete(&ps3av->done);
 	ps3av->wq = create_singlethread_workqueue("ps3avd");
-	if (!ps3av->wq)
+	if (!ps3av->wq) {
+		res = -ENOMEM;
 		goto fail;
+	}
 
 	switch (ps3_os_area_get_av_multi_out()) {
 	case PS3_PARAM_AV_MULTI_OUT_NTSC:

@@ -994,6 +996,12 @@ static int ps3av_probe(struct ps3_system_bus_device *dev)
 		safe_mode = 1;
 #endif /* CONFIG_FB */
 	id = ps3av_auto_videomode(&ps3av->av_hw_conf);
+	if (id < 0) {
+		printk(KERN_ERR "%s: invalid id :%d\n", __func__, id);
+		res = -EINVAL;
+		goto fail;
+	}
+
 	safe_mode = 0;
 
 	mutex_lock(&ps3av->mutex);

@@ -1007,7 +1015,7 @@ static int ps3av_probe(struct ps3_system_bus_device *dev)
 fail:
 	kfree(ps3av);
 	ps3av = NULL;
-	return -ENOMEM;
+	return res;
 }
 
 static int ps3av_remove(struct ps3_system_bus_device *dev)

Geert Uytterhoeven

2009-01-22 02:32 UTC #3 lore raw

Quoted 7 lines · Roel Kluin

Quoted 6 lines
Quoted 1 line
make id signed so a negative id will get noticed
Thanks for noticing! It looks OK, except...
1. You forgot to update the forward declaration of ps3av_set_video_mode() in arch/powerpc/include/asm/ps3av.h (did you compile this succesfully?)

Quoted 8 lines

fixed that (and no, sorry, I didn't compile test it, I have to focus a bit on a biotechnology exam, maybe later).
Quoted 1 line
2. You forgot to change `u32 id' in ps3av_probe().
ok, changed it. but I am not sure whether the last two hunks are ok. Should we error out like this or just let the negative value be assigned to ps3av->ps3av_mode? If not, is there more cleanup required?

I'll look into it and will take further care of it...

Quoted 8 lines

Quoted 2 lines
Can you please make these changes, too? Thx again!
No problem.
--------------------->8-------------------------8<------------------------ Make id signed so a negative id will get noticed. Error out if ps3av_auto_videomode() fails.

Thanks again!

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   [email protected]
Internet: http://www.sony-europe.com/

A division of Sony Europe (Belgium) N.V. VAT BE 0413.825.160 · RPR Brussels Fortis · BIC GEBABEBB · IBAN BE41293037680010

Geoff Levand

2009-02-12 17:24 UTC #4 lore raw

In reply to Roel Kluin

Quoted 8 lines · Roel Kluin

Make id signed so a negative id will get noticed. Error out if ps3av_auto_videomode() fails.
Signed-off-by Roel Kluin <[email protected]>
---
 arch/powerpc/include/asm/ps3av.h |    2 +-
 drivers/ps3/ps3av.c              |   16 ++++++++++++----
 2 files changed, 13 insertions(+), 5 deletions(-)

Just FYI, I added this to ps3-linux.git, since it is not a critical fix, I will submit for 2.6.30.

-Geoff