| Message ID | 1507266491-73971-1-git-send-email-preid@electromag.com.au |
|---|---|
| Headers | show |
| Series | pinctrl: mcp32s08: add support for mcp23018 | expand |
Hi, On Fri, Oct 06, 2017 at 01:08:11PM +0800, Phil Reid wrote: > Variable mask and val are not used in the mcp_pinconf_set(). > > Signed-off-by: Phil Reid <preid@electromag.com.au> Thanks, those are leftovers from before I added mcp_set_bit. Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> -- Sebastian > --- > drivers/pinctrl/pinctrl-mcp23s08.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c > index 8dceaa1..8e461cc 100644 > --- a/drivers/pinctrl/pinctrl-mcp23s08.c > +++ b/drivers/pinctrl/pinctrl-mcp23s08.c > @@ -279,8 +279,7 @@ static int mcp_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin, > { > struct mcp23s08 *mcp = pinctrl_dev_get_drvdata(pctldev); > enum pin_config_param param; > - u32 arg, mask; > - u16 val; > + u32 arg; > int ret = 0; > int i; > > @@ -290,8 +289,6 @@ static int mcp_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin, > > switch (param) { > case PIN_CONFIG_BIAS_PULL_UP: > - val = arg ? 0xFFFF : 0x0000; > - mask = BIT(pin); > ret = mcp_set_bit(mcp, MCP_GPPU, pin, arg); > break; > default: > -- > 1.8.3.1 >
Hi, On Fri, Oct 06, 2017 at 01:08:10PM +0800, Phil Reid wrote: > The irq_active_high flag is for controlling the polarity of the output > from the mcp23s08 series devices. The polarity of the irq could be altered > by additional logic (eg inverters) between the device and irq input device. > The device-tree already allows for this as the irq can be specified in the > binding. So hardcoding it in the driver is overly restrictive. > > There are no inkernel users of the mcp23s08 driver with irq's. > > Signed-off-by: Phil Reid <preid@electromag.com.au> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> -- Sebastian > --- > drivers/pinctrl/pinctrl-mcp23s08.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c > index 150f216..8dceaa1 100644 > --- a/drivers/pinctrl/pinctrl-mcp23s08.c > +++ b/drivers/pinctrl/pinctrl-mcp23s08.c > @@ -630,11 +630,6 @@ static int mcp23s08_irq_setup(struct mcp23s08 *mcp) > int err; > unsigned long irqflags = IRQF_ONESHOT | IRQF_SHARED; > > - if (mcp->irq_active_high) > - irqflags |= IRQF_TRIGGER_HIGH; > - else > - irqflags |= IRQF_TRIGGER_LOW; > - > err = devm_request_threaded_irq(chip->parent, mcp->irq, NULL, > mcp23s08_irq, > irqflags, dev_name(chip->parent), mcp); > -- > 1.8.3.1 >
Hi, On Fri, Oct 06, 2017 at 01:08:09PM +0800, Phil Reid wrote: > The mcp23s08 series device can be configured for wired and interupts > using an external pullup and open drain output via the IOCON_ODR bit. > And "microchip,irq-open-drain" property to enable this. > > Signed-off-by: Phil Reid <preid@electromag.com.au> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> -- Sebastian > --- > drivers/pinctrl/pinctrl-mcp23s08.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c > index 8d356df..150f216 100644 > --- a/drivers/pinctrl/pinctrl-mcp23s08.c > +++ b/drivers/pinctrl/pinctrl-mcp23s08.c > @@ -780,6 +780,7 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, > { > int status, ret; > bool mirror = false; > + bool open_drain = false; > > mutex_init(&mcp->lock); > > @@ -876,6 +877,8 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, > "microchip,irq-active-high"); > > mirror = device_property_read_bool(dev, "microchip,irq-mirror"); > + open_drain = device_property_read_bool(dev, > + "microchip,irq-open-drain"); > } > > if ((status & IOCON_SEQOP) || !(status & IOCON_HAEN) || mirror || > @@ -891,6 +894,9 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, > if (mirror) > status |= IOCON_MIRROR | (IOCON_MIRROR << 8); > > + if (open_drain) > + status |= IOCON_ODR | (IOCON_ODR << 8); > + > if (type == MCP_TYPE_S18 || type == MCP_TYPE_018) > status |= IOCON_INTCC | (IOCON_INTCC << 8); > > -- > 1.8.3.1 >
Hi, On Fri, Oct 06, 2017 at 01:08:07PM +0800, Phil Reid wrote: > This adds the required definitions for the mcp23018 which is the i2c > variant of the mcp23s18. > > Signed-off-by: Phil Reid <preid@electromag.com.au> > --- Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> -- Sebastian > drivers/pinctrl/pinctrl-mcp23s08.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c > index 3e40d42..8d356df 100644 > --- a/drivers/pinctrl/pinctrl-mcp23s08.c > +++ b/drivers/pinctrl/pinctrl-mcp23s08.c > @@ -25,6 +25,7 @@ > #define MCP_TYPE_008 2 > #define MCP_TYPE_017 3 > #define MCP_TYPE_S18 4 > +#define MCP_TYPE_018 5 > > #define MCP_MAX_DEV_PER_CS 8 > > @@ -837,6 +838,13 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, > mcp->chip.ngpio = 16; > mcp->chip.label = "mcp23017"; > break; > + > + case MCP_TYPE_018: > + mcp->regmap = devm_regmap_init_i2c(data, &mcp23x17_regmap); > + mcp->reg_shift = 1; > + mcp->chip.ngpio = 16; > + mcp->chip.label = "mcp23018"; > + break; > #endif /* CONFIG_I2C */ > > default: > @@ -883,7 +891,7 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, > if (mirror) > status |= IOCON_MIRROR | (IOCON_MIRROR << 8); > > - if (type == MCP_TYPE_S18) > + if (type == MCP_TYPE_S18 || type == MCP_TYPE_018) > status |= IOCON_INTCC | (IOCON_INTCC << 8); > > ret = mcp_write(mcp, MCP_IOCON, status); > @@ -964,6 +972,10 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, > .compatible = "microchip,mcp23017", > .data = (void *) MCP_TYPE_017, > }, > + { > + .compatible = "microchip,mcp23018", > + .data = (void *) MCP_TYPE_018, > + }, > /* NOTE: The use of the mcp prefix is deprecated and will be removed. */ > { > .compatible = "mcp,mcp23008", > @@ -1013,6 +1025,7 @@ static int mcp230xx_probe(struct i2c_client *client, > static const struct i2c_device_id mcp230xx_id[] = { > { "mcp23008", MCP_TYPE_008 }, > { "mcp23017", MCP_TYPE_017 }, > + { "mcp23018", MCP_TYPE_018 }, > { }, > }; > MODULE_DEVICE_TABLE(i2c, mcp230xx_id); > -- > 1.8.3.1 >
Hi, On Fri, Oct 06, 2017 at 01:08:05PM +0800, Phil Reid wrote: > This allows PINCTRL to be selected manually to allow enabling of the > mcp23s08 i2c/spi gpio driver. Which is not platform specific. FWIW with i2c/spi based pin controllers available it makes sense to have this available everywhere. I actually did test my changes using an i2c-tiny-usb controller with my x86 based notebook, so: Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> -- Sebastian > Signed-off-by: Phil Reid <preid@electromag.com.au> > --- > drivers/pinctrl/Kconfig | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig > index 1778cf4..8da29e9 100644 > --- a/drivers/pinctrl/Kconfig > +++ b/drivers/pinctrl/Kconfig > @@ -2,11 +2,10 @@ > # PINCTRL infrastructure and drivers > # > > -config PINCTRL > - bool > +menuconfig PINCTRL > + bool "Pin controllers" > > -menu "Pin controllers" > - depends on PINCTRL > +if PINCTRL > > config GENERIC_PINCTRL_GROUPS > bool > @@ -379,4 +378,4 @@ config PINCTRL_TB10X > depends on OF && ARC_PLAT_TB10X > select GPIOLIB > > -endmenu > +endif > -- > 1.8.3.1 >
On Fri, Oct 6, 2017 at 7:08 AM, Phil Reid <preid@electromag.com.au> wrote: > This allows PINCTRL to be selected manually to allow enabling of the > mcp23s08 i2c/spi gpio driver. Which is not platform specific. > > Signed-off-by: Phil Reid <preid@electromag.com.au> Patch applied with Sebastian's review tag. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 6, 2017 at 7:08 AM, Phil Reid <preid@electromag.com.au> wrote: > The mcp23s08 series device can be configured for wired and interupts > using an external pullup and open drain output via the IOCON_ODR bit. > And "microchip,irq-open-drain" property to enable this. > > Signed-off-by: Phil Reid <preid@electromag.com.au> See my comment on patch 4/7 for directions on what to do with this patch. Use standard binding, look at irq descriptor props, IRQF_SHARED. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 6, 2017 at 7:08 AM, Phil Reid <preid@electromag.com.au> wrote: > The irq_active_high flag is for controlling the polarity of the output > from the mcp23s08 series devices. The polarity of the irq could be altered > by additional logic (eg inverters) between the device and irq input device. > The device-tree already allows for this as the irq can be specified in the > binding. So hardcoding it in the driver is overly restrictive. > > There are no inkernel users of the mcp23s08 driver with irq's. > > Signed-off-by: Phil Reid <preid@electromag.com.au> Look this up from the irq descriptor as discussed in 4/7. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 6, 2017 at 7:08 AM, Phil Reid <preid@electromag.com.au> wrote: > Variable mask and val are not used in the mcp_pinconf_set(). > > Signed-off-by: Phil Reid <preid@electromag.com.au> Patch applied with Sebastian's ACK. Please rebase the remaining patches on my devel branch and focus on getting open drain and active high/low things right. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 6, 2017 at 7:08 AM, Phil Reid <preid@electromag.com.au> wrote: > The polarity of the irq could be altered > by additional logic (eg inverters) between the device and irq input device. Marc Zyngier has pointed out that inverters should be modeled using hierarchichal irq domains. You would need to add a new irqchip to invert the line. See this PDF: https://elinux.org/images/8/8c/Zyngier.pdf drivers/irqchips/irq-uniphier-aidet.c supports inversion of IRQs for example. For this it uses irq_domain_create_hierarchy() I guess it would be helpful with a reusable generic "inverter irq chip", that you can just enable and slam into your device tree to tell the system that a line is statically inverted, i.e. non-programmable logic on the board. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/10/2017 17:04, Linus Walleij wrote: > On Fri, Oct 6, 2017 at 7:08 AM, Phil Reid <preid@electromag.com.au> wrote: > >> The polarity of the irq could be altered >> by additional logic (eg inverters) between the device and irq input device. > > Marc Zyngier has pointed out that inverters should be modeled > using hierarchichal irq domains. You would need to add a new > irqchip to invert the line. > > See this PDF: > https://elinux.org/images/8/8c/Zyngier.pdf > > drivers/irqchips/irq-uniphier-aidet.c supports inversion of IRQs > for example. For this it uses irq_domain_create_hierarchy() > > I guess it would be helpful with a reusable generic "inverter > irq chip", that you can just enable and slam into your device > tree to tell the system that a line is statically inverted, i.e. > non-programmable logic on the board. > G'day Linus. Thanks for the pointers, I'll see what I can come up with.
G'day Linus, did this one in the series get applied? or did you have a problem with it and want it rebased with the other 3 patches needing work. haven't seen any of them on devel branch git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git On 9/10/2017 05:18, Sebastian Reichel wrote: > Hi, > > On Fri, Oct 06, 2017 at 01:08:07PM +0800, Phil Reid wrote: >> This adds the required definitions for the mcp23018 which is the i2c >> variant of the mcp23s18. >> >> Signed-off-by: Phil Reid <preid@electromag.com.au> >> --- > > Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> > > -- Sebastian > >> drivers/pinctrl/pinctrl-mcp23s08.c | 15 ++++++++++++++- >> 1 file changed, 14 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c >> index 3e40d42..8d356df 100644 >> --- a/drivers/pinctrl/pinctrl-mcp23s08.c >> +++ b/drivers/pinctrl/pinctrl-mcp23s08.c >> @@ -25,6 +25,7 @@ >> #define MCP_TYPE_008 2 >> #define MCP_TYPE_017 3 >> #define MCP_TYPE_S18 4 >> +#define MCP_TYPE_018 5 >> >> #define MCP_MAX_DEV_PER_CS 8 >> >> @@ -837,6 +838,13 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, >> mcp->chip.ngpio = 16; >> mcp->chip.label = "mcp23017"; >> break; >> + >> + case MCP_TYPE_018: >> + mcp->regmap = devm_regmap_init_i2c(data, &mcp23x17_regmap); >> + mcp->reg_shift = 1; >> + mcp->chip.ngpio = 16; >> + mcp->chip.label = "mcp23018"; >> + break; >> #endif /* CONFIG_I2C */ >> >> default: >> @@ -883,7 +891,7 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, >> if (mirror) >> status |= IOCON_MIRROR | (IOCON_MIRROR << 8); >> >> - if (type == MCP_TYPE_S18) >> + if (type == MCP_TYPE_S18 || type == MCP_TYPE_018) >> status |= IOCON_INTCC | (IOCON_INTCC << 8); >> >> ret = mcp_write(mcp, MCP_IOCON, status); >> @@ -964,6 +972,10 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, >> .compatible = "microchip,mcp23017", >> .data = (void *) MCP_TYPE_017, >> }, >> + { >> + .compatible = "microchip,mcp23018", >> + .data = (void *) MCP_TYPE_018, >> + }, >> /* NOTE: The use of the mcp prefix is deprecated and will be removed. */ >> { >> .compatible = "mcp,mcp23008", >> @@ -1013,6 +1025,7 @@ static int mcp230xx_probe(struct i2c_client *client, >> static const struct i2c_device_id mcp230xx_id[] = { >> { "mcp23008", MCP_TYPE_008 }, >> { "mcp23017", MCP_TYPE_017 }, >> + { "mcp23018", MCP_TYPE_018 }, >> { }, >> }; >> MODULE_DEVICE_TABLE(i2c, mcp230xx_id); >> -- >> 1.8.3.1 >>
On Thu, Oct 19, 2017 at 9:00 AM, Phil Reid <preid@electromag.com.au> wrote: > did this one in the series get applied? > or did you have a problem with it and want it rebased with the other 3 > patches needing work. No I just missed this patch, sorry :( I have applied it now. > haven't seen any of them on devel branch > git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git This is on linux-pinctrl.git as it is a pin control driver now. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html