On Wed, Aug 6, 2014 at 8:02 AM, Ivan T. Ivanov <iivanov@xxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
