From: Denis Kenzior <denkenz@gmail.com>
To: Adam Pigg <piggz1@gmail.com>, ofono@lists.linux.dev
Subject: Re: [RFC PATCH] qmimodem voicecall support
Date: Tue, 19 Mar 2024 21:49:14 -0500 [thread overview]
Message-ID: <c54f27a6-5f3f-488f-9970-62c0373370bf@gmail.com> (raw)
In-Reply-To: <5612854.ZASKD2KPVS@adam-laptop-hp>
Hi Adam,
On 3/18/24 17:51, Adam Pigg wrote:
> Hi
>
> As previously mentioned, ive been working on updating Ofono in Sailfish. There
> are several patches which probably are of no interest, but this one may be.
> It implements voicecall support for the qmimodem driver, and ive tested it on
> a PinephonePro, and can make/receive/hangup calls.
>
> I cant take credit for the original implementation, it was taken from other
> ofono forks out there, such as https://github.com/maemo-leste-upstream-forks/
> ofono/commit/9c8ec63c1bb41a03cd87d2733dd83ecd148d0105
>
> The changes I have made include:
> *moving voice_generated.* into voice.*
> This is because it isnt actually generated and reduces the files in the tree
> *removed glib and replaced with ell
> *tweaked the notify code in call_list.c
> Changed the disconnect type from UNKNOWN because in src/voicecall.c, if the
> reason is UNKNOWN then it doesnt then perform the dbus notify
> Made it call ofono_voicecall_disconnected when the state was DISCONNECTED
> (QMI state disconnecting and end)
>
That's an awesome start. Some preliminary comments:
- Since you've made so many changes already, even based on work by other
authors, it would be okay to add your copyright to the added / modified files.
- Coding style and formatting. There's quite a bit of formatting problems in
the file. See oFono's doc/coding-style.txt for our coding style guidelines.
Also, it may be a good idea to run this patch and future submissions through
Linux Kernel's checkpatch.pl script to avoid going through multiple rounds of
reviews due to style issues. Some minor nitpicks I can fix in place (assuming
the author doesn't mind) when applying the patch, but there's too many in this
RFC at the moment.
- Also watch out for mixed tabs and space indentation, > 80 char lines,
redundant double empty lines, etc.
- Redundant changes. Many changes seem to be spurious / redundant, particularly
in voice.h file. This makes the diff bigger for no apparent reason. Can this
be fixed?
- Much of voice.c seems to be not useful. The only user of these functions
would be in the voicecall driver itself. I'd rather not introduce intermediate
APIs for no reason. Could you inline the operations directly in the driver
method implementation? For example, static void dial() invokes
qmi_voice_dial_call(). Just inline the qmi_voice_dial_call() implementation
inside dial().
- I know this is an existing driver, but any chance you could split this up a
bit into multiple patches? Think of it as telling a story / writing a novel.
Introduce new data structures / operations as you use them. One strategy might
be to break this up like this:
- Dial implementation
- Event processing / all_call_status_ind()
- Answer implementation
- Release specific implementation
- hangup active implementation
- DTMF implementation
> The common/call_list.* files can potentially be used by other drivers.
>
Can you put call_list.[ch] into the voicecall.c for now? I understand the
reason for why this was invented / why this exists, but ultimately there's a
much better approach that we can utilize. We'll worry about this a bit later
once the initial set is upstream.
> Apologies for attaching the patch, git send-email is not my normal work flow!
> :)
Please use git send-email for submissions. Patchwork does not work well with
attachments. oFono CI (which is still a bit WIP) doesn't like attachments
either. It is also much easier to comment on patches sent via git send-email
since the patch contents can be easily replied to in-line.
As a side benefit, the CI runs checkpatch, checks compilation on multiple
platforms, etc.
Regards,
-Denis
prev parent reply other threads:[~2024-03-20 2:49 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-18 22:51 [RFC PATCH] qmimodem voicecall support Adam Pigg
2024-03-20 2:49 ` Denis Kenzior [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c54f27a6-5f3f-488f-9970-62c0373370bf@gmail.com \
--to=denkenz@gmail.com \
--cc=ofono@lists.linux.dev \
--cc=piggz1@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).