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

From: Andy Shevchenko
Date: Fri Mar 26 2021 - 07:27:51 EST


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