RE: [PATCH 00/83] staging: comedi: rtd520: initial cleanup

[H Hartley Sweeten <hartleys@visionengravers.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2806 bytes --]

On Wednesday, July 11, 2012 3:20 AM, Ian Abbott wrote:
> 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.
> 

Sorry about that. I meant to leave some of the comments as appropriate
but overlooked them during the cleanup.

Some of the magic write values are actually documented in the header with
#define'd values. But, the header is a bit of a mess and it's hard to determine
which bit values go with which register.

The header really needs to be cleaned up and then merged directly into the
source. It's only used by this driver so the defines shouldn't be exposed for
external use. I located the manual for the device and plan on doing that cleanup
eventually.

Regardless, thanks for adding back the comments. I'll look them over and
give you a Reviewed-by shortly.

Thanks,
Hartley

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥