>From: Jiri Pirko <jiri@xxxxxxxxxxx> >Sent: Monday, October 2, 2023 5:04 PM > >Mon, Oct 02, 2023 at 04:32:30PM CEST, arkadiusz.kubalewski@xxxxxxxxx wrote: >>>From: Vadim Fedorenko <vadim.fedorenko@xxxxxxxxx> >>>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@xxxxxxxxx> >>> >>>[...] >>> >>>> +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; >>>> +} >>>> +