Re: [PATCH 2/3] mmc: sdhci-dwcmshc: Add Canaan K230 DWCMSHC controller support

[Jiayu Du <jiayu.riscv@isrc.iscas.ac.cn>]

On Sat, Feb 07, 2026 at 10:32:55AM +0100, Krzysztof Kozlowski wrote:
> On 07/02/2026 09:45, Jiayu Du wrote:
> > On Fri, Feb 06, 2026 at 02:26:40PM +0100, Krzysztof Kozlowski wrote:
> >> On 04/02/2026 09:29, Jiayu Du wrote:
> >>> +static int dwcmshc_k230_init(struct device *dev, struct sdhci_host *host,
> >>> +			     struct dwcmshc_priv *dwc_priv)
> >>> +{
> >>> +	static const char * const clk_ids[] = {"base", "timer", "ahb"};
> >>> +	struct device_node *usb_phy_node;
> >>> +	struct k230_priv *k230_priv;
> >>> +	u32 data;
> >>> +	int ret;
> >>> +
> >>> +	k230_priv = devm_kzalloc(dev, sizeof(struct k230_priv), GFP_KERNEL);
> >>> +	if (!k230_priv)
> >>> +		return -ENOMEM;
> >>> +	dwc_priv->priv = k230_priv;
> >>> +
> >>> +	usb_phy_node = of_find_compatible_node(NULL, NULL, "canaan,k230-usb-phy");
> >>
> >> Hm? You should use phandles, not look for various nodes.
> > 
> > Only one usbphy node has the canaan, k230-usb-phy compatibility.
> > So in this situation, is it ok to continue using of_find_compatible_node?
> 
> Amount of nodes does not matter. This is not how you express
> links/dependencies between devices. Phandle is for this. This is wrong
> on many levels, including missing device links, bypassing kernel API/layers.

Thank you for your review. I will fix it.

> 
> 
> > 
> >>> +	if (!usb_phy_node) {
> >>
> >> Please follow Linux coding style.
> > 
> > I will fix it in next version.
> > 
> >>> +		return dev_err_probe(dev, -ENODEV,
> >>> +				     "Failed to find k230-usb-phy node\n");
> >>> +	}
> >>> +
> >>> +	k230_priv->hi_sys_regmap = device_node_to_regmap(usb_phy_node);
> >>> +	of_node_put(usb_phy_node);
> >>> +	if (IS_ERR(k230_priv->hi_sys_regmap)) {
> >>> +		return dev_err_probe(dev, PTR_ERR(k230_priv->hi_sys_regmap),
> >>> +				     "Failed to get k230-usb-phy regmap\n");
> >>> +	}
> >>> +
> >>> +	ret = dwcmshc_get_enable_other_clks(mmc_dev(host->mmc), dwc_priv,
> >>> +					    ARRAY_SIZE(clk_ids), clk_ids);
> >>> +	if (ret) {
> >>> +		return dev_err_probe(dev, ret,
> >>> +				     "Failed to get/enable k230 mmc other clocks\n");
> >>> +	}
> >>> +
> >>> +	if (of_device_is_compatible(dev->of_node, "canaan,k230-sdio")) {
> >>
> >> Driver match data is for this.
> > 
> > What you mean is that I shouldn't use of_find_compatible_node, but I can
> > use device_get_match_data instead? Then I can continue to distinguish
> > between SDIO and eMMC to do parameter configuration
> > 
> > Or do you mean that I should put the parameters to be adjusted into the
> > pdata structure? But currently, the dwcmshc structure is not suitable for
> > containing vendor-specific properties.
> 
> Parameters should go to driver match data. I already requested this for
> some other driver and this has to be fixed.

I will make fix to enable dwcmshc_pltfm_data to support the addition of
vendor-specific properties. And if possible, could you give me with some
examples? I would be very grateful.

> 
> Best regards,
> Krzysztof