Fri, May 26, 2023 at 05:06:03PM +0200, Jerome Neanne kirjoitti: > Add support for TPS65219 PMICs GPIO interface. > > 3 GPIO pins: > - GPIO0 only is IO but input mode reserved for MULTI_DEVICE_ENABLE usage > - GPIO1 and GPIO2 are Output only and referred as GPO1 and GPO2 in spec > > GPIO0 is statically configured as input or output prior to Linux boot. > it is used for MULTI_DEVICE_ENABLE function. > This setting is statically configured by NVM. > GPIO0 can't be used as a generic GPIO (specification Table 8-34). > It's either a GPO when MULTI_DEVICE_EN=0, > or a GPI when MULTI_DEVICE_EN=1. > Datasheet describes specific usage for non standard GPIO. > Link: https://www.ti.com/lit/ds/symlink/tps65219.pdf > This blank line makes Link: above not to be a tag. Tag block mustn't have blank lines. OTOH, the other text must be delimited by a blank line before the tag block. That said, move this blank line to before Link: line. > Co-developed-by: Jonathan Cormier <jcormier@xxxxxxxxxxxxxxxx> > Signed-off-by: Jonathan Cormier <jcormier@xxxxxxxxxxxxxxxx> > Signed-off-by: Jerome Neanne <jneanne@xxxxxxxxxxxx> ... > + help > + Select this option to enable GPIO driver for the TPS65219 > + chip family. > + GPIO0 is statically configured as input or output prior to Linux boot. > + it is used for MULTI_DEVICE_ENABLE function. > + This setting is statically configured by NVM. > + GPIO0 can't be used as a generic GPIO. > + It's either a GPO when MULTI_DEVICE_EN=0, > + or a GPI when MULTI_DEVICE_EN=1. This is strange indentation, we have longer lines, why not using all room available? Btw, seems the commit message itself has the same issue. > + This driver can also be built as a module. If so, the > + module will be called gpio_tps65219. ... Missing bits.h > +#include <linux/gpio/driver.h> > +#include <linux/mfd/tps65219.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> ... > +static int tps65219_gpio_get(struct gpio_chip *gc, unsigned int offset) > +{ > + struct tps65219_gpio *gpio = gpiochip_get_data(gc); With struct device *dev = gpio->tps->dev; you may make some code blocks shorter. > + int ret, val; > + > + if (offset != TPS65219_GPIO0_IDX) { > + dev_err(gpio->tps->dev, > + "GPIO%d is output only, cannot get\n", > + offset); Like here. > + return -EOPNOTSUPP; > + } > + > + ret = regmap_read(gpio->tps->regmap, TPS65219_REG_MFP_CTRL, &val); > + if (ret) > + return ret; > + > + dev_warn(gpio->tps->dev, > + "GPIO%d = %d, used for MULTI_DEVICE_ENABLE, not as standard GPIO\n", > + offset, !!(val & BIT(TPS65219_MFP_GPIO_STATUS_MASK))); Isn't it the same... > + /* depends on NVM config return error if dir output else the GPIO0 status bit */ > + if (tps65219_gpio_get_direction(gc, offset) == TPS65219_GPIO_DIR_OUT) > + return -EOPNOTSUPP; > + > + return !!(val & BIT(TPS65219_MFP_GPIO_STATUS_MASK)); ...as this one? What the point to evaluate it twice? > +} ... > + dev_err(gpio->tps->dev, "GPIO%d, set to value %d failed.\n", offset, value); Yeah, there is an inconsistency between line lengths in different functions. Define for yourself the style 80 or 100 and use it everywhere. ... > + /* Documentation is stating that GPIO0 direction must not be changed in Linux: > + * Table 8-34. MFP_1_CONFIG(3): MULTI_DEVICE_ENABLE, > + * Should only be changed in INITIALIZE state (prior to ON Request). > + * Set statically by NVM, changing direction in application can cause a hang. > + * Below can be used for test purpose only: > + */ > + /* * The style of multi-line comment * is incorrect. See this example. * Besides that, remove unneeded * blank line above. */ > + if (IS_ENABLED(DEBUG)) { > + int ret = regmap_update_bits(gpio->tps->regmap, TPS65219_REG_MFP_1_CONFIG, > + TPS65219_GPIO0_DIR_MASK, direction); > + if (ret) { > + dev_err(gpio->tps->dev, > + "DEBUG enabled: Fail to change direction to %u for GPIO%d. \ > + For test only\n", Do not split string literals on non-\n characters. > + direction, offset); > + return ret; Wrong indentation. > + } > + } -- With Best Regards, Andy Shevchenko
