| Message ID | 20230927092435.1565336-4-arkadiusz.kubalewski@intel.com |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | dpll: add phase-offset and phase-adjust | expand |
On 27/09/2023 10:24, Arkadiusz Kubalewski wrote: > Add callback op (get) for pin-dpll phase-offset measurment. > Add callback ops (get/set) for pin signal phase adjustment. > Add min and max phase adjustment values to pin proprties. > Invoke get callbacks when filling up the pin details to provide user > with phase related attribute values. > Invoke phase-adjust set callback when phase-adjust value is provided for > pin-set request. > > Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> [...] > +static int > +dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr *phase_adj_attr, > + struct netlink_ext_ack *extack) > +{ > + struct dpll_pin_ref *ref; > + unsigned long i; > + s32 phase_adj; > + int ret; > + > + phase_adj = nla_get_s32(phase_adj_attr); > + if (phase_adj > pin->prop->phase_range.max || > + phase_adj < pin->prop->phase_range.min) { > + NL_SET_ERR_MSG(extack, "phase adjust value not supported"); > + return -EINVAL; > + } > + xa_for_each(&pin->dpll_refs, i, ref) { > + const struct dpll_pin_ops *ops = dpll_pin_ops(ref); > + struct dpll_device *dpll = ref->dpll; > + > + if (!ops->phase_adjust_set) > + return -EOPNOTSUPP; I'm thinking about this part. We can potentially have dpll devices with different expectations on phase adjustments, right? And if one of them won't be able to adjust phase (or will fail in the next line), then netlink will return EOPNOTSUPP while _some_ of the devices will be adjusted. Doesn't look great. Can we think about different way to apply the change? > + ret = ops->phase_adjust_set(pin, > + dpll_pin_on_dpll_priv(dpll, pin), > + dpll, dpll_priv(dpll), phase_adj, > + extack); > + if (ret) > + return ret; > + } > + __dpll_pin_change_ntf(pin); > + > + return 0; > +} > +
>From: Vadim Fedorenko <vadim.fedorenko@linux.dev> >Sent: Wednesday, September 27, 2023 8:09 PM > >On 27/09/2023 10:24, Arkadiusz Kubalewski wrote: >> Add callback op (get) for pin-dpll phase-offset measurment. >> Add callback ops (get/set) for pin signal phase adjustment. >> Add min and max phase adjustment values to pin proprties. >> Invoke get callbacks when filling up the pin details to provide user >> with phase related attribute values. >> Invoke phase-adjust set callback when phase-adjust value is provided for >> pin-set request. >> >> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> > >[...] > >> +static int >> +dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr >> *phase_adj_attr, >> + struct netlink_ext_ack *extack) >> +{ >> + struct dpll_pin_ref *ref; >> + unsigned long i; >> + s32 phase_adj; >> + int ret; >> + >> + phase_adj = nla_get_s32(phase_adj_attr); >> + if (phase_adj > pin->prop->phase_range.max || >> + phase_adj < pin->prop->phase_range.min) { >> + NL_SET_ERR_MSG(extack, "phase adjust value not supported"); >> + return -EINVAL; >> + } >> + xa_for_each(&pin->dpll_refs, i, ref) { >> + const struct dpll_pin_ops *ops = dpll_pin_ops(ref); >> + struct dpll_device *dpll = ref->dpll; >> + >> + if (!ops->phase_adjust_set) >> + return -EOPNOTSUPP; > >I'm thinking about this part. We can potentially have dpll devices with >different expectations on phase adjustments, right? And if one of them >won't be able to adjust phase (or will fail in the next line), then >netlink will return EOPNOTSUPP while _some_ of the devices will be >adjusted. Doesn't look great. Can we think about different way to apply >the change? > Well makes sense to me. Does following makes sense as a fix? We would call op for all devices which has been provided with the op. If device has no op -> add extack error, continue If device fails to set -> add extack error, continue Function always returns 0. Thank you! Arkadiusz > >> + ret = ops->phase_adjust_set(pin, >> + dpll_pin_on_dpll_priv(dpll, pin), >> + dpll, dpll_priv(dpll), phase_adj, >> + extack); >> + if (ret) >> + return ret; >> + } >> + __dpll_pin_change_ntf(pin); >> + >> + return 0; >> +} >> +
Mon, Oct 02, 2023 at 04:32:30PM CEST, arkadiusz.kubalewski@intel.com wrote: >>From: Vadim Fedorenko <vadim.fedorenko@linux.dev> >>Sent: Wednesday, September 27, 2023 8:09 PM >> >>On 27/09/2023 10:24, Arkadiusz Kubalewski wrote: >>> Add callback op (get) for pin-dpll phase-offset measurment. >>> Add callback ops (get/set) for pin signal phase adjustment. >>> Add min and max phase adjustment values to pin proprties. >>> Invoke get callbacks when filling up the pin details to provide user >>> with phase related attribute values. >>> Invoke phase-adjust set callback when phase-adjust value is provided for >>> pin-set request. >>> >>> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >> >>[...] >> >>> +static int >>> +dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr >>> *phase_adj_attr, >>> + struct netlink_ext_ack *extack) >>> +{ >>> + struct dpll_pin_ref *ref; >>> + unsigned long i; >>> + s32 phase_adj; >>> + int ret; >>> + >>> + phase_adj = nla_get_s32(phase_adj_attr); >>> + if (phase_adj > pin->prop->phase_range.max || >>> + phase_adj < pin->prop->phase_range.min) { >>> + NL_SET_ERR_MSG(extack, "phase adjust value not supported"); >>> + return -EINVAL; >>> + } >>> + xa_for_each(&pin->dpll_refs, i, ref) { >>> + const struct dpll_pin_ops *ops = dpll_pin_ops(ref); >>> + struct dpll_device *dpll = ref->dpll; >>> + >>> + if (!ops->phase_adjust_set) >>> + return -EOPNOTSUPP; >> >>I'm thinking about this part. We can potentially have dpll devices with >>different expectations on phase adjustments, right? And if one of them >>won't be able to adjust phase (or will fail in the next line), then >>netlink will return EOPNOTSUPP while _some_ of the devices will be >>adjusted. Doesn't look great. Can we think about different way to apply >>the change? >> > >Well makes sense to me. > >Does following makes sense as a fix? >We would call op for all devices which has been provided with the op. >If device has no op -> add extack error, continue Is it real to expect some of the device support this and others don't? Is it true for ice? If not, I would got for all-or-nothing here. >If device fails to set -> add extack error, continue >Function always returns 0. > >Thank you! >Arkadiusz > >> >>> + ret = ops->phase_adjust_set(pin, >>> + dpll_pin_on_dpll_priv(dpll, pin), >>> + dpll, dpll_priv(dpll), phase_adj, >>> + extack); >>> + if (ret) >>> + return ret; >>> + } >>> + __dpll_pin_change_ntf(pin); >>> + >>> + return 0; >>> +} >>> +
On 02/10/2023 16:04, Jiri Pirko wrote: > Mon, Oct 02, 2023 at 04:32:30PM CEST, arkadiusz.kubalewski@intel.com wrote: >>> From: Vadim Fedorenko <vadim.fedorenko@linux.dev> >>> Sent: Wednesday, September 27, 2023 8:09 PM >>> >>> On 27/09/2023 10:24, Arkadiusz Kubalewski wrote: >>>> Add callback op (get) for pin-dpll phase-offset measurment. >>>> Add callback ops (get/set) for pin signal phase adjustment. >>>> Add min and max phase adjustment values to pin proprties. >>>> Invoke get callbacks when filling up the pin details to provide user >>>> with phase related attribute values. >>>> Invoke phase-adjust set callback when phase-adjust value is provided for >>>> pin-set request. >>>> >>>> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >>> >>> [...] >>> >>>> +static int >>>> +dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr >>>> *phase_adj_attr, >>>> + struct netlink_ext_ack *extack) >>>> +{ >>>> + struct dpll_pin_ref *ref; >>>> + unsigned long i; >>>> + s32 phase_adj; >>>> + int ret; >>>> + >>>> + phase_adj = nla_get_s32(phase_adj_attr); >>>> + if (phase_adj > pin->prop->phase_range.max || >>>> + phase_adj < pin->prop->phase_range.min) { >>>> + NL_SET_ERR_MSG(extack, "phase adjust value not supported"); >>>> + return -EINVAL; >>>> + } >>>> + xa_for_each(&pin->dpll_refs, i, ref) { >>>> + const struct dpll_pin_ops *ops = dpll_pin_ops(ref); >>>> + struct dpll_device *dpll = ref->dpll; >>>> + >>>> + if (!ops->phase_adjust_set) >>>> + return -EOPNOTSUPP; >>> >>> I'm thinking about this part. We can potentially have dpll devices with >>> different expectations on phase adjustments, right? And if one of them >>> won't be able to adjust phase (or will fail in the next line), then >>> netlink will return EOPNOTSUPP while _some_ of the devices will be >>> adjusted. Doesn't look great. Can we think about different way to apply >>> the change? >>> >> >> Well makes sense to me. >> >> Does following makes sense as a fix? >> We would call op for all devices which has been provided with the op. >> If device has no op -> add extack error, continue > > Is it real to expect some of the device support this and others don't? > Is it true for ice? > If not, I would got for all-or-nothing here. > But nothing blocks vendors to provide such configuration. Should we rollback the configuration? Otherwise we can easily make it inconsistent. I'm more thinking of checking if all the devices returned error (or absence of operation callback) and then return error instead of 0 with extack filled in. > >> If device fails to set -> add extack error, continue >> Function always returns 0. >> >> Thank you! >> Arkadiusz >> >>> >>>> + ret = ops->phase_adjust_set(pin, >>>> + dpll_pin_on_dpll_priv(dpll, pin), >>>> + dpll, dpll_priv(dpll), phase_adj, >>>> + extack); >>>> + if (ret) >>>> + return ret; >>>> + } >>>> + __dpll_pin_change_ntf(pin); >>>> + >>>> + return 0; >>>> +} >>>> +
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Monday, October 2, 2023 5:04 PM > >Mon, Oct 02, 2023 at 04:32:30PM CEST, arkadiusz.kubalewski@intel.com wrote: >>>From: Vadim Fedorenko <vadim.fedorenko@linux.dev> >>>Sent: Wednesday, September 27, 2023 8:09 PM >>> >>>On 27/09/2023 10:24, Arkadiusz Kubalewski wrote: >>>> Add callback op (get) for pin-dpll phase-offset measurment. >>>> Add callback ops (get/set) for pin signal phase adjustment. >>>> Add min and max phase adjustment values to pin proprties. >>>> Invoke get callbacks when filling up the pin details to provide user >>>> with phase related attribute values. >>>> Invoke phase-adjust set callback when phase-adjust value is provided >>>> for >>>> pin-set request. >>>> >>>> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >>> >>>[...] >>> >>>> +static int >>>> +dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr >>>> *phase_adj_attr, >>>> + struct netlink_ext_ack *extack) >>>> +{ >>>> + struct dpll_pin_ref *ref; >>>> + unsigned long i; >>>> + s32 phase_adj; >>>> + int ret; >>>> + >>>> + phase_adj = nla_get_s32(phase_adj_attr); >>>> + if (phase_adj > pin->prop->phase_range.max || >>>> + phase_adj < pin->prop->phase_range.min) { >>>> + NL_SET_ERR_MSG(extack, "phase adjust value not supported"); >>>> + return -EINVAL; >>>> + } >>>> + xa_for_each(&pin->dpll_refs, i, ref) { >>>> + const struct dpll_pin_ops *ops = dpll_pin_ops(ref); >>>> + struct dpll_device *dpll = ref->dpll; >>>> + >>>> + if (!ops->phase_adjust_set) >>>> + return -EOPNOTSUPP; >>> >>>I'm thinking about this part. We can potentially have dpll devices with >>>different expectations on phase adjustments, right? And if one of them >>>won't be able to adjust phase (or will fail in the next line), then >>>netlink will return EOPNOTSUPP while _some_ of the devices will be >>>adjusted. Doesn't look great. Can we think about different way to apply >>>the change? >>> >> >>Well makes sense to me. >> >>Does following makes sense as a fix? >>We would call op for all devices which has been provided with the op. >>If device has no op -> add extack error, continue > >Is it real to expect some of the device support this and others don't? >Is it true for ice? >If not, I would got for all-or-nothing here. > Let's step back a bit. The op itself is introduced as per pin-dpll tuple.. did this intentionally, to inform each dpll that the offset has been changed - in case dplls are controlled by separated driver/firmware instances but still sharing the pin. Same way a pin frequency is being set, from user perspective on a pin, but callback is called for each dpll the pin was registered with. Whatever we do here, it shall be probably done for frequency_set() callback as well. The answers: So far I don't know the device that might do it this way, it rather supports phase_adjust or not. In theory we allow such behavior to be implemented, i.e. pin is registered with 2 dplls, one has the callback, second not. Current hardware of ice sets phase offset for a pin no matter on which dpll device callback was invoked. "all-or-nothing" - do you mean to check all callback returns and then decide if it was successful? Thank you! Arkadiusz > >>If device fails to set -> add extack error, continue >>Function always returns 0. >> >>Thank you! >>Arkadiusz >> >>> >>>> + ret = ops->phase_adjust_set(pin, >>>> + dpll_pin_on_dpll_priv(dpll, pin), >>>> + dpll, dpll_priv(dpll), phase_adj, >>>> + extack); >>>> + if (ret) >>>> + return ret; >>>> + } >>>> + __dpll_pin_change_ntf(pin); >>>> + >>>> + return 0; >>>> +} >>>> +
>From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of >Vadim Fedorenko >Sent: Monday, October 2, 2023 5:09 PM > >On 02/10/2023 16:04, Jiri Pirko wrote: >> Mon, Oct 02, 2023 at 04:32:30PM CEST, arkadiusz.kubalewski@intel.com >> wrote: >>>> From: Vadim Fedorenko <vadim.fedorenko@linux.dev> >>>> Sent: Wednesday, September 27, 2023 8:09 PM >>>> >>>> On 27/09/2023 10:24, Arkadiusz Kubalewski wrote: >>>>> Add callback op (get) for pin-dpll phase-offset measurment. >>>>> Add callback ops (get/set) for pin signal phase adjustment. >>>>> Add min and max phase adjustment values to pin proprties. >>>>> Invoke get callbacks when filling up the pin details to provide user >>>>> with phase related attribute values. >>>>> Invoke phase-adjust set callback when phase-adjust value is provided >>>>> for >>>>> pin-set request. >>>>> >>>>> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >>>> >>>> [...] >>>> >>>>> +static int >>>>> +dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr >>>>> *phase_adj_attr, >>>>> + struct netlink_ext_ack *extack) >>>>> +{ >>>>> + struct dpll_pin_ref *ref; >>>>> + unsigned long i; >>>>> + s32 phase_adj; >>>>> + int ret; >>>>> + >>>>> + phase_adj = nla_get_s32(phase_adj_attr); >>>>> + if (phase_adj > pin->prop->phase_range.max || >>>>> + phase_adj < pin->prop->phase_range.min) { >>>>> + NL_SET_ERR_MSG(extack, "phase adjust value not supported"); >>>>> + return -EINVAL; >>>>> + } >>>>> + xa_for_each(&pin->dpll_refs, i, ref) { >>>>> + const struct dpll_pin_ops *ops = dpll_pin_ops(ref); >>>>> + struct dpll_device *dpll = ref->dpll; >>>>> + >>>>> + if (!ops->phase_adjust_set) >>>>> + return -EOPNOTSUPP; >>>> >>>> I'm thinking about this part. We can potentially have dpll devices with >>>> different expectations on phase adjustments, right? And if one of them >>>> won't be able to adjust phase (or will fail in the next line), then >>>> netlink will return EOPNOTSUPP while _some_ of the devices will be >>>> adjusted. Doesn't look great. Can we think about different way to apply >>>> the change? >>>> >>> >>> Well makes sense to me. >>> >>> Does following makes sense as a fix? >>> We would call op for all devices which has been provided with the op. >>> If device has no op -> add extack error, continue >> >> Is it real to expect some of the device support this and others don't? >> Is it true for ice? >> If not, I would got for all-or-nothing here. >> > >But nothing blocks vendors to provide such configuration. Should we >rollback the configuration? Otherwise we can easily make it >inconsistent. Good point, in such case rollback might be required. > >I'm more thinking of checking if all the devices returned error (or >absence of operation callback) and then return error instead of 0 with >extack filled in. > Well, what if different devices would return different errors? In general we would have to keep track of the error values returned in such case.. Assuming one is different than the other - still need to error extack them out? I guess it would be easier to return common error if there were only failures and let the driver fill the errors on extack, smt like: int miss_cb_num = 0, dev_num = 0, err_num; xa_for_each(&pin->dpll_refs, i, ref) { const struct dpll_pin_ops *ops = dpll_pin_ops(ref); struct dpll_device *dpll = ref->dpll; dev_num++; if (!ops->phase_adjust_set) { miss_cb_num++; continue; } ret = ops->phase_adjust_set(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll, dpll_priv(dpll), phase_adj, extack); if (ret) err_num++; } if (dev_num == miss_cb_num) return -EOPNOTSUPP; if (dev_num == err_num) return -EINVAL; __dpll_pin_change_ntf(pin); return 0; ?? Thank you! Arkadiusz >> >>> If device fails to set -> add extack error, continue >>> Function always returns 0. >>> >>> Thank you! >>> Arkadiusz >>> >>>> >>>>> + ret = ops->phase_adjust_set(pin, >>>>> + dpll_pin_on_dpll_priv(dpll, pin), >>>>> + dpll, dpll_priv(dpll), phase_adj, >>>>> + extack); >>>>> + if (ret) >>>>> + return ret; >>>>> + } >>>>> + __dpll_pin_change_ntf(pin); >>>>> + >>>>> + return 0; >>>>> +} >>>>> + > >_______________________________________________ >Intel-wired-lan mailing list >Intel-wired-lan@osuosl.org >https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Tue, Oct 03, 2023 at 01:10:39AM CEST, arkadiusz.kubalewski@intel.com wrote: >>From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of >>Vadim Fedorenko >>Sent: Monday, October 2, 2023 5:09 PM >> >>On 02/10/2023 16:04, Jiri Pirko wrote: >>> Mon, Oct 02, 2023 at 04:32:30PM CEST, arkadiusz.kubalewski@intel.com >>> wrote: >>>>> From: Vadim Fedorenko <vadim.fedorenko@linux.dev> >>>>> Sent: Wednesday, September 27, 2023 8:09 PM >>>>> >>>>> On 27/09/2023 10:24, Arkadiusz Kubalewski wrote: >>>>>> Add callback op (get) for pin-dpll phase-offset measurment. >>>>>> Add callback ops (get/set) for pin signal phase adjustment. >>>>>> Add min and max phase adjustment values to pin proprties. >>>>>> Invoke get callbacks when filling up the pin details to provide user >>>>>> with phase related attribute values. >>>>>> Invoke phase-adjust set callback when phase-adjust value is provided >>>>>> for >>>>>> pin-set request. >>>>>> >>>>>> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >>>>> >>>>> [...] >>>>> >>>>>> +static int >>>>>> +dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr >>>>>> *phase_adj_attr, >>>>>> + struct netlink_ext_ack *extack) >>>>>> +{ >>>>>> + struct dpll_pin_ref *ref; >>>>>> + unsigned long i; >>>>>> + s32 phase_adj; >>>>>> + int ret; >>>>>> + >>>>>> + phase_adj = nla_get_s32(phase_adj_attr); >>>>>> + if (phase_adj > pin->prop->phase_range.max || >>>>>> + phase_adj < pin->prop->phase_range.min) { >>>>>> + NL_SET_ERR_MSG(extack, "phase adjust value not supported"); >>>>>> + return -EINVAL; >>>>>> + } >>>>>> + xa_for_each(&pin->dpll_refs, i, ref) { >>>>>> + const struct dpll_pin_ops *ops = dpll_pin_ops(ref); >>>>>> + struct dpll_device *dpll = ref->dpll; >>>>>> + >>>>>> + if (!ops->phase_adjust_set) >>>>>> + return -EOPNOTSUPP; >>>>> >>>>> I'm thinking about this part. We can potentially have dpll devices with >>>>> different expectations on phase adjustments, right? And if one of them >>>>> won't be able to adjust phase (or will fail in the next line), then >>>>> netlink will return EOPNOTSUPP while _some_ of the devices will be >>>>> adjusted. Doesn't look great. Can we think about different way to apply >>>>> the change? >>>>> >>>> >>>> Well makes sense to me. >>>> >>>> Does following makes sense as a fix? >>>> We would call op for all devices which has been provided with the op. >>>> If device has no op -> add extack error, continue >>> >>> Is it real to expect some of the device support this and others don't? >>> Is it true for ice? >>> If not, I would got for all-or-nothing here. >>> >> >>But nothing blocks vendors to provide such configuration. Should we >>rollback the configuration? Otherwise we can easily make it >>inconsistent. > >Good point, in such case rollback might be required. > >> >>I'm more thinking of checking if all the devices returned error (or >>absence of operation callback) and then return error instead of 0 with >>extack filled in. >> > >Well, what if different devices would return different errors? >In general we would have to keep track of the error values returned in >such case.. Assuming one is different than the other - still need to error >extack them out? I guess it would be easier to return common error if there In this case, it is common to return the first error hit and bail out, not trying the rest. >were only failures and let the driver fill the errors on extack, smt like: > > int miss_cb_num = 0, dev_num = 0, err_num; > > xa_for_each(&pin->dpll_refs, i, ref) { > const struct dpll_pin_ops *ops = dpll_pin_ops(ref); > struct dpll_device *dpll = ref->dpll; > > dev_num++; > if (!ops->phase_adjust_set) { > miss_cb_num++; > continue; > } > ret = ops->phase_adjust_set(pin, > dpll_pin_on_dpll_priv(dpll, pin), > dpll, dpll_priv(dpll), phase_adj, > extack); > if (ret) > err_num++; > } > if (dev_num == miss_cb_num) > return -EOPNOTSUPP; > if (dev_num == err_num) > return -EINVAL; > __dpll_pin_change_ntf(pin); > return 0; > >?? > >Thank you! >Arkadiusz > >>> >>>> If device fails to set -> add extack error, continue >>>> Function always returns 0. >>>> >>>> Thank you! >>>> Arkadiusz >>>> >>>>> >>>>>> + ret = ops->phase_adjust_set(pin, >>>>>> + dpll_pin_on_dpll_priv(dpll, pin), >>>>>> + dpll, dpll_priv(dpll), phase_adj, >>>>>> + extack); >>>>>> + if (ret) >>>>>> + return ret; >>>>>> + } >>>>>> + __dpll_pin_change_ntf(pin); >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >> >>_______________________________________________ >>Intel-wired-lan mailing list >>Intel-wired-lan@osuosl.org >>https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Tue, Oct 03, 2023 at 01:03:00AM CEST, arkadiusz.kubalewski@intel.com wrote: >>From: Jiri Pirko <jiri@resnulli.us> >>Sent: Monday, October 2, 2023 5:04 PM >> >>Mon, Oct 02, 2023 at 04:32:30PM CEST, arkadiusz.kubalewski@intel.com wrote: >>>>From: Vadim Fedorenko <vadim.fedorenko@linux.dev> >>>>Sent: Wednesday, September 27, 2023 8:09 PM >>>> >>>>On 27/09/2023 10:24, Arkadiusz Kubalewski wrote: >>>>> Add callback op (get) for pin-dpll phase-offset measurment. >>>>> Add callback ops (get/set) for pin signal phase adjustment. >>>>> Add min and max phase adjustment values to pin proprties. >>>>> Invoke get callbacks when filling up the pin details to provide user >>>>> with phase related attribute values. >>>>> Invoke phase-adjust set callback when phase-adjust value is provided >>>>> for >>>>> pin-set request. >>>>> >>>>> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >>>> >>>>[...] >>>> >>>>> +static int >>>>> +dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr >>>>> *phase_adj_attr, >>>>> + struct netlink_ext_ack *extack) >>>>> +{ >>>>> + struct dpll_pin_ref *ref; >>>>> + unsigned long i; >>>>> + s32 phase_adj; >>>>> + int ret; >>>>> + >>>>> + phase_adj = nla_get_s32(phase_adj_attr); >>>>> + if (phase_adj > pin->prop->phase_range.max || >>>>> + phase_adj < pin->prop->phase_range.min) { >>>>> + NL_SET_ERR_MSG(extack, "phase adjust value not supported"); >>>>> + return -EINVAL; >>>>> + } >>>>> + xa_for_each(&pin->dpll_refs, i, ref) { >>>>> + const struct dpll_pin_ops *ops = dpll_pin_ops(ref); >>>>> + struct dpll_device *dpll = ref->dpll; >>>>> + >>>>> + if (!ops->phase_adjust_set) >>>>> + return -EOPNOTSUPP; >>>> >>>>I'm thinking about this part. We can potentially have dpll devices with >>>>different expectations on phase adjustments, right? And if one of them >>>>won't be able to adjust phase (or will fail in the next line), then >>>>netlink will return EOPNOTSUPP while _some_ of the devices will be >>>>adjusted. Doesn't look great. Can we think about different way to apply >>>>the change? >>>> >>> >>>Well makes sense to me. >>> >>>Does following makes sense as a fix? >>>We would call op for all devices which has been provided with the op. >>>If device has no op -> add extack error, continue >> >>Is it real to expect some of the device support this and others don't? >>Is it true for ice? >>If not, I would got for all-or-nothing here. >> > >Let's step back a bit. >The op itself is introduced as per pin-dpll tuple.. did this intentionally, >to inform each dpll that the offset has been changed - in case dplls are >controlled by separated driver/firmware instances but still sharing the pin. >Same way a pin frequency is being set, from user perspective on a pin, but >callback is called for each dpll the pin was registered with. >Whatever we do here, it shall be probably done for frequency_set() callback as >well. > >The answers: >So far I don't know the device that might do it this way, it rather supports >phase_adjust or not. In theory we allow such behavior to be implemented, i.e. >pin is registered with 2 dplls, one has the callback, second not. If there is only theoretical device like that now, implement all-or-nothing. If such theoretical device appears in real, this could be changed. The UAPI would not change, no problem. >Current hardware of ice sets phase offset for a pin no matter on which dpll >device callback was invoked. >"all-or-nothing" - do you mean to check all callback returns and then decide >if it was successful? Check if all dplls have ops and only perform the action in such case. In case one of the dplls does not have the op filled, return -EOPNOTSUPP. Regarding the successful/failed op, I think you can just return. In these cases, when user performs multiaction cmd, he should be prepared to deal with consequences if part of this cmd fails. We don't have rollback for any other multiaction cmd in dpll, I don't see why this should be treated differently. > >Thank you! >Arkadiusz > >> >>>If device fails to set -> add extack error, continue >>>Function always returns 0. >>> >>>Thank you! >>>Arkadiusz >>> >>>> >>>>> + ret = ops->phase_adjust_set(pin, >>>>> + dpll_pin_on_dpll_priv(dpll, pin), >>>>> + dpll, dpll_priv(dpll), phase_adj, >>>>> + extack); >>>>> + if (ret) >>>>> + return ret; >>>>> + } >>>>> + __dpll_pin_change_ntf(pin); >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Tuesday, October 3, 2023 8:27 AM >To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com> > >Tue, Oct 03, 2023 at 01:10:39AM CEST, arkadiusz.kubalewski@intel.com wrote: >>>From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of >>>Vadim Fedorenko >>>Sent: Monday, October 2, 2023 5:09 PM >>> >>>On 02/10/2023 16:04, Jiri Pirko wrote: >>>> Mon, Oct 02, 2023 at 04:32:30PM CEST, arkadiusz.kubalewski@intel.com >>>> wrote: >>>>>> From: Vadim Fedorenko <vadim.fedorenko@linux.dev> >>>>>> Sent: Wednesday, September 27, 2023 8:09 PM >>>>>> >>>>>> On 27/09/2023 10:24, Arkadiusz Kubalewski wrote: >>>>>>> Add callback op (get) for pin-dpll phase-offset measurment. >>>>>>> Add callback ops (get/set) for pin signal phase adjustment. >>>>>>> Add min and max phase adjustment values to pin proprties. >>>>>>> Invoke get callbacks when filling up the pin details to provide user >>>>>>> with phase related attribute values. >>>>>>> Invoke phase-adjust set callback when phase-adjust value is provided >>>>>>> for >>>>>>> pin-set request. >>>>>>> >>>>>>> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >>>>>> >>>>>> [...] >>>>>> >>>>>>> +static int >>>>>>> +dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr >>>>>>> *phase_adj_attr, >>>>>>> + struct netlink_ext_ack *extack) >>>>>>> +{ >>>>>>> + struct dpll_pin_ref *ref; >>>>>>> + unsigned long i; >>>>>>> + s32 phase_adj; >>>>>>> + int ret; >>>>>>> + >>>>>>> + phase_adj = nla_get_s32(phase_adj_attr); >>>>>>> + if (phase_adj > pin->prop->phase_range.max || >>>>>>> + phase_adj < pin->prop->phase_range.min) { >>>>>>> + NL_SET_ERR_MSG(extack, "phase adjust value not >>>>>>> supported"); >>>>>>> + return -EINVAL; >>>>>>> + } >>>>>>> + xa_for_each(&pin->dpll_refs, i, ref) { >>>>>>> + const struct dpll_pin_ops *ops = dpll_pin_ops(ref); >>>>>>> + struct dpll_device *dpll = ref->dpll; >>>>>>> + >>>>>>> + if (!ops->phase_adjust_set) >>>>>>> + return -EOPNOTSUPP; >>>>>> >>>>>> I'm thinking about this part. We can potentially have dpll devices >>>>>> with >>>>>> different expectations on phase adjustments, right? And if one of >>>>>> them >>>>>> won't be able to adjust phase (or will fail in the next line), then >>>>>> netlink will return EOPNOTSUPP while _some_ of the devices will be >>>>>> adjusted. Doesn't look great. Can we think about different way to >>>>>> apply >>>>>> the change? >>>>>> >>>>> >>>>> Well makes sense to me. >>>>> >>>>> Does following makes sense as a fix? >>>>> We would call op for all devices which has been provided with the op. >>>>> If device has no op -> add extack error, continue >>>> >>>> Is it real to expect some of the device support this and others don't? >>>> Is it true for ice? >>>> If not, I would got for all-or-nothing here. >>>> >>> >>>But nothing blocks vendors to provide such configuration. Should we >>>rollback the configuration? Otherwise we can easily make it >>>inconsistent. >> >>Good point, in such case rollback might be required. >> >>> >>>I'm more thinking of checking if all the devices returned error (or >>>absence of operation callback) and then return error instead of 0 with >>>extack filled in. >>> >> >>Well, what if different devices would return different errors? >>In general we would have to keep track of the error values returned in >>such case.. Assuming one is different than the other - still need to error >>extack them out? I guess it would be easier to return common error if >there > >In this case, it is common to return the first error hit and bail out, >not trying the rest. > OK, so now I see it like this: -> check if all device implement callback, if not return EOPNOTSUPP; -> get old phase_adjust -> if new == old, return EINVAL -> for each device: call phase_adjust_set, if fails, rollback all previous successful attempts and return the failure code ? Thank you! Arkadiusz > >>were only failures and let the driver fill the errors on extack, smt like: >> >> int miss_cb_num = 0, dev_num = 0, err_num; >> >> xa_for_each(&pin->dpll_refs, i, ref) { >> const struct dpll_pin_ops *ops = dpll_pin_ops(ref); >> struct dpll_device *dpll = ref->dpll; >> >> dev_num++; >> if (!ops->phase_adjust_set) { >> miss_cb_num++; >> continue; >> } >> ret = ops->phase_adjust_set(pin, >> dpll_pin_on_dpll_priv(dpll, pin), >> dpll, dpll_priv(dpll), phase_adj, >> extack); >> if (ret) >> err_num++; >> } >> if (dev_num == miss_cb_num) >> return -EOPNOTSUPP; >> if (dev_num == err_num) >> return -EINVAL; >> __dpll_pin_change_ntf(pin); >> return 0; >> >>?? >> >>Thank you! >>Arkadiusz >> >>>> >>>>> If device fails to set -> add extack error, continue >>>>> Function always returns 0. >>>>> >>>>> Thank you! >>>>> Arkadiusz >>>>> >>>>>> >>>>>>> + ret = ops->phase_adjust_set(pin, >>>>>>> + dpll_pin_on_dpll_priv(dpll, pin), >>>>>>> + dpll, dpll_priv(dpll), phase_adj, >>>>>>> + extack); >>>>>>> + if (ret) >>>>>>> + return ret; >>>>>>> + } >>>>>>> + __dpll_pin_change_ntf(pin); >>>>>>> + >>>>>>> + return 0; >>>>>>> +} >>>>>>> + >>> >>>_______________________________________________ >>>Intel-wired-lan mailing list >>>Intel-wired-lan@osuosl.org >>>https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Tuesday, October 3, 2023 8:32 AM > >Tue, Oct 03, 2023 at 01:03:00AM CEST, arkadiusz.kubalewski@intel.com wrote: >>>From: Jiri Pirko <jiri@resnulli.us> >>>Sent: Monday, October 2, 2023 5:04 PM >>> >>>Mon, Oct 02, 2023 at 04:32:30PM CEST, arkadiusz.kubalewski@intel.com >>>wrote: >>>>>From: Vadim Fedorenko <vadim.fedorenko@linux.dev> >>>>>Sent: Wednesday, September 27, 2023 8:09 PM >>>>> >>>>>On 27/09/2023 10:24, Arkadiusz Kubalewski wrote: >>>>>> Add callback op (get) for pin-dpll phase-offset measurment. >>>>>> Add callback ops (get/set) for pin signal phase adjustment. >>>>>> Add min and max phase adjustment values to pin proprties. >>>>>> Invoke get callbacks when filling up the pin details to provide user >>>>>> with phase related attribute values. >>>>>> Invoke phase-adjust set callback when phase-adjust value is provided >>>>>> for >>>>>> pin-set request. >>>>>> >>>>>> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >>>>> >>>>>[...] >>>>> >>>>>> +static int >>>>>> +dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr >>>>>> *phase_adj_attr, >>>>>> + struct netlink_ext_ack *extack) >>>>>> +{ >>>>>> + struct dpll_pin_ref *ref; >>>>>> + unsigned long i; >>>>>> + s32 phase_adj; >>>>>> + int ret; >>>>>> + >>>>>> + phase_adj = nla_get_s32(phase_adj_attr); >>>>>> + if (phase_adj > pin->prop->phase_range.max || >>>>>> + phase_adj < pin->prop->phase_range.min) { >>>>>> + NL_SET_ERR_MSG(extack, "phase adjust value not >>>>>> supported"); >>>>>> + return -EINVAL; >>>>>> + } >>>>>> + xa_for_each(&pin->dpll_refs, i, ref) { >>>>>> + const struct dpll_pin_ops *ops = dpll_pin_ops(ref); >>>>>> + struct dpll_device *dpll = ref->dpll; >>>>>> + >>>>>> + if (!ops->phase_adjust_set) >>>>>> + return -EOPNOTSUPP; >>>>> >>>>>I'm thinking about this part. We can potentially have dpll devices with >>>>>different expectations on phase adjustments, right? And if one of them >>>>>won't be able to adjust phase (or will fail in the next line), then >>>>>netlink will return EOPNOTSUPP while _some_ of the devices will be >>>>>adjusted. Doesn't look great. Can we think about different way to apply >>>>>the change? >>>>> >>>> >>>>Well makes sense to me. >>>> >>>>Does following makes sense as a fix? >>>>We would call op for all devices which has been provided with the op. >>>>If device has no op -> add extack error, continue >>> >>>Is it real to expect some of the device support this and others don't? >>>Is it true for ice? >>>If not, I would got for all-or-nothing here. >>> >> >>Let's step back a bit. >>The op itself is introduced as per pin-dpll tuple.. did this >>intentionally, >>to inform each dpll that the offset has been changed - in case dplls are >>controlled by separated driver/firmware instances but still sharing the >>pin. >>Same way a pin frequency is being set, from user perspective on a pin, but >>callback is called for each dpll the pin was registered with. >>Whatever we do here, it shall be probably done for frequency_set() >>callback as >>well. >> >>The answers: >>So far I don't know the device that might do it this way, it rather >>supports >>phase_adjust or not. In theory we allow such behavior to be implemented, >>i.e. >>pin is registered with 2 dplls, one has the callback, second not. > >If there is only theoretical device like that now, implement >all-or-nothing. If such theoretical device appears in real, this could >be changed. The UAPI would not change, no problem. > I can live with it :) > >>Current hardware of ice sets phase offset for a pin no matter on which >>dpll >>device callback was invoked. >>"all-or-nothing" - do you mean to check all callback returns and then >>decide >>if it was successful? > >Check if all dplls have ops and only perform the action in such case. In >case one of the dplls does not have the op filled, return -EOPNOTSUPP. > > >Regarding the successful/failed op, I think you can just return. In >these cases, when user performs multiaction cmd, he should be prepared >to deal with consequences if part of this cmd fails. We don't have >rollback for any other multiaction cmd in dpll, I don't see why this >should be treated differently. > We don't have it because no one have spotted it on review, as mentioned the frequency_set behaves the same way, we need one approach for all of those cases. I am opting for having the rollback as suggested on the other thread. Thank you! Arkadiusz > >> >>Thank you! >>Arkadiusz >> >>> >>>>If device fails to set -> add extack error, continue >>>>Function always returns 0. >>>> >>>>Thank you! >>>>Arkadiusz >>>> >>>>> >>>>>> + ret = ops->phase_adjust_set(pin, >>>>>> + dpll_pin_on_dpll_priv(dpll, pin), >>>>>> + dpll, dpll_priv(dpll), phase_adj, >>>>>> + extack); >>>>>> + if (ret) >>>>>> + return ret; >>>>>> + } >>>>>> + __dpll_pin_change_ntf(pin); >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>
Tue, Oct 03, 2023 at 04:29:13PM CEST, arkadiusz.kubalewski@intel.com wrote: >>From: Jiri Pirko <jiri@resnulli.us> >>Sent: Tuesday, October 3, 2023 8:27 AM >>To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com> >> >>Tue, Oct 03, 2023 at 01:10:39AM CEST, arkadiusz.kubalewski@intel.com wrote: >>>>From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of >>>>Vadim Fedorenko >>>>Sent: Monday, October 2, 2023 5:09 PM >>>> >>>>On 02/10/2023 16:04, Jiri Pirko wrote: >>>>> Mon, Oct 02, 2023 at 04:32:30PM CEST, arkadiusz.kubalewski@intel.com >>>>> wrote: >>>>>>> From: Vadim Fedorenko <vadim.fedorenko@linux.dev> >>>>>>> Sent: Wednesday, September 27, 2023 8:09 PM >>>>>>> >>>>>>> On 27/09/2023 10:24, Arkadiusz Kubalewski wrote: >>>>>>>> Add callback op (get) for pin-dpll phase-offset measurment. >>>>>>>> Add callback ops (get/set) for pin signal phase adjustment. >>>>>>>> Add min and max phase adjustment values to pin proprties. >>>>>>>> Invoke get callbacks when filling up the pin details to provide user >>>>>>>> with phase related attribute values. >>>>>>>> Invoke phase-adjust set callback when phase-adjust value is provided >>>>>>>> for >>>>>>>> pin-set request. >>>>>>>> >>>>>>>> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >>>>>>> >>>>>>> [...] >>>>>>> >>>>>>>> +static int >>>>>>>> +dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr >>>>>>>> *phase_adj_attr, >>>>>>>> + struct netlink_ext_ack *extack) >>>>>>>> +{ >>>>>>>> + struct dpll_pin_ref *ref; >>>>>>>> + unsigned long i; >>>>>>>> + s32 phase_adj; >>>>>>>> + int ret; >>>>>>>> + >>>>>>>> + phase_adj = nla_get_s32(phase_adj_attr); >>>>>>>> + if (phase_adj > pin->prop->phase_range.max || >>>>>>>> + phase_adj < pin->prop->phase_range.min) { >>>>>>>> + NL_SET_ERR_MSG(extack, "phase adjust value not >>>>>>>> supported"); >>>>>>>> + return -EINVAL; >>>>>>>> + } >>>>>>>> + xa_for_each(&pin->dpll_refs, i, ref) { >>>>>>>> + const struct dpll_pin_ops *ops = dpll_pin_ops(ref); >>>>>>>> + struct dpll_device *dpll = ref->dpll; >>>>>>>> + >>>>>>>> + if (!ops->phase_adjust_set) >>>>>>>> + return -EOPNOTSUPP; >>>>>>> >>>>>>> I'm thinking about this part. We can potentially have dpll devices >>>>>>> with >>>>>>> different expectations on phase adjustments, right? And if one of >>>>>>> them >>>>>>> won't be able to adjust phase (or will fail in the next line), then >>>>>>> netlink will return EOPNOTSUPP while _some_ of the devices will be >>>>>>> adjusted. Doesn't look great. Can we think about different way to >>>>>>> apply >>>>>>> the change? >>>>>>> >>>>>> >>>>>> Well makes sense to me. >>>>>> >>>>>> Does following makes sense as a fix? >>>>>> We would call op for all devices which has been provided with the op. >>>>>> If device has no op -> add extack error, continue >>>>> >>>>> Is it real to expect some of the device support this and others don't? >>>>> Is it true for ice? >>>>> If not, I would got for all-or-nothing here. >>>>> >>>> >>>>But nothing blocks vendors to provide such configuration. Should we >>>>rollback the configuration? Otherwise we can easily make it >>>>inconsistent. >>> >>>Good point, in such case rollback might be required. >>> >>>> >>>>I'm more thinking of checking if all the devices returned error (or >>>>absence of operation callback) and then return error instead of 0 with >>>>extack filled in. >>>> >>> >>>Well, what if different devices would return different errors? >>>In general we would have to keep track of the error values returned in >>>such case.. Assuming one is different than the other - still need to error >>>extack them out? I guess it would be easier to return common error if >>there >> >>In this case, it is common to return the first error hit and bail out, >>not trying the rest. >> > >OK, so now I see it like this: >-> check if all device implement callback, if not return EOPNOTSUPP; >-> get old phase_adjust >-> if new == old, return EINVAL 0 would be better, no? User has what he desired. >-> for each device: call phase_adjust_set, if fails, rollback all previous > successful attempts and return the failure code That would work. >? > >Thank you! >Arkadiusz > >> >>>were only failures and let the driver fill the errors on extack, smt like: >>> >>> int miss_cb_num = 0, dev_num = 0, err_num; >>> >>> xa_for_each(&pin->dpll_refs, i, ref) { >>> const struct dpll_pin_ops *ops = dpll_pin_ops(ref); >>> struct dpll_device *dpll = ref->dpll; >>> >>> dev_num++; >>> if (!ops->phase_adjust_set) { >>> miss_cb_num++; >>> continue; >>> } >>> ret = ops->phase_adjust_set(pin, >>> dpll_pin_on_dpll_priv(dpll, pin), >>> dpll, dpll_priv(dpll), phase_adj, >>> extack); >>> if (ret) >>> err_num++; >>> } >>> if (dev_num == miss_cb_num) >>> return -EOPNOTSUPP; >>> if (dev_num == err_num) >>> return -EINVAL; >>> __dpll_pin_change_ntf(pin); >>> return 0; >>> >>>?? >>> >>>Thank you! >>>Arkadiusz >>> >>>>> >>>>>> If device fails to set -> add extack error, continue >>>>>> Function always returns 0. >>>>>> >>>>>> Thank you! >>>>>> Arkadiusz >>>>>> >>>>>>> >>>>>>>> + ret = ops->phase_adjust_set(pin, >>>>>>>> + dpll_pin_on_dpll_priv(dpll, pin), >>>>>>>> + dpll, dpll_priv(dpll), phase_adj, >>>>>>>> + extack); >>>>>>>> + if (ret) >>>>>>>> + return ret; >>>>>>>> + } >>>>>>>> + __dpll_pin_change_ntf(pin); >>>>>>>> + >>>>>>>> + return 0; >>>>>>>> +} >>>>>>>> + >>>> >>>>_______________________________________________ >>>>Intel-wired-lan mailing list >>>>Intel-wired-lan@osuosl.org >>>>https://lists.osuosl.org/mailman/listinfo/intel-wired-lan >
Tue, Oct 03, 2023 at 04:29:43PM CEST, arkadiusz.kubalewski@intel.com wrote: >>From: Jiri Pirko <jiri@resnulli.us> >>Sent: Tuesday, October 3, 2023 8:32 AM >> >>Tue, Oct 03, 2023 at 01:03:00AM CEST, arkadiusz.kubalewski@intel.com wrote: >>>>From: Jiri Pirko <jiri@resnulli.us> >>>>Sent: Monday, October 2, 2023 5:04 PM >>>> >>>>Mon, Oct 02, 2023 at 04:32:30PM CEST, arkadiusz.kubalewski@intel.com >>>>wrote: >>>>>>From: Vadim Fedorenko <vadim.fedorenko@linux.dev> >>>>>>Sent: Wednesday, September 27, 2023 8:09 PM >>>>>> >>>>>>On 27/09/2023 10:24, Arkadiusz Kubalewski wrote: >>>>>>> Add callback op (get) for pin-dpll phase-offset measurment. >>>>>>> Add callback ops (get/set) for pin signal phase adjustment. >>>>>>> Add min and max phase adjustment values to pin proprties. >>>>>>> Invoke get callbacks when filling up the pin details to provide user >>>>>>> with phase related attribute values. >>>>>>> Invoke phase-adjust set callback when phase-adjust value is provided >>>>>>> for >>>>>>> pin-set request. >>>>>>> >>>>>>> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >>>>>> >>>>>>[...] >>>>>> >>>>>>> +static int >>>>>>> +dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr >>>>>>> *phase_adj_attr, >>>>>>> + struct netlink_ext_ack *extack) >>>>>>> +{ >>>>>>> + struct dpll_pin_ref *ref; >>>>>>> + unsigned long i; >>>>>>> + s32 phase_adj; >>>>>>> + int ret; >>>>>>> + >>>>>>> + phase_adj = nla_get_s32(phase_adj_attr); >>>>>>> + if (phase_adj > pin->prop->phase_range.max || >>>>>>> + phase_adj < pin->prop->phase_range.min) { >>>>>>> + NL_SET_ERR_MSG(extack, "phase adjust value not >>>>>>> supported"); >>>>>>> + return -EINVAL; >>>>>>> + } >>>>>>> + xa_for_each(&pin->dpll_refs, i, ref) { >>>>>>> + const struct dpll_pin_ops *ops = dpll_pin_ops(ref); >>>>>>> + struct dpll_device *dpll = ref->dpll; >>>>>>> + >>>>>>> + if (!ops->phase_adjust_set) >>>>>>> + return -EOPNOTSUPP; >>>>>> >>>>>>I'm thinking about this part. We can potentially have dpll devices with >>>>>>different expectations on phase adjustments, right? And if one of them >>>>>>won't be able to adjust phase (or will fail in the next line), then >>>>>>netlink will return EOPNOTSUPP while _some_ of the devices will be >>>>>>adjusted. Doesn't look great. Can we think about different way to apply >>>>>>the change? >>>>>> >>>>> >>>>>Well makes sense to me. >>>>> >>>>>Does following makes sense as a fix? >>>>>We would call op for all devices which has been provided with the op. >>>>>If device has no op -> add extack error, continue >>>> >>>>Is it real to expect some of the device support this and others don't? >>>>Is it true for ice? >>>>If not, I would got for all-or-nothing here. >>>> >>> >>>Let's step back a bit. >>>The op itself is introduced as per pin-dpll tuple.. did this >>>intentionally, >>>to inform each dpll that the offset has been changed - in case dplls are >>>controlled by separated driver/firmware instances but still sharing the >>>pin. >>>Same way a pin frequency is being set, from user perspective on a pin, but >>>callback is called for each dpll the pin was registered with. >>>Whatever we do here, it shall be probably done for frequency_set() >>>callback as >>>well. >>> >>>The answers: >>>So far I don't know the device that might do it this way, it rather >>>supports >>>phase_adjust or not. In theory we allow such behavior to be implemented, >>>i.e. >>>pin is registered with 2 dplls, one has the callback, second not. >> >>If there is only theoretical device like that now, implement >>all-or-nothing. If such theoretical device appears in real, this could >>be changed. The UAPI would not change, no problem. >> > >I can live with it :) > >> >>>Current hardware of ice sets phase offset for a pin no matter on which >>>dpll >>>device callback was invoked. >>>"all-or-nothing" - do you mean to check all callback returns and then >>>decide >>>if it was successful? >> >>Check if all dplls have ops and only perform the action in such case. In >>case one of the dplls does not have the op filled, return -EOPNOTSUPP. >> >> >>Regarding the successful/failed op, I think you can just return. In >>these cases, when user performs multiaction cmd, he should be prepared >>to deal with consequences if part of this cmd fails. We don't have >>rollback for any other multiaction cmd in dpll, I don't see why this >>should be treated differently. >> > >We don't have it because no one have spotted it on review, >as mentioned the frequency_set behaves the same way, >we need one approach for all of those cases. >I am opting for having the rollback as suggested on the other thread. Okay, but let's do that consistently. > >Thank you! >Arkadiusz > >> >>> >>>Thank you! >>>Arkadiusz >>> >>>> >>>>>If device fails to set -> add extack error, continue >>>>>Function always returns 0. >>>>> >>>>>Thank you! >>>>>Arkadiusz >>>>> >>>>>> >>>>>>> + ret = ops->phase_adjust_set(pin, >>>>>>> + dpll_pin_on_dpll_priv(dpll, pin), >>>>>>> + dpll, dpll_priv(dpll), phase_adj, >>>>>>> + extack); >>>>>>> + if (ret) >>>>>>> + return ret; >>>>>>> + } >>>>>>> + __dpll_pin_change_ntf(pin); >>>>>>> + >>>>>>> + return 0; >>>>>>> +} >>>>>>> + >>>
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Tuesday, October 3, 2023 7:20 PM >To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com> > >Tue, Oct 03, 2023 at 04:29:43PM CEST, arkadiusz.kubalewski@intel.com wrote: >>>From: Jiri Pirko <jiri@resnulli.us> >>>Sent: Tuesday, October 3, 2023 8:32 AM >>> >>>Tue, Oct 03, 2023 at 01:03:00AM CEST, arkadiusz.kubalewski@intel.com >>>wrote: >>>>>From: Jiri Pirko <jiri@resnulli.us> >>>>>Sent: Monday, October 2, 2023 5:04 PM >>>>> >>>>>Mon, Oct 02, 2023 at 04:32:30PM CEST, arkadiusz.kubalewski@intel.com >>>>>wrote: >>>>>>>From: Vadim Fedorenko <vadim.fedorenko@linux.dev> >>>>>>>Sent: Wednesday, September 27, 2023 8:09 PM >>>>>>> >>>>>>>On 27/09/2023 10:24, Arkadiusz Kubalewski wrote: >>>>>>>> Add callback op (get) for pin-dpll phase-offset measurment. >>>>>>>> Add callback ops (get/set) for pin signal phase adjustment. >>>>>>>> Add min and max phase adjustment values to pin proprties. >>>>>>>> Invoke get callbacks when filling up the pin details to provide >>>>>>>> user >>>>>>>> with phase related attribute values. >>>>>>>> Invoke phase-adjust set callback when phase-adjust value is >>>>>>>> provided >>>>>>>> for >>>>>>>> pin-set request. >>>>>>>> >>>>>>>> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >>>>>>> >>>>>>>[...] >>>>>>> >>>>>>>> +static int >>>>>>>> +dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr >>>>>>>> *phase_adj_attr, >>>>>>>> + struct netlink_ext_ack *extack) >>>>>>>> +{ >>>>>>>> + struct dpll_pin_ref *ref; >>>>>>>> + unsigned long i; >>>>>>>> + s32 phase_adj; >>>>>>>> + int ret; >>>>>>>> + >>>>>>>> + phase_adj = nla_get_s32(phase_adj_attr); >>>>>>>> + if (phase_adj > pin->prop->phase_range.max || >>>>>>>> + phase_adj < pin->prop->phase_range.min) { >>>>>>>> + NL_SET_ERR_MSG(extack, "phase adjust value not >>>>>>>> supported"); >>>>>>>> + return -EINVAL; >>>>>>>> + } >>>>>>>> + xa_for_each(&pin->dpll_refs, i, ref) { >>>>>>>> + const struct dpll_pin_ops *ops = dpll_pin_ops(ref); >>>>>>>> + struct dpll_device *dpll = ref->dpll; >>>>>>>> + >>>>>>>> + if (!ops->phase_adjust_set) >>>>>>>> + return -EOPNOTSUPP; >>>>>>> >>>>>>>I'm thinking about this part. We can potentially have dpll devices >>>>>>>with >>>>>>>different expectations on phase adjustments, right? And if one of >>>>>>>them >>>>>>>won't be able to adjust phase (or will fail in the next line), then >>>>>>>netlink will return EOPNOTSUPP while _some_ of the devices will be >>>>>>>adjusted. Doesn't look great. Can we think about different way to >>>>>>>apply >>>>>>>the change? >>>>>>> >>>>>> >>>>>>Well makes sense to me. >>>>>> >>>>>>Does following makes sense as a fix? >>>>>>We would call op for all devices which has been provided with the op. >>>>>>If device has no op -> add extack error, continue >>>>> >>>>>Is it real to expect some of the device support this and others don't? >>>>>Is it true for ice? >>>>>If not, I would got for all-or-nothing here. >>>>> >>>> >>>>Let's step back a bit. >>>>The op itself is introduced as per pin-dpll tuple.. did this >>>>intentionally, >>>>to inform each dpll that the offset has been changed - in case dplls are >>>>controlled by separated driver/firmware instances but still sharing the >>>>pin. >>>>Same way a pin frequency is being set, from user perspective on a pin, but >>>>callback is called for each dpll the pin was registered with. >>>>Whatever we do here, it shall be probably done for frequency_set() >>>>callback as >>>>well. >>>> >>>>The answers: >>>>So far I don't know the device that might do it this way, it rather >>>>supports >>>>phase_adjust or not. In theory we allow such behavior to be implemented, >>>>i.e. >>>>pin is registered with 2 dplls, one has the callback, second not. >>> >>>If there is only theoretical device like that now, implement >>>all-or-nothing. If such theoretical device appears in real, this could >>>be changed. The UAPI would not change, no problem. >>> >> >>I can live with it :) >> >>> >>>>Current hardware of ice sets phase offset for a pin no matter on which >>>>dpll >>>>device callback was invoked. >>>>"all-or-nothing" - do you mean to check all callback returns and then >>>>decide >>>>if it was successful? >>> >>>Check if all dplls have ops and only perform the action in such case. In >>>case one of the dplls does not have the op filled, return -EOPNOTSUPP. >>> >>> >>>Regarding the successful/failed op, I think you can just return. In >>>these cases, when user performs multiaction cmd, he should be prepared >>>to deal with consequences if part of this cmd fails. We don't have >>>rollback for any other multiaction cmd in dpll, I don't see why this >>>should be treated differently. >>> >> >>We don't have it because no one have spotted it on review, >>as mentioned the frequency_set behaves the same way, >>we need one approach for all of those cases. >>I am opting for having the rollback as suggested on the other thread. > >Okay, but let's do that consistently. > Sure, fixed in v2. Thanks! Arkadiusz >> >>Thank you! >>Arkadiusz >> >>> >>>> >>>>Thank you! >>>>Arkadiusz >>>> >>>>> >>>>>>If device fails to set -> add extack error, continue >>>>>>Function always returns 0. >>>>>> >>>>>>Thank you! >>>>>>Arkadiusz >>>>>> >>>>>>> >>>>>>>> + ret = ops->phase_adjust_set(pin, >>>>>>>> + dpll_pin_on_dpll_priv(dpll, pin), >>>>>>>> + dpll, dpll_priv(dpll), phase_adj, >>>>>>>> + extack); >>>>>>>> + if (ret) >>>>>>>> + return ret; >>>>>>>> + } >>>>>>>> + __dpll_pin_change_ntf(pin); >>>>>>>> + >>>>>>>> + return 0; >>>>>>>> +} >>>>>>>> + >>>>
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Tuesday, October 3, 2023 7:19 PM > >Tue, Oct 03, 2023 at 04:29:13PM CEST, arkadiusz.kubalewski@intel.com wrote: >>>From: Jiri Pirko <jiri@resnulli.us> >>>Sent: Tuesday, October 3, 2023 8:27 AM >>>To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com> >>> >>>Tue, Oct 03, 2023 at 01:10:39AM CEST, arkadiusz.kubalewski@intel.com >>>wrote: >>>>>From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of >>>>>Vadim Fedorenko >>>>>Sent: Monday, October 2, 2023 5:09 PM >>>>> >>>>>On 02/10/2023 16:04, Jiri Pirko wrote: >>>>>> Mon, Oct 02, 2023 at 04:32:30PM CEST, arkadiusz.kubalewski@intel.com >>>>>> wrote: >>>>>>>> From: Vadim Fedorenko <vadim.fedorenko@linux.dev> >>>>>>>> Sent: Wednesday, September 27, 2023 8:09 PM >>>>>>>> >>>>>>>> On 27/09/2023 10:24, Arkadiusz Kubalewski wrote: >>>>>>>>> Add callback op (get) for pin-dpll phase-offset measurment. >>>>>>>>> Add callback ops (get/set) for pin signal phase adjustment. >>>>>>>>> Add min and max phase adjustment values to pin proprties. >>>>>>>>> Invoke get callbacks when filling up the pin details to provide user >>>>>>>>> with phase related attribute values. >>>>>>>>> Invoke phase-adjust set callback when phase-adjust value is provided >>>>>>>>> for >>>>>>>>> pin-set request. >>>>>>>>> >>>>>>>>> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >>>>>>>> >>>>>>>> [...] >>>>>>>> >>>>>>>>> +static int >>>>>>>>> +dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr >>>>>>>>> *phase_adj_attr, >>>>>>>>> + struct netlink_ext_ack *extack) >>>>>>>>> +{ >>>>>>>>> + struct dpll_pin_ref *ref; >>>>>>>>> + unsigned long i; >>>>>>>>> + s32 phase_adj; >>>>>>>>> + int ret; >>>>>>>>> + >>>>>>>>> + phase_adj = nla_get_s32(phase_adj_attr); >>>>>>>>> + if (phase_adj > pin->prop->phase_range.max || >>>>>>>>> + phase_adj < pin->prop->phase_range.min) { >>>>>>>>> + NL_SET_ERR_MSG(extack, "phase adjust value not >>>>>>>>> supported"); >>>>>>>>> + return -EINVAL; >>>>>>>>> + } >>>>>>>>> + xa_for_each(&pin->dpll_refs, i, ref) { >>>>>>>>> + const struct dpll_pin_ops *ops = dpll_pin_ops(ref); >>>>>>>>> + struct dpll_device *dpll = ref->dpll; >>>>>>>>> + >>>>>>>>> + if (!ops->phase_adjust_set) >>>>>>>>> + return -EOPNOTSUPP; >>>>>>>> >>>>>>>> I'm thinking about this part. We can potentially have dpll devices >>>>>>>> with >>>>>>>> different expectations on phase adjustments, right? And if one of >>>>>>>> them >>>>>>>> won't be able to adjust phase (or will fail in the next line), then >>>>>>>> netlink will return EOPNOTSUPP while _some_ of the devices will be >>>>>>>> adjusted. Doesn't look great. Can we think about different way to >>>>>>>> apply >>>>>>>> the change? >>>>>>>> >>>>>>> >>>>>>> Well makes sense to me. >>>>>>> >>>>>>> Does following makes sense as a fix? >>>>>>> We would call op for all devices which has been provided with the op. >>>>>>> If device has no op -> add extack error, continue >>>>>> >>>>>> Is it real to expect some of the device support this and others don't? >>>>>> Is it true for ice? >>>>>> If not, I would got for all-or-nothing here. >>>>>> >>>>> >>>>>But nothing blocks vendors to provide such configuration. Should we >>>>>rollback the configuration? Otherwise we can easily make it >>>>>inconsistent. >>>> >>>>Good point, in such case rollback might be required. >>>> >>>>> >>>>>I'm more thinking of checking if all the devices returned error (or >>>>>absence of operation callback) and then return error instead of 0 with >>>>>extack filled in. >>>>> >>>> >>>>Well, what if different devices would return different errors? >>>>In general we would have to keep track of the error values returned in >>>>such case.. Assuming one is different than the other - still need to >>>>error >>>>extack them out? I guess it would be easier to return common error if >>>there >>> >>>In this case, it is common to return the first error hit and bail out, >>>not trying the rest. >>> >> >>OK, so now I see it like this: >>-> check if all device implement callback, if not return EOPNOTSUPP; >>-> get old phase_adjust >>-> if new == old, return EINVAL > >0 would be better, no? User has what he desired. > Yes, that makes sense. > >>-> for each device: call phase_adjust_set, if fails, rollback all previous >> successful attempts and return the failure code > >That would work. > Great, just sent v2. Thanks! Arkadiusz > >>? >> >>Thank you! >>Arkadiusz >> >>> >>>>were only failures and let the driver fill the errors on extack, smt >>>>like: >>>> >>>> int miss_cb_num = 0, dev_num = 0, err_num; >>>> >>>> xa_for_each(&pin->dpll_refs, i, ref) { >>>> const struct dpll_pin_ops *ops = dpll_pin_ops(ref); >>>> struct dpll_device *dpll = ref->dpll; >>>> >>>> dev_num++; >>>> if (!ops->phase_adjust_set) { >>>> miss_cb_num++; >>>> continue; >>>> } >>>> ret = ops->phase_adjust_set(pin, >>>> dpll_pin_on_dpll_priv(dpll, pin), >>>> dpll, dpll_priv(dpll), phase_adj, >>>> extack); >>>> if (ret) >>>> err_num++; >>>> } >>>> if (dev_num == miss_cb_num) >>>> return -EOPNOTSUPP; >>>> if (dev_num == err_num) >>>> return -EINVAL; >>>> __dpll_pin_change_ntf(pin); >>>> return 0; >>>> >>>>?? >>>> >>>>Thank you! >>>>Arkadiusz >>>> >>>>>> >>>>>>> If device fails to set -> add extack error, continue >>>>>>> Function always returns 0. >>>>>>> >>>>>>> Thank you! >>>>>>> Arkadiusz >>>>>>> >>>>>>>> >>>>>>>>> + ret = ops->phase_adjust_set(pin, >>>>>>>>> + dpll_pin_on_dpll_priv(dpll, pin), >>>>>>>>> + dpll, dpll_priv(dpll), phase_adj, >>>>>>>>> + extack); >>>>>>>>> + if (ret) >>>>>>>>> + return ret; >>>>>>>>> + } >>>>>>>>> + __dpll_pin_change_ntf(pin); >>>>>>>>> + >>>>>>>>> + return 0; >>>>>>>>> +} >>>>>>>>> + >>>>> >>>>>_______________________________________________ >>>>>Intel-wired-lan mailing list >>>>>Intel-wired-lan@osuosl.org >>>>>https://lists.osuosl.org/mailman/listinfo/intel-wired-lan >>
diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c index e20daba6896a..b48fd556e490 100644 --- a/drivers/dpll/dpll_netlink.c +++ b/drivers/dpll/dpll_netlink.c @@ -212,6 +212,53 @@ dpll_msg_add_pin_direction(struct sk_buff *msg, struct dpll_pin *pin, return 0; } +static int +dpll_msg_add_pin_phase_adjust(struct sk_buff *msg, struct dpll_pin *pin, + struct dpll_pin_ref *ref, + struct netlink_ext_ack *extack) +{ + const struct dpll_pin_ops *ops = dpll_pin_ops(ref); + struct dpll_device *dpll = ref->dpll; + s32 phase_adjust; + int ret; + + if (!ops->phase_adjust_get) + return 0; + ret = ops->phase_adjust_get(pin, dpll_pin_on_dpll_priv(dpll, pin), + dpll, dpll_priv(dpll), + &phase_adjust, extack); + if (ret) + return ret; + if (nla_put_s32(msg, DPLL_A_PIN_PHASE_ADJUST, phase_adjust)) + return -EMSGSIZE; + + return 0; +} + +static int +dpll_msg_add_phase_offset(struct sk_buff *msg, struct dpll_pin *pin, + struct dpll_pin_ref *ref, + struct netlink_ext_ack *extack) +{ + const struct dpll_pin_ops *ops = dpll_pin_ops(ref); + struct dpll_device *dpll = ref->dpll; + s64 phase_offset; + int ret; + + if (!ops->phase_offset_get) + return 0; + ret = ops->phase_offset_get(pin, dpll_pin_on_dpll_priv(dpll, pin), + dpll, dpll_priv(dpll), &phase_offset, + extack); + if (ret) + return ret; + if (nla_put_64bit(msg, DPLL_A_PIN_PHASE_OFFSET, sizeof(phase_offset), + &phase_offset, DPLL_A_PIN_PAD)) + return -EMSGSIZE; + + return 0; +} + static int dpll_msg_add_pin_freq(struct sk_buff *msg, struct dpll_pin *pin, struct dpll_pin_ref *ref, struct netlink_ext_ack *extack) @@ -330,6 +377,9 @@ dpll_msg_add_pin_dplls(struct sk_buff *msg, struct dpll_pin *pin, if (ret) goto nest_cancel; ret = dpll_msg_add_pin_direction(msg, pin, ref, extack); + if (ret) + goto nest_cancel; + ret = dpll_msg_add_phase_offset(msg, pin, ref, extack); if (ret) goto nest_cancel; nla_nest_end(msg, attr); @@ -377,6 +427,15 @@ dpll_cmd_pin_get_one(struct sk_buff *msg, struct dpll_pin *pin, if (nla_put_u32(msg, DPLL_A_PIN_CAPABILITIES, prop->capabilities)) return -EMSGSIZE; ret = dpll_msg_add_pin_freq(msg, pin, ref, extack); + if (ret) + return ret; + if (nla_put_s32(msg, DPLL_A_PIN_PHASE_ADJUST_MIN, + prop->phase_range.min)) + return -EMSGSIZE; + if (nla_put_s32(msg, DPLL_A_PIN_PHASE_ADJUST_MAX, + prop->phase_range.max)) + return -EMSGSIZE; + ret = dpll_msg_add_pin_phase_adjust(msg, pin, ref, extack); if (ret) return ret; if (xa_empty(&pin->parent_refs)) @@ -416,7 +475,7 @@ dpll_device_get_one(struct dpll_device *dpll, struct sk_buff *msg, if (nla_put_u32(msg, DPLL_A_TYPE, dpll->type)) return -EMSGSIZE; - return ret; + return 0; } static int @@ -705,6 +764,39 @@ dpll_pin_direction_set(struct dpll_pin *pin, struct dpll_device *dpll, return 0; } +static int +dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr *phase_adj_attr, + struct netlink_ext_ack *extack) +{ + struct dpll_pin_ref *ref; + unsigned long i; + s32 phase_adj; + int ret; + + phase_adj = nla_get_s32(phase_adj_attr); + if (phase_adj > pin->prop->phase_range.max || + phase_adj < pin->prop->phase_range.min) { + NL_SET_ERR_MSG(extack, "phase adjust value not supported"); + return -EINVAL; + } + xa_for_each(&pin->dpll_refs, i, ref) { + const struct dpll_pin_ops *ops = dpll_pin_ops(ref); + struct dpll_device *dpll = ref->dpll; + + if (!ops->phase_adjust_set) + return -EOPNOTSUPP; + ret = ops->phase_adjust_set(pin, + dpll_pin_on_dpll_priv(dpll, pin), + dpll, dpll_priv(dpll), phase_adj, + extack); + if (ret) + return ret; + } + __dpll_pin_change_ntf(pin); + + return 0; +} + static int dpll_pin_parent_device_set(struct dpll_pin *pin, struct nlattr *parent_nest, struct netlink_ext_ack *extack) @@ -793,6 +885,11 @@ dpll_pin_set_from_nlattr(struct dpll_pin *pin, struct genl_info *info) if (ret) return ret; break; + case DPLL_A_PIN_PHASE_ADJUST: + ret = dpll_pin_phase_adj_set(pin, a, info->extack); + if (ret) + return ret; + break; case DPLL_A_PIN_PARENT_DEVICE: ret = dpll_pin_parent_device_set(pin, a, info->extack); if (ret) diff --git a/include/linux/dpll.h b/include/linux/dpll.h index bbc480cd2932..578fc5fa3750 100644 --- a/include/linux/dpll.h +++ b/include/linux/dpll.h @@ -68,6 +68,18 @@ struct dpll_pin_ops { int (*prio_set)(const struct dpll_pin *pin, void *pin_priv, const struct dpll_device *dpll, void *dpll_priv, const u32 prio, struct netlink_ext_ack *extack); + int (*phase_offset_get)(const struct dpll_pin *pin, void *pin_priv, + const struct dpll_device *dpll, void *dpll_priv, + s64 *phase_offset, + struct netlink_ext_ack *extack); + int (*phase_adjust_get)(const struct dpll_pin *pin, void *pin_priv, + const struct dpll_device *dpll, void *dpll_priv, + s32 *phase_adjust, + struct netlink_ext_ack *extack); + int (*phase_adjust_set)(const struct dpll_pin *pin, void *pin_priv, + const struct dpll_device *dpll, void *dpll_priv, + const s32 phase_adjust, + struct netlink_ext_ack *extack); }; struct dpll_pin_frequency { @@ -91,6 +103,11 @@ struct dpll_pin_frequency { #define DPLL_PIN_FREQUENCY_DCF77 \ DPLL_PIN_FREQUENCY(DPLL_PIN_FREQUENCY_77_5_KHZ) +struct dpll_pin_phase_adjust_range { + s32 min; + s32 max; +}; + struct dpll_pin_properties { const char *board_label; const char *panel_label; @@ -99,6 +116,7 @@ struct dpll_pin_properties { unsigned long capabilities; u32 freq_supported_num; struct dpll_pin_frequency *freq_supported; + struct dpll_pin_phase_adjust_range phase_range; }; #if IS_ENABLED(CONFIG_DPLL)
Add callback op (get) for pin-dpll phase-offset measurment. Add callback ops (get/set) for pin signal phase adjustment. Add min and max phase adjustment values to pin proprties. Invoke get callbacks when filling up the pin details to provide user with phase related attribute values. Invoke phase-adjust set callback when phase-adjust value is provided for pin-set request. Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> --- drivers/dpll/dpll_netlink.c | 99 ++++++++++++++++++++++++++++++++++++- include/linux/dpll.h | 18 +++++++ 2 files changed, 116 insertions(+), 1 deletion(-)