Re: [PATCH 03/15] plugins: adding support for the SIMCom A7605E-H

[Denis Kenzior <denkenz@gmail.com>]

Hi Максим,

On 10/27/23 04:01, Любимов Максим wrote:
> 
> 
> On 18/10/2023 в 20:42 -0500, Denis Kenzior wrote:
>> On 10/17/23 05:48, MaxLyubimov wrote:
>>> The udevng plugin has been modified to detect the SIMCom A7605E-H
>>> modem and the simcom_a76xx plugin has been added to work with it
>>> ---
>>>    plugins/simcom_a76xx.c | 323
>>> +++++++++++++++++++++++++++++++++++++++++
>>>    plugins/udevng.c       |  55 +++++++
>>
>> Please see HACKING, 'Submitting patches' section, item 3.
> 
> I deliberately divided the patch into several parts so that they
> affected only one directory, does this patch only affect the plugins
> directory or did I understand something wrong?

After having another look, please ignore my comment.  You're fine.

<snip>

>>> + *  Copyright (C) 2008-2011  Intel Corporation. All rights
>>> reserved.
>>> + *  Copyright (C) 2009  Collabora Ltd. All rights reserved.
>>> + *  Copyright 2018 Purism SPC
>>
>> Copyrights seem suspect?
> 
> I slightly modified the file plugins/sim7100.c, I left the copyright
> unchanged
> 

Ok, fair enough.

>>
>> In fact this looks 95% the same to plugins/simcom7100.c.  Can we
>> unify the two?
> 
> Simcom modems of the SIM7x00 and A76xx series are built on different
> chips, Qualcomm and ASR, respectively, so their functionality is very
> different. I may try to combine these plugins, so the commands are
> generally compatible. But I don’t have a SIM7x00 series modem, so I
> won’t be able to check the correctness of my modification. There is
> also a question about the name of the plugin, SIM7100 is the name of a
> series of modems, as well as A76xx, how to combine them in the name of
> one plugin?
> 

I would rename this driver to simcom.c and try your best to combine the two. 
Even if you somehow break the 7100, then someone with a 7100 can come in and 
complain.  We'll then fix it.  In fact, I think we even had a simcom.c before.

>>
>> Again, this block seems rather similar to what is in
>> setup_simcom.  Just the
>> interface numbers are different.  Please unify the two.
> 
> I divided the code to make it more readable, since setup_sim7x00
> configures qmi, which is not supported in simcom a76xx, and the
> setup_simcom function, as far as I understand, is not used because
> there is no modem driver named simcom. Which of these setup_sim7x00 or
> setup_simcom functions should I use to add a simcom a76xx?

Long term I think I want to get rid of QMI setup in all vendor setup functions. 
Almost always these are the same across all modems in QMI mode, just the 
interface numbers might be slightly different.  Probably just need to create a 
lookup table instead.

So for now, I would just modify the setup_simcom function with the A76xx details.

Regards,
-Denis