On Mon, Mar 29, 2021 at 3:58 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
It seems you ignored my comments about the commit message. :-(
> +struct bd71815_gpio {
> + struct gpio_chip chip;
> + struct device *dev;
Wondering why you need this. Is it the same as chip.parent?
> + struct regmap *regmap;
> +};
...
> + int ret, bit;
> +
> + bit = BIT(offset);
I prefer
int bit = BIT(offset);
int ret;
but I think we already discussed that. OK.
...
> + default:
> + break;
> + }
> + return -ENOTSUPP;
Here is a waste of line. Why break instead of direct return?
...
> +/* Template for GPIO chip */
> +static const struct gpio_chip bd71815gpo_chip = {
> + .label = "bd71815",
> + .owner = THIS_MODULE,
> + .get = bd71815gpo_get,
> + .get_direction = bd71815gpo_direction_get,
> + .set = bd71815gpo_set,
> + .set_config = bd71815_gpio_set_config,
> + .can_sleep = 1,
Strictly speaking this should be true (boolean type value).
> +};
...
> +#define BD71815_TWO_GPIOS 0x3UL
> +#define BD71815_ONE_GPIO 0x1UL
Are they masks? Can you use BIT() and GENMASK()?
...
> +/*
> + * 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 in the
and the pin
> + * datasheet. The functionality is still there though! I guess driving a GPO
> + * connected to the ground is a bad idea. Thus 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".
> + */
...
> + /*
> + * As writing of this the sysfs interface for GPIO control does not
> + * respect the valid_mask. Do not trust it but rather set the ngpios
> + * to 1 if "rohm,enable-hidden-gpo" is not given.
> + *
> + * This check can be removed later if the sysfs export is fixed and
> + * if the fix is backported.
So, mark this comment with the TODO/FIXME keyword?
> + *
> + * For now it is safest to just set the ngpios though.
> + */
...
> + 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;
This entire piece can be simplified by
return devm_gpiochip_add_data(...);
...
> +static struct platform_driver gpo_bd71815_driver = {
> + .driver = {
> + .name = "bd71815-gpo",
> + .owner = THIS_MODULE,
Seems I commented on this. The module_*_driver() macro(s) will take care of it.
> + },
> + .probe = gpo_bd71815_probe,
> +};
> +
Extra blank line. Drop it.
> +module_platform_driver(gpo_bd71815_driver);
--
With Best Regards,
Andy Shevchenko
