Re: [PATCH v7 2/5] power: max77843_charger: Add Max77843 charger device driver

[Beomho Seo]

On 03/09/2015 09:13 PM, Krzysztof Kozlowski wrote:
> On pon, 2015-03-09 at 20:46 +0900, Beomho Seo wrote:
>> On 03/09/2015 08:02 PM, Krzysztof Kozlowski wrote:
>>> 2015-03-09 1:35 GMT+01:00 Beomho Seo <beomho.seo@xxxxxxxxxxx>:
>>>> On 03/08/2015 05:13 AM, Sebastian Reichel wrote:
>>>>> On Mon, Mar 02, 2015 at 07:10:35PM +0900, Jaewon Kim wrote:
>>>>>> From: Beomho Seo <beomho.seo@xxxxxxxxxxx>
>>>>>>
>>>>>> This patch adds device driver of max77843 charger. This driver provide
>>>>>> initialize each charging mode(e.g. fast charge, top-off mode and constant
>>>>>> charging mode so on.). Additionally, control charging paramters to use
>>>>>> i2c interface.
>>>>>>
>>>>>> Cc: Sebastian Reichel <sre@xxxxxxxxxx>
>>>>>> Signed-off-by: Beomho Seo <beomho.seo@xxxxxxxxxxx>
>>>>>
>>>>> Reviewed-By: Sebastian Reichel <sre@xxxxxxxxxx>
>>>>>
>>>>> I can't take it as is, since it depends on the private header file
>>>>> of PATCHv1.
>>>>>
>>>>> -- Sebastian
>>>>>
>>>>
>>>> This patch reviewed by Sebastian.
>>>> Could you Please merge that your git tree ?
>>>
>>> Hi,
>>>
>>> ... and again we are adding a new driver for very similar chipset to
>>> already supported. I looked at spec and the charger's registers are
>>> almost the same as for max77693. Their layout and addresses are the
>>> same. I see some minor differences, probably the most important would
>>> be different values current (fast-charge, top-off). But still 90% of
>>> registers are the same... Do we really have to add new driver?
>>>
>>> Best regards,
>>> Krzysztof
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-input" in
>>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
>> Hi,
>>
>> Thank you for your comment. As you say, both chip set are similar.
>> But new driver need for support max77843. It is support different below
>> - Provide Battery presence information.
> 
> Another set of power supply properties could be added for that chip.
> This way the get_property() function would be the same but actually the
> POWER_SUPPLY_PROP_PRESENT won't be called for max77693.
> 
>> - Can OTG FET control.
> 
> Where the OTG FET feature is it enabled in your driver? I couldn't find
> it.
> 

Sorry. This driver don't control OTG FET feature.

>> - Bigger Fast charge current, Top Off current Threshold selection.
>> - Various and bigger OTG current limitation.
>> - Bigger primary charger termination voltage setting.
>> - Different maximum input current limit selection(Different step).
> 
> Yes, I mentioned some of these differences (the Fast/top-off
> differences). These are differences in values so it does not require new
> driver. There is need to develop new driver just to support different
> current (3.0 A instead of 2.1 A) or voltage threshold.
> 

They are different charging current, OTG current limitation, top off current,
charging limitation value. In case OTG current limitation different not
limitation value but using register bit(max77843 use[7:6] max77693 use[7]
bit only). Even if this driver not support all feature, some register
different with max77693(support value, use register bit).

If this driver will combined with max77693 may even be beneficial for
new Maxim driver. But the present, this driver is related with
max77843 core driver and max77843-regulator. So I hope this driver
merge first. And then will extend two driver(max77843 charger and max77693 charger).

> So the only new feature - battery presence - can be easily added to
> existing driver. The driver can be extended for different
> current/voltage values. Such extension may even be beneficial for new
> Maxim MUIC/PMIC chipsets.
> 
> 
> Best regards,
> Krzysztof
> 

Best regards,
Beomho Seo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/