Messages in this thread | | | From | <> | Subject | Re: [Patch net-next v1 11/12] net: dsa: microchip: ptp: add periodic output signal | Date | Wed, 30 Nov 2022 04:41:37 +0000 |
| |
Hi Pavan,
On Tue, 2022-11-29 at 14:23 +0530, Pavan Chebbi wrote: > On Mon, Nov 28, 2022 at 4:05 PM Arun Ramadoss > <arun.ramadoss@microchip.com> wrote: > > > +static int ksz_ptp_enable(struct ptp_clock_info *ptp, > > + struct ptp_clock_request *req, int on) > > +{ > > + struct ksz_ptp_data *ptp_data = ptp_caps_to_data(ptp); > > + struct ksz_device *dev = ptp_data_to_ksz_dev(ptp_data); > > + struct ptp_perout_request *request = &req->perout; > > + int ret; > > + > > + switch (req->type) { > > + case PTP_CLK_REQ_PEROUT: > > + if (request->index > ptp->n_per_out) > > + return -EINVAL; > > Should be -EOPNOTSUPP ? I see some other places where -EOPNOTSUPP is > more appropriate.
I got a offline comment like This check is probably redundant (already checked in period_store() and ptp_ioctl()). I am looking into whether this check is required or already handled in upper layers. If the check is required, then I feel -EINVAL/-ERANGE should be reasonable. Because we are supporting periodic output only thing is index is out of bound. If we return -EOPNOTSUPP, it indicates we are not supporting periodic output.
> > > + > > + mutex_lock(&ptp_data->lock); > > + ret = ksz_ptp_enable_perout(dev, request, on); > > + mutex_unlock(&ptp_data->lock); > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + return ret; > > +} > > + > > /* Function is pointer to the do_aux_work in the ptp_clock > > capability */ > > static long ksz_ptp_do_aux_work(struct ptp_clock_info *ptp) > > { > > @@ -508,6 +823,8 @@ static const struct ptp_clock_info ksz_ptp_caps > > = { > > .adjfine = ksz_ptp_adjfine, > > .adjtime = ksz_ptp_adjtime, > > .do_aux_work = ksz_ptp_do_aux_work, > > + .enable = ksz_ptp_enable, > > + .n_per_out = 3, > > }; > >
| |