Re: [EXTERNAL] Re: [PATCH net-next v3 1/2] net: ti: icss-iep: Add pwidth configuration for perout signal

[Malladi, Meghana]

On 3/3/2025 10:33 PM, Kory Maincent wrote:
> On Mon, 3 Mar 2025 19: 21: 23 +0530 Meghana Malladi <m-malladi@ ti. com> 
> wrote: > icss_iep_perout_enable_hw() is a common function for generating 
>  > both pps and perout signals. When enabling pps, the application needs 
>  > to only pass
> ZjQcmQRYFpfptBannerStart
> This message was sent from outside of Texas Instruments.
> Do not click links or open attachments unless you recognize the source 
> of this email and know the content is safe.
> Report Suspicious
> <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/G3vK! 
> uVdqXRfP1m17KmZFPNPjnPB9kIuFmkbGwUjWeOt4PhpPuyUdhbWQXCWjPIg3CE7zH4vA7aR5lJOdoby6lh8SIqLqdFhb$>
> ZjQcmQRYFpfptBannerEnd
>
> On Mon, 3 Mar 2025 19:21:23 +0530
> Meghana Malladi <m-malladi@xxxxxx> wrote:
>
> > icss_iep_perout_enable_hw() is a common function for generating
> > both pps and perout signals. When enabling pps, the application needs
> > to only pass enable/disable argument, whereas for perout it supports
> > different flags to configure the signal.
> >
> > But icss_iep_perout_enable_hw() function is missing to hook the
> > configuration params passed by the app, causing perout to behave
> > same a pps (except being able to configure the period). As duty cycle
> > is also one feature which can configured for perout, incorporate this
> > in the function to get the expected signal.
>
> ...
>
> > IEP_SYNC_CTRL_SYNC_EN); @@ -474,7 +484,38 @@ static int
> > icss_iep_perout_enable_hw(struct icss_iep *iep, static int
> > icss_iep_perout_enable(struct icss_iep *iep, struct ptp_perout_request *req,
> > int on) {
> > -	return -EOPNOTSUPP;
> > +	int ret = 0;
> > +
> > +	mutex_lock(&iep->ptp_clk_mutex);
> > +
> > +	/* Reject requests with unsupported flags */
> > +	if (req->flags & ~PTP_PEROUT_DUTY_CYCLE) {
> > +		ret = -EOPNOTSUPP;
> > +		goto exit;
> > +	}
>
> The flags check does not need to be in the mutex lock.
> With this change:
> Reviewed-by: Kory Maincent <kory.maincent@xxxxxxxxxxx>

Yes agreed, will move it outside the mutex lock. Thanks.

> > +	if (iep->pps_enabled) {
> > +		ret = -EBUSY;
> > +		goto exit;
> > +	}
> > +
> > +	if (iep->perout_enabled == !!on)
> > +		goto exit;
> > +
> > +	/* Set default "on" time (1ms) for the signal if not passed by the
> > app */
> > +	if (!(req->flags & PTP_PEROUT_DUTY_CYCLE)) {
> > +		req->on.sec = 0;
> > +		req->on.nsec = NSEC_PER_MSEC;
> > +	}
> > +
> > +	ret = icss_iep_perout_enable_hw(iep, req, on);
> > +	if (!ret)
> > +		iep->perout_enabled = !!on;
> > +
> > +exit:
> > +	mutex_unlock(&iep->ptp_clk_mutex);
> > +
> > +	return ret;
> >  }
> >  
> >  static void icss_iep_cap_cmp_work(struct work_struct *work)
> > @@ -553,6 +594,8 @@ static int icss_iep_pps_enable(struct icss_iep *iep, int
> > on) rq.perout.period.nsec = 0;
> >  		rq.perout.start.sec = ts.tv_sec + 2;
> >  		rq.perout.start.nsec = 0;
> > +		rq.perout.on.sec = 0;
> > +		rq.perout.on.nsec = NSEC_PER_MSEC;
> >  		ret = icss_iep_perout_enable_hw(iep, &rq.perout, on);
> >  	} else {
> >  		ret = icss_iep_perout_enable_hw(iep, &rq.perout, on);
>
>
>
> --
> Köry Maincent, Bootlin
> Embedded Linux and kernel engineering
> https://urldefense.com/v3/__https://bootlin.com__;!!G3vK! 
> TJ19I0FAUivlehxg6fb4ka96Q2RiJDOZNyHXmEeRdYIPu4Cthp- 
> iiGCHOwcUnPvn8Ek_htiuIef9PMEWGUfBEA$ <https://urldefense.com/v3/__https://bootlin.com__;!!G3vK!TJ19I0FAUivlehxg6fb4ka96Q2RiJDOZNyHXmEeRdYIPu4Cthp-iiGCHOwcUnPvn8Ek_htiuIef9PMEWGUfBEA$>