Re: [PATCH] pinctrl/sunxi: adding input-debounce-ns property

[Marjan Pascolo]

Hi Maxime,

Il 17/02/2021 12:03, Maxime Ripard ha scritto:
> Hi,
>
> On Wed, Feb 10, 2021 at 05:22:37PM +0100, Marjan Pascolo wrote:
> > On Allwinner SoC interrupt debounce can be controlled by two oscillator
> > (32KHz and 24MHz) and a prescale divider.
> > Oscillator and prescale divider are set through
> > device tree property "input-debounce" which have 1uS accuracy.
> > For acheive nS precision a new device tree poperty is made
> > named "input-debounce-ns".
> > "input-debounce-ns" is checked only if "input-debounce"
> > property is not defined.
> >
> > Suggested-by: Maxime Ripard <maxime@xxxxxxxxxx>
> > Signed-off-by: Marjan Pascolo <marjan.pascolo@xxxxxxxxx>
> > ---
> > ---
> >   .../pinctrl/allwinner,sun4i-a10-pinctrl.yaml  |  9 +++++++
> >   drivers/pinctrl/sunxi/pinctrl-sunxi.c         | 25 ++++++++++++++++---
> >   2 files changed, 30 insertions(+), 4 deletions(-)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
> > b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
> > index 5240487dfe50..346776de3a44 100644
> > ---
> > a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
> > +++
> > b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
> > @@ -93,6 +93,15 @@ properties:
> >       minItems: 1
> >       maxItems: 5
> >
> > +  input-debounce-ns:
> > +    description:
> > +      Debouncing periods in nanoseconds, one period per interrupt
> > +      bank found in the controller.
> > +      Only checked if input-debounce is not present
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +    minItems: 1
> > +    maxItems: 5
> > +
> This should be a separate patch, with the DT maintainers in Cc.
>
> You should enforce that the properties are mutually exclusive through
> the schema too

I'm sorry, I've ignored documentaion about /Documentation.

I see that some additional YAML operator (like oneOf) are used.

oneOf should fit the schema, but I can't understand if oneOf's options 
must be a literal value, or if could also be a node.

Otherwise I'll use if ..then..else.


> >   patternProperties:
> >     # It's pretty scary, but the basic idea is that:
> >     #   - One node name can start with either s- or r- for PRCM nodes,
> > diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> > b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> > index dc8d39ae045b..869b6d5743ba 100644
> > --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> > +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> > @@ -1335,14 +1335,31 @@ static int sunxi_pinctrl_setup_debounce(struct
> > sunxi_pinctrl *pctl,
> >       struct clk *hosc, *losc;
> >       u8 div, src;
> >       int i, ret;
> > +    /* Keeping for loop below clean */
> > +    const char* debounce_prop_name;
> > +    unsigned long debounce_dividend;
> >
> >       /* Deal with old DTs that didn't have the oscillators */
> >       if (of_clk_get_parent_count(node) != 3)
> >           return 0;
> >
> > +    /*
> > +     * Distinguish between simple input-debounce
> > +     * and new input-debounce-ns
> > +     */
> > +
> I'm not sure that comment should stay, the code is obvious enough
>
> >       /* If we don't have any setup, bail out */
> > -    if (!of_find_property(node, "input-debounce", NULL))
> > -        return 0;
> > +    if (!of_find_property(node, "input-debounce", NULL)) {
> > +        if(!of_find_property(node, "input-debounce-ns", NULL)) {
> > +            return 0;
> > +        } else {
> > +            debounce_prop_name="input-debounce-ns";
> > +            debounce_dividend=NSEC_PER_SEC;
> > +        }
> > +    } else {
> > +        debounce_prop_name="input-debounce";
> > +        debounce_dividend=USEC_PER_SEC;
> > +    }
> This doesn't follow the kernel coding style, make sure to run
> scripts/checkpatch.pl on your patches before sending them.
Spaces between operators, right. After /Documentation submission I'll 
resubmit this one
> Maxime