On 2/17/22 18:29, Sergey Shtylyov wrote:
> Hello!
>
> On 2/17/22 3:22 AM, Damien Le Moal wrote:
> [...]
>>> In ata_sff_dev_classify(), replace a string of the *if* statements checking
>>> the device's class with the *switch* statement that fits better here...
>>>
>>> Signed-off-by: Sergey Shtylyov <s.shtylyov@xxxxxx>
>>>
>>> ---
>>> This patch is against the 'for-next' branch of Damien Le Moal's 'libata.git'
>>> repo.
>>>
>>> drivers/ata/libata-sff.c | 14 ++++++++------
>>> 1 file changed, 8 insertions(+), 6 deletions(-)
>>>
>>> Index: libata/drivers/ata/libata-sff.c
>>> ===================================================================
>>> --- libata.orig/drivers/ata/libata-sff.c
>>> +++ libata/drivers/ata/libata-sff.c
>>> @@ -1841,8 +1841,8 @@ unsigned int ata_sff_dev_classify(struct
>>>
>>
>> The err check before the hunk below could use a switch too.
>
> I have initially converted that one too but finally decided to keep
> the order of the comparisons intact -- it makes more sense to 1st check
> dev->devno in the last *if*...
Yeah. Not critical. Just tried to use a switch and that does not make
the code very clear anyway :)
>
>>
>>> /* determine if device is ATA or ATAPI */
>>
>> This comment is a bit weird since ATA_DEV_ATAPI is not used. Maybe
>
> Why? A call ata_port_classify() should detect the ATAPI devices,
> we just don't "post-process" that result...
>
>> change that to something like:
>>
>> /* Check the device class */
>
> No, I don't agree here. :-)
Matter of taste I guess. I do find that comment useless anyway as the
code is fairly obvious. Leave it as-is. No problem.
>
>>
>> Or just drop it... The code is clear enough I think.
>>
>>> class = ata_port_classify(ap, &tf);
>>> -
>>> - if (class == ATA_DEV_UNKNOWN) {
>>> + switch (class) {
>>> + case ATA_DEV_UNKNOWN:
>>> /* If the device failed diagnostic, it's likely to
>>
>> While at it, please correct the comment style here (start with a "/*"
>> line). There are a ton of these style problems all over, so when
>> touching code around them, let's fix them :)
>
> OK. :-)
>
> [...]
>
> MBR, Sergey
--
Damien Le Moal
Western Digital Research
