> > 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@xxxxxxx> > > +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/ Done. > > > + 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. But it does. Just wanted to document that exception control is controlled from user space, And avoid the eyebrow raises when getting invalid temperature reading. >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. Done. > > > +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". Done. > > > + 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(). Will check. Thanks, Avri > > Thanks, > > Bart.
