On Fri, 19 Mar 2021, Campion Kang wrote:

Please check [Campion] text in below as my reply.

This is a mess.  Please setup your mailer to quote correctly.

Sorry, due to the mail was rejected by vger.kernel.org as SPAM before 
so I reply the mail late and had some test email before.

-----------------------------------------------------------------------------------------
Date:   Tue, 9 Mar 2021 16:07:55 +0000
From:   Lee Jones <redacted>

[...]

quoted

+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?
[Campion] cannot arbitrary there, due to it is a index to identify its number of sub device. 

I'm asking what this is dictated by.

Are these ID/index values written into H/W?

[...]

quoted

+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.

[Campion] this is a common function for upper HWMON and brightness control used. 
          So far this API only used by HWMON, but then it will be used by 
          brightness in next stage. So i put this API here, OK?

I think it belongs in drivers/platform.  Take a look at some of the
other Embedded Controller code that lives there.

quoted

+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)
+{
+}

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

[Campion] Due to it has a flow need to cowork with EC chip and firmware, it cannot be interrupt
          by other functions. So it needs to keep in here. 

Please provide more details.

quoted

+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.

[Campion] This is a simple function to get module name from BIOS DMI table, it is not need to 
          access EC chip. But it can get once and other drivers can get this information,
          donot call DMI every time. Can it keep in here?

I thought this driver was for the EC chip.

quoted

+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?

[Campion] Just make sure they have empty pointer, but I will remove it. 

There's no need to do that if they are being allocated below.

quoted

+	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?

[Campion] Just make sure the memory is null all

devm_kzalloc() does that for you - that's what the 'z' means.

quoted

+	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?

[Campion] The dynamic table is reterived from EC firmware according to different platform HW device.
          it will based on dynamic table information to get HW pin definition based on its function.
          The pin value will retrive to calcute the value, for example, voltage value, vcore value. 
          

quoted

+	ret = adv_get_dynamic_tab(adv_ec_data);

return adv_get_dynamic_tab();

[Campion] OK

quoted

+	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.

---------------------------------------------------------------------------
[Campion] Yes, I will remove it, just use the following that defined in ahc1ec0.yaml. 
          I ever feedback related mail for "https://lore.kernel.org/linux-watchdog/20210118123749.4769-6-campion.kang@advantech.com.tw/t/#m5126adbc2453e3ab3e6bda717c257fab364b9f45 (local)". 
          But vger.kernel.org returned the mail to mail as spam mail. 
          I will modity it as the following, is it OK?
          examples:
            - |
          #include <dt-bindings/mfd/ahc1ec0-dt.h>
          ahc1ec0 {
                  compatible = "advantech,ahc1ec0";

                  advantech,hwmon-profile = <AHC1EC0_HWMON_PRO_UNO2271G>;
                  advantech,watchdog = true;

Shouldn't the watchdog be it's own sub-node?

Is that functionality not probably at all?

Surely it has it's own register set?

[...]

quoted

+	/* 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?

[Campion] There is a information in BIOS ACPI table according to different device to decide 
          which function drivers need to be probe. May be a device has HWMON, it will dynamic 
          to load HWMON driver, but other device may not.

The only thing hotplug does is hard-code the platform ID.

It's more of a win if you can omit the mfd_remove_devices() call.

quoted

+			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?  

[Campion] decide which drivers need to mount from BIOS ACPI table. This drive would built in Linux Kernel.
          I am not sure what's your meaning, can you describe more? Thank you

I really don't like the sub_dev_mask idea.

Are the ACPI tables available?

[...]

quoted

+struct adv_ec_platform_data {
+	char *bios_product_name;

Where is this used?

[Campion] Get the module name once and upper application can get it by EC driver.

From DMI?  What do you use it for?  Debug prints or something else?

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