Re: [PATCH 1/8] staging: mt7621-gpio: make use 'bgpio_init' from GPIO_GENERIC

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Jun 09 2018, Sergio Paracuellos wrote:

> Gpio complexity is just masking the fact that offset is always
> 0..n and writes to bits 0..n of some memory address. Because
> of this whole thing can just me converted to use GPIO_GENERIC
> and avoid duplications of a lot of driver custom functions.
> So use bgpio_init instead of custom code adding GPIO_GENERIC
> dependency to the Kconfig file. Also to make easier using
> bgpio_init function offset for each gpio bank, enumeration
> where register were defined has been replaced in favour of
> some macros which handle each gpio offset taking into account
> the bank where are located. Because of this change write and
> read functions which are being used for remaining irq handling
> stuff have been updated also as well as its dependencies along
> the code.

Thanks for this.  It is a big change, and unsurprisingly there are a few
bugs.
I haven't actually got the driver working yet so there is still
something not quite right after the things listed here get fixed.

Firstly, I don't think it is useful to create the macros that you
describe above.  Every time we access a register, we have an 'rg' which
has a bank number in it so I think it is much better to do the
multiplication in the read/write functions rather than in the macro.
But that isn't a bug exactly.... read on..


>
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx>
> ---
>  drivers/staging/mt7621-gpio/Kconfig       |   1 +
>  drivers/staging/mt7621-gpio/gpio-mt7621.c | 148 ++++++++++--------------------
>  2 files changed, 47 insertions(+), 102 deletions(-)
>
> diff --git a/drivers/staging/mt7621-gpio/Kconfig b/drivers/staging/mt7621-gpio/Kconfig
> index c741ec3..f4453e8 100644
> --- a/drivers/staging/mt7621-gpio/Kconfig
> +++ b/drivers/staging/mt7621-gpio/Kconfig
> @@ -1,6 +1,7 @@
>  config GPIO_MT7621
>  	bool "Mediatek GPIO Support"
>  	depends on SOC_MT7620 || SOC_MT7621
> +	select GPIO_GENERIC
>  	select ARCH_REQUIRE_GPIOLIB
>  	help
>  	  Say yes here to support the Mediatek SoC GPIO device
> diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> index 0c4fb4a..cc08505 100644
> --- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
> +++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> @@ -19,19 +19,18 @@
>  #define MTK_BANK_WIDTH		32
>  #define PIN_MASK(nr)		(1UL << ((nr % MTK_BANK_WIDTH)))
>  
> -enum mediatek_gpio_reg {
> -	GPIO_REG_CTRL = 0,
> -	GPIO_REG_POL,
> -	GPIO_REG_DATA,
> -	GPIO_REG_DSET,
> -	GPIO_REG_DCLR,
> -	GPIO_REG_REDGE,
> -	GPIO_REG_FEDGE,
> -	GPIO_REG_HLVL,
> -	GPIO_REG_LLVL,
> -	GPIO_REG_STAT,
> -	GPIO_REG_EDGE,
> -};
> +#define GPIO_BANK_WIDE           0x04
> +#define GPIO_REG_CTRL(bank)      (((bank) * GPIO_BANK_WIDE) + 0x00)
> +#define GPIO_REG_POL(bank)       (((bank) * GPIO_BANK_WIDE) + 0x10)
> +#define GPIO_REG_DATA(bank)      (((bank) * GPIO_BANK_WIDE) + 0x20)
> +#define GPIO_REG_DSET(bank)      (((bank) * GPIO_BANK_WIDE) + 0x30)
> +#define GPIO_REG_DCLR(bank)      (((bank) * GPIO_BANK_WIDE) + 0x40)
> +#define GPIO_REG_REDGE(bank)     (((bank) * GPIO_BANK_WIDE) + 0x50)
> +#define GPIO_REG_FEDGE(bank)     (((bank) * GPIO_BANK_WIDE) + 0x60)
> +#define GPIO_REG_HLVL(bank)      (((bank) * GPIO_BANK_WIDE) + 0x70)
> +#define GPIO_REG_LLVL(bank)      (((bank) * GPIO_BANK_WIDE) + 0x80)
> +#define GPIO_REG_STAT(bank)      (((bank) * GPIO_BANK_WIDE) + 0x90)
> +#define GPIO_REG_EDGE(bank)      (((bank) * GPIO_BANK_WIDE) + 0xA0)
>  
>  struct mtk_gc {
>  	struct gpio_chip chip;
> @@ -55,80 +54,21 @@ to_mediatek_gpio(struct gpio_chip *chip)
>  }
>  
>  static inline void
> -mtk_gpio_w32(struct mtk_gc *rg, u8 reg, u32 val)
> +mtk_gpio_w32(struct mtk_gc *rg, u32 offset, u32 val)
>  {
> -	struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
> -	u32 offset = (reg * 0x10) + (rg->bank * 0x4);
> +	struct gpio_chip *gc = &rg->chip;
> +	struct mtk_data *gpio_data = gpiochip_get_data(gc);
>  
> -	iowrite32(val, gpio_data->gpio_membase + offset);
> +	gc->write_reg(gpio_data->gpio_membase + offset, val);
>  }
>  
>  static inline u32
> -mtk_gpio_r32(struct mtk_gc *rg, u8 reg)
> -{
> -	struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
> -	u32 offset = (reg * 0x10) + (rg->bank * 0x4);
> -
> -	return ioread32(gpio_data->gpio_membase + offset);
> -}
> -
> -static void
> -mediatek_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
> -{
> -	struct mtk_gc *rg = to_mediatek_gpio(chip);
> -
> -	mtk_gpio_w32(rg, (value) ? GPIO_REG_DSET : GPIO_REG_DCLR, BIT(offset));
> -}
> -
> -static int
> -mediatek_gpio_get(struct gpio_chip *chip, unsigned int offset)
> -{
> -	struct mtk_gc *rg = to_mediatek_gpio(chip);
> -
> -	return !!(mtk_gpio_r32(rg, GPIO_REG_DATA) & BIT(offset));
> -}
> -
> -static int
> -mediatek_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
> -{
> -	struct mtk_gc *rg = to_mediatek_gpio(chip);
> -	unsigned long flags;
> -	u32 t;
> -
> -	spin_lock_irqsave(&rg->lock, flags);
> -	t = mtk_gpio_r32(rg, GPIO_REG_CTRL);
> -	t &= ~BIT(offset);
> -	mtk_gpio_w32(rg, GPIO_REG_CTRL, t);
> -	spin_unlock_irqrestore(&rg->lock, flags);
> -
> -	return 0;
> -}
> -
> -static int
> -mediatek_gpio_direction_output(struct gpio_chip *chip,
> -					unsigned int offset, int value)
> +mtk_gpio_r32(struct mtk_gc *rg, u32 offset)
>  {
> -	struct mtk_gc *rg = to_mediatek_gpio(chip);
> -	unsigned long flags;
> -	u32 t;
> -
> -	spin_lock_irqsave(&rg->lock, flags);
> -	t = mtk_gpio_r32(rg, GPIO_REG_CTRL);
> -	t |= BIT(offset);
> -	mtk_gpio_w32(rg, GPIO_REG_CTRL, t);
> -	mediatek_gpio_set(chip, offset, value);
> -	spin_unlock_irqrestore(&rg->lock, flags);
> +	struct gpio_chip *gc = &rg->chip;
> +	struct mtk_data *gpio_data = gpiochip_get_data(gc);
>  
> -	return 0;
> -}
> -
> -static int
> -mediatek_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
> -{
> -	struct mtk_gc *rg = to_mediatek_gpio(chip);
> -	u32 t = mtk_gpio_r32(rg, GPIO_REG_CTRL);
> -
> -	return (t & BIT(offset)) ? GPIOF_DIR_OUT : GPIOF_DIR_IN;
> +	return gc->read_reg(gpio_data->gpio_membase + offset);
>  }
>  
>  static int
> @@ -156,20 +96,22 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
>  	memset(rg, 0, sizeof(*rg));
>  
>  	spin_lock_init(&rg->lock);
> +	rg->bank = be32_to_cpu(*id);
> +
> +	ret = bgpio_init(&rg->chip, &pdev->dev, 4,
> +			 gpio_data->gpio_membase + GPIO_REG_DATA(rg->bank),
> +			 gpio_data->gpio_membase + GPIO_REG_DSET(rg->bank),
> +			 gpio_data->gpio_membase + GPIO_REG_DCLR(rg->bank),
> +			 gpio_data->gpio_membase + GPIO_REG_CTRL(rg->bank),
> +			 gpio_data->gpio_membase + GPIO_REG_CTRL(rg->bank),
> +			 0);

This is the first problem I git.  You've provided both 'dirout' and
'dirin' and that is not permitted.  You've given exactly the same value
for both, which isn't really meaningful.
dirout is correct, dirin should be NULL.

> +	if (ret) {
> +		dev_err(&pdev->dev, "bgpio_init() failed\n");
> +		return ret;
> +	}
>  
> -	rg->chip.parent = &pdev->dev;
> -	rg->chip.label = dev_name(&pdev->dev);

> -	rg->chip.of_node = bank;
> -	rg->chip.base = MTK_BANK_WIDTH * be32_to_cpu(*id);

These last two are still needed - bgpio_init doesn't set them up.

I think that is all in this patch.

Thanks,
NeilBrown

> -	rg->chip.ngpio = MTK_BANK_WIDTH;
> -	rg->chip.direction_input = mediatek_gpio_direction_input;
> -	rg->chip.direction_output = mediatek_gpio_direction_output;
> -	rg->chip.get_direction = mediatek_gpio_get_direction;
> -	rg->chip.get = mediatek_gpio_get;
> -	rg->chip.set = mediatek_gpio_set;
>  	if (gpio_data->gpio_irq_domain)
>  		rg->chip.to_irq = mediatek_gpio_to_irq;
> -	rg->bank = be32_to_cpu(*id);
>  
>  	ret = devm_gpiochip_add_data(&pdev->dev, &rg->chip, gpio_data);
>  	if (ret < 0) {
> @@ -179,7 +121,7 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
>  	}
>  
>  	/* set polarity to low for all gpios */
> -	mtk_gpio_w32(rg, GPIO_REG_POL, 0);
> +	mtk_gpio_w32(rg, GPIO_REG_POL(rg->bank), 0);
>  
>  	dev_info(&pdev->dev, "registering %d gpios\n", rg->chip.ngpio);
>  
> @@ -200,14 +142,14 @@ mediatek_gpio_irq_handler(struct irq_desc *desc)
>  		if (!rg)
>  			continue;
>  
> -		pending = mtk_gpio_r32(rg, GPIO_REG_STAT);
> +		pending = mtk_gpio_r32(rg, GPIO_REG_STAT(rg->bank));
>  
>  		for_each_set_bit(bit, &pending, MTK_BANK_WIDTH) {
>  			u32 map = irq_find_mapping(gpio_data->gpio_irq_domain,
>  						   (MTK_BANK_WIDTH * i) + bit);
>  
>  			generic_handle_irq(map);
> -			mtk_gpio_w32(rg, GPIO_REG_STAT, BIT(bit));
> +			mtk_gpio_w32(rg, GPIO_REG_STAT(i), BIT(bit));
>  		}
>  	}
>  }
> @@ -226,10 +168,12 @@ mediatek_gpio_irq_unmask(struct irq_data *d)
>  		return;
>  
>  	spin_lock_irqsave(&rg->lock, flags);
> -	rise = mtk_gpio_r32(rg, GPIO_REG_REDGE);
> -	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE);
> -	mtk_gpio_w32(rg, GPIO_REG_REDGE, rise | (PIN_MASK(pin) & rg->rising));
> -	mtk_gpio_w32(rg, GPIO_REG_FEDGE, fall | (PIN_MASK(pin) & rg->falling));
> +	rise = mtk_gpio_r32(rg, GPIO_REG_REDGE(bank));
> +	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE(bank));
> +	mtk_gpio_w32(rg, GPIO_REG_REDGE(bank),
> +		     rise | (PIN_MASK(pin) & rg->rising));
> +	mtk_gpio_w32(rg, GPIO_REG_FEDGE(bank),
> +		     fall | (PIN_MASK(pin) & rg->falling));
>  	spin_unlock_irqrestore(&rg->lock, flags);
>  }
>  
> @@ -247,10 +191,10 @@ mediatek_gpio_irq_mask(struct irq_data *d)
>  		return;
>  
>  	spin_lock_irqsave(&rg->lock, flags);
> -	rise = mtk_gpio_r32(rg, GPIO_REG_REDGE);
> -	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE);
> -	mtk_gpio_w32(rg, GPIO_REG_FEDGE, fall & ~PIN_MASK(pin));
> -	mtk_gpio_w32(rg, GPIO_REG_REDGE, rise & ~PIN_MASK(pin));
> +	rise = mtk_gpio_r32(rg, GPIO_REG_REDGE(bank));
> +	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE(bank));
> +	mtk_gpio_w32(rg, GPIO_REG_FEDGE(bank), fall & ~PIN_MASK(pin));
> +	mtk_gpio_w32(rg, GPIO_REG_REDGE(bank), rise & ~PIN_MASK(pin));
>  	spin_unlock_irqrestore(&rg->lock, flags);
>  }
>  
> -- 
> 2.7.4

Attachment: signature.asc
Description: PGP signature

_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux