Hi On Fri, Oct 13, 2017 at 9:54 PM, Guenter Roeck <linux@xxxxxxxxxxxx> wrote: > On 10/13/2017 09:38 PM, Hoan Tran wrote: >> >> Hi Guenter, >> >> On Fri, Oct 13, 2017 at 9:28 PM, Guenter Roeck <linux@xxxxxxxxxxxx> 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@xxxxxxx> >>>> --- >>>> 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. Yes, I'll fix it Thanks Hoan > > 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"}, >>>> {} >>>> >>> >> > -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html