* Re: [PATCH v4 1/4] platform/x86: think-lmi: add missing type attribute [not found] <20230320003221.561750-1-mpearson-lenovo@squebb.ca> @ 2023-03-20 13:56 ` Hans de Goede 2023-03-20 13:57 ` Hans de Goede [not found] ` <20230320003221.561750-2-mpearson-lenovo@squebb.ca> 1 sibling, 1 reply; 5+ messages in thread From: Hans de Goede @ 2023-03-20 13:56 UTC (permalink / raw) To: Mark Pearson, Thomas Weißschuh Cc: markgross, markpearson, thomas, pobrn, linux-kernel, platform-driver-x86 Hi Mark, It seems that while adding linux-kernel@vger.kernel.org you have dropped platform-driver-x86@vger.kernel.org. For future patches please Cc both. platform-driver-x86@vger.kernel.org needs to be Cc-ed for patches to show up in patchwork: https://patchwork.kernel.org/project/platform-driver-x86/list/ Which I use as my primary means of tracking which patches I need to review / merge (note no need to resend this series I have it on my radar). More importantly you seem to not have Cc-ed Thomas Weißschuh on this version ? I have added Thomas to the Cc now, so that he can respond to this version. Regards, Hans On 3/20/23 01:32, Mark Pearson wrote: > This driver was missing the mandatory type attribute...oops. > > Add it in along with logic to determine whether the attribute is an > enumeration type or a string by parsing the possible_values attribute. > > Upstream bug https://bugzilla.kernel.org/show_bug.cgi?id=216460 > > Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> > --- > Changes in v4: > - Unchanged. Sending to linux-kernel mailing list as recommended > Changes in v3: > - Rebased on latest pdx86, review_hans branch > Changes in v2: > - Simplify the code and move type determination into show function > - Don't use Fixes with URL in commit info > > drivers/platform/x86/think-lmi.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c > index cc66f7cbccf2..a765bf8c27d8 100644 > --- a/drivers/platform/x86/think-lmi.c > +++ b/drivers/platform/x86/think-lmi.c > @@ -947,6 +947,20 @@ static ssize_t possible_values_show(struct kobject *kobj, struct kobj_attribute > return sysfs_emit(buf, "%s\n", setting->possible_values); > } > > +static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr, > + char *buf) > +{ > + struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj); > + > + if (setting->possible_values) { > + /* Figure out what setting type is as BIOS does not return this */ > + if (strchr(setting->possible_values, ',')) > + return sysfs_emit(buf, "enumeration\n"); > + } > + /* Anything else is going to be a string */ > + return sysfs_emit(buf, "string\n"); > +} > + > static ssize_t current_value_store(struct kobject *kobj, > struct kobj_attribute *attr, > const char *buf, size_t count) > @@ -1036,10 +1050,13 @@ static struct kobj_attribute attr_possible_values = __ATTR_RO(possible_values); > > static struct kobj_attribute attr_current_val = __ATTR_RW_MODE(current_value, 0600); > > +static struct kobj_attribute attr_type = __ATTR_RO(type); > + > static struct attribute *tlmi_attrs[] = { > &attr_displ_name.attr, > &attr_current_val.attr, > &attr_possible_values.attr, > + &attr_type.attr, > NULL > }; > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4 1/4] platform/x86: think-lmi: add missing type attribute 2023-03-20 13:56 ` [PATCH v4 1/4] platform/x86: think-lmi: add missing type attribute Hans de Goede @ 2023-03-20 13:57 ` Hans de Goede 2023-03-20 13:58 ` Mark Pearson 0 siblings, 1 reply; 5+ messages in thread From: Hans de Goede @ 2023-03-20 13:57 UTC (permalink / raw) To: Mark Pearson, Thomas Weißschuh Cc: markgross, markpearson, thomas, pobrn, linux-kernel, platform-driver-x86 Hi, On 3/20/23 14:56, Hans de Goede wrote: > Hi Mark, > > It seems that while adding linux-kernel@vger.kernel.org you have dropped > platform-driver-x86@vger.kernel.org. For future patches please Cc both. > > platform-driver-x86@vger.kernel.org needs to be Cc-ed for patches > to show up in patchwork: > https://patchwork.kernel.org/project/platform-driver-x86/list/ > > Which I use as my primary means of tracking which patches I need > to review / merge (note no need to resend this series I have it > on my radar). > > More importantly you seem to not have Cc-ed > Thomas Weißschuh on this version ? Never mind, I just noticed that Thomas has 2 email addresses and you did Cc their other address... Regards, Hans > On 3/20/23 01:32, Mark Pearson wrote: >> This driver was missing the mandatory type attribute...oops. >> >> Add it in along with logic to determine whether the attribute is an >> enumeration type or a string by parsing the possible_values attribute. >> >> Upstream bug https://bugzilla.kernel.org/show_bug.cgi?id=216460 >> >> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> >> --- >> Changes in v4: >> - Unchanged. Sending to linux-kernel mailing list as recommended >> Changes in v3: >> - Rebased on latest pdx86, review_hans branch >> Changes in v2: >> - Simplify the code and move type determination into show function >> - Don't use Fixes with URL in commit info >> >> drivers/platform/x86/think-lmi.c | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) >> >> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c >> index cc66f7cbccf2..a765bf8c27d8 100644 >> --- a/drivers/platform/x86/think-lmi.c >> +++ b/drivers/platform/x86/think-lmi.c >> @@ -947,6 +947,20 @@ static ssize_t possible_values_show(struct kobject *kobj, struct kobj_attribute >> return sysfs_emit(buf, "%s\n", setting->possible_values); >> } >> >> +static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr, >> + char *buf) >> +{ >> + struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj); >> + >> + if (setting->possible_values) { >> + /* Figure out what setting type is as BIOS does not return this */ >> + if (strchr(setting->possible_values, ',')) >> + return sysfs_emit(buf, "enumeration\n"); >> + } >> + /* Anything else is going to be a string */ >> + return sysfs_emit(buf, "string\n"); >> +} >> + >> static ssize_t current_value_store(struct kobject *kobj, >> struct kobj_attribute *attr, >> const char *buf, size_t count) >> @@ -1036,10 +1050,13 @@ static struct kobj_attribute attr_possible_values = __ATTR_RO(possible_values); >> >> static struct kobj_attribute attr_current_val = __ATTR_RW_MODE(current_value, 0600); >> >> +static struct kobj_attribute attr_type = __ATTR_RO(type); >> + >> static struct attribute *tlmi_attrs[] = { >> &attr_displ_name.attr, >> &attr_current_val.attr, >> &attr_possible_values.attr, >> + &attr_type.attr, >> NULL >> }; >> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4 1/4] platform/x86: think-lmi: add missing type attribute 2023-03-20 13:57 ` Hans de Goede @ 2023-03-20 13:58 ` Mark Pearson 0 siblings, 0 replies; 5+ messages in thread From: Mark Pearson @ 2023-03-20 13:58 UTC (permalink / raw) To: Hans de Goede, Thomas Weißschuh Cc: markgross, Mark Pearson, Thomas Weißschuh, Barnabás Pőcze, linux-kernel, platform-driver-x86 On Mon, Mar 20, 2023, at 9:57 AM, Hans de Goede wrote: > Hi, > > On 3/20/23 14:56, Hans de Goede wrote: >> Hi Mark, >> >> It seems that while adding linux-kernel@vger.kernel.org you have dropped >> platform-driver-x86@vger.kernel.org. For future patches please Cc both. >> >> platform-driver-x86@vger.kernel.org needs to be Cc-ed for patches >> to show up in patchwork: >> https://patchwork.kernel.org/project/platform-driver-x86/list/ >> >> Which I use as my primary means of tracking which patches I need >> to review / merge (note no need to resend this series I have it >> on my radar). >> >> More importantly you seem to not have Cc-ed >> Thomas Weißschuh on this version ? > > Never mind, I just noticed that Thomas has 2 email addresses > and you did Cc their other address... Yep - and no idea how I missed pdx86 mailing list....must have been a copy paste error :( Sorry - not deliberate. Mark > > Regards, > > Hans > > > >> On 3/20/23 01:32, Mark Pearson wrote: >>> This driver was missing the mandatory type attribute...oops. >>> >>> Add it in along with logic to determine whether the attribute is an >>> enumeration type or a string by parsing the possible_values attribute. >>> >>> Upstream bug https://bugzilla.kernel.org/show_bug.cgi?id=216460 >>> >>> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> >>> --- >>> Changes in v4: >>> - Unchanged. Sending to linux-kernel mailing list as recommended >>> Changes in v3: >>> - Rebased on latest pdx86, review_hans branch >>> Changes in v2: >>> - Simplify the code and move type determination into show function >>> - Don't use Fixes with URL in commit info >>> >>> drivers/platform/x86/think-lmi.c | 17 +++++++++++++++++ >>> 1 file changed, 17 insertions(+) >>> >>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c >>> index cc66f7cbccf2..a765bf8c27d8 100644 >>> --- a/drivers/platform/x86/think-lmi.c >>> +++ b/drivers/platform/x86/think-lmi.c >>> @@ -947,6 +947,20 @@ static ssize_t possible_values_show(struct kobject *kobj, struct kobj_attribute >>> return sysfs_emit(buf, "%s\n", setting->possible_values); >>> } >>> >>> +static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr, >>> + char *buf) >>> +{ >>> + struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj); >>> + >>> + if (setting->possible_values) { >>> + /* Figure out what setting type is as BIOS does not return this */ >>> + if (strchr(setting->possible_values, ',')) >>> + return sysfs_emit(buf, "enumeration\n"); >>> + } >>> + /* Anything else is going to be a string */ >>> + return sysfs_emit(buf, "string\n"); >>> +} >>> + >>> static ssize_t current_value_store(struct kobject *kobj, >>> struct kobj_attribute *attr, >>> const char *buf, size_t count) >>> @@ -1036,10 +1050,13 @@ static struct kobj_attribute attr_possible_values = __ATTR_RO(possible_values); >>> >>> static struct kobj_attribute attr_current_val = __ATTR_RW_MODE(current_value, 0600); >>> >>> +static struct kobj_attribute attr_type = __ATTR_RO(type); >>> + >>> static struct attribute *tlmi_attrs[] = { >>> &attr_displ_name.attr, >>> &attr_current_val.attr, >>> &attr_possible_values.attr, >>> + &attr_type.attr, >>> NULL >>> }; >>> ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <20230320003221.561750-2-mpearson-lenovo@squebb.ca>]
* Re: [PATCH v4 2/4] platform/x86: think-lmi: use correct possible_values delimiters [not found] ` <20230320003221.561750-2-mpearson-lenovo@squebb.ca> @ 2023-03-20 15:22 ` Thomas Weißschuh 2023-03-22 14:22 ` Hans de Goede 0 siblings, 1 reply; 5+ messages in thread From: Thomas Weißschuh @ 2023-03-20 15:22 UTC (permalink / raw) To: Mark Pearson Cc: hdegoede, markgross, markpearson, pobrn, linux-kernel, platform-driver-x86 Hi Mark, Thanks for the series! For all of it: Reviewed-by: Thomas Weißschuh <linux@weissschuh.net> If you decide to reroll it, one more nitpick below. On Sun, Mar 19, 2023 at 08:32:19PM -0400, Mark Pearson wrote: > firmware-attributes class requires that possible values are delimited > using ';' but the Lenovo firmware uses ',' instead. > Parse string and replace where appropriate. > > Suggested-by: Thomas Weißschuh <linux@weissschuh.net> > Fixes: a40cd7ef22fb ("platform/x86: think-lmi: Add WMI interface support on Lenovo platforms") > Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> > --- > Changes in v4 > - Moved earlier in the series as recommended > - used strreplace function as recommended > Changes in v3: > - New patch added to the series. No v1 & v2. > > drivers/platform/x86/think-lmi.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c > index a765bf8c27d8..53f34b1adb8c 100644 > --- a/drivers/platform/x86/think-lmi.c > +++ b/drivers/platform/x86/think-lmi.c > @@ -954,7 +954,7 @@ static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr, > > if (setting->possible_values) { > /* Figure out what setting type is as BIOS does not return this */ > - if (strchr(setting->possible_values, ',')) > + if (strchr(setting->possible_values, ';')) > return sysfs_emit(buf, "enumeration\n"); If you make this patch the first on of the series it would * make the hunk above unnecessary. * make it easier to backport if somebody wants do do so. * make the then second patch easier to read as it would not introduce "incorrect" code that needs a fix-up in the following commit. > } > /* Anything else is going to be a string */ > @@ -1413,6 +1413,13 @@ static int tlmi_analyze(void) > pr_info("Error retrieving possible values for %d : %s\n", > i, setting->display_name); > } > + /* > + * firmware-attributes requires that possible_values are separated by ';' but > + * Lenovo FW uses ','. Replace appropriately. > + */ > + if (setting->possible_values) > + strreplace(setting->possible_values, ',', ';'); > + > kobject_init(&setting->kobj, &tlmi_attr_setting_ktype); > tlmi_priv.setting[i] = setting; > kfree(item); > -- > 2.39.2 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4 2/4] platform/x86: think-lmi: use correct possible_values delimiters 2023-03-20 15:22 ` [PATCH v4 2/4] platform/x86: think-lmi: use correct possible_values delimiters Thomas Weißschuh @ 2023-03-22 14:22 ` Hans de Goede 0 siblings, 0 replies; 5+ messages in thread From: Hans de Goede @ 2023-03-22 14:22 UTC (permalink / raw) To: Thomas Weißschuh, Mark Pearson Cc: markgross, markpearson, pobrn, linux-kernel, platform-driver-x86 Hi, On 3/20/23 16:22, Thomas Weißschuh wrote: > Hi Mark, > > Thanks for the series! > For all of it: > > Reviewed-by: Thomas Weißschuh <linux@weissschuh.net> > > If you decide to reroll it, one more nitpick below. Mark, thank you for the series. Thomas, thank you for the reviews. I've applied this series to the fixes branch now: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=fixes Note it will show up in my fixes branch once I've pushed my local branch there, which might take a while. I will include this patch in my next fixes pull-req to Linus for the current kernel development cycle. Regards, Hans > On Sun, Mar 19, 2023 at 08:32:19PM -0400, Mark Pearson wrote: >> firmware-attributes class requires that possible values are delimited >> using ';' but the Lenovo firmware uses ',' instead. >> Parse string and replace where appropriate. >> >> Suggested-by: Thomas Weißschuh <linux@weissschuh.net> >> Fixes: a40cd7ef22fb ("platform/x86: think-lmi: Add WMI interface support on Lenovo platforms") >> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> >> --- >> Changes in v4 >> - Moved earlier in the series as recommended >> - used strreplace function as recommended >> Changes in v3: >> - New patch added to the series. No v1 & v2. >> >> drivers/platform/x86/think-lmi.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c >> index a765bf8c27d8..53f34b1adb8c 100644 >> --- a/drivers/platform/x86/think-lmi.c >> +++ b/drivers/platform/x86/think-lmi.c >> @@ -954,7 +954,7 @@ static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr, >> >> if (setting->possible_values) { >> /* Figure out what setting type is as BIOS does not return this */ >> - if (strchr(setting->possible_values, ',')) >> + if (strchr(setting->possible_values, ';')) >> return sysfs_emit(buf, "enumeration\n"); > > If you make this patch the first on of the series it would > * make the hunk above unnecessary. > * make it easier to backport if somebody wants do do so. > * make the then second patch easier to read as it would not introduce > "incorrect" code that needs a fix-up in the following commit. > >> } >> /* Anything else is going to be a string */ >> @@ -1413,6 +1413,13 @@ static int tlmi_analyze(void) >> pr_info("Error retrieving possible values for %d : %s\n", >> i, setting->display_name); >> } >> + /* >> + * firmware-attributes requires that possible_values are separated by ';' but >> + * Lenovo FW uses ','. Replace appropriately. >> + */ >> + if (setting->possible_values) >> + strreplace(setting->possible_values, ',', ';'); >> + >> kobject_init(&setting->kobj, &tlmi_attr_setting_ktype); >> tlmi_priv.setting[i] = setting; >> kfree(item); >> -- >> 2.39.2 >> > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-03-22 14:23 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20230320003221.561750-1-mpearson-lenovo@squebb.ca> 2023-03-20 13:56 ` [PATCH v4 1/4] platform/x86: think-lmi: add missing type attribute Hans de Goede 2023-03-20 13:57 ` Hans de Goede 2023-03-20 13:58 ` Mark Pearson [not found] ` <20230320003221.561750-2-mpearson-lenovo@squebb.ca> 2023-03-20 15:22 ` [PATCH v4 2/4] platform/x86: think-lmi: use correct possible_values delimiters Thomas Weißschuh 2023-03-22 14:22 ` Hans de Goede
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).