Re: [PATCH v4 2/2] scsi: ufs: core: Add support for static TX Equalization settings

[Can Guo]

On 5/29/2026 12:02 AM, Bart Van Assche wrote:
> On 5/28/26 3:06 AM, Can Guo wrote:
> > +static void ufshcd_parse_static_tx_eq_settings(struct ufs_hba *hba)
> > +{
> > +    size_t sz = hba->lanes_per_direction * 2;
>
> Please mark constants with "const". Additionally, is "sz" a good name
> for this variable? The code below compares "count" and "sz". I haven't
> seen it before that a count and a size are compared with each other.
>
> Why "size_t" as data type? u32 should be sufficient, isn't it?
>
> > +    u32 lpd = hba->lanes_per_direction;
>
> Is this another constant?
>
> > +    if (!lpd || lpd > UFS_MAX_LANES)
> > +        return;
>
> Should a kernel warning perhaps be issued if lpd > UFS_MAX_LANES?
>
> > +    for (gear = UFS_HS_G1; gear <= UFS_HS_GEAR_MAX; gear++) {
> > +        snprintf(prop_name, MAX_PROP_SIZE, "txeq-preshoot-g%d", gear);
> > +        count = of_property_count_u32_elems(dev->of_node, prop_name);
> > +        if (count <= 0)
> > +            continue;
>
> The body of this for-loop is long. Please consider moving the body of
> this for-loop into a new function to reduce the indentation level of
> the code.
>
> > +        for (i = 0; i < count; i++) {
> > +            if (preshoot[i] >= TX_HS_NUM_PRESHOOT) {
> > +                dev_err(dev, "An invalid TX EQ PreShoot (%d) 
> > provided in %s property\n",
> > +                    preshoot[i], prop_name);
> > +                break;
> > +            }
> > +        }
> > +
> > +        if (i != count)
> > +            continue;
>
> The traditional way in the Linux kernel for breaking out of a nested
> loop is using a "goto" or "return" statement.
Hi Bart,

Thanks for the review, I will address them in next version.

Can Guo.
> Thanks,
>
> Bart.