Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 24 Nov 2020 08:54:25 -0600
From:   Bjorn Andersson <bjorn.andersson@linaro.org>
To:     Wilken Gottwalt <wilken.gottwalt@posteo.net>
Cc:     linux-kernel@vger.kernel.org, Ohad Ben-Cohen <ohad@wizery.com>,
        Baolin Wang <baolin.wang7@gmail.com>,
        Rob Herring <robh+dt@kernel.org>,
        Maxime Ripard <mripard@kernel.org>,
        Chen-Yu Tsai <wens@csie.org>,
        Jernej Skrabec <jernej.skrabec@siol.net>
Subject: Re: [PATCH 2/2] hwspinlock: add sunxi hardware spinlock support
Message-ID: <20201124145425.GB185852@builder.lan>
References: <cover.1605693132.git.wilken.gottwalt@posteo.net>
 <149526a0ba8d18ebb68baa24e95d946ede90b4c0.1605693132.git.wilken.gottwalt@posteo.net>
 <20201122051900.GH807@yoga>
 <20201123191712.72484b19@monster.powergraphx.local>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201123191712.72484b19@monster.powergraphx.local>
Precedence: bulk

On Mon 23 Nov 12:17 CST 2020, Wilken Gottwalt wrote:

> On Sat, 21 Nov 2020 23:19:00 -0600
> Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
> > > +static int hwlocks_inuse_show(struct seq_file *seqf, void *unused)
> > > +{
> > > +	struct sunxi_hwspinlock_data *priv = seqf->private;
> > > +	int inuse;
> > > +
> > > +	/* getting the status of only the main 32 spinlocks is supported */
> > > +	inuse = hweight32(readl(priv->io_base + SPINLOCK_STATUS_REG));
> > 
> > So this returns how many of the locks are taken? How is that useful?
> 
> It is a way to see if locks were taken from linux or the arisc core without
> touching the actual hwspinlock abi or the locks. So it is a nice way to debug
> hwspinlocks, hence it is part of debugfs.
> 

So in a scenario where two remote processors ping-pong the lock between
them, this will always read 1 and you won't know why?

> > > +	seq_printf(seqf, "%d\n", inuse);
[..]
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct of_device_id sunxi_hwspinlock_ids[] = {
> > > +	{ .compatible = "allwinner,sun8i-hwspinlock", },
> > > +	{ .compatible = "allwinner,sun50i-hwspinlock", },
> > > +	{},
> > > +};
> > > +MODULE_DEVICE_TABLE(of, sunxi_hwspinlock_ids);
> > > +
> > > +static struct platform_driver sunxi_hwspinlock_driver = {
> > > +	.probe	= sunxi_hwspinlock_probe,
> > > +	.remove	= sunxi_hwspinlock_remove,
> > > +	.driver	= {
> > > +		.name		= DRIVER_NAME,
> > > +		.of_match_table	= of_match_ptr(sunxi_hwspinlock_ids),
> > 
> > Please avoid of_match_ptr, as this will cause warnings about unused
> > variables when COMPILE_TEST without OF.
> 
> So did you mean to leave it out completely?
> 

Yes, "worst case" is that you include the reference to
sunxi_hwspinlock_ids on a build without CONFIG_OF and wasting a little
bit of memory.

Using of_match_ptr() with CONFIG_OF=n will result in NULL and as such
we'll get a compile warning that nothing references sunxi_hwspinlock_ids
- so then that will have to be marked __maybe_unused, or wrapped in an
#if...

So better just leave it as:
	.of_match_table = sunxi_hwspinlock_ids,

Regards,
Bjorn