Re: [PATCH] ata: pata_sil680: fix result type of sil680_sel{dev|reg}()

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

On 4/12/22 05:42, Sergey Shtylyov wrote:
> Hello!
> 
> On 4/11/22 2:47 AM, Damien Le Moal wrote:
> 
>>> sil680_sel{dev|reg}() return a PCI config space address but needlessly
>>> use the *unsigned long* type for that,  whereas the PCI config space
>>> accessors take *int* for the address parameter.  Switch these functions
>>> to returning *int*, updating the local variables at their call sites.
>>> Add the empty lines after some declarations, while at it...
>>>
>>> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
>>> analysis tool.
>>>
>>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>>>
>>> ---
>>> This patch is against the 'for-next' branch of Damien Le Moal's 'libata.git'
>>> repo.
>>>
>>>  drivers/ata/pata_sil680.c |   27 +++++++++++++++------------
>>>  1 file changed, 15 insertions(+), 12 deletions(-)
>>>
>>> Index: libata/drivers/ata/pata_sil680.c
>>> ===================================================================
>>> --- libata.orig/drivers/ata/pata_sil680.c
>>> +++ libata/drivers/ata/pata_sil680.c
>>> @@ -47,9 +47,10 @@
>>>   *	criticial.
>>>   */
>>>  
>>> -static unsigned long sil680_selreg(struct ata_port *ap, int r)
>>> +static int sil680_selreg(struct ata_port *ap, int r)
>>>  {
>>> -	unsigned long base = 0xA0 + r;
>>> +	int base = 0xA0 + r;
>>> +
>>>  	base += (ap->port_no << 4);
>>>  	return base;
>>
>> The variable "base" is rather useless here... A simple:
>>
>> 	return 0xA0 + r + (ap->port_no << 4);
>>
>> would work too and is a lot cleaner.
> 
>    Yes, probably... but it's a matter of a separate patch, I think.
> Note that both functions are inlined by gcc.

No need for a separate patch. Your patch already changes the base variable
type. It may as well remove it :)

> 
> [...]
> 
> MBR, Sergey


-- 
Damien Le Moal
Western Digital Research