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

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Fri, Jul 30, 2021 at 09:57:04PM +0200, Wolfram Sang wrote:

...

> > '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?

Nope. Below is the compile-tested one:

    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",
                    },
                },
            }
        })
    }


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

Very slowly but yes, the pin configuration from ACPI to pin control is not
forgotten.

...

> > > +	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.

Add a comment then.

...

> > > +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?

If you know the name of the folder, you may look up it, no need to keep a
variable for that.

...

> > > +		/* '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.

As I said, it is a nasty side effect that may provoke real bugs in the future
with simple realloc() cases.

...

> > > +	[ ! -d $CPUSETDIR ] && mkdir $CPUSETDIR
> > 
> > [ -d ... ] || ...
> 
> Will think about it. I think the former is a tad more readable.

Shell is nice when the script is a) short, b) readable. Neither I see in the
former, sorry. Ah, and there is subtle difference between two. You may easily
learn it if you start using -efu flags in shebang.

-- 
With Best Regards,
Andy Shevchenko