Hi Tzuyi!
thanks for your patch!
On Wed, Nov 1, 2023 at 3:58 AM Tzuyi Chang <tychang@xxxxxxxxxxx> wrote:
> This commit adds GPIO support for Realtek DHC RTD SoCs.
What does "DHC" mean? Please spell it out in the commit and Kconfig
so we know what it is.
> This driver enables configuration of GPIO direction, GPIO values, GPIO
> debounce settings and handles GPIO interrupts.
>
> Signed-off-by: Tzuyi Chang <tychang@xxxxxxxxxxx>
(...)
> +config GPIO_RTD
> + tristate "Realtek DHC GPIO support"
> + depends on ARCH_REALTEK
> + default y
> + select GPIOLIB_IRQCHIP
> + help
> + Say yes here to support GPIO on Realtek DHC SoCs.
Explain what DHC is i.e. the acronym expansion, family, use case or something.
> +#include <linux/bitops.h>
> +#include <linux/gpio.h>
Do not include this legacy header.
Include <linux/gpio/driver.h>
> +#include <linux/interrupt.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of_irq.h>
I don't think you need any of thexe of_* includes.
Try it without them.
> +#include <linux/pinctrl/consumer.h>
Why?
> +/**
> + * struct rtd_gpio_info - Specific GPIO register information
> + * @name: GPIO device name
> + * @type: RTD GPIO ID
> + * @gpio_base: GPIO base number
> + * @num_gpios: Number of GPIOs
> + * @dir_offset: Offset for GPIO direction registers
> + * @dato_offset: Offset for GPIO data output registers
> + * @dati_offset: Offset for GPIO data input registers
> + * @ie_offset: Offset for GPIO interrupt enable registers
> + * @dp_offset: Offset for GPIO detection polarity registers
> + * @gpa_offset: Offset for GPIO assert interrupt status registers
> + * @gpda_offset: Offset for GPIO deassert interrupt status registers
> + * @deb_offset: Offset for GPIO debounce registers
> + */
> +struct rtd_gpio_info {
> + const char *name;
> + enum rtd_gpio_type type;
> + unsigned int gpio_base;
> + unsigned int num_gpios;
> + unsigned int *dir_offset;
> + unsigned int *dato_offset;
> + unsigned int *dati_offset;
> + unsigned int *ie_offset;
> + unsigned int *dp_offset;
> + unsigned int *gpa_offset;
> + unsigned int *gpda_offset;
> + unsigned int *deb_offset;
Use u8 instead of unsigned int for the offsets. It is clear from
the arrays you assign them that they are all u8[].
> +struct rtd_gpio {
> + struct platform_device *pdev;
> + const struct rtd_gpio_info *info;
> + void __iomem *base;
> + void __iomem *irq_base;
> + struct gpio_chip gpio_chip;
> + struct irq_chip irq_chip;
Do not use a dynamic irq_chip, create an immutable irq_chip
using a const struct.
See recent commits and virtually all current drivers in the tree
for examples on how to do that.
> + int assert_irq;
> + int deassert_irq;
I don't quite understand these two, but let's see in the rest
of the driver.
> + .deb_offset = (unsigned int []){ 0x30, 0x34, 0x38, 0x3c, 0x40, 0x44, 0x48, 0x4c },
(...)
> + .deb_offset = (unsigned int []){ 0x50 },
So clearly u8[]
> +static unsigned int rtd_gpio_deb_offset(struct rtd_gpio *data, unsigned int offset)
> +{
> + return data->info->deb_offset[offset / 8];
> +}
So this is clearly counted by the GPIO number offset and the GPIO number
determines how far into the array we can index.
It looks a bit dangerous, it it possible to encode the array lengths better?
> + if (data->info->type == RTD1295_ISO_GPIO) {
> + shift = 0;
> + deb_val += 1;
> + write_en = BIT(shift + 3);
> + reg_offset = rtd1295_gpio_deb_offset(data, offset);
> + } else if (data->info->type == RTD1295_MISC_GPIO) {
> + shift = (offset >> 4) * 4;
> + deb_val += 1;
> + write_en = BIT(shift + 3);
> + reg_offset = rtd1295_gpio_deb_offset(data, offset);
> + } else {
> + shift = (offset % 8) * 4;
> + write_en = BIT(shift + 3);
> + reg_offset = rtd_gpio_deb_offset(data, offset);
> + }
These three different offset functions seem a bit awkward.
Can we do this by just another index instead?
> +static int rtd_gpio_request(struct gpio_chip *chip, unsigned int offset)
> +{
> + return pinctrl_gpio_request(chip->base + offset);
> +}
> +
> +static void rtd_gpio_free(struct gpio_chip *chip, unsigned int offset)
> +{
> + pinctrl_gpio_free(chip->base + offset);
> +}
IIRC Bartosz has changed this for kernel v6.7, please check his upstream
commits and adjust the code accordingly.
> +static int rtd_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
> +{
> + struct rtd_gpio *data = gpiochip_get_data(chip);
> + u32 irq = 0;
> +
> + irq = irq_find_mapping(data->domain, offset);
> + if (!irq) {
> + dev_err(&data->pdev->dev, "%s: can not find irq number for hwirq= %d\n",
> + __func__, offset);
> + return -EINVAL;
> + }
> + return irq;
> +}
Don't implement your own gpio_to_irq, just use the GPIOLIB_IRQCHIP
helpers. See other drivers that select GPIOLIB_IRQCHIP, this
driver is nothing special.
> + chained_irq_enter(chip, desc);
> +
> + for (i = 0; i < data->info->num_gpios; i = i + 31) {
> + gpa_reg_offset = rtd_gpio_gpa_offset(data, i);
> + status = readl_relaxed(data->irq_base + gpa_reg_offset) >> 1;
> + writel_relaxed(status << 1, data->irq_base + gpa_reg_offset);
> +
> + while (status) {
> + j = __ffs(status);
> + status &= ~BIT(j);
> + hwirq = i + j;
> + if (rtd_gpio_check_ie(data, hwirq)) {
> + int irq = irq_find_mapping(data->domain, hwirq);
> +
> + generic_handle_irq(irq);
> + }
So you skip the interrupt handler if the interrupt is not enabled?
I think you should report spurious interrupts if they occur without
being enabled, unless there is some hardware flunky making these
lines flicker with noise interrupts too much.
> +static void rtd_gpio_deassert_irq_handle(struct irq_desc *desc)
> +{
> + struct rtd_gpio *data = irq_desc_get_handler_data(desc);
> + struct irq_chip *chip = irq_desc_get_chip(desc);
> + unsigned int gpda_reg_offset;
> + u32 status;
> + int hwirq;
> + int i;
> + int j;
> +
> + chained_irq_enter(chip, desc);
> +
> + for (i = 0; i < data->info->num_gpios; i = i + 31) {
> + gpda_reg_offset = rtd_gpio_gpda_offset(data, i);
> + status = readl_relaxed(data->irq_base + gpda_reg_offset) >> 1;
> + writel_relaxed(status << 1, data->irq_base + gpda_reg_offset);
> +
> + while (status) {
> + j = __ffs(status);
> + status &= ~BIT(j);
> + hwirq = i + j;
> + if (rtd_gpio_check_ie(data, hwirq)) {
> + int irq = irq_find_mapping(data->domain, hwirq);
> + u32 irq_type = irq_get_trigger_type(irq);
> +
> + if ((irq_type & IRQ_TYPE_SENSE_MASK) == IRQ_TYPE_EDGE_BOTH)
> + generic_handle_irq(irq);
> + }
> + }
> + }
> +
> + chained_irq_exit(chip, desc);
> +}
There is some code duplication here. Create wrapper calls with parameters
so you don't need to have several functions that look almost the same.
> +static int rtd_gpio_probe(struct platform_device *pdev)
> +{
> + struct rtd_gpio *data;
> + const struct of_device_id *match;
> + struct device_node *node;
Don't go looking by the OF node, use the device:
struct device *dev = &pdev->dev;
> + int ret;
> + int i;
> +
> + node = pdev->dev.of_node;
Use #include <linux/property.h>
> + match = of_match_node(rtd_gpio_of_matches, pdev->dev.of_node);
> + if (!match || !match->data)
> + return -EINVAL;
Use
data->info = device_get_match_data(dev); instead
if (!data->info)...
> +
> + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
With a local dev you can just devm_kzalloc(dev, ...) etc.
> + data->assert_irq = irq_of_parse_and_map(node, 0);
> + if (!data->assert_irq)
> + goto deferred;
> +
> + data->deassert_irq = irq_of_parse_and_map(node, 1);
> + if (!data->deassert_irq)
> + goto deferred;
So one handler for rising and one handler for falling edge?
Hm that's different. I guess you need separate handlers.
> + data->base = of_iomap(node, 0);
> + if (!data->base)
> + return -ENXIO;
Use
data->base = devm_platform_ioremap_resource(pdev, 0);
> + data->irq_base = of_iomap(node, 1);
> + if (!data->irq_base)
> + return -ENXIO;
Use
data->irq_base = platform_get_irq(pdev, 1);
> + data->gpio_chip.parent = &pdev->dev;
Don't assign this, the core will handle it.
> + data->gpio_chip.label = dev_name(&pdev->dev);
> + data->gpio_chip.of_gpio_n_cells = 2;
This is the default, let the core handle OF translation.
> + data->gpio_chip.base = data->info->gpio_base;
> + data->gpio_chip.ngpio = data->info->num_gpios;
> + data->gpio_chip.request = rtd_gpio_request;
> + data->gpio_chip.free = rtd_gpio_free;
> + data->gpio_chip.get_direction = rtd_gpio_get_direction;
> + data->gpio_chip.direction_input = rtd_gpio_direction_input;
> + data->gpio_chip.direction_output = rtd_gpio_direction_output;
> + data->gpio_chip.set = rtd_gpio_set;
> + data->gpio_chip.get = rtd_gpio_get;
> + data->gpio_chip.set_config = rtd_gpio_set_config;
> + data->gpio_chip.to_irq = rtd_gpio_to_irq;
Use the GPIOLIB_IRQCHIP to provide this for you.
> + data->irq_chip = rtd_gpio_irq_chip;
Convert to use immutable irq_chip. (Maybe several struct irq_chip if you need!)
> + data->domain = irq_domain_add_linear(node, data->gpio_chip.ngpio,
> + &irq_domain_simple_ops, data);
> + if (!data->domain) {
> + devm_kfree(&pdev->dev, data);
> + return -ENOMEM;
> + }
> +
> + for (i = 0; i < data->gpio_chip.ngpio; i++) {
> + int irq = irq_create_mapping(data->domain, i);
> +
> + irq_set_chip_data(irq, data);
> + irq_set_chip_and_handler(irq, &data->irq_chip, handle_simple_irq);
> + }
> +
> + irq_set_chained_handler_and_data(data->assert_irq, rtd_gpio_assert_irq_handle, data);
> + irq_set_chained_handler_and_data(data->deassert_irq, rtd_gpio_deassert_irq_handle, data);
Instead of doing this use GPIOLIB_IRQCHIP.
Before registering the gpio_chip set up stuff somewhat like this:
girq = &data->gpio_chip.irq;
gpio_irq_chip_set_chip(girq, &my_irq_chip);
girq->parent_handler = my_gpio_irq_handler;
girq->num_parents = 1;
girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),
GFP_KERNEL);
if (!girq->parents)
ret = -ENOMEM;
girq->default_type = IRQ_TYPE_NONE;
girq->handler = handle_bad_irq;
girq->parents[0] = irq;
But maybe in this case you want two parent IRQs? Not sure.
> +deferred:
> + devm_kfree(&pdev->dev, data);
> + return -EPROBE_DEFER;
Nope, when you return with an error from probe() all
allocations using devm_* are automatically free:ed that
is kind of the point of the managed resources.
Yours,
Linus Walleij
