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

Andy Shevchenko
2022-10-20T16:58:57Z

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