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:   Tue, 3 May 2022 17:11:40 -0700
From:   Jakub Kicinski <kuba@kernel.org>
To:     Min Li <min.li.xe@renesas.com>
Cc:     richardcochran@gmail.com, lee.jones@linaro.org,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH net v3 1/2] ptp: ptp_clockmatrix: Add PTP_CLK_REQ_EXTTS
 support
Message-ID: <20220503171140.27dbf8cf@kernel.org>
In-Reply-To: <1651518530-25128-1-git-send-email-min.li.xe@renesas.com>
References: <1651518530-25128-1-git-send-email-min.li.xe@renesas.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Mon,  2 May 2022 15:08:49 -0400 Min Li wrote:
> Use TOD_READ_SECONDARY for extts to keep TOD_READ_PRIMARY
> for gettime and settime exclusively
> 
> Signed-off-by: Min Li <min.li.xe@renesas.com>
> Acked-by: Richard Cochran <richardcochran@gmail.com>

Since this is a new feature please tag the next version as 
[PATCH net-next v4]. Bug fixes get tagged with [PATCH net]
new features, refactoring etc with [PATCH net-next].

> +	err = idtcm_write(idtcm, channel->tod_read_secondary, tod_read_cmd,
> +			  &val, sizeof(val));
> +
> +	if (err)

Please remove the empty lines between calling a function and checking
if it returned an error (only in the new code you're adding in this
patch).

> +		dev_err(idtcm->dev, "%s: err = %d", __func__, err);
>  
> -	err = idtcm_write(idtcm, channel->tod_read_primary,
> -			  tod_read_cmd, &val, sizeof(val));
>  	return err;
>  }
>  
> -static int idtcm_enable_extts(struct idtcm_channel *channel, u8 todn, u8 ref,
> -			      bool enable)
> +static bool is_single_shot(u8 mask)
>  {
> -	struct idtcm *idtcm = channel->idtcm;
> -	u8 old_mask = idtcm->extts_mask;
> -	u8 mask = 1 << todn;
> +	/* Treat single bit ToD masks as continuous trigger */
> +	if ((mask == 1) || (mask == 2) || (mask == 4) || (mask == 8))
> +		return false;
> +	else
> +		return true;

This function is better written as:

	/* Treat single bit ToD masks as continuous trigger */
	return mask <= 8 && is_power_of_2(mask);

> +}

> +static int _idtcm_gettime_triggered(struct idtcm_channel *channel,
> +				    struct timespec64 *ts)
> +{
> +	struct idtcm *idtcm = channel->idtcm;
> +	u16 tod_read_cmd = IDTCM_FW_REG(idtcm->fw_ver, V520, TOD_READ_SECONDARY_CMD);
> +	u8 buf[TOD_BYTE_COUNT];
> +	u8 trigger;
> +	int err;
> +
> +	err = idtcm_read(idtcm, channel->tod_read_secondary,
> +			 tod_read_cmd, &trigger, sizeof(trigger));
> +	if (err)
> +		return err;
> +
> +	if (trigger & TOD_READ_TRIGGER_MASK)
> +		return -EBUSY;
> +
> +	err = idtcm_read(idtcm, channel->tod_read_secondary,
> +			 TOD_READ_SECONDARY_BASE, buf, sizeof(buf));
> +
> +	if (err)
> +		return err;
> +
> +	err = char_array_to_timespec(buf, sizeof(buf), ts);
> +
> +	return err;

Please return directly:

	return char_array_...

> +}


>  static int _idtcm_gettime_immediate(struct idtcm_channel *channel,
>  				    struct timespec64 *ts)
>  {
>  	struct idtcm *idtcm = channel->idtcm;
> -	u8 extts_mask = 0;
> +
> +	u16 tod_read_cmd = IDTCM_FW_REG(idtcm->fw_ver, V520, TOD_READ_PRIMARY_CMD);
> +	u8 val = (SCSR_TOD_READ_TRIG_SEL_IMMEDIATE << TOD_READ_TRIGGER_SHIFT);
>  	int err;
>  
> -	/* Disable extts */
> -	if (idtcm->extts_mask) {
> -		extts_mask = idtcm_enable_extts_mask(channel, idtcm->extts_mask,
> -						     false);
> -	}
> +	err = idtcm_write(idtcm, channel->tod_read_primary,
> +			  tod_read_cmd, &val, sizeof(val));
>  
> -	err = _idtcm_set_scsr_read_trig(channel,
> -					SCSR_TOD_READ_TRIG_SEL_IMMEDIATE, 0);
> -	if (err == 0)
> -		err = _idtcm_gettime(channel, ts, 10);
> +	if (err)
> +		return err;
>  
> -	/* Re-enable extts */
> -	if (extts_mask)
> -		idtcm_enable_extts_mask(channel, extts_mask, true);
> +	err = _idtcm_gettime(channel, ts, 10);
>  
>  	return err;

Same here

	return _idtcm_gettime(...

>  }

> @@ -2420,10 +2502,11 @@ static int idtcm_remove(struct platform_device *pdev)
>  {
>  	struct idtcm *idtcm = platform_get_drvdata(pdev);
>  
> -	ptp_clock_unregister_all(idtcm);
> -
> +	idtcm->extts_mask = 0;
>  	cancel_delayed_work_sync(&idtcm->extts_work);
>  
> +	ptp_clock_unregister_all(idtcm);

Why is the order of unregistering the clock and canceling the work
changed? There is no locking around this function so seems like 
the work can get scheduled right after the call to
cancel_delayed_work_sync(), anyway.

>  	return 0;
>  }