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