diff mbox series

[1/2] pinctrl: baytrail: Fix selecting gpio pinctrl state

Message ID 20240406123506.12078-1-hdegoede@redhat.com
State New
Headers show
Series [1/2] pinctrl: baytrail: Fix selecting gpio pinctrl state | expand

Commit Message

Hans de Goede April 6, 2024, 12:35 p.m. UTC 
For all the "score" pin-groups all the intel_pingroup-s to select
the non GPIO function are re-used for byt_score_gpio_groups[].

But this is incorrect since a pin-group includes the mode setting,
which for the non GPIO functions generally is 1, where as to select
the GPIO function mode must be set to 0.

So the GPIO function needs separate intel_pingroup-s with their own mode
value of 0.

Add foo_gpio entries for each function to byt_score_groups[] and make all
the byt_score_gpio_groups[] entries point to these instead to fix this.

The "sus" pin-groups got this correct until commit 2f46d7f7e959 ("pinctrl:
baytrail: Add pinconf group + function for the pmu_clk") added support for
the pmu_clk pins following the broken "score" model.

Add pmu_clk?_grp_gpio entries to byt_sus_groups[] and point to those
in byt_sus_gpio_groups[] to fix this.

Fixes: 2f46d7f7e959 ("pinctrl: baytrail: Add pinconf group + function for the pmu_clk")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/pinctrl/intel/pinctrl-baytrail.c | 45 ++++++++++++++++++++----
 1 file changed, 38 insertions(+), 7 deletions(-)

Comments

Andy Shevchenko April 6, 2024, 1:52 p.m. UTC | #1 
On Sat, Apr 6, 2024 at 3:37 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> For all the "score" pin-groups all the intel_pingroup-s to select
> the non GPIO function are re-used for byt_score_gpio_groups[].
>
> But this is incorrect since a pin-group includes the mode setting,
> which for the non GPIO functions generally is 1, where as to select
> the GPIO function mode must be set to 0.
>
> So the GPIO function needs separate intel_pingroup-s with their own mode
> value of 0.
>
> Add foo_gpio entries for each function to byt_score_groups[] and make all
> the byt_score_gpio_groups[] entries point to these instead to fix this.
>
> The "sus" pin-groups got this correct until commit 2f46d7f7e959 ("pinctrl:
> baytrail: Add pinconf group + function for the pmu_clk") added support for
> the pmu_clk pins following the broken "score" model.
>
> Add pmu_clk?_grp_gpio entries to byt_sus_groups[] and point to those
> in byt_sus_gpio_groups[] to fix this.

I'm wondering if it's possible to add some code to imply all these. I
mean to have a comparator to the _gpio in the naming and generate them
at runtime and add. In this case if we add / modify the original one
the rest (for _gpio cases) will be done automatically.
Hans de Goede April 6, 2024, 2:09 p.m. UTC | #2 
Hi,

On 4/6/24 3:52 PM, Andy Shevchenko wrote:
> On Sat, Apr 6, 2024 at 3:37 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> For all the "score" pin-groups all the intel_pingroup-s to select
>> the non GPIO function are re-used for byt_score_gpio_groups[].
>>
>> But this is incorrect since a pin-group includes the mode setting,
>> which for the non GPIO functions generally is 1, where as to select
>> the GPIO function mode must be set to 0.
>>
>> So the GPIO function needs separate intel_pingroup-s with their own mode
>> value of 0.
>>
>> Add foo_gpio entries for each function to byt_score_groups[] and make all
>> the byt_score_gpio_groups[] entries point to these instead to fix this.
>>
>> The "sus" pin-groups got this correct until commit 2f46d7f7e959 ("pinctrl:
>> baytrail: Add pinconf group + function for the pmu_clk") added support for
>> the pmu_clk pins following the broken "score" model.
>>
>> Add pmu_clk?_grp_gpio entries to byt_sus_groups[] and point to those
>> in byt_sus_gpio_groups[] to fix this.
> 
> I'm wondering if it's possible to add some code to imply all these. I
> mean to have a comparator to the _gpio in the naming and generate them
> at runtime and add. In this case if we add / modify the original one
> the rest (for _gpio cases) will be done automatically.

Yes some better solution for this would be nice but I don't have time
to work on this, so I suggest to just move forward with this fix for now.

Regards,

Hans
Andy Shevchenko April 6, 2024, 2:21 p.m. UTC | #3 
On Sat, Apr 6, 2024 at 5:09 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 4/6/24 3:52 PM, Andy Shevchenko wrote:
> > On Sat, Apr 6, 2024 at 3:37 PM Hans de Goede <hdegoede@redhat.com> wrote:

...

> > I'm wondering if it's possible to add some code to imply all these. I
> > mean to have a comparator to the _gpio in the naming and generate them
> > at runtime and add. In this case if we add / modify the original one
> > the rest (for _gpio cases) will be done automatically.
>
> Yes some better solution for this would be nice but I don't have time
> to work on this, so I suggest to just move forward with this fix for now.

I'm worrying that the _temporary_ solution becomes a permanent
solution, which is not good.

I'm okay to move forward, but we need to evaluate how harder it is to
implement the better one.
diff mbox series

Patch

diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
index 7865ef587788..019a886a37ae 100644
--- a/drivers/pinctrl/intel/pinctrl-baytrail.c
+++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
@@ -279,32 +279,59 @@  static const unsigned int byt_score_smbus_pins[] = { 51, 52, 53 };
 
 static const struct intel_pingroup byt_score_groups[] = {
 	PIN_GROUP("uart1_grp", byt_score_uart1_pins, 1),
+	PIN_GROUP("uart1_grp_gpio", byt_score_uart1_pins, 0),
 	PIN_GROUP("uart2_grp", byt_score_uart2_pins, 1),
+	PIN_GROUP("uart2_grp_gpio", byt_score_uart2_pins, 0),
 	PIN_GROUP("pwm0_grp", byt_score_pwm0_pins, 1),
+	PIN_GROUP("pwm0_grp_gpio", byt_score_pwm0_pins, 0),
 	PIN_GROUP("pwm1_grp", byt_score_pwm1_pins, 1),
+	PIN_GROUP("pwm1_grp_gpio", byt_score_pwm1_pins, 0),
 	PIN_GROUP("ssp2_grp", byt_score_ssp2_pins, 1),
+	PIN_GROUP("ssp2_grp_gpio", byt_score_ssp2_pins, 0),
 	PIN_GROUP("sio_spi_grp", byt_score_sio_spi_pins, 1),
+	PIN_GROUP("sio_spi_grp_gpio", byt_score_sio_spi_pins, 0),
 	PIN_GROUP("i2c5_grp", byt_score_i2c5_pins, 1),
+	PIN_GROUP("i2c5_grp_gpio", byt_score_i2c5_pins, 0),
 	PIN_GROUP("i2c6_grp", byt_score_i2c6_pins, 1),
+	PIN_GROUP("i2c6_grp_gpio", byt_score_i2c6_pins, 0),
 	PIN_GROUP("i2c4_grp", byt_score_i2c4_pins, 1),
+	PIN_GROUP("i2c4_grp_gpio", byt_score_i2c4_pins, 0),
 	PIN_GROUP("i2c3_grp", byt_score_i2c3_pins, 1),
+	PIN_GROUP("i2c3_grp_gpio", byt_score_i2c3_pins, 0),
 	PIN_GROUP("i2c2_grp", byt_score_i2c2_pins, 1),
+	PIN_GROUP("i2c2_grp_gpio", byt_score_i2c2_pins, 0),
 	PIN_GROUP("i2c1_grp", byt_score_i2c1_pins, 1),
+	PIN_GROUP("i2c1_grp_gpio", byt_score_i2c1_pins, 0),
 	PIN_GROUP("i2c0_grp", byt_score_i2c0_pins, 1),
+	PIN_GROUP("i2c0_grp_gpio", byt_score_i2c0_pins, 0),
 	PIN_GROUP("ssp0_grp", byt_score_ssp0_pins, 1),
+	PIN_GROUP("ssp0_grp_gpio", byt_score_ssp0_pins, 0),
 	PIN_GROUP("ssp1_grp", byt_score_ssp1_pins, 1),
+	PIN_GROUP("ssp1_grp_gpio", byt_score_ssp1_pins, 0),
 	PIN_GROUP("sdcard_grp", byt_score_sdcard_pins, byt_score_sdcard_mux_values),
+	PIN_GROUP("sdcard_grp_gpio", byt_score_sdcard_pins, 0),
 	PIN_GROUP("sdio_grp", byt_score_sdio_pins, 1),
+	PIN_GROUP("sdio_grp_gpio", byt_score_sdio_pins, 0),
 	PIN_GROUP("emmc_grp", byt_score_emmc_pins, 1),
+	PIN_GROUP("emmc_grp_gpio", byt_score_emmc_pins, 0),
 	PIN_GROUP("lpc_grp", byt_score_ilb_lpc_pins, 1),
+	PIN_GROUP("lpc_grp_gpio", byt_score_ilb_lpc_pins, 0),
 	PIN_GROUP("sata_grp", byt_score_sata_pins, 1),
+	PIN_GROUP("sata_grp_gpio", byt_score_sata_pins, 0),
 	PIN_GROUP("plt_clk0_grp", byt_score_plt_clk0_pins, 1),
+	PIN_GROUP("plt_clk0_grp_gpio", byt_score_plt_clk0_pins, 0),
 	PIN_GROUP("plt_clk1_grp", byt_score_plt_clk1_pins, 1),
+	PIN_GROUP("plt_clk1_grp_gpio", byt_score_plt_clk1_pins, 0),
 	PIN_GROUP("plt_clk2_grp", byt_score_plt_clk2_pins, 1),
+	PIN_GROUP("plt_clk2_grp_gpio", byt_score_plt_clk2_pins, 0),
 	PIN_GROUP("plt_clk3_grp", byt_score_plt_clk3_pins, 1),
+	PIN_GROUP("plt_clk3_grp_gpio", byt_score_plt_clk3_pins, 0),
 	PIN_GROUP("plt_clk4_grp", byt_score_plt_clk4_pins, 1),
+	PIN_GROUP("plt_clk4_grp_gpio", byt_score_plt_clk4_pins, 0),
 	PIN_GROUP("plt_clk5_grp", byt_score_plt_clk5_pins, 1),
+	PIN_GROUP("plt_clk5_grp_gpio", byt_score_plt_clk5_pins, 0),
 	PIN_GROUP("smbus_grp", byt_score_smbus_pins, 1),
+	PIN_GROUP("smbus_grp_gpio", byt_score_smbus_pins, 0),
 };
 
 static const char * const byt_score_uart_groups[] = {
@@ -332,12 +359,14 @@  static const char * const byt_score_plt_clk_groups[] = {
 };
 static const char * const byt_score_smbus_groups[] = { "smbus_grp" };
 static const char * const byt_score_gpio_groups[] = {
-	"uart1_grp", "uart2_grp", "pwm0_grp", "pwm1_grp", "ssp0_grp",
-	"ssp1_grp", "ssp2_grp", "sio_spi_grp", "i2c0_grp", "i2c1_grp",
-	"i2c2_grp", "i2c3_grp", "i2c4_grp", "i2c5_grp", "i2c6_grp",
-	"sdcard_grp", "sdio_grp", "emmc_grp", "lpc_grp", "sata_grp",
-	"plt_clk0_grp", "plt_clk1_grp", "plt_clk2_grp", "plt_clk3_grp",
-	"plt_clk4_grp", "plt_clk5_grp", "smbus_grp",
+	"uart1_grp_gpio", "uart2_grp_gpio", "pwm0_grp_gpio",
+	"pwm1_grp_gpio", "ssp0_grp_gpio", "ssp1_grp_gpio", "ssp2_grp_gpio",
+	"sio_spi_grp_gpio", "i2c0_grp_gpio", "i2c1_grp_gpio", "i2c2_grp_gpio",
+	"i2c3_grp_gpio", "i2c4_grp_gpio", "i2c5_grp_gpio", "i2c6_grp_gpio",
+	"sdcard_grp_gpio", "sdio_grp_gpio", "emmc_grp_gpio", "lpc_grp_gpio",
+	"sata_grp_gpio", "plt_clk0_grp_gpio", "plt_clk1_grp_gpio",
+	"plt_clk2_grp_gpio", "plt_clk3_grp_gpio", "plt_clk4_grp_gpio",
+	"plt_clk5_grp_gpio", "smbus_grp_gpio",
 };
 
 static const struct intel_function byt_score_functions[] = {
@@ -457,7 +486,9 @@  static const struct intel_pingroup byt_sus_groups[] = {
 	PIN_GROUP("usb_ulpi_grp_gpio", byt_sus_usb_ulpi_pins, byt_sus_usb_ulpi_gpio_mode_values),
 	PIN_GROUP("pcu_spi_grp_gpio", byt_sus_pcu_spi_pins, byt_sus_pcu_spi_gpio_mode_values),
 	PIN_GROUP("pmu_clk1_grp", byt_sus_pmu_clk1_pins, 1),
+	PIN_GROUP("pmu_clk1_grp_gpio", byt_sus_pmu_clk1_pins, 0),
 	PIN_GROUP("pmu_clk2_grp", byt_sus_pmu_clk2_pins, 1),
+	PIN_GROUP("pmu_clk2_grp_gpio", byt_sus_pmu_clk2_pins, 0),
 };
 
 static const char * const byt_sus_usb_groups[] = {
@@ -469,7 +500,7 @@  static const char * const byt_sus_pmu_clk_groups[] = {
 };
 static const char * const byt_sus_gpio_groups[] = {
 	"usb_oc_grp_gpio", "usb_ulpi_grp_gpio", "pcu_spi_grp_gpio",
-	"pmu_clk1_grp", "pmu_clk2_grp",
+	"pmu_clk1_grp_gpio", "pmu_clk2_grp_gpio",
 };
 
 static const struct intel_function byt_sus_functions[] = {