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).
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.
