LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Wolfram Sang <wsa+renesas@sang-engineering.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: 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 21:57:04 +0200	[thread overview]
Message-ID: <YQRZkFApESOIMRmv@ninjato> (raw)
In-Reply-To: <YKUlbsWhT45l5Zm0@smile.fi.intel.com>

[-- Attachment #1: Type: text/plain, Size: 4503 bytes --]

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@sang-engineering.com>
> > + * 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

  reply	other threads:[~2021-07-30 19:57 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 [this message]
2021-07-30 20:45       ` Andy Shevchenko
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=YQRZkFApESOIMRmv@ninjato \
    --to=wsa+renesas@sang-engineering.com \
    --cc=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 \
    --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).