Re: [RFC 01/14] Coldfire generic GPIO (m68knommu)

[Steven King <sfking@fdwdc.com>]

On Wednesday, March 18, 2009 11:32:44 Greg Ungerer wrote:
> Hi Steven,
>
> Steven King wrote:
> > The basic Coldfire generic gpio implementation.
> >
> > Signed-off-by: Steven King <sfking@fdwdc.com>
> >
> > diff --git a/arch/m68knommu/platform/coldfire/gpio.c
> > b/arch/m68knommu/platform/coldfire/gpio.c
> > new file mode 100644
> > index 0000000..c2504b9
> > --- /dev/null
> > +++ b/arch/m68knommu/platform/coldfire/gpio.c
> > @@ -0,0 +1,1969 @@
> > +/*
> > + * Coldfire generic GPIO support.
> > + *
> > + * (C) Copyright 2009, Steven King <sfking@fdwdc.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; version 2 of the License.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/init.h>
> > +#include <linux/sysdev.h>
> > +
> > +#include <asm/gpio.h>
> > +#include <asm/pinmux.h>
> > +
> > +struct mcf_gpio_chip {
> > +	struct gpio_chip gpio_chip;
> > +	void __iomem *pddr;
> > +	void __iomem *podr;
> > +	void __iomem *ppdr;
> > +	void __iomem *setr;
> > +	void __iomem *clrr;
> > +	const u8 *gpio_to_pinmux;
> > +};
> > +
> > +#define MCF_CHIP(chip) container_of(chip, struct mcf_gpio_chip,
> > gpio_chip) +
> > +static int mcf_gpio_direction_input(struct gpio_chip *chip, unsigned
> > offset) +{
> > +	unsigned long flags;
> > +	MCFGPIO_PORTTYPE dir;
> > +	struct mcf_gpio_chip *mcf_chip = MCF_CHIP(chip);
> > +
> > +	local_irq_save(flags);
> > +	dir = mcfgpio_read(mcf_chip->pddr);
> > +	dir &= ~mcfgpio_bit(chip->base + offset);
> > +	mcfgpio_write(dir, mcf_chip->pddr);
> > +	local_irq_restore(flags);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mcf_gpio_get_value(struct gpio_chip *chip, unsigned offset)
> > +{
> > +	struct mcf_gpio_chip *mcf_chip = MCF_CHIP(chip);
> > +
> > +	return mcfgpio_read(mcf_chip->ppdr) & mcfgpio_bit(chip->base + offset);
> > +}
> > +
> > +static int mcf_gpio_direction_output(struct gpio_chip *chip, unsigned
> > offset, +		int value)
> > +{
> > +	unsigned long flags;
> > +	MCFGPIO_PORTTYPE data;
> > +	struct mcf_gpio_chip *mcf_chip = MCF_CHIP(chip);
> > +
> > +	local_irq_save(flags);
> > +	/* write the value to the output latch */
> > +	data = mcfgpio_read(mcf_chip->podr);
> > +	if (value)
> > +		data |= mcfgpio_bit(chip->base + offset);
> > +	else
> > +		data &= ~mcfgpio_bit(chip->base + offset);
> > +	mcfgpio_write(data, mcf_chip->podr);
> > +
> > +	/* now set the direction to output */
> > +	data = mcfgpio_read(mcf_chip->pddr);
> > +	data |= mcfgpio_bit(chip->base + offset);
> > +	mcfgpio_write(data, mcf_chip->pddr);
> > +	local_irq_restore(flags);
> > +
> > +	return 0;
> > +}
> > +
> > +static void mcf_gpio_set_value(struct gpio_chip *chip, unsigned offset,
> > +		int value)
> > +{
> > +	struct mcf_gpio_chip *mcf_chip = MCF_CHIP(chip);
> > +
> > +	unsigned long flags;
> > +	MCFGPIO_PORTTYPE data;
> > +
> > +	local_irq_save(flags);
> > +	data = mcfgpio_read(mcf_chip->podr);
> > +	if (value)
> > +		data |= mcfgpio_bit(chip->base + offset);
> > +	else
> > +		data &= ~mcfgpio_bit(chip->base + offset);
> > +	mcfgpio_write(data, mcf_chip->podr);
> > +	local_irq_restore(flags);
> > +}
> > +
> > +#if defined(CONFIG_M520x) || defined(CONFIG_M523x) || \
> > +      defined(CONFIG_M527x) || defined(CONFIG_M528x) ||
> > defined(CONFIG_M532x) +static void mcf_gpio_set_value_fast(struct
> > gpio_chip *chip, unsigned offset, +		int value)
> > +{
> > +	struct mcf_gpio_chip *mcf_chip = MCF_CHIP(chip);
> > +
> > +	if (value)
> > +		mcfgpio_write(mcfgpio_bit(chip->base + offset), mcf_chip->setr);
> > +	else
> > +		mcfgpio_write(~mcfgpio_bit(chip->base + offset), mcf_chip->clrr);
> > +}
> > +#endif
> > +static int mcf_gpio_request(struct gpio_chip *chip, unsigned offset)
> > +{
> > +	struct mcf_gpio_chip *mcf_chip = MCF_CHIP(chip);
> > +
> > +	return mcf_chip->gpio_to_pinmux ?
> > +		mcf_pinmux_request(mcf_chip->gpio_to_pinmux[offset], 0) : 0;
> > +}
> > +
> > +static void mcf_gpio_free(struct gpio_chip *chip, unsigned offset)
> > +{
> > +	struct mcf_gpio_chip *mcf_chip = MCF_CHIP(chip);
> > +
> > +	mcf_gpio_direction_input(chip, offset);
> > +
> > +	if (mcf_chip->gpio_to_pinmux)
> > +		mcf_pinmux_release(mcf_chip->gpio_to_pinmux[offset], 0);
> > +}
> > +
> > +static struct mcf_gpio_chip mcf_gpio_chips[] = {
> > +#if defined(CONFIG_M5206) || defined(CONFIG_M5206e)
>
> I would rather see this table's contents in the per-platform
> setup code (so under ~/arch/m68knommu/platform/m5206, etc).
> Associated with each family members specific code. I would have a
> gpio.c in each platform directory just for this.
>
> Keep the common code above in coldfire/gpio.c (the support functions
> could obviously no longer be static). When new family members
> are created/added this common coldfire/gpio.c won't need to be
> touched.

Yeah, I was trying to keep the implementation details static as much as 
possible.  Having a per-platform table means the struct mcf_gpio_chip also 
needs to go into a header which I found mildly aesthetically unpleasing.  Oh 
well.

> Otherwise I don't see any specific problems with it.

Cool.  I'll fold the per-platform table into the next version of the patch which 
adds pinmux support (and which definately is going to require per-platform 
files).

-- 
Steven King -- sfking at fdwdc dot com