| 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
| ||
|
Date: Thu, 20 Oct 2022 19:58:38 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: chengwei <foxfly.lai.tw@...il.com>
Cc: lee@...nel.org, broonie@...nel.org, rafael@...nel.org,
mika.westerberg@...ux.intel.com, linus.walleij@...aro.org,
brgl@...ev.pl, linux-kernel@...r.kernel.org,
gregkh@...uxfoundation.org, lenb@...nel.org,
linux-acpi@...r.kernel.org, linux-gpio@...r.kernel.org,
GaryWang@...on.com.tw, musa.lin@...jingtech.com,
jack.chang@...jingtech.com, chengwei <larry.lai@...jingtech.com>,
Javier Arteaga <javier@...tex.com>,
Nicola Lunghi <nicola.lunghi@...tex.com>
Subject: Re: [PATCH 5/5] pinctrl: Add support pin control for UP board
CPLD/FPGA
On Wed, Oct 19, 2022 at 10:24:50AM +0800, chengwei wrote:
> The UP Squared board <http://www.upboard.com> implements certain
> features (pin control) through an on-board FPGA.
...
> +config PINCTRL_UPBOARD
> + tristate "UP board FPGA pin controller"
> + depends on ACPI
> + depends on MFD_UPBOARD_FPGA
> + depends on X86
No compile test coverage?
I think you wanted something like
depends on (X86 && ACPI) || COMPILE_TEST
I'm not even sure, why you have ACPI dependency. I do not see right now it has
a compile one.
> + select GENERIC_PINCONF
> + select PINMUX
> + select PINCONF
...
> +#include <linux/acpi.h>
See above.
> +#include <linux/dmi.h>
> +#include <linux/gpio.h>
No way, no new code should ever use this.
> +#include <linux/gpio/consumer.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/upboard-fpga.h>
> +#include <linux/module.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/string.h>
...
> +static int upboard_gpio_request_enable(struct pinctrl_dev *pctldev,
> + struct pinctrl_gpio_range *range,
> + unsigned int pin)
> +{
> + const struct pin_desc * const pd = pin_desc_get(pctldev, pin);
> + const struct upboard_pin *p;
> + int ret;
> + if (!pd)
> + return -EINVAL;
Why do you need this check?
Ditto for all the same checks over the code.
> + p = pd->drv_data;
> +
> + if (p->funcbit) {
> + ret = regmap_field_write(p->funcbit, 0);
> + if (ret)
> + return ret;
> + }
> +
> + if (p->enbit) {
> + ret = regmap_field_write(p->enbit, 1);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +};
...
> +
> +
One blank line is enough. Please, check all your code for this.
...
> +static int upboard_rpi_to_native_gpio(struct gpio_chip *gc, unsigned int gpio)
> +{
> + struct upboard_pinctrl *pctrl =
> + container_of(gc, struct upboard_pinctrl, chip);
> + unsigned int pin = pctrl->rpi_mapping[gpio];
> + struct pinctrl_gpio_range *range =
> + pinctrl_find_gpio_range_from_pin(pctrl->pctldev, pin);
Instead, split the assignment and the definition. Same amount of LoCs, but
reads and maintained better.
> + if (!range)
> + return -ENODEV;
> +
> + return range->base;
> +}
...
> +static int upboard_gpio_request(struct gpio_chip *gc, unsigned int offset)
> +{
> + int gpio = upboard_rpi_to_native_gpio(gc, offset);
> +
> + if (gpio < 0)
> + return gpio;
> +
> + return gpio_request(gpio, module_name(THIS_MODULE));
Nope, new code mustn't use this APIs.
> +}
...
> + gpio_free(gpio);
Ditto.
> + return gpio_get_value(gpio);
Ditto.
> + gpio_set_value(gpio, value);
Ditto.
> + return gpio_direction_input(gpio);
Ditto.
> + return gpio_direction_output(gpio, value);
Ditto.
...
> + {
> + .matches = { /* UP2 */
> + DMI_EXACT_MATCH(DMI_SYS_VENDOR, "AAEON"),
> + DMI_EXACT_MATCH(DMI_BOARD_NAME, "UP-APL01"),
> + DMI_EXACT_MATCH(DMI_BOARD_VERSION, "V0.3"),
> + },
> + .driver_data = (void *)&upboard_up2_bios_info_v0_3,
> + },
> + { },
No comma for the terminator entry.
...
> + hid = acpi_device_hid(adev);
> + if (!strcmp(hid, "AANT0F00") || !strcmp(hid, "AANT0F04")) {
> + pctldesc = &upboard_up_pinctrl_desc;
> + rpi_mapping = upboard_up_rpi_mapping;
> + ngpio = ARRAY_SIZE(upboard_up_rpi_mapping);
> + } else if (!strcmp(hid, "AANT0F01")) {
> + pctldesc = &upboard_up2_pinctrl_desc;
> + rpi_mapping = upboard_up2_rpi_mapping;
> + ngpio = ARRAY_SIZE(upboard_up2_rpi_mapping);
> + } else if (!strcmp(hid, "AANT0F02")) {
> + pctldesc = &upboard_upcore_crex_pinctrl_desc;
> + rpi_mapping = upboard_upcore_crex_rpi_mapping;
> + ngpio = ARRAY_SIZE(upboard_upcore_crex_rpi_mapping);
> + bios_info = &upboard_upcore_crex_bios_info;
> + } else if (!strcmp(hid, "AANT0F03")) {
> + pctldesc = &upboard_upcore_crst02_pinctrl_desc;
> + rpi_mapping = upboard_upcore_crst02_rpi_mapping;
> + ngpio = ARRAY_SIZE(upboard_upcore_crst02_rpi_mapping);
> + bios_info = &upboard_upcore_crst02_bios_info;
> + } else
> + return -ENODEV;
NIH device_get_match_data().
...
> + ret = acpi_node_add_pin_mapping(acpi_fwnode_handle(adev),
> + "external-gpios",
> + dev_name(&pdev->dev),
> + 0, UINT_MAX);
> + if (ret)
> + return ret;
This is something strange. Can you point out to the DSDT, etc.?
...
> +
Blank line is not needed.
> +module_platform_driver_probe(upboard_pinctrl_driver, upboard_pinctrl_probe);
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists