Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211101035635.26999-1-ricardo.martinez@linux.intel.com>
 <CAHNKnsSW15BXq7WXmyG7SrrNA+Rqp_bKVneVNrpegJvDrh688Q@mail.gmail.com> <27811a97-6368-dab2-5163-cbd0169b8666@linux.intel.com>
In-Reply-To: <27811a97-6368-dab2-5163-cbd0169b8666@linux.intel.com>
From:   Sergey Ryazanov <ryazanov.s.a@gmail.com>
Date:   Tue, 9 Nov 2021 14:35:40 +0300
Message-ID: <CAHNKnsQ8k-mKCfK2UEiC-EZn13-4VPU3ygoT3a3s4nUw5bHvhQ@mail.gmail.com>
Subject: Re: [PATCH v2 00/14] net: wwan: t7xx: PCIe driver for MediaTek M.2 modem
To:     "Martinez, Ricardo" <ricardo.martinez@linux.intel.com>
Cc:     netdev@vger.kernel.org, linux-wireless@vger.kernel.org,
        Jakub Kicinski <kuba@kernel.org>,
        David Miller <davem@davemloft.net>,
        Johannes Berg <johannes@sipsolutions.net>,
        Loic Poulain <loic.poulain@linaro.org>,
        M Chetan Kumar <m.chetan.kumar@intel.com>,
        chandrashekar.devegowda@intel.com,
        Intel Corporation <linuxwwan@intel.com>,
        chiranjeevi.rapolu@linux.intel.com, haijun.liu@mediatek.com,
        amir.hanania@intel.com,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        dinesh.sharma@intel.com, eliot.lee@intel.com,
        mika.westerberg@linux.intel.com, moises.veleta@intel.com,
        pierre-louis.bossart@intel.com, muralidharan.sethuraman@intel.com,
        Soumya.Prakash.Mishra@intel.com, sreehari.kancharla@intel.com,
        suresh.nagaraj@intel.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Nov 9, 2021 at 8:26 AM Martinez, Ricardo wrote:
> On 11/6/2021 11:10 AM, Sergey Ryazanov wrote:
>> A one nitpick that is common for the entire series. Please consider
>> using a common prefix for all driver function names (e.g. t7xx_) to
>> make them more specific. This should improve the code readability.
>> Thus, any reader will know for sure that the called functions belong
>> to the driver, and not to a generic kernel API. E.g. use the
>> t7xx_cldma_hw_init() name for the  CLDMA initialization function
>> instead of the too generic cldma_hw_init() name, etc.
>
> Does this apply to static functions as well?

As I wrote, this is a nitpick. As you can see in
Documentation/process/coding-style.rst, there are no general rules for
functions naming. My personal rule of thumb is that if  a function
performs a very general operation (like averaging, interpolation,
etc.), then a prefix can be omitted. If a function operation is
specific for a module, then add a common module prefix to the function
name. But again, this is my personal rule.

As for the driver, it was quite difficult to read the code that calls
functions such as cldma_alloc(), cldma_init(). It was hard to figure
out whether these functions are new kernel API or they are specific to
the driver. A common way to solve such ambiguity issues is to prefix
the driver function names. But again, this was just an attempt to draw
your attention to the function naming. Feel free to name functions as
you would like, just make the code readable for developers who are not
familiar with the specific HW chip.

>> Another common drawback is that the driver should break as soon as two
>> modems are connected simultaneously. This should happen due to the use
>> of multiple _global_ variables that keeps pointers to a modem runtime
>> state. Out of curiosity, did you test the driver with two or more
>> modems connected simultaneously?
>
> We haven't tested such configurations, we are focusing on platforms with one single modem.

Now you are aware of the potential kernel crash due to the global
variables misuse. Please fix it.

-- 
Sergey