Re: [PATCH 2/5] gpio: Congatec Board Controller gpio driver

[Thomas Richard <thomas.richard@bootlin.com>]

>> +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