Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 24 Nov 2022 12:22:21 +0200
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Arun.Ramadoss@microchip.com
Cc:     andrew@lunn.ch, linux-kernel@vger.kernel.org,
        UNGLinuxDriver@microchip.com, vivien.didelot@gmail.com,
        linux@armlinux.org.uk, Tristram.Ha@microchip.com,
        f.fainelli@gmail.com, kuba@kernel.org, edumazet@google.com,
        pabeni@redhat.com, richardcochran@gmail.com,
        netdev@vger.kernel.org, Woojung.Huh@microchip.com,
        davem@davemloft.net
Subject: Re: [RFC Patch net-next v2 3/8] net: dsa: microchip: Initial
 hardware time stamping support
Message-ID: <20221124102221.2xldwevfmjbekx43@skbuf>
References: <20221121154150.9573-1-arun.ramadoss@microchip.com>
 <20221121154150.9573-4-arun.ramadoss@microchip.com>
 <20221121231314.kabhej6ae6bl3qtj@skbuf>
 <298f4117872301da3e4fe4fed221f51e9faab5d0.camel@microchip.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <298f4117872301da3e4fe4fed221f51e9faab5d0.camel@microchip.com>
Precedence: bulk

On Wed, Nov 23, 2022 at 01:57:47PM +0000, Arun.Ramadoss@microchip.com wrote:
> > What's your excuse which such a horrible code pattern? What will happen
> > so bad with the packet if it's flagged with a TX timestamp request in
> > KSZ_SKB_CB(skb) at the same time as REG_PTP_MSG_CONF1 is written to?
> > 
> > Also, doesn't dev->ports[port].hwts_tx_en serve as a guard against
> > flagging packets for TX timestamps when you shouldn't?
> > 
> 
> I took this configuration template routine from other driver.

Not really a good excuse. The sja1105 driver has more hardware-specific
issues to deal with, not necessarily the same as ksz.

> Can I replace above snippet with
> 
> tagger_data->hwtstamp_set_state(dev->ds, rx_on);
> ret = ksz_ptp_enable_mode(dev, rx_on);
> if (ret)
>     return ret;

Why do you need to call hwtstamp_set_state anyway?