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@xxxxxxxxxxxxxxxxxxxx>
> > + * 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:
signature.asc
Description: PGP signature
