Re: [PATCH 2/2] platform: arm64: Add HP OmniBook 5 16 embedded controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] platform: arm64: hpec_command() skips reading HPEC_REPLY when
  reply is NULL
- [Low] platform: arm64: hp-omnibook-5-ec: direct hwmon registration in
  hpec_probe()
--

commit 9e10721f812ce769072addab8eb2bcaf582e4091
Author: Rahul Pon <theflyingrahul@gmail.com>

platform: arm64: Add HP OmniBook 5 16 embedded controller driver

This commit adds an I2C driver for the HP OmniBook 5 16 embedded controller,
exposing fan and thermistor monitoring via hwmon, mute LEDs, keyboard
backlight attributes, and hotkeys.

> diff --git a/drivers/platform/arm64/hp-omnibook-5-ec.c b/drivers/platform/arm64/hp-omnibook-5-ec.c
> new file mode 100644
> index 0000000000000..bfe20e12aafdf
> --- /dev/null
> +++ b/drivers/platform/arm64/hp-omnibook-5-ec.c
> @@ -0,0 +1,417 @@

[ ... ]

> +static int hpec_command(struct hpec *ec, u8 cmd, u8 sub, u8 reply[HPEC_REPLY_LEN])
> +{
> +	u8 request[] = { cmd, sub };
> +	int ret;
> +
> +	guard(mutex)(&ec->lock);
> +
> +	ret = hpec_wait(ec, HPEC_STATUS_IDLE);
> +	if (ret)
> +		return ret;
> +
> +	ret = i2c_smbus_write_i2c_block_data(ec->client, HPEC_REQUEST,
> +					     sizeof(request), request);
> +	if (ret)
> +		return ret;
> +
> +	ret = hpec_wait(ec, HPEC_STATUS_DONE);
> +	if (ret || !reply)
> +		return ret;
> +
> +	ret = i2c_smbus_read_i2c_block_data(ec->client, HPEC_REPLY,
> +					    HPEC_REPLY_LEN, reply);
> +	if (ret < 0)
> +		return ret;
> +
> +	return ret == HPEC_REPLY_LEN ? 0 : -EIO;
> +}

[Severity: Medium]
Does the EC require reading HPEC_REPLY to return HPEC_STATUS from
HPEC_STATUS_DONE to HPEC_STATUS_IDLE?

The file header describes a four-step mailbox protocol ending with reading
the reply, but hpec_command() returns right after HPEC_STATUS reaches
HPEC_STATUS_DONE when reply is NULL (as in hpec_led_set() and
kbd_backlight_timeout_store()).  If the EC only clears HPEC_STATUS_DONE once
HPEC_REPLY is read, the next hpec_command() call will time out in
hpec_wait(ec, HPEC_STATUS_IDLE).

[ ... ]

> +static int hpec_probe(struct i2c_client *client)
> +{

[ ... ]

> +	hwmon = devm_hwmon_device_register_with_info(dev, "hp_omnibook_ec", ec,
> +						     &hpec_hwmon_chip, NULL);
> +	if (IS_ERR(hwmon))
> +		return PTR_ERR(hwmon);

[Severity: Low]
New hardware monitoring drivers belong in drivers/hwmon/ rather than
registering hwmon devices directly from outside that directory.  Since this
EC is a multi-function device, should the hwmon functionality be split out
into an auxiliary device driver under drivers/hwmon/?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109104914.1534-1-theflyingrahul@gmail.com?part=2