[RFC PATCH RESEND 0/3] UP Squared board drivers

[RFC PATCH RESEND 0/3] UP Squared board drivers

[Javier Arteaga]

[linux-kernel correctly Cc'd on the series now. Sorry for the noise.]

Hi all,

This series adds platform support for AAEON's UP Squared, a x86 SBC
based on Apollo Lake [1]. Specifically, it enables control for the board
pin headers and for the LEDs, as both of these features are only
available behind a FPGA-based platform controller.

This is structured around a MFD driver that provides a regmap interface
to that platform controller to each MFD cell.

The series is carved out from an out-of-tree support patchset, initially
written by Dan O'Donovan for the original UP Board [2] and then extended
through the past year to support new entries in the lineup (UP Board, UP
Squared and UP Core) [3].

Here we only submit support for UP Squared to simplify review. Still,
the driver is designed so that it can easily gain support for the other
boards.

Questions:

* Is MFD the right fit for the platform controller?
  The intention here is to encapsulate the custom GPIO-bitbanged control
  protocol by sharing the regmap between the drivers.

* Is our use of two chained pinctrls (in patch 3/3) valid?

Thank you!

[1]: http://www.up-board.org/upsquared/specifications-up2/
[2]: https://lkml.kernel.org/r/1467648434-29080-1-git-send-email-dan@emutex.com
[3]: https://github.com/emutex/ubilinux-kernel/commits/upboard-4.9

Javier Arteaga (3):
  mfd: upboard: Add UP2 platform controller driver
  leds: upboard: Add LED support
  pinctrl: upboard: Add pinctrl and gpio driver

 drivers/leds/Kconfig              |  10 +
 drivers/leds/Makefile             |   1 +
 drivers/leds/leds-upboard.c       |  87 +++++
 drivers/mfd/Kconfig               |  17 +
 drivers/mfd/Makefile              |   1 +
 drivers/mfd/upboard.c             | 273 ++++++++++++++++
 drivers/pinctrl/Kconfig           |  13 +
 drivers/pinctrl/Makefile          |   1 +
 drivers/pinctrl/pinctrl-upboard.c | 523 ++++++++++++++++++++++++++++++
 include/linux/mfd/upboard.h       |  70 ++++
 10 files changed, 996 insertions(+)
 create mode 100644 drivers/leds/leds-upboard.c
 create mode 100644 drivers/mfd/upboard.c
 create mode 100644 drivers/pinctrl/pinctrl-upboard.c
 create mode 100644 include/linux/mfd/upboard.h

-- 
2.17.0

[RFC PATCH RESEND 1/3] mfd: upboard: Add UP2 platform controller driver

[Javier Arteaga]

UP Squared (UP2) is a x86 SBC from AAEON based on Intel Apollo Lake. It
features a MAX 10 FPGA that routes lines from both SoC and on-board
devices to two I/O headers:

                                +------------------------+
                                | 40-pin RPi-like header |
                         +------|         (HAT)          |
                         |      +------------------------+
    +-------+    +--------+
    |       |    |        |     +------------------------+
    |  SoC  |----|  FPGA  |-----|  Custom UP2 pin header |
    |       |    |        |     |        (EXHAT)         |
    +-------+    +--------+     +------------------------+
                         |
                         +------* On-board devices: LED, VLS...

This is intended to enable vendor-specific applications to customize I/O
header pinout, as well as include low-latency functionality. It also
performs voltage level translation between the SoC (1.8V) and HAT header
(3.3V).

Out of the box, this block implements a platform controller with a
GPIO-bitbanged control interface. It's enumerated by ACPI and provides
registers to control:

- Configuration of all FPGA-routed header lines. These can be driven
  SoC-to-header, header-to-SoC or set in high impedance.

- On-board LEDs and enable lines for other platform devices.

Add core support for this platform controller as a MFD device, exposing
these registers as a regmap.

Signed-off-by: Javier Arteaga <javier@emutex.com>
---
 drivers/mfd/Kconfig         |  17 +++
 drivers/mfd/Makefile        |   1 +
 drivers/mfd/upboard.c       | 253 ++++++++++++++++++++++++++++++++++++
 include/linux/mfd/upboard.h |  65 +++++++++
 4 files changed, 336 insertions(+)
 create mode 100644 drivers/mfd/upboard.c
 create mode 100644 include/linux/mfd/upboard.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index b860eb5aa194..d72927601fa2 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1812,6 +1812,23 @@ config MFD_STM32_TIMERS
 	  for PWM and IIO Timer. This driver allow to share the
 	  registers between the others drivers.
 
+config MFD_UPBOARD
+	tristate "UP Squared"
+	depends on ACPI
+	depends on GPIOLIB
+	select MFD_CORE
+	select REGMAP
+	help
+	  If you say yes here you get support for the platform controller
+	  of the UP Squared single-board computer.
+
+	  This driver provides common support for accessing the device,
+	  additional drivers must be enabled in order to use the
+	  functionality of the device.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called upboard.
+
 menu "Multimedia Capabilities Port drivers"
 	depends on ARCH_SA1100
 
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index d9d2cf0d32ef..ea5b29167a80 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -229,3 +229,4 @@ obj-$(CONFIG_MFD_MXS_LRADC)     += mxs-lradc.o
 obj-$(CONFIG_MFD_SC27XX_PMIC)	+= sprd-sc27xx-spi.o
 obj-$(CONFIG_RAVE_SP_CORE)	+= rave-sp.o
 
+obj-$(CONFIG_MFD_UPBOARD)  += upboard.o
diff --git a/drivers/mfd/upboard.c b/drivers/mfd/upboard.c
new file mode 100644
index 000000000000..8bae450cb83d
--- /dev/null
+++ b/drivers/mfd/upboard.c
@@ -0,0 +1,253 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * UP Board platform controller driver
+ *
+ * Copyright (c) 2018, Emutex Ltd.
+ *
+ * Author: Javier Arteaga <javier@emutex.com>
+ */
+
+#include <linux/acpi.h>
+#include <linux/gpio/consumer.h>
+#include <linux/kernel.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/upboard.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+static int upboard_read(void *context, unsigned int reg, unsigned int *val)
+{
+	const struct upboard * const upboard = context;
+	int i;
+
+	gpiod_set_value(upboard->clear_gpio, 0);
+	gpiod_set_value(upboard->clear_gpio, 1);
+
+	reg |= UPBOARD_READ_FLAG;
+
+	for (i = UPBOARD_ADDRESS_SIZE; i >= 0; i--) {
+		gpiod_set_value(upboard->strobe_gpio, 0);
+		gpiod_set_value(upboard->datain_gpio, (reg >> i) & 0x1);
+		gpiod_set_value(upboard->strobe_gpio, 1);
+	}
+
+	gpiod_set_value(upboard->strobe_gpio, 0);
+	*val = 0;
+
+	for (i = UPBOARD_REGISTER_SIZE - 1; i >= 0; i--) {
+		gpiod_set_value(upboard->strobe_gpio, 1);
+		gpiod_set_value(upboard->strobe_gpio, 0);
+		*val |= gpiod_get_value(upboard->dataout_gpio) << i;
+	}
+
+	gpiod_set_value(upboard->strobe_gpio, 1);
+
+	return 0;
+};
+
+static int upboard_write(void *context, unsigned int reg, unsigned int val)
+{
+	const struct upboard * const upboard = context;
+	int i;
+
+	gpiod_set_value(upboard->clear_gpio, 0);
+	gpiod_set_value(upboard->clear_gpio, 1);
+
+	for (i = UPBOARD_ADDRESS_SIZE; i >= 0; i--) {
+		gpiod_set_value(upboard->strobe_gpio, 0);
+		gpiod_set_value(upboard->datain_gpio, (reg >> i) & 0x1);
+		gpiod_set_value(upboard->strobe_gpio, 1);
+	}
+
+	gpiod_set_value(upboard->strobe_gpio, 0);
+
+	for (i = UPBOARD_REGISTER_SIZE - 1; i >= 0; i--) {
+		gpiod_set_value(upboard->datain_gpio, (val >> i) & 0x1);
+		gpiod_set_value(upboard->strobe_gpio, 1);
+		gpiod_set_value(upboard->strobe_gpio, 0);
+	}
+
+	gpiod_set_value(upboard->strobe_gpio, 1);
+
+	return 0;
+};
+
+struct upboard_data {
+	const struct regmap_config *regmapconf;
+	const struct mfd_cell *cells;
+	size_t ncells;
+};
+
+/* UP Squared */
+
+static const struct regmap_range upboard_up2_readable_ranges[] = {
+	regmap_reg_range(UPBOARD_REG_PLATFORM_ID, UPBOARD_REG_FIRMWARE_ID),
+	regmap_reg_range(UPBOARD_REG_FUNC_EN0, UPBOARD_REG_FUNC_EN1),
+	regmap_reg_range(UPBOARD_REG_GPIO_EN0, UPBOARD_REG_GPIO_EN2),
+	regmap_reg_range(UPBOARD_REG_GPIO_DIR0, UPBOARD_REG_GPIO_DIR2),
+};
+
+static const struct regmap_range upboard_up2_writable_ranges[] = {
+	regmap_reg_range(UPBOARD_REG_FUNC_EN0, UPBOARD_REG_FUNC_EN1),
+	regmap_reg_range(UPBOARD_REG_GPIO_EN0, UPBOARD_REG_GPIO_EN2),
+	regmap_reg_range(UPBOARD_REG_GPIO_DIR0, UPBOARD_REG_GPIO_DIR2),
+};
+
+static const struct regmap_access_table upboard_up2_readable_table = {
+	.yes_ranges = upboard_up2_readable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(upboard_up2_readable_ranges),
+};
+
+static const struct regmap_access_table upboard_up2_writable_table = {
+	.yes_ranges = upboard_up2_writable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(upboard_up2_writable_ranges),
+};
+
+static const struct regmap_config upboard_up2_regmap_config = {
+	.reg_bits = UPBOARD_ADDRESS_SIZE,
+	.val_bits = UPBOARD_REGISTER_SIZE,
+	.max_register = UPBOARD_REG_MAX,
+	.reg_read = upboard_read,
+	.reg_write = upboard_write,
+	.fast_io = false,
+	.cache_type = REGCACHE_RBTREE,
+	.rd_table = &upboard_up2_readable_table,
+	.wr_table = &upboard_up2_writable_table,
+};
+
+static const struct mfd_cell upboard_up2_mfd_cells[] = {
+};
+
+static const struct upboard_data upboard_up2_data = {
+	.regmapconf = &upboard_up2_regmap_config,
+	.cells = upboard_up2_mfd_cells,
+	.ncells = ARRAY_SIZE(upboard_up2_mfd_cells),
+};
+
+static int upboard_init_gpio(struct upboard *upboard)
+{
+	struct gpio_desc *enable_gpio;
+
+	enable_gpio = devm_gpiod_get(upboard->dev, "enable", GPIOD_OUT_LOW);
+	if (IS_ERR(enable_gpio))
+		return PTR_ERR(enable_gpio);
+
+	upboard->clear_gpio = devm_gpiod_get(upboard->dev, "clear", GPIOD_OUT_LOW);
+	if (IS_ERR(upboard->clear_gpio))
+		return PTR_ERR(upboard->clear_gpio);
+
+	upboard->strobe_gpio = devm_gpiod_get(upboard->dev, "strobe", GPIOD_OUT_LOW);
+	if (IS_ERR(upboard->strobe_gpio))
+		return PTR_ERR(upboard->strobe_gpio);
+
+	upboard->datain_gpio = devm_gpiod_get(upboard->dev, "datain", GPIOD_OUT_LOW);
+	if (IS_ERR(upboard->datain_gpio))
+		return PTR_ERR(upboard->datain_gpio);
+
+	upboard->dataout_gpio = devm_gpiod_get(upboard->dev, "dataout", GPIOD_IN);
+	if (IS_ERR(upboard->dataout_gpio))
+		return PTR_ERR(upboard->dataout_gpio);
+
+	gpiod_set_value(enable_gpio, 1);
+
+	return 0;
+}
+
+static int upboard_check_supported(struct upboard *upboard)
+{
+	const unsigned int AAEON_MANUFACTURER_ID = 0x01;
+	const unsigned int SUPPORTED_FW_MAJOR = 0x0;
+	unsigned int platform_id, manufacturer_id;
+	unsigned int firmware_id, build, major, minor, patch;
+	int ret;
+
+	ret = regmap_read(upboard->regmap, UPBOARD_REG_PLATFORM_ID, &platform_id);
+	if (ret)
+		return ret;
+
+	manufacturer_id = platform_id & 0xff;
+	if (manufacturer_id != AAEON_MANUFACTURER_ID) {
+		dev_dbg(upboard->dev,
+			"unsupported FPGA firmware from manufacturer 0x%02x",
+			manufacturer_id);
+		return -ENODEV;
+	}
+
+	ret = regmap_read(upboard->regmap, UPBOARD_REG_FIRMWARE_ID, &firmware_id);
+	if (ret)
+		return ret;
+
+	build = (firmware_id >> 12) & 0xf;
+	major = (firmware_id >> 8) & 0xf;
+	minor = (firmware_id >> 4) & 0xf;
+	patch = firmware_id & 0xf;
+	if (major != SUPPORTED_FW_MAJOR) {
+		dev_dbg(upboard->dev, "unsupported FPGA firmware v%u.%u.%u.%u",
+			major, minor, patch, build);
+		return -ENODEV;
+	}
+
+	dev_dbg(upboard->dev, "supported FPGA firmware v%u.%u.%u.%u",
+		major, minor, patch, build);
+	return 0;
+}
+
+static const struct acpi_device_id upboard_acpi_match[] = {
+	{ "AANT0F01", (kernel_ulong_t) &upboard_up2_data },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, upboard_acpi_match);
+
+static int upboard_probe(struct platform_device *pdev)
+{
+	struct upboard *upboard;
+	const struct acpi_device_id *id;
+	const struct upboard_data *upboard_data;
+	int ret;
+
+	id = acpi_match_device(upboard_acpi_match, &pdev->dev);
+	if (!id)
+		return -ENODEV;
+
+	upboard_data = (const struct upboard_data *) id->driver_data;
+
+	upboard = devm_kzalloc(&pdev->dev, sizeof(*upboard), GFP_KERNEL);
+	if (!upboard)
+		return -ENOMEM;
+
+	dev_set_drvdata(&pdev->dev, upboard);
+	upboard->dev = &pdev->dev;
+	upboard->regmap = devm_regmap_init(&pdev->dev, NULL, upboard,
+					   upboard_data->regmapconf);
+	if (IS_ERR(upboard->regmap))
+		return PTR_ERR(upboard->regmap);
+
+	ret = upboard_init_gpio(upboard);
+	if (ret && ret != -EPROBE_DEFER)
+		dev_err(&pdev->dev, "failed to init controller GPIOs: %d", ret);
+	if (ret)
+		return ret;
+
+	ret = upboard_check_supported(upboard);
+	if (ret)
+		return ret;
+
+	return devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_AUTO,
+				    upboard_data->cells, upboard_data->ncells,
+				    NULL, 0, NULL);
+}
+
+static struct platform_driver upboard_driver = {
+	.probe = upboard_probe,
+	.driver = {
+		.name = "upboard",
+		.acpi_match_table = upboard_acpi_match,
+	},
+};
+
+module_platform_driver(upboard_driver);
+
+MODULE_AUTHOR("Javier Arteaga <javier@emutex.com>");
+MODULE_DESCRIPTION("UP Board platform controller driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/upboard.h b/include/linux/mfd/upboard.h
new file mode 100644
index 000000000000..d9dd214f4d29
--- /dev/null
+++ b/include/linux/mfd/upboard.h
@@ -0,0 +1,65 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * UP Board MFD driver interface
+ *
+ * Copyright (c) 2018, Emutex Ltd.
+ *
+ * Author: Javier Arteaga <javier@emutex.com>
+ */
+
+#ifndef __LINUX_MFD_UPBOARD_H
+#define __LINUX_MFD_UPBOARD_H
+
+#define UPBOARD_ADDRESS_SIZE  7
+#define UPBOARD_REGISTER_SIZE 16
+#define UPBOARD_READ_FLAG     BIT(UPBOARD_ADDRESS_SIZE)
+
+/**
+ * enum upboard_reg - addresses for 16-bit controller registers
+ *
+ * @UPBOARD_REG_PLATFORM_ID:    [RO] BOARD_ID | MANUFACTURER_ID
+ * @UPBOARD_REG_FIRMWARE_ID:    [RO] BUILD | MAJOR | MINOR | PATCH
+ * @UPBOARD_REG_FUNC_EN0:       [RW] Toggles for board functions (bank 0)
+ * @UPBOARD_REG_FUNC_EN1:       [RW] Toggles for board functions (bank 1)
+ * @UPBOARD_REG_GPIO_EN0:       [RW] Hi-Z (0) / enabled (1) GPIO (bank 0)
+ * @UPBOARD_REG_GPIO_EN1:       [RW] Hi-Z (0) / enabled (1) GPIO (bank 1)
+ * @UPBOARD_REG_GPIO_EN2:       [RW] Hi-Z (0) / enabled (1) GPIO (bank 2)
+ * @UPBOARD_REG_GPIO_DIR0:      [RW] SoC- (0) / FPGA- (1) driven GPIO (bank 0)
+ * @UPBOARD_REG_GPIO_DIR1:      [RW] SoC- (0) / FPGA- (1) driven GPIO (bank 1)
+ * @UPBOARD_REG_GPIO_DIR2:      [RW] SoC- (0) / FPGA- (1) driven GPIO (bank 2)
+ * @UPBOARD_REG_MAX: one past the last valid address
+ */
+enum upboard_reg {
+	UPBOARD_REG_PLATFORM_ID   = 0x10,
+	UPBOARD_REG_FIRMWARE_ID   = 0x11,
+	UPBOARD_REG_FUNC_EN0      = 0x20,
+	UPBOARD_REG_FUNC_EN1      = 0x21,
+	UPBOARD_REG_GPIO_EN0      = 0x30,
+	UPBOARD_REG_GPIO_EN1      = 0x31,
+	UPBOARD_REG_GPIO_EN2      = 0x32,
+	UPBOARD_REG_GPIO_DIR0     = 0x40,
+	UPBOARD_REG_GPIO_DIR1     = 0x41,
+	UPBOARD_REG_GPIO_DIR2     = 0x42,
+	UPBOARD_REG_MAX,
+};
+
+/**
+ * struct upboard - internal data shared by the UP MFD and its child drivers
+ *
+ * @dev: pointer to the MFD device
+ * @regmap: pointer to the regmap of 16-bit control registers
+ * @clear_gpio: descriptor for the CLEAR line on the controller
+ * @strobe_gpio: descriptor for the STROBE line on the controller
+ * @datain_gpio: descriptor for the DATAIN line on the controller
+ * @dataout_gpio: descriptor for the DATAOUT line on the controller
+ */
+struct upboard {
+	struct device *dev;
+	struct regmap *regmap;
+	struct gpio_desc *clear_gpio;
+	struct gpio_desc *strobe_gpio;
+	struct gpio_desc *datain_gpio;
+	struct gpio_desc *dataout_gpio;
+};
+
+#endif /*  __LINUX_MFD_UPBOARD_H */
-- 
2.17.0

[RFC PATCH RESEND 2/3] leds: upboard: Add LED support

[Javier Arteaga]

Allow userspace to use the on-board LEDs as "upboard:<color>:".

Signed-off-by: Javier Arteaga <javier@emutex.com>
---
 drivers/leds/Kconfig        | 10 +++++
 drivers/leds/Makefile       |  1 +
 drivers/leds/leds-upboard.c | 87 +++++++++++++++++++++++++++++++++++++
 drivers/mfd/upboard.c       | 19 ++++++++
 include/linux/mfd/upboard.h |  5 +++
 5 files changed, 122 insertions(+)
 create mode 100644 drivers/leds/leds-upboard.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 2c896c0e69e1..2e77b46f2802 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -722,6 +722,16 @@ config LEDS_NIC78BX
 	  To compile this driver as a module, choose M here: the module
 	  will be called leds-nic78bx.
 
+config LEDS_UPBOARD
+	tristate "LED support for the UP Squared"
+	depends on LEDS_CLASS
+	depends on MFD_UPBOARD
+	help
+	  This option enables support for the LEDs on the UP Squared board.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called leds-upboard.
+
 comment "LED Triggers"
 source "drivers/leds/trigger/Kconfig"
 
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 91eca81cae82..4008e2061e1f 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -76,6 +76,7 @@ obj-$(CONFIG_LEDS_MLXREG)		+= leds-mlxreg.o
 obj-$(CONFIG_LEDS_NIC78BX)		+= leds-nic78bx.o
 obj-$(CONFIG_LEDS_MT6323)		+= leds-mt6323.o
 obj-$(CONFIG_LEDS_LM3692X)		+= leds-lm3692x.o
+obj-$(CONFIG_LEDS_UPBOARD)		+= leds-upboard.o
 
 # LED SPI Drivers
 obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
diff --git a/drivers/leds/leds-upboard.c b/drivers/leds/leds-upboard.c
new file mode 100644
index 000000000000..e9d7a1fbcde4
--- /dev/null
+++ b/drivers/leds/leds-upboard.c
@@ -0,0 +1,87 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * UP Board LED driver
+ *
+ * Copyright (c) 2018, Emutex Ltd.
+ *
+ * Author: Javier Arteaga <javier@emutex.com>
+ */
+
+#include <linux/kernel.h>
+#include <linux/leds.h>
+#include <linux/mfd/upboard.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+struct upboard_led {
+	struct regmap_field *field;
+	struct led_classdev cdev;
+};
+
+static enum led_brightness upboard_led_brightness_get(struct led_classdev *cdev)
+{
+	struct upboard_led *led = container_of(cdev, struct upboard_led, cdev);
+	int brightness = 0;
+
+	regmap_field_read(led->field, &brightness);
+
+	return brightness;
+};
+
+static void upboard_led_brightness_set(struct led_classdev *cdev,
+				       enum led_brightness brightness)
+{
+	struct upboard_led *led = container_of(cdev, struct upboard_led, cdev);
+
+	regmap_field_write(led->field, brightness != LED_OFF);
+};
+
+static int __init upboard_led_probe(struct platform_device *pdev)
+{
+	struct upboard_led_data * const pdata = pdev->dev.platform_data;
+	struct upboard *upboard;
+	struct upboard_led *led;
+	struct reg_field conf = {
+		.reg = UPBOARD_REG_FUNC_EN0,
+	};
+
+	if (!pdev->dev.parent)
+		return -EINVAL;
+
+	upboard = dev_get_drvdata(pdev->dev.parent);
+	if (!upboard || !pdata)
+		return -EINVAL;
+
+	led = devm_kzalloc(&pdev->dev, sizeof(*led), GFP_KERNEL);
+	if (!led)
+		return -ENOMEM;
+
+	conf.lsb = pdata->id;
+	conf.msb = pdata->id;
+	led->field = devm_regmap_field_alloc(&pdev->dev, upboard->regmap, conf);
+	if (IS_ERR(led->field))
+		return PTR_ERR(led->field);
+
+	led->cdev.brightness_get = upboard_led_brightness_get;
+	led->cdev.brightness_set = upboard_led_brightness_set;
+	led->cdev.name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "upboard:%s:",
+					pdata->color);
+	if (!led->cdev.name)
+		return -ENOMEM;
+
+	return devm_led_classdev_register(&pdev->dev, &led->cdev);
+};
+
+static struct platform_driver upboard_led_driver = {
+	.driver = {
+		.name = "upboard-led",
+	},
+};
+
+module_platform_driver_probe(upboard_led_driver, upboard_led_probe);
+
+MODULE_ALIAS("platform:upboard-led");
+MODULE_AUTHOR("Javier Arteaga <javier@emutex.com>");
+MODULE_DESCRIPTION("UP Board LED driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/mfd/upboard.c b/drivers/mfd/upboard.c
index 8bae450cb83d..6e4767e4dc41 100644
--- a/drivers/mfd/upboard.c
+++ b/drivers/mfd/upboard.c
@@ -79,6 +79,14 @@ struct upboard_data {
 	size_t ncells;
 };
 
+#define UPBOARD_LED_CELL(led_data, n)                   \
+	{                                               \
+		.name = "upboard-led",                  \
+		.id = (n),                              \
+		.platform_data = &led_data[(n)],        \
+		.pdata_size = sizeof(*(led_data)),      \
+	}
+
 /* UP Squared */
 
 static const struct regmap_range upboard_up2_readable_ranges[] = {
@@ -116,7 +124,18 @@ static const struct regmap_config upboard_up2_regmap_config = {
 	.wr_table = &upboard_up2_writable_table,
 };
 
+static struct upboard_led_data upboard_up2_led_data[] = {
+	{ .id = 0, .color = "blue" },
+	{ .id = 1, .color = "yellow" },
+	{ .id = 2, .color = "green" },
+	{ .id = 3, .color = "red" },
+};
+
 static const struct mfd_cell upboard_up2_mfd_cells[] = {
+	UPBOARD_LED_CELL(upboard_up2_led_data, 0),
+	UPBOARD_LED_CELL(upboard_up2_led_data, 1),
+	UPBOARD_LED_CELL(upboard_up2_led_data, 2),
+	UPBOARD_LED_CELL(upboard_up2_led_data, 3),
 };
 
 static const struct upboard_data upboard_up2_data = {
diff --git a/include/linux/mfd/upboard.h b/include/linux/mfd/upboard.h
index d9dd214f4d29..eed68caa23ce 100644
--- a/include/linux/mfd/upboard.h
+++ b/include/linux/mfd/upboard.h
@@ -62,4 +62,9 @@ struct upboard {
 	struct gpio_desc *dataout_gpio;
 };
 
+struct upboard_led_data {
+	unsigned int id;
+	const char *color;
+};
+
 #endif /*  __LINUX_MFD_UPBOARD_H */
-- 
2.17.0

[RFC PATCH RESEND 3/3] pinctrl: upboard: Add UP2 pinctrl and gpio driver

[Javier Arteaga]

The UP2 board features a Raspberry Pi compatible pin header (HAT) and a
board-specific expansion connector (EXHAT). Both expose assorted
functions from either the SoC (such as GPIO, I2C, SPI, UART...) or other
on-board devices (ADC, FPGA IP blocks...).

These lines are routed through an on-board FPGA. The platform controller
in its stock firmware provides register fields to change:

- Line enable (FPGA pins enabled / high impedance)
- Line direction (SoC driven / FPGA driven)

To enable using SoC GPIOs on the pin header, this arrangement requires
both configuring the platform controller, and updating the SoC pad
registers in sync.

Add a frontend pinctrl/GPIO driver that registers a new set of GPIO
lines for the header pins. When these are requested, the driver
propagates this request to the backend SoC pinctrl/GPIO driver by
grabbing a GPIO descriptor for the matching SoC GPIO line. The needed
mapping for this is retrieved via ACPI properties.

Signed-off-by: Javier Arteaga <javier@emutex.com>
---

For reference, here's the relevant ASL from the UP2 platform controller.
GPO0..GPO3 are the INT3452 GPIO community controllers on Apollo Lake.

	Device (PCTL)
	{
	    Name (_HID, "AANT0F01")
	    Name (_DDN, "UP Squared FPGA-based Pin Controller")
	    [...]
	    Name (_CRS, ResourceTemplate ()
	    {
	        /*
	         * FPGA SoC GPIO Pins 0-47
	         * These GPIOs are ordered relative to the corresponding
	         * bit position in the FPGA pin direction "DIR" register
	         */
	        GpioIo (Exclusive, PullDefault, 0, 0, IoRestrictionNone, "\\_SB.GPO0", 0) {43}
	        GpioIo (Exclusive, PullDefault, 0, 0, IoRestrictionNone, "\\_SB.GPO0", 0) {42}
	        GpioIo (Exclusive, PullDefault, 0, 0, IoRestrictionNone, "\\_SB.GPO0", 0) {44}
	        [...]
	        GpioIo (Exclusive, PullDefault, 0, 0, IoRestrictionNone, "\\_SB.GPO1", 0) {44}
	    })
	
	    Name (_DSD, Package ()
	    {
	        ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
	        Package ()
	        {
	            Package () { "external-gpios", Package() { ^PCTL,  0, 0, 0,
	                                                       ^PCTL,  1, 0, 0,
	                                                       ^PCTL,  2, 0, 0,
	                                                       [...]
	                                                       ^PCTL, 47, 0, 0 }},
	               Package () { "enable-gpio",  Package() { ^PCTL, 48, 0, 0 }},
	               Package () { "strobe-gpio",  Package() { ^PCTL, 49, 0, 0 }},
	               Package () { "datain-gpio",  Package() { ^PCTL, 50, 0, 0 }},
	               Package () { "clear-gpio",   Package() { ^PCTL, 51, 0, 0 }},
	               Package () { "reset-gpio",   Package() { ^PCTL, 52, 0, 0 }},
	               Package () { "dataout-gpio", Package() { ^PCTL, 53, 0, 0 }},
	        }
	    })

 drivers/mfd/upboard.c             |   1 +
 drivers/pinctrl/Kconfig           |  13 +
 drivers/pinctrl/Makefile          |   1 +
 drivers/pinctrl/pinctrl-upboard.c | 523 ++++++++++++++++++++++++++++++
 4 files changed, 538 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-upboard.c

diff --git a/drivers/mfd/upboard.c b/drivers/mfd/upboard.c
index 6e4767e4dc41..35111981dfdf 100644
--- a/drivers/mfd/upboard.c
+++ b/drivers/mfd/upboard.c
@@ -132,6 +132,7 @@ static struct upboard_led_data upboard_up2_led_data[] = {
 };
 
 static const struct mfd_cell upboard_up2_mfd_cells[] = {
+	{ .name = "upboard-pinctrl" },
 	UPBOARD_LED_CELL(upboard_up2_led_data, 0),
 	UPBOARD_LED_CELL(upboard_up2_led_data, 1),
 	UPBOARD_LED_CELL(upboard_up2_led_data, 2),
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 01fe8e0455a0..a973e9210d4e 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -337,6 +337,19 @@ config PINCTRL_OCELOT
 	select GENERIC_PINMUX_FUNCTIONS
 	select REGMAP_MMIO
 
+config PINCTRL_UPBOARD
+	tristate "UP Squared pinctrl and GPIO driver"
+	depends on ACPI
+	depends on MFD_UPBOARD
+	select PINMUX
+	help
+	  Pinctrl driver for the pin headers on the UP Squared board. It
+	  handles pin control for lines routed through the on-board FPGA and
+	  propagates changes to the SoC pinctrl to keep them in sync.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called pinctrl-upboard.
+
 source "drivers/pinctrl/aspeed/Kconfig"
 source "drivers/pinctrl/bcm/Kconfig"
 source "drivers/pinctrl/berlin/Kconfig"
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 657332b121fb..a88a8be87a0c 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -42,6 +42,7 @@ obj-$(CONFIG_PINCTRL_ZYNQ)	+= pinctrl-zynq.o
 obj-$(CONFIG_PINCTRL_INGENIC)	+= pinctrl-ingenic.o
 obj-$(CONFIG_PINCTRL_RK805)	+= pinctrl-rk805.o
 obj-$(CONFIG_PINCTRL_OCELOT)	+= pinctrl-ocelot.o
+obj-$(CONFIG_PINCTRL_UPBOARD)	+= pinctrl-upboard.o
 
 obj-$(CONFIG_ARCH_ASPEED)	+= aspeed/
 obj-y				+= bcm/
diff --git a/drivers/pinctrl/pinctrl-upboard.c b/drivers/pinctrl/pinctrl-upboard.c
new file mode 100644
index 000000000000..2be6f1a65fe6
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-upboard.c
@@ -0,0 +1,523 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * UP Board pin controller driver
+ *
+ * Copyright (c) 2018, Emutex Ltd.
+ *
+ * Authors: Javier Arteaga <javier@emutex.com>
+ *          Dan O'Donovan <dan@emutex.com>
+ */
+
+#include <linux/acpi.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
+#include <linux/kernel.h>
+#include <linux/mfd/upboard.h>
+#include <linux/module.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/string.h>
+
+#include "core.h"
+
+struct upboard_pin {
+	struct regmap_field *func_en;
+	struct regmap_field *gpio_en;
+	struct regmap_field *gpio_dir;
+};
+
+struct upboard_pinctrl {
+	struct pinctrl_dev *pctldev;
+	struct gpio_chip chip;
+	struct regmap *regmap;
+	unsigned int nsoc_gpios;
+	struct gpio_desc **soc_gpios;
+};
+
+enum upboard_func0_enables {
+	UPBOARD_I2C0_EN = 8,
+	UPBOARD_I2C1_EN = 9,
+};
+
+static const struct reg_field upboard_i2c0_reg =
+	REG_FIELD(UPBOARD_REG_FUNC_EN0, UPBOARD_I2C0_EN, UPBOARD_I2C0_EN);
+
+static const struct reg_field upboard_i2c1_reg =
+	REG_FIELD(UPBOARD_REG_FUNC_EN0, UPBOARD_I2C1_EN, UPBOARD_I2C1_EN);
+
+#define UPBOARD_BIT_TO_PIN(r, bit) \
+	((r) * UPBOARD_REGISTER_SIZE + (bit))
+
+/*
+ * UP Squared data
+ */
+
+#define UPBOARD_UP2_BIT_TO_PIN(r, id) (UPBOARD_BIT_TO_PIN(r, UPBOARD_UP2_##id))
+
+#define UPBOARD_UP2_PIN_ANON(r, bit)					\
+	{								\
+		.number = UPBOARD_BIT_TO_PIN(r, bit),			\
+	}
+
+#define UPBOARD_UP2_PIN_NAME(r, id)					\
+	{								\
+		.number = UPBOARD_UP2_BIT_TO_PIN(r, id),		\
+		.name = #id,						\
+	}
+
+#define UPBOARD_UP2_PIN_FUNC(r, id, data)				\
+	{								\
+		.number = UPBOARD_UP2_BIT_TO_PIN(r, id),		\
+		.name = #id,						\
+		.drv_data = (void *)(data),				\
+	}
+
+enum upboard_up2_reg0_bit {
+	UPBOARD_UP2_UART1_TXD,
+	UPBOARD_UP2_UART1_RXD,
+	UPBOARD_UP2_UART1_RTS,
+	UPBOARD_UP2_UART1_CTS,
+	UPBOARD_UP2_GPIO3,
+	UPBOARD_UP2_GPIO5,
+	UPBOARD_UP2_GPIO6,
+	UPBOARD_UP2_GPIO11,
+	UPBOARD_UP2_EXHAT_LVDS1n,
+	UPBOARD_UP2_EXHAT_LVDS1p,
+	UPBOARD_UP2_SPI2_TXD,
+	UPBOARD_UP2_SPI2_RXD,
+	UPBOARD_UP2_SPI2_FS1,
+	UPBOARD_UP2_SPI2_FS0,
+	UPBOARD_UP2_SPI2_CLK,
+	UPBOARD_UP2_SPI1_TXD,
+};
+
+enum upboard_up2_reg1_bit {
+	UPBOARD_UP2_SPI1_RXD,
+	UPBOARD_UP2_SPI1_FS1,
+	UPBOARD_UP2_SPI1_FS0,
+	UPBOARD_UP2_SPI1_CLK,
+	UPBOARD_UP2_BIT20,
+	UPBOARD_UP2_BIT21,
+	UPBOARD_UP2_BIT22,
+	UPBOARD_UP2_BIT23,
+	UPBOARD_UP2_PWM1,
+	UPBOARD_UP2_PWM0,
+	UPBOARD_UP2_EXHAT_LVDS0n,
+	UPBOARD_UP2_EXHAT_LVDS0p,
+	UPBOARD_UP2_I2C0_SCL,
+	UPBOARD_UP2_I2C0_SDA,
+	UPBOARD_UP2_I2C1_SCL,
+	UPBOARD_UP2_I2C1_SDA,
+};
+
+enum upboard_up2_reg2_bit {
+	UPBOARD_UP2_EXHAT_LVDS3n,
+	UPBOARD_UP2_EXHAT_LVDS3p,
+	UPBOARD_UP2_EXHAT_LVDS4n,
+	UPBOARD_UP2_EXHAT_LVDS4p,
+	UPBOARD_UP2_EXHAT_LVDS5n,
+	UPBOARD_UP2_EXHAT_LVDS5p,
+	UPBOARD_UP2_I2S_SDO,
+	UPBOARD_UP2_I2S_SDI,
+	UPBOARD_UP2_I2S_WS_SYNC,
+	UPBOARD_UP2_I2S_BCLK,
+	UPBOARD_UP2_EXHAT_LVDS6n,
+	UPBOARD_UP2_EXHAT_LVDS6p,
+	UPBOARD_UP2_EXHAT_LVDS7n,
+	UPBOARD_UP2_EXHAT_LVDS7p,
+	UPBOARD_UP2_EXHAT_LVDS2n,
+	UPBOARD_UP2_EXHAT_LVDS2p,
+};
+
+static struct pinctrl_pin_desc upboard_up2_pins[] = {
+	UPBOARD_UP2_PIN_NAME(0, UART1_TXD),
+	UPBOARD_UP2_PIN_NAME(0, UART1_RXD),
+	UPBOARD_UP2_PIN_NAME(0, UART1_RTS),
+	UPBOARD_UP2_PIN_NAME(0, UART1_CTS),
+	UPBOARD_UP2_PIN_NAME(0, GPIO3),
+	UPBOARD_UP2_PIN_NAME(0, GPIO5),
+	UPBOARD_UP2_PIN_NAME(0, GPIO6),
+	UPBOARD_UP2_PIN_NAME(0, GPIO11),
+	UPBOARD_UP2_PIN_NAME(0, EXHAT_LVDS1n),
+	UPBOARD_UP2_PIN_NAME(0, EXHAT_LVDS1p),
+	UPBOARD_UP2_PIN_NAME(0, SPI2_TXD),
+	UPBOARD_UP2_PIN_NAME(0, SPI2_RXD),
+	UPBOARD_UP2_PIN_NAME(0, SPI2_FS1),
+	UPBOARD_UP2_PIN_NAME(0, SPI2_FS0),
+	UPBOARD_UP2_PIN_NAME(0, SPI2_CLK),
+	UPBOARD_UP2_PIN_NAME(0, SPI1_TXD),
+	UPBOARD_UP2_PIN_NAME(1, SPI1_RXD),
+	UPBOARD_UP2_PIN_NAME(1, SPI1_FS1),
+	UPBOARD_UP2_PIN_NAME(1, SPI1_FS0),
+	UPBOARD_UP2_PIN_NAME(1, SPI1_CLK),
+	UPBOARD_UP2_PIN_ANON(1, 4),
+	UPBOARD_UP2_PIN_ANON(1, 5),
+	UPBOARD_UP2_PIN_ANON(1, 6),
+	UPBOARD_UP2_PIN_ANON(1, 7),
+	UPBOARD_UP2_PIN_NAME(1, PWM1),
+	UPBOARD_UP2_PIN_NAME(1, PWM0),
+	UPBOARD_UP2_PIN_NAME(1, EXHAT_LVDS0n),
+	UPBOARD_UP2_PIN_NAME(1, EXHAT_LVDS0p),
+	UPBOARD_UP2_PIN_FUNC(1, I2C0_SCL, &upboard_i2c0_reg),
+	UPBOARD_UP2_PIN_FUNC(1, I2C0_SDA, &upboard_i2c0_reg),
+	UPBOARD_UP2_PIN_FUNC(1, I2C1_SCL, &upboard_i2c1_reg),
+	UPBOARD_UP2_PIN_FUNC(1, I2C1_SDA, &upboard_i2c1_reg),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS3n),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS3p),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS4n),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS4p),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS5n),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS5p),
+	UPBOARD_UP2_PIN_NAME(2, I2S_SDO),
+	UPBOARD_UP2_PIN_NAME(2, I2S_SDI),
+	UPBOARD_UP2_PIN_NAME(2, I2S_WS_SYNC),
+	UPBOARD_UP2_PIN_NAME(2, I2S_BCLK),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS6n),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS6p),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS7n),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS7p),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS2n),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS2p),
+};
+
+static int upboard_get_functions_count(struct pinctrl_dev *pctldev)
+{
+	return 0;
+}
+
+static int upboard_get_function_groups(struct pinctrl_dev *pctldev,
+				       unsigned int selector,
+				       const char * const **groups,
+				       unsigned int *num_groups)
+{
+	*groups = NULL;
+	*num_groups = 0;
+	return 0;
+}
+
+static const char *upboard_get_function_name(struct pinctrl_dev *pctldev,
+					     unsigned int selector)
+{
+	return NULL;
+}
+
+static int upboard_set_mux(struct pinctrl_dev *pctldev, unsigned int function,
+			   unsigned int group)
+{
+	return 0;
+};
+
+static int upboard_gpio_request_enable(struct pinctrl_dev *pctldev,
+				       struct pinctrl_gpio_range *range,
+				       unsigned int pin)
+{
+	const struct pin_desc * const pd = pin_desc_get(pctldev, pin);
+	const struct upboard_pin *p;
+	int ret;
+
+	if (!pd)
+		return -EINVAL;
+	p = pd->drv_data;
+
+	/* if this pin has an associated function bit, disable it first */
+	if (p->func_en) {
+		ret = regmap_field_write(p->func_en, 0);
+		if (ret)
+			return ret;
+	}
+
+	if (p->gpio_en) {
+		ret = regmap_field_write(p->gpio_en, 1);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+};
+
+static int upboard_gpio_set_direction(struct pinctrl_dev *pctldev,
+				      struct pinctrl_gpio_range *range,
+				      unsigned int pin, bool input)
+{
+	const struct pin_desc * const pd = pin_desc_get(pctldev, pin);
+	const struct upboard_pin *p;
+
+	if (!pd)
+		return -EINVAL;
+	p = pd->drv_data;
+
+	return regmap_field_write(p->gpio_dir, input);
+};
+
+static const struct pinmux_ops upboard_pinmux_ops = {
+	.get_functions_count = upboard_get_functions_count,
+	.get_function_groups = upboard_get_function_groups,
+	.get_function_name = upboard_get_function_name,
+	.set_mux = upboard_set_mux,
+	.gpio_request_enable = upboard_gpio_request_enable,
+	.gpio_set_direction = upboard_gpio_set_direction,
+};
+
+static int upboard_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	return 0;
+}
+
+static const char *upboard_get_group_name(struct pinctrl_dev *pctldev,
+					  unsigned int selector)
+{
+	return NULL;
+}
+
+static const struct pinctrl_ops upboard_pinctrl_ops = {
+	.get_groups_count = upboard_get_groups_count,
+	.get_group_name = upboard_get_group_name,
+};
+
+static struct pinctrl_desc upboard_up2_pinctrl_desc = {
+	.pins = upboard_up2_pins,
+	.npins = ARRAY_SIZE(upboard_up2_pins),
+	.pctlops = &upboard_pinctrl_ops,
+	.pmxops = &upboard_pinmux_ops,
+	.owner = THIS_MODULE,
+};
+
+static struct gpio_desc *upboard_offset_to_soc_gpio(struct gpio_chip *gc,
+						    unsigned int offset)
+{
+	struct upboard_pinctrl *pctrl =
+		container_of(gc, struct upboard_pinctrl, chip);
+
+	if (offset + 1 > pctrl->nsoc_gpios || !pctrl->soc_gpios[offset])
+		return ERR_PTR(-ENODEV);
+
+	return pctrl->soc_gpios[offset];
+}
+
+static int upboard_gpio_request(struct gpio_chip *gc, unsigned int offset)
+{
+	struct upboard_pinctrl *pctrl =
+		container_of(gc, struct upboard_pinctrl, chip);
+	struct gpio_desc *desc;
+	int ret;
+
+	ret = pinctrl_gpio_request(gc->base + offset);
+	if (ret)
+		return ret;
+
+	desc = devm_gpiod_get_index(gc->parent, "external", offset, GPIOD_ASIS);
+	if (IS_ERR(desc))
+		return PTR_ERR(desc);
+
+	pctrl->soc_gpios[offset] = desc;
+	return 0;
+}
+
+static void upboard_gpio_free(struct gpio_chip *gc, unsigned int offset)
+{
+	struct upboard_pinctrl *pctrl =
+		container_of(gc, struct upboard_pinctrl, chip);
+
+	if (offset + 1 > pctrl->nsoc_gpios || !pctrl->soc_gpios[offset])
+		return;
+
+	devm_gpiod_put(gc->parent, pctrl->soc_gpios[offset]);
+	pctrl->soc_gpios[offset] = NULL;
+
+	pinctrl_gpio_free(gc->base + offset);
+}
+
+static int upboard_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+	struct gpio_desc *desc = upboard_offset_to_soc_gpio(gc, offset);
+
+	if (IS_ERR(desc))
+		return PTR_ERR(desc);
+
+	return gpiod_get_direction(desc);
+}
+
+static int upboard_gpio_direction_input(struct gpio_chip *gc,
+					unsigned int offset)
+{
+	struct gpio_desc *desc = upboard_offset_to_soc_gpio(gc, offset);
+	int ret;
+
+	if (IS_ERR(desc))
+		return PTR_ERR(desc);
+
+	ret = gpiod_direction_input(desc);
+	if (ret)
+		return ret;
+
+	return pinctrl_gpio_direction_input(gc->base + offset);
+}
+
+static int upboard_gpio_direction_output(struct gpio_chip *gc,
+					 unsigned int offset, int value)
+{
+	struct gpio_desc *desc = upboard_offset_to_soc_gpio(gc, offset);
+	int ret;
+
+	if (IS_ERR(desc))
+		return PTR_ERR(desc);
+
+	ret = pinctrl_gpio_direction_output(gc->base + offset);
+	if (ret)
+		return ret;
+
+	return gpiod_direction_output(desc, value);
+}
+
+static int upboard_gpio_get_value(struct gpio_chip *gc, unsigned int offset)
+{
+	struct gpio_desc *desc = upboard_offset_to_soc_gpio(gc, offset);
+
+	if (IS_ERR(desc))
+		return PTR_ERR(desc);
+
+	return gpiod_get_value(desc);
+}
+
+static void upboard_gpio_set_value(struct gpio_chip *gc, unsigned int offset,
+				   int value)
+{
+	struct gpio_desc *desc = upboard_offset_to_soc_gpio(gc, offset);
+
+	if (IS_ERR(desc))
+		return;
+
+	gpiod_set_value(desc, value);
+}
+
+static struct gpio_chip upboard_gpio_chip = {
+	.label = "UP pin controller",
+	.owner = THIS_MODULE,
+	.request = upboard_gpio_request,
+	.free = upboard_gpio_free,
+	.get_direction = upboard_gpio_get_direction,
+	.direction_input = upboard_gpio_direction_input,
+	.direction_output = upboard_gpio_direction_output,
+	.get = upboard_gpio_get_value,
+	.set = upboard_gpio_set_value,
+	.base = -1,
+};
+
+static struct regmap_field * __init upboard_field_alloc(struct device *dev,
+							struct regmap *regmap,
+							unsigned int base,
+							unsigned int number)
+{
+	const unsigned int reg = number / UPBOARD_REGISTER_SIZE;
+	const unsigned int bit = number % UPBOARD_REGISTER_SIZE;
+	const struct reg_field field = {
+		.reg = base + reg,
+		.msb = bit,
+		.lsb = bit,
+	};
+
+	return devm_regmap_field_alloc(dev, regmap, field);
+}
+
+static int __init upboard_pinctrl_probe(struct platform_device *pdev)
+{
+	struct acpi_device * const adev = ACPI_COMPANION(&pdev->dev);
+	struct upboard *upboard;
+	struct pinctrl_desc *pctldesc;
+	struct upboard_pinctrl *pctrl;
+	struct upboard_pin *pins;
+	unsigned int i;
+	int ret;
+
+	if (!adev)
+		return -ENODEV;
+
+	if (!pdev->dev.parent)
+		return -EINVAL;
+
+	upboard = dev_get_drvdata(pdev->dev.parent);
+	if (!upboard)
+		return -EINVAL;
+
+	if (strcmp(acpi_device_hid(adev), "AANT0F01"))
+		return -ENODEV;
+
+	pctldesc = &upboard_up2_pinctrl_desc;
+	pctldesc->name = dev_name(&pdev->dev);
+
+	pins = devm_kzalloc(&pdev->dev, sizeof(*pins) * pctldesc->npins, GFP_KERNEL);
+	if (!pins)
+		return -ENOMEM;
+
+	for (i = 0; i < pctldesc->npins; i++) {
+		struct upboard_pin *pin = &pins[i];
+		const struct pinctrl_pin_desc *pd = &pctldesc->pins[i];
+
+		pin->func_en = NULL;
+		if (pd->drv_data) {
+			struct reg_field *field = pd->drv_data;
+
+			pin->func_en = devm_regmap_field_alloc(&pdev->dev,
+							       upboard->regmap,
+							       *field);
+			if (IS_ERR(pin->func_en))
+				return PTR_ERR(pin->func_en);
+		}
+
+		pin->gpio_en = upboard_field_alloc(&pdev->dev, upboard->regmap,
+						   UPBOARD_REG_GPIO_EN0, i);
+		if (IS_ERR(pin->gpio_en))
+			return PTR_ERR(pin->gpio_en);
+
+		pin->gpio_dir = upboard_field_alloc(&pdev->dev, upboard->regmap,
+						    UPBOARD_REG_GPIO_DIR0, i);
+		if (IS_ERR(pin->gpio_dir))
+			return PTR_ERR(pin->gpio_dir);
+
+		((struct pinctrl_pin_desc *)pd)->drv_data = pin;
+	}
+
+	pctrl = devm_kzalloc(&pdev->dev, sizeof(*pctrl), GFP_KERNEL);
+	if (!pctrl)
+		return -ENOMEM;
+
+	pctrl->regmap = upboard->regmap;
+	pctrl->chip = upboard_gpio_chip;
+	pctrl->chip.parent = &pdev->dev;
+	pctrl->chip.ngpio = pctldesc->npins;
+
+	pctrl->nsoc_gpios = gpiod_count(&pdev->dev, "external");
+	pctrl->soc_gpios = devm_kzalloc(&pdev->dev,
+					pctrl->nsoc_gpios * sizeof(*pctrl->soc_gpios),
+					GFP_KERNEL);
+	if (!pctrl->soc_gpios)
+		return -ENOMEM;
+
+	pctrl->pctldev = devm_pinctrl_register(&pdev->dev, pctldesc, pctrl);
+	if (IS_ERR(pctrl->pctldev))
+		return PTR_ERR(pctrl->pctldev);
+
+	ret = devm_gpiochip_add_data(&pdev->dev, &pctrl->chip, &pctrl->chip);
+	if (ret)
+		return ret;
+
+	return gpiochip_add_pin_range(&pctrl->chip, dev_name(&pdev->dev), 0, 0,
+				      pctldesc->npins);
+}
+
+static struct platform_driver upboard_pinctrl_driver = {
+	.driver = {
+		.name = "upboard-pinctrl",
+	},
+};
+
+module_platform_driver_probe(upboard_pinctrl_driver, upboard_pinctrl_probe);
+
+MODULE_ALIAS("platform:upboard-pinctrl");
+MODULE_AUTHOR("Javier Arteaga <javier@emutex.com>");
+MODULE_AUTHOR("Dan O'Donovan <dan@emutex.com>");
+MODULE_DESCRIPTION("UP Board pin control and GPIO driver");
+MODULE_LICENSE("GPL");
-- 
2.17.0

Re: [RFC PATCH RESEND 2/3] leds: upboard: Add LED support

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1123 bytes --]

On Sat 2018-04-21 09:50:08, Javier Arteaga wrote:
> Allow userspace to use the on-board LEDs as "upboard:<color>:".
> 
> Signed-off-by: Javier Arteaga <javier@emutex.com>

> +static enum led_brightness upboard_led_brightness_get(struct led_classdev *cdev)
> +{
> +	struct upboard_led *led = container_of(cdev, struct upboard_led, cdev);
> +	int brightness = 0;
> +
> +	regmap_field_read(led->field, &brightness);
> +
> +	return brightness;
> +};

I'm slightly confused here. Is each led controlled by single bit?

> +static void upboard_led_brightness_set(struct led_classdev *cdev,
> +				       enum led_brightness brightness)
> +{
> +	struct upboard_led *led = container_of(cdev, struct upboard_led, cdev);
> +
> +	regmap_field_write(led->field, brightness != LED_OFF);
> +};

What is going on with ";" at end of function? We don't do that.

If it is single bit, max_brightness should be one, and != LED_OFF test
should not be needed.

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [RFC PATCH RESEND 2/3] leds: upboard: Add LED support

[Javier Arteaga]

Hi Pavel,

On Wed, Apr 25, 2018 at 08:41:33AM +0200, Pavel Machek wrote:
> On Sat 2018-04-21 09:50:08, Javier Arteaga wrote:
> > Allow userspace to use the on-board LEDs as "upboard:<color>:".
> > 
> > Signed-off-by: Javier Arteaga <javier@emutex.com>
> 
> > +static enum led_brightness upboard_led_brightness_get(struct led_classdev *cdev)
> > +{
> > +	struct upboard_led *led = container_of(cdev, struct upboard_led, cdev);
> > +	int brightness = 0;
> > +
> > +	regmap_field_read(led->field, &brightness);
> > +
> > +	return brightness;
> > +};
> 
> I'm slightly confused here. Is each led controlled by single bit?

Yes. Each regmap field is one bit wide.

> What is going on with ";" at end of function? We don't do that.

Huh, I totally missed this. It's all throughout the patchset too, and
it's going away in v2 of course.

> If it is single bit, max_brightness should be one, and != LED_OFF test
> should not be needed.

Got it. Will do for v2.

Thanks for your time!

Re: [RFC PATCH RESEND 2/3] leds: upboard: Add LED support

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 373 bytes --]

Hi!

> > If it is single bit, max_brightness should be one, and != LED_OFF test
> > should not be needed.
> 
> Got it. Will do for v2.

Good, you can add

Acked-by: Pavel Machek <pavel@ucw.cz>

for the v2.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [RFC PATCH RESEND 1/3] mfd: upboard: Add UP2 platform controller driver

[Mika Westerberg]

On Sat, Apr 21, 2018 at 09:50:07AM +0100, Javier Arteaga wrote:
> +static const struct acpi_device_id upboard_acpi_match[] = {
> +	{ "AANT0F01", (kernel_ulong_t) &upboard_up2_data },

No space after cast so write it like,

	{ "AANT0F01", (kernel_ulong_t)&upboard_up2_data },


> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, upboard_acpi_match);
> +
> +static int upboard_probe(struct platform_device *pdev)
> +{
> +	struct upboard *upboard;
> +	const struct acpi_device_id *id;
> +	const struct upboard_data *upboard_data;
> +	int ret;
> +
> +	id = acpi_match_device(upboard_acpi_match, &pdev->dev);
> +	if (!id)
> +		return -ENODEV;
> +
> +	upboard_data = (const struct upboard_data *) id->driver_data;

Ditto and to other places.

Otherwise looks good to me.

Re: [RFC PATCH RESEND 0/3] UP Squared board drivers

[Mika Westerberg]

On Sat, Apr 21, 2018 at 09:50:06AM +0100, Javier Arteaga wrote:
> [linux-kernel correctly Cc'd on the series now. Sorry for the noise.]
> 
> Hi all,
> 
> This series adds platform support for AAEON's UP Squared, a x86 SBC
> based on Apollo Lake [1]. Specifically, it enables control for the board
> pin headers and for the LEDs, as both of these features are only
> available behind a FPGA-based platform controller.
> 
> This is structured around a MFD driver that provides a regmap interface
> to that platform controller to each MFD cell.
> 
> The series is carved out from an out-of-tree support patchset, initially
> written by Dan O'Donovan for the original UP Board [2] and then extended
> through the past year to support new entries in the lineup (UP Board, UP
> Squared and UP Core) [3].
> 
> Here we only submit support for UP Squared to simplify review. Still,
> the driver is designed so that it can easily gain support for the other
> boards.
> 
> Questions:
> 
> * Is MFD the right fit for the platform controller?
>   The intention here is to encapsulate the custom GPIO-bitbanged control
>   protocol by sharing the regmap between the drivers.
> 
> * Is our use of two chained pinctrls (in patch 3/3) valid?

There are few minor comments but other than that, to me this looks like
a reasonable approach.

Re: [RFC PATCH RESEND 1/3] mfd: upboard: Add UP2 platform controller driver

[Javier Arteaga]

Hi Mika,

On Wed, Apr 25, 2018 at 12:51:41PM +0300, Mika Westerberg wrote:
> On Sat, Apr 21, 2018 at 09:50:07AM +0100, Javier Arteaga wrote:
> > +static const struct acpi_device_id upboard_acpi_match[] = {
> > +	{ "AANT0F01", (kernel_ulong_t) &upboard_up2_data },
> 
> No space after cast so write it like,
> 
> 	{ "AANT0F01", (kernel_ulong_t)&upboard_up2_data },

> > +	upboard_data = (const struct upboard_data *) id->driver_data;
> 
> Ditto and to other places.

Thanks! I've fixed it for the next series now.

Actually, checkpatch did not catch this one, and I can't see an explicit
mention to this rule in coding-style.rst either. Is this worth sending a
patch to the style guide?

> Otherwise looks good to me.

Thanks for your review!

Re: [RFC PATCH RESEND 1/3] mfd: upboard: Add UP2 platform controller driver

[Andy Shevchenko]

On Sat, 2018-04-21 at 09:50 +0100, Javier Arteaga wrote:
> UP Squared (UP2) is a x86 SBC from AAEON based on Intel Apollo Lake. 

> +config MFD_UPBOARD
> +	tristate "UP Squared"
> +	depends on ACPI
> +	depends on GPIOLIB
> +	select MFD_CORE
> +	select REGMAP
> +	help
> +	  If you say yes here you get support for the platform
> controller
> +	  of the UP Squared single-board computer.
> +
> +	  This driver provides common support for accessing the
> device,
> +	  additional drivers must be enabled in order to use the
> +	  functionality of the device.
> +
> +	  This driver can also be built as a module. If so, the
> module
> +	  will be called upboard.

"upboard"

> 
> +static int upboard_read(void *context, unsigned int reg, unsigned int
> *val)
> +{
> +	const struct upboard * const upboard = context;
> +	int i;
> +
> +	gpiod_set_value(upboard->clear_gpio, 0);
> +	gpiod_set_value(upboard->clear_gpio, 1);
> +
> +	reg |= UPBOARD_READ_FLAG;
> +
> +	for (i = UPBOARD_ADDRESS_SIZE; i >= 0; i--) {
> +		gpiod_set_value(upboard->strobe_gpio, 0);
> +		gpiod_set_value(upboard->datain_gpio, (reg >> i) &
> 0x1);
> +		gpiod_set_value(upboard->strobe_gpio, 1);
> +	}
> +

> +	gpiod_set_value(upboard->strobe_gpio, 0);
> +	*val = 0;
> +
> +	for (i = UPBOARD_REGISTER_SIZE - 1; i >= 0; i--) {
> +		gpiod_set_value(upboard->strobe_gpio, 1);
> +		gpiod_set_value(upboard->strobe_gpio, 0);
> +		*val |= gpiod_get_value(upboard->dataout_gpio) << i;
> +	}
> +
> +	gpiod_set_value(upboard->strobe_gpio, 1);

Can't you rewrite this like

for (addr) {
 strobe(0)
 data(x)
 strobe(1)
}

for (register) {
 strobe(0)
 val = data(x)
 strobe(1)
}

val &= BIT(register_size);
strobe(0)

?

> +
> +	return 0;
> +};
> +
> +static int upboard_write(void *context, unsigned int reg, unsigned
> int val)
> +{
> +	const struct upboard * const upboard = context;
> +	int i;
> +
> +	gpiod_set_value(upboard->clear_gpio, 0);
> +	gpiod_set_value(upboard->clear_gpio, 1);
> +
> +	for (i = UPBOARD_ADDRESS_SIZE; i >= 0; i--) {
> +		gpiod_set_value(upboard->strobe_gpio, 0);
> +		gpiod_set_value(upboard->datain_gpio, (reg >> i) &
> 0x1);
> +		gpiod_set_value(upboard->strobe_gpio, 1);
> +	}
> +
> +	gpiod_set_value(upboard->strobe_gpio, 0);
> +
> +	for (i = UPBOARD_REGISTER_SIZE - 1; i >= 0; i--) {
> +		gpiod_set_value(upboard->datain_gpio, (val >> i) &
> 0x1);
> +		gpiod_set_value(upboard->strobe_gpio, 1);
> +		gpiod_set_value(upboard->strobe_gpio, 0);
> +	}
> +
> +	gpiod_set_value(upboard->strobe_gpio, 1);

Similar here:

for (addr) {
 strobe(0)
 data(x)
 strobe(1)
}

for (register) {
 strobe(0)
 data(x)
 strobe(1)
}

strobe(0)
strobe(1)

?

> +
> +	return 0;
> +};

Moreover these two functions have duplications, i.e.

static ... upboard_clear()
{
 clear(0)
 clear(1)
}

static ... upboard_set_address()
{
 for (addr) {
  ...
 }
}


Additional question is about spi-bitbang and i2c-gpio. Can one of them
be utilized here? Why not?

> +struct upboard_data {
> +	const struct regmap_config *regmapconf;
> +	const struct mfd_cell *cells;
> +	size_t ncells;
> +};

> +static int upboard_init_gpio(struct upboard *upboard)
> +{
> +	struct gpio_desc *enable_gpio;
> +
> +	enable_gpio = devm_gpiod_get(upboard->dev, "enable",
> GPIOD_OUT_LOW);
> +	if (IS_ERR(enable_gpio))
> +		return PTR_ERR(enable_gpio);

> +	gpiod_set_value(enable_gpio, 1);

When do you disable it? Why not?

> +	return 0;
> +}

> +
> +static int upboard_check_supported(struct upboard *upboard)
> +{

> +	const unsigned int AAEON_MANUFACTURER_ID = 0x01;
> +	const unsigned int SUPPORTED_FW_MAJOR = 0x0;

Why to hide here instead of putting at the top of file as defined
constants?

> +	unsigned int platform_id, manufacturer_id;
> +	unsigned int firmware_id, build, major, minor, patch;
> +	int ret;

> +	build = (firmware_id >> 12) & 0xf;
> +	major = (firmware_id >> 8) & 0xf;
> +	minor = (firmware_id >> 4) & 0xf;

> +	patch = firmware_id & 0xf;

For style purposes you can use
(firmware >> 0) & 0xf here

> +static int upboard_probe(struct platform_device *pdev)
> +{
> +	struct upboard *upboard;
> +	const struct acpi_device_id *id;
> +	const struct upboard_data *upboard_data;
> +	int ret;

> +	id = acpi_match_device(upboard_acpi_match, &pdev->dev);
> +	if (!id)
> +		return -ENODEV;
> +
> +	upboard_data = (const struct upboard_data *) id->driver_data;

Use new API for that.

> +MODULE_LICENSE("GPL");

License mismatch.

> +#define UPBOARD_ADDRESS_SIZE  7
> +#define UPBOARD_REGISTER_SIZE 16

> +#define UPBOARD_READ_FLAG     BIT(UPBOARD_ADDRESS_SIZE)

It's not clear why this one is defined in this way.
Comment is needed.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [RFC PATCH RESEND 2/3] leds: upboard: Add LED support

[Andy Shevchenko]

On Sat, 2018-04-21 at 09:50 +0100, Javier Arteaga wrote:
> Allow userspace to use the on-board LEDs as "upboard:<color>:".

> +	struct upboard_led *led = container_of(cdev, struct
> upboard_led, cdev);

#define to_upboard_led(cdev) container_of(cdev, struct upboard_led,
cdev)

... led = to_upboard_led(cdev);

> +	struct upboard_led *led = container_of(cdev, struct
> upboard_led, cdev);

Ditto.

> +static int __init upboard_led_probe(struct platform_device *pdev)

Are you sure about __init here?

> +	struct upboard_led_data * const pdata = pdev-
> >dev.platform_data;

Don't use direct dereference to platform_data.

> +	if (!pdev->dev.parent)
> +		return -EINVAL;
> +
> +	upboard = dev_get_drvdata(pdev->dev.parent);
> +	if (!upboard || !pdata)
> +		return -EINVAL;

Wouldn't be better to supply regmap as part of platform data?
It will allow be flexible of parent device.

> +MODULE_LICENSE("GPL");

License mismatch.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [RFC PATCH RESEND 3/3] pinctrl: upboard: Add UP2 pinctrl and gpio driver

[Andy Shevchenko]

On Sat, 2018-04-21 at 09:50 +0100, Javier Arteaga wrote:
> The UP2 board features a Raspberry Pi compatible pin header (HAT) and
> a
> board-specific expansion connector (EXHAT). Both expose assorted
> functions from either the SoC (such as GPIO, I2C, SPI, UART...) or
> other
> on-board devices (ADC, FPGA IP blocks...).
> 
> These lines are routed through an on-board FPGA. The platform
> controller
> in its stock firmware provides register fields to change:
> 
> - Line enable (FPGA pins enabled / high impedance)
> - Line direction (SoC driven / FPGA driven)
> 
> To enable using SoC GPIOs on the pin header, this arrangement requires
> both configuring the platform controller, and updating the SoC pad
> registers in sync.
> 
> Add a frontend pinctrl/GPIO driver that registers a new set of GPIO
> lines for the header pins. When these are requested, the driver
> propagates this request to the backend SoC pinctrl/GPIO driver by
> grabbing a GPIO descriptor for the matching SoC GPIO line. The needed
> mapping for this is retrieved via ACPI properties.
> 
> 

> For reference, here's the relevant ASL from the UP2 platform
> controller.

It should be in Documentation file or in commit message.


>  static const struct mfd_cell upboard_up2_mfd_cells[] = {
> +	{ .name = "upboard-pinctrl" },

I guess it should be 3 lines.

>  	UPBOARD_LED_CELL(upboard_up2_led_data, 0),
>  	UPBOARD_LED_CELL(upboard_up2_led_data, 1),
>  	UPBOARD_LED_CELL(upboard_up2_led_data, 2),

...and honestly I would not use this macro and put 4 cells explicitly
here.

> +static int upboard_gpio_request_enable(struct pinctrl_dev *pctldev,
> +				       struct pinctrl_gpio_range
> *range,
> +				       unsigned int pin)
> +{
> +	const struct pin_desc * const pd = pin_desc_get(pctldev,
> pin);
> +	const struct upboard_pin *p;
> +	int ret;
> +

> +	if (!pd)
> +		return -EINVAL;

When it's possible?

> +	p = pd->drv_data;

> +	return 0;
> +};

> +	if (!pd)
> +		return -EINVAL;

Ditto.

> +	struct upboard_pinctrl *pctrl =
> +		container_of(gc, struct upboard_pinctrl, chip);

Do define and use to_upboard_pinctrl().

> +	if (offset + 1 > pctrl->nsoc_gpios || !pctrl-
> >soc_gpios[offset])
> +		return ERR_PTR(-ENODEV);

When this is a case?

> +static int upboard_gpio_get_direction(struct gpio_chip *gc, unsigned
> int offset)
> +{
> +	struct gpio_desc *desc = upboard_offset_to_soc_gpio(gc,
> offset);
> +

Split above to definition and assignment pieces. Put assignment
immediately before condition.

> +	if (IS_ERR(desc))
> +		return PTR_ERR(desc);


> +
> +	return gpiod_get_direction(desc);
> +}

> +static struct regmap_field * __init upboard_field_alloc(struct device
> *dev,
> +							struct regmap
> *regmap,
> +							unsigned int
> base,
> +							unsigned int
> number)

You should really understand what __init means and when it's appropriate
to use it.

> +static int __init upboard_pinctrl_probe(struct platform_device *pdev)
> +{
> +	struct acpi_device * const adev = ACPI_COMPANION(&pdev->dev);

Huh, const in that place? Why?

> +	if (!pdev->dev.parent)
> +		return -EINVAL;
> +
> +	upboard = dev_get_drvdata(pdev->dev.parent);
> +	if (!upboard)
> +		return -EINVAL;

Same comment as per LED driver.

> +	if (strcmp(acpi_device_hid(adev), "AANT0F01"))
> +		return -ENODEV;

Huh?

> +		((struct pinctrl_pin_desc *)pd)->drv_data = pin;

What is that?! I mean ugly casting.

> +	}

> +}

> +MODULE_LICENSE("GPL");

License mismatch.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [RFC PATCH RESEND 1/3] mfd: upboard: Add UP2 platform controller driver

[Javier Arteaga]

Hi Andy,

First off, many thanks for your thorough review! Replies inline.

On Wed, Apr 25, 2018 at 06:57:30PM +0300, Andy Shevchenko wrote:
> > +config MFD_UPBOARD
> > +	tristate "UP Squared"
> > +	depends on ACPI
> > +	depends on GPIOLIB
> > +	select MFD_CORE
> > +	select REGMAP
> > +	help
> > +	  If you say yes here you get support for the platform
> > controller
> > +	  of the UP Squared single-board computer.
> > +
> > +	  This driver provides common support for accessing the
> > device,
> > +	  additional drivers must be enabled in order to use the
> > +	  functionality of the device.
> > +
> > +	  This driver can also be built as a module. If so, the
> > module
> > +	  will be called upboard.
> 
> "upboard"

The module name in quotes reads better to me too, but the majority of
Kconfig entries do it this way, looks like:

linux $ rg 'module will be called [^"]' | wc -l
1275
linux $ rg 'module will be called "' | wc -l
5

> > +static int upboard_read(void *context, unsigned int reg, unsigned int
> > *val)
> Can't you rewrite this like
> 
> for (addr) {
>  strobe(0)
>  data(x)
>  strobe(1)
> }
> 
> for (register) {
>  strobe(0)
>  val = data(x)
>  strobe(1)
> }
> 
> val &= BIT(register_size);
> strobe(0)
> 
> ?

> > +static int upboard_write(void *context, unsigned int reg, unsigned
> > int val)
> Similar here:
> 
> for (addr) {
>  strobe(0)
>  data(x)
>  strobe(1)
> }
> 
> for (register) {
>  strobe(0)
>  data(x)
>  strobe(1)
> }
> 
> strobe(0)
> strobe(1)
> 
> ?

> Moreover these two functions have duplications, i.e.
> 
> static ... upboard_clear()
> {
>  clear(0)
>  clear(1)
> }
> 
> static ... upboard_set_address()
> {
>  for (addr) {
>   ...
>  }
> }

I'll look into making these R/W functions more compact.

> Additional question is about spi-bitbang and i2c-gpio. Can one of them
> be utilized here? Why not?

i2c-gpio would be closest, but unfortunately this isn't quite I2C:

- two in/out GPIOs instead of a single SDA line,
- R/W sequence start is signaled from yet _another_ line,
- ACK implicit in last rising edge of the address instead of an ACK pulse,
- etc...

Probably should explain this in a comment too.

> > +static int upboard_init_gpio(struct upboard *upboard)
> > +{
> > +	struct gpio_desc *enable_gpio;
> > +
> > +	enable_gpio = devm_gpiod_get(upboard->dev, "enable",
> > GPIOD_OUT_LOW);
> > +	if (IS_ERR(enable_gpio))
> > +		return PTR_ERR(enable_gpio);
> 
> > +	gpiod_set_value(enable_gpio, 1);
> 
> When do you disable it? Why not?

enable = 0/1 sets all FPGA pins for FPGA-routed lines Hi-Z/active. It's
probably safer to set enable = 0 on unload.

I'll go over this again (and add comments in any case).

> > +static int upboard_check_supported(struct upboard *upboard)
> > +{
> 
> > +	const unsigned int AAEON_MANUFACTURER_ID = 0x01;
> > +	const unsigned int SUPPORTED_FW_MAJOR = 0x0;
> 
> Why to hide here instead of putting at the top of file as defined
> constants?

No strong reason. I'll move them to the top.

> > +	build = (firmware_id >> 12) & 0xf;
> > +	major = (firmware_id >> 8) & 0xf;
> > +	minor = (firmware_id >> 4) & 0xf;
> 
> > +	patch = firmware_id & 0xf;
> 
> For style purposes you can use
> (firmware >> 0) & 0xf here

Sure, why not.

> > +static int upboard_probe(struct platform_device *pdev)
> > +{
> > +	struct upboard *upboard;
> > +	const struct acpi_device_id *id;
> > +	const struct upboard_data *upboard_data;
> > +	int ret;
> 
> > +	id = acpi_match_device(upboard_acpi_match, &pdev->dev);
> > +	if (!id)
> > +		return -ENODEV;
> > +
> > +	upboard_data = (const struct upboard_data *) id->driver_data;
> 
> Use new API for that.

Will do.

> > +MODULE_LICENSE("GPL");
> 
> License mismatch.

True, it should read "GPL v2". I'll update these.

> > +#define UPBOARD_ADDRESS_SIZE  7
> > +#define UPBOARD_REGISTER_SIZE 16
> 
> > +#define UPBOARD_READ_FLAG     BIT(UPBOARD_ADDRESS_SIZE)
> 
> It's not clear why this one is defined in this way.
> Comment is needed.

It means that the RW flag comes after the last bit of the address, like
in I2C. I'll likely drop this #define and make this clearer next
iteration.

Re: [RFC PATCH RESEND 2/3] leds: upboard: Add LED support

[Javier Arteaga]

On Wed, Apr 25, 2018 at 07:15:45PM +0300, Andy Shevchenko wrote:
> On Sat, 2018-04-21 at 09:50 +0100, Javier Arteaga wrote:
> > Allow userspace to use the on-board LEDs as "upboard:<color>:".
> 
> > +	struct upboard_led *led = container_of(cdev, struct
> > upboard_led, cdev);
> 
> #define to_upboard_led(cdev) container_of(cdev, struct upboard_led,
> cdev)
> 
> ... led = to_upboard_led(cdev);
> 
> > +	struct upboard_led *led = container_of(cdev, struct
> > upboard_led, cdev);
> 
> Ditto.

Will do - thanks!

> > +static int __init upboard_led_probe(struct platform_device *pdev)
> 
> Are you sure about __init here?

Not 100% now :)

My understanding was that in this context, __init allows this probe() to
be dropped from memory after module load.

What am I doing wrong there?

> > +	struct upboard_led_data * const pdata = pdev-
> > >dev.platform_data;
> 
> Don't use direct dereference to platform_data.

Sorry, I don't understand this one. What's the alternative?

> > +	if (!pdev->dev.parent)
> > +		return -EINVAL;
> > +
> > +	upboard = dev_get_drvdata(pdev->dev.parent);
> > +	if (!upboard || !pdata)
> > +		return -EINVAL;
> 
> Wouldn't be better to supply regmap as part of platform data?
> It will allow be flexible of parent device.

I'll play around with that.

> > +MODULE_LICENSE("GPL");
> 
> License mismatch.

Will fix.

Re: [RFC PATCH RESEND 3/3] pinctrl: upboard: Add UP2 pinctrl and gpio driver

[Javier Arteaga]

On Wed, Apr 25, 2018 at 07:49:12PM +0300, Andy Shevchenko wrote:
> > For reference, here's the relevant ASL from the UP2 platform
> > controller.
> 
> It should be in Documentation file or in commit message.

Perfect, I just wasn't sure whether dumping ASL this way would be
acceptable in commit history. In that case, I think I prefer the commit
body over keeping a Documentation/ file elsewhere that's easy to miss.

> >  static const struct mfd_cell upboard_up2_mfd_cells[] = {
> > +	{ .name = "upboard-pinctrl" },
> 
> I guess it should be 3 lines.

> 
> >  	UPBOARD_LED_CELL(upboard_up2_led_data, 0),
> >  	UPBOARD_LED_CELL(upboard_up2_led_data, 1),
> >  	UPBOARD_LED_CELL(upboard_up2_led_data, 2),
> 
> ...and honestly I would not use this macro and put 4 cells explicitly
> here.

The idea was to avoid repeating .platform_data + .pdata_size over and
over as there's a few LEDs per board, but explicit is good too.

May just go with your suggestion as that seems to be more popular than
macros in other mfd drivers.

> > +static int upboard_gpio_request_enable(struct pinctrl_dev *pctldev,
> > +				       struct pinctrl_gpio_range
> > *range,
> > +				       unsigned int pin)
> > +{
> > +	const struct pin_desc * const pd = pin_desc_get(pctldev,
> > pin);
> > +	const struct upboard_pin *p;
> > +	int ret;
> > +
> 
> > +	if (!pd)
> > +		return -EINVAL;
> 
> When it's possible?

Just reread pin_request() from pinctrl core. You're right, it isn't.
I'll take out the check.

> > +	if (!pd)
> > +		return -EINVAL;
> 
> Ditto.

As above.

> 
> > +	struct upboard_pinctrl *pctrl =
> > +		container_of(gc, struct upboard_pinctrl, chip);
> 
> Do define and use to_upboard_pinctrl().

Will do.

> > +	if (offset + 1 > pctrl->nsoc_gpios || !pctrl-
> > >soc_gpios[offset])
> > +		return ERR_PTR(-ENODEV);
> 
> When this is a case?

Another unnecessary check if gpiolib already guarantees valid inputs.

> > +static int upboard_gpio_get_direction(struct gpio_chip *gc, unsigned
> > int offset)
> > +{
> > +	struct gpio_desc *desc = upboard_offset_to_soc_gpio(gc,
> > offset);
> > +
> 
> Split above to definition and assignment pieces. Put assignment
> immediately before condition.
> 
> > +	if (IS_ERR(desc))
> > +		return PTR_ERR(desc);

I'll do that.

> > +static struct regmap_field * __init upboard_field_alloc(struct device
> > *dev,
> > +							struct regmap
> > *regmap,
> > +							unsigned int
> > base,
> > +							unsigned int
> > number)
> 
> You should really understand what __init means and when it's appropriate
> to use it.

As in the other email: the intention was to drop probe() and funcs only
used on module init, but I appear to have misunderstood something here.

> > +static int __init upboard_pinctrl_probe(struct platform_device *pdev)
> > +{
> > +	struct acpi_device * const adev = ACPI_COMPANION(&pdev->dev);
> 
> Huh, const in that place? Why?

You're right, it isn't a usual pattern. Dropped.

> > +	if (!pdev->dev.parent)
> > +		return -EINVAL;
> > +
> > +	upboard = dev_get_drvdata(pdev->dev.parent);
> > +	if (!upboard)
> > +		return -EINVAL;
> 
> Same comment as per LED driver.

I'll address that too.

> > +	if (strcmp(acpi_device_hid(adev), "AANT0F01"))
> > +		return -ENODEV;
> 
> Huh?

Ugh, this is left over from a bit of code that selected the right
pinctrl_desc* for each UP HID. Of course, it doesn't make sense now.

I'll take that out.

> > +		((struct pinctrl_pin_desc *)pd)->drv_data = pin;
> 
> What is that?! I mean ugly casting.

I agree, it's an eyesore. The intention was to drop const from
pinctrl_desc->pins as we're replacing the pins' drv_data on init.

It does look like I'm going at this the wrong way though. I'll take
another stab at it (pointers welcome of course).

> > +MODULE_LICENSE("GPL");
> 
> License mismatch.

Will fix.

Thanks again for your time Andy! I really appreciate your help :)

Re: [RFC PATCH RESEND 3/3] pinctrl: upboard: Add UP2 pinctrl and gpio driver

[Lee Jones]

On Sat, 21 Apr 2018, Javier Arteaga wrote:

> The UP2 board features a Raspberry Pi compatible pin header (HAT) and a
> board-specific expansion connector (EXHAT). Both expose assorted
> functions from either the SoC (such as GPIO, I2C, SPI, UART...) or other
> on-board devices (ADC, FPGA IP blocks...).
> 
> These lines are routed through an on-board FPGA. The platform controller
> in its stock firmware provides register fields to change:
> 
> - Line enable (FPGA pins enabled / high impedance)
> - Line direction (SoC driven / FPGA driven)
> 
> To enable using SoC GPIOs on the pin header, this arrangement requires
> both configuring the platform controller, and updating the SoC pad
> registers in sync.
> 
> Add a frontend pinctrl/GPIO driver that registers a new set of GPIO
> lines for the header pins. When these are requested, the driver
> propagates this request to the backend SoC pinctrl/GPIO driver by
> grabbing a GPIO descriptor for the matching SoC GPIO line. The needed
> mapping for this is retrieved via ACPI properties.
> 
> Signed-off-by: Javier Arteaga <javier@emutex.com>
> ---

[...]
> 
>  drivers/mfd/upboard.c             |   1 +
>  drivers/pinctrl/Kconfig           |  13 +
>  drivers/pinctrl/Makefile          |   1 +
>  drivers/pinctrl/pinctrl-upboard.c | 523 ++++++++++++++++++++++++++++++
>  4 files changed, 538 insertions(+)
>  create mode 100644 drivers/pinctrl/pinctrl-upboard.c
> 
> diff --git a/drivers/mfd/upboard.c b/drivers/mfd/upboard.c
> index 6e4767e4dc41..35111981dfdf 100644
> --- a/drivers/mfd/upboard.c
> +++ b/drivers/mfd/upboard.c
> @@ -132,6 +132,7 @@ static struct upboard_led_data upboard_up2_led_data[] = {
>  };
>  
>  static const struct mfd_cell upboard_up2_mfd_cells[] = {
> +	{ .name = "upboard-pinctrl" },
>  	UPBOARD_LED_CELL(upboard_up2_led_data, 0),
>  	UPBOARD_LED_CELL(upboard_up2_led_data, 1),
>  	UPBOARD_LED_CELL(upboard_up2_led_data, 2),

Please made this a separate patch.

There aren't any build dependencies between the files.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC PATCH RESEND 2/3] leds: upboard: Add LED support

[Lee Jones]

On Sat, 21 Apr 2018, Javier Arteaga wrote:

> Allow userspace to use the on-board LEDs as "upboard:<color>:".
> 
> Signed-off-by: Javier Arteaga <javier@emutex.com>
> ---
>  drivers/leds/Kconfig        | 10 +++++
>  drivers/leds/Makefile       |  1 +
>  drivers/leds/leds-upboard.c | 87 +++++++++++++++++++++++++++++++++++++

>  drivers/mfd/upboard.c       | 19 ++++++++

This change needs to be placed into a separate patch.

>  include/linux/mfd/upboard.h |  5 +++
>  5 files changed, 122 insertions(+)
>  create mode 100644 drivers/leds/leds-upboard.c

[...]

> diff --git a/drivers/mfd/upboard.c b/drivers/mfd/upboard.c
> index 8bae450cb83d..6e4767e4dc41 100644
> --- a/drivers/mfd/upboard.c
> +++ b/drivers/mfd/upboard.c
> @@ -79,6 +79,14 @@ struct upboard_data {
>  	size_t ncells;
>  };
>  
> +#define UPBOARD_LED_CELL(led_data, n)                   \
> +	{                                               \
> +		.name = "upboard-led",                  \
> +		.id = (n),                              \
> +		.platform_data = &led_data[(n)],        \
> +		.pdata_size = sizeof(*(led_data)),      \
> +	}
> +

There is a subsystem-level MACRO currently being reviewed on the list.

Just use the full format in your structs for now.

>  /* UP Squared */
>  
>  static const struct regmap_range upboard_up2_readable_ranges[] = {
> @@ -116,7 +124,18 @@ static const struct regmap_config upboard_up2_regmap_config = {
>  	.wr_table = &upboard_up2_writable_table,
>  };
>  
> +static struct upboard_led_data upboard_up2_led_data[] = {
> +	{ .id = 0, .color = "blue" },
> +	{ .id = 1, .color = "yellow" },
> +	{ .id = 2, .color = "green" },
> +	{ .id = 3, .color = "red" },
> +};

How is this data used?

Does it ever change, from board to board?

>  static const struct mfd_cell upboard_up2_mfd_cells[] = {
> +	UPBOARD_LED_CELL(upboard_up2_led_data, 0),
> +	UPBOARD_LED_CELL(upboard_up2_led_data, 1),
> +	UPBOARD_LED_CELL(upboard_up2_led_data, 2),
> +	UPBOARD_LED_CELL(upboard_up2_led_data, 3),
>  };
>  
>  static const struct upboard_data upboard_up2_data = {
> diff --git a/include/linux/mfd/upboard.h b/include/linux/mfd/upboard.h
> index d9dd214f4d29..eed68caa23ce 100644
> --- a/include/linux/mfd/upboard.h
> +++ b/include/linux/mfd/upboard.h
> @@ -62,4 +62,9 @@ struct upboard {
>  	struct gpio_desc *dataout_gpio;
>  };
>  
> +struct upboard_led_data {
> +	unsigned int id;
> +	const char *color;
> +};

If this is going to stick around, which I'm not sure it should, you
need to document it (using kerneldoc format), since 'id' is quite
ambiguous.

>  #endif /*  __LINUX_MFD_UPBOARD_H */

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC PATCH RESEND 2/3] leds: upboard: Add LED support

[Andy Shevchenko]

On Thu, 2018-04-26 at 03:34 +0100, Javier Arteaga wrote:
> On Wed, Apr 25, 2018 at 07:15:45PM +0300, Andy Shevchenko wrote:
> > On Sat, 2018-04-21 at 09:50 +0100, Javier Arteaga wrote:

> > > +static int __init upboard_led_probe(struct platform_device *pdev)
> > 
> > Are you sure about __init here?
> 
> Not 100% now :)
> 
> My understanding was that in this context, __init allows this probe()
> to
> be dropped from memory after module load.
> 
> What am I doing wrong there?

Just give another thought about it. The keyword(s) here is(are): time to
live of the objects in question. It's good to get knowing what unbind-
bind means (for built-in drivers).

> > > +	struct upboard_led_data * const pdata = pdev-
> > > > dev.platform_data;
> > 
> > Don't use direct dereference to platform_data.
> 
> Sorry, I don't understand this one. What's the alternative?

See other drivers how they do that stuff. Hint: check inline functions
in include/linux/device.h.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [RFC PATCH RESEND 2/3] leds: upboard: Add LED support

[Javier Arteaga]

Thanks Andy for these pointers :)

On Thu, Apr 26, 2018 at 10:55:49AM +0300, Andy Shevchenko wrote:
> On Thu, 2018-04-26 at 03:34 +0100, Javier Arteaga wrote:
> > My understanding was that in this context, __init allows this probe()
> > to
> > be dropped from memory after module load.
> > 
> > What am I doing wrong there?
> 
> Just give another thought about it. The keyword(s) here is(are): time to
> live of the objects in question. It's good to get knowing what unbind-
> bind means (for built-in drivers).

So this is the bit that I _believed_ applied to the platform drivers for
these MFD-registered devices (from driver-model/platform.txt):

  Or, in common situations where the device is known not to be hot-pluggable,
  the probe() routine can live in an init section to reduce the driver's
  runtime memory footprint:

  	int platform_driver_probe(struct platform_driver *drv,
  			  int (*probe)(struct platform_device *))

I'm thinking my misunderstanding probably stems from assuming that these
leds/pinctrl drivers will always find all devices registered at init
time. Can't say I've validated that assumption - I just didn't see
anything obviously blowing up in my tests so far :)

I'll keep reading and test out a few more things so I fully understand.
Until then, I've taken out __init annotations from next version.

> > > Don't use direct dereference to platform_data.
> > 
> > Sorry, I don't understand this one. What's the alternative?
> 
> See other drivers how they do that stuff. Hint: check inline functions
> in include/linux/device.h.

I wasn't looking at the right other drivers :)
I'll use the dev_get_platdata() wrapper going forwards.

Re: [RFC PATCH RESEND 2/3] leds: upboard: Add LED support

[Javier Arteaga]

Hi Lee,

On Thu, Apr 26, 2018 at 08:34:05AM +0100, Lee Jones wrote:
> >  drivers/mfd/upboard.c       | 19 ++++++++
> 
> This change needs to be placed into a separate patch.

OK. I'll do ("add driver" -> "add cell") pairs of patches then.

> > +#define UPBOARD_LED_CELL(led_data, n)                   \
> > +	{                                               \
> > +		.name = "upboard-led",                  \
> > +		.id = (n),                              \
> > +		.platform_data = &led_data[(n)],        \
> > +		.pdata_size = sizeof(*(led_data)),      \
> > +	}
> > +
> 
> There is a subsystem-level MACRO currently being reviewed on the list.
> 
> Just use the full format in your structs for now.

Will do.

> >  /* UP Squared */
> >  
> >  static const struct regmap_range upboard_up2_readable_ranges[] = {
> > @@ -116,7 +124,18 @@ static const struct regmap_config upboard_up2_regmap_config = {
> >  	.wr_table = &upboard_up2_writable_table,
> >  };
> >  
> > +static struct upboard_led_data upboard_up2_led_data[] = {
> > +	{ .id = 0, .color = "blue" },
> > +	{ .id = 1, .color = "yellow" },
> > +	{ .id = 2, .color = "green" },
> > +	{ .id = 3, .color = "red" },
> > +};
> 
> How is this data used?
> 
> Does it ever change, from board to board?

This provides indexes into the LED control register, so the leds driver
knows which regmap bits to flip, and maps them to color names, so it can
name the led devices accordingly. The mapping does change for each board
(UP1 has 3 LEDs, UP Core depends on the carrier board).

> > +struct upboard_led_data {
> > +	unsigned int id;
> > +	const char *color;
> > +};
> 
> If this is going to stick around, which I'm not sure it should, you
> need to document it (using kerneldoc format), since 'id' is quite
> ambiguous.

True, it's not very clear. Would it help here to also pass the regmap
explicitly as platform_data (instead of leds-upboard getting to the
regmap through parent driver drvdata)?

Thanks for your review!

Re: [RFC PATCH RESEND 3/3] pinctrl: upboard: Add UP2 pinctrl and gpio driver

[Javier Arteaga]

On Thu, Apr 26, 2018 at 07:50:28AM +0100, Lee Jones wrote:
> >  static const struct mfd_cell upboard_up2_mfd_cells[] = {
> > +	{ .name = "upboard-pinctrl" },
> >  	UPBOARD_LED_CELL(upboard_up2_led_data, 0),
> >  	UPBOARD_LED_CELL(upboard_up2_led_data, 1),
> >  	UPBOARD_LED_CELL(upboard_up2_led_data, 2),
> 
> Please made this a separate patch.
> 
> There aren't any build dependencies between the files.

Will do.

I have one further question about MFD in this patch too - should I keep
passing regmap into the driver via dev_get_drvdata(pdev->dev.parent),
or is explicit platform_data preferable?

Andy suggested platform_data allows more flexibility on the parent
device side (although I can't see upboard-pinctrl being used other than
as a child of the upboard driver).

I went with parent drvdata simply because that's what I found in other
MFD drivers and material [1].

Thank you!

[1]: http://events17.linuxfoundation.org/sites/events/files/slides/belloni-mfd-regmap-syscon_0.pdf

Re: [RFC PATCH RESEND 2/3] leds: upboard: Add LED support

[Lee Jones]

On Thu, 26 Apr 2018, Javier Arteaga wrote:

[...]

> > >  static const struct regmap_range upboard_up2_readable_ranges[] = {
> > > @@ -116,7 +124,18 @@ static const struct regmap_config upboard_up2_regmap_config = {
> > >  	.wr_table = &upboard_up2_writable_table,
> > >  };
> > >  
> > > +static struct upboard_led_data upboard_up2_led_data[] = {
> > > +	{ .id = 0, .color = "blue" },
> > > +	{ .id = 1, .color = "yellow" },
> > > +	{ .id = 2, .color = "green" },
> > > +	{ .id = 3, .color = "red" },
> > > +};
> > 
> > How is this data used?
> > 
> > Does it ever change, from board to board?
> 
> This provides indexes into the LED control register, so the leds driver
> knows which regmap bits to flip, and maps them to color names, so it can
> name the led devices accordingly. The mapping does change for each board
> (UP1 has 3 LEDs, UP Core depends on the carrier board).

I think this information should live in the driver which consumes it.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC PATCH RESEND 2/3] leds: upboard: Add LED support

[Andy Shevchenko]

On Thu, 2018-04-26 at 13:49 +0100, Javier Arteaga wrote:
> On Thu, Apr 26, 2018 at 10:55:49AM +0300, Andy Shevchenko wrote:
> > On Thu, 2018-04-26 at 03:34 +0100, Javier Arteaga wrote:
> > > My understanding was that in this context, __init allows this
> > > probe()
> > > to
> > > be dropped from memory after module load.
> > > 
> > > What am I doing wrong there?
> > 
> > Just give another thought about it. The keyword(s) here is(are):
> > time to
> > live of the objects in question. It's good to get knowing what
> > unbind-
> > bind means (for built-in drivers).
> 
> So this is the bit that I _believed_ applied to the platform drivers
> for
> these MFD-registered devices (from driver-model/platform.txt):
> 
>   Or, in common situations where the device is known not to be hot-
> pluggable,
>   the probe() routine can live in an init section to reduce the
> driver's
>   runtime memory footprint:
> 
>   	int platform_driver_probe(struct platform_driver *drv,
>   			  int (*probe)(struct platform_device *))
> 
> I'm thinking my misunderstanding probably stems from assuming that
> these
> leds/pinctrl drivers will always find all devices registered at init
> time. Can't say I've validated that assumption - I just didn't see
> anything obviously blowing up in my tests so far :)
> 
> I'll keep reading and test out a few more things so I fully
> understand.
> Until then, I've taken out __init annotations from next version.

Just do one small test. Try to unbind the (built-in) driver and bind it
back. Would it work? Would kernel survive this?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH v2 0/3] UP Squared board drivers

[Dan O'Donovan]

Hi all,

This series adds platform support for AAEON's UP Squared, a x86 SBC
based on Apollo Lake [1]. Specifically, it enables control for the board
pin headers and for the LEDs, as both of these features are only
available behind a FPGA-based platform controller.

This is structured around a MFD driver that provides a regmap interface
to that platform controller to each MFD cell.

The series is carved out from an out-of-tree support patchset, initially
written for the original UP Board [2] and then extended through the past
year to support new entries in the lineup (UP Board, UP Squared and
UP Core) [3].

Here we only submit support for UP Squared to simplify review. Still,
the driver is designed so that it can easily gain support for the other
boards.

V1 was originally submitted by Javier Arteaga in April 2018 as an RFC [4].
This V2 aims to address the feedback gratefully received from that.

[1]: http://www.up-board.org/upsquared/specifications-up2/
[2]: https://lkml.kernel.org/r/1467648434-29080-1-git-send-email-dan@emutex.com
[3]: https://github.com/emutex/ubilinux-kernel/commits/upboard-4.9
[4]: https://lore.kernel.org/lkml/20180421085009.28773-1-javier@emutex.com/

V2:
* Rewrote regmap r/w handlers to avoid duplication and improve readability
* Simplified data passing between MFD parent and child device drivers
* Removed incorrect __init attribute from probe functions
* Moved MFD cell definitions into same patch as MFD driver and simplified them
* Promoted some dev_dbg instances to dev_err for probe error cases
* Decoupled MFD cell reference from ACPI driver data
* Miscellanous coding style improvements
* Fixed license mismatches

Javier Arteaga (3):
  mfd: upboard: Add UP2 platform controller driver
  leds: upboard: Add LED support
  pinctrl: upboard: Add UP2 pinctrl and gpio driver

 drivers/leds/Kconfig              |  10 +
 drivers/leds/Makefile             |   1 +
 drivers/leds/leds-upboard.c       | 104 ++++++++
 drivers/mfd/Kconfig               |  17 ++
 drivers/mfd/Makefile              |   1 +
 drivers/mfd/upboard.c             | 344 +++++++++++++++++++++++++
 drivers/pinctrl/Kconfig           |  13 +
 drivers/pinctrl/Makefile          |   1 +
 drivers/pinctrl/pinctrl-upboard.c | 519 ++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/upboard.h       |  44 ++++
 10 files changed, 1054 insertions(+)
 create mode 100644 drivers/leds/leds-upboard.c
 create mode 100644 drivers/mfd/upboard.c
 create mode 100644 drivers/pinctrl/pinctrl-upboard.c
 create mode 100644 include/linux/mfd/upboard.h

-- 
2.7.4


------
This email has been scanned for spam and malware by The Email Laundry.

[PATCH v2 1/3] mfd: upboard: Add UP2 platform controller driver

[Dan O'Donovan]

From: Javier Arteaga <javier@emutex.com>

UP Squared (UP2) is a x86 SBC from AAEON based on Intel Apollo Lake. It
features a MAX 10 FPGA that routes lines from both SoC and on-board
devices to two I/O headers:

                                +------------------------+
                                | 40-pin RPi-like header |
                         +------|         (HAT)          |
                         |      +------------------------+
    +-------+    +--------+
    |       |    |        |     +------------------------+
    |  SoC  |----|  FPGA  |-----|  Custom UP2 pin header |
    |       |    |        |     |        (EXHAT)         |
    +-------+    +--------+     +------------------------+
                         |
                         +------* On-board devices: LED, VLS...

This is intended to enable vendor-specific applications to customize I/O
header pinout, as well as include low-latency functionality. It also
performs voltage level translation between the SoC (1.8V) and HAT header
(3.3V).

Out of the box, this block implements a platform controller with a
GPIO-bitbanged control interface. It's enumerated by ACPI and provides
registers to control:

- Configuration of all FPGA-routed header lines. These can be driven
  SoC-to-header, header-to-SoC or set in high impedance.

- On-board LEDs and enable lines for other platform devices.

Add core support for this platform controller as a MFD device, exposing
these registers as a regmap.

Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Javier Arteaga <javier@emutex.com>
Signed-off-by: Dan O'Donovan <dan@emutex.com>
---
 drivers/mfd/Kconfig         |  17 +++
 drivers/mfd/Makefile        |   1 +
 drivers/mfd/upboard.c       | 344 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/upboard.h |  44 ++++++
 4 files changed, 406 insertions(+)
 create mode 100644 drivers/mfd/upboard.c
 create mode 100644 include/linux/mfd/upboard.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 11841f4..4f91474 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1855,6 +1855,23 @@ config MFD_STM32_TIMERS
 	  for PWM and IIO Timer. This driver allow to share the
 	  registers between the others drivers.
 
+config MFD_UPBOARD
+	tristate "UP Squared"
+	depends on ACPI
+	depends on GPIOLIB
+	select MFD_CORE
+	select REGMAP
+	help
+	  If you say yes here you get support for the platform controller
+	  of the UP Squared single-board computer.
+
+	  This driver provides common support for accessing the device,
+	  additional drivers must be enabled in order to use the
+	  functionality of the device.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called "upboard".
+
 menu "Multimedia Capabilities Port drivers"
 	depends on ARCH_SA1100
 
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 5856a94..470f667 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -241,3 +241,4 @@ obj-$(CONFIG_MFD_SC27XX_PMIC)	+= sprd-sc27xx-spi.o
 obj-$(CONFIG_RAVE_SP_CORE)	+= rave-sp.o
 obj-$(CONFIG_MFD_ROHM_BD718XX)	+= rohm-bd718x7.o
 
+obj-$(CONFIG_MFD_UPBOARD)  += upboard.o
diff --git a/drivers/mfd/upboard.c b/drivers/mfd/upboard.c
new file mode 100644
index 0000000..6b3522a
--- /dev/null
+++ b/drivers/mfd/upboard.c
@@ -0,0 +1,344 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// UP Board platform controller driver
+//
+// Copyright (c) 2018, Emutex Ltd.
+//
+// Author: Javier Arteaga <javier@emutex.com>
+//
+
+#include <linux/acpi.h>
+#include <linux/gpio/consumer.h>
+#include <linux/kernel.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/upboard.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define UPBOARD_FW_BUILD_SHIFT 12
+#define UPBOARD_FW_MAJOR_SHIFT 8
+#define UPBOARD_FW_MINOR_SHIFT 4
+#define UPBOARD_FW_PATCH_SHIFT 0
+
+#define UPBOARD_FW_BUILD(id) (((id) >> UPBOARD_FW_BUILD_SHIFT) & 0x0f)
+#define UPBOARD_FW_MAJOR(id) (((id) >> UPBOARD_FW_MAJOR_SHIFT) & 0x0f)
+#define UPBOARD_FW_MINOR(id) (((id) >> UPBOARD_FW_MINOR_SHIFT) & 0x0f)
+#define UPBOARD_FW_PATCH(id) (((id) >> UPBOARD_FW_PATCH_SHIFT) & 0x0f)
+
+#define AAEON_MANUFACTURER_ID 0x01
+#define SUPPORTED_FW_MAJOR 0x0
+
+/* MSb of 8-bit address is an R/W flag */
+#define UPBOARD_ADDRESS_SIZE  8
+#define UPBOARD_READ_FLAG     BIT(7)
+
+enum upboard_id {
+	UPBOARD_ID_UP2 = 0,
+};
+
+struct upboard_ddata {
+	struct gpio_desc *clear_gpio;
+	struct gpio_desc *strobe_gpio;
+	struct gpio_desc *datain_gpio;
+	struct gpio_desc *dataout_gpio;
+	const struct regmap_config *regmapconf;
+	const struct mfd_cell *cells;
+	size_t ncells;
+};
+
+/*
+ * UP boards include a platform controller with a proprietary GPIO-bitbanged
+ * control interface to access its configuration registers.
+ *
+ * The following macros and functions implement the read/write handlers for
+ * that interface, to provide a regmap-based abstraction for the controller.
+ */
+
+#define set_clear(u, x) gpiod_set_value((u)->clear_gpio, (x))
+#define set_strobe(u, x) gpiod_set_value((u)->strobe_gpio, (x))
+#define set_datain(u, x) gpiod_set_value((u)->datain_gpio, (x))
+#define get_dataout(u) gpiod_get_value((u)->dataout_gpio)
+
+static void __reg_io_start(const struct upboard_ddata * const ddata)
+{
+	/*
+	 * CLEAR signal must be pulsed low before any register access.
+	 * This resets internal counters in the controller and marks
+	 * the start of a new register access.
+	 */
+	set_clear(ddata, 0);
+	set_clear(ddata, 1);
+}
+
+static void __reg_io_end(const struct upboard_ddata * const ddata)
+{
+	/*
+	 * STROBE signal must be cycled again to mark the end of a register
+	 * access.  Partial register accesses are discarded harmlessly
+	 * by the controller if this final strobe cycle is not sent
+	 */
+	set_strobe(ddata, 0);
+	set_strobe(ddata, 1);
+}
+
+static void __reg_io_write(const struct upboard_ddata * const ddata,
+			   unsigned int size, unsigned int val)
+{
+	int i;
+
+	/*
+	 * DATAIN is latched on each rising edge of the STROBE signal.
+	 * Data (register address or value) is sent MSb first.
+	 */
+	for (i = size - 1; i >= 0; i--) {
+		set_strobe(ddata, 0);
+		set_datain(ddata, (val >> i) & 0x1);
+		set_strobe(ddata, 1);
+	}
+}
+
+static void __reg_io_read(const struct upboard_ddata * const ddata,
+			  unsigned int size, unsigned int *val)
+{
+	int i;
+
+	/*
+	 * DATAOUT is latched on on each rising edge of the STROBE signal.
+	 * Data (register value) is received MSb first.
+	 */
+	*val = 0;
+	for (i = size - 1; i >= 0; i--) {
+		set_strobe(ddata, 0);
+		set_strobe(ddata, 1);
+		*val |= get_dataout(ddata) << i;
+	}
+}
+
+static int upboard_reg_read(void *context, unsigned int reg, unsigned int *val)
+{
+	const struct upboard_ddata * const ddata = context;
+
+	__reg_io_start(ddata);
+	__reg_io_write(ddata, UPBOARD_ADDRESS_SIZE, reg | UPBOARD_READ_FLAG);
+	__reg_io_read(ddata, UPBOARD_REGISTER_SIZE, val);
+	__reg_io_end(ddata);
+
+	return 0;
+}
+
+static int upboard_reg_write(void *context, unsigned int reg, unsigned int val)
+{
+	const struct upboard_ddata * const ddata = context;
+
+	__reg_io_start(ddata);
+	__reg_io_write(ddata, UPBOARD_ADDRESS_SIZE, reg);
+	__reg_io_write(ddata, UPBOARD_REGISTER_SIZE, val);
+	__reg_io_end(ddata);
+
+	return 0;
+}
+
+/* UP Squared */
+
+static const struct regmap_range upboard_up2_readable_ranges[] = {
+	regmap_reg_range(UPBOARD_REG_PLATFORM_ID, UPBOARD_REG_FIRMWARE_ID),
+	regmap_reg_range(UPBOARD_REG_FUNC_EN0, UPBOARD_REG_FUNC_EN1),
+	regmap_reg_range(UPBOARD_REG_GPIO_EN0, UPBOARD_REG_GPIO_EN2),
+	regmap_reg_range(UPBOARD_REG_GPIO_DIR0, UPBOARD_REG_GPIO_DIR2),
+};
+
+static const struct regmap_range upboard_up2_writable_ranges[] = {
+	regmap_reg_range(UPBOARD_REG_FUNC_EN0, UPBOARD_REG_FUNC_EN1),
+	regmap_reg_range(UPBOARD_REG_GPIO_EN0, UPBOARD_REG_GPIO_EN2),
+	regmap_reg_range(UPBOARD_REG_GPIO_DIR0, UPBOARD_REG_GPIO_DIR2),
+};
+
+static const struct regmap_access_table upboard_up2_readable_table = {
+	.yes_ranges = upboard_up2_readable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(upboard_up2_readable_ranges),
+};
+
+static const struct regmap_access_table upboard_up2_writable_table = {
+	.yes_ranges = upboard_up2_writable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(upboard_up2_writable_ranges),
+};
+
+static const struct regmap_config upboard_up2_regmap_config = {
+	.reg_bits = UPBOARD_ADDRESS_SIZE,
+	.val_bits = UPBOARD_REGISTER_SIZE,
+	.max_register = UPBOARD_REG_MAX,
+	.reg_read = upboard_reg_read,
+	.reg_write = upboard_reg_write,
+	.fast_io = false,
+	.cache_type = REGCACHE_RBTREE,
+	.rd_table = &upboard_up2_readable_table,
+	.wr_table = &upboard_up2_writable_table,
+};
+
+static const struct mfd_cell upboard_up2_mfd_cells[] = {
+	{
+		.name = "upboard-led",
+		.id = 0,
+	},
+	{
+		.name = "upboard-led",
+		.id = 1,
+	},
+	{
+		.name = "upboard-led",
+		.id = 2,
+	},
+	{
+		.name = "upboard-led",
+		.id = 3,
+	},
+	{
+		.name = "upboard-pinctrl"
+	},
+};
+
+static int upboard_init_gpio(struct device *dev)
+{
+	struct upboard_ddata *ddata = dev_get_drvdata(dev);
+	struct gpio_desc *enable_gpio;
+
+	ddata->clear_gpio = devm_gpiod_get(dev, "clear", GPIOD_OUT_LOW);
+	if (IS_ERR(ddata->clear_gpio))
+		return PTR_ERR(ddata->clear_gpio);
+
+	ddata->strobe_gpio = devm_gpiod_get(dev, "strobe", GPIOD_OUT_LOW);
+	if (IS_ERR(ddata->strobe_gpio))
+		return PTR_ERR(ddata->strobe_gpio);
+
+	ddata->datain_gpio = devm_gpiod_get(dev, "datain", GPIOD_OUT_LOW);
+	if (IS_ERR(ddata->datain_gpio))
+		return PTR_ERR(ddata->datain_gpio);
+
+	ddata->dataout_gpio = devm_gpiod_get(dev, "dataout", GPIOD_IN);
+	if (IS_ERR(ddata->dataout_gpio))
+		return PTR_ERR(ddata->dataout_gpio);
+
+	/* External I/O signals are gated by ENABLE - ensure this is high */
+	enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_HIGH);
+	if (IS_ERR(enable_gpio))
+		return PTR_ERR(enable_gpio);
+
+	return 0;
+}
+
+static int upboard_check_supported(struct device *dev, struct regmap *regmap)
+{
+	uint8_t manufacturer_id, build, major, minor, patch;
+	unsigned int platform_id, firmware_id;
+	int ret;
+
+	ret = regmap_read(regmap, UPBOARD_REG_PLATFORM_ID, &platform_id);
+	if (ret)
+		return ret;
+
+	manufacturer_id = platform_id & 0xff;
+	if (manufacturer_id != AAEON_MANUFACTURER_ID) {
+		dev_err(dev,
+			"unsupported FPGA firmware from manufacturer 0x%02x",
+			manufacturer_id);
+		return -ENODEV;
+	}
+
+	ret = regmap_read(regmap, UPBOARD_REG_FIRMWARE_ID, &firmware_id);
+	if (ret)
+		return ret;
+
+	build = UPBOARD_FW_BUILD(firmware_id);
+	major = UPBOARD_FW_MAJOR(firmware_id);
+	minor = UPBOARD_FW_MINOR(firmware_id);
+	patch = UPBOARD_FW_PATCH(firmware_id);
+	if (major != SUPPORTED_FW_MAJOR) {
+		dev_err(dev, "unsupported FPGA firmware v%u.%u.%u.%u",
+			 major, minor, patch, build);
+		return -ENODEV;
+	}
+
+	dev_dbg(dev, "supported FPGA firmware v%u.%u.%u.%u",
+		major, minor, patch, build);
+	return 0;
+}
+
+static const struct acpi_device_id upboard_acpi_match[] = {
+	{ "AANT0F01", (kernel_ulong_t)UPBOARD_ID_UP2 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, upboard_acpi_match);
+
+static int upboard_match_device(struct device *dev)
+{
+	struct upboard_ddata *ddata = dev_get_drvdata(dev);
+	const struct acpi_device_id *id;
+
+	id = acpi_match_device(upboard_acpi_match, dev);
+	if (!id)
+		return -ENODEV;
+
+	switch (id->driver_data) {
+	case UPBOARD_ID_UP2:
+		ddata->regmapconf = &upboard_up2_regmap_config;
+		ddata->cells = upboard_up2_mfd_cells;
+		ddata->ncells = ARRAY_SIZE(upboard_up2_mfd_cells);
+		break;
+	default:
+		dev_err(dev, "unsupported ID %lu\n", id->driver_data);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int upboard_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct upboard_ddata *ddata;
+	struct regmap *regmap;
+	int ret;
+
+	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
+	if (!ddata)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, ddata);
+
+	ret = upboard_match_device(dev);
+	if (ret)
+		return ret;
+
+	regmap = devm_regmap_init(dev, NULL, ddata, ddata->regmapconf);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	ret = upboard_init_gpio(dev);
+	if (ret) {
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "failed to init GPIOs: %d", ret);
+		return ret;
+	}
+
+	ret = upboard_check_supported(dev, regmap);
+	if (ret)
+		return ret;
+
+	return devm_mfd_add_devices(dev, 0, ddata->cells, ddata->ncells,
+				    NULL, 0, NULL);
+}
+
+static struct platform_driver upboard_driver = {
+	.probe = upboard_probe,
+	.driver = {
+		.name = "upboard",
+		.acpi_match_table = upboard_acpi_match,
+	},
+};
+
+module_platform_driver(upboard_driver);
+
+MODULE_AUTHOR("Javier Arteaga <javier@emutex.com>");
+MODULE_DESCRIPTION("UP Board platform controller driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/upboard.h b/include/linux/mfd/upboard.h
new file mode 100644
index 0000000..63e677f
--- /dev/null
+++ b/include/linux/mfd/upboard.h
@@ -0,0 +1,44 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// UP Board MFD driver interface
+//
+// Copyright (c) 2018, Emutex Ltd.
+//
+// Author: Javier Arteaga <javier@emutex.com>
+//
+
+#ifndef __LINUX_MFD_UPBOARD_H
+#define __LINUX_MFD_UPBOARD_H
+
+#define UPBOARD_REGISTER_SIZE 16
+
+/**
+ * enum upboard_reg - addresses for 16-bit controller registers
+ *
+ * @UPBOARD_REG_PLATFORM_ID:    [RO] BOARD_ID | MANUFACTURER_ID
+ * @UPBOARD_REG_FIRMWARE_ID:    [RO] BUILD | MAJOR | MINOR | PATCH
+ * @UPBOARD_REG_FUNC_EN0:       [RW] Toggles for board functions (bank 0)
+ * @UPBOARD_REG_FUNC_EN1:       [RW] Toggles for board functions (bank 1)
+ * @UPBOARD_REG_GPIO_EN0:       [RW] Hi-Z (0) / enabled (1) GPIO (bank 0)
+ * @UPBOARD_REG_GPIO_EN1:       [RW] Hi-Z (0) / enabled (1) GPIO (bank 1)
+ * @UPBOARD_REG_GPIO_EN2:       [RW] Hi-Z (0) / enabled (1) GPIO (bank 2)
+ * @UPBOARD_REG_GPIO_DIR0:      [RW] SoC- (0) / FPGA- (1) driven GPIO (bank 0)
+ * @UPBOARD_REG_GPIO_DIR1:      [RW] SoC- (0) / FPGA- (1) driven GPIO (bank 1)
+ * @UPBOARD_REG_GPIO_DIR2:      [RW] SoC- (0) / FPGA- (1) driven GPIO (bank 2)
+ * @UPBOARD_REG_MAX: one past the last valid address
+ */
+enum upboard_reg {
+	UPBOARD_REG_PLATFORM_ID   = 0x10,
+	UPBOARD_REG_FIRMWARE_ID   = 0x11,
+	UPBOARD_REG_FUNC_EN0      = 0x20,
+	UPBOARD_REG_FUNC_EN1      = 0x21,
+	UPBOARD_REG_GPIO_EN0      = 0x30,
+	UPBOARD_REG_GPIO_EN1      = 0x31,
+	UPBOARD_REG_GPIO_EN2      = 0x32,
+	UPBOARD_REG_GPIO_DIR0     = 0x40,
+	UPBOARD_REG_GPIO_DIR1     = 0x41,
+	UPBOARD_REG_GPIO_DIR2     = 0x42,
+	UPBOARD_REG_MAX,
+};
+
+#endif /*  __LINUX_MFD_UPBOARD_H */
-- 
2.7.4


------
This email has been scanned for spam and malware by The Email Laundry.

[PATCH v2 2/3] leds: upboard: Add LED support

[Dan O'Donovan]

From: Javier Arteaga <javier@emutex.com>

Allow userspace to use the on-board LEDs as "upboard:<color>:".

Acked-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Javier Arteaga <javier@emutex.com>
Signed-off-by: Dan O'Donovan <dan@emutex.com>
---
 drivers/leds/Kconfig        |  10 +++++
 drivers/leds/Makefile       |   1 +
 drivers/leds/leds-upboard.c | 104 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 115 insertions(+)
 create mode 100644 drivers/leds/leds-upboard.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 44097a3..0ed8857 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -756,6 +756,16 @@ config LEDS_NIC78BX
 	  To compile this driver as a module, choose M here: the module
 	  will be called leds-nic78bx.
 
+config LEDS_UPBOARD
+	tristate "LED support for the UP Squared"
+	depends on LEDS_CLASS
+	depends on MFD_UPBOARD
+	help
+	  This option enables support for the LEDs on the UP Squared board.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called leds-upboard.
+
 comment "LED Triggers"
 source "drivers/leds/trigger/Kconfig"
 
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 420b5d2..c85f18f 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -78,6 +78,7 @@ obj-$(CONFIG_LEDS_MT6323)		+= leds-mt6323.o
 obj-$(CONFIG_LEDS_LM3692X)		+= leds-lm3692x.o
 obj-$(CONFIG_LEDS_SC27XX_BLTC)		+= leds-sc27xx-bltc.o
 obj-$(CONFIG_LEDS_LM3601X)		+= leds-lm3601x.o
+obj-$(CONFIG_LEDS_UPBOARD)		+= leds-upboard.o
 
 # LED SPI Drivers
 obj-$(CONFIG_LEDS_CR0014114)		+= leds-cr0014114.o
diff --git a/drivers/leds/leds-upboard.c b/drivers/leds/leds-upboard.c
new file mode 100644
index 0000000..34a6973
--- /dev/null
+++ b/drivers/leds/leds-upboard.c
@@ -0,0 +1,104 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// UP Board LED driver
+//
+// Copyright (c) 2018, Emutex Ltd.
+//
+// Author: Javier Arteaga <javier@emutex.com>
+//
+
+#include <linux/kernel.h>
+#include <linux/leds.h>
+#include <linux/mfd/upboard.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/acpi.h>
+
+#define to_upboard_led(cdev) container_of(cdev, struct upboard_led, cdev)
+
+static const char * const upboard_led_names[] = {
+	"upboard:blue:",
+	"upboard:yellow:",
+	"upboard:green:",
+	"upboard:red:",
+};
+
+struct upboard_led {
+	struct regmap_field *field;
+	struct led_classdev cdev;
+};
+
+static enum led_brightness upboard_led_brightness_get(struct led_classdev *cdev)
+{
+	struct upboard_led *led = to_upboard_led(cdev);
+	int brightness = 0;
+
+	regmap_field_read(led->field, &brightness);
+
+	return brightness;
+}
+
+static void upboard_led_brightness_set(struct led_classdev *cdev,
+				       enum led_brightness brightness)
+{
+	struct upboard_led *led = to_upboard_led(cdev);
+
+	regmap_field_write(led->field, brightness);
+}
+
+static int upboard_led_probe(struct platform_device *pdev)
+{
+	unsigned int led_index = pdev->id;
+	struct device *dev = &pdev->dev;
+	struct acpi_device *adev;
+	struct upboard_led *led;
+	struct regmap *regmap;
+	struct reg_field conf = {
+		.reg = UPBOARD_REG_FUNC_EN0,
+		.lsb = led_index,
+		.msb = led_index,
+	};
+
+	adev = ACPI_COMPANION(dev);
+	if (!adev || strcmp(acpi_device_hid(adev), "AANT0F01"))
+		return -ENODEV;
+
+	if (led_index >= ARRAY_SIZE(upboard_led_names))
+		return -EINVAL;
+
+	if (!dev->parent)
+		return -EINVAL;
+
+	regmap = dev_get_regmap(dev->parent, NULL);
+	if (!regmap)
+		return -EINVAL;
+
+	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
+	if (!led)
+		return -ENOMEM;
+
+	led->field = devm_regmap_field_alloc(dev, regmap, conf);
+	if (IS_ERR(led->field))
+		return PTR_ERR(led->field);
+
+	led->cdev.max_brightness = 1;
+	led->cdev.brightness_get = upboard_led_brightness_get;
+	led->cdev.brightness_set = upboard_led_brightness_set;
+	led->cdev.name = upboard_led_names[led_index];
+
+	return devm_led_classdev_register(dev, &led->cdev);
+}
+
+static struct platform_driver upboard_led_driver = {
+	.driver = {
+		.name = "upboard-led",
+	},
+};
+
+module_platform_driver_probe(upboard_led_driver, upboard_led_probe);
+
+MODULE_ALIAS("platform:upboard-led");
+MODULE_AUTHOR("Javier Arteaga <javier@emutex.com>");
+MODULE_DESCRIPTION("UP Board LED driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4


------
This email has been scanned for spam and malware by The Email Laundry.

[PATCH v2 3/3] pinctrl: upboard: Add UP2 pinctrl and gpio driver

[Dan O'Donovan]

From: Javier Arteaga <javier@emutex.com>

The UP2 board features a Raspberry Pi compatible pin header (HAT) and a
board-specific expansion connector (EXHAT). Both expose assorted
functions from either the SoC (such as GPIO, I2C, SPI, UART...) or other
on-board devices (ADC, FPGA IP blocks...).

These lines are routed through an on-board FPGA. The platform controller
in its stock firmware provides register fields to change:

- Line enable (FPGA pins enabled / high impedance)
- Line direction (SoC driven / FPGA driven)

To enable using SoC GPIOs on the pin header, this arrangement requires
both configuring the platform controller, and updating the SoC pad
registers in sync.

Add a frontend pinctrl/GPIO driver that registers a new set of GPIO
lines for the header pins. When these are requested, the driver
propagates this request to the backend SoC pinctrl/GPIO driver by
grabbing a GPIO descriptor for the matching SoC GPIO line. The needed
mapping for this is retrieved via ACPI properties.

Signed-off-by: Javier Arteaga <javier@emutex.com>
Signed-off-by: Dan O'Donovan <dan@emutex.com>
---
 drivers/pinctrl/Kconfig           |  13 +
 drivers/pinctrl/Makefile          |   1 +
 drivers/pinctrl/pinctrl-upboard.c | 519 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 533 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-upboard.c

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index e86752b..c65438f 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -338,6 +338,19 @@ config PINCTRL_OCELOT
 	select GENERIC_PINMUX_FUNCTIONS
 	select REGMAP_MMIO
 
+config PINCTRL_UPBOARD
+	tristate "UP Squared pinctrl and GPIO driver"
+	depends on ACPI
+	depends on MFD_UPBOARD
+	select PINMUX
+	help
+	  Pinctrl driver for the pin headers on the UP Squared board. It
+	  handles pin control for lines routed through the on-board FPGA and
+	  propagates changes to the SoC pinctrl to keep them in sync.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called pinctrl-upboard.
+
 source "drivers/pinctrl/actions/Kconfig"
 source "drivers/pinctrl/aspeed/Kconfig"
 source "drivers/pinctrl/bcm/Kconfig"
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 46ef9bd..cfe59b7 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -42,6 +42,7 @@ obj-$(CONFIG_PINCTRL_ZYNQ)	+= pinctrl-zynq.o
 obj-$(CONFIG_PINCTRL_INGENIC)	+= pinctrl-ingenic.o
 obj-$(CONFIG_PINCTRL_RK805)	+= pinctrl-rk805.o
 obj-$(CONFIG_PINCTRL_OCELOT)	+= pinctrl-ocelot.o
+obj-$(CONFIG_PINCTRL_UPBOARD)	+= pinctrl-upboard.o
 
 obj-y				+= actions/
 obj-$(CONFIG_ARCH_ASPEED)	+= aspeed/
diff --git a/drivers/pinctrl/pinctrl-upboard.c b/drivers/pinctrl/pinctrl-upboard.c
new file mode 100644
index 0000000..b74383b
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-upboard.c
@@ -0,0 +1,519 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// UP Board pin controller driver
+//
+// Copyright (c) 2018, Emutex Ltd.
+//
+// Authors: Javier Arteaga <javier@emutex.com>
+//          Dan O'Donovan <dan@emutex.com>
+//
+
+#include <linux/acpi.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
+#include <linux/kernel.h>
+#include <linux/mfd/upboard.h>
+#include <linux/module.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/string.h>
+
+#include "core.h"
+
+struct upboard_pin {
+	struct regmap_field *func_en;
+	struct regmap_field *gpio_en;
+	struct regmap_field *gpio_dir;
+};
+
+struct upboard_pinctrl {
+	struct pinctrl_dev *pctldev;
+	struct gpio_chip chip;
+	unsigned int nsoc_gpios;
+	struct gpio_desc **soc_gpios;
+};
+
+enum upboard_func0_enables {
+	UPBOARD_I2C0_EN = 8,
+	UPBOARD_I2C1_EN = 9,
+};
+
+static const struct reg_field upboard_i2c0_reg =
+	REG_FIELD(UPBOARD_REG_FUNC_EN0, UPBOARD_I2C0_EN, UPBOARD_I2C0_EN);
+
+static const struct reg_field upboard_i2c1_reg =
+	REG_FIELD(UPBOARD_REG_FUNC_EN0, UPBOARD_I2C1_EN, UPBOARD_I2C1_EN);
+
+#define UPBOARD_BIT_TO_PIN(r, bit) \
+	((r) * UPBOARD_REGISTER_SIZE + (bit))
+
+/*
+ * UP Squared data
+ */
+
+#define UPBOARD_UP2_BIT_TO_PIN(r, id) (UPBOARD_BIT_TO_PIN(r, UPBOARD_UP2_##id))
+
+#define UPBOARD_UP2_PIN_ANON(r, bit)					\
+	{								\
+		.number = UPBOARD_BIT_TO_PIN(r, bit),			\
+	}
+
+#define UPBOARD_UP2_PIN_NAME(r, id)					\
+	{								\
+		.number = UPBOARD_UP2_BIT_TO_PIN(r, id),		\
+		.name = #id,						\
+	}
+
+#define UPBOARD_UP2_PIN_FUNC(r, id, data)				\
+	{								\
+		.number = UPBOARD_UP2_BIT_TO_PIN(r, id),		\
+		.name = #id,						\
+		.drv_data = (void *)(data),				\
+	}
+
+enum upboard_up2_reg0_bit {
+	UPBOARD_UP2_UART1_TXD,
+	UPBOARD_UP2_UART1_RXD,
+	UPBOARD_UP2_UART1_RTS,
+	UPBOARD_UP2_UART1_CTS,
+	UPBOARD_UP2_GPIO3,
+	UPBOARD_UP2_GPIO5,
+	UPBOARD_UP2_GPIO6,
+	UPBOARD_UP2_GPIO11,
+	UPBOARD_UP2_EXHAT_LVDS1n,
+	UPBOARD_UP2_EXHAT_LVDS1p,
+	UPBOARD_UP2_SPI2_TXD,
+	UPBOARD_UP2_SPI2_RXD,
+	UPBOARD_UP2_SPI2_FS1,
+	UPBOARD_UP2_SPI2_FS0,
+	UPBOARD_UP2_SPI2_CLK,
+	UPBOARD_UP2_SPI1_TXD,
+};
+
+enum upboard_up2_reg1_bit {
+	UPBOARD_UP2_SPI1_RXD,
+	UPBOARD_UP2_SPI1_FS1,
+	UPBOARD_UP2_SPI1_FS0,
+	UPBOARD_UP2_SPI1_CLK,
+	UPBOARD_UP2_BIT20,
+	UPBOARD_UP2_BIT21,
+	UPBOARD_UP2_BIT22,
+	UPBOARD_UP2_BIT23,
+	UPBOARD_UP2_PWM1,
+	UPBOARD_UP2_PWM0,
+	UPBOARD_UP2_EXHAT_LVDS0n,
+	UPBOARD_UP2_EXHAT_LVDS0p,
+	UPBOARD_UP2_I2C0_SCL,
+	UPBOARD_UP2_I2C0_SDA,
+	UPBOARD_UP2_I2C1_SCL,
+	UPBOARD_UP2_I2C1_SDA,
+};
+
+enum upboard_up2_reg2_bit {
+	UPBOARD_UP2_EXHAT_LVDS3n,
+	UPBOARD_UP2_EXHAT_LVDS3p,
+	UPBOARD_UP2_EXHAT_LVDS4n,
+	UPBOARD_UP2_EXHAT_LVDS4p,
+	UPBOARD_UP2_EXHAT_LVDS5n,
+	UPBOARD_UP2_EXHAT_LVDS5p,
+	UPBOARD_UP2_I2S_SDO,
+	UPBOARD_UP2_I2S_SDI,
+	UPBOARD_UP2_I2S_WS_SYNC,
+	UPBOARD_UP2_I2S_BCLK,
+	UPBOARD_UP2_EXHAT_LVDS6n,
+	UPBOARD_UP2_EXHAT_LVDS6p,
+	UPBOARD_UP2_EXHAT_LVDS7n,
+	UPBOARD_UP2_EXHAT_LVDS7p,
+	UPBOARD_UP2_EXHAT_LVDS2n,
+	UPBOARD_UP2_EXHAT_LVDS2p,
+};
+
+static struct pinctrl_pin_desc upboard_up2_pins[] = {
+	UPBOARD_UP2_PIN_NAME(0, UART1_TXD),
+	UPBOARD_UP2_PIN_NAME(0, UART1_RXD),
+	UPBOARD_UP2_PIN_NAME(0, UART1_RTS),
+	UPBOARD_UP2_PIN_NAME(0, UART1_CTS),
+	UPBOARD_UP2_PIN_NAME(0, GPIO3),
+	UPBOARD_UP2_PIN_NAME(0, GPIO5),
+	UPBOARD_UP2_PIN_NAME(0, GPIO6),
+	UPBOARD_UP2_PIN_NAME(0, GPIO11),
+	UPBOARD_UP2_PIN_NAME(0, EXHAT_LVDS1n),
+	UPBOARD_UP2_PIN_NAME(0, EXHAT_LVDS1p),
+	UPBOARD_UP2_PIN_NAME(0, SPI2_TXD),
+	UPBOARD_UP2_PIN_NAME(0, SPI2_RXD),
+	UPBOARD_UP2_PIN_NAME(0, SPI2_FS1),
+	UPBOARD_UP2_PIN_NAME(0, SPI2_FS0),
+	UPBOARD_UP2_PIN_NAME(0, SPI2_CLK),
+	UPBOARD_UP2_PIN_NAME(0, SPI1_TXD),
+	UPBOARD_UP2_PIN_NAME(1, SPI1_RXD),
+	UPBOARD_UP2_PIN_NAME(1, SPI1_FS1),
+	UPBOARD_UP2_PIN_NAME(1, SPI1_FS0),
+	UPBOARD_UP2_PIN_NAME(1, SPI1_CLK),
+	UPBOARD_UP2_PIN_ANON(1, 4),
+	UPBOARD_UP2_PIN_ANON(1, 5),
+	UPBOARD_UP2_PIN_ANON(1, 6),
+	UPBOARD_UP2_PIN_ANON(1, 7),
+	UPBOARD_UP2_PIN_NAME(1, PWM1),
+	UPBOARD_UP2_PIN_NAME(1, PWM0),
+	UPBOARD_UP2_PIN_NAME(1, EXHAT_LVDS0n),
+	UPBOARD_UP2_PIN_NAME(1, EXHAT_LVDS0p),
+	UPBOARD_UP2_PIN_FUNC(1, I2C0_SCL, &upboard_i2c0_reg),
+	UPBOARD_UP2_PIN_FUNC(1, I2C0_SDA, &upboard_i2c0_reg),
+	UPBOARD_UP2_PIN_FUNC(1, I2C1_SCL, &upboard_i2c1_reg),
+	UPBOARD_UP2_PIN_FUNC(1, I2C1_SDA, &upboard_i2c1_reg),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS3n),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS3p),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS4n),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS4p),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS5n),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS5p),
+	UPBOARD_UP2_PIN_NAME(2, I2S_SDO),
+	UPBOARD_UP2_PIN_NAME(2, I2S_SDI),
+	UPBOARD_UP2_PIN_NAME(2, I2S_WS_SYNC),
+	UPBOARD_UP2_PIN_NAME(2, I2S_BCLK),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS6n),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS6p),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS7n),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS7p),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS2n),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS2p),
+};
+
+static int upboard_get_functions_count(struct pinctrl_dev *pctldev)
+{
+	return 0;
+}
+
+static int upboard_get_function_groups(struct pinctrl_dev *pctldev,
+				       unsigned int selector,
+				       const char * const **groups,
+				       unsigned int *num_groups)
+{
+	*groups = NULL;
+	*num_groups = 0;
+	return 0;
+}
+
+static const char *upboard_get_function_name(struct pinctrl_dev *pctldev,
+					     unsigned int selector)
+{
+	return NULL;
+}
+
+static int upboard_set_mux(struct pinctrl_dev *pctldev, unsigned int function,
+			   unsigned int group)
+{
+	return 0;
+}
+
+static int upboard_gpio_request_enable(struct pinctrl_dev *pctldev,
+				       struct pinctrl_gpio_range *range,
+				       unsigned int pin)
+{
+	const struct pin_desc * const pd = pin_desc_get(pctldev, pin);
+	const struct upboard_pin *p;
+	int ret;
+
+	if (!pd)
+		return -EINVAL;
+	p = pd->drv_data;
+
+	/* if this pin has an associated function bit, disable it first */
+	if (p->func_en) {
+		ret = regmap_field_write(p->func_en, 0);
+		if (ret)
+			return ret;
+	}
+
+	if (p->gpio_en) {
+		ret = regmap_field_write(p->gpio_en, 1);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int upboard_gpio_set_direction(struct pinctrl_dev *pctldev,
+				      struct pinctrl_gpio_range *range,
+				      unsigned int pin, bool input)
+{
+	const struct pin_desc * const pd = pin_desc_get(pctldev, pin);
+	const struct upboard_pin *p;
+
+	if (!pd)
+		return -EINVAL;
+	p = pd->drv_data;
+
+	return regmap_field_write(p->gpio_dir, input);
+}
+
+static const struct pinmux_ops upboard_pinmux_ops = {
+	.get_functions_count = upboard_get_functions_count,
+	.get_function_groups = upboard_get_function_groups,
+	.get_function_name = upboard_get_function_name,
+	.set_mux = upboard_set_mux,
+	.gpio_request_enable = upboard_gpio_request_enable,
+	.gpio_set_direction = upboard_gpio_set_direction,
+};
+
+static int upboard_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	return 0;
+}
+
+static const char *upboard_get_group_name(struct pinctrl_dev *pctldev,
+					  unsigned int selector)
+{
+	return NULL;
+}
+
+static const struct pinctrl_ops upboard_pinctrl_ops = {
+	.get_groups_count = upboard_get_groups_count,
+	.get_group_name = upboard_get_group_name,
+};
+
+static struct pinctrl_desc upboard_up2_pinctrl_desc = {
+	.pins = upboard_up2_pins,
+	.npins = ARRAY_SIZE(upboard_up2_pins),
+	.pctlops = &upboard_pinctrl_ops,
+	.pmxops = &upboard_pinmux_ops,
+	.owner = THIS_MODULE,
+};
+
+static struct gpio_desc *upboard_offset_to_soc_gpio(struct gpio_chip *gc,
+						    unsigned int offset)
+{
+	struct upboard_pinctrl *pctrl =
+		container_of(gc, struct upboard_pinctrl, chip);
+
+	if (offset + 1 > pctrl->nsoc_gpios || !pctrl->soc_gpios[offset])
+		return ERR_PTR(-ENODEV);
+
+	return pctrl->soc_gpios[offset];
+}
+
+static int upboard_gpio_request(struct gpio_chip *gc, unsigned int offset)
+{
+	struct upboard_pinctrl *pctrl =
+		container_of(gc, struct upboard_pinctrl, chip);
+	struct gpio_desc *desc;
+	int ret;
+
+	ret = pinctrl_gpio_request(gc->base + offset);
+	if (ret)
+		return ret;
+
+	desc = devm_gpiod_get_index(gc->parent, "external", offset, GPIOD_ASIS);
+	if (IS_ERR(desc))
+		return PTR_ERR(desc);
+
+	pctrl->soc_gpios[offset] = desc;
+	return 0;
+}
+
+static void upboard_gpio_free(struct gpio_chip *gc, unsigned int offset)
+{
+	struct upboard_pinctrl *pctrl =
+		container_of(gc, struct upboard_pinctrl, chip);
+
+	if (offset + 1 > pctrl->nsoc_gpios || !pctrl->soc_gpios[offset])
+		return;
+
+	devm_gpiod_put(gc->parent, pctrl->soc_gpios[offset]);
+	pctrl->soc_gpios[offset] = NULL;
+
+	pinctrl_gpio_free(gc->base + offset);
+}
+
+static int upboard_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+	struct gpio_desc *desc = upboard_offset_to_soc_gpio(gc, offset);
+
+	if (IS_ERR(desc))
+		return PTR_ERR(desc);
+
+	return gpiod_get_direction(desc);
+}
+
+static int upboard_gpio_direction_input(struct gpio_chip *gc,
+					unsigned int offset)
+{
+	struct gpio_desc *desc = upboard_offset_to_soc_gpio(gc, offset);
+	int ret;
+
+	if (IS_ERR(desc))
+		return PTR_ERR(desc);
+
+	ret = gpiod_direction_input(desc);
+	if (ret)
+		return ret;
+
+	return pinctrl_gpio_direction_input(gc->base + offset);
+}
+
+static int upboard_gpio_direction_output(struct gpio_chip *gc,
+					 unsigned int offset, int value)
+{
+	struct gpio_desc *desc = upboard_offset_to_soc_gpio(gc, offset);
+	int ret;
+
+	if (IS_ERR(desc))
+		return PTR_ERR(desc);
+
+	ret = pinctrl_gpio_direction_output(gc->base + offset);
+	if (ret)
+		return ret;
+
+	return gpiod_direction_output(desc, value);
+}
+
+static int upboard_gpio_get_value(struct gpio_chip *gc, unsigned int offset)
+{
+	struct gpio_desc *desc = upboard_offset_to_soc_gpio(gc, offset);
+
+	if (IS_ERR(desc))
+		return PTR_ERR(desc);
+
+	return gpiod_get_value(desc);
+}
+
+static void upboard_gpio_set_value(struct gpio_chip *gc, unsigned int offset,
+				   int value)
+{
+	struct gpio_desc *desc = upboard_offset_to_soc_gpio(gc, offset);
+
+	if (IS_ERR(desc))
+		return;
+
+	gpiod_set_value(desc, value);
+}
+
+static struct gpio_chip upboard_gpio_chip = {
+	.label = "UP pin controller",
+	.owner = THIS_MODULE,
+	.request = upboard_gpio_request,
+	.free = upboard_gpio_free,
+	.get_direction = upboard_gpio_get_direction,
+	.direction_input = upboard_gpio_direction_input,
+	.direction_output = upboard_gpio_direction_output,
+	.get = upboard_gpio_get_value,
+	.set = upboard_gpio_set_value,
+	.base = -1,
+};
+
+static struct regmap_field *upboard_field_alloc(struct device *dev,
+						struct regmap *regmap,
+						unsigned int base,
+						unsigned int number)
+{
+	const unsigned int reg = number / UPBOARD_REGISTER_SIZE;
+	const unsigned int bit = number % UPBOARD_REGISTER_SIZE;
+	const struct reg_field field = {
+		.reg = base + reg,
+		.msb = bit,
+		.lsb = bit,
+	};
+
+	return devm_regmap_field_alloc(dev, regmap, field);
+}
+
+static int upboard_pinctrl_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct pinctrl_desc *pctldesc;
+	struct upboard_pinctrl *pctrl;
+	struct upboard_pin *pins;
+	struct acpi_device *adev;
+	struct regmap *regmap;
+	unsigned int i;
+	int ret;
+
+	adev = ACPI_COMPANION(dev);
+	if (!adev || strcmp(acpi_device_hid(adev), "AANT0F01"))
+		return -ENODEV;
+
+	if (!dev->parent)
+		return -EINVAL;
+
+	regmap = dev_get_regmap(dev->parent, NULL);
+	if (!regmap)
+		return -EINVAL;
+
+	pctldesc = &upboard_up2_pinctrl_desc;
+	pctldesc->name = dev_name(dev);
+
+	pins = devm_kzalloc(dev, sizeof(*pins) * pctldesc->npins, GFP_KERNEL);
+	if (!pins)
+		return -ENOMEM;
+
+	for (i = 0; i < pctldesc->npins; i++) {
+		struct upboard_pin *pin = &pins[i];
+		const struct pinctrl_pin_desc *pd = &pctldesc->pins[i];
+
+		pin->func_en = NULL;
+		if (pd->drv_data) {
+			struct reg_field *field = pd->drv_data;
+
+			pin->func_en = devm_regmap_field_alloc(dev, regmap,
+							       *field);
+			if (IS_ERR(pin->func_en))
+				return PTR_ERR(pin->func_en);
+		}
+
+		pin->gpio_en = upboard_field_alloc(dev, regmap,
+						   UPBOARD_REG_GPIO_EN0, i);
+		if (IS_ERR(pin->gpio_en))
+			return PTR_ERR(pin->gpio_en);
+
+		pin->gpio_dir = upboard_field_alloc(dev, regmap,
+						    UPBOARD_REG_GPIO_DIR0, i);
+		if (IS_ERR(pin->gpio_dir))
+			return PTR_ERR(pin->gpio_dir);
+
+		((struct pinctrl_pin_desc *)pd)->drv_data = pin;
+	}
+
+	pctrl = devm_kzalloc(dev, sizeof(*pctrl), GFP_KERNEL);
+	if (!pctrl)
+		return -ENOMEM;
+
+	pctrl->chip = upboard_gpio_chip;
+	pctrl->chip.parent = dev;
+	pctrl->chip.ngpio = pctldesc->npins;
+
+	pctrl->nsoc_gpios = gpiod_count(dev, "external");
+	pctrl->soc_gpios = devm_kzalloc(dev,
+					pctrl->nsoc_gpios * sizeof(*pctrl->soc_gpios),
+					GFP_KERNEL);
+	if (!pctrl->soc_gpios)
+		return -ENOMEM;
+
+	pctrl->pctldev = devm_pinctrl_register(dev, pctldesc, pctrl);
+	if (IS_ERR(pctrl->pctldev))
+		return PTR_ERR(pctrl->pctldev);
+
+	ret = devm_gpiochip_add_data(dev, &pctrl->chip, &pctrl->chip);
+	if (ret)
+		return ret;
+
+	return gpiochip_add_pin_range(&pctrl->chip, pctldesc->name, 0, 0,
+				      pctldesc->npins);
+}
+
+static struct platform_driver upboard_pinctrl_driver = {
+	.driver = {
+		.name = "upboard-pinctrl",
+	},
+};
+
+module_platform_driver_probe(upboard_pinctrl_driver, upboard_pinctrl_probe);
+
+MODULE_ALIAS("platform:upboard-pinctrl");
+MODULE_AUTHOR("Javier Arteaga <javier@emutex.com>");
+MODULE_AUTHOR("Dan O'Donovan <dan@emutex.com>");
+MODULE_DESCRIPTION("UP Board pin control and GPIO driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4


------
This email has been scanned for spam and malware by The Email Laundry.

Re: [PATCH v2 2/3] leds: upboard: Add LED support

[Andy Shevchenko]

On Fri, Oct 19, 2018 at 8:27 PM Dan O'Donovan <dan@emutex.com> wrote:
>
> From: Javier Arteaga <javier@emutex.com>
>
> Allow userspace to use the on-board LEDs as "upboard:<color>:".



> +#include <linux/kernel.h>
> +#include <linux/leds.h>
> +#include <linux/mfd/upboard.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/acpi.h>

For better maintenance keep this ordered.

> +       adev = ACPI_COMPANION(dev);
> +       if (!adev || strcmp(acpi_device_hid(adev), "AANT0F01"))
> +               return -ENODEV;

Why do you need this part?

> +       if (led_index >= ARRAY_SIZE(upboard_led_names))
> +               return -EINVAL;

> +       if (!dev->parent)
> +               return -EINVAL;

...I think this check will cover cases when driver is being probed standalone.
Otherwise it's caller's responsibility not to instantiate it if
platform doesn't have this LED feature.

> +       regmap = dev_get_regmap(dev->parent, NULL);
> +       if (!regmap)
> +               return -EINVAL;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/3] pinctrl: upboard: Add UP2 pinctrl and gpio driver

[Andy Shevchenko]

On Fri, Oct 19, 2018 at 8:24 PM Dan O'Donovan <dan@emutex.com> wrote:
>
> From: Javier Arteaga <javier@emutex.com>
>
> The UP2 board features a Raspberry Pi compatible pin header (HAT) and a
> board-specific expansion connector (EXHAT). Both expose assorted
> functions from either the SoC (such as GPIO, I2C, SPI, UART...) or other
> on-board devices (ADC, FPGA IP blocks...).
>
> These lines are routed through an on-board FPGA. The platform controller
> in its stock firmware provides register fields to change:
>
> - Line enable (FPGA pins enabled / high impedance)
> - Line direction (SoC driven / FPGA driven)
>
> To enable using SoC GPIOs on the pin header, this arrangement requires
> both configuring the platform controller, and updating the SoC pad
> registers in sync.
>
> Add a frontend pinctrl/GPIO driver that registers a new set of GPIO
> lines for the header pins. When these are requested, the driver
> propagates this request to the backend SoC pinctrl/GPIO driver by
> grabbing a GPIO descriptor for the matching SoC GPIO line. The needed
> mapping for this is retrieved via ACPI properties.

To Linus: please, don't consider this as anyhow part of Intel pin
control infrastructure. Thus, if you are okay with the driver
(personally I don't see any major issues with the code, though it
might be required some clarification on design level, e.g. ACPI
relationship) I have no objection.

> +#define UPBOARD_BIT_TO_PIN(r, bit) \
> +       ((r) * UPBOARD_REGISTER_SIZE + (bit))

One line?

> +static int upboard_get_functions_count(struct pinctrl_dev *pctldev)
> +{
> +       return 0;
> +}
> +
> +static int upboard_get_function_groups(struct pinctrl_dev *pctldev,
> +                                      unsigned int selector,
> +                                      const char * const **groups,
> +                                      unsigned int *num_groups)
> +{
> +       *groups = NULL;
> +       *num_groups = 0;
> +       return 0;
> +}
> +
> +static const char *upboard_get_function_name(struct pinctrl_dev *pctldev,
> +                                            unsigned int selector)
> +{
> +       return NULL;
> +}
> +
> +static int upboard_set_mux(struct pinctrl_dev *pctldev, unsigned int function,
> +                          unsigned int group)
> +{
> +       return 0;
> +}

Hmm... Do you need those stubs? Same Q for other stubs in the file.

> +static int upboard_gpio_request_enable(struct pinctrl_dev *pctldev,
> +                                      struct pinctrl_gpio_range *range,
> +                                      unsigned int pin)
> +{
> +       const struct pin_desc * const pd = pin_desc_get(pctldev, pin);
> +       const struct upboard_pin *p;
> +       int ret;
> +

> +       if (!pd)
> +               return -EINVAL;

When it possible to happen?
Same Q for the rest same excerpts.

> +       p = pd->drv_data;
> +
> +       /* if this pin has an associated function bit, disable it first */
> +       if (p->func_en) {
> +               ret = regmap_field_write(p->func_en, 0);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       if (p->gpio_en) {
> +               ret = regmap_field_write(p->gpio_en, 1);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       return 0;
> +}

> +static struct gpio_desc *upboard_offset_to_soc_gpio(struct gpio_chip *gc,
> +                                                   unsigned int offset)
> +{

> +       struct upboard_pinctrl *pctrl =
> +               container_of(gc, struct upboard_pinctrl, chip);

One line?

> +
> +       if (offset + 1 > pctrl->nsoc_gpios || !pctrl->soc_gpios[offset])
> +               return ERR_PTR(-ENODEV);

offset >= ?
Is it even possible?

> +
> +       return pctrl->soc_gpios[offset];
> +}
> +
> +static int upboard_gpio_request(struct gpio_chip *gc, unsigned int offset)
> +{
> +       struct upboard_pinctrl *pctrl =
> +               container_of(gc, struct upboard_pinctrl, chip);

One line?

> +}
> +
> +static void upboard_gpio_free(struct gpio_chip *gc, unsigned int offset)
> +{
> +       struct upboard_pinctrl *pctrl =
> +               container_of(gc, struct upboard_pinctrl, chip);

Ditto.

> +       if (offset + 1 > pctrl->nsoc_gpios || !pctrl->soc_gpios[offset])
> +               return;

offset >= ?
Is it even possible?

> +}

> +static int upboard_pinctrl_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct pinctrl_desc *pctldesc;
> +       struct upboard_pinctrl *pctrl;
> +       struct upboard_pin *pins;
> +       struct acpi_device *adev;
> +       struct regmap *regmap;
> +       unsigned int i;
> +       int ret;

> +       adev = ACPI_COMPANION(dev);
> +       if (!adev || strcmp(acpi_device_hid(adev), "AANT0F01"))
> +               return -ENODEV;

Same Q as per LED driver.

> +       for (i = 0; i < pctldesc->npins; i++) {
> +               struct upboard_pin *pin = &pins[i];
> +               const struct pinctrl_pin_desc *pd = &pctldesc->pins[i];

> +               pin->func_en = NULL;

Useless.

> +               if (pd->drv_data) {
> +                       struct reg_field *field = pd->drv_data;
> +
> +                       pin->func_en = devm_regmap_field_alloc(dev, regmap,
> +                                                              *field);
> +                       if (IS_ERR(pin->func_en))
> +                               return PTR_ERR(pin->func_en);
> +               }
> +
> +               pin->gpio_en = upboard_field_alloc(dev, regmap,
> +                                                  UPBOARD_REG_GPIO_EN0, i);
> +               if (IS_ERR(pin->gpio_en))
> +                       return PTR_ERR(pin->gpio_en);
> +
> +               pin->gpio_dir = upboard_field_alloc(dev, regmap,
> +                                                   UPBOARD_REG_GPIO_DIR0, i);
> +               if (IS_ERR(pin->gpio_dir))
> +                       return PTR_ERR(pin->gpio_dir);
> +

> +               ((struct pinctrl_pin_desc *)pd)->drv_data = pin;

I'm not sure I understand the purpose of this casting.

> +       }

> +       pctrl->soc_gpios = devm_kzalloc(dev,
> +                                       pctrl->nsoc_gpios * sizeof(*pctrl->soc_gpios),
> +                                       GFP_KERNEL);
> +       if (!pctrl->soc_gpios)
> +               return -ENOMEM;

kzalloc -> kcalloc

> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/3] mfd: upboard: Add UP2 platform controller driver

[Andy Shevchenko]

On Fri, Oct 19, 2018 at 8:26 PM Dan O'Donovan <dan@emutex.com> wrote:
>
> From: Javier Arteaga <javier@emutex.com>
>
> UP Squared (UP2) is a x86 SBC from AAEON based on Intel Apollo Lake. It
> features a MAX 10 FPGA that routes lines from both SoC and on-board
> devices to two I/O headers:
>
>                                 +------------------------+
>                                 | 40-pin RPi-like header |
>                          +------|         (HAT)          |
>                          |      +------------------------+
>     +-------+    +--------+
>     |       |    |        |     +------------------------+
>     |  SoC  |----|  FPGA  |-----|  Custom UP2 pin header |
>     |       |    |        |     |        (EXHAT)         |
>     +-------+    +--------+     +------------------------+
>                          |
>                          +------* On-board devices: LED, VLS...
>
> This is intended to enable vendor-specific applications to customize I/O
> header pinout, as well as include low-latency functionality. It also
> performs voltage level translation between the SoC (1.8V) and HAT header
> (3.3V).
>
> Out of the box, this block implements a platform controller with a
> GPIO-bitbanged control interface. It's enumerated by ACPI and provides
> registers to control:
>
> - Configuration of all FPGA-routed header lines. These can be driven
>   SoC-to-header, header-to-SoC or set in high impedance.
>
> - On-board LEDs and enable lines for other platform devices.
>
> Add core support for this platform controller as a MFD device, exposing
> these registers as a regmap.

Can we see a link to or an excerpt of ACPI table for this device?

> +#define set_clear(u, x) gpiod_set_value((u)->clear_gpio, (x))
> +#define set_strobe(u, x) gpiod_set_value((u)->strobe_gpio, (x))
> +#define set_datain(u, x) gpiod_set_value((u)->datain_gpio, (x))
> +#define get_dataout(u) gpiod_get_value((u)->dataout_gpio)

I think these macros don't bring much value. (Up to you and Lee to decide)


> +static void __reg_io_write(const struct upboard_ddata * const ddata,
> +                          unsigned int size, unsigned int val)
> +{
> +       int i;
> +
> +       /*
> +        * DATAIN is latched on each rising edge of the STROBE signal.
> +        * Data (register address or value) is sent MSb first.
> +        */

> +       for (i = size - 1; i >= 0; i--) {

while (size--)

> +               set_strobe(ddata, 0);
> +               set_datain(ddata, (val >> i) & 0x1);
> +               set_strobe(ddata, 1);
> +       }
> +}
> +
> +static void __reg_io_read(const struct upboard_ddata * const ddata,
> +                         unsigned int size, unsigned int *val)
> +{
> +       int i;
> +
> +       /*
> +        * DATAOUT is latched on on each rising edge of the STROBE signal.
> +        * Data (register value) is received MSb first.
> +        */
> +       *val = 0;

> +       for (i = size - 1; i >= 0; i--) {

Ditto.

> +               set_strobe(ddata, 0);
> +               set_strobe(ddata, 1);
> +               *val |= get_dataout(ddata) << i;
> +       }
> +}

> +static int upboard_check_supported(struct device *dev, struct regmap *regmap)
> +{
> +       uint8_t manufacturer_id, build, major, minor, patch;
> +       unsigned int platform_id, firmware_id;
> +       int ret;

> +       manufacturer_id = platform_id & 0xff;

Redundant & 0xff part.

> +       if (manufacturer_id != AAEON_MANUFACTURER_ID) {
> +               dev_err(dev,
> +                       "unsupported FPGA firmware from manufacturer 0x%02x",
> +                       manufacturer_id);
> +               return -ENODEV;
> +       }

> +static int upboard_match_device(struct device *dev)
> +{
> +       struct upboard_ddata *ddata = dev_get_drvdata(dev);
> +       const struct acpi_device_id *id;
> +

> +       id = acpi_match_device(upboard_acpi_match, dev);
> +       if (!id)
> +               return -ENODEV;
> +
> +       switch (id->driver_data) {

device_get_match_data() IIRC the name of the call.

> +       case UPBOARD_ID_UP2:
> +               ddata->regmapconf = &upboard_up2_regmap_config;
> +               ddata->cells = upboard_up2_mfd_cells;
> +               ddata->ncells = ARRAY_SIZE(upboard_up2_mfd_cells);
> +               break;
> +       default:
> +               dev_err(dev, "unsupported ID %lu\n", id->driver_data);
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}

> +       ret = upboard_init_gpio(dev);
> +       if (ret) {
> +               if (ret != -EPROBE_DEFER)
> +                       dev_err(dev, "failed to init GPIOs: %d", ret);
> +               return ret;
> +       }

I don't know if probe_err() helper is going to be a part of v4.21
(which this series targets), it would be good to use it.

> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] leds: upboard: Add LED support

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 736 bytes --]

On Sat 2018-10-20 14:17:58, Andy Shevchenko wrote:
1;2802;0c> On Fri, Oct 19, 2018 at 8:27 PM Dan O'Donovan <dan@emutex.com> wrote:
> >
> > From: Javier Arteaga <javier@emutex.com>
> >
> > Allow userspace to use the on-board LEDs as "upboard:<color>:".
> 
> 
> 
> > +#include <linux/kernel.h>
> > +#include <linux/leds.h>
> > +#include <linux/mfd/upboard.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/acpi.h>
> 
> For better maintenance keep this ordered.

This is good as-is.

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v2 3/3] pinctrl: upboard: Add UP2 pinctrl and gpio driver

[Linus Walleij]

On Fri, Oct 19, 2018 at 7:16 PM Dan O'Donovan <dan@emutex.com> wrote:

> From: Javier Arteaga <javier@emutex.com>
>
> The UP2 board features a Raspberry Pi compatible pin header (HAT) and a
> board-specific expansion connector (EXHAT).

Which makes me want to have Eric Anholt's review on this patch
so as to secure basic interoperability with that header.

> Both expose assorted
> functions from either the SoC (such as GPIO, I2C, SPI, UART...) or other
> on-board devices (ADC, FPGA IP blocks...).

OK
Look at how RPi define names for their GPIO lines in the
DTS file:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/bcm2835-rpi-b.dts

Please follow this pattern with your patch.

As you probably do not have device tree or anything similar
for ACPI to name the lines (correct me if I'm wrong)
you can use the .names array in struct gpio_chip for
hardcoding the proper line names.

lsgpio should give the same line names as it does on
the corresponding RPi header IMO.

> +config PINCTRL_UPBOARD
> +       tristate "UP Squared pinctrl and GPIO driver"
> +       depends on ACPI
> +       depends on MFD_UPBOARD
> +       select PINMUX

select GPIOLIB

as you're using it.

> +static int upboard_get_functions_count(struct pinctrl_dev *pctldev)
> +{
> +       return 0;
> +}
> +
> +static int upboard_get_function_groups(struct pinctrl_dev *pctldev,
> +                                      unsigned int selector,
> +                                      const char * const **groups,
> +                                      unsigned int *num_groups)
> +{
> +       *groups = NULL;
> +       *num_groups = 0;
> +       return 0;
> +}
> +
> +static const char *upboard_get_function_name(struct pinctrl_dev *pctldev,
> +                                            unsigned int selector)
> +{
> +       return NULL;
> +}
> +
> +static int upboard_set_mux(struct pinctrl_dev *pctldev, unsigned int function,
> +                          unsigned int group)
> +{
> +       return 0;
> +}

This just looks weird.

There seems to be code to disable pins and turn them into
GPIOs in upboard_gpio_request_enable() but no way to
switch them back to the original function, is that how it works?

I guess it is fine if that is how it's supposed to be used. But
won't some grumpy users come around and complain about
this one day?

We can fix it when it happens though.

> +static int upboard_gpio_request(struct gpio_chip *gc, unsigned int offset)
> +{
> +       struct upboard_pinctrl *pctrl =
> +               container_of(gc, struct upboard_pinctrl, chip);
> +       struct gpio_desc *desc;
> +       int ret;
> +
> +       ret = pinctrl_gpio_request(gc->base + offset);
> +       if (ret)
> +               return ret;
> +
> +       desc = devm_gpiod_get_index(gc->parent, "external", offset, GPIOD_ASIS);
> +       if (IS_ERR(desc))
> +               return PTR_ERR(desc);

No please don't do this. The consumers should request
the gpio, not the driver.

> +static void upboard_gpio_free(struct gpio_chip *gc, unsigned int offset)
> +{
> +       struct upboard_pinctrl *pctrl =
> +               container_of(gc, struct upboard_pinctrl, chip);
> +
> +       if (offset + 1 > pctrl->nsoc_gpios || !pctrl->soc_gpios[offset])
> +               return;
> +
> +       devm_gpiod_put(gc->parent, pctrl->soc_gpios[offset]);

Dito.

> +static int upboard_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
> +{
> +       struct gpio_desc *desc = upboard_offset_to_soc_gpio(gc, offset);
> +
> +       if (IS_ERR(desc))
> +               return PTR_ERR(desc);
> +
> +       return gpiod_get_direction(desc);
> +}

This is just confusing me even more...

If you need pinctrl_gpio_get_direction() then it should be
added to the API in <linux/pinctrl/consumer.h>.

> +static int upboard_gpio_direction_output(struct gpio_chip *gc,
> +                                        unsigned int offset, int value)
> +{
> +       struct gpio_desc *desc = upboard_offset_to_soc_gpio(gc, offset);
> +       int ret;
> +
> +       if (IS_ERR(desc))
> +               return PTR_ERR(desc);
> +
> +       ret = pinctrl_gpio_direction_output(gc->base + offset);
> +       if (ret)
> +               return ret;
> +
> +       return gpiod_direction_output(desc, value);

No this looks confusing too.

> +static int upboard_gpio_get_value(struct gpio_chip *gc, unsigned int offset)
> +{
> +       struct gpio_desc *desc = upboard_offset_to_soc_gpio(gc, offset);
> +
> +       if (IS_ERR(desc))
> +               return PTR_ERR(desc);
> +
> +       return gpiod_get_value(desc);

I don't get this masking one GPIO chip behind another GPIO chip.
It looks really weird.

What we usually have is a GPIO chip in front of a pin controller
utilizing
extern int pinctrl_gpio_request(unsigned gpio);
extern void pinctrl_gpio_free(unsigned gpio);
extern int pinctrl_gpio_direction_input(unsigned gpio);
extern int pinctrl_gpio_direction_output(unsigned gpio);
extern int pinctrl_gpio_set_config(unsigned gpio, unsigned long config);

these things for the GPIO chip to talk to the pin control
back-end.

This driver seems to use a GPIO chip in front of a
GPIO chip and a pin controller too (or something like
that) and that makes me very uneasy.

I need a clear picture of the internal architectur of
the GPIO parts of this driver, why the GPIO accessors
are calling back into the GPIO layer etc. It looks very
unorthodox to me, and I get very confused.

Yours.
Linus Walleij

Re: [PATCH v2 2/3] leds: upboard: Add LED support

[Jacek Anaszewski]

Hi Javier,

Thank you for the patch.

I have few trivial issues below, please take a look.

On 10/19/2018 07:15 PM, Dan O'Donovan wrote:
> From: Javier Arteaga <javier@emutex.com>
> 
> Allow userspace to use the on-board LEDs as "upboard:<color>:".
> 
> Acked-by: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: Javier Arteaga <javier@emutex.com>
> Signed-off-by: Dan O'Donovan <dan@emutex.com>
> ---
>  drivers/leds/Kconfig        |  10 +++++
>  drivers/leds/Makefile       |   1 +
>  drivers/leds/leds-upboard.c | 104 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 115 insertions(+)
>  create mode 100644 drivers/leds/leds-upboard.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 44097a3..0ed8857 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -756,6 +756,16 @@ config LEDS_NIC78BX
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called leds-nic78bx.
>  
> +config LEDS_UPBOARD
> +	tristate "LED support for the UP Squared"
> +	depends on LEDS_CLASS
> +	depends on MFD_UPBOARD
> +	help
> +	  This option enables support for the LEDs on the UP Squared board.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called leds-upboard.
> +
>  comment "LED Triggers"
>  source "drivers/leds/trigger/Kconfig"
>  
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 420b5d2..c85f18f 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -78,6 +78,7 @@ obj-$(CONFIG_LEDS_MT6323)		+= leds-mt6323.o
>  obj-$(CONFIG_LEDS_LM3692X)		+= leds-lm3692x.o
>  obj-$(CONFIG_LEDS_SC27XX_BLTC)		+= leds-sc27xx-bltc.o
>  obj-$(CONFIG_LEDS_LM3601X)		+= leds-lm3601x.o
> +obj-$(CONFIG_LEDS_UPBOARD)		+= leds-upboard.o
>  
>  # LED SPI Drivers
>  obj-$(CONFIG_LEDS_CR0014114)		+= leds-cr0014114.o
> diff --git a/drivers/leds/leds-upboard.c b/drivers/leds/leds-upboard.c
> new file mode 100644
> index 0000000..34a6973
> --- /dev/null
> +++ b/drivers/leds/leds-upboard.c
> @@ -0,0 +1,104 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// UP Board LED driver
> +//
> +// Copyright (c) 2018, Emutex Ltd.
> +//
> +// Author: Javier Arteaga <javier@emutex.com>
> +//
> +
> +#include <linux/kernel.h>
> +#include <linux/leds.h>
> +#include <linux/mfd/upboard.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/acpi.h>

The last include should go first to keep alphabetical order.

> +
> +#define to_upboard_led(cdev) container_of(cdev, struct upboard_led, cdev)
> +
> +static const char * const upboard_led_names[] = {
> +	"upboard:blue:",
> +	"upboard:yellow:",
> +	"upboard:green:",
> +	"upboard:red:",
> +};
> +
> +struct upboard_led {
> +	struct regmap_field *field;
> +	struct led_classdev cdev;
> +};
> +
> +static enum led_brightness upboard_led_brightness_get(struct led_classdev *cdev)
> +{
> +	struct upboard_led *led = to_upboard_led(cdev);
> +	int brightness = 0;
> +
> +	regmap_field_read(led->field, &brightness);

Please check the return value.

> +	return brightness;
> +}
> +
> +static void upboard_led_brightness_set(struct led_classdev *cdev,
> +				       enum led_brightness brightness)
> +{
> +	struct upboard_led *led = to_upboard_led(cdev);
> +
> +	regmap_field_write(led->field, brightness);

Ditto.

> +}
> +
> +static int upboard_led_probe(struct platform_device *pdev)
> +{
> +	unsigned int led_index = pdev->id;
> +	struct device *dev = &pdev->dev;
> +	struct acpi_device *adev;
> +	struct upboard_led *led;
> +	struct regmap *regmap;
> +	struct reg_field conf = {
> +		.reg = UPBOARD_REG_FUNC_EN0,
> +		.lsb = led_index,
> +		.msb = led_index,
> +	};
> +
> +	adev = ACPI_COMPANION(dev);
> +	if (!adev || strcmp(acpi_device_hid(adev), "AANT0F01"))
> +		return -ENODEV;
> +
> +	if (led_index >= ARRAY_SIZE(upboard_led_names))
> +		return -EINVAL;
> +
> +	if (!dev->parent)
> +		return -EINVAL;
> +
> +	regmap = dev_get_regmap(dev->parent, NULL);
> +	if (!regmap)
> +		return -EINVAL;
> +
> +	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
> +	if (!led)
> +		return -ENOMEM;
> +
> +	led->field = devm_regmap_field_alloc(dev, regmap, conf);
> +	if (IS_ERR(led->field))
> +		return PTR_ERR(led->field);
> +
> +	led->cdev.max_brightness = 1;

s/1/LED_ON/

> +	led->cdev.brightness_get = upboard_led_brightness_get;
> +	led->cdev.brightness_set = upboard_led_brightness_set;
> +	led->cdev.name = upboard_led_names[led_index];
> +
> +	return devm_led_classdev_register(dev, &led->cdev);
> +}
> +
> +static struct platform_driver upboard_led_driver = {
> +	.driver = {
> +		.name = "upboard-led",
> +	},
> +};
> +
> +module_platform_driver_probe(upboard_led_driver, upboard_led_probe);
> +
> +MODULE_ALIAS("platform:upboard-led");
> +MODULE_AUTHOR("Javier Arteaga <javier@emutex.com>");
> +MODULE_DESCRIPTION("UP Board LED driver");
> +MODULE_LICENSE("GPL v2");
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 2/3] leds: upboard: Add LED support

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 570 bytes --]

Hi!

> > +	led->field = devm_regmap_field_alloc(dev, regmap, conf);
> > +	if (IS_ERR(led->field))
> > +		return PTR_ERR(led->field);
> > +
> > +	led->cdev.max_brightness = 1;
> 
> s/1/LED_ON/

Actually, I prefer constant 1 here, as it makes it immediately obvious
this supports just 0/1.

Yes, LED_ON is also 1, but I had to grep the header files for
that... (I thought it was 255).

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v2 2/3] leds: upboard: Add LED support

[Jacek Anaszewski]

On 10/23/2018 08:54 PM, Pavel Machek wrote:
> Hi!
> 
>>> +	led->field = devm_regmap_field_alloc(dev, regmap, conf);
>>> +	if (IS_ERR(led->field))
>>> +		return PTR_ERR(led->field);
>>> +
>>> +	led->cdev.max_brightness = 1;
>>
>> s/1/LED_ON/
> 
> Actually, I prefer constant 1 here, as it makes it immediately obvious
> this supports just 0/1.
> 
> Yes, LED_ON is also 1, but I had to grep the header files for
> that... (I thought it was 255).

If we have the enum for that, let's use it.
Here's the commit message of the patch adding LED_ON - it should
be somehow familiar to you - see the ack.

commit 4e552c8cb5bc9137e67e035bab8df6dddbca7384
Author: Andi Shyti <andi.shyti@samsung.com>
Date:   Thu Jan 5 11:34:12 2017 +0900

    leds: add LED_ON brightness as boolean value

    Some devices do not handle the led brightness or simply don't
    care about it. Conceptually said devices want to just switch on
    or off the led. It is useless in this case to have a 255 range
    of brightness, while just having an LED_ON and LED_OFF improves
    the boolean meaning of the led status.

    Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
    Acked-by: Pavel Machek <pavel@ucw.cz>
    Signed-off-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>


-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 2/3] leds: upboard: Add LED support

[Joe Perches]

On Tue, 2018-10-23 at 20:50 +0200, Jacek Anaszewski wrote:
> > diff --git a/drivers/leds/leds-upboard.c b/drivers/leds/leds-upboard.c
> > new file mode 100644
> > index 0000000..34a6973
> > --- /dev/null
> > +++ b/drivers/leds/leds-upboard.c
> > @@ -0,0 +1,104 @@
[]
> > +#include <linux/kernel.h>
> > +#include <linux/leds.h>
> > +#include <linux/mfd/upboard.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/acpi.h>
> 
> The last include should go first to keep alphabetical order.

There is no accepted single kernel style for #include
file ordering.

drivers/leds does not use a single style nor is this
particular variant documented anywhere to my knowledge.

Until such a time when either a local preferred style
document or a treewide preferred style exists, please
stop asking people to modify #include ordering for
various styles like reverse christmas tree by length,
alphabetic ordering, or other individual styles.

My preferred style would always have kernel.h first
as that may help with precompiled headers and overall
kernel compilation time one day.

Re: [PATCH v2 2/3] leds: upboard: Add LED support

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2006 bytes --]

On Tue 2018-10-23 21:09:54, Jacek Anaszewski wrote:
> On 10/23/2018 08:54 PM, Pavel Machek wrote:
> > Hi!
> > 
> >>> +	led->field = devm_regmap_field_alloc(dev, regmap, conf);
> >>> +	if (IS_ERR(led->field))
> >>> +		return PTR_ERR(led->field);
> >>> +
> >>> +	led->cdev.max_brightness = 1;
> >>
> >> s/1/LED_ON/
> > 
> > Actually, I prefer constant 1 here, as it makes it immediately obvious
> > this supports just 0/1.
> > 
> > Yes, LED_ON is also 1, but I had to grep the header files for
> > that... (I thought it was 255).
> 
> If we have the enum for that, let's use it.
> Here's the commit message of the patch adding LED_ON - it should
> be somehow familiar to you - see the ack.

Well .. brightness = LED_ON; is good usage. max_brightness = LED_ON is
IMO less readable than max_brightness = 1.

Looking at situation again... Having LED_ON and LED_FULL, with some
leds having max brightness of 1023, so LED_FULL is not really full
brightness any more... Maybe it is time to get rid of the enum, and
make it plain int. It does not really enumarate anything, and it does
not help readability, either.
								Pavel


> commit 4e552c8cb5bc9137e67e035bab8df6dddbca7384
> Author: Andi Shyti <andi.shyti@samsung.com>
> Date:   Thu Jan 5 11:34:12 2017 +0900
> 
>     leds: add LED_ON brightness as boolean value
> 
>     Some devices do not handle the led brightness or simply don't
>     care about it. Conceptually said devices want to just switch on
>     or off the led. It is useless in this case to have a 255 range
>     of brightness, while just having an LED_ON and LED_OFF improves
>     the boolean meaning of the led status.
> 
>     Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
>     Acked-by: Pavel Machek <pavel@ucw.cz>
>     Signed-off-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> 
> 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v2 2/3] leds: upboard: Add LED support

[Jacek Anaszewski]

On 10/23/2018 09:23 PM, Joe Perches wrote:
> On Tue, 2018-10-23 at 20:50 +0200, Jacek Anaszewski wrote:
>>> diff --git a/drivers/leds/leds-upboard.c b/drivers/leds/leds-upboard.c
>>> new file mode 100644
>>> index 0000000..34a6973
>>> --- /dev/null
>>> +++ b/drivers/leds/leds-upboard.c
>>> @@ -0,0 +1,104 @@
> []
>>> +#include <linux/kernel.h>
>>> +#include <linux/leds.h>
>>> +#include <linux/mfd/upboard.h>
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/acpi.h>
>>
>> The last include should go first to keep alphabetical order.
> 
> There is no accepted single kernel style for #include
> file ordering.

Well, I myself have been asked several times for sorting
includes, that's why I keep requiring it for the LED subsystem too.

When it's by the occasion of a new driver submission, it costs
virtually nothing. And it allows for avoiding any prospective
noise on the list due to the submissions like "Order includes".

As a first shot the following returns 77:

git log | grep -i "include.*alphabetical" | wc -l

Aside of that, in case of this particular patch the intention seemingly
was to have includes ordered lexicographically, since only the last
item didn't fit for this pattern.

> drivers/leds does not use a single style nor is this
> particular variant documented anywhere to my knowledge.

Unspecified kind of includes sorting is mentioned in the
Documentation/process/coding-style.rst, line 637.

> Until such a time when either a local preferred style
> document or a treewide preferred style exists, please
> stop asking people to modify #include ordering for
> various styles like reverse christmas tree by length,
> alphabetic ordering, or other individual style
> My preferred style would always have kernel.h first
> as that may help with precompiled headers and overall
> kernel compilation time one day.

If that will happen we'll see massive rearrangement of includes
anyway.

But OK, I've skimmed through other subsystems and core kernel
files and realized that this is indeed often not preserved.

So, provided there are no other strong arguments in favor
of sorting, I will give up this nitpicking from now on.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 2/3] leds: upboard: Add LED support

[Andy Shevchenko]

On Tue, Oct 23, 2018 at 12:23:13PM -0700, Joe Perches wrote:
> On Tue, 2018-10-23 at 20:50 +0200, Jacek Anaszewski wrote:
> > > diff --git a/drivers/leds/leds-upboard.c b/drivers/leds/leds-upboard.c
> > > new file mode 100644
> > > index 0000000..34a6973
> > > --- /dev/null
> > > +++ b/drivers/leds/leds-upboard.c
> > > @@ -0,0 +1,104 @@
> []
> > > +#include <linux/kernel.h>
> > > +#include <linux/leds.h>
> > > +#include <linux/mfd/upboard.h>
> > > +#include <linux/module.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/acpi.h>
> > 
> > The last include should go first to keep alphabetical order.
> 
> There is no accepted single kernel style for #include
> file ordering.

There is a rule of (subtly) better maintenance.
If you need to add / remove some header later in a (long) list of unordered
list, it would be error prone.

Just run `make includecheck` and see the result.

I personally fixed some header duplications and removal of init.h in unsorted
lists, which have been missed by some reasons.

> drivers/leds does not use a single style nor is this
> particular variant documented anywhere to my knowledge.

Neither does kernel in general.
But kernel is evolving and styles also. When you do such statement consider to
divide by a time period when certain code was pushed to upstream.

> Until such a time when either a local preferred style
> document or a treewide preferred style exists, please
> stop asking people to modify #include ordering for
> various styles like reverse christmas tree by length,
> alphabetic ordering, or other individual styles.

Why? It makes a sense to ask for new code (and even for patches against old one in some cases).

> My preferred style would always have kernel.h first
> as that may help with precompiled headers and overall
> kernel compilation time one day.

How ordering would screw this up?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] leds: upboard: Add LED support

[Joe Perches]

On Wed, 2018-10-24 at 13:13 +0300, Andy Shevchenko wrote:
> On Tue, Oct 23, 2018 at 12:23:13PM -0700, Joe Perches wrote:
> > Until such a time when either a local preferred style
> > document or a treewide preferred style exists, please
> > stop asking people to modify #include ordering for
> > various styles like reverse christmas tree by length,
> > alphabetic ordering, or other individual styles.
> 
> Why? It makes a sense to ask for new code (and even for patches against old one in some cases).

It's just a nit and frequently impossible to require as
ordering dependencies between include files do exist.

> > My preferred style would always have kernel.h first
> > as that may help with precompiled headers and overall
> > kernel compilation time one day.
> 
> How ordering would screw this up?

gcc has many limits on the use of precompiled headers.

https://gcc.gnu.org/onlinedocs/gcc/Precompiled-Headers.html

Precompiled headers are often shared by multiple
compilation units and precompilation can be stopped
after a specific header.

Re: [PATCH v2 0/3] UP Squared board drivers

[Andy Shevchenko]

On Fri, Oct 19, 2018 at 06:15:31PM +0100, Dan O'Donovan wrote:
> Hi all,
> 
> This series adds platform support for AAEON's UP Squared, a x86 SBC
> based on Apollo Lake [1]. Specifically, it enables control for the board
> pin headers and for the LEDs, as both of these features are only
> available behind a FPGA-based platform controller.
> 
> This is structured around a MFD driver that provides a regmap interface
> to that platform controller to each MFD cell.
> 
> The series is carved out from an out-of-tree support patchset, initially
> written for the original UP Board [2] and then extended through the past
> year to support new entries in the lineup (UP Board, UP Squared and
> UP Core) [3].
> 
> Here we only submit support for UP Squared to simplify review. Still,
> the driver is designed so that it can easily gain support for the other
> boards.
> 
> V1 was originally submitted by Javier Arteaga in April 2018 as an RFC [4].
> This V2 aims to address the feedback gratefully received from that.

Are you attending ELCE in Edinburgh?

> 
> [1]: http://www.up-board.org/upsquared/specifications-up2/
> [2]: https://lkml.kernel.org/r/1467648434-29080-1-git-send-email-dan@emutex.com
> [3]: https://github.com/emutex/ubilinux-kernel/commits/upboard-4.9
> [4]: https://lore.kernel.org/lkml/20180421085009.28773-1-javier@emutex.com/
> 
> V2:
> * Rewrote regmap r/w handlers to avoid duplication and improve readability
> * Simplified data passing between MFD parent and child device drivers
> * Removed incorrect __init attribute from probe functions
> * Moved MFD cell definitions into same patch as MFD driver and simplified them
> * Promoted some dev_dbg instances to dev_err for probe error cases
> * Decoupled MFD cell reference from ACPI driver data
> * Miscellanous coding style improvements
> * Fixed license mismatches
> 
> Javier Arteaga (3):
>   mfd: upboard: Add UP2 platform controller driver
>   leds: upboard: Add LED support
>   pinctrl: upboard: Add UP2 pinctrl and gpio driver
> 
>  drivers/leds/Kconfig              |  10 +
>  drivers/leds/Makefile             |   1 +
>  drivers/leds/leds-upboard.c       | 104 ++++++++
>  drivers/mfd/Kconfig               |  17 ++
>  drivers/mfd/Makefile              |   1 +
>  drivers/mfd/upboard.c             | 344 +++++++++++++++++++++++++
>  drivers/pinctrl/Kconfig           |  13 +
>  drivers/pinctrl/Makefile          |   1 +
>  drivers/pinctrl/pinctrl-upboard.c | 519 ++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/upboard.h       |  44 ++++
>  10 files changed, 1054 insertions(+)
>  create mode 100644 drivers/leds/leds-upboard.c
>  create mode 100644 drivers/mfd/upboard.c
>  create mode 100644 drivers/pinctrl/pinctrl-upboard.c
>  create mode 100644 include/linux/mfd/upboard.h
> 
> -- 
> 2.7.4
> 
> 
> ------
> This email has been scanned for spam and malware by The Email Laundry.
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] leds: upboard: Add LED support

[Jacek Anaszewski]

On 10/23/2018 09:30 PM, Pavel Machek wrote:
> On Tue 2018-10-23 21:09:54, Jacek Anaszewski wrote:
>> On 10/23/2018 08:54 PM, Pavel Machek wrote:
>>> Hi!
>>>
>>>>> +	led->field = devm_regmap_field_alloc(dev, regmap, conf);
>>>>> +	if (IS_ERR(led->field))
>>>>> +		return PTR_ERR(led->field);
>>>>> +
>>>>> +	led->cdev.max_brightness = 1;
>>>>
>>>> s/1/LED_ON/
>>>
>>> Actually, I prefer constant 1 here, as it makes it immediately obvious
>>> this supports just 0/1.
>>>
>>> Yes, LED_ON is also 1, but I had to grep the header files for
>>> that... (I thought it was 255).
>>
>> If we have the enum for that, let's use it.
>> Here's the commit message of the patch adding LED_ON - it should
>> be somehow familiar to you - see the ack.
> 
> Well .. brightness = LED_ON; is good usage. max_brightness = LED_ON is
> IMO less readable than max_brightness = 1.
> 
> Looking at situation again... Having LED_ON and LED_FULL, with some
> leds having max brightness of 1023, so LED_FULL is not really full
> brightness any more... Maybe it is time to get rid of the enum, and
> make it plain int. It does not really enumarate anything, and it does
> not help readability, either.

I agree that it introduces confusion.

Yet, there are many out of LED subsystem files to update:

find -name "*.c" -o -name "*.h" | xargs grep  "enum led_brightness" |
awk -F: '{print $1}' | sort -u | grep -v "leds" | wc -l

returns 87.

>> commit 4e552c8cb5bc9137e67e035bab8df6dddbca7384
>> Author: Andi Shyti <andi.shyti@samsung.com>
>> Date:   Thu Jan 5 11:34:12 2017 +0900
>>
>>     leds: add LED_ON brightness as boolean value
>>
>>     Some devices do not handle the led brightness or simply don't
>>     care about it. Conceptually said devices want to just switch on
>>     or off the led. It is useless in this case to have a 255 range
>>     of brightness, while just having an LED_ON and LED_OFF improves
>>     the boolean meaning of the led status.
>>
>>     Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
>>     Acked-by: Pavel Machek <pavel@ucw.cz>
>>     Signed-off-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
>>
>>
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 2/3] leds: upboard: Add LED support

[Andy Shevchenko]

On Wed, Oct 24, 2018 at 10:07:30PM +0200, Jacek Anaszewski wrote:

> Yet, there are many out of LED subsystem files to update:
> 
> find -name "*.c" -o -name "*.h" | xargs grep  "enum led_brightness" |
> awk -F: '{print $1}' | sort -u | grep -v "leds" | wc -l
> 
> returns 87.

Side note: 

git grep -n 'enum led_brightness' | ...

a bit more effitient.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/3] mfd: upboard: Add UP2 platform controller driver

[Lee Jones]

On Sat, 20 Oct 2018, Andy Shevchenko wrote:

> On Fri, Oct 19, 2018 at 8:26 PM Dan O'Donovan <dan@emutex.com> wrote:
> >
> > From: Javier Arteaga <javier@emutex.com>
> >
> > UP Squared (UP2) is a x86 SBC from AAEON based on Intel Apollo Lake. It
> > features a MAX 10 FPGA that routes lines from both SoC and on-board
> > devices to two I/O headers:
> >
> >                                 +------------------------+
> >                                 | 40-pin RPi-like header |
> >                          +------|         (HAT)          |
> >                          |      +------------------------+
> >     +-------+    +--------+
> >     |       |    |        |     +------------------------+
> >     |  SoC  |----|  FPGA  |-----|  Custom UP2 pin header |
> >     |       |    |        |     |        (EXHAT)         |
> >     +-------+    +--------+     +------------------------+
> >                          |
> >                          +------* On-board devices: LED, VLS...
> >
> > This is intended to enable vendor-specific applications to customize I/O
> > header pinout, as well as include low-latency functionality. It also
> > performs voltage level translation between the SoC (1.8V) and HAT header
> > (3.3V).
> >
> > Out of the box, this block implements a platform controller with a
> > GPIO-bitbanged control interface. It's enumerated by ACPI and provides
> > registers to control:
> >
> > - Configuration of all FPGA-routed header lines. These can be driven
> >   SoC-to-header, header-to-SoC or set in high impedance.
> >
> > - On-board LEDs and enable lines for other platform devices.
> >
> > Add core support for this platform controller as a MFD device, exposing
> > these registers as a regmap.
> 
> Can we see a link to or an excerpt of ACPI table for this device?
> 
> > +#define set_clear(u, x) gpiod_set_value((u)->clear_gpio, (x))
> > +#define set_strobe(u, x) gpiod_set_value((u)->strobe_gpio, (x))
> > +#define set_datain(u, x) gpiod_set_value((u)->datain_gpio, (x))
> > +#define get_dataout(u) gpiod_get_value((u)->dataout_gpio)
> 
> I think these macros don't bring much value. (Up to you and Lee to decide)

I agree.

In fact I think they only serve to obfuscate instead of clarify.

[...]

> > +               set_strobe(ddata, 0);
> > +               set_strobe(ddata, 1);
> > +               *val |= get_dataout(ddata) << i;
> > +       }
> > +}
> 
> > +static int upboard_check_supported(struct device *dev, struct regmap *regmap)
> > +{
> > +       uint8_t manufacturer_id, build, major, minor, patch;
> > +       unsigned int platform_id, firmware_id;
> > +       int ret;
> 
> > +       manufacturer_id = platform_id & 0xff;
> 
> Redundant & 0xff part.

Maybe not required, but lets the reader know it's intended.

[...]

> > +       ret = upboard_init_gpio(dev);
> > +       if (ret) {
> > +               if (ret != -EPROBE_DEFER)
> > +                       dev_err(dev, "failed to init GPIOs: %d", ret);
> > +               return ret;
> > +       }
> 
> I don't know if probe_err() helper is going to be a part of v4.21
> (which this series targets), it would be good to use it.

Interesting.  What does it do?

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 1/3] mfd: upboard: Add UP2 platform controller driver

[Andy Shevchenko]

On Thu, Oct 25, 2018 at 12:05:05PM +0100, Lee Jones wrote:
> On Sat, 20 Oct 2018, Andy Shevchenko wrote:
> > On Fri, Oct 19, 2018 at 8:26 PM Dan O'Donovan <dan@emutex.com> wrote:

> > > +       ret = upboard_init_gpio(dev);
> > > +       if (ret) {
> > > +               if (ret != -EPROBE_DEFER)
> > > +                       dev_err(dev, "failed to init GPIOs: %d", ret);
> > > +               return ret;
> > > +       }
> > 
> > I don't know if probe_err() helper is going to be a part of v4.21
> > (which this series targets), it would be good to use it.
> 
> Interesting.  What does it do?

The code will tell better than I
https://lore.kernel.org/lkml/20181017132205.5204-1-a.hajda@samsung.com/


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] leds: upboard: Add LED support

[Jacek Anaszewski]

On 10/25/2018 11:22 AM, Andy Shevchenko wrote:
> On Wed, Oct 24, 2018 at 10:07:30PM +0200, Jacek Anaszewski wrote:
> 
>> Yet, there are many out of LED subsystem files to update:
>>
>> find -name "*.c" -o -name "*.h" | xargs grep  "enum led_brightness" |
>> awk -F: '{print $1}' | sort -u | grep -v "leds" | wc -l
>>
>> returns 87.
> 
> Side note: 
> 
> git grep -n 'enum led_brightness' | ...
> 
> a bit more effitient.

Right, thanks for the hint.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 3/3] pinctrl: upboard: Add UP2 pinctrl and gpio driver

[Dan O'Donovan]

Thanks for your review feedback, Andy!  I'll send a v3 shortly with those changes you suggested.  I've added some comments inline below.

On 10/20/2018 12:40 PM, Andy Shevchenko wrote:
> On Fri, Oct 19, 2018 at 8:24 PM Dan O'Donovan <dan@emutex.com> wrote:
>
>> +static int upboard_get_functions_count(struct pinctrl_dev *pctldev)
>> +{
>> +       return 0;
>> +}
>> +
>> +static int upboard_get_function_groups(struct pinctrl_dev *pctldev,
>> +                                      unsigned int selector,
>> +                                      const char * const **groups,
>> +                                      unsigned int *num_groups)
>> +{
>> +       *groups = NULL;
>> +       *num_groups = 0;
>> +       return 0;
>> +}
>> +
>> +static const char *upboard_get_function_name(struct pinctrl_dev *pctldev,
>> +                                            unsigned int selector)
>> +{
>> +       return NULL;
>> +}
>> +
>> +static int upboard_set_mux(struct pinctrl_dev *pctldev, unsigned int function,
>> +                          unsigned int group)
>> +{
>> +       return 0;
>> +}
> Hmm... Do you need those stubs? Same Q for other stubs in the file.
It looks like they're required by pinctrl core, which returns an error if they're not provided.

>> +static int upboard_gpio_request_enable(struct pinctrl_dev *pctldev,
>> +                                      struct pinctrl_gpio_range *range,
>> +                                      unsigned int pin)
>> +{
>> +       const struct pin_desc * const pd = pin_desc_get(pctldev, pin);
>> +       const struct upboard_pin *p;
>> +       int ret;
>> +
>> +       if (!pd)
>> +               return -EINVAL;
> When it possible to happen?
> Same Q for the rest same excerpts.
Agreed, it shouldn't be possible.  I will remove these checks.

>> +
>> +       if (offset + 1 > pctrl->nsoc_gpios || !pctrl->soc_gpios[offset])
>> +               return ERR_PTR(-ENODEV);
> offset >= ?
> Is it even possible?
Agreed, it shouldn't be possible.  I will remove these checks.

>> +static int upboard_pinctrl_probe(struct platform_device *pdev)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       struct pinctrl_desc *pctldesc;
>> +       struct upboard_pinctrl *pctrl;
>> +       struct upboard_pin *pins;
>> +       struct acpi_device *adev;
>> +       struct regmap *regmap;
>> +       unsigned int i;
>> +       int ret;
>> +       adev = ACPI_COMPANION(dev);
>> +       if (!adev || strcmp(acpi_device_hid(adev), "AANT0F01"))
>> +               return -ENODEV;
> Same Q as per LED driver.
I agree.  I will remove this check, both here and in the LED driver.

>> +               if (pd->drv_data) {
>> +                       struct reg_field *field = pd->drv_data;
>> +
>> +                       pin->func_en = devm_regmap_field_alloc(dev, regmap,
>> +                                                              *field);
>> +                       if (IS_ERR(pin->func_en))
>> +                               return PTR_ERR(pin->func_en);
>> +               }
>> +
>> +               pin->gpio_en = upboard_field_alloc(dev, regmap,
>> +                                                  UPBOARD_REG_GPIO_EN0, i);
>> +               if (IS_ERR(pin->gpio_en))
>> +                       return PTR_ERR(pin->gpio_en);
>> +
>> +               pin->gpio_dir = upboard_field_alloc(dev, regmap,
>> +                                                   UPBOARD_REG_GPIO_DIR0, i);
>> +               if (IS_ERR(pin->gpio_dir))
>> +                       return PTR_ERR(pin->gpio_dir);
>> +
>> +               ((struct pinctrl_pin_desc *)pd)->drv_data = pin;
> I'm not sure I understand the purpose of this casting.
When the pd pointer is retrieved from struct pinctrl_desc, it has a const constraint.  The purpose of the cast is to bypass that constraint for this use case, because this code is allocating and setting drv_data dynamically here at run-time rather than at compile-time.

Re: [PATCH v2 1/3] mfd: upboard: Add UP2 platform controller driver

[Dan O'Donovan]

On 10/20/2018 12:49 PM, Andy Shevchenko wrote:
> On Fri, Oct 19, 2018 at 8:26 PM Dan O'Donovan <dan@emutex.com> wrote:
>> From: Javier Arteaga <javier@emutex.com>
>>
>> UP Squared (UP2) is a x86 SBC from AAEON based on Intel Apollo Lake. It
>> features a MAX 10 FPGA that routes lines from both SoC and on-board
>> devices to two I/O headers:
>>
>>                                 +------------------------+
>>                                 | 40-pin RPi-like header |
>>                          +------|         (HAT)          |
>>                          |      +------------------------+
>>     +-------+    +--------+
>>     |       |    |        |     +------------------------+
>>     |  SoC  |----|  FPGA  |-----|  Custom UP2 pin header |
>>     |       |    |        |     |        (EXHAT)         |
>>     +-------+    +--------+     +------------------------+
>>                          |
>>                          +------* On-board devices: LED, VLS...
>>
>> This is intended to enable vendor-specific applications to customize I/O
>> header pinout, as well as include low-latency functionality. It also
>> performs voltage level translation between the SoC (1.8V) and HAT header
>> (3.3V).
>>
>> Out of the box, this block implements a platform controller with a
>> GPIO-bitbanged control interface. It's enumerated by ACPI and provides
>> registers to control:
>>
>> - Configuration of all FPGA-routed header lines. These can be driven
>>   SoC-to-header, header-to-SoC or set in high impedance.
>>
>> - On-board LEDs and enable lines for other platform devices.
>>
>> Add core support for this platform controller as a MFD device, exposing
>> these registers as a regmap.
> Can we see a link to or an excerpt of ACPI table for this device?
Sure: https://gist.github.com/Dan-Emutex/6382c25f4b8b8cdd80e6056889cdf48b
I'll add this to the cover letter for v3 as well.

>
>> +#define set_clear(u, x) gpiod_set_value((u)->clear_gpio, (x))
>> +#define set_strobe(u, x) gpiod_set_value((u)->strobe_gpio, (x))
>> +#define set_datain(u, x) gpiod_set_value((u)->datain_gpio, (x))
>> +#define get_dataout(u) gpiod_get_value((u)->dataout_gpio)
> I think these macros don't bring much value. (Up to you and Lee to decide)
These were added in response to one of your review comments on the original patch-set ;)
I think they do slightly improve the code readability so I'll leave them there if there are no objections.

>
>
>> +static void __reg_io_write(const struct upboard_ddata * const ddata,
>> +                          unsigned int size, unsigned int val)
>> +{
>> +       int i;
>> +
>> +       /*
>> +        * DATAIN is latched on each rising edge of the STROBE signal.
>> +        * Data (register address or value) is sent MSb first.
>> +        */
>> +       for (i = size - 1; i >= 0; i--) {
> while (size--)
Yep, much neater.  I'll change that, thanks!  Ditto for the other comments on this file.+ ret = upboard_init_gpio(dev);

>> +       if (ret) {
>> +               if (ret != -EPROBE_DEFER)
>> +                       dev_err(dev, "failed to init GPIOs: %d", ret);
>> +               return ret;
>> +       }
> I don't know if probe_err() helper is going to be a part of v4.21
> (which this series targets), it would be good to use it.
I left this out for now in case it causes any awkwardness with merging.  Hope that's ok.

[PATCH v3 0/3] UP Squared board drivers

[Dan O'Donovan]

Hi all,

This series adds platform support for AAEON's UP Squared, a x86 SBC
based on Apollo Lake [1]. Specifically, it enables control for the board
pin headers and for the LEDs, as both of these features are only
available behind a FPGA-based platform controller.

This is structured around a MFD driver that provides a regmap interface
to that platform controller to each MFD cell.

The series is carved out from an out-of-tree support patchset, initially
written for the original UP Board [2] and then extended through the past
year to support new entries in the lineup (UP Board, UP Squared and
UP Core) [3].

Here we only submit support for UP Squared to simplify review. Still,
the driver is designed so that it can easily gain support for the other
boards.

V1 was originally submitted by Javier Arteaga in April 2018 as an RFC [4].

The relevant ACPI SSDT from UP Squared can be found here: [5]

[1]: http://www.up-board.org/upsquared/specifications-up2/
[2]: https://lkml.kernel.org/r/1467648434-29080-1-git-send-email-dan@emutex.com
[3]: https://github.com/emutex/ubilinux-kernel/commits/upboard-4.9
[4]: https://lore.kernel.org/lkml/20180421085009.28773-1-javier@emutex.com/
[5]: https://gist.github.com/Dan-Emutex/6382c25f4b8b8cdd80e6056889cdf48b

V3:
* Javier's address is no longer valid. Replaced From: with my own address.
* Added error checks in LEDs driver, suggested by Jacek Anaszewski
* Added minor tidy-ups in MFD and pinctrl drivers, suggested by Andy Shevchenko
* Added link to relevant ACPI extract above [5]

V2:
* Rewrote regmap r/w handlers to avoid duplication and improve readability
* Simplified data passing between MFD parent and child device drivers
* Removed incorrect __init attribute from probe functions
* Moved MFD cell definitions into same patch as MFD driver and simplified them
* Promoted some dev_dbg instances to dev_err for probe error cases
* Decoupled MFD cell reference from ACPI driver data
* Miscellanous coding style improvements
* Fixed license mismatches

Dan O'Donovan (3):
  mfd: upboard: Add UP2 platform controller driver
  leds: upboard: Add LED support
  pinctrl: upboard: Add UP2 pinctrl and gpio driver

 drivers/leds/Kconfig              |  10 +
 drivers/leds/Makefile             |   1 +
 drivers/leds/leds-upboard.c       | 103 ++++++++
 drivers/mfd/Kconfig               |  17 ++
 drivers/mfd/Makefile              |   1 +
 drivers/mfd/upboard.c             | 336 ++++++++++++++++++++++++++
 drivers/pinctrl/Kconfig           |  13 +
 drivers/pinctrl/Makefile          |   1 +
 drivers/pinctrl/pinctrl-upboard.c | 496 ++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/upboard.h       |  44 ++++
 10 files changed, 1022 insertions(+)
 create mode 100644 drivers/leds/leds-upboard.c
 create mode 100644 drivers/mfd/upboard.c
 create mode 100644 drivers/pinctrl/pinctrl-upboard.c
 create mode 100644 include/linux/mfd/upboard.h

-- 
2.7.4


------
This email has been scanned for spam and malware by The Email Laundry.

[PATCH v3 1/3] mfd: upboard: Add UP2 platform controller driver

[Dan O'Donovan]

UP Squared (UP2) is a x86 SBC from AAEON based on Intel Apollo Lake. It
features a MAX 10 FPGA that routes lines from both SoC and on-board
devices to two I/O headers:

                                +------------------------+
                                | 40-pin RPi-like header |
                         +------|         (HAT)          |
                         |      +------------------------+
    +-------+    +--------+
    |       |    |        |     +------------------------+
    |  SoC  |----|  FPGA  |-----|  Custom UP2 pin header |
    |       |    |        |     |        (EXHAT)         |
    +-------+    +--------+     +------------------------+
                         |
                         +------* On-board devices: LED, VLS...

This is intended to enable vendor-specific applications to customize I/O
header pinout, as well as include low-latency functionality. It also
performs voltage level translation between the SoC (1.8V) and HAT header
(3.3V).

Out of the box, this block implements a platform controller with a
GPIO-bitbanged control interface. It's enumerated by ACPI and provides
registers to control:

- Configuration of all FPGA-routed header lines. These can be driven
  SoC-to-header, header-to-SoC or set in high impedance.

- On-board LEDs and enable lines for other platform devices.

Add core support for this platform controller as a MFD device, exposing
these registers as a regmap.

Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Dan O'Donovan <dan@emutex.com>
---
 drivers/mfd/Kconfig         |  17 +++
 drivers/mfd/Makefile        |   1 +
 drivers/mfd/upboard.c       | 336 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/upboard.h |  44 ++++++
 4 files changed, 398 insertions(+)
 create mode 100644 drivers/mfd/upboard.c
 create mode 100644 include/linux/mfd/upboard.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 8c5dfdc..cccb901 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1871,6 +1871,23 @@ config MFD_STM32_TIMERS
 	  for PWM and IIO Timer. This driver allow to share the
 	  registers between the others drivers.
 
+config MFD_UPBOARD
+	tristate "UP Squared"
+	depends on ACPI
+	depends on GPIOLIB
+	select MFD_CORE
+	select REGMAP
+	help
+	  If you say yes here you get support for the platform controller
+	  of the UP Squared single-board computer.
+
+	  This driver provides common support for accessing the device,
+	  additional drivers must be enabled in order to use the
+	  functionality of the device.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called "upboard".
+
 menu "Multimedia Capabilities Port drivers"
 	depends on ARCH_SA1100
 
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 12980a4..593b86f 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -242,3 +242,4 @@ obj-$(CONFIG_MFD_SC27XX_PMIC)	+= sprd-sc27xx-spi.o
 obj-$(CONFIG_RAVE_SP_CORE)	+= rave-sp.o
 obj-$(CONFIG_MFD_ROHM_BD718XX)	+= rohm-bd718x7.o
 
+obj-$(CONFIG_MFD_UPBOARD)  += upboard.o
diff --git a/drivers/mfd/upboard.c b/drivers/mfd/upboard.c
new file mode 100644
index 0000000..8b88731
--- /dev/null
+++ b/drivers/mfd/upboard.c
@@ -0,0 +1,336 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// UP Board platform controller driver
+//
+// Copyright (c) 2018, Emutex Ltd.
+//
+// Author: Javier Arteaga <javier@emutex.com>
+//
+
+#include <linux/acpi.h>
+#include <linux/gpio/consumer.h>
+#include <linux/kernel.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/upboard.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define UPBOARD_FW_BUILD_SHIFT 12
+#define UPBOARD_FW_MAJOR_SHIFT 8
+#define UPBOARD_FW_MINOR_SHIFT 4
+#define UPBOARD_FW_PATCH_SHIFT 0
+
+#define UPBOARD_FW_BUILD(id) (((id) >> UPBOARD_FW_BUILD_SHIFT) & 0x0f)
+#define UPBOARD_FW_MAJOR(id) (((id) >> UPBOARD_FW_MAJOR_SHIFT) & 0x0f)
+#define UPBOARD_FW_MINOR(id) (((id) >> UPBOARD_FW_MINOR_SHIFT) & 0x0f)
+#define UPBOARD_FW_PATCH(id) (((id) >> UPBOARD_FW_PATCH_SHIFT) & 0x0f)
+
+#define AAEON_MANUFACTURER_ID 0x01
+#define SUPPORTED_FW_MAJOR 0x0
+
+/* MSb of 8-bit address is an R/W flag */
+#define UPBOARD_ADDRESS_SIZE  8
+#define UPBOARD_READ_FLAG     BIT(7)
+
+enum upboard_id {
+	UPBOARD_ID_UP2 = 1,
+};
+
+struct upboard_ddata {
+	struct gpio_desc *clear_gpio;
+	struct gpio_desc *strobe_gpio;
+	struct gpio_desc *datain_gpio;
+	struct gpio_desc *dataout_gpio;
+	const struct regmap_config *regmapconf;
+	const struct mfd_cell *cells;
+	size_t ncells;
+};
+
+/*
+ * UP boards include a platform controller with a proprietary GPIO-bitbanged
+ * control interface to access its configuration registers.
+ *
+ * The following macros and functions implement the read/write handlers for
+ * that interface, to provide a regmap-based abstraction for the controller.
+ */
+
+#define set_clear(u, x) gpiod_set_value((u)->clear_gpio, (x))
+#define set_strobe(u, x) gpiod_set_value((u)->strobe_gpio, (x))
+#define set_datain(u, x) gpiod_set_value((u)->datain_gpio, (x))
+#define get_dataout(u) gpiod_get_value((u)->dataout_gpio)
+
+static void __reg_io_start(const struct upboard_ddata * const ddata)
+{
+	/*
+	 * CLEAR signal must be pulsed low before any register access.
+	 * This resets internal counters in the controller and marks
+	 * the start of a new register access.
+	 */
+	set_clear(ddata, 0);
+	set_clear(ddata, 1);
+}
+
+static void __reg_io_end(const struct upboard_ddata * const ddata)
+{
+	/*
+	 * STROBE signal must be cycled again to mark the end of a register
+	 * access.  Partial register accesses are discarded harmlessly
+	 * by the controller if this final strobe cycle is not sent
+	 */
+	set_strobe(ddata, 0);
+	set_strobe(ddata, 1);
+}
+
+static void __reg_io_write(const struct upboard_ddata * const ddata,
+			   unsigned int size, unsigned int val)
+{
+	/*
+	 * DATAIN is latched on each rising edge of the STROBE signal.
+	 * Data (register address or value) is sent MSb first.
+	 */
+	while (size--) {
+		set_strobe(ddata, 0);
+		set_datain(ddata, (val >> size) & 0x1);
+		set_strobe(ddata, 1);
+	}
+}
+
+static void __reg_io_read(const struct upboard_ddata * const ddata,
+			  unsigned int size, unsigned int *val)
+{
+	/*
+	 * DATAOUT is latched on on each rising edge of the STROBE signal.
+	 * Data (register value) is received MSb first.
+	 */
+	*val = 0;
+	while (size--) {
+		set_strobe(ddata, 0);
+		set_strobe(ddata, 1);
+		*val |= get_dataout(ddata) << size;
+	}
+}
+
+static int upboard_reg_read(void *context, unsigned int reg, unsigned int *val)
+{
+	const struct upboard_ddata * const ddata = context;
+
+	__reg_io_start(ddata);
+	__reg_io_write(ddata, UPBOARD_ADDRESS_SIZE, reg | UPBOARD_READ_FLAG);
+	__reg_io_read(ddata, UPBOARD_REGISTER_SIZE, val);
+	__reg_io_end(ddata);
+
+	return 0;
+}
+
+static int upboard_reg_write(void *context, unsigned int reg, unsigned int val)
+{
+	const struct upboard_ddata * const ddata = context;
+
+	__reg_io_start(ddata);
+	__reg_io_write(ddata, UPBOARD_ADDRESS_SIZE, reg);
+	__reg_io_write(ddata, UPBOARD_REGISTER_SIZE, val);
+	__reg_io_end(ddata);
+
+	return 0;
+}
+
+/* UP Squared */
+
+static const struct regmap_range upboard_up2_readable_ranges[] = {
+	regmap_reg_range(UPBOARD_REG_PLATFORM_ID, UPBOARD_REG_FIRMWARE_ID),
+	regmap_reg_range(UPBOARD_REG_FUNC_EN0, UPBOARD_REG_FUNC_EN1),
+	regmap_reg_range(UPBOARD_REG_GPIO_EN0, UPBOARD_REG_GPIO_EN2),
+	regmap_reg_range(UPBOARD_REG_GPIO_DIR0, UPBOARD_REG_GPIO_DIR2),
+};
+
+static const struct regmap_range upboard_up2_writable_ranges[] = {
+	regmap_reg_range(UPBOARD_REG_FUNC_EN0, UPBOARD_REG_FUNC_EN1),
+	regmap_reg_range(UPBOARD_REG_GPIO_EN0, UPBOARD_REG_GPIO_EN2),
+	regmap_reg_range(UPBOARD_REG_GPIO_DIR0, UPBOARD_REG_GPIO_DIR2),
+};
+
+static const struct regmap_access_table upboard_up2_readable_table = {
+	.yes_ranges = upboard_up2_readable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(upboard_up2_readable_ranges),
+};
+
+static const struct regmap_access_table upboard_up2_writable_table = {
+	.yes_ranges = upboard_up2_writable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(upboard_up2_writable_ranges),
+};
+
+static const struct regmap_config upboard_up2_regmap_config = {
+	.reg_bits = UPBOARD_ADDRESS_SIZE,
+	.val_bits = UPBOARD_REGISTER_SIZE,
+	.max_register = UPBOARD_REG_MAX,
+	.reg_read = upboard_reg_read,
+	.reg_write = upboard_reg_write,
+	.fast_io = false,
+	.cache_type = REGCACHE_RBTREE,
+	.rd_table = &upboard_up2_readable_table,
+	.wr_table = &upboard_up2_writable_table,
+};
+
+static const struct mfd_cell upboard_up2_mfd_cells[] = {
+	{
+		.name = "upboard-led",
+		.id = 0,
+	},
+	{
+		.name = "upboard-led",
+		.id = 1,
+	},
+	{
+		.name = "upboard-led",
+		.id = 2,
+	},
+	{
+		.name = "upboard-led",
+		.id = 3,
+	},
+	{
+		.name = "upboard-pinctrl"
+	},
+};
+
+static int upboard_init_gpio(struct device *dev)
+{
+	struct upboard_ddata *ddata = dev_get_drvdata(dev);
+	struct gpio_desc *enable_gpio;
+
+	ddata->clear_gpio = devm_gpiod_get(dev, "clear", GPIOD_OUT_LOW);
+	if (IS_ERR(ddata->clear_gpio))
+		return PTR_ERR(ddata->clear_gpio);
+
+	ddata->strobe_gpio = devm_gpiod_get(dev, "strobe", GPIOD_OUT_LOW);
+	if (IS_ERR(ddata->strobe_gpio))
+		return PTR_ERR(ddata->strobe_gpio);
+
+	ddata->datain_gpio = devm_gpiod_get(dev, "datain", GPIOD_OUT_LOW);
+	if (IS_ERR(ddata->datain_gpio))
+		return PTR_ERR(ddata->datain_gpio);
+
+	ddata->dataout_gpio = devm_gpiod_get(dev, "dataout", GPIOD_IN);
+	if (IS_ERR(ddata->dataout_gpio))
+		return PTR_ERR(ddata->dataout_gpio);
+
+	/* External I/O signals are gated by ENABLE - ensure this is high */
+	enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_HIGH);
+	if (IS_ERR(enable_gpio))
+		return PTR_ERR(enable_gpio);
+
+	return 0;
+}
+
+static int upboard_check_supported(struct device *dev, struct regmap *regmap)
+{
+	uint8_t manufacturer_id, build, major, minor, patch;
+	unsigned int platform_id, firmware_id;
+	int ret;
+
+	ret = regmap_read(regmap, UPBOARD_REG_PLATFORM_ID, &platform_id);
+	if (ret)
+		return ret;
+
+	manufacturer_id = platform_id;
+	if (manufacturer_id != AAEON_MANUFACTURER_ID) {
+		dev_err(dev,
+			"unsupported FPGA firmware from manufacturer 0x%02x",
+			manufacturer_id);
+		return -ENODEV;
+	}
+
+	ret = regmap_read(regmap, UPBOARD_REG_FIRMWARE_ID, &firmware_id);
+	if (ret)
+		return ret;
+
+	build = UPBOARD_FW_BUILD(firmware_id);
+	major = UPBOARD_FW_MAJOR(firmware_id);
+	minor = UPBOARD_FW_MINOR(firmware_id);
+	patch = UPBOARD_FW_PATCH(firmware_id);
+	if (major != SUPPORTED_FW_MAJOR) {
+		dev_err(dev, "unsupported FPGA firmware v%u.%u.%u.%u",
+			 major, minor, patch, build);
+		return -ENODEV;
+	}
+
+	dev_dbg(dev, "supported FPGA firmware v%u.%u.%u.%u",
+		major, minor, patch, build);
+	return 0;
+}
+
+static const struct acpi_device_id upboard_acpi_match[] = {
+	{ "AANT0F01", (kernel_ulong_t)UPBOARD_ID_UP2 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, upboard_acpi_match);
+
+static int upboard_match_device(struct device *dev)
+{
+	struct upboard_ddata *ddata = dev_get_drvdata(dev);
+	enum upboard_id id = (kernel_ulong_t)device_get_match_data(dev);
+
+	switch (id) {
+	case UPBOARD_ID_UP2:
+		ddata->regmapconf = &upboard_up2_regmap_config;
+		ddata->cells = upboard_up2_mfd_cells;
+		ddata->ncells = ARRAY_SIZE(upboard_up2_mfd_cells);
+		break;
+	default:
+		dev_err(dev, "unsupported ID %u\n", id);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int upboard_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct upboard_ddata *ddata;
+	struct regmap *regmap;
+	int ret;
+
+	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
+	if (!ddata)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, ddata);
+
+	ret = upboard_match_device(dev);
+	if (ret)
+		return ret;
+
+	regmap = devm_regmap_init(dev, NULL, ddata, ddata->regmapconf);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	ret = upboard_init_gpio(dev);
+	if (ret) {
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "failed to init GPIOs: %d", ret);
+		return ret;
+	}
+
+	ret = upboard_check_supported(dev, regmap);
+	if (ret)
+		return ret;
+
+	return devm_mfd_add_devices(dev, 0, ddata->cells, ddata->ncells,
+				    NULL, 0, NULL);
+}
+
+static struct platform_driver upboard_driver = {
+	.probe = upboard_probe,
+	.driver = {
+		.name = "upboard",
+		.acpi_match_table = upboard_acpi_match,
+	},
+};
+
+module_platform_driver(upboard_driver);
+
+MODULE_AUTHOR("Javier Arteaga <javier@emutex.com>");
+MODULE_DESCRIPTION("UP Board platform controller driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/upboard.h b/include/linux/mfd/upboard.h
new file mode 100644
index 0000000..63e677f
--- /dev/null
+++ b/include/linux/mfd/upboard.h
@@ -0,0 +1,44 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// UP Board MFD driver interface
+//
+// Copyright (c) 2018, Emutex Ltd.
+//
+// Author: Javier Arteaga <javier@emutex.com>
+//
+
+#ifndef __LINUX_MFD_UPBOARD_H
+#define __LINUX_MFD_UPBOARD_H
+
+#define UPBOARD_REGISTER_SIZE 16
+
+/**
+ * enum upboard_reg - addresses for 16-bit controller registers
+ *
+ * @UPBOARD_REG_PLATFORM_ID:    [RO] BOARD_ID | MANUFACTURER_ID
+ * @UPBOARD_REG_FIRMWARE_ID:    [RO] BUILD | MAJOR | MINOR | PATCH
+ * @UPBOARD_REG_FUNC_EN0:       [RW] Toggles for board functions (bank 0)
+ * @UPBOARD_REG_FUNC_EN1:       [RW] Toggles for board functions (bank 1)
+ * @UPBOARD_REG_GPIO_EN0:       [RW] Hi-Z (0) / enabled (1) GPIO (bank 0)
+ * @UPBOARD_REG_GPIO_EN1:       [RW] Hi-Z (0) / enabled (1) GPIO (bank 1)
+ * @UPBOARD_REG_GPIO_EN2:       [RW] Hi-Z (0) / enabled (1) GPIO (bank 2)
+ * @UPBOARD_REG_GPIO_DIR0:      [RW] SoC- (0) / FPGA- (1) driven GPIO (bank 0)
+ * @UPBOARD_REG_GPIO_DIR1:      [RW] SoC- (0) / FPGA- (1) driven GPIO (bank 1)
+ * @UPBOARD_REG_GPIO_DIR2:      [RW] SoC- (0) / FPGA- (1) driven GPIO (bank 2)
+ * @UPBOARD_REG_MAX: one past the last valid address
+ */
+enum upboard_reg {
+	UPBOARD_REG_PLATFORM_ID   = 0x10,
+	UPBOARD_REG_FIRMWARE_ID   = 0x11,
+	UPBOARD_REG_FUNC_EN0      = 0x20,
+	UPBOARD_REG_FUNC_EN1      = 0x21,
+	UPBOARD_REG_GPIO_EN0      = 0x30,
+	UPBOARD_REG_GPIO_EN1      = 0x31,
+	UPBOARD_REG_GPIO_EN2      = 0x32,
+	UPBOARD_REG_GPIO_DIR0     = 0x40,
+	UPBOARD_REG_GPIO_DIR1     = 0x41,
+	UPBOARD_REG_GPIO_DIR2     = 0x42,
+	UPBOARD_REG_MAX,
+};
+
+#endif /*  __LINUX_MFD_UPBOARD_H */
-- 
2.7.4


------
This email has been scanned for spam and malware by The Email Laundry.

[PATCH v3 2/3] leds: upboard: Add LED support

[Dan O'Donovan]

Allow userspace to use the on-board LEDs as "upboard:<color>:".

Acked-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Dan O'Donovan <dan@emutex.com>
---
 drivers/leds/Kconfig        |  10 +++++
 drivers/leds/Makefile       |   1 +
 drivers/leds/leds-upboard.c | 103 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 114 insertions(+)
 create mode 100644 drivers/leds/leds-upboard.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index a72f97f..4ae1ec3 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -766,6 +766,16 @@ config LEDS_NIC78BX
 	  To compile this driver as a module, choose M here: the module
 	  will be called leds-nic78bx.
 
+config LEDS_UPBOARD
+	tristate "LED support for the UP Squared"
+	depends on LEDS_CLASS
+	depends on MFD_UPBOARD
+	help
+	  This option enables support for the LEDs on the UP Squared board.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called leds-upboard.
+
 comment "LED Triggers"
 source "drivers/leds/trigger/Kconfig"
 
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 4c1b005..a35629d 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -79,6 +79,7 @@ obj-$(CONFIG_LEDS_MT6323)		+= leds-mt6323.o
 obj-$(CONFIG_LEDS_LM3692X)		+= leds-lm3692x.o
 obj-$(CONFIG_LEDS_SC27XX_BLTC)		+= leds-sc27xx-bltc.o
 obj-$(CONFIG_LEDS_LM3601X)		+= leds-lm3601x.o
+obj-$(CONFIG_LEDS_UPBOARD)		+= leds-upboard.o
 
 # LED SPI Drivers
 obj-$(CONFIG_LEDS_CR0014114)		+= leds-cr0014114.o
diff --git a/drivers/leds/leds-upboard.c b/drivers/leds/leds-upboard.c
new file mode 100644
index 0000000..90ebd9b
--- /dev/null
+++ b/drivers/leds/leds-upboard.c
@@ -0,0 +1,103 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// UP Board LED driver
+//
+// Copyright (c) 2018, Emutex Ltd.
+//
+// Author: Javier Arteaga <javier@emutex.com>
+//
+
+#include <linux/kernel.h>
+#include <linux/leds.h>
+#include <linux/mfd/upboard.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define to_upboard_led(cdev) container_of(cdev, struct upboard_led, cdev)
+
+static const char * const upboard_led_names[] = {
+	"upboard:blue:",
+	"upboard:yellow:",
+	"upboard:green:",
+	"upboard:red:",
+};
+
+struct upboard_led {
+	struct regmap_field *field;
+	struct led_classdev cdev;
+};
+
+static enum led_brightness upboard_led_brightness_get(struct led_classdev *cdev)
+{
+	struct upboard_led *led = to_upboard_led(cdev);
+	int ret, brightness = 0;
+
+	ret = regmap_field_read(led->field, &brightness);
+	if (ret < 0)
+		dev_err(cdev->dev, "Failed to get led brightness, %d", ret);
+
+	return brightness;
+}
+
+static void upboard_led_brightness_set(struct led_classdev *cdev,
+				       enum led_brightness brightness)
+{
+	struct upboard_led *led = to_upboard_led(cdev);
+	int ret;
+
+	ret = regmap_field_write(led->field, brightness);
+	if (ret < 0)
+		dev_err(cdev->dev, "Failed to set led brightness, %d", ret);
+}
+
+static int upboard_led_probe(struct platform_device *pdev)
+{
+	unsigned int led_index = pdev->id;
+	struct device *dev = &pdev->dev;
+	struct upboard_led *led;
+	struct regmap *regmap;
+	struct reg_field conf = {
+		.reg = UPBOARD_REG_FUNC_EN0,
+		.lsb = led_index,
+		.msb = led_index,
+	};
+
+	if (led_index >= ARRAY_SIZE(upboard_led_names))
+		return -EINVAL;
+
+	if (!dev->parent)
+		return -EINVAL;
+
+	regmap = dev_get_regmap(dev->parent, NULL);
+	if (!regmap)
+		return -EINVAL;
+
+	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
+	if (!led)
+		return -ENOMEM;
+
+	led->field = devm_regmap_field_alloc(dev, regmap, conf);
+	if (IS_ERR(led->field))
+		return PTR_ERR(led->field);
+
+	led->cdev.max_brightness = 1;
+	led->cdev.brightness_get = upboard_led_brightness_get;
+	led->cdev.brightness_set = upboard_led_brightness_set;
+	led->cdev.name = upboard_led_names[led_index];
+
+	return devm_led_classdev_register(dev, &led->cdev);
+}
+
+static struct platform_driver upboard_led_driver = {
+	.driver = {
+		.name = "upboard-led",
+	},
+};
+
+module_platform_driver_probe(upboard_led_driver, upboard_led_probe);
+
+MODULE_ALIAS("platform:upboard-led");
+MODULE_AUTHOR("Javier Arteaga <javier@emutex.com>");
+MODULE_DESCRIPTION("UP Board LED driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4


------
This email has been scanned for spam and malware by The Email Laundry.

[PATCH v3 3/3] pinctrl: upboard: Add UP2 pinctrl and gpio driver

[Dan O'Donovan]

The UP2 board features a Raspberry Pi compatible pin header (HAT) and a
board-specific expansion connector (EXHAT). Both expose assorted
functions from either the SoC (such as GPIO, I2C, SPI, UART...) or other
on-board devices (ADC, FPGA IP blocks...).

These lines are routed through an on-board FPGA. The platform controller
in its stock firmware provides register fields to change:

- Line enable (FPGA pins enabled / high impedance)
- Line direction (SoC driven / FPGA driven)

To enable using SoC GPIOs on the pin header, this arrangement requires
both configuring the platform controller, and updating the SoC pad
registers in sync.

Add a frontend pinctrl/GPIO driver that registers a new set of GPIO
lines for the header pins. When these are requested, the driver
propagates this request to the backend SoC pinctrl/GPIO driver by
grabbing a GPIO descriptor for the matching SoC GPIO line. The needed
mapping for this is retrieved via ACPI properties.

Signed-off-by: Dan O'Donovan <dan@emutex.com>
---
 drivers/pinctrl/Kconfig           |  13 +
 drivers/pinctrl/Makefile          |   1 +
 drivers/pinctrl/pinctrl-upboard.c | 496 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 510 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-upboard.c

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 4d8c00e..56c1345 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -350,6 +350,19 @@ config PINCTRL_OCELOT
 	select GENERIC_PINMUX_FUNCTIONS
 	select REGMAP_MMIO
 
+config PINCTRL_UPBOARD
+	tristate "UP Squared pinctrl and GPIO driver"
+	depends on ACPI
+	depends on MFD_UPBOARD
+	select PINMUX
+	help
+	  Pinctrl driver for the pin headers on the UP Squared board. It
+	  handles pin control for lines routed through the on-board FPGA and
+	  propagates changes to the SoC pinctrl to keep them in sync.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called pinctrl-upboard.
+
 source "drivers/pinctrl/actions/Kconfig"
 source "drivers/pinctrl/aspeed/Kconfig"
 source "drivers/pinctrl/bcm/Kconfig"
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 18a13c1..23cd292 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -43,6 +43,7 @@ obj-$(CONFIG_PINCTRL_ZYNQ)	+= pinctrl-zynq.o
 obj-$(CONFIG_PINCTRL_INGENIC)	+= pinctrl-ingenic.o
 obj-$(CONFIG_PINCTRL_RK805)	+= pinctrl-rk805.o
 obj-$(CONFIG_PINCTRL_OCELOT)	+= pinctrl-ocelot.o
+obj-$(CONFIG_PINCTRL_UPBOARD)	+= pinctrl-upboard.o
 
 obj-y				+= actions/
 obj-$(CONFIG_ARCH_ASPEED)	+= aspeed/
diff --git a/drivers/pinctrl/pinctrl-upboard.c b/drivers/pinctrl/pinctrl-upboard.c
new file mode 100644
index 0000000..2695863
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-upboard.c
@@ -0,0 +1,496 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// UP Board pin controller driver
+//
+// Copyright (c) 2018, Emutex Ltd.
+//
+// Authors: Javier Arteaga <javier@emutex.com>
+//          Dan O'Donovan <dan@emutex.com>
+//
+
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
+#include <linux/kernel.h>
+#include <linux/mfd/upboard.h>
+#include <linux/module.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/string.h>
+
+#include "core.h"
+
+#define to_upboard_pinctrl(gc) container_of(gc, struct upboard_pinctrl, chip)
+
+struct upboard_pin {
+	struct regmap_field *func_en;
+	struct regmap_field *gpio_en;
+	struct regmap_field *gpio_dir;
+};
+
+struct upboard_pinctrl {
+	struct pinctrl_dev *pctldev;
+	struct gpio_chip chip;
+	unsigned int nsoc_gpios;
+	struct gpio_desc **soc_gpios;
+};
+
+enum upboard_func0_enables {
+	UPBOARD_I2C0_EN = 8,
+	UPBOARD_I2C1_EN = 9,
+};
+
+static const struct reg_field upboard_i2c0_reg =
+	REG_FIELD(UPBOARD_REG_FUNC_EN0, UPBOARD_I2C0_EN, UPBOARD_I2C0_EN);
+
+static const struct reg_field upboard_i2c1_reg =
+	REG_FIELD(UPBOARD_REG_FUNC_EN0, UPBOARD_I2C1_EN, UPBOARD_I2C1_EN);
+
+#define UPBOARD_BIT_TO_PIN(r, bit) ((r) * UPBOARD_REGISTER_SIZE + (bit))
+
+/*
+ * UP Squared data
+ */
+
+#define UPBOARD_UP2_BIT_TO_PIN(r, id) (UPBOARD_BIT_TO_PIN(r, UPBOARD_UP2_##id))
+
+#define UPBOARD_UP2_PIN_ANON(r, bit)					\
+	{								\
+		.number = UPBOARD_BIT_TO_PIN(r, bit),			\
+	}
+
+#define UPBOARD_UP2_PIN_NAME(r, id)					\
+	{								\
+		.number = UPBOARD_UP2_BIT_TO_PIN(r, id),		\
+		.name = #id,						\
+	}
+
+#define UPBOARD_UP2_PIN_FUNC(r, id, data)				\
+	{								\
+		.number = UPBOARD_UP2_BIT_TO_PIN(r, id),		\
+		.name = #id,						\
+		.drv_data = (void *)(data),				\
+	}
+
+enum upboard_up2_reg0_bit {
+	UPBOARD_UP2_UART1_TXD,
+	UPBOARD_UP2_UART1_RXD,
+	UPBOARD_UP2_UART1_RTS,
+	UPBOARD_UP2_UART1_CTS,
+	UPBOARD_UP2_GPIO3,
+	UPBOARD_UP2_GPIO5,
+	UPBOARD_UP2_GPIO6,
+	UPBOARD_UP2_GPIO11,
+	UPBOARD_UP2_EXHAT_LVDS1n,
+	UPBOARD_UP2_EXHAT_LVDS1p,
+	UPBOARD_UP2_SPI2_TXD,
+	UPBOARD_UP2_SPI2_RXD,
+	UPBOARD_UP2_SPI2_FS1,
+	UPBOARD_UP2_SPI2_FS0,
+	UPBOARD_UP2_SPI2_CLK,
+	UPBOARD_UP2_SPI1_TXD,
+};
+
+enum upboard_up2_reg1_bit {
+	UPBOARD_UP2_SPI1_RXD,
+	UPBOARD_UP2_SPI1_FS1,
+	UPBOARD_UP2_SPI1_FS0,
+	UPBOARD_UP2_SPI1_CLK,
+	UPBOARD_UP2_BIT20,
+	UPBOARD_UP2_BIT21,
+	UPBOARD_UP2_BIT22,
+	UPBOARD_UP2_BIT23,
+	UPBOARD_UP2_PWM1,
+	UPBOARD_UP2_PWM0,
+	UPBOARD_UP2_EXHAT_LVDS0n,
+	UPBOARD_UP2_EXHAT_LVDS0p,
+	UPBOARD_UP2_I2C0_SCL,
+	UPBOARD_UP2_I2C0_SDA,
+	UPBOARD_UP2_I2C1_SCL,
+	UPBOARD_UP2_I2C1_SDA,
+};
+
+enum upboard_up2_reg2_bit {
+	UPBOARD_UP2_EXHAT_LVDS3n,
+	UPBOARD_UP2_EXHAT_LVDS3p,
+	UPBOARD_UP2_EXHAT_LVDS4n,
+	UPBOARD_UP2_EXHAT_LVDS4p,
+	UPBOARD_UP2_EXHAT_LVDS5n,
+	UPBOARD_UP2_EXHAT_LVDS5p,
+	UPBOARD_UP2_I2S_SDO,
+	UPBOARD_UP2_I2S_SDI,
+	UPBOARD_UP2_I2S_WS_SYNC,
+	UPBOARD_UP2_I2S_BCLK,
+	UPBOARD_UP2_EXHAT_LVDS6n,
+	UPBOARD_UP2_EXHAT_LVDS6p,
+	UPBOARD_UP2_EXHAT_LVDS7n,
+	UPBOARD_UP2_EXHAT_LVDS7p,
+	UPBOARD_UP2_EXHAT_LVDS2n,
+	UPBOARD_UP2_EXHAT_LVDS2p,
+};
+
+static struct pinctrl_pin_desc upboard_up2_pins[] = {
+	UPBOARD_UP2_PIN_NAME(0, UART1_TXD),
+	UPBOARD_UP2_PIN_NAME(0, UART1_RXD),
+	UPBOARD_UP2_PIN_NAME(0, UART1_RTS),
+	UPBOARD_UP2_PIN_NAME(0, UART1_CTS),
+	UPBOARD_UP2_PIN_NAME(0, GPIO3),
+	UPBOARD_UP2_PIN_NAME(0, GPIO5),
+	UPBOARD_UP2_PIN_NAME(0, GPIO6),
+	UPBOARD_UP2_PIN_NAME(0, GPIO11),
+	UPBOARD_UP2_PIN_NAME(0, EXHAT_LVDS1n),
+	UPBOARD_UP2_PIN_NAME(0, EXHAT_LVDS1p),
+	UPBOARD_UP2_PIN_NAME(0, SPI2_TXD),
+	UPBOARD_UP2_PIN_NAME(0, SPI2_RXD),
+	UPBOARD_UP2_PIN_NAME(0, SPI2_FS1),
+	UPBOARD_UP2_PIN_NAME(0, SPI2_FS0),
+	UPBOARD_UP2_PIN_NAME(0, SPI2_CLK),
+	UPBOARD_UP2_PIN_NAME(0, SPI1_TXD),
+	UPBOARD_UP2_PIN_NAME(1, SPI1_RXD),
+	UPBOARD_UP2_PIN_NAME(1, SPI1_FS1),
+	UPBOARD_UP2_PIN_NAME(1, SPI1_FS0),
+	UPBOARD_UP2_PIN_NAME(1, SPI1_CLK),
+	UPBOARD_UP2_PIN_ANON(1, 4),
+	UPBOARD_UP2_PIN_ANON(1, 5),
+	UPBOARD_UP2_PIN_ANON(1, 6),
+	UPBOARD_UP2_PIN_ANON(1, 7),
+	UPBOARD_UP2_PIN_NAME(1, PWM1),
+	UPBOARD_UP2_PIN_NAME(1, PWM0),
+	UPBOARD_UP2_PIN_NAME(1, EXHAT_LVDS0n),
+	UPBOARD_UP2_PIN_NAME(1, EXHAT_LVDS0p),
+	UPBOARD_UP2_PIN_FUNC(1, I2C0_SCL, &upboard_i2c0_reg),
+	UPBOARD_UP2_PIN_FUNC(1, I2C0_SDA, &upboard_i2c0_reg),
+	UPBOARD_UP2_PIN_FUNC(1, I2C1_SCL, &upboard_i2c1_reg),
+	UPBOARD_UP2_PIN_FUNC(1, I2C1_SDA, &upboard_i2c1_reg),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS3n),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS3p),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS4n),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS4p),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS5n),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS5p),
+	UPBOARD_UP2_PIN_NAME(2, I2S_SDO),
+	UPBOARD_UP2_PIN_NAME(2, I2S_SDI),
+	UPBOARD_UP2_PIN_NAME(2, I2S_WS_SYNC),
+	UPBOARD_UP2_PIN_NAME(2, I2S_BCLK),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS6n),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS6p),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS7n),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS7p),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS2n),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS2p),
+};
+
+static int upboard_get_functions_count(struct pinctrl_dev *pctldev)
+{
+	return 0;
+}
+
+static int upboard_get_function_groups(struct pinctrl_dev *pctldev,
+				       unsigned int selector,
+				       const char * const **groups,
+				       unsigned int *num_groups)
+{
+	*groups = NULL;
+	*num_groups = 0;
+	return 0;
+}
+
+static const char *upboard_get_function_name(struct pinctrl_dev *pctldev,
+					     unsigned int selector)
+{
+	return NULL;
+}
+
+static int upboard_set_mux(struct pinctrl_dev *pctldev, unsigned int function,
+			   unsigned int group)
+{
+	return 0;
+}
+
+static int upboard_gpio_request_enable(struct pinctrl_dev *pctldev,
+				       struct pinctrl_gpio_range *range,
+				       unsigned int pin)
+{
+	const struct pin_desc * const pd = pin_desc_get(pctldev, pin);
+	const struct upboard_pin *p = pd->drv_data;
+	int ret;
+
+	/* if this pin has an associated function bit, disable it first */
+	if (p->func_en) {
+		ret = regmap_field_write(p->func_en, 0);
+		if (ret)
+			return ret;
+	}
+
+	if (p->gpio_en) {
+		ret = regmap_field_write(p->gpio_en, 1);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int upboard_gpio_set_direction(struct pinctrl_dev *pctldev,
+				      struct pinctrl_gpio_range *range,
+				      unsigned int pin, bool input)
+{
+	const struct pin_desc * const pd = pin_desc_get(pctldev, pin);
+	const struct upboard_pin *p = pd->drv_data;
+
+	return regmap_field_write(p->gpio_dir, input);
+}
+
+static const struct pinmux_ops upboard_pinmux_ops = {
+	.get_functions_count = upboard_get_functions_count,
+	.get_function_groups = upboard_get_function_groups,
+	.get_function_name = upboard_get_function_name,
+	.set_mux = upboard_set_mux,
+	.gpio_request_enable = upboard_gpio_request_enable,
+	.gpio_set_direction = upboard_gpio_set_direction,
+};
+
+static int upboard_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	return 0;
+}
+
+static const char *upboard_get_group_name(struct pinctrl_dev *pctldev,
+					  unsigned int selector)
+{
+	return NULL;
+}
+
+static const struct pinctrl_ops upboard_pinctrl_ops = {
+	.get_groups_count = upboard_get_groups_count,
+	.get_group_name = upboard_get_group_name,
+};
+
+static struct pinctrl_desc upboard_up2_pinctrl_desc = {
+	.pins = upboard_up2_pins,
+	.npins = ARRAY_SIZE(upboard_up2_pins),
+	.pctlops = &upboard_pinctrl_ops,
+	.pmxops = &upboard_pinmux_ops,
+	.owner = THIS_MODULE,
+};
+
+static int upboard_gpio_request(struct gpio_chip *gc, unsigned int offset)
+{
+	struct upboard_pinctrl *pctrl = to_upboard_pinctrl(gc);
+	struct gpio_desc *desc;
+	int ret;
+
+	ret = pinctrl_gpio_request(gc->base + offset);
+	if (ret)
+		return ret;
+
+	desc = devm_gpiod_get_index(gc->parent, "external", offset, GPIOD_ASIS);
+	if (IS_ERR(desc))
+		return PTR_ERR(desc);
+
+	pctrl->soc_gpios[offset] = desc;
+	return 0;
+}
+
+static void upboard_gpio_free(struct gpio_chip *gc, unsigned int offset)
+{
+	struct upboard_pinctrl *pctrl = to_upboard_pinctrl(gc);
+
+	if (!pctrl->soc_gpios[offset])
+		return;
+
+	devm_gpiod_put(gc->parent, pctrl->soc_gpios[offset]);
+	pctrl->soc_gpios[offset] = NULL;
+
+	pinctrl_gpio_free(gc->base + offset);
+}
+
+static int upboard_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+	struct upboard_pinctrl *pctrl = to_upboard_pinctrl(gc);
+	struct gpio_desc *desc = pctrl->soc_gpios[offset];
+
+	if (!desc)
+		return -ENODEV;
+
+	return gpiod_get_direction(desc);
+}
+
+static int upboard_gpio_direction_input(struct gpio_chip *gc,
+					unsigned int offset)
+{
+	struct upboard_pinctrl *pctrl = to_upboard_pinctrl(gc);
+	struct gpio_desc *desc = pctrl->soc_gpios[offset];
+	int ret;
+
+	if (!desc)
+		return -ENODEV;
+
+	ret = gpiod_direction_input(desc);
+	if (ret)
+		return ret;
+
+	return pinctrl_gpio_direction_input(gc->base + offset);
+}
+
+static int upboard_gpio_direction_output(struct gpio_chip *gc,
+					 unsigned int offset, int value)
+{
+	struct upboard_pinctrl *pctrl = to_upboard_pinctrl(gc);
+	struct gpio_desc *desc = pctrl->soc_gpios[offset];
+	int ret;
+
+	if (!desc)
+		return -ENODEV;
+
+	ret = pinctrl_gpio_direction_output(gc->base + offset);
+	if (ret)
+		return ret;
+
+	return gpiod_direction_output(desc, value);
+}
+
+static int upboard_gpio_get_value(struct gpio_chip *gc, unsigned int offset)
+{
+	struct upboard_pinctrl *pctrl = to_upboard_pinctrl(gc);
+	struct gpio_desc *desc = pctrl->soc_gpios[offset];
+
+	if (!desc)
+		return -ENODEV;
+
+	return gpiod_get_value(desc);
+}
+
+static void upboard_gpio_set_value(struct gpio_chip *gc, unsigned int offset,
+				   int value)
+{
+	struct upboard_pinctrl *pctrl = to_upboard_pinctrl(gc);
+	struct gpio_desc *desc = pctrl->soc_gpios[offset];
+
+	if (!desc)
+		return;
+
+	gpiod_set_value(desc, value);
+}
+
+static struct gpio_chip upboard_gpio_chip = {
+	.label = "UP pin controller",
+	.owner = THIS_MODULE,
+	.request = upboard_gpio_request,
+	.free = upboard_gpio_free,
+	.get_direction = upboard_gpio_get_direction,
+	.direction_input = upboard_gpio_direction_input,
+	.direction_output = upboard_gpio_direction_output,
+	.get = upboard_gpio_get_value,
+	.set = upboard_gpio_set_value,
+	.base = -1,
+};
+
+static struct regmap_field *upboard_field_alloc(struct device *dev,
+						struct regmap *regmap,
+						unsigned int base,
+						unsigned int number)
+{
+	const unsigned int reg = number / UPBOARD_REGISTER_SIZE;
+	const unsigned int bit = number % UPBOARD_REGISTER_SIZE;
+	const struct reg_field field = {
+		.reg = base + reg,
+		.msb = bit,
+		.lsb = bit,
+	};
+
+	return devm_regmap_field_alloc(dev, regmap, field);
+}
+
+static int upboard_pinctrl_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct pinctrl_desc *pctldesc;
+	struct upboard_pinctrl *pctrl;
+	struct upboard_pin *pins;
+	struct regmap *regmap;
+	unsigned int i;
+	int ret;
+
+	if (!dev->parent)
+		return -EINVAL;
+
+	regmap = dev_get_regmap(dev->parent, NULL);
+	if (!regmap)
+		return -EINVAL;
+
+	pctldesc = &upboard_up2_pinctrl_desc;
+	pctldesc->name = dev_name(dev);
+
+	pins = devm_kcalloc(dev, pctldesc->npins, sizeof(*pins), GFP_KERNEL);
+	if (!pins)
+		return -ENOMEM;
+
+	for (i = 0; i < pctldesc->npins; i++) {
+		struct upboard_pin *pin = &pins[i];
+		struct pinctrl_pin_desc *pd;
+
+		pd = (struct pinctrl_pin_desc *)&pctldesc->pins[i];
+		if (pd->drv_data) {
+			struct reg_field *field = pd->drv_data;
+
+			pin->func_en = devm_regmap_field_alloc(dev, regmap,
+							       *field);
+			if (IS_ERR(pin->func_en))
+				return PTR_ERR(pin->func_en);
+		}
+
+		pin->gpio_en = upboard_field_alloc(dev, regmap,
+						   UPBOARD_REG_GPIO_EN0, i);
+		if (IS_ERR(pin->gpio_en))
+			return PTR_ERR(pin->gpio_en);
+
+		pin->gpio_dir = upboard_field_alloc(dev, regmap,
+						    UPBOARD_REG_GPIO_DIR0, i);
+		if (IS_ERR(pin->gpio_dir))
+			return PTR_ERR(pin->gpio_dir);
+
+		pd->drv_data = pin;
+	}
+
+	pctrl = devm_kzalloc(dev, sizeof(*pctrl), GFP_KERNEL);
+	if (!pctrl)
+		return -ENOMEM;
+
+	pctrl->chip = upboard_gpio_chip;
+	pctrl->chip.parent = dev;
+	pctrl->chip.ngpio = pctldesc->npins;
+
+	pctrl->nsoc_gpios = gpiod_count(dev, "external");
+	pctrl->soc_gpios = devm_kcalloc(dev, pctrl->nsoc_gpios,
+					sizeof(*pctrl->soc_gpios), GFP_KERNEL);
+	if (!pctrl->soc_gpios)
+		return -ENOMEM;
+
+	pctrl->pctldev = devm_pinctrl_register(dev, pctldesc, pctrl);
+	if (IS_ERR(pctrl->pctldev))
+		return PTR_ERR(pctrl->pctldev);
+
+	ret = devm_gpiochip_add_data(dev, &pctrl->chip, &pctrl->chip);
+	if (ret)
+		return ret;
+
+	return gpiochip_add_pin_range(&pctrl->chip, pctldesc->name, 0, 0,
+				      pctldesc->npins);
+}
+
+static struct platform_driver upboard_pinctrl_driver = {
+	.driver = {
+		.name = "upboard-pinctrl",
+	},
+};
+
+module_platform_driver_probe(upboard_pinctrl_driver, upboard_pinctrl_probe);
+
+MODULE_ALIAS("platform:upboard-pinctrl");
+MODULE_AUTHOR("Javier Arteaga <javier@emutex.com>");
+MODULE_AUTHOR("Dan O'Donovan <dan@emutex.com>");
+MODULE_DESCRIPTION("UP Board pin control and GPIO driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4


------
This email has been scanned for spam and malware by The Email Laundry.

Re: [PATCH v3 3/3] pinctrl: upboard: Add UP2 pinctrl and gpio driver

[Linus Walleij]

Hi Dan,

On Wed, Oct 31, 2018 at 9:45 PM Dan O'Donovan <dan@emutex.com> wrote:

> The UP2 board features a Raspberry Pi compatible pin header (HAT) and a
> board-specific expansion connector (EXHAT). Both expose assorted
> functions from either the SoC (such as GPIO, I2C, SPI, UART...) or other
> on-board devices (ADC, FPGA IP blocks...).
>
> These lines are routed through an on-board FPGA. The platform controller
> in its stock firmware provides register fields to change:
>
> - Line enable (FPGA pins enabled / high impedance)
> - Line direction (SoC driven / FPGA driven)
>
> To enable using SoC GPIOs on the pin header, this arrangement requires
> both configuring the platform controller, and updating the SoC pad
> registers in sync.
>
> Add a frontend pinctrl/GPIO driver that registers a new set of GPIO
> lines for the header pins. When these are requested, the driver
> propagates this request to the backend SoC pinctrl/GPIO driver by
> grabbing a GPIO descriptor for the matching SoC GPIO line. The needed
> mapping for this is retrieved via ACPI properties.
>
> Signed-off-by: Dan O'Donovan <dan@emutex.com>

It appears you missed my review comments so please read them
and reply or respin the patch accordingly.

Yours,
Linus Walleij

Re: [PATCH v3 3/3] pinctrl: upboard: Add UP2 pinctrl and gpio driver

[Dan O'Donovan]

On 10/31/2018 09:30 PM, Linus Walleij wrote:
> Hi Dan,
>
> On Wed, Oct 31, 2018 at 9:45 PM Dan O'Donovan <dan@emutex.com> wrote:
>
>> The UP2 board features a Raspberry Pi compatible pin header (HAT) and a
>> board-specific expansion connector (EXHAT). Both expose assorted
>> functions from either the SoC (such as GPIO, I2C, SPI, UART...) or other
>> on-board devices (ADC, FPGA IP blocks...).
>>
>> These lines are routed through an on-board FPGA. The platform controller
>> in its stock firmware provides register fields to change:
>>
>> - Line enable (FPGA pins enabled / high impedance)
>> - Line direction (SoC driven / FPGA driven)
>>
>> To enable using SoC GPIOs on the pin header, this arrangement requires
>> both configuring the platform controller, and updating the SoC pad
>> registers in sync.
>>
>> Add a frontend pinctrl/GPIO driver that registers a new set of GPIO
>> lines for the header pins. When these are requested, the driver
>> propagates this request to the backend SoC pinctrl/GPIO driver by
>> grabbing a GPIO descriptor for the matching SoC GPIO line. The needed
>> mapping for this is retrieved via ACPI properties.
>>
>> Signed-off-by: Dan O'Donovan <dan@emutex.com>
> It appears you missed my review comments so please read them
> and reply or respin the patch accordingly.
Hi Linus.  Unfortunately I did miss your comments completely.  Somehow, they weren't in my Inbox, but I found them in the online archives now.  Thanks for letting me know.
I'll reply to those soon.
>
> Yours,
> Linus Walleij

Re: [PATCH v3 1/3] mfd: upboard: Add UP2 platform controller driver

[Lee Jones]

On Wed, 31 Oct 2018, Dan O'Donovan wrote:

> UP Squared (UP2) is a x86 SBC from AAEON based on Intel Apollo Lake. It
> features a MAX 10 FPGA that routes lines from both SoC and on-board
> devices to two I/O headers:
> 
>                                 +------------------------+
>                                 | 40-pin RPi-like header |
>                          +------|         (HAT)          |
>                          |      +------------------------+
>     +-------+    +--------+
>     |       |    |        |     +------------------------+
>     |  SoC  |----|  FPGA  |-----|  Custom UP2 pin header |
>     |       |    |        |     |        (EXHAT)         |
>     +-------+    +--------+     +------------------------+
>                          |
>                          +------* On-board devices: LED, VLS...
> 
> This is intended to enable vendor-specific applications to customize I/O
> header pinout, as well as include low-latency functionality. It also
> performs voltage level translation between the SoC (1.8V) and HAT header
> (3.3V).
> 
> Out of the box, this block implements a platform controller with a
> GPIO-bitbanged control interface. It's enumerated by ACPI and provides
> registers to control:
> 
> - Configuration of all FPGA-routed header lines. These can be driven
>   SoC-to-header, header-to-SoC or set in high impedance.
> 
> - On-board LEDs and enable lines for other platform devices.
> 
> Add core support for this platform controller as a MFD device, exposing
> these registers as a regmap.
> 
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Dan O'Donovan <dan@emutex.com>
> ---
>  drivers/mfd/Kconfig         |  17 +++
>  drivers/mfd/Makefile        |   1 +
>  drivers/mfd/upboard.c       | 336 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/upboard.h |  44 ++++++
>  4 files changed, 398 insertions(+)
>  create mode 100644 drivers/mfd/upboard.c
>  create mode 100644 include/linux/mfd/upboard.h

Looks like you've ignored the review comments here too.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 1/3] mfd: upboard: Add UP2 platform controller driver

[Dan O'Donovan]

On 11/01/2018 08:07 AM, Lee Jones wrote:
> On Wed, 31 Oct 2018, Dan O'Donovan wrote:
>
>> UP Squared (UP2) is a x86 SBC from AAEON based on Intel Apollo Lake. It
>> features a MAX 10 FPGA that routes lines from both SoC and on-board
>> devices to two I/O headers:
>>
>>                                 +------------------------+
>>                                 | 40-pin RPi-like header |
>>                          +------|         (HAT)          |
>>                          |      +------------------------+
>>     +-------+    +--------+
>>     |       |    |        |     +------------------------+
>>     |  SoC  |----|  FPGA  |-----|  Custom UP2 pin header |
>>     |       |    |        |     |        (EXHAT)         |
>>     +-------+    +--------+     +------------------------+
>>                          |
>>                          +------* On-board devices: LED, VLS...
>>
>> This is intended to enable vendor-specific applications to customize I/O
>> header pinout, as well as include low-latency functionality. It also
>> performs voltage level translation between the SoC (1.8V) and HAT header
>> (3.3V).
>>
>> Out of the box, this block implements a platform controller with a
>> GPIO-bitbanged control interface. It's enumerated by ACPI and provides
>> registers to control:
>>
>> - Configuration of all FPGA-routed header lines. These can be driven
>>   SoC-to-header, header-to-SoC or set in high impedance.
>>
>> - On-board LEDs and enable lines for other platform devices.
>>
>> Add core support for this platform controller as a MFD device, exposing
>> these registers as a regmap.
>>
>> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>> Signed-off-by: Dan O'Donovan <dan@emutex.com>
>> ---
>>  drivers/mfd/Kconfig         |  17 +++
>>  drivers/mfd/Makefile        |   1 +
>>  drivers/mfd/upboard.c       | 336 ++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/mfd/upboard.h |  44 ++++++
>>  4 files changed, 398 insertions(+)
>>  create mode 100644 drivers/mfd/upboard.c
>>  create mode 100644 include/linux/mfd/upboard.h
> Looks like you've ignored the review comments here too.
Hi Lee.  Looks like I didn't receive your original email, most likely because of a crappy spam filtering service we were using here up until recently.  I've found your comments in online archives now and will address them in the next revision of the patches.  Sorry for the inconvenience. :(

Re: [PATCH v3 1/3] mfd: upboard: Add UP2 platform controller driver

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 586 bytes --]

Hi!

> +static void __reg_io_start(const struct upboard_ddata * const ddata)
> +{
> +	/*
> +	 * CLEAR signal must be pulsed low before any register access.
> +	 * This resets internal counters in the controller and marks
> +	 * the start of a new register access.
> +	 */
> +	set_clear(ddata, 0);
> +	set_clear(ddata, 1);
> +}

We normally reserve __ prefix for different stuff. This is not using
it appropriately.
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 1/3] mfd: upboard: Add UP2 platform controller driver

[Linus Walleij]

On Sun, Nov 11, 2018 at 12:29 PM Pavel Machek <pavel@ucw.cz> wrote:
>
> > +static void __reg_io_start(const struct upboard_ddata * const ddata)
> > +{
> > +     /*
> > +      * CLEAR signal must be pulsed low before any register access.
> > +      * This resets internal counters in the controller and marks
> > +      * the start of a new register access.
> > +      */
> > +     set_clear(ddata, 0);
> > +     set_clear(ddata, 1);
> > +}
>
> We normally reserve __ prefix for different stuff. This is not using
> it appropriately.

+1

I actively dislike __underscore_notation as it seems to be
completely arbitrary and ambiguous. I think even
do_the_stuff_reg_io_start() is better.

But please come up with something to the point and
descriptive! :)

Yours,
Linus Walleij