Re: [PATCH v2 1/7] ata: libata: Cleanup libata-transport

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux