Messages in this thread | | | Subject | Re: [PATCH 3/3] scsi: ufs-sysfs: Add sysfs entries for temperature notification | From | Bart Van Assche <> | Date | Wed, 1 Sep 2021 09:52:48 -0700 |
| |
On 9/1/21 5:37 AM, Avri Altman wrote: > +What: /sys/bus/platform/drivers/ufshcd/*/attributes/case_rough_temp > +Date: September 2021 > +Contact: Avri Altman <avri.altman@wdc.com> > +Description: The device case rough temperature (bDeviceCaseRoughTemperature > + attribute). It is termed "rough" due to the inherent inaccuracy > + of the temperature sensor inside a semiconductor device, > + e.g. +- 10 degrees centigrade error range.
My understanding is that the word Celsius is more common than centigrade so please use Celsius instead of centigrade. See also https://www.brannan.co.uk/celsius-centigrade-and-fahrenheit/
> + allowable range is [-79..170]. > + The temperature readings are in decimal degrees Celsius. > + > + Please note that the Tcase validity depends on the state of the > + wExceptionEventControl attribute: it is up to the user to > + verify that the applicable mask (TOO_HIGH_TEMP_EN, and / or > + TOO_LOW_TEMP_EN) is set for the exception handling control. > + This can be either done by ufs-bsg or ufs-debugfs.
Instead of making the user verify whether case_rough_temp is valid, please modify the kernel code such that case_rough_temp only reports a value if that value is valid. One possible approach is to make the show method return an error code if case_rough_temp is not valid. Another and probably better approach is to define a sysfs attribute group and to make case_rough_temp visible only if it is valid.
> diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c > index 5c405ff7b6ea..a9abe33c40e4 100644 > --- a/drivers/scsi/ufs/ufs-sysfs.c > +++ b/drivers/scsi/ufs/ufs-sysfs.c > @@ -1047,6 +1047,86 @@ static inline bool ufshcd_is_wb_attrs(enum attr_idn idn) > idn <= QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE; > } > > +static inline bool ufshcd_is_temp_attrs(enum attr_idn idn) > +{ > + return idn >= QUERY_ATTR_IDN_CASE_ROUGH_TEMP && > + idn <= QUERY_ATTR_IDN_LOW_TEMP_BOUND; > +}
Modern compilers are good at deciding when to inline a function so please leave out the 'inline' keyword from the above function.
> +static bool ufshcd_case_temp_legal(struct ufs_hba *hba)\
Please use another word than "legal" since the primary meaning of "legal" is "of or relating to law".
> + ufshcd_rpm_get_sync(hba); > + ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR, > + QUERY_ATTR_IDN_EE_CONTROL, 0, 0, &ee_mask); > + ufshcd_rpm_put_sync(hba);
Are there any ufshcd_query_attr() calls that are not surrounded by ufshcd_rpm_{get,put}_sync()? If not, please move the ufshcd_rpm_{get,put}_sync() calls into ufshcd_query_attr().
Thanks,
Bart.
| |