lkml.org 
[lkml]   [2021]   [Jul]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH v2 1/1] misc: add sloppy logic analyzer using polling
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


\
 
 \ /
  Last update: 2021-07-30 22:45    [W:5.032 / U:0.020 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site