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

Message ID 1568837198-27211-1-git-send-email-vincent.cheng.xh@renesas.com
State Changes Requested
Headers show
Series
  • [1/2] dt-bindings: ptp: Add binding doc for IDT ClockMatrix based PTP clock
Related show

Checks

Context Check Description
robh/checkpatch success

Commit Message

Vincent Cheng Sept. 18, 2019, 8:06 p.m. UTC
From: Vincent Cheng <vincent.cheng.xh@renesas.com>

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

Signed-off-by: Vincent Cheng <vincent.cheng.xh@renesas.com>
---
 Documentation/devicetree/bindings/ptp/ptp-idtcm.txt | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ptp/ptp-idtcm.txt

Comments

Andrew Lunn Sept. 18, 2019, 9:18 p.m. UTC | #1
On Wed, Sep 18, 2019 at 04:06:38PM -0400, vincent.cheng.xh@renesas.com wrote:
> From: Vincent Cheng <vincent.cheng.xh@renesas.com>
> 
> 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 <richardcochran@gmail.com>
> Signed-off-by: Richard Cochran <richardcochran@gmail.com>
> Signed-off-by: Vincent Cheng <vincent.cheng.xh@renesas.com>

Hi Vincent 

> +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 = &regaddr;
> +
> +	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.


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

Maybe use msleep_interruptable()? 

> +	}
> +
> +	return err;
> +}
> +
> +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()

> +
> +	err = request_firmware(&fw, 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()

	Andrew
Vincent Cheng Sept. 19, 2019, 8:24 p.m. UTC | #2
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.xh@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 = &regaddr;
>> +
>> +	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, &byte, 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, 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
Rob Herring Oct. 1, 2019, 10:09 p.m. UTC | #3
On Wed, Sep 18, 2019 at 04:06:37PM -0400, vincent.cheng.xh@renesas.com wrote:
> From: Vincent Cheng <vincent.cheng.xh@renesas.com>
> 
> Add device tree binding doc for the IDT ClockMatrix PTP clock driver.

Bindings are for h/w, not drivers...

> 
> Signed-off-by: Vincent Cheng <vincent.cheng.xh@renesas.com>
> ---
>  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.

> 
> diff --git a/Documentation/devicetree/bindings/ptp/ptp-idtcm.txt b/Documentation/devicetree/bindings/ptp/ptp-idtcm.txt
> new file mode 100644
> index 0000000..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)

If PTP is the only function of the chip, you don't need to append 
'-ptp'.

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

> +  - reg         I2C slave address of the device
> +
> +Example:
> +
> +	phc@5b {
> +		compatible = "idt,8a3400x-ptp";
> +		reg = <0x5b>;
> +	};
> -- 
> 2.7.4
>
Vincent Cheng Oct. 3, 2019, 3:12 p.m. UTC | #4
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.xh@renesas.com wrote:
>> From: Vincent Cheng <vincent.cheng.xh@renesas.com>

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
Rob Herring Oct. 3, 2019, 3:56 p.m. UTC | #5
On Thu, Oct 3, 2019 at 10:12 AM Vincent Cheng
<vincent.cheng.xh@renesas.com> wrote:
>
> 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.xh@renesas.com wrote:
> >> From: Vincent Cheng <vincent.cheng.xh@renesas.com>
>
> 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'?

Enumerate all the part numbers. Are the differences discoverable in
some other way? If so, then 'x' is fine, but just add a note how
models are distinguished.

Rob

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/ptp/ptp-idtcm.txt b/Documentation/devicetree/bindings/ptp/ptp-idtcm.txt
new file mode 100644
index 0000000..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>;
+	};