Re: [PATCH V2 5/6] rtc: max77xxx: add RTC driver for Maxim MAX77xxx series RTC IP

[Krzysztof Kozlowski <k.kozlowski@samsung.com>]

On 15.01.2016 10:50, Javier Martinez Canillas wrote:
> Hello,
> 
> On Thu, Jan 14, 2016 at 8:50 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>> On Thursday 14 January 2016 06:50 AM, Krzysztof Kozlowski wrote:
>>> On 14.01.2016 09:50, Krzysztof Kozlowski wrote:
> 
> [snip]
> 
>>>>
>>>> The max77802 does exactly the same (BTW, these should be merged as
>>>> well... I'll add this to the TODO list) so I think this is necessary.
>>>
>>> How about merging max77802 to max77686 first? The only differences I
>>> found are:
>>> 1. It uses main MFD/PMIC regmap.
>>>     This can be solved as part of decoupling code. The driver will get
>>> MFD's regmap and set up its own (only on max77686). The max77802 will
>>> only use parent's regmap.
>>>
>>> 2. It has different register address.
>>>     We need a register-layout/configuration structure. The logic is the
>>> same except few differences (e.g. presence of MAX77802_RTC_AE1).
>>>
>>> It may be easier to merge them now, before adding support for max77620?
> 
> Agreed.
> 
> When I originally posted the max77802 support, I had separate MFD,
> RTC, regulator and clock drivers and the feedback (IIRC) was that the
> MFD and clock blocks were too similar to the max77686 so I extended
> those drivers instead of adding new ones. But that the RTC and
> regulator blocks were different so a separate driver was justified...
> but it's true that are not that different and rtc-max77686 could be
> extended and rtc-max77802 removed.

Great!

> 
> In fact, the ChromiumOS vendor tree has a single RTC driver for both
> max77686 and max77802:
> 
> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.8/drivers/rtc/rtc-max77xxx.c
> 
>>> I could handle this probably next week or in the following week
>>> (assuming someone would test max77802 because I don't have the hardware).
>>>
> 
> I could also work on this next week if you want. After all I feel
> guilty for the code duplication :-)

So feel free to take that job from me. :) I will happily do the testing
and provide complains (I mean, comments).

> 
>>> Anyway I think we should develop these RTC patches having this in mind:
>>> merge all of them.
>>>
>>
>> I think we can do the merging of max77802 and max77686 at second step.
>>
>> At first, lets decouple the rtc-max77686.c with its mfd driver. This will
>> give better picture how will it be done.
> 
> I don't quite understand what you mean by decoupling here, the
> max77686 MFD driver today is not highly coupled to their cells devices
> drivers since it supports both max77802 and max77686 already. Yes,
> some changes will be needed but I think those should be small.

The decoupling needed is to move RTC-related stuff (i2c_new_dummy and
regmap) entirely to RTC driver. This work is independent of which driver
will be merged to rtc-max77xxx first.

> 
>> Once this is there then max77620 can use this and in parallel, can be work
>> for max77802 to use the same driver.
>>
>>
>> Let's first conclude and get accpepted for max77686 and max77620 as both of
>> us have related HW to verify the changes.
>>
> 
> I agree with Krzysztof that merging first before extending makes more
> sense but I don't have a strong opinion on this and can be done as a
> followup as well.

AFAIR, Javier have max77802 on Chromebook, right?

Best regards,
Krzysztof