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

["Ivan T. Ivanov" <iivanov@mm-sol.com>]

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

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