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

["Peter Wang (王信友)" <peter.wang@mediatek.com>]

On Wed, 2025-05-21 at 22:33 +0800, Huan Tang wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> > > +static const char * const hid_trigger_mode[] = {"disable",
> 
> > > "enable"};
> 
> > > +
> 
> > > +static ssize_t analysis_trigger_store(struct device *dev,
> 
> > > +               struct device_attribute *attr, const char *buf,
> 
> > > size_t count)
> 
> > > +{
> 
> > > +       struct ufs_hba *hba = dev_get_drvdata(dev);
> 
> > > +       int mode;
> 
> > > +       int ret;
> 
> > > +
> 
> > > +       mode = sysfs_match_string(hid_trigger_mode, buf);
> 
> > > +       if (mode < 0)
> 
> > > +               return -EINVAL;
> 
> > > 
> 
> > 
> 
> > Hi Haun,
> 
> > 
> 
> > Consider use below coding style for readability.
> 
> > 
> 
> > if (sysfs_streq(buf, "enable"))
> 
> >     mode = ...;
> 
> > else if (sysfs_streq(buf, "disable"))
> 
> >     mode = ...;
> 
> > else
> 
> >    return -EINVAL;
> 
> 
> 
> Hi peter sir,
> 
> 
> 
> Thank you for your comments and guidance��?
> I think your modification will indeed improve the readability of the
> code.
> 
> What do you think of the following changes?
> 
> 
> 
> ufs-sysfs.c
> 
> +static ssize_t analysis_trigger_store(struct device *dev,
> 
> +               struct device_attribute *attr, const char *buf,
> size_t count)
> 
> +{
> 
> +       struct ufs_hba *hba = dev_get_drvdata(dev);
> 
> +       int mode;
> 
> +       int ret;
> 
> +
> 
> +       if (sysfs_streq(buf, "enable"))
> 
> +               mode = HID_ANALYSIS_ENABLE;
> 
> +       else if (sysfs_streq(buf, "disable"))
> 
> +               mode = HID_ANALYSIS_AND_DEFRAG_DISABLE;
> 
> +       else
> 
> +               return -EINVAL;
> 
> +
> 
> +       ret = hid_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
> 
> +                       QUERY_ATTR_IDN_HID_DEFRAG_OPERATION, &mode);
> 
> +
> 
> +       return ret < 0 ? ret : count;
> 
> +}
> 
> +
> 
> +static DEVICE_ATTR_WO(analysis_trigger);
> 
> +
> 
> +static ssize_t defrag_trigger_store(struct device *dev,
> 
> +               struct device_attribute *attr, const char *buf,
> size_t count)
> 
> +{
> 
> +       struct ufs_hba *hba = dev_get_drvdata(dev);
> 
> +       int mode;
> 
> +       int ret;
> 
> +
> 
> +       if (sysfs_streq(buf, "enable"))
> 
> +               mode = HID_ANALYSIS_AND_DEFRAG_ENABLE;
> 
> +       else if (sysfs_streq(buf, "disable"))
> 
> +               mode = HID_ANALYSIS_AND_DEFRAG_DISABLE;
> 
> +       else
> 
> +               return -EINVAL;
> 
> +
> 
> +       ret = hid_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
> 
> +                       QUERY_ATTR_IDN_HID_DEFRAG_OPERATION, &mode);
> 
> +
> 
> +       return ret < 0 ? ret : count;
> 
> +}
> 
> 
> 
> 
> 
> Thanks
> 
> Huan
> 

Hi Huan,

looks good to me.

Thanks
Peter