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

[Yen-Chi Huang <jesse.huang@portwell.com.tw>]

Hi Ilpo,

Resend this reply due to encoding issue in the previous message.

Thank you for the review.

On Thu, 3 Apr 2025, Ilpo Jarvinen wrote:
> On Thu, 3 Apr 2025, Yen-Chi Huang wrote:
>
> Please add watchdog drivers people/lists from MAINTAINERS file into the 
> next submission.

Will add GPIO and watchdog driver maintainers/lists in PATCH v2.

>> +static u8 pwec_read(u8 address)
>> +{
>> +	return inb(PORTWELL_EC_IOSPACE + address);
>
> IIRC, CONFIG_HAS_IOPORT is these days required for iob/outb() so you 
> should add depends on HAS_IOPORT into Kconfig.

Fixed by adding `depends on HAS_IOPORT` to Kconfig in PATCH v2.

>> +static int pwec_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
>> +{
>> +	return (pwec_read(PORTWELL_GPIO_DIR_REG) & (1 << offset))
>> +			? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;
>
> Please move ? to the preceeding line
>
> I'd add a local variable for the read result to make this more readable.
Fixed by rewriting this using a local variable and `if...else` in PATCH v2.

>> +static int pwec_gpio_direction_input(struct gpio_chip *gc, unsigned int offset)
>> +{
>> +	/* Changing direction causes issues on some boards, so it's disabled for now. */
>> +	return -EOPNOTSUPP;
>> +}
>> +
>> +static int pwec_gpio_direction_output(struct gpio_chip *gc, unsigned int offset, int value)
>> +{
>> +	/* Changing direction causes issues on some boards, so it's disabled for now. */
>
> Perhaps just one comment above both functions would suffice. The functions 
> are right after another so it seems overkill to have the same comment for 
> both.

Fixed by combining the comments into one above both functions in PATCH v2.

>> +static int pwec_wdt_start(struct watchdog_device *wdd)
>> +{
>> +	int retry = 10;
>> +	u8 timeout;
>> +
>> +	do {
>> +		pwec_write(PORTWELL_WDT_EC_COUNT_SEC_ADDR, wdd->timeout);
>> +		pwec_write(PORTWELL_WDT_EC_CONFIG_ADDR, 0x01); // WDTCFG[1:0]=01
>
> Please use named defines and FIELD_PREP() instead of comments.

Fixed by defining `PORTWELL_WDT_CONFIG_ENABLE` and `PORTWELL_WDT_CONFIG_DISABLE`,  
and replacing the hardcoded value with named constants in PATCH v2.  
>> +	do {
>> +		pwec_write(PORTWELL_WDT_EC_COUNT_SEC_ADDR, wdd->timeout);
>> +		pwec_write(PORTWELL_WDT_EC_CONFIG_ADDR, 0x01); // WDTCFG[1:0]=01
>> +		timeout = pwec_read(PORTWELL_WDT_EC_COUNT_SEC_ADDR);
>> +		retry--;
>> +	} while (timeout != wdd->timeout && retry > 0);
>> +	return (retry > 0) ? 0 : -EIO;
>
> Duplicated code.

Fixed by reusing `pwec_wdt_trigger()` from `pwec_wdt_start()` to eliminate duplication in PATCH v2.

>> +static int __init pwec_init(void)
>> +{
>> +	int result;
>> +
>> +	result = pwec_firmware_vendor_check();
>
> So this goes immediately to poke some io ports? On any x86 machine?
> The cases should be narrowed down first with dmi matching.

Will add `dmi_check_system()` to limit EC access to supported platforms in PATCH v2.

The following style and formatting issues were also pointed out and have been fixed:
- Removed extra blank line in the file header comment block
- Sorted header includes
- Fixed missing blank lines before section headers (3 instances)
- Fixed 4 spacing issues around '+' operators
- Removed 7 `pr_info()` calls from success paths

Appreciate your detailed review. it was very helpful.

Best Regards,
Yen-Chi Huang