On Fri, Feb 16, 2018 at 2:30 AM, <s.gottschall@xxxxxxxxxx> wrote: > From: Sebastian Gottschall <s.gottschall@xxxxxxxxxxxxxxx> > > Adds LED and GPIO Control support for 988x, 9887, 9888, 99x0, 9984 and ipq4019 based chipsets with on chipset connected led's > using WMI Firmware API. > The LED device will get available named as "ath10k-phyX" at sysfs and can be controlled with various triggers. > adds also debugfs interface for gpio control. > Hi Sebastian, First off, let me say I love this and the effort to make the gpios hanging off the wifi chip as proper system gpios. Years ago I had to do a hack to make some wifi-chip provided pins available to a customer who needed more GPIOs on our SoM. It would have been nice if provided hardware functionality had already been properly supported by the driver. In looking through, it looks like you're hooking into the standard gpio-chip driver interface. Which is great. So, why are you providing a separate out-of-band debugfs access interface for gpio control? Seems to me to be a duplicate of the interface in sysfs that provides gpio access (`/sys/class/gpio`). My preference is we don't duplicate functionality and that we use the standard provided features. That way we don't possibly confuse users, and we also reduce our maintenance effort. Additionally, what happens when the relevant GPIO configuration options aren't enabled? Do we have a compilation issue? I'm more familiar with being a _consumer_ of gpiolib in my drivers and I know if CONFIG_GPIOLIB isn't checked I end up with problems. I'm unsure what happens when you're presenting a gpio-chip interface without the relevant CONFIG_ being enabled. Note that I didn't test this, I'm just asking the question to be sure we don't forget anything. - Steve
