linux-kernel - Re: [PATCH 00/83] staging: comedi: rtd520: initial cleanup

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] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 11 Jul 2012 11:20:27 +0100
From:	Ian Abbott <abbotti@....co.uk>
To:	H Hartley Sweeten <hartleys@...ionengravers.com>
CC:	Linux Kernel <linux-kernel@...r.kernel.org>,
	"devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
	Ian Abbott <ian.abbott@....co.uk>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
Subject: Re: [PATCH 00/83] staging: comedi: rtd520: initial cleanup

On 2012-07-11 00:36, H Hartley Sweeten wrote:
> The 'devpriv' macro usage in this driver is holding up other cleanup
> of the comedi drivers.
>
> This patch series removes all the macros used to read/write the
> hardware registers. All of them are simple wrappers around standard
> {read,write}[rwl] calls with some of them caching the write value
> in a shadow variable in the private data.
>
> Each of the macros is removed in a separate patch to ease reviewing.
> They can all be squashed into one patch if desired.
>
> After all the macros are gone, the devpriv macro itself can be removed
> along with the boardinfo macro.
>
> The "find pci device" code is also refactored to follow the style of
> the other comedi pci drivers.
>
> The range tables are also cleaned up for aesthetic reasons. The large
> whitespace causes some pretty nasty line breaks in order to keep the
> lines < 80 characters.

My main concern with this series of patches is they make it harder to 
determine what the register accesses actually do, mainly due to the 
removal of useful comments.  For example:

-	RtdPacerStart(dev);	/* Start PACER */
+	readl(devpriv->las0 + LAS0_PACER);

And:

-	RtdPacerStop(dev);	/* Stop PACER */
+	writel(0, devpriv->las0 + LAS0_PACER);

The comments in the original code were quite superfluous, but they would 
be useful for the replacement code.  In the above cases you could figure 
it out after looking at the rtd520.h header file where its reasonably 
clear that reading the LAS0_PACER register starts the pacer and writing 
the register (presumably with any value) stops the pacer.  But it's less 
clear in cases such as:

-		RtdAdcConversionSource(dev, 2);	/* BURST triggers ADC */
+		writel(2, devpriv->las0 + LAS0_ADC_CONVERSION);

Since the values for the LAS0_ADC_CONVERSION register are no longer 
documented anywhere.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@....co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-


--
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/