lkml.org 
[lkml]   [2007]   [May]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2] pcmcia: export pcmcia_bus_type
From
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 majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2007-05-14 01:51    [W:0.047 / U:0.500 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site