Received-SPF: pass (google.com: domain of linux-kernel+bounces-211320-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Wed, 12 Jun 2024 12:03:31 +0200
From: Wolfram Sang <wsa+renesas@sang-engineering.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: linux-renesas-soc@vger.kernel.org, Jonathan Corbet <corbet@lwn.net>, 
	Linus Walleij <linus.walleij@linaro.org>, Bartosz Golaszewski <brgl@bgdev.pl>, 
	Kent Gibson <warthog618@gmail.com>, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, 
	linux-gpio@vger.kernel.org
Subject: Re: [PATCH v9 1/1] gpio: add sloppy logic analyzer using polling
Message-ID: <wdvnbt2vpobukz3s6pxkowoizvsxjeycnb3rmfsmfbx5zsgvrp@mog64h2ogftv>
Mail-Followup-To: Wolfram Sang <wsa+renesas@sang-engineering.com>, 
	Andy Shevchenko <andy.shevchenko@gmail.com>, linux-renesas-soc@vger.kernel.org, 
	Jonathan Corbet <corbet@lwn.net>, Linus Walleij <linus.walleij@linaro.org>, 
	Bartosz Golaszewski <brgl@bgdev.pl>, Kent Gibson <warthog618@gmail.com>, linux-doc@vger.kernel.org, 
	linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org
References: <20240610112700.80819-1-wsa+renesas@sang-engineering.com>
 <20240610112700.80819-2-wsa+renesas@sang-engineering.com>
 <ZmcN6lJAGVxY3Ok2@surfacebook.localdomain>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="wj5fobjlv4oflgqg"
Content-Disposition: inline
In-Reply-To: <ZmcN6lJAGVxY3Ok2@surfacebook.localdomain>


--wj5fobjlv4oflgqg
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hi Andi,


> > +#include <linux/delay.h>
>=20
> + device.h
> + err.h

OK about the includes.

> > +	mutex_lock(&priv->blob_lock);
>=20
> guard() (from cleanup.h)?

If you insist. I doesn't save an exit path, so I don't think this will
improve readability of the code. But I don't mind...

> > +static const struct file_operations fops_trigger =3D {
> > +	.owner =3D THIS_MODULE,
> > +	.open =3D trigger_open,
> > +	.write =3D trigger_write,
> > +	.llseek =3D no_llseek,
> > +	.release =3D single_release,
> > +};
>=20
> Wondering if you can use DEFINE_SHOW_STORE_ATTRIBUTE(), or if it makes se=
nse.
> It might be that it requires to use DEFINE_SHOW_ATTRIBUTE() for the sake =
of
> consistency, but I don't remember if there is a difference WRT debugfs us=
age.

Well, then we can just leave it for now and change it later, if desired.

> > +	mutex_init(&priv->blob_lock);
>=20
> devm_mutex_init()

OK.

> > +		new_meta =3D devm_krealloc(dev, meta, meta_len + add_len, GFP_KERNEL=
);
>=20
> Can it be rewritten to use devm_krealloc_array()?

'meta' is not an array?

> > +	dev_info(dev, "initialized");
>=20
> Do we need this? Existence of folder in debugfs is enough indication of
> success, no?

Since the script can now list instances easily, this can be argued.
Still, I don't think this matters much for a debug-only device.

> > +static const struct of_device_id gpio_la_poll_of_match[] =3D {
> > +	{ .compatible =3D GPIO_LA_NAME, },
>=20
> Redundant inner comma.

Yes.

> > +late_initcall(gpio_la_poll_init);
>=20
> Why? Can we add a comment?

Sure.

> Btw, have you tried `shellcheck` against your script?

We did this in one of the 8 previous iterations.

All the best,

   Wolfram


--wj5fobjlv4oflgqg
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEOZGx6rniZ1Gk92RdFA3kzBSgKbYFAmZpcm8ACgkQFA3kzBSg
KbZG+Q/8DO5hqL7b1XR0sD1SMJviOoyciDYILbF19jHtnENPh96Sexa7fj5HdsfE
k8oyoeTUPX4LPLgFOTmr5SRHmiWjEb5s6oTgtNxJZqNFeFtglEK1QWUM5tANwQoy
L4S7bTO8J91J3WwdNChdvDQkfSLbAVGUTBvbm4iZcDihOCtGdScZCm+hGF1LqTxK
LE/2/8mwKfFymnGSbH/siPsb+CmtXMHuJNOhJlaI0KvNLIfHINkYJpBSPwMP3flV
v/VIqwt39AIm+5TbTPazDcrOY4NciJ9nRIjLfqHxTSnIbCZaMUfqPK/CYgLWTmD4
oFVvFZFP77yaVpPWjdkECDJJtfI9kkCMH+DgH7p6+6YoRhb2xDFWetS1JS25xJYR
vFodZggVj0wEsjqPb2GiaZRM+A+D6GEtjNszeDj3zlCInV6luHTcRuJ4NoNDLVNZ
XsFdHefQWoRv0/Kv5FY79tijNUNbGcwemt5d4wi4VFqfGZti5IFCbcb+/G+h18Hw
+EDb3KWhybHNENl3QPn0hYVCqDOAQrKInUcqVR0Ph0uYTzxMmfTLdk2wIX9I3+Li
fF7tI/JGxDHXZaT8KiQtFv9PEXgYheqLt7EdzSkTBrbydR9rcLg5l1cSy5Np8F7u
d+zalrnyfGKSxIxMGX16RQkMIya4Xd2R620+e64o6eTV7gV36O8=
=EuJ1
-----END PGP SIGNATURE-----

--wj5fobjlv4oflgqg--