Re: [RFC] ifxmodem: integrating new kernel-space MUX driver
Hi Marcel,
I am doing just a basic review here to get things going into the
right
direction.
I'll try to hash this out a bit, so maybe there won't many patch
re-submissions...
> + gboolean use_ofono_mux;
This is the thing I'd rather not do. We can just have a second GPRS
context driver. Not sure about the naming, but one could be "ifxmodem"
and the other "ifxmodem-internal" or similar. Maybe someone comes up
with some better names.
We had little internal discussion about this. We would like to use the
same gprs context driver instead, just as proposed. however, if you
insist on another gprs context driver ... Let me know what you think.
> + char if_name[IF_NAMESIZE];
This should not be needed. You can just reference the stack variable.
Ok.
> +
> +#define GSMIOC_ENABLE_NET _IOW('G', 2, struct gsm_netconfig)
> +#define GSMIOC_DISABLE_NET _IO('G', 3)
> +
This should be in drivers/ifxmodem/gsmmux.h and be a literal copy from
what the kernel has. Check STE and how it does it for CAIF.
Ok, I'll pull gsmmux.h into patch submission.
> chat = g_at_chat_get_slave(gcd->chat);
> @@ -475,6 +549,10 @@ static void ifx_gprs_context_remove(struct ofono_gprs_context
*gc)
> g_at_chat_resume(gcd->chat);
> }
>
> + if (gcd->state != STATE_IDLE && gcd->use_ofono_mux == FALSE) {
> + g_at_chat_resume(gcd->chat);
> + }
> +
What is this trying to fix? Either it is a general bug or not. Seems
more like a general bug that we tie rawip existence with the state.
It was just resuming chat when kernel mux was used.
Check if remove() will be actually ever called without calling
deactivate() first. It might be well that this is an oversight on my
part.
I'll test this.
> + int ret;
As a general rule I prefer err as variable name.
Sure.
> + ofono_error("Failed to get configuration on n_gsm
multiplexer: %d", ret);
> + goto error;
You forgot to restore the line discipline here.
Right, I'll inject this into the patch in those places where we can have
error... and watch for re-base.
There is updated patch in the works for MUX itself, as you know, once
it's in the kernel, I'll resubmit updated patch.
Thanks,
-- r.