Messages in this thread | | | Date | Fri, 25 Apr 2014 20:49:01 +0200 | From | Sebastian Reichel <> | Subject | Re: [PATCHv3 10/14] HSI: Introduce driver for SSI Protocol |
| |
On Sat, Apr 19, 2014 at 09:49:36PM +0200, Pavel Machek wrote: > Hi! > > > This adds a driver for the SSI McSAAB protocol as used in > > the Nokia N900. > > > > Signed-off-by: Carlos Chinea <carlos.chinea@nokia.com> > > Signed-off-by: Sebastian Reichel <sre@kernel.org> > > > +#define SSIP_MIN_PN_HDR 6 /* FIXME: Revisit */ > > +#define SSIP_WDTOUT 2000 /* FIXME: has to be 500 msecs> */ > > Can they be fixed now, or do they have to wait?
I would prefer to wait. The timing above works and having it in the git history doesn't hurt.
> > +#ifdef __LITTLE_ENDIAN > > + ((u16 *)skb->data)[2] = swab16(((u16 *)skb->data)[2]); > > + dev_dbg(&dev->dev, "RX length fixed (%04x -> %u)\n", > > + ((u16 *)skb->data)[2], ntohs(((u16 *)skb->data)[2])); > > +#endif > > Uh. Can this be replaced by > ((u16 *)skb->data)[2] = htons(((u16 *)skb->data)[2]); > > (without the ifdef?)
Yes.
> > + /* > > + * Modem sends Phonet messages over SSI with its own endianess... > > + * Assume that modem has the same endianess as we do. > > + */ > > + if (skb_cow_head(skb, 0)) > > + goto drop; > > +#ifdef __LITTLE_ENDIAN > > + ((u16 *)skb->data)[2] = swab16(((u16 *)skb->data)[2]); > > +#endif > > Here, too?
Fixed in PATCHv4.
> But it looks like the comment does not match the code, because we > swap.
The swap only converts the package length field from/to network byte order. I think the comment still applies for the other actual message.
I added a comment for the length field and kept the other comment as is for PATCHv4.
-- Sebastian [unhandled content-type:application/pgp-signature] | |