Richard, On Tue, 2018-12-18 at 20:27 -0800, Richard Cochran wrote: > On Thu, Dec 13, 2018 at 09:52:50AM -0800, dwest...@gmail.com wrote: > > Changes from V1: > > -> Remove debugfs for tod manipulation > > -> Reorder variable declarations in functions to order > > by length of declaration, largest to smallest > > -> Rename altera_ptp to intel_fpga_tod > > -> Rename functions in intel_fpga_tod so that they are not > > generic > > -> Use imply instead of select in Kconfig > > -> Re-write adjust time function to remove while loop with > > 64 bit divide. > > Overall this is looking better. A few more comments follow...
Thanks, all of your comments will be included in a v3 patch.
--dalon
>
> > +static int tse_get_ts_info(struct net_device *dev,
> > + struct ethtool_ts_info *info)
> > +{
> > + struct altera_tse_private *priv = netdev_priv(dev);
> > +
> > + if (priv->ptp_enable) {
> > + if (priv->ptp_priv.ptp_clock)
> > + info->phc_index =
> > + ptp_clock_index(priv->ptp_priv.ptp_clock);
>
> else
> info->phc_index = -1;
>
> > + info->so_timestamping = SOF_TIMESTAMPING_TX_HARDWARE |
> > + SOF_TIMESTAMPING_RX_HARDWARE |
> > + SOF_TIMESTAMPING_RAW_HARDWARE;
> > +
> > + info->tx_types = (1 << HWTSTAMP_TX_OFF) |
> > + (1 << HWTSTAMP_TX_ON);
>
> This fits on one line ^^^
>
> > + info->rx_filters = (1 << HWTSTAMP_FILTER_NONE) |
> > + (1 << HWTSTAMP_FILTER_ALL);
>
> Funky indentation here. One extra level is enough:
>
> info->rx_filters = (1 << HWTSTAMP_FILTER_NONE) |
> (1 << HWTSTAMP_FILTER_ALL);
>
> > + return 0;
> > + } else {
> > + return ethtool_op_get_ts_info(dev, info);
> > + }
> > +}
> > +
>
> ...
>
> > @@ -609,7 +613,14 @@ static netdev_tx_t tse_start_xmit(struct sk_buff *skb,
> > struct net_device *dev)
> > if (ret)
> > goto out;
> >
> > - skb_tx_timestamp(skb);
> > + /* Provide a hardware time stamp if requested.
> > + */
> > + if (unlikely((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
> > + priv->hwts_tx_en))
> > + /* declare that device is doing timestamping */
>
> Please drop those two comments. They are redundant because they only
> restate what the code does.
>
> > + skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> > + else
> > + skb_tx_timestamp(skb);
> >
> > priv->tx_prod++;
> > dev->stats.tx_bytes += skb->len;
>
> Thanks,
> Richard
smime.p7s
Description: S/MIME cryptographic signature
