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

[Wolfram Sang <wsa+renesas@sang-engineering.com>]

[-- Attachment #1: Type: text/plain, Size: 2741 bytes --]

Hi Andy,

thanks for the prompt review again!

> > +       /* upper limit is arbitrary */
> 
> Not really. I believe if the upper limit is > PAGE_SIZE, you would get
> -ENOMEM with much higher chances. So, I think the comment should be
> amended,

? Dunno, maybe it is not arbitrary that it is < PAGE_SIZE but other than
that the value I chose is arbitrary. There is no technical reason for
2048.

> 
> > +       if (count > 2048 || count & 1)
> > +               return -EINVAL;
> 
> ...
> 
> > +       ret = device_property_read_string_array(dev, "probe-names", gpio_names,
> > +                                               priv->descs->ndescs);
> > +       if (ret >= 0 && ret != priv->descs->ndescs)
> > +               ret = -ENODATA;
> > +       if (ret < 0) {
> 
> > +               dev_err(dev, "error naming the GPIOs: %d\n", ret);
> > +               return ret;
> > +       }
> 
> Perhaps
> 
>   return dev_err_probe() ?

Reading strings from DT can be deferred? I don't think so.

> And I think it might be split into two conditionals with
> distinguishable error messages.

I think "something is wrong with the names" is helpful enough for the
user.


> > +       dev_info(dev, "initialized");
> 
> Unneeded noise.

Nope, I added it because when I was adding more instances, this proved
useful. I'd agree if this is a regular driver. But this is a
only-for-special-case-debugging one.

> > +       [ -n "$cur_cpu" ] && fail "CPU$isol_cpu requested but CPU$cur_cpu already isolated"
> 
> For the sake of style (handle errors on the error) I would use
> 
> [ -z "..." ] || fail ...

I'll think about it. On first glimpse, this doesn't look more readable
to me. "if this is true then do that" is super readable in my book. But
yes, when calling external programs, I need '||' anyhow, true.

> > +       # Move tasks away from isolated CPU
> > +       for p in $(ps -o pid | tail -n +2); do
> > +               mask=$(taskset -p "$p") || continue
> > +               [ "${mask##*: }" != "$oldmask" ] && continue
> 
> Perhaps
> 
>   [ ... = ... ] || continue
> 
> to be in alignment with the rest of the similar lines here?

Yes.

> > +       *) fail "error parsing commandline: $*";;
> 
> command line

OK.

> > +if [ -n "$lainstance" ]; then
> 
> Shouldn't be rather '-d' ?

Nope, this is just a string for now. '-d' comes later with $lasysfsdir.

> > +[ ! -d "$lacpusetdir" ] && echo "Auto-Isolating CPU1" && init_cpu 1
> 
> This ! along with double && is hard to read. Simply

Same as above, will think about it. But "if there is not this directory,
then do a) and b)" is not hard to read in my book.

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]