Messages in this thread | | | From | Andy Shevchenko <> | Date | Mon, 18 Jul 2022 23:31:44 +0200 | Subject | Re: [PATCH v3 3/6] gpio: i8255: Introduce the Intel 8255 interface library module |
| |
On Mon, Jul 18, 2022 at 10:56 PM William Breathitt Gray <william.gray@linaro.org> wrote: > > Exposes consumer library functions providing support for interfaces > compatible with the venerable Intel 8255 Programmable Peripheral > Interface (PPI). > > The Intel 8255 PPI first appeared in the early 1970s, initially for the > Intel 8080 and later appearing in the original IBM-PC. The popularity of > the original Intel 8255 chip led to many subsequent variants and clones > of the interface in various chips and integrated circuits. Although > still popular, interfaces compatible with the Intel 8255 PPI are > nowdays typically found embedded in larger VLSI processing chips and > FPGA components rather than as discrete ICs. > > A CONFIG_GPIO_I8255 Kconfig option is introduced by this patch. Modules > wanting access to these i8255 library functions should select this > Kconfig option, and import the I8255 symbol namespace.
Thanks for an update, my comments below.
...
> +config GPIO_I8255 > + tristate > + help > + Enables support for the i8255 interface library functions. The i8255 > + interface library provides functions to facilitate communication with > + interfaces compatible with the venerable Intel 8255 Programmable > + Peripheral Interface (PPI). The Intel 8255 PPI chip was first released > + in the early 1970s but compatible interfaces are nowadays typically > + found embedded in larger VLSI processing chips and FPGA components.
+ "If built as a module its name will be ..." or similar sentence would be good to add.
...
> + case I8255_PORTC: > + /* Port C can be configured by nibble */ > + if (port_offset > 3)
More naturally looks >= 4 to show the beginning offset number for the UPPER.
> + return I8255_CONTROL_PORTC_UPPER_DIRECTION; > + return I8255_CONTROL_PORTC_LOWER_DIRECTION;
...
> + out_state = ioread8(&ppi[group].port[ppi_port]); > + out_state &= ~io_mask; > + out_state |= bit_mask;
Usual pattern is
out_state = (out_state & ~mask) | (bits & mask);
(and we call them mask and value or bits)
> +
No need for this blank line.
> + iowrite8(out_state, &ppi[group].port[ppi_port]);
...
> + bit_mask = bitmap_get_value8(bits, offset) & gpio_mask; > + io_port = offset / 8;
Exactly why I recommended reconsidering the above pattern, you won't need to do ' & mask' in the caller(s).
> + i8255_set_port(ppi, state, io_port, gpio_mask, bit_mask);
...
> + * Initializes the @state of each Intel 8255 Programmable Peripheral Interface > + * group for use in i8255 library functions.
I'm not sure about terminology. What's 'group'? We have a very well established term 'bank' isn't it what you meant here by 'group'?
...
> +int i8255_get(const struct i8255 __iomem *ppi, unsigned long offset);
I'm not sure what const with __iomem gives us? The purpose of that is?. And if it's about the content of the register, then const is a lie.
Ditto for the rest of the similar cases.
-- With Best Regards, Andy Shevchenko
| |