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

[Bjorn Andersson <bjorn@kryo.se>]

On Wed, Aug 6, 2014 at 8:02 AM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:
> 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.
>

I'm not sure I think the "optimization" is worth the cluttered names
of these defines, but I will give it a spin and see how it looks!

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

Either we have a table like the other pinctrl drivers, or we just go
with custom parsing where we shove the register value straight into
devicetree. Although the latter would reduce the number of mapping
tables we need in the kernel, it seems to not follow the general way
of doing things with pinctrl.

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

Note that for GPIOs we have normal/gpio, paried and a set of functions
(if we ignore the dtest ones). And for MPPs we have digital and analog
as paired and unpaired. Input/output should be controlled with the
separate means already available (gpio api, output-{low,high}.

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

If we're going to maintain this "table" in the kernel then I would
greatly prefer that we just stick with the standard way of
representing it in the pinctrl drivers. My concern is still related to
me lacking the appropriate documentation to create these tables.

I introduced the necessary tables for pm8058 and pm8921 and it looks
reasonable. I'll try to finish it up tomorrow and send out a copy for
you to have a look.

Regards,
Bjorn