| Message ID | 334505d3a13a73ad347427b408ed581832434289.1519468782.git.baolin.wang@linaro.org |
|---|---|
| State | Accepted |
| Commit | be520cbc8513ea796c00825e8189d98d67ead33f |
| Headers | show |
| Series | [v2,1/3] dt-bindings: gpio: Add Spreadtrum EIC controller documentation | expand |
On Sat, Feb 24, 2018 at 12:44 PM, Baolin Wang <baolin.wang@linaro.org> wrote: > The Spreadtrum PMIC EIC controller contains only one bank of debounce EIC, > and this bank contains 16 EICs. Each EIC can only be used as input mode, > as well as supporting the debounce and the capability to trigger interrupts > when detecting input signals. > +/* > + * These registers are modified under the irq bus lock and cached to avoid > + * unnecessary writes in bus_sync_unlock. > + */ > +enum { REG_IEV, REG_IE, REG_TRIG, CACHE_NR_REGS }; One item per line. > +static int sprd_pmic_eic_direction_input(struct gpio_chip *chip, > + unsigned int offset) > +{ > + /* EICs are always input, nothing need to do here. */ > + return 0; > +} > + > +static void sprd_pmic_eic_set(struct gpio_chip *chip, unsigned int offset, > + int value) > +{ > + /* EICs are always input, nothing need to do here. */ > +} Remove both. Look at what GPIO core does. > + value |= debounce / 1000; Possible overflow. > + for (n = 0; n < chip->ngpio; n++) { > + if (!(BIT(n) & val)) for_each_set_bit(). At some point you may need just to go across lib/ in the kernel and see what we have there. -- With Best Regards, Andy Shevchenko -- 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, Feb 26, 2018 at 5:01 AM, Baolin Wang <baolin.wang@linaro.org> wrote: > On 25 February 2018 at 20:19, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >> On Sat, Feb 24, 2018 at 12:44 PM, Baolin Wang <baolin.wang@linaro.org> wrote: >>> +static int sprd_pmic_eic_direction_input(struct gpio_chip *chip, >>> + unsigned int offset) >>> +{ >>> + /* EICs are always input, nothing need to do here. */ >>> + return 0; >>> +} >>> + >>> +static void sprd_pmic_eic_set(struct gpio_chip *chip, unsigned int offset, >>> + int value) >>> +{ >>> + /* EICs are always input, nothing need to do here. */ >>> +} >> >> Remove both. >> >> Look at what GPIO core does. > > I've checked the GPIO core, we need the > sprd_pmic_eic_direction_input() returns 0, since user can set GPIOD_IN > flag when requesting one GPIO, otherwise it will return errors. Right. I thought it depends on presence of direction_output(). > We also need one dummy sprd_pmic_eic_set() when setting debounce for > one GPIO, otherwise it will return errors. This is pretty much a "feature" of GPIO framework. It shouldn't require ->set() by logic if there is no output facility. OK. >>> + for (n = 0; n < chip->ngpio; n++) { >>> + if (!(BIT(n) & val)) >> >> for_each_set_bit(). >> >> At some point you may need just to go across lib/ in the kernel and >> see what we have there. > > I've considered the for_each_set_bit(), it need one 'unsigned long' > type parameter, but we get the value from regmap is 'u32' type. So we > need one extra conversion from 'u32' to 'unsigned long' like: > > unsigned long reg = val; > > for_each_set_bit(n, ®, chip->ngpio) { > ....... > } > > If you like this conversion, then I can change to use > for_each_set_bit(). Thanks. Wouldn't it work like unsigned long val; ...regmap_read(..., &val); ? -- With Best Regards, Andy Shevchenko -- 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 27 February 2018 at 23:54, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, Feb 27, 2018 at 4:35 AM, Baolin Wang <baolin.wang@linaro.org> wrote: >> On 26 February 2018 at 20:02, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >>> On Mon, Feb 26, 2018 at 5:01 AM, Baolin Wang <baolin.wang@linaro.org> wrote: >>>> On 25 February 2018 at 20:19, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > >>>>>> + for (n = 0; n < chip->ngpio; n++) { >>>>>> + if (!(BIT(n) & val)) >>>>> >>>>> for_each_set_bit(). >>>>> >>>>> At some point you may need just to go across lib/ in the kernel and >>>>> see what we have there. >>>> >>>> I've considered the for_each_set_bit(), it need one 'unsigned long' >>>> type parameter, but we get the value from regmap is 'u32' type. So we >>>> need one extra conversion from 'u32' to 'unsigned long' like: >>>> >>>> unsigned long reg = val; >>>> >>>> for_each_set_bit(n, ®, chip->ngpio) { >>>> ....... >>>> } >>>> >>>> If you like this conversion, then I can change to use >>>> for_each_set_bit(). Thanks. >>> >>> Wouldn't it work like >>> >>> unsigned long val; >>> >>> ...regmap_read(..., &val); >>> >>> ? >> >> It can not work, regmap_read() expects 'unsigned int *'. > > Ah, OK, than the temporary variable is a left approach. > >> But I can >> convert it like this: >> >> for_each_set_bit(n, (unsigned long *)&val, chip->ngpio) { >> ....... >> } > > No, this is a boilerplate for static analyzers and definitely UB. OK. Thanks for pointing this issue out. -- Baolin.wang Best Regards -- 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 Fri, Mar 2, 2018 at 11:48 AM, Baolin Wang <baolin.wang@linaro.org> wrote: > On 2 March 2018 at 17:53, Linus Walleij <linus.walleij@linaro.org> wrote: >> This is looking nice! >> >> I guess we agreed with the input maintainer that what >> needs to happen is to implement edge trigger emulation >> for all variants in the driver, test it with the keys and >> then we should be done. > > I want to send one separate patch to implement edge trigger emulation > for EIC drivers, since we can have some talks for the implementation. > If there are no other issues for this patch set, I will send out the > V3 after fixing the issues in patch 3 pointed by Andy. Is it OK? OK go for it. 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
diff --git a/Documentation/devicetree/bindings/gpio/gpio-eic-sprd.txt b/Documentation/devicetree/bindings/gpio/gpio-eic-sprd.txt new file mode 100644 index 0000000..93d98d0 --- /dev/null +++ b/Documentation/devicetree/bindings/gpio/gpio-eic-sprd.txt @@ -0,0 +1,97 @@ +Spreadtrum EIC controller bindings + +The EIC is the abbreviation of external interrupt controller, which can +be used only in input mode. The Spreadtrum platform has 2 EIC controllers, +one is in digital chip, and another one is in PMIC. The digital chip EIC +controller contains 4 sub-modules: EIC-debounce, EIC-latch, EIC-async and +EIC-sync. But the PMIC EIC controller contains only one EIC-debounce sub- +module. + +The EIC-debounce sub-module provides up to 8 source input signal +connections. A debounce mechanism is used to capture the input signals' +stable status (millisecond resolution) and a single-trigger mechanism +is introduced into this sub-module to enhance the input event detection +reliability. In addition, this sub-module's clock can be shut off +automatically to reduce power dissipation. Moreover the debounce range +is from 1ms to 4s with a step size of 1ms. The input signal will be +ignored if it is asserted for less than 1 ms. + +The EIC-latch sub-module is used to latch some special power down signals +and generate interrupts, since the EIC-latch does not depend on the APB +clock to capture signals. + +The EIC-async sub-module uses a 32kHz clock to capture the short signals +(microsecond resolution) to generate interrupts by level or edge trigger. + +The EIC-sync is similar with GPIO's input function, which is a synchronized +signal input register. It can generate interrupts by level or edge trigger +when detecting input signals. + +Required properties: +- compatible: Should be one of the following: + "sprd,sc9860-eic-debounce", + "sprd,sc9860-eic-latch", + "sprd,sc9860-eic-async", + "sprd,sc9860-eic-sync", + "sprd,sc27xx-eic". +- reg: Define the base and range of the I/O address space containing + the GPIO controller registers. +- gpio-controller: Marks the device node as a GPIO controller. +- #gpio-cells: Should be <2>. The first cell is the gpio number and + the second cell is used to specify optional parameters. +- interrupt-controller: Marks the device node as an interrupt controller. +- #interrupt-cells: Should be <2>. Specifies the number of cells needed + to encode interrupt source. +- interrupts: Should be the port interrupt shared by all the gpios. + +Example: + eic_debounce: gpio@40210000 { + compatible = "sprd,sc9860-eic-debounce"; + reg = <0 0x40210000 0 0x80>; + gpio-controller; + #gpio-cells = <2>; + interrupt-controller; + #interrupt-cells = <2>; + interrupts = <GIC_SPI 52 IRQ_TYPE_LEVEL_HIGH>; + }; + + eic_latch: gpio@40210080 { + compatible = "sprd,sc9860-eic-latch"; + reg = <0 0x40210080 0 0x20>; + gpio-controller; + #gpio-cells = <2>; + interrupt-controller; + #interrupt-cells = <2>; + interrupts = <GIC_SPI 52 IRQ_TYPE_LEVEL_HIGH>; + }; + + eic_async: gpio@402100a0 { + compatible = "sprd,sc9860-eic-async"; + reg = <0 0x402100a0 0 0x20>; + gpio-controller; + #gpio-cells = <2>; + interrupt-controller; + #interrupt-cells = <2>; + interrupts = <GIC_SPI 52 IRQ_TYPE_LEVEL_HIGH>; + }; + + eic_sync: gpio@402100c0 { + compatible = "sprd,sc9860-eic-sync"; + reg = <0 0x402100c0 0 0x20>; + gpio-controller; + #gpio-cells = <2>; + interrupt-controller; + #interrupt-cells = <2>; + interrupts = <GIC_SPI 52 IRQ_TYPE_LEVEL_HIGH>; + }; + + pmic_eic: gpio@300 { + compatible = "sprd,sc27xx-eic"; + reg = <0x300>; + interrupt-parent = <&sc2731_pmic>; + interrupts = <5 IRQ_TYPE_LEVEL_HIGH>; + gpio-controller; + #gpio-cells = <2>; + interrupt-controller; + #interrupt-cells = <2>; + };
This patch adds the device tree bindings for the Spreadtrum EIC controller. The EIC can be seen as a special type of GPIO, which can only be used as input mode. Signed-off-by: Baolin Wang <baolin.wang@linaro.org> --- Changes since v1: - Fix some typos and grammar issues. - Add more explanation to make things clear. - List all device nodes as examples. --- .../devicetree/bindings/gpio/gpio-eic-sprd.txt | 97 ++++++++++++++++++++ 1 file changed, 97 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpio/gpio-eic-sprd.txt -- 1.7.9.5 -- 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