[PATCH v1 1/4] platform/x86: intel_pmc_ipc: Use BIT() macro

[PATCH v1 1/4] platform/x86: intel_pmc_ipc: Use BIT() macro

[Andy Shevchenko]

Use BIT() and BIT_MASK() macros for definitions.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/intel_pmc_ipc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
index d37cbd1cf58c..eb0b342996ca 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -41,13 +41,13 @@
  * the IPC1 registers, updates the IPC_STS response register with the status.
  */
 #define IPC_CMD			0x0
-#define		IPC_CMD_MSI		0x100
+#define		IPC_CMD_MSI		BIT(8)
 #define		IPC_CMD_SIZE		16
 #define		IPC_CMD_SUBCMD		12
 #define IPC_STATUS		0x04
-#define		IPC_STATUS_IRQ		0x4
-#define		IPC_STATUS_ERR		0x2
-#define		IPC_STATUS_BUSY		0x1
+#define		IPC_STATUS_IRQ		BIT(2)
+#define		IPC_STATUS_ERR		BIT(1)
+#define		IPC_STATUS_BUSY		BIT(0)
 #define IPC_SPTR		0x08
 #define IPC_DPTR		0x0C
 #define IPC_WRITE_BUFFER	0x80
@@ -107,7 +107,7 @@
 /* PMC register bit definitions */
 
 /* PMC_CFG_REG bit masks */
-#define PMC_CFG_NO_REBOOT_MASK		(1 << 4)
+#define PMC_CFG_NO_REBOOT_MASK		BIT_MASK(4)
 #define PMC_CFG_NO_REBOOT_EN		(1 << 4)
 #define PMC_CFG_NO_REBOOT_DIS		(0 << 4)
 
-- 
2.20.1

[PATCH v1 2/4] platform/x86: intel_pmc_ipc: Apply same width for offset definitions

[Andy Shevchenko]

Apply same width for offset definitions to make code more consistent.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/intel_pmc_ipc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
index eb0b342996ca..9007aa717586 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -40,7 +40,7 @@
  * The ARC handles the interrupt and services it, writing optional data to
  * the IPC1 registers, updates the IPC_STS response register with the status.
  */
-#define IPC_CMD			0x0
+#define IPC_CMD			0x00
 #define		IPC_CMD_MSI		BIT(8)
 #define		IPC_CMD_SIZE		16
 #define		IPC_CMD_SUBCMD		12
@@ -101,8 +101,8 @@
 #define TELEM_SSRAM_SIZE		240
 #define TELEM_PMC_SSRAM_OFFSET		0x1B00
 #define TELEM_PUNIT_SSRAM_OFFSET	0x1A00
-#define TCO_PMC_OFFSET			0x8
-#define TCO_PMC_SIZE			0x4
+#define TCO_PMC_OFFSET			0x08
+#define TCO_PMC_SIZE			0x04
 
 /* PMC register bit definitions */
 
-- 
2.20.1

Re: [PATCH v1 2/4] platform/x86: intel_pmc_ipc: Apply same width for offset definitions

[sathyanarayanan kuppuswamy]

Hi,

On 4/9/19 4:25 AM, Andy Shevchenko wrote:
> Apply same width for offset definitions to make code more consistent.
Looks good.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Kuppuswamy Sathyanarayanan 
<sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>   drivers/platform/x86/intel_pmc_ipc.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
> index eb0b342996ca..9007aa717586 100644
> --- a/drivers/platform/x86/intel_pmc_ipc.c
> +++ b/drivers/platform/x86/intel_pmc_ipc.c
> @@ -40,7 +40,7 @@
>    * The ARC handles the interrupt and services it, writing optional data to
>    * the IPC1 registers, updates the IPC_STS response register with the status.
>    */
> -#define IPC_CMD			0x0
> +#define IPC_CMD			0x00
>   #define		IPC_CMD_MSI		BIT(8)
>   #define		IPC_CMD_SIZE		16
>   #define		IPC_CMD_SUBCMD		12
> @@ -101,8 +101,8 @@
>   #define TELEM_SSRAM_SIZE		240
>   #define TELEM_PMC_SSRAM_OFFSET		0x1B00
>   #define TELEM_PUNIT_SSRAM_OFFSET	0x1A00
> -#define TCO_PMC_OFFSET			0x8
> -#define TCO_PMC_SIZE			0x4
> +#define TCO_PMC_OFFSET			0x08
> +#define TCO_PMC_SIZE			0x04
>   
>   /* PMC register bit definitions */
>   

-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

[PATCH v1 3/4] platform/x86: intel_pmc_ipc: Don't map non-used optional resources

[Andy Shevchenko]

The intel_pmc_ipc driver has a placeholder for all possible resources
that may have been provided by ACPI. Since there are few optional ones,
the driver still uses them and binds to wrong ranges in resource tree:

  # grep intel_punit_ipc /proc/iomem
  00000000-00000000 : intel_punit_ipc
    00000000-00000000 : intel_punit_ipc
      00000000-00000000 : intel_punit_ipc
        00000000-00000000 : intel_punit_ipc

This leads to issues with resource management during inserting and
removing modules, such as intel_pmc_ipc itself, which can't be inserted
anymore after first removal.

Count the actual resources provided and supply only them to the child device.

This is a real fix of the commit 8cc7fb4a6523

  ("intel_pmc_ipc: update acpi resource structure for Punit")

that also fixes a symptoms described in the commit 6cc8cbbc8868

  ("platform/x86: intel_punit_ipc: Fix resource ioremap warning")

that is going to be reverted afterwards.

Reported-by: Junxiao Chang <junxiao.chang@intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Qipeng Zha <qipeng.zha@intel.com>
Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/platform/x86/intel_pmc_ipc.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
index 9007aa717586..55037ff258f8 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -131,6 +131,7 @@ static struct intel_pmc_ipc_dev {
 
 	/* punit */
 	struct platform_device *punit_dev;
+	unsigned int punit_res_count;
 
 	/* Telemetry */
 	resource_size_t telem_pmc_ssram_base;
@@ -682,7 +683,7 @@ static int ipc_create_punit_device(void)
 		.name = PUNIT_DEVICE_NAME,
 		.id = -1,
 		.res = punit_res_array,
-		.num_res = ARRAY_SIZE(punit_res_array),
+		.num_res = ipcdev.punit_res_count,
 		};
 
 	pdev = platform_device_register_full(&pdevinfo);
@@ -789,7 +790,7 @@ static int ipc_create_pmc_devices(void)
 
 static int ipc_plat_get_res(struct platform_device *pdev)
 {
-	struct resource *res, *punit_res;
+	struct resource *res, *punit_res = punit_res_array;
 	void __iomem *addr;
 	int size;
 
@@ -804,7 +805,8 @@ static int ipc_plat_get_res(struct platform_device *pdev)
 	ipcdev.acpi_io_size = size;
 	dev_info(&pdev->dev, "io res: %pR\n", res);
 
-	punit_res = punit_res_array;
+	ipcdev.punit_res_count = 0;
+
 	/* This is index 0 to cover BIOS data register */
 	res = platform_get_resource(pdev, IORESOURCE_MEM,
 				    PLAT_RESOURCE_BIOS_DATA_INDEX);
@@ -812,7 +814,7 @@ static int ipc_plat_get_res(struct platform_device *pdev)
 		dev_err(&pdev->dev, "Failed to get res of punit BIOS data\n");
 		return -ENXIO;
 	}
-	*punit_res = *res;
+	punit_res[ipcdev.punit_res_count++] = *res;
 	dev_info(&pdev->dev, "punit BIOS data res: %pR\n", res);
 
 	/* This is index 1 to cover BIOS interface register */
@@ -822,42 +824,38 @@ static int ipc_plat_get_res(struct platform_device *pdev)
 		dev_err(&pdev->dev, "Failed to get res of punit BIOS iface\n");
 		return -ENXIO;
 	}
-	*++punit_res = *res;
+	punit_res[ipcdev.punit_res_count++] = *res;
 	dev_info(&pdev->dev, "punit BIOS interface res: %pR\n", res);
 
 	/* This is index 2 to cover ISP data register, optional */
 	res = platform_get_resource(pdev, IORESOURCE_MEM,
 				    PLAT_RESOURCE_ISP_DATA_INDEX);
-	++punit_res;
 	if (res) {
-		*punit_res = *res;
+		punit_res[ipcdev.punit_res_count++] = *res;
 		dev_info(&pdev->dev, "punit ISP data res: %pR\n", res);
 	}
 
 	/* This is index 3 to cover ISP interface register, optional */
 	res = platform_get_resource(pdev, IORESOURCE_MEM,
 				    PLAT_RESOURCE_ISP_IFACE_INDEX);
-	++punit_res;
 	if (res) {
-		*punit_res = *res;
+		punit_res[ipcdev.punit_res_count++] = *res;
 		dev_info(&pdev->dev, "punit ISP interface res: %pR\n", res);
 	}
 
 	/* This is index 4 to cover GTD data register, optional */
 	res = platform_get_resource(pdev, IORESOURCE_MEM,
 				    PLAT_RESOURCE_GTD_DATA_INDEX);
-	++punit_res;
 	if (res) {
-		*punit_res = *res;
+		punit_res[ipcdev.punit_res_count++] = *res;
 		dev_info(&pdev->dev, "punit GTD data res: %pR\n", res);
 	}
 
 	/* This is index 5 to cover GTD interface register, optional */
 	res = platform_get_resource(pdev, IORESOURCE_MEM,
 				    PLAT_RESOURCE_GTD_IFACE_INDEX);
-	++punit_res;
 	if (res) {
-		*punit_res = *res;
+		punit_res[ipcdev.punit_res_count++] = *res;
 		dev_info(&pdev->dev, "punit GTD interface res: %pR\n", res);
 	}
 
-- 
2.20.1

Re: [PATCH v1 3/4] platform/x86: intel_pmc_ipc: Don't map non-used optional resources

[sathyanarayanan kuppuswamy]

Hi,

On 4/9/19 4:25 AM, Andy Shevchenko wrote:
> The intel_pmc_ipc driver has a placeholder for all possible resources
> that may have been provided by ACPI. Since there are few optional ones,
> the driver still uses them and binds to wrong ranges in resource tree:
>
>    # grep intel_punit_ipc /proc/iomem
>    00000000-00000000 : intel_punit_ipc
>      00000000-00000000 : intel_punit_ipc
>        00000000-00000000 : intel_punit_ipc
>          00000000-00000000 : intel_punit_ipc
>
> This leads to issues with resource management during inserting and
> removing modules, such as intel_pmc_ipc itself, which can't be inserted
> anymore after first removal.
>
> Count the actual resources provided and supply only them to the child device.
>
> This is a real fix of the commit 8cc7fb4a6523
>
>    ("intel_pmc_ipc: update acpi resource structure for Punit")
>
> that also fixes a symptoms described in the commit 6cc8cbbc8868
>
>    ("platform/x86: intel_punit_ipc: Fix resource ioremap warning")
>
> that is going to be reverted afterwards.

Looks good to me.

Reviewed-by: Kuppuswamy Sathyanarayanan 
<sathyanarayanan.kuppuswamy@linux.intel.com>

>
> Reported-by: Junxiao Chang <junxiao.chang@intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Qipeng Zha <qipeng.zha@intel.com>
> Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>   drivers/platform/x86/intel_pmc_ipc.c | 24 +++++++++++-------------
>   1 file changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
> index 9007aa717586..55037ff258f8 100644
> --- a/drivers/platform/x86/intel_pmc_ipc.c
> +++ b/drivers/platform/x86/intel_pmc_ipc.c
> @@ -131,6 +131,7 @@ static struct intel_pmc_ipc_dev {
>   
>   	/* punit */
>   	struct platform_device *punit_dev;
> +	unsigned int punit_res_count;
>   
>   	/* Telemetry */
>   	resource_size_t telem_pmc_ssram_base;
> @@ -682,7 +683,7 @@ static int ipc_create_punit_device(void)
>   		.name = PUNIT_DEVICE_NAME,
>   		.id = -1,
>   		.res = punit_res_array,
> -		.num_res = ARRAY_SIZE(punit_res_array),
> +		.num_res = ipcdev.punit_res_count,
>   		};
>   
>   	pdev = platform_device_register_full(&pdevinfo);
> @@ -789,7 +790,7 @@ static int ipc_create_pmc_devices(void)
>   
>   static int ipc_plat_get_res(struct platform_device *pdev)
>   {
> -	struct resource *res, *punit_res;
> +	struct resource *res, *punit_res = punit_res_array;
>   	void __iomem *addr;
>   	int size;
>   
> @@ -804,7 +805,8 @@ static int ipc_plat_get_res(struct platform_device *pdev)
>   	ipcdev.acpi_io_size = size;
>   	dev_info(&pdev->dev, "io res: %pR\n", res);
>   
> -	punit_res = punit_res_array;
> +	ipcdev.punit_res_count = 0;
> +
>   	/* This is index 0 to cover BIOS data register */
>   	res = platform_get_resource(pdev, IORESOURCE_MEM,
>   				    PLAT_RESOURCE_BIOS_DATA_INDEX);
> @@ -812,7 +814,7 @@ static int ipc_plat_get_res(struct platform_device *pdev)
>   		dev_err(&pdev->dev, "Failed to get res of punit BIOS data\n");
>   		return -ENXIO;
>   	}
> -	*punit_res = *res;
> +	punit_res[ipcdev.punit_res_count++] = *res;
>   	dev_info(&pdev->dev, "punit BIOS data res: %pR\n", res);
>   
>   	/* This is index 1 to cover BIOS interface register */
> @@ -822,42 +824,38 @@ static int ipc_plat_get_res(struct platform_device *pdev)
>   		dev_err(&pdev->dev, "Failed to get res of punit BIOS iface\n");
>   		return -ENXIO;
>   	}
> -	*++punit_res = *res;
> +	punit_res[ipcdev.punit_res_count++] = *res;
>   	dev_info(&pdev->dev, "punit BIOS interface res: %pR\n", res);
>   
>   	/* This is index 2 to cover ISP data register, optional */
>   	res = platform_get_resource(pdev, IORESOURCE_MEM,
>   				    PLAT_RESOURCE_ISP_DATA_INDEX);
> -	++punit_res;
>   	if (res) {
> -		*punit_res = *res;
> +		punit_res[ipcdev.punit_res_count++] = *res;
>   		dev_info(&pdev->dev, "punit ISP data res: %pR\n", res);
>   	}
>   
>   	/* This is index 3 to cover ISP interface register, optional */
>   	res = platform_get_resource(pdev, IORESOURCE_MEM,
>   				    PLAT_RESOURCE_ISP_IFACE_INDEX);
> -	++punit_res;
>   	if (res) {
> -		*punit_res = *res;
> +		punit_res[ipcdev.punit_res_count++] = *res;
>   		dev_info(&pdev->dev, "punit ISP interface res: %pR\n", res);
>   	}
>   
>   	/* This is index 4 to cover GTD data register, optional */
>   	res = platform_get_resource(pdev, IORESOURCE_MEM,
>   				    PLAT_RESOURCE_GTD_DATA_INDEX);
> -	++punit_res;
>   	if (res) {
> -		*punit_res = *res;
> +		punit_res[ipcdev.punit_res_count++] = *res;
>   		dev_info(&pdev->dev, "punit GTD data res: %pR\n", res);
>   	}
>   
>   	/* This is index 5 to cover GTD interface register, optional */
>   	res = platform_get_resource(pdev, IORESOURCE_MEM,
>   				    PLAT_RESOURCE_GTD_IFACE_INDEX);
> -	++punit_res;
>   	if (res) {
> -		*punit_res = *res;
> +		punit_res[ipcdev.punit_res_count++] = *res;
>   		dev_info(&pdev->dev, "punit GTD interface res: %pR\n", res);
>   	}
>   

-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

[PATCH v1 4/4] platform/x86: intel_punit_ipc: Revert "Fix resource ioremap warning"

[Andy Shevchenko]

Since we have a proper fix for intel_pmc_ipc driver for resource management,
get rid of unneeded commit in the intel_punit_ipc driver.

This reverts commit 6cc8cbbc8868033f279b63e98b26b75eaa0006ab.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/intel_punit_ipc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/intel_punit_ipc.c b/drivers/platform/x86/intel_punit_ipc.c
index 79671927f4ef..ab7ae1950867 100644
--- a/drivers/platform/x86/intel_punit_ipc.c
+++ b/drivers/platform/x86/intel_punit_ipc.c
@@ -252,28 +252,28 @@ static int intel_punit_get_bars(struct platform_device *pdev)
 	 * - GTDRIVER_IPC BASE_IFACE
 	 */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
-	if (res && resource_size(res) > 1) {
+	if (res) {
 		addr = devm_ioremap_resource(&pdev->dev, res);
 		if (!IS_ERR(addr))
 			punit_ipcdev->base[ISPDRIVER_IPC][BASE_DATA] = addr;
 	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 3);
-	if (res && resource_size(res) > 1) {
+	if (res) {
 		addr = devm_ioremap_resource(&pdev->dev, res);
 		if (!IS_ERR(addr))
 			punit_ipcdev->base[ISPDRIVER_IPC][BASE_IFACE] = addr;
 	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 4);
-	if (res && resource_size(res) > 1) {
+	if (res) {
 		addr = devm_ioremap_resource(&pdev->dev, res);
 		if (!IS_ERR(addr))
 			punit_ipcdev->base[GTDRIVER_IPC][BASE_DATA] = addr;
 	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 5);
-	if (res && resource_size(res) > 1) {
+	if (res) {
 		addr = devm_ioremap_resource(&pdev->dev, res);
 		if (!IS_ERR(addr))
 			punit_ipcdev->base[GTDRIVER_IPC][BASE_IFACE] = addr;
-- 
2.20.1

Re: [PATCH v1 4/4] platform/x86: intel_punit_ipc: Revert "Fix resource ioremap warning"

[sathyanarayanan kuppuswamy]

Hi,

On 4/9/19 4:25 AM, Andy Shevchenko wrote:
> Since we have a proper fix for intel_pmc_ipc driver for resource management,
> get rid of unneeded commit in the intel_punit_ipc driver.
>
> This reverts commit 6cc8cbbc8868033f279b63e98b26b75eaa0006ab.
Reviewed-by: Kuppuswamy Sathyanarayanan 
<sathyanarayanan.kuppuswamy@linux.intel.com>
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   drivers/platform/x86/intel_punit_ipc.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/platform/x86/intel_punit_ipc.c b/drivers/platform/x86/intel_punit_ipc.c
> index 79671927f4ef..ab7ae1950867 100644
> --- a/drivers/platform/x86/intel_punit_ipc.c
> +++ b/drivers/platform/x86/intel_punit_ipc.c
> @@ -252,28 +252,28 @@ static int intel_punit_get_bars(struct platform_device *pdev)
>   	 * - GTDRIVER_IPC BASE_IFACE
>   	 */
>   	res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> -	if (res && resource_size(res) > 1) {
> +	if (res) {
>   		addr = devm_ioremap_resource(&pdev->dev, res);
>   		if (!IS_ERR(addr))
>   			punit_ipcdev->base[ISPDRIVER_IPC][BASE_DATA] = addr;
>   	}
>   
>   	res = platform_get_resource(pdev, IORESOURCE_MEM, 3);
> -	if (res && resource_size(res) > 1) {
> +	if (res) {
>   		addr = devm_ioremap_resource(&pdev->dev, res);
>   		if (!IS_ERR(addr))
>   			punit_ipcdev->base[ISPDRIVER_IPC][BASE_IFACE] = addr;
>   	}
>   
>   	res = platform_get_resource(pdev, IORESOURCE_MEM, 4);
> -	if (res && resource_size(res) > 1) {
> +	if (res) {
>   		addr = devm_ioremap_resource(&pdev->dev, res);
>   		if (!IS_ERR(addr))
>   			punit_ipcdev->base[GTDRIVER_IPC][BASE_DATA] = addr;
>   	}
>   
>   	res = platform_get_resource(pdev, IORESOURCE_MEM, 5);
> -	if (res && resource_size(res) > 1) {
> +	if (res) {
>   		addr = devm_ioremap_resource(&pdev->dev, res);
>   		if (!IS_ERR(addr))
>   			punit_ipcdev->base[GTDRIVER_IPC][BASE_IFACE] = addr;

-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

Re: [PATCH v1 1/4] platform/x86: intel_pmc_ipc: Use BIT() macro

[Andy Shevchenko]

On Tue, Apr 09, 2019 at 10:07:35AM -0700, sathyanarayanan kuppuswamy wrote:
> On 4/9/19 4:25 AM, Andy Shevchenko wrote:
> > Use BIT() and BIT_MASK() macros for definitions.
> Looks good to me.

Thanks!

> >   /* PMC register bit definitions */
> >   /* PMC_CFG_REG bit masks */
> > -#define PMC_CFG_NO_REBOOT_MASK		(1 << 4)
> > +#define PMC_CFG_NO_REBOOT_MASK		BIT_MASK(4)
> >   #define PMC_CFG_NO_REBOOT_EN		(1 << 4)
> >   #define PMC_CFG_NO_REBOOT_DIS		(0 << 4)
> Do we need 0 << 4 ?

Yes, to explicitly show that this is a value for NO_REBOOT masked bit(s)
(single bit in this case).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/4] platform/x86: intel_pmc_ipc: Use BIT() macro

[Andy Shevchenko]

On Tue, Apr 09, 2019 at 08:39:39PM +0300, Andy Shevchenko wrote:
> On Tue, Apr 09, 2019 at 10:07:35AM -0700, sathyanarayanan kuppuswamy wrote:
> > On 4/9/19 4:25 AM, Andy Shevchenko wrote:
> > > Use BIT() and BIT_MASK() macros for definitions.
> > Looks good to me.
> 
> Thanks!

If you have no further comments, can you provide your tag here?

> 
> > >   /* PMC register bit definitions */
> > >   /* PMC_CFG_REG bit masks */
> > > -#define PMC_CFG_NO_REBOOT_MASK		(1 << 4)
> > > +#define PMC_CFG_NO_REBOOT_MASK		BIT_MASK(4)
> > >   #define PMC_CFG_NO_REBOOT_EN		(1 << 4)
> > >   #define PMC_CFG_NO_REBOOT_DIS		(0 << 4)
> > Do we need 0 << 4 ?
> 
> Yes, to explicitly show that this is a value for NO_REBOOT masked bit(s)
> (single bit in this case).
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/4] platform/x86: intel_pmc_ipc: Use BIT() macro

[sathyanarayanan kuppuswamy]

On 4/10/19 6:59 AM, Andy Shevchenko wrote:
> On Tue, Apr 09, 2019 at 08:39:39PM +0300, Andy Shevchenko wrote:
>> On Tue, Apr 09, 2019 at 10:07:35AM -0700, sathyanarayanan kuppuswamy wrote:
>>> On 4/9/19 4:25 AM, Andy Shevchenko wrote:
>>>> Use BIT() and BIT_MASK() macros for definitions.
>>> Looks good to me.
>> Thanks!
> If you have no further comments, can you provide your tag here?
Reviewed-by: Kuppuswamy Sathyanarayanan 
<sathyanarayanan.kuppuswamy@linux.intel.com>
>
>>>>    /* PMC register bit definitions */
>>>>    /* PMC_CFG_REG bit masks */
>>>> -#define PMC_CFG_NO_REBOOT_MASK		(1 << 4)
>>>> +#define PMC_CFG_NO_REBOOT_MASK		BIT_MASK(4)
>>>>    #define PMC_CFG_NO_REBOOT_EN		(1 << 4)
>>>>    #define PMC_CFG_NO_REBOOT_DIS		(0 << 4)
>>> Do we need 0 << 4 ?
>> Yes, to explicitly show that this is a value for NO_REBOOT masked bit(s)
>> (single bit in this case).
>>
>> -- 
>> With Best Regards,
>> Andy Shevchenko
>>
>>
-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer