Re: [PATCH v2 2/2] pinctrl: qcom: Add MSM8952 tlmm pinctrl driver

[Muzaffer Kadir <muzafferkadir@mainlining.org>]

Hi Konrad,

On Wed, 2 Sep 2026 15:03:33 +0200, Konrad Dybcio wrote:
> The following is a review from my AI agent who inspected the
> hw specification and prior art/review feedback regarding qcom
> pinctrl. I read it and approve of it:
> 
> 
>  Hi Muzaffer,
> 
>   The driver currently contains function/group declarations which are not connected to any PINGROUP() entry.
> 
>   For each item below, add the function to the corresponding GPIO’s PINGROUP() definition:
> 
>    Function                GPIO
>   ━━━━━━━━━━━━━━━━━━━━━━  ━━━━━━
>    mipi_dsi0                  0
>   ──────────────────────  ──────
>    smb_int                    1
>   ──────────────────────  ──────
>    qdss_cti_trig_out_b0       2
>   ──────────────────────  ──────
>    rcm_marker2                3
>   ──────────────────────  ──────
>    qdss_cti_trig_out_a1       3
>   ──────────────────────  ──────
>    codec_mad                 13
>   ──────────────────────  ──────
>    nfc_disable               16
>   ──────────────────────  ──────
>    nfc_irq                   17
>   ──────────────────────  ──────
>    flash_strobe              33
>   ──────────────────────  ──────
>    cam1_standby              35
>   ──────────────────────  ──────
>    cam1_rst                  36
>   ──────────────────────  ──────
>    cam2_standby              37
>   ──────────────────────  ──────
>    cam2_rst                  38
>   ──────────────────────  ──────
>    webcam_standby            39
>   ──────────────────────  ──────
>    webcam_rst                40
>   ──────────────────────  ──────
>    rcm_marker1               41
>   ──────────────────────  ──────
>    accel_int                 42
>   ──────────────────────  ──────
>    alsp_int                  43
>   ──────────────────────  ──────
>    mag_int                   44
>   ──────────────────────  ──────
>    gyro_int                  45
>   ──────────────────────  ──────
>    pressure_int              46
>   ──────────────────────  ──────
>    fp_gpio                   47
>   ──────────────────────  ──────
>    fp_int                    48
>   ──────────────────────  ──────
>    us_euro                   63
>   ──────────────────────  ──────
>    ts_resout                 64
>   ──────────────────────  ──────
>    ts_sample                 65
>   ──────────────────────  ──────
>    sdcard_det                67
>   ──────────────────────  ──────
>    audio_reset               68
>   ──────────────────────  ──────
>    codec_int1                73
>   ──────────────────────  ──────
>    codec_int2                74
>   ──────────────────────  ──────
>    key_volp                  85
>   ──────────────────────  ──────
>    key_snapshot              86
>   ──────────────────────  ──────
>    key_focus                 87
>   ──────────────────────  ──────
>    key_home                  88
>   ──────────────────────  ──────
>    backlight_en              91
>   ──────────────────────  ──────
>    lcd_en                    92
>   ──────────────────────  ──────
>    usbc_irq                  93
>   ──────────────────────  ──────
>    uim3_ldo                  96
>   ──────────────────────  ──────
>    wsa_irq                   97
> 
>   For example:
> 
>   > static const char *const mipi_dsi0_groups[] = { "gpio0" };
> 
>   must have a matching mipi_dsi0 entry in the GPIO0 PINGROUP() definition. The same consistency is required for every item in the table. The gpio function is intentionally excluded because it is inserted
>   implicitly by the PINGROUP() macro.

Sibling SoCs like msm8917, msm8953 and
msm8976 do leave them without any PINGROUP() too.
I don't know what function index they map to
and I don't want to cause bugs by wrong indexes.
I don't know what to do with them.
Should I remove them?

> 
>   There are also functions missing from the GPIO table. Please add these to the corresponding groups. The final column is the function index from the MSM8952 GPIO mapping:
> 
>    Function     GPIO    Function index
>   ━━━━━━━━━━━  ━━━━━━  ━━━━━━━━━━━━━━━━
>    gp_pdm_2a       7                 4
>   ───────────  ──────  ────────────────
>    gp_pdm_1b      20                 3
>   ───────────  ──────  ────────────────
>    gp_pdm_2b      38                 1
>   ───────────  ──────  ────────────────
>    gp_mn          39                 2
>   ───────────  ──────  ────────────────
>    gp_pdm_1a      45                 1
>   ───────────  ──────  ────────────────
>    gp0_clk        46                 1
>   ───────────  ──────  ────────────────
>    gp1_clk        47                 2
>   ───────────  ──────  ────────────────
>    gp_pdm_0b      48                 2
>   ───────────  ──────  ────────────────
>    gp_pdm_0a      66                 3

I don't have access to the hw specification,
so I didn't know these exist. I used downstream
kernels as my reference for my port until that point.
This is so useful information for me. Thank you so much.
I will add these in v3.

> 
>   Finally, please move:
> 
>   > MODULE_DEVICE_TABLE(of, msm8952_pinctrl_of_match);
> 
>   immediately after the OF match table, consistent with the other Qualcomm pinctrl drivers.

I will move it in v3.

Thanks a lot for review.

Best regards,
Muzaffer Kadir