[PATCH 08/11] staging: comedi: s626: make CRA and CRB setup conversions more readable

Ian Abbott abbotti at mev.co.uk
Sat Oct 19 23:43:36 UTC 2013


On 18/10/13 19:24, Hartley Sweeten wrote:
> On Friday, October 18, 2013 6:41 AM, Ian Abbott wrote:
>> Use the new macros defined in "s626.h" for constructing and decomposing
>> 'CRA', 'CRB' and standardized encoder setup values to make the
>> conversions between standardized encoder setup values, and CRA/CRB
>> register values easier to follow.
>>
>> There is some messing about with the 'IndxSrc' values which are 1-bit
>> wide in the standardized encoder setup, and 2-bit wide in the 'CRA' and
>> 'CRB' register values.  This will be addressed by a later patch.
>>
>> Signed-off-by: Ian Abbott <abbotti at mev.co.uk>
>> ---
>>   drivers/staging/comedi/drivers/s626.c | 317 ++++++++++++++++------------------
>>   1 file changed, 151 insertions(+), 166 deletions(-)
>>
>> diff --git a/drivers/staging/comedi/drivers/s626.c b/drivers/staging/comedi/drivers/s626.c
>> index 92062ed..ce320b3 100644
>> --- a/drivers/staging/comedi/drivers/s626.c
>> +++ b/drivers/staging/comedi/drivers/s626.c
>
> [snip]
>
>> @@ -765,50 +766,46 @@ static uint16_t s626_get_mode_b(struct comedi_device *dev,
>>   	 * Populate the standardized counter setup bit fields.
>>   	 * Note: IndexSrc is restricted to ENC_X or IndxPol.
>>   	 */
>> -	setup = ((crb << (S626_STDBIT_INTSRC - S626_CRBBIT_INTSRC_B)) &
>> -		 S626_STDMSK_INTSRC) |		/* IntSrc   = IntSrcB. */
>> -		((crb << (S626_STDBIT_LATCHSRC - S626_CRBBIT_LATCHSRC)) &
>> -		 S626_STDMSK_LATCHSRC) |	/* LatchSrc = LatchSrcB. */
>> -		((crb << (S626_STDBIT_LOADSRC - S626_CRBBIT_LOADSRC_B)) &
>> -		 S626_STDMSK_LOADSRC) |		/* LoadSrc  = LoadSrcB. */
>> -		((crb << (S626_STDBIT_INDXPOL - S626_CRBBIT_INDXPOL_B)) &
>> -		 S626_STDMSK_INDXPOL) |		/* IndxPol  = IndxPolB. */
>> -		((crb >> (S626_CRBBIT_CLKENAB_B - S626_STDBIT_CLKENAB)) &
>> -		 S626_STDMSK_CLKENAB) |		/* ClkEnab  = ClkEnabB. */
>> -		((cra >> ((S626_CRABIT_INDXSRC_B + 1) - S626_STDBIT_INDXSRC)) &
>> -		 S626_STDMSK_INDXSRC);		/* IndxSrc  = IndxSrcB<1>. */
>> +	setup =
>> +		/* IntSrc   = IntSrcB. */
>> +		S626_SET_STD_INTSRC(S626_GET_CRB_INTSRC_B(crb)) |
>> +		/* LatchSrc = LatchSrcB. */
>> +		S626_SET_STD_LATCHSRC(S626_GET_CRB_LATCHSRC(crb)) |
>> +		/* LoadSrc  = LoadSrcB. */
>> +		S626_SET_STD_LOADSRC(S626_GET_CRB_LOADSRC_B(crb)) |
>> +		/* IndxPol  = IndxPolB. */
>> +		S626_SET_STD_INDXPOL(S626_GET_CRB_INDXPOL_B(crb)) |
>> +		/* ClkEnab  = ClkEnabB. */
>> +		S626_SET_STD_CLKENAB(S626_GET_CRB_CLKENAB_B(crb)) |
>> +		/* IndxSrc  = IndxSrcB<1>. */
>> +		S626_SET_STD_INDXSRC(S626_GET_CRA_INDXSRC_B(cra) >> 1);
>>
>>   	/* Adjust mode-dependent parameters. */
>> -	if ((crb & S626_CRBMSK_CLKMULT_B) ==
>> -	    (S626_MULT_X0 << S626_CRBBIT_CLKMULT_B)) {
>> +	cntsrc = S626_GET_CRA_CNTSRC_B(cra);
>> +	clkmult = S626_GET_CRB_CLKMULT_B(crb);
>> +	if (clkmult == S626_MULT_X0) {
>>   		/* Extender mode (ClkMultB == S626_MULT_X0): */
>> -		/* Indicate Extender mode. */
>> -		setup |= S626_ENCMODE_EXTENDER << S626_STDBIT_ENCMODE;
>> +		encmode = S626_ENCMODE_EXTENDER;
>>   		/* Indicate multiplier is 1x. */
>> -		setup |= S626_MULT_X1 << S626_STDBIT_CLKMULT;
>> +		clkmult = S626_MULT_X1;
>>   		/* Set ClkPol equal to Timer count direction (CntSrcB<0>). */
>> -		setup |= (cra >> (S626_CRABIT_CNTSRC_B - S626_STDBIT_CLKPOL)) &
>> -			 S626_STDMSK_CLKPOL;
>> -	} else if (cra & (S626_CNTSRC_SYSCLK << S626_CRABIT_CNTSRC_B)) {
>> +		clkpol = cntsrc & 1;
>> +	} else if (cntsrc & S626_CNTSRC_SYSCLK) {
>>   		/* Timer mode (CntSrcB<1> == 1): */
>> -		/* Indicate Timer mode. */
>> -		setup |= S626_ENCMODE_TIMER << S626_STDBIT_ENCMODE;
>> +		encmode = S626_ENCMODE_TIMER;
>>   		/* Indicate multiplier is 1x. */
>> -		setup |= S626_MULT_X1 << S626_STDBIT_CLKMULT;
>> +		clkmult = S626_MULT_X1;
>>   		/* Set ClkPol equal to Timer count direction (CntSrcB<0>). */
>> -		setup |= (cra >> (S626_CRABIT_CNTSRC_B - S626_STDBIT_CLKPOL)) &
>> -			 S626_STDMSK_CLKPOL;
>> +		setup |= S626_SET_STD_CLKPOL(cntsrc & 1);
>
> Oops missed this.
>
> Based on the other changes I think you want this one to be:
>
> 		clkpol = cntsrc & 1;
>
> [snip]
>
>> +	setup |= S626_SET_STD_ENCMODE(encmode) | S626_SET_STD_CLKMULT(clkmult) |
>> +		 S626_SET_STD_CLKPOL(clkpol);
>
> Otherwise the code above produces this warning:
>
> drivers/staging/comedi/drivers/s626.c: In function 's626_get_mode_b':
> drivers/staging/comedi/drivers/s626.c:805:8: warning: 'clkpol' may be used uninitialized in this function [-Wmaybe-uninitialized]

Well spotted! I don't know how I missed that one. I don't recall seeing 
the compiler warning.


More information about the devel mailing list