linux-kernel - Re: [PATCH v1 1/3] gpio: gpio-hisi: Add HiSilicon GPIO support

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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);