lkml.org 
[lkml]   [2022]   [Jul]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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

\
 
 \ /
  Last update: 2022-07-18 23:33    [W:1.955 / U:0.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site