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

2015-12-23 Thread Ken Xue 
On Tue, 2015-12-15 at 16:59 +0200, Mika Westerberg wrote:
> 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,
>   }

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.

It sounds like odd that multi-speed can be applied over one I2C bus, But
everything goes well.
Does this behavior(multi-speed) violate the I2C spec?

2) In theory, I2C core driver also can get ACPI resource
"ConnectionSpeed" of device during creating new I2C device. 
Is it worth to add a new feature for parsing and using "ConnectionSpeed"
of device for determining bus speed?

3) For ACPI DW adapter, current driver only support Fast mode. I would
like to support other modes after previous 2 questions are clear.

--
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

2015-12-23 Thread Ken Xue 
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? 

--
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/2] acpi:apd: Add APM X-Gene ACPI I2C device support

2015-12-10 Thread Ken Xue 
On Thu, 2015-12-10 at 14:19 -0700, Loc Ho wrote:
> Add APM X-Gene ACPI I2C device support by hooks into existent
> ACPI apd driver. To fully enable support, require another
> patch to add the X-Gene ACPI node into the DW I2C driver.
> 
> Signed-off-by: Loc Ho <l...@apm.com>
> ---
>  drivers/acpi/acpi_apd.c |   16 +++-
>  1 files changed, 15 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c
> index a450e7a..d507cf6 100644
> --- a/drivers/acpi/acpi_apd.c
> +++ b/drivers/acpi/acpi_apd.c
> @@ -51,7 +51,7 @@ struct apd_private_data {
>   const struct apd_device_desc *dev_desc;
>  };
>  
> -#ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
> +#if defined(CONFIG_X86_AMD_PLATFORM_DEVICE) || defined(CONFIG_ARM64)
>  #define APD_ADDR(desc)   ((unsigned long))
>  
>  static int acpi_apd_setup(struct apd_private_data *pdata)
> @@ -71,6 +71,7 @@ static int acpi_apd_setup(struct apd_private_data *pdata)
>   return 0;
>  }
>  
> +#ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
>  static struct apd_device_desc cz_i2c_desc = {
>   .setup = acpi_apd_setup,
>   .fixed_clk_rate = 13300,
> @@ -80,6 +81,14 @@ static struct apd_device_desc cz_uart_desc = {
>   .setup = acpi_apd_setup,
>   .fixed_clk_rate = 4800,
>  };
> +#endif
> +
> +#ifdef CONFIG_ARM64
> +static struct apd_device_desc xgene_i2c_desc = {
> + .setup = acpi_apd_setup,
> + .fixed_clk_rate = 1,
> +};
> +#endif
>  
>  #else
>  
> @@ -132,9 +141,14 @@ static int acpi_apd_create_device(struct acpi_device 
> *adev,
>  
>  static const struct acpi_device_id acpi_apd_device_ids[] = {
>   /* Generic apd devices */
> +#ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
>   { "AMD0010", APD_ADDR(cz_i2c_desc) },
>   { "AMD0020", APD_ADDR(cz_uart_desc) },
>   { "AMD0030", },
> +#endif
> +#ifdef CONFIG_ARM64
> + { "APMC0D0F", APD_ADDR(xgene_i2c_desc) },
> +#endif
>   { }
>  };
>  

Reviewed-by: Ken Xue <ken@amd.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/2] acpi:apd: Add APM X-Gene ACPI I2C device support

2015-12-09 Thread Ken Xue 
On Wed, 2015-12-09 at 14:03 -0800, Loc Ho wrote:
> Hi Ken,
> 
> On Mon, Dec 7, 2015 at 6:49 PM, Ken Xue <ken@amd.com> wrote:
> >
> > On Mon, 2015-12-07 at 17:16 -0700, Loc Ho wrote:
> > > Add APM X-Gene ACPI I2C device support by hooks into existent
> > > ACPI apd driver. To fully enable support, require another
> > > patch to add the X-Gene ACPI node into the DW I2C driver.
> > >
> > > Signed-off-by: Loc Ho <l...@apm.com>
> > > ---
> > >  drivers/acpi/acpi_apd.c |8 +++-
> > >  1 files changed, 7 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c
> > > index a450e7a..6a9cb8d 100644
> > > --- a/drivers/acpi/acpi_apd.c
> > > +++ b/drivers/acpi/acpi_apd.c
> > > @@ -51,7 +51,7 @@ struct apd_private_data {
> > >   const struct apd_device_desc *dev_desc;
> > >  };
> > >
> > > -#ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
> > > +#if defined(CONFIG_X86_AMD_PLATFORM_DEVICE) || defined(CONFIG_ARM64)
> > >  #define APD_ADDR(desc)   ((unsigned long))
> > >
> > >  static int acpi_apd_setup(struct apd_private_data *pdata)
> > > @@ -76,6 +76,11 @@ static struct apd_device_desc cz_i2c_desc = {
> > >   .fixed_clk_rate = 13300,
> > >  };
> > >
> > > +static struct apd_device_desc xgene_i2c_desc = {
> > > + .setup = acpi_apd_setup,
> > > + .fixed_clk_rate = 1,
> > > +};
> > > +
> > >  static struct apd_device_desc cz_uart_desc = {
> > >   .setup = acpi_apd_setup,
> > >   .fixed_clk_rate = 4800,
> > > @@ -135,6 +140,7 @@ static const struct acpi_device_id 
> > > acpi_apd_device_ids[] = {
> > >   { "AMD0010", APD_ADDR(cz_i2c_desc) },
> > >   { "AMD0020", APD_ADDR(cz_uart_desc) },
> > >   { "AMD0030", },
> > > + { "APMC0D0F", APD_ADDR(xgene_i2c_desc) },
> > It is better to split AMD devices and ARM devices with macros:
> > CONFIG_X86_AMD_PLATFORM_DEVICE and CONFIG_ARM64.
> 
> This gets pretty ugly to me with define as it would look like this:
> 
> #if defined(CONFIG_X86_AMD_PLATFORM_DEVICE) || defined(CONFIG_ARM64)
> #define APD_ADDR(desc) ((unsigned long))
> 
> static int acpi_apd_setup(struct apd_private_data *pdata)
> {
>.
> }
> 
> #ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
> static struct apd_device_desc cz_i2c_desc = {
> 
> };
> 
> static struct apd_device_desc xgene_i2c_desc = {
> ...
> };
> #endif
> 
> #ifdef CONFIG_ARM64
> static struct apd_device_desc cz_uart_desc = {
> 
> };
> #endif
> 
> #else
> 
> #define APD_ADDR(desc) (0UL)
> 
> #endif /* CONFIG_X86_AMD_PLATFORM_DEVICE */
> 
> And...
> 
> #ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
> { "AMD0010", APD_ADDR(cz_i2c_desc) },
> { "AMD0020", APD_ADDR(cz_uart_desc) },
> { "AMD0030", },
> #endif
> #ifdef CONFIG_ARM64
> { "APMC0D0F", APD_ADDR(xgene_i2c_desc) },
> #endif
> 
> Sure you want this?
Yes. Even though it may look like too much macros for just several
devices now. But I think AMD and other ARM socs may also try to leverage
APD for more and more ACPI devices.
It is a good direction that 1)improve efficiency of matching ACPI
handler 2) split devices and potential hook routines into different
classes clearly

It also will be more convenient to move ARM devices out of APD if there
is a new design for ARM ACPI 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

Re: [PATCH 1/2] acpi:apd: Add APM X-Gene ACPI I2C device support

2015-12-09 Thread Ken Xue 
On Wed, 2015-12-09 at 22:57 -0800, Loc Ho wrote:
> >> Sure you want this?
> > Yes. Even though it may look like too much macros for just several
> > devices now. But I think AMD and other ARM socs may also try to leverage
> > APD for more and more ACPI devices.
> > It is a good direction that 1)improve efficiency of matching ACPI
> > handler 2) split devices and potential hook routines into different
> > classes clearly
> >
> > It also will be more convenient to move ARM devices out of APD if there
> > is a new design for ARM ACPI device.
> >
> 
> Okay... I will generate v2 when ready. One more question, does AMD
> ARM64 SoC need it later?
> 

Thanks. I am not sure about driver design for AMD ARM64 SoC.


--
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] acpi:apd: Add APM X-Gene ACPI I2C device support

2015-12-07 Thread Ken Xue 
On Mon, 2015-12-07 at 17:16 -0700, Loc Ho wrote:
> Add APM X-Gene ACPI I2C device support by hooks into existent
> ACPI apd driver. To fully enable support, require another
> patch to add the X-Gene ACPI node into the DW I2C driver.
> 
> Signed-off-by: Loc Ho 
> ---
>  drivers/acpi/acpi_apd.c |8 +++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c
> index a450e7a..6a9cb8d 100644
> --- a/drivers/acpi/acpi_apd.c
> +++ b/drivers/acpi/acpi_apd.c
> @@ -51,7 +51,7 @@ struct apd_private_data {
>   const struct apd_device_desc *dev_desc;
>  };
>  
> -#ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
> +#if defined(CONFIG_X86_AMD_PLATFORM_DEVICE) || defined(CONFIG_ARM64)
>  #define APD_ADDR(desc)   ((unsigned long))
>  
>  static int acpi_apd_setup(struct apd_private_data *pdata)
> @@ -76,6 +76,11 @@ static struct apd_device_desc cz_i2c_desc = {
>   .fixed_clk_rate = 13300,
>  };
>  
> +static struct apd_device_desc xgene_i2c_desc = {
> + .setup = acpi_apd_setup,
> + .fixed_clk_rate = 1,
> +};
> +
>  static struct apd_device_desc cz_uart_desc = {
>   .setup = acpi_apd_setup,
>   .fixed_clk_rate = 4800,
> @@ -135,6 +140,7 @@ static const struct acpi_device_id acpi_apd_device_ids[] 
> = {
>   { "AMD0010", APD_ADDR(cz_i2c_desc) },
>   { "AMD0020", APD_ADDR(cz_uart_desc) },
>   { "AMD0030", },
> + { "APMC0D0F", APD_ADDR(xgene_i2c_desc) },
It is better to split AMD devices and ARM devices with macros:
CONFIG_X86_AMD_PLATFORM_DEVICE and CONFIG_ARM64.


>   { }
>  };
>  


--
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

2015-10-22 Thread Ken Xue 
On Wed, 2015-10-21 at 13:46 +0300, Mika Westerberg wrote:
> 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...

You are right :-). The new patch for reverting a445900c906092 is
submitted.
Please help to review. 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

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

2015-10-22 Thread Ken Xue 
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>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 27 +--
 1 file changed, 1 insertion(+), 26 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c 
b/drivers/i2c/busses/i2c-designware-platdrv.c
index 472b882..4bf5fc1 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -97,7 +97,6 @@ static void dw_i2c_acpi_params(struct platform_device *pdev, 
char method[],
 static int dw_i2c_acpi_configure(struct platform_device *pdev)
 {
struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
-   const struct acpi_device_id *id;
 
dev->adapter.nr = -1;
dev->tx_fifo_depth = 32;
@@ -111,29 +110,9 @@ static int dw_i2c_acpi_configure(struct platform_device 
*pdev)
dw_i2c_acpi_params(pdev, "FMCN", >fs_hcnt, >fs_lcnt,
   >sda_hold_time);
 
-   /*
-* Provide a way for Designware I2C host controllers that are not
-* based on Intel LPSS to specify their input clock frequency via
-* id->driver_data.
-*/
-   id = acpi_match_device(pdev->dev.driver->acpi_match_table, >dev);
-   if (id && id->driver_data)
-   clk_register_fixed_rate(>dev, dev_name(>dev), NULL,
-   CLK_IS_ROOT, id->driver_data);
-
return 0;
 }
 
-static void dw_i2c_acpi_unconfigure(struct platform_device *pdev)
-{
-   struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
-   const struct acpi_device_id *id;
-
-   id = acpi_match_device(pdev->dev.driver->acpi_match_table, >dev);
-   if (id && id->driver_data)
-   clk_unregister(dev->clk);
-}
-
 static const struct acpi_device_id dw_i2c_acpi_match[] = {
{ "INT33C2", 0 },
{ "INT33C3", 0 },
@@ -141,7 +120,7 @@ static const struct acpi_device_id dw_i2c_acpi_match[] = {
{ "INT3433", 0 },
{ "80860F41", 0 },
{ "808622C1", 0 },
-   { "AMD0010", 133 * 1000 * 1000 },
+   { "AMD0010", 0 },
{ }
 };
 MODULE_DEVICE_TABLE(acpi, dw_i2c_acpi_match);
@@ -150,7 +129,6 @@ static inline int dw_i2c_acpi_configure(struct 
platform_device *pdev)
 {
return -ENODEV;
 }
-static inline void dw_i2c_acpi_unconfigure(struct platform_device *pdev) { }
 #endif
 
 static int dw_i2c_probe(struct platform_device *pdev)
@@ -306,9 +284,6 @@ static int dw_i2c_remove(struct platform_device *pdev)
pm_runtime_put_sync(>dev);
pm_runtime_disable(>dev);
 
-   if (has_acpi_companion(>dev))
-   dw_i2c_acpi_unconfigure(pdev);
-
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 1/2] i2c: designware: register clkdev during acpi device configuration

2015-10-21 Thread Ken Xue 
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.

> 
> 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.


--
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

2015-10-21 Thread Ken Xue 
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.

--
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

2015-10-21 Thread Ken Xue 
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.

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

--
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 1/2] i2c: designware: register clkdev during acpi device configuration

2015-10-20 Thread Ken Xue 
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.

Signed-off-by: Ken Xue <ken@amd.com>
Signed-off-by: Xiangliang Yu <xiangliang...@amd.com>
Cc: sta...@vger.kernel.org
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c 
b/drivers/i2c/busses/i2c-designware-platdrv.c
index 3dd2de3..9ee1fc6 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -27,6 +27,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -78,6 +79,7 @@ static int dw_i2c_acpi_configure(struct platform_device *pdev)
 {
struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
const struct acpi_device_id *id;
+   struct clk *clk;
 
dev->adapter.nr = -1;
dev->tx_fifo_depth = 32;
@@ -97,9 +99,11 @@ static int dw_i2c_acpi_configure(struct platform_device 
*pdev)
 * id->driver_data.
 */
id = acpi_match_device(pdev->dev.driver->acpi_match_table, >dev);
-   if (id && id->driver_data)
-   clk_register_fixed_rate(>dev, dev_name(>dev), NULL,
-   CLK_IS_ROOT, id->driver_data);
+   if (id && id->driver_data) {
+   clk = clk_register_fixed_rate(>dev, dev_name(>dev),
+   NULL, CLK_IS_ROOT, id->driver_data);
+   clk_register_clkdev(clk, NULL, dev_name(>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

[PATCH 2/2] i2c: designware: remove freq definition for "AMD0010" in acpi_device_id

2015-10-20 Thread Ken Xue 
Now, AMD ACPI devices can be reworked by APD which is similar to intel
lpss. And the clkdev for "AMD0010" can be registered in APD. So, try
to remove freq definition from id->driver_data for "AMD0010".

Make "AMD0010" look like other ones.

Signed-off-by: Ken Xue <ken@amd.com>
Signed-off-by: Xiangliang Yu <xiangliang...@amd.com>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c 
b/drivers/i2c/busses/i2c-designware-platdrv.c
index 9ee1fc6..c8acbff 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -125,7 +125,7 @@ static const struct acpi_device_id dw_i2c_acpi_match[] = {
{ "INT3433", 0 },
{ "80860F41", 0 },
{ "808622C1", 0 },
-   { "AMD0010", 133 * 1000 * 1000 },
+   { "AMD0010", 0 },
{ }
 };
 MODULE_DEVICE_TABLE(acpi, dw_i2c_acpi_match);
-- 
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 1/2] i2c: designware: register clkdev during acpi device configuration

2015-10-20 Thread Ken Xue 
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.

--
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