[lkml]   [2015]   [Jan]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH 06/10] mfd: rtsx: update phy register
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, wrote:
> >>> > >
> >>>> > >>From: Micky Ching<>
> >>>> > >>
> >>>> > >>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 │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

 \ /
  Last update: 2015-01-20 11:01    [W:0.068 / U:10.052 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site