Re: [PATCH 2/5] gpio: add driver for AAEON devices

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Tue, May 25, 2021 at 8:42 AM <aaeon.asus@xxxxxxxxx> wrote:
>
> From: Kunyang_Fan <kunyang_fan@xxxxxxxx>
>
> This patch add support for the GPIO pins whose control are
> transported to BIOS through ASUS WMI interface.
>
> Signed-off-by: Kunyang_Fan <kunyang_fan@xxxxxxxx>

Missed SoB of the submitter. Dunno if it's a critical issue.

...

> +obj-$(CONFIG_GPIO_AAEON)                += gpio-aaeon.o

Way too generic file name. So the configuration option name.

...

> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.

Same as for all patches.

...

> +static int aaeon_gpio_get_number(void);
> +static int aaeon_gpio_get_direction(struct gpio_chip *chip,
> +                                unsigned int offset);
> +static int aaeon_gpio_output_set_direction(struct gpio_chip *chip,
> +                                unsigned int offset, int value);
> +static int aaeon_gpio_input_set_direction(struct gpio_chip *chip,
> +                                unsigned int offset);
> +static int aaeon_gpio_get(struct gpio_chip *chip,
> +                                unsigned int offset);
> +static void aaeon_gpio_set(struct gpio_chip *chip, unsigned int offset,
> +                                int value);

Why forward declarations?!

...

> +static int aaeon_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
> +{

> +       int err, retval;
> +       u32 dev_id = 0x0;

Reversed xmas tree order.

> +       dev_id |= offset;

Just keep it in the definition block

       u32 dev_id = offset;

Ditto for all the rest similar lines.

> +       err = asus_wmi_evaluate_method(GET_DIRECTION_METHOD_ID, dev_id,
> +                                      0, &retval);
> +       if (err)
> +               return err;
> +
> +       return retval;
> +}

...

> +static int __init aaeon_gpio_probe(struct platform_device *pdev)
> +{
> +       int err, i;
> +       int dio_number = 0;
> +       struct aaeon_gpio_data *data;
> +       struct aaeon_gpio_bank *bank;

Reversed xmas tree order, same for the all similar places across the series.


> +       /* Prevent other drivers adding this platfom device */
> +       if (!wmi_has_guid(AAEON_WMI_MGMT_GUID)) {
> +               pr_debug("AAEON Management GUID not found\n");
> +               return -ENODEV;
> +       }

Dead code?

> +       dio_number = aaeon_gpio_get_number();
> +       if (dio_number < 0)
> +               return -ENODEV;

Why shadowing error code?

> +       data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       data->nr_bank = ARRAY_SIZE(aaeon_gpio_bank);
> +       data->bank = aaeon_gpio_bank;
> +       platform_set_drvdata(pdev, data);
> +       bank = &data->bank[0];
> +       bank->chip.parent = &pdev->dev;
> +       bank->chip.ngpio = dio_number;
> +       bank->data = data;

+ blank line

> +       err = devm_gpiochip_add_data(&pdev->dev, &bank->chip, bank);
> +       if (err)
> +               pr_debug("Failed to register gpiochip %d: %d\n", i, err);
> +
> +       return err;

Besides the fact of using dev_dbg() and another fact that it is not
needed (device core can print it for you)

return devm_...(...);

> +}

...

> +

Redundant blank line (same applies for entire series).

> +module_platform_driver_probe(aaeon_gpio_driver, aaeon_gpio_probe);

-- 
With Best Regards,
Andy Shevchenko