Thread (7 messages) 7 messages, 2 authors, 2021-08-11

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 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/

quoted
+#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 ;)
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

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help