> On Fri, Oct 25, 2024 at 06:00:42AM +0300, Wei Fang wrote: > > > Don't artificially create errors when there are really no errors to handle. > > > Both enetc_pf_ops and enetc4_pf_ops provide .set_si_primary_mac(), so it > > > is unnecessary to handle the case where it isn't present. Those functions > > > return void, and void can be propagated to enetc_set_si_hw_addr() as well. > > > > I thought checking the pointer is safer, so you mean that pointers that are > > definitely present in the current driver do not need to be checked? > > Yes, there is no point to check for a condition which is impossible > to trigger in the current code. The callee and the caller are tightly > coupled (in the same driver), it's not a rigid API, so if the function > pointers should be made optional for some future hardware IP, the error > handling will be added when necessary. Ideally any change passes through > review, and any inconsistency should be spotted when added. > > > > A bit inconsistent that pf->ops->set_si_primary_mac() goes through a > > > wrapper function but this doesn't. > > > > > > > If we really do not need to check these callback pointers, then I think I can > > remove the wrapper. > > Fine without wrapping throughout, my comment was first and foremost > about consistency. > > > > This one looks extremely weird in the existing code, but I suppose I'm > > > too late to the party to request you to clean up any of the PSFP code, > > > so I'll make a note to do it myself after your work. I haven't spotted > > > any actual bug, just weird coding patterns. > > > > > > No change request here. I see the netc4_pf doesn't implement > enable_psfp(), > > > so making it optional here is fine. > > > > Yes, PSFP is not supported in this patch set, I will remove it in future. > > If by "I will remove it in future" you mean "once NETC4 gains PSFP > support, I will make enable_psfp() non-optional", then ok, great. > > > Currently, we have not add the PCS support, so the 10G ENETC is not > supported > > yet. And we also disable the 10G ENETC in DTS. Only the 1G ENETCs (without > PCS) > > are supported for i.MX95. > > Also think about the case where the current version of the kernel > will boot on a newer version of the device tree, which does not have > 'status = "disabled";' for those ports. It should do something reasonable. > In any case, "they're now disabled in the device tree" is not an argument. > For this case where the kernel is lower but the device tree is newer, I think there is no problem. It just fails in probe() and does not affect other functions. The old kernel does not support PCS, it is reasonable to return a '- EOPNOTSUPP' error. > My anecdotal and vague understanding of the Arm SystemReady (IR I think) > requirements is that the device tree is provided by the platform, > separately from the kernel/rootfs. It relies on the device tree ABI > being stable, backwards-compatible and forwards-compatible. > > > > A message maybe, stating what's the deal? Just that users figure out > > > quickly that it's an expected behavior, and not spend hours debugging > > > until they find out it's not their fault? > > > > I will explain in the commit message that i.MX95 10G ENETC is not currently > > supported. > > By "a message, maybe" I actually meant dev_err("Message here\n"); rather > than silent/imprecise failure. Okay, I'll add an explicit error message.