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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20150120094557.GL5767@x1>
Date:	Tue, 20 Jan 2015 09:45:57 +0000
From:	Lee Jones <lee.jones@...aro.org>
To:	敬锐 <micky_ching@...lsil.com.cn>
Cc:	"sameo@...ux.intel.com" <sameo@...ux.intel.com>,
	"devel@...uxdriverproject.org" <devel@...uxdriverproject.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"rogerable@...ltek.com" <rogerable@...ltek.com>,
	王炜 <wei_wang@...lsil.com.cn>
Subject: Re: [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,micky_ching@...lsil.com.cn  wrote:
> >>> > >
> >>>> > >>From: Micky Ching<micky_ching@...lsil.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/