public inbox for linux-i2c@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] i2c: designware-baytrail: Pass dw_i2c_dev into helper functions
@ 2016-12-09 11:01 Hans de Goede
  2016-12-09 11:01 ` [PATCH 2/2] i2c: designware-baytrail: Add support for cherrytrail Hans de Goede
  2016-12-09 11:23 ` [PATCH 1/2] i2c: designware-baytrail: Pass dw_i2c_dev into helper functions Andy Shevchenko
  0 siblings, 2 replies; 5+ messages in thread
From: Hans de Goede @ 2016-12-09 11:01 UTC (permalink / raw)
  To: Jarkko Nikula, Wolfram Sang
  Cc: Andy Shevchenko, Mika Westerberg, Takashi Iwai, linux-i2c,
	Hans de Goede

Pass dw_i2c_dev into the helper functions, this is a preparation patch
for adding cherrytrail support.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Takashi Iwai <tiwai@suse.de>
Tested-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/i2c/busses/i2c-designware-baytrail.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index 1590ad0..a3f581c 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -28,14 +28,14 @@
 
 static unsigned long acquired;
 
-static int get_sem(struct device *dev, u32 *sem)
+static int get_sem(struct dw_i2c_dev *dev, u32 *sem)
 {
 	u32 data;
 	int ret;
 
 	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, PUNIT_SEMAPHORE, &data);
 	if (ret) {
-		dev_err(dev, "iosf failed to read punit semaphore\n");
+		dev_err(dev->dev, "iosf failed to read punit semaphore\n");
 		return ret;
 	}
 
@@ -44,18 +44,18 @@ static int get_sem(struct device *dev, u32 *sem)
 	return 0;
 }
 
-static void reset_semaphore(struct device *dev)
+static void reset_semaphore(struct dw_i2c_dev *dev)
 {
 	u32 data;
 
 	if (iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, PUNIT_SEMAPHORE, &data)) {
-		dev_err(dev, "iosf failed to reset punit semaphore during read\n");
+		dev_err(dev->dev, "iosf failed to reset punit semaphore during read\n");
 		return;
 	}
 
 	data &= ~PUNIT_SEMAPHORE_BIT;
 	if (iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, PUNIT_SEMAPHORE, data))
-		dev_err(dev, "iosf failed to reset punit semaphore during write\n");
+		dev_err(dev->dev, "iosf failed to reset punit semaphore during write\n");
 }
 
 static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
@@ -83,7 +83,7 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
 	start = jiffies;
 	end = start + msecs_to_jiffies(SEMAPHORE_TIMEOUT);
 	do {
-		ret = get_sem(dev->dev, &sem);
+		ret = get_sem(dev, &sem);
 		if (!ret && sem) {
 			acquired = jiffies;
 			dev_dbg(dev->dev, "punit semaphore acquired after %ums\n",
@@ -95,7 +95,7 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
 	} while (time_before(jiffies, end));
 
 	dev_err(dev->dev, "punit semaphore timed out, resetting\n");
-	reset_semaphore(dev->dev);
+	reset_semaphore(dev);
 
 	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, PUNIT_SEMAPHORE, &sem);
 	if (ret)
@@ -116,7 +116,7 @@ static void baytrail_i2c_release(struct dw_i2c_dev *dev)
 	if (!dev->acquire_lock)
 		return;
 
-	reset_semaphore(dev->dev);
+	reset_semaphore(dev);
 	dev_dbg(dev->dev, "punit semaphore held for %ums\n",
 		jiffies_to_msecs(jiffies - acquired));
 }
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH 2/2] i2c: designware-baytrail: Add support for cherrytrail
  2016-12-09 11:01 [PATCH 1/2] i2c: designware-baytrail: Pass dw_i2c_dev into helper functions Hans de Goede
@ 2016-12-09 11:01 ` Hans de Goede
  2016-12-09 11:29   ` Andy Shevchenko
  2016-12-09 11:23 ` [PATCH 1/2] i2c: designware-baytrail: Pass dw_i2c_dev into helper functions Andy Shevchenko
  1 sibling, 1 reply; 5+ messages in thread
From: Hans de Goede @ 2016-12-09 11:01 UTC (permalink / raw)
  To: Jarkko Nikula, Wolfram Sang
  Cc: Andy Shevchenko, Mika Westerberg, Takashi Iwai, linux-i2c,
	Hans de Goede

The cherrytrail punit has the pmic i2c bus access semaphore at a
different register address.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Takashi Iwai <tiwai@suse.de>
Tested-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/i2c/busses/i2c-designware-baytrail.c |  3 ++-
 drivers/i2c/busses/i2c-designware-core.h     |  2 ++
 drivers/i2c/busses/i2c-designware-pcidrv.c   | 25 ++++++++++++++++++-------
 drivers/i2c/busses/i2c-designware-platdrv.c  |  2 +-
 4 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index a3f581c..938ff3b 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -22,7 +22,8 @@
 #include "i2c-designware-core.h"
 
 #define SEMAPHORE_TIMEOUT	100
-#define PUNIT_SEMAPHORE		0x7
+#define PUNIT_SEMAPHORE		((dev->accessor_flags & ACCESS_IS_CHERRYTRAIL) \
+				 ? 0x10e : 0x7)
 #define PUNIT_SEMAPHORE_BIT	BIT(0)
 #define PUNIT_SEMAPHORE_ACQUIRE	BIT(1)
 
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 0d44d2a..4a66959 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -123,6 +123,8 @@ struct dw_i2c_dev {
 #define ACCESS_SWAP		0x00000001
 #define ACCESS_16BIT		0x00000002
 #define ACCESS_INTR_MASK	0x00000004
+/* Not really an accessor_flag but also set through driver_data */
+#define ACCESS_IS_CHERRYTRAIL	0x00000008
 
 extern int i2c_dw_init(struct dw_i2c_dev *dev);
 extern void i2c_dw_disable(struct dw_i2c_dev *dev);
diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index 96f8230..d774bab 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -45,6 +45,7 @@ enum dw_pci_ctl_id_t {
 	medfield,
 	merrifield,
 	baytrail,
+	cherrytrail,
 	haswell,
 };
 
@@ -174,6 +175,14 @@ static struct dw_pci_controller dw_pci_controllers[] = {
 		.functionality = I2C_FUNC_10BIT_ADDR,
 		.scl_sda_cfg = &hsw_config,
 	},
+	[cherrytrail] = {
+		.bus_num = -1,
+		.bus_cfg = INTEL_MID_STD_CFG | DW_IC_CON_SPEED_FAST,
+		.tx_fifo_depth = 32,
+		.rx_fifo_depth = 32,
+		.functionality = I2C_FUNC_10BIT_ADDR,
+		.scl_sda_cfg = &byt_config,
+	},
 };
 
 #ifdef CONFIG_PM
@@ -241,6 +250,8 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 	dev->base = pcim_iomap_table(pdev)[0];
 	dev->dev = &pdev->dev;
 	dev->irq = pdev->irq;
+	if (id->driver_data == cherrytrail)
+		dev->accessor_flags |= ACCESS_IS_CHERRYTRAIL;
 
 	if (controller->setup) {
 		r = controller->setup(pdev, controller);
@@ -321,13 +332,13 @@ static const struct pci_device_id i2_designware_pci_ids[] = {
 	{ PCI_VDEVICE(INTEL, 0x9c61), haswell },
 	{ PCI_VDEVICE(INTEL, 0x9c62), haswell },
 	/* Braswell / Cherrytrail */
-	{ PCI_VDEVICE(INTEL, 0x22C1), baytrail },
-	{ PCI_VDEVICE(INTEL, 0x22C2), baytrail },
-	{ PCI_VDEVICE(INTEL, 0x22C3), baytrail },
-	{ PCI_VDEVICE(INTEL, 0x22C4), baytrail },
-	{ PCI_VDEVICE(INTEL, 0x22C5), baytrail },
-	{ PCI_VDEVICE(INTEL, 0x22C6), baytrail },
-	{ PCI_VDEVICE(INTEL, 0x22C7), baytrail },
+	{ PCI_VDEVICE(INTEL, 0x22C1), cherrytrail },
+	{ PCI_VDEVICE(INTEL, 0x22C2), cherrytrail },
+	{ PCI_VDEVICE(INTEL, 0x22C3), cherrytrail },
+	{ PCI_VDEVICE(INTEL, 0x22C4), cherrytrail },
+	{ PCI_VDEVICE(INTEL, 0x22C5), cherrytrail },
+	{ PCI_VDEVICE(INTEL, 0x22C6), cherrytrail },
+	{ PCI_VDEVICE(INTEL, 0x22C7), cherrytrail },
 	{ 0,}
 };
 MODULE_DEVICE_TABLE(pci, i2_designware_pci_ids);
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 0b42a12..8974467 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -123,7 +123,7 @@ static const struct acpi_device_id dw_i2c_acpi_match[] = {
 	{ "INT3432", 0 },
 	{ "INT3433", 0 },
 	{ "80860F41", 0 },
-	{ "808622C1", 0 },
+	{ "808622C1", ACCESS_IS_CHERRYTRAIL },
 	{ "AMD0010", ACCESS_INTR_MASK },
 	{ "AMDI0010", ACCESS_INTR_MASK },
 	{ "AMDI0510", 0 },
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/2] i2c: designware-baytrail: Pass dw_i2c_dev into helper functions
  2016-12-09 11:01 [PATCH 1/2] i2c: designware-baytrail: Pass dw_i2c_dev into helper functions Hans de Goede
  2016-12-09 11:01 ` [PATCH 2/2] i2c: designware-baytrail: Add support for cherrytrail Hans de Goede
@ 2016-12-09 11:23 ` Andy Shevchenko
  1 sibling, 0 replies; 5+ messages in thread
From: Andy Shevchenko @ 2016-12-09 11:23 UTC (permalink / raw)
  To: Hans de Goede, Jarkko Nikula, Wolfram Sang
  Cc: Mika Westerberg, Takashi Iwai, linux-i2c

On Fri, 2016-12-09 at 12:01 +0100, Hans de Goede wrote:
> Pass dw_i2c_dev into the helper functions, this is a preparation patch
> for adding cherrytrail support.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: Takashi Iwai <tiwai@suse.de>
> Tested-by: Takashi Iwai <tiwai@suse.de>

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> ---
>  drivers/i2c/busses/i2c-designware-baytrail.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c
> b/drivers/i2c/busses/i2c-designware-baytrail.c
> index 1590ad0..a3f581c 100644
> --- a/drivers/i2c/busses/i2c-designware-baytrail.c
> +++ b/drivers/i2c/busses/i2c-designware-baytrail.c
> @@ -28,14 +28,14 @@
>  
>  static unsigned long acquired;
>  
> -static int get_sem(struct device *dev, u32 *sem)
> +static int get_sem(struct dw_i2c_dev *dev, u32 *sem)
>  {
>  	u32 data;
>  	int ret;
>  
>  	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ,
> PUNIT_SEMAPHORE, &data);
>  	if (ret) {
> -		dev_err(dev, "iosf failed to read punit
> semaphore\n");
> +		dev_err(dev->dev, "iosf failed to read punit
> semaphore\n");
>  		return ret;
>  	}
>  
> @@ -44,18 +44,18 @@ static int get_sem(struct device *dev, u32 *sem)
>  	return 0;
>  }
>  
> -static void reset_semaphore(struct device *dev)
> +static void reset_semaphore(struct dw_i2c_dev *dev)
>  {
>  	u32 data;
>  
>  	if (iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ,
> PUNIT_SEMAPHORE, &data)) {
> -		dev_err(dev, "iosf failed to reset punit semaphore
> during read\n");
> +		dev_err(dev->dev, "iosf failed to reset punit
> semaphore during read\n");
>  		return;
>  	}
>  
>  	data &= ~PUNIT_SEMAPHORE_BIT;
>  	if (iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE,
> PUNIT_SEMAPHORE, data))
> -		dev_err(dev, "iosf failed to reset punit semaphore
> during write\n");
> +		dev_err(dev->dev, "iosf failed to reset punit
> semaphore during write\n");
>  }
>  
>  static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
> @@ -83,7 +83,7 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev
> *dev)
>  	start = jiffies;
>  	end = start + msecs_to_jiffies(SEMAPHORE_TIMEOUT);
>  	do {
> -		ret = get_sem(dev->dev, &sem);
> +		ret = get_sem(dev, &sem);
>  		if (!ret && sem) {
>  			acquired = jiffies;
>  			dev_dbg(dev->dev, "punit semaphore acquired
> after %ums\n",
> @@ -95,7 +95,7 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev
> *dev)
>  	} while (time_before(jiffies, end));
>  
>  	dev_err(dev->dev, "punit semaphore timed out, resetting\n");
> -	reset_semaphore(dev->dev);
> +	reset_semaphore(dev);
>  
>  	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ,
> PUNIT_SEMAPHORE, &sem);
>  	if (ret)
> @@ -116,7 +116,7 @@ static void baytrail_i2c_release(struct dw_i2c_dev
> *dev)
>  	if (!dev->acquire_lock)
>  		return;
>  
> -	reset_semaphore(dev->dev);
> +	reset_semaphore(dev);
>  	dev_dbg(dev->dev, "punit semaphore held for %ums\n",
>  		jiffies_to_msecs(jiffies - acquired));
>  }

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 2/2] i2c: designware-baytrail: Add support for cherrytrail
  2016-12-09 11:01 ` [PATCH 2/2] i2c: designware-baytrail: Add support for cherrytrail Hans de Goede
@ 2016-12-09 11:29   ` Andy Shevchenko
  2016-12-10 10:34     ` Hans de Goede
  0 siblings, 1 reply; 5+ messages in thread
From: Andy Shevchenko @ 2016-12-09 11:29 UTC (permalink / raw)
  To: Hans de Goede, Jarkko Nikula, Wolfram Sang
  Cc: Mika Westerberg, Takashi Iwai, linux-i2c

On Fri, 2016-12-09 at 12:01 +0100, Hans de Goede wrote:
> The cherrytrail punit has the pmic i2c bus access semaphore at a
> different register address.

Thanks for the patch. My comments below.

> --- a/drivers/i2c/busses/i2c-designware-baytrail.c
> +++ b/drivers/i2c/busses/i2c-designware-baytrail.c
> @@ -22,7 +22,8 @@
>  #include "i2c-designware-core.h"
>  
>  #define SEMAPHORE_TIMEOUT	100
> -#define PUNIT_SEMAPHORE		0x7
> +#define PUNIT_SEMAPHORE		((dev->accessor_flags &
> ACCESS_IS_CHERRYTRAIL) \
> +				 ? 0x10e : 0x7)

Personally I don't like this.

What if we do it in a helper function

static u32 get_sem_addr()
{
 ...
 return addr;
}

And user

{
u32 addr = get_sem_addr(...);

iosf...(..., addr, ...);
}


> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -123,6 +123,8 @@ struct dw_i2c_dev {
>  #define ACCESS_SWAP		0x00000001
>  #define ACCESS_16BIT		0x00000002
>  #define ACCESS_INTR_MASK	0x00000004
> +/* Not really an accessor_flag but also set through driver_data */
> +#define ACCESS_IS_CHERRYTRAIL	0x00000008

Don't like it either. Here two ways I see:
Introduce another member to keep non-accessor flags, or rename existing
one and create new flags with some other prefix.

>  
>  extern int i2c_dw_init(struct dw_i2c_dev *dev);
>  extern void i2c_dw_disable(struct dw_i2c_dev *dev);
> diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c
> b/drivers/i2c/busses/i2c-designware-pcidrv.c
> index 96f8230..d774bab 100644
> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c
> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
> @@ -45,6 +45,7 @@ enum dw_pci_ctl_id_t {
>  	medfield,
>  	merrifield,
>  	baytrail,
> +	cherrytrail,
>  	haswell,
>  };
>  
> @@ -174,6 +175,14 @@ static struct dw_pci_controller
> dw_pci_controllers[] = {
>  		.functionality = I2C_FUNC_10BIT_ADDR,
>  		.scl_sda_cfg = &hsw_config,
>  	},

> +	[cherrytrail] = {
> +		.bus_num = -1,
> +		.bus_cfg = INTEL_MID_STD_CFG | DW_IC_CON_SPEED_FAST,
> +		.tx_fifo_depth = 32,
> +		.rx_fifo_depth = 32,
> +		.functionality = I2C_FUNC_10BIT_ADDR,
> +		.scl_sda_cfg = &byt_config,

Would be ugly if we actually put... 

> +	},
>  };
>  
>  #ifdef CONFIG_PM
> @@ -241,6 +250,8 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
>  	dev->base = pcim_iomap_table(pdev)[0];
>  	dev->dev = &pdev->dev;
>  	dev->irq = pdev->irq;
> +	if (id->driver_data == cherrytrail)
> +		dev->accessor_flags |= ACCESS_IS_CHERRYTRAIL;

...above inside the struct dw_pci_controller?

>  
>  	if (controller->setup) {
>  		r = controller->setup(pdev, controller);
> @@ -321,13 +332,13 @@ static const struct pci_device_id
> i2_designware_pci_ids[] = {
>  	{ PCI_VDEVICE(INTEL, 0x9c61), haswell },
>  	{ PCI_VDEVICE(INTEL, 0x9c62), haswell },
>  	/* Braswell / Cherrytrail */
> -	{ PCI_VDEVICE(INTEL, 0x22C1), baytrail },
> -	{ PCI_VDEVICE(INTEL, 0x22C2), baytrail },
> -	{ PCI_VDEVICE(INTEL, 0x22C3), baytrail },
> -	{ PCI_VDEVICE(INTEL, 0x22C4), baytrail },
> -	{ PCI_VDEVICE(INTEL, 0x22C5), baytrail },
> -	{ PCI_VDEVICE(INTEL, 0x22C6), baytrail },
> -	{ PCI_VDEVICE(INTEL, 0x22C7), baytrail },
> +	{ PCI_VDEVICE(INTEL, 0x22C1), cherrytrail },
> +	{ PCI_VDEVICE(INTEL, 0x22C2), cherrytrail },
> +	{ PCI_VDEVICE(INTEL, 0x22C3), cherrytrail },
> +	{ PCI_VDEVICE(INTEL, 0x22C4), cherrytrail },
> +	{ PCI_VDEVICE(INTEL, 0x22C5), cherrytrail },
> +	{ PCI_VDEVICE(INTEL, 0x22C6), cherrytrail },
> +	{ PCI_VDEVICE(INTEL, 0x22C7), cherrytrail },
>  	{ 0,}
>  };
>  MODULE_DEVICE_TABLE(pci, i2_designware_pci_ids);
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 0b42a12..8974467 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -123,7 +123,7 @@ static const struct acpi_device_id
> dw_i2c_acpi_match[] = {
>  	{ "INT3432", 0 },
>  	{ "INT3433", 0 },
>  	{ "80860F41", 0 },
> -	{ "808622C1", 0 },
> +	{ "808622C1", ACCESS_IS_CHERRYTRAIL },
>  	{ "AMD0010", ACCESS_INTR_MASK },
>  	{ "AMDI0010", ACCESS_INTR_MASK },
>  	{ "AMDI0510", 0 },

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 2/2] i2c: designware-baytrail: Add support for cherrytrail
  2016-12-09 11:29   ` Andy Shevchenko
@ 2016-12-10 10:34     ` Hans de Goede
  0 siblings, 0 replies; 5+ messages in thread
From: Hans de Goede @ 2016-12-10 10:34 UTC (permalink / raw)
  To: Andy Shevchenko, Jarkko Nikula, Wolfram Sang
  Cc: Mika Westerberg, Takashi Iwai, linux-i2c

Hi,

On 09-12-16 12:29, Andy Shevchenko wrote:
> On Fri, 2016-12-09 at 12:01 +0100, Hans de Goede wrote:
>> The cherrytrail punit has the pmic i2c bus access semaphore at a
>> different register address.
>
> Thanks for the patch. My comments below.
>
>> --- a/drivers/i2c/busses/i2c-designware-baytrail.c
>> +++ b/drivers/i2c/busses/i2c-designware-baytrail.c
>> @@ -22,7 +22,8 @@
>>  #include "i2c-designware-core.h"
>>
>>  #define SEMAPHORE_TIMEOUT	100
>> -#define PUNIT_SEMAPHORE		0x7
>> +#define PUNIT_SEMAPHORE		((dev->accessor_flags &
>> ACCESS_IS_CHERRYTRAIL) \
>> +				 ? 0x10e : 0x7)
>
> Personally I don't like this.
>
> What if we do it in a helper function
>
> static u32 get_sem_addr()
> {
>  ...
>  return addr;
> }
>
> And user
>
> {
> u32 addr = get_sem_addr(...);
>
> iosf...(..., addr, ...);
> }

Ok I will change this for the next version.

>
>
>> --- a/drivers/i2c/busses/i2c-designware-core.h
>> +++ b/drivers/i2c/busses/i2c-designware-core.h
>> @@ -123,6 +123,8 @@ struct dw_i2c_dev {
>>  #define ACCESS_SWAP		0x00000001
>>  #define ACCESS_16BIT		0x00000002
>>  #define ACCESS_INTR_MASK	0x00000004
>> +/* Not really an accessor_flag but also set through driver_data */
>> +#define ACCESS_IS_CHERRYTRAIL	0x00000008
>
> Don't like it either. Here two ways I see:
> Introduce another member to keep non-accessor flags, or rename existing
> one and create new flags with some other prefix.

Ok, I will introduce a new patch in the next version of the
series which renames the flags field as preparation
for the other changes.


>
>>
>>  extern int i2c_dw_init(struct dw_i2c_dev *dev);
>>  extern void i2c_dw_disable(struct dw_i2c_dev *dev);
>> diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c
>> b/drivers/i2c/busses/i2c-designware-pcidrv.c
>> index 96f8230..d774bab 100644
>> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c
>> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
>> @@ -45,6 +45,7 @@ enum dw_pci_ctl_id_t {
>>  	medfield,
>>  	merrifield,
>>  	baytrail,
>> +	cherrytrail,
>>  	haswell,
>>  };
>>
>> @@ -174,6 +175,14 @@ static struct dw_pci_controller
>> dw_pci_controllers[] = {
>>  		.functionality = I2C_FUNC_10BIT_ADDR,
>>  		.scl_sda_cfg = &hsw_config,
>>  	},
>
>> +	[cherrytrail] = {
>> +		.bus_num = -1,
>> +		.bus_cfg = INTEL_MID_STD_CFG | DW_IC_CON_SPEED_FAST,
>> +		.tx_fifo_depth = 32,
>> +		.rx_fifo_depth = 32,
>> +		.functionality = I2C_FUNC_10BIT_ADDR,
>> +		.scl_sda_cfg = &byt_config,
>
> Would be ugly if we actually put...
>
>> +	},
>>  };
>>
>>  #ifdef CONFIG_PM
>> @@ -241,6 +250,8 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
>>  	dev->base = pcim_iomap_table(pdev)[0];
>>  	dev->dev = &pdev->dev;
>>  	dev->irq = pdev->irq;
>> +	if (id->driver_data == cherrytrail)
>> +		dev->accessor_flags |= ACCESS_IS_CHERRYTRAIL;
>
> ...above inside the struct dw_pci_controller?

Ack.

Note BTW that while trying to get battery readings from the axp288
pmic on my tablet now that the punit semaphore is working I repeatedly
locked up my tablet. Without any i2c client drivers loaded a simple
"i2cdump -y 14 0x34" would lookup the tablet in 1 - 3 runs guaranteed,
and that is only reading from the pmic.

I managed to find a fix for this which I will include (as a separate
patch) in the next version of my series. This will also hopefully
fix the infamous: https://bugzilla.kernel.org/show_bug.cgi?id=109051,
as I could also make things work without my fix with
"intel_idle.max_cstate=0" so they seem to be related.

Regards,

Hans





>
>>
>>  	if (controller->setup) {
>>  		r = controller->setup(pdev, controller);
>> @@ -321,13 +332,13 @@ static const struct pci_device_id
>> i2_designware_pci_ids[] = {
>>  	{ PCI_VDEVICE(INTEL, 0x9c61), haswell },
>>  	{ PCI_VDEVICE(INTEL, 0x9c62), haswell },
>>  	/* Braswell / Cherrytrail */
>> -	{ PCI_VDEVICE(INTEL, 0x22C1), baytrail },
>> -	{ PCI_VDEVICE(INTEL, 0x22C2), baytrail },
>> -	{ PCI_VDEVICE(INTEL, 0x22C3), baytrail },
>> -	{ PCI_VDEVICE(INTEL, 0x22C4), baytrail },
>> -	{ PCI_VDEVICE(INTEL, 0x22C5), baytrail },
>> -	{ PCI_VDEVICE(INTEL, 0x22C6), baytrail },
>> -	{ PCI_VDEVICE(INTEL, 0x22C7), baytrail },
>> +	{ PCI_VDEVICE(INTEL, 0x22C1), cherrytrail },
>> +	{ PCI_VDEVICE(INTEL, 0x22C2), cherrytrail },
>> +	{ PCI_VDEVICE(INTEL, 0x22C3), cherrytrail },
>> +	{ PCI_VDEVICE(INTEL, 0x22C4), cherrytrail },
>> +	{ PCI_VDEVICE(INTEL, 0x22C5), cherrytrail },
>> +	{ PCI_VDEVICE(INTEL, 0x22C6), cherrytrail },
>> +	{ PCI_VDEVICE(INTEL, 0x22C7), cherrytrail },
>>  	{ 0,}
>>  };
>>  MODULE_DEVICE_TABLE(pci, i2_designware_pci_ids);
>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
>> b/drivers/i2c/busses/i2c-designware-platdrv.c
>> index 0b42a12..8974467 100644
>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>> @@ -123,7 +123,7 @@ static const struct acpi_device_id
>> dw_i2c_acpi_match[] = {
>>  	{ "INT3432", 0 },
>>  	{ "INT3433", 0 },
>>  	{ "80860F41", 0 },
>> -	{ "808622C1", 0 },
>> +	{ "808622C1", ACCESS_IS_CHERRYTRAIL },
>>  	{ "AMD0010", ACCESS_INTR_MASK },
>>  	{ "AMDI0010", ACCESS_INTR_MASK },
>>  	{ "AMDI0510", 0 },
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2016-12-10 10:34 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-12-09 11:01 [PATCH 1/2] i2c: designware-baytrail: Pass dw_i2c_dev into helper functions Hans de Goede
2016-12-09 11:01 ` [PATCH 2/2] i2c: designware-baytrail: Add support for cherrytrail Hans de Goede
2016-12-09 11:29   ` Andy Shevchenko
2016-12-10 10:34     ` Hans de Goede
2016-12-09 11:23 ` [PATCH 1/2] i2c: designware-baytrail: Pass dw_i2c_dev into helper functions Andy Shevchenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox