On Wed, 24 Mar 2021, Campion.Kang wrote:

Thanks a lot, please see below descriptions.

quoted

-----Original Message-----
From: Lee Jones <redacted>
Sent: Friday, March 19, 2021 10:15 PM
To: Campion.Kang <redacted>
Cc: chia-lin.kao@canonical.com; devicetree@vger.kernel.org;
jdelvare@suse.com; linux-hwmon@vger.kernel.org;
linux-kernel@vger.kernel.org; linux-watchdog@vger.kernel.org;
linux@roeck-us.net; robh+dt@kernel.org; wim@linux-watchdog.org;
Campion.Kang@gmail.com
Subject: Re: [PATCH v6 4/6] mfd: ahc1ec0: Add support for Advantech
embedded controller

On Fri, 19 Mar 2021, Campion Kang wrote:

quoted

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

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

quoted

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

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?

These index written in BIOS ACPI table. 

Please consider renaming to make this clear.

  ADVEC_ACPI_ID_{DEVICE}

Add a comment to express the importance of the order too.

[...]

quoted

quoted

quoted

+int write_gpio_dir(struct adv_ec_platform_data *adv_ec_data, unsigned

char PinNumber,

quoted

quoted

+		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

quoted

          by other functions. So it needs to keep in here.

Please provide more details.

These gpio functions are common used for gpio to adjust the gpio direction and access its status. It has a complete lower process with EC that cannot be interrupted likes others. The flow is:
0.mutex lock to get the EC access
1.it needs wait IBF (Input Buffer Full) to be clear and then can send the command
2.Send read command to EC command port
3.wait IBF clear that means command is got by EC
4.send read address to EC data port
5.wait OBF (Output Buffer Full) data ready
6.get data from EC data port
7.mutex unlock
So it cannot be interrupted as other EC lower access due to they use the same mutex to lock the flow. 

That's fine.  You can share a lock with the GPIO driver.

quoted

quoted

          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>;

quoted

                  advantech,watchdog = true;

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

it doesnot need so far. 

IMO, it's more clear than having a property.

[...]

[Campion.Kang] So far, some registers or settings are have default value. They can be adjusted by runtime. 
Is this ahc1ec0.yaml OK?

Rob has the final say on that.

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