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>
Message-ID: <f4d49b1b-0c42-e38b-c92f-93261045546c@roeck-us.net>
Date:   Sat, 10 Jun 2023 14:08:10 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Content-Language: en-US
To:     =?UTF-8?Q?Joaqu=c3=adn_Ignacio_Aramend=c3=ada?= 
        <samsagax@gmail.com>
Cc:     derekjohn.clark@gmail.com, jdelvare@suse.com,
        linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230610192400.80611-2-samsagax@gmail.com>
From:   Guenter Roeck <linux@roeck-us.net>
Subject: Re: [PATCH v2] hwmon: (oxp-sensors) Add tt_toggle attribute on
 supported boards
In-Reply-To: <20230610192400.80611-2-samsagax@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 6/10/23 12:23, 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>
> ---
> v2 changes:
> - Attach the attribute to the platform device as per review
> - Make the attribute return status 0|1 instead of read value
> ---
>   Documentation/hwmon/oxp-sensors.rst |  16 ++++
>   drivers/hwmon/oxp-sensors.c         | 142 +++++++++++++++++++++++++++-
>   2 files changed, 157 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/hwmon/oxp-sensors.rst b/Documentation/hwmon/oxp-sensors.rst
> index 4ab442301415..131c89fad03a 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,9 @@ 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.

Mention that this attribute is attached to the platform device.

> diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
> index 0ec7588610ad..59e4e906ced5 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,112 @@ 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;
> +	unsigned int value;
> +
> +	rval = kstrtouint(buf, 10, &value);
> +	if (rval)
> +		return rval;

Please use kstrtobool().

> +
> +	switch (value) {
> +	case 0:
> +		rval = tt_toggle_disable();
> +		if (rval)
> +			return rval;
> +		return count;
> +	case 1:
> +		rval = tt_toggle_enable();
> +		if (rval)
> +			return rval;
> +		return count;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static ssize_t tt_toggle_show(struct device *dev,
> +			      struct device_attribute *attr, char *buf)
> +{
> +	int retval;
> +	u8 reg;
> +	long val;
> +	int status;
> +
> +	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;
> +
> +	if (val)
> +		status = 1;
> +	else
> +		status = 0;
> +
> +	return sysfs_emit(buf, "%d\n", status);

	return sysfs_emit(buf, "%d\n", !!val);

is much simpler.

> +}
> +
> +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 +326,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 +368,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 +396,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 +420,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 +435,16 @@ 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);
>