This repository has been archived by the owner. It is now read-only.

ofono QMI: mainline kernel support #1381

Merged
merged 5 commits into from May 10, 2018

Conversation

Projects
None yet
4 participants
@scintill
Contributor

scintill commented Apr 1, 2018

Tested by @bhush9 on Debian. He sent some SMS!

TODO before this is completely ready

  • write udev rules like described in rpmsgexport, in a new modem-qcom-msm-mainline-common package, which depends on qcom_rmtfs
    • probably need to package rpmsgexport, for above
  • maybe transcribe other channels from the MSM fork (see the array above that for /dev names)? Probably not, if we don't have a need for them

Opening this PR might have been premature since I can't test the full polished solution... I didn't want the work to get lost though.

@bhush9

This comment has been minimized.

Collaborator

bhush9 commented Apr 1, 2018

probably it would be awesome to also include qmimodem call support.. :-)

@scintill

This comment has been minimized.

Contributor

scintill commented Apr 1, 2018

Here's my try at writing udev rules, to automatically create the device node. I'm not very experienced with that, and don't have a mainline device, so it'd be great if someone can test or review.

ACTION=="add", SUBSYSTEM=="rpmsg", \
               KERNEL=="rpmsg_ctrl[0-9]*", \
               ATTRS{rpmsg_name}=="modem", \
               RUN+="rpmsgexport /dev/$name DATA5_CNTL"

SUBSYSTEM=="rpmsg", ATTR{rpmsg_name}=="DATA5_CNTL", \
                    NAME="smdcntl0", \
                    ENV{OFONO_DRIVER}="gobi"

This is maybe controversial, but I've named the device node smdcntl0, like the MSM fork does. ofono doesn't care about the name, but it might be nice to have consistency for use with path-based tools like qmicli.

EDIT: "The name of a device node cannot be changed by udev, only additional symlinks can be created." - udev man page

@ollieparanoid

The change looks good, only missing the pkgrel increase 👍

@ollieparanoid

This comment has been minimized.

Member

ollieparanoid commented Apr 1, 2018

Awesome! 🎉 Added the dont_merge_pr label, as you've listed TODOs that need to be done first.

@scintill scintill changed the title from ofono: mainline kernel support to ofono QMI: mainline kernel support Apr 4, 2018

@scintill

This comment has been minimized.

Contributor

scintill commented Apr 4, 2018

I've pushed some refactoring work, which will hopefully make it easier and cleaner to add this to more devices. I will hopefully finish the mainline kernel pieces in a few days. What I've got here is tested on my i9195.

@ollieparanoid

ollieparanoid approved these changes Apr 4, 2018 edited

Looks good to me, leaving it open until the TODOs you wrote are addressed.
I really like the modem-qcom-msm-downstream-common and modem-qcom-msm-mainline-common solution! 🎉

scintill added some commits Apr 1, 2018

refactor and cleanup modem support
Add modem-qcom-msm-downstream-common package that pulls in the
dependencies.

qcom_rmtfs now has udev rules to find the storage partitions. My hope
is they can be written flexibly enough to cover all devices and kernel
flavors.
@scintill

This comment has been minimized.

Contributor

scintill commented Apr 8, 2018

These last changes should finish it. It's rebased onto master. Can @bhush9 or someone with a mainline device try installing modem-qcom-msm-mainline-common? (and/or add it as a dependency in your device package.) If it works, after booting you should have a /dev/modem symlink that works with qmicli, and ofono should detect the modem.

@ollieparanoid

Code still looks good! Waiting for a test by someone with a mainline kernel before merging, thanks again!

@@ -2,7 +2,7 @@
pkgname=ofono
_upstreamver=1.21
pkgver=1.21_p20180307
pkgrel=0
pkgrel=2

This comment has been minimized.

@ollieparanoid
@opendata26

This comment has been minimized.

Member

opendata26 commented Apr 29, 2018

run command seems wrong gives
[ 31.133508] udevd[781]: failed to execute '/lib/udev/rpmsgexport' 'rpmsgexport /dev/rpmsg_ctrl1 DATA5_CNTL': No such file or directory

@scintill

This comment has been minimized.

Contributor

scintill commented May 8, 2018

Thank you @opendata26. This should fix that RUN issue. I need to test this merged branch on my device too, but I don't think there should be any issues.

@scintill

This comment has been minimized.

Contributor

scintill commented May 8, 2018

It works on my device/kernel.

@bhush9

This comment has been minimized.

Collaborator

bhush9 commented May 10, 2018

I think this is good to merge in master, I've changes for hammerhead on top of this PR, @ollieparanoid @scintill Are you fine with merging this PR?

@ollieparanoid

This comment has been minimized.

Member

ollieparanoid commented May 10, 2018

Fine with me as well, what do you think @scintill?

(Maybe @MartijnBraam can merge this one once @scintill confirms?)

@scintill

This comment has been minimized.

Contributor

scintill commented May 10, 2018

Yes, it's ready.

@ollieparanoid ollieparanoid merged commit 6ff988f into postmarketOS:master May 10, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on master at 78.656%
Details
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.