Re: [PATCH v3] platform/x86: portwell-ec: Add GPIO and WDT driver for Portwell EC

[jesse huang]

Hi Ilpo, Guenter,

Thanks for your reviews and detailed comments. Please see my replies inline below.


On 10/04/2025 8:25 pm, Guenter Roeck wrote:
> On 4/10/25 05:07, Ilpo Jarvinen wrote:
>> On Thu, 10 Apr 2025, Yen-Chi Huang wrote:
>>
>> +#define PORTWELL_EC_IOSPACE 0xe300
>> +#define PORTWELL_EC_IOSPACE_LEN 0x100
> 
> SZ_256 + add #include for it.

Will fix in v4 and include <linux/sizes.h>.

>> +#define PORTWELL_WDT_EC_CONFIG_ADDR 0x06
>> +#define PORTWELL_WDT_CONFIG_ENABLE 0x1
>> +#define PORTWELL_WDT_CONFIG_DISABLE 0x0
> 
> Align values.

Will align all definitions in v4

>> +#define PORTWELL_WDT_EC_MAX_COUNT_SECOND 15300 //255*60secs
> 
> Move the formula from the comment to the define itself. While doing so, 
> you need to add () around it and add spaces around *.

Will use `(255 * 60)` in v4.

>> +MODULE_PARM_DESC(force, "Force loading ec driver without checking DMI boardname");
> 
> EC
> 

Will capitalize "EC" in v4.

>> +/* GPIO functions*/
> 
> Missing space. Please check all your comments as the one above seems to 
> have the same lack of space at the end.

Will review all comments and ensure consistent spacing.

>> +	pwec_write(PORTWELL_GPIO_VAL_REG, tmp);
> 
> Add empty line here.

Will add the missing empty line in v4.

>> +static int pwec_wdt_trigger(struct watchdog_device *wdd)
[...]
> Is this write until timeout matches the one written typical thing for 
> watchdog drivers, or is there something specific to this HW you should 
> comment + note in the changelog so it is recorded for future readers of 
> this code?

> No, this is absolutely not typical, and it has nothing to do with watchdog
> drivers in the first place. If the code was in drivers/watchdog/ I'd
> request a detailed comment explaining why it is needed.
> 
> Guenter

I will remove the retry loop. It was originally a workaround for an old EC firmware bug and is no longer needed.

> I'd add empty line here.
> Unnecessary else.

Will remove in v4 along with the retry logic.

>> +static int pwec_wdt_set_timeout(struct watchdog_device *wdd, unsigned int timeout)
> Add empty line here.
> Add empty line hre

Will add the missing empty lines.

>> +static unsigned int pwec_wdt_get_timeleft(struct watchdog_device *wdd)
>> +{
>> +	u8 min, sec;
>> +
>> +	min = pwec_read(PORTWELL_WDT_EC_COUNT_MIN_ADDR);
>> +	sec = pwec_read(PORTWELL_WDT_EC_COUNT_SEC_ADDR);
> 
> Does the HW "lock" sec value in place after min is read or do you need to 
> consider the possibility of these values getting updated while you're 
> reading them and the driver reading sec after it has wrapped?

Will fix by double-read and comparison in v4. The EC firmware team confirmed that the registers are not latched.

> Add an empty line here.

Will fix in v4.

>> +static struct watchdog_device ec_wdt_dev = {
>> +	.info = &(struct watchdog_info){
>> +	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
>> +	.identity = "Portwell EC Watchdog",
> 
> Please indent the inner struct correctly.

Will fix indentation in v4.

>> +	return (strcmp(PORTWELL_EC_FW_VENDOR_NAME, buf) == 0) ? 0 : -ENODEV;
> 
> return !strcmp() ? 0 : -ENODEV;

Will simplify the return expression.

>> +static int pwec_probe(struct platform_device *pdev)
>> +{
>> +	int ret;
>> +
>> +	if (!devm_request_region(&pdev->dev, PORTWELL_EC_IOSPACE,
>> +				PORTWELL_EC_IOSPACE_LEN, dev_name(&pdev->dev))) {
>> +		pr_err("I/O region 0xE300-0xE3FF busy\n");
> 
> Use dev_err() instead of pr_err().
> 
> I'd use the defines while printing the region's address.
> 

>> +		pr_err("failed to register Portwell EC Watchdog\n");
> 
> Watchdog -> watchdog ?

Will lowercase "Watchdog" in v4

>> +static struct platform_driver pwec_driver = {
>> +	.driver = {
>> +		.name = "portwell-ec",
>> +	},
>> +	.probe = pwec_probe
> 
> Add comma. In general, the comma is to be left out only from a 
> terminator entry that is used by some types of arrays.

Will add the missing comma in v4.

>> +	if (!force) {
>> +		if (!dmi_check_system(pwec_dmi_table)) {
>> +			pr_info("unsupported platform\n");
> 
> This will be unnecessary noise for most systems.
> 
>> +			return -ENODEV;
>> +		}
>> +	}
> 
> I think logically you should do it in a slightly different order:
> 
> 	if (!dmi_check_system(...)) {
> 		if (!force)
> 			return -ENODEV;
> 		pr_warn("...\n");
> 	}

Will rework the logic and remove the dummy message.

>> +	pwec_dev = platform_device_register_simple("portwell-ec", -1, NULL, 0);
> 
> If this fails, you need to unroll the other register.

Will add `platform_driver_unregister()` on failure in v4.

Thanks again for your feedback. I will address all comments in v4.

Best regards,  
Yen-Chi Huang