On Sunday 19 February 2006 16:55, Martin Michlmayr wrote: > * Martin Michlmayr <tbm@xxxxxxxxxx> [2006-02-19 21:15]: > > Can you please review and/or merge Skylark's IOC3 patch from > > ftp://ftp.linux-mips.org/pub/linux/mips/people/skylark/linux-mips-2.6.14-ioc3-r26.patch.bz2 > > > > From my basic understanding I believe that this patch needs to be split up > > and submitted to different sub-system maintainers. > > (Dmitry, this is only a RFC for now since the main support patch > for IOC3 has not been merged yet.) > OK... I have some comments, mostly cosmetic. > + > +struct ioc3kbd_data { > + struct ioc3_driver_data *idd; > + struct serio *kbd,*aux; > +}; Space after comma please. > + > +static int ioc3kbd_write(struct serio *dev, unsigned char val) > +{ > + struct ioc3kbd_data *d = (struct ioc3kbd_data *)(dev->port_data); Unneeded parens... > + unsigned mask; > + unsigned long timeout=0; Missing spaces around operators... > + > + mask = (dev==d->aux) ? KM_CSR_M_WRT_PEND : KM_CSR_K_WRT_PEND; Missing spaces around operators... > + while((d->idd->vma->km_csr & mask) && (timeout<1000)) { Missing a space after operator and unneeded parens again. > + if(dev==d->aux) > + d->idd->vma->m_wd=((unsigned)val)&0x000000ff; > + else > + d->idd->vma->k_wd=((unsigned)val)&0x000000ff; > + > + if(timeout>=1000) Missing spaces around operators... > + > +static int ioc3kbd_open(struct serio *dev) > +{ > + return 0; > +} > + > +static void ioc3kbd_close(struct serio *dev) > +{ > +} You don't need to implement dummy functions, serio core can live without these. > + > +static struct ioc3kbd_data * __init ioc3kbd_allocate_port(int idx, struct ioc3_driver_data *idd) > +{ > + struct serio *sk, *sa; > + struct ioc3kbd_data *d; > + > + sk = kmalloc(sizeof(struct serio), GFP_KERNEL); > + sa = kmalloc(sizeof(struct serio), GFP_KERNEL); kzalloc()? You fill them with 0 later anyway. > + d = kmalloc(sizeof(struct ioc3kbd_data), GFP_KERNEL); > + if (sk && sa && d) { > + memset(sk, 0, sizeof(struct serio)); > + sk->id.type = SERIO_8042; > + sk->write = ioc3kbd_write; > + sk->open = ioc3kbd_open; > + sk->close = ioc3kbd_close; > + snprintf(sk->name, sizeof(sk->name), "IOC3 keyboard %d", idx); > + snprintf(sk->phys, sizeof(sk->phys), "ioc3/serio%dkbd", idx); > + sk->port_data = d; > + sk->dev.parent = &(idd->pdev->dev); > + memset(sa, 0, sizeof(struct serio)); > + sa->id.type = SERIO_8042; > + sa->write = ioc3kbd_write; > + sa->open = ioc3kbd_open; > + sa->close = ioc3kbd_close; > + snprintf(sa->name, sizeof(sa->name), "IOC3 auxiliary %d", idx); > + snprintf(sa->phys, sizeof(sa->phys), "ioc3/serio%daux", idx); > + sa->port_data = d; > + sa->dev.parent = &(idd->pdev->dev); > + d->idd = idd; > + d->kbd = sk; > + d->aux = sa; > + return d; > + } Need to free() allocated stuff here in case it was not the first allocation that failed. > + return NULL; > +} > + > +static int ioc3kbd_probe(struct ioc3_submodule *is, struct ioc3_driver_data *idd) __devinit perhaps? > +{ > + struct ioc3kbd_data *d; > + if(idd->class != IOC3_CLASS_BASE_IP30 && idd->class != IOC3_CLASS_CADDUO) > + return 1; Usually failure indicator is negative. Serio core does not really care but people expect to see either -ENODEV or at least -1. > + d = ioc3kbd_allocate_port(idd->id, idd); > + idd->data[is->id] = d; > + if(!d) > + return 1; > + ioc3_enable(is, idd); > + serio_register_port(d->kbd); > + serio_register_port(d->aux); > + return 0; > +} > + > +static int ioc3kbd_remove(struct ioc3_submodule *is, struct ioc3_driver_data *idd) > +{ > + struct ioc3kbd_data *d = (struct ioc3kbd_data *)(idd->data[is->id]); > + serio_unregister_port(d->kbd); > + serio_unregister_port(d->aux); > + kfree(d->kbd); > + kfree(d->aux); Double free - serio core frees serio structure once the last reference is dropped. You are freeing already freed memory here. > + kfree(d); > + idd->data[is->id] = NULL; > + return 0; > +} > + > +static struct ioc3_submodule ioc3kbd_submodule = { > + .name = "serio", > + .probe = ioc3kbd_probe, > + .remove = ioc3kbd_remove, > + .irq_mask = SIO_IR_KBD_INT, > + .intr = ioc3kbd_intr, > + .owner = THIS_MODULE, > +}; > + > +static int __init ioc3kbd_init(void) > +{ > + ioc3_register_submodule(&ioc3kbd_submodule); Can this ioc3_... function fail? -- Dmitry
