On 11/6/2021 11:10 AM, Sergey Ryazanov wrote:

Hello Ricardo,

On Mon, Nov 1, 2021 at 6:57 AM Ricardo Martinez
[off-list ref] wrote:

...

Nice work! The driver generally looks good for me. But at the same
time the driver looks a bit raw, needs some style and functionality
improvements, and a lot of cleanup. Please find general thoughts below
and per-patch comments.

Thanks for the feedback Sergey, we will work on it.

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?

Interestingly, that you are using the common 't7xx_' prefix for all
driver file names. This is Ok, but it does not add to the specifics as
all driver files are already located in a common directory with the
specific name. But function names at the same time lack a common
prefix.

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.

Next, the driver entirely lacks the multibyte field endians handling.
Looks like it will be unable to run on a big-endians CPU. To fix this,
it is needed to find all the structures that are passed to the modem
and replace the multibyte fields of types u16/u32 with __le16/__le32.
Then examine all the field accesses and use
cpu_to_le{16,32}()/le{16,32}_to_cpu() to update/read field contents.
As soon as you change the types to __le16/__le32, sparse (a static
analyzing utility) will warn you about every unsafe field access. Just
build your kernel with make C=1.

Ricardo, please consider submitting at the next iteration a patch
series with the driver that will be cleaned from debug stuff and
questionable optimizations. Just a bare minimum functional: AT/MBIM
control ports and network interface for data communications. This will
cut the code in half. What will greatly facilitate the reviewing
process. And then extend the driver functionality with follow up
patches.

--
Sergey