Hi Barnabás
On 16/11/2020 05:24, Barnabás Pőcze wrote:
Hi
2020. november 16., hétfő 0:04 keltezéssel, Mark Pearson írta:
[...]
+static ssize_t platform_profile_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int err, i;
+
+ mutex_lock(&profile_lock);
+ if (!cur_profile) {
+ mutex_unlock(&profile_lock);
+ return -EOPNOTSUPP;
I'm not sure why you modified the errno. ENODEV seemed to me like a perfectly
fine error to return if `cur_profile` is not set. `platform_profile_choices_show()`
returns ENODEV, so there is a bit of inconsistency.
(same applies to `platform_profile_show()`)
My thinking was it seemed a better message. I can't really see any
conditions when you'd get here (a driver would have registered a driver
and then not provided a profile?) but it seemed that if that was
possible it was more because changing the settings wasn't supported.
I managed to convince myself it made more sense - but have no issue with
changing it back.
+ }
+
+ /* Scan for a matching profile */
+ i = sysfs_match_string(profile_names, buf);
+ if (i < 0) {
+ mutex_unlock(&profile_lock);
+ return -EINVAL;
+ }
+
+ if (!cur_profile->profile_set) {
+ mutex_unlock(&profile_lock);
+ return -EOPNOTSUPP;
+ }
+
One thing I have just noticed is that I believe the selected profile should be
checked against `cur_profile->choices`, don't you think? I have assumed for
some reason that this check is done, and `profile_set` is only called with values
that the handler supports (they are in `choices`) up until now, but I see
that that's not what's happening.
True - I guess no harm adding the check in platform_profile.c too.
+ err = cur_profile->profile_set(i);
+ mutex_unlock(&profile_lock);
+ if (err)
+ return err;
+
+ return count;
+}
[...]
diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
new file mode 100644
index 000000000000..f6592434c8ce
--- /dev/null
+++ b/include/linux/platform_profile.h
@@ -0,0 +1,36 @@
[...]
By the way, I still feel like the enum values are "too vague" and should be
prefixed with `platform_`. If you're not opposed to that change.
Sure - I can change that. For me it made the names long but I don't mind
changing them.
Short and succinct names a good, but I hope you can see why I'm saying what I'm
saying. I believe these names should be "properly namespaced" since they are in
a "kernel-global" header file.
I do and I will update :)
Thanks for all the pointers
Mark
