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