All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] gpio: Add support for IDT 79RC3243x GPIO controller
@ 2021-04-20 12:39 Thomas Bogendoerfer
  2021-04-20 12:39 ` [PATCH 2/2] dt-bindings: gpio: Add devicetree binding " Thomas Bogendoerfer
       [not found] ` <CAHp75VcQ4WXm3uS2r=uDpA4+0vPWdKjev6=vV_JDxMLPzpHDRw@mail.gmail.com>
  0 siblings, 2 replies; 9+ messages in thread
From: Thomas Bogendoerfer @ 2021-04-20 12:39 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, linux-kernel, linux-gpio

IDT 79RC3243x SoCs integrated a gpio controller, which handles up
to 32 gpios. All gpios could be used as interrupt source.

Signed-off-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
---
 drivers/gpio/Kconfig         |  10 ++
 drivers/gpio/Makefile        |   1 +
 drivers/gpio/gpio-idt3243x.c | 210 +++++++++++++++++++++++++++++++++++
 3 files changed, 221 insertions(+)
 create mode 100644 drivers/gpio/gpio-idt3243x.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index e3607ec4c2e8..6847a06ffcfe 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -770,6 +770,16 @@ config GPIO_MSC313
 	  Say Y here to support the main GPIO block on MStar/SigmaStar
 	  ARMv7 based SoCs.
 
+config GPIO_IDT3243X
+	bool "IDT 79RC3243X GPIO support"
+	default y if MIKROTIK_RB532
+	depends on MIKROTIK_RB532 || COMPILE_TEST
+	select GPIO_GENERIC
+	select GPIOLIB_IRQCHIP
+	help
+	  Select this option to enable GPIO driver for
+	  IDT 79RC3243X SoC devices.
+
 endmenu
 
 menu "Port-mapped I/O GPIO drivers"
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index c58a90a3c3b1..75dd9c5665c5 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_GPIO_HISI)                 += gpio-hisi.o
 obj-$(CONFIG_GPIO_HLWD)			+= gpio-hlwd.o
 obj-$(CONFIG_HTC_EGPIO)			+= gpio-htc-egpio.o
 obj-$(CONFIG_GPIO_ICH)			+= gpio-ich.o
+obj-$(CONFIG_GPIO_IDT3243X)		+= gpio-idt3243x.o
 obj-$(CONFIG_GPIO_IOP)			+= gpio-iop.o
 obj-$(CONFIG_GPIO_IT87)			+= gpio-it87.o
 obj-$(CONFIG_GPIO_IXP4XX)		+= gpio-ixp4xx.o
diff --git a/drivers/gpio/gpio-idt3243x.c b/drivers/gpio/gpio-idt3243x.c
new file mode 100644
index 000000000000..eaee46480268
--- /dev/null
+++ b/drivers/gpio/gpio-idt3243x.c
@@ -0,0 +1,210 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for IDT/Renesas 79RC3243x Interrupt Controller.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/gpio/driver.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+
+#define IDT_GPIO_NR_IRQS	32
+
+#define IDT_PIC_IRQ_PEND	0x00
+#define IDT_PIC_IRQ_MASK	0x08
+
+#define IDT_GPIO_DIR		0x04
+#define IDT_GPIO_DATA		0x08
+#define IDT_GPIO_ILEVEL		0x0C
+#define IDT_GPIO_ISTAT		0x10
+
+struct idt_gpio_ctrl {
+	struct gpio_chip gc;
+	void __iomem *pic;
+	void __iomem *gpio;
+	u32 mask_cache;
+};
+
+static struct idt_gpio_ctrl *irq_data_to_idt_gpio(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+
+	return container_of(gc, struct idt_gpio_ctrl, gc);
+}
+
+static void idt_gpio_dispatch(struct irq_desc *desc)
+{
+	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
+	struct idt_gpio_ctrl *ctrl = container_of(gc, struct idt_gpio_ctrl, gc);
+	struct irq_chip *host_chip = irq_desc_get_chip(desc);
+	u32 pending, hwirq, virq;
+
+	chained_irq_enter(host_chip, desc);
+
+	pending = readl(ctrl->pic + IDT_PIC_IRQ_PEND);
+	pending &= ~ctrl->mask_cache;
+	while (pending) {
+		hwirq = __fls(pending);
+		virq = irq_linear_revmap(gc->irq.domain, hwirq);
+		if (virq)
+			generic_handle_irq(virq);
+		pending &= ~(1 << hwirq);
+	}
+
+	chained_irq_exit(host_chip, desc);
+}
+
+static int idt_gpio_irq_set_type(struct irq_data *d, unsigned int flow_type)
+{
+	struct idt_gpio_ctrl *ctrl = irq_data_to_idt_gpio(d);
+	unsigned int sense = flow_type & IRQ_TYPE_SENSE_MASK;
+	u32 ilevel;
+
+	if (sense & ~(IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))
+		return -EINVAL;
+
+	ilevel = readl(ctrl->gpio + IDT_GPIO_ILEVEL);
+	if (sense & IRQ_TYPE_LEVEL_HIGH)
+		ilevel |= BIT(d->hwirq);
+	else if (sense & IRQ_TYPE_LEVEL_LOW)
+		ilevel &= ~BIT(d->hwirq);
+	else
+		return -EINVAL;
+
+	writel(ilevel, ctrl->gpio + IDT_GPIO_ILEVEL);
+	return 0;
+}
+
+static void idt_gpio_ack(struct irq_data *d)
+{
+	struct idt_gpio_ctrl *ctrl = irq_data_to_idt_gpio(d);
+
+	writel(~BIT(d->hwirq), ctrl->gpio + IDT_GPIO_ISTAT);
+}
+
+static void idt_gpio_mask(struct irq_data *d)
+{
+	struct idt_gpio_ctrl *ctrl = irq_data_to_idt_gpio(d);
+
+	ctrl->mask_cache |= BIT(d->hwirq);
+	writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK);
+}
+
+static void idt_gpio_unmask(struct irq_data *d)
+{
+	struct idt_gpio_ctrl *ctrl = irq_data_to_idt_gpio(d);
+
+	ctrl->mask_cache &= ~BIT(d->hwirq);
+	writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK);
+}
+
+static struct irq_chip idt_gpio_irqchip = {
+	.name = "IDTGPIO",
+	.irq_mask = idt_gpio_mask,
+	.irq_ack = idt_gpio_ack,
+	.irq_unmask = idt_gpio_unmask,
+	.irq_set_type = idt_gpio_irq_set_type
+};
+
+static int idt_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct gpio_irq_chip *girq;
+	struct idt_gpio_ctrl *ctrl;
+	unsigned int parent_irq;
+	int ngpios;
+	int ret;
+
+	ret = device_property_read_u32(dev, "ngpios", &ngpios);
+	if (ret) {
+		dev_err(dev, "ngpios property is not valid\n");
+		return ret;
+	}
+
+	ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
+	if (!ctrl)
+		return -ENOMEM;
+
+	ctrl->gpio = devm_platform_ioremap_resource_byname(pdev, "gpio");
+	if (!ctrl->gpio)
+		return -ENOMEM;
+
+	ctrl->gc.parent = dev;
+
+	ret = bgpio_init(&ctrl->gc, &pdev->dev, 4, ctrl->gpio + IDT_GPIO_DATA,
+			 NULL, NULL, ctrl->gpio + IDT_GPIO_DIR, NULL, 0);
+	if (ret) {
+		dev_err(dev, "bgpio_init failed\n");
+		return ret;
+	}
+	ctrl->gc.ngpio = ngpios;
+
+	ctrl->pic = devm_platform_ioremap_resource_byname(pdev, "pic");
+	if (!ctrl->pic)
+		return -ENOMEM;
+
+	parent_irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
+	if (!parent_irq) {
+		dev_err(&pdev->dev, "Failed to map parent IRQ!\n");
+		return -EINVAL;
+	}
+
+	/* Mask interrupts. */
+	ctrl->mask_cache = 0xffffffff;
+	writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK);
+
+	girq = &ctrl->gc.irq;
+	girq->chip = &idt_gpio_irqchip;
+	girq->parent_handler = idt_gpio_dispatch;
+	girq->num_parents = 1;
+	girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),
+				     GFP_KERNEL);
+	if (!girq->parents) {
+		ret = -ENOMEM;
+		goto out_unmap_irq;
+	}
+	girq->parents[0] = parent_irq;
+	girq->default_type = IRQ_TYPE_NONE;
+	girq->handler = handle_level_irq;
+
+	ret = devm_gpiochip_add_data(&pdev->dev, &ctrl->gc, NULL);
+	if (ret)
+		goto out_unmap_irq;
+
+	return 0;
+
+out_unmap_irq:
+	irq_dispose_mapping(parent_irq);
+	return ret;
+}
+
+static const struct of_device_id idt_gpio_of_match[] = {
+	{ .compatible = "idt,3243x-gpio" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, idt_gpio_of_match);
+
+static struct platform_driver idt_gpio_driver = {
+	.probe = idt_gpio_probe,
+	.driver = {
+		.name = "idt3243x-gpio",
+		.of_match_table = idt_gpio_of_match,
+	},
+};
+
+static int __init idt_gpio_init(void)
+{
+	return platform_driver_register(&idt_gpio_driver);
+}
+
+arch_initcall(idt_gpio_init);
+
+MODULE_DESCRIPTION("IDT 79RC3243x GPIO/PIC Driver");
+MODULE_AUTHOR("Thomas Bogendoerfer <tsbogend@alpha.franken.de>");
+MODULE_LICENSE("GPL");
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/2] dt-bindings: gpio: Add devicetree binding for IDT 79RC3243x GPIO controller
  2021-04-20 12:39 [PATCH 1/2] gpio: Add support for IDT 79RC3243x GPIO controller Thomas Bogendoerfer
@ 2021-04-20 12:39 ` Thomas Bogendoerfer
       [not found] ` <CAHp75VcQ4WXm3uS2r=uDpA4+0vPWdKjev6=vV_JDxMLPzpHDRw@mail.gmail.com>
  1 sibling, 0 replies; 9+ messages in thread
From: Thomas Bogendoerfer @ 2021-04-20 12:39 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, Rob Herring, linux-gpio,
	devicetree, linux-kernel

Add YAML devicetree binding for IDT 79RC3243x GPIO controller

Signed-off-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
---
 .../bindings/gpio/gpio-idt3243x.yaml          | 73 +++++++++++++++++++
 1 file changed, 73 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-idt3243x.yaml

diff --git a/Documentation/devicetree/bindings/gpio/gpio-idt3243x.yaml b/Documentation/devicetree/bindings/gpio/gpio-idt3243x.yaml
new file mode 100644
index 000000000000..346a57ef8298
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-idt3243x.yaml
@@ -0,0 +1,73 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/gpio-idt3243x.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: IDT 79RC32434x GPIO controller
+
+maintainers:
+  - Thomas Bogendoerfer <tsbogend@alpha.franken.de>
+
+properties:
+  compatible:
+    const: idt,3243x-gpio
+
+  reg:
+    maxItems: 2
+
+  reg-names:
+    items:
+      - const: gpio
+      - const: pic
+
+  gpio-controller: true
+
+  "#gpio-cells":
+    const: 2
+
+  ngpios:
+    description:
+      Number of available gpios in a bank.
+    minimum: 1
+    maximum: 32
+
+  interrupt-controller: true
+
+  "#interrupt-cells":
+    const: 2
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - gpio-controller
+  - "#gpio-cells"
+  - ngpios
+  - interrupt-controller
+  - "#interrupt-cells"
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    gpio0: interrupt-controller@50000 {
+        compatible = "idt,3243x-gpio";
+        reg = <0x50000 0x14>, <0x38030 0x0c>;
+        reg-names = "gpio", "pic";
+
+        interrupt-controller;
+        #interrupt-cells = <2>;
+
+        interrupt-parent = <&cpuintc>;
+        interrupts = <6>;
+
+        gpio-controller;
+        #gpio-cells = <2>;
+
+        ngpios = <14>;
+    };
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] gpio: Add support for IDT 79RC3243x GPIO controller
       [not found] ` <CAHp75VcQ4WXm3uS2r=uDpA4+0vPWdKjev6=vV_JDxMLPzpHDRw@mail.gmail.com>
@ 2021-04-21  8:32   ` Thomas Bogendoerfer
  2021-04-21  8:48     ` Andy Shevchenko
  0 siblings, 1 reply; 9+ messages in thread
From: Thomas Bogendoerfer @ 2021-04-21  8:32 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Linus Walleij, Bartosz Golaszewski, linux-kernel, linux-gpio

On Wed, Apr 21, 2021 at 11:09:51AM +0300, Andy Shevchenko wrote:
> On Tuesday, April 20, 2021, Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> wrote:
> 
> > IDT 79RC3243x SoCs integrated a gpio controller, which handles up
> > to 32 gpios. All gpios could be used as interrupt source.
> 
> 
> 
> I would recommend looking for latest new drivers in GPIO subsystem to see
> how you may improve yours.

Could give me a better pointer to it ? I looked at a lot of gpio driver
and took what fitted best.

> Here just one question, why it can not be a module

that's probably doable...

> why arch_initcall() is used

without that interrupts weren't avaiable early enough. 

> and why you put a dead code into it (see the first part of the
> question)?

hmm, pointer please ?

> I will perform a deep review later on.

thank you.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] gpio: Add support for IDT 79RC3243x GPIO controller
  2021-04-21  8:32   ` [PATCH 1/2] gpio: Add support " Thomas Bogendoerfer
@ 2021-04-21  8:48     ` Andy Shevchenko
  2021-04-21  9:18       ` Thomas Bogendoerfer
  0 siblings, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2021-04-21  8:48 UTC (permalink / raw)
  To: Thomas Bogendoerfer
  Cc: Linus Walleij, Bartosz Golaszewski, linux-kernel, linux-gpio

On Wed, Apr 21, 2021 at 11:32 AM Thomas Bogendoerfer
<tsbogend@alpha.franken.de> wrote:
> On Wed, Apr 21, 2021 at 11:09:51AM +0300, Andy Shevchenko wrote:
> > On Tuesday, April 20, 2021, Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> > wrote:
> >
> > > IDT 79RC3243x SoCs integrated a gpio controller, which handles up
> > > to 32 gpios. All gpios could be used as interrupt source.
> >
> >
> >
> > I would recommend looking for latest new drivers in GPIO subsystem to see
> > how you may improve yours.
>
> Could give me a better pointer to it ? I looked at a lot of gpio driver
> and took what fitted best.
>
> > Here just one question, why it can not be a module
>
> that's probably doable...
>
> > why arch_initcall() is used
>
> without that interrupts weren't avaiable early enough.
>
> > and why you put a dead code into it (see the first part of the
> > question)?
>
> hmm, pointer please ?

It's already in the question above, do your homework :-)

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] gpio: Add support for IDT 79RC3243x GPIO controller
  2021-04-21  8:48     ` Andy Shevchenko
@ 2021-04-21  9:18       ` Thomas Bogendoerfer
  2021-04-21  9:54         ` Andy Shevchenko
  0 siblings, 1 reply; 9+ messages in thread
From: Thomas Bogendoerfer @ 2021-04-21  9:18 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Linus Walleij, Bartosz Golaszewski, linux-kernel, linux-gpio

On Wed, Apr 21, 2021 at 11:48:59AM +0300, Andy Shevchenko wrote:
> On Wed, Apr 21, 2021 at 11:32 AM Thomas Bogendoerfer
> <tsbogend@alpha.franken.de> wrote:
> > On Wed, Apr 21, 2021 at 11:09:51AM +0300, Andy Shevchenko wrote:
> > > On Tuesday, April 20, 2021, Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> > > wrote:
> > >
> > > > IDT 79RC3243x SoCs integrated a gpio controller, which handles up
> > > > to 32 gpios. All gpios could be used as interrupt source.
> > >
> > >
> > >
> > > I would recommend looking for latest new drivers in GPIO subsystem to see
> > > how you may improve yours.
> >
> > Could give me a better pointer to it ? I looked at a lot of gpio driver
> > and took what fitted best.
> >
> > > Here just one question, why it can not be a module
> >
> > that's probably doable...
> >
> > > why arch_initcall() is used
> >
> > without that interrupts weren't avaiable early enough.
> >
> > > and why you put a dead code into it (see the first part of the
> > > question)?
> >
> > hmm, pointer please ?
> 
> It's already in the question above, do your homework :-)

is this some sort of joke I'm not getting ?

git log --oneline drivers/gpio/Makefile

2ad74f40dacc gpio: visconti: Add Toshiba Visconti GPIO support

that's the latest driver added in v5.12-rc8. Is that a good one ?

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] gpio: Add support for IDT 79RC3243x GPIO controller
  2021-04-21  9:18       ` Thomas Bogendoerfer
@ 2021-04-21  9:54         ` Andy Shevchenko
  2021-04-21 10:37           ` Thomas Bogendoerfer
  0 siblings, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2021-04-21  9:54 UTC (permalink / raw)
  To: Thomas Bogendoerfer
  Cc: Linus Walleij, Bartosz Golaszewski, linux-kernel, linux-gpio

On Wed, Apr 21, 2021 at 12:19 PM Thomas Bogendoerfer
<tsbogend@alpha.franken.de> wrote:
> On Wed, Apr 21, 2021 at 11:48:59AM +0300, Andy Shevchenko wrote:
> > On Wed, Apr 21, 2021 at 11:32 AM Thomas Bogendoerfer
> > <tsbogend@alpha.franken.de> wrote:
> > > On Wed, Apr 21, 2021 at 11:09:51AM +0300, Andy Shevchenko wrote:
> > > > On Tuesday, April 20, 2021, Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> > > > wrote:
> > > >
> > > > > IDT 79RC3243x SoCs integrated a gpio controller, which handles up
> > > > > to 32 gpios. All gpios could be used as interrupt source.
> > > >
> > > >
> > > >
> > > > I would recommend looking for latest new drivers in GPIO subsystem to see
> > > > how you may improve yours.
> > >
> > > Could give me a better pointer to it ? I looked at a lot of gpio driver
> > > and took what fitted best.
> > >
> > > > Here just one question, why it can not be a module
> > >
> > > that's probably doable...
> > >
> > > > why arch_initcall() is used
> > >
> > > without that interrupts weren't avaiable early enough.
> > >
> > > > and why you put a dead code into it (see the first part of the
> > > > question)?
> > >
> > > hmm, pointer please ?
> >
> > It's already in the question above, do your homework :-)
>
> is this some sort of joke I'm not getting ?

Maybe, but no, it's not a joke. You asked where the dead code is. I
answered that you as the author of the code should have a better
understanding of what you are doing. I can admit that it's hard to
cover all aspects of the kernel programming in one go, but at least
this part is a low hanging fruit.

As I promised you, I will do a deep review later on, I'm giving you
time to find issues yourself. That's how you may actually learn the
things. It's solely your choice to follow or not, my promise will be
kept and you will get an answer anyway.

> git log --oneline drivers/gpio/Makefile
>
> 2ad74f40dacc gpio: visconti: Add Toshiba Visconti GPIO support
>
> that's the latest driver added in v5.12-rc8. Is that a good one ?

Briefly looking, I haven't found any problems with that code. It looks neat.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] gpio: Add support for IDT 79RC3243x GPIO controller
  2021-04-21  9:54         ` Andy Shevchenko
@ 2021-04-21 10:37           ` Thomas Bogendoerfer
  2021-04-21 10:46             ` Andy Shevchenko
  0 siblings, 1 reply; 9+ messages in thread
From: Thomas Bogendoerfer @ 2021-04-21 10:37 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Linus Walleij, Bartosz Golaszewski, linux-kernel, linux-gpio

On Wed, Apr 21, 2021 at 12:54:53PM +0300, Andy Shevchenko wrote:
> As I promised you, I will do a deep review later on, I'm giving you
> time to find issues yourself. That's how you may actually learn the
> things. It's solely your choice to follow or not, my promise will be
> kept and you will get an answer anyway.

so let's make it a challenge ;-)

I see I could use gpiochip_get_data() in few place. 

Is there more you see ?

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] gpio: Add support for IDT 79RC3243x GPIO controller
  2021-04-21 10:37           ` Thomas Bogendoerfer
@ 2021-04-21 10:46             ` Andy Shevchenko
  2021-04-21 11:15               ` Thomas Bogendoerfer
  0 siblings, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2021-04-21 10:46 UTC (permalink / raw)
  To: Thomas Bogendoerfer
  Cc: Linus Walleij, Bartosz Golaszewski, linux-kernel, linux-gpio

On Wed, Apr 21, 2021 at 1:37 PM Thomas Bogendoerfer
<tsbogend@alpha.franken.de> wrote:
>
> On Wed, Apr 21, 2021 at 12:54:53PM +0300, Andy Shevchenko wrote:
> > As I promised you, I will do a deep review later on, I'm giving you
> > time to find issues yourself. That's how you may actually learn the
> > things. It's solely your choice to follow or not, my promise will be
> > kept and you will get an answer anyway.
>
> so let's make it a challenge ;-)
>
> I see I could use gpiochip_get_data() in few place.
>
> Is there more you see ?

Good.

For now:
- dead code due to driver not being compiled as module
- too verbose Kconfig machinery (it's not about the "help" part!)
- open coded stuff in IRQ handler
- whatever you found.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] gpio: Add support for IDT 79RC3243x GPIO controller
  2021-04-21 10:46             ` Andy Shevchenko
@ 2021-04-21 11:15               ` Thomas Bogendoerfer
  0 siblings, 0 replies; 9+ messages in thread
From: Thomas Bogendoerfer @ 2021-04-21 11:15 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Linus Walleij, Bartosz Golaszewski, linux-kernel, linux-gpio

On Wed, Apr 21, 2021 at 01:46:43PM +0300, Andy Shevchenko wrote:
> On Wed, Apr 21, 2021 at 1:37 PM Thomas Bogendoerfer
> <tsbogend@alpha.franken.de> wrote:
> >
> > On Wed, Apr 21, 2021 at 12:54:53PM +0300, Andy Shevchenko wrote:
> > > As I promised you, I will do a deep review later on, I'm giving you
> > > time to find issues yourself. That's how you may actually learn the
> > > things. It's solely your choice to follow or not, my promise will be
> > > kept and you will get an answer anyway.
> >
> > so let's make it a challenge ;-)
> >
> > I see I could use gpiochip_get_data() in few place.
> >
> > Is there more you see ?
> 
> Good.
> 
> For now:
> - dead code due to driver not being compiled as module

Can you explain, why it's dead code, if it's not compilable as module ?

> - too verbose Kconfig machinery (it's not about the "help" part!)

the default y part ? Well I'm converting the MIPS rb532 platform to
device tree. So I'm trying to make the whole process as bisectable
as possible. And this would help, but I've no problem dropping that.

> - open coded stuff in IRQ handler

done.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-04-21 11:16 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-20 12:39 [PATCH 1/2] gpio: Add support for IDT 79RC3243x GPIO controller Thomas Bogendoerfer
2021-04-20 12:39 ` [PATCH 2/2] dt-bindings: gpio: Add devicetree binding " Thomas Bogendoerfer
     [not found] ` <CAHp75VcQ4WXm3uS2r=uDpA4+0vPWdKjev6=vV_JDxMLPzpHDRw@mail.gmail.com>
2021-04-21  8:32   ` [PATCH 1/2] gpio: Add support " Thomas Bogendoerfer
2021-04-21  8:48     ` Andy Shevchenko
2021-04-21  9:18       ` Thomas Bogendoerfer
2021-04-21  9:54         ` Andy Shevchenko
2021-04-21 10:37           ` Thomas Bogendoerfer
2021-04-21 10:46             ` Andy Shevchenko
2021-04-21 11:15               ` Thomas Bogendoerfer

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.