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

[Kieran Bingham]

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 
>>
>> 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


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

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

[Russell King - ARM Linux]

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 
> > > 
> > > 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.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

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

[Laurent Pinchart]

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 
> > 
> > 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);
> }

-- 
Regards,

Laurent Pinchart

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

[Russell King - ARM Linux]

On Fri, Dec 02, 2016 at 01:43:26AM +0200, Laurent Pinchart wrote:
> From: Kieran Bingham 
> 
> 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);
}

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.