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/ > +#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
