>From: Jiri Pirko <jiri@xxxxxxxxxxx> >Sent: Tuesday, July 11, 2023 1:53 PM > >Tue, Jul 11, 2023 at 12:34:11PM CEST, arkadiusz.kubalewski@xxxxxxxxx wrote: >>>From: Jiri Pirko <jiri@xxxxxxxxxxx> >>>Sent: Monday, July 10, 2023 2:10 PM >>> >>>Mon, Jul 10, 2023 at 12:07:30PM CEST, arkadiusz.kubalewski@xxxxxxxxx wrote: >>>>>From: Jiri Pirko <jiri@xxxxxxxxxxx> >>>>>Sent: Wednesday, June 28, 2023 1:16 PM >>>>>Wed, Jun 28, 2023 at 11:15:11AM CEST, arkadiusz.kubalewski@xxxxxxxxx >>>>>wrote: >>>>>>>From: Jiri Pirko <jiri@xxxxxxxxxxx> >>>>>>>Sent: Tuesday, June 27, 2023 12:18 PM >>>>>>> >>>>>>>Fri, Jun 23, 2023 at 02:38:10PM CEST, arkadiusz.kubalewski@xxxxxxxxx >>>>>>>wrote: >>>>>>> >>>>>>>>v8 -> v9: >>>>>>> >>>>>>>Could you please address all the unresolved issues from v8 and send v10? >>>>>>>I'm not reviewing this one. >>>>>>> >>>>>>>Thanks! >>>>>> >>>>>>Sure, will do, but first missing to-do/discuss list: >>>>>>1) remove mode_set as not used by any driver >>>> >>>>I have implemented in ice (also added back the DPLL_MODE_FREERUN). >>> >>>Uh :/ Why exactly is it needed in this initial submission? >>> >> >>Without mode-set there is no need for device-set at all, right? >>So it is better to implement at least one set command, so we don't >>need remove device-set command entirely. > >The enum cmd valu could stay as a placeholder, the rest can go. > It doesn't make much sense to have a command which is not implemented, same as you wanted to remove enum values which are not used. > >> >>> >>>> >>>>>>2) remove "no-added-value" static functions descriptions in >>>>>> dpll_core/dpll_netlink >>>> >>>>Removed. >>>> >>>>>>3) merge patches [ 03/10, 04/10, 05/10 ] into patches that are compiling >>>>>> after each patch apply >>>> >>>>Hope Vadim will decide on this, the thing is merging in two patches >>>>doesn't make much sense as there won't be any linking until both patches >>>>are there, so most sense it would be if 3 are merged into one, but >>>>then we will be back to one big blob patch issue. >>>> >>>>>>4) remove function return values descriptions/lists >>>> >>>>Fixed. >>>> >>>>>>5) Fix patch [05/10]: >>>>>> - status Supported >>>>>> - additional maintainers >>>>>> - remove callback: >>>>>> int (*source_pin_idx_get)(...) from `struct dpll_device_ops` >>>>>>6) Fix patch [08/10]: rethink ice mutex locking scheme >>>> >>>>Fixed. >>>> >>>>>>7) Fix patch [09/10]: multiple comments on >>>>>>https://lore.kernel.org/netdev/ZIQu+%2Fo4J0ZBspVg@nanopsycho/#t >>>>>>8) add PPS DPLL phase offset to the netlink get-device API >>>>>> >>>> >>>>Added few things on this matter >>>>- 1 dpll level attribute: >>>> - phase-shift - measuring the phase difference between dpll input >>>> and it's output >>>>- 1 dpll-pin tuple level attribute: >>>> - pin-phase-adjust - set/get phase adjust of a pin on a dpll >>>>- 2 pin level attributes: >>>> - pin-phase-adjust-min - provide user with min value that can be set >>>> - pin-phase-adjust-max - provide user with max value that can be set >>>>- a constant: >>>> - DPLL_PHASE_SHIFT_DIVIDER similar to DPLL_TEMP_DIVIDER for producing >>>> fraction value of measured DPLL_A_PHASE_SHIFT >>> >>>Again, why do we need this in this initial submission? Why it can't be a >>>follow-up patchset to extend this? This way we never converge :/ >>>Please focus on what we have now and bring it in. Let the extensions to >>>be addressed later on, please. >>> >> >>Well AFAIK, RHEL is doing some monitoring software, so the end-users need >>this. > >They need it for the initial submission? Why? Why can't they wait 1 week >for follow-up patchset? > Probably best if they could respond here, though I know they are waiting for the dpll interface for a long time already. > >> >>> >>> >>>>- implemented in dpll netlink and in ice >>>> >>>>> >>>>>You are missing removal of pin->prop.package_label = dev_name(dev); in >>>>>ice. >>>>> >>>> >>>>I didn't touch it, as we still need to discuss it, Jakub didn't respond >>>>on v8 thread. >>>>I don't see why we shall not name it the way. This is most meaningful >>>>label for those pins for the user right now. >>> >>>This is not meaningful, at all. dev_name() changes upon which pci slot >>>you plug the card into. package_label should be an actual label on a >>>silicon package. Why you think this two are related in aby way, makes me >>>really wonder. Could you elaborate the meaningfulness of this? >>> >> >>Without this, from end-user perspective, it would be very confusing. >>As in ice without any label there would 4 pins which differs only with id. > >There you go, it does not have any label, yet you are trying hard to >make up some. Does not make sense. > Don't get this, they have the label, but you ask to remove it.. > >>What names would you suggest? > >That is the point I made previously. For synce usecase, the label does >not make sense. There should be no label. You reference the pin by ID >from netdev, that is enough. > Yea I understand, you are trying to hide this information from the user, while I am trying not to hide anything, and let the user know all the information can be somehow useful, this is the difference. The one might want to ask how label indicating internal pin label is useful, it is transparency, the label is there for identification nothing else. You mean SyncE use case where a software daemon controls the dpll for SyncE implementation, and this is already implemented and works as you described. >I think better to add the check to pin-register so future synce pin >users don't have similar weird ideas. Could you please add this check? > Don't think it is way to go, and I don't think there is anything good with preventing device drivers from labeling their pins the way they want. I don't understand why all the pins shall be targeted differently then SyncE ones, I mean the SyncE pins are special case (of dpll subsystem), and for that case, they are also targetable by netdevice, but what I don't understand is why they shall be only targetable with netdevice, they are still part of dpll subsystem not a SyncE subsystem, and with this understanding shall be targetable like all the other pins. While without the label this will not be possible. Thank you! Arkadiusz >Thanks! > > [...]
