On 31/10/2023 04:47, Jyan Chou [周芷安] wrote: > Hi Krzysztof, > > Thanks for you to review our code and give some advices. We will check carefully and fix coding style of all, > also, we will drop useless function and message in our next push. > Since some advice we were not sure, we would like to discuss first and modified correctly in our next version. > >>> + priv = devm_kzalloc(host->dev, sizeof(struct >>> + dw_mci_rtkemmc_host), GFP_KERNEL); > >> sizeof(*) > > Thanks. I will correct it in our next version. > >>> + priv->pins_default = pinctrl_lookup_state(priv->pinctrl, >>> + PINCTRL_STATE_DEFAULT); >>> + if (IS_ERR(priv->pins_default)) >>> + dev_warn(host->dev, "could not get default state\n"); >>> + > >> So this is required by the driver but not by the bindings. > > Since our pinctrl driver supports different driver's usage, we may use bindings to get the correct pinctrl setting, so we add > different pinctrl-names in our bindings. It is correct, or am I misunderstand what you say? Printing warning means this is required. If driver requires it, so should bindings. > >> dev_err_probe. Everywhere where applicable. > > Thanks. I will correct it in our next version. > >> Read Linux coding style. Multiple times if needed. > > Thanks. I will correct it in our next version. > >>> + host->dev = &pdev->dev; >>> + host->irq_flags = 0; >>> + host->pdata = pdev->dev.platform_data; >>> + >>> + regs = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>> + host->regs = devm_ioremap_resource(&pdev->dev, regs); > >> Use helper for this. > >> Open existing, recent drivers and use the them as template or some set of patterns. Sorry, but upstreaming your vendor code will be painful, because you started from some old, buggier version. > > Sorry for asking, but I would like to know what is the meaning of "Use helper for this." ? Does it mean we need to get rid of our code above or something else? Thanks a lot. There is a helper combining two calls above. Best regards, Krzysztof
