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@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 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 _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
