All of lore.kernel.org
 help / color / mirror / Atom feed
From: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
To: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Cc: Doug Anderson <dianders@chromium.org>,
	Lee Jones <lee.jones@linaro.org>,
	Alessandro Zummo <a.zummo@towertech.it>,
	Kukjin Kim <kgene.kim@samsung.com>,
	Mike Turquette <mturquette@linaro.org>,
	Samuel Ortiz <sameo@linux.intel.com>,
	Tomeu Vizoso <tomeu.vizoso@collabora.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	linux-samsung-soc <linux-samsung-soc@vger.kernel.org>,
	Sjoerd Simons <sjoerd.simons@collabora.co.uk>,
	Mark Brown <broonie@kernel.org>, Olof Johansson <olof@lixom.net>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	Daniel Stone <daniels@collabora.com>
Subject: Re: [PATCH 0/5] Add Maxim 77802 PMIC support
Date: Tue, 10 Jun 2014 09:50:27 +0200	[thread overview]
Message-ID: <F27F8EB2-39C2-4A03-A5AC-E11C5A35BF9C@collabora.co.uk> (raw)
In-Reply-To: <1402385558.6989.11.camel@AMDC1943>

Hello Krzysztof,

> On 10/06/2014, at 09:32, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:
> 
>> On pon, 2014-06-09 at 09:04 -0700, Doug Anderson wrote:
>> Krzystof,
>> 
>> On Mon, Jun 9, 2014 at 3:16 AM, Krzysztof Kozlowski
>> <k.kozlowski@samsung.com> wrote:
>>> On pon, 2014-06-09 at 11:37 +0200, Javier Martinez Canillas wrote:
>>>> MAX77802 is a PMIC that contains 10 high efficiency Buck regulators,
>>>> 32 Low-dropout (LDO) regulators, two 32kHz buffered clock outputs,
>>>> a Real-Time-Clock (RTC) and a I2C interface to program the individual
>>>> regulators, clocks and the RTC.
>>>> 
>>>> This series are based on drivers added by Simon Glass to the Chrome OS
>>>> kernel and adds support for the Maxim 77802 Power Management IC, their
>>>> regulators, clocks, RTC and I2C interface. It is composed of patches:
>>>> 
>>>> [PATCH 1/5] mfd: Add driver for Maxim 77802 Power Management IC
>>>> [PATCH 2/5] regulator: Add driver for Maxim 77802 PMIC regulators
>>>> [PATCH 3/5] clk: Add driver for Maxim 77802 PMIC clocks
>>>> [PATCH 4/5] rtc: Add driver for Maxim 77802 PMIC Real-Time-Clock
>>>> [PATCH 5/5] ARM: dts: Add max77802 device node for exynos5420-peach-pit
>>>> 
>>>> Patches 1-4 add support for the different devices and Patch 5 enables
>>>> the MAX77802 PMIC on the Exynos5420 based Peach pit board.
>>> 
>>> 
>>> Hi,
>>> 
>>> The main mfd, mfd irq, clk and rtc drivers look very similar to max77686
>>> drivers. I haven't checked other Maxim drivers but I think there will be
>>> a lot of similarities with them also. It is almost common for Maxim
>>> chipsets to share components between each other.
>>> 
>>> I think there is no need in duplicating all that stuff once again in new
>>> driver for another Maxim-almost-the-same-as-others-XYZ chipset. Just
>>> merge it with max77686 (or other better candidate).
>>> 
>>> The only difference is in regulator driver. I am not sure whether this
>>> is a result of differences in chip or differences in driver design.
>> 
>> Yes, we thought the same thing when we added support for the max77802
>> in the ChromeOS tree.  Unfortunately it didn't work out half as well
>> as we thought it would.  When Javier was asking advice about sending
>> things upstream we suggested that perhaps he should split the two up.
>> 
>> 
>> You can see the result of the combined driver the ChromeOS tree (the
>> code there is older, probably misnamed as max77xxx, and doesn't have
>> the proper clock pieces, but you can get the gist):
>> 
>> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.8/drivers/regulator/max77xxx-regulator.c
>> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.8/drivers/rtc/rtc-max77xxx.c
>> 
>> 
>> Specific problems that made it ugly to have a combined driver:
>> 
>> * The RTC has many subtle differences between the 77686 and 77802.
>> They expanded it to handle a 200 year timeframe instead of 100 and
>> that meant that they had to shuffle the bits around everywhere.  They
>> also moved it to have the same i2c address as the main PMIC so all
>> addresses are different (see max77686_map in the RTC link above).
> 
> The difference in RTC registers seems the biggest but it can be solved
> in readable manner. I see other differences but there aren't many. It
> just hurts seeing so much code duplication:
> $ sed -e 's/max77686/max77802/g' -e 's/MAX77686/MAX77802/g' \
>  -i drivers/rtc/rtc-max77686.c
> $ diff -ubB drivers/rtc/rtc-max77686.c drivers/rtc/rtc-max77802.c
> 
> The combined RTC driver from ChromeOS seems fine to me... but I do not
> insist.
> 
>> * The regulator itself has similar concepts between the two, but the
>> list of bucks / ldos and how they behave is quite different.  Trying
>> to understand the complex tables in
>> <https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.8/drivers/regulator/max77xxx-regulator.c>
>> was not easy.
>> 
>> 
>> If we really need to write a single driver it certainly can be done,
>> but please look at the above to be sure this is what you want.
> 
> Sure, I don't stick to the idea of one merged driver where this
> increases code size and complexity. I see your point that merging
> regulator drivers won't bring benefits but please:
> $ sed -e 's/max77686/max77802/g' -e 's/MAX77686/MAX77802/g' \
>  -i drivers/clk/clk-max77686.c
> $ diff -ubB drivers/clk/clk-max77686.c drivers/clk/clk-max77802.c
> 

I agree that there is too much duplicated code between those two and others Maxim PMIC drivers.

I also agree that at some point we have to stop duplicating code and clean up all this.

So, I think that instead of trying to make a single driver support two similar but different PMIC I can factor out the generic code in a max-core driver so the PMIC specific code is small and well contained.

I'll work on that and post a v2.

Thanks a lot for your feedback and best regards,

Javier

> The difference in number of clocks (2 vs 3) is not an obstacle here.
> 
> The same applies to main MFD driver and IRQ code. However MAX77686
> doesn't use regmap_irq_chip so it needs changes before merging the IRQ
> code into one piece.
> 
> Best regards,
> Krzysztof
> 
>> 
>> NOTE: it's possible that things could be more sane with more driver
>> redesign, possibly making things more data driven.  The thing that
>> would be really nice to do would be to avoid all of the crazy
>> "regulator_zzz_desc_zzz" macros, maybe?  I'd have to actually try
>> doing it to be sure it's cleaner, though...
>> 
>> 
>> -Doug
> 

WARNING: multiple messages have this Message-ID
From: javier.martinez@collabora.co.uk (Javier Martinez Canillas)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 0/5] Add Maxim 77802 PMIC support
Date: Tue, 10 Jun 2014 09:50:27 +0200	[thread overview]
Message-ID: <F27F8EB2-39C2-4A03-A5AC-E11C5A35BF9C@collabora.co.uk> (raw)
In-Reply-To: <1402385558.6989.11.camel@AMDC1943>

Hello Krzysztof,

> On 10/06/2014, at 09:32, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:
> 
>> On pon, 2014-06-09 at 09:04 -0700, Doug Anderson wrote:
>> Krzystof,
>> 
>> On Mon, Jun 9, 2014 at 3:16 AM, Krzysztof Kozlowski
>> <k.kozlowski@samsung.com> wrote:
>>> On pon, 2014-06-09 at 11:37 +0200, Javier Martinez Canillas wrote:
>>>> MAX77802 is a PMIC that contains 10 high efficiency Buck regulators,
>>>> 32 Low-dropout (LDO) regulators, two 32kHz buffered clock outputs,
>>>> a Real-Time-Clock (RTC) and a I2C interface to program the individual
>>>> regulators, clocks and the RTC.
>>>> 
>>>> This series are based on drivers added by Simon Glass to the Chrome OS
>>>> kernel and adds support for the Maxim 77802 Power Management IC, their
>>>> regulators, clocks, RTC and I2C interface. It is composed of patches:
>>>> 
>>>> [PATCH 1/5] mfd: Add driver for Maxim 77802 Power Management IC
>>>> [PATCH 2/5] regulator: Add driver for Maxim 77802 PMIC regulators
>>>> [PATCH 3/5] clk: Add driver for Maxim 77802 PMIC clocks
>>>> [PATCH 4/5] rtc: Add driver for Maxim 77802 PMIC Real-Time-Clock
>>>> [PATCH 5/5] ARM: dts: Add max77802 device node for exynos5420-peach-pit
>>>> 
>>>> Patches 1-4 add support for the different devices and Patch 5 enables
>>>> the MAX77802 PMIC on the Exynos5420 based Peach pit board.
>>> 
>>> 
>>> Hi,
>>> 
>>> The main mfd, mfd irq, clk and rtc drivers look very similar to max77686
>>> drivers. I haven't checked other Maxim drivers but I think there will be
>>> a lot of similarities with them also. It is almost common for Maxim
>>> chipsets to share components between each other.
>>> 
>>> I think there is no need in duplicating all that stuff once again in new
>>> driver for another Maxim-almost-the-same-as-others-XYZ chipset. Just
>>> merge it with max77686 (or other better candidate).
>>> 
>>> The only difference is in regulator driver. I am not sure whether this
>>> is a result of differences in chip or differences in driver design.
>> 
>> Yes, we thought the same thing when we added support for the max77802
>> in the ChromeOS tree.  Unfortunately it didn't work out half as well
>> as we thought it would.  When Javier was asking advice about sending
>> things upstream we suggested that perhaps he should split the two up.
>> 
>> 
>> You can see the result of the combined driver the ChromeOS tree (the
>> code there is older, probably misnamed as max77xxx, and doesn't have
>> the proper clock pieces, but you can get the gist):
>> 
>> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.8/drivers/regulator/max77xxx-regulator.c
>> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.8/drivers/rtc/rtc-max77xxx.c
>> 
>> 
>> Specific problems that made it ugly to have a combined driver:
>> 
>> * The RTC has many subtle differences between the 77686 and 77802.
>> They expanded it to handle a 200 year timeframe instead of 100 and
>> that meant that they had to shuffle the bits around everywhere.  They
>> also moved it to have the same i2c address as the main PMIC so all
>> addresses are different (see max77686_map in the RTC link above).
> 
> The difference in RTC registers seems the biggest but it can be solved
> in readable manner. I see other differences but there aren't many. It
> just hurts seeing so much code duplication:
> $ sed -e 's/max77686/max77802/g' -e 's/MAX77686/MAX77802/g' \
>  -i drivers/rtc/rtc-max77686.c
> $ diff -ubB drivers/rtc/rtc-max77686.c drivers/rtc/rtc-max77802.c
> 
> The combined RTC driver from ChromeOS seems fine to me... but I do not
> insist.
> 
>> * The regulator itself has similar concepts between the two, but the
>> list of bucks / ldos and how they behave is quite different.  Trying
>> to understand the complex tables in
>> <https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.8/drivers/regulator/max77xxx-regulator.c>
>> was not easy.
>> 
>> 
>> If we really need to write a single driver it certainly can be done,
>> but please look at the above to be sure this is what you want.
> 
> Sure, I don't stick to the idea of one merged driver where this
> increases code size and complexity. I see your point that merging
> regulator drivers won't bring benefits but please:
> $ sed -e 's/max77686/max77802/g' -e 's/MAX77686/MAX77802/g' \
>  -i drivers/clk/clk-max77686.c
> $ diff -ubB drivers/clk/clk-max77686.c drivers/clk/clk-max77802.c
> 

I agree that there is too much duplicated code between those two and others Maxim PMIC drivers.

I also agree that at some point we have to stop duplicating code and clean up all this.

So, I think that instead of trying to make a single driver support two similar but different PMIC I can factor out the generic code in a max-core driver so the PMIC specific code is small and well contained.

I'll work on that and post a v2.

Thanks a lot for your feedback and best regards,

Javier

> The difference in number of clocks (2 vs 3) is not an obstacle here.
> 
> The same applies to main MFD driver and IRQ code. However MAX77686
> doesn't use regmap_irq_chip so it needs changes before merging the IRQ
> code into one piece.
> 
> Best regards,
> Krzysztof
> 
>> 
>> NOTE: it's possible that things could be more sane with more driver
>> redesign, possibly making things more data driven.  The thing that
>> would be really nice to do would be to avoid all of the crazy
>> "regulator_zzz_desc_zzz" macros, maybe?  I'd have to actually try
>> doing it to be sure it's cleaner, though...
>> 
>> 
>> -Doug
> 

  reply	other threads:[~2014-06-10  7:50 UTC|newest]

Thread overview: 54+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-06-09  9:37 Javier Martinez Canillas
2014-06-09  9:37 ` Javier Martinez Canillas
2014-06-09  9:37 ` [PATCH 1/5] mfd: Add driver for Maxim 77802 Power Management IC Javier Martinez Canillas
2014-06-09  9:37   ` Javier Martinez Canillas
2014-06-09 10:22   ` Krzysztof Kozlowski
2014-06-09 10:22     ` Krzysztof Kozlowski
2014-06-09 11:56     ` Mark Brown
2014-06-09 11:56       ` Mark Brown
2014-06-09 23:07     ` Javier Martinez Canillas
2014-06-09 23:07       ` Javier Martinez Canillas
2014-06-09 19:47   ` Mark Brown
2014-06-09 19:47     ` Mark Brown
2014-06-09 23:40     ` Javier Martinez Canillas
2014-06-09 23:40       ` Javier Martinez Canillas
2014-06-09  9:37 ` [PATCH 2/5] regulator: Add driver for Maxim 77802 PMIC regulators Javier Martinez Canillas
2014-06-09  9:37   ` Javier Martinez Canillas
2014-06-09 19:38   ` Mark Brown
2014-06-09 19:38     ` Mark Brown
2014-06-09 19:38     ` Mark Brown
2014-06-09 23:29     ` Javier Martinez Canillas
2014-06-09 23:29       ` Javier Martinez Canillas
2014-06-10 10:53       ` Mark Brown
2014-06-10 10:53         ` Mark Brown
2014-06-09  9:37 ` [PATCH 3/5] clk: Add driver for Maxim 77802 PMIC clocks Javier Martinez Canillas
2014-06-09  9:37   ` Javier Martinez Canillas
2014-06-09  9:37   ` Javier Martinez Canillas
2014-06-16  8:44   ` Lee Jones
2014-06-16  8:44     ` Lee Jones
2014-06-16  8:54     ` Javier Martinez Canillas
2014-06-16  8:54       ` Javier Martinez Canillas
2014-06-09  9:37 ` [PATCH 4/5] rtc: Add driver for Maxim 77802 PMIC Real-Time-Clock Javier Martinez Canillas
2014-06-09  9:37   ` Javier Martinez Canillas
2014-06-09  9:37 ` [PATCH 5/5] ARM: dts: Add max77802 device node for exynos5420-peach-pit Javier Martinez Canillas
2014-06-09  9:37   ` Javier Martinez Canillas
2014-06-09 10:16 ` [PATCH 0/5] Add Maxim 77802 PMIC support Krzysztof Kozlowski
2014-06-09 10:16   ` Krzysztof Kozlowski
2014-06-09 16:04   ` Doug Anderson
2014-06-09 16:04     ` Doug Anderson
2014-06-09 16:04     ` Doug Anderson
2014-06-09 22:55     ` Javier Martinez Canillas
2014-06-09 22:55       ` Javier Martinez Canillas
2014-06-09 22:55       ` Javier Martinez Canillas
2014-06-09 23:57       ` Doug Anderson
2014-06-09 23:57         ` Doug Anderson
2014-06-09 23:57         ` Doug Anderson
2014-06-10  7:45       ` Krzysztof Kozlowski
2014-06-10  7:45         ` Krzysztof Kozlowski
2014-06-10  7:45         ` Krzysztof Kozlowski
2014-06-10  7:32     ` Krzysztof Kozlowski
2014-06-10  7:32       ` Krzysztof Kozlowski
2014-06-10  7:32       ` Krzysztof Kozlowski
2014-06-10  7:50       ` Javier Martinez Canillas [this message]
2014-06-10  7:50         ` Javier Martinez Canillas
2014-06-10  7:50         ` Javier Martinez Canillas

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=F27F8EB2-39C2-4A03-A5AC-E11C5A35BF9C@collabora.co.uk \
    --to=javier.martinez@collabora.co.uk \
    --cc=a.zummo@towertech.it \
    --cc=broonie@kernel.org \
    --cc=daniels@collabora.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dianders@chromium.org \
    --cc=k.kozlowski@samsung.com \
    --cc=kgene.kim@samsung.com \
    --cc=lee.jones@linaro.org \
    --cc=lgirdwood@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-samsung-soc@vger.kernel.org \
    --cc=mturquette@linaro.org \
    --cc=olof@lixom.net \
    --cc=sameo@linux.intel.com \
    --cc=sjoerd.simons@collabora.co.uk \
    --cc=tomeu.vizoso@collabora.com \
    --subject='Re: [PATCH 0/5] Add Maxim 77802 PMIC support' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.