[maz at misterjones.org: [PATCH 3/5] [PCMCIA] Add support for platform dependant quirks]

[Russell King]

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: