From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Luo Jiaxing <luojiaxing@huawei.com>
Cc: linus.walleij@linaro.org, bgolaszewski@baylibre.com,
catalin.marinas@arm.com, will@kernel.org,
linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
linuxarm@huawei.com
Subject: Re: [PATCH v1 1/3] gpio: gpio-hisi: Add HiSilicon GPIO support
Date: Wed, 2 Dec 2020 12:04:05 +0200 [thread overview]
Message-ID: <20201202100405.GY4077@smile.fi.intel.com> (raw)
In-Reply-To: <1606901543-8957-2-git-send-email-luojiaxing@huawei.com>
On Wed, Dec 02, 2020 at 05:32:21PM +0800, Luo Jiaxing wrote:
> This GPIO driver is for HiSilicon's ARM SoC.
>
> HiSilicon's GPIO controller support double-edge interrupt and multi-core
> concurrent access.
>
> ACPI table example for this GPIO controller:
> Device (GPO0)
> {
> Name (_HID, "HISI0184")
> Device (PRTA)
> {
> Name (_ADR, Zero)
> Name (_UID, Zero)
> Name (_DSD, Package (0x01)
> {
> Package (0x02)
> {
> "hisi-ngpio",
Can it be standard property?
Please, fix firmware.
> 0x20
> }
> })
> }
> }
...
> +config GPIO_HISI
> + tristate "HISILICON GPIO controller driver"
> + depends on (ARM64 && ACPI) || COMPILE_TEST
This is wrong. (Homework to understand why. Also see below)
> + select GPIO_GENERIC
> + select GENERIC_IRQ_CHIP
> + help
> + Say Y or M here to build support for the HiSilicon GPIO controller driver
> + GPIO block.
> + This controller support double-edge interrupt and multi-core concurrent
> + access.
No module name?
...
> +/*
> + * Copyright (c) 2020 HiSilicon Limited.
> + */
One line.
...
> +#include <linux/acpi.h>
Don't see user of it (but see above and below as well).
At the same time missed mod_devicetable.h.
> +#include <linux/gpio/driver.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
...
> +#include "gpiolib.h"
> +#include "gpiolib-acpi.h"
Any user of this?
...
> +#define HISI_GPIO_SWPORT_DR_SET_WX 0x0
...
> +#define HISI_GPIO_INT_DEDGE_SET 0xb0
...
> +#define HISI_GPIO_REG_MAX 0x100
Use fixed width for register offsets, like:
0x000
...
0x0b0
...
0x100
...
> +struct hisi_gpio {
> + struct device *dev;
> + void __iomem *reg_base;
> + unsigned int pin_num;
> + struct gpio_chip chip;
Moving this to be a first member of the struct will make corresponding
container_of() no-op.
> + struct irq_chip irq_chip;
> + int irq;
> +};
...
> + unsigned long mask = BIT(off);
No need to have temporary variable. Use directly BIT(off) which fits into 80.
> +
> + if (debounce)
> + hisi_gpio_write_reg(chip, HISI_GPIO_DEBOUNCE_SET_WX, mask);
> + else
> + hisi_gpio_write_reg(chip, HISI_GPIO_DEBOUNCE_CLR_WX, mask);
...
> + switch (config_para) {
> + case PIN_CONFIG_INPUT_DEBOUNCE:
> + config_arg = pinconf_to_config_argument(config);
> + hisi_gpio_set_debounce(chip, offset, config_arg);
> + break;
Move...
> + default:
> + return -ENOTSUPP;
> + }
> +
> + return 0;
...this above.
...
> + /* Return 0 if output, 1 if input */
Useless comment.
...
> +static int hisi_gpio_irq_set_type(struct irq_data *d, u32 type)
> +{
> + struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
> + unsigned int mask = BIT(irqd_to_hwirq(d));
> +
> + switch (type) {
> + case IRQ_TYPE_EDGE_BOTH:
> + hisi_gpio_write_reg(chip, HISI_GPIO_INT_DEDGE_SET, mask);
> + break;
> + case IRQ_TYPE_EDGE_RISING:
> + hisi_gpio_write_reg(chip, HISI_GPIO_INTTYPE_EDGE_SET_WX, mask);
> + hisi_gpio_write_reg(chip, HISI_GPIO_INT_POLARITY_SET_WX, mask);
> + break;
> + case IRQ_TYPE_EDGE_FALLING:
> + hisi_gpio_write_reg(chip, HISI_GPIO_INTTYPE_EDGE_SET_WX, mask);
> + hisi_gpio_write_reg(chip, HISI_GPIO_INT_POLARITY_CLR_WX, mask);
> + break;
> + case IRQ_TYPE_LEVEL_HIGH:
> + hisi_gpio_write_reg(chip, HISI_GPIO_INTTYPE_EDGE_CLR_WX, mask);
> + hisi_gpio_write_reg(chip, HISI_GPIO_INT_POLARITY_SET_WX, mask);
> + break;
> + case IRQ_TYPE_LEVEL_LOW:
> + hisi_gpio_write_reg(chip, HISI_GPIO_INTTYPE_EDGE_CLR_WX, mask);
> + hisi_gpio_write_reg(chip, HISI_GPIO_INT_POLARITY_CLR_WX, mask);
> + default:
> + return -EINVAL;
> + }
> +
> + /*
> + * The dual-edge interrupt and other interrupt's registers do not
> + * take effect at the same time. The registers of the two-edge
> + * interrupts have higher priorities, the configuration of
> + * the dual-edge interrupts must be disabled before the configuration
> + * of other kind of interrupts.
> + */
This comment sounds like below should be moved before switch-case. Can you elaborate?
> + if (type != IRQ_TYPE_EDGE_BOTH) {
> + unsigned int both = hisi_gpio_read_reg(chip, HISI_GPIO_INT_DEDGE_ST);
> +
> + if (both & mask)
> + hisi_gpio_write_reg(chip, HISI_GPIO_INT_DEDGE_CLR, mask);
> + }
> +
> + if (type & IRQ_TYPE_LEVEL_MASK)
> + irq_set_handler_locked(d, handle_level_irq);
> + else if (type & IRQ_TYPE_EDGE_BOTH)
> + irq_set_handler_locked(d, handle_edge_irq);
> +
> + return 0;
> +}
...
> + while (irq_msk) {
> + int hwirq = fls(irq_msk) - 1;
> + irq_msk &= ~BIT(hwirq);
> + }
NIH of for_each_set_bit().
...
> + res = bgpio_init(&hisi_gpio->chip, hisi_gpio->dev, HISI_GPIO_REG_SIZE, dat, set,
> + clr, NULL, NULL, 0);
> + if (res) {
> + dev_err(dev, "failed to init\n");
> + return res;
> + }
Wondering if you can use regmap GPIO.
...
> +static struct platform_driver hisi_gpio_driver = {
> + .driver = {
> + .name = HISI_GPIO_DRIVER_NAME,
> + .acpi_match_table = ACPI_PTR(hisi_gpio_acpi_match),
This is wrong. If you use COMPILE_TEST the ACPI_PTR in !ACPI case is no op.
Compiler will warn you about unused variable. Have you compile tested it in
such conditions?
Hint: remove ACPI_PTR(). In 99% this macro shouldn't be used.
> + },
> + .probe = hisi_gpio_probe,
> +};
> +
Redundant blank line.
> +module_platform_driver(hisi_gpio_driver);
--
With Best Regards,
Andy Shevchenko
next prev parent reply other threads:[~2020-12-02 10:04 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-02 9:32 [PATCH v1 0/3] gpio: gpio-hisi: Add HiSilicon GPIO support Luo Jiaxing
2020-12-02 9:32 ` [PATCH v1 1/3] " Luo Jiaxing
2020-12-02 10:04 ` Andy Shevchenko [this message]
2020-12-04 9:38 ` luojiaxing
2020-12-06 23:12 ` Linus Walleij
2020-12-09 8:18 ` luojiaxing
2020-12-09 9:03 ` Linus Walleij
2020-12-02 9:32 ` [PATCH v1 2/3] MAINTAINERS: Add maintainer for HiSilicon GPIO driver Luo Jiaxing
2020-12-02 9:32 ` [PATCH v1 3/3] arm64: defconfig: enable GPIO_HISI Luo Jiaxing
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201202100405.GY4077@smile.fi.intel.com \
--to=andy.shevchenko@gmail.com \
--cc=bgolaszewski@baylibre.com \
--cc=catalin.marinas@arm.com \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxarm@huawei.com \
--cc=luojiaxing@huawei.com \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).