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

Hi,

On 21/07/2015 at 08:05:32 +0000, Opensource [Steve Twiss] wrote :

quoted

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.

This is not needed if you simply change your strncmp with
of_device_is_compatible

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com