On Thu, Mar 31, 2016 at 10:28 PM, Aubrey Li <aubrey.li@xxxxxxxxxxxxxxx> wrote:
> Currently the optional IPC resources prevent telemetry driver from
> probing if these resources are not in ACPI table. This patch decouples
> telemetry driver from these optional resources, so that telemetry driver
> has dependency only on the necessary ACPI resources.
Darren, I have comments as well.
>
> Signed-off-by: Aubrey Li <aubrey.li@xxxxxxxxxxxxxxx>
> ---
> drivers/platform/x86/intel_pmc_ipc.c | 48 +++++++++++++++-----------------
> drivers/platform/x86/intel_punit_ipc.c | 48 +++++++++++++++++++++-----------
> 2 files changed, 54 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
> index 092519e..29d9c02 100644
> --- a/drivers/platform/x86/intel_pmc_ipc.c
> +++ b/drivers/platform/x86/intel_pmc_ipc.c
> @@ -686,8 +686,8 @@ static int ipc_plat_get_res(struct platform_device *pdev)
> ipcdev.acpi_io_size = size;
> dev_info(&pdev->dev, "io res: %pR\n", res);
>
> - /* This is index 0 to cover BIOS data register */
> punit_res = punit_res_array;
> + /* This is index 0 to cover BIOS data register */
> res = platform_get_resource(pdev, IORESOURCE_MEM,
> PLAT_RESOURCE_BIOS_DATA_INDEX);
> if (!res) {
> @@ -697,55 +697,51 @@ static int ipc_plat_get_res(struct platform_device *pdev)
> *punit_res = *res;
> dev_info(&pdev->dev, "punit BIOS data res: %pR\n", res);
>
> + /* This is index 1 to cover BIOS interface register */
> res = platform_get_resource(pdev, IORESOURCE_MEM,
> PLAT_RESOURCE_BIOS_IFACE_INDEX);
> if (!res) {
> dev_err(&pdev->dev, "Failed to get res of punit BIOS iface\n");
> return -ENXIO;
> }
> - /* This is index 1 to cover BIOS interface register */
> *++punit_res = *res;
> dev_info(&pdev->dev, "punit BIOS interface res: %pR\n", res);
>
> + /* This is index 2 to cover ISP data register, optional */
All above looks like a commentary fixes (except an additional
'optional' word in one case). Can you do this separately?
> res = platform_get_resource(pdev, IORESOURCE_MEM,
> PLAT_RESOURCE_ISP_DATA_INDEX);
> - if (!res) {
> - dev_err(&pdev->dev, "Failed to get res of punit ISP data\n");
> - return -ENXIO;
> + ++punit_res;
> + if (res) {
> + *punit_res = *res;
> + dev_info(&pdev->dev, "punit ISP data res: %pR\n", res);
Okay, what if you re-arrange this to some helper first
int …_assign_res(*pdev, index, *punit_res)
{
struct resource res;
res = platform_get_resource(pdev, …, index);
if (!res)
return -ERRNO;
*punit_res = *res;
dev_dbg(%pR);
return 0;
}
In this patch you move to optional by
dev_err -> dev_warn
and use
if (ret)
dev_warn( "…skip optional resource…" );
instead of
if (ret) {
dev_err();
return ret;
}
> }
> - /* This is index 2 to cover ISP data register */
> - *++punit_res = *res;
> - dev_info(&pdev->dev, "punit ISP data res: %pR\n", res);
>
> + /* This is index 3 to cover ISP interface register, optional */
> res = platform_get_resource(pdev, IORESOURCE_MEM,
> PLAT_RESOURCE_ISP_IFACE_INDEX);
> - if (!res) {
> - dev_err(&pdev->dev, "Failed to get res of punit ISP iface\n");
> - return -ENXIO;
> + ++punit_res;
> + if (res) {
> + *punit_res = *res;
> + dev_info(&pdev->dev, "punit ISP interface res: %pR\n", res);
> }
> - /* This is index 3 to cover ISP interface register */
> - *++punit_res = *res;
> - dev_info(&pdev->dev, "punit ISP interface res: %pR\n", res);
>
> + /* This is index 4 to cover GTD data register, optional */
> res = platform_get_resource(pdev, IORESOURCE_MEM,
> PLAT_RESOURCE_GTD_DATA_INDEX);
> - if (!res) {
> - dev_err(&pdev->dev, "Failed to get res of punit GTD data\n");
> - return -ENXIO;
> + ++punit_res;
> + if (res) {
> + *punit_res = *res;
> + dev_info(&pdev->dev, "punit GTD data res: %pR\n", res);
> }
> - /* This is index 4 to cover GTD data register */
> - *++punit_res = *res;
> - dev_info(&pdev->dev, "punit GTD data res: %pR\n", res);
>
> + /* This is index 5 to cover GTD interface register, optional */
> res = platform_get_resource(pdev, IORESOURCE_MEM,
> PLAT_RESOURCE_GTD_IFACE_INDEX);
> - if (!res) {
> - dev_err(&pdev->dev, "Failed to get res of punit GTD iface\n");
> - return -ENXIO;
> + ++punit_res;
> + if (res) {
> + *punit_res = *res;
> + dev_info(&pdev->dev, "punit GTD interface res: %pR\n", res);
> }
> - /* This is index 5 to cover GTD interface register */
> - *++punit_res = *res;
> - dev_info(&pdev->dev, "punit GTD interface res: %pR\n", res);
>
> res = platform_get_resource(pdev, IORESOURCE_MEM,
> PLAT_RESOURCE_IPC_INDEX);
> diff --git a/drivers/platform/x86/intel_punit_ipc.c b/drivers/platform/x86/intel_punit_ipc.c
> index bd87540..a47a41f 100644
> --- a/drivers/platform/x86/intel_punit_ipc.c
> +++ b/drivers/platform/x86/intel_punit_ipc.c
> @@ -227,6 +227,11 @@ static int intel_punit_get_bars(struct platform_device *pdev)
> struct resource *res;
> void __iomem *addr;
>
> + /*
> + * The following resources are required
> + * - BIOS_IPC BASE_DATA
> + * - BIOS_IPC BASE_IFACE
> + */
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> addr = devm_ioremap_resource(&pdev->dev, res);
> if (IS_ERR(addr))
> @@ -239,29 +244,40 @@ static int intel_punit_get_bars(struct platform_device *pdev)
> return PTR_ERR(addr);
> punit_ipcdev->base[BIOS_IPC][BASE_IFACE] = addr;
>
> + /*
> + * The following resources are optional
> + * - ISPDRIVER_IPC BASE_DATA
> + * - ISPDRIVER_IPC BASE_IFACE
> + * - GTDRIVER_IPC BASE_DATA
> + * - GTDRIVER_IPC BASE_IFACE
> + */
> res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> - addr = devm_ioremap_resource(&pdev->dev, res);
> - if (IS_ERR(addr))
> - return PTR_ERR(addr);
> - punit_ipcdev->base[ISPDRIVER_IPC][BASE_DATA] = addr;
> + if (res) {
> + addr = devm_ioremap_resource(&pdev->dev, res);
> + if (!IS_ERR(addr))
> + punit_ipcdev->base[ISPDRIVER_IPC][BASE_DATA] = addr;
> + }
And here, what about just replacing return to dev_warn()?
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 3);
> - addr = devm_ioremap_resource(&pdev->dev, res);
> - if (IS_ERR(addr))
> - return PTR_ERR(addr);
> - punit_ipcdev->base[ISPDRIVER_IPC][BASE_IFACE] = addr;
> + if (res) {
> + addr = devm_ioremap_resource(&pdev->dev, res);
> + if (!IS_ERR(addr))
> + punit_ipcdev->base[ISPDRIVER_IPC][BASE_IFACE] = addr;
> + }
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 4);
> - addr = devm_ioremap_resource(&pdev->dev, res);
> - if (IS_ERR(addr))
> - return PTR_ERR(addr);
> - punit_ipcdev->base[GTDRIVER_IPC][BASE_DATA] = addr;
> + if (res) {
> + addr = devm_ioremap_resource(&pdev->dev, res);
> + if (!IS_ERR(addr))
> + punit_ipcdev->base[GTDRIVER_IPC][BASE_DATA] = addr;
> + }
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 5);
> - addr = devm_ioremap_resource(&pdev->dev, res);
> - if (IS_ERR(addr))
> - return PTR_ERR(addr);
> - punit_ipcdev->base[GTDRIVER_IPC][BASE_IFACE] = addr;
> + if (res) {
> + addr = devm_ioremap_resource(&pdev->dev, res);
> + if (!IS_ERR(addr))
> + punit_ipcdev->base[GTDRIVER_IPC][BASE_IFACE] = addr;
> + }
>
> return 0;
> }
> --
> 1.7.10.4
>
--
With Best Regards,
Andy Shevchenko
--
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
