| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <492f6852-48f0-891f-c017-65d3562144a6@huawei.com>
Date: Fri, 4 Dec 2020 17:38:55 +0800
From: luojiaxing <luojiaxing@...wei.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
CC: <linus.walleij@...aro.org>, <bgolaszewski@...libre.com>,
<catalin.marinas@....com>, <will@...nel.org>,
<linux-gpio@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linuxarm@...wei.com>
Subject: Re: [PATCH v1 1/3] gpio: gpio-hisi: Add HiSilicon GPIO support
Hi
On 2020/12/2 18:04, Andy Shevchenko wrote:
> 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?
sure, I think you mean that "ngpios" should be used here.
> 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)]
I think it should be
depends on (ARM64 || COMPILE_TEST) && ACPI
>
>> + 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?
sorry, I didn't get what you mean. What module name should I add here?
>
> ...
>
>> +/*
>> + * Copyright (c) 2020 HiSilicon Limited.
>> + */
> One line.
ok
>
> ...
>
>> +#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.
sure, let me check it.
>> +#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?
This should be deleted, I used to use
acpi_gpiochip_request_interrupts(), but delete it later.
>
> ...
>
>> +#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
ok
> ...
>
>> +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.
sure
>
>> + 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.
sure
>> +
>> + 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.
Sorry, what do you mean by Move ... this above?
>
> ...
>
>> + /* Return 0 if output, 1 if input */
> Useless comment.
will be deleted
>
> ...
>
>> +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?
Our GPIO controller uses two separate registers to enable/disable the
double-edge interrupt,
and once enable double-edge, the setting of edge and polarity register()
will be ignored by the hardware.
Therefore, each time we update trigger type, an extra check is required:
if the interrupt is not double-edges, ensure that it is disabled.
>
>> + 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().
sure, it's better than fls
>
> ...
>
>> + 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.
I looked at gpio-regmap.c, which is a newly uploaded feature. I think
it's good, but I'd rather keep the current design.
Because it means my code needs to be modified and re-tested, this will
take a while, but the actual business needs will not allow it.
>
> ...
>
>> +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.
sure
>
>> + },
>> + .probe = hisi_gpio_probe,
>> +};
>> +
> Redundant blank line.
sure
Thanks
Jiaxing
>
>> +module_platform_driver(hisi_gpio_driver);
Powered by blists - more mailing lists