Re: [PATCH v3] hwmon: xgene: Support hwmon v2

[Hoan Tran]

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"},
>>>>          {}
>>>>
>>>
>>
>