There are some issues here.
On Thu, Sep 28, 2017 at 10:25:36AM -0500, Brandon Streiff wrote:
> +static int mv88e6xxx_config_periodic_trig(struct mv88e6xxx_chip *chip,
> + u32 ns, u16 picos)
> +{
> + int err;
> + u16 global_config;
> +
> + if (picos >= 1000)
> + return -ERANGE;
> +
> + /* TRIG generation is in units of 8 ns clock periods. Convert ns
> + * and ps into 8 ns clock periods and up to 8000 additional ps
> + */
> + picos += (ns & 0x7) * 1000;
> + ns = ns >> 3;
Again, the 8 nanosecounds shouldn't be hard coded.
...
> + return err;
> +}
> +static void mv88e6xxx_tai_event_work(struct work_struct *ugly)
> +{
> + struct delayed_work *dw = to_delayed_work(ugly);
> + struct mv88e6xxx_chip *chip =
> + container_of(dw, struct mv88e6xxx_chip, tai_event_work);
> + u16 ev_status[4];
> + int err;
> +
> + mutex_lock(&chip->reg_lock);
> +
> + err = mv88e6xxx_tai_read(chip, MV88E6XXX_TAI_EVENT_STATUS,
> + ev_status, ARRAY_SIZE(ev_status));
> + if (err) {
> + mutex_unlock(&chip->reg_lock);
> + return;
> + }
> +
> + if (ev_status[0] & MV88E6XXX_TAI_EVENT_STATUS_ERROR)
> + dev_warn(chip->dev, "missed event capture\n");
> +
> + if (ev_status[0] & MV88E6XXX_TAI_EVENT_STATUS_VALID) {
Avoid IfOk.
> + struct ptp_clock_event ev;
> + u32 raw_ts = ((u32)ev_status[2] << 16) | ev_status[1];
> +
> + /* Clear the valid bit so the next timestamp can come in */
> + ev_status[0] &= ~MV88E6XXX_TAI_EVENT_STATUS_VALID;
> + err = mv88e6xxx_tai_write(chip, MV88E6XXX_TAI_EVENT_STATUS,
> + ev_status[0]);
> +
> + if (ev_status[0] & MV88E6XXX_TAI_EVENT_STATUS_CAP_TRIG) {
> + /* TAI is configured to timestamp internal events.
> + * This will be a PPS event.
> + */
> + ev.type = PTP_CLOCK_PPS;
> + } else {
> + /* Otherwise this is an external timestamp */
> + ev.type = PTP_CLOCK_EXTTS;
> + }
> + /* We only have one timestamping channel. */
> + ev.index = 0;
> + ev.timestamp = timecounter_cyc2time(&chip->tstamp_tc, raw_ts);
> +
> + ptp_clock_event(chip->ptp_clock, &ev);
> + }
> +
> + mutex_unlock(&chip->reg_lock);
> +
> + schedule_delayed_work(&chip->tai_event_work, TAI_EVENT_WORK_INTERVAL);
> +}
> +
> +static int mv88e6xxx_ptp_enable_perout(struct mv88e6xxx_chip *chip,
> + struct ptp_clock_request *rq, int on)
> +{
> + struct timespec ts;
> + u64 ns;
> + int pin;
> + int err;
> +
> + pin = ptp_find_pin(chip->ptp_clock, PTP_PF_PEROUT, rq->extts.index);
> +
> + if (pin < 0)
> + return -EBUSY;
> +
> + ts.tv_sec = rq->perout.period.sec;
> + ts.tv_nsec = rq->perout.period.nsec;
> + ns = timespec_to_ns(&ts);
> +
> + if (ns > U32_MAX)
> + return -ERANGE;
> +
> + mutex_lock(&chip->reg_lock);
> +
> + err = mv88e6xxx_config_periodic_trig(chip, (u32)ns, 0);
Here you ignore the phase of the signal given in the trq->perout.start
field. That is not what the user expects. For periodic outputs where
the phase cannot be set, we really would need a new ioctl.
However, in this case, you should just drop this functionality. I
understand that this works with your adjustable external oscillator,
but we cannot support that in mainline (at least, not yet).
Thanks,
Richard
> + if (err)
> + goto out;
> +
> + if (on) {
> + err = mv88e6xxx_g2_set_gpio_config(
> + chip, pin, MV88E6XXX_G2_SCRATCH_GPIO_MODE_TRIG,
> + MV88E6XXX_G2_SCRATCH_GPIO_DIR_OUT);
> + } else {
> + err = mv88e6xxx_g2_set_gpio_config(
> + chip, pin, MV88E6XXX_G2_SCRATCH_GPIO_MODE_GPIO,
> + MV88E6XXX_G2_SCRATCH_GPIO_DIR_IN);
> + }
> +
> +out:
> + mutex_unlock(&chip->reg_lock);
> +
> + return err;
> +}
