Re: [PATCH v5] ufs: core: Add HID support

[Bean Huo <huobean@gmail.com>]

On Wed, 2025-05-21 at 22:32 +0800, Huan Tang wrote:
> > > > +static const char *ufs_hid_state_to_string(enum ufs_hid_state state)
> > > > +{
> > > > +       switch (state) {
> > > > +       case HID_IDLE:
> > > > +               return "idle";
> > > > +       case ANALYSIS_IN_PROGRESS:
> > > > +               return "analysis_in_progress";
> > > > +       case DEFRAG_REQUIRED:
> > > > +               return "defrag_required";
> > > > +       case DEFRAG_IN_PROGRESS:
> > > > +               return "defrag_in_progress";
> > > > +       case DEFRAG_COMPLETED:
> > > > +               return "defrag_completed";
> > > > +       case DEFRAG_NOT_REQUIRED:
> > > > +               return "defrag_not_required";
> > > > +       default:
> > > > +               return "unknown";
> > > > +       }
> > > > +}
> > > 
> > > The enum ufs_hid_state values are contiguous and start from 0, maybe change switch-state to :
> > > 
> > > #define NUM_UFS_HID_STATES 6
> > > static const char *ufs_hid_states[NUM_UFS_HID_STATES] = {
> > >     "idle",
> > >      "analysis_in_progress",
> > >      "defrag_required",
> > >      "defrag_in_progress",
> > >      "defrag_completed",
> > >      "defrag_not_required"
> > > };
> 
> > If this change is made, please use the designated initializer syntax
> > ([label] = "text"). This will make it easier to verify that the
> > enumeration labels and the strings match.
> 
> Hi bart and bean sir,
> 
> Thank you for your comments and guidance！
> 
> What do you think of the following changes?
> 
> ufs.h
>  +/* bHIDState attribute values */                                               
>  +enum ufs_hid_state {                                                           
>  +       HID_IDLE                = 0,                                            
>  +       ANALYSIS_IN_PROGRESS    = 1,                                            
>  +       DEFRAG_REQUIRED         = 2,                                            
>  +       DEFRAG_IN_PROGRESS      = 3,                                            
>  +       DEFRAG_COMPLETED        = 4,                                            
>  +       DEFRAG_NOT_REQUIRED     = 5,                                            
>  +       NUM_UFS_HID_STATES      = 6,                                            
>  +}; 
> 
> ufs-sysfs.c
>  +static const char * const ufs_hid_states[] = {                                 
>  +       [HID_IDLE]              = "idle",                                       
>  +       [ANALYSIS_IN_PROGRESS]  = "analysis_in_progress",                       
>  +       [DEFRAG_REQUIRED]       = "defrag_required",                            
>  +       [DEFRAG_IN_PROGRESS]    = "defrag_in_progress",                         
>  +       [DEFRAG_COMPLETED]      = "defrag_completed",                           
>  +       [DEFRAG_NOT_REQUIRED]   = "defrag_not_required",                        
>  +};                                                                             
>  +                                                                               
>  +static const char *ufs_hid_state_to_string(enum ufs_hid_state state)           
>  +{                                                                              
>  +       if (state < NUM_UFS_HID_STATES)                                         
>  +               return ufs_hid_states[state];                                   
>  +                                                                               
>  +       return "unknown";                                                       
>  +} 
> 
> ...
> 
> +static ssize_t state_show(struct device *dev,
> +               struct device_attribute *attr, char *buf)
> +{
> +       struct ufs_hba *hba = dev_get_drvdata(dev);
> +       u32 value;
> +       int ret;
> +
> +       ret = hid_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
> +                       QUERY_ATTR_IDN_HID_STATE, &value);
> +       if (ret)
> +               return ret;
> +
> +       return sysfs_emit(buf, "%s\n", ufs_hid_state_to_string(value));
> +}
> 
> Thanks
> Huan
> 
> 
> 


looks good to me, with this change, please add my reviewed-by tag in the next version.

Kind regards,
Bean