[PATCH] - Cleanup patch for sa1100_jornada pcmcia driver
Wolfram Sang
w.sang at pengutronix.de
Mon Aug 31 14:55:17 EDT 2009
Hi Kristoffer,
Kristoffer Ericson wrote:
> This patch cleans up the /drivers/pcmcia/sa1100_jornada.c file with respect
> to formatting. It also removes a build warning atleast for me
> is a pain to watch every compile and has yet to provide
> something usefull (= everything seems to work well).
>
> Signed-off-by: Kristoffer Ericson <kristoffer.ericson at gmail.com>
>
> diff --git a/drivers/pcmcia/sa1100_jornada720.c b/drivers/pcmcia/sa1100_jornada720.c
> index 57ca085..a19b321 100644
> --- a/drivers/pcmcia/sa1100_jornada720.c
> +++ b/drivers/pcmcia/sa1100_jornada720.c
> @@ -16,89 +16,100 @@
>
> #include "sa1111_generic.h"
>
> -#define SOCKET0_POWER GPIO_GPIO0
> -#define SOCKET0_3V GPIO_GPIO2
> -#define SOCKET1_POWER (GPIO_GPIO1 | GPIO_GPIO3)
> -#warning *** Does SOCKET1_3V actually do anything?
I guess you mean this warning? Can you confirm SOCKET1_3V does anything?
Looking at the code, the handling of SOCKET1_POWER is different. I think
the warning was there to state that. We might be able to convert the
warning into a comment, but just dropping this information doesn't look
good to me unless we can verify it is obsolete.
> +#define SOCKET0_POWER GPIO_GPIO0
> +#define SOCKET0_3V GPIO_GPIO2
> +#define SOCKET1_POWER (GPIO_GPIO1 | GPIO_GPIO3)
> #define SOCKET1_3V GPIO_GPIO3
>
> static int jornada720_pcmcia_hw_init(struct soc_pcmcia_socket *skt)
> {
> - /*
> - * What is all this crap for?
> - */
> - GRER |= 0x00000002;
> - /* Set GPIO_A<3:1> to be outputs for PCMCIA/CF power controller: */
> - sa1111_set_io_dir(SA1111_DEV(skt->dev), GPIO_A0|GPIO_A1|GPIO_A2|GPIO_A3, 0, 0);
> - sa1111_set_io(SA1111_DEV(skt->dev), GPIO_A0|GPIO_A1|GPIO_A2|GPIO_A3, 0);
> - sa1111_set_sleep_io(SA1111_DEV(skt->dev), GPIO_A0|GPIO_A1|GPIO_A2|GPIO_A3, 0);
> -
> - return sa1111_pcmcia_hw_init(skt);
> + /*
> + * What is all this crap for?
> + */
> + GRER |= 0x00000002;
Do you know what this does? Would be nice to get rid of the
'crap'-comment, me thinks.
> + /* Set GPIO_A<3:1> to be outputs for PCMCIA/CF power controller: */
> + sa1111_set_io_dir(SA1111_DEV(skt->dev), GPIO_A0|GPIO_A1|GPIO_A2|GPIO_A3, 0, 0);
> + sa1111_set_io(SA1111_DEV(skt->dev), GPIO_A0|GPIO_A1|GPIO_A2|GPIO_A3, 0);
> + sa1111_set_sleep_io(SA1111_DEV(skt->dev), GPIO_A0|GPIO_A1|GPIO_A2|GPIO_A3, 0);
> +
> + return sa1111_pcmcia_hw_init(skt);
> }
>
> static int
> jornada720_pcmcia_configure_socket(struct soc_pcmcia_socket *skt, const socket_state_t *state)
> {
> - unsigned int pa_dwr_mask, pa_dwr_set;
> - int ret;
> -
> -printk("%s(): config socket %d vcc %d vpp %d\n", __func__,
> - skt->nr, state->Vcc, state->Vpp);
> -
> - switch (skt->nr) {
> - case 0:
> - pa_dwr_mask = SOCKET0_POWER | SOCKET0_3V;
> -
> - switch (state->Vcc) {
> - default:
> - case 0: pa_dwr_set = 0; break;
> - case 33: pa_dwr_set = SOCKET0_POWER | SOCKET0_3V; break;
> - case 50: pa_dwr_set = SOCKET0_POWER; break;
> - }
> - break;
> -
> - case 1:
> - pa_dwr_mask = SOCKET1_POWER;
> -
> - switch (state->Vcc) {
> - default:
> - case 0: pa_dwr_set = 0; break;
> - case 33: pa_dwr_set = SOCKET1_POWER; break;
> - case 50: pa_dwr_set = SOCKET1_POWER; break;
> - }
> - break;
> -
> - default:
> - return -1;
> - }
> -
> - if (state->Vpp != state->Vcc && state->Vpp != 0) {
> - printk(KERN_ERR "%s(): slot cannot support VPP %u\n",
> - __func__, state->Vpp);
> - return -1;
> - }
> -
> - ret = sa1111_pcmcia_configure_socket(skt, state);
> - if (ret == 0) {
> - unsigned long flags;
> -
> - local_irq_save(flags);
> - sa1111_set_io(SA1111_DEV(skt->dev), pa_dwr_mask, pa_dwr_set);
> - local_irq_restore(flags);
> - }
> -
> - return ret;
> + unsigned int pa_dwr_mask, pa_dwr_set;
> + int ret;
> +
> + printk("%s(): config socket %d vcc %d vpp %d\n", __func__,
> + skt->nr, state->Vcc, state->Vpp);
Add a loglevel. I'd assume it is a KERN_DEBUG. dev_dbg might be even
better (without the __func__).
> +
> + switch (skt->nr) {
> + case 0:
> + pa_dwr_mask = SOCKET0_POWER | SOCKET0_3V;
> +
> + switch (state->Vcc) {
> + default:
> + case 0:
> + pa_dwr_set = 0;
> + break;
> + case 33:
> + pa_dwr_set = SOCKET0_POWER | SOCKET0_3V;
> + break;
> + case 50:
> + pa_dwr_set = SOCKET0_POWER;
> + break;
> + }
> + break;
> +
> + case 1:
> + pa_dwr_mask = SOCKET1_POWER;
> +
> + switch (state->Vcc) {
> + default:
> + case 0:
> + pa_dwr_set = 0;
> + break;
> + case 33:
> + pa_dwr_set = SOCKET1_POWER;
> + break;
> + case 50:
> + pa_dwr_set = SOCKET1_POWER;
> + break;
> + }
> + break;
> +
> + default:
> + return -1;
> + }
> +
> + if (state->Vpp != state->Vcc && state->Vpp != 0) {
> + printk(KERN_ERR "%s(): slot cannot support VPP %u\n",
> + __func__, state->Vpp);
The continuing line needs more indent. Also, dev_err?
> + return -1;
> + }
> +
> + ret = sa1111_pcmcia_configure_socket(skt, state);
> + if (ret == 0) {
> + unsigned long flags;
> +
> + local_irq_save(flags);
> + sa1111_set_io(SA1111_DEV(skt->dev), pa_dwr_mask, pa_dwr_set);
> + local_irq_restore(flags);
The above block needs to be indented one tab more.
> + }
> +
> + return ret;
> }
>
> static struct pcmcia_low_level jornada720_pcmcia_ops = {
> - .owner = THIS_MODULE,
> - .hw_init = jornada720_pcmcia_hw_init,
> - .hw_shutdown = sa1111_pcmcia_hw_shutdown,
> - .socket_state = sa1111_pcmcia_socket_state,
> - .configure_socket = jornada720_pcmcia_configure_socket,
> -
> - .socket_init = sa1111_pcmcia_socket_init,
> - .socket_suspend = sa1111_pcmcia_socket_suspend,
> + .owner = THIS_MODULE,
> + .hw_init = jornada720_pcmcia_hw_init,
> + .hw_shutdown = sa1111_pcmcia_hw_shutdown,
> + .socket_state = sa1111_pcmcia_socket_state,
> + .configure_socket = jornada720_pcmcia_configure_socket,
> +
> + .socket_init = sa1111_pcmcia_socket_init,
> + .socket_suspend = sa1111_pcmcia_socket_suspend,
> };
>
> int __devinit pcmcia_jornada720_init(struct device *dev)
>
>
>
Other than that, the code looks _much_ better after your patch.
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
More information about the linux-pcmcia
mailing list