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

From: Hoan Tran
Date: Sat Oct 14 2017 - 01:00:46 EST


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