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
