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

[Can Guo <can.guo@oss.qualcomm.com>]

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.