Re: [PATCH v2] regulator: event: Add regulator netlink event support

[Naresh Solanki]

Hi Mark,


On Thu, 23 Nov 2023 at 18:27, Mark Brown <broonie@xxxxxxxxxx> 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