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
