Re: [rtc-linux] [PATCH 01/02 resend] RTC: Add driver for DS1685 family of real time clocks

[Joshua Kinard <kumba@gentoo.org>]

On 02/09/2015 19:18, Andrew Morton wrote:
> On Fri, 12 Dec 2014 17:13:38 -0500 Joshua Kinard <kumba@gentoo.org> wrote:
> 
>> From: Joshua Kinard <kumba@gentoo.org>
>>
>> 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?


>> +	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.


>> +	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