gpio: Implement and utilize register structures for ISA drivers
Subsystems (7)
v2 cover gpio: Implement and utilize register structures for ISA drivers
Changes in v2:
- Implement support for the Intel 8255 interface as a new gpio-i8255 module; common code among gpio-104-dio-48e, gpio-104-idi-48, and gpio-gpio-mm are consolidated in the gpio-i8255 module
- Refactor the gpio-104-dio-48e, gpio-104-idi-48, and gpio-gpio-mm to utilize the new gpio-i8255 functions; this greatly simplifies the changes for these drivers
The PC104/ISA drivers were updated to use I/O memory accessor calls such as ioread8()/iowrite8() in a previous patch series [1]. This patchset is a continuation of the effort to improve the code readability and reduce magic numbers by implementing and utilizing named register data structures.
One of the benefits is that we can now observe more easily similarities in devices that share similar interfaces; such as the i8255 interfaces used by the 104-DIO-48E, 104-IDI-48, and GPIO-MM drivers -- as well as the similar interface used by the 104-IDIO-16 and PCI-IDIO-16 drivers.
A new module supporting the Intel 8255 interface is introduced to consolidate the common code found among the 104-DIO-48E, 104-IDI-48, and GPIO-MM drivers.
[1] https://lore.kernel.org/all/[email protected]/
William Breathitt Gray (6):
gpio: i8255: Introduce the i8255 module
gpio: 104-dio-48e: Implement and utilize register structures
gpio: 104-idi-48: Implement and utilize register structures
gpio: gpio-mm: Implement and utilize register structures
gpio: 104-idio-16: Implement and utilize register structures
gpio: ws16c48: Implement and utilize register structures
MAINTAINERS | 6 + drivers/gpio/Kconfig | 6 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-104-dio-48e.c | 224 +++++++++------------------- drivers/gpio/gpio-104-idi-48.c | 123 +++++++--------- drivers/gpio/gpio-104-idio-16.c | 58 +++++--- drivers/gpio/gpio-gpio-mm.c | 177 +++++------------------ drivers/gpio/gpio-i8255.c | 249 ++++++++++++++++++++++++++++++++ drivers/gpio/gpio-ws16c48.c | 119 ++++++++++----- include/linux/gpio/i8255.h | 34 +++++ 10 files changed, 575 insertions(+), 422 deletions(-) create mode 100644 drivers/gpio/gpio-i8255.c create mode 100644 include/linux/gpio/i8255.h
base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56
Signature · 1 line
2.36.1
PATCH v2 1/6 gpio: i8255: Introduce the i8255 module
Exposes consumer functions providing support for Intel 8255 Programmable Peripheral Interface devices. A CONFIG_GPIO_I8255 Kconfig option is introduced; modules wanting access to these functions should select this Kconfig option.
--- MAINTAINERS | 6 + drivers/gpio/Kconfig | 3 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-i8255.c | 249 +++++++++++++++++++++++++++++++++++++ include/linux/gpio/i8255.h | 34 +++++ 5 files changed, 293 insertions(+) create mode 100644 drivers/gpio/gpio-i8255.c create mode 100644 include/linux/gpio/i8255.h
MAINTAINERS +6 -0
@@ -9799,6 +9799,12 @@ L: [email protected] S: Maintained F: drivers/video/fbdev/i810/ +INTEL 8255 GPIO DRIVER +M: William Breathitt Gray <[email protected]> +L: [email protected] +S: Maintained +F: drivers/gpio/gpio-i8255.c + INTEL ASoC DRIVERS M: Cezary Rojewski <[email protected]> M: Pierre-Louis Bossart <[email protected]>
drivers/gpio/Kconfig +3 -0
@@ -829,6 +829,9 @@ endmenu menu "Port-mapped I/O GPIO drivers" depends on X86 # Unconditional I/O space access +config GPIO_I8255 + tristate + config GPIO_104_DIO_48E tristate "ACCES 104-DIO-48E GPIO support" depends on PC104
drivers/gpio/Makefile +1 -0
@@ -67,6 +67,7 @@ obj-$(CONFIG_GPIO_GW_PLD) += gpio-gw-pld.o 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_I8255) += gpio-i8255.o obj-$(CONFIG_GPIO_ICH) += gpio-ich.o obj-$(CONFIG_GPIO_IDT3243X) += gpio-idt3243x.o obj-$(CONFIG_GPIO_IOP) += gpio-iop.o
drivers/gpio/gpio-i8255.c new file +249 -0
249 lines not on this page · raw message
include/linux/gpio/i8255.h new file +34 -0
@@ -0,0 +1,34 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* Copyright 2022 William Breathitt Gray */ +#ifndef _I8255_H_ +#define _I8255_H_ + +#include <linux/compiler_types.h> +#include <linux/types.h> + +/** +* struct i8255 - Intel 8255 register structure +* @port: Port A, B, and C +* @control: Control register +*/ +struct i8255 { +u8 port[3 ]; +u8 control; +}; + +void i8255_direction_input (struct i8255 __iomem *ppi,u8 *control_state, +unsigned long offset); +void i8255_direction_output (struct i8255 __iomem *ppi,u8 *control_state, +unsigned long offset,unsigned long value); +int i8255_get (const struct i8255 __iomem *ppi,unsigned long offset); +int i8255_get_direction (const u8 *control_state,unsigned long offset); +void i8255_get_multiple (const struct i8255 __iomem *ppi, +const unsigned long *mask,unsigned long *bits, +unsigned long ngpio); +void i8255_mode0_output (struct i8255 __iomem *const ppi); +void i8255_set (struct i8255 __iomem *ppi,unsigned long offset, +unsigned long value); +void i8255_set_multiple (struct i8255 __iomem *ppi,const unsigned long *mask, +const unsigned long *bits,unsigned long ngpio); + +#endif /* _I8255_H_ */
Signature · 1 line
2.36.1
Re: [PATCH v2 1/6] gpio: i8255: Introduce the i8255 module
Quoted 5 lines · William Breathitt Gray
Exposes consumer functions providing support for Intel 8255 Programmable Peripheral Interface devices. A
CONFIG_GPIO_I8255Kconfigoption is introduced; modules wanting access to these functions should select thisKconfigoption.
Spent much time with these chips in my teenage times :-)
Very good written library, see my comments below.
...
Quoted 1 line
+#include <
linux/compiler_types.h>
Should be simple types.h as you are using u8, etc.
drivers/gpio/gpio-i8255.c line 10
Quoted 4 lines
drivers/gpio/gpio-i8255.c new file +3
-0+#include <linux/err.h> +#include <linux/export.h> +#include <linux/gpio/i8255.h>
gpio/driver.h ?
And since it belongs to GPIO, I would group them and move after all other include/* to emphasize the relationship.
Also, why is it in the global header folder? Do you expect some drivers outside of drivers/gpio/? Maybe we can move after when the user comes?
Quoted 2 lines
drivers/gpio/gpio-i8255.c new file +2
-0+#include <linux/io.h> +#include <linux/module.h>
...
drivers/gpio/gpio-i8255.c line 16
Quoted 2 lines
drivers/gpio/gpio-i8255.c new file +2
-0+#define I8255_CONTROL_PORTCLOWER_DIRECTION BIT(0) +#define I8255_CONTROL_PORTCUPPER_DIRECTION BIT(3)
Missed underscore.
...
Quoted 4 lines
drivers/gpio/gpio-i8255.c new file +4
-0+static u8 i8255_direction_mask (const unsigned long offset) +{ +const unsigned long io_port = offset /8 ; +const unsigned long ppi_port = io_port %3 ;
drivers/gpio/gpio-i8255.c line 44
Quoted 10 lines
drivers/gpio/gpio-i8255.c new file +9
-0+ +switch (ppi_port) { +case I8255_PORTA : +return I8255_CONTROL_PORTA_DIRECTION ; +case I8255_PORTB : +return I8255_CONTROL_PORTB_DIRECTION ; +case I8255_PORTC : +/* Port C can be configured by nibble */ +if (offset %8 >3 )
I would move it to the local definition block close to offset/8. On some architectures this may give one assembly instruction for two variables.
Quoted 8 lines
drivers/gpio/gpio-i8255.c new file +7
-0+return I8255_CONTROL_PORTCUPPER_DIRECTION ; +return I8255_CONTROL_PORTCLOWER_DIRECTION ; +default : +/* Should never reach this path */ +return 0 ; + } +}
drivers/gpio/gpio-i8255.c line 88
Quoted 10 lines
drivers/gpio/gpio-i8255.c new file +10
-0+void i8255_direction_input (struct i8255 __iomem *const ppi, +u8 *const control_state,const unsigned long offset) +{ +const unsigned long io_port = offset /8 ; +const unsigned long group = io_port /3 ; + + control_state[group] |=I8255_CONTROL_MODE_SET ; + control_state[group] |=i8255_direction_mask (offset); + +iowrite8 (control_state[group], &ppi[group].control);
No I/O serialization? Can this be accessed during interrupt? (It may be that the code is correct, but please go through it and check with a question "can this register be accessed during IRQ and if yes, will the user get the correct / meaningful data?")
Review
Quoted 2 lines
(quoted diff) +2
-0+} +EXPORT_SYMBOL_GPL(i8255_direction_input);
Make it with a namespace. Ditto for the rest.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v2 1/6] gpio: i8255: Introduce the i8255 module
Quoted 10 lines · Andy Shevchenko
Quoted 5 lines · William Breathitt Gray
Exposes consumer functions providing support for Intel 8255 Programmable Peripheral Interface devices. A
CONFIG_GPIO_I8255Kconfigoption is introduced; modules wanting access to these functions should select thisKconfigoption.Spent much time with these chips in my teenage times :-)
Quoted 7 lines
Very good written library, see my comments below.
...
Quoted 1 line
+#include <
linux/compiler_types.h>Should be simple
types.has you are using u8, etc.
Ack.
Quoted 5 lines
drivers/gpio/gpio-i8255.c line 10
Quoted 4 lines
drivers/gpio/gpio-i8255.c new file +3
-0+#include <linux/err.h> +#include <linux/export.h> +#include <linux/gpio/i8255.h>
Quoted 8 lines
gpio/driver.h?And since it belongs to GPIO, I would group them and move after all other
include/*to emphasize the relationship.Also, why is it in the global header folder? Do you expect some drivers outside of
drivers/gpio/? Maybe we can move after when the user comes?
I think gpio/driver.h does make more sense for now since all the users of library are located under drivers/gpio/. I'll move the header code into gpio/driver.h then and adjust the includes accordingly.
Quoted 9 lines
Quoted 2 lines
drivers/gpio/gpio-i8255.c new file +2
-0+#include <linux/io.h> +#include <linux/module.h> ...
drivers/gpio/gpio-i8255.c line 16
Quoted 2 lines
drivers/gpio/gpio-i8255.c new file +2
-0+#define I8255_CONTROL_PORTCLOWER_DIRECTION BIT(0) +#define I8255_CONTROL_PORTCUPPER_DIRECTION BIT(3)Missed underscore.
Ack.
Quoted 15 lines
...
Quoted 12 lines
drivers/gpio/gpio-i8255.c new file +12
-0+static u8 i8255_direction_mask (const unsigned long offset) +{ +const unsigned long io_port = offset /8 ; +const unsigned long ppi_port = io_port %3 ; + +switch (ppi_port) { +case I8255_PORTA : +return I8255_CONTROL_PORTA_DIRECTION ; +case I8255_PORTB : +return I8255_CONTROL_PORTB_DIRECTION ; +case I8255_PORTC : +/* Port C can be configured by nibble */
Quoted 5 lines
Quoted 1 line
+ if (offset % 8 > 3)
I would move it to the local definition block close to offset/8. On some architectures this may give one assembly instruction for two variables.
Ack.
Quoted 8 lines
Quoted 7 lines
drivers/gpio/gpio-i8255.c new file +7
-0+return I8255_CONTROL_PORTCUPPER_DIRECTION ; +return I8255_CONTROL_PORTCLOWER_DIRECTION ; +default : +/* Should never reach this path */ +return 0 ; + } +}
Quoted 15 lines
drivers/gpio/gpio-i8255.c line 88
Quoted 10 lines
drivers/gpio/gpio-i8255.c new file +10
-0+void i8255_direction_input (struct i8255 __iomem *const ppi, +u8 *const control_state,const unsigned long offset) +{ +const unsigned long io_port = offset /8 ; +const unsigned long group = io_port /3 ; + + control_state[group] |=I8255_CONTROL_MODE_SET ; + control_state[group] |=i8255_direction_mask (offset); + +iowrite8 (control_state[group], &ppi[group].control);No I/O serialization? Can this be accessed during interrupt? (It may be that the code is correct, but please go through it and check with a question "can this register be accessed during IRQ and if yes, will the user get the correct / meaningful data?")
Writing to the 8255 control register for the device shouldn't be a problem, but we do have a race condition with the control_state[group] value. This value is accessed and modified in other functions (e.g. the i8255_direction_output() right below) so if an interrupt occurs the value could end up clobbered before it's written.
I'm not sure what the best approach would be here. In the subsequent patches I have the GPIO drivers take a lock before calling these i8255_* functions in order to synchronize access to those state arrays. Do you think it would be better to move the sychronization lock acquisition internally to the i8255_* functions here?
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+} +EXPORT_SYMBOL_GPL(i8255_direction_input);Make it with a namespace. Ditto for the rest.
Ack.
William Breathitt Gray
Quoted 3 lines
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v2 1/6] gpio: i8255: Introduce the i8255 module
In reply to William Breathitt GrayQuoted 9 lines · William Breathitt Gray
Exposes consumer functions providing support for Intel 8255 Programmable Peripheral Interface devices. A
CONFIG_GPIO_I8255Kconfigoption is introduced; modules wanting access to these functions should select thisKconfigoption.Tested-by Fred Eckert <[email protected]>Cc John Hentges <[email protected]>Cc Jay Dolan <[email protected]>Signed-off-by William Breathitt Gray <[email protected]>
This chip is like 50 years old, but so am I and I am not obsolete, it's about time that we implement a proper driver for it!
But I suppose you are not really using the actual discrete i8255 component? This is certainly used as integrated into some bridge or so? (Should be mentioned in the commit.)
drivers/gpio/Kconfig endmenu · line 833
Quoted 2 lines
drivers/gpio/Kconfig +2
-0+config GPIO_I8255 + tristate
That's a bit terse :D Explain that this is a Intel 8255 PPI chip first developed in the first half of the 1970ies.
Quoted 1 line
+++
b/include/linux/gpio/i8255.h
You need to provide a rationale for the separate .h file in the commit message even if it is clear
how it is used in the following patches.
Yours,
Linus Walleij
Re: [PATCH v2 1/6] gpio: i8255: Introduce the i8255 module
Quoted 13 lines · Linus Walleij
Quoted 9 lines · William Breathitt Gray
Exposes consumer functions providing support for Intel 8255 Programmable Peripheral Interface devices. A
CONFIG_GPIO_I8255Kconfigoption is introduced; modules wanting access to these functions should select thisKconfigoption.Tested-by Fred Eckert <[email protected]>Cc John Hentges <[email protected]>Cc Jay Dolan <[email protected]>Signed-off-by William Breathitt Gray <[email protected]>
Quoted 6 lines
This chip is like 50 years old, but so am I and I am not obsolete, it's about time that we implement a proper driver for it!
But I suppose you are not really using the actual discrete i8255 component? This is certainly used as integrated into some bridge or so? (Should be mentioned in the commit.)
Interestingly, there are some PC/104 devices out there that use actual i8255 components (e.g. Diamond Systems Onyx-MM with its 82C55 chips), but honestly the majority of devices I come across are simply emulating the i8255 interface in an FPGA or similar.
I'll adjust the commit to make it clearer that this is a library for i8255-compatible interfaces rather than support for any physical Intel 8255 chip in particular.
Quoted 5 lines
drivers/gpio/Kconfig endmenu · line 833
Quoted 2 lines
drivers/gpio/Kconfig +2
-0+config GPIO_I8255 + tristateThat's a bit terse :D Explain that this is a Intel 8255 PPI chip first developed in the first half of the 1970ies.
Ack.
Quoted 8 lines
Quoted 1 line
+++
b/include/linux/gpio/i8255.hYou need to provide a rationale for the separate .h file in the commit message even if it is clear
how it is used in the following patches.Yours,
Linus Walleij
I think I'll move this to gpio/driver.h as per Andy Shevchenko's suggestion. For now only a few drivers under drivers/gpio/ use this library, so it probably doesn't need to be separate just yet.
William Breathitt Gray
Re: [PATCH v2 1/6] gpio: i8255: Introduce the i8255 module
Quoted 45 lines · William Breathitt Gray
Quoted 13 lines · Linus Walleij
Quoted 9 lines · William Breathitt Gray
Exposes consumer functions providing support for Intel 8255 Programmable Peripheral Interface devices. A
CONFIG_GPIO_I8255Kconfigoption is introduced; modules wanting access to these functions should select thisKconfigoption.Tested-by Fred Eckert <[email protected]>Cc John Hentges <[email protected]>Cc Jay Dolan <[email protected]>Signed-off-by William Breathitt Gray <[email protected]>Quoted 6 lines
This chip is like 50 years old, but so am I and I am not obsolete, it's about time that we implement a proper driver for it!
But I suppose you are not really using the actual discrete i8255 component? This is certainly used as integrated into some bridge or so? (Should be mentioned in the commit.)
Interestingly, there are some PC/104 devices out there that use actual i8255 components (e.g. Diamond Systems Onyx-MM with its 82C55 chips), but honestly the majority of devices I come across are simply emulating the i8255 interface in an FPGA or similar.
I'll adjust the commit to make it clearer that this is a library for i8255-compatible interfaces rather than support for any physical Intel 8255 chip in particular.
Quoted 5 lines
drivers/gpio/Kconfig endmenu · line 833
Quoted 2 lines
drivers/gpio/Kconfig +2
-0+config GPIO_I8255 + tristateThat's a bit terse :D Explain that this is a Intel 8255 PPI chip first developed in the first half of the 1970ies.
Ack.
Quoted 6 lines
Quoted 1 line
+++
b/include/linux/gpio/i8255.hYou need to provide a rationale for the separate .h file in the commit message even if it is clear
how it is used in the following patches.
Quoted 4 lines
Quoted 2 lines
Yours,
Linus WalleijI think I'll move this to
gpio/driver.has per Andy Shevchenko's
I don't think this is what Andy meant. I think he suggested moving this header into drivers/gpio/ because it doesn't make sense for it to be publicly accessible for anyone else than the GPIO drivers.
Andy: correct me if I'm wrong.
Bart
Quoted 3 lines
suggestion. For now only a few drivers under
drivers/gpio/use this library, so it probably doesn't need to be separate just yet.
Re: [PATCH v2 1/6] gpio: i8255: Introduce the i8255 module
Quoted 3 lines · Bartosz Golaszewski
Quoted 1 line · William Breathitt Gray
On Mon, Jul 11, 2022 at 03:02:10PM +0200, Linus Walleij wrote:
...
Quoted 7 lines
Quoted 1 line
I think I'll move this to
gpio/driver.has per Andy Shevchenko'sI don't think this is what Andy meant. I think he suggested moving this header into
drivers/gpio/because it doesn't make sense for it to be publicly accessible for anyone else than the GPIO drivers.Andy: correct me if I'm wrong.
No, you are right. I was talking about localizing the header to drivers/gpio.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v2 1/6] gpio: i8255: Introduce the i8255 module
Quoted 9 lines · Andy Shevchenko
Quoted 3 lines · Bartosz Golaszewski
Quoted 1 line · William Breathitt Gray
On Mon, Jul 11, 2022 at 03:02:10PM +0200, Linus Walleij wrote:
...
Quoted 2 lines
Quoted 1 line
I think I'll move this to
gpio/driver.has per Andy Shevchenko's
Quoted 11 lines
Quoted 5 lines
I don't think this is what Andy meant. I think he suggested moving this header into
drivers/gpio/because it doesn't make sense for it to be publicly accessible for anyone else than the GPIO drivers.Andy: correct me if I'm wrong.
No, you are right. I was talking about localizing the header to
drivers/gpio.Signature · 2 lines
With Best Regards, Andy Shevchenko
Sure, I can move it to drivers/gpio/i8255.h to keep it local to the GPIO drivers. It'll be trivia to move this out if the need ever arrives in the future so I have no problem with that.
William Breathitt Gray
Re: [PATCH v2 1/6] gpio: i8255: Introduce the i8255 module
Quoted 5 lines · William Breathitt Gray
Quoted 4 lines · Andy Shevchenko
Quoted 3 lines · Bartosz Golaszewski
Quoted 1 line · William Breathitt Gray
On Mon, Jul 11, 2022 at 03:02:10PM +0200, Linus Walleij wrote:
...
Quoted 11 lines
Quoted 9 lines
Quoted 7 lines
Quoted 1 line
I think I'll move this to
gpio/driver.has per Andy Shevchenko'sI don't think this is what Andy meant. I think he suggested moving this header into
drivers/gpio/because it doesn't make sense for it to be publicly accessible for anyone else than the GPIO drivers.Andy: correct me if I'm wrong.
No, you are right. I was talking about localizing the header to
drivers/gpio.Sure, I can move it to
drivers/gpio/i8255.hto keep it local to the GPIO
Please, keep it named in the same pattern as the C-file, but having .h extension. I believe it's gpio-i8255.h.
Quoted 2 lines
drivers. It'll be trivia to move this out if the need ever arrives in the future so I have no problem with that.
Thanks!
Signature · 2 lines
With Best Regards, Andy Shevchenko
PATCH v2 2/6 gpio: 104-dio-48e: Implement and utilize register structures
In reply to William Breathitt GrayReduce magic numbers and improve code readability by implementing and utilizing named register data structures. The 104-DIO-48E device features an Intel 8255 compatible GPIO interface, so the i8255 GPIO module is selected and utilized as well.
--- drivers/gpio/Kconfig | 1 + drivers/gpio/gpio-104-dio-48e.c | 224 ++++++++++---------------------- 2 files changed, 69 insertions(+), 156 deletions(-)
drivers/gpio/Kconfig +1 -0
@@ -837,6 +837,7 @@ config GPIO_104_DIO_48E depends on PC104 select ISA_BUS_API select GPIOLIB_IRQCHIP + select GPIO_I8255 help Enables GPIO support for the ACCES 104-DIO-48E series (104-DIO-48E, 104-DIO-24E). The base port addresses for the devices may be
drivers/gpio/gpio-104-dio-48e.c +68 -156
353 lines not on this page · raw message
Signature · 1 line
2.36.1
PATCH v2 3/6 gpio: 104-idi-48: Implement and utilize register structures
In reply to William Breathitt GrayReduce magic numbers and improve code readability by implementing and utilizing named register data structures. The 104-IDI-48 device features an Intel 8255 compatible GPIO interface, so the i8255 GPIO module is selected and utilized as well.
--- drivers/gpio/Kconfig | 1 + drivers/gpio/gpio-104-idi-48.c | 123 +++++++++++++-------------------- 2 files changed, 50 insertions(+), 74 deletions(-)
drivers/gpio/Kconfig +1 -0
@@ -861,6 +861,7 @@ config GPIO_104_IDI_48 depends on PC104 select ISA_BUS_API select GPIOLIB_IRQCHIP + select GPIO_I8255 help Enables GPIO support for the ACCES 104-IDI-48 family (104-IDI-48A, 104-IDI-48AC, 104-IDI-48B, 104-IDI-48BC). The base port addresses for
drivers/gpio/gpio-104-idi-48.c +49 -74
213 lines not on this page · raw message
Signature · 1 line
2.36.1
PATCH v2 4/6 gpio: gpio-mm: Implement and utilize register structures
In reply to William Breathitt GrayReduce magic numbers and improve code readability by implementing and utilizing named register data structures. The GPIO-MM device features an Intel 8255 compatible GPIO interface, so the i8255 GPIO module is selected and utilized as well.
--- drivers/gpio/Kconfig | 1 + drivers/gpio/gpio-gpio-mm.c | 177 ++++++++---------------------------- 2 files changed, 38 insertions(+), 140 deletions(-)
drivers/gpio/Kconfig +1 -0
@@ -882,6 +882,7 @@ config GPIO_GPIO_MM tristate "Diamond Systems GPIO-MM GPIO support" depends on PC104 select ISA_BUS_API + select GPIO_I8255 help Enables GPIO support for the Diamond Systems GPIO-MM and GPIO-MM-12.
drivers/gpio/gpio-gpio-mm.c +37 -140
282 lines not on this page · raw message
Signature · 1 line
2.36.1
PATCH v2 5/6 gpio: 104-idio-16: Implement and utilize register structures
In reply to William Breathitt GrayReduce magic numbers and improve code readability by implementing and utilizing named register data structures.
--- drivers/gpio/gpio-104-idio-16.c | 58 +++++++++++++++++++++++---------- 1 file changed, 41 insertions(+), 17 deletions(-)
drivers/gpio/gpio-104-idio-16.c +41 -17
@@ -19,6 +19,7 @@#include <linux/module.h> #include <linux/moduleparam.h> #include <linux/spinlock.h> +#include <linux/types.h> #define IDIO_16_EXTENT 8#define MAX_NUM_IDIO_16 max_num_isa_dev(IDIO_16_EXTENT)
@@ -32,19 +33,42 @@ static unsigned int irq[MAX_NUM_IDIO_16];module_param_hw_array (irq, uint, irq,NULL ,0 );MODULE_PARM_DESC (irq,"ACCES 104-IDIO-16 interrupt line numbers" ); +/** +* struct idio_16_reg - device registers structure +* @out0_7: Read: N/A +* Write: FET Drive Outputs 0-7 +* @in0_7: Read: Isolated Inputs 0-7 +* Write: Clear Interrupt +* @irq_ctl: Read: Enable IRQ +* Write: Disable IRQ +* @unused: N/A +* @out8_15: Read: N/A +* Write: FET Drive Outputs 8-15 +* @in8_15: Read: Isolated Inputs 8-15 +* Write: N/A +*/ +struct idio_16_reg { +u8 out0_7; +u8 in0_7; +u8 irq_ctl; +u8 unused; +u8 out8_15; +u8 in8_15; +}; +/** * struct idio_16_gpio - GPIO device private data structure * @chip: instance of the gpio_chip * @lock: synchronization lock to prevent I/O race conditions * @irq_mask: I/O bits affected by interrupts -* @base: base port address of the GPIO device +* @reg: I/O address offset for the device registers * @out_state: output bits state */ struct idio_16_gpio {struct gpio_chip chip;raw_spinlock_t lock;unsigned long irq_mask; -void __iomem *base; +struct idio_16_reg __iomem *reg;unsigned int out_state; };
@@ -79,9 +103,9 @@ static int idio_16_gpio_get(struct gpio_chip *chip, unsigned int offset)return -EINVAL ;if (offset <24 ) -return !!(ioread8 (idio16gpio->base +1 ) & mask); +return !!(ioread8 (&idio16gpio->reg->in0_7) & mask); -return !!(ioread8 (idio16gpio->base +5 ) & (mask>>8 )); +return !!(ioread8 (&idio16gpio->reg->in8_15) & (mask>>8 )); } static int idio_16_gpio_get_multiple(struct gpio_chip *chip,
@@ -91,9 +115,9 @@ static int idio_16_gpio_get_multiple(struct gpio_chip *chip, *bits =0 ;if (*mask &GENMASK (23 ,16 )) - *bits |= (unsigned long )ioread8 (idio16gpio->base +1 ) <<16 ; + *bits |= (unsigned long )ioread8 (&idio16gpio->reg->in0_7) <<16 ;if (*mask &GENMASK (31 ,24 )) - *bits |= (unsigned long )ioread8 (idio16gpio->base +5 ) <<24 ; + *bits |= (unsigned long )ioread8 (&idio16gpio->reg->in8_15) <<24 ;return 0 ; }
@@ -116,9 +140,9 @@ static void idio_16_gpio_set(struct gpio_chip *chip, unsigned int offset, idio16gpio->out_state &= ~mask;if (offset >7 ) -iowrite8 (idio16gpio->out_state >>8 , idio16gpio->base +4 ); +iowrite8 (idio16gpio->out_state >>8 , &idio16gpio->reg->out8_15);else -iowrite8 (idio16gpio->out_state, idio16gpio->base); +iowrite8 (idio16gpio->out_state, &idio16gpio->reg->out0_7);raw_spin_unlock_irqrestore (&idio16gpio->lock, flags); }
@@ -135,9 +159,9 @@ static void idio_16_gpio_set_multiple(struct gpio_chip *chip, idio16gpio->out_state |= *mask & *bits;if (*mask &0xFF ) -iowrite8 (idio16gpio->out_state, idio16gpio->base); +iowrite8 (idio16gpio->out_state, &idio16gpio->reg->out0_7);if ((*mask >>8 ) &0xFF ) -iowrite8 (idio16gpio->out_state >>8 , idio16gpio->base +4 ); +iowrite8 (idio16gpio->out_state >>8 , &idio16gpio->reg->out8_15);raw_spin_unlock_irqrestore (&idio16gpio->lock, flags); }
@@ -158,7 +182,7 @@ static void idio_16_irq_mask(struct irq_data *data)if (!idio16gpio->irq_mask) {raw_spin_lock_irqsave (&idio16gpio->lock, flags); -iowrite8 (0 , idio16gpio->base +2 ); +iowrite8 (0 , &idio16gpio->reg->irq_ctl);raw_spin_unlock_irqrestore (&idio16gpio->lock, flags); }
@@ -177,7 +201,7 @@ static void idio_16_irq_unmask(struct irq_data *data)if (!prev_irq_mask) {raw_spin_lock_irqsave (&idio16gpio->lock, flags); -ioread8 (idio16gpio->base +2 ); +ioread8 (&idio16gpio->reg->irq_ctl);raw_spin_unlock_irqrestore (&idio16gpio->lock, flags); }
@@ -212,7 +236,7 @@ static irqreturn_t idio_16_irq_handler(int irq, void *dev_id)raw_spin_lock (&idio16gpio->lock); -iowrite8 (0 , idio16gpio->base +1 ); +iowrite8 (0 , &idio16gpio->reg->in0_7);raw_spin_unlock (&idio16gpio->lock);
@@ -232,8 +256,8 @@ static int idio_16_irq_init_hw(struct gpio_chip *gc)struct idio_16_gpio *const idio16gpio =gpiochip_get_data (gc);/* Disable IRQ by default */ -iowrite8 (0 , idio16gpio->base +2 ); -iowrite8 (0 , idio16gpio->base +1 ); +iowrite8 (0 , &idio16gpio->reg->irq_ctl); +iowrite8 (0 , &idio16gpio->reg->in0_7);return 0 ; }
@@ -255,8 +279,8 @@ static int idio_16_probe(struct device *dev, unsigned int id)return -EBUSY ; } - idio16gpio->base =devm_ioport_map (dev, base[id],IDIO_16_EXTENT ); -if (!idio16gpio->base) + idio16gpio->reg =devm_ioport_map (dev, base[id],IDIO_16_EXTENT ); +if (!idio16gpio->reg)return -ENOMEM ; idio16gpio->chip.label = name;
Signature · 1 line
2.36.1
PATCH v2 6/6 gpio: ws16c48: Implement and utilize register structures
In reply to William Breathitt GrayReduce magic numbers and improve code readability by implementing and utilizing named register data structures.
--- drivers/gpio/gpio-ws16c48.c | 119 +++++++++++++++++++++++++----------- 1 file changed, 84 insertions(+), 35 deletions(-)
drivers/gpio/gpio-ws16c48.c +84 -35
@@ -17,8 +17,9 @@#include <linux/module.h> #include <linux/moduleparam.h> #include <linux/spinlock.h> +#include <linux/types.h> -#define WS16C48_EXTENT 16 +#define WS16C48_EXTENT 10#define MAX_NUM_WS16C48 max_num_isa_dev(WS16C48_EXTENT)static unsigned int base[MAX_NUM_WS16C48 ];
@@ -30,6 +31,20 @@ static unsigned int irq[MAX_NUM_WS16C48];module_param_hw_array (irq, uint, irq,NULL ,0 );MODULE_PARM_DESC (irq,"WinSystems WS16C48 interrupt line numbers" ); +/** +* struct ws16c48_reg - device register structure +* @port: Port 0 through 5 I/O +* @int_pending: Interrupt Pending +* @page_lock: Register page (Bits 7-6) and I/O port lock (Bits 5-0) +* @pol_enab_int_id: Interrupt polarity, enable, and ID +*/ +struct ws16c48_reg { +u8 port[6 ]; +u8 int_pending; +u8 page_lock; +u8 pol_enab_int_id[3 ]; +}; +/** * struct ws16c48_gpio - GPIO device private data structure * @chip: instance of the gpio_chip
@@ -38,7 +53,7 @@ MODULE_PARM_DESC(irq, "WinSystems WS16C48 interrupt line numbers");* @lock: synchronization lock to prevent I/O race conditions * @irq_mask: I/O bits affected by interrupts * @flow_mask: IRQ flow type mask for the respective I/O bits -* @base: base port address of the GPIO device +* @reg: I/O address offset for the device registers */ struct ws16c48_gpio {struct gpio_chip chip;
@@ -47,7 +62,7 @@ struct ws16c48_gpio {raw_spinlock_t lock;unsigned long irq_mask;unsigned long flow_mask; -void __iomem *base; +struct ws16c48_reg __iomem *reg; };static int ws16c48_gpio_get_direction (struct gpio_chip *chip,unsigned offset)
@@ -73,7 +88,7 @@ static int ws16c48_gpio_direction_input(struct gpio_chip *chip, unsigned offset) ws16c48gpio->io_state[port] |= mask; ws16c48gpio->out_state[port] &= ~mask; -iowrite8 (ws16c48gpio->out_state[port], ws16c48gpio->base + port); +iowrite8 (ws16c48gpio->out_state[port], ws16c48gpio->reg->port + port);raw_spin_unlock_irqrestore (&ws16c48gpio->lock, flags);
@@ -95,7 +110,7 @@ static int ws16c48_gpio_direction_output(struct gpio_chip *chip, ws16c48gpio->out_state[port] |= mask; else ws16c48gpio->out_state[port] &= ~mask; -iowrite8 (ws16c48gpio->out_state[port], ws16c48gpio->base + port); +iowrite8 (ws16c48gpio->out_state[port], ws16c48gpio->reg->port + port);raw_spin_unlock_irqrestore (&ws16c48gpio->lock, flags);
@@ -118,7 +133,7 @@ static int ws16c48_gpio_get(struct gpio_chip *chip, unsigned offset)return -EINVAL ; } - port_state =ioread8 (ws16c48gpio->base + port); + port_state =ioread8 (ws16c48gpio->reg->port + port);raw_spin_unlock_irqrestore (&ws16c48gpio->lock, flags);
@@ -131,14 +146,16 @@ static int ws16c48_gpio_get_multiple(struct gpio_chip *chip,struct ws16c48_gpio *const ws16c48gpio =gpiochip_get_data (chip);unsigned long offset;unsigned long gpio_mask; -void __iomem *port_addr; +size_t index; +u8 __iomem *port_addr;unsigned long port_state;/* clear bits array to a clean slate */ bitmap_zero (bits, chip->ngpio);for_each_set_clump8 (offset, gpio_mask, mask, chip->ngpio) { - port_addr = ws16c48gpio->base + offset /8 ; + index = offset /8 ; + port_addr = ws16c48gpio->reg->port + index; port_state =ioread8 (port_addr) & gpio_mask;bitmap_set_value8 (bits, port_state, offset);
@@ -166,7 +183,7 @@ static void ws16c48_gpio_set(struct gpio_chip *chip, unsigned offset, int value) ws16c48gpio->out_state[port] |= mask; else ws16c48gpio->out_state[port] &= ~mask; -iowrite8 (ws16c48gpio->out_state[port], ws16c48gpio->base + port); +iowrite8 (ws16c48gpio->out_state[port], ws16c48gpio->reg->port + port);raw_spin_unlock_irqrestore (&ws16c48gpio->lock, flags); }
@@ -178,13 +195,13 @@ static void ws16c48_gpio_set_multiple(struct gpio_chip *chip,unsigned long offset;unsigned long gpio_mask;size_t index; -void __iomem *port_addr; +u8 __iomem *port_addr;unsigned long bitmask;unsigned long flags;for_each_set_clump8 (offset, gpio_mask, mask, chip->ngpio) { index = offset /8 ; - port_addr = ws16c48gpio->base + index; + port_addr = ws16c48gpio->reg->port + index;/* mask out GPIO configured for input */ gpio_mask &= ~ws16c48gpio->io_state[index];
@@ -219,10 +236,15 @@ static void ws16c48_irq_ack(struct irq_data *data) port_state = ws16c48gpio->irq_mask >> (8 *port); -iowrite8 (0x80 , ws16c48gpio->base +7 ); -iowrite8 (port_state & ~mask, ws16c48gpio->base +8 + port); -iowrite8 (port_state | mask, ws16c48gpio->base +8 + port); -iowrite8 (0xC0 , ws16c48gpio->base +7 ); +/* Select Register Page 2; Unlock all I/O ports */ +iowrite8 (0x80 , &ws16c48gpio->reg->page_lock); + +/* Clear pending interrupt */ +iowrite8 (port_state & ~mask, ws16c48gpio->reg->pol_enab_int_id + port); +iowrite8 (port_state | mask, ws16c48gpio->reg->pol_enab_int_id + port); + +/* Select Register Page 3; Unlock all I/O ports */ +iowrite8 (0xC0 , &ws16c48gpio->reg->page_lock);raw_spin_unlock_irqrestore (&ws16c48gpio->lock, flags); }
@@ -235,6 +257,7 @@ static void ws16c48_irq_mask(struct irq_data *data)const unsigned long mask =BIT (offset);const unsigned port = offset /8 ;unsigned long flags; +unsigned long port_state;/* only the first 3 ports support interrupts */ if (port >2 )
@@ -243,10 +266,16 @@ static void ws16c48_irq_mask(struct irq_data *data)raw_spin_lock_irqsave (&ws16c48gpio->lock, flags); ws16c48gpio->irq_mask &= ~mask; + port_state = ws16c48gpio->irq_mask >> (8 * port); + +/* Select Register Page 2; Unlock all I/O ports */ +iowrite8 (0x80 , &ws16c48gpio->reg->page_lock); -iowrite8 (0x80 , ws16c48gpio->base +7 ); -iowrite8 (ws16c48gpio->irq_mask >> (8 *port), ws16c48gpio->base +8 + port); -iowrite8 (0xC0 , ws16c48gpio->base +7 ); +/* Disable interrupt */ +iowrite8 (port_state, ws16c48gpio->reg->pol_enab_int_id + port); + +/* Select Register Page 3; Unlock all I/O ports */ +iowrite8 (0xC0 , &ws16c48gpio->reg->page_lock);raw_spin_unlock_irqrestore (&ws16c48gpio->lock, flags); }
@@ -259,6 +288,7 @@ static void ws16c48_irq_unmask(struct irq_data *data)const unsigned long mask =BIT (offset);const unsigned port = offset /8 ;unsigned long flags; +unsigned long port_state;/* only the first 3 ports support interrupts */ if (port >2 )
@@ -267,10 +297,16 @@ static void ws16c48_irq_unmask(struct irq_data *data)raw_spin_lock_irqsave (&ws16c48gpio->lock, flags); ws16c48gpio->irq_mask |= mask; + port_state = ws16c48gpio->irq_mask >> (8 * port); + +/* Select Register Page 2; Unlock all I/O ports */ +iowrite8 (0x80 , &ws16c48gpio->reg->page_lock); -iowrite8 (0x80 , ws16c48gpio->base +7 ); -iowrite8 (ws16c48gpio->irq_mask >> (8 *port), ws16c48gpio->base +8 + port); -iowrite8 (0xC0 , ws16c48gpio->base +7 ); +/* Enable interrupt */ +iowrite8 (port_state, ws16c48gpio->reg->pol_enab_int_id + port); + +/* Select Register Page 3; Unlock all I/O ports */ +iowrite8 (0xC0 , &ws16c48gpio->reg->page_lock);raw_spin_unlock_irqrestore (&ws16c48gpio->lock, flags); }
@@ -283,6 +319,7 @@ static int ws16c48_irq_set_type(struct irq_data *data, unsigned flow_type)const unsigned long mask =BIT (offset);const unsigned port = offset /8 ;unsigned long flags; +unsigned long port_state;/* only the first 3 ports support interrupts */ if (port >2 )
@@ -304,9 +341,16 @@ static int ws16c48_irq_set_type(struct irq_data *data, unsigned flow_type)return -EINVAL ; } -iowrite8 (0x40 , ws16c48gpio->base +7 ); -iowrite8 (ws16c48gpio->flow_mask >> (8 *port), ws16c48gpio->base +8 + port); -iowrite8 (0xC0 , ws16c48gpio->base +7 ); + port_state = ws16c48gpio->flow_mask >> (8 * port); + +/* Select Register Page 1; Unlock all I/O ports */ +iowrite8 (0x40 , &ws16c48gpio->reg->page_lock); + +/* Set interrupt polarity */ +iowrite8 (port_state, ws16c48gpio->reg->pol_enab_int_id + port); + +/* Select Register Page 3; Unlock all I/O ports */ +iowrite8 (0xC0 , &ws16c48gpio->reg->page_lock);raw_spin_unlock_irqrestore (&ws16c48gpio->lock, flags);
@@ -325,25 +369,26 @@ static irqreturn_t ws16c48_irq_handler(int irq, void *dev_id) {struct ws16c48_gpio *const ws16c48gpio = dev_id;struct gpio_chip *const chip = &ws16c48gpio->chip; +struct ws16c48_reg __iomem *const reg = ws16c48gpio->reg;unsigned long int_pending;unsigned long port;unsigned long int_id;unsigned long gpio; - int_pending =ioread8 (ws16c48gpio->base +6 ) &0x7 ; + int_pending =ioread8 (®->int_pending) &0x7 ;if (!int_pending)return IRQ_NONE ;/* loop until all pending interrupts are handled */ do {for_each_set_bit (port, &int_pending,3 ) { - int_id =ioread8 (ws16c48gpio->base +8 + port); + int_id =ioread8 (reg->pol_enab_int_id + port);for_each_set_bit (gpio, &int_id,8 )generic_handle_domain_irq (chip->irq.domain, gpio +8 *port); } - int_pending =ioread8 (ws16c48gpio->base +6 ) &0x7 ; + int_pending =ioread8 (®->int_pending) &0x7 ; }while (int_pending);return IRQ_HANDLED ;
@@ -369,12 +414,16 @@ static int ws16c48_irq_init_hw(struct gpio_chip *gc) {struct ws16c48_gpio *const ws16c48gpio =gpiochip_get_data (gc); -/* Disable IRQ by default */ -iowrite8 (0x80 , ws16c48gpio->base +7 ); -iowrite8 (0 , ws16c48gpio->base +8 ); -iowrite8 (0 , ws16c48gpio->base +9 ); -iowrite8 (0 , ws16c48gpio->base +10 ); -iowrite8 (0xC0 , ws16c48gpio->base +7 ); +/* Select Register Page 2; Unlock all I/O ports */ +iowrite8 (0x80 , &ws16c48gpio->reg->page_lock); + +/* Disable interrupts for all lines */ +iowrite8 (0 , &ws16c48gpio->reg->pol_enab_int_id[0 ]); +iowrite8 (0 , &ws16c48gpio->reg->pol_enab_int_id[1 ]); +iowrite8 (0 , &ws16c48gpio->reg->pol_enab_int_id[2 ]); + +/* Select Register Page 3; Unlock all I/O ports */ +iowrite8 (0xC0 , &ws16c48gpio->reg->page_lock);return 0 ; }
@@ -396,8 +445,8 @@ static int ws16c48_probe(struct device *dev, unsigned int id)return -EBUSY ; } - ws16c48gpio->base =devm_ioport_map (dev, base[id],WS16C48_EXTENT ); -if (!ws16c48gpio->base) + ws16c48gpio->reg =devm_ioport_map (dev, base[id],WS16C48_EXTENT ); +if (!ws16c48gpio->reg)return -ENOMEM ; ws16c48gpio->chip.label = name;
Signature · 1 line
2.36.1