[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <wdvnbt2vpobukz3s6pxkowoizvsxjeycnb3rmfsmfbx5zsgvrp@mog64h2ogftv>
Date: Wed, 12 Jun 2024 12:03:31 +0200
From: Wolfram Sang <wsa+renesas@...g-engineering.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: linux-renesas-soc@...r.kernel.org, Jonathan Corbet <corbet@....net>,
Linus Walleij <linus.walleij@...aro.org>, Bartosz Golaszewski <brgl@...ev.pl>,
Kent Gibson <warthog618@...il.com>, linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-gpio@...r.kernel.org
Subject: Re: [PATCH v9 1/1] gpio: add sloppy logic analyzer using polling
Hi Andi,
> > +#include <linux/delay.h>
>
> + device.h
> + err.h
OK about the includes.
> > + mutex_lock(&priv->blob_lock);
>
> 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 = {
> > + .owner = THIS_MODULE,
> > + .open = trigger_open,
> > + .write = trigger_write,
> > + .llseek = no_llseek,
> > + .release = single_release,
> > +};
>
> Wondering if you can use DEFINE_SHOW_STORE_ATTRIBUTE(), or if it makes sense.
> 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 usage.
Well, then we can just leave it for now and change it later, if desired.
> > + mutex_init(&priv->blob_lock);
>
> devm_mutex_init()
OK.
> > + new_meta = devm_krealloc(dev, meta, meta_len + add_len, GFP_KERNEL);
>
> Can it be rewritten to use devm_krealloc_array()?
'meta' is not an array?
> > + dev_info(dev, "initialized");
>
> 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[] = {
> > + { .compatible = GPIO_LA_NAME, },
>
> Redundant inner comma.
Yes.
> > +late_initcall(gpio_la_poll_init);
>
> 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
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists