Make qcom-msim plugin able to use other SIM slot than first SIM slot.

[peat-psuwit]

This is improvement of qcom-msim plugin that allow you to use other SIM slot and be able to select which SIM will prefer 3G. This modem has limitation that only 1 SIM can prefer 3G at a time, so the new radio-settings driver will make sure that other SIMs prefer 2G before setting a SIM to prefer 3G. I code this to support more than 2 SIM slots, but there are some constants which assume that only 2 SIM slots are available. If, in the future, there are phones with more than 2 SIM slots, these constants can be edited to support them.

[alfonsosanchezbeato]

Please keep this as "GRil" instead of "struct _GRil"

[alfonsosanchezbeato]

This should be defined in a header specific to your driver, something like drivers/qcommsimmodem/qcomutil.h, so you can share with other atoms if/when needed.

[alfonsosanchezbeato]

If you need to export this, please change the name to ofono_radio_set_rat_mode()

[alfonsosanchezbeato]

The main issue I see is that the changes to plugins/qcom-msim.c and plugins/ril.* are not needed. I would rather change "create_gril" in ril.c to support multi-sim, which is simply a matter of retrieving the slot number and using it in a couple of places. Having one slot would still work. To create your specific radio-settings driver you can compare rd->vendor to OFONO_RIL_VENDOR_QCOM_MSIM and use RILMODEM or QCOMMSIMMODEM depending on that.

Beside that, in general looks great to me.

[peat-psuwit]

Ok, new commits pushed.

[alfonsosanchezbeato]

This should be renamed to something like QCOMMSIM_NUM_SLOTS

[alfonsosanchezbeato]

Defining the number of slots here makes sense, but I do not see the reason for defining in the header qcom_msim_pending_pref_setting and qcom_msim_set_2g_rat, which would be used only in the RS atom

[peat-psuwit]

Then there would be only SIM slots amount defined in this file. Maybe I should move this constant to another file. qcom_msim_constants.h?

[alfonsosanchezbeato]

I think that can be fine too

[alfonsosanchezbeato]

Remove the initialization or use { NULL } instead (you need to set the default value for the first array element, letting empty braces do the same is a gcc extension iirc)

[alfonsosanchezbeato]

This implementation is the same as the rilmodem one. Please declare ril_query_modem_rats in "drivers/rilmodem/radio-settings.h" and use it instead.

[alfonsosanchezbeato]

You do not need the "2" in these two definitions.

[tonyespy]

This looks pretty good, however in addition to a couple of in-line comments, there are a few issues with the commits themselves.

First, ofono uses the 50/72 rule for commit short/long descriptions.

Please revise:

• "rilmodem: declare ril_query_modem_rats..."; I would suggest:

"rilmodem: export ril_query_modem_rats
Export ril_query_modem_rats in radio_settings header
so that it can be used by other rilmodem-based drivers."

• qcommsimmodem: Add new radio-settings...."; I would suggest:

  "qcommsimmodem: Add radio-settings atom

  This atom ensures that only one modem slot can be
  used for 3g at any given time. When a modem slot's
  radio technology is set to 3g, this atom will set the other
  slot to 2g first."

Also, please split out the build change from the qcommsimmodem commit. It should be a subsequent commit which enables the new directory in the Makefile. Eg.

build: enable qcommsimmodem radio-settings atom

Finally, does your device support LTE, or is it a 3g only device?

[tonyespy]

I think I'd prefer to see this wrapped after the =, so it'd look like this:

struct qcom_msim_pending_pref_setting *pps =
                g_try_new0(struct qcom_msim_pending_pref_setting, 1); 

[tonyespy]

I think you need to free pps too, otherwise you have a leak.

[tonyespy]

You might want to change this to temp_rd, as it confused me because it's not really the current_rd ( ie. you return if multisim_rs[i] == rs ). Maybe temp_rd would be a better name?

[tonyespy]

I would change this to multisim_num_slots.

[peat-psuwit]

@tonyespy My device, LG L90 Dual, is 3G only device. But I think other dual SIM with LTE device from Qualcomm will benefit from this too.

[tonyespy]

The above line doesn't need to be wrapped; un-wrapped it's exactly 80 characters.

[tonyespy]

Please wrap the above line after the "=". It's easier to read. You'll find plenty of wraps after "=", but zero function calls ( at least that I could find ) with the first parameter wrapped like this.

[tonyespy]

Please put "temp_rd->ril," on the same line as the function name, as it fits within the 80 char limit and again prevents a line ending with "(".

[tonyespy]

This really should be plural... multisim_num_slots, as that's what it is, the number of slots.

[tonyespy]

Thanks for re-working the commit messages and addressing my comments. I still have three more formatting issues ( see inline comments ) that need fixing, and then it'll be good to merge.

[tonyespy]

Actually one other comment. I asked about whether or not your device supported LTE, as your code as written makes the same assumption about LTE as it does 3G with respect to slot capabilities. If the user sets the radio tech of one of the slots/modems to LTE, then the other slot is set first to 2G ( same as for 3G ), but it might be possible for one slot to do LTE and the other 3G. I guess we won't know till we see a QCom dual-SIM that supports LTE, so let's leave this as is.

[tonyespy]

@ratchanan

Please add a comment whenever you update, as it makes it a bit easier to notice.

Thanks for the last round of changes... It's almost merge-able, however when you changed the multisim_num_slots variable, you only changed its declaration, not it's usage elsewhere in the file, so the code doesn't compile.

[peat-psuwit]

@tonyespy Ok, new version pushed. It was shameful to forget that. I'll keep that as a lesson.

[tonyespy]

Approved.