Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1311311 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V2 5/6] rtc: max77xxx: add RTC driver for Maxim MAX77xxx  series RTC IP Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-01-18 05:30 +0100
    Re: [PATCH V2 5/6] rtc: max77xxx: add RTC driver for Maxim MAX77xxx  series RTC IP Laxman Dewangan <ldewangan@nvidia.com> - 2016-01-18 13:20 +0100
      Re: [PATCH V2 5/6] rtc: max77xxx: add RTC driver for Maxim MAX77xxx  series RTC IP Javier Martinez Canillas <javier@dowhile0.org> - 2016-01-18 14:00 +0100

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

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

[toc] | [next] | [standalone]

#1311510

On Monday 18 January 2016 09:57 AM, Krzysztof Kozlowski wrote:
> 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).
>

Hi Javier,
If you can provide the patch for moving MAX77802 to MAX77686 then I can 
create the other change to move the regmap and i2c dummy client for rtc 
of max77686 to rtc driver from core on top of your change. This will  
completely decouple the rtc driver from core and so will be easy to use 
for other chip MAX77620.

Thanks,
Laxman

[toc] | [prev] | [next] | [standalone]

#1311525

Hello Laxman and Krzysztof,

On Mon, Jan 18, 2016 at 9:01 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>
> On Monday 18 January 2016 09:57 AM, Krzysztof Kozlowski wrote:
>>
>> 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).
>>
>
> Hi Javier,
> If you can provide the patch for moving MAX77802 to MAX77686 then I can
> create the other change to move the regmap and i2c dummy client for rtc of
> max77686 to rtc driver from core on top of your change. This will
> completely decouple the rtc driver from core and so will be easy to use for
> other chip MAX77620.
>

Ok, sounds like a plan. I'll just do the change to extend rtc-max77686
to support the max77802 rtc then and let the other change to you.

I'll work on this today once I finish other task that I'm currently working on.

> Thanks,
> Laxman
>

Best regards,
Javier

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web