[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CACRpkdZQ7FCtLPEioWXn+MXQhnpuu-EY+CjhEeKqBe=Bvq777g@mail.gmail.com>
Date: Wed, 9 Nov 2022 10:27:59 +0100
From: Linus Walleij <linus.walleij@...aro.org>
To: chengwei <larry.lai@...jingtech.com>
Cc: lee@...nel.org, andriy.shevchenko@...ux.intel.com, pavel@....cz,
linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
linux-leds@...r.kernel.org, GaryWang@...on.com.tw,
musa.lin@...jingtech.com, jack.chang@...jingtech.com,
noah.hung@...jingtech.com, Javier Arteaga <javier@...tex.com>,
Nicola Lunghi <nicola.lunghi@...tex.com>
Subject: Re: [PATCH V3 2/3] pinctrl: Add support pin control for UP board CPLD/FPGA
On Wed, Nov 9, 2022 at 10:11 AM chengwei <larry.lai@...jingtech.com> 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
Powered by blists - more mailing lists