[PATCH] - Cleanup patch for sa1100_jornada pcmcia driver
Kristoffer Ericson
kristoffer.ericson at gmail.com
Tue Sep 1 06:39:13 EDT 2009
On Mon, 31 Aug 2009 20:55:17 +0200
Wolfram Sang <w.sang at pengutronix.de> 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 <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.
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 <kristoffer.ericson at gmail.com>
More information about the linux-pcmcia
mailing list