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