Re: [PATCH v2] hwmon: (oxp-sensors) Add AYANEO AIR and AIR Pro

From: Guenter Roeck
Date: Wed Dec 28 2022 - 10:55:52 EST


On Wed, Dec 14, 2022 at 07:47:13AM -0800, Derek J. Clark wrote:
> Add support for the AYANEO AIR and AYANEO AIR Pro models of handheld
> devices. These devices use the same EC registers and logic as the One X
> Player mini AMD. Previous AYANEO models are not supported as they use a
> different EC and do not have the necessary fan speed write enable and
> setting registers. Tihe driver is tested on Aya Neo AIR while AIR Pro
> model EC functionality and DMI data were verified using command line
> tools by another user.
>
> The added devices are:
> - AYANEO AIR (AMD 5560U)
> - AYANEO AIR Pro (AMD 5560U)
> - AYANEO AIR Pro (AMD 5825U)
>
> Signed-off-by: Derek J. Clark <derekjohn.clark@xxxxxxxxx>

Missed v2, sorry. Comments below.

> ---
> Documentation/hwmon/oxp-sensors.rst | 19 ++++++----
> MAINTAINERS | 1 +
> drivers/hwmon/oxp-sensors.c | 54 ++++++++++++++++++++++++-----
> 3 files changed, 59 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/hwmon/oxp-sensors.rst b/Documentation/hwmon/oxp-sensors.rst
> index 39c588ec5c50..a53c961065b2 100644
> --- a/Documentation/hwmon/oxp-sensors.rst
> +++ b/Documentation/hwmon/oxp-sensors.rst
> @@ -3,18 +3,21 @@
> Kernel driver oxp-sensors
> =========================
>
> -Author:
> +Authors:
> + - Derek John Clark <derekjohn.clark@xxxxxxxxx>
> - Joaquín Ignacio Aramendía <samsagax@xxxxxxxxx>
>
> -Description:
> +Description
> ------------

Why drop ":" here ?

>
> -One X Player devices from One Netbook provide fan readings and fan control
> -through its Embedded Controller.
> +Handheld devices from One Netbook and Aya Neo provide fan readings and fan
> +control through their embedded controllers.
>
> -Currently only supports AMD boards from the One X Player and AOK ZOE lineup.
> -Intel boards could be supported if we could figure out the EC registers and
> -values to write to since the EC layout and model is different.
> +Currently only supports AMD boards from One X Player, AOK ZOE, and some Aya
> +Neo devices. One X PLayer Intel boards could be supported if we could figure
> +out the EC registers and values to write to since the EC layout and model is
> +different. Aya Neo devices preceding the AIR may not be usable as the EC model

s/usable/supportable/ ?

> +is different and do not appear to have manual control capabiltities.
>
> Supported devices
> -----------------
> @@ -22,6 +25,8 @@ Supported devices
> Currently the driver supports the following handhelds:
>
> - AOK ZOE A1
> + - Aya Neo AIR
> + - Aya Neo AIR Pro
> - OneXPlayer AMD
> - OneXPlayer mini AMD
> - OneXPlayer mini AMD PRO
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 90220659206c..8bce95170f12 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15346,6 +15346,7 @@ F: drivers/mtd/nand/onenand/
> F: include/linux/mtd/onenand*.h
>
> ONEXPLAYER FAN DRIVER
> +M: Derek John Clark <derekjohn.clark@xxxxxxxxx>
> M: Joaquín Ignacio Aramendía <samsagax@xxxxxxxxx>
> L: linux-hwmon@xxxxxxxxxxxxxxx
> S: Maintained
> diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
> index f84ec8f8eda9..7adc0199ea66 100644
> --- a/drivers/hwmon/oxp-sensors.c
> +++ b/drivers/hwmon/oxp-sensors.c
> @@ -1,12 +1,12 @@
> // SPDX-License-Identifier: GPL-2.0+
> /*
> - * Platform driver for OXP Handhelds that expose fan reading and control
> - * via hwmon sysfs.
> + * Platform driver for Handhelds that expose fan reading and control via
> + * hwmon sysfs.

Still too generic. Refer to "One Netbook and Aya Neo" as above.

> *
> - * Old boards have the same DMI strings and they are told appart by the
> - * boot cpu vendor (Intel/AMD). Currently only AMD boards are supported
> - * but the code is made to be simple to add other handheld boards in the
> - * future.
> + * Old OXP boards have the same DMI strings and they are told appart by

Old problem, but s/appart/apart/

> + * the boot cpu vendor (Intel/AMD). Currently only AMD boards are
> + * supported but the code is made to be simple to add other handheld
> + * boards in the future.
> * Fan control is provided via pwm interface in the range [0-255].
> * Old AMD boards use [0-100] as range in the EC, the written value is
> * scaled to accommodate for that. Newer boards like the mini PRO and
> @@ -42,6 +42,8 @@ static bool unlock_global_acpi_lock(void)
>
> enum oxp_board {
> aok_zoe_a1 = 1,
> + aya_neo_air,
> + aya_neo_air_pro,
> oxp_mini_amd,
> oxp_mini_amd_pro,
> };
> @@ -60,6 +62,20 @@ static const struct dmi_system_id dmi_table[] = {
> },
> .driver_data = (void *) &(enum oxp_board) {aok_zoe_a1},
> },
> + {
> + .matches = {
> + DMI_MATCH(DMI_BOARD_VENDOR, "AYANEO"),
> + DMI_EXACT_MATCH(DMI_BOARD_NAME, "AIR"),
> + },
> + .driver_data = (void *) &(enum oxp_board) {aya_neo_air},
> + },
> + {
> + .matches = {
> + DMI_MATCH(DMI_BOARD_VENDOR, "AYANEO"),
> + DMI_EXACT_MATCH(DMI_BOARD_NAME, "AIR Pro"),
> + },
> + .driver_data = (void *) &(enum oxp_board) {aya_neo_air_pro},
> + },
> {
> .matches = {
> DMI_MATCH(DMI_BOARD_VENDOR, "ONE-NETBOOK"),
> @@ -161,8 +177,19 @@ static int oxp_platform_read(struct device *dev, enum hwmon_sensor_types type,
> ret = read_from_ec(OXP_SENSOR_PWM_REG, 1, val);
> if (ret)
> return ret;
> - if (board == oxp_mini_amd)
> + switch (board) {
> + case aok_zoe_a1:
> + break;
> + case aya_neo_air:
> + case aya_neo_air_pro:
> + case oxp_mini_amd:
> *val = (*val * 255) / 100;
> + break;
> + case oxp_mini_amd_pro:
> + break;
> + default:
> + break;

bundle case statements

> + }
> return 0;
> case hwmon_pwm_enable:
> return read_from_ec(OXP_SENSOR_PWM_ENABLE_REG, 1, val);
> @@ -191,8 +218,19 @@ static int oxp_platform_write(struct device *dev, enum hwmon_sensor_types type,
> case hwmon_pwm_input:
> if (val < 0 || val > 255)
> return -EINVAL;
> - if (board == oxp_mini_amd)
> + switch (board) {
> + case aok_zoe_a1:
> + break;
> + case aya_neo_air:
> + case aya_neo_air_pro:
> + case oxp_mini_amd:
> val = (val * 100) / 255;
> + break;
> + case oxp_mini_amd_pro:
> + break;
> + default:
> + break;
> + }

Same as above.

> return write_to_ec(dev, OXP_SENSOR_PWM_REG, val);
> default:
> break;