On 02/09/2015 19:18, Andrew Morton wrote:

On Fri, 12 Dec 2014 17:13:38 -0500 Joshua Kinard [off-list ref] wrote:

quoted

From: Joshua Kinard <redacted>

This adds a driver for the Dallas/Maxim DS1685-family of RTC chips.  It
supports the DS1685/DS1687, DS1688/DS1691, DS1689/DS1693, DS17285/DS17287,
DS17485/DS17487, and DS17885/DS17887 RTC chips.  These chips are commonly found
in SGI O2 and SGI Octane systems.  It was originally derived from a driver
patch submitted by Matthias Fuchs many years ago for use in EPPC-405-UC
modules, which also used these RTCs.  In addition to the time-keeping
functions, this RTC also handles the shutdown mechanism of the O2 and Octane
and acts as a partial NVRAM for the boot PROMS in these systems.

Verified on both an SGI O2 and an SGI Octane.

...

+static int
+ds1685_rtc_probe(struct platform_device *pdev)
+{
+	struct rtc_device *rtc_dev;
+	struct resource *res;
+	struct ds1685_priv *rtc;
+	struct ds1685_rtc_platform_data *pdata;
+	u8 ctrla, ctrlb, hours;
+	unsigned char am_pm;
+	int ret = 0;
+
+	/* Get the platform data. */
+	pdata = (struct ds1685_rtc_platform_data *) pdev->dev.platform_data;

That cast isn't needed.

Huh, I thought GCC complained about that once, but it doesn't now (gcc-4.7.4).
 Would you like me to remove it and re-send the patch, even though it looks
like you've added it to -mm?

quoted

+	if (!pdata)
+		return -ENODEV;
+
+	/* Allocate memory for the rtc device. */
+	rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
+	if (!rtc)
+		return -ENOMEM;
+
+	/*
+	 * Allocate/setup any IORESOURCE_MEM resources, if required.  Not all
+	 * platforms put the RTC in an easy-access place.  Like the SGI Octane,
+	 * which attaches the RTC to a "ByteBus", hooked to a SuperIO chip
+	 * that sits behind the IOC3 PCI metadevice.
+	 */
+	if (pdata->alloc_io_resources) {
+		/* Get the platform resources. */
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+		if (!res)
+			return -ENXIO;
+		rtc->size = resource_size(res);
+
+		/* Request a memory region. */
+		/* XXX: mmio-only for now. */
+		if (!devm_request_mem_region(&pdev->dev, res->start, rtc->size,
+					     pdev->name))
+			return -EBUSY;
+
+		/*
+		 * Set the base address for the rtc, and ioremap its
+		 * registers.
+		 */
+		rtc->baseaddr = res->start;
+		rtc->regs = devm_ioremap(&pdev->dev, res->start, rtc->size);
+		if (!rtc->regs)
+			return -ENOMEM;
+	}
+	rtc->alloc_io_resources = pdata->alloc_io_resources;
+
+	/* Get the register step size. */
+	if (pdata->regstep > 0)
+		rtc->regstep = pdata->regstep;
+	else
+		rtc->regstep = 1;
+
+	/* Platform read function, else default if mmio setup */
+	if (pdata->plat_read)
+		rtc->read = pdata->plat_read;

I'm trying to understand how this works and I'm not getting very far. 
Perhaps it is the intention that some code(?) is to allocate and
populate a ds1685_rtc_platform_data and use platform_device_add_data()
on it, but no such code exists.

Or something.  What's going on here?

Yeah, as you saw in the second patch, this is a mechanism to keep arch or
machine-specific code out of a general driver.  SGI O2's use MMIO to read/set
the RTC (which are the default methods in this patch), but SGI Octane's, whose
code is not in-tree yet, use this same RTC (DS1687-5) via PIO access because
the RTC is tucked behind the IOC3 PCI Metadevice's "ByteBus" (write an address
port, read a data port).  Thus, two different methods are needed by each
machine to talk to the same RTC driver, so this looked like the best approach,
after I looked at a few other drivers.

quoted

+	else
+		if (pdata->alloc_io_resources)
+			rtc->read = ds1685_read;
+		else
+			return -ENXIO;
+
+	/* Platform write function, else default if mmio setup */
+	if (pdata->plat_write)
+		rtc->write = pdata->plat_write;
+	else
+		if (pdata->alloc_io_resources)
+			rtc->write = ds1685_write;
+		else
+			return -ENXIO;
+
+	/* Platform pre-shutdown function, if defined. */
+	if (pdata->plat_prepare_poweroff)
+		rtc->prepare_poweroff = pdata->plat_prepare_poweroff;
+
+	/* Platform wake_alarm function, if defined. */
+	if (pdata->plat_wake_alarm)
+		rtc->wake_alarm = pdata->plat_wake_alarm;
+
+	/* Platform post_ram_clear function, if defined. */
+	if (pdata->plat_post_ram_clear)
+		rtc->post_ram_clear = pdata->plat_post_ram_clear;
+
+	/* Init the spinlock, workqueue, & set the driver data. */
+	spin_lock_init(&rtc->lock);
+	INIT_WORK(&rtc->work, ds1685_rtc_work_queue);
+	platform_set_drvdata(pdev, rtc);

--J