Re: [PATCH v4 09/24] regulator: pwm: use pwm_get/set_default_xxx() helpers where appropriate
On Mon, Nov 16, 2015 at 01:23:59PM +0100, Boris Brezillon wrote: > Mark Brown wrote: > > On Mon, Nov 16, 2015 at 09:56:32AM +0100, Boris Brezillon wrote: > > > - pwm_reg_period = pwm_get_period(drvdata->pwm); > > > + pwm_reg_period = pwm_get_default_period(drvdata->pwm); > > It's not clear to me that we're not looking for the current period here > > or in the other use. Won't configuring based on a period other than the > > one that has been set give the wrong answer? > Hm, maybe that's naming problem. What I call the 'default' period here > is actually the period configured in your board file (using a PWM lookup > table) or your DT. This value represent the period requested by the PWM > user not a default value specified by the PWM chip driver. > The reason we're not using the 'current' period value is because it may > have been set by the bootloader, and may be inappropriate for our use > case (ie. the period may be to small to represent the different > voltages). > ITOH, we're using the current period value when calculating the current > voltage, because we want to get the correct voltage value, and the PWM > device may still use the configuration set by the bootloader (not the > default one specified in your board or DT files). > I hope this clarifies the differences between the current and default > period, and why we should use the default value here. To be honest I'm still a bit confused here. When do we actually apply the default setting and why do we keep on having to constantly override it rather than doing this once at boot? It feels wrong to be using it every time we set anything. I'd expect it to be something we only need to do at probe time or which would automatically be handled by the PWM framework (but that'd have issues changing the state and potentially breaking things if done in an uncoordiated fashion). signature.asc Description: PGP signature
Re: [PATCH v4 09/24] regulator: pwm: use pwm_get/set_default_xxx() helpers where appropriate
On Mon, Nov 16, 2015 at 09:56:32AM +0100, Boris Brezillon wrote: > +++ b/drivers/regulator/pwm-regulator.c > @@ -56,7 +56,7 @@ static int pwm_regulator_set_voltage_sel(struct > regulator_dev *rdev, > int dutycycle; > int ret; > > - pwm_reg_period = pwm_get_period(drvdata->pwm); > + pwm_reg_period = pwm_get_default_period(drvdata->pwm); > > dutycycle = (pwm_reg_period * > drvdata->duty_cycle_table[selector].dutycycle) / 100; It's not clear to me that we're not looking for the current period here or in the other use. Won't configuring based on a period other than the one that has been set give the wrong answer? signature.asc Description: PGP signature
Re: [PATCH v3] Input: tsc2005 - Add support for tsc2004
On Thu, Oct 29, 2015 at 03:23:31PM -0700, Dmitry Torokhov wrote: > However, you have regmap in the driver core already. Mark, is it > possible to have regmap API also allow doing raw underlying protocol > transfer so that consumers could issue command requests without needing > to know if they need to do it over i2c or spi or whatever. Or we need a > notion of command registers in regmap... I don't think it's a good idea to break the encapsulation of the regmap and export the raw I/O functionality directly, there seem to be more bad ways of using that than good. The driver must at some point know what bus it is dealing with and be able to manage this itself. I don't know what "command registers" are. signature.asc Description: PGP signature
[PATCH] MAINTAINERS: Remove wm97xx entry
Neither myself or Liam is especially interested in this driver any more and the devices are already covered by the general ex-Wolfson entry so just remove this. Signed-off-by: Mark Brown Acked-by: Liam Girdwood --- This depends on an update to all the Wolfson entries in the ASoC tree so I'll just apply it there. MAINTAINERS | 9 - 1 file changed, 9 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 508f442..17f6cb5 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11393,15 +11393,6 @@ W: http://oops.ghostprotocols.net:81/blog S: Maintained F: drivers/net/wireless/wl3501* -WM97XX TOUCHSCREEN DRIVERS -M: Mark Brown -M: Liam Girdwood -L: linux-input@vger.kernel.org -W: https://github.com/CirrusLogic/linux-drivers/wiki -S: Supported -F: drivers/input/touchscreen/*wm97* -F: include/linux/wm97xx.h - WOLFSON MICROELECTRONICS DRIVERS L: patc...@opensource.wolfsonmicro.com T: git https://github.com/CirrusLogic/linux-drivers.git -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] ALSA: ac97: Switch to dev_pm_ops
On Fri, Aug 21, 2015 at 06:27:08PM +0200, Takashi Iwai wrote: > Mark, did you already put into your tree? Otherwise I'm going to take > it to for-next branch. No, and I discarded the previous version so should be good for your tree. signature.asc Description: Digital signature
Re: [PATCH v2] ALSA: ac97: Switch to dev_pm_ops
On Fri, Aug 21, 2015 at 10:25:59AM +0200, Takashi Iwai wrote: > Lars-Peter Clausen wrote: > > This patch touches code in both ALSA and the input subsystem. Ideally this > > patch will be merged via the ALSA tree. Given that the wm97xx touchscreen > > driver is not seeing too many changes these days the risk of conflicts > > should be low. > If Dmitry is happy with this, I'll take this. He indicated at Plumbers yesterday that he's OK with fixing any remaining stylistic issues after the merge window. signature.asc Description: Digital signature
Re: [PATCH] ALSA: ac97: Switch to dev_pm_ops
On Thu, Aug 20, 2015 at 06:35:24PM +0200, Lars-Peter Clausen wrote: > On 08/20/2015 06:33 PM, Mark Brown wrote: > > Just discussed this in person with Dmitry: I'll apply the patch just now > > for v4.3 and we can incrementally improve the ifdef handling after. > Great, thanks. I was about to send a patch with the ifdefs removed tomorrow. Oh, that'd be even better if we could get that into v4.3 instead - this was partly given that I'd just met Dmitry and the merge window will be opening very soon. If you could get that patch done that'd be even better. signature.asc Description: Digital signature
Re: [PATCH] ALSA: ac97: Switch to dev_pm_ops
On Fri, Aug 07, 2015 at 09:32:13AM -0700, Dmitry Torokhov wrote: > On Fri, Aug 07, 2015 at 10:13:48AM +0200, Lars-Peter Clausen wrote: > > We know that it is used when CONFIG_PM_SLEEP is defined and we know that it > > is unused CONFIG_PM_SLEEP is not defined. Marking the function as > > __maybe_unused will cause the compiler to not generate a warning when the > > function is really unused. Making this explicit works much better. > It will also drop the code form the final image and having the functions > in provides better compile coverage. Just discussed this in person with Dmitry: I'll apply the patch just now for v4.3 and we can incrementally improve the ifdef handling after. signature.asc Description: Digital signature
Re: [PATCH] ALSA: ac97: Switch to dev_pm_ops
On Fri, Aug 07, 2015 at 09:30:29AM -0700, Dmitry Torokhov wrote: > On Fri, Aug 07, 2015 at 02:30:05PM +0100, Mark Brown wrote: > > Indeed, a major goal of disabling PM support is to save space. > Then maybe we should adjust dev_pm_ops definition to omit members that > are not used if given state is not supported? We have a lot of drivers > that are not doing silly pointer #define games. Yeah, I've always been vaugely surprised that we don't do that. signature.asc Description: Digital signature
Re: [PATCH] ALSA: ac97: Switch to dev_pm_ops
On Fri, Aug 07, 2015 at 10:13:48AM +0200, Lars-Peter Clausen wrote: > On 08/07/2015 12:55 AM, Dmitry Torokhov wrote: > > Pull this out of #ifdef block and kill entire #else/endif along with > > WM97XX_PM_OPS define: SIMPLE_DEV_PM_OPS will result in an empty > > structure if CONFIG_PM_SLEEP is not set. > It will create a struct dev_pm_ops full of NULLs. That's kind of > counterproductive to removing PM related data and functions from the kernel > if PM support is no enabled. Indeed, a major goal of disabling PM support is to save space. signature.asc Description: Digital signature
Re: [PATCH V3 2/4] regulator: da9062: DA9062 regulator driver
On Tue, May 19, 2015 at 02:10:30PM +0100, S Twiss wrote: > From: S Twiss > > Add BUCK and LDO regulator driver support for DA9062 Applied, thanks. signature.asc Description: Digital signature
Re: [PATCH 04/10] regulator: max77693: Support different register configurations
On Wed, Apr 29, 2015 at 07:58:29PM +0900, Krzysztof Kozlowski wrote: > Add support for different configurations of charger's registers so the > same driver could be used on other devices (e.g. MAX77843). Acked-by: Mark Brown signature.asc Description: Digital signature
Re: [PATCH v2 09/17] spi: add locomo SPI driver
On Tue, Apr 28, 2015 at 02:55:46AM +0300, Dmitry Eremin-Solenikov wrote:
> +static int locomospi_reg_open(struct locomospi_dev *spidev)
> +{
> +static int locomospi_reg_release(struct locomospi_dev *spidev)
> +{
These are a bit weird - as far as I can tell they're just doing some
init done on probe and release? I think I'd expect to see them either
inlined there or done as part of the PM operations too (including
runtime ones).
> + for (j = 0; j < wait; j++) {
> + regmap_read(spidev->regmap, LOCOMO_SPIST, &r);
> + if (r & LOCOMO_SPI_RFW)
> + break;
> + }
> + if (j == wait)
> + dev_err(&spi->dev, "rfw timeout\n");
But we don't return an error if we time out?
> +static int locomo_spi_setup_transfer(struct spi_device *spi,
> + struct spi_transfer *t)
> +{
> + struct locomospi_dev *spidev;
> + u32 hz = 0;
> +
> + if (t)
> + hz = t->speed_hz;
> + if (!hz)
> + hz = spi->max_speed_hz;
The core will set a speed on every transfer for you.
> + if (!tx && !rx && t->len)
> + return -EINVAL;
Put this in the core if it's needed.
> +static int locomo_spi_remove(struct platform_device *pdev)
> +{
> + struct spi_master *master = platform_get_drvdata(pdev);
> + struct locomospi_dev *spidev = spi_master_get_devdata(master);
> +
> + return locomospi_reg_release(spidev);
We're doing this release before we free the master which is potentially
racy - but do we even need to do it at all?
signature.asc
Description: Digital signature
Re: [PATCH: RESEND] Update email-id of author
On Mon, Apr 27, 2015 at 12:56:02PM +0530, Rajeev Kumar wrote: > .mailmap |1 + > arch/arm/mach-spear/generic.h|2 +- > arch/arm/mach-spear/include/mach/irqs.h |2 +- > arch/arm/mach-spear/include/mach/spear.h |2 +- > arch/arm/mach-spear/spear6xx.c |2 +- > drivers/input/keyboard/spear-keyboard.c |2 +- > drivers/rtc/rtc-spear.c |4 ++-- > include/linux/platform_data/keyboard-spear.h |2 +- > include/sound/designware_i2s.h |2 +- > include/sound/spear_dma.h|2 +- > 10 files changed, 11 insertions(+), 10 deletions(-) Please split this up into per-subsystem patches so it's easier to apply. signature.asc Description: Digital signature
Re: [PATCH V1 2/6] regulator: da9062: DA9062 regulator driver
On Fri, Apr 24, 2015 at 02:47:06PM +, Opensource [Steve Twiss] wrote: > On 18 April 2015 12:48 Mark Brown wrote: > Okay. I think I am getting this. > As of v3.18 there are newer parts to regulator_desc from the commit > a0c7b16 "regulator: of: Provide simplified DT parsing method" > The search function is now in the core. > Am I on on the right track with this one? Yes. signature.asc Description: Digital signature
Re: [PATCH V1 2/6] regulator: da9062: DA9062 regulator driver
On Fri, Apr 17, 2015 at 03:23:32PM +0100, S Twiss wrote:
> +/* Regulator interrupt handlers */
> +static irqreturn_t da9062_ldo_lim_event(int irq, void *data)
> +{
> + struct da9062_regulators *regulators = data;
> + struct da9062 *hw = regulators->regulator[0].hw;
> + struct da9062_regulator *regl;
> + int bits, i, ret;
> +
> + ret = regmap_read(hw->regmap, DA9062AA_STATUS_D, &bits);
> + if (ret < 0)
> + return IRQ_NONE;
Please log an error for this, if we're having trouble talking to the
device that seems like a serious issue.
> + for (i = regulators->n_regulators - 1; i >= 0; i--) {
> + regl = ®ulators->regulator[i];
> + if (regl->info->oc_event.reg != DA9062AA_STATUS_D)
> + continue;
> +
> + if (BIT(regl->info->oc_event.lsb) & bits)
> + regulator_notifier_call_chain(regl->rdev,
> + REGULATOR_EVENT_OVER_CURRENT, NULL);
> + }
> +
> + return IRQ_HANDLED;
This will return IRQ_HANDLED even if none of the regulators were
flagginng an event.
> +static irqreturn_t da9062_vdd_warn_event(int irq, void *data)
> +{
> + return IRQ_HANDLED;
> +}
Ignoring an interrupt is not usefully handling it - at the *least* this
should be generating a log message.
> +static struct da9062_regulators_pdata *da9062_parse_regulators_dt(
> + struct platform_device *pdev,
> + struct of_regulator_match **reg_matches)
> +{
> + struct da9062_regulators_pdata *pdata;
> + struct da9062_regulator_data *rdata;
> + struct device_node *node;
> + int i, n, num;
> +
> + node = of_get_child_by_name(pdev->dev.parent->of_node, "regulators");
> + if (!node) {
> + dev_err(&pdev->dev, "Regulators device node not found\n");
> + return ERR_PTR(-ENODEV);
> + }
> +
> + num = of_regulator_match(&pdev->dev, node, da9062_matches,
> + ARRAY_SIZE(da9062_matches));
Don't open code this, describe the DT names in the regualtor_desc and
let the core register.
> + if (IS_ERR(pdata) || pdata->n_regulators == 0) {
> + dev_err(&pdev->dev,
> + "No regulators defined for the platform\n");
> + return PTR_ERR(pdata);
> + }
> +
> + n_regulators = ARRAY_SIZE(local_regulator_info),
This is broken, the set of regulators in the silicon is not a property
of the platform. The driver should just register all the regualtors
that are present in the silicon. I'm fairly sure I've been through this
before...
> + ret = request_threaded_irq(irq,
> +NULL, da9062_vdd_warn_event,
> +IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> +"VDD_WARN", regulators);
devm_request_threaded_irq().
signature.asc
Description: Digital signature
Re: Using gpio_keys with regmapped gpio?
On Tue, Mar 31, 2015 at 02:57:35PM -0500, Thor Thayer wrote: > Which brings up my next question. Can the gpio_keys framework be used with a > regmapped gpio? I haven't been able to find any examples of gpio_keys with > an external gpio expander and maybe this isn't valid usage. Why would there be any problem doing that? I suspect a lack of examples is more to do with it being rare to build hardware like that (usually on SoC GPIOs are fairly abundant and much more performant) than anything else. signature.asc Description: Digital signature
Re: [PATCH 01/15] mfd: add new driver for Sharp LoCoMo
On Fri, Nov 14, 2014 at 04:47:00PM +0400, Dmitry Eremin-Solenikov wrote: > I'm actually looking at the regulator interface. Since this DAC serves mostly > like a (semi-)constant voltage interface, would it be rather logical to use > regulator subsystem to drive it? Possibly... it's mostly a function of what the consumers of the functionality look like - if that code looks weird calling into the regulator API then it's bad, if that code looks OK it's fine. I haven't looked at the hardware at all so don't have strong feelings either way so long as the result looks tasteful. signature.asc Description: Digital signature
Re: [PATCH 02/15] GPIO: port LoCoMo gpio support from old driver
On Tue, Nov 11, 2014 at 05:16:38PM +0400, Dmitry Eremin-Solenikov wrote: > Just to better understand your suggestions: do you want me to convert > to regmap only gpio driver or do you suggest to convert all LoCoMo drivers? > That is doable, of course, but the amount of changes is overwhelming. > Also I'm concerned about the performance impact from going through > regmap layers. I don't care. signature.asc Description: Digital signature
Re: [PATCH 02/15] GPIO: port LoCoMo gpio support from old driver
On Thu, Nov 06, 2014 at 01:33:24AM +0400, Dmitry Eremin-Solenikov wrote: > 2014-11-03 16:43 GMT+03:00 Linus Walleij : > > Yes that's the point: if you use regmap mmio you get the RMW-locking > > for free, with the regmap implementation. > Just to be more concrete. Currently locomo_gpio_ack_irq() function uses > one lock and one unlock for doing 3 consecutive RMW I I convert locomo > to regmap, will that be 3 lock/unlock calls or still one? (Or maybe I'm > trying to be over-protective here and adding more lock/unlock cycles > won't matter that much?) You'll get three lock/unlocks, we could add an interface for bulk updates under one lock if it's important though. > Next question: if I have to export regmap to several subdrivers, is it better > to have one big regmap or to have one-map-per-driver approach? One regmap for the physical register map which is shared between all the users. signature.asc Description: Digital signature
Re: [PATCH 01/15] mfd: add new driver for Sharp LoCoMo
On Thu, Nov 06, 2014 at 12:02:49AM +0400, Dmitry Eremin-Solenikov wrote: > 2014-11-03 16:41 GMT+03:00 Linus Walleij : > > The point is still the same: no unrelated code in drivers/mfd, > > then either use IIO DAC as a middle layer or sink the DAC handling > > into respective subdriver, i.e. push it into the backlight or > > volume directly then. > The problem is that the DAC is equally used by backlight and by sound > device (WIP). > What about true i2c device driver sitting in drivers/misc and exporting a > regmap > of 2 8-bit registers? If it can just export registers that sounds like a MFD. If it needs to export functionality then like Linus says the IIO subsystem abstracts DACs. signature.asc Description: Digital signature
Re: [PATCH 11/15] sound: soc: poodle: make use of new locomo GPIO interface
On Tue, Oct 28, 2014 at 03:02:04AM +0300, Dmitry Eremin-Solenikov wrote:
> Since LoCoMo driver has been converted to provide proper gpiolib
> interface, make poodle ASoC platform driver use gpiolib API.
Please use subject lines matching the style for the subsystem.
> + ret = gpio_request_array(poodle_gpios, ARRAY_SIZE(poodle_gpios));
> + if (ret) {
> + dev_err(&pdev->dev, "gpio_request_array() failed: %d\n",
> + ret);
> + return ret;
> + }
I sense a need for devm_gpio_request_array() here. Otherwise this looks
fine - ideally it'd move to gpiod but moving to gpiolib is a clear win
so no need to block on this.
Acked-by: Mark Brown
with at least the subject line fixed.
signature.asc
Description: Digital signature
Re: [PATCH 15/15] spi: add locomo SPI driver
On Tue, Oct 28, 2014 at 03:02:08AM +0300, Dmitry Eremin-Solenikov wrote:
> LoCoMo chip has a built-in simple SPI controller. On Sharp SL-5500 PDDAs
> it is connected to external MMC slot.
> +config SPI_LOCOMO
> + tristate "Locomo SPI master"
> + depends on MFD_LOCOMO
> + select SPI_BITBANG
Rather than using SPI_BITBANG it'd be good for new drivers to convert to
using the core transfer_one() functionality which replaces most of what
the bitbang code is doing. The bitbang functionality was misnamed for
most of the users and we're going to try to move most of the functionality
not actually related to bitbanging out of it.
> + /* if (locomospi_carddetect()) { */
> + r = readw(spidev->base + LOCOMO_SPIMD);
> + r |= LOCOMO_SPIMD_XON;
> + writew(r, spidev->base + LOCOMO_SPIMD);
> +
> + r = readw(spidev->base + LOCOMO_SPIMD);
> + r |= LOCOMO_SPIMD_XEN;
> + writew(r, spidev->base + LOCOMO_SPIMD);
> + /* } */
Either remove or implement the comments.
> + r = readw(spidev->base + LOCOMO_SPICT);
> + r |= LOCOMO_SPIMD_XEN; /* FIXME */
> + writew(r, spidev->base + LOCOMO_SPICT);
FIXME?
> + if (t)
> + hz = t->speed_hz;
> + if (!hz)
> + hz = spi->max_speed_hz;
The core will ensure that the transfer always has a speed set in it.
> +static int locomo_spi_probe(struct platform_device *pdev)
> +{
> + struct resource *res;
> + struct spi_master *master;
> + struct locomospi_dev *spidev;
> + int ret;
> +
> + dev_info(&pdev->dev, "LoCoO SPI Driver\n");
Remove this, it's not adding anything.
signature.asc
Description: Digital signature
Re: [PATCH 00/15] new locomo driver
On Tue, Oct 28, 2014 at 12:13:39AM +, Russell King - ARM Linux wrote: > On Tue, Oct 28, 2014 at 03:01:53AM +0300, Dmitry Eremin-Solenikov wrote: > > Sharp Zaurus SL-5500 and SL-5600 use special companion Gate Array. Current > > drivers present in Linux kernel has some problems: > > * It uses custom bus instead of platform bus/mfd core. > I believe Greg wouldn't see that as a positive point. > Don't think that the platform bus _should_ always be used. It shouldn't > (Greg has said he'd like to see the platform bus to be totally killed off.) > Instead, custom buses properly suited to the class of device in question > is much preferred, especially if it aids in... > > * Device drivers are not well layered/separated. > ... better layering or separation of drivers. > So, thinking that converting from a custom bus to a platform bus > definitely is /not/ a positive step forward. > (Why mfd was ever allowed to re-use the platform bus stuff is a separate > question not relevent to these patches.) The reason we ended up reusing the platform bus so much was that originally people were doing custom buses but people started complaining that the code was (or should be) a cut'n'paste of the platform bus and that this duplication was both not pretty and a bit tedious for anyone doing anything that involved deploying good practice over a lot of buses. Early MFDs were actually MMIO devices so the platform bus was a good fit, then people (including me when I upstreamed the wm97xx drivers and apparently whoever worked on these devices) started adding custom buses and then people complianed that we should reuse both the bus type and the MFD infrastructure so here we are. Probably a way of sharing the platform code but giving the bus another name and bus_type would allow for better separation here. signature.asc Description: Digital signature
Re: [PATCH 2/4] Input: pmic8xxx-keypad - use regmap_field for register access
On Wed, Oct 08, 2014 at 01:32:33PM -0700, Dmitry Torokhov wrote: > On Wed, Oct 08, 2014 at 09:04:26PM +0100, Mark Brown wrote: > > On Wed, Oct 08, 2014 at 11:20:58AM -0700, Stephen Boyd wrote: > > > Srini/Mark, any reason why the regmap_field structure is opaque? > > So you can't peer into it and rely on the contents. I can see it being > > useful to add a bulk allocator. > And then one have to define offsets in an array and use awkward syntax > to access individual fields. Can we just reply on reviews/documentation > for users to not do wrong thing? I have very little confidence in users not doing awful things to be honest, this is the sort of API where the users are just random things all over the kernel so this sort of thing tends to be found after the fact. I get a lot of these in drivers that just got thrown over the wall so nobody really knows what things are doing when you do find them. If the standard allocators aren't doing a good job (I've not checked) I'd much rather handle this inside the API if we can. signature.asc Description: Digital signature
Re: [PATCH 2/4] Input: pmic8xxx-keypad - use regmap_field for register access
On Wed, Oct 08, 2014 at 11:20:58AM -0700, Stephen Boyd wrote: > On 10/08/2014 11:13 AM, Dmitry Torokhov wrote: > >>>Oops. struct regmap_field is opaque. It seems that the allocation > >>>is the only way that I could have instance of it. > >>Maybe we can add an API to allocate an array of fields? > >Maybe we could make the structure public instead? I do not see any > >reason for allocating something separately that has exactly the same > >lifetime as owning structure. The lifetime may be different to that of the register map it references, consider MFD function devices for example. > Srini/Mark, any reason why the regmap_field structure is opaque? So you can't peer into it and rely on the contents. I can see it being useful to add a bulk allocator. signature.asc Description: Digital signature
Re: [PATCH] input/joystick: use get_cycles on ARMv8
On Tue, Aug 12, 2014 at 10:01:49AM -0700, Dmitry Torokhov wrote: > On Mon, Aug 11, 2014 at 04:42:10PM +0100, Mark Brown wrote: > > As with ARM the ARMv8 architecture provides a cycle counter which can be > > used to provide a high resolution time for the joystick driver and silence > > the build warning that results from not having a precise timer on ARMv8, > > making allmodconfig and allyesconfig quieter. > Applied, although I wonder if it is time to retire whole gamecon subsystem. I > don't think there are many users left... OTOH it doesn't really do much harm either, the rate of updates seems pretty low. I was wondering about converting the warning to a runtime one - it'll probably be more likely to be seen by anyone who is using it and makes for less of these porting issues but I couldn't quite find the motivation. signature.asc Description: Digital signature
[PATCH] input/joystick: use get_cycles on ARMv8
From: Mark Brown
As with ARM the ARMv8 architecture provides a cycle counter which can be
used to provide a high resolution time for the joystick driver and silence
the build warning that results from not having a precise timer on ARMv8,
making allmodconfig and allyesconfig quieter.
Signed-off-by: Mark Brown
---
drivers/input/joystick/analog.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/joystick/analog.c b/drivers/input/joystick/analog.c
index 9135606c8649..ab0fdcd36e18 100644
--- a/drivers/input/joystick/analog.c
+++ b/drivers/input/joystick/analog.c
@@ -158,7 +158,7 @@ static unsigned int get_time_pit(void)
#define GET_TIME(x)rdtscl(x)
#define DELTA(x,y) ((y)-(x))
#define TIME_NAME "TSC"
-#elif defined(__alpha__) || defined(CONFIG_MN10300) || defined(CONFIG_ARM) ||
defined(CONFIG_TILE)
+#elif defined(__alpha__) || defined(CONFIG_MN10300) || defined(CONFIG_ARM) ||
defined(CONFIG_ARM64) || defined(CONFIG_TILE)
#define GET_TIME(x)do { x = get_cycles(); } while (0)
#define DELTA(x,y) ((y)-(x))
#define TIME_NAME "get_cycles"
--
2.0.1
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 0/7] mfd: AXP20x: Add support for AXP202 and AXP209
On Mon, May 19, 2014 at 09:47:41PM +0200, Carlo Caione wrote: > This set of patches introduces the core driver and support for two different > subsystems: > - Regulators > .../ABI/testing/sysfs-driver-input-axp-pek | 11 + > Documentation/devicetree/bindings/mfd/axp20x.txt | 93 +++ > .../devicetree/bindings/vendor-prefixes.txt| 1 + > arch/arm/configs/multi_v7_defconfig| 3 + > arch/arm/configs/sunxi_defconfig | 4 + > drivers/input/misc/Kconfig | 11 + > drivers/input/misc/Makefile| 1 + > drivers/input/misc/axp20x-pek.c| 281 > + > drivers/mfd/Kconfig| 12 + > drivers/mfd/Makefile | 1 + > drivers/mfd/axp20x.c | 258 +++ > include/linux/mfd/axp20x.h | 180 + > 12 files changed, 856 insertions(+) The regulator changes don't appear to be showing up in the diffstat or obviously in the series? signature.asc Description: Digital signature
Re: [PATCH v4 6/9] regulator: AXP20x: Add support for regulators subsystem
On Thu, May 15, 2014 at 08:03:06PM +0200, Boris BREZILLON wrote: > I know I'm late, and this patch has already been applied, but shouldn't > we use of_get_child_by_name instead of of_find_node_by_name. > This might lead to wrong regulators node parsing if other regulators are > defined in the DT after the axp20x node, because, AFAIK, this function > searches for DT nodes defined after the specified np node, but not > necessarely children of the np node. Yes. signature.asc Description: Digital signature
Re: [PATCH 0/4] Input: atmel_mxt_ts - make it work on Tegra
On Thu, May 08, 2014 at 08:56:04PM +0100, Nick Dyer wrote: > Thanks! I had wondered why you had left it, you never communicated this to > me unfortunately. Would it be preferable for me to switch to > request_firmware_nowait() and complete the probe asynchronously after it > returns? Another common option is to defer the firmware request/load until the device is opened, but that doesn't work for all devices. That should mean that userspace is up and running. signature.asc Description: Digital signature
[PATCH] input: Request a shared interrupt for AMBA KMI devices.
From: Liviu Dudau
Recent ARM boards have the KMI devices share one interrupt line rather
than having dedicated IRQs. Update the driver to take that into account.
Signed-off-by: Liviu Dudau
Signed-off-by: Mark Brown
---
drivers/input/serio/ambakmi.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/input/serio/ambakmi.c b/drivers/input/serio/ambakmi.c
index 762b08432de0..8b748d99b934 100644
--- a/drivers/input/serio/ambakmi.c
+++ b/drivers/input/serio/ambakmi.c
@@ -79,7 +79,8 @@ static int amba_kmi_open(struct serio *io)
writeb(divisor, KMICLKDIV);
writeb(KMICR_EN, KMICR);
- ret = request_irq(kmi->irq, amba_kmi_int, 0, "kmi-pl050", kmi);
+ ret = request_irq(kmi->irq, amba_kmi_int, IRQF_SHARED, "kmi-pl050",
+ kmi);
if (ret) {
printk(KERN_ERR "kmi: failed to claim IRQ%d\n", kmi->irq);
writeb(0, KMICR);
--
2.0.0.rc2
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [alsa-devel] [PATCH 01/15] ASoC: CS42L51 and WM8962 codecs depend on INPUT
On Thu, May 01, 2014 at 02:16:27AM +, Austin, Brian wrote: > Apparently not. > I would like to come up with a better solution than making INPUT a > requirement. I just need some time. > In the meantime I suppose it’s OK to apply it? Yeah. Realistically it's probably not going to ever be a practical problem - the number of systems with audio but no input is likely to be very close to zero. signature.asc Description: Digital signature
Re: [alsa-devel] [PATCH 01/15] ASoC: CS42L51 and WM8962 codecs depend on INPUT
On Tue, Apr 29, 2014 at 09:31:30PM -0500, Brian Austin wrote: > I assume you mean the CS42L52 instead of the L51. INPUT is used for a BEEP > Generator but when I disable CONFIG_INPUT I do not get an error. Is there > any information available on what the error is? I suspect it's ASoC built in and INPUT build as a module. signature.asc Description: Digital signature
Re: [PATCH 01/15] ASoC: CS42L51 and WM8962 codecs depend on INPUT
On Tue, Apr 29, 2014 at 07:18:22PM +0800, Xia Kaixu wrote: > From: Arnd Bergmann > > Building ARM randconfig got into a situation where CONFIG_INPUT > is turned off and SND_SOC_ALL_CODECS is turned on, which failed > for two codecs trying to use the input subsystem. Some other Applied, but... > Cc: Mark Brown > Cc: Liam Girdwood > Cc: Ben Dooks > Cc: Kukjin Kim > Cc: Sangbeom Kim > Cc: Lars-Peter Clausen > Cc: Timur Tabi > Cc: linux-arm-ker...@lists.infradead.org > Cc: linux-samsung-...@vger.kernel.org > Cc: linux-input@vger.kernel.org > Cc: alsa-de...@alsa-project.org ...please don't include noise like this in patch submissions, especially with such long lists (which still manage to miss the driver maintainers concerned). signature.asc Description: Digital signature
Re: [PATCH v4 7/9] ARM: sun7i/sun4i: dt: Add AXP209 support to various boards
On Thu, Apr 24, 2014 at 05:58:47PM +0100, Charles Keepax wrote: > Ah ok seems I am getting an error but for some reason for me it > shows up looking very unrelated to the supply mapping. In that it > shows up much later in the log and doesn't seem to mention the > MFD at all: If you look at the warning you'll see that it's complaining that it's trying to probe a device which has devres stuff attached to it which is happens at the time the function driver gets loaded. signature.asc Description: Digital signature
Re: [PATCH v4 7/9] ARM: sun7i/sun4i: dt: Add AXP209 support to various boards
On Wed, Apr 23, 2014 at 10:25:46PM +0200, Carlo Caione wrote: > I'm having a really hard time with this problem, so any hint is welcome > :) The small modification I'm using on top of the patches in this series > is here: http://bpaste.net/show/228330/ > Unfortunately as I said I got this when booting: > http://bpaste.net/show/nUhUTzELT32v9HNPathL/ Huh, actually the arizona drivers do show this (it was being masked in my logs by another unrelated bug). I guess the Wolfson guys aren't working with upstream much (though Charles did write the orignal code here...). The issue is the MFD core, it shouldn't be using managed allocations here - the error reported by the assert is entirely correct. If the CODEC driver is bound and unbound it'll not be possible to reload it as things stand. Your driver is correct but the implementation needs to be fixed - possibly with an API change on free since at the minute the cells to be freed don't get passed back into the MFD core when deallocating. signature.asc Description: Digital signature
Re: [PATCH] Input: ads7877: Remove bitrotted comment
On Tue, Apr 22, 2014 at 05:39:28PM -0700, Dmitry Torokhov wrote: > On Tue, Apr 22, 2014 at 10:18:21PM +0100, Mark Brown wrote: > > Remove the bitrotted comment, though in actual fact the use case mentioned > > is a great use for spi_async() since it would cut down on latency handling > > the interrupt by saving us a context switch before we start SPI. > > This was previously implemented, it was removed in commit b534422b2d11 > > (Input: ad7877 - switch to using threaded IRQ) for code complexity reasons. > > It may be better to revert that commit instead. > Hmm, maybe.. although I think original would cause device 'stuck' if > call to spi_async() fails, so probably not a straight revert... Probably best just to apply this, then - someone can always reimplement if they need to. signature.asc Description: Digital signature
[PATCH] Input: ads7846: Correct log message for spi_sync() errors
From: Mark Brown
While searching for users of spi_async() I got a false positive in the
ads7846 driver, fix that.
Signed-off-by: Mark Brown
---
drivers/input/touchscreen/ads7846.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/touchscreen/ads7846.c
b/drivers/input/touchscreen/ads7846.c
index 7f8aa981500d..da201b8e37dc 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -706,7 +706,7 @@ static void ads7846_read_state(struct ads7846 *ts)
m = &ts->msg[msg_idx];
error = spi_sync(ts->spi, m);
if (error) {
- dev_err(&ts->spi->dev, "spi_async --> %d\n", error);
+ dev_err(&ts->spi->dev, "spi_sync --> %d\n", error);
packet->tc.ignore = true;
return;
}
--
1.9.2
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Input: ads7877: Remove bitrotted comment
From: Mark Brown
While searching for users of spi_async() I found a reference in the ad7877
driver to using it to initiate data transfer from the interrupt handler.
However there is no code for this, instead the interrupt handler is a
threaded handler and uses spi_sync() instead.
Remove the bitrotted comment, though in actual fact the use case mentioned
is a great use for spi_async() since it would cut down on latency handling
the interrupt by saving us a context switch before we start SPI.
This was previously implemented, it was removed in commit b534422b2d11
(Input: ad7877 - switch to using threaded IRQ) for code complexity reasons.
It may be better to revert that commit instead.
Signed-off-by: Mark Brown
---
drivers/input/touchscreen/ad7877.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/input/touchscreen/ad7877.c
b/drivers/input/touchscreen/ad7877.c
index 6793c85903ae..523865daa1d3 100644
--- a/drivers/input/touchscreen/ad7877.c
+++ b/drivers/input/touchscreen/ad7877.c
@@ -210,11 +210,6 @@ static bool gpio3;
module_param(gpio3, bool, 0);
MODULE_PARM_DESC(gpio3, "If gpio3 is set to 1 AUX3 acts as GPIO3");
-/*
- * ad7877_read/write are only used for initial setup and for sysfs controls.
- * The main traffic is done using spi_async() in the interrupt handler.
- */
-
static int ad7877_read(struct spi_device *spi, u16 reg)
{
struct ser_req *req;
--
1.9.2
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 7/9] ARM: sun7i/sun4i: dt: Add AXP209 support to various boards
On Thu, Apr 17, 2014 at 12:06:34PM +0200, Carlo Caione wrote: > I'm fighting with a small issue when using the > regulator_bulk_register_supply_alias(). Problem is that when using the > .parent_supplies entry in the MFD driver, I hit the > > WARN_ON(!list_empty(&dev->devres_head)); > > in linux/drivers/base/dd.c#L272, but, apart from the warning, > everything seems to work correctly. > A possible explanation I gave myself is that in the mfd_add_device() > we try to use the devm_* API when the regulator device is not bound to > the driver yet (I found some information here > http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/104442.html). > Is this the case? Without knowing more about the case you're hitting it's hard to say - I do run a board which exercises the API for a MFD (with the arizona drivers) regularly and haven't noticed an issue so there must be something different about what you're trying to do. signature.asc Description: Digital signature
Re: [linux-sunxi] Re: [PATCH v4 7/9] ARM: sun7i/sun4i: dt: Add AXP209 support to various boards
On Mon, Apr 14, 2014 at 12:20:32PM +0200, Hans de Goede wrote: Please fix your mailer to word wrap at less than 80 columns. > As Mark has also mentioned we should probably pin the regulators to a > certain voltage, except for those which we expect to be controlled by > a driver, so basically all of them should be pinned to a certain > voltage except for DCDC2 which gets used for the cpu voltage which we > will want to scale as soon as we've a cpufreq driver. If you don't know what to do with the regulators and don't have any information on what's safe then you shouldn't be specifying a voltage at all. > While testing the latest revision of your code I also noticed that the > kernel ends up disabling LDO3 and LDO4, which could be fine on some > boards and a problem on other boards. > I think we need to be careful here. For now it may be best to only add > the DCDC2 regulator to the dts, as we know that dcdc2 is used for the > cpu voltage everywhere, and we will actually want to control that > later on. You need to at least specify that regulators that need to be kept on are always-on. > For the others, for the boards where we've schematics (*) it would be > good to add the other regulators with fixed voltages as specified in > the schematics. For the rest it may be best to simply leave the > regulators alone / at their default settings. If everyone has been running the board at a voltage different to that in the schematic then I'd not assume that everything has been validated at the voltage in the schematic, if production firmware is available that's going to be a more reliable guide than the schematic but it sounds like all these boards have just been left to run at their default voltages. signature.asc Description: Digital signature
Re: [PATCH v4 7/9] ARM: sun7i/sun4i: dt: Add AXP209 support to various boards
On Fri, Apr 11, 2014 at 03:04:32PM +0200, Carlo Caione wrote:
> On Fri, Apr 11, 2014 at 2:29 PM, Mark Brown wrote:
> >> + regulators {
> >> + compatible = "x-powers,axp20x-reg";
> > This compatible isn't part of the driver.
> Yes I know. The problem here is that in v4 I had to fill in the field
> .of_compatible of the mfd_cell with "x-powers,axp20x-reg". This
> because the regulator_dev_lookup() checks for dev->of_node when
> looking for the supply so I needed the compatible string in the DT to
> have the dev->of_node filled in by mfd_add_device().
> What do you suggest? Modify the regulator driver?
You're looking for regulator_bulk_register_supply_alias() in the MFD
driver (via parent_supplies in the MFD cell probably).
signature.asc
Description: Digital signature
Re: [PATCH v4 7/9] ARM: sun7i/sun4i: dt: Add AXP209 support to various boards
On Fri, Apr 11, 2014 at 11:38:11AM +0200, Carlo Caione wrote:
> In all the DTs the min and max microvolt allowed for each regulator are
> actually
> the min and max voltage possible for the regulator itself. This is not safe
> but
> we do not have the ranges allowed for each board and the original Allwinner
> driver does exactly this way.
Is there any code in their kernel which varies the supply voltages? If
there isn't then simply omitting the voltage ranges is the best option,
leaving the supplies fixed. If there is then the range it uses is a
good starting point. In general supplies will be fixed voltage on a
given board unless there is a specific reason to vary them.
> + regulators {
> + compatible = "x-powers,axp20x-reg";
This compatible isn't part of the driver.
signature.asc
Description: Digital signature
Re: [PATCH v4 6/9] regulator: AXP20x: Add support for regulators subsystem
On Fri, Apr 11, 2014 at 11:38:10AM +0200, Carlo Caione wrote: > AXP202 and AXP209 come with two synchronous step-down DC-DCs and five > LDOs. This patch introduces basic support for those regulators. Applied, thanks, but I had to resolve some trivial add/add conflicts with the Broadcom regulator driver Makefile and Kconfig additions - please remember to submit patches against current code. signature.asc Description: Digital signature
Re: [PATCH v4 1/9] mfd: AXP20x: Add mfd driver for AXP20x PMIC
On Fri, Apr 11, 2014 at 01:25:03PM +0200, Arnd Bergmann wrote: > Why do you have to enumerate the interrupts here? Can't you just > put all the numbers into the DT nodes of the devices using them? > In general, I would say that the mfd driver should not care about > what is connected to it. This then means that all the machines using the device need to define the interrupt table and have the MFD cells represented in the DT which means encoding Linux abstractions into the DT. In cases where the device is also used with ACPI or platform data that's a definite issue since they have different idioms. That applies less to PMICs tightly bound to particular SoCs but is an issue in general, not all the world is DT. There's also issues here with us changing our subsystems. Things like clocks are a bit indistinct at present, they're sort of floating between clock and other subsystems. We've also done things like invent extcon, making completely new subdevices. Keeping the data out of DT avoids problems when this happens. The balance changes a bit if there are clearly reusable IPs within the device but sadly hardware designers don't always give us that and even then sometimes we don't want to use them like that. signature.asc Description: Digital signature
Re: [linux-sunxi] Re: [PATCH v3 06/10] regulator: AXP20x: Add support for regulators subsystem
On Sat, Mar 29, 2014 at 06:52:01PM +0100, Carlo Caione wrote: > On Fri, Mar 28, 2014 at 01:39:34PM +0000, Mark Brown wrote: > > This is fairly obviously broken - it's overwriting the normal runtime > > value, this will disrupt the running system if we want the value we use > > on suspend is different to the value we want at runtime. > Ok, silly question: isn't it exactly what we want? Set the voltage for > the regulator when the system is suspended? This needs to be done without affecting the current system state - many PMICs are aware of system suspend and have support for transitioning into a separate suspend state, this should only be configuring that. We may call this function separately to the actual suspend. > > Think about it - if this was a sane thing to do the core would just do > > it without needing driver specific code, we already know how to set the > > voltage for the device. > I thought it was because some regulators can have specific regs for > managing the suspend mode. Right, but if the PMIC doesn't have that feature then it should indicate that by not implementing the operation and letting the core worry about how to handle the situation. > BTW, but then what is the difference between my code and (i.e.) the same > routine in da9055-regulator.c? > http://lxr.linux.no/linux+v3.13.5/drivers/regulator/da9055-regulator.c#L276 That's updating the B register set which IIRC is used only in suspend mode. signature.asc Description: Digital signature
Re: [PATCH v3 06/10] regulator: AXP20x: Add support for regulators subsystem
On Thu, Mar 27, 2014 at 10:29:20PM +0100, Carlo Caione wrote:
> +static int axp20x_set_suspend_voltage(struct regulator_dev *rdev, int uV)
> +{
> + int sel = regulator_map_voltage_iterate(rdev, uV, uV);
> +
> + if (sel < 0)
> + return sel;
> +
> + return regulator_set_voltage_sel_regmap(rdev, sel);
> +}
This is fairly obviously broken - it's overwriting the normal runtime
value, this will disrupt the running system if we want the value we use
on suspend is different to the value we want at runtime.
Think about it - if this was a sane thing to do the core would just do
it without needing driver specific code, we already know how to set the
voltage for the device.
signature.asc
Description: Digital signature
Re: [PATCH v3 07/10] ARM: sunxi: dt: Add x-powers-axp209.dtsi file
On Thu, Mar 27, 2014 at 10:29:21PM +0100, Carlo Caione wrote:
> This dtsi describes the axp209 PMIC, and is to be included from inside
> the i2c controller node to which the axp209 is connected.
> arch/arm/boot/dts/x-powers-axp209.dtsi | 54
> ++
Something is wrong here. Either the changelog is inaccurate or this is
broken, either way this should be cleaned up because this looks like
very bad practice. If this is a common include file for boards derived
from some reference design then the changelog is misleading and the DT
should be clearer about the board tie ins. Otherwise the .dtsi is
defining what should be board specific parameters.
The fact that the DT names everything after the regulator on the PMIC
and not after the supply names on the board is especially suspicious and
glancing at a couple of the regulators it looks like the constraints
here are the maximum ranges the PMIC supports rather than anything to do
with any board.
Please take a step back and think about what the regulator constraints
are intended to do - they're about matching the regulator capabilities
to the board since it is rare for boards to be able to do everything the
regulator can support. Duplicating the basic device capabilities into
the DT would be a pointless waste of time.
> + axp_dcdc2: dcdc2 {
> + regulator-min-microvolt = <70>;
> + regulator-max-microvolt = <2275000>;
> + regulator-always-on;
> + };
What is this configuration actually for - what guarantee do we have that
the above is safe for a given board using this regulator?
In general I'd expect to see anything specifying variability for a
regulator to also include the relevant consumer node.
signature.asc
Description: Digital signature
Re: [PATCH v3 08/10] ARM: sun7i/sun4i: dt: Add AXP209 support to various boards
On Fri, Mar 28, 2014 at 01:54:12PM +0100, Maxime Ripard wrote: > On Fri, Mar 28, 2014 at 11:38:39AM +0000, Mark Brown wrote: > > Hang on, what is an include file setting up regulators for? > Look at patch 7. To share the regulators declaration across all boards > and avoid duplication. Oh, wonderful. I'll go and reply to that... signature.asc Description: Digital signature
Re: [PATCH v3 08/10] ARM: sun7i/sun4i: dt: Add AXP209 support to various boards
On Fri, Mar 28, 2014 at 11:11:46AM +0100, Maxime Ripard wrote:
> Here, you would just include the dtsi at the top of the file, and in
> the DTSI, you would have something like:
> &axp209 {
>regulators {
> ...
>}
> }
Hang on, what is an include file setting up regulators for?
signature.asc
Description: Digital signature
Re: [linux-sunxi] Re: [PATCH v2 1/8] mfd: AXP20x: Add mfd driver for AXP20x PMIC
On Sat, Mar 22, 2014 at 08:08:03PM +0100, Carlo Caione wrote: > On Sat, Mar 22, 2014 at 11:42:01AM -0700, Dmitry Torokhov wrote: > > > drivers/mfd/axp20x.c:172:18: warning: assignment makes integer > > > frompointer without a cast [enabled by default] > > You need to cast to long, otherwise you will get warnings when compiling > > on 64 bit arches. > Actually no warning also with (int) cast. That's because you squashed it to something smaller than a long using the cast. The other option is to make the variable you're assigning to suitable to hold the value - intptr_t or a long. signature.asc Description: Digital signature
Re: [linux-sunxi] Re: [PATCH v2 1/8] mfd: AXP20x: Add mfd driver for AXP20x PMIC
On Sat, Mar 22, 2014 at 05:51:32PM +0100, Carlo Caione wrote:
> On Tue, Mar 18, 2014 at 03:59:19PM +, Lee Jones wrote:
> > > + of_id = of_match_device(axp20x_of_match, &i2c->dev);
> > > + if (!of_id) {
> > > + dev_err(&i2c->dev, "Unable to setup AXP20X data\n");
> > > + return -ENODEV;
> > > + }
> > > + axp20x->variant = (int) of_id->data;
> > No need to cast from void *.
> My compiler says otherwise :)
Are you sure your compiler isn't correctly telling you not to cast away
const? If the compiler is complaining about a cast on void then it's
spotted a definite bug.
signature.asc
Description: Digital signature
Re: [PATCH v2 5/8] regulator: AXP20x: Add support for regulators subsystem
On Sat, Mar 15, 2014 at 04:43:42PM +0100, Carlo Caione wrote:
> AXP202 and AXP209 come with two synchronous step-down DC-DCs and five
> LDOs. This patch introduces basic support for those regulators.
This is mostly fine apart from the things Krzysztof mentioned and...
> +static int axp20x_set_dcdc_freq(struct platform_device *pdev, u32 dcdcfreq)
> +{
> + struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
> +
> + if (dcdcfreq < 750)
> + dcdcfreq = 750;
> +
> + if (dcdcfreq > 1875)
> + dcdcfreq = 1875;
I'd expect at least a warning on out of spec values being specified
here.
signature.asc
Description: Digital signature
Re: [PATCH v3 4/4] mfd: max8997: move regmap handling to function drivers
On Thu, Mar 13, 2014 at 12:55:04PM +, Lee Jones wrote: > > Is it necessary? If previous mfd driver has various i2c line, previous mfd > > driver > > initialize regmap/i2c setting on mfd driver. > > I'm not sure that regmap/i2c setting code move from mfd driver to each > > driver. > > > > Dear Lee Jones, > > I need your opinion about moving regmap/i2c code from mfd driver to each > > driver. > I'd rather take advice from Mark on this one. I don't really case that much; I'm having a hard time seeing it as particularly useful to do the refactoring but if it makes people happy... Keeping things in the core would help promote reusability I guess but I'm not sure that's likely to actually happen with this sort of driver/device. signature.asc Description: Digital signature
Re: [linux-sunxi] Re: [PATCH 6/7] regulator: AXP20x: Add support for regulators subsystem
On Tue, Mar 11, 2014 at 10:06:59PM +0100, Carlo Caione wrote: > On Tue, Mar 11, 2014 at 07:29:40PM +0000, Mark Brown wrote: > > With the above code the driver will return an error and never get as far > > as registering the regulator. > I agree, but if I register the regulators without having the constrains > defined in the DT, when regulator_init_complete() is > called, the regulators are disabled powering off the SoC (at least for > DCDC2 and DCDC3). Right, but the kernel will say what it's doing before it does so and then the user can provide the appropriate setup in DT to force things on. signature.asc Description: Digital signature
Re: [linux-sunxi] Re: [PATCH 6/7] regulator: AXP20x: Add support for regulators subsystem
On Tue, Mar 11, 2014 at 08:24:11PM +0100, Carlo Caione wrote:
> On Mon, Mar 03, 2014 at 10:56:16AM +0900, Mark Brown wrote:
> > > + regulators = of_find_node_by_name(np, "regulators");
> > > + if (!regulators) {
> > > + dev_err(&pdev->dev, "regulators node not found\n");
> > > + return -EINVAL;
> > > + }
> > The driver should be able to start up with no configuration provided at
> > all except for the device being registered - the user won't be able to
> > change anything but they will be able to read the current state of the
> > hardware which can be useful for diagnostics.
> If the device is DT enabled and no configuration is provided for
> regulators, these are disabled according to:
> http://lxr.linux.no/linux+v3.13.5/drivers/regulator/core.c#L3797
> Am I missing something or do you have any pointer?
With the above code the driver will return an error and never get as far
as registering the regulator.
signature.asc
Description: Digital signature
Re: [linux-sunxi] Re: [PATCH 6/7] regulator: AXP20x: Add support for regulators subsystem
On Sat, Mar 08, 2014 at 12:43:04PM +0100, Carlo Caione wrote: > On Fri, Mar 7, 2014 at 7:22 PM, Maxime Ripard > > On Sat, Mar 01, 2014 at 05:45:51PM +0100, Carlo Caione wrote: > >> + return platform_driver_register(&axp20x_regulator_driver); > > I thought the AXP was only connected through I2C? How is that a > > platform device then? > Not really. It is plain wrong. > I'll fix it in v2. Are you sure this is wrong? For MFDs the core MFD is a driver of whatever bus type is in use but the function drivers are platform devices which talk to the hardware via the core device. signature.asc Description: Digital signature
Re: [PATCH v2] mfd: max8997: use regmap to access registers
On Wed, Mar 05, 2014 at 10:54:39AM -0800, Dmitry Torokhov wrote: > On Wed, Mar 05, 2014 at 03:58:17PM +0100, Robert Baldyga wrote: > > -int max8997_write_reg(struct i2c_client *i2c, u8 reg, u8 value) > > +int max8997_write_reg(struct regmap *map, u8 reg, u8 value) > Why don't you make read/write reg to take struct max8997_dev as argument > instead of regmap? regmap seems to be the current implementation du jur, > but that is core's detail, functions do not need to care. Indeed, and had this been done originally this refactoring would be much smoother. signature.asc Description: Digital signature
Re: [PATCH 6/7] regulator: AXP20x: Add support for regulators subsystem
On Sat, Mar 01, 2014 at 05:45:51PM +0100, Carlo Caione wrote:
> index d59c826..0cef101 100644
> --- a/arch/arm/configs/sunxi_defconfig
> +++ b/arch/arm/configs/sunxi_defconfig
> @@ -69,3 +69,4 @@ CONFIG_NFS_FS=y
> CONFIG_ROOT_NFS=y
> CONFIG_NLS=y
> CONFIG_PRINTK_TIME=y
> +CONFIG_REGULATOR_AXP20X=y
If you want to update the defconfig do it in a separate patch.
> +static int axp20x_set_suspend_voltage(struct regulator_dev *rdev, int uV)
> +{
> + return regulator_set_voltage_sel_regmap(rdev, 0);
> +}
I'm not entirely clear what this is supposed to do but it's broken - it
both ignores the voltage that is passed in and immediately writes to the
normal voltage select register which will presumably distrupt the system.
> +static int axp20x_io_enable_regmap(struct regulator_dev *rdev)
> +{
> + return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
> + rdev->desc->enable_mask, AXP20X_IO_ENABLED);
> +}
Please make some helpers for this (or extend the existing ones to cope
with more than a single bit control) - there's several other devices
which have similar multi-bit controls so we should factor this out
rather than open coding.
> + np = of_node_get(pdev->dev.parent->of_node);
> + if (!np)
> + return 0;
> +
> + regulators = of_find_node_by_name(np, "regulators");
> + if (!regulators) {
> + dev_err(&pdev->dev, "regulators node not found\n");
> + return -EINVAL;
> + }
The driver should be able to start up with no configuration provided at
all except for the device being registered - the user won't be able to
change anything but they will be able to read the current state of the
hardware which can be useful for diagnostics.
> +static int axp20x_set_dcdc_workmode(struct regulator_dev *rdev, int id, u32
> workmode)
> +{
> + unsigned int mask = AXP20X_WORKMODE_DCDC2_MASK;
> +
> + if ((id != AXP20X_DCDC2) && (id != AXP20X_DCDC3))
> + return -EINVAL;
> +
> + if (id == AXP20X_DCDC3)
> + mask = AXP20X_WORKMODE_DCDC3_MASK;
> +
> + return regmap_update_bits(rdev->regmap, AXP20X_DCDC_MODE, mask,
> workmode);
> +}
What is a workmode?
> + pmic->rdev[i] = regulator_register(&pmic->rdesc[i], &config);
> + if (IS_ERR(pmic->rdev[i])) {
Use devm_regulator_register() and then you don't need to clean up the
regulators either.
> +static int __init axp20x_regulator_init(void)
> +{
> + return platform_driver_register(&axp20x_regulator_driver);
> +}
> +
> +subsys_initcall(axp20x_regulator_init);
module_platform_driver().
signature.asc
Description: Digital signature
Re: [PATCH 2/7] Input: pmic8xxx-pwrkey - Migrate to regmap APIs
On Thu, Jan 02, 2014 at 11:17:57AM -0800, Dmitry Torokhov wrote: > On Thu, Jan 02, 2014 at 06:49:59PM +0000, Mark Brown wrote: > > On Sun, Dec 15, 2013 at 03:33:37AM -0800, Dmitry Torokhov wrote: > > > > + regmap = dev_get_regmap(pdev->dev.parent, NULL); > > > > + if (!regmap) > > > > + return -ENODEV; > > > And you are leaking memory here... > > He's not, dev_get_regmap() just gets a pointer to an existing regmap - > > no reference is taken and nothing is allocated. It's a helper that's > I was not talking about data returned by dev_get_regmap() but all other > memory that was allocated before as this was pre devm conversion of the > driver. Ah, OK - I thought you meant that as that was the code immediately prior to your reply. Sorry for the confusion. signature.asc Description: Digital signature
Re: [PATCH 2/7] Input: pmic8xxx-pwrkey - Migrate to regmap APIs
On Sun, Dec 15, 2013 at 03:33:37AM -0800, Dmitry Torokhov wrote: > On Tue, Dec 10, 2013 at 03:43:11PM -0800, Stephen Boyd wrote: > > + regmap = dev_get_regmap(pdev->dev.parent, NULL); > > + if (!regmap) > > + return -ENODEV; > And you are leaking memory here... He's not, dev_get_regmap() just gets a pointer to an existing regmap - no reference is taken and nothing is allocated. It's a helper that's mainly there so that generic code can be written without needing the regmap to be passed around. The caller is responsible for ensuring that it will stick around for as long as it's used (generally by having it lifetime managed with the device). signature.asc Description: Digital signature
Re: [PATCH v4] Input: add regulator haptic driver
On Fri, Oct 25, 2013 at 03:29:51PM +0900, Hyunhee Kim wrote: > From: Hyunhee Kim > Date: Wed, 9 Oct 2013 16:21:36 +0900 > Subject: [PATCH] Input: add regulator haptic driver This is OK from a regulator point of view though there's still a lot of use of regulator devm_ and input devm_ that could happen - it wasn't just the kzalloc(), it was most of the allocations in the driver. You also have some word wrapping in your e-mail so the patch probably won't apply. signature.asc Description: Digital signature
Re: [PATCH] Input: add regulator haptic driver
On Fri, Oct 11, 2013 at 11:22:00AM +0900, hyunhee.kim wrote:
> + if (enable && !haptic->enabled) {
> + haptic->enabled = true;
> + ret = regulator_enable(haptic->regulator);
> + if (ret)
> + pr_err("haptic: %s failed to enable regulator\n",
> + __func__);
These should probably set the flag after the regulator API call has
succeeded, otherwise if the call fails the driver will incorrectly
remember that the regulator is enabled and never disable it (or vice
versa on disable).
> +static void regulator_haptic_work(struct work_struct *work)
> +{
> + struct regulator_haptic *haptic = container_of(work,
> +struct
> regulator_haptic,
> +work);
> + if (haptic->level)
> + regulator_haptic_enable(haptic, true);
> + else
> + regulator_haptic_enable(haptic, false);
> +
> +}
Should the mutex for the level be at this level rather than in the
subfunctions? Though it'd probably be just as well to inline the true
and false cases since there's basically two equivalent forks in the
enable function anyway.
> +
> + haptic = kzalloc(sizeof(*haptic), GFP_KERNEL);
> + if (!haptic) {
> + dev_err(&pdev->dev, "unable to allocate memory for
> haptic\n");
> + return -ENOMEM;
> + }
Might be better to use devm_ functions throughout here, saves error
handling and cleanup code...
> +static int regulator_haptic_remove(struct platform_device *pdev)
> +{
> + struct regulator_haptic *haptic = platform_get_drvdata(pdev);
> +
> + input_unregister_device(haptic->input_dev);
> +
> + return 0;
...which is currently missing.
signature.asc
Description: Digital signature
Re: [PATCH 10/53] Input: atmel_mxt_ts - Add memory access interface via sysfs
On Thu, Jul 11, 2013 at 08:41:41AM +0100, Nick Dyer wrote: > Mark Brown wrote: > > On Fri, Jun 21, 2013 at 09:16:16AM -0700, Nick Dyer wrote: > >> For some operations it does. For example updating the whole chip config > >> (which is a common thing to want to do), it would turn a couple of write > >> operations into ~20 on recent chips. > > Is that really happening on peformance critical paths other than initial > > power up (which could be handled more neatly anyway). > Well, you're right that we could probably add more API for performance > critical stuff. But that wasn't your original question. You probably don't need another API here - for example, if the driver understands that the device is powered off and knows enugh about what the device is doing to understand that it doesn't need to be running then it could for example cache what is being written then flush in a more optimal fashion. > > If absoluely nobody has used the separate wakeup pin then the hardware > > designers are wasting a pin there... my point isn't that nobody would > > use the reference design it's that some boards will have the separate > > signal. > That's entirely hypothetical, and you're wasting our time until you can > actually point to such hardware, happy to write patches to support that > mode of operation as well if you do. See above - it's not just about the possibility that someone might have done a better job with the hardware design, better power management is just a good idea in general. signature.asc Description: Digital signature
Re: [PATCH 01/22] mfd/ti_am335x_tscadc: remove regmap
On Thu, Jul 04, 2013 at 11:02:41AM +0200, Sebastian Andrzej Siewior wrote: > The driver here does not use atomic updates but read followed by write > so your locking here is futile. So the API/regmap alone does not make Doesn't that sound like the driver ought to be using a r/m/w primitive though? > it right. And look: the MFD part uses regmap. Its children (IIO & > input) do not use it. After I told this Samuel he said that it is okay. Again I think the point here was that they probably ought to do so. But I guess if you're saying there's no problem that's fine... signature.asc Description: Digital signature
Re: [PATCH 10/53] Input: atmel_mxt_ts - Add memory access interface via sysfs
On Fri, Jun 21, 2013 at 09:16:16AM -0700, Nick Dyer wrote: > Mark Brown wrote: > > Yes, to be honest. I'd hope it wouldn't be increasing the number of > > read/write operations... > For some operations it does. For example updating the whole chip config > (which is a common thing to want to do), it would turn a couple of write > operations into ~20 on recent chips. Is that really happening on peformance critical paths other than initial power up (which could be handled more neatly anyway). > > That still sounds like something the driver can handle (for example, by > > eating the first error silently if it knows the chip is powered down) > We've tried to implement this idea of tracking the chip power state in > the driver and only eating the first error silently when necessary. But > there are various entertaining corner cases (for example, it may or may > not be in sleep on probe, how do you deal with intermittent i2c glitch). It > would end up either being very brittle or an extremely complex mechanism > involving tracking state and timers, the result of which is only to > suppress a dmesg debug output saying "i2c retry", and to fail very slightly > earlier in the normal i2c failure case. The normal fast path through > this code is exactly the same. It seems very suspicous that this device has all these problems but others don't... > > and of course a system integrator may choose not to copy the reference > > design in this respect, it does seem a bit odd after all. > You're being a bit optimistic there. Examples of devices that require > this are Samsung Galaxy Tab 10.1, Asus Transformer TF101. If absoluely nobody has used the separate wakeup pin then the hardware designers are wasting a pin there... my point isn't that nobody would use the reference design it's that some boards will have the separate signal. signature.asc Description: Digital signature
Re: [PATCH 10/53] Input: atmel_mxt_ts - Add memory access interface via sysfs
On Tue, Jun 11, 2013 at 01:16:31PM +0100, Nick Dyer wrote: > Without being able to name all of the registers (which would require a > large amount of architecture to keep up-to-date and would probably turn > into an unmaintainable mess), you can only split up the register map into > separate parts. You'd end up with binary attributes like this (refer to the > document I linked for explanation): ... > Is that a nicer design from your point of view? I don't personally think > that is really gains anything functional other than making the API more > complex and increasing the number of read/write operations. I guess you Yes, to be honest. I'd hope it wouldn't be increasing the number of read/write operations... > would stop bugs in user space code from accidentally writing into the wrong > object. That seems like a useful thing, and it does allow the driver to relatively easily understand what any of the attributes is doing and take it over if it needs to. > > Well, there's also the potential issues with standard application layer > > code either not being able to do things that the hardware supports or > > getting into a fight with the magic custom stuff. > I think it's better to present a clean API cut at the right level. If you > look at the drivers in various Android trees for these maXTouch chips, > there's an awful lot of phone specific code which is not very general and > it would be impossible to mainline without having a 20,000 line driver full > of #ifdefs. Surely it's better for that to go into a userspace daemon if > possible? Yes, having some of the stuff that understands the contents of the controls in userspace is sensible. However the kernel does offer an API for controlling devices it supports and it seems reasonable to expect that to work too - callibration and whatnot is a bit different but core functionality should just work from the kernel. > >> On suspend the driver puts chip into "deep sleep" where touch acquisition > >> is halted and minimal power consumed. But it will still come out of its > >> sleep state temporarily to service I2C comms if necessary (although one > >> particular family requires that I2C retry for this case). > > So these errors are just part of waking the chip up - in that case > > shouldn't the driver be waking the chip up automatically? > In the reference design for that particular model of chip (mXT1386), there > is a WAKEUP pin cross-wired to an I2C line. The only way of getting it to > wake up when it is asleep is by trying to perform an I2C operation, which > will fail, and then retrying before it times out and goes back to sleep > again. There isn't any other way of waking it up. That still sounds like something the driver can handle (for example, by eating the first error silently if it knows the chip is powered down) and of course a system integrator may choose not to copy the reference design in this respect, it does seem a bit odd after all. signature.asc Description: Digital signature
Re: [PATCH 01/22] mfd/ti_am335x_tscadc: remove regmap
On Mon, Jun 17, 2013 at 01:41:40PM +0200, Sebastian Andrzej Siewior wrote: > On 06/14/2013 03:53 PM, Mark Brown wrote: > > It does give you tracepoints and debugfs. If it's making things at > > all complicated we need to look at why that is and figure out how > > to fix that since it's probably an issue for other users. > debugfs are tracepoints is our offer? Let me check the price one more > time. OK... > This is a lot of for a simple mmio access. In terms of performance: If > I add a trace point to my read and write I have still less code which > is called and it can be disabled. This regmap overhead is always there > chasing pointers. Equally well what you're implementing here is support for something that's typically implemented with an I2C or SPI control interface so you're already going to be quite a way ahead of the norm. This is part of what's confusing me, usually for this application things aren't that bad performance wise even on a massively slower bus. If all you're saying here is that there's some overhead that's fine if a bit surprising, but the way you're talking made it sound like there was some issue that made the API actually unusable. > As I said before: I doubt that you get this regmap access in an one GiB > network driver and in turn remove their trace points to register access. Well, of course for that sort of thing the general trick is not to talk to the hardware at all and do as much as possible with memory that the hardware can DMA - there's actually still non-trivial costs in talking over the buses. > Please don't get me wrong: It is still nice for slow buses and this ADC > driver isn't anything close to high performance like a 1GiB network > driver but it adds a lot of unwanted overhead which I prefer not to > have. OK, but equally well remember that from a subsystem maintainer point of view having things factored out is a win in itself; for example with the MFDs locking on the register I/O has been a persistent issue in the past. signature.asc Description: Digital signature
Re: am335x: TSC & ADC reworking including DT pieces, take 4
On Tue, Jun 11, 2013 at 05:29:22PM +0200, Sebastian Andrzej Siewior wrote: > On 06/11/2013 04:23 PM, Samuel Ortiz wrote: > > Please fix your commit logs, and your subject lines. It should be e.g. > > mfd: input: ti_am335x_adc: Blablabla > > if it's mostly an mfd patch that also touches an input driver. > I usually do "subsystem / driver: short description" but if you want > the ":" as delimiter I can do this. You should always aim to be consistent with the style used by the code you're updating - do a git log and make sure your patches don't have changelogs that are obivously using a different style. signature.asc Description: Digital signature
Re: [PATCH 01/22] mfd/ti_am335x_tscadc: remove regmap
On Tue, Jun 11, 2013 at 04:34:53PM +0200, Sebastian Andrzej Siewior wrote: > >> Therefore this patch removes regmap part of the driver. > > NAK. Using regmap is better than open coding your register accesses, and > > the children not using this API is not a reason for the MFD driver to do > > the same. > There is no advantage over using regmap in the first place. It goes > through a few layers, uses no caching because almost all registers are > volatile and this is a direct bus. In the end it complicates more than > it helps. It does give you tracepoints and debugfs. If it's making things at all complicated we need to look at why that is and figure out how to fix that since it's probably an issue for other users. signature.asc Description: Digital signature
Re: [PATCH 10/53] Input: atmel_mxt_ts - Add memory access interface via sysfs
On Tue, Jun 11, 2013 at 11:39:37AM +0100, Nick Dyer wrote: > Mark Brown wrote: > > I don't think you're using the usual definition of "register map" here. > > You seem to be switching between talking about this object model the > > device has and device registers - perhaps the objects are also registers > > sometimes? > Yes, in Atmel Object Protocol "instances" of "objects" do each have an > assigned register range (they do also correspond to modules of code in the > firmware). OK, so you're talking about the objects which happen to be implemented in terms of the register map here. I think this is confusing to me because you are moving between the abstraction levels. > Essentially, the device is designed (and tested) on the assumption that > touch processing can be going on whilst various parameters are changed in a > live fashion. If poking around in the register map caused it to lock up or > behave oddly then that would be considered a firmware bug. In general it > will fail gracefully - for example if you write a configuration that is > invalid it will just stop and emit a CFGERR message. That's not really it, it's the fact that this is being done with no abstraction - exposing the entire device register map directly to application layer so the application can totally ignore what's going on in the kernel doesn't seem like awesome design. > The absolute worst thing that I can think of is that you can try to beat > the interrupt handler to reading the "message processor" registers, which > would possibly leave touches stuck on screen. But even that operation is > useful in debugging interrupt line issues. Well, there's also the potential issues with standard application layer code either not being able to do things that the hardware supports or getting into a fight with the magic custom stuff. > > Won't the driver end up getting into a fight with the magic userspace > > stuff if the driver has no idea what the magic userspace is doing? How > > would suspend and resume work? > On suspend the driver puts chip into "deep sleep" where touch acquisition > is halted and minimal power consumed. But it will still come out of its > sleep state temporarily to service I2C comms if necessary (although one > particular family requires that I2C retry for this case). So these errors are just part of waking the chip up - in that case shouldn't the driver be waking the chip up automatically? signature.asc Description: Digital signature
Re: [PATCH 10/53] Input: atmel_mxt_ts - Add memory access interface via sysfs
On Fri, Jun 07, 2013 at 05:11:28PM +0100, Nick Dyer wrote: > Mark Brown wrote: > > I thought there was this protocol you're concerned aboout, not raw > > registers? Presenting the actual data in binary form seems sane, how > > one gets to the data is the issue. > OK, so we seem to have gone round in a circle here. Initially I understood > you to say that providing a binary read/write attribute for access to the > configuration data was not acceptable. What was being proposed was just mapping the register map straight into userspace and implementing the entire protocol in userspace. Having the unparsed attributes visible themselves is relatively normal. > >> The chip itself will enforce which registers are read-only (by ignoring > >> writes) or write-only (by returning zeros if read). Encoding all this > >> information in the driver would just make it more brittle in the face of > >> touch controller firmware updates. > > So not only do you interact with the firmware via this protocol but the > > actual hardware register map is unstable > The register map is fixed at firmware compile time. The driver contains > code which parses the object table and figures out the correct register > offsets which are used on the particular chip that it is talking to. > The user space tools that we have written contain an equivalent parser. Is > it the duplication of this code that is your concern? I don't think you're using the usual definition of "register map" here. You seem to be switching between talking about this object model the device has and device registers - perhaps the objects are also registers sometimes? > Are you saying that your concern is that user space shouldn't be able to > directly access these registers, for example to trigger a reset? In which > case, how should user space reset the chip if required? If the application layer can reset the device underneath the driver that doesn't seem awesome; similarly if the application layer is having to worry about the device getting powered off underneath it this doesn't seem great either. > >> It would be possible for the driver to intermediate for some of the > >> registers that it cares about. For example, if we change the screen width > >> then the driver could reinitialise the input device. But I can't see that > >> it makes sense if you are changing several settings in a row for the input > >> device to be reinitialised several times. It is far less buggy to provide a > >> single way of tearing down everything and reinitialising (which could be > >> simply triggered from user space) than to encode all of the dependencies > >> (which is bound to introduce bugs). > > I am having an extremely hard time connecting anything you're talking > > about here with the discussion at hand I'm afraid... > I'm trying to provide the background to this design as you've requested, I > apologise if you find it confusing. You appear to be assuming a great deal of familiarity with the specifics of this device here. Where does all thi stuff about reinitialsing the device come from? What are these dependencies and what does all this have to do with setting parameters? > > Nobody is talking about changing the protocol for interacting with the > > device. The discussion here is about the ABI the driver offers to the > > application layer. > OK. I think that the ABI offered to the application layer should also be > object protocol, implemented over a binary attribute, which is what the > patch under discussion does. Is the problem that I need to provide better > documentation of object protocol? As Greg says you do need to document any new sysfs ABIs you're adding anyway. However if this is some stateful protocol you're implementing then does it really map onto sysfs well, sysfs attributes are normally more just data values? Won't the driver end up getting into a fight with the magic userspace stuff if the driver has no idea what the magic userspace is doing? How would suspend and resume work? signature.asc Description: Digital signature
Re: [PATCH 10/53] Input: atmel_mxt_ts - Add memory access interface via sysfs
On Fri, Jun 07, 2013 at 04:12:25PM +0100, Nick Dyer wrote: > Mark Brown wrote: > > Who said anything about names? I'd assume the userspace stuff is > > eventually talking to the device based on numbers of some kind and I'd > > expect that this would be what ends up in the API. > OK. But if user-space is talking to the device based on register numbers, a > binary attribute seems like the correct way to present that - isn't that > what they're designed for? I thought there was this protocol you're concerned aboout, not raw registers? Presenting the actual data in binary form seems sane, how one gets to the data is the issue. > >> And we wouldn't have won anything, because the user could still write to > >> the register that turns off reporting (for example) by mistake with both > >> methods. > > You'd not have access to the entire device register map which seems like > > a win > Not really. > The chip itself will enforce which registers are read-only (by ignoring > writes) or write-only (by returning zeros if read). Encoding all this > information in the driver would just make it more brittle in the face of > touch controller firmware updates. So not only do you interact with the firmware via this protocol but the actual hardware register map is unstable and there's nothing in the device that the driver itself actually interacts with, all it does is ferry these messages from the application layer to the device? Given the number of other patches here that doesn't seem to be the case... > It would be possible for the driver to intermediate for some of the > registers that it cares about. For example, if we change the screen width > then the driver could reinitialise the input device. But I can't see that > it makes sense if you are changing several settings in a row for the input > device to be reinitialised several times. It is far less buggy to provide a > single way of tearing down everything and reinitialising (which could be > simply triggered from user space) than to encode all of the dependencies > (which is bound to introduce bugs). I am having an extremely hard time connecting anything you're talking about here with the discussion at hand I'm afraid... > > Having lots of configuration and having the parameters change regularly > > doesn't immediately mean that it has to be complex - the requirement is > > very common, touchscreens aren't too remarkable here. The usual thing > > is for the kernel to understand how to transfer data back and forth but > > not the content of the data. > Sure, that's what I'm trying to accomplish. It's just that as far as I can > see it makes more sense to use the established protocol that these chips > have implemented rather than trying to bodge something else over the top or > crowbar it into a different model that will cause abstraction problems. We > have thought about this problem at great length, and discussed it on these > lists, and with other kernel engineers at customers, etc. Nobody is talking about changing the protocol for interacting with the device. The discussion here is about the ABI the driver offers to the application layer. signature.asc Description: Digital signature
Re: [PATCH 10/53] Input: atmel_mxt_ts - Add memory access interface via sysfs
On Thu, Jun 06, 2013 at 05:13:32PM +0100, Nick Dyer wrote: > I am not a legal expert, but I have described what you suggest to people > who are more expert in the NDA terms and they say I cannot implement a > solution which exposes the names of all the parameters. In any case, Who said anything about names? I'd assume the userspace stuff is eventually talking to the device based on numbers of some kind and I'd expect that this would be what ends up in the API. > Without the register names all you really have is the object protocol that > we started with, you would end up accessing > /sys/bus/i2c/drivers/atmel_mxt_ts/1-0020/objectX/registerX > rather than parsing the object table once when your program started, then > performing a read/write to offset X. Well, assuming sysfs makes sense for this. It's not immediately clear to me that it does. > And we wouldn't have won anything, because the user could still write to > the register that turns off reporting (for example) by mistake with both > methods. You'd not have access to the entire device register map which seems like a win and people would be able to integrate sensibly with things like the overall device power management (which might help with whatever is causing you to have to cope with failing I/O) more smoothly. > > So this goes back to what I was saying before about the interface being > > badly designed - if the API you have to present is really this complex > > then you've got a big problem in kernel or out of kernel. > Touch controllers are inherently complex system with a lot of configurable > parameters. The fact that it's complex and changes quickly doesn't make it > badly designed per se. Having lots of configuration and having the parameters change regularly doesn't immediately mean that it has to be complex - the requirement is very common, touchscreens aren't too remarkable here. The usual thing is for the kernel to understand how to transfer data back and forth but not the content of the data. signature.asc Description: Digital signature
Re: [PATCH 10/53] Input: atmel_mxt_ts - Add memory access interface via sysfs
On Thu, Jun 06, 2013 at 12:34:57PM +0100, Nick Dyer wrote: > Mark Brown wrote: > > This is yet another reason for implementing the protocol properly > > instead of trying to bodge around the kernel. It really seems like > > the biggest problem here is the decision to try to bodge the entire > > thing into userspace with no kernel support. > With the interface I am proposing it is handled properly, in the kernel > driver. You're proposing doing this using direct register I/O... > From an Atmel perspective, Linux is just another platform and we want to > use our existing investment in tools and documentation to manage & debug > chips embedded in Linux based devices. So providing a bridge using a Broadly speaking the kernel upstream just don't care about this. > > That sounded like what you were talking about, it's pretty common and is > > sane enough for reads. > The address pointer is shared between reads and writes on maXTouch, but I > guess that's not a huge problem. That's totally normal. signature.asc Description: Digital signature
Re: [PATCH 10/53] Input: atmel_mxt_ts - Add memory access interface via sysfs
On Thu, Jun 06, 2013 at 12:17:56PM +0100, Nick Dyer wrote: > Mark Brown wrote: > > If that's a big problem just put the data table in a section of the > > firmware (or a separate file that gets requested as a firmware). Or > > have the firmware be able to enumerate itself when asked. > That still would breach the NDA, I'm afraid. And there's hundreds of > existing versions of maXTouch chips already out there which don't have the > infrastructure in place to do what you describe. I'm sorry, this just doesn't seem plausible. The data is going to be on the running system and accessed via the kernel - as soon as people start using this back door they're going to be revealing what they're accessing and the information is going to be present in the binary blobs. You're only revealing that the parameters are present, not what they mean, and if it's a big concern then do something like strip down the data file that gets shipped in production to just the controls that are being accessed. Again, the fact that you have shipped this stuff doesn't make much difference here - you really should work with upstream on interfaces like this sooner rather than later otherwise you're going to have to cope with pushback. > If we expose every single parameter as a get/set as you suggest, we haven't > added anything that stops a binary of the wrong version doing something > stupid. We've just added a complex API to the same underlying thing, which > gains nothing. So this goes back to what I was saying before about the interface being badly designed - if the API you have to present is really this complex then you've got a big problem in kernel or out of kernel. > In practice, where there is a risk of the user mucking up their > configuration, the open-source user-space tools that we have released > provide an easy way to back up and restore the configuration in use, and > the kernel driver provides a way to load a known good configuration on > probe. The same configuration formats and tools are used across maXTouch > products on many platforms. So you're saying that this is all nice and consistent. If that's the case then there should be no problem putting at least the core bit that does the actual physical interaction with the device into the kernel. signature.asc Description: Digital signature
Re: [PATCH 10/53] Input: atmel_mxt_ts - Add memory access interface via sysfs
On Thu, Jun 06, 2013 at 12:00:54PM +0100, Nick Dyer wrote: > Mark Brown wrote: > > The retries can just be done further up the stack? All regmap is doing > > with I/O errors is punting them straight back up to the caller so the > > caller can retry just as well using regmap as it can using the raw I/O > > protocol. > It would have to be put into users of the debugfs interface as well. > There's quite tight timing required to make it work properly (see patch > [40/53]). This is yet another reason for implementing the protocol properly instead of trying to bodge around the kernel. It really seems like the biggest problem here is the decision to try to bodge the entire thing into userspace with no kernel support. > > Without seeing the address thing it's hard to comment. > Patch [36/53]. If the T5 message processor is from address 100-110, you can > do a read of 50 bytes starting at address 100, and it will return 10 > messages, but anything in regmap that tries to do bounds checking would get > confused, I think. That's just not going to be supported, sorry. You can implement custom locks and access the device directly where you need to do stuff like that while still using regmap for actual registers though. > Also, we would like to implement address pointer caching. maXTouch allows > us to skip the address part of the i2c transaction if the address pointer > in the chip hasn't changed. This speeds up interrupt handler slightly. But > it requires extra housekeeping at a low level to remember what the address > pointer was on the previous transaction to know whether to send it or not. That sounded like what you were talking about, it's pretty common and is sane enough for reads. signature.asc Description: Digital signature
Re: [PATCH 10/53] Input: atmel_mxt_ts - Add memory access interface via sysfs
On Thu, Jun 06, 2013 at 11:31:57AM +0100, Nick Dyer wrote: > Having to define every parameter in each object (there are thousands) in > the kernel driver would be impractically technically, since it would result > in a huge, and constantly updating API, which would be always out-of-date, > and impossible to support. > Also, I'm afraid it would also be impractical legally, since it would > breach the NDA terms that Atmel require on these parameter definitions. If that's a big problem just put the data table in a section of the firmware (or a separate file that gets requested as a firmware). Or have the firmware be able to enumerate itself when asked. > >> product-specific complexity to user space. Hence exposing the register map > >> and implementing user-space libraries to deal with this kind of > >> customisation. > > This sounds like a bad design decision for Linux, it's just asking for > > fragility if userspace can go randomly poking round the entire register > > map of the device with nothing coordinating with the driver code. > It works very well in practice. This same abstraction is used across > maXTouch products on many platforms to provide tool support. I agree that > its use should be restricted to system programs. It works well so long as people use the supplied binaries in the way that's been proscribed. As soon as people start upgrading kernels, customising things out of your expected flow (because they can or they're on a tight deadline) things will get more fragile. This sort of stuff is really common, the approaches I'm describing are fairly standard solutions. signature.asc Description: Digital signature
Re: [PATCH 10/53] Input: atmel_mxt_ts - Add memory access interface via sysfs
On Thu, Jun 06, 2013 at 11:40:50AM +0100, Nick Dyer wrote: > I am more worried about the address pointer handling and the I2C retries. The retries can just be done further up the stack? All regmap is doing with I/O errors is punting them straight back up to the caller so the caller can retry just as well using regmap as it can using the raw I/O protocol. Without seeing the address thing it's hard to comment. signature.asc Description: Digital signature
Re: [PATCH 10/53] Input: atmel_mxt_ts - Add memory access interface via sysfs
On Wed, Jun 05, 2013 at 10:36:45PM +0100, Nick Dyer wrote: > Dmitry Torokhov wrote: > > What other purposes does it serve? I'd expect you need it during new > > board bringup. > Run-time examples would be adjusting noise suppression or touch suppression > parameters based on something going on in the app layer (eg having > different parameters during unlock screen), or tuning report rates based on > application requirements, ot to inspect debug data if the touch sensor is > faulty. You might say, well we should implement an kernel driver interface > for these requirements, but they will vary hugely between different > products. We are trying to keep the driver as generic as possible and push If this interface varies dramatically between products then that sounds like a badly designed interface. Obviously the way the interface is being used would be likely to vary between products but what you're talking about sounds like parameter get/set stuff which sounds pretty generic to me. What userspace chooses to do with the parameters is of course another story. > product-specific complexity to user space. Hence exposing the register map > and implementing user-space libraries to deal with this kind of customisation. This sounds like a bad design decision for Linux, it's just asking for fragility if userspace can go randomly poking round the entire register map of the device with nothing coordinating with the driver code. If you expose the paramters to userspace wouldn't that address the issue? signature.asc Description: Digital signature
Re: [PATCH 10/53] Input: atmel_mxt_ts - Add memory access interface via sysfs
On Wed, Jun 05, 2013 at 02:07:15PM -0700, Dmitry Torokhov wrote: > On Wed, Jun 05, 2013 at 09:31:39PM +0100, Nick Dyer wrote: > > It's partly path dependence - it was implemented like this because regmap > > wasn't in mainline at the point when I wrote it. Having a dependency on > > regmap would now be a API break complicating support of customers using > > older kernels than mainline. I would also have to update a bunch of > This was never a good argument for introducing an interface into the > kernel. Indeed, and regmap is very easy to backport - it demands little from the rest of the kernel and most of that is bus specific so you can pretty much just copy it into an older kernel. signature.asc Description: Digital signature
Re: [PATCH 19/19 v2] mfd/ti_am335x_tscadc: add private lock/unlock function for regmap read/write
On Wed, May 29, 2013 at 10:46:42AM +0200, Sebastian Andrzej Siewior wrote: > Without this, devm_regmap_init_mmio() creates & uses a simple > spin_lock() and this should be enough. Within the probe function the > registers are read and written in process context. Later they are > accessed from the ISR and lockdep complains because now the lock is > taken suddenly with IRQs enabled. Currently I don't see any other way to > keep lockdep quiet than doing this. This is not a good place to make this change, there's nothing specific to the device here and in actual fact there's already a change in the works from Lars-Peter Clausen converting the spinlock to always use spin_lock_irqsave() so it should be resolved soon. The thread was on lkml in the past few days. signature.asc Description: Digital signature
Re: [PATCH 1/2] Input: touchscreen: ads7846: copy info from pdata to private struct
On Mon, May 06, 2013 at 12:34:23PM +0200, Daniel Mack wrote: > But I can browse my reflog and switch back to the other approach if > that's preferred. The only concern I have is what I already mentioned: > the allocation of function pointers which are definitely unused for DT. I don't particularly care either way; what you say sounds sensible. signature.asc Description: Digital signature
Re: [PATCH 1/2] Input: touchscreen: ads7846: copy info from pdata to private struct
On Sun, May 05, 2013 at 08:24:44PM -0700, Dmitry Torokhov wrote: > On Thu, Apr 25, 2013 at 01:33:52PM +0200, Daniel Mack wrote: > > In preparation for DT bindings, we have to store all runtime information > > inside struct ads7846. Add more variable to struct ads7846 and refactor > > some code so the probe-time supplied pdata is not used from any other > > function than the probe() callback. > I think more common pattern is to allocate platform data structure when > parsing device tree, often with devm_kzalloc() so it is cleaned up after > driver is unbound. Both are used fairly widely. It's very common to do the separate allocation when converting an existing driver to device tree as the code using the platform data is frequently written with lots of pdata-> in it and may potentially have some different behaviour if there's no platform data at all (though that's a bit questionable) so allocating a new struct is pretty natural and makes for a much less invasive patch. When there's no existing platform data code it's probably more common to embed the structure as this saves an allocation and means that the users can assume that there's a platform data struct there which makes them a little simpler. signature.asc Description: Digital signature
Re: [alsa-devel] [PATCH] ASoC: wm8962: Convert to devm_input_allocate_device()
On Mon, Apr 29, 2013 at 09:52:34PM +0300, Leon Romanovsky wrote: > Mark, please take my apologies, I was mislead by the following comment > on the code: No problem, thanks for taking the time to check into this. signature.asc Description: Digital signature
Re: [PATCH 17/25] wm97xx: don't use [delayed_]work_pending()
On Sat, Mar 09, 2013 at 03:53:36PM -0800, Dmitry Torokhov wrote: > On Mon, Dec 24, 2012 at 04:18:27PM +0000, Mark Brown wrote: > > I'm a bit nervous about the fact that currently both the pen down IRQ > > and the coordinate read are pushed through a single workqueue so are > > serialised but after your patch they'll be split into the IRQ thread and > > the workqueue. It *should* be fine but I'd have to sit there and study > > it to convince myself that it's safe. > Mark, > Did yo have a chance to review the patch? > Thanks! Sort of. I'd be much happier keeping them serialised, it's too much work on legacy code to convince myself otherwise. signature.asc Description: Digital signature
Re: [PATCH 3/4] Input: wm9712: Fix wrong pen up readings
On Fri, Mar 08, 2013 at 05:15:08PM +0100, Markus Pargmann wrote: > Often a reading can be wrong. This patch assures that this is really a > pen up event and not a false reading. Hrm, it feels like we should wait for the device to tell us another reading is ready but on the other hand I can see the sort of issue that might mean this is needed and if it improves performance in your testing: Acked-by: Mark Brown signature.asc Description: Digital signature
Re: [PATCH 4/4] Input: wm9712: Fix dev_dbg newlines
On Fri, Mar 08, 2013 at 05:15:09PM +0100, Markus Pargmann wrote: > dev_dbg should end with a new line. Acked-by: Mark Brown signature.asc Description: Digital signature
Re: [PATCH 2/4] Input: wm9712: Fix returncode for wrong sample
On Fri, Mar 08, 2013 at 05:15:07PM +0100, Markus Pargmann wrote: > Instead of interpreting a wrong measurement as pen up, we should try to read > again. Acked-by: Mark Brown signature.asc Description: Digital signature
Re: [PATCH 1/4] Input: wm97xx: Drop out of range inputs
On Fri, Mar 08, 2013 at 05:15:06PM +0100, Markus Pargmann wrote:
> + if (
> + abs_x[0] > (data.x & 0xfff)
> + || abs_x[1] < (data.x & 0xfff)
> + || abs_y[0] > (data.y & 0xfff)
> + || abs_y[1] < (data.y & 0xfff)) {
> + dev_dbg(wm->dev, "Measurement out of range, dropping
> it\n");
> + rc = RC_AGAIN;
> + goto out;
The change is good but not a fan of the coding style here. Otherwise
Acked-by: Mark Brown
signature.asc
Description: Digital signature
Re: [PATCH 8/8] Input: atmel-wm97xx: Use module_platform_driver_probe macro
On Tue, Mar 05, 2013 at 09:17:27AM +0530, Sachin Kamat wrote: > On 5 March 2013 09:06, Mark Brown wrote: > >I know the existing code does this, it's not > > clear to me that that's a good idea either. > However, if your question is why return platform_driver_probe() at all > instead of platform_driver_register() in the first place, then I am > sorry I do not have much idea about it. Yes, I'm questioning if this is the right cleanup to do. signature.asc Description: Digital signature
Re: [PATCH 8/8] Input: atmel-wm97xx: Use module_platform_driver_probe macro
On Tue, Mar 05, 2013 at 08:53:47AM +0530, Sachin Kamat wrote: > module_platform_driver_probe() eliminates the boilerplate and simplifies > the code. Is there really any great reason to use this as opposed to module_platform_driver()? I know the existing code does this, it's not clear to me that that's a good idea either. signature.asc Description: Digital signature
[PATCH] Input: mms114 - Fix regulator enable and disable paths
When it uses regulators the mms114 driver checks to see if it managed to
acquire regulators and ignores errors. This is not the intended usage and
not great style in general.
Since the driver already refuses to probe if it fails to allocate the
regulators simply make the enable and disable calls unconditional and
add appropriate error handling, including adding cleanup of the
regulators if setup_reg() fails.
Signed-off-by: Mark Brown
---
drivers/input/touchscreen/mms114.c | 34 +-
1 file changed, 25 insertions(+), 9 deletions(-)
diff --git a/drivers/input/touchscreen/mms114.c
b/drivers/input/touchscreen/mms114.c
index 4a29ddf..662e34b 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -314,15 +314,27 @@ static int mms114_start(struct mms114_data *data)
struct i2c_client *client = data->client;
int error;
- if (data->core_reg)
- regulator_enable(data->core_reg);
- if (data->io_reg)
- regulator_enable(data->io_reg);
+ error = regulator_enable(data->core_reg);
+ if (error != 0) {
+ dev_err(&client->dev, "Failed to enable avdd: %d\n", error);
+ return error;
+ }
+
+ error = regulator_enable(data->io_reg);
+ if (error != 0) {
+ dev_err(&client->dev, "Failed to enable vdd: %d\n", error);
+ regulator_disable(data->core_reg);
+ return error;
+ }
+
mdelay(MMS114_POWERON_DELAY);
error = mms114_setup_regs(data);
- if (error < 0)
+ if (error < 0) {
+ regulator_disable(data->io_reg);
+ regulator_disable(data->core_reg);
return error;
+ }
if (data->pdata->cfg_pin)
data->pdata->cfg_pin(true);
@@ -335,16 +347,20 @@ static int mms114_start(struct mms114_data *data)
static void mms114_stop(struct mms114_data *data)
{
struct i2c_client *client = data->client;
+ int error;
disable_irq(client->irq);
if (data->pdata->cfg_pin)
data->pdata->cfg_pin(false);
- if (data->io_reg)
- regulator_disable(data->io_reg);
- if (data->core_reg)
- regulator_disable(data->core_reg);
+ error = regulator_disable(data->io_reg);
+ if (error != 0)
+ dev_warn(&client->dev, "Failed to disable vdd: %d\n", error);
+
+ error = regulator_disable(data->core_reg);
+ if (error != 0)
+ dev_warn(&client->dev, "Failed to disable avdd: %d\n", error);
}
static int mms114_input_open(struct input_dev *dev)
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Input: mms114 - Fix regulator enable and disable paths
On Sat, Mar 02, 2013 at 05:11:00PM +0900, Joonyoung Shim wrote: > On 03/02/2013 04:00 PM, Mark Brown wrote: > > mdelay(MMS114_POWERON_DELAY); > > error = mms114_setup_regs(data); > If error, we will have to disable regulators here. That's a preexisting error, of course... signature.asc Description: Digital signature
[PATCH] Input: mms114 - Fix regulator enable and disable paths
When it uses regulators the mms114 driver checks to see if it managed to
acquire regulators and ignores errors. This is not the intended usage and
not great style in general.
Since the driver already refuses to probe if it fails to allocate the
regulators simply make the enable and disable calls unconditional and
add appropriate error handling.
Signed-off-by: Mark Brown
---
drivers/input/touchscreen/mms114.c | 31 +++
1 file changed, 23 insertions(+), 8 deletions(-)
diff --git a/drivers/input/touchscreen/mms114.c
b/drivers/input/touchscreen/mms114.c
index 4a29ddf..bb95c89 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -314,10 +314,21 @@ static int mms114_start(struct mms114_data *data)
struct i2c_client *client = data->client;
int error;
- if (data->core_reg)
- regulator_enable(data->core_reg);
- if (data->io_reg)
- regulator_enable(data->io_reg);
+ error = regulator_enable(data->core_reg);
+ if (error != 0) {
+ dev_err(&client->dev, "Failed to enable avdd: %d\n",
+ error);
+ return error;
+ }
+
+ error = regulator_enable(data->io_reg);
+ if (error != 0) {
+ dev_err(&client->dev, "Failed to enable vdd: %d\n",
+ error);
+ regulator_disable(data->core_reg);
+ return error;
+ }
+
mdelay(MMS114_POWERON_DELAY);
error = mms114_setup_regs(data);
@@ -335,16 +346,20 @@ static int mms114_start(struct mms114_data *data)
static void mms114_stop(struct mms114_data *data)
{
struct i2c_client *client = data->client;
+ int error;
disable_irq(client->irq);
if (data->pdata->cfg_pin)
data->pdata->cfg_pin(false);
- if (data->io_reg)
- regulator_disable(data->io_reg);
- if (data->core_reg)
- regulator_disable(data->core_reg);
+ error = regulator_disable(data->io_reg);
+ if (error != 0)
+ dev_warn(&client->dev, "Failed to disable vdd: %d\n", error);
+
+ error = regulator_disable(data->core_reg);
+ if (error != 0)
+ dev_warn(&client->dev, "Failed to disable avdd: %d\n", error);
}
static int mms114_input_open(struct input_dev *dev)
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Input: ads7864 - Check return value of regulator enable
At least print a warning if we can't power the device up.
Signed-off-by: Mark Brown
---
drivers/input/touchscreen/ads7846.c |7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/input/touchscreen/ads7846.c
b/drivers/input/touchscreen/ads7846.c
index 4f702b3..e0c8b7a 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -236,7 +236,12 @@ static void __ads7846_disable(struct ads7846 *ts)
/* Must be called with ts->lock held */
static void __ads7846_enable(struct ads7846 *ts)
{
- regulator_enable(ts->reg);
+ int ret;
+
+ ret = regulator_enable(ts->reg);
+ if (ret != 0)
+ dev_err(&ts->spi->dev, "Failed to enable supply: %d\n", ret);
+
ads7846_restart(ts);
}
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 17/25] wm97xx: don't use [delayed_]work_pending()
On Sun, Dec 23, 2012 at 01:54:50AM -0800, Dmitry Torokhov wrote: > This is not 100% equivalent transformation as now we schedule first and > disable IRQ later... Anyway, I think the driver shoudl be converted to > threaded IRQ instead. Mark, does the patch below make any sense to you? I'm a bit nervous about the fact that currently both the pen down IRQ and the coordinate read are pushed through a single workqueue so are serialised but after your patch they'll be split into the IRQ thread and the workqueue. It *should* be fine but I'd have to sit there and study it to convince myself that it's safe. signature.asc Description: Digital signature
[PATCH] Input: wm831x-on - Convert to devm_input_allocate_device()
Signed-off-by: Mark Brown
---
drivers/input/misc/wm831x-on.c |4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/input/misc/wm831x-on.c b/drivers/input/misc/wm831x-on.c
index 558767d..caa2c406 100644
--- a/drivers/input/misc/wm831x-on.c
+++ b/drivers/input/misc/wm831x-on.c
@@ -86,7 +86,7 @@ static int wm831x_on_probe(struct platform_device *pdev)
wm831x_on->wm831x = wm831x;
INIT_DELAYED_WORK(&wm831x_on->work, wm831x_poll_on);
- wm831x_on->dev = input_allocate_device();
+ wm831x_on->dev = devm_input_allocate_device(&pdev->dev);
if (!wm831x_on->dev) {
dev_err(&pdev->dev, "Can't allocate input dev\n");
ret = -ENOMEM;
@@ -119,7 +119,6 @@ static int wm831x_on_probe(struct platform_device *pdev)
err_irq:
free_irq(irq, wm831x_on);
err_input_dev:
- input_free_device(wm831x_on->dev);
err:
return ret;
}
@@ -131,7 +130,6 @@ static int wm831x_on_remove(struct platform_device *pdev)
free_irq(irq, wm831x_on);
cancel_delayed_work_sync(&wm831x_on->work);
- input_unregister_device(wm831x_on->dev);
return 0;
}
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Input: wm831x-ts: Convert to devm_input_allocate_device()
Signed-off-by: Mark Brown
---
drivers/input/touchscreen/wm831x-ts.c |4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/input/touchscreen/wm831x-ts.c
b/drivers/input/touchscreen/wm831x-ts.c
index f88fab5..6be2eb6 100644
--- a/drivers/input/touchscreen/wm831x-ts.c
+++ b/drivers/input/touchscreen/wm831x-ts.c
@@ -247,7 +247,7 @@ static int wm831x_ts_probe(struct platform_device *pdev)
wm831x_ts = devm_kzalloc(&pdev->dev, sizeof(struct wm831x_ts),
GFP_KERNEL);
- input_dev = input_allocate_device();
+ input_dev = devm_input_allocate_device(&pdev->dev);
if (!wm831x_ts || !input_dev) {
error = -ENOMEM;
goto err_alloc;
@@ -376,7 +376,6 @@ err_pd_irq:
err_data_irq:
free_irq(wm831x_ts->data_irq, wm831x_ts);
err_alloc:
- input_free_device(input_dev);
return error;
}
@@ -387,7 +386,6 @@ static int wm831x_ts_remove(struct platform_device *pdev)
free_irq(wm831x_ts->pd_irq, wm831x_ts);
free_irq(wm831x_ts->data_irq, wm831x_ts);
- input_unregister_device(wm831x_ts->input_dev);
return 0;
}
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
