Re: [PATCH v4 09/16] gpio: support ROHM BD71815 GPOs

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Wed, Mar 24, 2021 at 12:20 PM Matti Vaittinen
<matti.vaittinen@xxxxxxxxxxxxxxxxx> wrote:
>
> Support GPO(s) found from ROHM BD71815 power management IC. The IC has two
> GPO pins but only one is properly documented in data-sheet. The driver

in the datasheet

> exposes by default only the documented GPO. The second GPO is connected to
> E5 pin and is marked as GND in data-sheet. Control for this undocumented

in the datasheet

> pin can be enabled using a special DT property.
>
> This driver is derived from work by Peter Yang <yanglsh@xxxxxxxxxxxxxxx>
> although not so much of original is left.

of the original

Below my comments independently on the fact if this driver will be
completely rewritten, consider them as a good practice for your new
contribution.

...

> +/*
> + * Support to GPOs on ROHM BD71815
> + */

This is effectively one line.

...

> +/* For the BD71815 register definitions */
> +#include <linux/mfd/rohm-bd71815.h>

Since it's component specific header(s) I would move it to a separate
group and locate...

> +#include <linux/module.h>

> +#include <linux/of.h>

You may do better than be OF-centric. See below.

> +#include <linux/platform_device.h>
> +

...somewhere here.

...

> +       /*
> +        * Sigh. The BD71815 and BD71817 were originally designed to support two
> +        * GPO pins. At some point it was noticed the second GPO pin which is
> +        * the E5 pin located at the center of IC is hard to use on PCB (due to
> +        * the location). It was decided to not promote this second GPO and pin
> +        * is marked as GND on the data-sheet. The functionality is still there
> +        * though! I guess driving GPO connected to ground is a bad idea. Thus

a GPO
to the ground

> +        * we do not support it by default. OTOH - the original driver written
> +        * by colleagues at Embest did support controlling this second GPO. It
> +        * is thus possible this is used in some of the products.
> +        *
> +        * This driver does not by default support configuring this second GPO
> +        * but allows using it by providing the DT property
> +        * "rohm,enable-hidden-gpo".
> +        */

...

> +       int ret = 0;

Redundant assignment.

> +       int val;
> +
> +       ret = regmap_read(bd71815->regmap, BD71815_REG_GPO, &val);
> +       if (ret)
> +               return ret;

> +       return (val >> offset) & 1;

!!(val & BIT(offset)) can also work and be in alignment with the below code.

...

> +       if (!bd71815->e5_pin_is_gpo && offset)
> +               return;

I wonder if you can use valid_mask instead of this.

...

> +       bit = BIT(offset);

Can be moved to the definition block.

...

> +       if (!bdgpio->e5_pin_is_gpo && offset)
> +               return -EOPNOTSUPP;

As above.

...

> +       default:
> +               break;
> +       }
> +       return -EOPNOTSUPP;

You may return directly from default.

...

> +       int ret;
> +       struct bd71815_gpio *g;
> +       struct device *dev;
> +       struct device *parent;

Reversed xmas tree order.

...

> +       /*
> +        * Bind devm lifetime to this platform device => use dev for devm.
> +        * also the prints should originate from this device.
> +        */

Why is this comment needed?

...

> +       dev = &pdev->dev;

Can be done in the definition block.

...

> +       /* The device-tree and regmap come from MFD => use parent for that */

Why do you need this comment?

> +       parent = dev->parent;

Ditto, can be moved to the definition block.

...

> +       g->e5_pin_is_gpo = of_property_read_bool(parent->of_node,
> +                                                "rohm,enable-hidden-gpo");

You may use device_property_read_bool().

...

> +       g->chip.of_node = parent->of_node;

Redundant. GPIO library does it for you and even better.

...

> +       ret = devm_gpiochip_add_data(dev, &g->chip, g);
> +       if (ret < 0) {
> +               dev_err(dev, "could not register gpiochip, %d\n", ret);
> +               return ret;
> +       }
> +
> +       return ret;

It's as simply as
return devm_gpiochip_add_data(...);

...

> +static const struct platform_device_id bd7181x_gpo_id[] = {
> +       { "bd71815-gpo" },

> +       { },

No comma for the terminator line.

> +};
> +MODULE_DEVICE_TABLE(platform, bd7181x_gpo_id);

Why do you need this ID table exactly?
You have the same name as in the platform driver structure below.

> +static struct platform_driver gpo_bd71815_driver = {
> +       .driver = {
> +               .name   = "bd71815-gpo",

> +               .owner  = THIS_MODULE,

This is done by module_*_driver() macros, drop it.

> +       },
> +       .probe          = gpo_bd71815_probe,
> +       .id_table       = bd7181x_gpo_id,
> +};

> +

Extra blank line.

> +module_platform_driver(gpo_bd71815_driver);

> +/* Note:  this hardware lives inside an I2C-based multi-function device. */
> +MODULE_ALIAS("platform:bd71815-gpo");

> +

Ditto.

> +MODULE_AUTHOR("Peter Yang <yanglsh@xxxxxxxxxxxxxxx>");

And I don't see a match with a committer/submitter/co-developer/etc.
Please, make corresponding fields and this macro (or macros, you may
have as many MODULE_AUTHOR() entries as developers of the code)
aligned to each other.

> +MODULE_DESCRIPTION("GPO interface for BD71815");
> +MODULE_LICENSE("GPL");

-- 
With Best Regards,
Andy Shevchenko