On 5/21/25 10:56, Tasos Sahanidis wrote:
> On 2025-05-20 12:29, Damien Le Moal wrote:
>> Nit: "it has been renamed" -> "it is renamed"
>
> Will do.
>
>>> @@ -530,13 +534,17 @@ int ata_acpi_cbl_80wire(struct ata_port *ap, const struct ata_acpi_gtm *gtm)
>>> xfer_mask = ata_acpi_gtm_xfermask(dev, gtm);
>>> ata_unpack_xfermask(xfer_mask, NULL, NULL, &udma_mask);
>>>
>>> - if (udma_mask & ~ATA_UDMA_MASK_40C)
>>> - return 1;
>>> + ret = ATA_CBL_PATA40;
>>> +
>>> + if (udma_mask & ~ATA_UDMA_MASK_40C) {
>>> + ret = ATA_CBL_PATA80;
>>
>> Please change this to "return ATA_CBL_PATA80;" and change the last return at the
>> end of the function to "return ATA_CBL_PATA40;". That will be cleaner.
>>
>> Other than these, this looks good.
>>
>
> Apologies, but I am not sure I understand.
>
> Wouldn't changing the last return to ATA_CBL_PATA40 undo the point of
> this change? The function must return ATA_CBL_PATA_UNK if the loop is
> never entered (no enabled devices).
Yes, my bad. That last return should return unknown.
Let me apply the patches and see how it looks. This is all cosmetic anyway.
>
> If it does enter the loop but the mask hasn't matched at all after it
> has gone through all the devices, it must return ATA_CBL_PATA40, which
> is why there's the unconditional assignment ret = ATA_CBL_PATA40. If,
> however, there is at least one "80 wire device", the whole cable is
> considered 80 wire (thus the immediate break in the if).
>
> I believe the most that can be done is:
>
> if (udma_mask & ~ATA_UDMA_MASK_40C)
> return ATA_CBL_PATA80;
>
> with the rest remaining the same. I opted for assignment + break there
> instead of return as it seemed more readable to me, and it's consistent.
--
Damien Le Moal
Western Digital Research
