Re: [PATCH v2 00/16] intel-lpss: support non-ACPI platforms

[Mika Westerberg]

On Wed, Jan 06, 2016 at 08:19:49AM -0800, Laura Abbott wrote:
> I picked up all the patches from the device-properties merge but the
> problem still shows up. Are there others I should pick up? Hardware
> details about the touchpad are at 
> https://bugzilla.redhat.com/show_bug.cgi?id=1275718#c34

Can you get full dmesg of the failure and acpidump as well? I did not
find those from the bug report.

Also dmesg of non-failure case would be nice.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/2] i2c:dw: Add APM X-Gene ACPI I2C device support

[Mika Westerberg]

On Wed, Dec 23, 2015 at 05:34:01PM +0800, Ken Xue wrote:
> 1) Regarding
> https://msdn.microsoft.com/en-us/library/windows/hardware/dn919852(v=vs.85).aspx
> , Window I2C driver should pass MITT test. There are 5 I2C devices
> connect to one I2C bus for test. And those devices defined different
> "ConnectionSpeed" over the I2C bus by ACPI resource "I2CSerialBus".
> 
> During test, I2C bus should run in different "ConnectionSpeed" of
> device.
> 
> That means windows driver can modify I2C bus speed to match the
> "ConnectionSpeed" of device on-the-fly. Static value from SSCN and FMCN
> can not work for WITT test cases.

That is why there are *CNT methods for all supported I2C modes:

  - SSCN() - returns for standard mode (100kHz)
  - FMCN() - returns for fast mode (400kHz)
  - FPCN() - returns for fast mode+ (1MHz)
 
for High-speed mode I'm not sure what the method name is ;-)

Then the Windows driver switches between those based on what the
ConnectionSpeed is in the ACPI I2C connector.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/2] i2c:dw: Add APM X-Gene ACPI I2C device support

[Mika Westerberg]

On Wed, Dec 23, 2015 at 10:02:04AM +, Zheng, Ivan wrote:
> Why/how can Linux driver make use of such non-ACPI defined methods?

What do you mean exactly?

I added support for these (well, SSCN and FMCN) because I found out that
some vendors include such methods with their BIOS implementation.
Windows driver uses these if present and so do we.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/2] i2c:dw: Add APM X-Gene ACPI I2C device support

[Mika Westerberg]

On Wed, Dec 23, 2015 at 05:59:49PM +0800, Ken Xue wrote:
> On Wed, 2015-12-23 at 11:52 +0200, Mika Westerberg wrote:
> > On Wed, Dec 23, 2015 at 05:34:01PM +0800, Ken Xue wrote:
> > > 1) Regarding
> > > https://msdn.microsoft.com/en-us/library/windows/hardware/dn919852(v=vs.85).aspx
> > > , Window I2C driver should pass MITT test. There are 5 I2C devices
> > > connect to one I2C bus for test. And those devices defined different
> > > "ConnectionSpeed" over the I2C bus by ACPI resource "I2CSerialBus".
> > > 
> > > During test, I2C bus should run in different "ConnectionSpeed" of
> > > device.
> > > 
> > > That means windows driver can modify I2C bus speed to match the
> > > "ConnectionSpeed" of device on-the-fly. Static value from SSCN and FMCN
> > > can not work for WITT test cases.
> > 
> > That is why there are *CNT methods for all supported I2C modes:
> > 
> >   - SSCN() - returns for standard mode (100kHz)
> >   - FMCN() - returns for fast mode (400kHz)
> >   - FPCN() - returns for fast mode+ (1MHz)
> >  
> > for High-speed mode I'm not sure what the method name is ;-)
> > 
> > Then the Windows driver switches between those based on what the
> > ConnectionSpeed is in the ACPI I2C connector.
> 
> Window driver can set Bus speed based on "ConnectionSpeed". But Current
> Linux driver only sets Bus speed during probe. How can Linux diver
> determine which Bus speed should be applied, if all *CNT methods return
> non-zero? 

By default Linux driver uses 400kHz so it picks values returned from
FMCN(). You can hack clk_freq in the driver to use 100kHz which then
picks SSCN().

There is no support for reading ConnectionSpeed in Linux yet.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/2] i2c:dw: Add APM X-Gene ACPI I2C device support

[Mika Westerberg]

On Wed, Dec 23, 2015 at 11:24:44AM +, Zheng, Ivan wrote:
> I'm a BIOS engineer and my point is that such non-ACPI defined methods
> rely on BIOS implementation, so how the generic I2C driver works on a
> platform without such methods? Should the vendor implement their own
> driver in such case?

Those methods are by *no* means meant for generic driver. They are
specific to DesignWare I2C driver.

If you need to specify bus speed that the device is supposed to use,
you put it to the I2cSerialBus() resource (ConnectionSpeed)...

> I think Windows driver use ConnectionSpeed field in I2CSerialBus
> declaration for a certain device and it's defined in ACPI 5.0.

..like you say here.

We just need to add support for ConnectionSpeed to Linux I2C core.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] i2c: designware: Do not require clock when SSCN and FFCN are provided

[Mika Westerberg]

On Tue, Dec 22, 2015 at 02:51:13PM -0600, Suravee Suthikulanit wrote:
> So, at this point, I can re-submit the V3 and combine these changes, and we
> both can sign-off. How does that sound?

Sounds good to me :)
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] i2c: designware: Do not require clock when SSCN and FFCN are provided

[Mika Westerberg]

On Wed, Dec 16, 2015 at 08:23:45PM -0600, Suravee Suthikulpanit wrote:
> The current driver uses input clock source frequency to calculate
> values for [SS|FS]_[HC|LC] registers. However, when booting ACPI, we do not
> currently have a good way to provide the frequency information.
> Instead, we can leverage the SSCN and FFCN ACPI methods, which can be used
> to directly provide these values. So, the clock information should
> no longer be required during probing.
> 
> However, since clk can be invalid, additional checks must be done where
> we are making use of it.
> 
> Signed-off-by: Suravee Suthikulpanit 

This works fine on Intel Baytrail and Skylake. However, I think we could
do this a bit better still ;-)

> ---
> 
> Note: This has been tested on AMD Seattle RevB for both DT and ACPI.
> 
> Changes in V2:
> In v1, I disregarded the clock if SSCN and FMCN are provided,
> assuming that it was not needed. That was incorrect assumption,
> and is now fixed in v2.
> 
>  drivers/i2c/busses/i2c-designware-platdrv.c | 14 +-
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c 
> b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 8ffc36b..4615fe3 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -44,6 +44,9 @@
>  
>  static u32 i2c_dw_get_clk_rate_khz(struct dw_i2c_dev *dev)
>  {
> + if (IS_ERR(dev->clk))
> + return 0;
> +
>   return clk_get_rate(dev->clk)/1000;
>  }

So instead of this, what if we do not assign dev->get_clk_rate_khz at
all and then do something like below in the core driver?

Of course we still need the other changes you did in this patch to cope
with the missing clock.

diff --git a/drivers/i2c/busses/i2c-designware-core.c 
b/drivers/i2c/busses/i2c-designware-core.c
index 8c48b27ba059..25dccd8df772 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -271,6 +271,17 @@ static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool 
enable)
 enable ? "en" : "dis");
 }
 
+static unsigned long i2c_dw_clk_rate(struct dw_i2c_dev *dev)
+{
+   /*
+* Clock is not necessary if we got LCNT/HCNT values directly from
+* the platform code.
+*/
+   if (WARN_ON_ONCE(!dev->get_clk_rate_khz))
+   return 0;
+   return dev->get_clk_rate_khz(dev);
+}
+
 /**
  * i2c_dw_init() - initialize the designware i2c master hardware
  * @dev: device private data
@@ -281,7 +292,6 @@ static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool 
enable)
  */
 int i2c_dw_init(struct dw_i2c_dev *dev)
 {
-   u32 input_clock_khz;
u32 hcnt, lcnt;
u32 reg;
u32 sda_falling_time, scl_falling_time;
@@ -295,8 +305,6 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
}
}
 
-   input_clock_khz = dev->get_clk_rate_khz(dev);
-
reg = dw_readl(dev, DW_IC_COMP_TYPE);
if (reg == ___constant_swab32(DW_IC_COMP_TYPE_VALUE)) {
/* Configure register endianess access */
@@ -325,12 +333,12 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
hcnt = dev->ss_hcnt;
lcnt = dev->ss_lcnt;
} else {
-   hcnt = i2c_dw_scl_hcnt(input_clock_khz,
+   hcnt = i2c_dw_scl_hcnt(i2c_dw_clk_rate(dev),
4000,   /* tHD;STA = tHIGH = 4.0 us */
sda_falling_time,
0,  /* 0: DW default, 1: Ideal */
0); /* No offset */
-   lcnt = i2c_dw_scl_lcnt(input_clock_khz,
+   lcnt = i2c_dw_scl_lcnt(i2c_dw_clk_rate(dev),
4700,   /* tLOW = 4.7 us */
scl_falling_time,
0); /* No offset */
@@ -344,12 +352,12 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
hcnt = dev->fs_hcnt;
lcnt = dev->fs_lcnt;
} else {
-   hcnt = i2c_dw_scl_hcnt(input_clock_khz,
+   hcnt = i2c_dw_scl_hcnt(i2c_dw_clk_rate(dev),
600,/* tHD;STA = tHIGH = 0.6 us */
sda_falling_time,
0,  /* 0: DW default, 1: Ideal */
0); /* No offset */
-   lcnt = i2c_dw_scl_lcnt(input_clock_khz,
+   lcnt = i2c_dw_scl_lcnt(i2c_dw_clk_rate(dev),
1300,   /* tLOW = 1.3 us */
scl_falling_time,
0); /* No offset */
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to 

Re: [PATCH] i2c: make i2c_parse_fw_timings() always visible

[Mika Westerberg]

On Thu, Dec 17, 2015 at 01:32:36PM +0100, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+rene...@sang-engineering.com>
> 
> This function used to be DT only, so it lived inside a CONFIG_OF block.
> Now it uses device attributes and must be moved outside of it. No
> further code changes.
> 
> Reported-by: Jim Davis <jim.ep...@gmail.com>
> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>

Reviewed-by: Mika Westerberg <mika.westerb...@linux.intel.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: designware: Do not require clock when SSCN and FFCN are provided

[Mika Westerberg]

On Wed, Dec 16, 2015 at 08:44:34AM -0600, Suravee Suthikulpanit wrote:
> I am trying to avoid having to hard-coded clock frequency value in the
> driver. Would it be alright to not return w/ error, and just do the
> following?
> 
>   dev->clk = devm_clk_get(>dev, NULL);
>   if (!IS_ERR(dev->clk))
>   dev->get_clk_rate_khz = i2c_dw_get_clk_rate_khz;
> 
> This should work for the Intel case when clock is also provided.

Does it also work when clk_prepare_enable()/disable() is called for the
clock (in PM callbacks for example)? If yes, then I don't see problems
with what you are suggesting.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: designware: Do not require clock when SSCN and FFCN are provided

[Mika Westerberg]

On Wed, Dec 16, 2015 at 08:11:12AM -0600, Suravee Suthikulpanit wrote:
> >The clk framework should work fine if the returned clock is NULL (which
> >I think is your case).
> >
> >The driver gates clocks when the device is suspended and on Intel LPSS
> >there actually is a clock that gets gated.
> >
> >>[..]
> >>@@ -203,13 +223,11 @@ static int dw_i2c_plat_probe(struct platform_device 
> >>*pdev)
> >>dev->master_cfg =  DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
> >>DW_IC_CON_RESTART_EN | DW_IC_CON_SPEED_FAST;
> >>
> >>-   dev->clk = devm_clk_get(>dev, NULL);
> >>-   dev->get_clk_rate_khz = i2c_dw_get_clk_rate_khz;
> >>-   if (IS_ERR(dev->clk))
> >>-   return PTR_ERR(dev->clk);
> 
> Actually, if we don't provide the clock (which is the case for ACPI), this
> would also return and cause the probing to fail.

Indeed it seems that when you have CONFIG_COMMON_CLK selected the clock
framework starts returning errors if the clock is not found.

Since we need the clock for Intel LPSS I2C host controllers (and they
may have *CNT methods), I think you just need to provide the clock for
AMD I2C host controller in similar way than we do in
drivers/acpi/acpi_lpss.c.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: designware: Add support for AMD Seattle I2C

[Mika Westerberg]

On Wed, Dec 16, 2015 at 08:29:38AM -0600, Suravee Suthikulpanit wrote:
> 
> 
> On 12/16/2015 03:16 AM, Mika Westerberg wrote:
> >On Tue, Dec 15, 2015 at 08:14:34PM -0600, Suravee Suthikulpanit wrote:
> >>Hi Mika,
> >>
> >>On 12/15/15 15:55, Suravee Suthikulpanit wrote:
> >>>Add device HID AMDI0510 to match the I2C controlers on AMD Seattle platform
> >>>
> >>>Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpa...@amd.com>
> >>>---
> >>>  drivers/i2c/busses/i2c-designware-platdrv.c | 1 +
> >>>  1 file changed, 1 insertion(+)
> >>>
> >>>diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c 
> >>>b/drivers/i2c/busses/i2c-designware-platdrv.c
> >>>index 57f623b..a027154 100644
> >>>--- a/drivers/i2c/busses/i2c-designware-platdrv.c
> >>>+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> >>>@@ -117,6 +117,7 @@ static const struct acpi_device_id dw_i2c_acpi_match[] 
> >>>= {
> >>>   { "80860F41", 0 },
> >>>   { "808622C1", 0 },
> >>>   { "AMD0010", 0 },
> >>>+  { "AMDI0510", 0 },
> >>>   { }
> >>
> >>Since this driver seems to be used by several SOCs, and we have been adding
> >>the HID from various SOC vendors. Do you think it would be better to assign
> >>a CID so that each SOC vendor can specify in their ACPI DSDT and we can
> >>match them here?
> >
> >Sure _CID would work here.
> 
> Do you know if Synopsys has already provided a CID that we can use for this?

No.

> If not, who do you think should provide this? 

Why can't you make _CID for AMD part only? For Intel we are going to get
new IDs for every major SoC release no matter what.

> Also, do you think the FMCN
> and SSCN should be documented somewhere in the spec so that FW and OSes can
> agree upon going forward?

Since this is designware I2C specific thing it should not be part of the
ACPI spec, I think.

But, yes I agree should be documented *somewhere* ;-)

> >>Then, we can also associate the FMCN and SSCN along with the CID, and
> >>guarantee compatibility.
> >
> >Well, the driver checks those everytime it finds that the device has
> >ACPI companion regardless of _HID/_CID.
> 
> Not sure what you mean by "device has ACPI companion". Do you mean the
> driver check those for every matched devices here?

That's right:

if (has_acpi_companion(>dev))
dw_i2c_acpi_configure(pdev);
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: designware: Add support for AMD Seattle I2C

[Mika Westerberg]

On Tue, Dec 15, 2015 at 08:14:34PM -0600, Suravee Suthikulpanit wrote:
> Hi Mika,
> 
> On 12/15/15 15:55, Suravee Suthikulpanit wrote:
> >Add device HID AMDI0510 to match the I2C controlers on AMD Seattle platform
> >
> >Signed-off-by: Suravee Suthikulpanit 
> >---
> >  drivers/i2c/busses/i2c-designware-platdrv.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> >diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c 
> >b/drivers/i2c/busses/i2c-designware-platdrv.c
> >index 57f623b..a027154 100644
> >--- a/drivers/i2c/busses/i2c-designware-platdrv.c
> >+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> >@@ -117,6 +117,7 @@ static const struct acpi_device_id dw_i2c_acpi_match[] = 
> >{
> > { "80860F41", 0 },
> > { "808622C1", 0 },
> > { "AMD0010", 0 },
> >+{ "AMDI0510", 0 },
> > { }
> 
> Since this driver seems to be used by several SOCs, and we have been adding
> the HID from various SOC vendors. Do you think it would be better to assign
> a CID so that each SOC vendor can specify in their ACPI DSDT and we can
> match them here?

Sure _CID would work here.

> Then, we can also associate the FMCN and SSCN along with the CID, and
> guarantee compatibility.

Well, the driver checks those everytime it finds that the device has
ACPI companion regardless of _HID/_CID.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: designware: Do not require clock when SSCN and FFCN are provided

[Mika Westerberg]

+Jarkko and Andy

On Tue, Dec 15, 2015 at 04:38:58PM -0600, Suravee Suthikulpanit wrote:
> The current driver uses input clock source frequency to calculate
> values for [SS|FS]_[HC|LC] registers. However, when booting ACPI, we do not
> currently have a good way to provide the frequency information.
> Instead, we can leverage the SSCN and FFCN ACPI methods, which can be used
> to directly provide these values.
> 
> So, this patch removes the clock requirement when SSCN and FFCN
> are provided.

Actually I think the only thing you need to change is i2c_dw_init() so
that it does not call dev->get_clk_rate_khz(dev) if *CNT values are
already provided.

The clk framework should work fine if the returned clock is NULL (which
I think is your case).

The driver gates clocks when the device is suspended and on Intel LPSS
there actually is a clock that gets gated.

> 
> Signed-off-by: Suravee Suthikulpanit 
> ---
> 
> Note: This has been tested on AMD Seattle RevB for both DT and ACPI.
> 
>  drivers/i2c/busses/i2c-designware-core.c|  5 +++--
>  drivers/i2c/busses/i2c-designware-platdrv.c | 30 
> +++--
>  2 files changed, 27 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-core.c 
> b/drivers/i2c/busses/i2c-designware-core.c
> index 8c48b27..ec458ec 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -281,7 +281,7 @@ static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool 
> enable)
>   */
>  int i2c_dw_init(struct dw_i2c_dev *dev)
>  {
> - u32 input_clock_khz;
> + u32 input_clock_khz = 0;
>   u32 hcnt, lcnt;
>   u32 reg;
>   u32 sda_falling_time, scl_falling_time;
> @@ -295,7 +295,8 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
>   }
>   }
>  
> - input_clock_khz = dev->get_clk_rate_khz(dev);
> + if (dev->get_clk_rate_khz)
> + input_clock_khz = dev->get_clk_rate_khz(dev);
>  
>   reg = dw_readl(dev, DW_IC_COMP_TYPE);
>   if (reg == ___constant_swab32(DW_IC_COMP_TYPE_VALUE)) {
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c 
> b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 809579e..57f623b 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -127,6 +127,26 @@ static inline int dw_i2c_acpi_configure(struct 
> platform_device *pdev)
>  }
>  #endif
>  
> +static int dw_i2c_configure_clk(struct platform_device *pdev,
> + struct dw_i2c_dev *dev)
> +{
> + /* For ACPI, if SSCN and FMCN is provided, we should not
> +  * need the clock value.
> +  */
> + if (has_acpi_companion(>dev) &&
> + dev->ss_hcnt && dev->ss_lcnt &&
> + dev->fs_hcnt && dev->fs_lcnt)
> + return 0;
> +
> + dev->clk = devm_clk_get(>dev, NULL);
> + dev->get_clk_rate_khz = i2c_dw_get_clk_rate_khz;
> + if (IS_ERR(dev->clk))
> + return PTR_ERR(dev->clk);
> + clk_prepare_enable(dev->clk);
> +
> + return 0;
> +}
> +
>  static int dw_i2c_plat_probe(struct platform_device *pdev)
>  {
>   struct dw_i2c_dev *dev;
> @@ -203,13 +223,11 @@ static int dw_i2c_plat_probe(struct platform_device 
> *pdev)
>   dev->master_cfg =  DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
>   DW_IC_CON_RESTART_EN | DW_IC_CON_SPEED_FAST;
>  
> - dev->clk = devm_clk_get(>dev, NULL);
> - dev->get_clk_rate_khz = i2c_dw_get_clk_rate_khz;
> - if (IS_ERR(dev->clk))
> - return PTR_ERR(dev->clk);
> - clk_prepare_enable(dev->clk);
> + r = dw_i2c_configure_clk(pdev, dev);
> + if (r)
> + return r;
>  
> - if (!dev->sda_hold_time && ht) {
> + if (!dev->sda_hold_time && ht && dev->get_clk_rate_khz) {
>   u32 ic_clk = dev->get_clk_rate_khz(dev);
>  
>   dev->sda_hold_time = div_u64((u64)ic_clk * ht + 50,
> -- 
> 2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/2] i2c:dw: Add APM X-Gene ACPI I2C device support

[Mika Westerberg]

On Tue, Dec 15, 2015 at 11:20:03AM -0800, Loc Ho wrote:
> Can we not just add an AML method that will return the operation clock
> frequency that the I2C driver can use? If the method doesn't existed,
> we will just bail and do nothing or assume what ever default behavior?

Why would you add yet another method? There already are existing ACPI
*CNT methods that you can use, and are used in systems out there.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/2] i2c:dw: Add APM X-Gene ACPI I2C device support

[Mika Westerberg]

On Mon, Dec 14, 2015 at 06:53:25PM -0600, Suravee Suthikulanit wrote:
> I'm not sure if this has been discussed earlier. But after looking at the
> the acpi_apd driver, all we need is just the platform-specific input clock
> frequency value used by the drivers/i2c/busses/i2c-designware-core.c:
> i2c_dw_init() to calculate the values to program into the DW_IC_SS_SCL_HCNT
> and DW_IC_SS_SCL_LCNT registers.

There is a way to pass *CNT values already from ACPI to the driver -- It
looks for method called FMCN (or SSCN) and retrieves the values from
there if found.

The driver could be modified not to require clock if it already knows
*CNT values.

> So, instead of hard-coding this value into the driver/acpi/acpi_apd.c (which
> really has nothing to do with the I2C driver), what if we introduce a new
> ACPI key-value pair in the ACPI DSDT such as:
> 
> Device(I2C0)// I2C controller
> {
>   Name(_HID, "AMDI0510")
>   Name(_UID, 0)
>   Name(_CRS, ResourceTemplate() {
> Memory32Fixed (ReadWrite, 0xE100, 0x1000)
> Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive,,,) { 389
> } // GSIV
>   })
>   Name (_DSD, Package () {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package () {
>   Package (2) {"clock-source-frequency", 1 }

If this is to be done then I think it is better to use Linux clk
framework to provide the clock (with corresponding properties in place).

But ACPI specification wants clocks as PowerResources and those do not
have support for reading rate of the clock.

> }
>   }) // _DSD()
> }
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/2] i2c:dw: Add APM X-Gene ACPI I2C device support

[Mika Westerberg]

On Tue, Dec 15, 2015 at 08:52:19AM -0600, Suravee Suthikulpanit wrote:
> 
> 
> On 12/15/2015 07:27 AM, Mika Westerberg wrote:
> >On Mon, Dec 14, 2015 at 06:53:25PM -0600, Suravee Suthikulanit wrote:
> >>>I'm not sure if this has been discussed earlier. But after looking at the
> >>>the acpi_apd driver, all we need is just the platform-specific input clock
> >>>frequency value used by the drivers/i2c/busses/i2c-designware-core.c:
> >>>i2c_dw_init() to calculate the values to program into the DW_IC_SS_SCL_HCNT
> >>>and DW_IC_SS_SCL_LCNT registers.
> >There is a way to pass *CNT values already from ACPI to the driver -- It
> >looks for method called FMCN (or SSCN) and retrieves the values from
> >there if found.
> 
> Right, I also noticed this afterward. By the way, are FMCN and SSCN
> documented anywhere in the ACPI spec?  I am trying to figure out how to
> update the ACPI table to add this information for the AMD Seattle (ARM64)
> platform, and I will also submit a patch to add the new HID for this driver.

No, they are Intel inventions for the Windows I2C driver.

Here is what I know about it:
  SSCN - Standard Mode CNTs
  FMCN - Fast Mode CNTs

They both return a package:

  Package() {
HCNT,
LCNT,
SDA_hold_time,
  }
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/9] i2c: add generic routine to parse DT for timing information

[Mika Westerberg]

On Tue, Dec 08, 2015 at 01:53:07PM +0100, Wolfram Sang wrote:
> 
> > I wonder if it makes sense to add "i2c-sda-hold-time-ns" (taken from the
> > designware driver DT binding) to the timings structure? It is tHD;DAT
> > parameter in the I2C bus specification.
> 
> It totally makes sense. I just didn't need it for the RCar driver and
> didn't want to implement something which isn't actually used. So feel
> free to add it, or ask me to do it, if you promise to use it in the
> designware driver :)

I can add it in a separate patch later on. Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] i2c:dw: Add APM X-Gene ACPI I2C device support

[Mika Westerberg]

On Mon, Dec 07, 2015 at 05:16:14PM -0700, Loc Ho wrote:
> Enable APM X-Gene ACPI I2C device support by adding the
> corresponding ACPI ID. The platform ACPI APD corresponding
> change is required to provide the proper clock frequency input.
> 
> Signed-off-by: Loc Ho <l...@apm.com>

Acked-by: Mika Westerberg <mika.westerb...@linux.intel.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/9] i2c: add generic routine to parse DT for timing information

[Mika Westerberg]

On Tue, Dec 08, 2015 at 10:37:46AM +0100, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+rene...@sang-engineering.com>
> 
> Inspired from the i2c-rk3x driver (thanks guys!) but refactored and
> extended. See built-in docs for further information.
> 
> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>

Looks good. I think we can take advantage of this in the designware
driver as well.

Reviewed-by: Mika Westerberg <mika.westerb...@linux.intel.com>

I wonder if it makes sense to add "i2c-sda-hold-time-ns" (taken from the
designware driver DT binding) to the timings structure? It is tHD;DAT
parameter in the I2C bus specification.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 0/2] ACPI: Add irq_type to gpio interrupt

[Mika Westerberg]

On Sat, Dec 05, 2015 at 12:36:19AM +0100, Christophe Ricard wrote:
> >No, but I wonder if it would be better to do this in acpi_dev_gpio_irq_get()
> >instead of acpi_find_gpio() which gets called everytime a GPIO is looked up?
> I believe, setting the irq type requires triggering and polarity data stored
> into a struct acpi_resource_gpio.
> 
> acpi_dev_gpio_irq_get call acpi_get_gpiod_by_index which run acpi_find_gpio.
> 
> Actually acpi_dev_gpio_irq_get is called everytime an i2c device slave is
> probed, acpi_find_gpio will get called to "register" gpios.
> If done correctly, i think this will be done only once...
> 
> The only improvement i may see would be to add an extra field in the
> acpi_gpio_info structure in drivers/gpio/gpiolib.h (for example int
> irq_type).
> And call irq_set_irq_type in acpi_dev_gpio_irq_get if the gpio is an
> interrupt.
> 
> I guess the current proposal and this one are equivalent...
> 
> What do you think ?

Actually I think it may be useful to add triggering flags to
acpi_gpio_info.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/9] i2c: add generic routine to parse DT for timing information

[Mika Westerberg]

On Thu, Dec 03, 2015 at 04:51:32PM +0100, Wolfram Sang wrote:
> From: Wolfram Sang 
> 
> Inspired from the i2c-rk3x driver (thanks guys!) but refactored and
> extended. See built-in docs for further information.
> 
> Signed-off-by: Wolfram Sang 
> ---
>  drivers/i2c/i2c-core.c | 50 
> ++
>  include/linux/i2c.h| 22 ++
>  2 files changed, 72 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index ba8eb087f22465..5c269dd51b2de7 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -1438,6 +1438,56 @@ static void of_i2c_register_devices(struct i2c_adapter 
> *adap)
>   }
>  }
>  
> +/**
> + * of_i2c_parse_timings - get I2C related timing parameters from DT
> + * @node: The DT node to scan for I2C timing properties
> + * @t: the i2c_timings struct to be filled with values
> + * @use_defaults: bool to use sane defaults derived from the I2C 
> specification
> + * when properties are not found, otherwise use 0
> + *
> + * Scan the node pointer for the generic I2C DT properties describing timing
> + * parameters for the signal and fill the given struct with the results. If a
> + * property was not found and use_defaults was true, then maximum timings are
> + * assumed which are derived from the I2C specification. If use_defaults is 
> not
> + * used, the result will be 0, so drivers can apply their own defaults later.
> + * The latter is mainly intended for avoiding regressions of existing drivers
> + * which want to switch to this function. New drivers almost always should 
> use
> + * the defaults.
> + */
> +void of_i2c_parse_timings(struct device_node *node, struct i2c_timings *t, 
> bool use_defaults)
> +{
> + memset(t, 0, sizeof(*t));
> +
> + if (of_property_read_u32(node, "clock-frequency", >bus_freq_hz) && 
> use_defaults)
> + t->bus_freq_hz = 10;

Why not create device_i2c_parse_timings() instead and use unified device
properties API?
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 16/16] i2c: designware: Convert to use unified device property API

[Mika Westerberg]

On Wed, Dec 02, 2015 at 11:23:40AM +0200, Andy Shevchenko wrote:
> On Wed, 2015-12-02 at 02:28 +0100, Rafael J. Wysocki wrote:
> > On Tuesday, December 01, 2015 12:33:51 PM Andy Shevchenko wrote:
> > > On Mon, 2015-11-30 at 20:58 +0100, Wolfram Sang wrote:
> > > > On Mon, Nov 30, 2015 at 05:11:44PM +0200, Andy Shevchenko wrote:
> 
> 
> 
> > > > What is the bug fix here described in the cover letter?
> > > 
> > > The cover letter mentioned 'last part' which I refer to as patches
> > > 14,
> > > 15 (though this is for UART), and 16.
> > 
> > Hmm.
> > 
> > So may I assume that patches [1-13/16] are for me and the rest is to
> > be applied
> > by the other respective maintainers?
> > 
> > That should be easiest logistically IMHO.
> 
> Have no objections.

Unfortunately the patches (except this one) depend on each other so they
cannot be applied separately.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 0/2] ACPI: Add irq_type to gpio interrupt

[Mika Westerberg]

On Mon, Nov 30, 2015 at 11:47:51PM +0100, Christophe Ricard wrote:
> ACPI probing method does not retrieve irq_type from a gpio interrupt declared
> with GpioInt as it is done with devicetree probing. In other terms, 
> irq_get_trigger_type
> will always send back 0.

Is this real problem you are solving here?

Also where does the device tree version set triggering flags for a GPIO
irq?
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 0/2] ACPI: Add irq_type to gpio interrupt

[Mika Westerberg]

On Tue, Dec 01, 2015 at 01:25:50PM +0100, Christophe Ricard wrote:
>For example during an i2c_device_probe where an i2c slave device
>describe in devicetree has an interrupts property.
>i2c_device_probe (drivers/i2c/i2c-core.c), retrieves irq property from
>of_irq_get which will looks for an "interrupts" property in
>of_irq_parse_of (drivers/of/irq.c).
>of_irq_get will then call irq_create_mapping (kernel/irq/irqdomain.c)
>which will set the irq_type retrieved during the interrupts node
>parsing.

Found it now thanks.

>This will allow from an i2c slave drivers to configure an interrupt
>handler matching the exact devicetree data for the interrupts property
>of the i2c slave node.

Makes sense.

>Now for the same kind of i2c driver using acpi description, the GpioInt
>polarity/type is at the moment never kept in the irq property.
>It is possible to check that following about the same path...
>i2c_device_probe (drivers/i2c/i2c-core.c), retrieves irq property from
>acpi_dev_gpio_irq_get but does not save the irq_type.
>This would allow not to have to use an additional gpio field and all
>the configuration step to configure the gpio interrupt correctly in a
>device driver and taking a real benefit of the GpioInt acpi keyword
>compare to GpioIo keyword.
>Most the of the drivers based on acpi description retrieve gpio number
>to assign an interrupt and a fix polarity. I believe my patchset
>proposal would improve this and allow to
>be much closer with devicetree.
>Do you see any issue with this ?

No, but I wonder if it would be better to do this in acpi_dev_gpio_irq_get()
instead of acpi_find_gpio() which gets called everytime a GPIO is looked up?
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Issues with touchpad / touchscreen on yoga 900

[Mika Westerberg]

On Tue, Dec 01, 2015 at 01:35:54AM +0100, Wolfram Sang wrote:
> I didn't find any patch coming out of this? Mika, did I miss something?

The fix is included in the "intel-lpss: support for non-ACPI platforms"
series by Andy [1]. You and Kevin are both Cc'd.

https://lkml.org/lkml/2015/11/30/441

The patch solving this issue is "[14/16] mfd: intel-lpss: Pass SDA hold
time to I2C host controller driver".
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 11/13] mfd: intel-lpss: Pass HSUART configuration via properties

[Mika Westerberg]

On Tue, Nov 24, 2015 at 08:53:04PM +0100, Arnd Bergmann wrote:
> On Tuesday 24 November 2015 12:22:57 Andy Shevchenko wrote:
> > +static struct property_entry uart_properties[] = {
> > +   PROPERTY_ENTRY_U32("reg-io-width", 4),
> > +   PROPERTY_ENTRY_U32("reg-shift", 2),
> > +   PROPERTY_ENTRY_U8("snps,uart-16550-compatible", 1),
> > +   { },
> > 
> 
> If I read the binding correctly, the "snps,uart-16550-compatible" property
> is meant to be boolean, meaning true if present and zero-length or false
> if absent. Using a u8 propert instead feels wrong.
> 
> Maybe we can have a PROPERTY_ENTRY_BOOL() for that?

That's a good idea. We'll add it to the next revision of the series.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 13/13] i2c: designware: Convert to use unified device property API

[Mika Westerberg]

On Tue, Nov 24, 2015 at 12:53:06PM +0200, Jarkko Nikula wrote:
> Mika, Andy: Was this one able to go separately? At least it builds without
> rest of the set but is there anything that could break DT based system if
> there are no patches 1-8/13?

As far as I can tell this should not break existing DT systems but I
have not been able to test myself since I don't have any DT systems
here.

> Acked-by: Jarkko Nikula 

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 11/13] mfd: intel-lpss: Pass HSUART configuration via properties

[Mika Westerberg]

On Tue, Nov 24, 2015 at 06:44:53PM +0800, kbuild test robot wrote:
> Hi Mika,
> 
> [auto build test ERROR on v4.4-rc2]
> [also build test ERROR on next-20151124]
> [cannot apply to ljones-mfd/for-mfd-next]
> 
> url:
> https://github.com/0day-ci/linux/commits/Andy-Shevchenko/device-property-always-check-for-fwnode-type/20151124-183221
> config: x86_64-randconfig-x019-11241713 (attached as .config)
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=x86_64 
> 
> All error/warnings (new ones prefixed by >>):
> 
> >> drivers/mfd/intel-lpss-pci.c:68:30: error: array type has incomplete 
> >> element type 'struct property_entry'
> static struct property_entry uart_properties[] = {

Thanks kbuild robot!

This patch misses include of . It seems that it got
added by a subsequent patch which was not sent out.

We will fix this.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/3] i2c-piix4: Add support for multiplexed main adapter in SB800

[Mika Westerberg]

On Sun, Nov 15, 2015 at 12:33:03PM +0100, Christian Fetzer wrote:
> The SB800 chipset supports a multiplexed main SMBus controller with
> four ports. The multiplexed ports share the same SMBus address and
> register set. The port is selected by bits 2:1 of the smb_en register
> (0x2C).
> 
> Only one port can be active at any point in time therefore a mutex is
> needed in order to synchronize access.
> 
> Additionally, the commit avoids requesting and releasing the SMBus base
> address index region on every multiplexed transfer by moving the
> request_region call into piix4_probe.
> 
> Tested on HP ProLiant MicroServer G7 N54L (where this patch adds
> support to access sensor data from the w83795adg).
> 
> Cc: Thomas Brandon <tbrando...@gmail.com>
> Cc: Eddi De Pieri <e...@depieri.net>
> Signed-off-by: Christian Fetzer <fetzer...@gmail.com>

Reviewed-by: Mika Westerberg <mika.westerb...@linux.intel.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/5] i2c-piix4: Optionally release smba in piix4_adap_remove

[Mika Westerberg]

On Sat, Nov 07, 2015 at 12:35:22PM +0100, Christian Fetzer wrote:
> This is in preparation to support the multiplexed SMBus main controller
> in the SB800 chipset where the controller address is shared among
> the four multiplexed ports. As such the address region should be
> only freed for the first multiplexed adapter to avoid double free
> warnings.
> 
> Signed-off-by: Christian Fetzer <fetzer...@gmail.com>

Reviewed-by: Mika Westerberg <mika.westerb...@linux.intel.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] I2C: designware: fix IO timeout issue for AMD controller

[Mika Westerberg]

On Fri, Nov 06, 2015 at 04:34:19AM +, Yu, Xiangliang wrote:
> > -Original Message-
> > From: Mika Westerberg [mailto:mika.westerb...@linux.intel.com]
> > Sent: Thursday, November 05, 2015 9:52 PM
> > To: Yu, Xiangliang
> > Cc: andriy.shevche...@linux.intel.com; jarkko.nik...@linux.intel.com;
> > w...@the-dreams.de; linux-i2c@vger.kernel.org; linux-
> > ker...@vger.kernel.org; Xue, Ken; Wan, Vincent; Huang, Ray; Wang, Annie;
> > Li, Tony
> > Subject: Re: [PATCH 1/1] I2C: designware: fix IO timeout issue for AMD
> > controller
> > 
> > On Thu, Nov 05, 2015 at 08:34:44PM +0800, Xiangliang Yu wrote:
> > > Because of some hardware limitation, AMD I2C controller can't trigger
> > > next interrupt if interrupt status has been changed after clearing
> > > interrupt status bits. Then, I2C will lost interrupt and IO timeout.
> > >
> > > According to hardware design, this patch implements a workaround to
> > > disable i2c controller interrupt after clearing interrupt bits when
> > > entering ISR and to enable i2c interrupt before exiting ISR.
> > >
> > > To reduce the performance impacts on other vendors, use unlikely
> > > function to check flag in ISR.
> > >
> > > Signed-off-by: Xiangliang Yu <xiangliang...@amd.com>
> > > ---
> > >  drivers/i2c/busses/i2c-designware-core.c| 6 ++
> > >  drivers/i2c/busses/i2c-designware-core.h| 1 +
> > >  drivers/i2c/busses/i2c-designware-platdrv.c | 4 
> > >  3 files changed, 11 insertions(+)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-designware-core.c
> > > b/drivers/i2c/busses/i2c-designware-core.c
> > > index 7441cdc..04e7b65 100644
> > > --- a/drivers/i2c/busses/i2c-designware-core.c
> > > +++ b/drivers/i2c/busses/i2c-designware-core.c
> > > @@ -783,6 +783,9 @@ irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
> > >
> > >   stat = i2c_dw_read_clear_intrbits(dev);
> > 
> > What if the status changes right here, before you go and mask the interrupt?
> Have no effect, because i2c controller can't trigger next interrupt.
> 
> > 
> > >
> > > + if (unlikely(dev->accessor_flags & ACCESS_INTR_MASK))
> > > + i2c_dw_disable_int(dev);
> > > +
> > >   if (stat & DW_IC_INTR_TX_ABRT) {
> > >   dev->cmd_err |= DW_IC_ERR_TX_ABRT;
> > >   dev->status = STATUS_IDLE;
> > > @@ -811,6 +814,9 @@ tx_aborted:
> > >   if ((stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET)) ||
> > dev->msg_err)
> > >   complete(>cmd_complete);
> > >
> > > + if (unlikely(dev->accessor_flags & ACCESS_INTR_MASK))
> > > + dw_writel(dev, DW_IC_INTR_DEFAULT_MASK,
> > DW_IC_INTR_MASK);
> > 
> > The driver disables TX interrupt when it is not needed anymore or when TX
> > gets aborted but the above will re-enable all interrupts regardless.
> > Is that the intention?
> No, i2c controller can trigger next interrupt only after re-enable all 
> interrupt.

If you get an error the function masks all interrupts and jumps to
tx_aborted label. With this patch you unmask all interrupts again before
exiting the function.

> > 
> > > +
> > >   return IRQ_HANDLED;
> > >  }
> > >  EXPORT_SYMBOL_GPL(i2c_dw_isr);
> > > diff --git a/drivers/i2c/busses/i2c-designware-core.h
> > > b/drivers/i2c/busses/i2c-designware-core.h
> > > index 9630222..808ef6a 100644
> > > --- a/drivers/i2c/busses/i2c-designware-core.h
> > > +++ b/drivers/i2c/busses/i2c-designware-core.h
> > > @@ -111,6 +111,7 @@ struct dw_i2c_dev {
> > >
> > >  #define ACCESS_SWAP  0x0001
> > >  #define ACCESS_16BIT 0x0002
> > > +#define ACCESS_INTR_MASK 0x0004
> > >
> > >  extern u32 dw_readl(struct dw_i2c_dev *dev, int offset);  extern void
> > > dw_writel(struct dw_i2c_dev *dev, u32 b, int offset); diff --git
> > > a/drivers/i2c/busses/i2c-designware-platdrv.c
> > > b/drivers/i2c/busses/i2c-designware-platdrv.c
> > > index 472b882..0c59357 100644
> > > --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> > > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> > > @@ -251,6 +251,10 @@ static int dw_i2c_probe(struct platform_device
> > *pdev)
> > >   dev->rx_fifo_depth = ((param1 >> 8)  & 0xff) + 1;
> > >   dev->adapter.nr = pdev->id;
> > >   }
> > > +
> > 

Re: [PATCH 1/1] I2C: designware: fix IO timeout issue for AMD controller

[Mika Westerberg]

On Thu, Nov 05, 2015 at 08:34:44PM +0800, Xiangliang Yu wrote:
> Because of some hardware limitation, AMD I2C controller can't
> trigger next interrupt if interrupt status has been changed
> after clearing interrupt status bits. Then, I2C will lost
> interrupt and IO timeout.
> 
> According to hardware design, this patch implements a workaround
> to disable i2c controller interrupt after clearing interrupt bits
> when entering ISR and to enable i2c interrupt before exiting ISR.
> 
> To reduce the performance impacts on other vendors, use unlikely
> function to check flag in ISR.
> 
> Signed-off-by: Xiangliang Yu 
> ---
>  drivers/i2c/busses/i2c-designware-core.c| 6 ++
>  drivers/i2c/busses/i2c-designware-core.h| 1 +
>  drivers/i2c/busses/i2c-designware-platdrv.c | 4 
>  3 files changed, 11 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-core.c 
> b/drivers/i2c/busses/i2c-designware-core.c
> index 7441cdc..04e7b65 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -783,6 +783,9 @@ irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
>  
>   stat = i2c_dw_read_clear_intrbits(dev);

What if the status changes right here, before you go and mask the
interrupt?

>  
> + if (unlikely(dev->accessor_flags & ACCESS_INTR_MASK))
> + i2c_dw_disable_int(dev);
> +
>   if (stat & DW_IC_INTR_TX_ABRT) {
>   dev->cmd_err |= DW_IC_ERR_TX_ABRT;
>   dev->status = STATUS_IDLE;
> @@ -811,6 +814,9 @@ tx_aborted:
>   if ((stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET)) || dev->msg_err)
>   complete(>cmd_complete);
>  
> + if (unlikely(dev->accessor_flags & ACCESS_INTR_MASK))
> + dw_writel(dev, DW_IC_INTR_DEFAULT_MASK, DW_IC_INTR_MASK);

The driver disables TX interrupt when it is not needed anymore or when
TX gets aborted but the above will re-enable all interrupts regardless.
Is that the intention?

> +
>   return IRQ_HANDLED;
>  }
>  EXPORT_SYMBOL_GPL(i2c_dw_isr);
> diff --git a/drivers/i2c/busses/i2c-designware-core.h 
> b/drivers/i2c/busses/i2c-designware-core.h
> index 9630222..808ef6a 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -111,6 +111,7 @@ struct dw_i2c_dev {
>  
>  #define ACCESS_SWAP  0x0001
>  #define ACCESS_16BIT 0x0002
> +#define ACCESS_INTR_MASK 0x0004
>  
>  extern u32 dw_readl(struct dw_i2c_dev *dev, int offset);
>  extern void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset);
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c 
> b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 472b882..0c59357 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -251,6 +251,10 @@ static int dw_i2c_probe(struct platform_device *pdev)
>   dev->rx_fifo_depth = ((param1 >> 8)  & 0xff) + 1;
>   dev->adapter.nr = pdev->id;
>   }
> +
> + if (!strncmp(pdev->name, "AMD0010", 7))
> + dev->accessor_flags |= ACCESS_INTR_MASK;
> +

Can't you put this to ->driver_data? For example it could refer to a
configuration structure that then contains quirk flags.

>   r = i2c_dw_init(dev);
>   if (r)
>   return r;
> -- 
> 1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Issues with touchpad / touchscreen on yoga 900

[Mika Westerberg]

On Mon, Nov 02, 2015 at 12:47:28PM -0700, Kevin Fenzi wrote:
> Any other ideas or things to try? 
> 
> It would be nice to have a working touchpad. ;)

Please attach acpidump of that machine to the bug (or send it directly
to me) and provide full dmesg with i2c_hid.debug=1.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/5] i2c-piix4: Convert piix4_main_adapter to array

[Mika Westerberg]

On Sun, Nov 01, 2015 at 05:32:06PM +0100, Christian Fetzer wrote:
> The SB800 chipset supports a multiplexed main SMBus controller with
> four ports. Therefore the static variable piix4_main_adapter is
> converted into a piix4_main_adapters array that can hold one
> i2c_adapter for each multiplexed port.
> 
> The auxiliary adapter remains unchanged since it represents the second
> (not multiplexed) SMBus controller on the SB800 chipset.
> 
> Signed-off-by: Christian Fetzer <fetzer...@gmail.com>

Looks good to me,

Reviewed-by: Mika Westerberg <mika.westerb...@linux.intel.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/5] i2c-piix4: Add adapter port name support for SB800 chipset

[Mika Westerberg]

On Sun, Nov 01, 2015 at 05:32:09PM +0100, Christian Fetzer wrote:
> This patch adds support for port names for the SB800 chipset.
> Since the chipset supports a multiplexed main SMBus controller, adding
> the channel name to the adapter name is necessary to differentiate the
> ports better (for example in sensors output).
> 
> Signed-off-by: Christian Fetzer <fetzer...@gmail.com>
> ---
>  drivers/i2c/busses/i2c-piix4.c | 14 ++
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index 54f8af5..18ea5d7 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -132,6 +132,10 @@ static const struct dmi_system_id piix4_dmi_ibm[] = {
>  /* SB800 globals */
>  DEFINE_MUTEX(piix4_mutex_sb800);
>  static unsigned short piix4_smb_idx_sb800;
> +static const char *piix4_main_port_names_sb800[4] = {
> + "SDA0", "SDA2", "SDA3", "SDA4"
> +};
> +static const char *piix4_aux_port_name_sb800 = "SDA1";
>  
>  struct i2c_piix4_adapdata {
>   unsigned short smba;
> @@ -613,7 +617,7 @@ static struct i2c_adapter 
> *piix4_main_adapters[PIIX4_MAX_ADAPTERS];
>  static struct i2c_adapter *piix4_aux_adapter;
>  
>  static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
> -  struct i2c_adapter **padap)
> +  const char *name, struct i2c_adapter **padap)
>  {
>   struct i2c_adapter *adap;
>   struct i2c_piix4_adapdata *adapdata;
> @@ -642,7 +646,7 @@ static int piix4_add_adapter(struct pci_dev *dev, 
> unsigned short smba,
>   adap->dev.parent = >dev;
>  
>   snprintf(adap->name, sizeof(adap->name),
> - "SMBus PIIX4 adapter at %04x", smba);
> + "SMBus PIIX4 adapter %s at %04x", name, smba);
>  
>   i2c_set_adapdata(adap, adapdata);
>  
> @@ -667,6 +671,7 @@ static int piix4_add_adapters_sb800(struct pci_dev *dev, 
> unsigned short smba)
>  
>   for (port = 0; port < PIIX4_MAX_ADAPTERS; port++) {
>   retval = piix4_add_adapter(dev, smba,
> +piix4_main_port_names_sb800[port],
>  _main_adapters[port]);
>   if (retval < 0)
>   goto error;
> @@ -726,7 +731,7 @@ static int piix4_probe(struct pci_dev *dev, const struct 
> pci_device_id *id)
>   return retval;
>  
>   /* Try to register main SMBus adapter, give up if we can't */
> - retval = piix4_add_adapter(dev, retval,
> + retval = piix4_add_adapter(dev, retval, "",

Why not use some useful name here instead of "" ?

Otherwise looks good,

Reviewed-by: Mika Westerberg <mika.westerb...@linux.intel.com>


>  _main_adapters[0]);
>   }
>  
> @@ -755,7 +760,8 @@ static int piix4_probe(struct pci_dev *dev, const struct 
> pci_device_id *id)
>   if (retval > 0) {
>   /* Try to add the aux adapter if it exists,
>* piix4_add_adapter will clean up if this fails */
> - piix4_add_adapter(dev, retval, _aux_adapter);
> + piix4_add_adapter(dev, retval, piix4_aux_port_name_sb800,
> +   _aux_adapter);
>   }
>  
>   return 0;
> -- 
> 1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/5] i2c-piix4: Optionally release smba in piix4_adap_remove

[Mika Westerberg]

On Sun, Nov 01, 2015 at 05:32:05PM +0100, Christian Fetzer wrote:
> -static void piix4_adap_remove(struct i2c_adapter *adap)
> +static void piix4_adap_remove(struct i2c_adapter *adap, int free_smba)

How about bool instead?
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/5] i2c-piix4: Request base address index region once for SB800

[Mika Westerberg]

On Sun, Nov 01, 2015 at 05:32:07PM +0100, Christian Fetzer wrote:
> Request the SMBus base address index region once in piix4_probe. This
> is particularly useful when using the multiplexed adapter in SB800 as
> it avoids requesting and releasing the region on every transfer.
> 
> Signed-off-by: Christian Fetzer 
> ---
>  drivers/i2c/busses/i2c-piix4.c | 35 ++-
>  1 file changed, 22 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index 35eeb0d..d866b58 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -125,6 +125,9 @@ static const struct dmi_system_id piix4_dmi_ibm[] = {
>   { },
>  };
>  
> +/* SB800 globals */
> +static unsigned short piix4_smb_idx_sb800;
> +
>  struct i2c_piix4_adapdata {
>   unsigned short smba;
>  };
> @@ -232,7 +235,6 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
>const struct pci_device_id *id, u8 aux)
>  {
>   unsigned short piix4_smba;
> - unsigned short smba_idx = 0xcd6;
>   u8 smba_en_lo, smba_en_hi, smb_en, smb_en_status;
>   u8 i2ccfg, i2ccfg_offset = 0x10;
>  
> @@ -254,16 +256,10 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
>   else
>   smb_en = (aux) ? 0x28 : 0x2c;
>  
> - if (!request_region(smba_idx, 2, "smba_idx")) {
> - dev_err(_dev->dev, "SMBus base address index region "
> - "0x%x already in use!\n", smba_idx);
> - return -EBUSY;
> - }
> - outb_p(smb_en, smba_idx);
> - smba_en_lo = inb_p(smba_idx + 1);
> - outb_p(smb_en + 1, smba_idx);
> - smba_en_hi = inb_p(smba_idx + 1);
> - release_region(smba_idx, 2);
> + outb_p(smb_en, piix4_smb_idx_sb800);
> + smba_en_lo = inb_p(piix4_smb_idx_sb800 + 1);
> + outb_p(smb_en + 1, piix4_smb_idx_sb800);
> + smba_en_hi = inb_p(piix4_smb_idx_sb800 + 1);
>  
>   if (!smb_en) {
>   smb_en_status = smba_en_lo & 0x10;
> @@ -616,16 +612,26 @@ static int piix4_add_adapter(struct pci_dev *dev, 
> unsigned short smba,
>  
>  static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  {
> + unsigned short smba_idx = 0xcd6;
>   int retval;
>  
>   if ((dev->vendor == PCI_VENDOR_ID_ATI &&
>dev->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS &&
>dev->revision >= 0x40) ||
> - dev->vendor == PCI_VENDOR_ID_AMD)
> + dev->vendor == PCI_VENDOR_ID_AMD) {
> +
> + if (!request_region(smba_idx, 2, "smba_idx")) {
> + dev_err(>dev, "SMBus base address index region "
> + "0x%x already in use!\n", smba_idx);
> + return -EBUSY;
> + }
> + piix4_smb_idx_sb800 = smba_idx;

If I read this right, piix4_smb_idx_sb800 will always be 0xcd6. Why do
you need the global variable at all then? Why not have constant like
below?

#define SB800_PIIX4_SMB_IDX 0xcd6
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/5] i2c-piix4: Add support for multiplexed main adapter in SB800

[Mika Westerberg]

On Sun, Nov 01, 2015 at 05:32:08PM +0100, Christian Fetzer wrote:
> The SB800 chipset supports a multiplexed main SMBus controller with
> four ports. The multiplexed ports share the same SMBus address and
> register set. The port is selected by bits 2:1 of the smb_en register
> (0x2C).
> 
> Only one port can be active at any point in time therefore a mutex is
> needed in order to synchronize access.
> 
> Tested on HP ProLiant MicroServer G7 N54L (where this patch adds
> support to access sensor data from the w83795adg).
> 
> Cc: Thomas Brandon <tbrando...@gmail.com>
> Cc: Eddi De Pieri <e...@depieri.net>
> Signed-off-by: Christian Fetzer <fetzer...@gmail.com>

Few minor comments, see below.

Regardless of them,

Reviewed-by: Mika Westerberg <mika.westerb...@linux.intel.com>

> ---
>  drivers/i2c/busses/i2c-piix4.c | 102 
> +++--
>  1 file changed, 97 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index d866b58..54f8af5 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -23,6 +23,9 @@
>  
> Note: we assume there can only be one device, with one or more
> SMBus interfaces.
> +   The device can register multiple i2c_adapters (up to PIIX4_MAX_ADAPTERS).
> +   For devices supporting multiple ports the i2c_adapter should provide
> +   an i2c_algorithm to access them.
>  */
>  
>  #include 
> @@ -37,6 +40,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  
>  /* PIIX4 SMBus address offsets */
> @@ -126,10 +130,12 @@ static const struct dmi_system_id piix4_dmi_ibm[] = {
>  };
>  
>  /* SB800 globals */
> +DEFINE_MUTEX(piix4_mutex_sb800);
>  static unsigned short piix4_smb_idx_sb800;
>  
>  struct i2c_piix4_adapdata {
>   unsigned short smba;
> + unsigned short port;
>  };
>  
>  static int piix4_setup(struct pci_dev *PIIX4_dev,
> @@ -309,6 +315,8 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
>   else
>   dev_dbg(_dev->dev, "Using SMI# for SMBus\n");
>  
> + mutex_init(_mutex_sb800);
> +
>   dev_info(_dev->dev,
>"SMBus Host Controller at 0x%x, revision %d\n",
>piix4_smba, i2ccfg >> 4);
> @@ -523,6 +531,42 @@ static s32 piix4_access(struct i2c_adapter * adap, u16 
> addr,
>   return 0;
>  }
>  
> +/* Handles access to multiple SMBus ports on the SB800.
> + * The port is selected by bits 2:1 of the smb_en register (0x2C).
> + * Returns negative errno on error.

Block comments look like:

   /*
* Handles access to multiple...
*

> + *
> + * Note: The selected port must be returned to the initial selection to avoid
> + * problems on certain systems.
> + */
> +static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
> +  unsigned short flags, char read_write,
> +  u8 command, int size, union i2c_smbus_data *data)
> +{
> + struct i2c_piix4_adapdata *adapdata = i2c_get_adapdata(adap);
> + u8 smba_en_lo, smb_en = 0x2c;
> + u8 port;
> + int retval;
> +
> + mutex_lock(_mutex_sb800);
> +
> + outb_p(smb_en, piix4_smb_idx_sb800);
> + smba_en_lo = inb_p(piix4_smb_idx_sb800 + 1);
> +
> + port = adapdata->port;
> + if ((smba_en_lo & 6) != (port << 1))
> + outb_p((smba_en_lo & ~6) | (port << 1),
> +piix4_smb_idx_sb800 + 1);
> +
> + retval = piix4_access(adap, addr, flags, read_write,
> +   command, size, data);
> +
> + outb_p(smba_en_lo, piix4_smb_idx_sb800 + 1);
> +
> + mutex_unlock(_mutex_sb800);
> +
> + return retval;
> +}
> +
>  static u32 piix4_func(struct i2c_adapter *adapter)
>  {
>   return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
> @@ -535,6 +579,11 @@ static const struct i2c_algorithm smbus_algorithm = {
>   .functionality  = piix4_func,
>  };
>  
> +static const struct i2c_algorithm piix4_smbus_algorithm_sb800 = {
> + .smbus_xfer = piix4_access_sb800,
> + .functionality  = piix4_func,
> +};
> +
>  static const struct pci_device_id piix4_ids[] = {
>   { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371AB_3) },
>   { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82443MX_3) },
> @@ -610,6 +659,42 @@ static int piix4_add_adapter(struct pci_dev *dev, 
> unsigned short smba,
>   return 0;
>  }
>  
> +static int piix4_add_adapters_sb800(struct pci_dev *dev, unsigned short smba)
> +{
> + unsigned short port;
> + 

Re: ACPI I2C device-driver matching issue

[Mika Westerberg]

On Sat, Oct 24, 2015 at 05:32:29PM +0200, Rafael J. Wysocki wrote:
> CC: Mika
> 
> On Thursday, October 22, 2015 01:05:42 PM Ben Gardner wrote:
> > Hi all,
> > 
> > I have a custom Baytrail board with a M24C02 EEPROM attached to I2C bus 3.
> > I am using coreboot/SeaBIOS, so I have complete control over the ACPI 
> > tables.
> > I am using Linux 4.2.3.
> > 
> > I have defined a EEPROM device on I2C3 using I2cSerialBus() and it
> > shows up as expected.
> > 
> > Scope (\_SB.PCI0.I2C3) {
> >   Device (EEP0) {
> > Name (_CID, Package() { "24c02" })
> > Name (_CRS, ResourceTemplate () {
> >   I2cSerialBus (0x0057, ControllerInitiated, 40,
> > AddressingMode7Bit, "\\_SB.PCI0.I2C3", 0x00,
> > ResourceConsumer,,)
> > })
> >   }
> > }

This is being discussed in another thread:

http://marc.info/?l=linux-acpi=144562104914442=2
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: i801: Add support for Intel DNV

[Mika Westerberg]

On Sun, Oct 25, 2015 at 11:45:18AM +0100, Jean Delvare wrote:
> Hi Mika,
> 
> On Tue, 13 Oct 2015 15:41:39 +0300, Mika Westerberg wrote:
> > Intel DNV SoC has the same legacy SMBus host controller than Intel
> > Sunrisepoint PCH. It also has same iTCO watchdog on the bus.
> > 
> > Add DNV PCI ID to the list of supported devices.
> > 
> > Signed-off-by: Mika Westerberg <mika.westerb...@linux.intel.com>
> > ---
> >  drivers/i2c/busses/i2c-i801.c | 4 
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> > index eaef9bc9d88c..47c2ddf76264 100644
> > --- a/drivers/i2c/busses/i2c-i801.c
> > +++ b/drivers/i2c/busses/i2c-i801.c
> > @@ -60,6 +60,7 @@
> >   * BayTrail (SOC)  0x0f12  32  hardyes yes yes
> >   * Sunrise Point-H (PCH)   0xa123  32  hardyes yes yes
> >   * Sunrise Point-LP (PCH)  0x9d23  32  hardyes yes yes
> > + * DNV (SOC)   0x19df  32  hardyes yes 
> > yes
> >   *
> >   * Features supported by this driver:
> >   * Software PECno
> > @@ -202,6 +203,7 @@
> >  #define PCI_DEVICE_ID_INTEL_WILDCATPOINT_LP_SMBUS  0x9ca2
> >  #define PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_SMBUS   0xa123
> >  #define PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_SMBUS  0x9d23
> > +#define PCI_DEVICE_ID_INTEL_DNV_SMBUS  0x19df
> 
> Can you please get this added to pci.ids?
> 
> http://pci-ids.ucw.cz/read/PC/8086

Sure.

> >  
> >  struct i801_mux_config {
> > char *gpio_chip;
> > @@ -863,6 +865,7 @@ static const struct pci_device_id i801_ids[] = {
> > { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BRASWELL_SMBUS) },
> > { PCI_DEVICE(PCI_VENDOR_ID_INTEL, 
> > PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_SMBUS) },
> > { PCI_DEVICE(PCI_VENDOR_ID_INTEL, 
> > PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_SMBUS) },
> > +   { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_DNV_SMBUS) },
> > { 0, }
> >  };
> >  
> > @@ -1256,6 +1259,7 @@ static int i801_probe(struct pci_dev *dev, const 
> > struct pci_device_id *id)
> > switch (dev->device) {
> > case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_SMBUS:
> > case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_SMBUS:
> > +   case PCI_DEVICE_ID_INTEL_DNV_SMBUS:
> > priv->features |= FEATURE_I2C_BLOCK_READ;
> > priv->features |= FEATURE_IRQ;
> > priv->features |= FEATURE_SMBUS_PEC;
> 
> Looks good, but please also update Documentation/i2c/busses/i2c-i801
> and drivers/i2c/busses/Kconfig.

Will do thanks.

Wolfram, do you want followup patch on top of this or do you prefer
updated patch?
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: i801: Document Intel DNV and Broxton

[Mika Westerberg]

On Mon, Oct 26, 2015 at 01:26:56PM +0200, Jarkko Nikula wrote:
> Add missing entries into i2c-i801 documentation and Kconfig about recently
> added Intel DNV and Broxton.
> 
> Suggested-by: Jean Delvare <jdelv...@suse.de>
> Signed-off-by: Jarkko Nikula <jarkko.nik...@linux.intel.com>
> Cc: Mika Westerberg <mika.westerb...@linux.intel.com>

Thanks Jarkko!

Reviewed-by: Mika Westerberg <mika.westerb...@linux.intel.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/2] i2c: add ACPI support for I2C mux ports

[Mika Westerberg]

On Fri, Oct 23, 2015 at 12:16:11PM +0200, Wolfram Sang wrote:
> On Fri, Oct 23, 2015 at 11:40:13AM +0300, Mika Westerberg wrote:
> > On Thu, Oct 22, 2015 at 02:17:42AM -0700, Dustin Byford wrote:
> > > Although I2C mux devices are easily enumerated using ACPI (_HID/_CID or
> > > device property compatible string match), enumerating I2C client devices
> > > connected through an I2C mux needs a little extra work.
> > > 
> > > This change implements a method for describing an I2C device hierarchy 
> > > that
> > > includes mux devices by using an ACPI Device() for each mux channel along
> > > with an _ADR to set the channel number for the device.  See
> > > Documentation/acpi/i2c-muxes.txt for a simple example.
> > > 
> > > To make this work the ismt, i801, and designware pci/platform devs now
> > > share an ACPI companion with their I2C adapter dev similar to how it's 
> > > done
> > > in OF.  This is done on the assumption that power management functions 
> > > will
> > > not be called directly on the I2C dev that is sharing the ACPI node.
> > > 
> > > Signed-off-by: Dustin Byford <dus...@cumulusnetworks.com>
> > 
> > This looks good to me.
> > 
> > You did also some stylistic changes to the drivers in question which I
> > think should be placed to a separate patches.
> 
> I am fine with those.
> 
> > Regardless of that,
> > 
> > Acked-by: Mika Westerberg <mika.westerb...@linux.intel.com>
> 
> I would love to get a Tested-by for the designware part. Then, I could
> queue it for 4.4 already.

Tested on Intel Baytrail and Skylake and the existing I2C devices
(touchpad, touchscreen) still work so for the designware part you can
also add my,

Tested-by: Mika Westerberg <mika.westerb...@linux.intel.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/2] i2c: add ACPI support for I2C mux ports

[Mika Westerberg]

On Fri, Oct 23, 2015 at 04:13:11PM +0300, Mika Westerberg wrote:
> On Fri, Oct 23, 2015 at 12:16:11PM +0200, Wolfram Sang wrote:
> > On Fri, Oct 23, 2015 at 11:40:13AM +0300, Mika Westerberg wrote:
> > > On Thu, Oct 22, 2015 at 02:17:42AM -0700, Dustin Byford wrote:
> > > > Although I2C mux devices are easily enumerated using ACPI (_HID/_CID or
> > > > device property compatible string match), enumerating I2C client devices
> > > > connected through an I2C mux needs a little extra work.
> > > > 
> > > > This change implements a method for describing an I2C device hierarchy 
> > > > that
> > > > includes mux devices by using an ACPI Device() for each mux channel 
> > > > along
> > > > with an _ADR to set the channel number for the device.  See
> > > > Documentation/acpi/i2c-muxes.txt for a simple example.
> > > > 
> > > > To make this work the ismt, i801, and designware pci/platform devs now
> > > > share an ACPI companion with their I2C adapter dev similar to how it's 
> > > > done
> > > > in OF.  This is done on the assumption that power management functions 
> > > > will
> > > > not be called directly on the I2C dev that is sharing the ACPI node.
> > > > 
> > > > Signed-off-by: Dustin Byford <dus...@cumulusnetworks.com>
> > > 
> > > This looks good to me.
> > > 
> > > You did also some stylistic changes to the drivers in question which I
> > > think should be placed to a separate patches.
> > 
> > I am fine with those.
> > 
> > > Regardless of that,
> > > 
> > > Acked-by: Mika Westerberg <mika.westerb...@linux.intel.com>
> > 
> > I would love to get a Tested-by for the designware part. Then, I could
> > queue it for 4.4 already.
> 
> Tested on Intel Baytrail and Skylake and the existing I2C devices
> (touchpad, touchscreen) still work so for the designware part you can
> also add my,
> 
> Tested-by: Mika Westerberg <mika.westerb...@linux.intel.com>

Ah, forgot to mention that the i2c-designware-pcidrv.c misses include of
 so that needs to be fixed. Otherwise we get:

drivers/i2c/busses/i2c-designware-pcidrv.c: In function ‘i2c_dw_pci_probe’:
drivers/i2c/busses/i2c-designware-pcidrv.c:259:2: error: implicit declaration 
of function ‘ACPI_COMPANION_SET’ [-Werror=implicit-function-declaration]
drivers/i2c/busses/i2c-designware-pcidrv.c:259:33: error: implicit declaration 
of function ‘ACPI_COMPANION’ [-Werror=implicit-function-declaration]
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/2] acpi: add acpi_preset_companion() stub

[Mika Westerberg]

On Thu, Oct 22, 2015 at 02:17:41AM -0700, Dustin Byford wrote:
> Add a stub for acpi_preset_companion().  Fixes build failures when
> acpi_preset_companion() is used and CONFIG_ACPI is not set.
> 
> Signed-off-by: Dustin Byford <dus...@cumulusnetworks.com>
> ---
>  include/linux/acpi.h | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 51a96a8..66564f8 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -505,6 +505,12 @@ static inline bool has_acpi_companion(struct device *dev)
>   return false;
>  }
>  
> +static inline void acpi_preset_companion(struct device *dev,
> +  struct acpi_device *parent, u64 addr)
> +{
> + return;

This return is not needed.

Acked-by: Mika Westerberg <mika.westerb...@linux.intel.com>

> +}
> +
>  static inline const char *acpi_dev_name(struct acpi_device *adev)
>  {
>   return NULL;
> -- 
> 2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] i2c: designware: reverts "i2c: designware: Add support for AMD I2C controller"

[Mika Westerberg]

On Fri, Oct 23, 2015 at 01:28:47PM +0800, Ken Xue wrote:
> The patch reverts commit a445900c9060 (i2c: designware: Add support for
> AMD I2C controller)
> 
> Since kernel starts to support APD(drivers/acpi/acpi_apd.c), there is
> no need to get freq from id->driver_data for AMD0010. clkdev is supposed
> to be already registered in APD.
> 
> So, revert old design and make AMD0010 looks like other ones.
> 
> Signed-off-by: Ken Xue <ken@amd.com>
> Signed-off-by: Xiangliang Yu <xiangliang...@amd.com>

You could also mention that the commit never even worked.

Anyway, I'm happy that this is removed so

Acked-by: Mika Westerberg <mika.westerb...@linux.intel.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/2] i2c: add ACPI support for I2C mux ports

[Mika Westerberg]

On Thu, Oct 22, 2015 at 02:17:42AM -0700, Dustin Byford wrote:
> Although I2C mux devices are easily enumerated using ACPI (_HID/_CID or
> device property compatible string match), enumerating I2C client devices
> connected through an I2C mux needs a little extra work.
> 
> This change implements a method for describing an I2C device hierarchy that
> includes mux devices by using an ACPI Device() for each mux channel along
> with an _ADR to set the channel number for the device.  See
> Documentation/acpi/i2c-muxes.txt for a simple example.
> 
> To make this work the ismt, i801, and designware pci/platform devs now
> share an ACPI companion with their I2C adapter dev similar to how it's done
> in OF.  This is done on the assumption that power management functions will
> not be called directly on the I2C dev that is sharing the ACPI node.
> 
> Signed-off-by: Dustin Byford <dus...@cumulusnetworks.com>

This looks good to me.

You did also some stylistic changes to the drivers in question which I
think should be placed to a separate patches.

Regardless of that,

Acked-by: Mika Westerberg <mika.westerb...@linux.intel.com>

I'll leave this up to Rafael and Wolfram to decide how to go forward
with this patch.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [MinnowBoard] Linux x86 I2C device probing with ACPI

[Mika Westerberg]

On Wed, Oct 21, 2015 at 09:19:24PM +0200, Christophe Ricard wrote:
> From: Christophe Ricard 
> 
> Hi,
> 
> I am trying to probe slave i2c devices with ACPI running Ubuntu 15.04 and 
> kernel 4.3
> without success so far.
> 
> I've followed guidance here:
> http://elinux.org/Minnowboard:MinnowMaxLinuxKernel 
> 
> 
> but i found no i2c device are probed at boot.

By probed do you mean that they are not listed under
/sys/bus/i2c/devices/ or that a driver is not probed against an existing
device?

> For example, one default device available in the acpi table is never 
> detected: RTEK node (with ID 10EC5640).

If the device _STA() method returns 0 then we do not enumerate it. You
can check this by looking for the corresponding ACPI device node. For
example I have here I2C connected touch screen:

# cat /sys/bus/acpi/devices/NTRG0001\:00/status 
15

15 means that it is there (among other things). Also the device is then
available as I2C device here:

# ls -1 /sys/bus/i2c/devices/
i2c-0
i2c-1
i2c-2
i2c-NTRG0001:00

> When adding my own device to I2C7, my device is never detected as well .
> 
> For example the complete modified I2C7 is at the end of the message.
> 
> I am compiling the kernel with options:
> CONFIG_ACPI_CUSTOM_DSDT_FILE="dsdt.hex"
> CONFIG_ACPI_CUSTOM_DSDT=y
> CONFIG_ACPI_INITRD_TABLE_OVERRIDE=y

You don't need the last one.

> I am running on Minnowboard firmware 0.83 with default options.
> 
> Best Regards
> Christophe
> 
> Device (I2C7)
> {
> Name (_ADR, Zero)  // _ADR: Address
> Name (_HID, "80860F41" /* Intel Baytrail I2C Host Controller */)  
> // _HID: Hardware ID
> Name (_CID, "80860F41" /* Intel Baytrail I2C Host Controller */)  
> // _CID: Compatible ID
> Name (_DDN, "Intel(R) I2C Controller #7 - 80860F47") // _DDN: DOS 
> Device Name
> Name (_UID, 0x07)  // _UID: Unique ID
> Name (_DEP, Package (One)  // _DEP: Dependencies
> {
> PEPD
> })
> Name (RBUF, ResourceTemplate ()
> {
> Memory32Fixed (ReadWrite,
> 0x, // Address Base
> 0x1000, // Address Length
> _Y1F)
> Interrupt (ResourceConsumer, Level, ActiveLow, Exclusive, ,, )
> {
> 0x0026,
> }
> FixedDMA (0x001C, 0x0004, Width32bit, )
> FixedDMA (0x001D, 0x0005, Width32bit, )
> })
> Method (SSCN, 0, NotSerialized)
> {
> Name (PKG, Package (0x03)
> {
> 0x0200,
> 0x0200,
> 0x06
> })
> Return (PKG) /* \_SB_.I2C7.SSCN.PKG_ */
> }
> 
> Method (FMCN, 0, NotSerialized)
> {
> Name (PKG, Package (0x03)
> {
> 0x55,
> 0x99,
> 0x06
> })
> Return (PKG) /* \_SB_.I2C7.FMCN.PKG_ */
> }
> 
> Method (FPCN, 0, NotSerialized)
> {
> Name (PKG, Package (0x03)
> {
> 0x1B,
> 0x3A,
> 0x06
> })
> Return (PKG) /* \_SB_.I2C7.FPCN.PKG_ */
> }
> 
> Method (_HRV, 0, NotSerialized)  // _HRV: Hardware Revision
> {
> Return (SOCS) /* \SOCS */
> }
> 
> Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource 
> Settings
> {
> CreateDWordField (RBUF, \_SB.I2C7._Y1F._BAS, B0BA)  // _BAS: 
> Base Address
> CreateDWordField (RBUF, \_SB.I2C7._Y1F._LEN, B0LN)  // _LEN: 
> Length
> B0BA = I70A /* \I70A */
> B0LN = I70L /* \I70L */
> Return (RBUF) /* \_SB_.I2C7.RBUF */
> }
> 
> Method (_STA, 0, NotSerialized)  // _STA: Status
> {
> If ((PCIM == One))
> {
> Return (Zero)
> }
> 
> If (((I70A == Zero) || (L27D == One)))
> {
> Return (Zero)
> }
> 
> Return (0x0F)
> }
> 
> Method (_PS3, 0, NotSerialized)  // _PS3: Power State 3
> {
> PSAT |= 0x03
> PSAT |= Zero
> }
> 
> Method (_PS0, 0, NotSerialized)  // _PS0: Power State 0
> {
> PSAT &= 0xFFFC
> PSAT |= Zero
> }
> 
> OperationRegion (KEYS, SystemMemory, I71A, 0x0100)
> Field (KEYS, DWordAcc, NoLock, 

Re: [PATCH] i2c: designware: Disable runtime PM in case i2c_dw_probe() fails

[Mika Westerberg]

On Wed, Oct 21, 2015 at 05:21:46PM +0300, Jarkko Nikula wrote:
> Call to pm_runtime_disable() got dropped while handling the merge conflict
> between commit 36d48fb5766a ("i2c: designware-platdrv: enable RuntimePM
> before registering to the core") and commit d80d134182ba ("i2c: designware:
> Move common probe code into i2c_dw_probe()").
> 
> Signed-off-by: Jarkko Nikula <jarkko.nik...@linux.intel.com>

Acked-by: Mika Westerberg <mika.westerb...@linux.intel.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] Support multiplexed main SMBus interface on SB800

[Mika Westerberg]

On Tue, Oct 20, 2015 at 05:19:35PM +0200, Wolfram Sang wrote:
> On Tue, Aug 25, 2015 at 01:05:01PM +0200, Christian Fetzer wrote:
> > This is an attempt to upstream the patches created by Thomas Brandon and
> > Eddi De Pieri to support the multiplexed main SMBus interface on the SB800
> > chipset. 
> > (https://www.mail-archive.com/linux-i2c@vger.kernel.org/msg06757.html)
> > 
> > I have mainly rebased the latest patch version and tested the driver on a
> > HP ProLiant MicroServer G7 N54L (where this patch allows to access sensor 
> > data
> > from a w83795adg).
> > 
> > The patched driver is running stable on the machine, given that ic2_piix4 is
> > loaded before jc42 and w83795. If jc42 is loaded before i2c_piix4 calling
> > sensors triggers some errors:
> > ERROR: Can't get value of subfeature temp1_min_alarm: Can't read
> > 
> > While the kernel log shows:
> > i2c i2c-1: Transaction (pre): CNT=0c, CMD=05, ADD=31, DAT0=03, DAT1=c0
> > i2c i2c-1: Error: no response!
> > i2c i2c-1: Transaction (post): CNT=0c, CMD=05, ADD=31, DAT0=ff, DAT1=ff
> > Unfortunately I don't know how to tackle this specific issue.
> > 
> > Please review and let me know required changes in order to get this upstream
> > finally.
> > 
> > Eddi, Thomas, it would be great if you could verify the changes on your
> > machines.
> 
> Yes, additional tests are always good for a patch series
> 
> Asking the Intel guys for help, I have not much expierence with x86
> platforms... Mika, Jarkko, Andy any chance to help?

Unfortunately I don't have hardware this old to test on :-/
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] Support multiplexed main SMBus interface on SB800

[Mika Westerberg]

On Thu, Oct 22, 2015 at 01:03:25PM +0200, Wolfram Sang wrote:
> 
> > > > Please review and let me know required changes in order to get this 
> > > > upstream
> > > > finally.
> > > > 
> > > > Eddi, Thomas, it would be great if you could verify the changes on your
> > > > machines.
> > > 
> > > Yes, additional tests are always good for a patch series
> > > 
> > > Asking the Intel guys for help, I have not much expierence with x86
> > > platforms... Mika, Jarkko, Andy any chance to help?
> > 
> > Unfortunately I don't have hardware this old to test on :-/
> 
> And visual review? (That's what I need to do mostly, too)

Sure.

I don't have a copy of these patches but I went ahead and looked them up
from archives. Christian can you Cc me on next iteration?

Mostly they look good to me. Few comments though.

Patch 2/4: should we remove adapter in reverse order?

Patch 3/4: some stylistic issues, like:
- ERROR label should not be in capital letters actually it is
  not needed at all if you do unlock and return -EBUSY if
  request_region() fails.
- Block comment style

In addition I'm not sure if requesting io region for each transfer is
good idea. Can't we just request it once for this driver and handle the
necessary serialization using the mutex?
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] i2c: designware: register clkdev during acpi device configuration

[Mika Westerberg]

On Wed, Oct 21, 2015 at 05:50:02PM +0800, Ken Xue wrote:
> After apd was accept in kernel V4.1, there is no issue. But between 3.18
> and V4.1, there will be a problem.

We care about that because?
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports

[Mika Westerberg]

On Wed, Oct 21, 2015 at 01:52:41AM -0700, Dustin Byford wrote:
> On Wed Oct 21 11:34, Mika Westerberg wrote:
> > On Wed, Oct 21, 2015 at 01:21:16AM -0700, Dustin Byford wrote:
> > > On Wed Oct 21 11:12, Mika Westerberg wrote:
> > > > On Tue, Oct 20, 2015 at 10:49:59AM -0700, Dustin Byford wrote:
> > > > > I considered it, but I thought a default that fairly closely matches 
> > > > > the
> > > > > old behavior was more convenient.
> > > > > 
> > > > > On the other hand, leaving it up to the controllers makes it all very
> > > > > explicit and perhaps simpler to reason about.
> > > > > 
> > > > > 
> > > > > I could be convinced either way.  But, if we move it to the controller
> > > > > drivers, which ones need the change?
> > > > > 
> > > > > grep -i acpi drivers/i2c/busses/i2c*
> > > > > 
> > > > > shows 18 drivers that might care.
> > > > 
> > > > I'm quite confident the designware I2C is enough for now. Intel uses it
> > > > for all SoCs with LPSS and I think AMD has the same block for their I2C
> > > > solution.
> > > 
> > > I certainly care about i801, ismt, and isch.  Doesn't this affect any
> > > i2c controller with clients that may be enumerated through ACPI?
> > 
> > Yes, but so far I haven't seen any other devices being used for this
> > than the I2C designware.
> > 
> > Which hardware you are testing this patch on?
> 
> I'm working with a number of x86-based network switch platforms.  Mostly
> rangeley at the moment, but I'm sure others are in the works.  Have a
> look at:
> 
> http://www.opencompute.org/wiki/Networking/SpecsAndDesigns
> 
> for examples.

Cool :)

> My goal, hence the recent patches, is to help the network switch
> industry move a lot of platform description into ACPI.  That means lots
> of complicated I2C trees; switches are full of I2C devices.

I see.

I don't really have strong feelings whether it should be the I2C core or
individual drivers setting the ACPI companion. However, it would be nice
to match DT here and they assign their of_node per driver.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] i2c: designware: register clkdev during acpi device configuration

[Mika Westerberg]

On Wed, Oct 21, 2015 at 04:42:23PM +0800, Ken Xue wrote:
> On Wed, 2015-10-21 at 10:28 +0300, Mika Westerberg wrote:
> > On Wed, Oct 21, 2015 at 09:11:33AM +0800, Ken Xue wrote:
> > > On Tue, 2015-10-20 at 14:17 +0300, Mika Westerberg wrote:
> > > > On Tue, Oct 20, 2015 at 02:38:01PM +0800, Ken Xue wrote:
> > > > > DW I2C driver tries to register a clk from id->driver_data as an
> > > > > alternative way besides intel lpss. But code doesn't register the
> > > > > clk to clkdev. So, devm_clk_get will fail during probe.
> > > > > 
> > > > > The patch can fix this issue.
> > > > 
> > > > Since you now have drivers/acpi/acpi_apd.c for AMD ACPI stuff, can you
> > > > create the clock there just like we do for Intel stuff?
> > > Sure. APD already creates the clock for AMD0010 as you expected. And the
> > > next patch([PATCH 2/2] i2c: designware: remove freq definition for
> > > "AMD0010" in acpi_device_id) is dropping the old way for getting freq.
> > 
> > So this patch is not necessary, right?
> Even though there is no use case that getting freq from id->driver_data,
> But if we want to keep this design, then we should use current patch for
> fixing the potential issue. So, the patch is nice to have.

What potential issue?

If you pass clock from drivers/acpi/acpi_apd.c and drop the hard coded
freq for AMD0010 in the I2C designware driver, the driver still works
just fine.

> Otherwise, we have to revert whole old design(a445900c).

Yes please :-)
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] i2c: designware: register clkdev during acpi device configuration

[Mika Westerberg]

On Wed, Oct 21, 2015 at 05:37:53PM +0800, Ken Xue wrote:
> On Wed, 2015-10-21 at 12:25 +0300, Mika Westerberg wrote:
> > On Wed, Oct 21, 2015 at 04:42:23PM +0800, Ken Xue wrote:
> > > On Wed, 2015-10-21 at 10:28 +0300, Mika Westerberg wrote:
> > > > On Wed, Oct 21, 2015 at 09:11:33AM +0800, Ken Xue wrote:
> > > > > On Tue, 2015-10-20 at 14:17 +0300, Mika Westerberg wrote:
> > > > > > On Tue, Oct 20, 2015 at 02:38:01PM +0800, Ken Xue wrote:
> > > > > > > DW I2C driver tries to register a clk from id->driver_data as an
> > > > > > > alternative way besides intel lpss. But code doesn't register the
> > > > > > > clk to clkdev. So, devm_clk_get will fail during probe.
> > > > > > > 
> > > > > > > The patch can fix this issue.
> > > > > > 
> > > > > > Since you now have drivers/acpi/acpi_apd.c for AMD ACPI stuff, can 
> > > > > > you
> > > > > > create the clock there just like we do for Intel stuff?
> > > > > Sure. APD already creates the clock for AMD0010 as you expected. And 
> > > > > the
> > > > > next patch([PATCH 2/2] i2c: designware: remove freq definition for
> > > > > "AMD0010" in acpi_device_id) is dropping the old way for getting freq.
> > > > 
> > > > So this patch is not necessary, right?
> > > Even though there is no use case that getting freq from id->driver_data,
> > > But if we want to keep this design, then we should use current patch for
> > > fixing the potential issue. So, the patch is nice to have.
> > 
> > What potential issue?
> devm_clk_get will fail during probe for AMD0010 without current patch.

How can it fail if you provide the very clock from drivers/acpi/acpi_apd.c?

> > 
> > If you pass clock from drivers/acpi/acpi_apd.c and drop the hard coded
> > freq for AMD0010 in the I2C designware driver, the driver still works
> > just fine.
> > 
> > > Otherwise, we have to revert whole old design(a445900c).
> > 
> > Yes please :-)
> Glad to do.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] i2c: designware: register clkdev during acpi device configuration

[Mika Westerberg]

On Wed, Oct 21, 2015 at 05:50:02PM +0800, Ken Xue wrote:
> On Wed, 2015-10-21 at 12:49 +0300, Mika Westerberg wrote:
> > On Wed, Oct 21, 2015 at 05:37:53PM +0800, Ken Xue wrote:
> > > On Wed, 2015-10-21 at 12:25 +0300, Mika Westerberg wrote:
> > > > On Wed, Oct 21, 2015 at 04:42:23PM +0800, Ken Xue wrote:
> > > > > On Wed, 2015-10-21 at 10:28 +0300, Mika Westerberg wrote:
> > > > > > On Wed, Oct 21, 2015 at 09:11:33AM +0800, Ken Xue wrote:
> > > > > > > On Tue, 2015-10-20 at 14:17 +0300, Mika Westerberg wrote:
> > > > > > > > On Tue, Oct 20, 2015 at 02:38:01PM +0800, Ken Xue wrote:
> > > > > > > > > DW I2C driver tries to register a clk from id->driver_data as 
> > > > > > > > > an
> > > > > > > > > alternative way besides intel lpss. But code doesn't register 
> > > > > > > > > the
> > > > > > > > > clk to clkdev. So, devm_clk_get will fail during probe.
> > > > > > > > > 
> > > > > > > > > The patch can fix this issue.
> > > > > > > > 
> > > > > > > > Since you now have drivers/acpi/acpi_apd.c for AMD ACPI stuff, 
> > > > > > > > can you
> > > > > > > > create the clock there just like we do for Intel stuff?
> > > > > > > Sure. APD already creates the clock for AMD0010 as you expected. 
> > > > > > > And the
> > > > > > > next patch([PATCH 2/2] i2c: designware: remove freq definition for
> > > > > > > "AMD0010" in acpi_device_id) is dropping the old way for getting 
> > > > > > > freq.
> > > > > > 
> > > > > > So this patch is not necessary, right?
> > > > > Even though there is no use case that getting freq from 
> > > > > id->driver_data,
> > > > > But if we want to keep this design, then we should use current patch 
> > > > > for
> > > > > fixing the potential issue. So, the patch is nice to have.
> > > > 
> > > > What potential issue?
> > > devm_clk_get will fail during probe for AMD0010 without current patch.
> > 
> > How can it fail if you provide the very clock from drivers/acpi/acpi_apd.c?
> After apd was accept in kernel V4.1, there is no issue. But between 3.18
> and V4.1, there will be a problem.

Ah, now I got it.

You are saying that the original commit a445900c906092 ("i2c:
designware: Add support for AMD I2C controller") actually never worked
because it failed to register the clock with clkdev? In that case it is
not even a regression ;-) Oh my...

In that case I don't think we need to fix that for 3.18+ because it
never worked in the first place.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports

[Mika Westerberg]

On Wed, Oct 21, 2015 at 01:12:01AM +0200, Rafael J. Wysocki wrote:
> Well, we already have that in the MFD case, but in principle it may be
> problematic for things like power management (say you want to put a
> child device into D3, so you use _PS3 on its ACPI companion and then
> the parent is powere down instead).

That case I understand and we should not allow that. However, here we
have an I2C adapter which is purely Linux abstraction that does not have
any representation either in DT nor ACPI. And we don't do any power
management for that either.

If I understand correctly, DT shares the same of_node for both the host
controller device and the adapter which then makes it possible to
enumerate devices behind by just looking adap->dev.of_node. In case of
ACPI we need to know that it's the parent device that we should look for
child devices which may not be true always (like what this patch is
trying to resolve).

> At least, devices in that setup should not be attached to the ACPI PM
> domain.

Agreed.

Currently acpi_dev_pm_attach() only attaches the first physical device
to the ACPI power domain so this should be taken care already.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports

[Mika Westerberg]

On Tue, Oct 20, 2015 at 10:49:59AM -0700, Dustin Byford wrote:
> I considered it, but I thought a default that fairly closely matches the
> old behavior was more convenient.
> 
> On the other hand, leaving it up to the controllers makes it all very
> explicit and perhaps simpler to reason about.
> 
> 
> I could be convinced either way.  But, if we move it to the controller
> drivers, which ones need the change?
> 
> grep -i acpi drivers/i2c/busses/i2c*
> 
> shows 18 drivers that might care.

I'm quite confident the designware I2C is enough for now. Intel uses it
for all SoCs with LPSS and I think AMD has the same block for their I2C
solution.

> > adap->dev.parent = >dev;
> > adap->dev.of_node = pdev->dev.of_node;
> > ACPI_COMPANION_SET(>dev, ACPI_COMPANION(>dev));
> 
> Interesting, this code isn't in my tree.  I wonder why it was added,
> what code looks at the acpi companion on the i2c dev?  Before my change
> it was supposed to be NULL, and it is NULL on every other controller.

It is not in any tree. I meant that before b34bb1ee71158d5b it looked
something like that :-)
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports

[Mika Westerberg]

On Wed, Oct 21, 2015 at 01:21:16AM -0700, Dustin Byford wrote:
> On Wed Oct 21 11:12, Mika Westerberg wrote:
> > On Tue, Oct 20, 2015 at 10:49:59AM -0700, Dustin Byford wrote:
> > > I considered it, but I thought a default that fairly closely matches the
> > > old behavior was more convenient.
> > > 
> > > On the other hand, leaving it up to the controllers makes it all very
> > > explicit and perhaps simpler to reason about.
> > > 
> > > 
> > > I could be convinced either way.  But, if we move it to the controller
> > > drivers, which ones need the change?
> > > 
> > > grep -i acpi drivers/i2c/busses/i2c*
> > > 
> > > shows 18 drivers that might care.
> > 
> > I'm quite confident the designware I2C is enough for now. Intel uses it
> > for all SoCs with LPSS and I think AMD has the same block for their I2C
> > solution.
> 
> I certainly care about i801, ismt, and isch.  Doesn't this affect any
> i2c controller with clients that may be enumerated through ACPI?

Yes, but so far I haven't seen any other devices being used for this
than the I2C designware.

Which hardware you are testing this patch on?
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] i2c: designware: register clkdev during acpi device configuration

[Mika Westerberg]

On Wed, Oct 21, 2015 at 09:11:33AM +0800, Ken Xue wrote:
> On Tue, 2015-10-20 at 14:17 +0300, Mika Westerberg wrote:
> > On Tue, Oct 20, 2015 at 02:38:01PM +0800, Ken Xue wrote:
> > > DW I2C driver tries to register a clk from id->driver_data as an
> > > alternative way besides intel lpss. But code doesn't register the
> > > clk to clkdev. So, devm_clk_get will fail during probe.
> > > 
> > > The patch can fix this issue.
> > 
> > Since you now have drivers/acpi/acpi_apd.c for AMD ACPI stuff, can you
> > create the clock there just like we do for Intel stuff?
> Sure. APD already creates the clock for AMD0010 as you expected. And the
> next patch([PATCH 2/2] i2c: designware: remove freq definition for
> "AMD0010" in acpi_device_id) is dropping the old way for getting freq.

So this patch is not necessary, right?
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] i2c: designware: register clkdev during acpi device configuration

[Mika Westerberg]

On Tue, Oct 20, 2015 at 02:38:01PM +0800, Ken Xue wrote:
> DW I2C driver tries to register a clk from id->driver_data as an
> alternative way besides intel lpss. But code doesn't register the
> clk to clkdev. So, devm_clk_get will fail during probe.
> 
> The patch can fix this issue.

Since you now have drivers/acpi/acpi_apd.c for AMD ACPI stuff, can you
create the clock there just like we do for Intel stuff?
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports

[Mika Westerberg]

On Mon, Oct 19, 2015 at 03:29:00PM -0700, Dustin Byford wrote:
> Although I2C mux devices are easily enumerated using ACPI (_HID/_CID or
> device property compatible string match) enumerating I2C client devices
> connected through a I2C mux device requires a little extra work.
> 
> This change implements a method for describing an I2C device hierarchy that
> includes mux devices by using an ACPI Device() for each mux channel along
> with an _ADR to set the channel number for the device.  See
> Documentation/acpi/i2c-muxes.txt for a simple example.
> 
> Signed-off-by: Dustin Byford 

In general this looks good to me.

> ---
>  Documentation/acpi/i2c-muxes.txt | 58 
> 
>  drivers/i2c/i2c-core.c   | 15 +--
>  drivers/i2c/i2c-mux.c|  8 ++
>  include/linux/acpi.h |  6 +
>  4 files changed, 85 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/acpi/i2c-muxes.txt
> 

[...]

> + /*
> +  * By default, associate I2C adapters with their parent device's ACPI
> +  * node.
> +  */
> + if (!has_acpi_companion(dev)) {
> + struct acpi_device *adev = ACPI_COMPANION(dev->parent);
> +
> + if (adev)
> + ACPI_COMPANION_SET(dev, adev);

Instead of always doing this in the I2C core, maybe we can make it
dependent on the host controller driver. For example the I2C designware
driver already did this for both DT and ACPI:

adap->dev.parent = >dev;
adap->dev.of_node = pdev->dev.of_node;
ACPI_COMPANION_SET(>dev, ACPI_COMPANION(>dev));

Also I would like to ask what Rafael thinks about this since he authored
b34bb1ee71158d5b ("ACPI / I2C: Use parent's ACPI_HANDLE() in
acpi_i2c_register_devices()").

I don't see a problem multiple Linux devices sharing a single ACPI
companion device like in this patch but I may be forgetting something ;-)
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/4] i2c: designware-platdrv: enable RuntimePM before registering to the core

[Mika Westerberg]

On Thu, Oct 15, 2015 at 01:49:25PM +0200, Wolfram Sang wrote:
> On Fri, Oct 09, 2015 at 10:39:24AM +0100, Wolfram Sang wrote:
> > From: Wolfram Sang 
> > 
> > The core may register clients attached to this master which may use
> > funtionality from the master. So, RuntimePM must be enabled before, 
> > otherwise
> > this will fail.
> > 
> > Signed-off-by: Wolfram Sang 
> 
> No feedback here, but for the other two drivers the same principle was
> acked. CCing Mika, just in case.

Looks good to me :-)

> 
> Applied to for-current, thanks!
> 
> > ---
> >  drivers/i2c/busses/i2c-designware-platdrv.c | 13 +++--
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c 
> > b/drivers/i2c/busses/i2c-designware-platdrv.c
> > index 3dd2de31a2f8d3..73d58415bbc100 100644
> > --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> > @@ -253,12 +253,6 @@ static int dw_i2c_probe(struct platform_device *pdev)
> > adap->dev.parent = >dev;
> > adap->dev.of_node = pdev->dev.of_node;
> >  
> > -   r = i2c_add_numbered_adapter(adap);
> > -   if (r) {
> > -   dev_err(>dev, "failure adding adapter\n");
> > -   return r;
> > -   }
> > -
> > if (dev->pm_runtime_disabled) {
> > pm_runtime_forbid(>dev);
> > } else {
> > @@ -268,6 +262,13 @@ static int dw_i2c_probe(struct platform_device *pdev)
> > pm_runtime_enable(>dev);
> > }
> >  
> > +   r = i2c_add_numbered_adapter(adap);
> > +   if (r) {
> > +   dev_err(>dev, "failure adding adapter\n");
> > +   pm_runtime_disable(>dev);
> > +   return r;
> > +   }
> > +
> > return 0;
> >  }
> >  
> > -- 
> > 2.1.4
> > 


--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] MAINTAINERS: add maintainers for Synopsis Designware I2C drivers

[Mika Westerberg]

On Thu, Oct 15, 2015 at 03:58:53PM +0200, Wolfram Sang wrote:
> Those guys already have been helpful in the past and are actively
> working on this driver, unlike me.
> 
> Signed-off-by: Wolfram Sang <w...@the-dreams.de>
> Acked-by: Jarkko Nikula <jarkko.nik...@linux.intel.com>
> Cc: Andy Shevchenko <andriy.shevche...@linux.intel.com>
> Cc: Mika Westerberg <mika.westerb...@linux.intel.com>

Acked-by: Mika Westerberg <mika.westerb...@linux.intel.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] i2c: i801: Add support for Intel DNV

[Mika Westerberg]

Intel DNV SoC has the same legacy SMBus host controller than Intel
Sunrisepoint PCH. It also has same iTCO watchdog on the bus.

Add DNV PCI ID to the list of supported devices.

Signed-off-by: Mika Westerberg <mika.westerb...@linux.intel.com>
---
 drivers/i2c/busses/i2c-i801.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index eaef9bc9d88c..47c2ddf76264 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -60,6 +60,7 @@
  * BayTrail (SOC)  0x0f12  32  hardyes yes yes
  * Sunrise Point-H (PCH)   0xa123  32  hardyes yes yes
  * Sunrise Point-LP (PCH)  0x9d23  32  hardyes yes yes
+ * DNV (SOC)   0x19df  32  hardyes yes yes
  *
  * Features supported by this driver:
  * Software PECno
@@ -202,6 +203,7 @@
 #define PCI_DEVICE_ID_INTEL_WILDCATPOINT_LP_SMBUS  0x9ca2
 #define PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_SMBUS   0xa123
 #define PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_SMBUS  0x9d23
+#define PCI_DEVICE_ID_INTEL_DNV_SMBUS  0x19df
 
 struct i801_mux_config {
char *gpio_chip;
@@ -863,6 +865,7 @@ static const struct pci_device_id i801_ids[] = {
{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BRASWELL_SMBUS) },
{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 
PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_SMBUS) },
{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 
PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_SMBUS) },
+   { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_DNV_SMBUS) },
{ 0, }
 };
 
@@ -1256,6 +1259,7 @@ static int i801_probe(struct pci_dev *dev, const struct 
pci_device_id *id)
switch (dev->device) {
case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_SMBUS:
case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_SMBUS:
+   case PCI_DEVICE_ID_INTEL_DNV_SMBUS:
priv->features |= FEATURE_I2C_BLOCK_READ;
priv->features |= FEATURE_IRQ;
priv->features |= FEATURE_SMBUS_PEC;
-- 
2.5.3

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] i2c: add ACPI support for I2C mux ports

[Mika Westerberg]

On Mon, Oct 12, 2015 at 11:32:31AM -0700, Dustin Byford wrote:
> I've been trying to consider the options, perhaps you can help my
> understanding.  Using the i801 driver as an example, the device is PCI
> and the companion is associated with the PCI dev.  The driver creates
> another device for the I2C interface (parented by the PCI device) by
> calling i2c_add_adapter().  The I2C dev has no ACPI companion.
> 
> In the case of an I2C mux port, I've used acpi_preset_companion() to
> associate each mux port I2C device with a ACPI node.  Unlike the i801,
> which has a single port, these companions are one per channel.  It's not
> an option to associate them all with the I2C mux device.
> 
> It seems like the options are to:
> 
> a) Special case the I2C mux to use the per-port I2C companions as I've
>done here.
> 
> b) Move (or copy?) the companion from the i801 PCI dev to the i801 I2C
>dev.  Then we would always look in the same place for the companion.
>I think this approach has some advantages, at least it would make
>more sense if an I2C PCI controller had more than one I2C port, but
>I'm not sure that case exists.  I didn't pursue this approach because
>it was specifically avoided in change b34bb1ee.
> 
> 
> What do you think?  I'd be happy to try out any ideas you have.

I would favour b) because that follows DT (the I2C host controller
device and I2C adapter share the same DT node as far as I can tell).
Neither of them have similar concept of I2C adapter as we have in Linux
(which is the "virtual" device on top of the I2C host controller).
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v2 0/1] i2c: acpi: scan ACPI enumerated I2C mux channels

[Mika Westerberg]

On Tue, Sep 29, 2015 at 04:19:12PM -0700, Dustin Byford wrote:
> Hi Mika,
> 
> On Mon Aug 17 15:03, Mika Westerberg wrote:
> > I think the current code in I2C core is not actually doing the right
> > thing according the ACPI spec at least. To my understanding you can have
> > device with I2cSerialBus resource _anywhere_ in the namespace, not just
> > directly below the host controller. It's the ResourceSource attribute
> > that tells the corresponding host controller.
> > 
> > I wonder if it helps if we scan the whole namespace for devices with
> > I2cSerialBus that matches the just registered adapter? Something like
> > the patch below.
> 
> I've been working with the patch you suggested below.
> 
> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > index c83e4d13cfc5..2a309d27421a 100644
> > --- a/drivers/i2c/i2c-core.c
> > +++ b/drivers/i2c/i2c-core.c
> 
> ...
> 
> >  static void acpi_i2c_register_devices(struct i2c_adapter *adap)
> >  {
> > - acpi_handle handle;
> >   acpi_status status;
> >  
> > - if (!adap->dev.parent)
> > - return;
> > -
> > - handle = ACPI_HANDLE(adap->dev.parent);
> > - if (!handle)
> > + if (!adap->dev.parent || !has_acpi_companion(adap->dev.parent))
> >   return;
> >  
> > - status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
> > + status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
> > +  ACPI_I2C_MAX_SCAN_DEPTH,
> >acpi_i2c_add_device, NULL,
> >adap, NULL);
> 
> On my systems (which admittedly all define their i2c clients below the
> controller) this works as expected, i.e. there's no change in behavior.
> As far as I can tell it more accurately implements the spec.
> 
> 
> However, it doesn't quite solve my problem.  When
> acpi_i2c_register_devices(adap) is called on the "virtual" controller
> that is created for an i2c mux channel, the adap->dev.parent (set to the
> parent i2c bus for the mux) does not have an acpi companion.  That
> ultimately causes acpi_i2c_add_device() to never find a match.
> 
> I'll recap a bit since it's been a while and I've learned a few things
> that might affect the discussion.  For now, I'll focus on my proposed
> ASL for an I2C mux using device properties.
> 
> Lets say we have i2c hardware attached like this:
> 
> i801 controller (PCI)
>pca9548 8-channel mux (address 0x70)
>lm75 temperature sensor (channel 0 on the mux with address 0x50)
> 
> I think this is a sensible way to represent it:
> 
> Device (MUX0) {
> Name (_ADR, 0x70)

This..

> Name (_HID, "PRP0001")
> Name (_CRS, ResourceTemplate()
> {
> I2cSerialBus (0x70, ControllerInitiated, I2C_SPEED,
>   AddressingMode7Bit, "^^SMB2", 0x00,
>   ResourceConsumer,,)
> })
> Name (_DSD, Package ()
> {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package () {
> Package (2) { "compatible", "nxp,pca9548" },
> }
> })
> 
> // MUX channel 0
> Device (CH00) {
> Name (_ADR, 0x0)
> 
> Device (TMP0) {
> Name (_ADR, 0x50)

... and this are not needed. I2cSerialBus already contains the address.

Also I think you need to have "PRP0001" here as well.

> Name (_CRS, ResourceTemplate()
> {
> I2cSerialBus (0x60, ControllerInitiated, I2C_SPEED,
>   AddressingMode7Bit, "^CH00", 0x00,
>   ResourceConsumer,,)
> })
> Name (_DSD, Package ()
> {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package () {
> Package (2) { "compatible", "national,lm75" },
> }
> })
> }
> }
> }
> 
> The new thing here is using _ADR to treat each mux channel as a device
> and referencing those devices elsewhere (CH00).  I arrived at this
> because it seems to fit the ACPI model reasonably well* and it's easy to
> implement (just like in other callers to acpi_preset_companion())
> 
> * by reasonably well, I think it's clear and works naturally but this
> use of _ADR isn't explicitly defined in the spec [ACPI 6, Table 6-168
> (page 278)]
> 
> Hopefully the spec ambiguity isn't too much effort to clarify.  I th

Re: [RFC v2 0/1] i2c: acpi: scan ACPI enumerated I2C mux channels

[Mika Westerberg]

On Wed, Sep 30, 2015 at 02:52:28PM +0200, Rafael J. Wysocki wrote:
> > > Device (CH00) {
> > > Name (_ADR, 0x0)
> > > 
> > > Device (TMP0) {
> > > Name (_ADR, 0x50)
> > 
> > ... and this are not needed. I2cSerialBus already contains the address.
> > 
> > Also I think you need to have "PRP0001" here as well.
> 
> The idea is to use _ADR kind of instead of "PRP0001" to express the "you
> don't need a driver for this" idea AFAICS.

But I think it needs a driver and it even includes "compatible" string
below :-)

> > > Name (_CRS, ResourceTemplate()
> > > {
> > > I2cSerialBus (0x60, ControllerInitiated, I2C_SPEED,
> > >   AddressingMode7Bit, "^CH00", 0x00,
> > >   ResourceConsumer,,)
> > > })
> > > Name (_DSD, Package ()
> > > {
> > > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > > Package () {
> > > Package (2) { "compatible", "national,lm75" },
> > > }
> > > })
> > > }
> > > }
> > > }
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/6] i2c: ismt: improve usage of devres API

[Mika Westerberg]

On Mon, Sep 28, 2015 at 01:45:32PM +0300, Andy Shevchenko wrote:
> On Wed, 2015-09-16 at 17:23 +0300, Andy Shevchenko wrote:
> > pcim_release() will release any requested region. There is no need to 
> > duplicate
> > this effort in the driver.
> 
> Any comments on that clean up series?

FWIW the whole series looks good to me,

Reviewed-by: Mika Westerberg <mika.westerb...@linux.intel.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] i2c: designware: Do not use parameters from ACPI on Dell Inspiron 7348

[Mika Westerberg]

ACPI SSCN/FMCN methods were originally added because then the platform can
provide the most accurate HCNT/LCNT values to the driver. However, this
seems not to be true for Dell Inspiron 7348 where using these causes the
touchpad to fail in boot:

  i2c_hid i2c-DLL0675:00: failed to retrieve report from device.
  i2c_designware INT3433:00: i2c_dw_handle_tx_abort: lost arbitration
  i2c_hid i2c-DLL0675:00: failed to retrieve report from device.
  i2c_designware INT3433:00: controller timed out

The values received from ACPI are (in fast mode):

  HCNT: 72
  LCNT: 160

this translates to following timings (input clock is 100MHz on Broadwell):

  tHIGH: 720 ns (spec min 600 ns)
  tLOW: 1600 ns (spec min 1300 ns)
  Bus period: 2920 ns (assuming 300 ns tf and tr)
  Bus speed: 342.5 kHz

Both tHIGH and tLOW are within the I2C specification.

The calculated values when ACPI parameters are not used are (in fast mode):

  HCNT: 87
  LCNT: 159

which translates to:

  tHIGH: 870 ns (spec min 600 ns)
  tLOW: 1590 ns (spec min 1300 ns)
  Bus period 3060 ns (assuming 300 ns tf and tr)
  Bus speed 326.8 kHz

These values are also within the I2C specification.

Since both ACPI and calculated values meet the I2C specification timing
requirements it is hard to say why the touchpad does not function properly
with the ACPI values except that the bus speed is higher in this case (but
still well below the max 400kHz).

Solve this by adding DMI quirk to the driver that disables using ACPI
parameters on this particulare machine.

Reported-by: Pavel Roskin <plros...@gmail.com>
Signed-off-by: Mika Westerberg <mika.westerb...@linux.intel.com>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c 
b/drivers/i2c/busses/i2c-designware-platdrv.c
index 3dd2de31a2f8..e76514ff440c 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -24,6 +24,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -51,6 +52,22 @@ static u32 i2c_dw_get_clk_rate_khz(struct dw_i2c_dev *dev)
 }
 
 #ifdef CONFIG_ACPI
+/*
+ * The HCNT/LCNT information coming from ACPI should be the most accurate
+ * for given platform. However, some systems get it wrong. On such systems
+ * we get better results by calculating those based on the input clock.
+ */
+static const struct dmi_system_id dw_i2c_no_acpi_params[] = {
+   {
+   .ident = "Dell Inspiron 7348",
+   .matches = {
+   DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+   DMI_MATCH(DMI_PRODUCT_NAME, "Inspiron 7348"),
+   },
+   },
+   { }
+};
+
 static void dw_i2c_acpi_params(struct platform_device *pdev, char method[],
   u16 *hcnt, u16 *lcnt, u32 *sda_hold)
 {
@@ -58,6 +75,9 @@ static void dw_i2c_acpi_params(struct platform_device *pdev, 
char method[],
acpi_handle handle = ACPI_HANDLE(>dev);
union acpi_object *obj;
 
+   if (dmi_check_system(dw_i2c_no_acpi_params))
+   return;
+
if (ACPI_FAILURE(acpi_evaluate_object(handle, method, NULL, )))
return;
 
-- 
2.5.1

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Fix for Dell P57G touchpad found, need advice on patch

[Mika Westerberg]

Adding Jarkko as well.

On Tue, Sep 15, 2015 at 07:11:51PM +0200, Wolfram Sang wrote:
> On Tue, Sep 15, 2015 at 06:42:14PM +0200, Wolfram Sang wrote:
> > Hi Pavel,
> > 
> > On Thu, Sep 10, 2015 at 11:11:41PM -0700, Pavel Roskin wrote:
> > > Hi!
> > 
> > Thanks for this detailed info. Adding Mika to CC who is an expert of
> > this driver. Mika, maybe we should add you to MAINTAINERS, too?
> > 
> 
> And forgot to add Mika :)
> 
> > > I'm using Dell Inspiron 13, model P57G. It has a problem with the
> > > touchpad under Linux. Approximately half of the time, the touchpad is
> > > not working at all - the mouse cursor is not moving. In this case,
> > > there are error messages written to the kernel log every second:
> > > 
> > > [   53.127339] i2c_designware INT3433:00: controller timed out
> > > [   54.219336] i2c_designware INT3433:00: controller timed out
> > > [   55.311346] i2c_designware INT3433:00: controller timed out
> > > [   56.403326] i2c_designware INT3433:00: controller timed out
> > > 
> > > There is a simple fix - blacklist the driver
> > > (i2c_designware_platform). In this case, the touchpad words as a mouse
> > > and used IRQ 12. Googling for "blackist i2c_designware_platform" shows
> > > that I'm not the one using that approach.
> > > 
> > > I started looking at the driver in git. A patch by Romain Baeriswyl
> > > applied on 2014-08-20 adds support for the "standard" mode with the
> > > 100kHz clock, as opposed to the 400kHz "fast" mode. Unfortunately,
> > > that patch only affects OpenFirmware systems, and I have ACPI. As soon
> > > as I set the clock to 100kHz, the touchpad started working every time.
> > > 
> > > One fix would be to have a module parameter to force slower clock. It
> > > would still require users to deal with modprobe, so it's not optimal.
> > > 
> > > I noticed that dev->get_clk_rate_khz(dev) returns 10, but that
> > > would be 10kHz = 100MHz. Not sure is somebody confused the units
> > > or it's the correct clock.
> > > 
> > > Considering that I have no other systems to test, what would be the
> > > best approach? Set the clock to 100kHz for INT3433? Recognize Dell
> > > P57G system specifically? How? Preserve the original clock by reading
> > > the DW_IC_CON register and keeping some bits?
> > 
> > Note: The driver has been using 400kHz from its beginning. However,
> > 100kHz is usually a better default for I2C busses. I wonder if we should
> > take the risk of a performance regression in favour of a sane and
> > working default?

Pavel,

Can you send me acpidump of that machine (or contents of
/sys/firmware/acpi/tables/DSDT)? It may be that the ACPI I2cSerialBus
connector actually has 100kHz there - we just don't use it currently.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v2 0/1] i2c: acpi: scan ACPI enumerated I2C mux channels

[Mika Westerberg]

On Fri, Aug 14, 2015 at 12:31:32PM -0700, Dustin Byford wrote:
 
 (v2 corrects cc: list)
 
 I would like to add support for scanning I2C devices connected to ACPI
 OF compatible muxes described in ASL like this:
 
 Device (MUX0)
 {
 Name (_ADR, 0x70)
 Name (_HID, PRP0001)
 Name (_CRS, ResourceTemplate()
 {
 I2cSerialBus (0x70, ControllerInitiated, I2C_SPEED,
   AddressingMode7Bit, ^^SMB2, 0x00,
   ResourceConsumer,,)
 })
 Name (_DSD, Package ()
 {
 ToUUID(daffd814-6eba-4d8c-8a91-bc9bbf4aa301),
 Package () {
 Package (2) { compatible, nxp,pca9548 },
 }

Nice, you are using _DSD :-)

 })
 
 // MUX channels
 Device (CH00) { Name (_ADR, 0x0) }
 }
 
 Scope(MUX0.CH00)
 {
 Device (TMP0) {
 /* Temp sensor ASL, for example. */
 }
 }
 
 It seems like a reasonable way to describe a common I2C component and
 kernel support is almost there.
 
 I had to:
 
 1) Find and set an ACPI companion for the virtual I2C adapters created
for each mux channel.
 
 2) Make sure to scan adap.dev when registering devices under each mux
channel.
 
 At first, I was confused about why adap.dev-parent is used in
 acpi_i2c_register_devices().  I found b34bb1ee from 4/2013 (ACPI / I2C:
 Use parent's ACPI_HANDLE()), which offers an explanation.
 
 This patch works well, but I'm not sure about the code to just fall back
 to using adap.dev when adap.dev-parent doesn't have an ACPI companion.
 Is there a more explicit check I can make to determine if the adapter
 represents a mux channel?

I think the current code in I2C core is not actually doing the right
thing according the ACPI spec at least. To my understanding you can have
device with I2cSerialBus resource _anywhere_ in the namespace, not just
directly below the host controller. It's the ResourceSource attribute
that tells the corresponding host controller.

I wonder if it helps if we scan the whole namespace for devices with
I2cSerialBus that matches the just registered adapter? Something like
the patch below.

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index c83e4d13cfc5..2a309d27421a 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -94,27 +94,40 @@ struct gsb_buffer {
};
 } __packed;
 
-static int acpi_i2c_add_resource(struct acpi_resource *ares, void *data)
+struct acpi_i2c_lookup {
+   struct i2c_board_info *info;
+   acpi_handle adapter_handle;
+   acpi_handle device_handle;
+};
+
+static int acpi_i2c_find_address(struct acpi_resource *ares, void *data)
 {
-   struct i2c_board_info *info = data;
+   struct acpi_i2c_lookup *lookup = data;
+   struct i2c_board_info *info = lookup-info;
+   struct acpi_resource_i2c_serialbus *sb;
+   acpi_handle adapter_handle;
+   acpi_status status;
 
-   if (ares-type == ACPI_RESOURCE_TYPE_SERIAL_BUS) {
-   struct acpi_resource_i2c_serialbus *sb;
+   if (info-addr || ares-type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
+   return 1;
 
-   sb = ares-data.i2c_serial_bus;
-   if (!info-addr  sb-type == ACPI_RESOURCE_SERIAL_TYPE_I2C) {
-   info-addr = sb-slave_address;
-   if (sb-access_mode == ACPI_I2C_10BIT_MODE)
-   info-flags |= I2C_CLIENT_TEN;
-   }
-   } else if (!info-irq) {
-   struct resource r;
+   sb = ares-data.i2c_serial_bus;
+   if (sb-type != ACPI_RESOURCE_SERIAL_TYPE_I2C)
+   return 1;
 
-   if (acpi_dev_resource_interrupt(ares, 0, r))
-   info-irq = r.start;
+   /*
+* Extract the ResourceSource and make sure that the handle matches
+* with the I2C adapter handle.
+*/
+   status = acpi_get_handle(lookup-device_handle,
+sb-resource_source.string_ptr,
+adapter_handle);
+   if (ACPI_SUCCESS(status)  adapter_handle == lookup-adapter_handle) {
+   info-addr = sb-slave_address;
+   if (sb-access_mode == ACPI_I2C_10BIT_MODE)
+   info-flags |= I2C_CLIENT_TEN;
}
 
-   /* Tell the ACPI core to skip this resource */
return 1;
 }
 
@@ -123,6 +136,8 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, 
u32 level,
 {
struct i2c_adapter *adapter = data;
struct list_head resource_list;
+   struct acpi_i2c_lookup lookup;
+   struct resource_entry *entry;
struct i2c_board_info info;
struct acpi_device *adev;
int ret;
@@ -135,14 +150,37 @@ static acpi_status acpi_i2c_add_device(acpi_handle 
handle, u32 level,
memset(info, 0, sizeof(info));
info.fwnode = acpi_fwnode_handle(adev);
 
+   memset(lookup, 0, sizeof(lookup));
+   lookup.adapter_handle = ACPI_HANDLE(adapter-dev.parent);
+   

Re: [PATCH 1/1] i2c: designware-pci: use IRQF_COND_SUSPEND flag

[Mika Westerberg]

On Fri, Jul 31, 2015 at 12:07:23PM +0200, Wolfram Sang wrote:
 On Mon, Jul 27, 2015 at 02:37:23PM +0300, Andy Shevchenko wrote:
  On Wed, 2015-07-08 at 13:15 +0300, Andy Shevchenko wrote:
   The mentioned flag fixes a warning on Intel Edison board since one of 
   the I2C
   controller shares IRQ line with watchdog timer.
   
  
  Wolfram, do you have any comments on this?
 
 Adding Mika to cc, he is more experienced with these platforms.

intel_mid_wdt.c requests the same IRQ with IRQF_NO_SUSPEND so this patch
makes sense to me,

Acked-by: Mika Westerberg mika.westerb...@linux.intel.com
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: designware: use enable on resume instead initialization

[Mika Westerberg]

On Wed, Jun 24, 2015 at 09:36:43AM +0200, christian.rupp...@alitech.com wrote:
 Dear Lucas,
 
 Lucas De Marchi lucas.de.mar...@gmail.com wrote on 23.06.2015 19:02:03:
  On Tue, Jun 23, 2015 at 1:45 PM,  christian.rupp...@alitech.com wrote:
   Hello,
  
   Christian Ruppert/ALi_GVA/ALi wrote on 10.06.2015 17:05:16:
  [...]
   The result is not very encouraging: Out of five (identical) designware 
 i2c
   controllers we have on my test SOC, the first one initialises properly 
 but
   the second one gets stuck in the famous irq loop right away when the
   module is enabled in i2c_dw_init. The system never gets around to try
  
  Are you using the pci or platform driver?  I noticed yesterday the pci
  version is failing here with a NULL pointer dereference.
 
 The test was performed with the platform driver (instantiated through 
 device tree).
 I just re-checked and the ultimate problem which hangs/kills the system in 
 my case is the IRQ loop.
 I haven't observed any NULL pointer dereferences on the road.

Thanks Christian for testing.

Since the patch causes problems on your hardware, I don't think it is
good idea to merge it.
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] i2c: designware: Make sure the device is suspended before disabling runtime PM

[Mika Westerberg]

The driver calls pm_runtime_put() right before pm_runtime_disable() in its
-remove() hook to make sure clock is gated etc. However, it turns out that
pm_runtime_put() only calls -idle() hook without actually suspending
anything. The following pm_runtime_disable() will prevent the driver from
suspending thus leaving it active.

It is better to suspend the device synchronously to make sure it is
actually suspended before disabling runtime PM from it.

While there, undo call to pm_runtime_use_autosuspend().

Signed-off-by: Mika Westerberg mika.westerb...@linux.intel.com
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c 
b/drivers/i2c/busses/i2c-designware-platdrv.c
index 4794911a6165..3dd2de31a2f8 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -281,7 +281,8 @@ static int dw_i2c_remove(struct platform_device *pdev)
 
i2c_dw_disable(dev);
 
-   pm_runtime_put(pdev-dev);
+   pm_runtime_dont_use_autosuspend(pdev-dev);
+   pm_runtime_put_sync(pdev-dev);
pm_runtime_disable(pdev-dev);
 
if (has_acpi_companion(pdev-dev))
-- 
2.1.4

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: designware: Make sure the device is suspended before disabling runtime PM

[Mika Westerberg]

On Wed, Jun 17, 2015 at 12:16:45PM +0200, Wolfram Sang wrote:
 On Wed, Jun 17, 2015 at 12:08:38PM +0300, Mika Westerberg wrote:
  The driver calls pm_runtime_put() right before pm_runtime_disable() in its
  -remove() hook to make sure clock is gated etc. However, it turns out that
  pm_runtime_put() only calls -idle() hook without actually suspending
  anything. The following pm_runtime_disable() will prevent the driver from
  suspending thus leaving it active.
  
  It is better to suspend the device synchronously to make sure it is
  actually suspended before disabling runtime PM from it.
  
  While there, undo call to pm_runtime_use_autosuspend().
  
  Signed-off-by: Mika Westerberg mika.westerb...@linux.intel.com
 
 For current? For next? stable?
 

Right, sorry about that.

I think it is fine to get this for next.
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: designware: use enable on resume instead initialization

[Mika Westerberg]

On Fri, Jun 12, 2015 at 07:45:00PM -0300, Lucas De Marchi wrote:
 Hi Mika,
 
 On Wed, Jun 10, 2015 at 4:55 AM, Mika Westerberg
 mika.westerb...@linux.intel.com wrote:
  On Mon, Jun 08, 2015 at 02:50:28PM -0300, lucas.de.mar...@gmail.com wrote:
  @@ -320,7 +320,7 @@ static int dw_i2c_resume(struct device *dev)
clk_prepare_enable(i_dev-clk);
 
if (!i_dev-pm_runtime_disabled)
  - i2c_dw_init(i_dev);
  + i2c_dw_enable(i_dev);
 
  This will not work if the device power gets removed (for example being
  put to D3cold) as it looses context.
 
 Do you mean we should keep the i2c_dw_init() here?

Yes, that's safer if the controller power gets removed.
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: designware: use enable on resume instead initialization

[Mika Westerberg]

On Wed, Jun 10, 2015 at 05:05:16PM +0200, christian.rupp...@alitech.com wrote:
We should understand why the controller was disabled after successful
transfers in the first place, however. Maybe some quirk with older
versions of the hardware? Mika, do you have any memories about this?

I think it was in the original driver even before I did Intel specific
changes to that. I have no idea why it is done. Probably, like you said,
because some older version of the hardware needed it.
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: designware: use enable on resume instead initialization

[Mika Westerberg]

On Mon, Jun 08, 2015 at 02:50:28PM -0300, lucas.de.mar...@gmail.com wrote:
 @@ -320,7 +320,7 @@ static int dw_i2c_resume(struct device *dev)
   clk_prepare_enable(i_dev-clk);
  
   if (!i_dev-pm_runtime_disabled)
 - i2c_dw_init(i_dev);
 + i2c_dw_enable(i_dev);

This will not work if the device power gets removed (for example being
put to D3cold) as it looses context.
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: designware: use enable on resume instead initialization

[Mika Westerberg]

On Tue, Jun 09, 2015 at 03:29:01PM -0300, Lucas De Marchi wrote:
 Hi Mika,
 
 On Tue, Jun 9, 2015 at 5:51 AM, Mika Westerberg
 mika.westerb...@linux.intel.com wrote:
  On Mon, Jun 08, 2015 at 02:50:28PM -0300, lucas.de.mar...@gmail.com wrote:
  From: Fabio Mello fabio.me...@intel.com
 
  According to documentation and tests, initialization is not
  necessary on module resume, since the controller keeps its state
  between disable/enable. Change the target address is also allowed.
 
  So, this patch replaces the initialization on module resume with a
  simple enable, and removes the (non required anymore) enables and
  disables.
 
  Signed-off-by: Fabio Mello fabio.me...@intel.com
  Signed-off-by: Lucas De Marchi lucas.demar...@intel.com
  ---
 
  These pictures explain a little more the consequence of letting the
  enable+disable in the code:
 
http://pub.politreco.com/paste/TEK0011-before.jpg
http://pub.politreco.com/paste/TEK0007-after.jpg
 
  The yellow line is a GPIO toggle in userspace to mark when we start and 
  finish
  the i2c transactions.  The blue line is the SCL in that i2c bus. Take a 
  look on
  the huge pauses we have between any 2 transactions.  These pauses are 
  removed
  with this patch and we are able to read our sensor's values in 950usec 
  rather
  than 5.24msec we had before.  We are testing this using a Minnowboard Max 
  that
  has a designware i2c controller.
 
  Did you test this on any other platform than Intel Baytrail?
 
 No. The only soc we have here with this controller is the Baytrail.

My concern is that this patch might break some non-Intel platform. It
would be nice if someone (Christian?) could try this out.
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: designware: use enable on resume instead initialization

[Mika Westerberg]

On Mon, Jun 08, 2015 at 02:50:28PM -0300, lucas.de.mar...@gmail.com wrote:
 From: Fabio Mello fabio.me...@intel.com
 
 According to documentation and tests, initialization is not
 necessary on module resume, since the controller keeps its state
 between disable/enable. Change the target address is also allowed.
 
 So, this patch replaces the initialization on module resume with a
 simple enable, and removes the (non required anymore) enables and
 disables.
 
 Signed-off-by: Fabio Mello fabio.me...@intel.com
 Signed-off-by: Lucas De Marchi lucas.demar...@intel.com
 ---
 
 These pictures explain a little more the consequence of letting the
 enable+disable in the code:
 
   http://pub.politreco.com/paste/TEK0011-before.jpg
   http://pub.politreco.com/paste/TEK0007-after.jpg
 
 The yellow line is a GPIO toggle in userspace to mark when we start and finish
 the i2c transactions.  The blue line is the SCL in that i2c bus. Take a look 
 on
 the huge pauses we have between any 2 transactions.  These pauses are removed
 with this patch and we are able to read our sensor's values in 950usec rather
 than 5.24msec we had before.  We are testing this using a Minnowboard Max that
 has a designware i2c controller.

Did you test this on any other platform than Intel Baytrail?

I'm adding Christian Ruppert (keeping the context) if he has any
comments. IIRC he added the per transfer enable/disable some time ago.

 
 There's this comment in the code suggesting the designware controller might
 have problems if we don't disable it after each transfer.  We left a stress
 code running for 3 days to check if anything bad would happen.  This is the
 test code using a PCA9685 (just because it was the easiest device we had
 available to check read+write commands):
 
   http://pub.politreco.com/paste/test-i2c.c
 
  drivers/i2c/busses/i2c-designware-core.c| 19 ---
  drivers/i2c/busses/i2c-designware-platdrv.c |  2 +-
  2 files changed, 5 insertions(+), 16 deletions(-)
 
 diff --git a/drivers/i2c/busses/i2c-designware-core.c 
 b/drivers/i2c/busses/i2c-designware-core.c
 index 6e25c01..b386c10 100644
 --- a/drivers/i2c/busses/i2c-designware-core.c
 +++ b/drivers/i2c/busses/i2c-designware-core.c
 @@ -375,8 +375,12 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
   /* configure the i2c master */
   dw_writel(dev, dev-master_cfg , DW_IC_CON);
  
 + /* Enable the adapter */
 + __i2c_dw_enable(dev, true);
 +
   if (dev-release_lock)
   dev-release_lock(dev);
 +
   return 0;
  }
  EXPORT_SYMBOL_GPL(i2c_dw_init);
 @@ -405,9 +409,6 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
   struct i2c_msg *msgs = dev-msgs;
   u32 ic_con, ic_tar = 0;
  
 - /* Disable the adapter */
 - __i2c_dw_enable(dev, false);
 -
   /* if the slave address is ten bit address, enable 10BITADDR */
   ic_con = dw_readl(dev, DW_IC_CON);
   if (msgs[dev-msg_write_idx].flags  I2C_M_TEN) {
 @@ -434,9 +435,6 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
   /* enforce disabled interrupts (due to HW issues) */
   i2c_dw_disable_int(dev);
  
 - /* Enable the adapter */
 - __i2c_dw_enable(dev, true);
 -
   /* Clear and enable interrupts */
   i2c_dw_clear_int(dev);
   dw_writel(dev, DW_IC_INTR_DEFAULT_MASK, DW_IC_INTR_MASK);
 @@ -665,15 +663,6 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg 
 msgs[], int num)
   goto done;
   }
  
 - /*
 -  * We must disable the adapter before unlocking the dev-lock mutex
 -  * below. Otherwise the hardware might continue generating interrupts
 -  * which in turn causes a race condition with the following transfer.
 -  * Needs some more investigation if the additional interrupts are
 -  * a hardware bug or this driver doesn't handle them correctly yet.
 -  */
 - __i2c_dw_enable(dev, false);
 -
   if (dev-msg_err) {
   ret = dev-msg_err;
   goto done;
 diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c 
 b/drivers/i2c/busses/i2c-designware-platdrv.c
 index c270f5f..0598695 100644
 --- a/drivers/i2c/busses/i2c-designware-platdrv.c
 +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
 @@ -320,7 +320,7 @@ static int dw_i2c_resume(struct device *dev)
   clk_prepare_enable(i_dev-clk);
  
   if (!i_dev-pm_runtime_disabled)
 - i2c_dw_init(i_dev);
 + i2c_dw_enable(i_dev);
  
   return 0;
  }
 -- 
 2.4.2
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] i2c: designware: Avoid unnecessary resuming during system suspend

[Mika Westerberg]

[Adding Jarkko, just in case he has concerns about this]

On Wed, May 20, 2015 at 10:33:13PM +0800, Jisheng Zhang wrote:
 Commit 1fc2fe204cb9 (i2c: designware: Add runtime PM hooks) adds
 runtime pm support using the same ops for system pm and runtime pm.
 When suspend to ram, the i2c host may have been runtime suspended, thus
 i2c_dw_disable() hangs.
 
 Previously, I fixed this issue by separating ops for system pm and
 runtime pm, then in the system suspend/resume path, runtime pm apis are
 used to ensure the device is at correct state.
 
 But as Mika Westerberg pointed out: it sounds a bit silly to resume the
 device just because you want to call i2c_dw_disable() for it before
 suspending again. He then suggested an elegant solution which keeps the
 device runtime suspended during system suspend with the help of
 'dev-power.direct_complete'. This patch adopted this solution, and in
 fact Mika provided the main code.
 
 Signed-off-by: Jisheng Zhang jszh...@marvell.com
 ---
  v2 change:
   - adopt Mika's suggestion to make use of direct_complete flag
 
  drivers/i2c/busses/i2c-designware-platdrv.c | 33 
 +
  1 file changed, 29 insertions(+), 4 deletions(-)
 
 diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c 
 b/drivers/i2c/busses/i2c-designware-platdrv.c
 index 0a80e4a..f89650f 100644
 --- a/drivers/i2c/busses/i2c-designware-platdrv.c
 +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
 @@ -298,6 +298,22 @@ static const struct of_device_id dw_i2c_of_match[] = {
  MODULE_DEVICE_TABLE(of, dw_i2c_of_match);
  #endif
  
 +#ifdef CONFIG_PM_SLEEP
 +static int dw_i2c_prepare(struct device *dev)
 +{
 + return pm_runtime_suspended(dev);
 +}
 +
 +static void dw_i2c_complete(struct device *dev)
 +{
 + if (dev-power.direct_complete)
 + pm_request_resume(dev);
 +}
 +#else
 +#define dw_i2c_prepare   NULL
 +#define dw_i2c_complete  NULL
 +#endif
 +
  #ifdef CONFIG_PM
  static int dw_i2c_suspend(struct device *dev)
  {
 @@ -322,10 +338,19 @@ static int dw_i2c_resume(struct device *dev)
  
   return 0;
  }
 -#endif
  
 -static UNIVERSAL_DEV_PM_OPS(dw_i2c_dev_pm_ops, dw_i2c_suspend,
 - dw_i2c_resume, NULL);
 +static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
 + .prepare = dw_i2c_prepare,
 + .complete = dw_i2c_complete,
 + SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_suspend, dw_i2c_resume)
 + SET_RUNTIME_PM_OPS(dw_i2c_suspend,
 + dw_i2c_resume, NULL)

No need to wrap here. It will fit into 80 chars limit.

Otherwise looks good to me. I also tested this on Braswell and Skylake
machines where it didn't cause any problems,

Acked-by: Mika Westerberg mika.westerb...@linux.intel.com
Tested-by: Mika Westerberg mika.westerb...@linux.intel.com

 +};
 +
 +#define DW_I2C_DEV_PMOPS (dw_i2c_dev_pm_ops)
 +#else
 +#define DW_I2C_DEV_PMOPS NULL
 +#endif
  
  /* work with hotplug and coldplug */
  MODULE_ALIAS(platform:i2c_designware);
 @@ -337,7 +362,7 @@ static struct platform_driver dw_i2c_driver = {
   .name   = i2c_designware,
   .of_match_table = of_match_ptr(dw_i2c_of_match),
   .acpi_match_table = ACPI_PTR(dw_i2c_acpi_match),
 - .pm = dw_i2c_dev_pm_ops,
 + .pm = DW_I2C_DEV_PMOPS,
   },
  };
  
 -- 
 2.1.4
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC] i2c: Use ID table to detect ACPI I2C devices

[Mika Westerberg]

On Wed, May 20, 2015 at 01:49:02PM +0300, Robert Dolca wrote:
 On Wed, May 20, 2015 at 12:48 PM, Mika Westerberg wrote:
  On Wed, May 20, 2015 at 12:39:22PM +0300, Robert Dolca wrote:
  Currently, if the name used for DT (in dts) matches one of the names
  specified in the id table you will have a match. Isn't that an
  intended behavior?
 
  I thought one needs to put IDs to the driver .of_match_table. This is
  also what i2c_device_match() is expecting, if I read it right.
 
 If you put the DT id in of_match_table it will match here:
 
 i2c_device_match
 /* Attempt an OF style match */
 if (of_driver_match_device(dev, drv))
 return 1;
 
 If you don't specify of_match_table and you put the same ID in
 i2c_device_id table it wil match here:
 
 i2c_device_match
 driver = to_i2c_driver(drv);
 /* match on an id table if there is one */
 if (driver-id_table)
 return i2c_match_id(driver-id_table, client) != NULL;
 
 This is happening because the name from dts is used for client-name.
 i2c_match_id does the matching based on the client name.

OK.

  BTW, how modules are supposed to be matched if we allow putting ACPI
  identifiers to i2c_device_id table?
 
 My aproach was like this: if the driver specifies .acpi_match table it
 will work like before.
 
 i2c_device_match
 /* Then ACPI style match */
 if (acpi_driver_match_device(dev, drv))
 return 1;
 
 If the driver does not specify .acpi_match table the i2c core will
 atempt to match against the  i2c_match_id table (the same way it does
 for DT). In the ACPI case the client-name has that :nn suffix and
 what the patch does is to ignore that when i2c_match_id is called.
 
 i2c_device_match
 driver = to_i2c_driver(drv);
 /* match on an id table if there is one */
 if (driver-id_table)
 return i2c_match_id(driver-id_table, client) != NULL;

Yeah but when you have device with modalias of acpi:FOO: how
udev/modprobe is supposed find the correct module?

 The final goal is to simplify the driver and remove redundant code.

IMHO mixing ACPI identifiers with I2C device identifiers does not
simplify anything. And since you need to stick the ACPI ID somewhere
anyway I don't get the point of removing redundant code either.
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: designware: separate ops for system_sleep_pm and runtime_pm

[Mika Westerberg]

On Wed, May 20, 2015 at 07:34:22PM +0800, Jisheng Zhang wrote:
 Sorry for confusion. Considering one platform which doesn't support power off
 the i2c host but it can disable the host's clock. So in such platform, when
 the host is runtime suspended, its clock is disabled, then i2c_dw_disable() 
 will
 hang when s2ram.

Right. This happens also when the platform powers off the device.

 Except using the runtime pm API to ensure the host is in
 a correct state, is there any other solution? AFAIK, 
 'dev-power.direct_complete'
 doesn't help such case.

What I had in mind is something like below:

static int i2c_dw_prepare(struct device *dev)
{
return pm_runtime_suspended(dev);
}

static void i2c_dw_complete(struct device *dev)
{
if (dev-power.direct_complete)
pm_request_resume(dev);
}

In other words it checks if the device is already runtime suspended and
prevents -suspend() etc. from being called.

If that does not work (I didn't try as this problem does not exist on
ACPI platforms because we have PM domain handling things) then we do not
have much of a choice than do what you suggest and runtime resume the
device on -suspend()
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] i2c: designware: Suppress error message if platform_get_irq() 0

[Mika Westerberg]

On Mon, Mar 09, 2015 at 12:03:12PM +0300, Alexey Brodkin wrote:
 As discussed here https://lkml.org/lkml/2015/3/3/568 and here
 https://lkml.org/lkml/2015/3/3/453 we're looking forward for
 implementing warnings and errors outputs right in platform_get_irq()
 instead of having all kind of different outputs in each and every driver
 that uses platform_get_irq().
 
 Signed-off-by: Alexey Brodkin abrod...@synopsys.com
 Cc: Vineet Gupta vgu...@synopsys.com
 Cc: Christian Ruppert christian.rupp...@abilis.com
 Cc: Mika Westerberg mika.westerb...@linux.intel.com

Acked-by: Mika Westerberg mika.westerb...@linux.intel.com
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: designware-baytrail: use proper Kconfig dependencies

[Mika Westerberg]

On Mon, Jan 26, 2015 at 05:49:34PM +0100, Wolfram Sang wrote:
 IOSF_MBI depends on PCI, so we should not select it but depend on it.
 This ensures also we compile on X86 only, other archs will break because
 of an arch specific include. Also depend on ACPI since this driver uses
 it.
 
 Signed-off-by: Wolfram Sang w...@the-dreams.de

Thanks for taking care of this,

Acked-by: Mika Westerberg mika.westerb...@linux.intel.com
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V4 2/2] i2c-designware: Add Intel Baytrail PMIC I2C bus support

[Mika Westerberg]

On Thu, Jan 22, 2015 at 12:48:51PM -0800, David E. Box wrote:
  It would be nice if it was possible to compile this as a module.
 
 In the makefile the driver is now part of the composite object for
 I2C_DESIGNWARE_PLATFORM. So if selected it will compile as the platform driver
 is compiled. This makes the most sense since the linking doesn't work right if
 both drivers aren't built the same. This is really an extension to the 
 platform
 driver anyway and not standalone code.

Makes sense. Thanks for the explanation.
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/4] i2c: designware-pci: shrink dw_pci_controllers array

[Mika Westerberg]

On Fri, Jan 23, 2015 at 01:41:55PM +0100, Wolfram Sang wrote:
 On Fri, Jan 23, 2015 at 01:54:03PM +0200, Andy Shevchenko wrote:
  There is no need to duplicate same data for each controller. If we need
  specific stuff for a certain controller in the future we may add it later. 
  The
  patch leaves one controller per platform.
  
  Signed-off-by: Andy Shevchenko andriy.shevche...@linux.intel.com
  ---
   drivers/i2c/busses/i2c-designware-pcidrv.c | 63 
  ++
   1 file changed, 11 insertions(+), 52 deletions(-)
  
  diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c 
  b/drivers/i2c/busses/i2c-designware-pcidrv.c
  index 5c6fca7..435a8ec 100644
  --- a/drivers/i2c/busses/i2c-designware-pcidrv.c
  +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
  @@ -40,13 +40,7 @@
   #define DRIVER_NAME i2c-designware-pci
   
   enum dw_pci_ctl_id_t {
  -   medfield_0,
  -   medfield_1,
  -   medfield_2,
  -   medfield_3,
  -   medfield_4,
  -   medfield_5,
  -
  +   medfield,
  baytrail,
  haswell,
   };
  @@ -98,47 +92,12 @@ static struct dw_scl_sda_cfg hsw_config = {
   };
   
   static struct  dw_pci_controller  dw_pci_controllers[] = {
  -   [medfield_0] = {
  -   .bus_num = 0,
 
 Wasn't that bus_num used to ensure stable bus numbers? Adding Mika.

That's right.

However, I don't think anybody really uses Medfield outside Intel so in
that sense this patch should not break anything.
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V4 2/2] i2c-designware: Add Intel Baytrail PMIC I2C bus support

[Mika Westerberg]

On Thu, Jan 15, 2015 at 01:12:17AM -0800, David E. Box wrote:
 This patch implements an I2C bus sharing mechanism between the host and 
 platform
 hardware on select Intel BayTrail SoC platforms using the X-Powers AXP288 
 PMIC.
 
 On these platforms access to the PMIC must be shared with platform hardware. 
 The
 hardware unit assumes full control of the I2C bus and the host must request
 access through a special semaphore. Hardware control of the bus also makes it
 necessary to disable runtime pm to avoid interfering with hardware 
 transactions.
 
 Signed-off-by: David E. Box david.e@linux.intel.com

Reviewed-by: Mika Westerberg mika.westerb...@linux.intel.com

One comment, though:

 ---
  drivers/i2c/busses/Kconfig   |  11 ++
  drivers/i2c/busses/Makefile  |   1 +
  drivers/i2c/busses/i2c-designware-baytrail.c | 160 
 +++
  drivers/i2c/busses/i2c-designware-core.h |   6 +
  drivers/i2c/busses/i2c-designware-platdrv.c  |  20 +++-
  5 files changed, 193 insertions(+), 5 deletions(-)
  create mode 100644 drivers/i2c/busses/i2c-designware-baytrail.c
 
 diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
 index 917c358..9a83c46 100644
 --- a/drivers/i2c/busses/Kconfig
 +++ b/drivers/i2c/busses/Kconfig
 @@ -464,6 +464,17 @@ config I2C_DESIGNWARE_PCI
 This driver can also be built as a module.  If so, the module
 will be called i2c-designware-pci.
  
 +config I2C_DESIGNWARE_BAYTRAIL
 + bool Intel Baytrail I2C semaphore support

It would be nice if it was possible to compile this as a module.

 + depends on I2C_DESIGNWARE_PLATFORM
 + select IOSF_MBI
 + help
 +   This driver enables managed host access to the PMIC I2C bus on select
 +   Intel BayTrail platforms using the X-Powers AXP288 PMIC. It allows
 +   the host to request uninterrupted access to the PMIC's I2C bus from
 +   the platform firmware controlling it. You should say Y if running on
 +   a BayTrail system using the AXP288.
 +
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V4 1/2] i2c-designware: Add i2c bus locking support

[Mika Westerberg]

On Thu, Jan 15, 2015 at 01:12:16AM -0800, David E. Box wrote:
 Adds support for acquiring and releasing a hardware bus lock in the i2c
 designware core transfer function. This is needed for i2c bus controllers
 that are shared with but not controlled by the kernel.
 
 Signed-off-by: David E. Box david.e@linux.intel.com

Looks good.

Reviewed-by: Mika Westerberg mika.westerb...@linux.intel.com
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: designware: use {readl|writel}_relaxed instead of readl/writel

[Mika Westerberg]

On Tue, Jan 13, 2015 at 12:52:05PM +0100, Wolfram Sang wrote:
 On Fri, Dec 19, 2014 at 10:43:15AM +0800, Jisheng Zhang wrote:
  Dear all,
  
  Is there any issue I need to resolve so that the patch can be merged?
 
 Adding Mika to the loop. He uses the driver a lot (and knows other
 people who do)...

I don't see problems with this patch.

On x86 we have readl_relaxed() the same as readl() so this patch does
not change anything there.

Feel free to add,

Acked-by: Mika Westerberg mika.westerb...@linux.intel.com
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c / ACPI: Pick the first address if device has multiple

[Mika Westerberg]

On Tue, Jan 13, 2015 at 05:48:29PM +0100, Wolfram Sang wrote:
 On Tue, Jan 13, 2015 at 08:44:37AM -0800, Srinivas Pandruvada wrote:
  On Tue, 2015-01-13 at 16:50 +0100, Wolfram Sang wrote: 
   On Mon, Dec 29, 2014 at 03:48:48PM +0200, Mika Westerberg wrote:
ACPI specification allows I2C devices with multiple addresses. The 
current
implementation goes over all addresses and assigns the last one to the
device. This is typically not the primary address of the device.

Instead of doing that we assign the first address to the device and then
let the driver handle rest of the addresses as it wishes.

Signed-off-by: Mika Westerberg mika.westerb...@linux.intel.com
Cc: Srinivas Pandruvada srinivas.pandruv...@linux.intel.com
   
   Yes, seems better than what we do know. But maybe taking the lowest
   address is a bit better heuristic than taking the first address?
   Not sure, though...
  The problem in taking lowest is that in many cases in current devices,
  the lowest address may end being 0x0C, which is reserved address for
  SMBUS (ARA). This will require different handling. Unfortunately ACPI
  doesn't have a way to distinguish whether SMBUS support is desired or
  not. 
  The other option is to skip all reserved addresses for SMBUS also and
  then create on the lowest.
 
 Well, this makes me think that Mika's approach is probably the sanest
 one...

Also I think it is more consistent that way.
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] i2c / ACPI: Pick the first address if device has multiple

[Mika Westerberg]

ACPI specification allows I2C devices with multiple addresses. The current
implementation goes over all addresses and assigns the last one to the
device. This is typically not the primary address of the device.

Instead of doing that we assign the first address to the device and then
let the driver handle rest of the addresses as it wishes.

Signed-off-by: Mika Westerberg mika.westerb...@linux.intel.com
Cc: Srinivas Pandruvada srinivas.pandruv...@linux.intel.com
---
 drivers/i2c/i2c-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 39d25a8cb1ad..a06be43b7842 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -102,7 +102,7 @@ static int acpi_i2c_add_resource(struct acpi_resource 
*ares, void *data)
struct acpi_resource_i2c_serialbus *sb;
 
sb = ares-data.i2c_serial_bus;
-   if (sb-type == ACPI_RESOURCE_SERIAL_TYPE_I2C) {
+   if (!info-addr  sb-type == ACPI_RESOURCE_SERIAL_TYPE_I2C) {
info-addr = sb-slave_address;
if (sb-access_mode == ACPI_I2C_10BIT_MODE)
info-flags |= I2C_CLIENT_TEN;
-- 
2.1.4

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 1/2] i2c-designware: Add i2c bus locking support

[Mika Westerberg]

On Mon, Dec 01, 2014 at 04:09:32PM -0800, David E. Box wrote:
 Adds support for acquiring and releasing a hardware bus lock in the i2c
 designware core transfer function. This is needed for i2c bus controllers
 that are shared with but not controlled by the kernel.
 
 Signed-off-by: David E. Box david.e@linux.intel.com
 ---
  drivers/i2c/busses/i2c-designware-core.c| 11 +++
  drivers/i2c/busses/i2c-designware-core.h|  6 ++
  drivers/i2c/busses/i2c-designware-platdrv.c | 18 +-
  3 files changed, 30 insertions(+), 5 deletions(-)
 
 diff --git a/drivers/i2c/busses/i2c-designware-core.c 
 b/drivers/i2c/busses/i2c-designware-core.c
 index 3c20e4b..377deeb 100644
 --- a/drivers/i2c/busses/i2c-designware-core.c
 +++ b/drivers/i2c/busses/i2c-designware-core.c
 @@ -631,6 +631,14 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg 
 msgs[], int num)
   dev-abort_source = 0;
   dev-rx_outstanding = 0;
  
 + if (dev-acquire_lock) {
 + ret = dev-acquire_lock(dev);
 + if (ret) {
 + dev_err(dev-dev, couldn't acquire bus ownership\n);
 + goto done;

I wonder what happens now since you failed to acquire the lock...

 + }
 + }
 +
   ret = i2c_dw_wait_bus_not_busy(dev);
   if (ret  0)
   goto done;
 @@ -676,6 +684,9 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg 
 msgs[], int num)
   ret = -EIO;
  
  done:
 + if (dev-release_lock)
 + dev-release_lock(dev);

... but here you unconditionally release it?

Otherwise the patch looks good to me.

 +
   pm_runtime_mark_last_busy(dev-dev);
   pm_runtime_put_autosuspend(dev-dev);
   mutex_unlock(dev-lock);
 diff --git a/drivers/i2c/busses/i2c-designware-core.h 
 b/drivers/i2c/busses/i2c-designware-core.h
 index d66b6cb..a472c91 100644
 --- a/drivers/i2c/busses/i2c-designware-core.h
 +++ b/drivers/i2c/busses/i2c-designware-core.h
 @@ -65,6 +65,9 @@
   * @ss_lcnt: standard speed LCNT value
   * @fs_hcnt: fast speed HCNT value
   * @fs_lcnt: fast speed LCNT value
 + * @acquire_lock: function to acquire a hardware lock on the bus
 + * @release_lock: function to release a hardware lock on the bus
 + * @pm_runtime_disabled: true if pm runtime is disabled
   *
   * HCNT and LCNT parameters can be used if the platform knows more accurate
   * values than the one computed based only on the input clock frequency.
 @@ -105,6 +108,9 @@ struct dw_i2c_dev {
   u16 ss_lcnt;
   u16 fs_hcnt;
   u16 fs_lcnt;
 + int (*acquire_lock)(struct dw_i2c_dev *dev);
 + void(*release_lock)(struct dw_i2c_dev *dev);
 + boolpm_runtime_disabled;
  };
  
  #define ACCESS_SWAP  0x0001
 diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c 
 b/drivers/i2c/busses/i2c-designware-platdrv.c
 index a743115..afdff3b 100644
 --- a/drivers/i2c/busses/i2c-designware-platdrv.c
 +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
 @@ -261,10 +261,16 @@ static int dw_i2c_probe(struct platform_device *pdev)
   return r;
   }
  
 - pm_runtime_set_autosuspend_delay(pdev-dev, 1000);
 - pm_runtime_use_autosuspend(pdev-dev);
 - pm_runtime_set_active(pdev-dev);
 - pm_runtime_enable(pdev-dev);
 + i2c_dw_eval_lock_support(dev);
 +
 + if (dev-pm_runtime_disabled) {
 + pm_runtime_forbid(pdev-dev);
 + } else {
 + pm_runtime_set_autosuspend_delay(pdev-dev, 1000);
 + pm_runtime_use_autosuspend(pdev-dev);
 + pm_runtime_set_active(pdev-dev);
 + pm_runtime_enable(pdev-dev);
 + }
  
   return 0;
  }
 @@ -314,7 +320,9 @@ static int dw_i2c_resume(struct device *dev)
   struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
  
   clk_prepare_enable(i_dev-clk);
 - i2c_dw_init(i_dev);
 +
 + if (!i_dev-pm_runtime_disabled)
 + i2c_dw_init(i_dev);
  
   return 0;
  }
 -- 
 1.9.1
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 2/2] i2c-designware: Add Intel Baytrail PMIC I2C bus support

[Mika Westerberg]

On Mon, Dec 01, 2014 at 04:09:33PM -0800, David E. Box wrote:
 This patch implements an I2C bus sharing mechanism between the host and 
 platform
 hardware on select Intel BayTrail SoC platforms using the X-Powers AXP288 
 PMIC.
 
 On these platforms access to the PMIC must be shared with platform hardware. 
 The
 hardware unit assumes full control of the I2C bus and the host must request
 access through a special semaphore. Hardware control of the bus also makes it
 necessary to disable runtime pm to avoid interfering with hardware 
 transactions.
 
 Signed-off-by: David E. Box david.e@linux.intel.com
 ---
  drivers/i2c/busses/Kconfig   |  12 +++
  drivers/i2c/busses/Makefile  |   1 +
  drivers/i2c/busses/i2c-designware-baytrail.c | 155 
 +++
  drivers/i2c/busses/i2c-designware-core.h |   6 ++
  4 files changed, 174 insertions(+)
  create mode 100644 drivers/i2c/busses/i2c-designware-baytrail.c
 
 diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
 index 917c358..d2bfd88 100644
 --- a/drivers/i2c/busses/Kconfig
 +++ b/drivers/i2c/busses/Kconfig
 @@ -464,6 +464,18 @@ config I2C_DESIGNWARE_PCI
 This driver can also be built as a module.  If so, the module
 will be called i2c-designware-pci.
  
 +config I2C_DESIGNWARE_BAYTRAIL
 + bool Intel Baytrail I2C semaphore support
 + depends on I2C_DESIGNWARE_PLATFORM=y

Hmm, is there something preventing to compile this as module?

What comes to the driver itself, no objections from me.

Reviewed-by: Mika Westerberg mika.westerb...@linux.intel.com
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Freeze of mouse and keyboard after suspend due to i2c-designware

[Mika Westerberg]

On Thu, Nov 27, 2014 at 05:31:31PM +0100, Prof. H.-M. Hoogewoud wrote:
 Hello,
 
 The crash result during resume and happen while suspending to disk and mem.
 During resume I can see the screen and the login dialog, but keyboard, mous,
 touchscreen and touchpad are frozen. Caps lock does not work!

OK.

Can you run 'dmesg  /to/some/file  sync' from your resume script,
then reboot the machine and send me the resulting file?
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2] ACPI: Add _DEP(Operation Region Dependencies) support to fix battery issue on the Asus T100TA

[Mika Westerberg]

On Thu, Nov 20, 2014 at 10:12:12PM +0800, Lan Tianyu wrote:
 ACPI 5.0 introduces _DEP to designate device objects that OSPM should
 assign a higher priority in start ordering due to future operation region
 accesses.
 
 On Asus T100TA, ACPI battery info are read from a I2C slave device via
 I2C operation region. Before I2C operation region handler is installed,
 battery _STA always returns 0. There is a _DEP method of designating
 start order under battery device node.
 
 This patch is to implement _DEP feature to fix battery issue on the Asus 
 T100TA.
 Introducing acpi_dep_list and adding dep_unmet count in the struct
 acpi_device. During ACPI namespace scan, create struct acpi_dep_data for a
 valid pair of master (device pointed to by _DEP)/slave(device with _DEP), 
 record
 master's and slave's ACPI handle in it and put it into acpi_dep_list. The 
 dep_unmet
 count will increase by one if there is a device under its _DEP. Driver's 
 probe() should
 return EPROBE_DEFER when find dep_unmet is larger than 0. When I2C operation
 region handler is installed, remove all struct acpi_dep_data on the 
 acpi_dep_list
 whose master is pointed to I2C host controller and decrease slave's dep_unmet.
 When dep_unmet decreases to 0, all _DEP conditions are met and then do 
 acpi_bus_attach()
 for the device in order to resolve battery _STA issue on the Asus T100TA.
 
 Reference: https://bugzilla.kernel.org/show_bug.cgi?id=69011
 Tested-by: Jan-Michael Brummer jan.brum...@tabos.org
 Tested-by: Adam Williamson ad...@happyassassin.net
 Tested-by: Michael Shigorin shigo...@gmail.com
 Acked-by: Wolfram Sang w...@the-dreams.de
 Signed-off-by: Lan Tianyu tianyu@intel.com

Looks good to me now,

Acked-by: Mika Westerberg mika.westerb...@linux.intel.com
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: designware: prevent early stop on TX FIFO empty

[Mika Westerberg]

On Wed, Nov 19, 2014 at 10:21:22AM +0100, Wolfram Sang wrote:
 On Fri, Nov 07, 2014 at 12:10:44PM +, Andrew Jackson wrote:
  If the Designware core is configured with IC_EMPTYFIFO_HOLD_MASTER_EN
  set to zero, allowing the TX FIFO to become empty causes a STOP
  condition to be generated on the I2C bus. If the transmit FIFO
  threshold is set too high, an erroneous STOP condition can be
  generated on long transfers - particularly where the interrupt
  latency is extended.

Makes sense to give some slack so that the interrupt handler is still
able to fill the FIFO.

  
  Signed-off-by: Andrew Jackson andrew.jack...@arm.com
  Signed-off-by: Liviu Dudau liviu.du...@arm.com
 
 So, what do other designware users think of this change (nice CC list
 BTW, Andrew). Adding Mika, too.

I quickly tested this on Haswell machine with touch screen connected to
the I2C bus and it still works fine, so

Tested-by: Mika Westerberg mika.westerb...@linux.intel.com
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ACPI: Add _DEP(Operation Region Dependencies) support to fix battery issue on the Asus T100TA

[Mika Westerberg]

On Wed, Nov 19, 2014 at 11:20:41PM +0800, Lan Tianyu wrote:
 ACPI 5.0 introduces _DEP to designate device objects that OSPM should
 assign a higher priority in start ordering due to future operation region
 accesses.
 
 On Asus T100TA, ACPI battery info are read from a I2C slave device via
 I2C operation region. Before I2C operation region handler is installed,
 battery _STA always returns 0. There is a _DEP method of designating
 start order under battery device node.
 
 This patch is to implement _DEP feature to fix battery issue on the Asus 
 T100TA.
 Introducing acpi_dep_list and adding dep_unmet count in the struct
 acpi_device. During ACPI namespace scan, create struct acpi_dep_data for a
 valid pair of master (device pointed to by _DEP)/slave(device with _DEP), 
 record
 master's and slave's ACPI handle in it and put it into acpi_dep_list. The 
 dep_unmet
 count will increase by one if there is a device under its _DEP. Driver's 
 probe() should
 return EPROBE_DEFER when find dep_unmet larger than 0. When I2C operation
 region handler is installed, remove all struct acpi_dep_data on the 
 acpi_dep_list
 whose master is pointed to I2C host controller and decrease slave's dep_unmet.
 When dep_unmet decreases to 0, all _DEP conditions are met and then do 
 acpi_bus_attach()
 for the device in order to resolve battery _STA issue on the Asus T100TA.
 
 Reference: https://bugzilla.kernel.org/show_bug.cgi?id=69011 
 Tested-by: Jan-Michael Brummer jan.brum...@tabos.org
 Tested-by: Adam Williamson ad...@happyassassin.net
 Tested-by: Michael Shigorin shigo...@gmail.com
 Signed-off-by: Lan Tianyu tianyu@intel.com
 ---
  drivers/acpi/battery.c  |  4 +++
  drivers/acpi/scan.c | 89 
 +
  drivers/i2c/i2c-core.c  |  1 +
  include/acpi/acpi_bus.h |  1 +
  include/linux/acpi.h|  3 ++
  5 files changed, 98 insertions(+)
 
 diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
 index 8ec8a89..d98ba43 100644
 --- a/drivers/acpi/battery.c
 +++ b/drivers/acpi/battery.c
 @@ -1180,6 +1180,10 @@ static int acpi_battery_add(struct acpi_device *device)
  
   if (!device)
   return -EINVAL;
 +
 + if (device-dep_unmet)
 + return -EPROBE_DEFER;
 +
   battery = kzalloc(sizeof(struct acpi_battery), GFP_KERNEL);
   if (!battery)
   return -ENOMEM;
 diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
 index 9cb5cca..0e58666 100644
 --- a/drivers/acpi/scan.c
 +++ b/drivers/acpi/scan.c
 @@ -36,6 +36,8 @@ bool acpi_force_hot_remove;
  
  static const char *dummy_hid = device;
  
 +static LIST_HEAD(acpi_dep_list);
 +static DEFINE_MUTEX(acpi_dep_list_lock);
  static LIST_HEAD(acpi_bus_id_list);
  static DEFINE_MUTEX(acpi_scan_lock);
  static LIST_HEAD(acpi_scan_handlers_list);
 @@ -43,6 +45,12 @@ DEFINE_MUTEX(acpi_device_lock);
  LIST_HEAD(acpi_wakeup_device_list);
  static DEFINE_MUTEX(acpi_hp_context_lock);
  
 +struct acpi_dep_data {
 + struct list_head node;
 + acpi_handle master;
 + acpi_handle slave;
 +};
 +
  struct acpi_device_bus_id{
   char bus_id[15];
   unsigned int instance_no;
 @@ -2193,6 +2201,61 @@ static void acpi_scan_init_hotplug(struct acpi_device 
 *adev)
   }
  }
  
 +static void acpi_device_dep_initialize(struct acpi_device *adev)
 +{
 + struct acpi_dep_data *dep;
 + struct acpi_handle_list dep_devices;
 + struct acpi_device_info *info;
 + acpi_status status;
 + int i, skip;
 +
 + if (!acpi_has_method(adev-handle, _DEP))
 + return;
 +
 + status = acpi_evaluate_reference(adev-handle, _DEP, NULL,
 + dep_devices);
 + if (ACPI_FAILURE(status)) {
 + dev_err(adev-dev, Failed to evaluate _DEP.\n);
 + return;
 + }
 +
 + for (i = 0; i  dep_devices.count; i++) {
 +

Delete the black line.

 + status = acpi_get_object_info(dep_devices.handles[i], info);
 + if (ACPI_FAILURE(status)) {
 + dev_err(adev-dev, Error reading device info\n);
 + continue;
 + }
 +
 + /*
 +  * Skip the dependency of Windows System Power
 +  * Management Controller
 +  */
 + if (info-valid  ACPI_VALID_HID
 +  !strcmp(info-hardware_id.string, INT3396))
 + skip = 1;
 + else
 + skip = 0;
 +
 + kfree(info);
 +
 + if (skip)
 + continue;
 +
 + dep = kzalloc(sizeof(struct acpi_dep_data), GFP_KERNEL);
 + if (!dep)
 + return;
 +
 + dep-master = dep_devices.handles[i];
 + dep-slave  = adev-handle;
 + adev-dep_unmet++;
 +
 + mutex_lock(acpi_dep_list_lock);
 + list_add_tail(dep-node , acpi_dep_list);
 + mutex_unlock(acpi_dep_list_lock);
 + }
 +}
 +
  

Re: [PATCH V2] i2c-designware: Add Intel Baytrail PMIC I2C bus support

[Mika Westerberg]

On Tue, Sep 23, 2014 at 11:40:26AM -0700, David E. Box wrote:
 +void baytrail_evaluate_sem(struct dw_i2c_dev *dev)
 +{
 + acpi_status status;
 + unsigned long long shared_host = 0;
 + acpi_handle handle;
 +
 + if (!dev || !dev-dev) {
 + pr_err(%s:%d: device is NULL\n, __func__, __LINE__);

Not sure if it is useful to print things like above.

 + return;
 + }
 +
 + handle = ACPI_HANDLE(dev-dev);
 + if (!handle)
 + return;
 +
 + status = acpi_evaluate_integer(handle, _SEM, NULL, shared_host);

Maybe it is better to check first if the operation succeeded before
touching shared_host?

if (ACPI_SUCCESS(status)  shared_host) {
}

Otherwise ACPI parts look good to me.

 +
 + if (shared_host) {
 + dev_info(dev-dev, I2C bus managed by PUNIT\n);
 + dev-has_hw_lock = true;
 + dev-pm_runtime_disabled = true;
 + }
 +}
 +EXPORT_SYMBOL(baytrail_evaluate_sem);
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH V2] ACPI: Add _DEP(Operation Region Dependencies) support to fix battery issue on the Asus T100TA

[Mika Westerberg]

On Fri, Nov 07, 2014 at 12:00:34AM +0100, Rafael J. Wysocki wrote:
  So if we have a device, like SDHB above (SDIO controller), that needs
  GPIOs for powering the SDIO card and that is done by using GPIO
  operation regions how do we ensure that the GPIO controller driver is
  there?
  
  I think the answer is that we just make sure the the GPIO driver is
  there before anything that is going to use it. E.g make it
  subsys_initcall() or so?
 
 The current approach, which arguably is lame, has been to (1) compile in
 all drivers that *may* be depended on (like all drviers registering operation
 region handlers) and then (2) trick the devices to be registered in the right
 order.

Right, I kind of expected that it be the case :-/
 
 I'd rather have a more robust mechanism than that, but so far no one has
 proposed anything remotely usable.
 
 As far as _DEP is concerned, it seems that in *practice* it is used to
 reflect functional dependencies between devices rather than the operation
 regions thing only (as specified).  In that case we may decide to follow
 the practice rather than the spec (and move to update the spec to reflect
 the practice at the same time), but that requires some consideration.

Indeed it looks very much that it contains more than just operation
depedencies. In T100 for example there is this PEPD device that does not
have any operation regions and it is still in the _DEP list of the SDIO
controller device.
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html