[RFC V1] mfd: da9063: Add support for production silicon variant code

[RFC V1] mfd: da9063: Add support for production silicon variant code

[Opensource [Steve Twiss]]

From: Opensource [Steve Twiss] <stwiss.opensource@diasemi.com>

Add the correct silicon variant code ID (0x5) to the driver. This
new code is the 'production' variant code ID for DA9063.
 
This patch will remove the older variant code ID which matches the
pre-production silicon ID (0x3) for the DA9063 chip.

There is also some small amount of correction done in this patch: it
renames various incorrectly named variables and moves the dev_info()
call before the variant ID test.

Signed-off-by: Opensource [Steve Twiss] <stwiss.opensource@diasemi.com>
---
Checks performed with next-20140213/scripts/checkpatch.pl
 da9063-core.c             total: 0 errors, 0 warnings, 189 lines checked
 core.h                    total: 0 errors, 0 warnings, 98 lines checked

This patch applies against kernel version linux-next next-20140213

These changes are meant to be a pre-cursor to the upcoming patch set to 
support the new silicon revision.

The previous test silicon (0x3) was only sent out in sample form and is
no longer produced by Dialog Semiconductor Ltd.

Samples of this chip pre-production chip were supplied under the proviso
that the production silicon would replace this test variant. The new
silicon variant code (0x5) correctly identifies the new production
silicon revision for the DA9063 PMIC

Regards,
Steve Twiss, Dialog Semiconductor Ltd.



 drivers/mfd/da9063-core.c       | 36 ++++++++++++++++++++----------------
 include/linux/mfd/da9063/core.h |  7 ++++++-
 2 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
index 26937cd..80ce35a 100644
--- a/drivers/mfd/da9063-core.c
+++ b/drivers/mfd/da9063-core.c
@@ -1,3 +1,4 @@
+
 /*
  * da9063-core.c: Device access for Dialog DA9063 modules
  *
@@ -110,7 +111,7 @@ static const struct mfd_cell da9063_devs[] = {
 int da9063_device_init(struct da9063 *da9063, unsigned int irq)
 {
 	struct da9063_pdata *pdata = da9063->dev->platform_data;
-	int model, revision;
+	int chip_id, variant_id, variant_code;
 	int ret;
 
 	if (pdata) {
@@ -131,33 +132,36 @@ int da9063_device_init(struct da9063 *da9063, unsigned int irq)
 		}
 	}
 
-	ret = regmap_read(da9063->regmap, DA9063_REG_CHIP_ID, &model);
+	ret = regmap_read(da9063->regmap, DA9063_REG_CHIP_ID, &chip_id);
 	if (ret < 0) {
-		dev_err(da9063->dev, "Cannot read chip model id.\n");
+		dev_err(da9063->dev, "Cannot read chip id.\n");
 		return -EIO;
 	}
-	if (model != PMIC_DA9063) {
-		dev_err(da9063->dev, "Invalid chip model id: 0x%02x\n", model);
+	if (chip_id != PMIC_DA9063) {
+		dev_err(da9063->dev, "Invalid chip id: 0x%02x\n", chip_id);
 		return -ENODEV;
 	}
 
-	ret = regmap_read(da9063->regmap, DA9063_REG_CHIP_VARIANT, &revision);
+	ret = regmap_read(da9063->regmap, DA9063_REG_CHIP_VARIANT, &variant_id);
 	if (ret < 0) {
-		dev_err(da9063->dev, "Cannot read chip revision id.\n");
+		dev_err(da9063->dev, "Cannot read chip variant id.\n");
 		return -EIO;
 	}
-	revision >>= DA9063_CHIP_VARIANT_SHIFT;
-	if (revision != 3) {
-		dev_err(da9063->dev, "Unknown chip revision: %d\n", revision);
-		return -ENODEV;
-	}
 
-	da9063->model = model;
-	da9063->revision = revision;
+	variant_code = variant_id >> DA9063_CHIP_VARIANT_SHIFT;
 
 	dev_info(da9063->dev,
-		 "Device detected (model-ID: 0x%02X  rev-ID: 0x%02X)\n",
-		 model, revision);
+		 "Device detected (chip-ID: 0x%02X, var-ID: 0x%02X)\n",
+		 chip_id, variant_id);
+
+	if (variant_code != PMIC_DA9063_BB) {
+		dev_err(da9063->dev, "Unknown chip variant code: 0x%02X\n",
+				variant_code);
+		return -ENODEV;
+	}
+
+	da9063->model = chip_id;
+	da9063->variant_code = variant_code;
 
 	ret = da9063_irq_init(da9063);
 	if (ret) {
diff --git a/include/linux/mfd/da9063/core.h b/include/linux/mfd/da9063/core.h
index 2d2a0af..2265ccb 100644
--- a/include/linux/mfd/da9063/core.h
+++ b/include/linux/mfd/da9063/core.h
@@ -1,3 +1,4 @@
+
 /*
  * Definitions for DA9063 MFD driver
  *
@@ -33,6 +34,10 @@ enum da9063_models {
 	PMIC_DA9063 = 0x61,
 };
 
+enum da9063_variant_codes {
+	PMIC_DA9063_BB = 0x5
+};
+
 /* Interrupts */
 enum da9063_irqs {
 	DA9063_IRQ_ONKEY = 0,
@@ -72,7 +77,7 @@ struct da9063 {
 	/* Device */
 	struct device	*dev;
 	unsigned short	model;
-	unsigned short	revision;
+	unsigned char	variant_code;
 	unsigned int	flags;
 
 	/* Control interface */
-- 
end-of-patch for RFC V1

Re: [RFC V1] mfd: da9063: Add support for production silicon variant code

[Lee Jones]

> From: Opensource [Steve Twiss] <stwiss.opensource@diasemi.com>
> 
> Add the correct silicon variant code ID (0x5) to the driver. This
> new code is the 'production' variant code ID for DA9063.
>  
> This patch will remove the older variant code ID which matches the
> pre-production silicon ID (0x3) for the DA9063 chip.
> 
> There is also some small amount of correction done in this patch: it
> renames various incorrectly named variables and moves the dev_info()
> call before the variant ID test.
> 
> Signed-off-by: Opensource [Steve Twiss] <stwiss.opensource@diasemi.com>
> ---
> 
>  drivers/mfd/da9063-core.c       | 36 ++++++++++++++++++++----------------
>  include/linux/mfd/da9063/core.h |  7 ++++++-
>  2 files changed, 26 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
> index 26937cd..80ce35a 100644
> --- a/drivers/mfd/da9063-core.c
> +++ b/drivers/mfd/da9063-core.c
> @@ -1,3 +1,4 @@
> +

Unnecessary new line.

<snip>

> +	da9063->model = chip_id;

Why have you gone to lengths to rename 'model' to 'chip_id' locally,
but still call it 'model' in the global container?

> diff --git a/include/linux/mfd/da9063/core.h b/include/linux/mfd/da9063/core.h
> index 2d2a0af..2265ccb 100644
> --- a/include/linux/mfd/da9063/core.h
> +++ b/include/linux/mfd/da9063/core.h
> @@ -1,3 +1,4 @@
> +

Remove this.

>  /*
>   * Definitions for DA9063 MFD driver
>   *
> @@ -33,6 +34,10 @@ enum da9063_models {
>  	PMIC_DA9063 = 0x61,
>  };
>  
> +enum da9063_variant_codes {
> +	PMIC_DA9063_BB = 0x5

Why not support both? It's only an extra few chars in the if().

> +};
> +
>  /* Interrupts */
>  enum da9063_irqs {
>  	DA9063_IRQ_ONKEY = 0,
> @@ -72,7 +77,7 @@ struct da9063 {
>  	/* Device */
>  	struct device	*dev;
>  	unsigned short	model;

Don't you want to change this to chip_id?

> -	unsigned short	revision;
> +	unsigned char	variant_code;
>  	unsigned int	flags;
>  
>  	/* Control interface */

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

RE: [RFC V1] mfd: da9063: Add support for production silicon variant code

[Opensource [Steve Twiss]]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3607 bytes --]

Thanks for your reply

On  14 February 2014 09:35 Lee Jones [mailto:lee.jones@linaro.org] wrote:

>> From: Opensource [Steve Twiss] <stwiss.opensource@diasemi.com>
>>
>> Add the correct silicon variant code ID (0x5) to the driver. This
>> new code is the 'production' variant code ID for DA9063.
>>
>> This patch will remove the older variant code ID which matches the
>> pre-production silicon ID (0x3) for the DA9063 chip.
>>
>> There is also some small amount of correction done in this patch: it
>> renames various incorrectly named variables and moves the dev_info()
>> call before the variant ID test.
>>
>> Signed-off-by: Opensource [Steve Twiss] <stwiss.opensource@diasemi.com>
>> ---
>>
>>  drivers/mfd/da9063-core.c       | 36 ++++++++++++++++++++----------------
>>  include/linux/mfd/da9063/core.h |  7 ++++++-
>>  2 files changed, 26 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
>> index 26937cd..80ce35a 100644
>> --- a/drivers/mfd/da9063-core.c
>> +++ b/drivers/mfd/da9063-core.c
>> @@ -1,3 +1,4 @@
>> +
>
>Unnecessary new line.
>
><snip>
>

Thanks! I spotted that after I set it yesterday 
It's gone in my latest patch.

>> +	da9063->model = chip_id;
>
>Why have you gone to lengths to rename 'model' to 'chip_id' locally,
>but still call it 'model' in the global container?
>

It's just a style change so naming convention matches the content I get
from the H/W engineers -- I was going to change this globally when I made 
the next set of changes, but I didn't add it here because I would have to
change the rest of the code and I just wanted to concentrate on the chip
variant detection with this patch.

>> diff --git a/include/linux/mfd/da9063/core.h
>b/include/linux/mfd/da9063/core.h
>> index 2d2a0af..2265ccb 100644
>> --- a/include/linux/mfd/da9063/core.h
>> +++ b/include/linux/mfd/da9063/core.h
>> @@ -1,3 +1,4 @@
>> +
>
>Remove this.
>

And again :(
I'm sorry you had to review that part.

>>  /*
>>   * Definitions for DA9063 MFD driver
>>   *
>> @@ -33,6 +34,10 @@ enum da9063_models {
>>  	PMIC_DA9063 = 0x61,
>>  };
>>
>> +enum da9063_variant_codes {
>> +	PMIC_DA9063_BB = 0x5
>
>Why not support both? It's only an extra few chars in the if().
>

Yep, it is only a small change -- but the implication of keeping support for the
previous silicon variant is fairly large at this point.

The previous silicon was only sent out in sample form to selected customers
and will no longer be available. I have been informed that the new silicon
has been sent out, and everybody should have received the new variant by
now. 

The new variant is the only one going into production and the previous silicon
variant will no longer be available. Also, the production silicon of DA9063
makes an alteration to the registers which makes the two register maps
incompatible.

These were known changes.

So, for those two reasons. I would prefer to remove support for the old variant
from the kernel.

>> +};
>> +
>>  /* Interrupts */
>>  enum da9063_irqs {
>>  	DA9063_IRQ_ONKEY = 0,
>> @@ -72,7 +77,7 @@ struct da9063 {
>>  	/* Device */
>>  	struct device	*dev;
>>  	unsigned short	model;
>
>Don't you want to change this to chip_id?
>

Yep, I will .. same reason as above.

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

Thanks for your comments.

Regards,
Steve

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [RFC V1] mfd: da9063: Add support for production silicon variant code

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1150 bytes --]

On Fri, Feb 14, 2014 at 10:28:38AM +0000, Opensource [Steve Twiss] wrote:

> The previous silicon was only sent out in sample form to selected customers
> and will no longer be available. I have been informed that the new silicon
> has been sent out, and everybody should have received the new variant by
> now. 

> The new variant is the only one going into production and the previous silicon
> variant will no longer be available. Also, the production silicon of DA9063
> makes an alteration to the registers which makes the two register maps
> incompatible.

The usual thing if it's straightforward to do so is to keep support for
old versions even if the early revisions are not expected to be widely
available.  We do fairly often see problems with people still using old
boards for various reasons - the fact that new silicon is available does
not mean that the old silicon has been retired by users (even if just
for comparison purposes while new board revisions are being brought up -
"that worked on the rev A boards, did we break the software?") or that
the old silicon isn't sitting in an assembly pipeline somewhere.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [RFC V1] mfd: da9063: Add support for production silicon variant code

[Opensource [Steve Twiss]]

On 14 February 2014 14:57, Mark Brown [mailto:broonie@kernel.org] wrote:

>On Fri, Feb 14, 2014 at 10:28:38AM +0000, Opensource [Steve Twiss] wrote:
>
>> The previous silicon was only sent out in sample form to selected customers
>> and will no longer be available. I have been informed that the new silicon
>> has been sent out, and everybody should have received the new variant by
>> now.
>
>> The new variant is the only one going into production and the previous silicon
>> variant will no longer be available. Also, the production silicon of DA9063
>> makes an alteration to the registers which makes the two register maps
>> incompatible.

My apologies for not replying to this in time -- it appeared in my inbox after
I had sent RFC V2.

>
>The usual thing if it's straightforward to do so is to keep support for
>old versions even if the early revisions are not expected to be widely
>available.

Yes, I take your point here.

Some registers in the new variant make backwards compatibility easy,
however attempting to combine the two register maps in other components
makes the driver a mess. 

By drawing the line in the probe() function I am requesting that there be no
support for older silicon. I have been requested to do this because that is 
the line that Dialog Semiconductor Ltd wants to impose -- and this is not
just at the level of the S/W driver.

I realise that the Linux community will have different aims however and 
I would like to support those as much as I can.

> We do fairly often see problems with people still using old
>boards for various reasons - the fact that new silicon is available does
>not mean that the old silicon has been retired by users (even if just
>for comparison purposes while new board revisions are being brought up -
>"that worked on the rev A boards, did we break the software?") 

I see -- yes.

I don't see any straightforward way to support some of the components
in parallel, and merging the registers.h file for both variants does make a
mess even before reaching the driver code.

I'm not sure how easy this will be, but if I can retain support for some of
the older variant features already in the kernel I will try to do so during
my upcoming submissions.

Regards,
Steve

Re: [RFC V1] mfd: da9063: Add support for production silicon variant code

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 696 bytes --]

On Sun, Feb 16, 2014 at 12:18:43PM +0000, Opensource [Steve Twiss] wrote:

> Some registers in the new variant make backwards compatibility easy,
> however attempting to combine the two register maps in other components
> makes the driver a mess. 

OK, that makes sense - if the changes really are substantial then
dropping the old revisions can be reasonable.

> I realise that the Linux community will have different aims however and 
> I would like to support those as much as I can.

It's not just the community here - it's just a general thing with users,
the same issues face people using out of tree drivers (though
communication with the developers is often weaker there).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]