On Wed, Jan 4, 2017 at 5:31 PM, Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx> wrote: > The next generation Intel GPIO hardware has two additional registers > PADCFG2 and PADCFG3. The latter is marked as reserved but the former > includes configuration for per-pad hardware debouncer. > > This patch adds support for that in the Intel pinctrl core driver. Since > these are additional features on top of the current generation hardware, > we use revision number and feature flags to enable this if detected. Few comments below. > @@ -244,6 +264,7 @@ static void intel_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s, > unsigned pin) > { > struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); > + void __iomem *padcfg; Perhaps padcfg2 ? I don't remember if we had such a pattern earlier in the code, though it would be better for my opinion to map local variable name to register name. At least you are using it later here. > @@ -552,6 +596,53 @@ static int intel_config_set_pull(struct intel_pinctrl *pctrl, unsigned pin, > return ret; > } > > +static int intel_config_set_debounce(struct intel_pinctrl *pctrl, unsigned pin, > + unsigned debounce) > +{ > + void __iomem *padcfg0, *padcfg2; > + unsigned long flags; > + u32 value0, value2; > + int ret = 0; > + > + padcfg2 = intel_get_padcfg(pctrl, pin, PADCFG2); > + if (!padcfg2) > + return -ENOTSUPP; > + > + padcfg0 = intel_get_padcfg(pctrl, pin, PADCFG0); > + > + raw_spin_lock_irqsave(&pctrl->lock, flags); > + > + value0 = readl(padcfg0); > + value2 = readl(padcfg2); > + > + /* Disable glitch filter and debouncer */ > + value0 &= ~PADCFG0_PREGFRXSEL; > + value2 &= ~(PADCFG2_DEBEN | PADCFG2_DEBOUNCE_MASK); > + > + if (debounce) { > + unsigned long v; > + > + v = order_base_2(debounce * 1000 / DEBOUNCE_PERIOD); > + if (v < 3 || v > 15) { > + ret = -EINVAL; > + } else { > + /* Enable glitch filter and debouncer */ > + value0 |= PADCFG0_PREGFRXSEL; > + value2 |= v << PADCFG2_DEBOUNCE_SHIFT; > + value2 |= PADCFG2_DEBEN; > + } > + } > + > + if (!ret) { Would be cleaner to if (ret) goto exit_unlock; ... > + writel(value0, padcfg0); > + writel(value2, padcfg2); > + } > + exit_unlock: ? Or even do this above if (v < 3 || v > 15) { ret = ... goto exit_unlock; } > + raw_spin_unlock_irqrestore(&pctrl->lock, flags); > + > + return ret; > +} > @@ -1000,6 +1110,18 @@ int intel_pinctrl_probe(struct platform_device *pdev, > if (IS_ERR(regs)) > return PTR_ERR(regs); > > + /* > + * Determine community features based on the revision if > + * not specified already. > + */ > + if (!community->features) { > + u32 rev; > + > + rev = (readl(regs + REVID) & REVID_MASK) >> REVID_SHIFT; > + if (rev >= 0x94) Can we define revision ID as well? > + community->features |= FEATURE_DEBOUNCE; > + } > + > /* Read offset of the pad configuration registers */ > padbar = readl(regs + PADBAR); > > @@ -1073,6 +1195,7 @@ int intel_pinctrl_suspend(struct device *dev) > pads = pctrl->context.pads; > for (i = 0; i < pctrl->soc->npins; i++) { > const struct pinctrl_pin_desc *desc = &pctrl->soc->pins[i]; > + void __iomem *padcfg; padcfg2 > @@ -72,11 +73,15 @@ struct intel_community { > unsigned pin_base; > unsigned gpp_size; > size_t npins; > + unsigned features; > void __iomem *regs; > void __iomem *pad_regs; > size_t ngpps; > }; > > +/* Additional features supported by the hardware */ > +#define FEATURE_DEBOUNCE BIT(0) INTEL_PINCTRL_FEATURE_* ? -- With Best Regards, Andy Shevchenko -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html