| Message ID | 1459508407-22011-4-git-send-email-cristina.ciocan@intel.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Apr 1, 2016 at 1:00 PM, Cristina Ciocan <cristina.ciocan@intel.com> wrote: > This patch updates the gpio chip implementation in order to interact with > the pin control model: the chip contains reference to SOC data and > pin/group/community information is retrieved through the SOC reference. > > Signed-off-by: Cristina Ciocan <cristina.ciocan@intel.com> Patch applied with Mika's ACK. Cristina & Mika, can you provide feedback on a patch I sent last week: http://marc.info/?l=linux-gpio&m=145864063724362&w=2 This makes it possible for a GPIO driver to use native open drain if the hardware supports this instead of relying on switching the pin to input and thus expecting high impedance. With a backing pin control driver I think that maybe we need a pin control back-end performing things like this on behalf of the GPIO driver, something like pinctrl_gpio_set_config(unsigned gpio, enum pin_config_param param, u16 argument); So the pin controller can perform config on behalf of the GPIO driver (e.g. setting a backing pin to open drain). Do you think we will need this? Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 04, 2016 at 04:08:47PM +0200, Linus Walleij wrote: > On Fri, Apr 1, 2016 at 1:00 PM, Cristina Ciocan > <cristina.ciocan@intel.com> wrote: > > > This patch updates the gpio chip implementation in order to interact with > > the pin control model: the chip contains reference to SOC data and > > pin/group/community information is retrieved through the SOC reference. > > > > Signed-off-by: Cristina Ciocan <cristina.ciocan@intel.com> > > Patch applied with Mika's ACK. Thanks! > Cristina & Mika, can you provide feedback on a patch I sent last week: > http://marc.info/?l=linux-gpio&m=145864063724362&w=2 > > This makes it possible for a GPIO driver to use native > open drain if the hardware supports this instead of relying > on switching the pin to input and thus expecting high impedance. Looks like a good idea to me. Recent Intel hardware (Skylake, Broxton) is capable of taking advantage of this. Not sure if Baytrail supports this at hardware level, though. > With a backing pin control driver I think that maybe we need > a pin control back-end performing things like this on behalf > of the GPIO driver, something like > pinctrl_gpio_set_config(unsigned gpio, enum pin_config_param param, > u16 argument); > > So the pin controller can perform config on behalf of the > GPIO driver (e.g. setting a backing pin to open drain). > > Do you think we will need this? If I understand this right, GPIO part of the pinctrl driver just calls pinctrl_gpio_set_config() with correct parameters in its ->set_single_ended() to get the pin to the right mode. So yes, I think we could use it, at least from Intel pinctrl/GPIO drivers perspective :) -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05.04.2016 11:48, Mika Westerberg wrote: > On Mon, Apr 04, 2016 at 04:08:47PM +0200, Linus Walleij wrote: >> On Fri, Apr 1, 2016 at 1:00 PM, Cristina Ciocan >> <cristina.ciocan@intel.com> wrote: >> >>> This patch updates the gpio chip implementation in order to interact with >>> the pin control model: the chip contains reference to SOC data and >>> pin/group/community information is retrieved through the SOC reference. >>> >>> Signed-off-by: Cristina Ciocan <cristina.ciocan@intel.com> >> >> Patch applied with Mika's ACK. > > Thanks! > >> Cristina & Mika, can you provide feedback on a patch I sent last week: >> http://marc.info/?l=linux-gpio&m=145864063724362&w=2 >> >> This makes it possible for a GPIO driver to use native >> open drain if the hardware supports this instead of relying >> on switching the pin to input and thus expecting high impedance. > > Looks like a good idea to me. Recent Intel hardware (Skylake, Broxton) > is capable of taking advantage of this. Not sure if Baytrail supports > this at hardware level, though. Based solely on the official documentation, Baytrail does not seem to support this, but the issue is that there are plenty of bits marked as reserved in the conf0 register. From what I have seen from the initial GPIO driver implementation, bit 31 from conf0 register can be used for reading open drain (not setting, since it is RO). This information is in byt_gpio_dbg_show. Since all the reserved bits are RO, it seems that setting open drain is not a possibility for Baytrail. > >> With a backing pin control driver I think that maybe we need >> a pin control back-end performing things like this on behalf >> of the GPIO driver, something like >> pinctrl_gpio_set_config(unsigned gpio, enum pin_config_param param, >> u16 argument); >> >> So the pin controller can perform config on behalf of the >> GPIO driver (e.g. setting a backing pin to open drain). >> >> Do you think we will need this? > > If I understand this right, GPIO part of the pinctrl driver just calls > pinctrl_gpio_set_config() with correct parameters in its > ->set_single_ended() to get the pin to the right mode. So yes, I think > we could use it, at least from Intel pinctrl/GPIO drivers perspective :) > -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c index 415d2d5..a24b51e 100644 --- a/drivers/pinctrl/intel/pinctrl-baytrail.c +++ b/drivers/pinctrl/intel/pinctrl-baytrail.c @@ -20,6 +20,7 @@ #include <linux/types.h> #include <linux/bitops.h> #include <linux/interrupt.h> +#include <linux/gpio.h> #include <linux/gpio/driver.h> #include <linux/acpi.h> #include <linux/platform_device.h> @@ -1363,44 +1364,45 @@ static void byt_gpio_set(struct gpio_chip *chip, unsigned offset, int value) unsigned long flags; u32 old_val; - raw_spin_lock_irqsave(&vg->lock, flags); + if (!reg) + return; + raw_spin_lock_irqsave(&vg->lock, flags); old_val = readl(reg); - if (value) writel(old_val | BYT_LEVEL, reg); else writel(old_val & ~BYT_LEVEL, reg); - raw_spin_unlock_irqrestore(&vg->lock, flags); } -static int byt_gpio_direction_input(struct gpio_chip *chip, unsigned offset) +static int byt_gpio_get_direction(struct gpio_chip *chip, unsigned int offset) { struct byt_gpio *vg = gpiochip_get_data(chip); void __iomem *reg = byt_gpio_reg(vg, offset, BYT_VAL_REG); unsigned long flags; u32 value; - raw_spin_lock_irqsave(&vg->lock, flags); - - value = readl(reg) | BYT_DIR_MASK; - value &= ~BYT_INPUT_EN; /* active low */ - writel(value, reg); + if (!reg) + return -EINVAL; + raw_spin_lock_irqsave(&vg->lock, flags); + value = readl(reg); raw_spin_unlock_irqrestore(&vg->lock, flags); - return 0; + if (!(value & BYT_OUTPUT_EN)) + return GPIOF_DIR_OUT; + if (!(value & BYT_INPUT_EN)) + return GPIOF_DIR_IN; + + return -EINVAL; } -static int byt_gpio_direction_output(struct gpio_chip *chip, - unsigned gpio, int value) +static int byt_gpio_direction_input(struct gpio_chip *chip, unsigned int offset) { struct byt_gpio *vg = gpiochip_get_data(chip); - void __iomem *conf_reg = byt_gpio_reg(vg, gpio, BYT_CONF0_REG); - void __iomem *reg = byt_gpio_reg(vg, gpio, BYT_VAL_REG); + void __iomem *conf_reg = byt_gpio_reg(vg, offset, BYT_CONF0_REG); unsigned long flags; - u32 reg_val; raw_spin_lock_irqsave(&vg->lock, flags); @@ -1413,15 +1415,18 @@ static int byt_gpio_direction_output(struct gpio_chip *chip, WARN(readl(conf_reg) & BYT_DIRECT_IRQ_EN, "Potential Error: Setting GPIO with direct_irq_en to output"); - reg_val = readl(reg) | BYT_DIR_MASK; - reg_val &= ~(BYT_OUTPUT_EN | BYT_INPUT_EN); + return pinctrl_gpio_direction_input(chip->base + offset); +} - if (value) - writel(reg_val | BYT_LEVEL, reg); - else - writel(reg_val & ~BYT_LEVEL, reg); +static int byt_gpio_direction_output(struct gpio_chip *chip, + unsigned int offset, int value) +{ + int ret = pinctrl_gpio_direction_output(chip->base + offset); - raw_spin_unlock_irqrestore(&vg->lock, flags); + if (ret) + return ret; + + byt_gpio_set(chip, offset, value); return 0; } @@ -1430,20 +1435,42 @@ static void byt_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip) { struct byt_gpio *vg = gpiochip_get_data(chip); int i; - u32 conf0, val, offs; + u32 conf0, val; - for (i = 0; i < vg->chip.ngpio; i++) { + for (i = 0; i < vg->soc_data->npins; i++) { + const struct byt_community *comm; const char *pull_str = NULL; const char *pull = NULL; + void __iomem *reg; unsigned long flags; const char *label; - offs = vg->range->pins[i] * 16; + unsigned int pin; raw_spin_lock_irqsave(&vg->lock, flags); - conf0 = readl(vg->reg_base + offs + BYT_CONF0_REG); - val = readl(vg->reg_base + offs + BYT_VAL_REG); + pin = vg->soc_data->pins[i].number; + reg = byt_gpio_reg(vg, pin, BYT_CONF0_REG); + if (!reg) { + seq_printf(s, + "Could not retrieve pin %i conf0 reg\n", + pin); + continue; + } + conf0 = readl(reg); + + reg = byt_gpio_reg(vg, pin, BYT_VAL_REG); + if (!reg) { + seq_printf(s, + "Could not retrieve pin %i val reg\n", pin); + } + val = readl(reg); raw_spin_unlock_irqrestore(&vg->lock, flags); + comm = byt_get_community(vg, pin); + if (!comm) { + seq_printf(s, + "Could not get community for pin %i\n", pin); + continue; + } label = gpiochip_is_requested(chip, i); if (!label) label = "Unrequested"; @@ -1474,12 +1501,12 @@ static void byt_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip) seq_printf(s, " gpio-%-3d (%-20.20s) %s %s %s pad-%-3d offset:0x%03x mux:%d %s%s%s", - i, + pin, label, val & BYT_INPUT_EN ? " " : "in", val & BYT_OUTPUT_EN ? " " : "out", val & BYT_LEVEL ? "hi" : "lo", - vg->range->pins[i], offs, + comm->pad_map[i], comm->pad_map[i] * 32, conf0 & 0x7, conf0 & BYT_TRIG_NEG ? " fall" : " ", conf0 & BYT_TRIG_POS ? " rise" : " ", @@ -1519,6 +1546,18 @@ static void byt_gpio_irq_handler(struct irq_desc *desc) chip->irq_eoi(data); } +static const struct gpio_chip byt_gpio_chip = { + .owner = THIS_MODULE, + .request = gpiochip_generic_request, + .free = gpiochip_generic_free, + .get_direction = byt_gpio_get_direction, + .direction_input = byt_gpio_direction_input, + .direction_output = byt_gpio_direction_output, + .get = byt_gpio_get, + .set = byt_gpio_set, + .dbg_show = byt_gpio_dbg_show, +}; + static void byt_irq_ack(struct irq_data *d) { struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
This patch updates the gpio chip implementation in order to interact with the pin control model: the chip contains reference to SOC data and pin/group/community information is retrieved through the SOC reference. Signed-off-by: Cristina Ciocan <cristina.ciocan@intel.com> --- drivers/pinctrl/intel/pinctrl-baytrail.c | 97 ++++++++++++++++++++++---------- 1 file changed, 68 insertions(+), 29 deletions(-)