Hello, thanks so much to take the time for the feedback!
Maybe this one needs some clarification as for some design choices in
regards to some of the structures you see. I know there is only one
board supported at the moment, but those structures are meant to be
the boilerplate for possibly more boards. I'm aware that these devices
are a mess and the EC registers are all over the place. I wanted to
make it easier for myself. That said I'll try to address some of the
concerns and redo the patcha according to standards :)
El sáb, 29 oct 2022 a la(s) 20:30, Guenter Roeck (linux@xxxxxxxxxxxx) escribió:
>
> On 10/29/22 15:50, Joaquín Ignacio Aramendía wrote:
> > +
> > +static bool fan_control;
> > +module_param_hw(fan_control, bool, other, 0644);
>
> Runtime writeable parameter is unacceptable. Why would this be needed anyway ?
> What is it supposed to accomplish that can not be done with a sysfs attribute ?
Is meant for safety, I suppose, but you are right that it is better to
have the parameter non-writable at runtime.
The goal is to be able to use the fan readings without allowing the
pwm to be used unless you really know what you are doing, but I guess
there is no point in having this if already is the pwm_enable
attribute.
> > +struct oxp_ec_sensor_addr {
> > + enum hwmon_sensor_types type;
> > + u8 reg;
> > + short size;
> > + union {
> > + struct {
> > + u8 enable;
> > + u8 val_enable;
> > + u8 val_disable;
> > + };
> > + struct {
> > + int max_speed;
> > + };
> > + };
> > +};
> > +
> > +
>
> Extra empty line.
Removed. Thanks.
> > +/* AMD board EC addresses */
> > +static const struct oxp_ec_sensor_addr amd_sensors[] = {
> > + [oxp_sensor_fan] = {
> > + .type = hwmon_fan,
> > + .reg = 0x76,
> > + .size = 2,
> > + .max_speed = 5000,
> > + },
> > + [oxp_sensor_pwm] = {
> > + .type = hwmon_pwm,
> > + .reg = 0x4B,
> > + .size = 1,
> > + .enable = 0x4A,
> > + .val_enable = 0x01,
> > + .val_disable = 0x00,
> > + },
>
> I don't see the point of this data structure. There is just one
> entry. Why not use defines ?
It is one entry now, but i figured in a while there will be other
boards to support with different values. Having this structure seems
easier for future updates.
I can remove it and only use defines for now.
> > + {},
> > +};
> > +
> > +struct ec_board_info {
> > + const char *board_names[MAX_IDENTICAL_BOARD_VARIATIONS];
> > + enum board_family family;
> > + const struct oxp_ec_sensor_addr *sensors;
> > +};
> > +
> > +static const struct ec_board_info board_info[] = {
> > + {
> > + .board_names = {"ONE XPLAYER", "ONEXPLAYER mini A07"},
> > + .family = family_mini_amd,
> > + .sensors = amd_sensors,
> > + },
>
> There is just one family. What is the point of this data structure ?
It is meant for expansion on other boards. I only own a OXP mini AMD,
but others exist with their own quirks. For example, the same device
but with Intel CPU has completely different EC registers, values and
ranges.
I guess i can remove the whole "family" thing and bring it back when
it is appropriate.
> > + {}
> > +};
> > +
> > +struct oxp_status {
> > + struct ec_board_info board;
> > + struct lock_data lock_data;
> > +};
> > +
> > +/* Helper functions */
> > +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;
>
> space between i and 8
Ok. Will remove.
> > + *val += buffer;
> > + }
> > + return ret;
>
> return 0;
>
> > +}
> > +
> > +static int write_to_ec(const struct device *dev, u8 reg, u8 value)
> > +{
> > + struct oxp_status *state = dev_get_drvdata(dev);
> > + int ret = -1;
>
> unnecessary (and bad) variable initialization
Ok. Will improve on this.
> > +
> > + if (!state->lock_data.lock(&state->lock_data)) {
> > + dev_warn(dev, "Failed to acquire mutex");
> > + return -EBUSY;
> > + }
> > +
> > + ret = ec_write(reg, value);
> > +
> > + if (!state->lock_data.unlock(&state->lock_data))
> > + dev_err(dev, "Failed to release mutex");
> > +
> > + return ret;
> > +}
> > +
> > +/* 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 S_IRUGO;
> > + case hwmon_pwm:
> > + return S_IRUGO | S_IWUSR;
>
> Please use 0444 and 0644 directly. Checkpatch will tell.
Oh. I did as the documentation suggested. I must confess I didn't run
checkpatch, will don in the next submission.
> > + default:
> > + return 0;
> > + }
> > + return 0;
> > +}
> > +
> > +static int oxp_platform_read(struct device *dev, enum hwmon_sensor_types type,
> > + u32 attr, int channel, long *val)
>
> Align continuation line with '('. Checkpatch will tell.
Will do. I guess my vim settings are messed up.
> > +{
> > + int ret = -1;
> > + const struct oxp_status *state = dev_get_drvdata(dev);
> > + const struct ec_board_info *board = &state->board;
> > +
> > + switch(type) {
> > + case hwmon_fan:
> > + switch(attr) {
> > + case hwmon_fan_input:
> > + ret = read_from_ec(board->sensors[oxp_sensor_fan].reg,
> > + board->sensors[oxp_sensor_fan].size, val);
> > + break;
> > + case hwmon_fan_max:
> > + ret = 0;
> > + *val = board->sensors[oxp_sensor_fan].max_speed;
> > + break;
> > + case hwmon_fan_min:
> > + ret = 0;
> > + *val = 0;
> > + break;
>
> If fan_max and fan_min are not sent to/from the EC, do not provide the attributes.
They are not, but they are in the spec of the fan (in the attached
sticker, that is). Should I keep it if it's not read directly but has
a known value?
> > + default:
> > + dev_dbg(dev, "Unknown attribute for type %d: %d\n", type, attr);
>
> missing break;
Ooops, sorry. Will improve on this one.
> > + }
> > + return ret;
> > + case hwmon_pwm:
> > + switch(attr) {
> > + case hwmon_pwm_input:
> > + ret = read_from_ec(board->sensors[oxp_sensor_pwm].reg,
> > + board->sensors[oxp_sensor_pwm].size, val);
> > + if (board->family == family_mini_amd) {
> > + *val = (*val * 255) / 100;
> > + }
> > + break;
> > + case hwmon_pwm_enable:
> > + ret = read_from_ec(board->sensors[oxp_sensor_pwm].enable, 1, val);
> > + if (*val == board->sensors[oxp_sensor_pwm].val_enable) {
> > + *val = 1;
> > + } else {
> > + *val = 0;
> > + }
>
> Unnecessary { }. Checkpatch would tell.
>
> I don't see the point of this code. Why have board->sensors[oxp_sensor_pwm].val_enable
> to start with ? It is 1. Can the EC return something else ? If so, what is the
> rationale to map it to 0 ?
>
The enable/disable values are configurable, since they can vary from
board to board. AMD happens to be just 1 and 0, so it fits in this
case. My goal is to map them to 1 and 0 to be exposed in the same way
across all devices. That way userspace tools like fancontrold can use
this interface as is.
> > + break;
> > + default:
> > + dev_dbg(dev, "Unknown attribute for type %d: %d\n", type, attr);
>
> missing break;
Oops, will correct. Thanks
> > + }
> > + return ret;
> > + default:
> > + dev_dbg(dev, "Unknown sensor type %d.\n", type);
> > + return -1;
>
> bad error code.
Should I return EINVAL in this case? Seems appropriate reading error
code headers.
> > + }
> > +}
> > +
> > +static int oxp_pwm_enable(const struct device *dev)
> > +{
> > + int ret = -1;
>
> Unnecessary (and bad) variable initialization.
Ok. Will improve.
> > + struct oxp_status *state = dev_get_drvdata(dev);
> > + const struct ec_board_info *board = &state->board;
> > +
> > + if (!fan_control) {
> > + dev_info(dev, "Can't enable pwm, fan_control=0");
> > + return -EPERM;
> > + }
> > +
> > + ret = write_to_ec(dev, board->sensors[oxp_sensor_pwm].enable,
> > + board->sensors[oxp_sensor_pwm].val_enable);
> > +
> > + return ret;
>
> ... and unnecessary variable.
Then I would just do
return write_to_ec():
as stated below.
> > +}
> > +
> > +static int oxp_pwm_disable(const struct device *dev)
> > +{
> > + int ret = -1;
>
> Unnecessary initialization, and bad error code.
Seems like I really like to mess this up, really... Sorry.
> > + struct oxp_status *state = dev_get_drvdata(dev);
> > + const struct ec_board_info *board = &state->board;
> > +
> > + if (!fan_control) {
> > + dev_info(dev, "Can't disable pwm, fan_control=0");
> > + return -EPERM;
> > + }
>
> I really don't see the point of the "fan_control" module parameter.
> One can set it to 1 (or true) to enable the pwm_enable attribute,
> or set it to 0 to disable it. It is effectively the same as just
> another attribute, forcing users to write two attributes instead
> of one. That really doesn't make sense.
>
Ok. I can remove the `fan_control` parameter entirely and just leave
it to the userspace to handle pwm_enable without any other safeguards.
> > +
> > + ret = write_to_ec(dev, board->sensors[oxp_sensor_pwm].enable,
> > + board->sensors[oxp_sensor_pwm].val_disable);
> > +
> > + return ret;
>
> Just
> return write_to_ec(...);
>
> would do. There is no need for the ret variable. Same elsewhere.
>
Same as above, so I'll just do this.
> > +}
> > +
> > +static int oxp_platform_write(struct device *dev, enum hwmon_sensor_types type,
> > + u32 attr, int channel, long val)
> > +{
> > + int ret = -1;
>
> bad error code.
Ok. Will improve on this.
> > + struct oxp_status *state = dev_get_drvdata(dev);
> > + const struct ec_board_info *board = &state->board;
> > +
> > + switch(type) {
> > + case hwmon_pwm:
> > + if (!fan_control) {
> > + dev_info(dev, "Can't control fans, fan_control=0");
> > + return -EPERM;
> > + }
> > + switch(attr) {
> > + case hwmon_pwm_enable:
> > + if (val == 1) {
> > + ret = oxp_pwm_enable(dev);
> > + } else if (val == 0) {
> > + ret = oxp_pwm_disable(dev);
> > + } else {
> > + return -EINVAL;
> > + }
>
> Unnecessary { }, and the single return on error instead of
> ret = -EINVAL;
> is inconsistent.
Ok. Will improve on this one.
> > + return ret;
> > + case hwmon_pwm_input:
> > + if (val < 0 || val > 255)
> > + return -EINVAL;
> > + if (board->family == family_mini_amd)
> > + val = (val * 100) / 255;
> > + ret = write_to_ec(dev, board->sensors[oxp_sensor_pwm].reg, val);
> > + return ret;
> > + default:
> > + dev_dbg(dev, "Unknown attribute for type %d: %d", type, attr);
> > + return ret;
> > + }
> > + default:
> > + dev_dbg(dev, "Unknown sensor type: %d", type);
>
> break missing
Oops... noted.
> > + }
> > + return ret;
> > +}
> > +
> > +/* Known sensors in the OXP EC controllers */
> > +static const struct hwmon_channel_info *oxp_platform_sensors[] = {
> > + HWMON_CHANNEL_INFO(fan,
> > + HWMON_F_INPUT | HWMON_F_MAX | HWMON_F_MIN),
> > + HWMON_CHANNEL_INFO(pwm,
> > + HWMON_PWM_INPUT | HWMON_PWM_ENABLE),
>
> bad alignment. Please use checkpatch --strict and fix what it reports.
Will do, sorry.
> > + 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 const struct ec_board_info * __init get_board_info(struct device *dev)
> > +{
> > + const char *dmi_board_vendor = dmi_get_system_info(DMI_BOARD_VENDOR);
> > + const char *dmi_board_name = dmi_get_system_info(DMI_BOARD_NAME);
> > + const struct ec_board_info *board;
> > +
> > + if (!dmi_board_vendor || !dmi_board_name ||
> > + (strcasecmp(dmi_board_vendor, "ONE-NETBOOK TECHNOLOGY CO., LTD.") &&
> > + strcasecmp(dmi_board_vendor, "ONE-NETBOOK")))
> > + return NULL;
> > +
> > + /* Match our known boards */
> > + /* Need to check for AMD/Intel here */
> > + for (board = board_info; board->sensors; board++) {
> > + if (match_string(board->board_names,
> > + MAX_IDENTICAL_BOARD_VARIATIONS,
> > + dmi_board_name) >= 0) {
> > + if (board->family == family_mini_amd &&
> > + boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
> > + return board;
> > + }
> > + }
> > + }
> > + return NULL;
>
> Why not use a dmi match table for the above code ?
I could use a dmi match table.
> > +}
> > +
> > +static int __init oxp_platform_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct device *hwdev;
> > + const struct ec_board_info *pboard_info;
> > + struct oxp_status *state;
> > +
> > + pboard_info = get_board_info(dev);
> > + if (!pboard_info)
> > + return -ENODEV;
> > +
> > + state = devm_kzalloc(dev, sizeof(struct oxp_status), GFP_KERNEL);
> > + if (!state)
> > + return -ENOMEM;
> > +
> > + state->board = *pboard_info;
> > +
> > + state->lock_data.mutex = 0;
> > + state->lock_data.lock = lock_global_acpi_lock;
> > + state->lock_data.unlock = unlock_global_acpi_lock;
> > +
> > + hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", state,
> > + &oxp_ec_chip_info, NULL);
> > +
> > + return PTR_ERR_OR_ZERO(hwdev);
>
> This only configures a hwmon driver and thus should reside in the hwmon
> subsystem/directory.
For now yes, it only provides hwmon. I'll move to it then.
> > +}
> > +
> > +static const struct acpi_device_id acpi_ec_ids[] = {
> > + /* Embedded Controller Device */
> > + { "PNP0C09", 0 },
> > + {}
> > +};
>
> I am not sure if this works. There are other drivers mapping to the same ACPI ID;
> those may refuse to load if this driver is in the system. We had problems with
> this before, so I am very concerned about side effects.
So should I just remove this and go for the DMI match table instead?
> > +
> > +static struct platform_driver oxp_platform_driver = {
> > + .driver = {
> > + .name = "oxp-platform",
> > + .acpi_match_table = acpi_ec_ids,
> > + },
> > +};
> > +
> > +MODULE_DEVICE_TABLE(acpi, acpi_ec_ids);
> > +module_platform_driver_probe(oxp_platform_driver, oxp_platform_probe);
> > +
> > +MODULE_AUTHOR("Joaquín Ignacio Aramendía <samsagax@xxxxxxxxx>");
> > +MODULE_DESCRIPTION(
> > + "Platform driver that handles ACPI EC of OneXPlayer devices");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.38.1
> >
>
Thanks very much for taking the time.
--
Joaquín I. Aramendía
