On 9/2/24 15:16, Hannes Reinecke wrote:
>> /*
>> * ATA device attributes
>> */
>> @@ -660,14 +525,14 @@ static int ata_tdev_match(struct attribute_container *cont,
>> }
>>
>> /**
>> - * ata_tdev_free -- free a ATA LINK
>> - * @dev: ATA PHY to free
>> + * ata_tdev_free -- free a transport ATA device structure
>
> Odd wording; maybe 'ATA transport device' ?
Indeed.
>
>> + * @dev: target ATA device
>
> Why 'target ATA device'? Wouldn't 'ATA transport device' be better?
Because that function really takes a pointer to struct ata_dev, not to struct
ata_dev->tdev...
>
>> *
>> - * Frees the specified ATA PHY.
>> + * Free the transport ATA device structure for the specified ATA device.
>
> Same here.
>
>> *
>> * Note:
>> - * This function must only be called on a PHY that has not
>> - * successfully been added using ata_tdev_add().
>> + * This function must only be called on a device that has not successfully
>
> 'device'? Shouldn't we not use the same wording as in the description?
Not really. Here the reference is to the struct ata_device. Will clarify that.
>
>> + * been added using ata_tdev_add().
>> */
>> static void ata_tdev_free(struct ata_device *dev)
>> {
>> @@ -676,10 +541,10 @@ static void ata_tdev_free(struct ata_device *dev)
>> }
>>
>> /**
>> - * ata_tdev_delete -- remove ATA device
>> - * @ata_dev: ATA device to remove
>> + * ata_tdev_delete -- remove an ATA device sysfs entry
>> + * @ata_dev: target ATA device
>> *
>
> And here we should be consistent with whatever wording we've been using
> in ata_tdev_free().
Yep, will fix.
>> +/**
>> + * ata_is_link -- check if a struct device represents a ATA link
>> + * @dev: device to check
>> + *
>> + * Returns:
>> + * %1 if the device represents a ATA link, %0 else
>> + */
>> +static int ata_is_link(const struct device *dev)
>
> Why is this not a boolean ?
It was like that. I can make it a boolean.
>
>> +{
>> + return dev->release == ata_tlink_release;
>> +}
>> +
>> +static int ata_tlink_match(struct attribute_container *cont,
>> + struct device *dev)
>> +{
>> + struct ata_internal *i = to_ata_internal(ata_scsi_transport_template);
>> +
>> + if (!ata_is_link(dev))
>> + return 0;
>> + return &i->link_attr_cont.ac == cont;
>> +}
>> +
>> +/**
>> + * ata_tlink_delete -- remove ATA LINK
>> + * @link: ATA LINK to remove
>
> Why is the 'link' in capital letters?
No idea. It was like that. Will clean that up as well.
>> + *
>> + * Initialize an ATA LINK structure for sysfs. It will be added in the
>> + * device tree below the ATA PORT it belongs to.
>> + *
>> + * Returns %0 on success
>
> And what on failure?
Another one that I did not touch but clearly needs cleanup too :)
--
Damien Le Moal
Western Digital Research
