Re: [RFC PATCH v2 1/1] misc: add sloppy logic analyzer using polling
From: Wolfram Sang <wsa+renesas@sang-engineering.com>
Date: 2021-07-30 19:57:13
Also in:
linux-renesas-soc, lkml
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?
quoted
+ * Copyright (C) Wolfram Sang [off-list ref] + * Copyright (C) Renesas Electronics CorporationNo years?
After reading this*, I agreed they are not really needed. * https://www.linuxfoundation.org/blog/copyright-notices-in-open-source-software-projects/
quoted
+#define GPIO_LA_MAX_PROBES 8 +#define GPIO_LA_NUM_TESTS 1024I prefer TAB indentation of the values for better reading, but it's up to you.
I don't ;)
quoted
+ struct debugfs_blob_wrapper meta; + unsigned long gpio_acq_delay; + struct device *dev;quoted
+ 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.
quoted
+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?
quoted
+ /* '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.
quoted
+ [ ! -d $CPUSETDIR ] && mkdir $CPUSETDIR[ -d ... ] || ...
Will think about it. I think the former is a tad more readable.
quoted
+ # 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.
quoted
+[ $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
Attachments
- signature.asc [application/pgp-signature] 833 bytes