| 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: <87r19itrjx.wl-maz@kernel.org>
Date: Sat, 08 Jan 2022 11:06:42 +0000
From: Marc Zyngier <maz@...nel.org>
To: Qianggui Song <qianggui.song@...ogic.com>
Cc: Thomas Gleixner <tglx@...utronix.de>,
Kevin Hilman <khilman@...libre.com>,
Neil Armstrong <narmstrong@...libre.com>,
Jerome Brunet <jbrunet@...libre.com>,
Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
<linux-kernel@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-amlogic@...ts.infradead.org>
Subject: Re: [PATCH 4/4] irqchip/meson-gpio: Add support for meson s4 SoCs
On Sat, 08 Jan 2022 08:42:18 +0000,
Qianggui Song <qianggui.song@...ogic.com> wrote:
>
> The meson s4 SoCs support 12 gpio irq lines compared with previous
> serial chips and have something different, details are as below.
>
> IRQ Number:
> - 80:68 13 pins on bank Z
> - 67:48 20 pins on bank X
> - 47:36 12 pins on bank H
> - 35:24 12 pins on bank D
> - 23:22 2 pins on bank E
> - 21:14 8 pins on bank C
> - 13:0 13 pins on bank B
>
> - PADCTRL_GPIO_IRQ_CTRL0
> bit[31]: enable/disable the whole irq lines
s/the whole/all the/
> bit[12-23]: single edge trigger
> bit[0-11]: poll trigger
>
> - PADCTRL_GPIO_IRQ_CTRL[X]
> - bit[0-16]: 7 bits to chooge gpio source for irq line 2*[X] - 2
choose?
> - bit[16-22]:7 bits to chooge gpio source for irq line 2*[X] - 1
> where X = 1-6
>
> - PADCTRL_GPIO_IRQ_CTRL[7]
> bit[0-11]: both edge trigger
This information would fit better in the code than in the commit
message.
>
> Signed-off-by: Qianggui Song <qianggui.song@...ogic.com>
> ---
> drivers/irqchip/irq-meson-gpio.c | 51 ++++++++++++++++++++++++++++++++
> 1 file changed, 51 insertions(+)
>
> diff --git a/drivers/irqchip/irq-meson-gpio.c b/drivers/irqchip/irq-meson-gpio.c
> index 98419428fcbd..c5d20a866c37 100644
> --- a/drivers/irqchip/irq-meson-gpio.c
> +++ b/drivers/irqchip/irq-meson-gpio.c
> @@ -42,6 +42,11 @@
> #define REG_PIN_SEL_SHIFT(x) (((x) % 4) * 8)
> #define REG_FILTER_SEL_SHIFT(x) ((x) * 4)
>
> +/* use for s4 chips */
s/use/Used/
> +#define REG_EDGE_POL_S4 0x1c
> +#define REG_EDGE_POL_MASK_S4(x) \
> + ({typeof(x) _x = (x); BIT(_x) | BIT(12 + (_x)); })
Why on Earth should this macro handle multiple types?
> +
> struct meson_gpio_irq_controller;
> static void meson8_gpio_irq_sel_pin(struct meson_gpio_irq_controller *ctl,
> unsigned int channel, unsigned long hwirq);
> @@ -50,6 +55,9 @@ static void meson_a1_gpio_irq_sel_pin(struct meson_gpio_irq_controller *ctl,
> unsigned int channel,
> unsigned long hwirq);
> static void meson_a1_gpio_irq_init(struct meson_gpio_irq_controller *ctl);
> +static unsigned int
> +meson_s4_gpio_irq_sel_type(struct meson_gpio_irq_controller *ctl,
> + unsigned int idx, u32 val);
>
> struct irq_ctl_ops {
> void (*gpio_irq_sel_pin)(struct meson_gpio_irq_controller *ctl,
> @@ -96,6 +104,17 @@ struct meson_gpio_irq_params {
> .pin_sel_mask = 0x7f, \
> .channel_num = 8, \
>
> +#define INIT_MESON_S4_COMMON_DATA(irqs) \
> + INIT_MESON_COMMON(irqs, meson_a1_gpio_irq_init, \
> + meson_a1_gpio_irq_sel_pin, \
> + meson_s4_gpio_irq_sel_type) \
> + .support_edge_both = true, \
> + .edge_both_offset = 0, \
> + .edge_single_offset = 12, \
> + .pol_low_offset = 0, \
> + .pin_sel_mask = 0xff, \
> + .channel_num = 12, \
> +
> static const struct meson_gpio_irq_params meson8_params = {
> INIT_MESON8_COMMON_DATA(134)
> };
> @@ -126,6 +145,10 @@ static const struct meson_gpio_irq_params a1_params = {
> INIT_MESON_A1_COMMON_DATA(62)
> };
>
> +static const struct meson_gpio_irq_params s4_params = {
> + INIT_MESON_S4_COMMON_DATA(82)
> +};
> +
> static const struct of_device_id meson_irq_gpio_matches[] = {
> { .compatible = "amlogic,meson8-gpio-intc", .data = &meson8_params },
> { .compatible = "amlogic,meson8b-gpio-intc", .data = &meson8b_params },
> @@ -135,6 +158,7 @@ static const struct of_device_id meson_irq_gpio_matches[] = {
> { .compatible = "amlogic,meson-g12a-gpio-intc", .data = &axg_params },
> { .compatible = "amlogic,meson-sm1-gpio-intc", .data = &sm1_params },
> { .compatible = "amlogic,meson-a1-gpio-intc", .data = &a1_params },
> + { .compatible = "amlogic,meson-s4-gpio-intc", .data = &s4_params },
> { }
> };
>
> @@ -202,6 +226,33 @@ static void meson_a1_gpio_irq_init(struct meson_gpio_irq_controller *ctl)
> meson_gpio_irq_update_bits(ctl, REG_EDGE_POL, BIT(31), BIT(31));
> }
>
> +static unsigned int
> +meson_s4_gpio_irq_sel_type(struct meson_gpio_irq_controller *ctl,
> + unsigned int idx, unsigned int type)
An 'unsigned int' return type, directly returned by a caller that
has 'int' as its return type. What could possibly go wrong?
> +{
> + unsigned int val = 0;
> +
> + meson_gpio_irq_update_bits(ctl, REG_EDGE_POL_S4, BIT(0 + (idx)), 0);
Drop the 0 + as well as the useless bracketing all over this function.
> +
> + if (type == IRQ_TYPE_EDGE_BOTH) {
> + val |= BIT(ctl->params->edge_both_offset + (idx));
> + meson_gpio_irq_update_bits(ctl, REG_EDGE_POL_S4,
> + BIT(ctl->params->edge_both_offset + (idx)), val);
> + return 0;
> + }
> +
> + if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING))
> + val |= BIT(ctl->params->pol_low_offset + (idx));
> +
> + if (type & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING))
> + val |= BIT(ctl->params->edge_single_offset + (idx));
> +
> + meson_gpio_irq_update_bits(ctl, REG_EDGE_POL,
> + REG_EDGE_POL_MASK_S4(idx), val);
> +
> + return 0;
> +};
> +
> static int
> meson_gpio_irq_request_channel(struct meson_gpio_irq_controller *ctl,
> unsigned long hwirq,
M.
--
Without deviation from the norm, progress is not possible.
Powered by blists - more mailing lists