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

[Bart Van Assche <bvanassche@acm.org>]

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.

Thanks,

Bart.