Re: [PATCH] Add OneXPlayer mini AMD board driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux