>> +static int cgbc_gpio_get(struct gpio_chip *chip, unsigned int offset) >> +{ >> + struct cgbc_gpio_data *gpio = gpiochip_get_data(chip); >> + struct cgbc_device_data *cgbc = gpio->cgbc; >> + int ret; >> + u8 val; >> + > > Can you use scoped_guard() here and elsewhere? Hi Bartosz, Thanks for the review. For the next iteration I added scoped_guard() in cgbc_gpio_get(), and guard() in cgbc_gpio_set(), cgbc_gpio_direction_input(), and cgbc_gpio_direction_output(). > >> + mutex_lock(&gpio->lock); >> + >> + ret = cgbc_gpio_cmd(cgbc, CGBC_GPIO_CMD_GET, (offset > 7) ? 1 : 0, 0, &val); >> + >> + mutex_unlock(&gpio->lock); >> + >> + offset %= 8; >> + >> + if (ret) >> + return ret; ... >> +static int cgbc_gpio_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct cgbc_device_data *cgbc = dev_get_drvdata(dev->parent); >> + struct cgbc_gpio_data *gpio; >> + struct gpio_chip *chip; >> + int ret; >> + >> + gpio = devm_kzalloc(dev, sizeof(*gpio), GFP_KERNEL); >> + if (!gpio) >> + return -ENOMEM; >> + >> + gpio->cgbc = cgbc; >> + >> + platform_set_drvdata(pdev, gpio); >> + >> + chip = &gpio->chip; >> + chip->label = dev_name(&pdev->dev); >> + chip->owner = THIS_MODULE; >> + chip->parent = dev; >> + chip->base = -1; >> + chip->direction_input = cgbc_gpio_direction_input; >> + chip->direction_output = cgbc_gpio_direction_output; >> + chip->get_direction = cgbc_gpio_get_direction; >> + chip->get = cgbc_gpio_get; >> + chip->set = cgbc_gpio_set; >> + chip->ngpio = CGBC_GPIO_NGPIO; >> + >> + mutex_init(&gpio->lock); > > Please use devm_mutex_init() so that it gets cleaned up at exit. It's > not strictly necessary but helps with lock debugging. Fixed in the next iteration. Regards, Thomas -- Thomas Richard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com