Re: [PATCH 2/5] ata: libata-core: Refactor force_tbl definition

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 4/25/22 02:53, Sergey Shtylyov wrote:
> On 4/7/22 3:37 PM, Damien Le Moal wrote:
> 
>> Introduce the macro definitions force_cbl(), force_spd_limit(),
>> force_xfer(), force_lflag_on(), force_horkage_on() and
>> force_horkage_onoff() to define with a more compact syntax the struct
>> ata_force_param entries in the force_tbl array defined in the function
>> ata_parse_force_one().
>> To reduce the indentation of the array declaration, force_tbl definition
>> is also moved out of the ata_parse_force_one() function.
> 
>    Some entries are reordered too... :-)
> 
>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> 
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index bc59c77b99b6..c0cf42ffcc38 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -6143,67 +6143,90 @@ int ata_platform_remove_one(struct platform_device *pdev)
>>  EXPORT_SYMBOL_GPL(ata_platform_remove_one);
>>  
>>  #ifdef CONFIG_ATA_FORCE
>> +
>> +#define force_cbl(name, flag)				\
>> +	{ # name,	.cbl		= (flag) }
> 
>    Why not #name here and below?
> 
>> +
>> +#define force_spd_limit(spd, val)			\
>> +	{ # spd,	.spd_limit	= (val) }
>> +
>> +#define force_xfer(mode, shift)				\
>> +	{ # mode,	.xfer_mask	= (1UL << (shift)) }
>> +
>> +#define force_lflag_on(name, flags)			\
> 
>    Not just force_lflag()?

Patch 3 adds force_lflag_onoff(), so I added the on version here. Will
move this change to patch 3.

> 
>> +	{ # name,	.lflags		= (flags) }
>> +
>> +#define force_horkage_on(name, flag)			\
>> +	{ # name,	.horkage_on	= (flag) }
>> +
>> +#define force_horkage_onoff(name, flag)			\
>> +	{ "no" # name,	.horkage_on	= (flag) },	\
>> +	{ # name,	.horkage_off	= (flag) }
>> +
> [...]
>> @@ -6285,7 +6308,7 @@ static void __init ata_parse_force_param(void)
>>  	int last_port = -1, last_device = -1;
>>  	char *p, *cur, *next;
>>  
>> -	/* calculate maximum number of params and allocate force_tbl */
>> +	/* Calculate maximum number of params and allocate ata_force_tbl */
> 
>    Drove-by change? :-)

Yeah, just a comment bug I caught along the way. I do not think it
deserves its own patch. Will mention it in the commit message.

> 
> [...]
> 
> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> 
> MBR, Sergey


-- 
Damien Le Moal
Western Digital Research