Received-SPF: pass (google.com: domain of linux-kernel+bounces-177496-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Mon, 13 May 2024 14:20:09 +0300
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: lakshmi.sowjanya.d@intel.com
Cc: tglx@linutronix.de, jstultz@google.com, giometti@enneenne.com,
	corbet@lwn.net, linux-kernel@vger.kernel.org, x86@kernel.org,
	netdev@vger.kernel.org, linux-doc@vger.kernel.org,
	intel-wired-lan@lists.osuosl.org, eddie.dong@intel.com,
	christopher.s.hall@intel.com, jesse.brandeburg@intel.com,
	davem@davemloft.net, alexandre.torgue@foss.st.com,
	joabreu@synopsys.com, mcoquelin.stm32@gmail.com, perex@perex.cz,
	linux-sound@vger.kernel.org, anthony.l.nguyen@intel.com,
	peter.hilber@opensynergy.com, pandith.n@intel.com,
	subramanian.mohan@intel.com, thejesh.reddy.t.r@intel.com
Subject: Re: [PATCH v8 10/12] pps: generators: Add PPS Generator TIO Driver
Message-ID: <ZkH3aUDaFMR-8Mlo@smile.fi.intel.com>
References: <20240513103813.5666-1-lakshmi.sowjanya.d@intel.com>
 <20240513103813.5666-11-lakshmi.sowjanya.d@intel.com>
 <ZkH3GP2b9WTz9W3W@smile.fi.intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZkH3GP2b9WTz9W3W@smile.fi.intel.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

On Mon, May 13, 2024 at 02:18:49PM +0300, Andy Shevchenko wrote:
> On Mon, May 13, 2024 at 04:08:11PM +0530, lakshmi.sowjanya.d@intel.com wrote:

> > +	pps_tio_disable(tio);
> 
> This...

> > +	tio->enabled = false;
> 
> ...and this should go together, which makes me look at the enabled flag over
> the code and it seems there are a few places where you missed to sync it with
> the reality.
> 
> I would think of something like this:
> 
> 	pps_tio_direction_output() ==> true
> 	pps_tio_disable(tio) ==> false
> 
> where "==> X" means assignment of enabled flag.
> 
> And perhaps this:
> 
> 	tio->enabled = pps_generate_next_pulse(tio, expires + SAFE_TIME_NS);
> 	if (!tio->enabled)
> 		...
> 
> But the above is just thinking out loudly, you may find the better approach(es).

You might need to introduce pps_tio_enable() counterpart, in such case it would
be more natural to have enabled be assigned accordingly.

-- 
With Best Regards,
Andy Shevchenko