Re: [PATCH v9 1/1] gpio: add sloppy logic analyzer using polling

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux