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

[Opensource [Steve Twiss]]

Thanks for your reply

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

>> From: Opensource [Steve Twiss] <stwiss.opensource@xxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxx>
>> ---
>>
>>  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