[PATCH 2/5] mfd: tc3589x: detect the precise version

[PATCH 2/5] mfd: tc3589x: detect the precise version

[Linus Walleij]

Instead of detecting the "tc3589x" and hard-coding the number of
GPIO pins to 24, encode all the possible subtypes and set the
number of GPIO pins from the type.

Signed-off-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/mfd/tc3589x.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 47 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/tc3589x.c b/drivers/mfd/tc3589x.c
index 70f4909f..0eabbb0 100644
--- a/drivers/mfd/tc3589x.c
+++ b/drivers/mfd/tc3589x.c
@@ -16,6 +16,19 @@
 #include <linux/mfd/core.h>
 #include <linux/mfd/tc3589x.h>
 
+/**
+ * enum tc3589x_version - indicates the TC3589x version
+ */
+enum tc3589x_version {
+	TC3589X_TC35890,
+	TC3589X_TC35892,
+	TC3589X_TC35893,
+	TC3589X_TC35894,
+	TC3589X_TC35895,
+	TC3589X_TC35896,
+	TC3589X_UNKNOWN,
+};
+
 #define TC3589x_CLKMODE_MODCTL_SLEEP		0x0
 #define TC3589x_CLKMODE_MODCTL_OPERATION	(1 << 0)
 
@@ -361,7 +374,33 @@ static int tc3589x_probe(struct i2c_client *i2c,
 	tc3589x->i2c = i2c;
 	tc3589x->pdata = pdata;
 	tc3589x->irq_base = pdata->irq_base;
-	tc3589x->num_gpio = id->driver_data;
+
+	switch (id->driver_data) {
+	case TC3589X_TC35890:
+		tc3589x->num_gpio = 24;
+		break;
+	case TC3589X_TC35892:
+		tc3589x->num_gpio = 24;
+		break;
+	case TC3589X_TC35893:
+		tc3589x->num_gpio = 20;
+		break;
+	case TC3589X_TC35894:
+		tc3589x->num_gpio = 24;
+		break;
+	case TC3589X_TC35895:
+		tc3589x->num_gpio = 20;
+		break;
+	case TC3589X_TC35896:
+		tc3589x->num_gpio = 20;
+		break;
+	case TC3589X_UNKNOWN:
+		tc3589x->num_gpio = 24;
+		break;
+	default:
+		tc3589x->num_gpio = 24;
+		break;
+	}
 
 	i2c_set_clientdata(i2c, tc3589x);
 
@@ -432,7 +471,13 @@ static int tc3589x_resume(struct device *dev)
 static SIMPLE_DEV_PM_OPS(tc3589x_dev_pm_ops, tc3589x_suspend, tc3589x_resume);
 
 static const struct i2c_device_id tc3589x_id[] = {
-	{ "tc3589x", 24 },
+	{ "tc35890", TC3589X_TC35890 },
+	{ "tc35892", TC3589X_TC35892 },
+	{ "tc35893", TC3589X_TC35893 },
+	{ "tc35894", TC3589X_TC35894 },
+	{ "tc35895", TC3589X_TC35895 },
+	{ "tc35896", TC3589X_TC35896 },
+	{ "tc3589x", TC3589X_UNKNOWN },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, tc3589x_id);
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] mfd: tc3589x: detect the precise version

[Lee Jones]

On Tue, 15 Oct 2013, Linus Walleij wrote:

> Instead of detecting the "tc3589x" and hard-coding the number of
> GPIO pins to 24, encode all the possible subtypes and set the
> number of GPIO pins from the type.
> 
> Signed-off-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/mfd/tc3589x.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 47 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mfd/tc3589x.c b/drivers/mfd/tc3589x.c
> index 70f4909f..0eabbb0 100644
> --- a/drivers/mfd/tc3589x.c
> +++ b/drivers/mfd/tc3589x.c
> @@ -16,6 +16,19 @@
>  #include <linux/mfd/core.h>
>  #include <linux/mfd/tc3589x.h>
>  
> +/**
> + * enum tc3589x_version - indicates the TC3589x version
> + */
> +enum tc3589x_version {
> +	TC3589X_TC35890,
> +	TC3589X_TC35892,
> +	TC3589X_TC35893,
> +	TC3589X_TC35894,
> +	TC3589X_TC35895,
> +	TC3589X_TC35896,
> +	TC3589X_UNKNOWN,
> +};
> +
>  #define TC3589x_CLKMODE_MODCTL_SLEEP		0x0
>  #define TC3589x_CLKMODE_MODCTL_OPERATION	(1 << 0)
>  
> @@ -361,7 +374,33 @@ static int tc3589x_probe(struct i2c_client *i2c,
>  	tc3589x->i2c = i2c;
>  	tc3589x->pdata = pdata;
>  	tc3589x->irq_base = pdata->irq_base;
> -	tc3589x->num_gpio = id->driver_data;
> +
> +	switch (id->driver_data) {
> +	case TC3589X_TC35890:
> +		tc3589x->num_gpio = 24;
> +		break;
> +	case TC3589X_TC35892:
> +		tc3589x->num_gpio = 24;
> +		break;
> +	case TC3589X_TC35893:
> +		tc3589x->num_gpio = 20;
> +		break;
> +	case TC3589X_TC35894:
> +		tc3589x->num_gpio = 24;
> +		break;
> +	case TC3589X_TC35895:
> +		tc3589x->num_gpio = 20;
> +		break;
> +	case TC3589X_TC35896:
> +		tc3589x->num_gpio = 20;
> +		break;
> +	case TC3589X_UNKNOWN:
> +		tc3589x->num_gpio = 24;
> +		break;
> +	default:
> +		tc3589x->num_gpio = 24;
> +		break;
> +	}
>  
>  	i2c_set_clientdata(i2c, tc3589x);
>  
> @@ -432,7 +471,13 @@ static int tc3589x_resume(struct device *dev)
>  static SIMPLE_DEV_PM_OPS(tc3589x_dev_pm_ops, tc3589x_suspend, tc3589x_resume);
>  
>  static const struct i2c_device_id tc3589x_id[] = {
> -	{ "tc3589x", 24 },
> +	{ "tc35890", TC3589X_TC35890 },
> +	{ "tc35892", TC3589X_TC35892 },
> +	{ "tc35893", TC3589X_TC35893 },
> +	{ "tc35894", TC3589X_TC35894 },
> +	{ "tc35895", TC3589X_TC35895 },
> +	{ "tc35896", TC3589X_TC35896 },
> +	{ "tc3589x", TC3589X_UNKNOWN },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, tc3589x_id);

This is an awful lot of code for what we're trying to achieve. I
propose two alternitives:

  1. I guess that other OSes will need to capture the same
  information, so wouldn't it be prudant to do so via a DT property?
  That way it's just a one or two liner.

Or:

  2. As we're only using .driver_data for num_gpio at this time, just
  place them in tc3589x_id as you have done for tc3589x.

Or, my least favourate alternitive, but still an improvment with
regards to saving code:

  3. Again, as all of this extra code is only used for pulling out
  num_gpio, group the devices by num_gpio:

> +	switch (id->driver_data) {
> +	case TC3589X_TC35893:
> +	case TC3589X_TC35895:
> +	case TC3589X_TC35896:
> +		tc3589x->num_gpio = 20;
> +		break;
> +	case TC3589X_TC35890:
> +	case TC3589X_TC35892:
> +	case TC3589X_TC35894:
> +	case TC3589X_UNKNOWN:
> +	default:
> +		tc3589x->num_gpio = 24;
> +		break;
> +	}

My personal preference is 1 and 2 in equal measure.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] mfd: tc3589x: detect the precise version

[Linus Walleij]

On Wed, Oct 16, 2013 at 11:23 AM, Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Tue, 15 Oct 2013, Linus Walleij wrote:

>>  static const struct i2c_device_id tc3589x_id[] = {
>> -     { "tc3589x", 24 },
>> +     { "tc35890", TC3589X_TC35890 },
>> +     { "tc35892", TC3589X_TC35892 },
>> +     { "tc35893", TC3589X_TC35893 },
>> +     { "tc35894", TC3589X_TC35894 },
>> +     { "tc35895", TC3589X_TC35895 },
>> +     { "tc35896", TC3589X_TC35896 },
>> +     { "tc3589x", TC3589X_UNKNOWN },
>>       { }
>>  };
>>  MODULE_DEVICE_TABLE(i2c, tc3589x_id);
>
> This is an awful lot of code for what we're trying to achieve.

I am not only trying to achive setting the number of GPIOs right,
I want the system to be aware of exactly which version of the chip
it is dealing with because they all have subtle differences, and
people will run into this in the future when adding more features.
http://www.toshiba-components.com/mobile/data/General_Presentation_IO_Expander_www_Jan_2012.pdf
See table on p.14 for example.

>   1. I guess that other OSes will need to capture the same
>   information, so wouldn't it be prudant to do so via a DT property?
>   That way it's just a one or two liner.

This patch is not about Device Tree. It is about probing from
the device name.

>   2. As we're only using .driver_data for num_gpio at this time, just
>   place them in tc3589x_id as you have done for tc3589x.

This doesn't scale to the other pecularities that differ and I
then just push the problem onto the next person dealing with
this chip.

>   3. Again, as all of this extra code is only used for pulling out
>   num_gpio, group the devices by num_gpio:
>
>> +     switch (id->driver_data) {
>> +     case TC3589X_TC35893:
>> +     case TC3589X_TC35895:
>> +     case TC3589X_TC35896:
>> +             tc3589x->num_gpio = 20;
>> +             break;
>> +     case TC3589X_TC35890:
>> +     case TC3589X_TC35892:
>> +     case TC3589X_TC35894:
>> +     case TC3589X_UNKNOWN:
>> +     default:
>> +             tc3589x->num_gpio = 24;
>> +             break;
>> +     }

This I can do. The switch tree can then be extended as need
be in the future.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] mfd: tc3589x: detect the precise version

[Lee Jones]

Whoops, read this after.

Fair enough.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog