Messages in this thread | | | Subject | Re: [PATCH v3] hwmon: xgene: Support hwmon v2 | From | Guenter Roeck <> | Date | Fri, 13 Oct 2017 21:54:51 -0700 |
| |
On 10/13/2017 09:38 PM, Hoan Tran wrote: > Hi Guenter, > > On Fri, Oct 13, 2017 at 9:28 PM, Guenter Roeck <linux@roeck-us.net> wrote: >> On 10/13/2017 04:10 PM, Hoan Tran wrote: >>> >>> This patch supports xgene-hwmon v2 which uses the non-cachable memory >>> as the PCC shared memory. >>> >>> Signed-off-by: Hoan Tran <hotran@apm.com> >>> --- >>> v3 >>> - Use local version variable >>> - Use inline calls instead of the private map function >>> >>> v2 >>> - Map PCC shared mem by ioremap() in case hwmon is v2 >>> >>> drivers/hwmon/xgene-hwmon.c | 40 >>> +++++++++++++++++++++++++++++++--------- >>> 1 file changed, 31 insertions(+), 9 deletions(-) >>> >>> diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c >>> index 9c0dbb8..6b6c732 100644 >>> --- a/drivers/hwmon/xgene-hwmon.c >>> +++ b/drivers/hwmon/xgene-hwmon.c >>> @@ -91,6 +91,11 @@ >>> #define to_xgene_hwmon_dev(cl) \ >>> container_of(cl, struct xgene_hwmon_dev, mbox_client) >>> +enum xgene_hwmon_version { >>> + XGENE_HWMON_V1 = 0, >>> + XGENE_HWMON_V2 = 1, >>> +}; >>> + >>> struct slimpro_resp_msg { >>> u32 msg; >>> u32 param1; >>> @@ -609,6 +614,15 @@ static void xgene_hwmon_tx_done(struct mbox_client >>> *cl, void *msg, int ret) >>> } >>> } >>> +#ifdef CONFIG_ACPI >>> +static const struct acpi_device_id xgene_hwmon_acpi_match[] = { >>> + {"APMC0D29", XGENE_HWMON_V1}, >>> + {"APMC0D8A", XGENE_HWMON_V2}, >>> + {}, >>> +}; >>> +MODULE_DEVICE_TABLE(acpi, xgene_hwmon_acpi_match); >>> +#endif >>> + >>> static int xgene_hwmon_probe(struct platform_device *pdev) >>> { >>> struct xgene_hwmon_dev *ctx; >>> @@ -650,6 +664,16 @@ static int xgene_hwmon_probe(struct platform_device >>> *pdev) >>> } >>> } else { >>> struct acpi_pcct_hw_reduced *cppc_ss; >>> + const struct acpi_device_id *acpi_id; >>> + int version; >>> + >>> +#ifdef CONFIG_ACPI >>> + acpi_id = acpi_match_device(xgene_hwmon_acpi_match, >>> &pdev->dev); >>> + if (!acpi_id) >>> + return -EINVAL; >>> + >>> + version = (int)acpi_id->driver_data; >>> +#endif >>> >> >> This leaves "version" uninitialized if CONFIG_ACPI is not defined. > > If CONFIG_ACPI is not defined, "acpi_disabled" is true. So this piece > of code is not executed >
Then why do you need the ifdef in code that isn't executed anyway if CONFIG_ACPI is not defined ? Because xgene_hwmon_acpi_match is conditional ? Not an argument, because that doesn't have to be.
Also, the compiler doesn't know that, and I am quite sure that it is going to complain that version may be used uninitialized if CONFIG_ACPI is not defined.
Sorry, I won't accept that code.
Guenter
> Thanks > Hoan > >> >> Guenter >> >> >>> if (device_property_read_u32(&pdev->dev, "pcc-channel", >>> &ctx->mbox_idx)) { >>> @@ -690,7 +714,13 @@ static int xgene_hwmon_probe(struct platform_device >>> *pdev) >>> */ >>> ctx->comm_base_addr = cppc_ss->base_address; >>> if (ctx->comm_base_addr) { >>> - ctx->pcc_comm_addr = memremap(ctx->comm_base_addr, >>> + if (version == XGENE_HWMON_V2) >>> + ctx->pcc_comm_addr = (void __force >>> *)ioremap( >>> + >>> ctx->comm_base_addr, >>> + cppc_ss->length); >>> + else >>> + ctx->pcc_comm_addr = memremap( >>> + >>> ctx->comm_base_addr, >>> cppc_ss->length, >>> MEMREMAP_WB); >>> } else { >>> @@ -758,14 +788,6 @@ static int xgene_hwmon_remove(struct platform_device >>> *pdev) >>> return 0; >>> } >>> -#ifdef CONFIG_ACPI >>> -static const struct acpi_device_id xgene_hwmon_acpi_match[] = { >>> - {"APMC0D29", 0}, >>> - {}, >>> -}; >>> -MODULE_DEVICE_TABLE(acpi, xgene_hwmon_acpi_match); >>> -#endif >>> - >>> static const struct of_device_id xgene_hwmon_of_match[] = { >>> {.compatible = "apm,xgene-slimpro-hwmon"}, >>> {} >>> >> >
| |