mbox series

[v3,0/5] ARM: Add GPIO support

Message ID 20230606014234.29491-1-nick.hawkins@hpe.com
Headers show
Series ARM: Add GPIO support | expand

Message

Hawkins, Nick June 6, 2023, 1:42 a.m. UTC
From: Nick Hawkins <nick.hawkins@hpe.com>

The GXP SoC supports GPIO on multiple interfaces. The interfaces are
CPLD and Host. The GPIOs is a combination of both physical and virtual
I/O across the interfaces. The gpio-gxp driver specifically covers the
CSM(physical), FN2(virtual), and VUHC(virtual) which are the host. The
gpio-gxp-pl driver covers the CPLD which takes physical I/O from the
board and shares it with GXP via a propriety interface that maps the I/O
onto a specific register area of the GXP. The drivers both support
interrupts but from different interrupt parents.

The gxp-fan-ctrl driver in HWMON no longer will report fan presence
or fan failure states as these GPIOs providing this information will be
consumed by the host. It will be the hosts function to keep track of
fan presence and status.

---
Changes since v2:
 *Removed shared fan variables between HWMON and GPIO based on feedback
 *Removed reporting fan presence and failure from hwmon gxp-fan-ctrl
  driver
 *Removed GPIO dependency from gxp-fan-ctrl driver
 *Changed description and title for hpe,gxp-gpio binding
 *Corrected indention on example for hpe,gxp-gpio binding
 *Removed additional example from hpe,gxp-gpio binding
 
Changes since v1:
 *Removed ARM device tree changes and defconfig changes to reduce
  patchset size
 *Removed GXP PSU changes to reduce patchset size
 *Corrected hpe,gxp-gpio YAML file based on feedback
 *Created new gpio-gxp-pl file to reduce complexity
 *Separated code into two files to keep size down: gpio-gxp.c and
  gpio-gxp-pl.c
 *Fixed Kconfig indentation as well as add new entry for gpio-gxp-pl
 *Removed use of linux/of.h and linux/of_device.h
 *Added mod_devicetable.h and property.h
 *Fixed indentation of defines and uses consistent number of digits
 *Corrected defines with improper GPIO_ namespace.
 *For masks now use BIT()
 *Added comment for PLREG offsets
 *Move gpio_chip to be first in structure
 *Calculate offset for high and low byte GPIO reads instead of having
  H(High) and L(Low) letters added to the variables.
 *Removed repeditive use of "? 1 : 0"
 *Switched to handle_bad_irq()
 *Removed improper bailout on gpiochip_add_data
 *Used GENMASK to arm interrupts
 *Removed use of of_match_device
 *fixed sizeof in devm_kzalloc
 *Added COMPILE_TEST to Kconfig
 *Added dev_err_probe where applicable
 *Removed unecessary parent and compatible checks

Nick Hawkins (5):
  dt-bindings: gpio: Add HPE GXP GPIO
  gpio: gxp: Add HPE GXP GPIO
  dt-bindings: hwmon: hpe,gxp-fan-ctrl: remove fn2 and pl registers
  hwmon: (gxp_fan_ctrl) Provide fan info via gpio
  MAINTAINERS: hpe: Add GPIO

 .../bindings/gpio/hpe,gxp-gpio.yaml           | 139 ++++
 .../bindings/hwmon/hpe,gxp-fan-ctrl.yaml      |  16 +-
 MAINTAINERS                                   |   2 +
 drivers/gpio/Kconfig                          |  18 +
 drivers/gpio/Makefile                         |   2 +
 drivers/gpio/gpio-gxp-pl.c                    | 519 ++++++++++++++
 drivers/gpio/gpio-gxp.c                       | 637 ++++++++++++++++++
 drivers/hwmon/Kconfig                         |   2 +-
 drivers/hwmon/gxp-fan-ctrl.c                  | 108 +--
 9 files changed, 1324 insertions(+), 119 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/hpe,gxp-gpio.yaml
 create mode 100644 drivers/gpio/gpio-gxp-pl.c
 create mode 100644 drivers/gpio/gpio-gxp.c

Comments

Hawkins, Nick June 7, 2023, 8:45 p.m. UTC | #1
> It does care about things the average GPIO controller driver needs to
> repeat. So at least you may try and see how it will look.


> > If gpio_regmap is required, how do I create a direct correlation
> > between a specific gpio-line and a register offset? For example, in
> > gpio-gxp-pl.c. Gpio-line at offset 0 (IOPLED) is at register 0x04. The
> > gpio-line at offset 8 (FAN_INST) is at register 0x27.


> You may remap registers. See, for example, gpio-pca953x, where some of
> the registers (with high bit set) are actually virtual rather than
> real offsets. Similar idea can be used in your case.

Greetings Andy,

Is there any documents available describing how regmap_gpio
populates the GPIO lines? Does it automatically go through and add lines
for each successful regmap_read and bits per byte?

I have taken your advice and used the additional readable and writeable
on regmap_config to limit the number of accessible registers.

static const struct regmap_config gxp_regmap_config = {
        .reg_bits = 8,
        .reg_stride = 1,
        .val_bits = 8,
        .readable_reg = gxp_readable_register,
        .writeable_reg = gxp_writeable_register,
        .max_register = 0x80,
        .name = "gxp-gpio-pl",
};

static const struct regmap_config gxp_int_regmap_config = {
        .reg_bits = 8,
        .reg_stride = 1,
        .val_bits = 8,
        .readable_reg = gxp_read_write_int_register,
        .writeable_reg = gxp_read_write_int_register,
        .max_register = 0x7f
        .name = "gxp-gpio-pl-int"
};

Thanks,

-Nick Hawkins
Andy Shevchenko June 8, 2023, 10:22 a.m. UTC | #2
On Wed, Jun 7, 2023 at 11:45 PM Hawkins, Nick <nick.hawkins@hpe.com> wrote:
>
> > It does care about things the average GPIO controller driver needs to
> > repeat. So at least you may try and see how it will look.

...

> Is there any documents available describing how regmap_gpio
> populates the GPIO lines? Does it automatically go through and add lines
> for each successful regmap_read and bits per byte?

Nope, it assumes one bit per register or something different if xlate
callback is defined. This is my understanding. That said, it might be
that this is a limitation which does not allow you to switch to that
library.

...

> static const struct regmap_config gxp_int_regmap_config = {
>         .reg_bits = 8,

>         .reg_stride = 1,

AFAIU 0 is the same as 1, so this can be dropped.

>         .val_bits = 8,
>         .readable_reg = gxp_read_write_int_register,
>         .writeable_reg = gxp_read_write_int_register,
>         .max_register = 0x7f
>         .name = "gxp-gpio-pl-int"
> };
Hawkins, Nick June 8, 2023, 2:58 p.m. UTC | #3
> > Is there any documents available describing how regmap_gpio
> > populates the GPIO lines? Does it automatically go through and add lines
> > for each successful regmap_read and bits per byte?


> Nope, it assumes one bit per register or something different if xlate
> callback is defined. This is my understanding. That said, it might be
> that this is a limitation which does not allow you to switch to that
> library.


Greetings Andy,

Thank you for this feedback. After exploring the gpio_regmap it seems
it does not fit my needs. Some of the GPIOs are a combination of
several bits in a byte. For instance the Health LED or Identify LED have
more than 2 states. If acceptable I believe the gxp-gpio-pl.c file should
not use the gpio_regmap.

Thanks,

-Nick Hawkins
Andy Shevchenko June 8, 2023, 5:15 p.m. UTC | #4
On Thu, Jun 8, 2023 at 5:58 PM Hawkins, Nick <nick.hawkins@hpe.com> wrote:
> > > Is there any documents available describing how regmap_gpio
> > > populates the GPIO lines? Does it automatically go through and add lines
> > > for each successful regmap_read and bits per byte?
>
> > Nope, it assumes one bit per register or something different if xlate
> > callback is defined. This is my understanding. That said, it might be
> > that this is a limitation which does not allow you to switch to that
> > library.
>
> Thank you for this feedback. After exploring the gpio_regmap it seems
> it does not fit my needs. Some of the GPIOs are a combination of
> several bits in a byte. For instance the Health LED or Identify LED have
> more than 2 states. If acceptable I believe the gxp-gpio-pl.c file should
> not use the gpio_regmap.

Yes, just mention this reasoning in the cover letter.