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

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

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