Re: [PATCH v6 4/6] mfd: ahc1ec0: Add support for Advantech embedded controller

[Lee Jones <lee.jones@linaro.org>]

On Mon, 18 Jan 2021, Campion Kang wrote:

> AHC1EC0 is the embedded controller driver for Advantech industrial

It would be nice to have the model number in the subject line.

Drop "driver".

> products. This provides sub-devices such as hwmon and watchdog, and
> also

"HWMON and Watchdog"

> expose functions for sub-devices to read/write the value to embedded

"exposes"

> controller.
> 
> Changed since V5:
> 	- Kconfig: add "AHC1EC0" string to clearly define the EC name
> 	- fix the code according to reviewer's suggestion
> 	- remove unnecessary header files
> 	- change the structure name to lower case, align with others
> naming
> 
> Signed-off-by: Campion Kang <campion.kang@advantech.com.tw>
> ---
>  drivers/mfd/Kconfig         |  10 +
>  drivers/mfd/Makefile        |   2 +
>  drivers/mfd/ahc1ec0.c       | 808 ++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/ahc1ec0.h | 276 ++++++++++++
>  4 files changed, 1096 insertions(+)
>  create mode 100644 drivers/mfd/ahc1ec0.c
>  create mode 100644 include/linux/mfd/ahc1ec0.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index bdfce7b15621..7d5fb5c17d9a 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2154,5 +2154,15 @@ config MFD_INTEL_M10_BMC
>  	  additional drivers must be enabled in order to use the functionality
>  	  of the device.
>  
> +config MFD_AHC1EC0
> +	tristate "Advantech AHC1EC0 Embedded Controller Module"

Please remove "Module"

> +	depends on X86
> +	select MFD_CORE
> +	help
> +	  This is the core function that for Advantech EC drivers. It
> +	  includes the sub-devices such as hwmon, watchdog, etc. And also
> +	  provides expose functions for sub-devices to read/write the value
> +	  to embedded controller.

"This provides core functionality for the Advantech AHC1EC0 Embedded
Controller (EC) along with registration for HWMON and Watchdog
sub-devices.  It also provides read and write APIs for communication
with the EC."

>  endmenu
>  endif
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 14fdb188af02..a6af9d8825f4 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -268,3 +268,5 @@ obj-$(CONFIG_MFD_KHADAS_MCU) 	+= khadas-mcu.o
>  obj-$(CONFIG_SGI_MFD_IOC3)	+= ioc3.o
>  obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)	+= simple-mfd-i2c.o
>  obj-$(CONFIG_MFD_INTEL_M10_BMC)   += intel-m10-bmc.o
> +

You don't need to space this out.

> +obj-$(CONFIG_MFD_AHC1EC0)	+= ahc1ec0.o
> diff --git a/drivers/mfd/ahc1ec0.c b/drivers/mfd/ahc1ec0.c
> new file mode 100644
> index 000000000000..015f4307a54e
> --- /dev/null
> +++ b/drivers/mfd/ahc1ec0.c
> @@ -0,0 +1,808 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Advantech embedded controller core driver AHC1EC0

Advantech AHC1EC0 Embedded Controller
 
> + * Copyright 2020 Advantech IIoT Group

This is out of date.

> + *

Drop this '\n' please.

> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/delay.h>
> +#include <linux/dmi.h>
> +#include <linux/errno.h>
> +#include <linux/mfd/ahc1ec0.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +
> +#define DRV_NAME      "ahc1ec0"

Please don't do this.  Just use the string in-place.

> +enum {
> +	ADVEC_SUBDEV_BRIGHTNESS = 0,
> +	ADVEC_SUBDEV_EEPROM,
> +	ADVEC_SUBDEV_GPIO,
> +	ADVEC_SUBDEV_HWMON,
> +	ADVEC_SUBDEV_LED,
> +	ADVEC_SUBDEV_WDT,
> +	ADVEC_SUBDEV_MAX,
> +};

Are these arbitrary?

> +/* Wait IBF (Input Buffer Full) clear */
> +static int ec_wait_write(void)
> +{
> +	int i;
> +
> +	for (i = 0; i < EC_MAX_TIMEOUT_COUNT; i++) {
> +		if ((inb(EC_COMMAND_PORT) & EC_COMMAND_BIT_IBF) == 0)
> +			return 0;
> +
> +		udelay(EC_RETRY_UDELAY);
> +	}
> +
> +	return -ETIMEDOUT;
> +}
> 
> +/* Wait OBF (Output Buffer Full) data ready */
> +static int ec_wait_read(void)
> +{
> +	int i;
> +
> +	for (i = 0; i < EC_MAX_TIMEOUT_COUNT; i++) {
> +		if ((inb(EC_COMMAND_PORT) & EC_COMMAND_BIT_OBF) != 0)
> +			return 0;
> +
> +		udelay(EC_RETRY_UDELAY);
> +	}
> +
> +	return -ETIMEDOUT;
> +}
> +
> +/* Read data from EC HW RAM, the process is the following:
> + * Step 0. Wait IBF clear to send command
> + * Step 1. Send read command to EC command port
> + * Step 2. Wait IBF clear that means command is got by EC
> + * Step 3. Send read address to EC data port
> + * Step 4. Wait OBF data ready
> + * Step 5. Get data from EC data port
> + */
> +int read_hw_ram(struct adv_ec_platform_data *adv_ec_data, unsigned char addr, unsigned char *data)
> +{
> +	int ret;
> +
> +	mutex_lock(&adv_ec_data->lock);
> +
> +	ret = ec_wait_write();
> +	if (ret)
> +		goto error;
> +	outb(EC_HW_RAM_READ, EC_COMMAND_PORT);
> +
> +	ret = ec_wait_write();
> +	if (ret)
> +		goto error;
> +	outb(addr, EC_STATUS_PORT);
> +
> +	ret = ec_wait_read();
> +	if (ret)
> +		goto error;
> +	*data = inb(EC_STATUS_PORT);
> +
> +	mutex_unlock(&adv_ec_data->lock);
> +
> +	return ret;
> +
> +error:
> +	mutex_unlock(&adv_ec_data->lock);
> +	dev_err(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d\n", __func__,
> +	       __LINE__);
> +
> +	return ret;
> +}

EXPORT?

> +/* Write data to EC HW RAM
> + * Step 0. Wait IBF clear to send command
> + * Step 1. Send write command to EC command port
> + * Step 2. Wait IBF clear that means command is got by EC
> + * Step 3. Send write address to EC data port
> + * Step 4. Wait IBF clear that means command is got by EC
> + * Step 5. Send data to EC data port
> + */
> +int write_hw_ram(struct adv_ec_platform_data *adv_ec_data, unsigned char addr, unsigned char data)
> +{
> +	int ret;
> +
> +	mutex_lock(&adv_ec_data->lock);
> +
> +	ret = ec_wait_write();
> +	if (ret)
> +		goto error;
> +	outb(EC_HW_RAM_WRITE, EC_COMMAND_PORT);
> +
> +	ret = ec_wait_write();
> +	if (ret)
> +		goto error;
> +	outb(addr, EC_STATUS_PORT);
> +
> +	ret = ec_wait_write();
> +	if (ret)
> +		goto error;
> +	outb(data, EC_STATUS_PORT);
> +
> +	mutex_unlock(&adv_ec_data->lock);
> +
> +	return 0;
> +
> +error:
> +	mutex_unlock(&adv_ec_data->lock);
> +
> +	dev_err(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d\n", __func__,
> +	       __LINE__);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(write_hw_ram);

If you're going to export these, they should be in their own
namespace.

> +/* Get dynamic control table */
> +static int adv_get_dynamic_tab(struct adv_ec_platform_data *adv_ec_data)
> +{
> +	int i, ret;
> +	unsigned char pin_tmp, device_id;
> +
> +	mutex_lock(&adv_ec_data->lock);
> +
> +	for (i = 0; i < EC_MAX_TBL_NUM; i++) {
> +		adv_ec_data->dym_tbl[i].device_id = 0xff;
> +		adv_ec_data->dym_tbl[i].hw_pin_num = 0xff;
> +	}
> +
> +	for (i = 0; i < EC_MAX_TBL_NUM; i++) {
> +		ret = ec_wait_write();
> +		if (ret) {
> +			dev_dbg(adv_ec_data->dev, "%s: ec_wait_write. line: %d\n", __func__,
> +				__LINE__);

Please remove all of this debug code.

> +			goto error;
> +		}
> +		outb(EC_TBL_WRITE_ITEM, EC_COMMAND_PORT);

More basic commentary is required throughout I think.

> +		ret = ec_wait_write();
> +		if (ret) {
> +			dev_dbg(adv_ec_data->dev, "%s: ec_wait_write. line: %d\n", __func__,
> +				__LINE__);
> +			goto error;
> +		}
> +		outb(i, EC_STATUS_PORT);
> +
> +		ret = ec_wait_read();
> +		if (ret) {
> +			dev_dbg(adv_ec_data->dev, "%s: ec_wait_read. line: %d\n", __func__,
> +				__LINE__);
> +			goto error;
> +		}
> +
> +		/*
> +		 *  If item is defined, EC will return item number.
> +		 *  If table item is not defined, EC will return 0xFF.
> +		 */
> +		pin_tmp = inb(EC_STATUS_PORT);
> +		if (pin_tmp == 0xff) {

Please define all magic numbers.

> +			dev_dbg(adv_ec_data->dev, "%s: inb(EC_STATUS_PORT)=0x%02x != 0xff.\n",
> +				__func__, pin_tmp);
> +			goto pass;
> +		}
> +
> +		ret = ec_wait_write();
> +		if (ret) {
> +			dev_dbg(adv_ec_data->dev, "%s: ec_wait_write. line: %d\n", __func__,
> +				__LINE__);
> +			goto error;
> +		}
> +		outb(EC_TBL_GET_PIN, EC_COMMAND_PORT);
> +
> +		ret = ec_wait_read();
> +		if (ret) {
> +			dev_dbg(adv_ec_data->dev, "%s: ec_wait_read. line: %d\n", __func__,
> +				__LINE__);
> +			goto error;
> +		}
> +		pin_tmp = inb(EC_STATUS_PORT) & 0xff;
> +		if (pin_tmp == 0xff) {
> +			dev_dbg(adv_ec_data->dev, "%s: pin_tmp(0x%02X). line: %d\n", __func__,
> +				pin_tmp, __LINE__);
> +			goto pass;
> +		}
> +
> +		ret = ec_wait_write();
> +		if (ret)
> +			goto error;
> +		outb(EC_TBL_GET_DEVID, EC_COMMAND_PORT);
> +
> +		ret = ec_wait_read();
> +		if (ret) {
> +			dev_dbg(adv_ec_data->dev, "%s: ec_wait_read. line: %d\n", __func__,
> +				__LINE__);
> +			goto error;
> +		}
> +		device_id = inb(EC_STATUS_PORT) & 0xff;
> +
> +		dev_dbg(adv_ec_data->dev, "%s: device_id=0x%02X. line: %d\n", __func__,
> +			device_id, __LINE__);
> +
> +		adv_ec_data->dym_tbl[i].device_id = device_id;
> +		adv_ec_data->dym_tbl[i].hw_pin_num = pin_tmp;
> +	}
> +
> +pass:
> +	mutex_unlock(&adv_ec_data->lock);
> +	return 0;
> +
> +error:
> +	mutex_unlock(&adv_ec_data->lock);
> +	dev_err(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d\n",
> +		__func__, __LINE__);

Remove all _func_ and _LINE_ info.  It's seldom helpful.

> +	return ret;
> +}
> +
> +int read_ad_value(struct adv_ec_platform_data *adv_ec_data, unsigned char hwpin,

What is 'ad'?

> +		unsigned char multi)
> +{
> +	int ret;
> +	u32 ret_val;
> +	unsigned int LSB, MSB;
> +
> +	mutex_lock(&adv_ec_data->lock);
> +	ret = ec_wait_write();
> +	if (ret)
> +		goto error;
> +	outb(EC_AD_INDEX_WRITE, EC_COMMAND_PORT);
> +
> +	ret = ec_wait_write();
> +	if (ret)
> +		goto error;
> +	outb(hwpin, EC_STATUS_PORT);
> +
> +	ret = ec_wait_read();
> +	if (ret)
> +		goto error;
> +
> +	if (inb(EC_STATUS_PORT) == 0xff) {
> +		mutex_unlock(&adv_ec_data->lock);
> +		return -1;
> +	}
> +
> +	ret = ec_wait_write();
> +	if (ret)
> +		goto error;
> +	outb(EC_AD_LSB_READ, EC_COMMAND_PORT);
> +
> +	ret = ec_wait_read();
> +	if (ret)
> +		goto error;
> +	LSB = inb(EC_STATUS_PORT);
> +
> +	ret = ec_wait_write();
> +	if (ret)
> +		goto error;
> +	outb(EC_AD_MSB_READ, EC_COMMAND_PORT);
> +
> +	ret = ec_wait_read();
> +	if (ret)
> +		goto error;
> +	MSB = inb(EC_STATUS_PORT);
> +	ret_val = ((MSB << 8) | LSB) & 0x03FF;
> +	ret_val = ret_val * multi * 100;
> +
> +	mutex_unlock(&adv_ec_data->lock);
> +	return ret_val;
> +
> +error:
> +	mutex_unlock(&adv_ec_data->lock);
> +
> +	dev_warn(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d\n", __func__,
> +		__LINE__);

dev_err()

> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(read_ad_value);
> +
> +int read_acpi_value(struct adv_ec_platform_data *adv_ec_data, unsigned char addr,
> +		unsigned char *pvalue)
> +{
> +	int ret;
> +	unsigned char value;
> +
> +	mutex_lock(&adv_ec_data->lock);
> +
> +	ret = ec_wait_write();
> +	if (ret)
> +		goto error;
> +	outb(EC_ACPI_RAM_READ, EC_COMMAND_PORT);
> +
> +	ret = ec_wait_write();
> +	if (ret)
> +		goto error;
> +	outb(addr, EC_STATUS_PORT);
> +
> +	ret = ec_wait_read();
> +	if (ret)
> +		goto error;
> +	value = inb(EC_STATUS_PORT);
> +	*pvalue = value;
> +
> +	mutex_unlock(&adv_ec_data->lock);
> +
> +	return 0;
> +
> +error:
> +	mutex_unlock(&adv_ec_data->lock);
> +
> +	dev_warn(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d\n", __func__,
> +		__LINE__);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(read_acpi_value);

Namespace.

> +int write_acpi_value(struct adv_ec_platform_data *adv_ec_data, unsigned char addr,
> +		unsigned char value)
> +{
> +	int ret;
> +
> +	mutex_lock(&adv_ec_data->lock);
> +
> +	ret = ec_wait_write();
> +	if (ret)
> +		goto error;
> +	outb(EC_ACPI_DATA_WRITE, EC_COMMAND_PORT);
> +
> +	ret = ec_wait_write();
> +	if (ret)
> +		goto error;
> +	outb(addr, EC_STATUS_PORT);
> +
> +	ret = ec_wait_write();
> +	if (ret)
> +		goto error;
> +	outb(value, EC_STATUS_PORT);
> +
> +	mutex_unlock(&adv_ec_data->lock);
> +	return 0;
> +
> +error:
> +	mutex_unlock(&adv_ec_data->lock);
> +
> +	dev_warn(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d\n", __func__,
> +		__LINE__);
> +
> +	return ret;
> +}

EXPORT?

I think this API (i.e. all of the functions above) should be moved
into drivers/platform.  They really don't have a place in MFD.

> +int read_gpio_status(struct adv_ec_platform_data *adv_ec_data, unsigned char PinNumber,
> +		unsigned char *pvalue)
> +{
> +	int ret;
> +
> +	unsigned char gpio_status_value;
> +
> +	mutex_lock(&adv_ec_data->lock);
> +
> +	ret = ec_wait_write();
> +	if (ret)
> +		goto error;
> +	outb(EC_GPIO_INDEX_WRITE, EC_COMMAND_PORT);
> +
> +	ret = ec_wait_write();
> +	if (ret)
> +		goto error;
> +	outb(PinNumber, EC_STATUS_PORT);
> +
> +	ret = ec_wait_read();
> +	if (ret)
> +		goto error;
> +
> +	if (inb(EC_STATUS_PORT) == 0xff) {
> +		dev_err(adv_ec_data->dev, "%s: Read Pin Number error!!\n", __func__);
> +		mutex_unlock(&adv_ec_data->lock);
> +		return -1;
> +	}
> +
> +	ret = ec_wait_write();
> +	if (ret)
> +		goto error;
> +	outb(EC_GPIO_STATUS_READ, EC_COMMAND_PORT);
> +
> +	ret = ec_wait_read();
> +	if (ret)
> +		goto error;
> +	gpio_status_value = inb(EC_STATUS_PORT);
> +
> +	*pvalue = gpio_status_value;
> +	mutex_unlock(&adv_ec_data->lock);
> +	return 0;
> +
> +error:
> +	mutex_unlock(&adv_ec_data->lock);
> +
> +	dev_warn(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d\n", __func__,
> +		__LINE__);
> +	return ret;
> +}
> +
> +int write_gpio_status(struct adv_ec_platform_data *adv_ec_data, unsigned char PinNumber,
> +		unsigned char value)
> +{
> +	int ret;
> +
> +	mutex_lock(&adv_ec_data->lock);
> +
> +	ret = ec_wait_write();
> +	if (ret)
> +		goto error;
> +	outb(EC_GPIO_INDEX_WRITE, EC_COMMAND_PORT);
> +
> +	ret = ec_wait_write();
> +	if (ret)
> +		goto error;
> +	outb(PinNumber, EC_STATUS_PORT);
> +
> +	ret = ec_wait_read();
> +	if (ret)
> +		goto error;
> +
> +	if (inb(EC_STATUS_PORT) == 0xff) {
> +		mutex_unlock(&adv_ec_data->lock);
> +		dev_err(adv_ec_data->dev, "%s: Read Pin Number error!!\n", __func__);
> +		return -1;
> +	}
> +
> +	ret = ec_wait_write();
> +	if (ret)
> +		goto error;
> +	outb(EC_GPIO_STATUS_WRITE, EC_COMMAND_PORT);
> +
> +	ret = ec_wait_write();
> +	if (ret)
> +		goto error;
> +	outb(value, EC_STATUS_PORT);
> +
> +	mutex_unlock(&adv_ec_data->lock);
> +	return 0;
> +
> +error:
> +	mutex_unlock(&adv_ec_data->lock);
> +	dev_err(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d", __func__,
> +		__LINE__);
> +
> +	return ret;
> +}
> +
> +int read_gpio_dir(struct adv_ec_platform_data *adv_ec_data, unsigned char PinNumber,
> +		unsigned char *pvalue)
> +{
> +	int ret;
> +	unsigned char gpio_dir_value;
> +
> +	mutex_lock(&adv_ec_data->lock);
> +
> +	ret = ec_wait_write();
> +	if (ret)
> +		goto error;
> +	outb(EC_GPIO_INDEX_WRITE, EC_COMMAND_PORT);
> +
> +	ret = ec_wait_write();
> +	if (ret)
> +		goto error;
> +	outb(PinNumber, EC_STATUS_PORT);
> +
> +	ret = ec_wait_read();
> +	if (ret)
> +		goto error;
> +
> +	if (inb(EC_STATUS_PORT) == 0xff) {
> +		mutex_unlock(&adv_ec_data->lock);
> +		dev_err(adv_ec_data->dev, "%s: Read Pin Number error!! line: %d\n", __func__,
> +			__LINE__);
> +		return -1;
> +	}
> +
> +	ret = ec_wait_write();
> +	if (ret)
> +		goto error;
> +	outb(EC_GPIO_DIR_READ, EC_COMMAND_PORT);
> +
> +	ret = ec_wait_read();
> +	if (ret)
> +		goto error;
> +	gpio_dir_value = inb(EC_STATUS_PORT);
> +	*pvalue = gpio_dir_value;
> +
> +	mutex_unlock(&adv_ec_data->lock);
> +	return 0;
> +
> +error:
> +	mutex_unlock(&adv_ec_data->lock);
> +
> +	dev_warn(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d\n", __func__,
> +			__LINE__);
> +
> +	return ret;
> +}
> +
> +int write_gpio_dir(struct adv_ec_platform_data *adv_ec_data, unsigned char PinNumber,
> +		unsigned char value)
> +{
> +	int ret;
> +
> +	mutex_lock(&adv_ec_data->lock);
> +
> +	ret = ec_wait_write();
> +	if (ret)
> +		goto error;
> +	outb(EC_GPIO_INDEX_WRITE, EC_COMMAND_PORT);
> +
> +	ret = ec_wait_write();
> +	if (ret)
> +		goto error;
> +	outb(PinNumber, EC_STATUS_PORT);
> +
> +	ret = ec_wait_read();
> +	if (ret)
> +		goto error;
> +
> +	if (inb(EC_STATUS_PORT) == 0xff) {
> +		mutex_unlock(&adv_ec_data->lock);
> +		dev_warn(adv_ec_data->dev, "%s: Read Pin Number error!! line: %d\n", __func__,
> +			__LINE__);
> +
> +		return -1;
> +	}
> +
> +	ret = ec_wait_write();
> +	if (ret)
> +		goto error;
> +	outb(EC_GPIO_DIR_WRITE, EC_COMMAND_PORT);
> +
> +	ret = ec_wait_write();
> +	if (ret)
> +		goto error;
> +	outb(value, EC_STATUS_PORT);
> +
> +	mutex_unlock(&adv_ec_data->lock);
> +	return 0;
> +
> +error:
> +	mutex_unlock(&adv_ec_data->lock);
> +
> +	dev_warn(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d\n", __func__,
> +			__LINE__);
> +
> +	return ret;
> +}

All of the GPIO functions above should move into drivers/gpio.

> +int write_hwram_command(struct adv_ec_platform_data *adv_ec_data, unsigned char data)
> +{
> +	int ret;
> +
> +	mutex_lock(&adv_ec_data->lock);
> +
> +	ret = ec_wait_write();
> +	if (ret)
> +		goto error;
> +	outb(data, EC_COMMAND_PORT);
> +
> +	mutex_unlock(&adv_ec_data->lock);
> +	return 0;
> +
> +error:
> +	mutex_unlock(&adv_ec_data->lock);
> +
> +	dev_warn(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d\n", __func__,
> +			__LINE__);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(write_hwram_command);
> +
> +static int adv_ec_get_productname(struct adv_ec_platform_data *adv_ec_data, char *product)
> +{
> +	const char *vendor, *device;
> +	int length = 0;
> +
> +	/* Check it is Advantech board */
> +	vendor = dmi_get_system_info(DMI_SYS_VENDOR);
> +	if (memcmp(vendor, "Advantech", sizeof("Advantech")) != 0)
> +		return -ENODEV;
> +
> +	/* Get product model name */
> +	device = dmi_get_system_info(DMI_PRODUCT_NAME);
> +	if (device) {
> +		while ((device[length] != ' ')
> +			&& (length < AMI_ADVANTECH_BOARD_ID_LENGTH))
> +			length++;
> +		memset(product, 0, AMI_ADVANTECH_BOARD_ID_LENGTH);
> +		memmove(product, device, length);
> +
> +		dev_info(adv_ec_data->dev, "BIOS Product Name = %s\n", product);
> +
> +		return 0;
> +	}
> +
> +	dev_warn(adv_ec_data->dev, "This device is not Advantech Board (%s)!\n", product);
> +
> +	return -ENODEV;
> +}

These should go into drivers/platform.

> +static const struct mfd_cell adv_ec_sub_cells[] = {
> +	{ .name = "adv-ec-brightness", },
> +	{ .name = "adv-ec-eeprom", },
> +	{ .name = "adv-ec-gpio", },
> +	{ .name = "ahc1ec0-hwmon", },
> +	{ .name = "adv-ec-led", },
> +	{ .name = "ahc1ec0-wdt", },
> +};
> +
> +static int adv_ec_init_ec_data(struct adv_ec_platform_data *adv_ec_data)
> +{
> +	int ret;
> +
> +	adv_ec_data->sub_dev_mask = 0;
> +	adv_ec_data->sub_dev_nb = 0;
> +	adv_ec_data->dym_tbl = NULL;
> +	adv_ec_data->bios_product_name = NULL;

Why are pre-initialising these?

> +	mutex_init(&adv_ec_data->lock);
> +
> +	/* Get product name */
> +	adv_ec_data->bios_product_name =
> +		devm_kzalloc(adv_ec_data->dev, AMI_ADVANTECH_BOARD_ID_LENGTH, GFP_KERNEL);
> +	if (!adv_ec_data->bios_product_name)
> +		return -ENOMEM;
> +
> +	memset(adv_ec_data->bios_product_name, 0, AMI_ADVANTECH_BOARD_ID_LENGTH);

Why are you doing this?

> +	ret = adv_ec_get_productname(adv_ec_data, adv_ec_data->bios_product_name);
> +	if (ret)
> +		return ret;
> +
> +	/* Get pin table */
> +	adv_ec_data->dym_tbl = devm_kzalloc(adv_ec_data->dev,
> +					EC_MAX_TBL_NUM * sizeof(struct ec_dynamic_table),
> +					GFP_KERNEL);
> +	if (!adv_ec_data->dym_tbl)
> +		return -ENOMEM;

What does a dynamic table do?

> +	ret = adv_get_dynamic_tab(adv_ec_data);

return adv_get_dynamic_tab();

> +	return ret;
> +}
> +
> +static int adv_ec_parse_prop(struct adv_ec_platform_data *adv_ec_data)
> +{
> +	int i, ret;
> +	u32 nb, sub_dev[ADVEC_SUBDEV_MAX];
> +
> +	ret = device_property_read_u32(adv_ec_data->dev, "advantech,sub-dev-nb", &nb);

Indexing devices is generally not a good strategy.

> +	if (ret < 0) {
> +		dev_err(adv_ec_data->dev, "get sub-dev-nb failed! (%d)\n", ret);
> +		return ret;
> +	}
> +	adv_ec_data->sub_dev_nb = nb;

'nb' is not a good choice for a variable name.

> +	ret = device_property_read_u32_array(adv_ec_data->dev, "advantech,sub-dev",
> +					     sub_dev, nb);
> +	if (ret < 0) {

Is '> 0' valid?

> +		dev_err(adv_ec_data->dev, "get sub-dev failed! (%d)\n", ret);

Please use proper error messages.

"Failed to read 'advantech,sub-dev' property"

> +		return ret;
> +	}
> +
> +	for (i = 0; i < nb; i++) {
> +		switch (sub_dev[i]) {
> +		case ADVEC_SUBDEV_BRIGHTNESS:
> +		case ADVEC_SUBDEV_EEPROM:
> +		case ADVEC_SUBDEV_GPIO:
> +		case ADVEC_SUBDEV_HWMON:
> +		case ADVEC_SUBDEV_LED:
> +		case ADVEC_SUBDEV_WDT:
> +			adv_ec_data->sub_dev_mask |= BIT(sub_dev[i]);
> +			break;
> +		default:
> +			dev_err(adv_ec_data->dev, "invalid prop value(%d)!\n",
> +				sub_dev[i]);
> +		}
> +	}
> +	dev_info(adv_ec_data->dev, "sub-dev mask = 0x%x\n", adv_ec_data->sub_dev_mask);
> +
> +	return 0;
> +}
> +
> +static int adv_ec_probe(struct platform_device *pdev)
> +{
> +	int ret, i;
> +	struct device *dev = &pdev->dev;
> +	struct adv_ec_platform_data *adv_ec_data;

This is not platform data.  This is driver data.

  struct adv_ec_ddata *ddata;

> +	adv_ec_data = devm_kzalloc(dev, sizeof(struct adv_ec_platform_data), GFP_KERNEL);

sizeof(*adv_ec_data)

> +	if (!adv_ec_data)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(dev, adv_ec_data);
> +	adv_ec_data->dev = dev;
> +
> +	ret = adv_ec_init_ec_data(adv_ec_data);
> +	if (ret)
> +		goto err_init_data;
> +
> +	ret = adv_ec_parse_prop(adv_ec_data);
> +	if (ret)
> +		goto err_prop;
> +
> +	/* check whether this EC has the following subdevices. */
> +	for (i = 0; i < ARRAY_SIZE(adv_ec_sub_cells); i++) {
> +		if (adv_ec_data->sub_dev_mask & BIT(i)) {
> +			ret = mfd_add_hotplug_devices(dev, &adv_ec_sub_cells[i], 1);

Why have you chosen to use hotplug here?

> +			dev_info(adv_ec_data->dev, "mfd_add_hotplug_devices[%d] %s\n", i,
> +				adv_ec_sub_cells[i].name);
> +			if (ret)
> +				dev_err(dev, "failed to add %s subdevice: %d\n",
> +					adv_ec_sub_cells[i].name, ret);
> +		}
> +	}

This is a mess!

Where are you pulling these devices from?

> +	dev_info(adv_ec_data->dev, "Advantech EC probe done");
> +
> +	return 0;
> +
> +err_prop:
> +	dev_err(dev, "failed to probe\n");
> +
> +err_init_data:
> +	mutex_destroy(&adv_ec_data->lock);
> +
> +	dev_err(dev, "failed to init data\n");

You don't need 2 error messages in the error path.

> +	return ret;
> +}
> +
> +static int adv_ec_remove(struct platform_device *pdev)
> +{
> +	struct adv_ec_platform_data *adv_ec_data;
> +
> +	adv_ec_data = dev_get_drvdata(&pdev->dev);
> +
> +	mutex_destroy(&adv_ec_data->lock);
> +
> +	mfd_remove_devices(&pdev->dev);

If you don't use the hotplug variant, you can use devm_* and omit
this.

> +	return 0;
> +}
> +
> +static const struct of_device_id adv_ec_of_match[] __maybe_unused = {
> +	{
> +		.compatible = "advantech,ahc1ec0",
> +	},

Put this on one line please.

> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, adv_ec_of_match);
> +
> +static const struct acpi_device_id adv_ec_acpi_match[] __maybe_unused = {
> +	{"AHC1EC0", 0},

Spaces inside the '{' and '}' please.

> +	{},

',' here, but not on the one above.  Please be consistent.

> +};
> +MODULE_DEVICE_TABLE(acpi, adv_ec_acpi_match);
> +
> +static struct platform_driver adv_ec_driver = {
> +	.driver = {
> +		.name = DRV_NAME,
> +		.of_match_table = of_match_ptr(adv_ec_of_match),
> +		.acpi_match_table = ACPI_PTR(adv_ec_acpi_match),
> +	},
> +	.probe = adv_ec_probe,
> +	.remove = adv_ec_remove,
> +};
> +module_platform_driver(adv_ec_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:" DRV_NAME);
> +MODULE_DESCRIPTION("Advantech Embedded Controller core driver.");

Name as in the header.

> +MODULE_AUTHOR("Campion Kang <campion.kang@advantech.com.tw>");
> +MODULE_AUTHOR("Jianfeng Dai <jianfeng.dai@advantech.com.cn>");
> +MODULE_VERSION("1.0");

Is this used?

> diff --git a/include/linux/mfd/ahc1ec0.h b/include/linux/mfd/ahc1ec0.h
> new file mode 100644
> index 000000000000..1b01e10c1fef
> --- /dev/null
> +++ b/include/linux/mfd/ahc1ec0.h
> @@ -0,0 +1,276 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __LINUX_MFD_AHC1EC0_H
> +#define __LINUX_MFD_AHC1EC0_H
> +
> +#include <linux/device.h>
> +
> +#define EC_COMMAND_PORT             0x29A /* EC I/O command port */
> +#define EC_STATUS_PORT              0x299 /* EC I/O data port */
> +
> +#define EC_RETRY_UDELAY              200 /* EC command retry delay in microseconds */
> +#define EC_MAX_TIMEOUT_COUNT        5000 /* EC command max retry count */
> +#define EC_COMMAND_BIT_OBF          0x01 /* Bit 0 is for OBF ready (Output buffer full) */
> +#define EC_COMMAND_BIT_IBF          0x02 /* Bit 1 is for IBF ready (Input buffer full) */
> +
> +/* Analog to digital converter command */
> +#define EC_AD_INDEX_WRITE   0x15 /* Write ADC port number into index */
> +#define EC_AD_LSB_READ      0x16 /* Read ADC LSB value from ADC port */
> +#define EC_AD_MSB_READ      0x1F /* Read ADC MSB value from ADC port */
> +
> +/* Voltage device ID */
> +#define EC_DID_SMBOEM0      0x28 /* SMBUS/I2C. Smbus channel 0 */
> +#define EC_DID_CMOSBAT      0x50 /* CMOS coin battery voltage */
> +#define EC_DID_CMOSBAT_X2   0x51 /* CMOS coin battery voltage*2 */
> +#define EC_DID_CMOSBAT_X10  0x52 /* CMOS coin battery voltage*10 */
> +#define EC_DID_5VS0         0x56 /* 5VS0 voltage */
> +#define EC_DID_5VS0_X2      0x57 /* 5VS0 voltage*2 */
> +#define EC_DID_5VS0_X10     0x58 /* 5VS0 voltage*10 */
> +#define EC_DID_5VS5         0x59 /* 5VS5 voltage */
> +#define EC_DID_5VS5_X2      0x5A /* 5VS5 voltage*2 */
> +#define EC_DID_5VS5_X10     0x5B /* 5VS5 voltage*10 */
> +#define EC_DID_12VS0        0x62 /* 12VS0 voltage */
> +#define EC_DID_12VS0_X2     0x63 /* 12VS0 voltage*2 */
> +#define EC_DID_12VS0_X10    0x64 /* 12VS0 voltage*10 */
> +#define EC_DID_VCOREA       0x65 /* CPU A core voltage */
> +#define EC_DID_VCOREA_X2    0x66 /* CPU A core voltage*2 */
> +#define EC_DID_VCOREA_X10   0x67 /* CPU A core voltage*10 */
> +#define EC_DID_VCOREB       0x68 /* CPU B core voltage */
> +#define EC_DID_VCOREB_X2    0x69 /* CPU B core voltage*2 */
> +#define EC_DID_VCOREB_X10   0x6A /* CPU B core voltage*10 */
> +#define EC_DID_DC           0x6B /* ADC. onboard voltage */
> +#define EC_DID_DC_X2        0x6C /* ADC. onboard voltage*2 */
> +#define EC_DID_DC_X10       0x6D /* ADC. onboard voltage*10 */
> +
> +/* Current device ID */
> +#define EC_DID_CURRENT              0x74
> +
> +/* ACPI commands */
> +#define EC_ACPI_RAM_READ            0x80
> +#define EC_ACPI_RAM_WRITE           0x81
> +
> +/*
> + *  Dynamic control table commands
> + *  The table includes HW pin number, Device ID, and Pin polarity
> + */
> +#define EC_TBL_WRITE_ITEM           0x20
> +#define EC_TBL_GET_PIN              0x21
> +#define EC_TBL_GET_DEVID            0x22
> +#define EC_MAX_TBL_NUM              32
> +
> +/* LED Device ID table */
> +#define EC_DID_LED_RUN              0xE1
> +#define EC_DID_LED_ERR              0xE2
> +#define EC_DID_LED_SYS_RECOVERY     0xE3
> +#define EC_DID_LED_D105_G           0xE4
> +#define EC_DID_LED_D106_G           0xE5
> +#define EC_DID_LED_D107_G           0xE6
> +
> +/* LED control HW RAM address 0xA0-0xAF */
> +#define EC_HWRAM_LED_BASE_ADDR      0xA0
> +#define EC_HWRAM_LED_PIN(N)         (EC_HWRAM_LED_BASE_ADDR + (4 * (N))) // N:0-3
> +#define EC_HWRAM_LED_CTRL_HIBYTE(N) (EC_HWRAM_LED_BASE_ADDR + (4 * (N)) + 1)
> +#define EC_HWRAM_LED_CTRL_LOBYTE(N) (EC_HWRAM_LED_BASE_ADDR + (4 * (N)) + 2)
> +#define EC_HWRAM_LED_DEVICE_ID(N)   (EC_HWRAM_LED_BASE_ADDR + (4 * (N)) + 3)
> +
> +/* LED control bit */
> +#define LED_CTRL_ENABLE_BIT()           BIT(4)
> +#define LED_CTRL_INTCTL_BIT()           BIT(5)
> +#define LED_CTRL_LEDBIT_MASK            (0x03FF << 6)
> +#define LED_CTRL_POLARITY_MASK          (0x000F << 0)
> +#define LED_CTRL_INTCTL_EXTERNAL        0
> +#define LED_CTRL_INTCTL_INTERNAL        1
> +
> +#define LED_DISABLE  0x0
> +#define LED_ON       0x1
> +#define LED_FAST     0x3
> +#define LED_NORMAL   0x5
> +#define LED_SLOW     0x7
> +#define LED_MANUAL   0xF
> +
> +#define LED_CTRL_LEDBIT_DISABLE	0x0000
> +#define LED_CTRL_LEDBIT_ON		0x03FF
> +#define LED_CTRL_LEDBIT_FAST	0x02AA
> +#define LED_CTRL_LEDBIT_NORMAL	0x0333
> +#define LED_CTRL_LEDBIT_SLOW	0x03E0
> +
> +/* Get the device name */
> +#define AMI_ADVANTECH_BOARD_ID_LENGTH	32
> +
> +/*
> + * Advantech Embedded Controller watchdog commands
> + * EC can send multi-stage watchdog event. System can setup watchdog event
> + * independently to make up event sequence.
> + */
> +#define EC_COMMANS_PORT_IBF_MASK	0x02
> +#define EC_RESET_EVENT				0x04
> +#define	EC_WDT_START				0x28
> +#define	EC_WDT_STOP					0x29
> +#define	EC_WDT_RESET				0x2A
> +#define	EC_WDT_BOOTTMEWDT_STOP		0x2B
> +
> +#define EC_HW_RAM					0x89
> +
> +#define EC_EVENT_FLAG				0x57
> +#define EC_ENABLE_DELAY_H			0x58
> +#define EC_ENABLE_DELAY_L			0x59
> +#define EC_POWER_BTN_TIME_H			0x5A
> +#define EC_POWER_BTN_TIME_L			0x5B
> +#define EC_RESET_DELAY_TIME_H		0x5E
> +#define EC_RESET_DELAY_TIME_L		0x5F
> +#define EC_PIN_DELAY_TIME_H			0x60
> +#define EC_PIN_DELAY_TIME_L			0x61
> +#define EC_SCI_DELAY_TIME_H			0x62
> +#define EC_SCI_DELAY_TIME_L			0x63
> +
> +/* EC ACPI commands */
> +#define EC_ACPI_DATA_READ			0x80
> +#define EC_ACPI_DATA_WRITE			0x81
> +
> +/* Brightness ACPI Addr */
> +#define BRIGHTNESS_ACPI_ADDR		0x50
> +
> +/* EC HW RAM commands */
> +#define EC_HW_EXTEND_RAM_READ		0x86
> +#define EC_HW_EXTEND_RAM_WRITE		0x87
> +#define	EC_HW_RAM_READ				0x88
> +#define EC_HW_RAM_WRITE				0x89
> +
> +/* EC Smbus commands */
> +#define EC_SMBUS_CHANNEL_SET		0x8A	 /* Set selector number (SMBUS channel) */
> +#define EC_SMBUS_ENABLE_I2C			0x8C	 /* Enable channel I2C */
> +#define EC_SMBUS_DISABLE_I2C		0x8D	 /* Disable channel I2C */
> +
> +/* Smbus transmit protocol */
> +#define EC_SMBUS_PROTOCOL			0x00
> +
> +/* SMBUS status */
> +#define EC_SMBUS_STATUS				0x01
> +
> +/* SMBUS device slave address (bit0 must be 0) */
> +#define EC_SMBUS_SLV_ADDR			0x02
> +
> +/* SMBUS device command */
> +#define EC_SMBUS_CMD				0x03
> +
> +/* 0x04-0x24 Data In read process, return data are stored in this address */
> +#define EC_SMBUS_DATA				0x04
> +
> +#define EC_SMBUS_DAT_OFFSET(n)	(EC_SMBUS_DATA + (n))
> +
> +/* SMBUS channel selector (0-4) */
> +#define EC_SMBUS_CHANNEL			0x2B
> +
> +/* EC SMBUS transmit Protocol code */
> +#define SMBUS_QUICK_WRITE			0x02 /* Write Quick Command */
> +#define SMBUS_QUICK_READ			0x03 /* Read Quick Command */
> +#define SMBUS_BYTE_SEND				0x04 /* Send Byte */
> +#define SMBUS_BYTE_RECEIVE			0x05 /* Receive Byte */
> +#define SMBUS_BYTE_WRITE			0x06 /* Write Byte */
> +#define SMBUS_BYTE_READ				0x07 /* Read Byte */
> +#define SMBUS_WORD_WRITE			0x08 /* Write Word */
> +#define SMBUS_WORD_READ				0x09 /* Read Word */
> +#define SMBUS_BLOCK_WRITE			0x0A /* Write Block */
> +#define SMBUS_BLOCK_READ			0x0B /* Read Block */
> +#define SMBUS_PROC_CALL				0x0C /* Process Call */
> +#define SMBUS_BLOCK_PROC_CALL		0x0D /* Write Block-Read Block Process Call */
> +#define SMBUS_I2C_READ_WRITE		0x0E /* I2C block Read-Write */
> +#define SMBUS_I2C_WRITE_READ		0x0F /* I2C block Write-Read */
> +
> +/* GPIO control commands */
> +#define EC_GPIO_INDEX_WRITE			0x10
> +#define EC_GPIO_STATUS_READ			0x11
> +#define EC_GPIO_STATUS_WRITE		0x12
> +#define EC_GPIO_DIR_READ			0x1D
> +#define EC_GPIO_DIR_WRITE			0x1E
> +
> +/* One Key Recovery commands */
> +#define EC_ONE_KEY_FLAG				0x9C
> +
> +/* ASG OEM commands */
> +#define EC_ASG_OEM					0xEA
> +#define EC_ASG_OEM_READ				0x00
> +#define EC_ASG_OEM_WRITE			0x01
> +#define EC_OEM_POWER_STATUS_VIN1	0X10
> +#define EC_OEM_POWER_STATUS_VIN2	0X11
> +#define EC_OEM_POWER_STATUS_BAT1	0X12
> +#define EC_OEM_POWER_STATUS_BAT2	0X13
> +
> +/* GPIO DEVICE ID */
> +#define EC_DID_ALTGPIO_0			0x10    /* 0x10 AltGpio0 User define gpio */
> +#define EC_DID_ALTGPIO_1			0x11    /* 0x11 AltGpio1 User define gpio */
> +#define EC_DID_ALTGPIO_2			0x12    /* 0x12 AltGpio2 User define gpio */
> +#define EC_DID_ALTGPIO_3			0x13    /* 0x13 AltGpio3 User define gpio */
> +#define EC_DID_ALTGPIO_4			0x14    /* 0x14 AltGpio4 User define gpio */
> +#define EC_DID_ALTGPIO_5			0x15    /* 0x15 AltGpio5 User define gpio */
> +#define EC_DID_ALTGPIO_6			0x16    /* 0x16 AltGpio6 User define gpio */
> +#define EC_DID_ALTGPIO_7			0x17    /* 0x17 AltGpio7 User define gpio */
> +
> +/* Lmsensor Chip Register */
> +#define NSLM96163_CHANNEL			0x02
> +
> +/* NS_LM96163 address 0x98 */
> +#define NSLM96163_ADDR				0x98
> +
> +/* LM96163 index(0x00) Local Temperature (Signed MSB) */
> +#define NSLM96163_LOC_TEMP			0x00
> +
> +/* HWMON registers */
> +#define INA266_REG_VOLTAGE          0x02    /* 1.25mV */
> +#define INA266_REG_POWER            0x03    /* 25mW */
> +#define INA266_REG_CURRENT          0x04    /* 1mA */
> +
> +struct ec_hw_pin_table {
> +	unsigned int vbat[2];
> +	unsigned int v5[2];
> +	unsigned int v12[2];
> +	unsigned int vcore[2];
> +	unsigned int vdc[2];
> +	unsigned int ec_current[2];
> +	unsigned int power[2];
> +};
> +
> +struct ec_dynamic_table {
> +	unsigned char device_id;
> +	unsigned char hw_pin_num;
> +};
> +
> +struct ec_smbuso_em0 {
> +	unsigned char hw_pin_num;
> +};
> +
> +struct pled_hw_pin_tbl {
> +	unsigned int pled[6];
> +};
> +
> +struct adv_ec_platform_data {
> +	char *bios_product_name;

Where is this used?

> +	int sub_dev_nb;
> +	u32 sub_dev_mask;
> +	struct mutex lock;
> +	struct device *dev;
> +	struct class *adv_ec_class;
> +
> +	struct ec_dynamic_table *dym_tbl;
> +};

Check whether all of these really need to be in device data i.e. check
that they are all used in sub-devices.

> +int read_ad_value(struct adv_ec_platform_data *adv_ec_data, unsigned char hwpin,
> +			unsigned char multi);
> +int read_acpi_value(struct adv_ec_platform_data *adv_ec_data, unsigned char addr,
> +			unsigned char *pvalue);
> +int write_acpi_value(struct adv_ec_platform_data *adv_ec_data, unsigned char addr,
> +			unsigned char value);
> +int read_hw_ram(struct adv_ec_platform_data *adv_ec_data, unsigned char addr,
> +			unsigned char *data);
> +int write_hw_ram(struct adv_ec_platform_data *adv_ec_data, unsigned char addr,
> +			unsigned char data);
> +int write_hwram_command(struct adv_ec_platform_data *adv_ec_data, unsigned char data);
> +int read_gpio_status(struct adv_ec_platform_data *adv_ec_data, unsigned char PinNumber,
> +			unsigned char *pvalue);
> +int write_gpio_status(struct adv_ec_platform_data *adv_ec_data, unsigned char PinNumber,
> +			unsigned char value);
> +int read_gpio_dir(struct adv_ec_platform_data *adv_ec_data, unsigned char PinNumber,
> +			unsigned char *pvalue);
> +int write_gpio_dir(struct adv_ec_platform_data *adv_ec_data, unsigned char PinNumber,
> +			unsigned char value);
> +
> +#endif /* __LINUX_MFD_AHC1EC0_H */

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