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

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

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

Hi Andy,

finally I found some time to get back to this one. For anyhting I didn't
comment on, it means I am okay with your suggestion. Thanks for the
review!

> 'For ACPI one may use PRP0001 approach with the following ASL excerpt example::
> 
>     Device (GSLA) {
>         Name (_HID, "PRP0001")
>         Name (_DDN, "GPIO sloppy logic analyzer")
>         Name (_CRS, ResourceTemplate () {
>             GpioIo(Exclusive, PullNone, 0, 0, IoRestrictionNone,
>                 "\\_SB.PCI0.GPIO", 0, ResourceConsumer, , ) { 13 }
>             PinConfig(Exclusive, 0x07, 0, "\\_SB.PCI0.GPIO", 0, ResourceConsumer, ) { 7 }
>             GpioIo(Exclusive, PullNone, 0, 0, IoRestrictionNone,
>                 "\\_SB.PCI0.GPIO", 0, ResourceConsumer, , ) { 12 }
>             PinConfig(Exclusive, 0x07, 0, "\\_SB.PCI0.GPIO", 0, ResourceConsumer, ) { 6 }
>         })
> 
>         Name (_DSD, Package () {
>             ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>             Package () {
>                 Package () { "compatible", Package () { "gpio-sloppy-logic-analyzer" } },
>                 Package () {
>                     "probe-gpios", Package () {
>                         ^GSLA, 0, 0, 0,
>                         ^GSLA, 1, 0, 0,
>                     },
>                 Package () {
>                     "probe-names", Package () {
>                         "SCL",
>                         "SDA",
>                     },
>             }
>         })
> 
> Note, that pin configuration uses pin numbering space, while GPIO resources
> are in GPIO numbering space, which may be different in ACPI. In other words,
> there is no guarantee that GPIO and pins are mapped 1:1, that's why there are
> two different pairs in the example, i.e. {13,12} GPIO vs. {7,6} pin.
> 
> Yet pin configuration support in Linux kernel is subject to implement.'

Have you tested this snippet? I am totally open to add ACPI but it
should be tested, of course. Is there any on-going effort to add ACPI
pin config?

> > + * Copyright (C) Wolfram Sang <wsa@sang-engineering.com>
> > + * Copyright (C) Renesas Electronics Corporation
> 
> No years?

After reading this*, I agreed they are not really needed.

* https://www.linuxfoundation.org/blog/copyright-notices-in-open-source-software-projects/


> > +#define GPIO_LA_MAX_PROBES 8
> > +#define GPIO_LA_NUM_TESTS 1024
> 
> I prefer TAB indentation of the values for better reading, but it's up to you.

I don't ;)

> > +	struct debugfs_blob_wrapper meta;
> > +	unsigned long gpio_acq_delay;
> > +	struct device *dev;
> 
> > +	unsigned int trig_len;
> 
> On 64-bit arch you may save 4 bytes by moving this to be together with u32
> member above.

I don't want to save bytes here. I sorted the struct for cachelines,
important members first.

> > +static struct dentry *gpio_la_poll_debug_dir;
> 
> I have seen the idea of looking up the debugfs entry. That said, do we actually
> need this global variable?

I don't understand the first sentence. And we still need it to clean up?


> > +		/* '10' is length of 'probe00=\n\0' */
> > +		add_len = strlen(gpio_names[i]) + 10;
> > +		meta = devm_krealloc(dev, meta, meta_len + add_len, GFP_KERNEL);
> 
> First of all, this realloc() pattern *) is bad. While it's tricky and has side
> effects (i.e. it has no leaks) better not to use it to avoid confusion.
> 
> *) foo = realloc(foo, ...); is 101 mistake.

Because generally you lose the old pointer on error. But we don't here
because we are using managed devices.

However, I see that all kernel users of devm_krealloc() are using a
seperate variable and then update the old one. I can do this, too.

> But second, all your use is based on:
>  - all strings are of equal lengths

They are not. The gpio names come from the user via DT or ACPI and can
have an arbitrary length.

> > +	[ ! -d $CPUSETDIR ] && mkdir $CPUSETDIR
> 
> [ -d ... ] || ...

Will think about it. I think the former is a tad more readable.

> > +			# Check if we could parse something and the channel number fits
> > +			[ $chan != $c -a $chan -le $MAX_CHANS ] 2> /dev/null || { echo "Syntax error: $c" && exit 1; }
> 
> Why 2>/dev/null ?

I forgot, have to recheck.

> > +[ $SAMPLEFREQ -eq 0 ] &&
> 
>  echo "Invalid sample frequency" && exit 1
> 
> This kind of stuff deserves an exit function, like

I'll think about it!

All the best,

   Wolfram


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