Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Guenter Roeck <groeck7@gmail.com>
Date:   Sat, 17 Jun 2023 10:16:04 -0700
From:   Guenter Roeck <linux@roeck-us.net>
To:     =?iso-8859-1?Q?Joaqu=EDn_Ignacio_Aramend=EDa?= <samsagax@gmail.com>
Cc:     derekjohn.clark@gmail.com, jdelvare@suse.com,
        linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] hwmon: (oxp-sensors) Add tt_toggle attribute on
 supported boards
Message-ID: <4e3913cb-8f7c-4df1-85cb-36e2d562dd6d@roeck-us.net>
References: <20230611143332.40590-2-samsagax@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230611143332.40590-2-samsagax@gmail.com>
Precedence: bulk

On Sun, Jun 11, 2023 at 11:33:20AM -0300, Joaqu?n Ignacio Aramend?a wrote:
> OneXPlayer boards from the last generation (both for OneXPlayer and AOK
> ZOE brands) have a toggle in the EC to switch the "Turbo/Silent" button
> into a different keyboard event.
> 
> Add a means to use that "Turbo button takeover" function and expose it
> to userspace in a custom sysfs `tt_toggle` attribute. It can be read to
> take the current state. Write 1|0 to activate the function. The specific
> keycode is dependent on the board but can be checked by running
> `evtest` utility.
> 
> Newer BIOS on the OneXPlayer added this function aside from string changes.
> Add a board enum to differentiate it from the old OneXplayer Mini AMD BIOS.
> 
> Currently known supported boards:
> - AOK ZOE A1
> - OneXPlayer Mini AMD (only newer BIOS version supported)
> - OneXPlayer Mini Pro
> 
> Signed-off-by: Joaqu?n Ignacio Aramend?a <samsagax@gmail.com>

Applied to hwmon-next.

Thanks,
Guenter

> ---
> v2 changes:
> - Attach the attribute to the platform device as per review
> - Make the attribute return status 0|1 instead of read value
> 
> v3 changes:
> - Mention the attribute attached to the platform device on Documentation
> - Use boolean instead of base 10 int in tt_toggle_store()
> - Remove unnecesary status logic in tt_toggle_show() (use !!val instead)
> - Add missing breaks in probe()
> ---
>  Documentation/hwmon/oxp-sensors.rst |  17 ++++
>  drivers/hwmon/oxp-sensors.c         | 134 +++++++++++++++++++++++++++-
>  2 files changed, 150 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/hwmon/oxp-sensors.rst b/Documentation/hwmon/oxp-sensors.rst
> index 4ab442301415..0ca1f7728c34 100644
> --- a/Documentation/hwmon/oxp-sensors.rst
> +++ b/Documentation/hwmon/oxp-sensors.rst
> @@ -19,6 +19,11 @@ 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 supportable as the EC
>  model is different and do not appear to have manual control capabilities.
>  
> +Some models have a toggle for changing the behaviour of the "Turbo/Silent"
> +button of the device. It will change the key event that it triggers with
> +a flip of the `tt_toggle` attribute. See below for boards that support this
> +function.
> +
>  Supported devices
>  -----------------
>  
> @@ -33,6 +38,11 @@ Currently the driver supports the following handhelds:
>   - OneXPlayer mini AMD
>   - OneXPlayer mini AMD PRO
>  
> +"Turbo/Silent" button behaviour toggle is only supported on:
> + - AOK ZOE A1
> + - OneXPlayer mini AMD (only with updated alpha BIOS)
> + - OneXPlayer mini AMD PRO
> +
>  Sysfs entries
>  -------------
>  
> @@ -49,3 +59,10 @@ pwm1
>    Read Write. Read this attribute to see current duty cycle in the range [0-255].
>    When pwm1_enable is set to "1" (manual) write any value in the range [0-255]
>    to set fan speed.
> +
> +tt_toggle
> +  Read Write. Read this attribute to check the status of the turbo/silent
> +  button behaviour function. Write "1" to activate the switch and "0" to
> +  deactivate it. The specific keycodes and behaviour is specific to the device
> +  both with this function on and off. This attribute is attached to the platform
> +  driver and not to the hwmon driver (/sys/devices/platform/oxp-platform/tt_toggle)
> diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
> index 0ec7588610ad..be36d38f13d9 100644
> --- a/drivers/hwmon/oxp-sensors.c
> +++ b/drivers/hwmon/oxp-sensors.c
> @@ -47,15 +47,29 @@ enum oxp_board {
>  	aya_neo_air_pro,
>  	aya_neo_geek,
>  	oxp_mini_amd,
> +	oxp_mini_amd_a07,
>  	oxp_mini_amd_pro,
>  };
>  
>  static enum oxp_board board;
>  
> +/* Fan reading and PWM */
>  #define OXP_SENSOR_FAN_REG		0x76 /* Fan reading is 2 registers long */
>  #define OXP_SENSOR_PWM_ENABLE_REG	0x4A /* PWM enable is 1 register long */
>  #define OXP_SENSOR_PWM_REG		0x4B /* PWM reading is 1 register long */
>  
> +/* Turbo button takeover function
> + * Older boards have different values and EC registers
> + * for the same function
> + */
> +#define OXP_OLD_TURBO_SWITCH_REG	0x1E
> +#define OXP_OLD_TURBO_TAKE_VAL		0x01
> +#define OXP_OLD_TURBO_RETURN_VAL	0x00
> +
> +#define OXP_TURBO_SWITCH_REG		0xF1
> +#define OXP_TURBO_TAKE_VAL		0x40
> +#define OXP_TURBO_RETURN_VAL		0x00
> +
>  static const struct dmi_system_id dmi_table[] = {
>  	{
>  		.matches = {
> @@ -104,7 +118,7 @@ static const struct dmi_system_id dmi_table[] = {
>  			DMI_MATCH(DMI_BOARD_VENDOR, "ONE-NETBOOK"),
>  			DMI_EXACT_MATCH(DMI_BOARD_NAME, "ONEXPLAYER mini A07"),
>  		},
> -		.driver_data = (void *)oxp_mini_amd,
> +		.driver_data = (void *)oxp_mini_amd_a07,
>  	},
>  	{
>  		.matches = {
> @@ -156,6 +170,102 @@ static int write_to_ec(u8 reg, u8 value)
>  	return ret;
>  }
>  
> +/* Turbo button toggle functions */
> +static int tt_toggle_enable(void)
> +{
> +	u8 reg;
> +	u8 val;
> +
> +	switch (board) {
> +	case oxp_mini_amd_a07:
> +		reg = OXP_OLD_TURBO_SWITCH_REG;
> +		val = OXP_OLD_TURBO_TAKE_VAL;
> +		break;
> +	case oxp_mini_amd_pro:
> +	case aok_zoe_a1:
> +		reg = OXP_TURBO_SWITCH_REG;
> +		val = OXP_TURBO_TAKE_VAL;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	return write_to_ec(reg, val);
> +}
> +
> +static int tt_toggle_disable(void)
> +{
> +	u8 reg;
> +	u8 val;
> +
> +	switch (board) {
> +	case oxp_mini_amd_a07:
> +		reg = OXP_OLD_TURBO_SWITCH_REG;
> +		val = OXP_OLD_TURBO_RETURN_VAL;
> +		break;
> +	case oxp_mini_amd_pro:
> +	case aok_zoe_a1:
> +		reg = OXP_TURBO_SWITCH_REG;
> +		val = OXP_TURBO_RETURN_VAL;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	return write_to_ec(reg, val);
> +}
> +
> +/* Callbacks for turbo toggle attribute */
> +static ssize_t tt_toggle_store(struct device *dev,
> +			       struct device_attribute *attr, const char *buf,
> +			       size_t count)
> +{
> +	int rval;
> +	bool value;
> +
> +	rval = kstrtobool(buf, &value);
> +	if (rval)
> +		return rval;
> +
> +	if (value) {
> +		rval = tt_toggle_enable();
> +		if (rval)
> +			return rval;
> +	} else {
> +		rval = tt_toggle_disable();
> +		if (rval)
> +			return rval;
> +	}
> +	return count;
> +}
> +
> +static ssize_t tt_toggle_show(struct device *dev,
> +			      struct device_attribute *attr, char *buf)
> +{
> +	int retval;
> +	u8 reg;
> +	long val;
> +
> +	switch (board) {
> +	case oxp_mini_amd_a07:
> +		reg = OXP_OLD_TURBO_SWITCH_REG;
> +		break;
> +	case oxp_mini_amd_pro:
> +	case aok_zoe_a1:
> +		reg = OXP_TURBO_SWITCH_REG;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	retval = read_from_ec(reg, 1, &val);
> +	if (retval)
> +		return retval;
> +
> +	return sysfs_emit(buf, "%d\n", !!val);
> +}
> +
> +static DEVICE_ATTR_RW(tt_toggle);
> +
> +/* PWM enable/disable functions */
>  static int oxp_pwm_enable(void)
>  {
>  	return write_to_ec(OXP_SENSOR_PWM_ENABLE_REG, 0x01);
> @@ -206,6 +316,7 @@ static int oxp_platform_read(struct device *dev, enum hwmon_sensor_types type,
>  			case aya_neo_air_pro:
>  			case aya_neo_geek:
>  			case oxp_mini_amd:
> +			case oxp_mini_amd_a07:
>  				*val = (*val * 255) / 100;
>  				break;
>  			case oxp_mini_amd_pro:
> @@ -247,6 +358,7 @@ static int oxp_platform_write(struct device *dev, enum hwmon_sensor_types type,
>  			case aya_neo_air_pro:
>  			case aya_neo_geek:
>  			case oxp_mini_amd:
> +			case oxp_mini_amd_a07:
>  				val = (val * 100) / 255;
>  				break;
>  			case aok_zoe_a1:
> @@ -274,6 +386,13 @@ static const struct hwmon_channel_info * const oxp_platform_sensors[] = {
>  	NULL,
>  };
>  
> +static struct attribute *oxp_ec_attrs[] = {
> +	&dev_attr_tt_toggle.attr,
> +	NULL
> +};
> +
> +ATTRIBUTE_GROUPS(oxp_ec);
> +
>  static const struct hwmon_ops oxp_ec_hwmon_ops = {
>  	.is_visible = oxp_ec_hwmon_is_visible,
>  	.read = oxp_platform_read,
> @@ -291,6 +410,7 @@ static int oxp_platform_probe(struct platform_device *pdev)
>  	const struct dmi_system_id *dmi_entry;
>  	struct device *dev = &pdev->dev;
>  	struct device *hwdev;
> +	int ret;
>  
>  	/*
>  	 * Have to check for AMD processor here because DMI strings are the
> @@ -305,6 +425,18 @@ static int oxp_platform_probe(struct platform_device *pdev)
>  
>  	board = (enum oxp_board)(unsigned long)dmi_entry->driver_data;
>  
> +	switch (board) {
> +	case aok_zoe_a1:
> +	case oxp_mini_amd_a07:
> +	case oxp_mini_amd_pro:
> +		ret = devm_device_add_groups(dev, oxp_ec_groups);
> +		if (ret)
> +			return ret;
> +		break;
> +	default:
> +		break;
> +	}
> +
>  	hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", NULL,
>  						     &oxp_ec_chip_info, NULL);
>