On Wed, 4 Nov 2020 19:22:13 -0500 min.li...@renesas.com wrote:
> From: Min Li <min.li...@renesas.com>
>
> Add idt82p33_adjphase() to support PHC write phase mode.
>
> Changes since v1:
> -Fix broken build
>
> Changes since v2:
> -Fix trailing space
>
> Signed-off-by: Min Li <min.li...@renesas.com>
>
Please drop the empty line between tags.
> Acked-by: Richard Cochran <richardcoch...@gmail.com>
> diff --git a/drivers/ptp/ptp_idt82p33.c b/drivers/ptp/ptp_idt82p33.c
> index 179f6c4..d52fa67 100644
> --- a/drivers/ptp/ptp_idt82p33.c
> +++ b/drivers/ptp/ptp_idt82p33.c
> @@ -21,6 +21,7 @@ MODULE_DESCRIPTION("Driver for IDT 82p33xxx clock devices");
> MODULE_AUTHOR("IDT support-1588 <idt-support-1...@lm.renesas.com>");
> MODULE_VERSION("1.0");
> MODULE_LICENSE("GPL");
> +MODULE_FIRMWARE(FW_FILENAME);
>
> /* Module Parameters */
> static u32 sync_tod_timeout = SYNC_TOD_TIMEOUT_SEC;
> @@ -129,8 +130,9 @@ static int idt82p33_page_offset(struct idt82p33
> *idt82p33, unsigned char val)
> static int idt82p33_rdwr(struct idt82p33 *idt82p33, unsigned int regaddr,
> unsigned char *buf, unsigned int count, bool write)
> {
> - u8 offset, page;
> int err;
> + u8 page;
> + u8 offset;
Please don't make unrelated changes in your patches.
> page = _PAGE(regaddr);
> offset = _OFFSET(regaddr);
> @@ -145,13 +147,13 @@ static int idt82p33_rdwr(struct idt82p33 *idt82p33,
> unsigned int regaddr,
> }
>
> static int idt82p33_read(struct idt82p33 *idt82p33, unsigned int regaddr,
> - unsigned char *buf, unsigned int count)
> + unsigned char *buf, unsigned int count)
Unrelated change.
> {
> return idt82p33_rdwr(idt82p33, regaddr, buf, count, false);
> }
>
> static int idt82p33_write(struct idt82p33 *idt82p33, unsigned int regaddr,
> - unsigned char *buf, unsigned int count)
> + unsigned char *buf, unsigned int count)
Unrelated change.
> {
> return idt82p33_rdwr(idt82p33, regaddr, buf, count, true);
> }
> @@ -541,20 +543,13 @@ static int idt82p33_sync_tod(struct idt82p33_channel
> *channel, bool enable)
> if (err)
> return err;
>
> - channel->sync_tod_on = enable;
> -
> - if (enable && sync_tod_timeout) {
> - mod_delayed_work(system_wq, &channel->sync_tod_work,
> - sync_tod_timeout * HZ);
> - }
> -
> return 0;
You can simplify
err = idt82...
if (err)
return err;
return 0;
to:
return idt82p33_write(idt82p33, channel->dpll_sync_cnfg,
&sync_cnfg, sizeof(sync_cnfg));
> }
> -static int idt82p33_pps_enable(struct idt82p33_channel *channel, bool enable)
> +static int idt82p33_output_enable(struct idt82p33_channel *channel,
> + bool enable, unsigned int outn)
> {
> struct idt82p33 *idt82p33 = channel->idt82p33;
> - u8 mask, outn, val;
> int err;
> + u8 val;
> +
> + err = idt82p33_read(idt82p33, OUT_MUX_CNFG(outn), &val, sizeof(val));
> +
unnecessary empty line
> + if (err)
> + return err;
> +
> + if (enable)
> + val &= ~SQUELCH_ENABLE;
> + else
> + val |= SQUELCH_ENABLE;
> +
> + return idt82p33_write(idt82p33, OUT_MUX_CNFG(outn), &val, sizeof(val));
> +}
> +
> +static int idt82p33_output_mask_enable(struct idt82p33_channel *channel,
> + bool enable)
> +{
> + u16 mask;
> + int err;
> + u8 outn;
>
> mask = channel->output_mask;
> outn = 0;
>
> while (mask) {
> - if (mask & 0x1) {
> - err = idt82p33_read(idt82p33, OUT_MUX_CNFG(outn),
> - &val, sizeof(val));
> - if (err)
> - return err;
>
unnecessary empty line
> - if (enable)
> - val &= ~SQUELCH_ENABLE;
> - else
> - val |= SQUELCH_ENABLE;
> + if (mask & 0x1) {
>
unnecessary empty line
> - err = idt82p33_write(idt82p33, OUT_MUX_CNFG(outn),
> - &val, sizeof(val));
> + err = idt82p33_output_enable(channel, enable, outn);
>
unnecessary empty line
> if (err)
> return err;
> }
> +
> mask >>= 0x1;
> outn++;
> }
> @@ -659,14 +680,16 @@ static int idt82p33_enable(struct ptp_clock_info *ptp,
>
> if (rq->type == PTP_CLK_REQ_PEROUT) {
> if (!on)
> - err = idt82p33_pps_enable(channel, false);
> + err = idt82p33_perout_enable(channel, false,
> + &rq->perout);
>
unnecessary empty line
> /* Only accept a 1-PPS aligned to the second. */
> else if (rq->perout.start.nsec || rq->perout.period.sec != 1 ||
> rq->perout.period.nsec) {
> err = -ERANGE;
> } else
> - err = idt82p33_pps_enable(channel, true);
> + err = idt82p33_perout_enable(channel, true,
> + &rq->perout);
> }
>
> mutex_unlock(&idt82p33->reg_lock);
> @@ -674,6 +697,49 @@ static int idt82p33_enable(struct ptp_clock_info *ptp,
> return err;
> }
>
> +static int idt82p33_adjwritephase(struct ptp_clock_info *ptp, s32 offsetNs)
> +{
> + struct idt82p33_channel *channel =
> + container_of(ptp, struct idt82p33_channel, caps);
> + struct idt82p33 *idt82p33 = channel->idt82p33;
> + s64 offsetInFs;
> + s64 offsetRegVal;
please don't use cammelCase, I think checkpatch tries to warn about
this.
Also please try to order the variable declaration lines longest to
shortest (where possible, the channel declaration here should stay
first).
> + u8 val[4] = {0};
> + int err;
> @@ -839,19 +930,22 @@ static int idt82p33_enable_channel(struct idt82p33
> *idt82p33, u32 index)
>
> static int idt82p33_load_firmware(struct idt82p33 *idt82p33)
> {
> + char fname[128] = FW_FILENAME;
This variable seems unnecessary.
> const struct firmware *fw;
> struct idt82p33_fwrc *rec;
> u8 loaddr, page, val;
> int err;
> s32 len;
>
> - dev_dbg(&idt82p33->client->dev,
> - "requesting firmware '%s'\n", FW_FILENAME);
> + dev_dbg(&idt82p33->client->dev, "requesting firmware '%s'\n", fname);
>
> - err = request_firmware(&fw, FW_FILENAME, &idt82p33->client->dev);
> + err = request_firmware(&fw, fname, &idt82p33->client->dev);
>
> - if (err)
> + if (err) {
> + dev_err(&idt82p33->client->dev,
> + "Failed in %s with err %d!\n", __func__, err);
> return err;
> + }
>
> dev_dbg(&idt82p33->client->dev, "firmware size %zu bytes\n", fw->size);
>
