Hi, On Tue, Apr 28, 2020 at 5:44 AM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote: > > On Thu, Apr 23, 2020 at 6:26 PM Douglas Anderson <dianders@xxxxxxxxxxxx> wrote: > > > The ti-sn65dsi86 MIPI DSI to eDP bridge chip has 4 pins on it that can > > be used as GPIOs in a system. Each pin can be configured as input, > > output, or a special function for the bridge chip. These are: > > - GPIO1: SUSPEND Input > > - GPIO2: DSIA VSYNC > > - GPIO3: DSIA HSYNC or VSYNC > > - GPIO4: PWM > > > > Let's expose these pins as GPIOs. A few notes: > > - Access to ti-sn65dsi86 is via i2c so we set "can_sleep". > > - These pins can't be configured for IRQ. > > - There are no programmable pulls or other fancy features. > > - Keeping the bridge chip powered might be expensive. The driver is > > setup such that if all used GPIOs are only inputs we'll power the > > bridge chip on just long enough to read the GPIO and then power it > > off again. Setting a GPIO as output will keep the bridge powered. > > - If someone releases a GPIO we'll implicitly switch it to an input so > > we no longer need to keep the bridge powered for it. > > > > Because of all of the above limitations we just need to implement a > > bare-bones GPIO driver. The device tree bindings already account for > > this device being a GPIO controller so we only need the driver changes > > for it. > > > > NOTE: Despite the fact that these pins are nominally muxable I don't > > believe it makes sense to expose them through the pinctrl interface as > > well as the GPIO interface. The special functions are things that the > > bridge chip driver itself would care about and it can just configure > > the pins as needed. > > > > Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx> > > Cc: Linus Walleij <linus.walleij@xxxxxxxxxx> > > Cc: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx> > > Pretty cool. > > I wonder if this chip could use the generic regmap GPIO helpers > that we are working on when they come around? > https://lore.kernel.org/linux-gpio/20200423174543.17161-11-michael@xxxxxxxx/ An important part of my patch is the handling of power management. Specifically: * If the GPIO is an input we don't need to keep the device powered, just power it temporarily to read the pin. * If the GPIO is an output we do need to keep the device powered. I suppose that could be common for other similar devices so as long as the generic interfaces can handle this concept we can try to use it. > > +#include <linux/gpio/driver.h> > > +#include <linux/gpio.h> > > Only <linux/gpio/driver.h> should be needed else you are doing > something wrong. It's because I needed GPIOF_DIR_OUT / GPIOF_DIR_IN which was apparently wrong. See below. > > + * @gchip: If we expose our GPIOs, this is used. > > + * @gchip_output: A cache of whether we've set GPIOs to output. This > > + * serves double-duty of keeping track of the direction and > > + * also keeping track of whether we've incremented the > > + * pm_runtime reference count for this pin, which we do > > + * whenever a pin is configured as an output. > > That sounds a bit hairy but I guess it's fine. > > > + */ > > struct ti_sn_bridge { > > struct device *dev; > > struct regmap *regmap; > > @@ -102,6 +136,9 @@ struct ti_sn_bridge { > > struct gpio_desc *enable_gpio; > > struct regulator_bulk_data supplies[SN_REGULATOR_SUPPLY_NUM]; > > int dp_lanes; > > + > > + struct gpio_chip gchip; > > + DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS); > > Do you really need a bitmap for 4 bits? Can't you just have something > like an u8 and check bit 0,1,2,3 ... well I suppose it has some elegance to > it as well but... hm. Doing so requires adding a lock to this driver to handle concurrent users of the different GPIOs. I can go back and do that but I'd rather not. Some prior discussion: https://lore.kernel.org/r/CAD=FV=WJONhm4ukwZa2vGtozrz_SmLuTCLxVimnGba7wRPPzgQ@xxxxxxxxxxxxxx ...if you want me to change this to a u8 + a mutex then please let me know, otherwise I'll assume keeping it a bitmap is fine. > > +static struct ti_sn_bridge *gchip_to_pdata(struct gpio_chip *chip) > > +{ > > + return container_of(chip, struct ti_sn_bridge, gchip); > > +} > > + > > +static int ti_sn_bridge_gpio_get_direction(struct gpio_chip *chip, > > + unsigned int offset) > > +{ > > + struct ti_sn_bridge *pdata = gchip_to_pdata(chip); > > Is there some specific reason why you don't just use > gpiochip_get_data()? I guess I'm used to interfaces that don't have a data pointer. I'll change it to gpiochip_get_data() at your suggestion, though (I think) it might be slightly less efficient (a function call and a pointer dereference compared to a subtract operation). > > + /* > > + * We already have to keep track of the direction because we use > > + * that to figure out whether we've powered the device. We can > > + * just return that rather than (maybe) powering up the device > > + * to ask its direction. > > + */ > > + return test_bit(offset, pdata->gchip_output) ? > > + GPIOF_DIR_OUT : GPIOF_DIR_IN; > > +} > > Don't use these legacy defines, they are for consumers. > Use GPIO_LINE_DIRECTION_IN and GPIO_LINE_DIRECTION_OUT. > from <linux/gpio/driver.h> That's what I get for reading the comments. I'll change this in the next version. I've also sent the following patch to help keep other people from falling into my trap: https://lore.kernel.org/r/20200428172322.1.I396f351e364f3c09df7c7606e79abefb8682c092@changeid/ > > + ret = regmap_read(pdata->regmap, SN_GPIO_IO_REG, &val); > > + pm_runtime_put(pdata->dev); > > + > > + if (ret) > > + return ret; > > + > > + return (val >> (SN_GPIO_INPUT_SHIFT + offset)) & 1; > > My preferred way to do this is: > > #include <linux/bits.h> > > return !!(val & BIT(SN_GPIO_INPUT_SHIFT + offset)); Somehow I think of "!!" as being a bool and this function as returning something that's logically an int. It really doesn't matter a whole lot and I'm happy to change it, so I'll change it in the next version. > > +static void ti_sn_bridge_gpio_set(struct gpio_chip *chip, unsigned int offset, > > + int val) > > +{ > > + struct ti_sn_bridge *pdata = gchip_to_pdata(chip); > > + int ret; > > + > > + if (!test_bit(offset, pdata->gchip_output)) { > > + dev_err(pdata->dev, "Ignoring GPIO set while input\n"); > > + return; > > + } > > + > > + val &= 1; > > + ret = regmap_update_bits(pdata->regmap, SN_GPIO_IO_REG, > > + BIT(SN_GPIO_OUTPUT_SHIFT + offset), > > + val << (SN_GPIO_OUTPUT_SHIFT + offset)); > > Looks like a job for the generic helper library. I think that (for now) this comment is a no-op since the generic helper library isn't landed yet, right? ...and it wouldn't handle the power management I need? If I'm confused and I need to act on this comment, please let me know. > > +static int ti_sn_bridge_gpio_direction_input(struct gpio_chip *chip, > > + unsigned int offset) > > +{ > > + struct ti_sn_bridge *pdata = gchip_to_pdata(chip); > > + int shift = offset * 2; > > + int ret; > > + > > + if (!test_and_clear_bit(offset, pdata->gchip_output)) > > + return 0; > > + > > + ret = regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG, > > + 0x3 << shift, SN_GPIO_MUX_INPUT << shift); > > But this 0x03 does not look very generic, it's not just 1 bit but 2. Sure, I can add #define SN_GPIO_MUX_MASK 0x3. Basically the mux is: * 0: input * 1: output * 2: special function As talked about in the patch comments, I don't define this as an official pinmux driver because that seems overkill. I'll assume it's OK to just do the #define and use it. If you want something more, let me know. > Overall it looks good, just the minor things above need fixing or > looking into. Thank you very much for the review! I'll plan to send a new patch out in the next day or two with minor comments addressed and making the assumptions I've documented above. If I got something wrong then please yell. ...or yell after I send the next version and I'll send yet another version after that! :-) -Doug
