>> +struct intel_punit_ipc_controller {
>> + struct platform_device *pdev;
>Usually we keep pointer to struct device. Any specific reason to hold
>platform_device here?
Because intel_punit_get_bars() need to use platform_device pointer to get
resources.
>> +
>> +static int intel_punit_ipc_check_status(void)
>> +{
>> + int loops = CMD_TIMEOUT_SECONDS * USEC_PER_SEC;
>From where is this timeout in *seconds* coming? Sounds too
> big for me.
Empirical value from SCU ipc driver.
> + res0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res0) {
> + dev_err(&pdev->dev, "Failed to get iomem
> resource\n");
> + return -ENXIO;
> + }
> + size = resource_size(res0);
> + if (!devm_request_mem_region(&pdev->dev, res0->start,
> + size, pdev-
> >name)) {
>Why not to use devm_ioremap_resource() ?
>> + ipcdev.base[BIOS_MAILBOX] = addr;
>> + addr += MAILBOX_REGISTER_SPACE;
>> + ipcdev.base[GTDRIVER_MAILBOX] = addr;
>> + addr += MAILBOX_REGISTER_SPACE;
>> + ipcdev.base[ISPDRIVER_MAILBOX] = addr;
>Looks akward, does the platform have the several resources for different
>purpose? Why do you unify them (who does guarantee the non-breakable segment
>for all resources?) first and then split up?
>Please, refactor.
>From spec, these three parts are consecutive, so only define one acpi resource
>entry is reasonable way,
But BIOS maintainer finally make the resource like this due to request from
Window os driver,
So can't treat these three as three separate parts.
>> +
>> +/* Some modules are dependent on this, so init earlier */
>> +fs_initcall(intel_punit_ipc_init);
>So, what exactly requires this?
Those drivers which need to use this Punit APIs in its Probe when do module
init.
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
