[PATCH] thinkpad_acpi: support new BIOS version string pattern

[PATCH] thinkpad_acpi: support new BIOS version string pattern

[Adam Lee]

Latest ThinkPad models use a new string pattern of BIOS version,
thinkpad_acpi won't be loaded automatically without this fix.

Signed-off-by: Adam Lee <adam.lee@canonical.com>
---
 drivers/platform/x86/thinkpad_acpi.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index c3d11fa..39a1017 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -8883,17 +8883,38 @@ static bool __pure __init tpacpi_is_fw_digit(const char c)
 	return (c >= '0' && c <= '9') || (c >= 'A' && c <= 'Z');
 }
 
-/* Most models: xxyTkkWW (#.##c); Ancient 570/600 and -SL lacks (#.##c) */
 static bool __pure __init tpacpi_is_valid_fw_id(const char * const s,
 						const char t)
 {
-	return s && strlen(s) >= 8 &&
+	bool is_most = 0;
+	bool is_new = 0;
+
+	/*
+	 * Most models: xxyTkkWW (#.##c)
+	 * Ancient 570/600 and -SL lacks (#.##c)
+	 */
+	is_most = s && strlen(s) >= 8 &&
 		tpacpi_is_fw_digit(s[0]) &&
 		tpacpi_is_fw_digit(s[1]) &&
 		s[2] == t &&
 		(s[3] == 'T' || s[3] == 'N') &&
 		tpacpi_is_fw_digit(s[4]) &&
 		tpacpi_is_fw_digit(s[5]);
+
+	if (is_most)
+		return is_most;
+
+	/* New models: xxxyTkkW (#.##c); T550 and some others */
+	is_new = s && strlen(s) >= 8 &&
+		tpacpi_is_fw_digit(s[0]) &&
+		tpacpi_is_fw_digit(s[1]) &&
+		tpacpi_is_fw_digit(s[2]) &&
+		s[3] == t &&
+		(s[4] == 'T' || s[4] == 'N') &&
+		tpacpi_is_fw_digit(s[5]) &&
+		tpacpi_is_fw_digit(s[6]);
+
+	return is_new;
 }
 
 /* returns 0 - probe ok, or < 0 - probe error.
-- 
2.1.4

Re: [PATCH] thinkpad_acpi: support new BIOS version string pattern

[Darren Hart]

On Wed, Feb 04, 2015 at 05:00:19PM +0800, Adam Lee wrote:
> Latest ThinkPad models use a new string pattern of BIOS version,
> thinkpad_acpi won't be loaded automatically without this fix.
> 

Hi Adam,

> Signed-off-by: Adam Lee <adam.lee@canonical.com>
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index c3d11fa..39a1017 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -8883,17 +8883,38 @@ static bool __pure __init tpacpi_is_fw_digit(const char c)
>  	return (c >= '0' && c <= '9') || (c >= 'A' && c <= 'Z');
>  }
>  
> -/* Most models: xxyTkkWW (#.##c); Ancient 570/600 and -SL lacks (#.##c) */
>  static bool __pure __init tpacpi_is_valid_fw_id(const char * const s,
>  						const char t)
>  {
> -	return s && strlen(s) >= 8 &&
> +	bool is_most = 0;
> +	bool is_new = 0;

The new variables aren't really necessary, you certainly do not need two of
them.

> +
> +	/*
> +	 * Most models: xxyTkkWW (#.##c)
> +	 * Ancient 570/600 and -SL lacks (#.##c)
> +	 */
> +	is_most = s && strlen(s) >= 8 &&

Try:

if (s && strlen(s) >=8 &&
	...
	)
	return true;


>  		tpacpi_is_fw_digit(s[0]) &&
>  		tpacpi_is_fw_digit(s[1]) &&
>  		s[2] == t &&
>  		(s[3] == 'T' || s[3] == 'N') &&
>  		tpacpi_is_fw_digit(s[4]) &&
>  		tpacpi_is_fw_digit(s[5]);
> +
> +	if (is_most)
> +		return is_most;
> +
> +	/* New models: xxxyTkkW (#.##c); T550 and some others */

Is the second W intentionally left off in xxxyTkkW ? We don't test for it, so I
wasn't sure.

> +	is_new = s && strlen(s) >= 8 &&
> +		tpacpi_is_fw_digit(s[0]) &&
> +		tpacpi_is_fw_digit(s[1]) &&
> +		tpacpi_is_fw_digit(s[2]) &&

As far as I can tell, the only significant difference here (compared to above)
is an additional leading digit and the subsequent string indices?

Seems to me this could be done fairly easily in the same block with only a minor
adjustment at the beginning and the use of an incrementing index. Should be
cleaner than duplicating 90% of the block?

> +		s[3] == t &&
> +		(s[4] == 'T' || s[4] == 'N') &&
> +		tpacpi_is_fw_digit(s[5]) &&
> +		tpacpi_is_fw_digit(s[6]);
> +
> +	return is_new;
>  }
>  
>  /* returns 0 - probe ok, or < 0 - probe error.
> -- 
> 2.1.4
> 
> 

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH] thinkpad_acpi: support new BIOS version string pattern

[Adam Lee]

On Mon, Feb 09, 2015 at 08:35:22PM -0800, Darren Hart wrote:
> >  static bool __pure __init tpacpi_is_valid_fw_id(const char * const s,
> >  						const char t)
> >  {
> > -	return s && strlen(s) >= 8 &&
> > +	bool is_most = 0;
> > +	bool is_new = 0;
> 
> The new variables aren't really necessary, you certainly do not need two of
> them.
> 
> > +
> > +	/*
> > +	 * Most models: xxyTkkWW (#.##c)
> > +	 * Ancient 570/600 and -SL lacks (#.##c)
> > +	 */
> > +	is_most = s && strlen(s) >= 8 &&
> 
> Try:
> 
> if (s && strlen(s) >=8 &&
> 	...
> 	)
> 	return true;

Yes, you are right.

> >  		tpacpi_is_fw_digit(s[0]) &&
> >  		tpacpi_is_fw_digit(s[1]) &&
> >  		s[2] == t &&
> >  		(s[3] == 'T' || s[3] == 'N') &&
> >  		tpacpi_is_fw_digit(s[4]) &&
> >  		tpacpi_is_fw_digit(s[5]);
> > +
> > +	if (is_most)
> > +		return is_most;
> > +
> > +	/* New models: xxxyTkkW (#.##c); T550 and some others */
> 
> Is the second W intentionally left off in xxxyTkkW ? We don't test for it, so I
> wasn't sure.

Yes, we have the latest models, tested it.

> > +	is_new = s && strlen(s) >= 8 &&
> > +		tpacpi_is_fw_digit(s[0]) &&
> > +		tpacpi_is_fw_digit(s[1]) &&
> > +		tpacpi_is_fw_digit(s[2]) &&
> 
> As far as I can tell, the only significant difference here (compared to above)
> is an additional leading digit and the subsequent string indices?
> 
> Seems to me this could be done fairly easily in the same block with only a minor
> adjustment at the beginning and the use of an incrementing index. Should be
> cleaner than duplicating 90% of the block?

I'd like to separate them still, it will be more readable and
extendible(Lenovo may release some BIOSes with other patterns for
non-classic ThinkPad models in the near future).

Thanks, Darren, will send the v2.

-- 
Adam Lee

[PATCH v2] thinkpad_acpi: support new BIOS version string pattern

[Adam Lee]

Latest ThinkPad models use a new string pattern of BIOS version,
thinkpad_acpi won't be loaded automatically without this fix.

Signed-off-by: Adam Lee <adam.lee@canonical.com>
---
 drivers/platform/x86/thinkpad_acpi.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index c3d11fa..18b7594 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -8883,17 +8883,31 @@ static bool __pure __init tpacpi_is_fw_digit(const char c)
 	return (c >= '0' && c <= '9') || (c >= 'A' && c <= 'Z');
 }
 
-/* Most models: xxyTkkWW (#.##c); Ancient 570/600 and -SL lacks (#.##c) */
 static bool __pure __init tpacpi_is_valid_fw_id(const char * const s,
 						const char t)
 {
-	return s && strlen(s) >= 8 &&
+	/*
+	 * Most models: xxyTkkWW (#.##c)
+	 * Ancient 570/600 and -SL lacks (#.##c)
+	 */
+	if (s && strlen(s) >= 8 &&
+			tpacpi_is_fw_digit(s[0]) &&
+			tpacpi_is_fw_digit(s[1]) &&
+			s[2] == t &&
+			(s[3] == 'T' || s[3] == 'N') &&
+			tpacpi_is_fw_digit(s[4]) &&
+			tpacpi_is_fw_digit(s[5]))
+		return true;
+
+	/* New models: xxxyTkkW (#.##c); T550 and some others */
+	return  s && strlen(s) >= 8 &&
 		tpacpi_is_fw_digit(s[0]) &&
 		tpacpi_is_fw_digit(s[1]) &&
-		s[2] == t &&
-		(s[3] == 'T' || s[3] == 'N') &&
-		tpacpi_is_fw_digit(s[4]) &&
-		tpacpi_is_fw_digit(s[5]);
+		tpacpi_is_fw_digit(s[2]) &&
+		s[3] == t &&
+		(s[4] == 'T' || s[4] == 'N') &&
+		tpacpi_is_fw_digit(s[5]) &&
+		tpacpi_is_fw_digit(s[6]);
 }
 
 /* returns 0 - probe ok, or < 0 - probe error.
-- 
2.1.4

Re: [PATCH v2] thinkpad_acpi: support new BIOS version string pattern

[Darren Hart]

On Wed, Feb 11, 2015 at 01:43:10PM +0800, Adam Lee wrote:
> Latest ThinkPad models use a new string pattern of BIOS version,
> thinkpad_acpi won't be loaded automatically without this fix.
> 
> Signed-off-by: Adam Lee <adam.lee@canonical.com>
> ---

When sending updated versions of a patch or series, please include a changelog
after the ---, see Documentation/SubmittingPatches.

>  drivers/platform/x86/thinkpad_acpi.c | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index c3d11fa..18b7594 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -8883,17 +8883,31 @@ static bool __pure __init tpacpi_is_fw_digit(const char c)
>  	return (c >= '0' && c <= '9') || (c >= 'A' && c <= 'Z');
>  }
>  
> -/* Most models: xxyTkkWW (#.##c); Ancient 570/600 and -SL lacks (#.##c) */
>  static bool __pure __init tpacpi_is_valid_fw_id(const char * const s,
>  						const char t)
>  {
> -	return s && strlen(s) >= 8 &&
> +	/*
> +	 * Most models: xxyTkkWW (#.##c)
> +	 * Ancient 570/600 and -SL lacks (#.##c)
> +	 */
> +	if (s && strlen(s) >= 8 &&
> +			tpacpi_is_fw_digit(s[0]) &&
> +			tpacpi_is_fw_digit(s[1]) &&
> +			s[2] == t &&
> +			(s[3] == 'T' || s[3] == 'N') &&
> +			tpacpi_is_fw_digit(s[4]) &&
> +			tpacpi_is_fw_digit(s[5]))

I'm not wild about these big boolean statements, but I also don't see something
that is obviously better. Typically we want to see indentation with tabs and
alignment with the initial line's arguments with spaces... but here that just
looks ugly as the too seemingly similar blocks end up with different
indentations due to the if and return... anyway, subjective and all that. So I'm
leaving it mostly alone, but I dropped one set of tabs from the above lines and
queued it.

Thanks Adam.

> +		return true;
> +
> +	/* New models: xxxyTkkW (#.##c); T550 and some others */
> +	return  s && strlen(s) >= 8 &&
>  		tpacpi_is_fw_digit(s[0]) &&
>  		tpacpi_is_fw_digit(s[1]) &&
> -		s[2] == t &&
> -		(s[3] == 'T' || s[3] == 'N') &&
> -		tpacpi_is_fw_digit(s[4]) &&
> -		tpacpi_is_fw_digit(s[5]);
> +		tpacpi_is_fw_digit(s[2]) &&
> +		s[3] == t &&
> +		(s[4] == 'T' || s[4] == 'N') &&
> +		tpacpi_is_fw_digit(s[5]) &&
> +		tpacpi_is_fw_digit(s[6]);



>  }
>  
>  /* returns 0 - probe ok, or < 0 - probe error.
> -- 
> 2.1.4
> 
> 

-- 
Darren Hart
Intel Open Source Technology Center