Re: [PATCH v3 1/6] pinctrl: Device tree bindings for Qualcomm pm8xxx gpio block

Ivan T. Ivanov
2014-08-25T13:14:13Z

On Wed, 2014-08-20 at 15:27 -0700, Bjorn Andersson wrote:
> On Mon 11 Aug 08:40 PDT 2014, Ivan T. Ivanov wrote:
> [...]
> > diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt
> [...]
> > +SUBNODES:
> [...]
> > +- function:
> > + Usage: required
> > + Value type: <string>
> > + Definition: Specify the alternative function to be configured for the
> > + specified pins. Valid values are:
> > + "normal",
> > + "paired",
> > + "func1",
> > + "func2",
> > + "dtest1",
> > + "dtest2",
> > + "dtest3",
> > + "dtest4"
> > +
>
> I still think it looks better with "real" functions, but I rather go with this
> than discussing it forever.
>
> > +- qcom,pull-up-strength:
> > + Usage: optional
> > + Value type: <u32>
> > + Definition: Specifies the strength to use for pull up, if selected.
> > + Valid values are; as defined in
> > + <dt-bindings/pinctrl/qcom,pmic-gpio.h>:
> > + 1: 30uA (PMIC_GPIO_PULL_UP_30)
> > + 2: 1.5uA (PMIC_GPIO_PULL_UP_1P5)
> > + 3: 31.5uA (PMIC_GPIO_PULL_UP_31P5)
> > + 4: 1.5uA + 30uA boost (PMIC_GPIO_PULL_UP_1P5_30)
> > + If this property is ommited 30uA strength will be used if
> > + pull up is selected
>
> I would prefer if we decrement this one step, as it will follow the register
> values of both the "ssbi" and "spmi" based pmics.

Ok.

>
> [...]
> > +
> > +- power-source:
> > + Usage: optional
> > + Value type: <u32>
> > + Definition: Selects the power source for the specified pins. Valid
> > + power sources are defined per chip in
> > + <dt-bindings/pinctrl/qcom,pmic-gpio.h>
> > + xxxx_GPIO_L6, xxxx_GPIO_L5...
>
> After implementing this my only concern is that debugfs output is not as useful
> anymore; saying VIN2 instead of S4. But I can live with this.
>
> > diff --git a/include/dt-bindings/pinctrl/qcom,pmic-gpio.h b/include/dt-bindings/pinctrl/qcom,pmic-gpio.h
> [...]
> > +
> > +#define PM8038_GPIO1_2_LPG_DRV PMIC_GPIO_FUNC_FUNC1
> > +#define PM8038_GPIO3_5V_BOOST_EN PMIC_GPIO_FUNC_FUNC1
> > +#define PM8038_GPIO4_SSBI_ALT_CLK PMIC_GPIO_FUNC_FUNC1
> > +#define PM8038_GPIO5_6_EXT_REG_EN PMIC_GPIO_FUNC_FUNC1
> > +#define PM8038_GPIO10_11_EXT_REG_EN PMIC_GPIO_FUNC_FUNC1
> > +#define PM8038_GPIO6_7_CLK PMIC_GPIO_FUNC_FUNC1
> > +#define PM8038_GPIO9_BAT_ALRM_OUT PMIC_GPIO_FUNC_FUNC1
> > +#define PM8038_GPIO6_12_KYPD_DRV PMIC_GPIO_FUNC_FUNC2
> > +
> > +#define PM8058_GPIO7_8_MP3_CLK PMIC_GPIO_FUNC_FUNC1
> > +#define PM8058_GPIO7_8_BCLK_19P2MHZ PMIC_GPIO_FUNC_FUNC2
> > +#define PM8058_GPIO9_26_KYPD_DRV PMIC_GPIO_FUNC_FUNC1
> > +#define PM8058_GPIO21_23_UART_TX PMIC_GPIO_FUNC_FUNC2
> > +#define PM8058_GPIO24_26_LPG_DRV PMIC_GPIO_FUNC_FUNC2
> > +#define PM8058_GPIO33_BCLK_19P2MHZ PMIC_GPIO_FUNC_FUNC1
> > +#define PM8058_GPIO34_35_MP3_CLK PMIC_GPIO_FUNC_FUNC1
> > +#define PM8058_GPIO36_BCLK_19P2MHZ PMIC_GPIO_FUNC_FUNC1
> > +#define PM8058_GPIO37_UPL_OUT PMIC_GPIO_FUNC_FUNC1
> > +#define PM8058_GPIO37_UART_M_RX PMIC_GPIO_FUNC_FUNC2
> > +#define PM8058_GPIO38_XO_SLEEP_CLK PMIC_GPIO_FUNC_FUNC1
> > +#define PM8058_GPIO38_39_CLK_32KHZ PMIC_GPIO_FUNC_FUNC2
> > +#define PM8058_GPIO39_MP3_CLK PMIC_GPIO_FUNC_FUNC1
> > +#define PM8058_GPIO40_EXT_BB_EN PMIC_GPIO_FUNC_FUNC1
> > +
> > +#define PM8917_GPIO9_18_KEYP_DRV PMIC_GPIO_FUNC_FUNC1
> > +#define PM8917_GPIO20_BAT_ALRM_OUT PMIC_GPIO_FUNC_FUNC1
> > +#define PM8917_GPIO21_23_UART_TX PMIC_GPIO_FUNC_FUNC2
> > +#define PM8917_GPIO25_26_EXT_REG_EN PMIC_GPIO_FUNC_FUNC1
> > +#define PM8917_GPIO37_38_XO_SLEEP_CLK PMIC_GPIO_FUNC_FUNC1
> > +#define PM8917_GPIO37_38_MP3_CLK PMIC_GPIO_FUNC_FUNC2
>
> Stephens argument was that we don't want to have huge tables for the functions
> and I can see his point, it will be some work to build all the tables.
> Adding all this defines is unfortunately doing just that.

It is still little bit simpler :-)

>
> I had a version of my driver that exposed real functions by name from the
> driver, following the pattern we have for other pinctrl drivers and making the
> dts very easy to read. But if you don't think we should go that route then I
> suggest that we just call it func1/func2 and skip these defines.
>
> Regards,
> Bjorn

Regards,
Ivan