On Thu, 28 Oct 2010 18:30:09 -0700
"Mills, Ken K" <[email protected]> wrote:
>
> These fixes have been verified with the IFX6160 modem.
Most of this looks either right, or in some cases its clearly sorting
something but I am not sure it is doing it the right way, so some
explanation is definitely called for
> +#define GSM_DEFAULT_MRU GSM_DEFAULT_MTU
These seem odd - the default frame size is laid down in the standard
and if I remember rightly is 64 bytes.
> static const struct tty_port_operations gsm_port_ops;
> +static void gsm_dlci_data_kick(struct gsm_dlci *dlci);
Why add this ?
>
> /*
> * CRC table for GSM 0710
> @@ -986,13 +995,23 @@ static void gsm_control_reply(struct gsm_mux
> *gsm, int cmd, u8 *data,
> * layer 2 is processed. Sort out the local modem state and
> throttles */
>
> +#define TWO_OCTETS 0x4000
> static void gsm_process_modem(struct tty_struct *tty, struct
> gsm_dlci *dlci,
> - u32 modem)
> + u32 _modem)
Please don't use _xxx names in variables except for internal compiler
stuff and inline magic
> {
> int mlines = 0;
> - u8 brk = modem >> 6;
> + u8 modem;
> + u8 brk = 0;
> +
> + /* only two octets are defined at this time (control and
> break) */
> + if (_modem & TWO_OCTETS) {
> + modem = (_modem >> 7) & 0x7f;
> + brk = _modem & 0x7f;
> + } else
> + modem = _modem & 0x7f;
This seems odd so some explanation would be good in terms of the spec
> + if (tty && (mlines & TIOCM_RTS)) {
> + pr_debug("wake up port\n");
> + wake_up_interruptible(&dlci->port.open_wait);
RTS is flow control not open control ? I can see why we would want to
wake anyone blocked for write, but not why you want to wake blocked
opens ?
> + }
> }
>
> /**
> @@ -1036,7 +1059,7 @@ static void gsm_process_modem(struct tty_struct
> *tty, struct gsm_dlci *dlci, static void gsm_control_modem(struct
> gsm_mux *gsm, u8 *data, int clen) {
> unsigned int addr = 0;
> - unsigned int modem = 0;
> + unsigned int modem = 1;
This makes no sense - modem is an EA so starts at zero. If you need to
adjust it then something else is up.
>
> /*
> * DLCI level handling: Needs krefs
> */
> @@ -1392,7 +1413,7 @@ static void gsm_dlci_open(struct gsm_dlci *dlci)
> /* This will let a tty open continue */
> dlci->state = DLCI_OPEN;
> if (debug & 8)
> - printk(KERN_DEBUG "DLCI %d goes open.\n",
> dlci->addr);
> + printk(KERN_INFO "DLCI %d goes open.\n", dlci->addr);
KERN_DEBUG but otherwise rather a good idea to add the dlci
> - unsigned int modem = 0;
> + unsigned int modem = 1;
Same comment - an EA starts at zero
> - if (gsm->fcs != GOOD_FCS) {
> + fcs = ~gsm->fcs;
> + if (fcs != gsm->good_fcs) {
Not sure I understand the point of this lot either. The valid FCS value
is defined in the spec and isn't a variable.
> - gsm->encoding = 1;
> - gsm->mru = 64; /* Default to encoding 1 so these
> should be 64 */
> - gsm->mtu = 64;
> + gsm->encoding = 0;
> + gsm->mru = GSM_DEFAULT_MTU;
> + gsm->mtu = GSM_DEFAULT_MTU;
No - set those from user space. The defaults the driver users are taken
from the spec.
> - gsm->encoding = 1;
> + /*gsm->encoding = 1; */
> return gsmld_attach_gsm(tty, gsm);
Ditto
> }
>
> @@ -2377,6 +2426,7 @@ static int gsmld_config(struct tty_struct *tty,
> struct gsm_mux *gsm, gsm->mru = c->mru;
> gsm->encoding = c->encapsulation;
> gsm->adaption = c->adaption;
> + gsm->n2 = c->n2;
Looks right to me.
Alan
_______________________________________________
MeeGo-kernel mailing list
[email protected]
http://lists.meego.com/listinfo/meego-kernel
