Hi,
On Mon, May 14, 2007 at 12:10:29AM +0100, Russell King wrote:
> Some random review comments on your driver, while I was passing. Spotted
> a few things.
Thanks, I appreciate the feedback.
> On Sun, May 13, 2007 at 04:40:07PM -0500, Olof Johansson wrote:
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/sched.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/platform_device.h>
>
> Silly duplicate includes.
Silly indeed.
> > +#include <linux/errno.h>
> > +#include <linux/init.h>
> > +#include <linux/delay.h>
> > +#include <linux/interrupt.h>
> > +
> > +#include <pcmcia/ss.h>
> > +#include <asm/of_platform.h>
> > +
> > +static const char driver_name[] = "electra-cf";
> >...
> > +static int electra_cf_get_status(struct pcmcia_socket *s, u_int *sp)
> > +{
> > + struct electra_cf_socket *cf;
> > +
> > + if (!sp)
> > + return -EINVAL;
>
> Never called with a NULL argument, so useless check.
Ok. Carried over from omap_cf, but I'll definitely fix it here.
> > +
> > + cf = container_of(s, struct electra_cf_socket, socket);
> > +
> > + /* NOTE CF is always 3VCARD */
> > + if (electra_cf_present(cf)) {
> > + struct electra_cf_socket *cf;
> > +
> > + *sp = SS_READY | SS_DETECT | SS_POWERON | SS_3VCARD;
> > + cf = container_of(s, struct electra_cf_socket, socket);
> > + s->pci_irq = cf->irq;
> > + } else
> > + *sp = 0;
> > + return 0;
> > +}
> >...
> > +static int electra_cf_ss_suspend(struct pcmcia_socket *s)
> > +{
> > + pr_debug("%s: %s\n", driver_name, __FUNCTION__);
> > + return electra_cf_set_socket(s, &dead_socket);
> > +}
>
> That's already done for you. Remove this function entirely and set
> the ".suspend" method to NULL.
Thanks, will do.
>
> > +static int electra_cf_set_io_map(struct pcmcia_socket *s,
> > + struct pccard_io_map *io)
> > +{
> > + struct electra_cf_socket *cf;
> > +
> > + cf = container_of(s, struct electra_cf_socket, socket);
> > + io->flags &= MAP_ACTIVE|MAP_ATTRIB|MAP_16BIT;
> > + io->start = (unsigned long)cf->io_base;
> > + io->stop = (unsigned long)cf->io_base + 0x800 - 1;
> > + return 0;
> > +}
>
> PCMCIA code will ignore your writes to 'io'. Therefore, does this
> do anything useful? If not, an empty function will do.
Not sure if it does, it came over from omap_cf with some modifications
by me. I'll take a closer look when I rework the rest of the I/O
mapping stuff.
> > + cf->socket.owner = THIS_MODULE;
> > + cf->socket.dev.parent = &ofdev->dev;
> > + cf->socket.ops = &electra_cf_ops;
> > + cf->socket.resource_ops = &pccard_static_ops;
> > + cf->socket.features = SS_CAP_PCCARD | SS_CAP_STATIC_MAP |
> > + SS_CAP_MEM_ALIGN;
> > + cf->socket.map_size = 0x800;
> > + cf->socket.io[0].res = &cf->iomem;
>
> Not a good idea - PCMCIA manages this resources itself and will free it
> when the card is removed. Use ss_cap_static_map and socket.io_offset.
Right, I'm going to rework those parts alltogether.
> > +static int __devexit electra_cf_remove(struct of_device *ofdev)
> > +{
> > + struct device *device = &ofdev->dev;
> > + struct electra_cf_socket *cf;
> > +
> > + cf = dev_get_drvdata(device);
> > +
> > + cf->active = 0;
> > + pcmcia_unregister_socket(&cf->socket);
> > + del_timer_sync(&cf->timer);
> > + free_irq(cf->irq, cf);
>
> What if an IRQ occurs after the timer has been deleted? Doesn't the
> interrupt handler re-add the timer back?
Ah, yes, good point.
Thanks,
-Olof
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
