lkml.org 
[lkml]   [2014]   [Jul]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 2/4] pinctrl: qpnp: Qualcomm PMIC pin controller driver
    On 07/17/2014 08:25 AM, Ivan T. Ivanov wrote:
    > From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
    >
    > This is the pinctrl, pinmux, pinconf and gpiolib driver for the
    > Qualcomm GPIO and MPP sub-function blocks found in the PMIC chips.
    > QPNP_REG_STATUS1_GPIO_EN_REV0_MASK
    > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>

    (...)
    > +static int qpnp_conv_to_pin(struct qpnp_pinctrl *qctrl,
    > + struct qpnp_padinfo *pad, unsigned param,
    > + unsigned val)
    (...)
    > + switch (param) {
    (...)
    > + case PIN_CONFIG_OUTPUT:
    > + nattrs = 3;
    > + attr[0].addr = QPNP_REG_MODE_CTL;
    > + attr[0].shift = QPNP_REG_OUT_SRC_SEL_SHIFT;
    > + attr[0].mask = QPNP_REG_OUT_SRC_SEL_MASK;
    > + attr[0].val = !!val;

    It seems that this patch provides no means to configure the output source
    select bits to be anything besides 0 (constant low) or 1 (constant high).
    Some non-generic property is needed to configure this for both GPIOs and
    MPPs. Passing the value in via the output-high property does not seem
    like a good approach since that is a generic pin config property that is
    defined to take no value. The special functions available for GPIOs (e.g.
    PWM/LPG, clock, keypad, etc.) which are configured via this register are
    used by many boards.

    Something else to consider is that QPNP_REG_OUT_SRC_SEL_MASK is being
    defined as 0xf which would imply that there are 16 possible output source
    select options. While technically true, this makes the situation more
    complicated since half of those options are the inverted version of the
    other half. In the GPIO hardware this corresponds to an 8-way mux
    followed by an XOR gate to conditionally invert the mux output. If output
    source select is handled this way, then the following values would need to
    be supported in device tree for GPIOs:
    * 0: constant low (already supported via output-low;)
    * 1: constant high (already supported via output-high;)
    * 2: paired GPIO
    * 3: inverted paired GPIO
    * 4: special function 1
    * 5: inverted special function 1
    * 6: special function 2
    * 7: inverted special function 2
    * 8: dtest1
    * 9: inverted dtest1
    * 10: dtest2
    * 11: inverted dtest2
    * 12: dtest3
    * 13: inverted dtest3
    * 14: dtest4
    * 15: inverted dtest4
    The same options are supported by MPPs except for special function 1,
    inverted special function 1, special function 2, and inverted special
    function 2.

    If the output source select register parameter is instead treated as a
    3-bit value along with an inversion bit, then the list of output selection
    options that needs to be supported in device tree is cut in half:
    * 0: constant (already supported)
    * 1: paired GPIO
    * 2: special function 1
    * 3: special function 2
    * 4: dtest1
    * 5: dtest2
    * 6: dtest3
    * 7: dtest4
    Another DT pin configuration property would then need to be used to
    specify if the signal should be inverted or not.

    > + attr[1].addr = QPNP_REG_MODE_CTL;
    > + attr[1].shift = QPNP_REG_MODE_SEL_SHIFT;
    > + attr[1].mask = QPNP_REG_MODE_SEL_MASK;
    > + attr[1].val = QPNP_PIN_MODE_DIG_OUT;
    > + attr[2].addr = QPNP_REG_EN_CTL;
    > + attr[2].shift = QPNP_REG_MASTER_EN_SHIFT;
    > + attr[2].mask = QPNP_REG_MASTER_EN_MASK;
    > + attr[2].val = 1;
    > + break;

    (...)

    > +static int qpnp_of_xlate(struct gpio_chip *chip,
    > + const struct of_phandle_args *gpio_desc, u32 *flags)
    > +{
    > + struct qpnp_pinctrl *qctrl = to_qpnp_pinctrl(chip);
    > + struct qpnp_padinfo *pad;
    > +
    > + if (chip->of_gpio_n_cells < 2) {
    > + dev_err(qctrl->dev, "of_gpio_n_cells < 2\n");
    > + return -EINVAL;
    > + }
    > +
    > + pad = qpnp_get_desc(qctrl, gpio_desc->args[0]);
    > + if (!pad)
    > + return -EINVAL;
    > +
    > + if (flags)
    > + *flags = gpio_desc->args[1];
    > +
    > + return gpio_desc->args[0];
    > +}

    This of_xlate callback function will result in the following situation:
    If for example, a device tree consumer node wishes to use PM8941 GPIO 7
    within gpiolib, then it would need to specify a gpiospec like this:
    <&pm8941_gpio 6 0>
    There is an off-by-one issue with the indexing between the hardware GPIO
    numbers (1-based) and the gpiolib gpio offsets (0-based). Do you agree
    that the indexing used within the device tree gpiospecs should match the
    hardware numbering scheme? I feel like this would be much less confusing
    for users to work with. If so, I think that a change to qpnp_of_xlate
    like this would achieve it:

    +#define QPNP_PIN_PHYSICAL_OFFSET 1

    static int qpnp_of_xlate(struct gpio_chip *chip,
    const struct of_phandle_args *gpio_desc, u32 *flags)
    {
    struct qpnp_pinctrl *qctrl = to_qpnp_pinctrl(chip);
    struct qpnp_padinfo *pad;
    + unsigned pin = gpio_desc->args[0] - QPNP_PIN_PHYSICAL_OFFSET;

    if (chip->of_gpio_n_cells < 2) {
    dev_err(qctrl->dev, "of_gpio_n_cells < 2\n");
    return -EINVAL;
    }

    - pad = qpnp_get_desc(qctrl, gpio_desc->args[0]);
    + pad = qpnp_get_desc(qctrl, pin);
    if (!pad)
    return -EINVAL;

    if (flags)
    *flags = gpio_desc->args[1];

    - return gpio_desc->args[0];
    + return pin;
    }

    Take care,
    David Collins

    --
    The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
    hosted by The Linux Foundation


    \
     
     \ /
      Last update: 2014-07-26 04:21    [W:2.644 / U:0.068 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site