Messages in this thread | | | From | Naresh Solanki <> | Date | Mon, 27 Nov 2023 16:07:45 +0530 | Subject | Re: [PATCH v2] regulator: event: Add regulator netlink event support |
| |
Hi Mark,
On Thu, 23 Nov 2023 at 18:27, Mark Brown <broonie@kernel.org> wrote: > > On Tue, Nov 21, 2023 at 10:13:30AM +0000, Naresh Solanki wrote: > > > This commit introduces netlink event support to the regulator subsystem. > > This looks a lot nicer, there's some feedback below but nothing > maissively substantial. > > > +#ifdef CONFIG_NET > > This is essentially the entire file - it's probably better to just put > the stub functions in the header and do the building with Kconfig. Sure. As I understood, Will add an option REGULATOR_NETLINK to include the event.c file during build. > > > +static unsigned int reg_event_seqnum; > > +struct reg_genl_event { > > + char reg_name[15]; > > + u64 event; > > +}; > > Given that this is going to get sent to userspace shouldn't it be in a > uapi header? Some of the other event types don't seem to do this > though... that might be an issue with those examples though. We'll > also make the event numbers into uapi so they should go in a uapi header > as well. Will update next revision to include uapi header. > > I'm also not clear on where the 15 byte limit comes from. I felt 15 characters would be sufficient for regulator names. Would need your inputs to make sure here. > > > +EXPORT_SYMBOL(reg_generate_netlink_event); > > Everything else in regulator is EXPORT_SYMBOL_GPL() so this should be > too - though given that the only caller is _notifier_call_chain() which > is in the core does it need to be exported at all? I can't see a case > for anything calling it independently of that. Agree. The current scope is limited to regulator core so will remove the export.
Regards, Naresh
| |