Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <cover.1657907849.git.william.gray@linaro.org> <e0f9ff79a4229cf0ccce4844745453dae633a1aa.1657907849.git.william.gray@linaro.org>
In-Reply-To: <e0f9ff79a4229cf0ccce4844745453dae633a1aa.1657907849.git.william.gray@linaro.org>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Mon, 18 Jul 2022 23:31:44 +0200
Message-ID: <CAHp75VeQkpr9Ww-C+1w13BiLSuCY=FMMtjjAdVCq0a5z1bkSVA@mail.gmail.com>
Subject: Re: [PATCH v3 3/6] gpio: i8255: Introduce the Intel 8255 interface
 library module
To:     William Breathitt Gray <william.gray@linaro.org>
Cc:     Linus Walleij <linus.walleij@linaro.org>,
        Bartosz Golaszewski <brgl@bgdev.pl>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Fred Eckert <Frede@cmslaser.com>,
        John Hentges <jhentges@accesio.com>,
        Jay Dolan <jay.dolan@accesio.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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