lkml.org 
[lkml]   [2017]   [Oct]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3] hwmon: xgene: Support hwmon v2
From
Date
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"},
>>> {}
>>>
>>
>

\
 
 \ /
  Last update: 2017-10-14 06:55    [W:0.218 / U:0.464 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site