| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Y35bgFmiMW3uTm/O@smile.fi.intel.com>
Date: Wed, 23 Nov 2022 19:42:24 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: William Breathitt Gray <william.gray@...aro.org>
Cc: linus.walleij@...aro.org, brgl@...ev.pl,
linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
michael@...le.cc, broonie@...nel.org
Subject: Re: [PATCH v3 6/9] gpio: i8255: Migrate to gpio-regmap API
On Tue, Nov 22, 2022 at 02:11:03AM -0500, William Breathitt Gray wrote:
> The regmap API supports IO port accessors so we can take advantage of
> regmap abstractions rather than handling access to the device registers
> directly in the driver.
>
> By leveraging the gpio-regmap API, the i8255 library is reduced to
> simply a devm_i8255_regmap_register() function, a configuration
> structure struct i8255_regmap_config, and a helper macro
> i8255_volatile_regmap_range() provided to simplify volatile PPI register
> hinting for the regmap.
>
> Legacy functions and code will be removed once all consumers have
> migrated to the new i8255 library interface.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
(See one remark below and once comment to address)
> Suggested-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> Signed-off-by: William Breathitt Gray <william.gray@...aro.org>
> ---
> drivers/gpio/Kconfig | 1 +
> drivers/gpio/gpio-i8255.c | 119 ++++++++++++++++++++++++++++++++++----
> drivers/gpio/gpio-i8255.h | 27 +++++++++
> 3 files changed, 135 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index dd34039fc31b..88dfdc62992f 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -831,6 +831,7 @@ menu "Port-mapped I/O GPIO drivers"
>
> config GPIO_I8255
> tristate
> + select GPIO_REGMAP
> help
> Enables support for the i8255 interface library functions. The i8255
> interface library provides functions to facilitate communication with
> diff --git a/drivers/gpio/gpio-i8255.c b/drivers/gpio/gpio-i8255.c
> index 9b97db418df1..9ecb2e9b97f9 100644
> --- a/drivers/gpio/gpio-i8255.c
> +++ b/drivers/gpio/gpio-i8255.c
> @@ -4,23 +4,31 @@
> * Copyright (C) 2022 William Breathitt Gray
> */
> #include <linux/bitmap.h>
> +#include <linux/device.h>
> #include <linux/err.h>
> #include <linux/export.h>
> +#include <linux/gpio/regmap.h>
> #include <linux/io.h>
> #include <linux/module.h>
> +#include <linux/regmap.h>
> #include <linux/spinlock.h>
> #include <linux/types.h>
>
> #include "gpio-i8255.h"
>
> +#define I8255_NGPIO 24
> +#define I8255_NGPIO_PER_REG 8
> #define I8255_CONTROL_PORTC_LOWER_DIRECTION BIT(0)
> #define I8255_CONTROL_PORTB_DIRECTION BIT(1)
> #define I8255_CONTROL_PORTC_UPPER_DIRECTION BIT(3)
> #define I8255_CONTROL_PORTA_DIRECTION BIT(4)
> #define I8255_CONTROL_MODE_SET BIT(7)
> -#define I8255_PORTA 0
> -#define I8255_PORTB 1
> -#define I8255_PORTC 2
> +#define I8255_PORTA 0x0
> +#define I8255_PORTB 0x1
> +#define I8255_PORTC 0x2
> +#define I8255_CONTROL 0x3
> +#define I8255_REG_DAT_BASE I8255_PORTA
> +#define I8255_REG_DIR_IN_BASE I8255_CONTROL
>
> static int i8255_get_port(struct i8255 __iomem *const ppi,
> const unsigned long io_port, const unsigned long mask)
> @@ -31,20 +39,19 @@ static int i8255_get_port(struct i8255 __iomem *const ppi,
> return ioread8(&ppi[bank].port[ppi_port]) & mask;
> }
>
> -static u8 i8255_direction_mask(const unsigned long offset)
> +static int i8255_direction_mask(const unsigned int offset)
> {
> - const unsigned long port_offset = offset % 8;
> - const unsigned long io_port = offset / 8;
> - const unsigned long ppi_port = io_port % 3;
> + const unsigned int stride = offset / I8255_NGPIO_PER_REG;
> + const unsigned int line = offset % I8255_NGPIO_PER_REG;
>
> - switch (ppi_port) {
> + switch (stride) {
> 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 (port_offset >= 4)
> + if (line >= 4)
> return I8255_CONTROL_PORTC_UPPER_DIRECTION;
> return I8255_CONTROL_PORTC_LOWER_DIRECTION;
> default:
> @@ -53,6 +60,49 @@ static u8 i8255_direction_mask(const unsigned long offset)
> }
> }
>
> +static int i8255_ppi_init(struct regmap *const map, const unsigned int base)
> +{
> + int err;
> +
> + /* Configure all ports to MODE 0 output mode */
> + err = regmap_write(map, base + I8255_CONTROL, I8255_CONTROL_MODE_SET);
> + if (err)
> + return err;
> +
> + /* Initialize all GPIO to output 0 */
> + err = regmap_write(map, base + I8255_PORTA, 0x00);
> + if (err)
> + return err;
> + err = regmap_write(map, base + I8255_PORTB, 0x00);
> + if (err)
> + return err;
> + return regmap_write(map, base + I8255_PORTC, 0x00);
> +}
> +
> +static int i8255_reg_mask_xlate(struct gpio_regmap *gpio, unsigned int base,
> + unsigned int offset, unsigned int *reg,
> + unsigned int *mask)
> +{
> + const unsigned int ppi = offset / I8255_NGPIO;
> + const unsigned int ppi_offset = offset % I8255_NGPIO;
> + const unsigned int stride = ppi_offset / I8255_NGPIO_PER_REG;
> + const unsigned int line = ppi_offset % I8255_NGPIO_PER_REG;
> +
> + switch (base) {
> + case I8255_REG_DAT_BASE:
> + *reg = base + stride + ppi * 4;
> + *mask = BIT(line);
> + return 0;
> + case I8255_REG_DIR_IN_BASE:
> + *reg = base + ppi * 4;
> + *mask = i8255_direction_mask(ppi_offset);
> + return 0;
> + default:
> + /* Should never reach this path */
> + return -EINVAL;
> + }
> +}
> +
> static void i8255_set_port(struct i8255 __iomem *const ppi,
> struct i8255_state *const state,
> const unsigned long io_port,
> @@ -93,7 +143,7 @@ void i8255_direction_input(struct i8255 __iomem *const ppi,
> spin_lock_irqsave(&state[bank].lock, flags);
>
> state[bank].control_state |= I8255_CONTROL_MODE_SET;
> - state[bank].control_state |= i8255_direction_mask(offset);
> + state[bank].control_state |= i8255_direction_mask(offset % 24);
>
> iowrite8(state[bank].control_state, &ppi[bank].control);
>
> @@ -125,7 +175,7 @@ void i8255_direction_output(struct i8255 __iomem *const ppi,
> spin_lock_irqsave(&state[bank].lock, flags);
>
> state[bank].control_state |= I8255_CONTROL_MODE_SET;
> - state[bank].control_state &= ~i8255_direction_mask(offset);
> + state[bank].control_state &= ~i8255_direction_mask(offset % 24);
>
> iowrite8(state[bank].control_state, &ppi[bank].control);
>
> @@ -165,7 +215,7 @@ int i8255_get_direction(const struct i8255_state *const state,
> const unsigned long io_port = offset / 8;
> const unsigned long bank = io_port / 3;
>
> - return !!(state[bank].control_state & i8255_direction_mask(offset));
> + return !!(state[bank].control_state & i8255_direction_mask(offset % 24));
> }
> EXPORT_SYMBOL_NS_GPL(i8255_get_direction, I8255);
>
> @@ -282,6 +332,51 @@ void i8255_state_init(struct i8255_state *const state,
> }
> EXPORT_SYMBOL_NS_GPL(i8255_state_init, I8255);
>
> +/**
> + * devm_i8255_regmap_register - Register an i8255 GPIO controller
> + * @dev: device that is registering this i8255 GPIO device
> + * @config: configuration for i8255_regmap_config
> + *
> + * Registers an Intel 8255 Programmable Peripheral Interface GPIO controller.
> + * Returns 0 on success and negative error number on failure.
> + */
> +int devm_i8255_regmap_register(struct device *const dev,
> + const struct i8255_regmap_config *const config)
> +{
> + struct gpio_regmap_config gpio_config = {0};
> + unsigned long i;
> + int err;
> +
> + if (!config->parent)
> + return -EINVAL;
> +
> + if (!config->map)
> + return -EINVAL;
> +
> + if (!config->num_ppi)
> + return -EINVAL;
> +
> + for (i = 0; i < config->num_ppi; i++) {
> + err = i8255_ppi_init(config->map, i * 4);
> + if (err)
> + return err;
> + }
> +
> + gpio_config.parent = config->parent;
> + gpio_config.regmap = config->map;
> + gpio_config.ngpio = I8255_NGPIO * config->num_ppi;
> + gpio_config.names = config->names;
> + gpio_config.reg_dat_base = GPIO_REGMAP_ADDR(I8255_REG_DAT_BASE);
> + gpio_config.reg_set_base = GPIO_REGMAP_ADDR(I8255_REG_DAT_BASE);
> + gpio_config.reg_dir_in_base = GPIO_REGMAP_ADDR(I8255_REG_DIR_IN_BASE);
> + gpio_config.ngpio_per_reg = I8255_NGPIO_PER_REG;
> + gpio_config.irq_domain = config->domain;
> + gpio_config.reg_mask_xlate = i8255_reg_mask_xlate;
> +
> + return PTR_ERR_OR_ZERO(devm_gpio_regmap_register(dev, &gpio_config));
> +}
> +EXPORT_SYMBOL_NS_GPL(devm_i8255_regmap_register, I8255);
> +
> MODULE_AUTHOR("William Breathitt Gray");
> MODULE_DESCRIPTION("Intel 8255 Programmable Peripheral Interface");
> MODULE_LICENSE("GPL");
> diff --git a/drivers/gpio/gpio-i8255.h b/drivers/gpio/gpio-i8255.h
> index d9084aae9446..6ec987835c14 100644
> --- a/drivers/gpio/gpio-i8255.h
> +++ b/drivers/gpio/gpio-i8255.h
> @@ -3,6 +3,9 @@
> #ifndef _I8255_H_
> #define _I8255_H_
> +#include <linux/device.h>
> +#include <linux/irqdomain.h>
> +#include <linux/regmap.h>
As far as I can see you have no users for these headers. You may inform
compiler by providing forward declarations. Like
struct device;
after the inclusion block.
> #include <linux/spinlock.h>
> #include <linux/types.h>
>
> @@ -26,6 +29,30 @@ struct i8255_state {
> u8 control_state;
> };
>
> +#define i8255_volatile_regmap_range(_base) regmap_reg_range(_base, _base + 0x2)
> +
> +/**
> + * struct i8255_regmap_config - Configuration for the register map of an i8255
> + * @parent: parent device
> + * @map: regmap for the i8255
> + * @num_ppi: number of i8255 Programmable Peripheral Interface
> + * @names: (optional) array of names for gpios
> + * @domain: (optional) IRQ domain if the controller is interrupt-capable
> + * Note: The regmap is expected to have cache enabled and i8255 control
> + * registers not marked as volatile.
Have you considered to catch wrong configurations by BUILD_BUG_ON() /
static_assert() / another means of validation?
> + */
> +struct i8255_regmap_config {
> + struct device *parent;
> + struct regmap *map;
> + int num_ppi;
> + const char *const *names;
> + struct irq_domain *domain;
> +};
> +
> +int devm_i8255_regmap_register(struct device *dev,
> + const struct i8255_regmap_config *config);
> +
> void i8255_direction_input(struct i8255 __iomem *ppi, struct i8255_state *state,
> unsigned long offset);
> void i8255_direction_output(struct i8255 __iomem *ppi,
> --
> 2.38.1
>
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists