mbox series

[v1,0/2] gpio: fxl6408: add I2C GPIO expander driver

Message ID 20230306083446.41082-1-francesco@dolcini.it
Headers show
Series gpio: fxl6408: add I2C GPIO expander driver | expand

Message

Francesco Dolcini March 6, 2023, 8:34 a.m. UTC
From: Francesco Dolcini <francesco.dolcini@toradex.com>

Add support for Fairchild (now ON Semiconductor) fxl6408 8-bit I2C-controlled
GPIO expander, see data-sheet [0].

[0] https://www.onsemi.com/download/data-sheet/pdf/fxl6408-d.pdf

Emanuele Ghidoli (2):
  dt-bindings: gpio: add fcs,fxl6408-gpio binding document
  gpio: fxl6408: add I2C GPIO expander driver

 .../bindings/gpio/fcs,fxl6408-gpio.yaml       |  73 ++++++++
 drivers/gpio/Kconfig                          |   6 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-fxl6408.c                   | 160 ++++++++++++++++++
 4 files changed, 240 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/fcs,fxl6408-gpio.yaml
 create mode 100644 drivers/gpio/gpio-fxl6408.c

Comments

Francesco Dolcini March 6, 2023, 8:55 a.m. UTC | #1
On Mon, Mar 06, 2023 at 09:34:46AM +0100, Francesco Dolcini wrote:
> From: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>
> 
> Support Fairchild (now ON Semiconductor) fxl6408 which has 8 GPIO lines
> and is controlled by I2C bus.
> 
> Signed-off-by: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>
Whoops. As already noticed by Krzysztof in patch 1/2 I forgot my SoB,
will fix in v2.

Francesco
Andy Shevchenko March 6, 2023, 11:54 p.m. UTC | #2
Mon, Mar 06, 2023 at 09:34:46AM +0100, Francesco Dolcini kirjoitti:
> From: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>
> 
> Support Fairchild (now ON Semiconductor) fxl6408 which has 8 GPIO lines
> and is controlled by I2C bus.

Is it really GPIO expander and not a (semi-)featured pin control with GPIO
capability?

Can we have a Datasheet: tag here?

> Signed-off-by: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>

...

> +	help
> +	  GPIO driver for Fairchild Semiconductor FXL6408 GPIO expander

Checkpatch usually complains on the help < 3 lines.
You may add the module name for M choice.

...

> + * Author: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>

> + *

Unneeded blank line.

...

> +#include <linux/gpio.h>

No way. This must not be in any code.

...

> +#include <linux/of_platform.h>

Why?
For discrete components make sure you have not an OF-centric code.

...

> +static const struct regmap_range rd_range[] = {
> +	{ FXL6408_REG_DEVICE_ID, FXL6408_REG_DEVICE_ID },
> +	{ FXL6408_REG_IO_DIR, FXL6408_REG_OUTPUT },
> +	{ FXL6408_REG_INPUT_STATUS, FXL6408_REG_INPUT_STATUS }

In all definitions where the entry is _not_ a terminator, leave the trailing
comma in place.

> +};

...

> +	};

> +	gpio_config.regmap = devm_regmap_init_i2c(client, &regmap);

> +

This blank line is misplaced. Should be before devm_regmap_init_i2c() call.

> +	if (IS_ERR(gpio_config.regmap)) {

> +		dev_err(dev, "failed to allocate register map\n");
> +		return PTR_ERR(gpio_config.regmap);

	return dev_err_probe();

> +	}

...

> +	/* Disable High-Z of outputs, so that our OUTPUT updates
> +	 * actually take effect.
> +	 */

/*
 * This is correct style for multi-line
 * comments. Yours needs to be fixed.
 */

...

> +	ret = regmap_write(gpio_config.regmap, FXL6408_REG_OUTPUT_HIGH_Z, 0);
> +	if (ret) {

> +		dev_err(dev, "failed to write 'output high Z' register\n");
> +		return ret;

	return dev_err_probe(...);

> +	}

...

> +static const struct i2c_device_id fxl6408_id[] = {
> +	{ "fxl6408", 0 },
> +	{ },

But no comma for a terminator entry.
 
> +};

...

> +

Unneeded blank line.

> +module_i2c_driver(fxl6408_driver);