On Mon, 31 Aug 2009 20:55:17 +0200 Wolfram Sang <[email protected]> wrote:
> 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 <[email protected]> > > > > 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. The only proof I have is essentially that the driver has worked seemingly flawless in 3-4 years. But I agree that putting it as an comment is probably the way to go. > > > +#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. No idea, and havent tracked who put it there. > > > + /* 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__). > Agreed. > > + > > + 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? > Agreed. > > + 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. > Thanks. > > + } > > + > > + 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. > Groovie, thanks for feedback. Please see the latest version. > Regards, > > Wolfram > > -- > Pengutronix e.K. | Wolfram Sang | > Industrial Linux Solutions | http://www.pengutronix.de/ | > -- Kristoffer Ericson <[email protected]> _______________________________________________ Linux PCMCIA reimplementation list http://lists.infradead.org/mailman/listinfo/linux-pcmcia
