| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aAFBwANy47y0DAhY@smile.fi.intel.com>
Date: Thu, 17 Apr 2025 21:00:32 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Thomas Richard <thomas.richard@...tlin.com>
Cc: Linus Walleij <linus.walleij@...aro.org>,
Bartosz Golaszewski <brgl@...ev.pl>,
Geert Uytterhoeven <geert+renesas@...der.be>,
linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
thomas.petazzoni@...tlin.com, DanieleCleri@...on.eu,
GaryWang@...on.com.tw
Subject: Re: [PATCH v3 10/10] pinctrl: Add pin controller driver for AAEON UP
boards
On Wed, Apr 16, 2025 at 04:08:18PM +0200, Thomas Richard wrote:
> This enables the pin control support of the onboard FPGA on AAEON UP
> boards.
>
> This FPGA acts as a level shifter between the Intel SoC pins and the pin
> header, and also as a mux or switch.
>
> +---------+ +--------------+ +---+
> | | | | |
> | PWM0 | \ | | H |
> |----------|------ \-----|-------------| E |
> | I2C0_SDA | | | A |
> Intel SoC |----------|------\ | | D |
> | GPIO0 | \------|-------------| E |
> |----------|------ | | R |
> | | FPGA | | |
> ----------+ +--------------+ +---+
>
> For most of the pins, the FPGA opens/closes a switch to enable/disable
> the access to the SoC pin from a pin header.
> Each switch, has a direction flag that is set depending the status of the
> SoC pin.
>
> For some other pins, the FPGA acts as a mux, and routes one pin (or the
> other one) to the header.
>
> The driver provides also a gpiochip. It requests SoC pins in GPIO mode,
> and drives them in tandem with FPGA pins (switch/mux direction).
>
> This commit adds support only for UP Squared board
Missed period.
...
> +/*
> + * UP board pin control driver.
> + *
> + * Copyright (C) 2024 Bootlin
My calendar shows something different :-)
> + *
> + * Author: Thomas Richard <thomas.richard@...tlin.com>
> + */
> +#include <linux/array_size.h>
> +#include <linux/container_of.h>
> +#include <linux/device.h>
> +#include <linux/dmi.h>
> +#include <linux/err.h>
> +#include <linux/gpio/forwarder.h>
> +#include <linux/mfd/upboard-fpga.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/seq_file.h>
+ types.h (bool and NULL from stddef.h that will be included).
...
> +static const enum upboard_pin_mode upboard_up2_uart1_modes[] = {
> + UPBOARD_PIN_MODE_GPIO_OUT,
> + UPBOARD_PIN_MODE_GPIO_IN,
> + UPBOARD_PIN_MODE_GPIO_OUT,
> + UPBOARD_PIN_MODE_GPIO_IN
Leave trailing comma in such cases.
> +};
...
> +static int upboard_pinctrl_set_mux(struct pinctrl_dev *pctldev, unsigned int func_selector,
> + unsigned int group_selector)
> +{
> + struct upboard_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> + const struct upboard_pinctrl_data *pctrl_data = pctrl->pctrl_data;
> + const struct upboard_pingroup *upgroups = pctrl_data->groups;
> + struct group_desc *grp;
> + unsigned int mode, i;
> + int ret;
> +
> + grp = pinctrl_generic_get_group(pctldev, group_selector);
> + if (!grp)
> + return -EINVAL;
> +
> + for (i = 0; i < grp->grp.npins; i++) {
> + mode = upgroups[group_selector].mode ?: upgroups[group_selector].modes[i];
> +
Unneeded blank line.
> + if (mode == UPBOARD_PIN_MODE_FUNCTION) {
> + ret = upboard_pinctrl_set_function(pctldev, grp->grp.pins[i]);
> + if (ret)
> + return ret;
> +
> + continue;
> + }
> +
> + ret = upboard_pinctrl_gpio_commit_enable(pctldev, grp->grp.pins[i]);
> + if (ret)
> + return ret;
> +
> + ret = upboard_pinctrl_gpio_commit_direction(pctldev, grp->grp.pins[i],
> + mode == UPBOARD_PIN_MODE_GPIO_IN);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
...
> +static void upboard_pinctrl_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
> + unsigned int offset)
> +{
> + int ret;
> +
> + ret = upboard_pinctrl_pin_get_mode(pctldev, offset);
> +
Unneeded blank line.
> + if (ret == UPBOARD_PIN_MODE_FUNCTION)
> + seq_puts(s, "mode function ");
> + else if (ret == UPBOARD_PIN_MODE_DISABLED)
> + seq_puts(s, "HIGH-Z");
> + else
> + seq_printf(s, "GPIO (%s) ", ret == UPBOARD_PIN_MODE_GPIO_IN ? "input" : "output");
str_input_output() from string_choices.h ?
Ah, we still have no such... In case you have motivation, you can add a patch,
I will Ack/Review it.
> +}
...
> +static int upboard_gpio_request(struct gpio_chip *gc, unsigned int offset)
> +{
> + struct gpiochip_fwd *fwd = container_of(gc, struct gpiochip_fwd, chip);
> + struct upboard_pinctrl *pctrl = fwd->data;
Yeah, something like
struct upboard_pinctrl *pctrl = gpio_fwd_get_data(fwd);
> + unsigned int pin = pctrl->pctrl_data->pin_header[offset];
> + struct gpio_desc *desc;
> + int ret;
> +
> + ret = pinctrl_gpio_request(gc, offset);
> + if (ret)
> + return ret;
> + /* GPIO desc is already registered */
> + if (fwd->descs[offset])
> + return 0;
As mentioned in another reply, why 0 and even though, why can't it be simply
filtered by EEXIST from the below?
In worst scenario, you can add an API gpio_fwd_is_registered(fwd, offset).
> + desc = gpiod_get_index(pctrl->dev, "external", pin, 0);
> + if (IS_ERR(desc)) {
> + pinctrl_gpio_free(gc, offset);
> + return PTR_ERR(desc);
> + }
> +
> + return gpio_fwd_add_gpio_desc(fwd, desc, offset);
> +}
...
> +static int upboard_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
> +{
> + struct gpiochip_fwd *fwd = container_of(gc, struct gpiochip_fwd, chip);
> + struct upboard_pinctrl *pctrl = fwd->data;
> + unsigned int pin = pctrl->pctrl_data->pin_header[offset];
> + int mode;
> +
> + /* If the pin is in function mode or high-z, input direction is returned */
> + mode = upboard_pinctrl_pin_get_mode(pctrl->pctldev, pin);
> + if (mode < 0)
> + return mode;
> +
> + if (mode == UPBOARD_PIN_MODE_GPIO_OUT)
> + return GPIO_LINE_DIRECTION_OUT;
> + else
Redundant 'else'.
> + return GPIO_LINE_DIRECTION_IN;
> +}
...
> +static int upboard_pinctrl_register_functions(struct upboard_pinctrl *pctrl)
> +{
> + const struct pinfunction *funcs = pctrl->pctrl_data->funcs;
> + size_t nfuncs = pctrl->pctrl_data->nfuncs;
> + unsigned int i;
> + int ret;
> +
> + for (i = 0; i < nfuncs ; ++i) {
Why out of a sudden pre-increment?
> + ret = pinmux_generic_add_function(pctrl->pctldev, funcs[i].name,
> + funcs[i].groups, funcs[i].ngroups, NULL);
> + if (ret < 0)
> + return ret;
> + }
> +
> + return 0;
> +}
...
> +static int upboard_pinctrl_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct upboard_fpga *fpga = dev_get_drvdata(dev->parent);
> + const struct dmi_system_id *dmi_id;
> + enum upboard_board_id board_id;
> + struct pinctrl_desc *pctldesc;
> + struct upboard_pinctrl *pctrl;
> + struct upboard_pin *pins;
> + struct gpiochip_fwd *fwd;
> + struct pinctrl *pinctrl;
> + struct gpio_chip *chip;
> + int ret, i;
Why i is signed while other code uses unsigned loop iterators?
> + pctldesc = devm_kzalloc(dev, sizeof(*pctldesc), GFP_KERNEL);
> + if (!pctldesc)
> + return -ENOMEM;
> +
> + pctrl = devm_kzalloc(dev, sizeof(*pctrl), GFP_KERNEL);
> + if (!pctrl)
> + return -ENOMEM;
> +
> + switch (fpga->fpga_data->type) {
> + case UPBOARD_UP_FPGA:
> + pctldesc->pins = upboard_up_pins;
> + pctldesc->npins = ARRAY_SIZE(upboard_up_pins);
> + pctrl->pctrl_data = &upboard_up_pinctrl_data;
> + break;
> + case UPBOARD_UP2_FPGA:
> + pctldesc->pins = upboard_up2_pins;
> + pctldesc->npins = ARRAY_SIZE(upboard_up2_pins);
> + pctrl->pctrl_data = &upboard_up2_pinctrl_data;
> + break;
> + default:
> + return dev_err_probe(dev, -ENODEV, "Unsupported device type %d\n",
> + fpga->fpga_data->type);
> + }
> +
> + dmi_id = dmi_first_match(dmi_platform_info);
> + if (!dmi_id)
> + return -ENODEV;
> +
> + board_id = (enum upboard_board_id)dmi_id->driver_data;
This might need to have an intermediate cast to (unsigned long). Have you run
the build with Clang 19 and `make W=1`?
> +
> + switch (board_id) {
> + case UPBOARD_APL01:
> + pctrl->maps = upboard_pinctrl_mapping_apl01;
> + pctrl->nmaps = ARRAY_SIZE(upboard_pinctrl_mapping_apl01);
> + break;
> + default:
> + return dev_err_probe(dev, -ENODEV, "Unsupported board\n");
> + }
> +
> + pctldesc->name = dev_name(dev);
> + pctldesc->owner = THIS_MODULE;
> + pctldesc->pctlops = &upboard_pinctrl_ops;
> + pctldesc->pmxops = &upboard_pinmux_ops;
> +
> + pctrl->dev = dev;
> +
> + pins = devm_kcalloc(dev, pctldesc->npins, sizeof(*pins), GFP_KERNEL);
> + if (!pins)
> + return -ENOMEM;
> +
> + /* Initialize pins */
> + for (i = 0; i < pctldesc->npins; i++) {
> + const struct pinctrl_pin_desc *pin_desc = &pctldesc->pins[i];
> + unsigned int regoff = pin_desc->number / UPBOARD_REGISTER_SIZE;
> + unsigned int lsb = pin_desc->number % UPBOARD_REGISTER_SIZE;
> + struct reg_field * const fld_func = pin_desc->drv_data;
> + struct upboard_pin *pin = &pins[i];
> + struct reg_field fldconf = {};
> +
> + if (fld_func) {
> + pin->funcbit = devm_regmap_field_alloc(dev, fpga->regmap, *fld_func);
> + if (IS_ERR(pin->funcbit))
> + return PTR_ERR(pin->funcbit);
> + }
> +
> + fldconf.reg = UPBOARD_REG_GPIO_EN0 + regoff;
> + fldconf.lsb = lsb;
> + fldconf.msb = lsb;
> + pin->enbit = devm_regmap_field_alloc(dev, fpga->regmap, fldconf);
> + if (IS_ERR(pin->enbit))
> + return PTR_ERR(pin->enbit);
> +
> + fldconf.reg = UPBOARD_REG_GPIO_DIR0 + regoff;
> + fldconf.lsb = lsb;
> + fldconf.msb = lsb;
> + pin->dirbit = devm_regmap_field_alloc(dev, fpga->regmap, fldconf);
> + if (IS_ERR(pin->dirbit))
> + return PTR_ERR(pin->dirbit);
> + }
> +
> + pctrl->pins = pins;
> +
> + ret = devm_pinctrl_register_and_init(dev, pctldesc, pctrl, &pctrl->pctldev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to register pinctrl\n");
> +
> + ret = upboard_pinctrl_register_groups(pctrl);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to register groups\n");
> +
> + ret = upboard_pinctrl_register_functions(pctrl);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to register functions\n");
> +
> + ret = devm_pinctrl_register_mappings(dev, pctrl->maps, pctrl->nmaps);
> + if (ret)
> + return ret;
> +
> + pinctrl = devm_pinctrl_get_select_default(dev);
> + if (IS_ERR(pinctrl))
> + return dev_err_probe(dev, PTR_ERR(pinctrl), "Failed to select pinctrl\n");
> +
> + ret = pinctrl_enable(pctrl->pctldev);
> + if (ret)
> + return ret;
> +
> + fwd = devm_gpio_fwd_alloc(dev, pctrl->pctrl_data->ngpio);
> + if (IS_ERR(fwd))
> + return dev_err_probe(dev, PTR_ERR(fwd), "Failed to allocate the gpiochip forwarder\n");
> +
> + chip = &fwd->chip;
> + chip->request = upboard_gpio_request;
> + chip->free = upboard_gpio_free;
> + chip->get_direction = upboard_gpio_get_direction;
> + chip->direction_output = upboard_gpio_direction_output;
> + chip->direction_input = upboard_gpio_direction_input;
> +
> + ret = gpio_fwd_register(fwd, pctrl);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to register the gpiochip forwarder\n");
> + return gpiochip_add_sparse_pin_range(chip, dev_name(dev), 0, pctrl->pctrl_data->pin_header,
> + pctrl->pctrl_data->ngpio);
> +
> + return ret;
Stray return.
> +}
...
> +static struct platform_driver upboard_pinctrl_driver = {
> + .driver = {
> + .name = "upboard-pinctrl",
> + },
> + .probe = upboard_pinctrl_probe,
> +};
> +
Unneeded blank line.
> +module_platform_driver(upboard_pinctrl_driver);
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists