| Message ID | 1363978188-31200-2-git-send-email-simon.guinot@sequanux.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Linus, On Fri, Mar 22, 2013 at 07:49:47PM +0100, Simon Guinot wrote: > This patch adds a dedicated dbg_show function to the gpio-mvebu driver. > In addition to the generic gpiolib informations, this function displays > informations related with the specific Marvell registers (blink enable, > data in polarity, interrupt masks and cause). > > Signed-off-by: Simon Guinot <simon.guinot@sequanux.org> > --- > drivers/gpio/gpio-mvebu.c | 59 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 59 insertions(+) It'd probably be easiest to split this series up since the two patches don't really depend on one another at all. If you agree, I'll pull in the plat-orion/ patch. thx, Jason.
Dear Simon Guinot, On Fri, 22 Mar 2013 19:49:47 +0100, Simon Guinot wrote: > + for (i = 0; i < chip->ngpio; i++) { > + const char *label; > + int msk; > + bool is_out; > + > + label = gpiochip_is_requested(chip, i); > + if (!label) > + continue; > + > + msk = 1 << i; > + is_out = !(io_conf & msk); Maybe instead of using 'msk' you could use test_bit() ? is_out = !test_bit(i, io_conf); > + seq_printf(s, " gpio-%-3d (%-20.20s)", chip->base + i, label); > + > + if (is_out) { > + seq_printf(s, " out %s %s\n", > + out & msk ? "hi" : "lo", test_bit(i, out) ? "hi" : "lo", > + blink & msk ? "(blink )" : ""); test_bit(i, blink) ? "(blink )" : "" etc. Thomas
On Sat, Mar 23, 2013 at 04:21:18PM +0100, Thomas Petazzoni wrote: > Dear Simon Guinot, Hi Thomas, > > On Fri, 22 Mar 2013 19:49:47 +0100, Simon Guinot wrote: > > + for (i = 0; i < chip->ngpio; i++) { > > + const char *label; > > + int msk; > > + bool is_out; > > + > > + label = gpiochip_is_requested(chip, i); > > + if (!label) > > + continue; > > + > > + msk = 1 << i; > > + is_out = !(io_conf & msk); > > Maybe instead of using 'msk' you could use test_bit() ? test_bit implies more extra bit shifting which are not needed. But maybe it makes the function more readable. Your call :) Simon > > is_out = !test_bit(i, io_conf); > > > + seq_printf(s, " gpio-%-3d (%-20.20s)", chip->base + i, label); > > + > > + if (is_out) { > > + seq_printf(s, " out %s %s\n", > > + out & msk ? "hi" : "lo", > > test_bit(i, out) ? "hi" : "lo", > > > + blink & msk ? "(blink )" : ""); > > test_bit(i, blink) ? "(blink )" : "" > > etc. > > Thomas > -- > Thomas Petazzoni, Free Electrons > Kernel, drivers, real-time and embedded Linux > development, consulting, training and support. > http://free-electrons.com > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Sat, Mar 23, 2013 at 04:21:18PM +0100, Thomas Petazzoni wrote: > Dear Simon Guinot, > > On Fri, 22 Mar 2013 19:49:47 +0100, Simon Guinot wrote: > > + for (i = 0; i < chip->ngpio; i++) { > > + const char *label; > > + int msk; > > + bool is_out; > > + > > + label = gpiochip_is_requested(chip, i); > > + if (!label) > > + continue; > > + > > + msk = 1 << i; > > + is_out = !(io_conf & msk); > > Maybe instead of using 'msk' you could use test_bit() ? > > is_out = !test_bit(i, io_conf); > > > + seq_printf(s, " gpio-%-3d (%-20.20s)", chip->base + i, label); > > + > > + if (is_out) { > > + seq_printf(s, " out %s %s\n", > > + out & msk ? "hi" : "lo", > > test_bit(i, out) ? "hi" : "lo", > > > + blink & msk ? "(blink )" : ""); > > test_bit(i, blink) ? "(blink )" : "" I don't think this is a good idea - and it's wrong. test_bit() operates on an array of bits, and needs to be passed an unsigned long pointer. blink etc are "u32"s which aren't unsigned long. Moreover, what if ngpio becomes greater than 32 (for whatever reason)? test_bit(), when used correctly, will access the following word, which is probably a bad thing. The original won't have that behaviour, which is safer. Last point on the original - msk wants to also be of type 'u32' as well to match io_conf/out/blink.
Russell, On Sat, 23 Mar 2013 15:43:18 +0000, Russell King - ARM Linux wrote: > I don't think this is a good idea - and it's wrong. test_bit() operates > on an array of bits, and needs to be passed an unsigned long pointer. > blink etc are "u32"s which aren't unsigned long. > > Moreover, what if ngpio becomes greater than 32 (for whatever reason)? > test_bit(), when used correctly, will access the following word, which > is probably a bad thing. The original won't have that behaviour, > which is safer. You're right, indeed. Thomas
diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c index 61a6fde..c6a9f14 100644 --- a/drivers/gpio/gpio-mvebu.c +++ b/drivers/gpio/gpio-mvebu.c @@ -470,6 +470,64 @@ static void mvebu_gpio_irq_handler(unsigned int irq, struct irq_desc *desc) } } +#ifdef CONFIG_DEBUG_FS +#include <linux/seq_file.h> + +static void mvebu_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip) +{ + struct mvebu_gpio_chip *mvchip = + container_of(chip, struct mvebu_gpio_chip, chip); + u32 out, io_conf, blink, in_pol, data_in, cause, edg_msk, lvl_msk; + int i; + + out = readl_relaxed(mvebu_gpioreg_out(mvchip)); + io_conf = readl_relaxed(mvebu_gpioreg_io_conf(mvchip)); + blink = readl_relaxed(mvebu_gpioreg_blink(mvchip)); + in_pol = readl_relaxed(mvebu_gpioreg_in_pol(mvchip)); + data_in = readl_relaxed(mvebu_gpioreg_data_in(mvchip)); + cause = readl_relaxed(mvebu_gpioreg_edge_cause(mvchip)); + edg_msk = readl_relaxed(mvebu_gpioreg_edge_mask(mvchip)); + lvl_msk = readl_relaxed(mvebu_gpioreg_level_mask(mvchip)); + + for (i = 0; i < chip->ngpio; i++) { + const char *label; + int msk; + bool is_out; + + label = gpiochip_is_requested(chip, i); + if (!label) + continue; + + msk = 1 << i; + is_out = !(io_conf & msk); + + seq_printf(s, " gpio-%-3d (%-20.20s)", chip->base + i, label); + + if (is_out) { + seq_printf(s, " out %s %s\n", + out & msk ? "hi" : "lo", + blink & msk ? "(blink )" : ""); + continue; + } + + seq_printf(s, " in %s (act %s) - IRQ", + (data_in ^ in_pol) & msk ? "hi" : "lo", + in_pol & msk ? "lo" : "hi"); + if (!((edg_msk | lvl_msk) & msk)) { + seq_printf(s, " disabled\n"); + continue; + } + if (edg_msk & msk) + seq_printf(s, " edge "); + if (lvl_msk & msk) + seq_printf(s, " level"); + seq_printf(s, " (%s)\n", cause & msk ? "pending" : "clear "); + } +} +#else +#define mvebu_gpio_dbg_show NULL +#endif + static struct of_device_id mvebu_gpio_of_match[] = { { .compatible = "marvell,orion-gpio", @@ -550,6 +608,7 @@ static int mvebu_gpio_probe(struct platform_device *pdev) mvchip->chip.ngpio = ngpios; mvchip->chip.can_sleep = 0; mvchip->chip.of_node = np; + mvchip->chip.dbg_show = mvebu_gpio_dbg_show; spin_lock_init(&mvchip->lock); mvchip->membase = devm_ioremap_resource(&pdev->dev, res);
This patch adds a dedicated dbg_show function to the gpio-mvebu driver. In addition to the generic gpiolib informations, this function displays informations related with the specific Marvell registers (blink enable, data in polarity, interrupt masks and cause). Signed-off-by: Simon Guinot <simon.guinot@sequanux.org> --- drivers/gpio/gpio-mvebu.c | 59 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+)