[11/22] drm: bridge: dw-hdmi: Refactor hdmi_phy_configure resolution parameter

Commit Message

Laurent Pinchart Dec. 1, 2016, 11:43 p.m. UTC

From: Kieran Bingham <[e-mail]>

The current code hard codes the call of hdmi_phy_configure() to be 8bpp
and provides extraneous error checking to verify that this hardcoded
value is correct.

Simplify the passing of the data by setting the parameter to be of the
enum type it represents rather than converting and then verifying the
value. This will allow the compiler to check the value is acceptable
based on the type, and remove the dead code that we currently have.

Signed-off-by: Kieran Bingham <[e-mail]>
Signed-off-by: Laurent Pinchart <[e-mail]>
---
 drivers/gpu/drm/bridge/dw-hdmi.c | 20 ++------------------
 include/drm/bridge/dw_hdmi.h     |  2 +-
 2 files changed, 3 insertions(+), 19 deletions(-)

Comments

Russell King (Oracle) Dec. 2, 2016, 2:18 p.m. UTC | #1

On Fri, Dec 02, 2016 at 01:43:26AM +0200, Laurent Pinchart wrote:
> From: Kieran Bingham <[e-mail]>
> 
> The current code hard codes the call of hdmi_phy_configure() to be 8bpp
> and provides extraneous error checking to verify that this hardcoded
> value is correct.
> 
> Simplify the passing of the data by setting the parameter to be of the
> enum type it represents rather than converting and then verifying the
> value. This will allow the compiler to check the value is acceptable
> based on the type, and remove the dead code that we currently have.

I think you're expecting too much of the compiler there.  There's no
requirement for the compiler to check that an enum type is passed one
of it's defined values.

Try building this and see if it even produces a warning:

enum foo {
	FOO_1,
	FOO_2,
};

int func(enum foo foo)
{
	return foo;
}

int test_1(void)
{
	return func(FOO_1);
}

int test_2(void)
{
	return func(5);
}

Laurent Pinchart Dec. 2, 2016, 3:51 p.m. UTC | #2

Hi Russell,

On Friday 02 Dec 2016 14:18:08 Russell King - ARM Linux wrote:
> On Fri, Dec 02, 2016 at 01:43:26AM +0200, Laurent Pinchart wrote:
> > From: Kieran Bingham <[e-mail]>
> > 
> > The current code hard codes the call of hdmi_phy_configure() to be 8bpp
> > and provides extraneous error checking to verify that this hardcoded
> > value is correct.
> > 
> > Simplify the passing of the data by setting the parameter to be of the
> > enum type it represents rather than converting and then verifying the
> > value. This will allow the compiler to check the value is acceptable
> > based on the type, and remove the dead code that we currently have.
> 
> I think you're expecting too much of the compiler there.  There's no
> requirement for the compiler to check that an enum type is passed one
> of it's defined values.

You're right.

Given that the current driver hardcodes the resolution value to 8bpp, how 
about just dropping the argument ? We can always add it back later if/when 
needed.

> Try building this and see if it even produces a warning:
> 
> enum foo {
> 	FOO_1,
> 	FOO_2,
> };
> 
> int func(enum foo foo)
> {
> 	return foo;
> }
> 
> int test_1(void)
> {
> 	return func(FOO_1);
> }
> 
> int test_2(void)
> {
> 	return func(5);
> }

Russell King (Oracle) Dec. 2, 2016, 4:08 p.m. UTC | #3

On Fri, Dec 02, 2016 at 05:51:18PM +0200, Laurent Pinchart wrote:
> Hi Russell,
> 
> On Friday 02 Dec 2016 14:18:08 Russell King - ARM Linux wrote:
> > On Fri, Dec 02, 2016 at 01:43:26AM +0200, Laurent Pinchart wrote:
> > > From: Kieran Bingham <[e-mail]>
> > > 
> > > The current code hard codes the call of hdmi_phy_configure() to be 8bpp
> > > and provides extraneous error checking to verify that this hardcoded
> > > value is correct.
> > > 
> > > Simplify the passing of the data by setting the parameter to be of the
> > > enum type it represents rather than converting and then verifying the
> > > value. This will allow the compiler to check the value is acceptable
> > > based on the type, and remove the dead code that we currently have.
> > 
> > I think you're expecting too much of the compiler there.  There's no
> > requirement for the compiler to check that an enum type is passed one
> > of it's defined values.
> 
> You're right.
> 
> Given that the current driver hardcodes the resolution value to 8bpp, how 
> about just dropping the argument ? We can always add it back later if/when 
> needed.

Definitely - there's no point having features in the driver which no one
uses.

Kieran Bingham Dec. 5, 2016, 7:53 a.m. UTC | #4

On 02/12/16 14:18, Russell King - ARM Linux wrote:
> On Fri, Dec 02, 2016 at 01:43:26AM +0200, Laurent Pinchart wrote:
>> From: Kieran Bingham <[e-mail]>
>>
>> The current code hard codes the call of hdmi_phy_configure() to be 8bpp
>> and provides extraneous error checking to verify that this hardcoded
>> value is correct.
>>
>> Simplify the passing of the data by setting the parameter to be of the
>> enum type it represents rather than converting and then verifying the
>> value. This will allow the compiler to check the value is acceptable
>> based on the type, and remove the dead code that we currently have.
> 
> I think you're expecting too much of the compiler there.  There's no
> requirement for the compiler to check that an enum type is passed one
> of it's defined values.
> 
> Try building this and see if it even produces a warning:
> 
> enum foo {
> 	FOO_1,
> 	FOO_2,
> };
> 
> int func(enum foo foo)
> {
> 	return foo;
> }
> 
> int test_1(void)
> {
> 	return func(FOO_1);
> }
> 
> int test_2(void)
> {
> 	return func(5);
> }
> 

Ahh, yes - Sorry - I appear to have got confused between the effects of
gcc/g++.

I knew I had it in my head that the compiler can do enum-type checking
... but it was from when I was working on  C++ projects.

gcc /tmp/test.c -o /tmp/test
	<no stderr output>

g++     /tmp/test.cpp   -o /tmp/test
/tmp/test.cpp: In function ‘int test_2()’:
/tmp/test.cpp:23:16: error: invalid conversion from ‘int’ to ‘foo’
[-fpermissive]
   return func(5);

C++ will provide type checking on enums, but of course not C.

Sorry for the confusion, and it looks like Laurent is handling this
already, Thanks


Regards
--
Kieran Bingham

Patch

diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index 107fea49c4c6..074ceb1e4897 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -931,30 +931,14 @@  static void dw_hdmi_phy_sel_interface_control(struct dw_hdmi *hdmi, u8 enable)
 }
 
 static int hdmi_phy_configure(struct dw_hdmi *hdmi,
-			      unsigned char res, int cscon)
+			      enum dw_hdmi_resolution res_idx, int cscon)
 {
-	unsigned res_idx;
 	u8 val, msec;
 	const struct dw_hdmi_plat_data *pdata = hdmi->plat_data;
 	const struct dw_hdmi_mpll_config *mpll_config = pdata->mpll_cfg;
 	const struct dw_hdmi_curr_ctrl *curr_ctrl = pdata->cur_ctr;
 	const struct dw_hdmi_phy_config *phy_config = pdata->phy_config;
 
-	switch (res) {
-	case 0:	/* color resolution 0 is 8 bit colour depth */
-	case 8:
-		res_idx = DW_HDMI_RES_8;
-		break;
-	case 10:
-		res_idx = DW_HDMI_RES_10;
-		break;
-	case 12:
-		res_idx = DW_HDMI_RES_12;
-		break;
-	default:
-		return -EINVAL;
-	}
-
 	/* PLL/MPLL Cfg - always match on final entry */
 	for (; mpll_config->mpixelclock != ~0UL; mpll_config++)
 		if (hdmi->hdmi_data.video_mode.mpixelclock <=
@@ -1068,7 +1052,7 @@  static int dw_hdmi_phy_init(struct dw_hdmi *hdmi)
 		dw_hdmi_phy_enable_powerdown(hdmi, true);
 
 		/* Enable CSC */
-		ret = hdmi_phy_configure(hdmi, 8, cscon);
+		ret = hdmi_phy_configure(hdmi, DW_HDMI_RES_8, cscon);
 		if (ret)
 			return ret;
 	}
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index 3bb22a849830..e551b457c100 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -14,7 +14,7 @@ 
 
 struct dw_hdmi;
 
-enum {
+enum dw_hdmi_resolution {
 	DW_HDMI_RES_8,
 	DW_HDMI_RES_10,
 	DW_HDMI_RES_12,