Re: [PATCH v2 4/4] mmc: sdhci-cadence6: read SD6HC DDR50 tuning from device tree

["NG, TZE YEE" <tze.yee.ng@altera.com>]

Hi Adrian,

Thanks for the review. All three will be addressed in v3:

On 29/10/2026 9:25 pm, Adrian Hunter wrote:
> On 09/10/2026 05:14, tze.yee.ng@altera.com wrote:
>> +	if ((phy->mode == MMC_TIMING_UHS_DDR50 ||
>> +	     phy->mode == MMC_TIMING_MMC_DDR52) && phy->ddr_read_dqs_delay_ps) {
> 
> The same (phy->mode == MMC_TIMING_UHS_DDR50 || phy->mode == MMC_TIMING_MMC_DDR52)
> appears 4 times.  Might be neater to assign it to a local variable.
> 

Agreed. I will move this check to a local "bool ddr" and used in all 
four places.

>> +	/*
>> +	 * Optional read-path tuning for the extended-read DDR modes (SD DDR50,
>> +	 * eMMC DDR52).
>> +	 */
>> +	phy->ddr_read_dqs_delay_ps = 0;
>> +	of_property_read_u32(dev->of_node, "cdns,ddr-read-dqs-delay-ps",
>> +			     &phy->ddr_read_dqs_delay_ps);
>> +	if (phy->ddr_read_dqs_delay_ps / phy->delay_element_org >
>> +	    FIELD_MAX(SDHCI_CDNS6_PHY_DLL_SLAVE_CTRL_READ_DQS_DELAY))
> 
> This is still effectively validating the firmware values
> 

I will drop the probe-time range check and dev_warn entirely.

>> +		dev_warn(dev,
>> +			 "cdns,ddr-read-dqs-delay-ps %u too large, clamping to %lu elements\n",
>> +			 phy->ddr_read_dqs_delay_ps,
>> +			 (unsigned long)FIELD_MAX(SDHCI_CDNS6_PHY_DLL_SLAVE_CTRL_READ_DQS_DELAY));
>> +
>> +	phy->ddr_no_lpbk_dqs = of_property_read_bool(dev->of_node,
>> +						     "cdns,ddr-no-lpbk-dqs");
> 
> Unneeded line wrapping - can use 100 cols
> 

I will change both of_property_read*() calls in single line.

>> +
>> +	phy->ddr_phony_dqs_timing = -1;
>> +	if (!of_property_read_u32(dev->of_node, "cdns,ddr-phony-dqs-timing",
>> +				  &prop))
> 
> Unneeded line wrapping - can use 100 cols
> 
>> +		phy->ddr_phony_dqs_timing = prop;
>> +
>>   	priv->phy = phy;
>>   
>>   	return 0;
> 

Thanks,
Tze Yee