Received-SPF: pass (google.com: domain of linux-kernel+bounces-213791-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240527144054.155503-1-brgl@bgdev.pl> <ZleXc6tLbiWQ59i-@surfacebook.localdomain>
 <CAMRc=MftW0y7GicBy4vwABomUYuMndsJBUTdsQzZijDtgX1ohQ@mail.gmail.com>
 <ZmcUbe1aQfezZy5B@surfacebook.localdomain> <CAMRc=McjAAFX1R4wAbLu5bcUkpPuy8rf=fwDOqCiUY1hbjmhMg@mail.gmail.com>
In-Reply-To: <CAMRc=McjAAFX1R4wAbLu5bcUkpPuy8rf=fwDOqCiUY1hbjmhMg@mail.gmail.com>
From: Andy Shevchenko <andy.shevchenko@gmail.com>
Date: Thu, 13 Jun 2024 20:03:21 +0200
Message-ID: <CAHp75Vec7D2EGVt+Mo-85FN4TEjMRkMBbJxT_f7FX5GJeYvidA@mail.gmail.com>
Subject: Re: [PATCH v7] gpio: virtuser: new virtual driver
To: Bartosz Golaszewski <brgl@bgdev.pl>
Cc: Kent Gibson <warthog618@gmail.com>, Linus Walleij <linus.walleij@linaro.org>, 
	linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org, 
	Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Jun 12, 2024 at 8:38=E2=80=AFPM Bartosz Golaszewski <brgl@bgdev.pl>=
 wrote:
> On Mon, Jun 10, 2024 at 4:57=E2=80=AFPM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > Mon, Jun 10, 2024 at 03:22:32PM +0200, Bartosz Golaszewski kirjoitti:
> > > On Wed, May 29, 2024 at 11:00=E2=80=AFPM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > Mon, May 27, 2024 at 04:40:54PM +0200, Bartosz Golaszewski kirjoitt=
i:

...

> > > > > +static ssize_t gpio_virtuser_sysfs_emit_value_array(char *buf,
> > > > > +                                                 unsigned long *=
values,
> > > > > +                                                 size_t num_valu=
es)
> > > > > +{
> > > > > +     ssize_t len =3D 0;
> > > > > +     size_t i;
> > > > > +
> > > > > +     for (i =3D 0; i < num_values; i++)
> > > > > +             len +=3D sysfs_emit_at(buf, len, "%d",
> > > > > +                                  test_bit(i, values) ? 1 : 0);
> > > > > +     return len + sysfs_emit_at(buf, len, "\n");
> > > >
> > > > Why not use %pb?
> > >
> > > Because it outputs hex? I want to output binary, can I do it?
> >
> > But why do you need that? You can also print a list of numbers of bits =
that
> > set (%pbl).
> >
> > We have a few ABIs in the kernel that works nice and people are familia=
r with
> > (CPU sets, IRQ affinity masks, etc). Why to reinvent the wheel?
>
> If I see "11001011" as output, I can immediately convert that to pins
> in my head. If I see 0xcb, I need to use a calculator.

From my experience I see the opposite, as the hex values are
condensed, it will be easy to get lost in binary > 8 bits. Despite
that, I am really against Yet Another Parsers in the kernel. Here I do
not see a good justification for one more. As I pointed out the kernel
has very known bitmap ABIs that users are familiar with. Are you
creating a driver for yourself or for a wider audience?

> > > > > +}

...

> > > > > +static int gpio_virtuser_sysfs_parse_value_array(const char *buf=
, size_t len,
> > > > > +                                              unsigned long *val=
ues)
> > > > > +{
> > > > > +     size_t i;
> > > > > +
> > > > > +     for (i =3D 0; i < len; i++) {
> > > >
> > > > Perhaps
> > > >
> > > >                 bool val;
> > > >                 int ret;
> > > >
> > > >                 ret =3D kstrtobool(...);
> > >
> > > kstrtobool() accepts values we don't want here like [Tt]rue and [Ff]a=
lse.
> >
> > Yes, see below.
> >
> > > >                 if (ret)
> > > >                         return ret;
> > > >
> > > >                 assign_bit(...); // btw, why atomic?
> > > >
> > > > > +             if (buf[i] =3D=3D '0')
> > > > > +                     clear_bit(i, values);
> > > > > +             else if (buf[i] =3D=3D '1')
> > > > > +                     set_bit(i, values);
> > > > > +             else
> > > > > +                     return -EINVAL;
> > > >
> > > > > +     }
> > > >
> > > > BUT, why not bitmap_parse()?
> > >
> > > Because it parses hex, not binary.
> >
> > So, why do we reinvent a wheel? Wouldn't be better that users may apply=
 the
> > knowledge they are familiar with (and I believe the group of the users =
who know
> > about bitmaps is much bigger than those who will use this driver).

You see, you ignored this comment.

> > > > > +     return 0;
> > > > > +}

...

> > At bare minumum you can reduce the range by using kstrtou8().
>
> As opposed to just checking '0' and '1'? Meh...

Okay, can you explain why you need to take bigger numbers when it's
going to be used only in a very reduced range? Esp. negative ones.
Whatever, your choice.

...

> > > > > +     int i =3D 0;
> > > >
> > > > Why signed? And in all this kind of case, I would split assignment.=
..
> >
> > (1)
> >
> > > > > +     memset(properties, 0, sizeof(properties));
> > > > > +
> > > > > +     num_ids =3D list_count_nodes(&dev->lookup_list);
> > > > > +     char **ids __free(kfree) =3D kcalloc(num_ids + 1, sizeof(*i=
ds),
> > > > > +                                        GFP_KERNEL);
> > > > > +     if (!ids)
> > > > > +             return ERR_PTR(-ENOMEM);
> > > > > +
> > > >
> > > > To be here, that the reader will see immediately (close enough) wha=
t is the
> > > > initial values. Moreover this code will be robuse against changes i=
n between
> > > > (if i become reusable).
> > >
> > > Sorry, I can't parse it.
> >
> > I meant to see here
> >
> >         i =3D 0;
> >
> > instead of the above (1).
>
> Why? I see no good reason.

There are two:
1) readability as explained above;
2) maintenance.

The second one is good in case the same variable (that's the very
often case for such as "i" is going to be reused in the future by some
new code. But okay, in this case you probably will be the only one for
maintenance.

> > > > > +     list_for_each_entry(lookup, &dev->lookup_list, siblings)
> > > > > +             ids[i++] =3D lookup->con_id;


--=20
With Best Regards,
Andy Shevchenko