RE: [v3,1/1] adv_mix955x is a scheme that multiplexes PCA9554/PCA9555 into LED and GPIO

RE: [v3,1/1] adv_mix955x is a scheme that multiplexes PCA9554/PCA9555 into LED and GPIO

[yuechao.zhao(赵越超)]

From: Yuechao Zhao <yuechao.zhao@advantech.com.cn>

With this driver, we can multiplex PCA9554/PCA9555 into LED and GPIO
based on the ACPI data of BIOS.

Signed-off-by: Yuechao Zhao <yuechao.zhao@advantech.com.cn>
Signed-off-by: Sui Jia <jia.sui@advantech.com.cn>

v3:
        - Cc LED drivers to the LED maintainers
        - delete useless comments
v2:
        - alphabetical order title ("ADVANTECH MIX995X DRIVER")
---
 MAINTAINERS                        |   6 +
 drivers/platform/x86/Kconfig       |  14 +
 drivers/platform/x86/Makefile      |   3 +
 drivers/platform/x86/adv_mix955x.c | 699 +++++++++++++++++++++++++++++
 4 files changed, 722 insertions(+)
 create mode 100644 drivers/platform/x86/adv_mix955x.c

diff --git a/MAINTAINERS b/MAINTAINERS
index d92f85ca831d..90033699334c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -573,6 +573,12 @@ S:	Maintained
 F:	Documentation/scsi/advansys.rst
 F:	drivers/scsi/advansys.c
 
+ADVANTECH MIX995X DRIVER
+M:	Yuechao Zhao <yuechao.zhao@advantech.com.cn>
+M:	Sui Jia <jia.sui@advantech.com.cn>
+S:	Maintained
+F:	drivers/platform/x86/adv_mix955x.c
+
 ADXL34X THREE-AXIS DIGITAL ACCELEROMETER DRIVER (ADXL345/ADXL346)
 M:	Michael Hennerich <michael.hennerich@analog.com>
 S:	Supported
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index ad4e630e73e2..ccead75c764b 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1284,6 +1284,20 @@ config INTEL_TELEMETRY
 	  directly via debugfs files. Various tools may use
 	  this interface for SoC state monitoring.
 
+config ADV_MIX955X
+    tristate "Advantech LEDs and GPIOs Driver"
+    depends on X86_64
+    depends on ACPI && GPIOLIB
+    select LEDS_CLASS
+    select LEDS_TRIGGERS
+    help
+      This driver is based on the kernel driver leds-pca955x
+      and gpio-pca953x but rewrite for PCA9554/PCA9555.
+      This driver requires LED data which is defined inside
+      ACPI DSDT or devicetree.
+      In addition, the pin that is not defined as LED will
+      be declared as GPIO.
+
 endif # X86_PLATFORM_DEVICES
 
 config PMC_ATOM
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 60d554073749..5ecad0ae925e 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -138,3 +138,6 @@ obj-$(CONFIG_INTEL_TELEMETRY)		+= intel_telemetry_core.o \
 					   intel_telemetry_pltdrv.o \
 					   intel_telemetry_debugfs.o
 obj-$(CONFIG_PMC_ATOM)			+= pmc_atom.o
+
+# Advantech
+obj-$(CONFIG_ADV_MIX955X)		+= adv_mix955x.o
diff --git a/drivers/platform/x86/adv_mix955x.c b/drivers/platform/x86/adv_mix955x.c
new file mode 100644
index 000000000000..7f8c7b41dee7
--- /dev/null
+++ b/drivers/platform/x86/adv_mix955x.c
@@ -0,0 +1,699 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright 2019 Advantech Corp.
+ *
+ * Author:
+ *	Jia Sui <jia.sui@advantech.com.cn>
+ *	Yuechao Zhao <yuechao.zhao@advantech.com.cn>
+ *
+ * A driver for AMIX955<X>(Advantech mixed device based on PCA955X).
+ * This driver provides LED and GPIO functions.
+ *
+ * Supported devices:
+ *
+ *	Device		ACPI_DEVICE_ID	Description	7-bit slave address
+ *	--------	--------------	---------------	-------------------
+ *	AMIX9554	AHC0320		Base on PCA9554	0x20 .. 0x27
+ *	AMIX9555	AHC0321		Base on PCA9555	0x20 .. 0x27
+ */
+
+#include <linux/i2c.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/version.h>
+#include <linux/fwnode.h>
+#include <linux/acpi.h>
+#include <linux/gpio/driver.h>
+#include <linux/gpio/consumer.h>
+#include <asm/unaligned.h>
+
+#define VERSION "0.05"
+
+enum AMIX955X_LED {
+	AMIX955X_LED_ON = 0,
+	AMIX955X_LED_OFF = 1,
+};
+
+#define BANK_SIZE 8
+/*
+ * Only support pca9554 and pca9555,
+ * The max pin numbers is 16
+ * Defind MAX_BANK = 2
+ */
+#define MAX_BANK 2
+
+/* PCA9554 GPIO register */
+#define AMIX955X_INPUT 0x00
+#define AMIX955X_OUTPUT 0x01
+#define AMIX955X_INVERT 0x02
+#define AMIX955X_DIRECTION 0x03
+
+/* Default Value of GPIO register */
+#define AMIX955X_OUTPUT_DEFAULT 0xFF
+#define AMIX955X_DIRECTION_DEFAULT 0xFFFF
+
+enum chip_id {
+	amix9554,
+	amix9555,
+};
+
+struct chip_info_t {
+	u8 bits;
+	u8 slv_addr; /* 7-bit slave address mask */
+	int slv_addr_shift; /* Number of bits to ignore */
+};
+
+static struct chip_info_t chip_info_tables[] = {
+	[amix9554] = {
+		.bits = 8,
+		.slv_addr = 0x20,
+		.slv_addr_shift = 3,
+	},
+	[amix9555] = {
+		.bits = 16,
+		.slv_addr = 0x20,
+		.slv_addr_shift = 3,
+	},
+};
+
+struct amix955x_reg_config {
+	int direction;
+	int output;
+	int input;
+	int invert;
+};
+
+static const struct amix955x_reg_config amix955x_regs = {
+	.direction = AMIX955X_DIRECTION,
+	.output = AMIX955X_OUTPUT,
+	.input = AMIX955X_INPUT,
+	.invert = AMIX955X_INVERT,
+};
+
+struct driver_data {
+	struct mutex lock; //protect read/write PCA955X
+	struct i2c_client *client;
+	struct chip_info_t *chip_info;
+	struct amix955x_led *leds;
+	struct amix955x_gpio *gpio;
+	int (*write_regs)(struct i2c_client *i2c, int reg, u8 *val);
+	int (*read_regs)(struct i2c_client *i2c, int reg, u8 *val);
+};
+
+struct amix955x_led {
+	struct driver_data *data;
+	struct led_classdev led_cdev;
+	int led_id;
+	char name[32];
+	const char *default_trigger;
+};
+
+struct amix955x_gpio {
+	u8 reg_output[MAX_BANK];
+	u8 reg_direction[MAX_BANK];
+	int gpio_id[MAX_BANK * BANK_SIZE];//the id list of gpio pin
+
+	struct gpio_chip gpio_chip;
+	const struct amix955x_reg_config *regs;
+};
+
+struct amix955x_platform_data {
+	struct driver_data *data;
+	struct amix955x_led *leds;
+	struct amix955x_gpio *gpio;
+	int num_leds;
+	int gpio_start;
+	int num_gpio;
+};
+
+static const struct acpi_device_id acpi_amix955x_match[] = {
+	{ "AHC0320", amix9554 },
+	{ "AHC0321", amix9555 },
+	{},
+};
+MODULE_DEVICE_TABLE(acpi, acpi_amix955x_match);
+
+static int get_device_index(struct i2c_client *client,
+			    const struct i2c_device_id *id)
+{
+	const struct acpi_device_id *acpi_id;
+
+	if (id)
+		return id->driver_data;
+
+	acpi_id = acpi_match_device(acpi_amix955x_match, &client->dev);
+	if (!acpi_id)
+		return -ENODEV;
+
+	return acpi_id->driver_data;
+}
+
+static inline u8 amix955x_set_bit(u8 val, int led_id, enum AMIX955X_LED state)
+{
+	led_id %= BANK_SIZE;
+
+	switch (state) {
+	case AMIX955X_LED_ON:
+		val &= ~(1 << led_id);
+		break;
+	case AMIX955X_LED_OFF:
+		val |= 1 << led_id;
+		break;
+	}
+
+	return val;
+}
+
+static int amix955x_read_regs_8(struct i2c_client *client, int reg, u8 *val)
+{
+	int ret = i2c_smbus_read_byte_data(client, reg);
+	*val = ret;
+	return ret;
+}
+
+static int amix955x_read_regs_16(struct i2c_client *client, int reg, u8 *val)
+{
+	int ret = i2c_smbus_read_word_data(client, reg << 1);
+
+	val[0] = (u16)ret & 0xFF;
+	val[1] = (u16)ret >> 8;
+
+	return ret;
+}
+
+static int amix955x_read_regs(struct driver_data *data, int reg, u8 *val)
+{
+	int ret;
+
+	ret = data->read_regs(data->client, reg, val);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "failed reading register\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int amix955x_read_single(struct driver_data *data, int reg, u32 *val,
+				int off)
+{
+	int ret;
+	int bank_shift = fls((data->chip_info->bits - 1) / BANK_SIZE);
+	int offset = off / BANK_SIZE;
+
+	ret = i2c_smbus_read_byte_data(data->client,
+				       (reg << bank_shift) + offset);
+
+	*val = ret;
+
+	if (ret < 0)
+		dev_err(&data->client->dev, "failed reading register\n");
+
+	return ret;
+}
+
+static int amix955x_write_regs_8(struct i2c_client *client, int reg, u8 *val)
+{
+	return i2c_smbus_write_byte_data(client, reg, *val);
+}
+
+static int amix955x_write_regs_16(struct i2c_client *client, int reg, u8 *val)
+{
+	__le16 word = cpu_to_le16(get_unaligned((u16 *)val));
+
+	return i2c_smbus_write_word_data(client, reg << 1, (__force u16)word);
+}
+
+static int amix955x_write_regs(struct driver_data *data, int reg, u8 *val)
+{
+	int ret;
+
+	ret = data->write_regs(data->client, reg, val);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "failed writing register\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int amix955x_write_single(struct driver_data *data, int reg, u32 val,
+				 int off)
+{
+	int ret;
+	int bank_shift = fls((data->chip_info->bits - 1) / BANK_SIZE);
+	int offset = off / BANK_SIZE;
+
+	ret = i2c_smbus_write_byte_data(data->client,
+					(reg << bank_shift) + offset, val);
+
+	if (ret < 0)
+		dev_err(&data->client->dev, "failed writing register\n");
+
+	return ret;
+}
+
+static int amix955x_led_set(struct led_classdev *led_cdev,
+			    enum led_brightness value)
+{
+	struct amix955x_led *led;
+	struct driver_data *data;
+	int led_id;
+	int ret = 0;
+	u32 reg_val = 0;
+
+	led = container_of(led_cdev, struct amix955x_led, led_cdev);
+	data = led->data;
+	led_id = led->led_id;
+
+	mutex_lock(&data->lock);
+
+	//get current value
+	ret = amix955x_read_single(data, AMIX955X_OUTPUT, &reg_val, led_id);
+	if (ret < 0)
+		goto set_failed;
+
+	switch (value) {
+	case LED_OFF:
+		reg_val = amix955x_set_bit(reg_val, led_id, AMIX955X_LED_OFF);
+		break;
+	default:
+		reg_val = amix955x_set_bit(reg_val, led_id, AMIX955X_LED_ON);
+		break;
+	}
+
+	ret = amix955x_write_single(data, AMIX955X_OUTPUT, reg_val, led_id);
+
+set_failed:
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+static int amix955x_gpio_direction_input(struct gpio_chip *gc, unsigned int off)
+{
+	struct driver_data *data = gpiochip_get_data(gc);
+	u8 reg_val;
+	unsigned int index;
+	int ret;
+
+	index = data->gpio->gpio_id[off];
+
+	mutex_lock(&data->lock);
+	reg_val = data->gpio->reg_direction[index / BANK_SIZE]
+					| (1u << (index % BANK_SIZE));
+
+	ret = amix955x_write_single(data, data->gpio->regs->direction,
+				    reg_val, index);
+	if (ret)
+		goto exit;
+
+	data->gpio->reg_direction[index / BANK_SIZE] = reg_val;
+exit:
+	mutex_unlock(&data->lock);
+	return ret;
+}
+
+static int amix955x_gpio_direction_output(struct gpio_chip *gc,
+					  unsigned int off, int val)
+{
+	struct driver_data *data = gpiochip_get_data(gc);
+	u8 reg_val;
+	unsigned int index;
+	int ret;
+
+	index = data->gpio->gpio_id[off];
+
+	mutex_lock(&data->lock);
+	if (val) {
+		reg_val = data->gpio->reg_output[index / BANK_SIZE]
+			| (1u << (index & BANK_SIZE));
+	} else {
+		reg_val = data->gpio->reg_output[index / BANK_SIZE]
+			& ~(1u << (index % BANK_SIZE));
+	}
+
+	ret = amix955x_write_single(data, data->gpio->regs->output,
+				    reg_val, index);
+	if (ret)
+		goto exit;
+
+	data->gpio->reg_output[index / BANK_SIZE] = reg_val;
+
+	reg_val = data->gpio->reg_direction[index / BANK_SIZE]
+					& ~(1u << (index % BANK_SIZE));
+
+	ret = amix955x_write_single(data, data->gpio->regs->direction,
+				    reg_val, index);
+	if (ret)
+		goto exit;
+
+	data->gpio->reg_direction[index / BANK_SIZE] = reg_val;
+exit:
+	mutex_unlock(&data->lock);
+	return ret;
+}
+
+static int amix955x_gpio_get_value(struct gpio_chip *gc, unsigned int off)
+{
+	struct driver_data *data = gpiochip_get_data(gc);
+	u32 reg_val;
+	int ret;
+	unsigned int index;
+
+	index = data->gpio->gpio_id[off];
+
+	mutex_lock(&data->lock);
+	ret = amix955x_read_single(data, data->gpio->regs->input,
+				   &reg_val, index);
+	mutex_unlock(&data->lock);
+
+	if (ret < 0)
+		return 0;
+
+	return (reg_val & (1u << (index % BANK_SIZE))) ? 1 : 0;
+}
+
+static void amix955x_gpio_set_value(struct gpio_chip *gc, unsigned int off,
+				    int val)
+{
+	struct driver_data *data = gpiochip_get_data(gc);
+	u8 reg_val;
+	int ret;
+	unsigned int index;
+
+	index = data->gpio->gpio_id[off];
+
+	mutex_lock(&data->lock);
+	if (val)
+		reg_val = data->gpio->reg_output[index / BANK_SIZE]
+						| (1u << (index % BANK_SIZE));
+	else
+		reg_val = data->gpio->reg_output[index / BANK_SIZE]
+						& ~(1u << (index % BANK_SIZE));
+
+	ret = amix955x_write_single(data, data->gpio->regs->output,
+				    reg_val, index);
+	if (ret)
+		goto exit;
+
+	data->gpio->reg_output[index / BANK_SIZE] = reg_val;
+
+exit:
+	mutex_unlock(&data->lock);
+}
+
+static int amix955x_gpio_get_direction(struct gpio_chip *gc, unsigned int off)
+{
+	struct driver_data *data = gpiochip_get_data(gc);
+	u32 reg_val;
+	int ret;
+	unsigned int index;
+
+	index = data->gpio->gpio_id[off];
+
+	mutex_lock(&data->lock);
+	ret = amix955x_read_single(data, data->gpio->regs->direction,
+				   &reg_val, index);
+	mutex_unlock(&data->lock);
+	if (ret < 0)
+		return ret;
+
+	return !!(reg_val & (1u << (index % BANK_SIZE)));
+}
+
+static void amix955x_setup_gpio(struct amix955x_platform_data *pdata,
+				struct driver_data *data)
+{
+	struct gpio_chip *gc;
+
+	gc = &pdata->gpio->gpio_chip;
+
+	gc->direction_input = amix955x_gpio_direction_input;
+	gc->direction_output = amix955x_gpio_direction_output;
+	gc->get = amix955x_gpio_get_value;
+	gc->set = amix955x_gpio_set_value;
+	gc->get_direction = amix955x_gpio_get_direction;
+	gc->can_sleep = true;
+
+	gc->base = pdata->gpio_start;
+	gc->ngpio = pdata->num_gpio;
+
+	gc->label = dev_name(&data->client->dev);
+	gc->parent = &data->client->dev;
+
+	gc->owner = THIS_MODULE;
+}
+
+static struct amix955x_platform_data *
+amix955x_get_pdata(struct i2c_client *client, struct chip_info_t *chip)
+{
+	struct amix955x_platform_data *pdata;
+	struct fwnode_handle *child;
+	u32 bitmask = 0;
+	int pin_id = 0;
+	int count = 0;
+	int index = 0;
+
+	count = device_get_child_node_count(&client->dev);
+	if (count > chip->bits)
+		return ERR_PTR(-ENODEV);
+
+	pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return ERR_PTR(-ENOMEM);
+
+	pdata->num_leds = 0;
+	//BIOS declares that the LED information is in the ACPI table
+	if (count > 0) {
+		pdata->leds = devm_kcalloc(&client->dev, count,
+					   sizeof(struct amix955x_led),
+					   GFP_KERNEL);
+		if (!pdata->leds)
+			return ERR_PTR(-ENOMEM);
+
+		device_for_each_child_node(&client->dev, child) {
+			const char *name;
+			u32 reg;
+			int res;
+
+			res = fwnode_property_read_u32(child, "reg", &reg);
+			if (res != 0 || reg >= chip->bits)
+				continue;
+
+			pdata->leds[index].led_id = reg;
+			bitmask |= 1 << reg;
+
+			res = fwnode_property_read_string(child, "label",
+							  &name);
+			if (res != 0 && is_of_node(child))
+				name = to_of_node(child)->name;
+
+			snprintf(pdata->leds[index].name,
+				 sizeof(pdata->leds[index].name), "%s", name);
+
+			index++;
+		}
+
+		pdata->num_leds = count;
+	}
+
+	//No pin needs to be initialized to gpio
+	if (count == chip->bits)
+		return pdata;
+
+	pdata->gpio = devm_kzalloc(&client->dev, sizeof(struct amix955x_gpio),
+				   GFP_KERNEL);
+	if (!pdata->gpio)
+		return ERR_PTR(-ENOMEM);
+
+	for (pin_id = 0, index = 0; pin_id < chip->bits; pin_id++) {
+		//this pin is used be led
+		if ((bitmask >> pin_id) & 0x01)
+			continue;
+
+		pdata->gpio->gpio_id[index] = pin_id;
+		index++;
+	}
+
+	pdata->num_gpio = index;
+
+	/*
+	 * Please pass -1 as base to let gpiolib select the chip base
+	 * in all possible cases
+	 */
+	pdata->gpio_start = -1;
+
+	return pdata;
+}
+
+static void
+amix955x_set_direction_as_output(struct amix955x_platform_data *pdata)
+{
+	u8 i;
+	u16 val = AMIX955X_DIRECTION_DEFAULT;
+	struct driver_data *data = pdata->data;
+	struct i2c_client *client = data->client;
+
+	for (i = 0; i < pdata->num_leds; i++)
+		val &= ~(1 << pdata->leds[i].led_id);
+
+	data->write_regs(client, AMIX955X_DIRECTION, (u8 *)&val);
+}
+
+static void amix955x_init_output_reg(struct driver_data *data)
+{
+	u8 val[MAX_BANK] = {AMIX955X_OUTPUT_DEFAULT};
+	struct i2c_client *client = data->client;
+
+	data->write_regs(client, AMIX955X_OUTPUT, val);
+}
+
+static int device_amix955x_init(struct amix955x_platform_data *pdata)
+{
+	int ret;
+	u8 val[MAX_BANK] = {0};
+
+	pdata->gpio->regs = &amix955x_regs;
+
+	ret = amix955x_read_regs(pdata->data, pdata->gpio->regs->output,
+				 pdata->gpio->reg_output);
+	if (ret)
+		goto out;
+
+	ret = amix955x_read_regs(pdata->data, pdata->gpio->regs->direction,
+				 pdata->gpio->reg_direction);
+	if (ret)
+		goto out;
+
+	//set non invert mode
+	ret = amix955x_write_regs(pdata->data, AMIX955X_INVERT, val);
+out:
+	return ret;
+}
+
+static int amix955x_probe(struct i2c_client *client,
+			  const struct i2c_device_id *id)
+{
+	struct driver_data *data;
+	struct chip_info_t *chip;
+	int ret;
+	int i, err;
+	struct amix955x_platform_data *pdata;
+
+	int device_id = 0;
+
+	device_id = get_device_index(client, id);
+	if (device_id < 0 || device_id >= ARRAY_SIZE(chip_info_tables))
+		return -ENODEV;
+
+	chip = &chip_info_tables[device_id];
+
+	pdata = amix955x_get_pdata(client, chip);
+	if (IS_ERR(pdata))
+		return PTR_ERR(pdata);
+
+	/* Make sure the slave address given is possible */
+	if ((client->addr & ~((1 << chip->slv_addr_shift) - 1)) !=
+	    chip->slv_addr) {
+		dev_err(&client->dev, "invalid slave address %02x\n",
+			client->addr);
+		return -ENODEV;
+	}
+
+	dev_info(&client->dev,
+		 "%s: Using %s %d-bit LED driver at slave address 0x%02x\n",
+		 client->dev.driver->name, client->name, chip->bits,
+		 client->addr);
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
+		return -EIO;
+
+	//Initial driver data
+	data = devm_kzalloc(&client->dev, sizeof(struct driver_data),
+			    GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	mutex_init(&data->lock);
+
+	data->leds = pdata->leds;
+	data->gpio = pdata->gpio;
+	data->client = client;
+	data->chip_info = chip;
+
+	switch (chip->bits) {
+	case 8:
+		data->read_regs = amix955x_read_regs_8;
+		data->write_regs = amix955x_write_regs_8;
+		break;
+	case 16:
+		data->read_regs = amix955x_read_regs_16;
+		data->write_regs = amix955x_write_regs_16;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	pdata->data = data;
+
+	i2c_set_clientdata(client, data);
+
+	/* Set GPIO direction as output */
+	amix955x_set_direction_as_output(pdata);
+
+	/* Turn off all LEDs */
+	amix955x_init_output_reg(data);
+
+	for (i = 0; i < pdata->num_leds; i++) {
+		struct amix955x_led *led;
+		struct led_classdev *led_cdev;
+
+		led = &data->leds[i];
+		led_cdev = &led->led_cdev;
+
+		led->data = data;
+
+		led_cdev->name = led->name;
+		led_cdev->brightness_set_blocking = amix955x_led_set;
+
+		if (pdata->leds[i].default_trigger) {
+			led_cdev->default_trigger =
+				pdata->leds[i].default_trigger;
+		}
+
+		err = devm_led_classdev_register(&client->dev, led_cdev);
+		if (err)
+			return err;
+
+		amix955x_led_set(led_cdev, LED_OFF);
+	}
+
+	amix955x_setup_gpio(pdata, data);
+
+	ret = device_amix955x_init(pdata);
+	if (ret < 0)
+		return ret;
+
+	ret = devm_gpiochip_add_data(&client->dev, &data->gpio->gpio_chip,
+				     data);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static struct i2c_driver amix955x_driver = {
+	.driver = {
+		.name = "amix955x",
+		.acpi_match_table = ACPI_PTR(acpi_amix955x_match),
+	},
+	.probe = amix955x_probe,
+};
+
+module_i2c_driver(amix955x_driver);
+
+MODULE_AUTHOR("Jia Sui <jia.sui@advantech.com.cn>");
+MODULE_DESCRIPTION("AMIX955X LED and GPIO driver");
+MODULE_LICENSE("GPL v2");
+MODULE_VERSION(VERSION);
-- 
2.27.0

Re: [v3,1/1] adv_mix955x is a scheme that multiplexes PCA9554/PCA9555 into LED and GPIO

[Andy Shevchenko]

On Fri, Apr 30, 2021 at 8:27 AM yuechao.zhao(赵越超)
<yuechao.zhao@advantech.com.cn> wrote:
>
> From: Yuechao Zhao <yuechao.zhao@advantech.com.cn>
>
> With this driver, we can multiplex PCA9554/PCA9555 into LED and GPIO
> based on the ACPI data of BIOS.

NAK as per v2.

Please, add a proper documentation and show ACPI excerpt, and last but
not least is missing justification.


-- 
With Best Regards,
Andy Shevchenko

Re: [v3,1/1] adv_mix955x is a scheme that multiplexes PCA9554/PCA9555 into LED and GPIO

[yuechao.zhao(赵越超)]

Hi Andy
Sorry for the late reply

The reason for writing this driver is that our customers hope that the LED and GPIO can be used out of the box
When they using our X86-Platform

About the document and ACPI expert, I will provide them after I improve them. Please wait for a few days.

Thank you very much

Yuechao Zhao

在 2021/5/2 下午6:50，“Andy Shevchenko”<andy.shevchenko@gmail.com> 写入:

    On Fri, Apr 30, 2021 at 8:27 AM yuechao.zhao(赵越超)
    <yuechao.zhao@advantech.com.cn> wrote:
    >
    > From: Yuechao Zhao <yuechao.zhao@advantech.com.cn>
    >
    > With this driver, we can multiplex PCA9554/PCA9555 into LED and GPIO
    > based on the ACPI data of BIOS.

    NAK as per v2.

    Please, add a proper documentation and show ACPI excerpt, and last but
    not least is missing justification.


    -- 
    With Best Regards,
    Andy Shevchenko

Re: [v3,1/1] adv_mix955x is a scheme that multiplexes PCA9554/PCA9555 into LED and GPIO

[Hans de Goede]

Hi,

On 5/3/21 3:16 PM, yuechao.zhao(赵越超) wrote:
> Hi Andy
> Sorry for the late reply
> 
> The reason for writing this driver is that our customers hope that the LED and GPIO can be used out of the box
> When they using our X86-Platform
> 
> About the document and ACPI expert, I will provide them after I improve them. Please wait for a few days.

So my initial assessment of this code matches Andy's I don't like the idea
of duplicating the GPIO chip functionality from drivers/gpio/gpio-pca953x.c here.

I understand that you want to have things working out of the box and I believe
that we all agree that that is what we want.

But you can get there without duplicating the code.

We already have cases where there is an I2C device with an existing driver
where we need some "glue" code to translate the ACPI provided info to
what the i2c drivers expect, some examples of these are:

drivers/platform/x86/i2c-multi-instantiate.c
drivers/platform/x86/intel_cht_int33fe_typec.c

So what you need to do is basically start writing a completely new driver
which:

1. Instantiates an i2c-client for the drivers/gpio/gpio-pca953x.c driver
to bind to.

2. Creates and attaches software-fwnodes which provide info for the 
drivers/leds/leds-gpio.c code to parse and have that code instantiate
LED class devices which drive the leds through the GPIO interface
offered by the drivers/gpio/gpio-pca953x.c driver.

Regards,

Hans


> 在 2021/5/2 下午6:50，“Andy Shevchenko”<andy.shevchenko@gmail.com> 写入:
> 
>     On Fri, Apr 30, 2021 at 8:27 AM yuechao.zhao(赵越超)
>     <yuechao.zhao@advantech.com.cn> wrote:
>     >
>     > From: Yuechao Zhao <yuechao.zhao@advantech.com.cn>
>     >
>     > With this driver, we can multiplex PCA9554/PCA9555 into LED and GPIO
>     > based on the ACPI data of BIOS.
> 
>     NAK as per v2.
> 
>     Please, add a proper documentation and show ACPI excerpt, and last but
>     not least is missing justification.
> 
> 
>     -- 
>     With Best Regards,
>     Andy Shevchenko
> 

Re: [v3,1/1] adv_mix955x is a scheme that multiplexes PCA9554/PCA9555 into LED and GPIO

[Hans de Goede]

Hi,

On 5/5/21 3:23 PM, Hans de Goede wrote:
> Hi,
> 
> On 5/3/21 3:16 PM, yuechao.zhao(赵越超) wrote:
>> Hi Andy
>> Sorry for the late reply
>>
>> The reason for writing this driver is that our customers hope that the LED and GPIO can be used out of the box
>> When they using our X86-Platform
>>
>> About the document and ACPI expert, I will provide them after I improve them. Please wait for a few days.
> 
> So my initial assessment of this code matches Andy's I don't like the idea
> of duplicating the GPIO chip functionality from drivers/gpio/gpio-pca953x.c here.
> 
> I understand that you want to have things working out of the box and I believe
> that we all agree that that is what we want.
> 
> But you can get there without duplicating the code.
> 
> We already have cases where there is an I2C device with an existing driver
> where we need some "glue" code to translate the ACPI provided info to
> what the i2c drivers expect, some examples of these are:
> 
> drivers/platform/x86/i2c-multi-instantiate.c
> drivers/platform/x86/intel_cht_int33fe_typec.c
> 
> So what you need to do is basically start writing a completely new driver
> which:
> 
> 1. Instantiates an i2c-client for the drivers/gpio/gpio-pca953x.c driver
> to bind to.
> 
> 2. Creates and attaches software-fwnodes which provide info for the 
> drivers/leds/leds-gpio.c code to parse and have that code instantiate
> LED class devices which drive the leds through the GPIO interface
> offered by the drivers/gpio/gpio-pca953x.c driver.

I just saw the "Advantech AHC1EC0 embedded controller" patch series
posted by your colleague Campion Kang (added to the Cc) it seems that
that series is using devicetree config bits embedded inside the ACPI
tables. If you can still change / update the ACPI tables then perhaps
using that would be an option here too ?  And then just include the
whole GPIO LED descriptions inside the embedded devicetree bits.

Regards,

Hans



>> 在 2021/5/2 下午6:50，“Andy Shevchenko”<andy.shevchenko@gmail.com> 写入:
>>
>>     On Fri, Apr 30, 2021 at 8:27 AM yuechao.zhao(赵越超)
>>     <yuechao.zhao@advantech.com.cn> wrote:
>>     >
>>     > From: Yuechao Zhao <yuechao.zhao@advantech.com.cn>
>>     >
>>     > With this driver, we can multiplex PCA9554/PCA9555 into LED and GPIO
>>     > based on the ACPI data of BIOS.
>>
>>     NAK as per v2.
>>
>>     Please, add a proper documentation and show ACPI excerpt, and last but
>>     not least is missing justification.
>>
>>
>>     -- 
>>     With Best Regards,
>>     Andy Shevchenko
>>