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

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

On Tue, 2025-05-20 at 13:13 -0700, Bart Van Assche wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On 5/20/25 6:14 AM, Peter Wang (王信友) wrote:
> > On Tue, 2025-05-20 at 17:40 +0800, Huan Tang wrote:
> > > diff --git a/drivers/ufs/core/ufshcd.c
> > > b/drivers/ufs/core/ufshcd.c
> > > index 3e2097e65964..8ccd923a5761 100644
> > > --- a/drivers/ufs/core/ufshcd.c
> > > +++ b/drivers/ufs/core/ufshcd.c
> > > @@ -8390,6 +8390,10 @@ static int ufs_get_device_desc(struct
> > > ufs_hba
> > > *hba)
> > > 
> > >         dev_info->rtt_cap = desc_buf[DEVICE_DESC_PARAM_RTT_CAP];
> > > 
> > > +       dev_info->hid_sup = get_unaligned_be32(desc_buf +
> > > +
> > > DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP) &
> > > +                               UFS_DEV_HID_SUPPORT;
> > > +
> > > 
> > 
> > Could add the double negation (!!) ensures the value is exactly 0
> > or 1.
> > dev_info->hid_sup = !!(get_unaligned_be32(desc_buf +
> >              DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP) &
> >              UFS_DEV_HID_SUPPORT);
> 
> Hi Peter,
> 
> I do not agree that this change is required. ufs_dev_info.hid_sup has
> type bool and hence the compiler will convert the result of the
> binary
> and operation implicitly into a boolean value (0/1). From the C23
> standard: "When any scalar value is converted to bool, the result is
> false if the value is a zero (for arithmetic types), null (for
> pointer
> types), or the scalar has type nullptr_t; otherwise, the result is
> true."
> 
> A double negation does not occur elsewhere in the kernel if an
> integer
> value is assigned to a boolean variable so I think that it shouldn't
> be
> added here either.
> 
> Bart.

Hi Bart,

Yes, this is not strictly necessary.
For me, this just makes it immediately clear that 
hid_sup is a bool. But I understand that you might 
think it redundant and some people will feel confused 
when reading this code.

It doesn't affect the correctness of this patch, and it is 
totally fine even if no changes are made at all. 
Hence I've added my review tag.

Thanks
Peter