Re: [PATCH] gpio: sim: don't fiddle with GPIOLIB private members

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Fri, Sep 1, 2023 at 11:10 PM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> On Fri, Sep 01, 2023 at 08:32:40PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> >
> > We access internals of struct gpio_device and struct gpio_desc because
> > it's easier but it can actually be avoided and we're working towards a
> > better encapsulation of GPIO data structures across the kernel so let's
> > start at home.
> >
> > Instead of checking gpio_desc flags, let's just track the requests of
> > GPIOs in the driver. We also already store the information about
> > direction of simulated lines.
> >
> > For kobjects needed by sysfs callbacks: we can leverage the fact that
> > once created for a software node, struct device is accessible from that
> > fwnode_handle. We don't need to dereference gpio_device.
> >
> > While at it: fix one line break and remove the untrue part about
> > configfs callbacks using dev_get_drvdata() from a comment.
>
> ...
>
> > -static void gpio_sim_free(struct gpio_chip *gc, unsigned int offset)
>
> Why is this?
>

Dunno, some git shenanigans?

> > +static int gpio_sim_request(struct gpio_chip *gc, unsigned int offset)
> >  {
> >       struct gpio_sim_chip *chip = gpiochip_get_data(gc);
> >
> >       scoped_guard(mutex, &chip->lock)
> > +             __set_bit(offset, chip->request_map);
> > +
> > +     return 0;
> > +}
> > +
> > +static void gpio_sim_free(struct gpio_chip *gc, unsigned int offset)
> > +{
> > +     struct gpio_sim_chip *chip = gpiochip_get_data(gc);
> > +
> > +     scoped_guard(mutex, &chip->lock) {
> >               __assign_bit(offset, chip->value_map,
> >                            !!test_bit(offset, chip->pull_map));
> > +             __clear_bit(offset, chip->request_map);
> > +     }
> >  }
>
> Seems to me like you. shuffled the order of the two functions.
> Can you leave _free() at the same location in the file?
>

I didn't. Request comes before free but they're next to each other.

> ...
>
> > -     /* Used by sysfs and configfs callbacks. */
> > -     dev_set_drvdata(&gc->gpiodev->dev, chip);
> > +     /* Used by sysfs callbacks. */
> > +     dev_set_drvdata(swnode->dev, chip);
>
> dev pointer of firmware node is solely for dev links. Is it the case here?
> Seems to me you luckily abuse it.
>

I don't think so. If anything we have a helper in the form of
get_dev_from_fwnode() but it takes reference to the device while we
don't need it - we know it'll be there because we created it.

This information (struct device of the GPIO device) can also be
retrieved by iterating over the device children of the top platform
device and comparing their fwnodes against the one we got passed down
from probe() but it's just so many extra steps.

Or we can have a getter in gpio/driver.h for that but I don't want to
expose another interface is we can simply use the fwnode.

Bart

> --
> With Best Regards,
> Andy Shevchenko
>
>