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

[Bjorn Andersson]

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.

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/