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? > +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? ... > - /* 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. -- With Best Regards, Andy Shevchenko