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

On 13.01.2016 13:07, Laxman Dewangan wrote:

On Wednesday 13 January 2016 05:36 AM, Krzysztof Kozlowski wrote:

quoted

On 12.01.2016 18:17, Laxman Dewangan wrote:

quoted

Maxim Semiconductor's PMIC MAX77620, MAX77686, MAX20024 have
same RTC IP on these PMICs.

Add generic MAX77xxxx series RTC driver which can be used as
RTC driver for these PMIC and avoids duplication of RTC driver
for each PMICs. Their MFD driver can be different here.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
Changes from V1:
- Rename the file to rtc-max77xxx.c and make the generic implementation.
- Direct regmap apis are used for the register access.
- Decouped from max77620 driver.
- Taken care of cleanup comments form V1 version.

  drivers/rtc/Kconfig        |  10 +
  drivers/rtc/Makefile       |   1 +
  drivers/rtc/rtc-max77xxx.c | 500
+++++++++++++++++++++++++++++++++++++++++++++
  3 files changed, 511 insertions(+)
  create mode 100644 drivers/rtc/rtc-max77xxx.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 376322f..4972dd5 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig

@@ -315,6 +315,16 @@ config RTC_DRV_MAX8997
        This driver can also be built as a module. If so, the module
        will be called rtc-max8997.
  +config RTC_DRV_MAX77XXX
+    tristate "Maxim MAX77XXX series generic RTC driver"
+    help
+      If you say yes here you will get support for the generic RTC

driver
+      for Maxim Semiconductor MAX77XXX series of PMIC like MAX77620.
+      This also supports the RTC driver for Maxim PMIC MaX20024 which
+      is almost same as MAX77620.
+      This driver can also be built as a module. If so, the module
+      will be called rtc-max77xxx.
+

That was not the consensus... You still added a new driver - but now
with different name.

That is useless duplication

Please work with existing code. Use existing maxim RTC drivers: either
max77686 or max77802.

There is no need for new one.

If we modify the existing one then that work will be outside of this
series to make it independent.

And that is the problem? The series evolve. The ultimate goal is to
support max77686, max77802, max77620 and max20024.

However, the file name does not suggest common in older file.

This is not a sensible argument. The name does not matter. But if really
needed we can rename it...

Also this
will require mfd and rtc driver changes to decouple it.

Yes, decouple everything! I like it! :) Make it robust, generic,
readable, fix bugs etc. :)

Here is my approach:
- Let's have common driver in implementation and file name. This will be
independent of the mfd driver on all sense.
- Once it is merged, move the max77686 and max77802 to use this driver,
this will need the modification on the mfd driver, related defconfig
file and if specific stuff needed in the rtc then addition of that.

Nope, because *the second part won't happen*. Never. After merging you
will be happy and another duplicated stuff ends in the kernel.

Fix things before merging. Not after.

Per your approach:
- Modify rtc max77686 and mfd driver max77686 to decouple and proper
registration.
- Add support of max77620 on the max77686 driver if any specific is
required.

That is the way we usually extend the drivers for new devices.

That is also fine to me but still I am not comfortable with the config
name and driver file name as this does not suggest the common.

The name does not matter. Really. We have a lot of drivers with a
specific device-like name and supporting different devices. To point
that your argument is invalid - your initial name of driver
"rtc-max77620.c" supported totally different "names": the max77620 and
max20024. It also wasn't suggesting something "common"...

With my approach we are not developing common think neither. We just
want to extend/re-use existing max77686 (or max77802) driver for new
devices. Just like everywhere else.

Best regards,
Krzysztof