RE: [PATCH RFC V1 2/3] rtc: da9063: Add DA9062 RTC capability to DA9063 RTC driver

Hi Alexandre,

Thank you for your quick responses on these patches.
I will probably need your further advice on the  of_compatible issue with the
DA9063 RTC  ... I've made some quick replies below.

On 20 July 2015 22:51 Alexandre Belloni wrote:

On 20/07/2015 at 17:57:50 +0000, Opensource [Steve Twiss] wrote :

quoted

On 18 July 2015 00:45, Alexandre Belloni wrote:

[...]

quoted

quoted

quoted

diff --git a/drivers/rtc/rtc-da9063.c b/drivers/rtc/rtc-da9063.c
index 7ffc570..e94fb6d 100644
--- a/drivers/rtc/rtc-da9063.c
+++ b/drivers/rtc/rtc-da9063.c

@@ -1,127 +1,274 @@
-/* rtc-da9063.c - Real time clock device driver for DA9063
- * Copyright (C) 2013-14  Dialog Semiconductor Ltd.
+/*
+ * Real time clock device driver for DA9063/DA9062
+ * Copyright (C) 2013-15  Dialog Semiconductor Ltd.

[...]

quoted

quoted

Please also list that license change in the commit log. It should also
probably be separated in another patch.

I can add this to a different patch and change it at a later date.
It is to fix a minor wording problem with the GPL header so it matches the
correct MODULE_LICENSE(""); macro.

[...]

Yeah, I understood the change, it can go in as soon as you send the
patch.

Thanks,
I'll send this later.

quoted

quoted

quoted

+
+	rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
+	if (!rtc)
+		return -ENOMEM;
+
+	if (strncmp(match->name, "dlg,da9063-rtc", 14) == 0) {
+		struct da9063 *chip = dev_get_drvdata(pdev->dev.parent);
+
+		if (chip->variant_code == PMIC_DA9063_AD)
+			rtc->config = &da9063_ad_regs;
+	} else
+		rtc->config = match->data;

You must not do that.
You should add a new compatible and change the of_compatible string of
the mfd_cell in drivers/mfd/da9063-core.c onc you know the variant.

I can check for a binary comparison against the address of the
static const struct of_device_id da9063_compatible_reg_id_table[] = {}
entry for DA9063.

You also must not compare pointers that way. You can use
of_device_is_compatible().

quoted

The DA9063 driver already associates "dlg,da9063-rtc" with both BB and AD
register maps. I think that altering the string at this point would break backwards
compatibility in the device tree for the DA9063.

I'm not fond of the backward compatibility exactly for that kind of
issue. The proper way to handle it is to have tow different compatible
strings because obviously, the BB and AD variants are not compatible.

I think I understand what you are saying about modifying the mfd_cell in the
DA9063 core driver at run-time so that it changes the of_compatible string to [something like]
"dlg,da9063-ad-rtc". That way the RTC driver can be probed using the correct
string.

Ok, I think I see now .. I'll make a patch v2 and send it soon.

Regards,
Steve

CC:
I've swapped Lee Jones into the TO field of this patch so he can start to have some
context on the next change to the da9063-core.c