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
