Re: [PATCH v4 4/5] platform/x86: intel_pmc_ipc: Fix iTCO GCS memory mapping failure

[Sathyanarayanan Kuppuswamy Natarajan <sathyaosid@gmail.com>]

Hi Andy,



On Sun, Apr 2, 2017 at 7:10 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Sat, Apr 1, 2017 at 2:27 AM, Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> iTCO watchdog driver need access to PMC_CFG GCR register to modify
>
> iTCO_wdt or use above in the rest of the series.
>
> So, choose one and use it everywhere in your patch series.

will go with iTCO_wdt.

>
>> the no reboot setting. Currently, this is done by passing PMC_CFG reg
>> address as memory resource to watchdog driver and allowing it directly
>> modify the PMC_CFG register. But currently PMC driver also has
>> requirement to memory map the entire GCR register space in this driver.
>> This causes mem request failure in watchdog driver. So this patch fixes
>> this issue by adding api to update noreboot flag and passes them
>> to watchdog driver via platform data.
>
>
>> +static int update_noreboot_bit(bool status)
>> +{
>> +       if (status)
>
>> +               return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG, BIT(4),
>> +                                           BIT(4));
>
> BIT(4) is a magic. Moreover, it's used as mask and value here, you
> might introduce temporary variables for better understanding what is
> what.
> As an example you may look at drivers/acpi/acpi_lpss.c (code related
> to IOSF MBI interaction).

I will create macros with some meaningful name to explain what BIT(4)
represents.

>
>> +       else
>
> Redundant.
>
>
> --
> With Best Regards,
> Andy Shevchenko



-- 
--

Sathya