From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
Cc: Matti Vaittinen <mazziesaccount@gmail.com>,
Lee Jones <lee.jones@linaro.org>,
Rob Herring <robh+dt@kernel.org>,
Linus Walleij <linus.walleij@linaro.org>,
Bartosz Golaszewski <bgolaszewski@baylibre.com>,
devicetree <devicetree@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
linux-power <linux-power@fi.rohmeurope.com>,
"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>
Subject: Re: [PATCH v4 09/16] gpio: support ROHM BD71815 GPOs
Date: Fri, 26 Mar 2021 13:26:26 +0200 [thread overview]
Message-ID: <CAHp75VdxRkX15Ts+L1UJdXbpoaTu3Ue6o9o=Yh2cRCXCEi1jwA@mail.gmail.com> (raw)
In-Reply-To: <b2164e5965218f270e17bf29e00ad5c5a0b54bcf.1616566395.git.matti.vaittinen@fi.rohmeurope.com>
On Wed, Mar 24, 2021 at 12:20 PM Matti Vaittinen
<matti.vaittinen@fi.rohmeurope.com> 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@embest-tech.com>
> 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@embest-tech.com>");
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
next prev parent reply other threads:[~2021-03-26 11:27 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-24 7:21 [PATCH v4 00/16] Support ROHM BD71815 PMIC Matti Vaittinen
2021-03-24 7:23 ` [PATCH v4 03/16] dt_bindings: bd71828: Add clock output mode Matti Vaittinen
2021-03-24 7:25 ` [PATCH v4 06/16] mfd: Add ROHM BD71815 ID Matti Vaittinen
2021-03-24 7:26 ` [PATCH v4 07/16] mfd: Sort ROHM chip ID list for better readability Matti Vaittinen
2021-03-24 7:26 ` [PATCH v4 08/16] mfd: Support for ROHM BD71815 PMIC core Matti Vaittinen
2021-03-24 7:29 ` [PATCH v4 09/16] gpio: support ROHM BD71815 GPOs Matti Vaittinen
2021-03-25 9:35 ` Linus Walleij
2021-03-25 10:32 ` Matti Vaittinen
2021-05-10 12:58 ` regmap-gpio: Support set_config and other not quite so standard ICs? Matti Vaittinen
2021-05-10 16:54 ` Andy Shevchenko
2021-05-11 3:59 ` Matti Vaittinen
2021-05-14 20:34 ` Michael Walle
2021-05-17 4:46 ` Vaittinen, Matti
2021-03-26 11:26 ` Andy Shevchenko [this message]
2021-03-26 11:27 ` [PATCH v4 09/16] gpio: support ROHM BD71815 GPOs Andy Shevchenko
2021-03-26 13:33 ` Matti Vaittinen
2021-03-26 17:51 ` Andy Shevchenko
2021-03-28 16:59 ` Matti Vaittinen
2021-03-24 7:31 ` [PATCH v4 16/16] MAINTAINERS: Add ROHM BD71815AGW Matti Vaittinen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAHp75VdxRkX15Ts+L1UJdXbpoaTu3Ue6o9o=Yh2cRCXCEi1jwA@mail.gmail.com' \
--to=andy.shevchenko@gmail.com \
--cc=bgolaszewski@baylibre.com \
--cc=devicetree@vger.kernel.org \
--cc=lee.jones@linaro.org \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-power@fi.rohmeurope.com \
--cc=matti.vaittinen@fi.rohmeurope.com \
--cc=mazziesaccount@gmail.com \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).