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

[Lee Jones <lee.jones@linaro.org>]

On Fri, 27 Mar 2015, Beomho Seo wrote:

> On 03/27/2015 04:57 PM, Lee Jones wrote:
> > On Fri, 27 Mar 2015, Beomho Seo wrote:
> >> On 03/26/2015 10:54 PM, Lee Jones wrote:
> >>> On Thu, 26 Mar 2015, Beomho Seo wrote:
> >>>> On 03/24/2015 05:38 PM, Krzysztof Kozlowski wrote:
> >>>>> 2015-03-24 9:01 GMT+01:00 Beomho Seo <beomho.seo@samsung.com>:
> >>>>>> On 03/10/2015 10:44 PM, Beomho Seo wrote:
> >>>>>>> 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@samsung.com>:
> >>>>>>>>>>> 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@samsung.com>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> 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@kernel.org>
> >>>>>>>>>>>>> Signed-off-by: Beomho Seo <beomho.seo@samsung.com>
> >>>>>>>>>>>>
> >>>>>>>>>>>> Reviewed-By: Sebastian Reichel <sre@kernel.org>
> >>>>>>>>>>>>
> >>>>>>>>>>>> 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
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> 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).
> >>>>>
> >>>>> I still prefer merging common drivers into one instead of creating
> >>>>> some more of them.
> >>>>> However I understand your point and I am not entirely opposed against.
> >>>>> Especially that you invested quite a bit of time for developing this
> >>>>> and my feedback was quite late. To summarize I am fine with your
> >>>>> approach.
> >>>>>
> >>>>> Best regards,
> >>>>> Krzysztof
> >>>>>
> >>>>
> >>>> Dear Lee Jones,
> >>>>
> >>>> Could you please merge that your git tree ?
> >>>
> >>> Sorry, I'm lost.  Why am I taking this though the MFD tree?  What
> >>> patches are left?  Where are they going?  Am I taking any other
> >>> patches?
> >>>
> >>
> >> Max77843 charger driver is max77843 mfd core dependency.
> > 
> > What kind of dependancy?  Runtime or build?  Where is the patch that
> > it depends on?  Is it in -next for in Mainline already?
> > 
> 
> Build. Max77843 charger driver use max77843-private.h. It is in for-mfd-next branch.
> 
> c7f585f mfd: max77843: Add max77843 MFD driver core driver

If that's the case, then yes, I can take this patch through the MFD
tree with the correct Acks applied.

-- 
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 linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html