by David Collins

[permalink] [raw]

Subject: Re: [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" <[email protected]>
>
> 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 <[email protected]>

(...)
> +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

2014-07-28 08:39:58

by Ivan T. Ivanov

[permalink] [raw]

Subject: Re: [PATCH v2 2/4] pinctrl: qpnp: Qualcomm PMIC pin controller driver

On Fri, 2014-07-25 at 18:43 -0700, David Collins wrote:
> On 07/17/2014 08:25 AM, Ivan T. Ivanov wrote:
> > From: "Ivan T. Ivanov" <[email protected]>
> >
> > 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 <[email protected]>
>
> (...)
> > +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.

True.

> The special functions available for GPIOs (e.g.
> PWM/LPG, clock, keypad, etc.) which are configured via this register are
> used by many boards.

I was not sure what those features are and how to connect the numbers to
the function, which is why I have restricted the values of 0 and 1.

>
> 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.

<snip>

I am working on proposal from Stephen Boyd to encode GPIO/MPP mode and
source select into combined function. Something like this one:

#define PM8XXX_DIGITAL_IN 0
#define PM8XXX_DIGITAL_OUT 1
#define PM8XXX_DIGITAL_IN_OUT 2

...

/* mode and source select */
#define PM8XXX_FUNCTION(m,s) ((m) << 16 | (s))

#define PM8921_GPIO1_14_KYPD_SNS PM8XXX_FUNCTION(PM8XXX_DIGITAL_IN, 1)
#define PM8921_GPIO9_14_KYPD_DRV PM8XXX_FUNCTION(PM8XXX_DIGITAL_OUT, 1)
#define PM8921_GPIO33_35_PWM PM8XXX_FUNCTION(PM8XXX_DIGITAL_OUT, 3)

..

<snip>

> 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.

Yep, will fix it. Thank you for review.

Regards,
Ivan