On Fri, 2014-12-05 at 16:40 -0800, Ray Jui wrote:
> This GPIO driver supports all 3 GPIO controllers in the Broadcom Cygnus
> SoC. The 3 GPIO controllers are 1) the ASIU GPIO controller
> ("brcm,cygnus-asiu-gpio"), 2) the chipCommonG GPIO controller
> ("brcm,cygnus-ccm-gpio"), and 3) the ALWAYS-ON GPIO controller
> ("brcm,cygnus-crmu-gpio")
trivia:
> diff --git a/drivers/gpio/gpio-bcm-cygnus.c b/drivers/gpio/gpio-bcm-cygnus.c
> +static inline struct bcm_cygnus_gpio *to_bcm_cygnus_gpio(
> + struct gpio_chip *gc)
> +{
> + return container_of(gc, struct bcm_cygnus_gpio, gc);
> +}
Probably all of these inlines can just be static.
The compiler does a pretty good job these days
of inlining where appropriate.
> +static void bcm_cygnus_gpio_irq_handler(unsigned int irq,
> + struct irq_desc *desc)
> +{
> + struct bcm_cygnus_gpio *cygnus_gpio;
> + struct irq_chip *chip = irq_desc_get_chip(desc);
> + int i, bit;
> +
> + chained_irq_enter(chip, desc);
> +
> + cygnus_gpio = irq_get_handler_data(irq);
> +
> + /* go through the entire GPIO banks and handle all interrupts */
> + for (i = 0; i < cygnus_gpio->num_banks; i++) {
> + unsigned long val = readl(cygnus_gpio->base +
> + (i * GPIO_BANK_SIZE) +
> + CYGNUS_GPIO_INT_MSTAT_OFFSET);
> + if (val) {
This if (val) and indentation isn't really necessary
> + for_each_set_bit(bit, &val, 32) {
for_each_set_bit will effectively do the if above.
32 bit only code?
otherwise isn't this endian unsafe?
> + unsigned pin = NGPIOS_PER_BANK * i + bit;
> + int child_irq = bcm_cygnus_gpio_to_irq(
> + &cygnus_gpio->gc, pin);
> +
> + /*
> + * Clear the interrupt before invoking the
> + * handler, so we do not leave any window
> + */
> + writel(1 << bit,
> + cygnus_gpio->base +
> + (i * GPIO_BANK_SIZE) +
> + CYGNUS_GPIO_INT_CLR_OFFSET);
> +
> + generic_handle_irq(child_irq);
> + }
> +
> + }
> + }
> +
> + chained_irq_exit(chip, desc);
> +}
> +
> +static void bcm_cygnus_gpio_irq_ack(struct irq_data *d)
> +{
> + struct bcm_cygnus_gpio *cygnus_gpio = irq_data_get_irq_chip_data(d);
> + unsigned gpio = d->hwirq;
> + unsigned int offset, shift;
> + u32 val;
> +
> + offset = __gpio_reg_offset(cygnus_gpio, gpio) +
> + CYGNUS_GPIO_INT_CLR_OFFSET;
> + shift = __gpio_bitpos(cygnus_gpio, gpio);
> +
> + val = 1 << shift;
> + writel(val, cygnus_gpio->base + offset);
> +
> + dev_dbg(cygnus_gpio->dev, "gpio:%u offset:0x%04x shift:%u\n", gpio,
> + offset, shift);
> +}
> +static struct irq_chip bcm_cygnus_gpio_irq_chip = {
> + .name = "bcm-cygnus-gpio",
> + .irq_ack = bcm_cygnus_gpio_irq_ack,
> + .irq_mask = bcm_cygnus_gpio_irq_mask,
> + .irq_unmask = bcm_cygnus_gpio_irq_unmask,
> + .irq_set_type = bcm_cygnus_gpio_irq_set_type,
> +};
const?
> +static struct irq_domain_ops bcm_cygnus_irq_ops = {
> + .map = bcm_cygnus_gpio_irq_map,
> + .unmap = bcm_cygnus_gpio_irq_unmap,
> + .xlate = irq_domain_xlate_twocell,
> +};
const here too?
> +#ifdef CONFIG_OF_GPIO
> +static void bcm_cygnus_gpio_set_pull(struct bcm_cygnus_gpio *cygnus_gpio,
> + unsigned gpio, enum gpio_pull pull)
> +{
> + unsigned int offset, shift;
> + u32 val, up;
bool up; ?
> + unsigned long flags;
> +
> + switch (pull) {
> + case GPIO_PULL_NONE:
> + return;
> + case GPIO_PULL_UP:
> + up = 1;
> + break;
> + case GPIO_PULL_DOWN:
> + up = 0;
> + break;
> + case GPIO_PULL_INVALID:
> + default:
> + return;
> + }
Maybe more sensible to group GPIO_PULL_NONE with GPIO_PULL_INVALID
> +static int bcm_cygnus_gpio_probe(struct platform_device *pdev)
> +{
[]
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(&pdev->dev, "unable to get I/O resource");
missing newline
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
