lkml.org 
[lkml]   [2017]   [Jan]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    SubjectRe: [PATCH 4/6] pinctrl: intel: Add support for hardware debouncer
    On Wed, Jan 4, 2017 at 5:31 PM, Mika Westerberg
    <mika.westerberg@linux.intel.com> 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

    \
     
     \ /
      Last update: 2017-01-05 00:15    [W:4.136 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site