From: Andy Shevchenko <andy@kernel.org>
To: TY Chang <tychang@realtek.com>
Cc: Linus Walleij <linus.walleij@linaro.org>,
Bartosz Golaszewski <brgl@bgdev.pl>,
Rob Herring <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Conor Dooley <conor+dt@kernel.org>,
linux-gpio@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 2/2] Add GPIO support for Realtek DHC(Digital Home Center) RTD SoCs.
Date: Thu, 7 Dec 2023 15:45:17 +0200 [thread overview]
Message-ID: <ZXHMbZRXLXGa_tq8@smile.fi.intel.com> (raw)
In-Reply-To: <20231207100723.15015-3-tychang@realtek.com>
On Thu, Dec 07, 2023 at 06:07:23PM +0800, TY Chang wrote:
> From: Tzuyi Chang <tychang@realtek.com>
>
> This driver enables configuration of GPIO direction, GPIO values, GPIO
> debounce settings and handles GPIO interrupts.
Why gpio-regmap can't be used?
...
> +struct rtd_gpio_info {
> + u8 *dir_offset;
> + u8 num_dir;
> + u8 *dato_offset;
> + u8 num_dato;
> + u8 *dati_offset;
> + u8 num_dati;
> + u8 *ie_offset;
> + u8 num_ie;
> + u8 *dp_offset;
> + u8 num_dp;
> + u8 *gpa_offset;
> + u8 num_gpa;
> + u8 *gpda_offset;
> + u8 num_gpda;
> + u8 *deb_offset;
> + u8 num_deb;
A lot of wasted space. Can you group pointers followed by u8 members?
Note, use `pahole` tool to check the struct layout in C.
> +};
...
> +struct rtd_gpio {
> + struct platform_device *pdev;
Why
struct device *dev;
is not suffice?
> + const struct rtd_gpio_info *info;
> + void __iomem *base;
> + void __iomem *irq_base;
> + struct gpio_chip gpio_chip;
Make this to be the first member, it might reduce some code
(due to pointer arithmetics).
> + unsigned int irqs[2];
> + spinlock_t lock;
> +};
> +
> +
One blank line is enough.
...
> +static const struct rtd_gpio_info rtd_iso_gpio_info = {
> + .name = "rtd_iso_gpio",
> + .type = RTD_ISO_GPIO,
> + .gpio_base = 0,
> + .num_gpios = 82,
> + .dir_offset = (u8 []){ 0x0, 0x18, 0x2c },
> + .num_dir = 3,
> + .dato_offset = (u8 []){ 0x4, 0x1c, 0x30 },
> + .num_dato = 3,
> + .dati_offset = (u8 []){ 0x8, 0x20, 0x34 },
> + .num_dati = 3,
> + .ie_offset = (u8 []){ 0xc, 0x24, 0x38 },
> + .num_ie = 3,
> + .dp_offset = (u8 []){ 0x10, 0x28, 0x3c },
> + .num_dp = 3,
> + .gpa_offset = (u8 []){ 0x8, 0xe0, 0x90 },
> + .num_gpa = 3,
> + .gpda_offset = (u8 []){ 0xc, 0xe4, 0x94 },
> + .num_gpda = 3,
> + .deb_offset = (u8 []){ 0x44, 0x48, 0x4c, 0x50, 0x54, 0x58, 0x5c,
> + 0x60, 0x64, 0x68, 0x6c },
> + .num_deb = 11,
Use ARRAY_SIZE() from array_size.h for all num_* assignments.
> +};
...
> +static const struct rtd_gpio_info rtd1619_iso_gpio_info = {
Ditto.
> +};
...
> +static const struct rtd_gpio_info rtd1395_iso_gpio_info = {
Ditto.
> +};
> +
> +static const struct rtd_gpio_info rtd1295_misc_gpio_info = {
Ditto.
> +};
> +
> +static const struct rtd_gpio_info rtd1295_iso_gpio_info = {
Ditto.
> +};
...
> +static int rtd_gpio_dir_offset(struct rtd_gpio *data, unsigned int offset)
> +{
> + int index = offset / 32;
> + if (index > data->info->num_dir)
> + return -EINVAL;
When this conditional can be true?
Same Q to the similar checks over the code.
> + return data->info->dir_offset[index];
> +}
...
> + if (data->info->type == RTD1295_ISO_GPIO) {
> + reg_offset = rtd_gpio_deb_offset(data, 0);
> + if (reg_offset < 0)
> + return reg_offset;
> + shift = 0;
> + deb_val += 1;
> + write_en = BIT(shift + 3);
> + } else if (data->info->type == RTD1295_MISC_GPIO) {
> + reg_offset = rtd_gpio_deb_offset(data, 0);
> + if (reg_offset < 0)
> + return reg_offset;
> + shift = (offset >> 4) * 4;
> + deb_val += 1;
> + write_en = BIT(shift + 3);
> + } else {
> + reg_offset = rtd_gpio_deb_offset(data, offset);
> + if (reg_offset < 0)
> + return reg_offset;
> + shift = (offset % 8) * 4;
> + write_en = BIT(shift + 3);
> + }
You should probably have kind of chip_info constant structure that goes via
driver_data and will have a callback, so, here you will call one and get all
three at once:
- register offset;
- shift
- updated debounce value
...
> +static int rtd_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
> +{
> + struct rtd_gpio *data = gpiochip_get_data(chip);
> + unsigned long flags;
> + int reg_offset;
> + u32 val;
> +
> + reg_offset = rtd_gpio_dir_offset(data, offset);
> + if (reg_offset < 0)
> + return reg_offset;
> + spin_lock_irqsave(&data->lock, flags);
So, is your IRQ chip going to work with CONFIG_PREEMT_RT?
> + val = readl_relaxed(data->base + reg_offset);
> + val &= BIT(offset % 32);
Why this is is under lock?
> + spin_unlock_irqrestore(&data->lock, flags);
> +
> + return val ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;
> +}
...
> +static int rtd_gpio_set_direction(struct gpio_chip *chip, unsigned int offset, bool out)
> +{
> + unsigned long flags;
> + spin_lock_irqsave(&data->lock, flags);
> + spin_unlock_irqrestore(&data->lock, flags);
Consider to utilise guard() / scoped_guard() from cleanup.h.
> +}
...
> +static int rtd_gpio_direction_output(struct gpio_chip *chip, unsigned int offset, int value)
> +{
> + chip->set(chip, offset, value);
Why? Can't you call the function by its name directly?
> +
> + return rtd_gpio_set_direction(chip, offset, true);
> +}
...
> +static int rtd_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> + struct rtd_gpio *data = gpiochip_get_data(chip);
> + int dir_reg_offset, dat_reg_offset;
> + unsigned long flags;
> + u32 val;
> +
> + dir_reg_offset = rtd_gpio_dir_offset(data, offset);
> + if (dir_reg_offset < 0)
> + return dir_reg_offset;
> +
> + spin_lock_irqsave(&data->lock, flags);
> +
> + val = readl_relaxed(data->base + dir_reg_offset);
> + val &= BIT(offset % 32);
> + dat_reg_offset = val ?
> + rtd_gpio_dato_offset(data, offset) : rtd_gpio_dati_offset(data, offset);
Can't you have the direction be cached and already know which offset to use
even before the lock?
> + val = readl_relaxed(data->base + dat_reg_offset);
> + val >>= offset % 32;
> + val &= 0x1;
Why were these operations done under the lock?
> + spin_unlock_irqrestore(&data->lock, flags);
> +
> + return val;
> +}
...
> +static void rtd_gpio_irq_handle(struct irq_desc *desc)
> +{
> + int (*get_reg_offset)(struct rtd_gpio *gpio, unsigned int offset);
> + struct rtd_gpio *data = irq_desc_get_handler_data(desc);
> + struct irq_domain *domain = data->gpio_chip.irq.domain;
> + struct irq_chip *chip = irq_desc_get_chip(desc);
> + unsigned int irq = irq_desc_get_irq(desc);
> + int reg_offset;
> + u32 status;
> + int hwirq;
Why signed?
> + int i;
> + int j;
> +
> + chained_irq_enter(chip, desc);
> + if (irq == data->irqs[0])
> + get_reg_offset = &rtd_gpio_gpa_offset;
> + else if (irq == data->irqs[1])
> + get_reg_offset = &rtd_gpio_gpda_offset;
Can't it be done before entering into chained IRQ handler?
> + for (i = 0; i < data->info->num_gpios; i = i + 31) {
31 ?! In any case i += 31 is simply shorter.
> + reg_offset = get_reg_offset(data, i);
> + if (reg_offset < 0)
> + return;
> +
> + status = readl_relaxed(data->irq_base + reg_offset) >> 1;
> + writel_relaxed(status << 1, data->irq_base + reg_offset);
> + while (status) {
> + j = __ffs(status);
> + status &= ~BIT(j);
NIH for_each_set_bit()
> + hwirq = i + j;
> + if (rtd_gpio_check_ie(data, hwirq)) {
> + int girq = irq_find_mapping(domain, hwirq);
> + u32 irq_type = irq_get_trigger_type(girq);
> +
> + if ((irq == data->irqs[1]) && ((irq_type & IRQ_TYPE_SENSE_MASK) !=
> + IRQ_TYPE_EDGE_BOTH))
> + break;
> + generic_handle_irq(girq);
Why you can't use generic_handle_domain_irq()?
> + }
> + }
> + }
> +
> + chained_irq_exit(chip, desc);
> +}
...
> + u32 mask = BIT(d->hwirq % 32);
Use proper type and getter for hwirq. It's mentioned in the Documentation.
...
> +static const struct irq_chip rtd_gpio_irq_chip = {
> + .name = "rtd-gpio",
> + .irq_enable = rtd_gpio_enable_irq,
> + .irq_disable = rtd_gpio_disable_irq,
> + .irq_set_type = rtd_gpio_irq_set_type,
> + .flags = IRQCHIP_IMMUTABLE,
Is it? You seems missed something to fulfill the immutability requirements.
Please consult with the Documentation, it's all written there.
> +};
...
> +static const struct of_device_id rtd_gpio_of_matches[] = {
> + { .compatible = "realtek,rtd1295-misc-gpio", .data = &rtd1295_misc_gpio_info },
> + { .compatible = "realtek,rtd1295-iso-gpio", .data = &rtd1295_iso_gpio_info },
> + { .compatible = "realtek,rtd1395-iso-gpio", .data = &rtd1395_iso_gpio_info },
> + { .compatible = "realtek,rtd1619-iso-gpio", .data = &rtd1619_iso_gpio_info },
> + { .compatible = "realtek,rtd1319-iso-gpio", .data = &rtd_iso_gpio_info },
> + { .compatible = "realtek,rtd1619b-iso-gpio", .data = &rtd_iso_gpio_info },
> + { .compatible = "realtek,rtd1319d-iso-gpio", .data = &rtd_iso_gpio_info },
> + { .compatible = "realtek,rtd1315e-iso-gpio", .data = &rtd_iso_gpio_info },
> + { },
No comma in the terminator entry.
> +};
> +MODULE_DEVICE_TABLE(of, rtd_gpio_of_matches);
Move all these closer to its user (struct platform_device below).
...
> + data->gpio_chip.label = dev_name(&pdev->dev);
dev
...
> + data->gpio_chip.fwnode = dev_fwnode(&pdev->dev);
dev
But why setting parent device is not suffice?
...
> +static int rtd_gpio_init(void)
> +{
> + return platform_driver_register(&rtd_gpio_platform_driver);
> +}
> +
Unneeded blank line.
> +subsys_initcall(rtd_gpio_init);
Why? Anything that is not on standard initcall must be justified.
--
With Best Regards,
Andy Shevchenko
next prev parent reply other threads:[~2023-12-07 13:45 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-07 10:07 [PATCH v3 0/2] Add gpio driver support for Realtek DHC SoCs TY Chang
2023-12-07 10:07 ` [PATCH v3 1/2] dt-bindings: gpio: realtek: Add realtek,rtd-gpio TY Chang
2023-12-07 13:54 ` Krzysztof Kozlowski
2023-12-08 9:03 ` TY_Chang[張子逸]
2023-12-08 12:19 ` Krzysztof Kozlowski
2023-12-07 10:07 ` [PATCH v3 2/2] Add GPIO support for Realtek DHC(Digital Home Center) RTD SoCs TY Chang
2023-12-07 13:45 ` Andy Shevchenko [this message]
2023-12-12 9:55 ` TY_Chang[張子逸]
2023-12-13 13:40 ` Andy Shevchenko
2023-12-14 7:57 ` TY_Chang[張子逸]
2023-12-14 13:58 ` Andy Shevchenko
2023-12-14 14:35 ` Michael Walle
2023-12-14 14:49 ` Andy Shevchenko
2023-12-15 6:50 ` TY_Chang[張子逸]
2023-12-18 8:50 ` Michael Walle
[not found] ` <202312181420.3BIEK9gtC1692907@rtits1.realtek.com.tw>
2023-12-20 7:13 ` TY_Chang[張子逸]
2023-12-15 3:29 ` TY_Chang[張子逸]
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=ZXHMbZRXLXGa_tq8@smile.fi.intel.com \
--to=andy@kernel.org \
--cc=brgl@bgdev.pl \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=robh+dt@kernel.org \
--cc=tychang@realtek.com \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.