Re: [PATCH v3] Add OneXPlayer mini AMD sensors driver

[Joaquin Aramendia <samsagax@gmail.com>]

Thanks for your time and review. I'll follow your advice and
corrections. Should I send the next patch version in a separate
thread? Or should I answer to this one?

El lun, 31 oct 2022 a la(s) 16:56, Guenter Roeck (linux@roeck-us.net) escribió:
>
> On Mon, Oct 31, 2022 at 11:53:08AM -0300, Joaquín Ignacio Aramendía wrote:
> > Sensors driver for OXP Handhelds from One-Netbook that expose fan reading
> > and control via hwmon sysfs.
> >
> > As far as I could gather all OXP boards have the same DMI strings and
> > they are told appart by the boot cpu vendor (Intel/AMD).
> > Currently only AMD boards are supported.
> >
> > Fan control is provided via pwm interface in the range [0-255]. AMD
> > boards have [0-100] as range in the EC, the written value is scaled to
> > accommodate for that.
> >
> > Signed-off-by: Joaquín Ignacio Aramendía <samsagax@gmail.com>
>
> Please run "checkpatch --strict" on your patch and fix the CHECK
> messages. Also see Documentation/hwmon/submitting-patches.rst.

There is a Warning about MAINTAINERS to be updated. Should I add
myself to it? If yes, Should it be under a new header?

> > ---
> > Removed fan_control reference in comment.
> > Bugfix MIX/MIN reporting not available
> > Bugfix pwm_enable register set wrong
> > ---
> >  drivers/hwmon/Kconfig       |  13 +-
> >  drivers/hwmon/Makefile      |   1 +
> >  drivers/hwmon/oxp-sensors.c | 277 ++++++++++++++++++++++++++++++++++++
>
> Also needs Documentation/hwmon/oxp-sensors.rst

Will add.

> >  3 files changed, 290 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/hwmon/oxp-sensors.c
> >
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 7ac3daaf59ce..a1cdb03b4d13 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -1607,6 +1607,17 @@ config SENSORS_NZXT_SMART2
> >
> >  source "drivers/hwmon/occ/Kconfig"
> >
> > +config SENSORS_OXP
> > +     tristate "OneXPlayer EC fan control"
> > +     depends on ACPI
> > +     depends on X86
> > +     help
> > +             If you say yes here you get support for fan readings and control over
> > +             OneXPlayer handheld devices. Only OneXPlayer mini AMD handheld variant
> > +             boards are supported.
> > +
> > +             Can also be built as a module. In that case it will be called oxp-sensors.
> > +
> >  config SENSORS_PCF8591
> >       tristate "Philips PCF8591 ADC/DAC"
> >       depends on I2C
> > @@ -1957,7 +1968,7 @@ config SENSORS_ADS7871
> >
> >  config SENSORS_AMC6821
> >       tristate "Texas Instruments AMC6821"
> > -     depends on I2C
> > +     depends on I2C
>
> Please refrain from making unrelated changes. If you want to fix the extra
> space, submit a separate patch.

Sorry this must have been vim removing trailing spaces. Will remove
this chunk from the patch.

> >       help
> >         If you say yes here you get support for the Texas Instruments
> >         AMC6821 hardware monitoring chips.
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index 11d076cad8a2..35824f8be455 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -167,6 +167,7 @@ obj-$(CONFIG_SENSORS_NSA320)      += nsa320-hwmon.o
> >  obj-$(CONFIG_SENSORS_NTC_THERMISTOR) += ntc_thermistor.o
> >  obj-$(CONFIG_SENSORS_NZXT_KRAKEN2) += nzxt-kraken2.o
> >  obj-$(CONFIG_SENSORS_NZXT_SMART2) += nzxt-smart2.o
> > +obj-$(CONFIG_SENSORS_OXP) += oxp-sensors.o
> >  obj-$(CONFIG_SENSORS_PC87360)        += pc87360.o
> >  obj-$(CONFIG_SENSORS_PC87427)        += pc87427.o
> >  obj-$(CONFIG_SENSORS_PCF8591)        += pcf8591.o
> > diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
> > new file mode 100644
> > index 000000000000..f5895dc11094
> > --- /dev/null
> > +++ b/drivers/hwmon/oxp-sensors.c
> > @@ -0,0 +1,277 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Platform driver for OXP Handhelds that expose fan reading and control
> > + * via hwmon sysfs.
> > + *
> > + * All 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.
> > + * Fan control is provided via pwm interface in the range [0-255]. AMD
> > + * boards use [0-100] as range in the EC, the written value is scaled to
> > + * accommodate for that.
> > + *
> > + * Copyright (C) 2022 Joaquín I. Aramendía <samsagax@gmail.com>
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/dev_printk.h>
> > +#include <linux/dmi.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/processor.h>
> > +
> > +#define ACPI_LOCK_DELAY_MS   500
> > +
> > +/* Handle ACPI lock mechanism */
> > +struct lock_data {
> > +     u32 mutex;
> > +     bool (*lock)(struct lock_data *data);
> > +     bool (*unlock)(struct lock_data *data);
> > +};
> > +
> > +static bool lock_global_acpi_lock(struct lock_data *data)
> > +{
> > +     return ACPI_SUCCESS(acpi_acquire_global_lock(ACPI_LOCK_DELAY_MS,
> > +                                                              &data->mutex));
> > +}
> > +
> > +static bool unlock_global_acpi_lock(struct lock_data *data)
> > +{
> > +     return ACPI_SUCCESS(acpi_release_global_lock(data->mutex));
> > +}
> > +
> > +#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 */
> > +
> > +static const struct dmi_system_id dmi_table[] = {
> > +     {
> > +             .matches = {
> > +                     DMI_EXACT_MATCH(DMI_BOARD_VENDOR,
> > +                                     "ONE-NETBOOK TECHNOLOGY CO., LTD."),
> > +             },
> > +     },
> > +     {
> > +             .matches = {
> > +                     DMI_EXACT_MATCH(DMI_BOARD_VENDOR,
> > +                                     "ONE-NETBOOK"),
>
> Are there any others devices which start with "ONE-NETBOOK" but are not
> supported ? If not a single entry with DMI_MATCH() woud be sufficient.
> Either case I would like to see some additional entry or entries here.
> Just matching on the vendor name seems risky. At the very least there
> should also be a match for the "ONE XPLAYER" sku.

I would add a match for the board name instead of the sku if that is
ok. The rest will be added.

> > +             },
> > +     },
> > +     {},
> > +};
> > +
> > +struct oxp_status {
> > +     struct lock_data lock_data;
> > +};
> > +
> > +/* Helper functions to handle EC read/write */
> > +static int read_from_ec(u8 reg, int size, long *val)
> > +{
> > +     int i;
> > +     int ret;
> > +     u8 buffer;
> > +
> > +     *val = 0;
> > +     for (i = 0; i < size; i++) {
> > +             ret = ec_read(reg + i, &buffer);
> > +             if (ret)
> > +                     return ret;
> > +             (*val) <<= i * 8;
>
> Unnecessary ( )

Will remove.

> > +             *val += buffer;
> > +     }
> > +     return ret;
> > +}
> > +
> > +static int write_to_ec(const struct device *dev, u8 reg, u8 value)
> > +{
> > +     struct oxp_status *state = dev_get_drvdata(dev);
> > +     int ret;
> > +
> > +     if (!state->lock_data.lock(&state->lock_data)) {
> > +             dev_warn(dev, "Failed to acquire mutex");
>
> Is that message necessary ? If so it should be dev_err().
> If it is expected, ie if acquiring the lock is observed
> to fail sometimes, there should be no log message.

The messages are there in case this fails, never failed on me,
honestly, but I've seen it in other ec-sensors drivers and adopted it
as a "good practice", I guess? Anyway I'll add a _once error message
and return error if it fails.

> > +             return -EBUSY;
> > +     }
> > +
> > +     ret = ec_write(reg, value);
> > +
> > +     if (!state->lock_data.unlock(&state->lock_data))
> > +             dev_err(dev, "Failed to release mutex");
>
> No error return ? Then it is not an error and should not be
> logged as one.
>
> I am a bit concerned about those error messages. If they are seen
> the errors are either sporadic and there should be no log,
> or they are persistent and would clog the kernel log. If you think
> that will indeed happen, is not normal operation, and that the
> message is essential enough to be logged, please at least consider
> using _once variants of the message to avoid clogging the kernel
> log.

Never saw those errors in about a month I used this driver on my own
device. As said, I saw the practice in other drivers. I think the best
way is to check for it and return an error while reporting it with the
_once variant if that is ok.

> > +
> > +     return ret;
> > +}
> > +
> > +static int oxp_pwm_enable(const struct device *dev)
> > +{
> > +     return write_to_ec(dev, OXP_SENSOR_PWM_ENABLE_REG, 0x01);
> > +}
> > +
> > +static int oxp_pwm_disable(const struct device *dev)
> > +{
> > +     return write_to_ec(dev, OXP_SENSOR_PWM_ENABLE_REG, 0x00);
> > +}
> > +
> > +/* Callbacks for hwmon interface */
> > +static umode_t oxp_ec_hwmon_is_visible(const void *drvdata,
> > +                                     enum hwmon_sensor_types type, u32 attr, int channel)
> > +{
> > +     switch (type) {
> > +     case hwmon_fan:
> > +             return 0444;
> > +     case hwmon_pwm:
> > +             return 0644;
> > +     default:
> > +             return 0;
> > +     }
> > +     return 0;
> > +}
> > +
> > +static int oxp_platform_read(struct device *dev, enum hwmon_sensor_types type,
> > +                          u32 attr, int channel, long *val)
> > +{
> > +     int ret;
> > +
> > +     switch (type) {
> > +     case hwmon_fan:
> > +             switch (attr) {
> > +             case hwmon_fan_input:
> > +                     return read_from_ec(OXP_SENSOR_FAN_REG,
> > +                                        2,
> > +                                        val);
>
> Unnecessary continuation lines

Ok. Will correct.

> > +             default:
> > +                     dev_dbg(dev, "Unknown attribute for type %d: %d\n", type, attr);
> > +                     return -EOPNOTSUPP;
> > +             }
> > +     case hwmon_pwm:
> > +             switch (attr) {
> > +             case hwmon_pwm_input:
> > +                     ret = read_from_ec(OXP_SENSOR_PWM_REG,
> > +                                        2, val);
>
> Please, no unnecessary continuation lines, and make sure that continuation
> lines match with '(' in the preceding line.

Ok. Will correct.

> > +                     *val = (*val * 255) / 100;
> > +                     return ret;
> > +             case hwmon_pwm_enable:
> > +                     return read_from_ec(OXP_SENSOR_PWM_ENABLE_REG, 1, val);
> > +             default:
> > +                     dev_dbg(dev, "Unknown attribute for type %d: %d\n", type, attr);
> > +                     return -EOPNOTSUPP;
> > +             }
> > +     default:
> > +             dev_dbg(dev, "Unknown sensor type %d.\n", type);
> > +             return -EOPNOTSUPP;
> > +     }
> > +}
> > +
> > +static int oxp_platform_write(struct device *dev, enum hwmon_sensor_types type,
> > +             u32 attr, int channel, long val)
> > +{
> > +     switch (type) {
> > +     case hwmon_pwm:
> > +             switch (attr) {
> > +             case hwmon_pwm_enable:
> > +                     if (val == 1)
> > +                             return oxp_pwm_enable(dev);
> > +                     else if (val == 0)
> > +                             return oxp_pwm_disable(dev);
> > +                     else
> > +                             return -EINVAL;
> > +             case hwmon_pwm_input:
> > +                     if (val < 0 || val > 255)
> > +                             return -EINVAL;
> > +                     val = (val * 100) / 255;
> > +                     return write_to_ec(dev, OXP_SENSOR_PWM_REG, val);
> > +             default:
> > +                     dev_dbg(dev, "Unknown attribute for type %d: %d", type, attr);
> > +                     return -EOPNOTSUPP;
> > +             }
> > +     default:
> > +             dev_dbg(dev, "Unknown sensor type: %d", type);
> > +             return -EOPNOTSUPP;
> > +     }
> > +     return -EINVAL;
> > +}
> > +
> > +/* Known sensors in the OXP EC controllers */
> > +static const struct hwmon_channel_info *oxp_platform_sensors[] = {
> > +     HWMON_CHANNEL_INFO(fan,
> > +             HWMON_F_INPUT),
> > +     HWMON_CHANNEL_INFO(pwm,
> > +             HWMON_PWM_INPUT | HWMON_PWM_ENABLE),
> > +     NULL,
> > +};
> > +
> > +static const struct hwmon_ops oxp_ec_hwmon_ops = {
> > +     .is_visible = oxp_ec_hwmon_is_visible,
> > +     .read = oxp_platform_read,
> > +     .write = oxp_platform_write,
> > +};
> > +
> > +static const struct hwmon_chip_info oxp_ec_chip_info = {
> > +     .ops = &oxp_ec_hwmon_ops,
> > +     .info = oxp_platform_sensors,
> > +};
> > +
> > +/* Initialization logic */
> > +static int oxp_platform_probe(struct platform_device *pdev)
> > +{
> > +     const struct dmi_system_id *dmi_entry;
> > +     struct device *dev = &pdev->dev;
> > +     struct device *hwdev;
> > +     struct oxp_status *state;
> > +
> > +     /* Have to check for AMD processor here */
> > +     dmi_entry = dmi_first_match(dmi_table);
> > +     if (!dmi_entry || boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> > +             return -ENODEV;
> > +
> > +     state = devm_kzalloc(dev, sizeof(struct oxp_status), GFP_KERNEL);
> > +     if (!state)
> > +             return -ENOMEM;
> > +
> > +     state->lock_data.mutex = 0;
>
> Unnecessary initialization. The data structure is initialized with
> devm_kzalloc().

Ok. Will remove.

> > +     state->lock_data.lock = lock_global_acpi_lock;
> > +     state->lock_data.unlock = unlock_global_acpi_lock;
>
> What is the purpose of this callback function ?
> Why not just call the lock and unlock functions directly ?

Can be called directly and simplify the structure a little. Will change it.

> > +
> > +     hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", state,
> > +                                                     &oxp_ec_chip_info, NULL);
> > +
> > +     return PTR_ERR_OR_ZERO(hwdev);
> > +}
> > +
> > +static struct platform_driver oxp_platform_driver = {
> > +     .driver = {
> > +             .name = "oxp-platform",
> > +     },
> > +     .probe = oxp_platform_probe,
> > +};
> > +
> > +static struct platform_device *oxp_platform_device;
> > +
> > +static int __init oxp_platform_init(void)
> > +{
> > +     oxp_platform_device =
> > +             platform_create_bundle(&oxp_platform_driver,
> > +                                    oxp_platform_probe, NULL, 0, NULL, 0);
> > +
> > +     if (IS_ERR(oxp_platform_device))
> > +             return PTR_ERR(oxp_platform_device);
> > +
> > +     return 0;
> > +}
> > +
> > +static void __exit oxp_platform_exit(void)
> > +{
> > +     platform_device_unregister(oxp_platform_device);
> > +     platform_driver_unregister(&oxp_platform_driver);
> > +}
> > +
> > +MODULE_DEVICE_TABLE(dmi, dmi_table);
> > +module_init(oxp_platform_init);
> > +module_exit(oxp_platform_exit);
> > +
> > +MODULE_AUTHOR("Joaquín Ignacio Aramendía <samsagax@gmail.com>");
> > +MODULE_DESCRIPTION(
> > +     "Platform driver that handles ACPI EC of OneXPlayer devices");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.38.1
> >

Thanks again :)
-- 
Joaquín I. Aramendía