Re: [PATCH 06/10] mfd: rtsx: update phy register

[Lee Jones <lee.jones@linaro.org>]

On Tue, 20 Jan 2015, 敬锐 wrote:
> On 01/19/2015 03:47 PM, Lee Jones wrote:
> > On Mon, 19 Jan 2015, 敬锐 wrote:
> >> >On 01/18/2015 08:29 PM, Lee Jones wrote:
> >>> > >On Thu, 15 Jan 2015,micky_ching@realsil.com.cn  wrote:
> >>> > >
> >>>> > >>From: Micky Ching<micky_ching@realsil.com.cn>
> >>>> > >>
> >>>> > >>update phy register value and using direct value instead of macros.
> >>>> > >>It is much easier to debug using constant value than a lot of macros.
> >>>> > >>We usually need compare the value directly to check the configure.
> >>> > >NACK.  This is the opposite of what I would like to see.
> >>> > >
> >> >When we debug, we usually need to compare the value provided from hardware,
> >> >of course we can compare by print them to kernel log. but I think it more
> >> >convenient from source code. And we only set phy register when
> >> >initialize chip,
> >> >so the value will not scattered everywhere, we will not benefit from macros.
> >> >
> >> >if we want to know the meaning of setting, we can look at the register
> >> >define.
> > This is not acceptable and is the complete opposite of what we're
> > trying to achieve.  I promote readability (by humans) as one of the
> > highest priorities when writing code.  0xFE6C is far from readable.
> >
> Because the truly thing we concerned is the address and value, not
> a lot of macros. 0xFE6C is a magic number, but a lot of macros
> even hide the magic number we curious. To verify if the source
> code is right, we only need to compare the value, if too much macros
> joined together, the work is boring and easy to inject errors.

I completely disagree and think the converse to be true.  It's _much_
easier to get the magic number incorrect.

> I hate magic number too, but in this case using magic number
> is deserved for correctness.
> 
> Two method can help enhance readability.
> (1). define register address and values
> If we want to know what the bit field means, we can jump to the register
> define. This is very easy by tag system.
> eg.
> rtsx_pci_write_phy_register(pcr, PHY_BPCR, 0x05C0);
> we can jump to PHY_BPCR to see bit-field define.
>
> (2). add comment for magic number.
> I don't like comment.
> 
> If you still like macro list, I will update the register next patch.

Defining register values and bit fields is the _correct_ way to do
this.  Using magic numbers is the _wrong_ way.

I'm sorry Micky, but I am not moving on this.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog