On Thu, Aug 07, 2008 at 10:22:36AM +0200, Dominik Brodowski wrote:
> what's your take on this?
Rather too complex for starters.
> @@ -226,7 +226,23 @@ static int pxa2xx_drv_pcmcia_resume(struct
> platform_device *dev)
> struct pcmcia_low_level *ops = dev->dev.platform_data;
> int nr = ops ? ops->nr : 0;
>
> - MECR = nr > 1 ? MECR_CIT | MECR_NOS : (nr > 0 ? MECR_CIT : 0);
> + if (nr > 0) {
> + u32 quirks = ops ? ops->quirks : 0;
> + int v;
> +
> + /*
> + * We have at least one socket, so set MECR:CIT
> + * (Card Is There)
> + */
> + v = MECR_CIT;
> +
> + /* Set MECR:NOS (Number Of Sockets) */
> + if (nr > 1 || (quirks & PXA2XX_QUIRK_NEEDS_MECR_NOS))
> + v |= MECR_NOS;
> +
> + MECR = v;
> + } else
> + MECR = 0;
Surely if we have initialised PCMCIA, we know that we have more than one
socket, so why bother testing 'nr > 0' ? ops also will never be null
here - in the probe function, we do:
if (!dev || !dev->platform_data)
return -ENODEV;
so its impossible for the driver to initialise with NULL platform data.
Also, the setting of MECR should be moved to a separate static function -
the setup should be indentical for both the successful probe and the
resume paths.
Finally, the question of setting MECR itself. I think there's more to
the support than what we see here - MECR doesn't power on the PCMCIA
sockets... Maybe that's a misunderstanding because of the loseness of
the patch description.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
_______________________________________________
Linux PCMCIA reimplementation list
http://lists.infradead.org/mailman/listinfo/linux-pcmcia
