linux-renesas-soc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Wolfram Sang <wsa+renesas@sang-engineering.com>,
	linux-kernel@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
	linux-gpio@vger.kernel.org,
	Linus Walleij <linus.walleij@linaro.org>,
	Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
Subject: Re: [RFC PATCH v2 1/1] misc: add sloppy logic analyzer using polling
Date: Fri, 30 Jul 2021 23:45:06 +0300	[thread overview]
Message-ID: <YQRk0vpo1V709z/Z@smile.fi.intel.com> (raw)
In-Reply-To: <YQRZkFApESOIMRmv@ninjato>

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



  reply	other threads:[~2021-07-30 20:45 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-19 13:25 [RFC PATCH v2 0/1] gpio: add simple " Wolfram Sang
2021-05-19 13:25 ` [RFC PATCH v2 1/1] misc: add sloppy " Wolfram Sang
2021-05-19 14:49   ` Andy Shevchenko
2021-07-30 19:57     ` Wolfram Sang
2021-07-30 20:45       ` Andy Shevchenko [this message]
2021-08-10  8:32         ` Wolfram Sang
2021-08-11 12:40           ` Andy Shevchenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YQRk0vpo1V709z/Z@smile.fi.intel.com \
    --to=andriy.shevchenko@linux.intel.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=ulrich.hecht+renesas@gmail.com \
    --cc=wsa+renesas@sang-engineering.com \
    --subject='Re: [RFC PATCH v2 1/1] misc: add sloppy logic analyzer using polling' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).