Re: [PATCH RESEND v2 1/4] pinctrl: Update Qualcomm pm8xxx GPIO parameters definitions

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

On Fri, 2014-07-25 at 18:15 +0300, Ivan T. Ivanov wrote:
> On Thu, 2014-07-24 at 17:23 -0700, Stephen Boyd wrote:
> > On 07/24/14 08:40, Linus Walleij wrote:
> > > On Thu, Jul 24, 2014 at 1:47 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > >
> > >>> Please add these constants to the table of valid power-source values and use
> > >>> something like I did to translate them to register values - it makes the DT
> > >>> much more readable.
> > >> The DT could be similarly readable if we had a bunch of #defines for the
> > >> different VIN settings that resolved to the final register value for
> > >> that pmic. Something like PM8921_GPIO1_14_VPH, PM8921_GPIO19_36_VPH,
> > >> etc. There would be a lot of them, but then the driver could be really
> > >> simple and just jam whatever value is in the DT into the register
> > >> without having to bounce through a mapping table in software to figure
> > >> out the register value. 

This is ok.

> If we did this for the functions also then I
> > >> believe we achieve readability without requiring a bunch of drivers for
> > >> each and every single pmic?


> > > Not sure but it sounds like you want to make the device tree a jam table,
> > > (know about individual register offsets, sequences etc). That has been
> > > throrougly NACKed in the past, because DT is not Open Firmware.
> > >
> > > The exception is pinctrl-single which is restricted to single register
> > > per pin use cases and is still a point of contention...
> > >
> > 
> > I'm not proposing a jam table. I'm proposing that we make the
> > function/source property convenient to the driver by having the actual
> > function field value encoded there instead of some string that has to be
> > translated through a table in a driver. There's still going to be
> > shifting and masking of bits in the driver to put the value in the right
> > place in the register, but we avoid needing N number of drivers for each
> > pmic just to translate strings into integers (for functions) and
> > integers into other integers (for the power source). 
> 
> This sounds good to me. Drawback is that we will have custom 
> function parsing code, but I will try and see what that looks like.

Hm, I played with this, using numbers for functions. Unfortunately it is
not easy possible to hack existing DT pin-control parsing code to use
numbers instead strings, so I ended coping Samsung parsing code, but unless
this is separated in library we will end with huge code duplication for
little benefit.

Furthermore meaning of number 2 and 3, which represent PMIC GPIO Special
Function 1 and 2 are not consistent across PMIC chips. For example KEYPD
function in PM8038 is encoded with 3, but in PM8058 it is 2.

I tend to agree with Bjorn that "function" property should be "normal", 
"paired", "func1", "func2","dtest1", "dtest2", "dtest3", "dtest4" and we
can add new property "qcom,mode" which will select between digital/analog
input/output.

In DT include file we can still have something like this:

/* To be used with "function = <>" */
#define QCOM_GPIO_FUNC_NORMAL		"normal"
#define QCOM_GPIO_FUNC_PAIRED		"paired"
#define QCOM_GPIO_FUNC_FUNC1		"func1"
#define QCOM_GPIO_FUNC_FUNC2		"func2"
...

#define PM8038_GPIO1_2_LPG_DRV		QCOM_GPIO_FUNC_FUNC1
#define PM8038_GPIO3_5V_BOOST_EN	QCOM_GPIO_FUNC_FUNC1
#define PM8038_GPIO4_SSBI_ALT_CLK	QCOM_GPIO_FUNC_FUNC1
#define PM8038_GPIO5_6_EXT_REG_EN	QCOM_GPIO_FUNC_FUNC1
#define PM8038_GPIO10_11_EXT_REG_EN	QCOM_GPIO_FUNC_FUNC1
#define PM8038_GPIO6_7_CLK		QCOM_GPIO_FUNC_FUNC1
#define PM8038_GPIO9_BAT_ALRM_OUT	QCOM_GPIO_FUNC_FUNC1
#define PM8038_GPIO6_12_KYPD_DRV	QCOM_GPIO_FUNC_FUNC2

#define PM8058_GPIO7_8_MP3_CLK		QCOM_GPIO_FUNC_FUNC1
#define PM8058_GPIO7_8_BCLK_19P2MHZ	QCOM_GPIO_FUNC_FUNC2
#define PM8058_GPIO9_26_KYPD_DRV	QCOM_GPIO_FUNC_FUNC1
#define PM8058_GPIO21_23_UART_TX	QCOM_GPIO_FUNC_FUNC2
#define PM8058_GPIO24_26_LPG_DRV	QCOM_GPIO_FUNC_FUNC2
#define PM8058_GPIO33_BCLK_19P2MHZ	QCOM_GPIO_FUNC_FUNC1
#define PM8058_GPIO34_35_MP3_CLK	QCOM_GPIO_FUNC_FUNC1
#define PM8058_GPIO36_BCLK_19P2MHZ	QCOM_GPIO_FUNC_FUNC1
#define PM8058_GPIO37_UPL_OUT		QCOM_GPIO_FUNC_FUNC1
#define PM8058_GPIO37_UART_M_RX		QCOM_GPIO_FUNC_FUNC2
#define PM8058_GPIO38_XO_SLEEP_CLK	QCOM_GPIO_FUNC_FUNC1
#define PM8058_GPIO38_39_CLK_32KHZ	QCOM_GPIO_FUNC_FUNC2
#define PM8058_GPIO39_MP3_CLK		QCOM_GPIO_FUNC_FUNC1
#define PM8058_GPIO40_EXT_BB_EN		QCOM_GPIO_FUNC_FUNC1

#define PM8917_GPIO9_18_KEYP_DRV	QCOM_GPIO_FUNC_FUNC1
#define PM8917_GPIO20_BAT_ALRM_OUT	QCOM_GPIO_FUNC_FUNC1
#define PM8917_GPIO21_23_UART_TX	QCOM_GPIO_FUNC_FUNC2
#define PM8917_GPIO25_26_EXT_REG_EN	QCOM_GPIO_FUNC_FUNC1
#define PM8917_GPIO37_38_XO_SLEEP_CLK	QCOM_GPIO_FUNC_FUNC1
#define PM8917_GPIO37_38_MP3_CLK	QCOM_GPIO_FUNC_FUNC2
...