Re: [PATCH v2 net-next 1/3] ptp: ptp_clockmatrix: Add wait_for_sys_apll_dpll_lock.

2021-02-16 Thread Vincent Cheng
On Mon, Feb 15, 2021 at 02:48:22PM EST, Jakub Kicinski wrote:
>On Sat, 13 Feb 2021 00:06:04 -0500 vincent.cheng...@renesas.com wrote:

>> +static int read_sys_apll_status(struct idtcm *idtcm, u8 *status)
>> +{
>> +int err;
>> +
>> +err = idtcm_read(idtcm, STATUS, DPLL_SYS_APLL_STATUS, status,
>> + sizeof(u8));
>> +return err;
>
>Please remove the unnecessary 'err' variable:

Yes, fixed in v3 patch.

>   return idtcm_read(..
>
>There are bots scanning the tree for such code simplifications, 
>better to get this right from the start than deal with flood of 
>simplifications patches.

Totally, agree.  Thanks.

>> +{
>> +int err;
>> +
>> +err = idtcm_read(idtcm, STATUS, DPLL_SYS_STATUS, status, sizeof(u8));
>> +
>> +return err;
>
>same here

Fixed in v3 patch.

>
>> +}
>> +
>> +static int wait_for_sys_apll_dpll_lock(struct idtcm *idtcm)
>> +{
>> +const char *fmt = "%d ms SYS lock timeout: APLL Loss Lock %d  DPLL 
>> state %d";
>> +u8 i = LOCK_TIMEOUT_MS / LOCK_POLL_INTERVAL_MS;
>
>Using msleep() and loops is quite inaccurate. I'd recommend you switch
>to:
>
>   unsigned long timeout = jiffies + msecs_to_jiffies(LOCK_TIMEOUT_MS);
>
>And then use:
>
>   while (time_is_after_jiffies(timeout))
>

To clarify, the suggestion is to use jiffies for accuracy, but
the msleep(LOCK_POLL_INTERVAL_MS) remains to prevent the do-while
loop from becoming a busy-wait loop.

#define LOCK_TIMEOUT_MS (2000)
#define LOCK_POLL_INTERVAL_MS   (10)

unsigned long timeout = jiffies + msecs_to_jiffies(LOCK_TIMEOUT_MS);

do {
...
/*refresh 'locked' variable */
if (locked)
return 0;

msleep(LOCK_POLL_INTERVAL_MS);

} while (time_is_after_jiffies(timeout));


>For the condition.
>
>> +u8 apll = 0;
>> +u8 dpll = 0;
>> +
>> +int err;
>
>No empty lines between variables, please.

Yes, will fix in v3 patch.

>> +
>> +do {
>> +err = read_sys_apll_status(idtcm, );
>> +
>
>No empty lines between call and the if, please.

Okay, will fix in v3 patch.

>> +dpll &= DPLL_SYS_STATE_MASK;
>> +
>> +if ((apll == SYS_APLL_LOSS_LOCK_LIVE_LOCKED)
>
>parenthesis around a == b are unnecessary.

Yes, will fix in v3 patch.


>> +} else if ((dpll == DPLL_STATE_FREERUN) ||
>> +   (dpll == DPLL_STATE_HOLDOVER) ||
>> +   (dpll == DPLL_STATE_OPEN_LOOP)) {
>
>same here.

Ditto.

>
>> +dev_warn(>client->dev,
>> +"No wait state: DPLL_SYS_STATE %d", dpll);
>
>It looks like other prints in this function use \n at the end of the
>lines, should we keep it consistent?

Looks like the \n is not needed for dev_warn.  Will remove \n 
of existing messages for consistency.

>
>> +return -EPERM;
>> +}
>> +
>> +msleep(LOCK_POLL_INTERVAL_MS);
>> +i--;
>> +
>
>unnecessary empty line

Yes, will fix in v3 patch.

>> +dev_warn(>client->dev, fmt, LOCK_TIMEOUT_MS, apll, dpll);
>
>I'd recommend leaving the format in place, that way static code
>checkers can validate the arguments.

Good point.  The fmt was used to keep 80 column rule.
But now that 100 column rule is in place, the fmt
workaround is not needed anymore.  Will fix in v3 patch.

>> +static void wait_for_chip_ready(struct idtcm *idtcm)
>> +{
>> +if (wait_for_boot_status_ready(idtcm))
>> +dev_warn(>client->dev, "BOOT_STATUS != 0xA0");
>
>no new line?

Nope.  Tried an experiment and \n is not neeeded.

dev_warn(>client->dev, "debug: has \\n at end\n");
dev_warn(>client->dev, "debug: does not have \\n at end");
dev_warn(>client->dev, "debug: has \\n\\n at end\n\n");
dev_warn(>client->dev, "debug: hello");
dev_warn(>client->dev, "debug: world");

[   99.069100] idtcm 15-005b: debug: has \n at end
[   99.073623] idtcm 15-005b: debug: does not have \n at end
[   99.079017] idtcm 15-005b: debug: has \n\n at end
[   99.079017]
[   99.085194] idtcm 15-005b: debug: hello
[   99.089025] idtcm 15-005b: debug: world

>> +
>> +if (wait_for_sys_apll_dpll_lock(idtcm))
>> +dev_warn(>client->dev,
>> + "Continuing while SYS APLL/DPLL is not locked");
>
>And here.

\n not needed.

Thank-you for the comments, helps make cleaner code.

Vincent


Re: [PATCH net-next 1/2] ptp: ptp_clockmatrix: Add wait_for_sys_apll_dpll_lock.

2021-02-12 Thread Vincent Cheng
On Fri, Feb 12, 2021 at 10:31:40AM EST, Richard Cochran wrote:

>On Thu, Feb 11, 2021 at 11:38:44PM -0500, vincent.cheng...@renesas.com wrote:
>
>> +static int wait_for_sys_apll_dpll_lock(struct idtcm *idtcm)
>> +{
>> +char *fmt = "%d ms SYS lock timeout: APLL Loss Lock %d  DPLL state %d";
>
>Probably you want: const char *fmt

Good point, will change in V2 patch.

>
>> diff --git a/drivers/ptp/ptp_clockmatrix.h b/drivers/ptp/ptp_clockmatrix.h
>> index 645de2c..fb32327 100644
>> --- a/drivers/ptp/ptp_clockmatrix.h
>...
>
>> @@ -123,7 +137,6 @@ struct idtcm_channel {
>>  enum pll_mode   pll_mode;
>>  u8  pll;
>>  u16 output_mask;
>> -u8  output_phase_adj[MAX_OUTPUT][4];
>>  };
>
>Looks like this removal is unrelated to the patch subject, and so it
>deserves its own small patch.

Ok, will separate into separate patch for V2.

Vincent



Re: [PATCH net-next 3/3] ptp: ptp_clockmatrix: Add adjphase() to support PHC write phase mode.

2020-05-01 Thread Vincent Cheng
On Thu, Apr 30, 2020 at 11:56:01PM EDT, Richard Cochran wrote:
>On Wed, Apr 29, 2020 at 08:28:25PM -0400, vincent.cheng...@renesas.com wrote:
>> @@ -871,6 +880,69 @@ static int idtcm_set_pll_mode(struct idtcm_channel 
>> *channel,
>>  
>> +int err;
>> +u8 i;
>> +u8 buf[4] = {0};
>> +s32 phaseIn50Picoseconds;
>> +s64 phaseOffsetInPs;
>
>Kernel coding style uses lower_case_underscores rather than lowerCamelCase.

Sorry, missed that.  Will fix.

>> +kthread_queue_delayed_work(channel->kworker,
>> +   >write_phase_ready_work,
>> +   msecs_to_jiffies(WR_PHASE_SETUP_MS));
>
>Each PHC driver automatically has a kworker provided by the class
>layer.  In order to use it, set ptp_clock_info.do_aux_work to your
>callback function and then call ptp_schedule_worker() when needed.
>
>See drivers/net/ethernet/ti/cpts.c for example.

That is nice of the API, thank-you for the example.  Will fix and re-submit.

Thanks,
Vincent


Re: [PATCH net-next 1/3] ptp: Add adjphase function to support phase offset control.

2020-05-01 Thread Vincent Cheng
On Thu, Apr 30, 2020 at 11:37:34PM EDT, Richard Cochran wrote:
>On Wed, Apr 29, 2020 at 08:28:23PM -0400, vincent.cheng...@renesas.com wrote:
>> diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
>> index acabbe7..c46ff98 100644
>> --- a/drivers/ptp/ptp_clock.c
>> +++ b/drivers/ptp/ptp_clock.c
>> @@ -146,6 +146,8 @@ static int ptp_clock_adjtime(struct posix_clock *pc, 
>> struct __kernel_timex *tx)
>>  else
>>  err = ops->adjfreq(ops, ppb);
>>  ptp->dialed_frequency = tx->freq;
>> +} else if (tx->modes & ADJ_OFFSET) {
>> +err = ops->adjphase(ops, tx->offset);
>
>This is a new method, and no drivers have it, so there must be a check
>that the function pointer is non-null.

Yes, good point.  Will fix and resubmit.

Thanks,
Vincent


[PATCH v3 2/2] ptp: Add a ptp clock driver for IDT ClockMatrix.

2019-10-21 Thread vincent . cheng . xh
From: Vincent Cheng 

The IDT ClockMatrix (TM) family includes integrated devices that provide
eight PLL channels.  Each PLL channel can be independently configured as a
frequency synthesizer, jitter attenuator, digitally controlled
oscillator (DCO), or a digital phase lock loop (DPLL).  Typically
these devices are used as timing references and clock sources for PTP
applications.  This patch adds support for the device.

Co-developed-by: Richard Cochran 
Signed-off-by: Richard Cochran 
Signed-off-by: Vincent Cheng 
---
Changes since v2:
 - As suggested by Andrew Lunn:
   1. 'err' is an int, replace s32 with int.
   2. Return -EINVAL instead of -1
   3. Remove indirection that is used only for unit testing.

 - As suggested by Rob Herring:
   1. Remove '-ptp' from compatible string
   2. Replace wildcard 'x' with the part numbers.

Changes since v1:
 - Reported-by: kbuild test robot 
   1. Fix ARCH=i386 build failure: ERROR: "__divdi3" undefined!

 - As suggested by Andrew Lunn:
   1. Replace pr_err with dev_err because we are an i2c device
   2. Replace set_current_state()+schedule_timeout() with
  msleep_interruptable()
   3. Downgrade pr_info to dev_dbg where appropriate
---
 drivers/ptp/Kconfig   |   12 +
 drivers/ptp/Makefile  |1 +
 drivers/ptp/idt8a340_reg.h|  659 +++
 drivers/ptp/ptp_clockmatrix.c | 1424 +
 drivers/ptp/ptp_clockmatrix.h |  104 +++
 5 files changed, 2200 insertions(+)
 create mode 100644 drivers/ptp/idt8a340_reg.h
 create mode 100644 drivers/ptp/ptp_clockmatrix.c
 create mode 100644 drivers/ptp/ptp_clockmatrix.h

diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
index 0517272..c48ad23 100644
--- a/drivers/ptp/Kconfig
+++ b/drivers/ptp/Kconfig
@@ -119,4 +119,16 @@ config PTP_1588_CLOCK_KVM
  To compile this driver as a module, choose M here: the module
  will be called ptp_kvm.
 
+config PTP_1588_CLOCK_IDTCM
+   tristate "IDT CLOCKMATRIX as PTP clock"
+   select PTP_1588_CLOCK
+   default n
+   help
+ This driver adds support for using IDT CLOCKMATRIX(TM) as a PTP
+ clock. This clock is only useful if your time stamping MAC
+ is connected to the IDT chip.
+
+ To compile this driver as a module, choose M here: the module
+ will be called ptp_clockmatrix.
+
 endmenu
diff --git a/drivers/ptp/Makefile b/drivers/ptp/Makefile
index 677d1d1..69a06f8 100644
--- a/drivers/ptp/Makefile
+++ b/drivers/ptp/Makefile
@@ -12,3 +12,4 @@ obj-$(CONFIG_PTP_1588_CLOCK_KVM)  += ptp_kvm.o
 obj-$(CONFIG_PTP_1588_CLOCK_QORIQ) += ptp-qoriq.o
 ptp-qoriq-y+= ptp_qoriq.o
 ptp-qoriq-$(CONFIG_DEBUG_FS)   += ptp_qoriq_debugfs.o
+obj-$(CONFIG_PTP_1588_CLOCK_IDTCM) += ptp_clockmatrix.o
\ No newline at end of file
diff --git a/drivers/ptp/idt8a340_reg.h b/drivers/ptp/idt8a340_reg.h
new file mode 100644
index 000..9263bc3
--- /dev/null
+++ b/drivers/ptp/idt8a340_reg.h
@@ -0,0 +1,659 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/* idt8a340_reg.h
+ *
+ * Originally generated by regen.tcl on Thu Feb 14 19:23:44 PST 2019
+ * https://github.com/richardcochran/regen
+ *
+ * Hand modified to include some HW registers.
+ * Based on 4.8.0, SCSR rev C commit a03c7ae5
+ */
+#ifndef HAVE_IDT8A340_REG
+#define HAVE_IDT8A340_REG
+
+#define PAGE_ADDR_BASE0x
+#define PAGE_ADDR 0x00fc
+
+#define HW_REVISION   0x8180
+#define REV_ID0x007a
+
+#define HW_DPLL_0 (0x8a00)
+#define HW_DPLL_1 (0x8b00)
+#define HW_DPLL_2 (0x8c00)
+#define HW_DPLL_3 (0x8d00)
+
+#define HW_DPLL_TOD_SW_TRIG_ADDR__0   (0x080)
+#define HW_DPLL_TOD_CTRL_1(0x089)
+#define HW_DPLL_TOD_CTRL_2(0x08A)
+#define HW_DPLL_TOD_OVR__0(0x098)
+#define HW_DPLL_TOD_OUT_0__0  (0x0B0)
+
+#define HW_Q0_Q1_CH_SYNC_CTRL_0   (0xa740)
+#define HW_Q0_Q1_CH_SYNC_CTRL_1   (0xa741)
+#define HW_Q2_Q3_CH_SYNC_CTRL_0   (0xa742)
+#define HW_Q2_Q3_CH_SYNC_CTRL_1   (0xa743)
+#define HW_Q4_Q5_CH_SYNC_CTRL_0   (0xa744)
+#define HW_Q4_Q5_CH_SYNC_CTRL_1   (0xa745)
+#define HW_Q6_Q7_CH_SYNC_CTRL_0   (0xa746)
+#define HW_Q6_Q7_CH_SYNC_CTRL_1   (0xa747)
+#define HW_Q8_CH_SYNC_CTRL_0  (0xa748)
+#define HW_Q8_CH_SYNC_CTRL_1  (0xa749)
+#define HW_Q9_CH_SYNC_CTRL_0  (0xa74a)
+#define HW_Q9_CH_SYNC_CTRL_1  (0xa74b)
+#define HW_Q10_CH_SYNC_CTRL_0 (0xa74c)
+#define HW_Q10_CH_SYNC_CTRL_1 (0xa74d)
+#define HW_Q11_CH_SYNC_CTRL_0 (0xa74e)
+#define HW_Q11_CH_SYNC_CTRL_1 (0xa74f)
+
+#define SYNC_SOURCE_DPLL0_TOD_PPS  0x14
+#define SYNC_SOURCE_DPLL1_TOD_PP

[PATCH v3 1/2] dt-bindings: ptp: Add bindings doc for IDT ClockMatrix based PTP clock

2019-10-21 Thread vincent . cheng . xh
From: Vincent Cheng 

Add device tree binding doc for the IDT ClockMatrix PTP clock.

Co-developed-by: Richard Cochran 
Signed-off-by: Richard Cochran 
Signed-off-by: Vincent Cheng 
---

Changes since v2:
 - As suggested by Rob Herring:
   1. Replace with DT schema
   2. Remove '-ptp' from compatible string
   3. Replace wildcard 'x' with the part numbers.

Changes since v1:
 - No changes
---
 .../devicetree/bindings/ptp/ptp-idtcm.yaml | 63 ++
 1 file changed, 63 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ptp/ptp-idtcm.yaml

diff --git a/Documentation/devicetree/bindings/ptp/ptp-idtcm.yaml 
b/Documentation/devicetree/bindings/ptp/ptp-idtcm.yaml
new file mode 100644
index 000..d3771e0
--- /dev/null
+++ b/Documentation/devicetree/bindings/ptp/ptp-idtcm.yaml
@@ -0,0 +1,63 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/ptp/ptp-idtcm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: IDT ClockMatrix (TM) PTP Clock Device Tree Bindings
+
+maintainers:
+  - Vincent Cheng 
+
+properties:
+  compatible:
+enum:
+  # For System Synchronizer
+  - idt,8a34000
+  - idt,8a34001
+  - idt,8a34002
+  - idt,8a34003
+  - idt,8a34004
+  - idt,8a34005
+  - idt,8a34006
+  - idt,8a34007
+  - idt,8a34008
+  - idt,8a34009
+  # For Port Synchronizer
+  - idt,8a34010
+  - idt,8a34011
+  - idt,8a34012
+  - idt,8a34013
+  - idt,8a34014
+  - idt,8a34015
+  - idt,8a34016
+  - idt,8a34017
+  - idt,8a34018
+  - idt,8a34019
+  # For Universal Frequency Translator (UFT)
+  - idt,8a34040
+  - idt,8a34041
+  - idt,8a34042
+  - idt,8a34043
+  - idt,8a34044
+  - idt,8a34045
+  - idt,8a34046
+  - idt,8a34047
+  - idt,8a34048
+  - idt,8a34049
+
+  reg:
+maxItems: 1
+description:
+  I2C slave address of the device.
+
+required:
+  - compatible
+  - reg
+
+examples:
+  - |
+phc@5b {
+  compatible = "idt,8a34000";
+  reg = <0x5b>;
+};
-- 
2.7.4



Re: [PATCH 1/2] dt-bindings: ptp: Add binding doc for IDT ClockMatrix based PTP clock

2019-10-03 Thread Vincent Cheng
On Tue, Oct 01, 2019 at 06:09:06PM EDT, Rob Herring wrote:
>On Wed, Sep 18, 2019 at 04:06:37PM -0400, vincent.cheng...@renesas.com wrote:
>> From: Vincent Cheng 

Hi Rob,

Welcome back.  Thank-you for providing feedback.

>> 
>> Add device tree binding doc for the IDT ClockMatrix PTP clock driver.
>
>Bindings are for h/w, not drivers...

Yes, will remove 'driver'.

>>  Documentation/devicetree/bindings/ptp/ptp-idtcm.txt | 15 +++
>>  1 file changed, 15 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/ptp/ptp-idtcm.txt
>
>Please make this a DT schema.

Sure, will give it a try.

>> +  - compatible  Should be "idt,8a3400x-ptp" for System Synchronizer
>> +Should be "idt,8a3401x-ptp" for Port Synchronizer
>> +Should be "idt,8a3404x-ptp" for Universal Frequency 
>> Translator (UFT)
>
>If PTP is the only function of the chip, you don't need to append 
>'-ptp'.

Okay, will remove '-ptp'.  Thanks.


>What's the 'x' for? We generally don't use wildcards in compatible 
>strings.

We were hoping to use 'x' to represent a single driver to match the various
part numbers 8A34001, 8A34002, 8A34003, 8A34004, 8A34011, 8A34012, etc.

What should be used instead of 'x'?

Thanks,
Vincent


Re: [PATCH v2 2/2] ptp: Add a ptp clock driver for IDT ClockMatrix.

2019-09-27 Thread Vincent Cheng
On Fri, Sep 27, 2019 at 08:25:18AM EDT, Andrew Lunn wrote:
>> +static s32 idtcm_xfer(struct idtcm *idtcm,
>> +  u8 regaddr,
>> +  u8 *buf,
>> +  u16 count,
>> +  bool write)
>> +{
>> +struct i2c_client *client = idtcm->client;
>> +struct i2c_msg msg[2];
>> +s32 cnt;
>> +
>> +msg[0].addr = client->addr;
>> +msg[0].flags = 0;
>> +msg[0].len = 1;
>> +msg[0].buf = 
>> +
>> +msg[1].addr = client->addr;
>> +msg[1].flags = write ? 0 : I2C_M_RD;
>> +msg[1].len = count;
>> +msg[1].buf = buf;
>> +
>> +cnt = i2c_transfer(client->adapter, msg, 2);
>> +
>> +if (cnt < 0) {
>> +dev_err(>dev, "i2c_transfer returned %d\n", cnt);
>> +return cnt;
>> +} else if (cnt != 2) {
>> +dev_err(>dev,
>> +"i2c_transfer sent only %d of %d messages\n", cnt, 2);
>> +return -EIO;
>> +}
>> +
>> +return 0;
>> +}
>> +
>> +static s32 idtcm_page_offset(struct idtcm *idtcm, u8 val)
>> +{
>> +u8 buf[4];
>> +s32 err;
>
>Hi Vincent

Hi Andrew,

Thank-you for looking at the patch.

>All your functions return s32, rather than the usual int. err is an
>s32.  i2c_transfer() will return an int, which you then assign to an
>s32.  I've no idea, but maybe the static code checkers like smatch
>will complain about this, especially on 64 bit systems? I suspect on
>64 bit machines, the compiler will be generating worse code, masking
>registers? Maybe use int, not s32?

Oops.  You are correct, I messed up when trying to standardize
on linux types.h.  I will go through the code to ensure int is used
for error codes and return values.

>> +case OUTPUT_MASK_PLL2_ADDR + 1:
>> +SET_U16_MSB(idtcm->channel[2].output_mask, val);
>> +break;
>> +case OUTPUT_MASK_PLL3_ADDR:
>> +SET_U16_LSB(idtcm->channel[3].output_mask, val);
>> +break;
>> +case OUTPUT_MASK_PLL3_ADDR + 1:
>> +SET_U16_MSB(idtcm->channel[3].output_mask, val);
>> +break;
>> +default:
>> +err = -1;
>
>EINVAL?

Yes, will replace with -EINVAL.  Thanks.

>> +static void set_default_function_pointers(struct idtcm *idtcm)
>> +{
>> +idtcm->_idtcm_gettime = _idtcm_gettime;
>> +idtcm->_idtcm_settime = _idtcm_settime;
>> +idtcm->_idtcm_rdwr = idtcm_rdwr;
>> +idtcm->_sync_pll_output = sync_pll_output;
>> +}
>
>Why does this indirection? Are the SPI versions of the silicon?

The indirection is to enable us to replace those functions in
our unit tests with mocked functions.

I read somewhere that I should leave a week between sending a
revised patch series.  Is this a good rule to follow?

Regards,
Vincent


[PATCH v2 2/2] ptp: Add a ptp clock driver for IDT ClockMatrix.

2019-09-26 Thread vincent . cheng . xh
From: Vincent Cheng 

The IDT ClockMatrix (TM) family includes integrated devices that provide
eight PLL channels.  Each PLL channel can be independently configured as a
frequency synthesizer, jitter attenuator, digitally controlled
oscillator (DCO), or a digital phase lock loop (DPLL).  Typically
these devices are used as timing references and clock sources for PTP
applications.  This patch adds support for the device.

Co-developed-by: Richard Cochran 
Signed-off-by: Richard Cochran 
Signed-off-by: Vincent Cheng 
---
Changes since v1:
 - Reported-by: kbuild test robot 
   Fix ARCH=i386 build failure: ERROR: "__divdi3" undefined!

 - As suggested by Andrew Lunn:
   1. Replace pr_err with dev_err because we are an i2c device
   2. Replace set_current_state()+schedule_timeout() with
  msleep_interruptable()
   3. Downgrade pr_info to dev_dbg where appropriate
---
 drivers/ptp/Kconfig   |   12 +
 drivers/ptp/Makefile  |1 +
 drivers/ptp/idt8a340_reg.h|  659 
 drivers/ptp/ptp_clockmatrix.c | 1385 +
 drivers/ptp/ptp_clockmatrix.h |  123 
 5 files changed, 2180 insertions(+)
 create mode 100644 drivers/ptp/idt8a340_reg.h
 create mode 100644 drivers/ptp/ptp_clockmatrix.c
 create mode 100644 drivers/ptp/ptp_clockmatrix.h

diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
index 960961f..16c7c90 100644
--- a/drivers/ptp/Kconfig
+++ b/drivers/ptp/Kconfig
@@ -119,4 +119,16 @@ config PTP_1588_CLOCK_KVM
  To compile this driver as a module, choose M here: the module
  will be called ptp_kvm.
 
+config PTP_1588_CLOCK_IDTCM
+   tristate "IDT CLOCKMATRIX as PTP clock"
+   select PTP_1588_CLOCK
+   default n
+   help
+ This driver adds support for using IDT CLOCKMATRIX(TM) as a PTP
+ clock. This clock is only useful if your time stamping MAC
+ is connected to the IDT chip.
+
+ To compile this driver as a module, choose M here: the module
+ will be called ptp_clockmatrix.
+
 endmenu
diff --git a/drivers/ptp/Makefile b/drivers/ptp/Makefile
index 677d1d1..69a06f8 100644
--- a/drivers/ptp/Makefile
+++ b/drivers/ptp/Makefile
@@ -12,3 +12,4 @@ obj-$(CONFIG_PTP_1588_CLOCK_KVM)  += ptp_kvm.o
 obj-$(CONFIG_PTP_1588_CLOCK_QORIQ) += ptp-qoriq.o
 ptp-qoriq-y+= ptp_qoriq.o
 ptp-qoriq-$(CONFIG_DEBUG_FS)   += ptp_qoriq_debugfs.o
+obj-$(CONFIG_PTP_1588_CLOCK_IDTCM) += ptp_clockmatrix.o
\ No newline at end of file
diff --git a/drivers/ptp/idt8a340_reg.h b/drivers/ptp/idt8a340_reg.h
new file mode 100644
index 000..9263bc3
--- /dev/null
+++ b/drivers/ptp/idt8a340_reg.h
@@ -0,0 +1,659 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/* idt8a340_reg.h
+ *
+ * Originally generated by regen.tcl on Thu Feb 14 19:23:44 PST 2019
+ * https://github.com/richardcochran/regen
+ *
+ * Hand modified to include some HW registers.
+ * Based on 4.8.0, SCSR rev C commit a03c7ae5
+ */
+#ifndef HAVE_IDT8A340_REG
+#define HAVE_IDT8A340_REG
+
+#define PAGE_ADDR_BASE0x
+#define PAGE_ADDR 0x00fc
+
+#define HW_REVISION   0x8180
+#define REV_ID0x007a
+
+#define HW_DPLL_0 (0x8a00)
+#define HW_DPLL_1 (0x8b00)
+#define HW_DPLL_2 (0x8c00)
+#define HW_DPLL_3 (0x8d00)
+
+#define HW_DPLL_TOD_SW_TRIG_ADDR__0   (0x080)
+#define HW_DPLL_TOD_CTRL_1(0x089)
+#define HW_DPLL_TOD_CTRL_2(0x08A)
+#define HW_DPLL_TOD_OVR__0(0x098)
+#define HW_DPLL_TOD_OUT_0__0  (0x0B0)
+
+#define HW_Q0_Q1_CH_SYNC_CTRL_0   (0xa740)
+#define HW_Q0_Q1_CH_SYNC_CTRL_1   (0xa741)
+#define HW_Q2_Q3_CH_SYNC_CTRL_0   (0xa742)
+#define HW_Q2_Q3_CH_SYNC_CTRL_1   (0xa743)
+#define HW_Q4_Q5_CH_SYNC_CTRL_0   (0xa744)
+#define HW_Q4_Q5_CH_SYNC_CTRL_1   (0xa745)
+#define HW_Q6_Q7_CH_SYNC_CTRL_0   (0xa746)
+#define HW_Q6_Q7_CH_SYNC_CTRL_1   (0xa747)
+#define HW_Q8_CH_SYNC_CTRL_0  (0xa748)
+#define HW_Q8_CH_SYNC_CTRL_1  (0xa749)
+#define HW_Q9_CH_SYNC_CTRL_0  (0xa74a)
+#define HW_Q9_CH_SYNC_CTRL_1  (0xa74b)
+#define HW_Q10_CH_SYNC_CTRL_0 (0xa74c)
+#define HW_Q10_CH_SYNC_CTRL_1 (0xa74d)
+#define HW_Q11_CH_SYNC_CTRL_0 (0xa74e)
+#define HW_Q11_CH_SYNC_CTRL_1 (0xa74f)
+
+#define SYNC_SOURCE_DPLL0_TOD_PPS  0x14
+#define SYNC_SOURCE_DPLL1_TOD_PPS  0x15
+#define SYNC_SOURCE_DPLL2_TOD_PPS  0x16
+#define SYNC_SOURCE_DPLL3_TOD_PPS  0x17
+
+#define SYNCTRL1_MASTER_SYNC_RST   BIT(7)
+#define SYNCTRL1_MASTER_SYNC_TRIG  BIT(5)
+#define SYNCTRL1_TOD_SYNC_TRIG BIT(4)
+#define SYNCTRL1_FBDIV_FRAME_SYNC_TRIG BIT(3)
+#define SYNCTRL1_FBDIV_SYNC_TRIG  

[PATCH v2 1/2] dt-bindings: ptp: Add bindings doc for IDT ClockMatrix based PTP clock

2019-09-26 Thread vincent . cheng . xh
From: Vincent Cheng 

Add device tree binding doc for the IDT ClockMatrix PTP clock driver.

Co-developed-by: Richard Cochran 
Signed-off-by: Richard Cochran 
Signed-off-by: Vincent Cheng 
---

Changes since v1:
 - No changes
---
 Documentation/devicetree/bindings/ptp/ptp-idtcm.txt | 15 +++
 1 file changed, 15 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ptp/ptp-idtcm.txt

diff --git a/Documentation/devicetree/bindings/ptp/ptp-idtcm.txt 
b/Documentation/devicetree/bindings/ptp/ptp-idtcm.txt
new file mode 100644
index 000..4eaa34d
--- /dev/null
+++ b/Documentation/devicetree/bindings/ptp/ptp-idtcm.txt
@@ -0,0 +1,15 @@
+* IDT ClockMatrix (TM) PTP clock
+
+Required properties:
+
+  - compatible  Should be "idt,8a3400x-ptp" for System Synchronizer
+Should be "idt,8a3401x-ptp" for Port Synchronizer
+Should be "idt,8a3404x-ptp" for Universal Frequency Translator 
(UFT)
+  - reg I2C slave address of the device
+
+Example:
+
+   phc@5b {
+   compatible = "idt,8a3400x-ptp";
+   reg = <0x5b>;
+   };
-- 
2.7.4



Re: [PATCH 2/2] ptp: Add a ptp clock driver for IDT ClockMatrix.

2019-09-19 Thread Vincent Cheng
Hi Andrew,

On Wed, Sep 18, 2019 at 05:18:03PM EDT, Andrew Lunn wrote:
>On Wed, Sep 18, 2019 at 04:06:38PM -0400, vincent.cheng...@renesas.com wrote:
>
>> +static s32 idtcm_xfer(struct idtcm *idtcm,
>> +  u8 regaddr,
>> +  u8 *buf,
>> +  u16 count,
>> +  bool write)
>> +{
>> +struct i2c_client *client = idtcm->client;
>> +struct i2c_msg msg[2];
>> +s32 cnt;
>> +
>> +msg[0].addr = client->addr;
>> +msg[0].flags = 0;
>> +msg[0].len = 1;
>> +msg[0].buf = 
>> +
>> +msg[1].addr = client->addr;
>> +msg[1].flags = write ? 0 : I2C_M_RD;
>> +msg[1].len = count;
>> +msg[1].buf = buf;
>> +
>> +cnt = i2c_transfer(client->adapter, msg, 2);
>> +
>> +if (cnt < 0) {
>> +pr_err("i2c_transfer returned %d\n", cnt);
>
>dev_err(client->dev, "i2c_transfer returned %d\n", cnt);
>
>We then have an idea which device has a transfer error.
>
>Please try to not use pr_err() when you have some sort of device.

Sure thing, will replace pr_err() with dev_err().

>> +static s32 idtcm_state_machine_reset(struct idtcm *idtcm)
>> +{
>> +s32 err;
>> +u8 byte = SM_RESET_CMD;
>> +
>> +err = idtcm_write(idtcm, RESET_CTRL, SM_RESET, , sizeof(byte));
>> +
>> +if (!err) {
>> +/* delay */
>> +set_current_state(TASK_INTERRUPTIBLE);
>> +schedule_timeout(_msecs_to_jiffies(POST_SM_RESET_DELAY_MS));
>
>Maybe use msleep_interruptable()? 

Yes, will try using msleep_interruptable() and will replace if it works.

>> +static s32 idtcm_load_firmware(struct idtcm *idtcm,
>> +   struct device *dev)
>> +{
>> +const struct firmware *fw;
>> +struct idtcm_fwrc *rec;
>> +u32 regaddr;
>> +s32 err;
>> +s32 len;
>> +u8 val;
>> +u8 loaddr;
>> +
>> +pr_info("requesting firmware '%s'\n", FW_FILENAME);
>
>dev_debug()

Thanks, will make the change.

>> +
>> +err = request_firmware(, FW_FILENAME, dev);
>> +
>> +if (err)
>> +return err;
>> +
>> +pr_info("firmware size %zu bytes\n", fw->size);
>
>dev_debug()
>
>Maybe look through all your pr_info and downgrade most of them to
>dev_debug()

Yes, will go through and downgrade to dev_debug() accordingly.

Thanks,
Vincent


[PATCH 2/2] ptp: Add a ptp clock driver for IDT ClockMatrix.

2019-09-18 Thread vincent . cheng . xh
From: Vincent Cheng 

The IDT ClockMatrix (TM) family includes integrated devices that provide
eight PLL channels.  Each PLL channel can be independently configured as a
frequency synthesizer, jitter attenuator, digitally controlled
oscillator (DCO), or a digital phase lock loop (DPLL).  Typically
these devices are used as timing references and clock sources for PTP
applications.  This patch adds support for the device.

Co-developed-by: Richard Cochran 
Signed-off-by: Richard Cochran 
Signed-off-by: Vincent Cheng 
---
 drivers/ptp/Kconfig   |   12 +
 drivers/ptp/Makefile  |1 +
 drivers/ptp/idt8a340_reg.h|  659 
 drivers/ptp/ptp_clockmatrix.c | 1384 +
 drivers/ptp/ptp_clockmatrix.h |  123 
 5 files changed, 2179 insertions(+)
 create mode 100644 drivers/ptp/idt8a340_reg.h
 create mode 100644 drivers/ptp/ptp_clockmatrix.c
 create mode 100644 drivers/ptp/ptp_clockmatrix.h

diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
index 960961f..16c7c90 100644
--- a/drivers/ptp/Kconfig
+++ b/drivers/ptp/Kconfig
@@ -119,4 +119,16 @@ config PTP_1588_CLOCK_KVM
  To compile this driver as a module, choose M here: the module
  will be called ptp_kvm.
 
+config PTP_1588_CLOCK_IDTCM
+   tristate "IDT CLOCKMATRIX as PTP clock"
+   select PTP_1588_CLOCK
+   default n
+   help
+ This driver adds support for using IDT CLOCKMATRIX(TM) as a PTP
+ clock. This clock is only useful if your time stamping MAC
+ is connected to the IDT chip.
+
+ To compile this driver as a module, choose M here: the module
+ will be called ptp_clockmatrix.
+
 endmenu
diff --git a/drivers/ptp/Makefile b/drivers/ptp/Makefile
index 677d1d1..9791ba9 100644
--- a/drivers/ptp/Makefile
+++ b/drivers/ptp/Makefile
@@ -12,3 +12,4 @@ obj-$(CONFIG_PTP_1588_CLOCK_KVM)  += ptp_kvm.o
 obj-$(CONFIG_PTP_1588_CLOCK_QORIQ) += ptp-qoriq.o
 ptp-qoriq-y+= ptp_qoriq.o
 ptp-qoriq-$(CONFIG_DEBUG_FS)   += ptp_qoriq_debugfs.o
+obj-$(CONFIG_PTP_1588_CLOCK_IDTCM) += ptp_clockmatrix.o
diff --git a/drivers/ptp/idt8a340_reg.h b/drivers/ptp/idt8a340_reg.h
new file mode 100644
index 000..9263bc3
--- /dev/null
+++ b/drivers/ptp/idt8a340_reg.h
@@ -0,0 +1,659 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/* idt8a340_reg.h
+ *
+ * Originally generated by regen.tcl on Thu Feb 14 19:23:44 PST 2019
+ * https://github.com/richardcochran/regen
+ *
+ * Hand modified to include some HW registers.
+ * Based on 4.8.0, SCSR rev C commit a03c7ae5
+ */
+#ifndef HAVE_IDT8A340_REG
+#define HAVE_IDT8A340_REG
+
+#define PAGE_ADDR_BASE0x
+#define PAGE_ADDR 0x00fc
+
+#define HW_REVISION   0x8180
+#define REV_ID0x007a
+
+#define HW_DPLL_0 (0x8a00)
+#define HW_DPLL_1 (0x8b00)
+#define HW_DPLL_2 (0x8c00)
+#define HW_DPLL_3 (0x8d00)
+
+#define HW_DPLL_TOD_SW_TRIG_ADDR__0   (0x080)
+#define HW_DPLL_TOD_CTRL_1(0x089)
+#define HW_DPLL_TOD_CTRL_2(0x08A)
+#define HW_DPLL_TOD_OVR__0(0x098)
+#define HW_DPLL_TOD_OUT_0__0  (0x0B0)
+
+#define HW_Q0_Q1_CH_SYNC_CTRL_0   (0xa740)
+#define HW_Q0_Q1_CH_SYNC_CTRL_1   (0xa741)
+#define HW_Q2_Q3_CH_SYNC_CTRL_0   (0xa742)
+#define HW_Q2_Q3_CH_SYNC_CTRL_1   (0xa743)
+#define HW_Q4_Q5_CH_SYNC_CTRL_0   (0xa744)
+#define HW_Q4_Q5_CH_SYNC_CTRL_1   (0xa745)
+#define HW_Q6_Q7_CH_SYNC_CTRL_0   (0xa746)
+#define HW_Q6_Q7_CH_SYNC_CTRL_1   (0xa747)
+#define HW_Q8_CH_SYNC_CTRL_0  (0xa748)
+#define HW_Q8_CH_SYNC_CTRL_1  (0xa749)
+#define HW_Q9_CH_SYNC_CTRL_0  (0xa74a)
+#define HW_Q9_CH_SYNC_CTRL_1  (0xa74b)
+#define HW_Q10_CH_SYNC_CTRL_0 (0xa74c)
+#define HW_Q10_CH_SYNC_CTRL_1 (0xa74d)
+#define HW_Q11_CH_SYNC_CTRL_0 (0xa74e)
+#define HW_Q11_CH_SYNC_CTRL_1 (0xa74f)
+
+#define SYNC_SOURCE_DPLL0_TOD_PPS  0x14
+#define SYNC_SOURCE_DPLL1_TOD_PPS  0x15
+#define SYNC_SOURCE_DPLL2_TOD_PPS  0x16
+#define SYNC_SOURCE_DPLL3_TOD_PPS  0x17
+
+#define SYNCTRL1_MASTER_SYNC_RST   BIT(7)
+#define SYNCTRL1_MASTER_SYNC_TRIG  BIT(5)
+#define SYNCTRL1_TOD_SYNC_TRIG BIT(4)
+#define SYNCTRL1_FBDIV_FRAME_SYNC_TRIG BIT(3)
+#define SYNCTRL1_FBDIV_SYNC_TRIG   BIT(2)
+#define SYNCTRL1_Q1_DIV_SYNC_TRIG  BIT(1)
+#define SYNCTRL1_Q0_DIV_SYNC_TRIG  BIT(0)
+
+#define RESET_CTRL0xc000
+#define SM_RESET  0x0012
+#define SM_RESET_CMD  0x5A
+
+#define GENERAL_STATUS0xc014
+#define HW_REV_ID 0x000A
+#define BOND_ID   0x000

[PATCH 1/2] dt-bindings: ptp: Add binding doc for IDT ClockMatrix based PTP clock

2019-09-18 Thread vincent . cheng . xh
From: Vincent Cheng 

Add device tree binding doc for the IDT ClockMatrix PTP clock driver.

Signed-off-by: Vincent Cheng 
---
 Documentation/devicetree/bindings/ptp/ptp-idtcm.txt | 15 +++
 1 file changed, 15 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ptp/ptp-idtcm.txt

diff --git a/Documentation/devicetree/bindings/ptp/ptp-idtcm.txt 
b/Documentation/devicetree/bindings/ptp/ptp-idtcm.txt
new file mode 100644
index 000..4eaa34d
--- /dev/null
+++ b/Documentation/devicetree/bindings/ptp/ptp-idtcm.txt
@@ -0,0 +1,15 @@
+* IDT ClockMatrix (TM) PTP clock
+
+Required properties:
+
+  - compatible  Should be "idt,8a3400x-ptp" for System Synchronizer
+Should be "idt,8a3401x-ptp" for Port Synchronizer
+Should be "idt,8a3404x-ptp" for Universal Frequency Translator 
(UFT)
+  - reg I2C slave address of the device
+
+Example:
+
+   phc@5b {
+   compatible = "idt,8a3400x-ptp";
+   reg = <0x5b>;
+   };
-- 
2.7.4



(no subject)

2015-08-07 Thread Mr. Vincent Cheng



--
I have a business proposal of USD $22,500,000.00 only for you to
transact with me from my bank to your country. Reply to
address: vincentche3...@gmail.com and I will let you know what is
required of you.

Best Regards,
Mr. Vincent Cheng
Your Brother
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


(no subject)

2015-08-07 Thread Mr. Vincent Cheng



--
I have a business proposal of USD $22,500,000.00 only for you to
transact with me from my bank to your country. Reply to
address: vincentche3...@gmail.com and I will let you know what is
required of you.

Best Regards,
Mr. Vincent Cheng
Your Brother
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


(no subject)

2015-07-29 Thread Mr. Vincent Cheng


-- 
I have a business proposal of USD $22,500,000.00 only for you to
transact with me from my bank to your country. Reply to
address: vincentche3...@gmail.com and I will let you know what is
required of you.

Best Regards,
Mr. Vincent Cheng
Your Brother
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


(no subject)

2015-07-29 Thread Mr. Vincent Cheng


-- 
I have a business proposal of USD $22,500,000.00 only for you to
transact with me from my bank to your country. Reply to
address: vincentche3...@gmail.com and I will let you know what is
required of you.

Best Regards,
Mr. Vincent Cheng
Your Brother
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [BISECTED] snd-hda-intel audio distortion in Linus' current tree

2012-10-07 Thread Vincent Cheng
[Cc: alsa-de...@alsa-project.org; also, please cc: me explicitly as
well, since I'm not subscribed to either list]

On Wed, Sep 26, 2012 at 12:29 AM, Steven Noonan  wrote:
> Started having audio problems when trying out the latest tree
> (v3.6-rc7-10-g56d27ad). When playing any kind of audio, there was
> significant distortion, mostly crackling noise. I'm using a Lenovo
> ThinkPad X230 (Panther Point).
>
> I did a git-bisect to locate the problem, and it seems this commit is to
> blame:
>
> c20c5a841cbe47f5b7812b57bd25397497e5fbc0 is the first bad commit
> commit c20c5a841cbe47f5b7812b57bd25397497e5fbc0
> Author: Seth Heasley 
> Date:   Thu Jun 14 14:23:53 2012 -0700
>
> ALSA: hda_intel: activate COMBO mode for Intel client chipsets
>
> This patch activates the COMBO position_fix for recent Intel 
> client chipsets.
> COMBO mode is the recommended setting for Intel chipsets and 
> eliminates HD
> audio warnings in dmesg.  This patch has been tested on Lynx 
> Point, Panther
> Point, and Cougar Pont.
>
> Signed-off-by: Seth Heasley 
> Signed-off-by: Takashi Iwai 
>
> It's pretty clear-cut. If I revert this patch, my sound starts
> functioning normally again.
>
> Any thoughts on how to proceed here? Can someone revert this, or is
> there some testing that I can do?
>
> Here's a pretty-printed bisection log, if needed:
>
>  # good: [28a33cbc] Linux 3.5
>  # bad:  [b13bc8dd] Merge tag 'staging-3.6-rc1' of git://git.kernel.or
>  # good: [3c4cfade] Merge git://git.kernel.org/pub/scm/linux/kernel/gi
>  # bad:  [9fc37779] Merge tag 'usb-3.6-rc1' of git://git.kernel.org/pu
>  # bad:  [f14121ab] Merge tag 'dt-for-3.6' of git://sources.calxeda.co
>  # good: [d14b7a41] Merge branch 'for-linus' of git://git.kernel.org/p
>  # good: [15d47763] Merge branch 'for-3.5' into for-3.6
>  # bad:  [dbf7b591] Merge tag 'sound-3.6' of git://git.kernel.org/pub/
>  # bad:  [1c76684d] ALSA: hda - add Haswell HDMI codec id
>  # bad:  [8b8d654b] ALSA: hda - Move one-time init codes from generic_
>  # good: [80c8bfbe] ALSA: HDA: Create phantom jacks for fixed inputs a
>  # bad:  [ceaa86ba] ALSA: hda - Remove invalid init verbs for Nvidia 2
>  # bad:  [4b6ace9e] ALSA: hda - Add the support for VIA HDMI pin detec
>  # bad:  [c20c5a84] ALSA: hda_intel: activate COMBO mode for Intel cli
>
>
> - Steven
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
>

I can confirm that I've also hit this bug as well, and that it's still
present in stable 3.6.0. Strangely enough however, this only seems to
affect VLC for me; while playing audio through mplayer or any
gstreamer-based players (Rhythmbox, Totem, etc.), I don't encounter
any audio distortion. Possibly also related to [1]?

A workaround (other than reverting this commit) is to not use COMBO
mode, i.e. load snd-hda-intel with position_fix=2.

Please let me know if any more information is needed.

$ lspci -vvnn | grep -A8 Audio
00:1b.0 Audio device [0403]: Intel Corporation 7 Series/C210 Series
Chipset Family High Definition Audio Controller [8086:1e20] (rev 04)
Subsystem: Toshiba America Info Systems Device [1179:fb30]
Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr-
Stepping- SERR- FastB2B- DisINTx+
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort-
SERR- 
Kernel driver in use: snd_hda_intel

Machine: Toshiba Satellite P850
Distro: Debian wheezy/sid
ALSA 1.0.25; PulseAudio 2.0

Regards,
Vincent

[1] 
http://mailman.alsa-project.org/pipermail/alsa-devel/2012-September/055161.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [BISECTED] snd-hda-intel audio distortion in Linus' current tree

2012-10-07 Thread Vincent Cheng
[Cc: alsa-de...@alsa-project.org; also, please cc: me explicitly as
well, since I'm not subscribed to either list]

On Wed, Sep 26, 2012 at 12:29 AM, Steven Noonan ste...@uplinklabs.net wrote:
 Started having audio problems when trying out the latest tree
 (v3.6-rc7-10-g56d27ad). When playing any kind of audio, there was
 significant distortion, mostly crackling noise. I'm using a Lenovo
 ThinkPad X230 (Panther Point).

 I did a git-bisect to locate the problem, and it seems this commit is to
 blame:

 c20c5a841cbe47f5b7812b57bd25397497e5fbc0 is the first bad commit
 commit c20c5a841cbe47f5b7812b57bd25397497e5fbc0
 Author: Seth Heasley seth.heas...@intel.com
 Date:   Thu Jun 14 14:23:53 2012 -0700

 ALSA: hda_intel: activate COMBO mode for Intel client chipsets

 This patch activates the COMBO position_fix for recent Intel 
 client chipsets.
 COMBO mode is the recommended setting for Intel chipsets and 
 eliminates HD
 audio warnings in dmesg.  This patch has been tested on Lynx 
 Point, Panther
 Point, and Cougar Pont.

 Signed-off-by: Seth Heasley seth.heas...@intel.com
 Signed-off-by: Takashi Iwai ti...@suse.de

 It's pretty clear-cut. If I revert this patch, my sound starts
 functioning normally again.

 Any thoughts on how to proceed here? Can someone revert this, or is
 there some testing that I can do?

 Here's a pretty-printed bisection log, if needed:

  # good: [28a33cbc] Linux 3.5
  # bad:  [b13bc8dd] Merge tag 'staging-3.6-rc1' of git://git.kernel.or
  # good: [3c4cfade] Merge git://git.kernel.org/pub/scm/linux/kernel/gi
  # bad:  [9fc37779] Merge tag 'usb-3.6-rc1' of git://git.kernel.org/pu
  # bad:  [f14121ab] Merge tag 'dt-for-3.6' of git://sources.calxeda.co
  # good: [d14b7a41] Merge branch 'for-linus' of git://git.kernel.org/p
  # good: [15d47763] Merge branch 'for-3.5' into for-3.6
  # bad:  [dbf7b591] Merge tag 'sound-3.6' of git://git.kernel.org/pub/
  # bad:  [1c76684d] ALSA: hda - add Haswell HDMI codec id
  # bad:  [8b8d654b] ALSA: hda - Move one-time init codes from generic_
  # good: [80c8bfbe] ALSA: HDA: Create phantom jacks for fixed inputs a
  # bad:  [ceaa86ba] ALSA: hda - Remove invalid init verbs for Nvidia 2
  # bad:  [4b6ace9e] ALSA: hda - Add the support for VIA HDMI pin detec
  # bad:  [c20c5a84] ALSA: hda_intel: activate COMBO mode for Intel cli


 - Steven
 --
 To unsubscribe from this list: send the line unsubscribe linux-kernel in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 Please read the FAQ at  http://www.tux.org/lkml/



I can confirm that I've also hit this bug as well, and that it's still
present in stable 3.6.0. Strangely enough however, this only seems to
affect VLC for me; while playing audio through mplayer or any
gstreamer-based players (Rhythmbox, Totem, etc.), I don't encounter
any audio distortion. Possibly also related to [1]?

A workaround (other than reverting this commit) is to not use COMBO
mode, i.e. load snd-hda-intel with position_fix=2.

Please let me know if any more information is needed.

$ lspci -vvnn | grep -A8 Audio
00:1b.0 Audio device [0403]: Intel Corporation 7 Series/C210 Series
Chipset Family High Definition Audio Controller [8086:1e20] (rev 04)
Subsystem: Toshiba America Info Systems Device [1179:fb30]
Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr-
Stepping- SERR- FastB2B- DisINTx+
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast TAbort-
TAbort- MAbort- SERR- PERR- INTx-
Latency: 0, Cache Line Size: 64 bytes
Interrupt: pin A routed to IRQ 46
Region 0: Memory at f731 (64-bit, non-prefetchable) [size=16K]
Capabilities: access denied
Kernel driver in use: snd_hda_intel

Machine: Toshiba Satellite P850
Distro: Debian wheezy/sid
ALSA 1.0.25; PulseAudio 2.0

Regards,
Vincent

[1] 
http://mailman.alsa-project.org/pipermail/alsa-devel/2012-September/055161.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/