Re: [PATCH V3 2/3] pinctrl: Add support pin control for UP board CPLD/FPGA

[Linus Walleij]

On Wed, Nov 9, 2022 at 10:11 AM chengwei <larry.lai@xxxxxxxxxxxxxxx> wrote:

> +config PINCTRL_UPBOARD
> +       tristate "UP board FPGA pin controller"
> +       depends on ACPI
> +       depends on MFD_UPBOARD_FPGA
> +       depends on X86

This is Andy territory as it is x86 and ACPI but...

> +/*
> + * Init patches applied to the registers until the BIOS sets proper defaults
> + */
> +static const struct reg_sequence upboard_upcore_crex_reg_patches[] __initconst = {
> +       // enable I2C voltage-level shifters
> +       { UPFPGA_REG_FUNC_EN0,
> +               BIT(UPFPGA_I2C0_EN) |
> +               BIT(UPFPGA_I2C1_EN)
> +       },
> +       // HAT function pins initially set as inputs
> +       { UPFPGA_REG_GPIO_DIR0,
> +               BIT(UPFPGA_UPCORE_CREX_SPI2_MISO) |
> +               BIT(UPFPGA_UPCORE_CREX_UART1_RXD) |
> +               BIT(UPFPGA_UPCORE_CREX_I2S2_FRM) |
> +               BIT(UPFPGA_UPCORE_CREX_I2S2_CLK) |
> +               BIT(UPFPGA_UPCORE_CREX_I2S2_RX)
> +       },
> +};
> +
> +static const struct upboard_bios upboard_upcore_crex_bios_info __initconst = {
> +       .patches = upboard_upcore_crex_reg_patches,
> +       .npatches = ARRAY_SIZE(upboard_upcore_crex_reg_patches),
> +};

This "patches" terminology is quite confusing for kernel developers.
Writing some sequence of numbers into some registers at init is called
a "jam table" a term from Bunnie Huang (in his book "Hacking the Xbox" IIRC)

> +static int upboard_get_functions_count(struct pinctrl_dev *pctldev)
> +{
> +       //dev_info(pctldev->dev,"upboard_get_functions_count");
> +       return 0;
> +}
> +
> +static const char *upboard_get_function_name(struct pinctrl_dev *pctldev,
> +                                            unsigned int selector)
> +{
> +       //dev_info(pctldev->dev,"upboard_get_function_name:%d",selector);
> +       return NULL;
> +}

Don't leave this kind of commented out debug code around
in upstream submissions. Delete or use dev_dbg(), actually
dev_dbg() is pretty easy to use, just put an extra flag -DDEBUG
into your Makefile and the debug prints come out.

> +               //of_pinctrl_get(gc->parent->of_node);

What is this even? A commented out call to an OF function in an ACPI driver?

> +       switch (irqd_get_trigger_type(d)) {
> +       case IRQ_TYPE_LEVEL_HIGH:
> +               //value |= BYT_TRIG_LVL;
> +               fallthrough;
> +       case IRQ_TYPE_EDGE_RISING:
> +               //value |= BYT_TRIG_POS;
> +               break;
> +       case IRQ_TYPE_LEVEL_LOW:
> +               //value |= BYT_TRIG_LVL;
> +               fallthrough;
> +       case IRQ_TYPE_EDGE_FALLING:
> +               //value |= BYT_TRIG_NEG;
> +               break;
> +       case IRQ_TYPE_EDGE_BOTH:
> +               //value |= (BYT_TRIG_NEG | BYT_TRIG_POS);
> +               break;
> +       }

So this looks like it should be uncommented and used or deleted?
It just looks unfinished, and this patch is not an RFC.

> +       //display mapping info.
> +       //for(i=0;i<pctldesc->npins;i++){
> +       //      dev_info(&pdev->dev,"Name:%s, GPIO:%d, IRQ:%d, regs:0x%08x",
> +       //      pctldesc->pins[i].name,pins[i].gpio, pins[i].irq, pins[i].regs);
> +       //      if(pins[i].regs)
> +       //              dev_info(&pdev->dev,"val:%pS", readl(pins[i].regs));
> +       //}

This isn't helpful, also there are existing debugfs hooks to be used
for exactly this kind of stuff.

The driver looks a bit unfinished.

Yours,
Linus Walleij