On wto, 2014-06-10 at 00:55 +0200, Javier Martinez Canillas wrote: > Hello Krzystof, > > Thanks a lot for your feedback. > > On 06/09/2014 06:04 PM, Doug Anderson wrote: > > Krzystof, > > > > On Mon, Jun 9, 2014 at 3:16 AM, Krzysztof Kozlowski > > <k.kozlowski@xxxxxxxxxxx> 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 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. > > > > > > There are other differences that were not mentioned: > > - The max77802 uses a single register to enable RTC alarm while max77686 uses 1 > bit from a set of registers. But still this will be one additional 'if' statement in one common code... > - Each chip has some regulators that are not available and you have to take care > of those exceptions (e.g: LDO16, LDO22 and LD31 on max77802). Missing/new regulator does not look like a problem to me in combined driver from ChromeOS. The main problem with combined driver for regulators is that you still need to provide all the regulator_ops/regulator_desc for each of the chipsets. Thus the combined driver is almost as big as two drivers. > - The max77802 has 2 clocks outputs while the max77686 has three. This leads to one exception in combined clock driver. This will still reduce LOC. > So, as Doug said there are many differences on these two chips besides the > regulators. It's true that these two drivers share a lot of the structure and > there is code duplication (this applies to most maxim drivers btw) but I have my > doubts that the combined approach will make the code more easy to maintain. > > The biggest problem is the i2c addresses space being different so you need an > indirection level to access registers and have duplicated registers definition > for each chip. Yep, I agree. I had the same problem with S5M/S2M RTC driver and I am not quite sure that I've chosen the right solution :). > > 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. > > > > > > Yes, if the combined driver is preferred over having a separate driver for > max77802 then I will try to find the more elegant way to merge both drivers. But > I tried to do it already and I can't say I liked the end result more than having > two separate drivers. > > > 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... > > > > Another option is to use an hybrid approach. Merge the mfd core, irq and clk > drivers but have different platform drivers for rtc and regulators. I think that would be the best solution. Probably it would be useful to convert the max77686 IRQ code to regmap_irq_chip but I do not insist on that. Best regards, Krzysztof > Still the > end result is not great imho but better than trying merging the regulators and > RTC drivers where most of the differences are. > > > > > -Doug > > > > Best regards, > Javier -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
