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

[æé]

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@xxxxxxxxxxxxxx  wrote:
>>> > >
>>>> > >>From: Micky Ching<micky_ching@xxxxxxxxxxxxxx>
>>>> > >>
>>>> > >>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 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.

regards.
micky.