| Message ID | 20230224113837.874264-1-jneanne@baylibre.com |
|---|---|
| Headers | show |
| Series | Add support for TI TPS65219 PMIC GPIO interface. | expand |
Fri, Feb 24, 2023 at 12:38:36PM +0100, 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. Linux > 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. Something wrong with the indentation. > Link: https://www.ti.com/lit/ds/symlink/tps65219.pdf Can it be Datasheet tag? > Signed-off-by: Jonathan Cormier <jcormier@criticallink.com> > Signed-off-by: Jerome Neanne <jneanne@baylibre.com> Not sure how to interpet this along with the From line. ... > +config GPIO_TPS65219 > + tristate "TPS65219 GPIO" > + depends on MFD_TPS65219 > + 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. Similar issues as with commit message. Also if chosen as M, how will it be called? > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > + > +#include <linux/gpio/driver.h> > +#include <linux/mfd/tps65219.h> > + > +#define TPS65219_GPIO0_DIR_MASK BIT(3) > +#define TPS65219_GPIO0_OFFSET 2 > +#define TPS65219_GPIO0_IDX 0 > +#define TPS65219_GPIO_DIR_IN 1 > +#define TPS65219_GPIO_DIR_OUT 0 > + > +struct tps65219_gpio { > + struct gpio_chip gpio_chip; > + struct tps65219 *tps; > +}; ... > +static void tps65219_gpio_set(struct gpio_chip *gc, unsigned int offset, > + int value) > +{ > + struct tps65219_gpio *gpio = gpiochip_get_data(gc); > + > + if (offset != TPS65219_GPIO0_IDX) > + regmap_update_bits(gpio->tps->regmap, > + TPS65219_REG_GENERAL_CONFIG, > + BIT(offset - 1), value ? BIT(offset - 1) : 0); > + else > + regmap_update_bits(gpio->tps->regmap, > + TPS65219_REG_GENERAL_CONFIG, > + BIT(TPS65219_GPIO0_OFFSET), > + value ? BIT(TPS65219_GPIO0_OFFSET) : 0); With tenporary variables for value and mask this can be simplified. bit = (offset == _IDX) ? GPIO0_OFFSET : offset - 1; mask = BIT(bit); v = value ? mask : 0; regmap_update_bits(..., mask, v); > +} ... > +static int tps65219_gpio_change_direction(struct gpio_chip *gc, unsigned int offset, > + unsigned int direction) > +{ > + struct tps65219_gpio *gpio = gpiochip_get_data(gc); > + > + /* 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: You may rather put code into #if 0 ... #endif, so it will be easier to get the test. Also, probably you would need to take care about error handling properly. > + * ret = regmap_update_bits(gpio->tps->regmap, TPS65219_REG_MFP_1_CONFIG, > + * TPS65219_GPIO0_DIR_MASK, direction); > + */ > + dev_err(gpio->tps->dev, > + "GPIO%d direction set by NVM, change to %u failed, not allowed by specification\n", > + offset, direction); > + return -EOPNOTSUPP; > +} ... > +static struct platform_driver tps65219_gpio_driver = { > + .driver = {.name = "tps65219-gpio",}, Can you write it as .driver = { ... }, ? > + .probe = tps65219_gpio_probe, > +};
Fri, Feb 24, 2023 at 02:16:06PM +0200, andy.shevchenko@gmail.com kirjoitti: > Fri, Feb 24, 2023 at 12:38:36PM +0100, Jerome Neanne kirjoitti: ... > > +#include <linux/of.h> Forgot to add that there is no user of this header.
On Fri, Feb 24, 2023 at 7:16 AM <andy.shevchenko@gmail.com> wrote: > > Fri, Feb 24, 2023 at 12:38:36PM +0100, 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. > > Linux > > > 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. > > Something wrong with the indentation. > > > Link: https://www.ti.com/lit/ds/symlink/tps65219.pdf > > Can it be Datasheet tag? > > > Signed-off-by: Jonathan Cormier <jcormier@criticallink.com> > > Signed-off-by: Jerome Neanne <jneanne@baylibre.com> > > Not sure how to interpet this along with the From line. Are two sign-offs not allowed/expected? I wrote the initial implementation of this driver and Jerome updated it and is handling submitting it since he did the rest of the TPS65219 drivers. > > ... > > > +config GPIO_TPS65219 > > + tristate "TPS65219 GPIO" > > + depends on MFD_TPS65219 > > + 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. > > Similar issues as with commit message. Also if chosen as M, how will it be > called? Based on INPUT_TPS65219_PWRBUTTON, this should have: To compile this driver as a module, choose M here. The module will be called tps65219-gpio. -- Jonathan Cormier Software Engineer Voice: 315.425.4045 x222 http://www.CriticalLink.com 6712 Brooklawn Parkway, Syracuse, NY 13211
On Mon, Feb 27, 2023 at 9:20 PM Jon Cormier <jcormier@criticallink.com> wrote: > On Fri, Feb 24, 2023 at 7:16 AM <andy.shevchenko@gmail.com> wrote: > > Fri, Feb 24, 2023 at 12:38:36PM +0100, Jerome Neanne kirjoitti: ... > > > Signed-off-by: Jonathan Cormier <jcormier@criticallink.com> > > > Signed-off-by: Jerome Neanne <jneanne@baylibre.com> > > > > Not sure how to interpet this along with the From line. > Are two sign-offs not allowed/expected? I wrote the initial > implementation of this driver and Jerome updated it and is handling > submitting it since he did the rest of the TPS65219 drivers. 1. Submitter's SoB must be the last SoB in the chain. 2. Developers also need to be marked with Co-developed-by.
On 27/02/2023 20:51, Andy Shevchenko wrote: >>>> Signed-off-by: Jonathan Cormier<jcormier@criticallink.com> >>>> Signed-off-by: Jerome Neanne<jneanne@baylibre.com> >>> Not sure how to interpet this along with the From line. >> Are two sign-offs not allowed/expected? I wrote the initial >> implementation of this driver and Jerome updated it and is handling >> submitting it since he did the rest of the TPS65219 drivers. > 1. Submitter's SoB must be the last SoB in the chain. > 2. Developers also need to be marked with Co-developed-by. Got it! My mistake. I'll fix following your instructions.