Mailing List Archive

[PATCH] hwmon: (oxp-sensors) Add tt_toggle attribute on supported boards
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>
---
This patch includes the one in https://lore.kernel.org/linux-hwmon/20230526215621.16075-1-samsagax@gmail.com/
so it should be discarded.
---
Documentation/hwmon/oxp-sensors.rst | 16 ++++
drivers/hwmon/oxp-sensors.c | 140 +++++++++++++++++++++++++++-
2 files changed, 153 insertions(+), 3 deletions(-)

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.
diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
index 0ec7588610ad..80fd153253fc 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,108 @@ 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;
+
+ 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;
+
+ 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;
+ }
+
+ retval = read_from_ec(reg, 1, &val);
+ if (retval)
+ return retval;
+
+ return sysfs_emit(buf, "%ld\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 +322,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 +364,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 +392,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,
@@ -305,8 +430,17 @@ static int oxp_platform_probe(struct platform_device *pdev)

board = (enum oxp_board)(unsigned long)dmi_entry->driver_data;

- hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", NULL,
- &oxp_ec_chip_info, NULL);
+ switch (board) {
+ case aok_zoe_a1:
+ case oxp_mini_amd_a07:
+ case oxp_mini_amd_pro:
+ hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", NULL,
+ &oxp_ec_chip_info, oxp_ec_groups);
+ break;
+ default:
+ hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", NULL,
+ &oxp_ec_chip_info, NULL);
+ }

return PTR_ERR_OR_ZERO(hwdev);
}
--
2.40.1
Re: [PATCH] hwmon: (oxp-sensors) Add tt_toggle attribute on supported boards [ In reply to ]
On Fri, May 26, 2023 at 10:22:06PM -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.
>

This attribute is a no-go. It is not even remotely related to hardware
monitoring, and thus must not be attached to the hwmon device.

I don't know exactly where it belongs, but it appears to be related
to the keyboard. Its natural place therefore seems to be a keyboard driver.
We could possibly also attach it to the platform device, but there would
have to be some precedence of other drivers doing the same. Question
in that case though would be if this is just the first of many attributes
to come. If so, we would need to find a different solution.

Guenter

> 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>
> ---
> This patch includes the one in https://lore.kernel.org/linux-hwmon/20230526215621.16075-1-samsagax@gmail.com/
> so it should be discarded.
> ---
> Documentation/hwmon/oxp-sensors.rst | 16 ++++
> drivers/hwmon/oxp-sensors.c | 140 +++++++++++++++++++++++++++-
> 2 files changed, 153 insertions(+), 3 deletions(-)
>
> 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.
> diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
> index 0ec7588610ad..80fd153253fc 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,108 @@ 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;
> +
> + 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;
> +
> + 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;
> + }
> +
> + retval = read_from_ec(reg, 1, &val);
> + if (retval)
> + return retval;
> +
> + return sysfs_emit(buf, "%ld\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 +322,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 +364,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 +392,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,
> @@ -305,8 +430,17 @@ static int oxp_platform_probe(struct platform_device *pdev)
>
> board = (enum oxp_board)(unsigned long)dmi_entry->driver_data;
>
> - hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", NULL,
> - &oxp_ec_chip_info, NULL);
> + switch (board) {
> + case aok_zoe_a1:
> + case oxp_mini_amd_a07:
> + case oxp_mini_amd_pro:
> + hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", NULL,
> + &oxp_ec_chip_info, oxp_ec_groups);
> + break;
> + default:
> + hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", NULL,
> + &oxp_ec_chip_info, NULL);
> + }
>
> return PTR_ERR_OR_ZERO(hwdev);
> }
> --
> 2.40.1
>
Re: [PATCH] hwmon: (oxp-sensors) Add tt_toggle attribute on supported boards [ In reply to ]
> This attribute is a no-go. It is not even remotely related to hardware
> monitoring, and thus must not be attached to the hwmon device.
>
> I don't know exactly where it belongs, but it appears to be related
> to the keyboard. Its natural place therefore seems to be a keyboard driver.
> We could possibly also attach it to the platform device, but there would
> have to be some precedence of other drivers doing the same. Question
> in that case though would be if this is just the first of many attributes
> to come. If so, we would need to find a different solution.
>
> Guenter

Sure! Should this driver with those changes go into a platform driver?
Seems a better fit to me. The case against keyboard driver is the
switch changes behaviour of the key but both the behaviour with the
switch on and off is device defined. Some use the key as part of an AT
Translated Keyboard and others just operate on the EC itself to grab
the fan and set a special TDP for "Silent mode".
For now this is the first such attribute found by the community and
some talks with the manufacturer but it doesn't mean there wouldn't be
others. Specially with such new form factors adding some "control
panels" on Windows to set some hardware behaviour via EC writes. My
goal is to allow those same functions to be available to linux users
in a way some other userspace tools can serve as front ends.

Would taking this same driver to the platform side be a solution to
that going forward? It would be a combination of hwmon monitoring
attributes and some other special functions with custom attributes.
Seems a better fit to me.
--
Joaquín I. Aramendía
Re: [PATCH] hwmon: (oxp-sensors) Add tt_toggle attribute on supported boards [ In reply to ]
On 5/31/23 05:04, Joaquin Aramendia wrote:
>> This attribute is a no-go. It is not even remotely related to hardware
>> monitoring, and thus must not be attached to the hwmon device.
>>
>> I don't know exactly where it belongs, but it appears to be related
>> to the keyboard. Its natural place therefore seems to be a keyboard driver.
>> We could possibly also attach it to the platform device, but there would
>> have to be some precedence of other drivers doing the same. Question
>> in that case though would be if this is just the first of many attributes
>> to come. If so, we would need to find a different solution.
>>
>> Guenter
>
> Sure! Should this driver with those changes go into a platform driver?

You are attaching the attribute to the hwmon device. Moving the driver
to another directory would just be an attempt to avoid review by a
hwmon maintainer. Just for that reason I would NACK such an attempt.

Guenter

> Seems a better fit to me. The case against keyboard driver is the
> switch changes behaviour of the key but both the behaviour with the
> switch on and off is device defined. Some use the key as part of an AT
> Translated Keyboard and others just operate on the EC itself to grab
> the fan and set a special TDP for "Silent mode".
> For now this is the first such attribute found by the community and
> some talks with the manufacturer but it doesn't mean there wouldn't be
> others. Specially with such new form factors adding some "control
> panels" on Windows to set some hardware behaviour via EC writes. My
> goal is to allow those same functions to be available to linux users
> in a way some other userspace tools can serve as front ends.
>
> Would taking this same driver to the platform side be a solution to
> that going forward? It would be a combination of hwmon monitoring
> attributes and some other special functions with custom attributes.
> Seems a better fit to me.
Re: [PATCH] hwmon: (oxp-sensors) Add tt_toggle attribute on supported boards [ In reply to ]
Hello again. And sorry for my late reply. I'm struggling to find a
good solution to this functionality. It most definitely feels icky to
be attached to the hwmon driver so it should be a different driver for
those devices. Would misc be a good place for that single toggle? or
maybe platform? The latter seems most fitting since is very device
specific.

> You are attaching the attribute to the hwmon device. Moving the driver
> to another directory would just be an attempt to avoid review by a
> hwmon maintainer. Just for that reason I would NACK such an attempt.

By no means I intend to avoid review. Don't get me wrong. I'm trying
to understand what the best practice would be in this scenario. The
driver is part reverse engineering and part asking contacts from the
manufacturer (who is no longer in the company so it is reverse
engineering at this point). So any new functionality we find might be
a different driver for what i gather since it is not related to
hardware monitoring. The oxp-sensors as is now it is mostly done and
it works well for the users.

Thanks for your review and explanation. Any pointers would be greatly
appreciated.
--
Joaquín I. Aramendía
Re: [PATCH] hwmon: (oxp-sensors) Add tt_toggle attribute on supported boards [ In reply to ]
On 6/7/23 05:40, Joaquin Aramendia wrote:
> Hello again. And sorry for my late reply. I'm struggling to find a
> good solution to this functionality. It most definitely feels icky to
> be attached to the hwmon driver so it should be a different driver for
> those devices. Would misc be a good place for that single toggle? or
> maybe platform? The latter seems most fitting since is very device
> specific.
>
>> You are attaching the attribute to the hwmon device. Moving the driver
>> to another directory would just be an attempt to avoid review by a
>> hwmon maintainer. Just for that reason I would NACK such an attempt.
>
> By no means I intend to avoid review. Don't get me wrong. I'm trying
> to understand what the best practice would be in this scenario. The
> driver is part reverse engineering and part asking contacts from the
> manufacturer (who is no longer in the company so it is reverse
> engineering at this point). So any new functionality we find might be
> a different driver for what i gather since it is not related to
> hardware monitoring. The oxp-sensors as is now it is mostly done and
> it works well for the users.
>

Why don't you just attach the attribute to the platform device as I
had suggested earlier ?

Guenter
Re: [PATCH] hwmon: (oxp-sensors) Add tt_toggle attribute on supported boards [ In reply to ]
> Why don't you just attach the attribute to the platform device as I
> had suggested earlier ?

You mean I should do something like this in probe():

static int oxp_platform_probe(struct platform_device *pdev)
{
...

switch (board) {
case aok_zoe_a1:
case oxp_mini_amd_a07:
case oxp_mini_amd_pro:
pdev->dev.groups = oxp_ec_groups;
}
hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", NULL,
&oxp_ec_chip_info, NULL);
...
}

Would that work? Or even be correct?
--
Joaquín I. Aramendía
Re: [PATCH] hwmon: (oxp-sensors) Add tt_toggle attribute on supported boards [ In reply to ]
On 6/9/23 05:50, Joaquin Aramendia wrote:
>> Why don't you just attach the attribute to the platform device as I
>> had suggested earlier ?
>
> You mean I should do something like this in probe():
>
> static int oxp_platform_probe(struct platform_device *pdev)
> {
> ...
>
> switch (board) {
> case aok_zoe_a1:
> case oxp_mini_amd_a07:
> case oxp_mini_amd_pro:
> pdev->dev.groups = oxp_ec_groups;
> }
> hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", NULL,
> &oxp_ec_chip_info, NULL);
> ...
> }
>
> Would that work? Or even be correct?


No, that would not work, because the platform device already exists.
It would have to be added in the init function, or if done in the probe
function it would have to be added to the platform device with an
explicit call to devm_device_add_group().

Guenter