Re: [PATCH] gpio: omap: prevent module from being unloaded while in use
On Fri, Jun 26, 2015 at 12:13 AM, Grygorii Strashko
wrote:
> OMAP GPIO driver allowed to be built as loadable module, but it
> doesn't set owner field in GPIO chip structure. As result,
> module_get/put() API is not working and it's possible to unload
> OMAP driver while in use:
>
> omap_gpio 48051000.gpio: REMOVING GPIOCHIP WITH GPIOS STILL REQUESTED
>
> Hence, add missing configuration.
Isn't this also fixed by your other patch "gpiolib: assign chip owner
to dev->driver->owner if not set"?
Nevertheless,
Acked-by: Alexandre Courbot
For inclusion into -rc if the other patch is for the next cycle.
>
> Cc: Tony Lindgren
> Fixes: cac089f9026e ('gpio: omap: Allow building as a loadable module')
> Signed-off-by: Grygorii Strashko
> ---
> Hi Linus,
>
> Seems this one is for 4.2-rc.
>
> drivers/gpio/gpio-omap.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index a0ad803..61a731f 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -1187,6 +1187,7 @@ static int omap_gpio_probe(struct platform_device *pdev)
> bank->irq = res->start;
> bank->dev = dev;
> bank->chip.dev = dev;
> + bank->chip.owner = THIS_MODULE;
> bank->dbck_flag = pdata->dbck_flag;
> bank->stride = pdata->bank_stride;
> bank->width = pdata->bank_width;
> --
> 2.4.4
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: "advanced" LED controllers
On Mon, Mar 2, 2015 at 6:21 PM, Pavel Machek wrote: > On Wed 2015-02-25 18:06:07, Alexandre Courbot wrote: >> On Wed, Feb 25, 2015 at 5:25 PM, Geert Uytterhoeven >> wrote: >> > CC linux-gpio, as this looks like the LED equivalent of bulk gpio? >> >> Indeed. The LED core could implement something similar to >> gpiod_set_array() to allow several LEDs to be set in one call. If the >> controller supports it, it would then set all the LEDs at once, >> otherwise the core would apply the values serially. >> >> In leds-gpio.c, this multiple LED setting could be implemented by a >> single call to gpiod_set_array() and the right thing would happen. > > Actually, there are two issues: some controlles can set all LEDs at > once, and some can program pwm level smoothly using given program (and > handle multiple leds in the process). In any case, that would be handled at the controller level and should work with a function similar to gpiod_set_array(), wouldn't it? -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: "advanced" LED controllers
On Wed, Feb 25, 2015 at 5:25 PM, Geert Uytterhoeven wrote: > CC linux-gpio, as this looks like the LED equivalent of bulk gpio? Indeed. The LED core could implement something similar to gpiod_set_array() to allow several LEDs to be set in one call. If the controller supports it, it would then set all the LEDs at once, otherwise the core would apply the values serially. In leds-gpio.c, this multiple LED setting could be implemented by a single call to gpiod_set_array() and the right thing would happen. > > On Thu, Feb 19, 2015 at 10:14 PM, Felipe Balbi wrote: >> Do we have support for LED controllers which can handle patterns of >> different kinds ? I mean, currently, if we have an LED controller such >> as TPIC2810 [1] which can control 8 different leds and each LED >> corresponds to one bit on register 0x44, we could control leds by just >> "playing" a wave file on the controller and create easy patterns with >> that. >> >> AFAICT, in linux today we would have to register each of the 8 LEDs as a >> different LED and have driver magic to write the proper bits on register >> 0x44, that seems a bit overkill, specially when we want to make >> patterns: instead of writing 0xff we would have to write 0x80, 0x40, >> 0x20, 0x10, 0x08, 0x04, 0x02, 0x01 separately and have the driver cache >> the previous results so we don't end up switching off other LEDs. >> >> IOW, what could be handled with a single write, currently needs 8. >> >> I wonder if there's any work happening to support these slightly more >> inteligent LED engines. >> >> regards >> >> [1] http://www.ti.com/product/tpic2810 >> >> ps: tpic2810 is probably the simplest example, lp551, lp5523 and others >> have even more advanced pattern engines which can even handle RGB leds. >> >> Currently the driver loads patterns as if it was a firmware blob and >> registers each of R, G and B components as separate LEDs. Each component >> also has its own brightness controls (something tpic2810 doesn't have, >> it's either on or off). > -- > To unsubscribe from this list: send the line "unsubscribe linux-gpio" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gpio / PM: Replace CONFIG_PM_RUNTIME with CONFIG_PM
On Wed, Dec 3, 2014 at 10:50 AM, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki > > After commit b2b49ccbdd54 (PM: Kconfig: Set PM_RUNTIME if PM_SLEEP is > selected) PM_RUNTIME is always set if PM is set, so #ifdef blocks > depending on CONFIG_PM_RUNTIME may now be changed to depend on > CONFIG_PM. > > Replace CONFIG_PM_RUNTIME with CONFIG_PM in drivers/gpio/gpio-omap.c. > > Signed-off-by: Rafael J. Wysocki Acked-by: Alexandre Courbot -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH] gpio: omap: Fix interrupt names
On Sat, Sep 6, 2014 at 4:52 AM, Nishanth Menon wrote: > When viewing the /proc/interrupts, there is no information about which > GPIO bank a specific gpio interrupt is hooked on to. This is more than a > bit irritating as such information can esily be provided back to the > user and at times, can be crucial for debug. > > So, instead of displaying something like: > 31: 0 0 GPIO 0 palmas > 32: 0 0 GPIO 27 mmc0 > > Display the following with appropriate device name: > 31: 0 0 4ae1.gpio 0 palmas > 32: 0 0 4805d000.gpio 27 mmc0 > > This requires that we create irq_chip instance specific for each GPIO > bank which is trivial to achieve. Acked-by: Alexandre Courbot -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] regulator: palmas: fix typo in enable_reg calculation
On Tue, Jun 24, 2014 at 5:53 AM, Stephen Warren wrote: > From: Stephen Warren > > When setting up .enable_reg for an SMPS regulator, presumably we should > call PALMAS_BASE_TO_REG(PALMAS_SMPS_BASE, ...) rather than using > LDO_BASE. This change makes the LCD panel and HDMI work again on the > NVIDIA Dalmore board anyway. Tested-by: Alexandre Courbot Interestingly the first fix was enough for Tegra Note 7's panel - probably due to the fact the panel was already on when the kernel took over. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] regulator: palmas: Fix SMPS enable/disable/is_enabled
On Sat, Jun 21, 2014 at 2:26 AM, Nishanth Menon wrote: > We use regmap regulator ops to enable/disable and check if regulator > is enabled for various SMPS. However, these depend on valid > enable_reg, enable_mask and enable_value in regulator descriptor. > > Currently we do not populate these for SMPS other than SMPS10, this > results in spurious results as regmap assumes that the values are > valid and ends up reading register 0x0 RTC:SECONDS_REG on Palmas > variants that do have RTC! To fix this, we update proper parameters > for the descriptor fields. > > Further, we want to ensure the behavior consistent with logic > prior to commit dbabd624d4eec50b6, where, once you do a set_mode, > enable/disable ensure the logic remains consistent and configures > Palmas to the configuration that we set with set_mode (since the > configuration register is common). To do this, we can rely on the > regulator core's regulator_register behavior where the regulator > descriptor pointer provided by the regulator driver is stored. (no > reallocation and copy is done). This lets us update the enable_value > post registration, to remain consistent with the mode we configure as > part of set_mode. Tested-by: Alexandre Courbot Thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Palmas regulator broken (was Re: [PATCH] ARM: tegra: TN7: relax some regulators)
On Fri, Jun 20, 2014 at 10:54 PM, Nishanth Menon wrote: > On 08:23-20140620, Nishanth Menon wrote: >> + l-o, >> http://marc.info/?t=14031642754&r=1&w=2 full thread >> >> Minor change in subject to indicate palmas regulator fail >> >> On 18:49-20140620, Alexandre Courbot wrote: >> > On 06/20/2014 06:41 PM, Mark Brown wrote: >> > >* PGP Signed by an unknown key >> > > >> > >On Fri, Jun 20, 2014 at 03:44:46PM +0900, Alexandre Courbot wrote: >> > > >> > >>dbabd624d >> > >>regulator: palmas: Reemove open coded functions with helper functions >> > > >> > >>Keerthy, Nishanth, could it be that there is still something wrong with >> > >>the >> > >>REGULATOR_LINEAR_RANGE() definitions? >> > > >> > >>This seems to be the cause for our trouble, but the other questions might >> > >>still stand, in case there is interest in discussing them. >> > > >> > >There was a bug fix to the Palmas driver which just went to Linus the >> > >other day, are you sure this isn't fixed in mainline (or -next, it's >> > >been in -next for a week or something)? >> > >> > If you are talking about >> > >> > 6b7f2d82d5 >> > regulator: palmas: Fix SMPS list for 0V >> > >> > then it is in my tree. There is actually no difference on >> > palmas-regulator.c between my tree and the current -next (or Linus' >> > tree for that instance). >> > >> > So it seems to be something else we are dealing with here. >> >> Your quote earlier in the thread >> " >> _regulator_is_enabled() *also* returns false >> " >> >> Got me curious. Looking at the patch: >> dbabd624d4eec50b623bab070d1e39a854b2d65c (regulator: palmas: Reemove >> open coded functions with helper functions) >> I noticed the following change >> palmas_is_enabled_smps -> regulator_is_enabled_regmap >> >> So I decided to search for enable_reg in palmas-regulator.c and I think >> it needs valid enable_reg, mask, value for regulator_is_enabled_regmap to >> work >> :). >> >> Maybe to be sure, we could print the following: >> PALMAS_SMPS8_VOLTAGE, PALMAS_SMPS8_CTRL, PALMAS_SMPS8_TSTEP, >> >> Anyways, I quickly boot tested the following on DRA7evm (which also uses >> Palmas): >> [1.933939] palmas-pmic 4807.i2c:tps659038@58:tps659038_pmic: >> enable_reg = 0x00, mask =0x00 >> [1.944210] smps123: 850 <--> 1250 mV at 1060 mV >> [1.950717] palmas-pmic 4807.i2c:tps659038@58:tps659038_pmic: >> enable_reg = 0x00, mask =0x00 >> [1.960754] smps45: 850 <--> 1150 mV at 1060 mV >> [1.967048] palmas-pmic 4807.i2c:tps659038@58:tps659038_pmic: >> enable_reg = 0x00, mask =0x00 >> [1.977072] smps6: 850 <--> 1650 mV at 1060 mV >> [1.983077] palmas-pmic 4807.i2c:tps659038@58:tps659038_pmic: >> enable_reg = 0x00, mask =0x00 >> [1.992994] smps7: 850 <--> 1030 mV at 1030 mV >> [1.999238] palmas-pmic 4807.i2c:tps659038@58:tps659038_pmic: >> enable_reg = 0x00, mask =0x00 >> [2.009161] smps8: 850 <--> 1250 mV at 1060 mV >> [2.015304] palmas-pmic 4807.i2c:tps659038@58:tps659038_pmic: >> enable_reg = 0x00, mask =0x00 >> >> It does seem to me that either set_mode also should use core functions >> OR you still need a palmas specific is_enable, enable/disable functions >> (contrary to the claim of the patch in question - which I think >> introduced regressions). >> >> Otherwise, completely untested diff below - can you give this a shot? >> >> diff --git a/drivers/regulator/palmas-regulator.c >> b/drivers/regulator/palmas-regulator.c >> index b982f0f..bbfe22f 100644 >> --- a/drivers/regulator/palmas-regulator.c >> +++ b/drivers/regulator/palmas-regulator.c >> @@ -964,6 +964,20 @@ static int palmas_regulators_probe(struct >> platform_device *pdev) >> return ret; >> pmic->current_reg_mode[id] = reg & >> PALMAS_SMPS12_CTRL_MODE_ACTIVE_MASK; >> + >> + dev_err(&pdev->dev, "enable_reg = 0x%02x, mask >> =0x%02x\n", >> + pmic->desc[id].enable_reg, >> + pmic->desc[id].enable_mask); >> + pmic->desc[id].enable_reg = >> +
Re: [RFC PATCH 0/5] add gpio_chip_ops to hold GPIO operations
On Wed, Apr 9, 2014 at 3:20 AM, Javier Martinez Canillas wrote: > In the kernel there are basically two patterns to implement object > oriented code in C. You can either embedded a set of function pointers s/embedded/embed > in a struct along with other members or have a separate virtual function > table (vtable) structure that hold all the functions and only store a > pointer to that vtable on our particular object. > > The struct gpio_chip uses the former approach, but I don't know if that > is a design decision or is just that this code predates the fact that > the separate structure pattern is now so popular. Since the having a > the operations on a different structure has a number of benefits: "Since having the operations" maybe? > > - A clean separation between state (fields) and operations (functions). > - Size reduction of struct gpio_chip since will only hold one pointer. > - These functions are not supposed to change at runtime so the const > qualifier can be used to prevent pointers modification during execution. > - Similar drivers for a chip family can reuse their function vtable. > > There is a drawback though which is that now two memory accesses are > needed to execute a GPIO operation since an additional level of > indirection is introduced but that should be minimized due temporal and > spatial memory locality. I think I really do like this. Having ops in a separate structure is a very common pattern in the kernel and makes things a lot cleaner. On top of the advantages you listed, it also only requires a single assignment in the driver's init function vs. a lot more today. If no one complains about the additional memory access, I'd like to go forward with this. I did much worse performance-hurting changes when introducing gpiod, so I suppose it will be fine. > > So this is an RFC patch-set to add a virtual table to be used by > GPIO chip controllers and consist of the following patches: > > Javier Martinez Canillas (5): > gpio: add a vtable to abstract GPIO controller operations > gpiolib: set gpio_chip operations on add using a gpio_chip_ops > gpio: omap: convert driver to use gpio_chip_ops > gpio: twl4030: convert driver to use gpio_chip_ops > gpio: switch to use struct struct gpio_chip_ops > > drivers/gpio/gpio-omap.c| 19 - > drivers/gpio/gpio-twl4030.c | 10 +-- > drivers/gpio/gpiolib.c | 64 - > include/linux/gpio/driver.h | 69 > +++-- > 4 files changed, 93 insertions(+), 69 deletions(-) > > The patch-set is not a complete one though since only the GPIO OMAP > and GPIO TWL4030 drivers have been converted so I could test it on > my platform (DM3730 OMAP IGEPv2 board). > > But I preferred to send an early RFC than changing every single driver > before discussing if doing the split is worth it or not. > > To not break git bisect-ability, I added some patches that are > transitional changes. If you have a better suggestion on how to > handle that please let me know. We will probably need that transition phase. We will also need to switch every single driver to your new scheme, so please wait until we hear from Linus before proceeding. :) Thanks, Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
