Message ID | 20200423162548.129661-1-dianders@chromium.org |
---|---|
Headers | show |
Series | drm: Prepare to use a GPIO on ti-sn65dsi86 for Hot Plug Detect | expand |
Quoting Douglas Anderson (2020-04-23 09:25:43) > 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@chromium.org> > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com> > --- Reviewed-by: Stephen Boyd <swboyd@chromium.org>
On Thu, Apr 23, 2020 at 6:26 PM Douglas Anderson <dianders@chromium.org> wrote: > People use panel-simple when they have panels that are builtin to > their device. In these cases the HPD (Hot Plug Detect) signal isn't > really used for hotplugging devices but instead is used for power > sequencing. Panel timing diagrams (especially for eDP panels) usually > have the HPD signal in them and it acts as an indicator that the panel > is ready for us to talk to it. > > Sometimes the HPD signal is hooked up to a normal GPIO on a system. > In this case we need to poll it in the correct place to know that the > panel is ready for us. In some system designs the right place for > this is panel-simple. > > When adding this support, we'll account for the case that there might > be a circular dependency between panel-simple and the provider of the > GPIO. The case this was designed for was for the "ti-sn65dsi86" > bridge chip. If HPD is hooked up to one of the GPIOs provided by the > bridge chip then in our probe function we'll always get back > -EPROBE_DEFER. Let's handle this by allowing this GPIO to show up > late if we saw -EPROBE_DEFER during probe. NOTE: since the > gpio_get_optional() is used, if the "hpd-gpios" isn't there our > variable will just be NULL and we won't do anything in prepare(). > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > Reviewed-by: Stephen Boyd <swboyd@chromium.org> > --- > > Changes in v3: > - Remind how gpio_get_optional() works in the commit message. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> I have a small inkling to protest against calling this driver "panel-simple" as we tend to stockpile things like this. I suppose panel-panacea.c is a better name at this point :/ Yours, Linus Walleij
On Thu, Apr 23, 2020 at 6:26 PM Douglas Anderson <dianders@chromium.org> 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@chromium.org> > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com> 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@walle.cc/ > +#include <linux/gpio/driver.h> > +#include <linux/gpio.h> Only <linux/gpio/driver.h> should be needed else you are doing something wrong. > + * @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. > +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()? > + /* > + * 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> > + 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)); > +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. > +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. Overall it looks good, just the minor things above need fixing or looking into. Yours, Linus Walleij
Hi, On Tue, Apr 28, 2020 at 5:44 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Thu, Apr 23, 2020 at 6:26 PM Douglas Anderson <dianders@chromium.org> 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@chromium.org> > > Cc: Linus Walleij <linus.walleij@linaro.org> > > Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > 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@walle.cc/ 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@mail.gmail.com ...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