Re: [External] Re: [PATCH v3] ACPI: platform-profile: Add platform profile support

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

 



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



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

  Powered by Linux