| Message ID | 1568837198-27211-1-git-send-email-vincent.cheng.xh@renesas.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [1/2] dt-bindings: ptp: Add binding doc for IDT ClockMatrix based PTP clock | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success |
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 = ®addr; > + > + 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
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 = ®addr; >> + >> + 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
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 >
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
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
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>; + };