回覆: [RFC 2/3] pinctrl: Add support pin control for UP board CPLD/FPGA
From: Larry Lai
Date: Tue Apr 25 2023 - 11:29:13 EST
Dear Andy,
Thank you for spending time to review this code before, we already fixed some of the issues, please kindly check the following comments with “>>>” beginning.
Some of issues we will fix in new RFC_230425 patch, some may need you give us more examples or comments, thanks again !
Best Regards,
Larry Lai
寄件者: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
日期: 星期四, 2022年12月8日 上午5:32
收件者: Larry Lai <larry.lai@xxxxxxxxxxxxxxx>
副本: lee@xxxxxxxxxx <lee@xxxxxxxxxx>, linus.walleij@xxxxxxxxxx <linus.walleij@xxxxxxxxxx>, pavel@xxxxxx <pavel@xxxxxx>, linux-kernel@xxxxxxxxxxxxxxx <linux-kernel@xxxxxxxxxxxxxxx>, linux-gpio@xxxxxxxxxxxxxxx <linux-gpio@xxxxxxxxxxxxxxx>, linux-leds@xxxxxxxxxxxxxxx <linux-leds@xxxxxxxxxxxxxxx>, GaryWang@xxxxxxxxxxxx <GaryWang@xxxxxxxxxxxx>, Musa Lin <musa.lin@xxxxxxxxxxxxxxx>, Jack Chang <jack.chang@xxxxxxxxxxxxxxx>, Noah Hung <noah.hung@xxxxxxxxxxxxxxx>
主旨: Re: [RFC 2/3] pinctrl: Add support pin control for UP board CPLD/FPGA
On Thu, Dec 08, 2022 at 12:33:58AM +0800, larry.lai wrote:
> The UP Squared board <http://www.upboard.com> implements certain
> features (pin control) through an on-board FPGA.
(I already reviewed previous version and you have my tag, but some new comments
which may be already answered, sorry for the repetition in such cases).
Actually... based on the below comments I have to withdraw my tag.
This driver may not go into upstream in the current form.
I'm puzzled if it's indeed the code I have reviewed?
...
> +#include <linux/acpi.h>
This is actually not needed, but property.h.
See below how.
>>> This pinctrl driver read HID from (acpi_devices) mfd acpi driver, so will need to use some of acpi.h api.
...
> +static int upboard_fpga_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;
When this can be true?
Ditto for other functions with this conditional.
>>> Confirm this will not be true, so remove this code in new patch, please ignore it.
> + 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;
> +};
...
> + struct upboard_pinctrl *pctrl = container_of(gc, struct upboard_pinctrl, chip);
Don't you set a private data of GPIO chip to point to your custom structure?
>>> We give the gpio chip data in the pinctrl probe function to distinguish the different board HID to instead use private data of GPIO chip. This can determine which board used in every function.
Either way, I would recommend to make gpio_chip a first member in your
upboard_pinctrl.
>>> fixed.
...
> + int mode = 1;
See below about default.
> + unsigned int val;
> +
> + switch (pctrl->ident) {
> + case 15:
> + mode = 2;
> + break;
> + }
Wrong indentation, missing default.
Same for the rest below.
>>> fixed.
> + } else if (strstr(pctrl->pctldesc->pins[offset[i]].name,
> + "UART")) {
One line would be better to read, same for the rest below.
>>> fixed.
...
> + /* input pins */
> + if (strstr(pctrl->pctldesc->pins[offset[i]].name, "RX"))
> + input = true;
> + if (strstr(pctrl->pctldesc->pins[offset[i]].name, "CTS"))
> + input = true;
> + if (strstr(pctrl->pctldesc->pins[offset[i]].name, "ADC"))
> + input = true;
> + if (strstr(pctrl->pctldesc->pins[offset[i]].name, "MISO"))
> + input = true;
> + if (strstr(pctrl->pctldesc->pins[offset[i]].name, "DIN"))
> + input = true;
Can you have this in some static const data structure that is associated with
pin list?
>>> Due to the offsets of boards are different, so we need to scan all boards offset first, so we didn't use the static const data to be associated with.
...
> +static void upboard_gpio_free(struct gpio_chip *gc, unsigned int offset)
> +{
> + struct upboard_pinctrl *pctrl = container_of(gc, struct upboard_pinctrl, chip);
> + int gpio = upboard_rpi_to_native_gpio(gc, offset);
> + unsigned int pin = pctrl->rpi_mapping[offset];
> + char name[strlen(pctrl->pctldesc->pins[pin].name)];
> + char *p;
> + if (gpio < 0)
> + return;
When this can be true?
Same to the rest below.
>>> Confirm this will not be true, so remove this code in new patch, please ignore it.
> + pinctrl_gpio_free(gpio);
> +
> + strcpy(name, pctrl->pctldesc->pins[pin].name);
> + p = name;
> + upboard_alt_func_enable(gc, strsep(&p, "_"));
> +}
...
> + /* APL03 open drain GPIO */
> + if (pctrl->ident == 9) {
> + if (pin == 0 || pin == 1) {
> + int val = readl(pctrl->pins[pin].regs);
> +
> + if (value)
> + val |= PADCFG0_GPIOTXDIS;
> + else
> + val &= ~PADCFG0_GPIOTXDIS;
> + writel(val, pctrl->pins[pin].regs);
Huh?!
If we need OD support in the pinctrl-intel, add it there.
>>> Due to this is only for APL03 upboard pin0, pin1 open-drain and this board is already in the wild, so we should not add it to pinctrl-intel to affect others Intel board.
> + }
...
> + if (strstr(dev_name(&pdev->dev), "INTC1055:00") ||
> + strstr(dev_name(&pdev->dev), "INT34C5:00")) {
> + struct intel_pinctrl *intel_pctrl = gpiochip_get_data(gc);
> + struct pinctrl_dev *pctldev;
> + struct pinctrl_gpio_range *range;
> +
> + pctldev = intel_pctrl->pctldev;
> + if (pctldev == NULL)
> + return NULL;
> +
> + range = pinctrl_find_gpio_range_from_pin(pctldev, pin);
> + if (range)
> + pin = pin - range->pin_base;
> + else
> + return NULL;
> +
> + if (range->pin_base < 67)
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + else if (range->pin_base < 171)
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + else if (range->pin_base < 260)
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> + else
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 3);
> + }
> +
> + if (IS_ERR(res)) {
> + dev_err(gc->parent, "upboard resource get failed");
> + return NULL;
> + }
> + base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> + offset = readl(base + PADBAR);
> + value = readl(base + REVID);
> +
> + if (((value & REVID_MASK) >> REVID_SHIFT) >= 0x94)
> + nregs = 4;
> + else
> + nregs = 2;
> +
> + return base + offset + reg + pin * nregs * 4;
> +}
What the heck is this?!
Was it really in the version I have reviewed before?
>>> This code is for supporting multi-function pin with runtime change. Do you have any suggestion?
...
> +static void upboard_irq_ack(struct irq_data *d)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +
> + dev_info(NULL, "upboard_irq_ack");
Besides dev_info() and strange NULL, why do you need these messages? What for?
Same to the rest below.
>>> This section has been removed in new RFC patch, please ignore it.
> +}
...
> + data->parent_data = irq_get_irq_data(pctrl->pins[offset].irq);
> + if (data->parent_data) {
> + dev_info(NULL, "%s: no NULL", __func__);
What?!
>>> This section has been removed in new RFC patch, please ignore it.
> + return irq_chip_set_type_parent(data, type);
> + }
> +
> + dev_info(NULL, "%s: NULL", __func__);
Ditto.
>>> This section has been removed in new RFC patch, please ignore it.
...
> +static struct irq_chip upboard_gpio_irqchip = {
> + .name = "upboard-gpio-irq",
> + .irq_ack = upboard_irq_ack,
> + .irq_mask = upboard_irq_mask,
> + .irq_unmask = upboard_irq_unmask,
> + .irq_set_type = upboard_irq_chip_set_type,
> + .flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE,
It should be set IMMUTABLE.
>>> This section has been removed in new RFC patch, please ignore it.
> +};
...
> + hid = acpi_device_hid(adev);
> + if (!strcmp(hid, "AANT0F00") || !strcmp(hid, "AANT0F04") ||
> + !strcmp(hid, "AANT0000")) {
This is done via ACPI ID table (but without announcing it as module supported
hardware) and corresponding match functions.
>>> This is for assigning driver data so we need to know HID.
> + 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);
> + } 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);
> + } else
> + return -ENODEV;
...
> + pins = devm_kzalloc(&pdev->dev,
> + sizeof(*pins) * pctldesc->npins,
> + GFP_KERNEL);
devm_kcalloc() or even devm_kmalloc_array().
>>> fixed.
> + if (!pins)
> + return -ENOMEM;
...
I stopped here since I don't believe that I have given a tag to this earlier...
If it's the case, I don't know what I have got that day.
--
With Best Regards,
Andy Shevchenko