| Message ID | cover.1657216200.git.william.gray@linaro.org |
|---|---|
| Headers | show |
| Series | gpio: Implement and utilize register structures for ISA drivers | expand |
On Fri, Jul 8, 2022 at 1:16 AM William Breathitt Gray <william.gray@linaro.org> wrote: > > 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. Spent much time with these chips in my teenage times :-) Very good written library, see my comments below. ... > +#include <linux/compiler_types.h> Should be simple types.h as you are using u8, etc. > +#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? > +#include <linux/io.h> > +#include <linux/module.h> ... > +#define I8255_CONTROL_PORTCLOWER_DIRECTION BIT(0) > +#define I8255_CONTROL_PORTCUPPER_DIRECTION BIT(3) Missed underscore. ... > +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 */ > + 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. > + return I8255_CONTROL_PORTCUPPER_DIRECTION; > + return I8255_CONTROL_PORTCLOWER_DIRECTION; > + default: > + /* Should never reach this path */ > + return 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?") > +} > +EXPORT_SYMBOL_GPL(i8255_direction_input); Make it with a namespace. Ditto for the rest.
On Fri, Jul 08, 2022 at 04:40:01PM +0200, Andy Shevchenko wrote: > On Fri, Jul 8, 2022 at 1:16 AM William Breathitt Gray > <william.gray@linaro.org> wrote: > > > > 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. > > Spent much time with these chips in my teenage times :-) > > Very good written library, see my comments below. > > ... > > > +#include <linux/compiler_types.h> > > Should be simple types.h as you are using u8, etc. Ack. > > +#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? 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. > > +#include <linux/io.h> > > +#include <linux/module.h> > > ... > > > +#define I8255_CONTROL_PORTCLOWER_DIRECTION BIT(0) > > +#define I8255_CONTROL_PORTCUPPER_DIRECTION BIT(3) > > Missed underscore. Ack. > ... > > > +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 */ > > > + 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. > > + return I8255_CONTROL_PORTCUPPER_DIRECTION; > > + return I8255_CONTROL_PORTCLOWER_DIRECTION; > > + default: > > + /* Should never reach this path */ > > + return 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? > > +} > > +EXPORT_SYMBOL_GPL(i8255_direction_input); > > Make it with a namespace. Ditto for the rest. Ack. William Breathitt Gray > -- > With Best Regards, > Andy Shevchenko