>> +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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
