linux-kernel - Re: [PATCH V4][3/4] mmc: Add dw mobile mmc cmdq rtk driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9a003bb8-f4da-4ae5-9597-647474f05ad2@linaro.org>
Date:   Tue, 31 Oct 2023 06:01:01 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Jyan Chou [周芷安] <jyanchou@...ltek.com>,
        "ulf.hansson@...aro.org" <ulf.hansson@...aro.org>,
        "adrian.hunter@...el.com" <adrian.hunter@...el.com>,
        "jh80.chung@...sung.com" <jh80.chung@...sung.com>,
        "riteshh@...eaurora.org" <riteshh@...eaurora.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>,
        "conor+dt@...nel.org" <conor+dt@...nel.org>,
        "asutoshd@...eaurora.org" <asutoshd@...eaurora.org>,
        "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>
Cc:     "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "arnd@...db.de" <arnd@...db.de>,
        "briannorris@...omium.org" <briannorris@...omium.org>,
        "doug@...morgal.com" <doug@...morgal.com>,
        "tonyhuang.sunplus@...il.com" <tonyhuang.sunplus@...il.com>,
        "abel.vesa@...aro.org" <abel.vesa@...aro.org>,
        "william.qiu@...rfivetech.com" <william.qiu@...rfivetech.com>
Subject: Re: [PATCH V4][3/4] mmc: Add dw mobile mmc cmdq rtk driver

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