Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
MIME-Version: 1.0
References: <20231121101331.22837-1-naresh.solanki@9elements.com> <ZV9MNdOCkIpLB+6Y@finisterre.sirena.org.uk>
In-Reply-To: <ZV9MNdOCkIpLB+6Y@finisterre.sirena.org.uk>
From:   Naresh Solanki <naresh.solanki@9elements.com>
Date:   Mon, 27 Nov 2023 16:07:45 +0530
Message-ID: <CABqG17jRbqcw=QioyZTPFTidUDiPDgBFbm45vfvG2Sc5TOznvw@mail.gmail.com>
Subject: Re: [PATCH v2] regulator: event: Add regulator netlink event support
To:     Mark Brown <broonie@kernel.org>
Cc:     Liam Girdwood <lgirdwood@gmail.com>, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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