Re: [Intel-wired-lan] [PATCH RFC net-next 02/13] dpll: Allow registering pin with firmware node
Mon, Dec 15, 2025 at 02:51:36PM +0100, [email protected] wrote: >On 12/15/25 2:08 PM, Jiri Pirko wrote: >> Sun, Dec 14, 2025 at 08:35:01PM +0100, [email protected] wrote: >> > >> > >> > On December 12, 2025 12:25:12 PM GMT+01:00, Jiri Pirko >> > wrote: >> > > Thu, Dec 11, 2025 at 08:47:45PM +0100, [email protected] wrote: >> > > >> > > [..] >> > > >> > > > @@ -559,7 +563,8 @@ EXPORT_SYMBOL(dpll_netdev_pin_clear); >> > > > */ >> > > > struct dpll_pin * >> > > > dpll_pin_get(u64 clock_id, u32 pin_idx, struct module *module, >> > > > - const struct dpll_pin_properties *prop) >> > > > + const struct dpll_pin_properties *prop, >> > > > + struct fwnode_handle *fwnode) >> > > > { >> > > >struct dpll_pin *pos, *ret = NULL; >> > > >unsigned long i; >> > > > @@ -568,14 +573,15 @@ dpll_pin_get(u64 clock_id, u32 pin_idx, struct >> > > > module *module, >> > > >xa_for_each(&dpll_pin_xa, i, pos) { >> > > >if (pos->clock_id == clock_id && >> > > >pos->pin_idx == pin_idx && >> > > > - pos->module == module) { >> > > > + pos->module == module && >> > > > + pos->fwnode == fwnode) { >> > > >> > > Is fwnode part of the key? Doesn't look to me like that. Then you can >> > > have a simple helper to set fwnode on struct dpll_pin *, and leave >> > > dpll_pin_get() out of this, no? >> > >> > IMHO yes, because particular fwnode identifies exact dpll pin, so >> > I think it should be a part of the key. >> >> The key items serve for userspace identification purposes as well. For >> that, fwnode is non-sense. >> fwnode identifies exact pin, that is nice. But is it the only >> differentiator among other key items? I don't expect so. > >From this point of view, not. I will not touch dpll_pin_get() and rather >use new helper like dpll_pin_fwnode_set(), ok? Yes please. Thanks! > >Thanks, >Ivan >
Re: [Intel-wired-lan] [PATCH RFC net-next 02/13] dpll: Allow registering pin with firmware node
On 12/15/25 2:08 PM, Jiri Pirko wrote: Sun, Dec 14, 2025 at 08:35:01PM +0100, [email protected] wrote: On December 12, 2025 12:25:12 PM GMT+01:00, Jiri Pirko wrote: Thu, Dec 11, 2025 at 08:47:45PM +0100, [email protected] wrote: [..] @@ -559,7 +563,8 @@ EXPORT_SYMBOL(dpll_netdev_pin_clear); */ struct dpll_pin * dpll_pin_get(u64 clock_id, u32 pin_idx, struct module *module, -const struct dpll_pin_properties *prop) +const struct dpll_pin_properties *prop, +struct fwnode_handle *fwnode) { struct dpll_pin *pos, *ret = NULL; unsigned long i; @@ -568,14 +573,15 @@ dpll_pin_get(u64 clock_id, u32 pin_idx, struct module *module, xa_for_each(&dpll_pin_xa, i, pos) { if (pos->clock_id == clock_id && pos->pin_idx == pin_idx && - pos->module == module) { + pos->module == module && + pos->fwnode == fwnode) { Is fwnode part of the key? Doesn't look to me like that. Then you can have a simple helper to set fwnode on struct dpll_pin *, and leave dpll_pin_get() out of this, no? IMHO yes, because particular fwnode identifies exact dpll pin, so I think it should be a part of the key. The key items serve for userspace identification purposes as well. For that, fwnode is non-sense. fwnode identifies exact pin, that is nice. But is it the only differentiator among other key items? I don't expect so. From this point of view, not. I will not touch dpll_pin_get() and rather use new helper like dpll_pin_fwnode_set(), ok? Thanks, Ivan
Re: [Intel-wired-lan] [PATCH RFC net-next 02/13] dpll: Allow registering pin with firmware node
Sun, Dec 14, 2025 at 08:35:01PM +0100, [email protected] wrote: > > >On December 12, 2025 12:25:12 PM GMT+01:00, Jiri Pirko >wrote: >>Thu, Dec 11, 2025 at 08:47:45PM +0100, [email protected] wrote: >> >>[..] >> >>>@@ -559,7 +563,8 @@ EXPORT_SYMBOL(dpll_netdev_pin_clear); >>> */ >>> struct dpll_pin * >>> dpll_pin_get(u64 clock_id, u32 pin_idx, struct module *module, >>>- const struct dpll_pin_properties *prop) >>>+ const struct dpll_pin_properties *prop, >>>+ struct fwnode_handle *fwnode) >>> { >>> struct dpll_pin *pos, *ret = NULL; >>> unsigned long i; >>>@@ -568,14 +573,15 @@ dpll_pin_get(u64 clock_id, u32 pin_idx, struct module >>>*module, >>> xa_for_each(&dpll_pin_xa, i, pos) { >>> if (pos->clock_id == clock_id && >>> pos->pin_idx == pin_idx && >>>-pos->module == module) { >>>+pos->module == module && >>>+pos->fwnode == fwnode) { >> >>Is fwnode part of the key? Doesn't look to me like that. Then you can >>have a simple helper to set fwnode on struct dpll_pin *, and leave >>dpll_pin_get() out of this, no? > >IMHO yes, because particular fwnode identifies exact dpll pin, so >I think it should be a part of the key. The key items serve for userspace identification purposes as well. For that, fwnode is non-sense. fwnode identifies exact pin, that is nice. But is it the only differentiator among other key items? I don't expect so. >
Re: [Intel-wired-lan] [PATCH RFC net-next 02/13] dpll: Allow registering pin with firmware node
On December 12, 2025 12:25:12 PM GMT+01:00, Jiri Pirko wrote: >Thu, Dec 11, 2025 at 08:47:45PM +0100, [email protected] wrote: > >[..] > >>@@ -559,7 +563,8 @@ EXPORT_SYMBOL(dpll_netdev_pin_clear); >> */ >> struct dpll_pin * >> dpll_pin_get(u64 clock_id, u32 pin_idx, struct module *module, >>- const struct dpll_pin_properties *prop) >>+ const struct dpll_pin_properties *prop, >>+ struct fwnode_handle *fwnode) >> { >> struct dpll_pin *pos, *ret = NULL; >> unsigned long i; >>@@ -568,14 +573,15 @@ dpll_pin_get(u64 clock_id, u32 pin_idx, struct module >>*module, >> xa_for_each(&dpll_pin_xa, i, pos) { >> if (pos->clock_id == clock_id && >> pos->pin_idx == pin_idx && >>- pos->module == module) { >>+ pos->module == module && >>+ pos->fwnode == fwnode) { > >Is fwnode part of the key? Doesn't look to me like that. Then you can >have a simple helper to set fwnode on struct dpll_pin *, and leave >dpll_pin_get() out of this, no? IMHO yes, because particular fwnode identifies exact dpll pin, so I think it should be a part of the key.
Re: [Intel-wired-lan] [PATCH RFC net-next 02/13] dpll: Allow registering pin with firmware node
Thu, Dec 11, 2025 at 08:47:45PM +0100, [email protected] wrote: [..] >@@ -559,7 +563,8 @@ EXPORT_SYMBOL(dpll_netdev_pin_clear); > */ > struct dpll_pin * > dpll_pin_get(u64 clock_id, u32 pin_idx, struct module *module, >- const struct dpll_pin_properties *prop) >+ const struct dpll_pin_properties *prop, >+ struct fwnode_handle *fwnode) > { > struct dpll_pin *pos, *ret = NULL; > unsigned long i; >@@ -568,14 +573,15 @@ dpll_pin_get(u64 clock_id, u32 pin_idx, struct module >*module, > xa_for_each(&dpll_pin_xa, i, pos) { > if (pos->clock_id == clock_id && > pos->pin_idx == pin_idx && >- pos->module == module) { >+ pos->module == module && >+ pos->fwnode == fwnode) { Is fwnode part of the key? Doesn't look to me like that. Then you can have a simple helper to set fwnode on struct dpll_pin *, and leave dpll_pin_get() out of this, no? > ret = pos; > refcount_inc(&ret->refcount); > break; > } [..]
