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

[NG, TZE YEE]

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@xxxxxxxxxx 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