[2/2] gpio: max7317: Add gpio expander driver

Message ID 20221206124530.5431-1-edmund.berenson@emlix.com
State New
Headers
Series [1/2] dt-bindings: gpio: max7317: add gpio driver bindings |

Commit Message

Edmund Berenson Dec. 6, 2022, 12:45 p.m. UTC
  Add driver for maxim MAX7317 SPI-Interfaced 10 Port
GPIO Expander.

Co-developed-by: Lukasz Zemla <Lukasz.Zemla@woodward.com>
Signed-off-by: Lukasz Zemla <Lukasz.Zemla@woodward.com>
Signed-off-by: Edmund Berenson <edmund.berenson@emlix.com>
---
 drivers/gpio/Kconfig        |   8 ++
 drivers/gpio/Makefile       |   1 +
 drivers/gpio/gpio-max7317.c | 221 ++++++++++++++++++++++++++++++++++++
 3 files changed, 230 insertions(+)
 create mode 100644 drivers/gpio/gpio-max7317.c
  

Comments

Linus Walleij Dec. 6, 2022, 1:48 p.m. UTC | #1
Hi Edmund & Lukasz,

thanks for your patch!

On Tue, Dec 6, 2022 at 1:45 PM Edmund Berenson
<edmund.berenson@emlix.com> wrote:
>
> Add driver for maxim MAX7317 SPI-Interfaced 10 Port
> GPIO Expander.
>
> Co-developed-by: Lukasz Zemla <Lukasz.Zemla@woodward.com>
> Signed-off-by: Lukasz Zemla <Lukasz.Zemla@woodward.com>
> Signed-off-by: Edmund Berenson <edmund.berenson@emlix.com>

(...)

> +config GPIO_MAX7317
> +       tristate "Maxim MAX7317 GPIO expander"
> +       depends on SPI

Skip this. Just put it inside the section:

menu "SPI GPIO expanders"
        depends on SPI_MASTER

But consider this:

select REGMAP_SPI

> +/* A write to the MAX7317 means one message with one transfer */
> +static int max7317_spi_write(struct device *dev, unsigned int reg,
> +                               unsigned int val)
> +{
> +       struct spi_device *spi = to_spi_device(dev);
> +       u16 word = ((reg & 0x7F) << 8) | (val & 0xFF);
> +
> +       return spi_write_then_read(spi, &word, sizeof(word), NULL, 0);
> +}
> +
> +/* A read from the MAX7317 means two transfers, with chip select going high between them. */
> +static int max7317_spi_read(struct device *dev, unsigned int reg)
> +{
> +       int ret;
> +       u16 word;
> +       struct spi_device *spi = to_spi_device(dev);
> +
> +       word = 0x8000 | (reg << 8);
> +
> +       /* First transfer: ask to read register */
> +       ret = spi_write(spi, &word, sizeof(word));
> +       if (ret)
> +               return ret;
> +
> +       /* Second transfer: read register value */
> +       ret = spi_read(spi, &word, sizeof(word));
> +       if (ret)
> +               return ret;
> +
> +       return word & 0xff;
> +}

I bet this is standard SPI addressing of some kind, if in doubt Mark Brown
will know. Look into REGMAP_SPI and use regmap accessors in the driver.
Look at: drivers/gpio/gpio-xra1403.c for an example.

> +static int max7317_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct max7317 *ts = gpiochip_get_data(chip);
> +       unsigned int reg, val, mask, shift;

Switch to unsigned long when using regmap.

> +       int inputs;
> +
> +       if (offset < 8) {
> +               reg = REG_CODE_READ_PORTS_7_TO_0;
> +               mask = 1u << offset;

mask = BIT(offset);

> +               shift = offset;
> +       } else {
> +               reg = REG_CODE_READ_PORTS_9_TO_8;
> +               mask = 1u << (offset - 8);

mask = BIT(offset - 8);

> +               shift = offset - 8;
> +       }
> +
> +       mutex_lock(&ts->lock);
> +       inputs = max7317_spi_read(ts->dev, reg);
> +       mutex_unlock(&ts->lock);

Regmap also contains a mutex already so you will just need a oneliner
for things like this.

> +       val = ((unsigned int)inputs & mask) >> shift;
> +       return (int)val;

Ugh get rid of this shifting and casting.

I tend to do:

return !!(inputs & mask);

That will clamp whatever value to a bool.

Once you have converted the driver to use REGMAP_SPI, take a
look at GPIO_REGMAP in drivers/gpio/gpio-regmap.c and see if
you can also use that helper to slim this down even more. (Uncertain
about that, but check it!)

Yours,
Linus Walleij
  
kernel test robot Dec. 6, 2022, 8:29 p.m. UTC | #2
Hi Edmund,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on brgl/gpio/for-next]
[also build test ERROR on linus/master v6.1-rc8 next-20221206]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Edmund-Berenson/dt-bindings-gpio-max7317-add-gpio-driver-bindings/20221206-205508
base:   https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next
patch link:    https://lore.kernel.org/r/20221206124530.5431-1-edmund.berenson%40emlix.com
patch subject: [PATCH 2/2] gpio: max7317: Add gpio expander driver
config: s390-allyesconfig
compiler: s390-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/7945c937dd9f6cca9a8b30fea9e674a88946705e
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Edmund-Berenson/dt-bindings-gpio-max7317-add-gpio-driver-bindings/20221206-205508
        git checkout 7945c937dd9f6cca9a8b30fea9e674a88946705e
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash drivers/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/gpio/gpio-max7317.c:14:10: fatal error: linux/platform_data/max7317.h: No such file or directory
      14 | #include <linux/platform_data/max7317.h>
         |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   compilation terminated.


vim +14 drivers/gpio/gpio-max7317.c

  > 14	#include <linux/platform_data/max7317.h>
    15
  

Patch

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index a01af1180616..24ed968a2b7f 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1622,6 +1622,14 @@  config GPIO_MAX7301
 	help
 	  GPIO driver for Maxim MAX7301 SPI-based GPIO expander.
 
+config GPIO_MAX7317
+	tristate "Maxim MAX7317 GPIO expander"
+	depends on SPI
+	help
+	  GPIO driver for Maxim MAX7317 SPI-based GPIO expander.
+	  The MAX7317 is a serial-interfaced gpio extender, with
+	  10 ports.
+
 config GPIO_MC33880
 	tristate "Freescale MC33880 high-side/low-side switch"
 	help
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 29e3beb6548c..edbfb3d918ce 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -89,6 +89,7 @@  obj-$(CONFIG_GPIO_MAX7300)		+= gpio-max7300.o
 obj-$(CONFIG_GPIO_MAX7301)		+= gpio-max7301.o
 obj-$(CONFIG_GPIO_MAX730X)		+= gpio-max730x.o
 obj-$(CONFIG_GPIO_MAX732X)		+= gpio-max732x.o
+obj-$(CONFIG_GPIO_MAX7317)		+= gpio-max7317.o
 obj-$(CONFIG_GPIO_MAX77620)		+= gpio-max77620.o
 obj-$(CONFIG_GPIO_MAX77650)		+= gpio-max77650.o
 obj-$(CONFIG_GPIO_MB86S7X)		+= gpio-mb86s7x.o
diff --git a/drivers/gpio/gpio-max7317.c b/drivers/gpio/gpio-max7317.c
new file mode 100644
index 000000000000..58b66f763beb
--- /dev/null
+++ b/drivers/gpio/gpio-max7317.c
@@ -0,0 +1,221 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2021, Lukasz Zemla, Woodward Inc.
+ *
+ */
+
+#include <linux/gpio.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+#include <linux/platform_data/max7317.h>
+
+/* MAX7317 has 10 pins, controlled by 10 internal registers,
+ * addressed as 0x00 - 0x09.
+ */
+#define PIN_NUMBER 10
+
+/* Register code to read inputs from 7 to 0 */
+#define REG_CODE_READ_PORTS_7_TO_0	((u8)0x0E)
+/* Register code to read inputs from 9 to 8 */
+#define REG_CODE_READ_PORTS_9_TO_8	((u8)0x0F)
+
+struct max7317 {
+	struct mutex		lock;
+	struct gpio_chip	chip;
+	struct device		*dev;
+};
+
+struct max7317_platform_data {
+	/* number assigned to the first GPIO */
+	unsigned int	gpio_base;
+};
+
+/* A write to the MAX7317 means one message with one transfer */
+static int max7317_spi_write(struct device *dev, unsigned int reg,
+				unsigned int val)
+{
+	struct spi_device *spi = to_spi_device(dev);
+	u16 word = ((reg & 0x7F) << 8) | (val & 0xFF);
+
+	return spi_write_then_read(spi, &word, sizeof(word), NULL, 0);
+}
+
+/* A read from the MAX7317 means two transfers, with chip select going high between them. */
+static int max7317_spi_read(struct device *dev, unsigned int reg)
+{
+	int ret;
+	u16 word;
+	struct spi_device *spi = to_spi_device(dev);
+
+	word = 0x8000 | (reg << 8);
+
+	/* First transfer: ask to read register */
+	ret = spi_write(spi, &word, sizeof(word));
+	if (ret)
+		return ret;
+
+	/* Second transfer: read register value */
+	ret = spi_read(spi, &word, sizeof(word));
+	if (ret)
+		return ret;
+
+	return word & 0xff;
+}
+
+static void max7317_set(struct gpio_chip *chip, unsigned int offset, int value)
+{
+	struct max7317 *ts = gpiochip_get_data(chip);
+
+	mutex_lock(&ts->lock);
+	max7317_spi_write(ts->dev, offset, value);
+	mutex_unlock(&ts->lock);
+}
+
+static int max7317_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct max7317 *ts = gpiochip_get_data(chip);
+	unsigned int reg, val, mask, shift;
+	int inputs;
+
+	if (offset < 8) {
+		reg = REG_CODE_READ_PORTS_7_TO_0;
+		mask = 1u << offset;
+		shift = offset;
+	} else {
+		reg = REG_CODE_READ_PORTS_9_TO_8;
+		mask = 1u << (offset - 8);
+		shift = offset - 8;
+	}
+
+	mutex_lock(&ts->lock);
+	inputs = max7317_spi_read(ts->dev, reg);
+	mutex_unlock(&ts->lock);
+
+	val = ((unsigned int)inputs & mask) >> shift;
+
+	return (int)val;
+}
+
+static int max7317_direction_input(struct gpio_chip *chip, unsigned int offset)
+{
+	/* Per documentation:
+	 * 'Configure a port as an input by setting its output register to 0x01,
+	 * which sets the port output high impedance (Table 4).
+	 */
+	max7317_set(chip, offset, 1);
+	return 0;
+}
+
+static int max7301_direction_output(struct gpio_chip *chip, unsigned int offset,
+				    int value)
+{
+	max7317_set(chip, offset, value);
+	return 0;
+}
+
+static int max7317_probe(struct spi_device *spi)
+{
+	struct max7317 *ts;
+	struct device *dev;
+	struct max7317_platform_data *pdata;
+	int ret;
+
+	/* bits_per_word cannot be configured in platform data */
+	spi->bits_per_word = 16;
+	ret = spi_setup(spi);
+	if (ret < 0)
+		return ret;
+
+	ts = devm_kzalloc(&spi->dev, sizeof(struct max7317), GFP_KERNEL);
+	if (!ts)
+		return -ENOMEM;
+
+	ts->dev = &spi->dev;
+
+	dev = ts->dev;
+
+	pdata = dev_get_platdata(dev);
+
+	mutex_init(&ts->lock);
+	dev_set_drvdata(dev, ts);
+
+	if (pdata)
+		ts->chip.base = pdata->gpio_base;
+	else
+		ts->chip.base = -1;
+
+	ts->chip.label = dev->driver->name;
+
+	ts->chip.direction_input = max7317_direction_input;
+	ts->chip.get = max7317_get;
+	ts->chip.direction_output = max7301_direction_output;
+	ts->chip.set = max7317_set;
+
+	ts->chip.ngpio = PIN_NUMBER;
+	ts->chip.can_sleep = true;
+	ts->chip.parent = dev;
+	ts->chip.owner = THIS_MODULE;
+
+	ret = gpiochip_add_data(&ts->chip, ts);
+	if (!ret)
+		return ret;
+
+	mutex_destroy(&ts->lock);
+	return ret;
+}
+
+static void max7317_remove(struct spi_device *spi)
+{
+	struct max7317 *ts = dev_get_drvdata(&spi->dev);
+
+	if (!ts)
+		return;
+
+	gpiochip_remove(&ts->chip);
+	mutex_destroy(&ts->lock);
+}
+
+static const struct spi_device_id max7317_id[] = {
+	{ "max7317", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, max7317_id);
+
+static const struct of_device_id max7317_of_table[] = {
+	{ .compatible = "maxim,max7317" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, max7317_of_table);
+
+static struct spi_driver max7317_driver = {
+	.driver = {
+		.name = "max7317",
+		.of_match_table	= of_match_ptr(max7317_of_table),
+	},
+	.probe = max7317_probe,
+	.remove = max7317_remove,
+	.id_table = max7317_id,
+};
+
+static int __init max7317_init(void)
+{
+	return spi_register_driver(&max7317_driver);
+}
+/* register after spi postcore initcall and before
+ * subsys initcalls that may rely on these GPIOs
+ */
+subsys_initcall(max7317_init);
+
+static void __exit max7317_exit(void)
+{
+	spi_unregister_driver(&max7317_driver);
+}
+module_exit(max7317_exit);
+
+MODULE_AUTHOR("Lukasz Zemla");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("MAX7317 GPIO-Expander");