Re: [PATCH 1/7] pwm: Add pwm core driver

2010-09-28 Thread Lars-Peter Clausen
Arun MURTHY wrote:
 Shouldn't PWM_DEVICES select HAVE_PWM?

 No not required, the entire concept is to remove HAVE_PWM and use
 PWM_CORE.

 Well in patch 4 you say that PWM_CORE is currently limited to ARM.
 Furthermore you
 change the pwm-backlight and pwm-led Kconfig entries to depend on
 HAVE_PWM ||
 PWM_CORE. Adding a select HAVE_PWM here would make those changes
 unnecessary.
 HAVE_PWM is retained just because the mips pwm driver is not aligned with the 
 pwm core driver.
 On mips pwm driver aligning to the pwm core driver HAVE_PWM will be replaced 
 by PWM_CORE.
 
 HAVE_PWM should be set, when pwm_* functions are available. When your
 pwm-core driver
 is selected they are available.
 On applying this patch set pwm_* function will be exported in pwm_core driver 
 and in mips pwm driver.
 Since mips pwm driver is not aligned with the pwm core, HAVE_PWM is retained 
 and removed in places where pwm drivers register to pwm core driver.

pwm_{enable,disable,request,free} are the interface of the pwm api. Your 
pwm-core is
one implementation of that interface. A somewhat special though, because it 
tries to
be a generic implementation.
There are still other implementations though. For example right now the mips 
jz4740 one.
In my opinion HAVE_PWM should be defined if there is a implementation for the 
pwm
interface is available.
I know that your plan is that in the end pwm-core is the only implementation of 
the
pwm interface.

But right now it is not and on the other hand some SoC implementors might 
choose that
they want to provide their own minimal pwm interface implementation.
Furthermore this would allow you to start with pwm-core for one SoC which you 
have on
your desk and where you can properly test things and keep the patches clean from
clutter changing all the different archs.
Once pwm-core is in a proper shape you or other people can start porting all the
different SoC support code to pwm-core.

Similar behavior is for example true for the gpio api. There is gpiolib which 
is the
generic implementation which allows having gpio chips outside of the chip. On 
the
other hand there are still archs which choose to have their own gpio api 
implementation.

 
 pwm_device will be passed to each and every pwm driver that are
 registered as client with pwm core.
 The list consists of the registered pwm drivers and is to be handled
 by pwm core.
 Why should each and every pwm driver get to know about the entire pwm
 driver list?
 Declare the list field to be private, by saying that it should only be
 touched by the
 core. Right now you allocate a rather small additional structure for
 each registered
 device. This could be easily be avoided by embedding the list field
 into the
 pwm_device struct.
 
 The one that is being allocated in register is the pwm_device and this has 
 to. Because each pwm driver will have their own data related to ops, pwm_id.
 Also note that there exists an element data that points to the pwm device 
 specific information. Hence this allocation is required.
 
 + }
 + pwm-pwm_dev = pwm_dev;
 + list_add_tail(pwm-list, di-list);
 + up_write(pwm_list_lock);
 +
 I guess you only need to lock the list when accessing the list and
 adding the new
 pwm_dev.
 Oops, thanks for pointing out, will implement this in the v2 patch.
 Coming back to this, I guess the locking has to be done while traversing the 
 list also, as my present pointer in the list my get over written by the time 
 I add an element to list. Please let me know if I am wrong.
 
 +struct pwm_ops {
 + int (*pwm_config)(struct pwm_device *pwm, int duty_ns, int
 period_ns);
 + int (*pwm_enable)(struct pwm_device *pwm);
 + int (*pwm_disable)(struct pwm_device *pwm);
 + char *name;
 +};
 +
 Shouldn't name be part of the pwm_device? That would allow the ops
 to
 be shared
 between different devices.
 Good catch, the reason being that 2 or more devices can share the
 same ops and get registered to pwm core.
 But the catch lies while identifying the pwm device while the clients
 are requesting for.
 The pwm backlight will request the pwm driver by name. This is
 parameter that distinguishes among different pwm devices irrespective
 of same ops or not.
 Yes. And thats why it should go into the pwm_device struct itself.

 If an additional ops struct is allocated for each device anyway we
 would be better of
 embedding it directly into the device struct instead of just holding a
 pointer to it.
 Yes ops structure will be allocated. But how can we get access to the ops 
 structure of another driver?
 And moreover two pwm driver sharing same ops ideally means a single pwm 
 module. If not everything atleast the pwm registers of two different modules 
 changes. So this scenario can never occur.
 
  #endif /* __LINUX_PWM_H */
 It might be also a good idea to add a device class for pwm devices.
 Sure, but can you please explain with an example the use case.

 Well, for one it helps to keep data structured.
 And there would be functions to 

Re: [alsa-devel] [PATCH] ASoC: omap: convert per-board modules to platform drivers

2011-09-08 Thread Lars-Peter Clausen
On 09/08/2011 05:05 PM, Mans Rullgard wrote:
 This converts the per-board modules to platform drivers for a
 device created by in main platform setup.  These drivers call
 snd_soc_register_card() directly instead of going via a soc-audio
 device and the corresponding driver in soc-core.

 diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
 index 5b8ca68..7cb93d9 100644
 --- a/arch/arm/mach-omap2/devices.c
 +++ b/arch/arm/mach-omap2/devices.c
 @@ -299,6 +299,11 @@ static struct platform_device omap_pcm = {
   .id = -1,
  };
  
 +static struct platform_device omap_soc_audio = {
 + .name   = omap-soc-audio,
 + .id = -1,
 +};
 +
  /*
   * OMAP2420 has 2 McBSP ports
   * OMAP2430 has 5 McBSP ports
 @@ -323,6 +328,7 @@ static void omap_init_audio(void)
   platform_device_register(omap_mcbsp5);
  
   platform_device_register(omap_pcm);
 + platform_device_register(omap_soc_audio);
  }
  
  #else
 diff --git a/sound/soc/omap/am3517evm.c b/sound/soc/omap/am3517evm.c
 index 73dde4a..fcd18af 100644
 --- a/sound/soc/omap/am3517evm.c
 +++ b/sound/soc/omap/am3517evm.c
 @@ -151,45 +151,60 @@ static struct snd_soc_card snd_soc_am3517evm = {
   .num_links = 1,
  };
  
 [...]
 +static struct platform_driver am3517evm_driver = {
 + .driver = {
 + .name = omap-soc-audio,
 + .owner = THIS_MODULE,
 + },
  
 - return ret;
 + .probe = am3517evm_soc_probe,
 + .remove = __devexit_p(am3517evm_soc_remove),
 +};
 +[...]
 +
 +static struct platform_driver igep2_driver = {
 + .driver = {
 + .name = omap-soc-audio,
 + .owner = THIS_MODULE,
 + },
 +
 + .probe = igep2_soc_probe,
 + .remove = __devexit_p(igep2_soc_remove),
 +};
 [...]
  
 +static struct platform_driver n810_driver = {
 + .driver = {
 + .name = omap-soc-audio,
 + .owner = THIS_MODULE,
 + },
 +
 + .probe = n810_soc_probe,
 + .remove = __devexit_p(n810_soc_remove),
 +};
 [...]

This isn't really any better then using the soc-core device, since all your
drivers are still named the same. udev still wouldn't know which one to load.
Use different device driver names for different drivers.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 09/11] video: Remove redundant spi driver bus initialization

2011-11-24 Thread Lars-Peter Clausen
In ancient times it was necessary to manually initialize the bus field of an
spi_driver to spi_bus_type. These days this is done in spi_driver_register(),
so we can drop the manual assignment.

The patch was generated using the following coccinelle semantic patch:
// smpl
@@
identifier _driver;
@@
struct spi_driver _driver = {
.driver = {
-   .bus = spi_bus_type,
},
};
// /smpl

Signed-off-by: Lars-Peter Clausen l...@metafoo.de
Cc: Tomi Valkeinen tomi.valkei...@ti.com
Cc: Florian Tobias Schandinat florianschandi...@gmx.de
Cc: linux-fb...@vger.kernel.org
Cc: linux-omap@vger.kernel.org
---
 drivers/video/omap/lcd_mipid.c |1 -
 drivers/video/omap2/displays/panel-acx565akm.c |1 -
 drivers/video/omap2/displays/panel-n8x0.c  |1 -
 .../omap2/displays/panel-nec-nl8048hl11-01b.c  |1 -
 .../video/omap2/displays/panel-tpo-td043mtea1.c|1 -
 5 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/drivers/video/omap/lcd_mipid.c b/drivers/video/omap/lcd_mipid.c
index eb381db..8d546dd 100644
--- a/drivers/video/omap/lcd_mipid.c
+++ b/drivers/video/omap/lcd_mipid.c
@@ -603,7 +603,6 @@ static int mipid_spi_remove(struct spi_device *spi)
 static struct spi_driver mipid_spi_driver = {
.driver = {
.name   = MIPID_MODULE_NAME,
-   .bus= spi_bus_type,
.owner  = THIS_MODULE,
},
.probe  = mipid_spi_probe,
diff --git a/drivers/video/omap2/displays/panel-acx565akm.c 
b/drivers/video/omap2/displays/panel-acx565akm.c
index dbd59b8..51a87e1 100644
--- a/drivers/video/omap2/displays/panel-acx565akm.c
+++ b/drivers/video/omap2/displays/panel-acx565akm.c
@@ -803,7 +803,6 @@ static int acx565akm_spi_remove(struct spi_device *spi)
 static struct spi_driver acx565akm_spi_driver = {
.driver = {
.name   = acx565akm,
-   .bus= spi_bus_type,
.owner  = THIS_MODULE,
},
.probe  = acx565akm_spi_probe,
diff --git a/drivers/video/omap2/displays/panel-n8x0.c 
b/drivers/video/omap2/displays/panel-n8x0.c
index 150e8ba..dc9408d 100644
--- a/drivers/video/omap2/displays/panel-n8x0.c
+++ b/drivers/video/omap2/displays/panel-n8x0.c
@@ -708,7 +708,6 @@ static int mipid_spi_remove(struct spi_device *spi)
 static struct spi_driver mipid_spi_driver = {
.driver = {
.name   = lcd_mipid,
-   .bus= spi_bus_type,
.owner  = THIS_MODULE,
},
.probe  = mipid_spi_probe,
diff --git a/drivers/video/omap2/displays/panel-nec-nl8048hl11-01b.c 
b/drivers/video/omap2/displays/panel-nec-nl8048hl11-01b.c
index 2ba9d0c..8365e77 100644
--- a/drivers/video/omap2/displays/panel-nec-nl8048hl11-01b.c
+++ b/drivers/video/omap2/displays/panel-nec-nl8048hl11-01b.c
@@ -303,7 +303,6 @@ static struct spi_driver nec_8048_spi_driver = {
.resume = nec_8048_spi_resume,
.driver = {
.name   = nec_8048_spi,
-   .bus= spi_bus_type,
.owner  = THIS_MODULE,
},
 };
diff --git a/drivers/video/omap2/displays/panel-tpo-td043mtea1.c 
b/drivers/video/omap2/displays/panel-tpo-td043mtea1.c
index 2462b9e..e6649aa 100644
--- a/drivers/video/omap2/displays/panel-tpo-td043mtea1.c
+++ b/drivers/video/omap2/displays/panel-tpo-td043mtea1.c
@@ -512,7 +512,6 @@ static int __devexit tpo_td043_spi_remove(struct spi_device 
*spi)
 static struct spi_driver tpo_td043_spi_driver = {
.driver = {
.name   = tpo_td043mtea1_panel_spi,
-   .bus= spi_bus_type,
.owner  = THIS_MODULE,
},
.probe  = tpo_td043_spi_probe,
-- 
1.7.7.1


--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] drivers/rtc/rtc-twl.c: fix threaded IRQ to use IRQF_ONESHOT

2012-07-10 Thread Lars-Peter Clausen
On 07/10/2012 12:15 AM, Andrew Morton wrote:
 On Fri,  6 Jul 2012 09:33:54 -0700
 Kevin Hilman khil...@ti.com wrote:
 
 Requesting a threaded interrupt without a primary handler and without
 IRQF_ONESHOT is dangerous, and after commit 1c6c6952 (genirq: Reject
 bogus threaded irq requests), these requests are rejected.  This
 causes -probe() to fail, and the RTC driver not to be availble.

 To fix, add IRQF_ONESHOT to the IRQ flags.

 Tested on OMAP3730/OveroSTORM and OMAP4430/Panda board using rtcwake
 to wake from system suspend multiple times.

 Signed-off-by: Kevin Hilman khil...@ti.com
 ---
 Resending to broader audience and including Andrew.  Since, I understand
 that drivers/rtc is somewhat orphaned, Andrew, can you queue this fix for
 v3.5.  Thanks.

  drivers/rtc/rtc-twl.c |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/drivers/rtc/rtc-twl.c b/drivers/rtc/rtc-twl.c
 index 258abea..c5d06fe 100644
 --- a/drivers/rtc/rtc-twl.c
 +++ b/drivers/rtc/rtc-twl.c
 @@ -510,7 +510,7 @@ static int __devinit twl_rtc_probe(struct 
 platform_device *pdev)
  }
  
  ret = request_threaded_irq(irq, NULL, twl_rtc_interrupt,
 -   IRQF_TRIGGER_RISING,
 +   IRQF_TRIGGER_RISING | IRQF_ONESHOT,
 dev_name(rtc-dev), rtc);
  if (ret  0) {
  dev_err(pdev-dev, IRQ is not free.\n);
 
 OK, this is the second such patch I've seen and it's time to wonder if
 we should get grumpy at tglx.  afacit 1c6c6952 broke the following
 drivers:

If you want to be grumpy, be grumpy at Linus. Thomas actually posted a patch
which just emits a warning, instead of failing to request the IRQ, but Linus
didn't want to take it. The whole thread is here
https://lkml.org/lkml/2012/6/12/151

 
 
 sound/soc/codecs/wm8994.c
 sound/soc/codecs/max98095.c
 sound/soc/codecs/twl6040.c
 drivers/usb/otg/ab8500-usb.c
 drivers/usb/otg/twl4030-usb.c
 drivers/gpio/gpio-sx150x.c
 drivers/gpio/gpio-ab8500.c
 drivers/mfd/ab8500-gpadc.c
 drivers/mfd/ti-ssp.c
 drivers/mfd/twl4030-madc.c
 drivers/mfd/htc-i2cpld.c
 drivers/mfd/wm831x-auxadc.c
 drivers/mfd/twl6040-core.c
 drivers/mfd/wm8350-core.c
 drivers/extcon/extcon-max8997.c
 drivers/mmc/host/of_mmc_spi.c
 drivers/mmc/core/cd-gpio.c
 drivers/net/can/mcp251x.c
 drivers/nfc/pn544_hci.c
 drivers/nfc/pn544.c
 drivers/power/ab8500_btemp.c
 drivers/power/twl4030_charger.c
 drivers/power/lp8727_charger.c
 drivers/power/smb347-charger.c
 drivers/power/max17042_battery.c
 drivers/power/wm831x_power.c
 drivers/power/ab8500_fg.c
 drivers/power/max8903_charger.c
 drivers/power/ab8500_charger.c
 drivers/regulator/wm831x-isink.c
 drivers/regulator/wm831x-ldo.c
 drivers/regulator/wm831x-dcdc.c
 drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c
 drivers/staging/iio/adc/adt7310.c
 drivers/staging/iio/adc/adt7410.c
 drivers/staging/iio/adc/ad7816.c
 drivers/staging/iio/cdc/ad7150.c
 drivers/staging/iio/accel/sca3000_core.c
 drivers/staging/cptm1217/clearpad_tm1217.c
 drivers/input/keyboard/tc3589x-keypad.c
 drivers/input/keyboard/twl4030_keypad.c
 drivers/input/misc/twl4030-pwrbutton.c
 drivers/input/misc/twl6040-vibra.c
 drivers/input/misc/wm831x-on.c
 drivers/media/radio/si470x/radio-si470x-i2c.c
 drivers/base/regmap/regmap-irq.c
 drivers/rtc/rtc-wm831x.c
 drivers/rtc/rtc-twl.c
 drivers/rtc/rtc-ab8500.c
 drivers/rtc/rtc-max8998.c
 drivers/rtc/rtc-isl1208.c
 drivers/platform/x86/intel_mid_powerbtn.c
 include/linux/mfd/wm8994/core.h
 include/linux/mfd/wm8350/core.h
 
 what am I missing here?
 

Most of these drivers are not broken (anymore), the majority drivers which
were broken by Thomas's patch have already been fixed. As for your list:
I've already sent out a fix for all of the staging/iio/ drivers a couple of
days ago. And drivers which use nested interrupts are not affected by this
problem. In your list these are basically all drivers with wm*, twl* and
ab8500 in their names. But still leaves a handful of broken driver which
should be fixed.

- Lars
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] PWM: Add support for configuring polarity of PWM

2012-07-23 Thread Lars-Peter Clausen
On 07/23/2012 10:30 AM, Thierry Reding wrote:
 On Wed, Jul 18, 2012 at 06:24:13PM +0530, Philip, Avinash wrote:
[...]
 diff --git a/include/linux/pwm.h b/include/linux/pwm.h
 index 21d076c..2e4e960 100644
 --- a/include/linux/pwm.h
 +++ b/include/linux/pwm.h
 @@ -21,6 +21,16 @@ void pwm_free(struct pwm_device *pwm);
   */
  int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns);
  
 +enum {
 +PWM_POLARITY_NORMAL,/* ON period depends on duty_ns */
 +PWM_POLARITY_INVERSE,   /* OFF period depends on duty_ns */
 +};
 
 You should name this enumeration so that it can actually be used as a
 type (enum pwm_polarity). Also you can drop the comments because they
 only apply to the specific use-case of simulating duty-cycle inversion

I think we should make it very explicit what normal polarity and inverse
polarity is. There are certain applications where it is important. E.g. one
such application would be using it in the IIO framework to generate a trigger
pulse to synchronize devices. If we do not specify how each of these modes
should behave drivers may interpret and implement them differently.

I'd vote for the following definitions:
PWM_POLARITY_NORMAL: A high signal for the duration of duty_ns, followed by a
low signal for the duration of (period_ns - duty_ns).
PWM_POLARITY_INVERSE: A low signal for the duration duty_ns, followed by a high
signal for the duration of (period_ns - duty_ns).

Maybe even rename them to PWM_POLARITY_ACTIVE_HIGH and PWM_POLARITY_ACTIVE_LOW
since it is a bit more explicit on how the waveform should look like. NORMAL
and INVERSE sort of depend on what you consider to be normal.

- Lars
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] PWM: Add support for configuring polarity of PWM

2012-07-24 Thread Lars-Peter Clausen
On 07/24/2012 08:51 AM, Thierry Reding wrote:
 
 How about the following?
 
 /**
  * enum pwm_polarity - polarity of a PWM signal
  * @PWM_POLARITY_NORMAL: a high signal for the duration of the duty-
  *   cycle, followed by a low signal for the remainder of the pulse
  *   period
  * @PWM_POLARITY_INVERSED: a low signal for the duration of the duty-
  *   cycle, followed by a high signal for the remainder of the pulse
  *   period
  */
 enum pwm_polarity {
   PWM_POLARITY_NORMAL,
   PWM_POLARITY_INVERSED,
 };
 

Looks fine :)

Thanks,
- Lars
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [alsa-devel] [RFC 1/3] ASoC: dmaengine: Don't use runtime private data for dmaengine data

2012-09-03 Thread Lars-Peter Clausen
On 09/03/2012 06:59 PM, Liam Girdwood wrote:
 Use a dedicated member to store dmaengine data so that drivers can
 use private data for their own purposes.
 

The idea was that we'll eventually get to a point where we won't need private
data for the drivers using the generic dmaengine code. But for the transitional
period there is snd_dmaengine_pcm_{set,get}_data which allows to attach driver
private data to the dmaengine pcm. For an example see how the other users of
dmaengine pcm handle this.

 Signed-off-by: Liam Girdwood l...@ti.com
 Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
 ---
  include/sound/pcm.h   |2 ++
  sound/soc/soc-dmaengine-pcm.c |2 +-
  2 files changed, 3 insertions(+), 1 deletions(-)
 
 diff --git a/include/sound/pcm.h b/include/sound/pcm.h
 index cdca2ab..f9e4909 100644
 --- a/include/sound/pcm.h
 +++ b/include/sound/pcm.h
 @@ -269,6 +269,7 @@ struct snd_pcm_hw_constraint_list {
  };
  
  struct snd_pcm_hwptr_log;
 +struct dmaengine_pcm_runtime_data;
  
  struct snd_pcm_runtime {
   /* -- Status -- */
 @@ -345,6 +346,7 @@ struct snd_pcm_runtime {
   unsigned char *dma_area;/* DMA area */
   dma_addr_t dma_addr;/* physical bus address (not accessible 
 from main CPU) */
   size_t dma_bytes;   /* size of DMA area */
 + struct dmaengine_pcm_runtime_data *dmaengine_data;
  
   struct snd_dma_buffer *dma_buffer_p;/* allocated buffer */
  
 diff --git a/sound/soc/soc-dmaengine-pcm.c b/sound/soc/soc-dmaengine-pcm.c
 index 5df529e..27fa5ad 100644
 --- a/sound/soc/soc-dmaengine-pcm.c
 +++ b/sound/soc/soc-dmaengine-pcm.c
 @@ -40,7 +40,7 @@ struct dmaengine_pcm_runtime_data {
  static inline struct dmaengine_pcm_runtime_data *substream_to_prtd(
   const struct snd_pcm_substream *substream)
  {
 - return substream-runtime-private_data;
 + return substream-runtime-dmaengine_data;
  }
  
  /**

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [alsa-devel] [RFC 1/3] ASoC: dmaengine: Don't use runtime private data for dmaengine data

2012-09-03 Thread Lars-Peter Clausen
On 09/03/2012 10:43 PM, Russell King - ARM Linux wrote:
 On Mon, Sep 03, 2012 at 10:25:49PM +0200, Lars-Peter Clausen wrote:
 On 09/03/2012 06:59 PM, Liam Girdwood wrote:
 Use a dedicated member to store dmaengine data so that drivers can
 use private data for their own purposes.


 The idea was that we'll eventually get to a point where we won't need private
 data for the drivers using the generic dmaengine code. But for the 
 transitional
 period there is snd_dmaengine_pcm_{set,get}_data which allows to attach 
 driver
 private data to the dmaengine pcm. For an example see how the other users of
 dmaengine pcm handle this.
 
 That's fine if you are writing new drivers from scatch, or know the
 driver you're converting inside-out.  Neither applies here (I've
 struggled to do anything with the OMAP audio stuff for many many
 reasons.)
 
 I rather wish that people who did know the OMAP ASoC driver had stepped
 up to this conversion, but alas they haven't.
 
 In any case, if you want people to use the this soc-dmaengine helper
 then you have to make the conversion to it simple, and requiring
 everyone to totally restructure their drivers to use it does not make
 that process simple.
 
 What you have here is the result of several transformations to the
 driver, which would _not_ have been possible without this first patch
 from Liam.

Ok, it might have been helpful in the conversion process, but for the final
patch it would be nice if you could replace

struct snd_pcm_runtime *runtime = substream-runtime;
struct omap_runtime_data *prtd = runtime-private_data;
struct omap_pcm_dma_data *dma_data = prtd-dma_data;
with
struct omap_pcm_dma_data *dma_data = snd_dmaengine_pcm_get_data(substream);

and in the hwparams callback use

snd_dmaengine_pcm_set_data(substream, dma_data);

and then drop patch 1 and 2 from the series.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] Don't clobber access methods when !regmap

2012-09-05 Thread Lars-Peter Clausen
On 09/06/2012 03:45 PM, Pantelis Antoniou wrote:
 A snd-soc driver that doesn't support regmap blow up horribly
 when you assume that regmap is available. Fix it by marking
 the driver as not supporting regmap  not clobbering the codec
 access methods.
 
 This is immediately noticeable on the beagleboard where we crash,
 since we might have REGMAP enabled, but it doesn't mean that the
 omap driver uses it.
 
 Signed-off-by: Pantelis Antoniou pa...@antoniou-consulting.com

But calling snd_soc_codec_set_cache_io sort of implies that you are using
regmap. If you are not using regmap your codec should not call
snd_soc_codec_set_cache_io.

- Lars

 
 ---
  sound/soc/soc-io.c | 14 ++
  1 file changed, 10 insertions(+), 4 deletions(-)
 
 diff --git a/sound/soc/soc-io.c b/sound/soc/soc-io.c
 index 29183ef..4e5b4ae 100644
 --- a/sound/soc/soc-io.c
 +++ b/sound/soc/soc-io.c
 @@ -117,9 +117,6 @@ int snd_soc_codec_set_cache_io(struct snd_soc_codec 
 *codec,
   int ret;
  
   memset(config, 0, sizeof(config));
 - codec-write = hw_write;
 - codec-read = hw_read;
 - codec-bulk_write_raw = snd_soc_hw_bulk_write_raw;
  
   config.reg_bits = addr_bits;
   config.val_bits = data_bits;
 @@ -151,7 +148,9 @@ int snd_soc_codec_set_cache_io(struct snd_soc_codec 
 *codec,
* multiples */
   if (ret  0)
   codec-val_bytes = ret;
 - }
 + } else
 + codec-using_regmap = false;
 +
   break;
  
   default:
 @@ -161,6 +160,13 @@ int snd_soc_codec_set_cache_io(struct snd_soc_codec 
 *codec,
   if (IS_ERR(codec-control_data))
   return PTR_ERR(codec-control_data);
  
 + /* only when using regmap; don't modify unconditionally */
 + if (codec-using_regmap) {
 + codec-write = hw_write;
 + codec-read = hw_read;
 + codec-bulk_write_raw = snd_soc_hw_bulk_write_raw;
 + }
 +
   return 0;
  }
  EXPORT_SYMBOL_GPL(snd_soc_codec_set_cache_io);

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v2 03/15] dmaengine: Add no_wakeup parameter to dmaengine_prep_dma_cyclic()

2012-09-13 Thread Lars-Peter Clausen
On 09/13/2012 03:37 PM, Peter Ujfalusi wrote:
 The dmaengine_prep_dma_cyclic() function primarily used by audio for cyclic
 transfer required by ALSA.
 With this new parameter it is going to be possible to enable the
 SNDRV_PCM_INFO_NO_PERIOD_WAKEUP mode on platforms where it is possible.
 This patch only changes the public API first. Followup patch will change
 the device_prep_dma_cyclic() callback parameters to pass this information
 towards the dma drivers.
 

Hi,

Hm... Do you think it would work as well if we implement this by setting the
callback for the descriptor to NULL? If the callback is NULL there is
nothing to at the end of a transfer/period and the dma engine driver may
choose to disable interrupts. This would also benefit non cyclic transfers
where the callback is NULL and we do not need add the new parameter to
dmaengine_prep_dma_cyclic.

- Lars

 ---
  include/linux/dmaengine.h | 3 ++-
  sound/soc/soc-dmaengine-pcm.c | 3 ++-
  2 files changed, 4 insertions(+), 2 deletions(-)
 
 diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
 index 9c02a45..990444b 100644
 --- a/include/linux/dmaengine.h
 +++ b/include/linux/dmaengine.h
 @@ -653,7 +653,8 @@ static inline struct dma_async_tx_descriptor 
 *dmaengine_prep_rio_sg(
  
  static inline struct dma_async_tx_descriptor *dmaengine_prep_dma_cyclic(
   struct dma_chan *chan, dma_addr_t buf_addr, size_t buf_len,
 - size_t period_len, enum dma_transfer_direction dir)
 + size_t period_len, enum dma_transfer_direction dir,
 + bool no_wakeup)
  {
   return chan-device-device_prep_dma_cyclic(chan, buf_addr, buf_len,
   period_len, dir, NULL);
 diff --git a/sound/soc/soc-dmaengine-pcm.c b/sound/soc/soc-dmaengine-pcm.c
 index 5df529e..6b70adb 100644
 --- a/sound/soc/soc-dmaengine-pcm.c
 +++ b/sound/soc/soc-dmaengine-pcm.c
 @@ -147,7 +147,8 @@ static int dmaengine_pcm_prepare_and_submit(struct 
 snd_pcm_substream *substream)
   desc = dmaengine_prep_dma_cyclic(chan,
   substream-runtime-dma_addr,
   snd_pcm_lib_buffer_bytes(substream),
 - snd_pcm_lib_period_bytes(substream), direction);
 + snd_pcm_lib_period_bytes(substream), direction,
 + substream-runtime-no_period_wakeup);
  
   if (!desc)
   return -ENOMEM;

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v2 03/15] dmaengine: Add no_wakeup parameter to dmaengine_prep_dma_cyclic()

2012-09-14 Thread Lars-Peter Clausen
On 09/14/2012 05:26 AM, Vinod Koul wrote:
 On Thu, 2012-09-13 at 17:27 +0200, Lars-Peter Clausen wrote:
 Hi,

 Hm... Do you think it would work as well if we implement this by
 setting the
 callback for the descriptor to NULL? If the callback is NULL there is
 nothing to at the end of a transfer/period and the dma engine driver
 may
 choose to disable interrupts. This would also benefit non cyclic
 transfers
 where the callback is NULL and we do not need add the new parameter to
 dmaengine_prep_dma_cyclic.
 That will work too BUT the idea of no_wake mode in ALSA is that we
 should not have any interrupts, so anything which is going to cause
 interrupts to AP in undesired. The interrupts still happen and it just
 that dmaengine driver is not notifying client.

Well, the idea was that the driver would disable interrupts if there is no
callback to call, since there would be nothing to do in the interrupt
handler anyway. But I guess the flags approach should work fine as well.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] ti_tscadc: Match mfd sub devices to regmap interface

2012-10-30 Thread Lars-Peter Clausen
On 10/31/2012 04:55 PM, Pantelis Antoniou wrote:
 The MFD parent device now uses a regmap, instead of direct
 memory access. Use the same method in the sub devices to avoid
 nasty surprises.
 
 Also rework the channel initialization of tiadc a bit.

Those two bits are not even closely related, they should really go into
separate patches.

 
 Signed-off-by: Pantelis Antoniou pa...@antoniou-consulting.com
 ---
  drivers/iio/adc/ti_am335x_adc.c   | 27 +++
  drivers/input/touchscreen/ti_am335x_tsc.c | 16 +---
  drivers/mfd/ti_am335x_tscadc.c|  7 +--
  3 files changed, 37 insertions(+), 13 deletions(-)
 
[...]
  
 @@ -213,6 +222,8 @@ static int __devinit tiadc_probe(struct platform_device 
 *pdev)
  
   platform_set_drvdata(pdev, indio_dev);
  
 + dev_info(pdev-dev, Initialized\n);

That's just noise, please don't add it. Imagine every driver did this you'd
end up with a lot of noise your debug log.

 +
   return 0;
  
  err_free_channels:
 diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c 
 b/drivers/input/touchscreen/ti_am335x_tsc.c
 index 7a26810..d09e1a7 100644
 --- a/drivers/input/touchscreen/ti_am335x_tsc.c
 +++ b/drivers/input/touchscreen/ti_am335x_tsc.c
 @@ -26,6 +26,7 @@
[...]
  /*
 @@ -455,10 +460,15 @@ static int __devinit titsc_probe(struct platform_device 
 *pdev)
  
   /* register to the input system */
   err = input_register_device(input_dev);
 - if (err)
 + if (err) {
 + dev_err(pdev-dev, Failed to register input device\n);
   goto err_free_irq;
 + }
  
   platform_set_drvdata(pdev, ts_dev);
 +
 + dev_info(pdev-dev, Initialized OK\n);

Same here


--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] ti_tscadc: Update with IIO map interface deal with partial activation

2012-10-30 Thread Lars-Peter Clausen
On 10/31/2012 04:55 PM, Pantelis Antoniou wrote:
 Add an IIO map interface that consumers can use.
 Also make sure the mfd device doesn't activate a driver which
 the configuration doesn't require.

Same here, two completely different changes in the same patch.

 
 Signed-off-by: Pantelis Antoniou pa...@antoniou-consulting.com
 ---
  drivers/iio/adc/ti_am335x_adc.c  | 53 
 ++--
  drivers/mfd/ti_am335x_tscadc.c   | 30 ++--
  include/linux/mfd/ti_am335x_tscadc.h |  8 ++
  3 files changed, 68 insertions(+), 23 deletions(-)
 
 diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
 index 02a43c8..d48fd79 100644
 --- a/drivers/iio/adc/ti_am335x_adc.c
 +++ b/drivers/iio/adc/ti_am335x_adc.c
 @@ -20,8 +20,9 @@
  #include linux/slab.h
  #include linux/interrupt.h
  #include linux/platform_device.h
 -#include linux/io.h
  #include linux/iio/iio.h
 +#include linux/iio/machine.h
 +#include linux/iio/driver.h
  
  #include linux/mfd/ti_am335x_tscadc.h
  #include linux/platform_data/ti_am335x_adc.h
 @@ -29,6 +30,8 @@
  struct tiadc_device {
   struct ti_tscadc_dev *mfd_tscadc;
   int channels;
 + char *buf;

As far as I can see 'buf' is not used otherwise in this patch.

 + struct iio_map *map;
  };
  
  static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
 @@ -75,25 +78,57 @@ static void tiadc_step_config(struct tiadc_device 
 *adc_dev)
  static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
  {
   struct iio_chan_spec *chan_array;
 - int i;
 -
 - indio_dev-num_channels = channels;
 - chan_array = kcalloc(indio_dev-num_channels,
 - sizeof(struct iio_chan_spec), GFP_KERNEL);
 + struct iio_chan_spec *chan;
 + char *s;
 + int i, len, size, ret;
  
 + size = indio_dev-num_channels * (sizeof(struct iio_chan_spec) + 6);
 + chan_array = kzalloc(size, GFP_KERNEL);
   if (chan_array == NULL)
   return -ENOMEM;
  
 - for (i = 0; i  (indio_dev-num_channels); i++) {
 - struct iio_chan_spec *chan = chan_array + i;
 + /* buffer space is after the array */
 + s = (char *)(chan_array + indio_dev-num_channels);
 + chan = chan_array;
 + for (i = 0; i  indio_dev-num_channels; i++, chan++, s += len + 1) {
 +
 + len = sprintf(s, AIN%d, i);
 +
   chan-type = IIO_VOLTAGE;
   chan-indexed = 1;
   chan-channel = i;
 - chan-info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT;
 + chan-datasheet_name = s;

 + chan-scan_type.sign = 'u';
 + chan-scan_type.realbits = 12;
 + chan-scan_type.storagebits = 32;
 + chan-scan_type.shift = 0;

This part is another separate thing done by this patch and is not even in
the patch description.

   }
  
   indio_dev-channels = chan_array;
  
 + size = (indio_dev-num_channels + 1) * sizeof(struct iio_map);
 + adc_dev-map = kzalloc(size, GFP_KERNEL);
 + if (adc_dev-map == NULL) {
 + kfree(chan_array);
 + return -ENOMEM;
 + }
 +
 + for (i = 0; i  indio_dev-num_channels; i++) {
 + adc_dev-map[i].adc_channel_label = 
 chan_array[i].datasheet_name;
 + adc_dev-map[i].consumer_dev_name = any;
 + adc_dev-map[i].consumer_channel = chan_array[i].datasheet_name;
 + }
 + adc_dev-map[i].adc_channel_label = NULL;
 + adc_dev-map[i].consumer_dev_name = NULL;
 + adc_dev-map[i].consumer_channel = NULL;
 +
 + ret = iio_map_array_register(indio_dev, adc_dev-map);
 + if (ret != 0) {
 + kfree(adc_dev-map);
 + kfree(chan_array);
 + return -ENOMEM;
 + }

The consumer data is supposed to be passed in by platform data, as it will
depend on the actual consumer. E.g. the consumer_dev_name has to match the
name of device which requests the channel. Same goes for the consumer
channel attribute, this is consumer specific as well.

 +
   return indio_dev-num_channels;
  }
  
 diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
 index e947dd8..cbb8b70c 100644
 --- a/drivers/mfd/ti_am335x_tscadc.c
 +++ b/drivers/mfd/ti_am335x_tscadc.c
 @@ -176,26 +176,38 @@ static  int __devinit ti_tscadc_probe(struct 
 platform_device *pdev)
   ctrl |= CNTRLREG_TSCSSENB;
   tscadc_writel(tscadc, REG_CTRL, ctrl);
  
 + tscadc-used_cells = 0;
 + tscadc-tsc_cell = -1;
 + tscadc-adc_cell = -1;
 +
   /* TSC Cell */
 - cell = tscadc-cells[TSC_CELL];
 - cell-name = tsc;
 - cell-platform_data = tscadc;
 - cell-pdata_size = sizeof(*tscadc);
 + if (tsc_wires  0) {
 + tscadc-tsc_cell = tscadc-used_cells;
 + cell = tscadc-cells[tscadc-used_cells++];
 + cell-name = tsc;
 + cell-platform_data = tscadc;
 + cell-pdata_size = sizeof(*tscadc);
 + }

Re: [PATCH] ti_tscadc: Match mfd sub devices to regmap interface

2012-10-31 Thread Lars-Peter Clausen
On 10/31/2012 05:41 AM, Russ Dill wrote:
 On Wed, Oct 31, 2012 at 8:55 AM, Pantelis Antoniou
 pa...@antoniou-consulting.com wrote:
 The MFD parent device now uses a regmap, instead of direct
 memory access. Use the same method in the sub devices to avoid
 nasty surprises.

 Also rework the channel initialization of tiadc a bit.

 Signed-off-by: Pantelis Antoniou pa...@antoniou-consulting.com
 ---
  drivers/iio/adc/ti_am335x_adc.c   | 27 +++
  drivers/input/touchscreen/ti_am335x_tsc.c | 16 +---
  drivers/mfd/ti_am335x_tscadc.c|  7 +--
  3 files changed, 37 insertions(+), 13 deletions(-)

 diff --git a/drivers/iio/adc/ti_am335x_adc.c 
 b/drivers/iio/adc/ti_am335x_adc.c
 index d48fd79..5f325c1 100644
 --- a/drivers/iio/adc/ti_am335x_adc.c
 +++ b/drivers/iio/adc/ti_am335x_adc.c
 @@ -23,7 +23,9 @@
  #include linux/iio/iio.h
  #include linux/iio/machine.h
  #include linux/iio/driver.h
 +#include linux/regmap.h

 +#include linux/io.h
  #include linux/mfd/ti_am335x_tscadc.h
  #include linux/platform_data/ti_am335x_adc.h

 @@ -36,13 +38,17 @@ struct tiadc_device {

  static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
  {
 -   return readl(adc-mfd_tscadc-tscadc_base + reg);
 +   unsigned int val;
 +
 +   val = (unsigned int)-1;
 +   regmap_read(adc-mfd_tscadc-regmap_tscadc, reg, val);
 +   return val;
  }
 
 Would it be cleaner to instead do:
 
 static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
 {
unsigned int val;
 
return regmap_read(adc-mfd_tscadc-regmap_tscadc, reg, val) ? : val;
 }

In my opinion the best would be to just mimic the regmap interface here. Return
an error code or 0 and pass the value back through a pointer parameter.



--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/3] ti_adc: Update with IIO map interface

2012-10-31 Thread Lars-Peter Clausen
On 11/01/2012 04:24 PM, Pantelis Antoniou wrote:
 Add an IIO map interface that consumers can use.

Hi,

Looks like you overlooked the review comments I had inline last time. I've
put them in again, see below.

 
 Signed-off-by: Pantelis Antoniou pa...@antoniou-consulting.com
 ---
  drivers/iio/adc/ti_am335x_adc.c | 60 
 +
  1 file changed, 49 insertions(+), 11 deletions(-)
 
 diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
 index 02a43c8..8595a90 100644
 --- a/drivers/iio/adc/ti_am335x_adc.c
 +++ b/drivers/iio/adc/ti_am335x_adc.c
 @@ -20,8 +20,9 @@
  #include linux/slab.h
  #include linux/interrupt.h
  #include linux/platform_device.h
 -#include linux/io.h
  #include linux/iio/iio.h
 +#include linux/iio/machine.h
 +#include linux/iio/driver.h
  
  #include linux/mfd/ti_am335x_tscadc.h
  #include linux/platform_data/ti_am335x_adc.h
 @@ -29,6 +30,8 @@
  struct tiadc_device {
   struct ti_tscadc_dev *mfd_tscadc;
   int channels;
 + char *buf;

buf doesn't seem to be used anywhere

 + struct iio_map *map;
  };
  
  static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
 @@ -72,27 +75,62 @@ static void tiadc_step_config(struct tiadc_device 
 *adc_dev)
   tiadc_writel(adc_dev, REG_SE, STPENB_STEPENB);
  }
  
 -static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
 +static int tiadc_channel_init(struct iio_dev *indio_dev,
 + struct tiadc_device *adc_dev)
  {
   struct iio_chan_spec *chan_array;
 - int i;
 -
 - indio_dev-num_channels = channels;
 - chan_array = kcalloc(indio_dev-num_channels,
 - sizeof(struct iio_chan_spec), GFP_KERNEL);
 + struct iio_chan_spec *chan;
 + char *s;
 + int i, len, size, ret;
 + int channels = adc_dev-channels;
  
 + size = channels * (sizeof(struct iio_chan_spec) + 6);
 + chan_array = kzalloc(size, GFP_KERNEL);
   if (chan_array == NULL)
   return -ENOMEM;
  
 - for (i = 0; i  (indio_dev-num_channels); i++) {
 - struct iio_chan_spec *chan = chan_array + i;
 + /* buffer space is after the array */
 + s = (char *)(chan_array + channels);
 + chan = chan_array;
 + for (i = 0; i  channels; i++, chan++, s += len + 1) {
 +
 + len = sprintf(s, AIN%d, i);
 +
   chan-type = IIO_VOLTAGE;
   chan-indexed = 1;
   chan-channel = i;
 - chan-info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT;
 + chan-datasheet_name = s;
 + chan-scan_type.sign = 'u';
 + chan-scan_type.realbits = 12;
 + chan-scan_type.storagebits = 32;
 + chan-scan_type.shift = 0;

The scan type assignment should go in a separate patch if possible.

   }
  
   indio_dev-channels = chan_array;
 + indio_dev-num_channels = channels;
 +
 + size = (channels + 1) * sizeof(struct iio_map);
 + adc_dev-map = kzalloc(size, GFP_KERNEL);
 + if (adc_dev-map == NULL) {
 + kfree(chan_array);
 + return -ENOMEM;
 + }
 +
 + for (i = 0; i  indio_dev-num_channels; i++) {
 + adc_dev-map[i].adc_channel_label = 
 chan_array[i].datasheet_name;
 + adc_dev-map[i].consumer_dev_name = any;
 + adc_dev-map[i].consumer_channel = chan_array[i].datasheet_name;
 + }
 + adc_dev-map[i].adc_channel_label = NULL;
 + adc_dev-map[i].consumer_dev_name = NULL;
 + adc_dev-map[i].consumer_channel = NULL;

The map should be passed in via platform data or similar. All the fields of
the map depend on the specific user, so you can't use a generic map. In fact
if we were able to use a generic map, we wouldn't need a map at all.

 +
 + ret = iio_map_array_register(indio_dev, adc_dev-map);
 + if (ret != 0) {
 + kfree(adc_dev-map);
 + kfree(chan_array);
 + return -ENOMEM;
 + }
  
   return indio_dev-num_channels;
  }
 @@ -168,7 +206,7 @@ static int __devinit tiadc_probe(struct platform_device 
 *pdev)
  
   tiadc_step_config(adc_dev);
  
 - err = tiadc_channel_init(indio_dev, adc_dev-channels);
 + err = tiadc_channel_init(indio_dev, adc_dev);
   if (err  0)
   goto err_free_device;
  

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/3] ti_adc: Update with IIO map interface

2012-10-31 Thread Lars-Peter Clausen
On 10/31/2012 06:55 PM, Pantelis Antoniou wrote:
 [...]
 }

 indio_dev-channels = chan_array;
 +   indio_dev-num_channels = channels;
 +
 +   size = (channels + 1) * sizeof(struct iio_map);
 +   adc_dev-map = kzalloc(size, GFP_KERNEL);
 +   if (adc_dev-map == NULL) {
 +   kfree(chan_array);
 +   return -ENOMEM;
 +   }
 +
 +   for (i = 0; i  indio_dev-num_channels; i++) {
 +   adc_dev-map[i].adc_channel_label = 
 chan_array[i].datasheet_name;
 +   adc_dev-map[i].consumer_dev_name = any;
 +   adc_dev-map[i].consumer_channel = chan_array[i].datasheet_name;
 +   }
 +   adc_dev-map[i].adc_channel_label = NULL;
 +   adc_dev-map[i].consumer_dev_name = NULL;
 +   adc_dev-map[i].consumer_channel = NULL;

 The map should be passed in via platform data or similar. All the fields of
 the map depend on the specific user, so you can't use a generic map. In fact
 if we were able to use a generic map, we wouldn't need a map at all.
 
 There's no platform data in the board I'm using. It's board-generic using
 device tree only.

That's the 'or similar' ;) Unfortunately we do not have a device tree
binding for IIO yet. But I think we should aim at a interface similar like
we have in other subsystems like the clk, regulator or dma framework.

- Lars
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/3] ti_adc: Update with IIO map interface

2012-10-31 Thread Lars-Peter Clausen
On 10/31/2012 07:12 PM, Pantelis Antoniou wrote:
 
 On Oct 31, 2012, at 8:07 PM, Lars-Peter Clausen wrote:
 
 On 10/31/2012 06:55 PM, Pantelis Antoniou wrote:
 [...]
   }

   indio_dev-channels = chan_array;
 + indio_dev-num_channels = channels;
 +
 + size = (channels + 1) * sizeof(struct iio_map);
 + adc_dev-map = kzalloc(size, GFP_KERNEL);
 + if (adc_dev-map == NULL) {
 + kfree(chan_array);
 + return -ENOMEM;
 + }
 +
 + for (i = 0; i  indio_dev-num_channels; i++) {
 + adc_dev-map[i].adc_channel_label = 
 chan_array[i].datasheet_name;
 + adc_dev-map[i].consumer_dev_name = any;
 + adc_dev-map[i].consumer_channel = chan_array[i].datasheet_name;
 + }
 + adc_dev-map[i].adc_channel_label = NULL;
 + adc_dev-map[i].consumer_dev_name = NULL;
 + adc_dev-map[i].consumer_channel = NULL;

 The map should be passed in via platform data or similar. All the fields of
 the map depend on the specific user, so you can't use a generic map. In 
 fact
 if we were able to use a generic map, we wouldn't need a map at all.

 There's no platform data in the board I'm using. It's board-generic using
 device tree only.

 That's the 'or similar' ;) Unfortunately we do not have a device tree
 binding for IIO yet. But I think we should aim at a interface similar like
 we have in other subsystems like the clk, regulator or dma framework.

 - Lars
 
 So in the meantime no-one can use IIO ADC in any OF only platform.

Yes, nobody can use it until somebody implements it. So far nobody needed
it, so that's why it hasn't been implemented yet. The whole in kernel
consumer API for IIO is still very young and only a very few drivers support
it yet.

 
 In the meantime, this is pretty reasonable IMO. This is only for a specific 
 board with known channel mappings.

Unfortunately it is not. It is adding a device specific hack to a generic
driver and it is also completely misusing the API.

 
 I'm not out to fix IIO, I'm out to fix a single board.
 

It's not about fixing IIO, it's about extending IIO to be able to serve your
needs. See, the issue is if everybody would work around the lack of DT
bindings we'll never have DT bindings for IIO, so the right thing to do is
to implement them instead of working around the lack of.

- Lars
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/3] ti_adc: Update with IIO map interface

2012-10-31 Thread Lars-Peter Clausen
On 10/31/2012 07:43 PM, Pantelis Antoniou wrote:
 
 On Oct 31, 2012, at 8:36 PM, Lars-Peter Clausen wrote:
 
 On 10/31/2012 07:12 PM, Pantelis Antoniou wrote:

 On Oct 31, 2012, at 8:07 PM, Lars-Peter Clausen wrote:

 On 10/31/2012 06:55 PM, Pantelis Antoniou wrote:
 [...]
 }

 indio_dev-channels = chan_array;
 +   indio_dev-num_channels = channels;
 +
 +   size = (channels + 1) * sizeof(struct iio_map);
 +   adc_dev-map = kzalloc(size, GFP_KERNEL);
 +   if (adc_dev-map == NULL) {
 +   kfree(chan_array);
 +   return -ENOMEM;
 +   }
 +
 +   for (i = 0; i  indio_dev-num_channels; i++) {
 +   adc_dev-map[i].adc_channel_label = 
 chan_array[i].datasheet_name;
 +   adc_dev-map[i].consumer_dev_name = any;
 +   adc_dev-map[i].consumer_channel = 
 chan_array[i].datasheet_name;
 +   }
 +   adc_dev-map[i].adc_channel_label = NULL;
 +   adc_dev-map[i].consumer_dev_name = NULL;
 +   adc_dev-map[i].consumer_channel = NULL;

 The map should be passed in via platform data or similar. All the fields 
 of
 the map depend on the specific user, so you can't use a generic map. In 
 fact
 if we were able to use a generic map, we wouldn't need a map at all.

 There's no platform data in the board I'm using. It's board-generic using
 device tree only.

 That's the 'or similar' ;) Unfortunately we do not have a device tree
 binding for IIO yet. But I think we should aim at a interface similar like
 we have in other subsystems like the clk, regulator or dma framework.

 - Lars

 So in the meantime no-one can use IIO ADC in any OF only platform.

 Yes, nobody can use it until somebody implements it. So far nobody needed
 it, so that's why it hasn't been implemented yet. The whole in kernel
 consumer API for IIO is still very young and only a very few drivers support
 it yet.


 In the meantime, this is pretty reasonable IMO. This is only for a specific 
 board with known channel mappings.

 Unfortunately it is not. It is adding a device specific hack to a generic
 driver and it is also completely misusing the API.


 I'm not out to fix IIO, I'm out to fix a single board.


 It's not about fixing IIO, it's about extending IIO to be able to serve your
 needs. See, the issue is if everybody would work around the lack of DT
 bindings we'll never have DT bindings for IIO, so the right thing to do is
 to implement them instead of working around the lack of.

 - Lars
 
 OK, OK,
 

ok :)

 I see the point. It's just that I'm under the gun for more pressing matters 
 ATM.
 Can at least get a small write-up about how the bindings should look like?
 
 There's absolutely nothing, not even a hint of one out there in the 
 intertubes,
 on how the channel mapping should look like.

Again, that's because nobody probably has given this much though yet. As I said
the in-kernel consumer framework is still young and so far only a tiny fraction
of the drivers support it. If you want to I can try to give this some though
and come up with a small proof of concept, but this would have to wait until
next week, since I have a few other things on my desk as well.

I think a good start might be to take a closer look at the clk dt bindings
(Documentation/devicetree/bindings/clock/clock-bindings.txt).

- Lars
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 10/14] PM / MIPS: Use struct syscore_ops instead of sysdevs for PM

2011-04-18 Thread Lars-Peter Clausen
On 04/17/2011 11:12 PM, Rafael J. Wysocki wrote:
 From: Rafael J. Wysocki r...@sisk.pl
 
 Convert some MIPS architecture's code to using struct syscore_ops
 objects for power management instead of sysdev classes and sysdevs.
 
 This simplifies the code and reduces the kernel's memory footprint.
 It also is necessary for removing sysdevs from the kernel entirely in
 the future.
 
 Signed-off-by: Rafael J. Wysocki r...@sisk.pl

For the jz4740 part:
Acked-and-tested-by: Lars-Peter Clausen l...@metafoo.de

 ---
  arch/mips/alchemy/common/dbdma.c |   92 
 +++
  arch/mips/alchemy/common/irq.c   |   62 +-
  arch/mips/jz4740/gpio.c  |   52 +-
  arch/mips/kernel/i8259.c |   26 +++
  4 files changed, 80 insertions(+), 152 deletions(-)
 
 Index: linux-2.6/arch/mips/alchemy/common/irq.c
 ===
 --- linux-2.6.orig/arch/mips/alchemy/common/irq.c
 +++ linux-2.6/arch/mips/alchemy/common/irq.c
 @@ -30,7 +30,7 @@
  #include linux/interrupt.h
  #include linux/irq.h
  #include linux/slab.h
 -#include linux/sysdev.h
 +#include linux/syscore_ops.h
  
  #include asm/irq_cpu.h
  #include asm/mipsregs.h
 @@ -556,17 +556,15 @@ void __init arch_init_irq(void)
   }
  }
  
 -struct alchemy_ic_sysdev {
 - struct sys_device sysdev;
 +struct alchemy_ic {
   void __iomem *base;
   unsigned long pmdata[7];
  };
  
 -static int alchemy_ic_suspend(struct sys_device *dev, pm_message_t state)
 -{
 - struct alchemy_ic_sysdev *icdev =
 - container_of(dev, struct alchemy_ic_sysdev, sysdev);
 +static struct alchemy_ic alchemy_ic_data[2];
  
 +static void alchemy_suspend_one_ic(struct alchemy_ic *icdev)
 +{
   icdev-pmdata[0] = __raw_readl(icdev-base + IC_CFG0RD);
   icdev-pmdata[1] = __raw_readl(icdev-base + IC_CFG1RD);
   icdev-pmdata[2] = __raw_readl(icdev-base + IC_CFG2RD);
 @@ -574,15 +572,17 @@ static int alchemy_ic_suspend(struct sys
   icdev-pmdata[4] = __raw_readl(icdev-base + IC_ASSIGNRD);
   icdev-pmdata[5] = __raw_readl(icdev-base + IC_WAKERD);
   icdev-pmdata[6] = __raw_readl(icdev-base + IC_MASKRD);
 +}
  
 +static int alchemy_ic_suspend(void)
 +{
 + alchemy_suspend_one_ic(alchemy_ic_data);
 + alchemy_suspend_one_ic(alchemy_ic_data + 1);
   return 0;
  }
  
 -static int alchemy_ic_resume(struct sys_device *dev)
 +static void alchemy_resume_one_ic(struct alchemy_ic *icdev)
  {
 - struct alchemy_ic_sysdev *icdev =
 - container_of(dev, struct alchemy_ic_sysdev, sysdev);
 -
   __raw_writel(0x, icdev-base + IC_MASKCLR);
   __raw_writel(0x, icdev-base + IC_CFG0CLR);
   __raw_writel(0x, icdev-base + IC_CFG1CLR);
 @@ -604,42 +604,26 @@ static int alchemy_ic_resume(struct sys_
  
   __raw_writel(icdev-pmdata[6], icdev-base + IC_MASKSET);
   wmb();
 +}
  
 - return 0;
 +static void alchemy_ic_resume(void)
 +{
 + alchemy_resume_one_ic(alchemy_ic_data + 1);
 + alchemy_resume_one_ic(alchemy_ic_data);
  }
  
 -static struct sysdev_class alchemy_ic_sysdev_class = {
 - .name   = ic,
 +static struct syscore_ops alchemy_ic_syscore_ops = {
   .suspend= alchemy_ic_suspend,
   .resume = alchemy_ic_resume,
  };
  
 -static int __init alchemy_ic_sysdev_init(void)
 +static int __init alchemy_ic_syscore_init(void)
  {
 - struct alchemy_ic_sysdev *icdev;
 - unsigned long icbase[2] = { IC0_PHYS_ADDR, IC1_PHYS_ADDR };
 - int err, i;
 -
 - err = sysdev_class_register(alchemy_ic_sysdev_class);
 - if (err)
 - return err;
 -
 - for (i = 0; i  2; i++) {
 - icdev = kzalloc(sizeof(struct alchemy_ic_sysdev), GFP_KERNEL);
 - if (!icdev)
 - return -ENOMEM;
 -
 - icdev-base = ioremap(icbase[i], 0x1000);
 -
 - icdev-sysdev.id = i;
 - icdev-sysdev.cls = alchemy_ic_sysdev_class;
 - err = sysdev_register(icdev-sysdev);
 - if (err) {
 - kfree(icdev);
 - return err;
 - }
 - }
 + alchemy_ic_data[0].base = ioremap(icbase[IC0_PHYS_ADDR], 0x1000);
 + alchemy_ic_data[1].base = ioremap(icbase[IC1_PHYS_ADDR], 0x1000);
 +
 + register_syscore_ops(alchemy_ic_syscore_ops);
  
   return 0;
  }
 -device_initcall(alchemy_ic_sysdev_init);
 +device_initcall(alchemy_ic_syscore_init);
 Index: linux-2.6/arch/mips/alchemy/common/dbdma.c
 ===
 --- linux-2.6.orig/arch/mips/alchemy/common/dbdma.c
 +++ linux-2.6/arch/mips/alchemy/common/dbdma.c
 @@ -36,7 +36,7 @@
  #include linux/spinlock.h
  #include linux/interrupt.h
  #include linux/module.h
 -#include linux/sysdev.h
 +#include linux/syscore_ops.h
  #include asm/mach-au1x00/au1000.h
  #include asm/mach-au1x00/au1xxx_dbdma.h

Re: [PATCH] mmc: fix handling NULL returned from regulator_get()

2011-05-18 Thread Lars-Peter Clausen
On 05/18/2011 02:47 PM, Chris Ball wrote:
 Hi Antonio, thanks for doing this,
 
 On Wed, May 18 2011, Antonio Ospite wrote:
 When regulator_get() is stubbed down it returns NULL, handle this case
 when deciding whether the driver can use the regulator or not.

 Remember: IS_ERR(NULL) is false, see also this discussion for more
 insight: http://comments.gmane.org/gmane.linux.kernel.mmc/7934

 Now that regulator_get() is handled correctly, the ifdef on
 CONFIG_REGULATOR in mmci.c can go away as well.

 Signed-off-by: Antonio Ospite osp...@studenti.unina.it
 ---

  drivers/mmc/host/dw_mmc.c |2 +-
  drivers/mmc/host/mmci.c   |4 +---
  drivers/mmc/host/mxcmmc.c |2 +-
  drivers/mmc/host/omap_hsmmc.c |4 ++--
  drivers/mmc/host/sdhci.c  |2 +-
  5 files changed, 6 insertions(+), 8 deletions(-)

 diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
 index 87e1f57..5be1325 100644
 --- a/drivers/mmc/host/dw_mmc.c
 +++ b/drivers/mmc/host/dw_mmc.c
 @@ -1442,7 +1442,7 @@ static int __init dw_mci_init_slot(struct dw_mci 
 *host, unsigned int id)
  #endif /* CONFIG_MMC_DW_IDMAC */
  
  host-vmmc = regulator_get(mmc_dev(mmc), vmmc);
 -if (IS_ERR(host-vmmc)) {
 +if (IS_ERR_OR_NULL(host-vmmc)) {
  printk(KERN_INFO %s: no vmmc regulator found\n, 
 mmc_hostname(mmc));
  host-vmmc = NULL;
  } else
 diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
 index 259ece0..45fd4fe 100644
 --- a/drivers/mmc/host/omap_hsmmc.c
 +++ b/drivers/mmc/host/omap_hsmmc.c
 @@ -405,7 +405,7 @@ static int omap_hsmmc_reg_get(struct omap_hsmmc_host 
 *host)
  }
  
  reg = regulator_get(host-dev, vmmc);
 -if (IS_ERR(reg)) {
 +if (IS_ERR_OR_NULL(reg)) {
  dev_dbg(host-dev, vmmc regulator missing\n);
  /*
  * HACK: until fixed.c regulator is usable,
  } else {
 [...] 
 I think I like this change for every driver *except* sdhci -- users who
 compile with CONFIG_REGULATOR=n (i.e. most distros) will start seeing
 mmc0: no vmmc regulator found when they boot their x86 laptops, and
 printing a cryptic regulator error message when regulator support isn't
 even compiled in seems uncalled for.
 
 So, I think my suggestion is to merge all but the last hunk of the
 patch.  How do others feel about it?
 

dw_mmc and omap_hsmmc will also print error messages if the regulator framework
is not build-in. I suggest to use something like:

if (IS_ERR(vmmc)) {
... do error handling
} else if(vmmc) {
... use regulator
} else {
... fallback code to initialize ocr_avail
}

- Lars
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 5/5] bq27x00 - don't report power-supply change so often.

2011-12-31 Thread Lars-Peter Clausen
On 12/30/2011 12:13 PM, Grazvydas Ignotas wrote:
 CCing Lars who added this. I vaguely recall something about generating
 events to make some battery monitors update but I forget the details
 now, maybe it was about something else. Also CCing Anton (the
 maintainer).
 
 On Fri, Dec 30, 2011 at 2:58 AM, NeilBrown ne...@suse.de wrote:
 A power_supply_changed should only be reported on significant changes
 such as transition between charging and not.  Incremental changes
 such as charge increasing should not be reported - that can easily
 be polled for.

Well, you can also poll for those significant changes, too. The point of
adding this was to have a centralized point where polling takes place instead
of letting each battery monitor do this on its own. Though if, as you wrote in
the cover letter, the some properties change every time the values are read it
might makes sense to exclude these from the comparison. On the other hand one
event every 6 minutes doesn't really sound harmful and I would suspect that
battery monitors will use a similar interval when manually polling the device.

- Lars


 Signed-off-by: NeilBrown ne...@suse.de
 ---

  drivers/power/bq27x00_battery.c |   15 ---
  1 files changed, 12 insertions(+), 3 deletions(-)

 diff --git a/drivers/power/bq27x00_battery.c 
 b/drivers/power/bq27x00_battery.c
 index bb16f5b..7993a17 100644
 --- a/drivers/power/bq27x00_battery.c
 +++ b/drivers/power/bq27x00_battery.c
 @@ -57,11 +57,15 @@
  #define BQ27000_FLAG_CHGS  BIT(7)
  #define BQ27000_FLAG_FCBIT(5)

 +#define BQ27000_FLAGS_IMPORTANT
 (BQ27000_FLAG_FC|BQ27000_FLAG_CHGS|BIT(31))
 +
  #define BQ27500_REG_SOC0x2C
  #define BQ27500_REG_DCAP   0x3C /* Design capacity */
  #define BQ27500_FLAG_DSC   BIT(0)
  #define BQ27500_FLAG_FCBIT(9)

 +#define BQ27500_FLAGS_IMPORTANT
 (BQ27500_FLAG_FC|BQ27500_FLAG_DSC|BIT(31))
 +
  #define BQ27000_RS 20 /* Resistor sense */

  struct bq27x00_device_info;
 @@ -259,6 +263,7 @@ static void bq27x00_update(struct bq27x00_device_info 
 *di)
  {
struct bq27x00_reg_cache cache = {0, };
bool is_bq27500 = di-chip == BQ27500;
 +   int flags_changed;

cache.flags = bq27x00_read(di, BQ27x00_REG_FLAGS, is_bq27500);
if (cache.flags = 0) {
 @@ -280,10 +285,14 @@ static void bq27x00_update(struct bq27x00_device_info 
 *di)

/* Ignore current_now which is a snapshot of the current battery state
 * and is likely to be different even between two consecutive reads */
 -   if (memcmp(di-cache, cache, sizeof(cache) - sizeof(int)) != 0) {
 -   di-cache = cache;
 +   flags_changed = di-cache.flags ^ cache.flags;
 +   di-cache = cache;
 +   if (is_bq27500)
 +   flags_changed = BQ27500_FLAGS_IMPORTANT;
 +   else
 +   flags_changed = BQ27000_FLAGS_IMPORTANT;
 +   if (flags_changed)
power_supply_changed(di-bat);
 -   }

di-last_update = jiffies;
  }

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-26 Thread Lars-Peter Clausen
On 11/23/2012 10:44 AM, Peter Ujfalusi wrote:
 Hi Grant,
 
 On 11/23/2012 10:13 AM, Peter Ujfalusi wrote:
 Hi Grant,

 On 11/23/2012 08:55 AM, Grant Likely wrote:
 Ugh. and this is why I wanted the PWM and GPIO subsystems to use the
 same namespace and binding. grumble, mutter But that's not your fault.

 It's pretty horrible to have a separate translator node to convert a PWM
 into a GPIO (with output only of course). The gpio properties should
 appear directly in the PWM node itself and the translation code should
 be in either the pwm or the gpio core. I don't think it should look like
 a separate device.

 Let me see if I understand your suggestion correctly. In the DT you suggest
 something like this:

 twl_pwmled: pwmled {
  compatible = ti,twl4030-pwmled;
  #pwm-cells = 2;
  #gpio-cells = 2;
  gpio-controller;
 };
 
 After I thought about this.. Is this what we really want?
 After all what we have here is a PWM generator used to emulate a GPIO signal.
 The PWM itself can be used for driving a LED (standard LED or backlight and we
 have DT bindings for these already), vibra motor, or other things.
 If we combine the PWM with GPIO we should go and 'bloat' the DT node to also
 include all sort of other uses of PWM at once?
 
 IMHO it is better to keep them as separate things.
 pwm node to describe the PWM generator,
 separate nodes to describe it's uses like led, backlight, motor and gpio.
 

The difference here is that the LED, backlight, etc are all different
physical devices begin driven by the pwm pin, so it makes sense to have a
device tree node for them, while using the pwm as gpio is just a different
function of the same physical pin.  So in a sense the pwm controller also
becomes a gpio controller. I like the idea of the pwm core automatically
instantiating a pwm-gpo device if it sees a gpio-controller property in the
pwm device devicetree node.

- Lars
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-28 Thread Lars-Peter Clausen
On 11/28/2012 09:54 AM, Peter Ujfalusi wrote:
 Hi Grant, Lars, Thierry,
 
 On 11/26/2012 04:46 PM, Grant Likely wrote:
 You're effectively asking the pwm layer to behave like a gpio (which
 is completely reasonable). Having a completely separate translation node
 really doesn't make sense because it is entirely a software construct.
 In fact, the way your using it is *entirely* to make the Linux driver
 model instantiate the translation code. It has *nothing* to do with the
 structure of the hardware. It makes complete sense that if a PWM is
 going to be used as a GPIO, then the PWM node should conform to the GPIO
 binding.
 
 I understand your point around this. I might say I agree with it as well...
 I spent yesterday with prototyping and I'm not really convinced that it is a
 good approach from C code point of view. I got it working, yes.
 In essence this is what I have on top of the slightly modified gpio-pwm.c
 driver I have submitted:
 
 DTS files:
 twl_pwm: pwm {
   /* provides two PWMs (id 0, 1 for PWM1 and PWM2) */
   compatible = ti,twl6030-pwm;
   #pwm-cells = 2;
 
   /* Enable GPIO us of the PWMs */
   gpio-controller = 1;
   #gpio-cells = 2;
   pwm,period_ns = 7812500;
 };
 
 leds {
   compatible = gpio-leds;
   backlight {
   label = omap4::backlight;
   gpios = twl_pwm 1 0; /* PWM1 of twl6030 */
   };
 
   keypad {
   label = omap4::keypad;
   gpios = twl_pwm 0 0; /* PWM0 of twl6030 */
   };
 };
 
 The bulk of the code in drivers/pwm/core.c to create the pwm-gpo device when
 it is requested going to look something like this. I have removed the error
 checks for now and I still don't have the code to clean up the allocated
 memory for the created device on error, or in case the module is unloaded. We
 should also prevent the pwm core from removal when the pwm-gpo driver is 
 loaded.
 We need to create the platform device for gpo-pwm, create the pdata structure
 for it and fill it in. We also need to hand craft the pwm_lookup table so we
 can use pwm_get() to request the PWM. I have other minor changes around this
 to get things working when we booted with DT.
 So the function to do the heavy lifting is something like this:
 static void of_pwmchip_as_gpio(struct pwm_chip *chip)
 {
   struct platform_device *pdev;
   struct gpio_pwm *gpos;
   struct gpio_pwm_pdata *pdata;
   struct pwm_lookup *lookup;
   char gpodev_name[15];
   int i;
   u32 gpio_mode = 0;
   u32 period_ns = 0;
 
   of_property_read_u32(chip-dev-of_node, gpio-controller,
gpio_mode);
   if (!gpio_mode)
   return;
 
   of_property_read_u32(chip-dev-of_node, pwm,period_ns, period_ns);
   if (!period_ns) {
   dev_err(chip-dev,
   period_ns is not specified for GPIO use\n);
   return;
   }
 
   lookup = devm_kzalloc(chip-dev, sizeof(*lookup) * chip-npwm,
 GFP_KERNEL);
   pdata = devm_kzalloc(chip-dev, sizeof(*pdata), GFP_KERNEL);
   gpos = devm_kzalloc(chip-dev, sizeof(*gpos) * chip-npwm,
   GFP_KERNEL);
 
   pdata-gpos = gpos;
   pdata-num_gpos = chip-npwm;
   pdata-gpio_base = -1;
 
   pdev = platform_device_alloc(pwm-gpo, chip-base);
   pdev-dev.parent = chip-dev;
 
   sprintf(gpodev_name, pwm-gpo.%d, chip-base);
   for (i = 0; i  chip-npwm; i++) {
   struct gpio_pwm *gpo = gpos[i];
   struct pwm_lookup *pl = lookup[i];
   char con_id[15];
 
   sprintf(con_id, pwm-gpo.%d, chip-base + i);
 
   /* prepare GPO information */
   gpo-pwm_period_ns = period_ns;
   gpo-name = kmemdup(con_id, sizeof(con_id), GFP_KERNEL);;
 
   /* prepare pwm lookup table */
   pl-provider = dev_name(chip-dev);
   pl-index = i;
   pl-dev_id = kmemdup(gpodev_name, sizeof(gpodev_name),
GFP_KERNEL);
   pl-con_id = kmemdup(con_id, sizeof(con_id), GFP_KERNEL);
   }
 
   platform_device_add_data(pdev, pdata, sizeof(*pdata));
   pwm_add_table(lookup, chip-npwm);
   platform_device_add(pdev);
 }
 

The whole pwm lookup table creation is a bit ugly. I wonder if we can somehow
bypass this by using pwm_request_from_chip directly in the pwm-gpo driver.

- Lars

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3 4/8] MFD: ti_am335x_tscadc: add device tree binding information

2013-01-19 Thread Lars-Peter Clausen
Hi,

On 01/18/2013 11:48 AM, Patil, Rachna wrote:
 From: Patil, Rachna rac...@ti.com
 
 Signed-off-by: Patil, Rachna rac...@ti.com
 ---
  .../devicetree/bindings/mfd/ti_am335x_tscadc.txt   |   35 
 
  1 file changed, 35 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/mfd/ti_am335x_tscadc.txt
 
 diff --git a/Documentation/devicetree/bindings/mfd/ti_am335x_tscadc.txt 
 b/Documentation/devicetree/bindings/mfd/ti_am335x_tscadc.txt
 new file mode 100644
 index 000..c13c492
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/mfd/ti_am335x_tscadc.txt
 @@ -0,0 +1,35 @@
 +Texas Instruments - TSC / ADC multi-functional device
 +
 +ti_tscadc is a multi-function device with touchscreen and ADC on chip.
 +This document describes the binding for mfd device.
 +
 +Required properties:
 +- compatible: ti,ti-tscadc
 +- reg: Specifies the address of MFD block
 +- interrupts: IRQ line connected to the main SoC
 +- interrupt-parent: The parent interrupt controller

The subnodes and their properties also need documentation.

 +
 +Optional properties:
 +- ti,hwmods: Hardware information related to TSC/ADC MFD device
 +
 +Example:
 +
 + tscadc: tscadc@44e0d000 {
 + compatible = ti,ti-tscadc;
 + reg = 0x44e0d000 0x1000;
 +
 + interrupt-parent = intc;
 + interrupts = 16;
 + ti,hwmods = adc_tsc;
 +
 + tsc {
 + wires = 4;
 + x-plate-resistance = 200;
 + steps-to-configure = 5;
 + wire-config = 0x00 0x11 0x22 0x33;

Non-standard properties should have a vendor prefix.

 + };
 +
 + adc {
 + adc-channels = 4;
 + };
 + };

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH] ASoC: omap: Call omap_mcbsp_set_threshold() from mcbsp hw_params

2013-03-25 Thread Lars-Peter Clausen
The omap PCM driver provides a set_threshold callback which gets called by the
PCM driver when either playback or capture is started. The only DAI driver which
sets this callback is the mcbsp driver. This patch removes the callback from the
PCM driver and moves the invocation of the omap_mcbsp_set_threshold() function
to the mcbsp hw_params callback since this is the only place where the threshold
size can change. Doing so allows us to use the default dmaengine PCM trigger
callback in the omap PCM driver.

Signed-off-by: Lars-Peter Clausen l...@metafoo.de

---
Only compile tested, but I don't see why it should not work

Signed-off-by: Lars-Peter Clausen l...@metafoo.de
---
 sound/soc/omap/omap-mcbsp.c | 12 +---
 sound/soc/omap/omap-pcm.c   | 33 +
 sound/soc/omap/omap-pcm.h   |  1 -
 3 files changed, 6 insertions(+), 40 deletions(-)

diff --git a/sound/soc/omap/omap-mcbsp.c b/sound/soc/omap/omap-mcbsp.c
index 8d2defd..406fc87 100644
--- a/sound/soc/omap/omap-mcbsp.c
+++ b/sound/soc/omap/omap-mcbsp.c
@@ -62,24 +62,22 @@ enum {
  * Stream DMA parameters. DMA request line and port address are set runtime
  * since they are different between OMAP1 and later OMAPs
  */
-static void omap_mcbsp_set_threshold(struct snd_pcm_substream *substream)
+static void omap_mcbsp_set_threshold(struct snd_pcm_substream *substream,
+   unsigned int packet_size)
 {
struct snd_soc_pcm_runtime *rtd = substream-private_data;
struct snd_soc_dai *cpu_dai = rtd-cpu_dai;
struct omap_mcbsp *mcbsp = snd_soc_dai_get_drvdata(cpu_dai);
-   struct omap_pcm_dma_data *dma_data;
int words;
 
-   dma_data = snd_soc_dai_get_dma_data(rtd-cpu_dai, substream);
-
/*
 * Configure McBSP threshold based on either:
 * packet_size, when the sDMA is in packet mode, or based on the
 * period size in THRESHOLD mode, otherwise use McBSP threshold = 1
 * for mono streams.
 */
-   if (dma_data-packet_size)
-   words = dma_data-packet_size;
+   if (packet_size)
+   words = packet_size;
else
words = 1;
 
@@ -245,7 +243,6 @@ static int omap_mcbsp_dai_hw_params(struct 
snd_pcm_substream *substream,
return -EINVAL;
}
if (mcbsp-pdata-buffer_size) {
-   dma_data-set_threshold = omap_mcbsp_set_threshold;
if (mcbsp-dma_op_mode == MCBSP_DMA_MODE_THRESHOLD) {
int period_words, max_thrsh;
int divider = 0;
@@ -276,6 +273,7 @@ static int omap_mcbsp_dai_hw_params(struct 
snd_pcm_substream *substream,
/* Use packet mode for non mono streams */
pkt_size = channels;
}
+   omap_mcbsp_set_threshold(substream, pkt_size);
}
 
dma_data-packet_size = pkt_size;
diff --git a/sound/soc/omap/omap-pcm.c b/sound/soc/omap/omap-pcm.c
index c722c2e..1626fb7 100644
--- a/sound/soc/omap/omap-pcm.c
+++ b/sound/soc/omap/omap-pcm.c
@@ -129,37 +129,6 @@ static int omap_pcm_hw_free(struct snd_pcm_substream 
*substream)
return 0;
 }
 
-static int omap_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
-{
-   struct snd_soc_pcm_runtime *rtd = substream-private_data;
-   struct omap_pcm_dma_data *dma_data;
-   int ret = 0;
-
-   dma_data = snd_soc_dai_get_dma_data(rtd-cpu_dai, substream);
-
-   switch (cmd) {
-   case SNDRV_PCM_TRIGGER_START:
-   case SNDRV_PCM_TRIGGER_RESUME:
-   case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
-   /* Configure McBSP internal buffer usage */
-   if (dma_data-set_threshold)
-   dma_data-set_threshold(substream);
-   break;
-
-   case SNDRV_PCM_TRIGGER_STOP:
-   case SNDRV_PCM_TRIGGER_SUSPEND:
-   case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
-   break;
-   default:
-   ret = -EINVAL;
-   }
-
-   if (ret == 0)
-   ret = snd_dmaengine_pcm_trigger(substream, cmd);
-
-   return ret;
-}
-
 static snd_pcm_uframes_t omap_pcm_pointer(struct snd_pcm_substream *substream)
 {
snd_pcm_uframes_t offset;
@@ -208,7 +177,7 @@ static struct snd_pcm_ops omap_pcm_ops = {
.ioctl  = snd_pcm_lib_ioctl,
.hw_params  = omap_pcm_hw_params,
.hw_free= omap_pcm_hw_free,
-   .trigger= omap_pcm_trigger,
+   .trigger= snd_dmaengine_pcm_trigger,
.pointer= omap_pcm_pointer,
.mmap   = omap_pcm_mmap,
 };
diff --git a/sound/soc/omap/omap-pcm.h b/sound/soc/omap/omap-pcm.h
index cabe74c..39e6e45 100644
--- a/sound/soc/omap/omap-pcm.h
+++ b/sound/soc/omap/omap-pcm.h
@@ -31,7 +31,6 @@ struct omap_pcm_dma_data {
char*name;  /* stream identifier */
int dma_req;/* DMA request line */
unsigned long   port_addr

[PATCH] OMAPDSS: nec-nl8048 panel: Use dev_pm_ops

2013-04-07 Thread Lars-Peter Clausen
Use dev_pm_ops instead of the deprecated legacy suspend/resume callbacks.

Signed-off-by: Lars-Peter Clausen l...@metafoo.de
---
 .../video/omap2/displays/panel-nec-nl8048hl11-01b.c  | 20 
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/video/omap2/displays/panel-nec-nl8048hl11-01b.c 
b/drivers/video/omap2/displays/panel-nec-nl8048hl11-01b.c
index 1e0097d..20c3cd9 100644
--- a/drivers/video/omap2/displays/panel-nec-nl8048hl11-01b.c
+++ b/drivers/video/omap2/displays/panel-nec-nl8048hl11-01b.c
@@ -242,16 +242,22 @@ static int nec_8048_spi_remove(struct spi_device *spi)
return 0;
 }
 
-static int nec_8048_spi_suspend(struct spi_device *spi, pm_message_t mesg)
+#ifdef CONFIG_PM_SLEEP
+
+static int nec_8048_spi_suspend(struct device *dev)
 {
+   struct spi_device *spi = to_spi_device(dev);
+
nec_8048_spi_send(spi, 2, 0x01);
mdelay(40);
 
return 0;
 }
 
-static int nec_8048_spi_resume(struct spi_device *spi)
+static int nec_8048_spi_resume(struct device *dev)
 {
+   struct spi_device *spi = to_spi_device(dev);
+
/* reinitialize the panel */
spi_setup(spi);
nec_8048_spi_send(spi, 2, 0x00);
@@ -260,14 +266,20 @@ static int nec_8048_spi_resume(struct spi_device *spi)
return 0;
 }
 
+static SIMPLE_DEV_PM_OPS(nec_8048_spi_pm_ops, nec_8048_spi_suspend,
+   nec_8048_spi_resume);
+#define NEC_8048_SPI_PM_OPS (nec_8048_spi_pm_ops)
+#else
+#define NEC_8048_SPI_PM_OPS NULL
+#endif
+
 static struct spi_driver nec_8048_spi_driver = {
.probe  = nec_8048_spi_probe,
.remove = nec_8048_spi_remove,
-   .suspend= nec_8048_spi_suspend,
-   .resume = nec_8048_spi_resume,
.driver = {
.name   = nec_8048_spi,
.owner  = THIS_MODULE,
+   .pm = NEC_8048_SPI_PM_OPS,
},
 };
 
-- 
1.8.0

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: am335x: TSC ADC reworking including DT pieces, take 4

2013-06-11 Thread Lars-Peter Clausen
On 06/11/2013 02:05 PM, Lee Jones wrote:
 I believe the whole thing should go via the MFD tree. It touches also
 input  iio subsystem. I collected ACKs where I got some in the meantime.

 I added Lee Jones because I hear no sign of life from Samuel.
 
 Unfortunately I can't be of any added help here, as I also send my
 pull-requests though Sam.
 
 Sorry I couldn't be of any more use Sebastian.
 

It sometimes takes him a week or two to respond, but Samuel is pretty reliable
when it comes to merging patches, so I wouldn't worry about it.

- Lars
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: am335x: TSC ADC reworking including DT pieces, take 4

2013-06-11 Thread Lars-Peter Clausen
On 06/11/2013 06:27 PM, Jonathan Cameron wrote:
 
 
 Samuel Ortiz sa...@linux.intel.com wrote:
 
 Hi Dmitry,

 On Tue, Jun 11, 2013 at 09:04:16AM -0700, Dmitry Torokhov wrote:
 On Tue, Jun 11, 2013 at 04:23:58PM +0200, Samuel Ortiz wrote:
 Hi Sebastian,

 On Tue, Jun 11, 2013 at 01:30:46PM +0200, Sebastian Andrzej Siewior
 wrote:
 I believe the whole thing should go via the MFD tree. It touches
 also
 input  iio subsystem. I collected ACKs where I got some in the
 meantime.
 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.

 Then, this is a pretty big patchset, with iio, input and mfd all
 mixed
 together and it is likely to create merge conflicts.
 From what I can see from it, and please correct me if I'm
 wrong, the iio and input changes depend on the mfd ones, and not
 the
 other way around. If that's so, I'm going to ask you to reshuffle
 your
 patch set and separate the MFD changes from the iio and input ones.
 I'll
 take the MFD ones and will create an immutable branch for Jonathan
 and
 Dmitry to pull from and apply the iio and input changes on top of
 it.
 Merge conflicts should be mostly avoided that way.

 I purposely left this driver alone as I expected it would be merged
 through MFD tree, so there should not be any merging issues on my
 side.
 Thanks for the notice.
 Jonathan, can you guarantee the same for the iio parts ?
 I have also been avoiding taking any of these and there are unlikely to be 
 any iio wide changes merging at this stage in cycle.  Hence these going 
 through MFD is best plan.
 
 Lars raised a couple of issues so would be best to wait for his ack if he 
 hasn't already looked at this revision.

The IIO bits look fine to me in this version.

- Lars

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3 2/2] iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver

2013-07-12 Thread Lars-Peter Clausen


A couple of comments inline.

On 07/12/2013 09:18 AM, Oleksandr Kozaruk wrote:

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index ab0767e6..87d699e 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -157,4 +157,12 @@ config VIPERBOARD_ADC
  Say yes here to access the ADC part of the Nano River
  Technologies Viperboard.

+config TWL6030_GPADC
+   tristate TWL6030 GPADC (General Purpose A/D Convertor) Support
+   depends on TWL4030_CORE
+   default n
+   help
+ Say yes here if you want support for the TWL6030 General Purpose
+ A/D Convertor.
+


Keep it in alphabetical order


  endmenu
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 0a825be..8b05633 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -17,3 +17,4 @@ obj-$(CONFIG_MAX1363) += max1363.o
  obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
  obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
  obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
+obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o


Same here.


diff --git a/drivers/iio/adc/twl6030-gpadc.c b/drivers/iio/adc/twl6030-gpadc.c
new file mode 100644
index 000..6ceb789
--- /dev/null
+++ b/drivers/iio/adc/twl6030-gpadc.c
@@ -0,0 +1,1019 @@

[...]

+static u8 twl6032_channel_to_reg(int channel)
+{
+   return TWL6032_GPADC_GPCH0_LSB;


There is more than one channel, isn't there?

[...]
 +static int twl6030_gpadc_read_raw(struct iio_dev *indio_dev,
 +   const struct iio_chan_spec *chan,
 +   int *val, int *val2, long mask)
 +{
 +  struct twl6030_gpadc_data *gpadc = iio_priv(indio_dev);
 +  int ret = -EINVAL;
 +
 +  mutex_lock(gpadc-lock);
 +
 +  ret = gpadc-pdata-start_conversion(chan-channel);
 +  if (ret) {
 +  dev_err(gpadc-dev, failed to start conversion\n);
 +  goto err;
 +  }
 +  /* wait for conversion to complete */
 +  wait_for_completion_interruptible_timeout(gpadc-irq_complete,
 +  msecs_to_jiffies(5000));

wait_for_completion_interruptible_timeout() can return either a negative error 
code if it was interrupted, 0 if it timed out, or a positive value if it was 
successfully completed. You need to handle all three cases. Have a look at 
other existing drivers to see how to do this properly.


 +
 +  switch (mask) {
 +  case IIO_CHAN_INFO_RAW:
 +  ret = twl6030_gpadc_get_raw(gpadc, chan-channel, val);
 +  ret = ret ? -EIO : IIO_VAL_INT;
 +  break;
 +
 +  case IIO_CHAN_INFO_PROCESSED:
 +  ret = twl6030_gpadc_get_processed(gpadc, chan-channel, val);
 +  ret = ret ? -EIO : IIO_VAL_INT;
 +  break;
 +
 +  default:
 +  break;
 +  }
 +err:
 +  mutex_unlock(gpadc-lock);
 +
 +  return ret;
 +}


+}
+

[...]

+static int twl6030_gpadc_probe(struct platform_device *pdev)
+{
+   struct device *dev = pdev-dev;
+   struct twl6030_gpadc_data *gpadc;
+   const struct twl6030_gpadc_platform_data *pdata;
+   const struct of_device_id *match;
+   struct iio_dev *indio_dev;
+   int irq;
+   int ret = 0;
+
+   match = of_match_device(of_match_ptr(of_twl6030_match_tbl), dev);
+   pdata = match ? match-data : dev-platform_data;
+
+   if (!pdata)
+   return -EINVAL;
+
+   indio_dev = iio_device_alloc(sizeof(struct twl6030_gpadc_data));


sizeof(*gpadc)


+   if (!indio_dev) {
+   dev_err(dev, failed allocating iio device\n);
+   ret = -ENOMEM;
+   }
+
+   gpadc = iio_priv(indio_dev);
+
+   gpadc-twl6030_cal_tbl = devm_kzalloc(dev,
+   sizeof(struct twl6030_chnl_calib) *
+   pdata-nchannels, GFP_KERNEL);
+   if (!gpadc-twl6030_cal_tbl)
+   goto err;
+
+   gpadc-dev = dev;
+   gpadc-pdata = pdata;
+
+   platform_set_drvdata(pdev, indio_dev);
+   mutex_init(gpadc-lock);
+   init_completion(gpadc-irq_complete);
+
+   ret = pdata-calibrate(gpadc);
+   if (ret  0) {
+   dev_err(pdev-dev, failed to read calibration registers\n);
+   goto err;
+   }
+
+   irq = platform_get_irq(pdev, 0);
+   if (irq  0) {
+   dev_err(pdev-dev, failed to get irq\n);
+   goto err;
+   }
+
+   ret = devm_request_threaded_irq(dev, irq, NULL,
+   twl6030_gpadc_irq_handler,
+   IRQF_ONESHOT, twl6030_gpadc, gpadc);


You access memory in the interrupt handler which is freed before the interrupt 
handler is freed.



+   if (ret) {
+   dev_dbg(pdev-dev, could not request irq\n);
+   goto err;
+   }
+
+   ret = twl6030_gpadc_enable_irq(TWL6030_GPADC_RT_SW1_EOC_MASK);
+   if (ret  0) {
+   dev_err(pdev-dev, failed to enable GPADC interrupt\n);
+   goto err;
+   

Re: [PATCH v3 2/2] iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver

2013-07-15 Thread Lars-Peter Clausen
On 07/15/2013 01:09 PM, Kozaruk, Oleksandr wrote:
[...]
 
 + ret = devm_request_threaded_irq(dev, irq, NULL,
 + twl6030_gpadc_irq_handler,
 + IRQF_ONESHOT, twl6030_gpadc, gpadc);

 You access memory in the interrupt handler which is freed before the 
 interrupt
 handler is freed.
 Thanks for pointing this. devm_* will free memory for irq after the driver
 is removed and memory for the device is freed. I took me awhile to understand
 this. Is there going to be something like devm_iio_device_alloc? whould it be 
 helpfull?
 

Yes, I think it certainly makes sense to add a devm_iio_device_alloc(), care
to send a patch?
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3 2/2] iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver

2013-07-15 Thread Lars-Peter Clausen
On 07/15/2013 01:56 PM, Grygorii Strashko wrote:
 Hi All,
 
 I have a question regarding this patch and IIO in general
 - Does IIO provide sync mechanism with system wide suspend/resume or this
 should be handled by each driver itself?
 
 What if during system suspend iio_read_channel_raw() (or any other consumer
 API) will be called after gpadc driver have been suspended already? (I did
 some investigation and seems it's possible - Am I right?)
 
 If no, could info_exist_lock be reused for such purposes?

I think you can use the mlock for this purpose. Usually you'd have a flag in
your driver struct which you'd set to true in suspend and to false in
resume. And in the read_raw callback you'd check that flag before accessing
the hardware. If it turns out that this is a common pattern it probably
makes sense to add infrastructure for this to the IIO core. Something along
the lines of:

static int drv_suspend(...)
{
...
iio_device_set_suspended(iio_dev, true);
...
}

static int drv_suspend(...)
{
...
iio_device_set_suspended(iio_dev, false);
...
}

and then have the IIO core check that flag before calling the read_raw callback.

- Lars

 
 Regards,
 -grygorii
 
 
 On 07/12/2013 10:56 PM, Lars-Peter Clausen wrote:

 A couple of comments inline.

 On 07/12/2013 09:18 AM, Oleksandr Kozaruk wrote:
 diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
 index ab0767e6..87d699e 100644
 --- a/drivers/iio/adc/Kconfig
 +++ b/drivers/iio/adc/Kconfig
 @@ -157,4 +157,12 @@ config VIPERBOARD_ADC
 Say yes here to access the ADC part of the Nano River
 Technologies Viperboard.

 +config TWL6030_GPADC
 +tristate TWL6030 GPADC (General Purpose A/D Convertor) Support
 +depends on TWL4030_CORE
 +default n
 +help
 +  Say yes here if you want support for the TWL6030 General Purpose
 +  A/D Convertor.
 +

 Keep it in alphabetical order

   endmenu
 diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
 index 0a825be..8b05633 100644
 --- a/drivers/iio/adc/Makefile
 +++ b/drivers/iio/adc/Makefile
 @@ -17,3 +17,4 @@ obj-$(CONFIG_MAX1363) += max1363.o
   obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
   obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
   obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
 +obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o

 Same here.

 diff --git a/drivers/iio/adc/twl6030-gpadc.c
 b/drivers/iio/adc/twl6030-gpadc.c
 new file mode 100644
 index 000..6ceb789
 --- /dev/null
 +++ b/drivers/iio/adc/twl6030-gpadc.c
 @@ -0,0 +1,1019 @@
 [...]
 +static u8 twl6032_channel_to_reg(int channel)
 +{
 +return TWL6032_GPADC_GPCH0_LSB;

 There is more than one channel, isn't there?

 [...]
   +static int twl6030_gpadc_read_raw(struct iio_dev *indio_dev,
   + const struct iio_chan_spec *chan,
   + int *val, int *val2, long mask)
   +{
   +struct twl6030_gpadc_data *gpadc = iio_priv(indio_dev);
   +int ret = -EINVAL;
   +
   +mutex_lock(gpadc-lock);
   +
   +ret = gpadc-pdata-start_conversion(chan-channel);
   +if (ret) {
   +dev_err(gpadc-dev, failed to start conversion\n);
   +goto err;
   +}
   +/* wait for conversion to complete */
   +wait_for_completion_interruptible_timeout(gpadc-irq_complete,
   +msecs_to_jiffies(5000));

 wait_for_completion_interruptible_timeout() can return either a negative
 error code if it was interrupted, 0 if it timed out, or a positive value
 if it was successfully completed. You need to handle all three cases.
 Have a look at other existing drivers to see how to do this properly.

   +
   +switch (mask) {
   +case IIO_CHAN_INFO_RAW:
   +ret = twl6030_gpadc_get_raw(gpadc, chan-channel, val);
   +ret = ret ? -EIO : IIO_VAL_INT;
   +break;
   +
   +case IIO_CHAN_INFO_PROCESSED:
   +ret = twl6030_gpadc_get_processed(gpadc, chan-channel, val);
   +ret = ret ? -EIO : IIO_VAL_INT;
   +break;
   +
   +default:
   +break;
   +}
   +err:
   +mutex_unlock(gpadc-lock);
   +
   +return ret;
   +}

 +}
 +
 [...]
 +static int twl6030_gpadc_probe(struct platform_device *pdev)
 +{
 +struct device *dev = pdev-dev;
 +struct twl6030_gpadc_data *gpadc;
 +const struct twl6030_gpadc_platform_data *pdata;
 +const struct of_device_id *match;
 +struct iio_dev *indio_dev;
 +int irq;
 +int ret = 0;
 +
 +match = of_match_device(of_match_ptr(of_twl6030_match_tbl), dev);
 +pdata = match ? match-data : dev-platform_data;
 +
 +if (!pdata)
 +return -EINVAL;
 +
 +indio_dev = iio_device_alloc(sizeof(struct twl6030_gpadc_data));

 sizeof(*gpadc)

 +if (!indio_dev) {
 +dev_err(dev, failed allocating iio device\n);
 +ret = -ENOMEM;
 +}
 +
 +gpadc = iio_priv(indio_dev);
 +
 +gpadc-twl6030_cal_tbl = devm_kzalloc(dev,
 +sizeof(struct

Re: [PATCH v5 1/2] ARM: dts: twl: Add GPADC data to device tree

2013-07-17 Thread Lars-Peter Clausen
On 07/17/2013 04:33 PM, Sergei Shtylyov wrote:
 Hello.
 
 On 17-07-2013 15:12, Oleksandr Kozaruk wrote:
 
 GPADC is the general purpose ADC present on twl6030.
 The dt data is interrupt used to trigger end of ADC
 conversion.
 
 Signed-off-by: Oleksandr Kozaruk oleksandr.koza...@ti.com
 ---
   arch/arm/boot/dts/twl6030.dtsi | 6 ++
   1 file changed, 6 insertions(+)
 
 diff --git a/arch/arm/boot/dts/twl6030.dtsi b/arch/arm/boot/dts/twl6030.dtsi
 index 2e3bd31..322aa8e 100644
 --- a/arch/arm/boot/dts/twl6030.dtsi
 +++ b/arch/arm/boot/dts/twl6030.dtsi
 @@ -103,4 +103,10 @@
   compatible = ti,twl6030-pwmled;
   #pwm-cells = 2;
   };
 +
 +adc: twl6030_gpadc {
 
I was talking about the device name, not label. The twl6030_gpadc part.

The compatible property should also be: 'twl6030-gpadc' instead of
'twl6030_gpadc' and you need to add documentation for it.

- Lars
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3 2/2] iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver

2013-07-17 Thread Lars-Peter Clausen
On 07/17/2013 03:45 PM, Oleksandr Kozaruk wrote:
 On Mon, Jul 15, 2013 at 01:33:53PM +0200, Lars-Peter Clausen wrote:
 On 07/15/2013 01:09 PM, Kozaruk, Oleksandr wrote:
 [...]
 
  + ret = devm_request_threaded_irq(dev, irq, NULL,
  + twl6030_gpadc_irq_handler,
  + IRQF_ONESHOT, twl6030_gpadc, gpadc);
 
  You access memory in the interrupt handler which is freed before the
 interrupt
  handler is freed.
  Thanks for pointing this. devm_* will free memory for irq after the driver
  is removed and memory for the device is freed. I took me awhile to
 understand
  this. Is there going to be something like devm_iio_device_alloc? whould
 it be helpfull?
 

 Yes, I think it certainly makes sense to add a devm_iio_device_alloc(), care
 to send a patch?
 
 Anything like this? (of course it's not a patch)
 

No. I think you can for example use devm_regulator_get() as a template. But
instead of regulator_get() and regulator_put() use iio_device_alloc() and
iio_device_free().

 struct iio_dev *devm_iio_device_alloc(struct device *dev, int sizeof_priv)
 {
 struct iio_dev *indio_dev;
 size_t alloc_size;
 
 alloc_size = sizeof(struct iio_dev);
 if (sizeof_priv) {
 alloc_size = ALIGN(alloc_size, IIO_ALIGN);
 alloc_size += sizeof_priv;
 }
 /* ensure 32-byte alignment of whole construct ? */
 alloc_size += IIO_ALIGN - 1;
 
 indio_dev = devm_kzalloc(dev, alloc_size, GFP_KERNEL);
 if (indio_dev) {
 indio_dev-dev.groups = indio_dev-groups;
 indio_dev-dev.type = iio_device_type;
 indio_dev-dev.bus = iio_bus_type;
 device_initialize(indio_dev-dev);
 dev_set_drvdata(indio_dev-dev, (void *)indio_dev);
 mutex_init(indio_dev-mlock);
 mutex_init(indio_dev-info_exist_lock);
 INIT_LIST_HEAD(indio_dev-channel_attr_list);
 
 indio_dev-id = ida_simple_get(iio_ida, 0, 0, GFP_KERNEL);
 if (indio_dev-id  0) {
 /* cannot use a dev_err as the name isn't available */
 printk(KERN_ERR Failed to get id\n);
 kfree(dev);
 return NULL;
 }
 dev_set_name(indio_dev-dev, iio:device%d, indio_dev-id);
 INIT_LIST_HEAD(indio_dev-buffer_list);
 }
 
 return indio_dev;
 }
 EXPORT_SYMBOL(devm_iio_device_alloc);
 
 Regards,
 OK

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v5 2/2] iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver

2013-07-17 Thread Lars-Peter Clausen
On 07/17/2013 01:12 PM, Oleksandr Kozaruk wrote:
 The GPADC is general purpose ADC found on TWL6030, and TWL6032 PMIC,
 known also as Phoenix and PhoenixLite.
 
 The TWL6030 and TWL6032 have GPADC with 17 and 19 channels
 respectively. Some channels have current source and are used for
 measuring voltage drop on resistive load for detecting battery ID
 resistance, or measuring voltage drop on NTC resistors for external
 temperature measurements. Some channels measure voltage, (i.e. battery
 voltage), and have voltage dividers, thus, capable to scale voltage.
 Some channels are dedicated for measuring die temperature.
 
 Some channels are calibrated in 2 points, having offsets from ideal
 values kept in trim registers. This is used to correct measurements.
 
 The differences between GPADC in TWL6030 and TWL6032:
 - 10 bit vs 12 bit ADC;
 - 17 vs 19 channels;
 - channels have different purpose(i.e. battery voltage
   channel 8 vs channel 18);
 - trim values are interpreted differently.
 
 Based on the driver patched from Balaji TK, Graeme Gregory, Ambresh K,
 Girish S Ghongdemath.
 
 Signed-off-by: Balaji T K balaj...@ti.com
 Signed-off-by: Graeme Gregory g...@slimlogic.co.uk
 Signed-off-by: Oleksandr Kozaruk oleksandr.koza...@ti.com

Almost there (I hope). But please run scripts/checkpatch, make C=2 and make
coccicheck on your driver and fix all errors those tools give you before
submitting the driver upstream.

[...]
 +static int twl6030_gpadc_get_raw(struct twl6030_gpadc_data *gpadc,
 + int channel, int *res)
 +{
 + u8 reg = gpadc-pdata-channel_to_reg(channel);
 + __le16 val;
 + int raw_code;
 + int ret;
 +
 + ret = twl6030_gpadc_read(reg, (u8 *)val);
 + if (ret) {
 + dev_dbg(gpadc-dev, unable to read register 0x%X\n, reg);
 + return ret;
 + }
 +
 + raw_code = le16_to_cpup(val);

raw_code = le16_to_cpu(val)

 + dev_dbg(gpadc-dev, GPADC raw code: %d, raw_code);
 +
 + if (twl6030_channel_calibrated(gpadc-pdata, channel))
 + *res = twl6030_gpadc_make_correction(gpadc, channel, raw_code);
 + else
 + *res = raw_code;
 +
 + return ret;
 +}
 +
[...]
 +static int twl6030_gpadc_read_raw(struct iio_dev *indio_dev,
 +  const struct iio_chan_spec *chan,
 +  int *val, int *val2, long mask)
 +{
 + struct twl6030_gpadc_data *gpadc = iio_priv(indio_dev);
 + int ret = -EINVAL;
 + long timeout;
 +
 + mutex_lock(gpadc-lock);
 +
 + ret = gpadc-pdata-start_conversion(chan-channel);
 + if (ret) {
 + dev_err(gpadc-dev, failed to start conversion\n);
 + goto err;
 + }
 + /* wait for conversion to complete */
 + timeout = wait_for_completion_interruptible_timeout(
 + gpadc-irq_complete, msecs_to_jiffies(5000));
 + if (!timeout)
 + return -ETIMEDOUT;
 + else if (timeout  0)
 + return EINTR;

You still hold the mutex, try this instead:

ret = wait_for_
if (ret == 0)
ret = -ETIMEDOUT;
if (ret  0)
goto err;


 +
 + switch (mask) {
 + case IIO_CHAN_INFO_RAW:
 + ret = twl6030_gpadc_get_raw(gpadc, chan-channel, val);
 + ret = ret ? -EIO : IIO_VAL_INT;
 + break;
 +
 + case IIO_CHAN_INFO_PROCESSED:
 + ret = twl6030_gpadc_get_processed(gpadc, chan-channel, val);
 + ret = ret ? -EIO : IIO_VAL_INT;
 + break;
 +
 + default:
 + break;
 + }
 +err:
 + mutex_unlock(gpadc-lock);
 +
 + return ret;
 +}
[...]
 +
 +static int twl6032_calibration(struct twl6030_gpadc_data *gpadc)
 +{
 + int chn, d1 = 0, d2 = 0, temp;
 + u8 trim_regs[17];
 + int ret;
 +
 + ret = twl_i2c_read(TWL6030_MODULE_ID2, trim_regs + 1,
 + TWL6030_GPADC_TRIM1, 16);
 + if (ret  0) {
 + dev_err(gpadc-dev, calibration failed\n);
 + return ret;
 + }
 +
 + /*
 +  * Loop to calculate the value needed for returning voltages from
 +  * GPADC not values.
 +  *
 +  * gain is calculated to 3 decimal places fixed point.
 +  */
 + for (chn = 0; chn  TWL6032_GPADC_MAX_CHANNELS; chn++) {
 +
 + switch (chn) {
 + case 0:
 + case 1:
 + case 2:
 + case 3:
 + case 4:
 + case 5:
 + case 6:
 + case 11:
 + case 12:
 + case 13:
 + case 14:
 + /* D1 */
 + d1 = (trim_regs[3]  0x1F)  2;
 + d1 |= (trim_regs[1]  0x06)  1;
 + if (trim_regs[1]  0x01)
 + d1 = -d1;
 +
 + /* D2 */
 + d2 = (trim_regs[4]  0x3F)  2;
 + d2 |= (trim_regs[2]  0x06)  1;
 + if 

Re: [PATCH v5 2/2] iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver

2013-07-18 Thread Lars-Peter Clausen
On 07/18/2013 10:36 AM, Oleksandr Kozaruk wrote:
 Hello Lars,
 
 On Wed, Jul 17, 2013 at 9:04 PM, Lars-Peter Clausen l...@metafoo.de wrote:
 +static int twl6032_calibration(struct twl6030_gpadc_data *gpadc)
 +{
 + int chn, d1 = 0, d2 = 0, temp;
 + u8 trim_regs[17];
 + int ret;
 +
 + ret = twl_i2c_read(TWL6030_MODULE_ID2, trim_regs + 1,
 + TWL6030_GPADC_TRIM1, 16);
 + if (ret  0) {
 + dev_err(gpadc-dev, calibration failed\n);
 + return ret;
 + }
 +
 + /*
 +  * Loop to calculate the value needed for returning voltages from
 +  * GPADC not values.
 +  *
 +  * gain is calculated to 3 decimal places fixed point.
 +  */
 + for (chn = 0; chn  TWL6032_GPADC_MAX_CHANNELS; chn++) {
 +
 + switch (chn) {
 + case 0:
 + case 1:
 + case 2:
 + case 3:
 + case 4:
 + case 5:
 + case 6:
 + case 11:
 + case 12:
 + case 13:
 + case 14:
 + /* D1 */
 + d1 = (trim_regs[3]  0x1F)  2;
 + d1 |= (trim_regs[1]  0x06)  1;
 + if (trim_regs[1]  0x01)
 + d1 = -d1;
 +
 + /* D2 */
 + d2 = (trim_regs[4]  0x3F)  2;
 + d2 |= (trim_regs[2]  0x06)  1;
 + if (trim_regs[2]  0x01)
 + d2 = -d2;
 + break;
 + case 8:
 + /* D1 */
 + temp = (trim_regs[3]  0x1F)  2;
 + temp |= (trim_regs[1]  0x06)  1;
 + if (trim_regs[1]  0x01)
 + temp = -temp;
 +
 + d1 = (trim_regs[8]  0x18)  1;
 + d1 |= (trim_regs[7]  0x1E)  1;
 + if (trim_regs[7]  0x01)
 + d1 = -d1;
 +
 + d1 += temp;
 +
 + /* D2 */
 + temp = (trim_regs[4]  0x3F)  2;
 + temp |= (trim_regs[2]  0x06)  1;
 + if (trim_regs[2]  0x01)
 + temp = -temp;
 +
 + d2 = (trim_regs[10]  0x1F)  2;
 + d2 |= (trim_regs[8]  0x06)  1;
 + if (trim_regs[8]  0x01)
 + d2 = -d2;
 +
 + d2 += temp;
 + break;
 + case 9:
 + /* D1 */
 + temp = (trim_regs[3]  0x1F)  2;
 + temp |= (trim_regs[1]  0x06)  1;
 + if (trim_regs[1]  0x01)
 + temp = -temp;
 +
 + d1 = (trim_regs[14]  0x18)  1;
 + d1 |= (trim_regs[12]  0x1E)  1;
 + if (trim_regs[12]  0x01)
 + d1 = -d1;
 +
 + d1 += temp;
 +
 + /* D2 */
 + temp = (trim_regs[4]  0x3F)  2;
 + temp |= (trim_regs[2]  0x06)  1;
 + if (trim_regs[2]  0x01)
 + temp = -temp;
 +
 + d2 = (trim_regs[16]  0x1F)  2;
 + d2 |= (trim_regs[14]  0x06)  1;
 + if (trim_regs[14]  0x01)
 + d2 = -d2;
 +
 + d2 += temp;
 + case 10:
 + /* D1 */
 + d1 = (trim_regs[11]  0x0F)  3;
 + d1 |= (trim_regs[9]  0x0E)  1;
 + if (trim_regs[9]  0x01)
 + d1 = -d1;
 +
 + /* D2 */
 + d2 = (trim_regs[15]  0x0F)  3;
 + d2 |= (trim_regs[13]  0x0E)  1;
 + if (trim_regs[13]  0x01)
 + d2 = -d2;
 + break;
 + case 7:
 + case 18:
 + /* D1 */
 + temp = (trim_regs[3]  0x1F)  2;
 + temp |= (trim_regs[1]  0x06)  1;
 + if (trim_regs[1]  0x01)
 + temp = -temp;
 +
 + d1 = (trim_regs[5]  0x7E)  1;
 + if (trim_regs[5]  0x01)
 + d1 = -d1;
 +.
 + d1 += temp;
 +
 + /* D2 */
 + temp = (trim_regs[4]  0x3F)  2;
 + temp |= (trim_regs[2]  0x06)  1;
 + if (trim_regs[2]  0x01)
 + temp = -temp;
 +
 + d2 = (trim_regs[6]  0xFE)  1;
 + if (trim_regs[6]  0x01)
 + d2 = -d2;
 +
 + d2 += temp;
 + break;


 There is quite a bit of copy paste

Re: [PATCH] dma: edma: add device_slave_caps() support

2013-07-24 Thread Lars-Peter Clausen
On 07/23/2013 06:43 PM, Joel Fernandes wrote:
 Implement device_slave_caps(). EDMA has a limited number of slots.
 Slave drivers such as omap_hsmmc will query the driver to make
 sure they don't pass in more than these many scatter segments.
 
 Signed-off-by: Joel Fernandes jo...@ti.com
 ---
 Vinod, or Dan- If this patch looks ok, can you please merge in for
 -rc cycle? This patch is required to fix MMC support on AM33xx. This
 patch is blocking 3 other patches which fix various MMC things. Thanks!
 
 Notes:
 (1) this approach is temporary and only for -rc cycle to fix MMC on
 AM335x. It will be replace by the RFC series in future kernels:
 http://www.spinics.net/lists/arm-kernel/msg260094.html
 
 (2) Patch depends Vinod's patch at:
 http://permalink.gmane.org/gmane.linux.kernel/1525112
 
  drivers/dma/edma.c |9 +
  1 file changed, 9 insertions(+)
 
 diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
 index 7222cbe..81d5429 100644
 --- a/drivers/dma/edma.c
 +++ b/drivers/dma/edma.c
 @@ -517,6 +517,14 @@ static void edma_issue_pending(struct dma_chan *chan)
   spin_unlock_irqrestore(echan-vchan.lock, flags);
  }
  
 +static inline int edma_slave_caps(struct dma_chan *chan,
 + struct dma_slave_caps *caps)
 +{
 + caps-max_sg_nr = MAX_NR_SG;

Hm, what about the other fields?

- Lars

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] dma: edma: add device_slave_caps() support

2013-07-24 Thread Lars-Peter Clausen
On 07/24/2013 10:11 AM, Joel Fernandes wrote:
 On 07/24/2013 03:03 AM, Lars-Peter Clausen wrote:
 On 07/23/2013 06:43 PM, Joel Fernandes wrote:
 Implement device_slave_caps(). EDMA has a limited number of slots.
 Slave drivers such as omap_hsmmc will query the driver to make
 sure they don't pass in more than these many scatter segments.

 Signed-off-by: Joel Fernandes jo...@ti.com
 ---
 Vinod, or Dan- If this patch looks ok, can you please merge in for
 -rc cycle? This patch is required to fix MMC support on AM33xx. This
 patch is blocking 3 other patches which fix various MMC things. Thanks!

 Notes:
 (1) this approach is temporary and only for -rc cycle to fix MMC on
 AM335x. It will be replace by the RFC series in future kernels:
 http://www.spinics.net/lists/arm-kernel/msg260094.html

 (2) Patch depends Vinod's patch at:
 http://permalink.gmane.org/gmane.linux.kernel/1525112

  drivers/dma/edma.c |9 +
  1 file changed, 9 insertions(+)

 diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
 index 7222cbe..81d5429 100644
 --- a/drivers/dma/edma.c
 +++ b/drivers/dma/edma.c
 @@ -517,6 +517,14 @@ static void edma_issue_pending(struct dma_chan *chan)
 spin_unlock_irqrestore(echan-vchan.lock, flags);
  }
  
 +static inline int edma_slave_caps(struct dma_chan *chan,
 +   struct dma_slave_caps *caps)
 +{
 +   caps-max_sg_nr = MAX_NR_SG;

 Hm, what about the other fields?

 
 Other fields are unused, the max segment size is supposed to be
 calculated given the address width and burst size. Since these
 can't be provided to get_caps, I have left it out for now.
 See: https://lkml.org/lkml/2013/3/6/464

The PL330 driver is similar in this regard, the maximum segment size also
depends on address width and burst width. What I did for the get_slave_caps
implementation is to set it to the minimum maximum size. E.g. in you case
that should be SZ_64K - 1 (burstsize and addrwidth both set to 1).

 
 Even if it did, the segment size itself is unused in the MMC driver
 that this is supposed to fix, unlike the number of segments which I'm
 populating above.
 

E.g. for ALSA we'll need to know the max segment size, so I think it doesn't
hurt add this in this patch as well.

And you should also initialize all the other fields, even though if there
are no users yet. It will be really painful to write generic drivers using
the dmaengine API if none of the dmaengine drivers actually initializes the
caps struct properly.

- Lars
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] dma: edma: add device_slave_caps() support

2013-07-24 Thread Lars-Peter Clausen
On 07/24/2013 10:28 AM, Fernandes, Joel wrote:
 
 On Jul 24, 2013, at 3:23 AM, Lars-Peter Clausen l...@metafoo.de wrote:
 
 On 07/24/2013 10:11 AM, Joel Fernandes wrote:
 On 07/24/2013 03:03 AM, Lars-Peter Clausen wrote:
 On 07/23/2013 06:43 PM, Joel Fernandes wrote:
 Implement device_slave_caps(). EDMA has a limited number of slots.
 Slave drivers such as omap_hsmmc will query the driver to make
 sure they don't pass in more than these many scatter segments.

 Signed-off-by: Joel Fernandes jo...@ti.com
 ---
 Vinod, or Dan- If this patch looks ok, can you please merge in for
 -rc cycle? This patch is required to fix MMC support on AM33xx. This
 patch is blocking 3 other patches which fix various MMC things. Thanks!

 Notes:
 (1) this approach is temporary and only for -rc cycle to fix MMC on
 AM335x. It will be replace by the RFC series in future kernels:
 http://www.spinics.net/lists/arm-kernel/msg260094.html

 (2) Patch depends Vinod's patch at:
 http://permalink.gmane.org/gmane.linux.kernel/1525112

 drivers/dma/edma.c |9 +
 1 file changed, 9 insertions(+)

 diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
 index 7222cbe..81d5429 100644
 --- a/drivers/dma/edma.c
 +++ b/drivers/dma/edma.c
 @@ -517,6 +517,14 @@ static void edma_issue_pending(struct dma_chan *chan)
spin_unlock_irqrestore(echan-vchan.lock, flags);
 }

 +static inline int edma_slave_caps(struct dma_chan *chan,
 +struct dma_slave_caps *caps)
 +{
 +caps-max_sg_nr = MAX_NR_SG;

 Hm, what about the other fields?

 Other fields are unused, the max segment size is supposed to be
 calculated given the address width and burst size. Since these
 can't be provided to get_caps, I have left it out for now.
 See: https://lkml.org/lkml/2013/3/6/464

 The PL330 driver is similar in this regard, the maximum segment size also
 depends on address width and burst width. What I did for the get_slave_caps
 implementation is to set it to the minimum maximum size. E.g. in you case
 that should be SZ_64K - 1 (burstsize and addrwidth both set to 1).
 
 So you're setting max to minimum maximum size? Isn't that like telling the 
 driver that its segments can't be bigger than this... Unless I'm missing 
 something..

Yes. This is a limitation of the current slave_caps API. The maximum needs
to be the maximum for all possible configurations. A specific configuration
may allow a larger maximum. So we maybe have to extend the API to be able to
query the limits for a certain configuration. Not sure what the best way
would be to do that, either adding a config parameter to get_slave_caps or
to break it into two functions like you proposed one for the static
capabilities and one for the sg limits.

 


 Even if it did, the segment size itself is unused in the MMC driver
 that this is supposed to fix, unlike the number of segments which I'm
 populating above.

 E.g. for ALSA we'll need to know the max segment size, so I think it doesn't
 hurt add this in this patch as well.
 
 For alsa it would dma only the minimum max size even if the dma controller 
 could do more?
 

Yes, but I think 64k is still plenty enough for the max period size. The
current davinci PCM driver even claims to only support up to 8k.

- Lars
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] dma: edma: add device_slave_caps() support

2013-07-24 Thread Lars-Peter Clausen

On 07/24/2013 08:55 PM, Joel Fernandes wrote:

On 07/24/2013 03:40 AM, Lars-Peter Clausen wrote:

On 07/24/2013 10:28 AM, Fernandes, Joel wrote:


On Jul 24, 2013, at 3:23 AM, Lars-Peter Clausen l...@metafoo.de wrote:


On 07/24/2013 10:11 AM, Joel Fernandes wrote:

On 07/24/2013 03:03 AM, Lars-Peter Clausen wrote:

On 07/23/2013 06:43 PM, Joel Fernandes wrote:

Implement device_slave_caps(). EDMA has a limited number of slots.
Slave drivers such as omap_hsmmc will query the driver to make
sure they don't pass in more than these many scatter segments.

Signed-off-by: Joel Fernandes jo...@ti.com
---
Vinod, or Dan- If this patch looks ok, can you please merge in for
-rc cycle? This patch is required to fix MMC support on AM33xx. This
patch is blocking 3 other patches which fix various MMC things. Thanks!

Notes:
(1) this approach is temporary and only for -rc cycle to fix MMC on
AM335x. It will be replace by the RFC series in future kernels:
http://www.spinics.net/lists/arm-kernel/msg260094.html

(2) Patch depends Vinod's patch at:
http://permalink.gmane.org/gmane.linux.kernel/1525112

drivers/dma/edma.c |9 +
1 file changed, 9 insertions(+)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 7222cbe..81d5429 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -517,6 +517,14 @@ static void edma_issue_pending(struct dma_chan *chan)
spin_unlock_irqrestore(echan-vchan.lock, flags);
}

+static inline int edma_slave_caps(struct dma_chan *chan,
+struct dma_slave_caps *caps)
+{
+caps-max_sg_nr = MAX_NR_SG;


Hm, what about the other fields?


Other fields are unused, the max segment size is supposed to be
calculated given the address width and burst size. Since these
can't be provided to get_caps, I have left it out for now.
See: https://lkml.org/lkml/2013/3/6/464


The PL330 driver is similar in this regard, the maximum segment size also
depends on address width and burst width. What I did for the get_slave_caps
implementation is to set it to the minimum maximum size. E.g. in you case
that should be SZ_64K - 1 (burstsize and addrwidth both set to 1).


So you're setting max to minimum maximum size? Isn't that like telling the 
driver that its segments can't be bigger than this... Unless I'm missing 
something..


Yes. This is a limitation of the current slave_caps API. The maximum needs
to be the maximum for all possible configurations. A specific configuration
may allow a larger maximum. So we maybe have to extend the API to be able to
query the limits for a certain configuration. Not sure what the best way
would be to do that, either adding a config parameter to get_slave_caps or
to break it into two functions like you proposed one for the static
capabilities and one for the sg limits.


I am OK with either approach as long as a decision can be made quickly
by maintainers. Right now lot of back and forth has happened and 3
different versions of the same thing have been posted since January.
Since this is such a trivial change, it doesn't make sense to spend so
much time on it IMO The sad part is though this change is trivial,
other drivers such as MMC are broken and cannot be enabled due to this.
We cannot afford to leave them broken.


Well this is a new API, so it is kind of expected that there is some back and 
forth and that there will be a few revisions.




If Vinod is not available, can Dan please respond on how to proceed on
this? We really need this trivial change to go into this -rc cycle and
not delay it by another kernel release. Thank you.


This is not something you'd merge for rc3 or even later. If the MMC driver does 
not work without this I guess it never worked, so strictly speaking there is no 
regression and it is just a new feature.


- Lars

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] omapdss: Add new panel driver for Topolly td028ttec1 LCD.

2013-10-10 Thread Lars-Peter Clausen
On 10/10/2013 02:13 PM, Tomi Valkeinen wrote:
 On 10/10/13 14:52, Dr. H. Nikolaus Schaller wrote:
 
 Yes, I agree and I am willing to help if someone comes up with such a SoC.
 At the moment we have connected it to the OMAP3 only.
 
 True, but even without that kind of SoC, SPI bitbanging should be
 handled by an SPI driver, not by the drivers that use the bus.
 
 I.e. I want not to do a lot of work for others who we just guess about that 
 they
 might exist...
 
 Yep. It's fine for me, it's not that much extra code in the panel driver.
 
 The panel hardware has three wires, so the panel driver (if it does
 handle the bus by bitbanging) can only refer to three gpios.

 Hm. The panle hardware has 3 but the SoC (OMAP3) the driver
 is running on has 4.
 
 Right, but this panel driver is a driver for the panel hardware. Not for
 the SoC, or the SoC+panel combination. So the panel driver must only use
 resources as seen by the panel hardware.
 
 So either
 the bus details should be hidden by using the normal spi framework, or
 if the driver does the bitbanging, use the gpios as specified in the
 panel spec. The panel driver cannot contain any board specific stuff.

 The bitbang driver shown below can handle either 3 or 4 gpios (except
 for initialization).
 
 It's not a bitbang driver, it's a panel driver. And anyway, if I
 understood right, your use of 4 gpios was just a hack to try to make it
 look like a normal 4-wire SPI bus. What you really have is 3 wires, 3
 gpios. I don't see any reason to use 4 gpios, as two of them are the same.
 
 Hmm, how does it work anyway. Did I understand it right, the panel's
 'DIN' pin is connected to two gpios on the SoC, and one of those gpios
 is set as output, and the other as input? So the SoC is always pulling
 that line up or down, and the panel is also pulling it up or down when
 it's sending data. I'm no HW guy but that sounds quite bad =).
 
 I've never written or studied a bitbanging driver, but shouldn't there
 be just one gpio used on the SoC for DIN, and it would be set to either
 output or input mode, depending on if we are reading or writing?

Back in the OpenMoko days we used the panel in normal 4-wire SPI mode with
the GPIO bitbang SPI master. The bitbang code in this driver also looks like
normal 4 wire.

- Lars

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] omapdss: Add new panel driver for Topolly td028ttec1 LCD.

2013-10-10 Thread Lars-Peter Clausen

On 10/10/2013 03:42 PM, Dr. H. Nikolaus Schaller wrote:


Am 10.10.2013 um 14:26 schrieb Lars-Peter Clausen:


On 10/10/2013 02:13 PM, Tomi Valkeinen wrote:

On 10/10/13 14:52, Dr. H. Nikolaus Schaller wrote:


Yes, I agree and I am willing to help if someone comes up with such a SoC.
At the moment we have connected it to the OMAP3 only.


True, but even without that kind of SoC, SPI bitbanging should be
handled by an SPI driver, not by the drivers that use the bus.


I.e. I want not to do a lot of work for others who we just guess about that they
might exist...


Yep. It's fine for me, it's not that much extra code in the panel driver.


The panel hardware has three wires, so the panel driver (if it does
handle the bus by bitbanging) can only refer to three gpios.


Hm. The panle hardware has 3 but the SoC (OMAP3) the driver
is running on has 4.


Right, but this panel driver is a driver for the panel hardware. Not for
the SoC, or the SoC+panel combination. So the panel driver must only use
resources as seen by the panel hardware.


So either
the bus details should be hidden by using the normal spi framework, or
if the driver does the bitbanging, use the gpios as specified in the
panel spec. The panel driver cannot contain any board specific stuff.


The bitbang driver shown below can handle either 3 or 4 gpios (except
for initialization).


It's not a bitbang driver, it's a panel driver. And anyway, if I
understood right, your use of 4 gpios was just a hack to try to make it
look like a normal 4-wire SPI bus. What you really have is 3 wires, 3
gpios. I don't see any reason to use 4 gpios, as two of them are the same.


There is a protection resistor in the one defined as output so that
protocol errors don't have two outputs work against each other.



Hmm, how does it work anyway. Did I understand it right, the panel's
'DIN' pin is connected to two gpios on the SoC, and one of those gpios
is set as output, and the other as input?


Yes.


So the SoC is always pulling
that line up or down, and the panel is also pulling it up or down when
it's sending data. I'm no HW guy but that sounds quite bad =).


Yes, that is the strange thing with this protocol. It does a direction reversal
after some time. I.e. the panel switches its input to output and the SoC has
to be fast enough to do the same. Therefore, we have one output GPIO
(protected by a resistor) and a separate input GPIO.

Sometimes interfacing hardware is indeed strange.



I've never written or studied a bitbanging driver, but shouldn't there
be just one gpio used on the SoC for DIN, and it would be set to either
output or input mode, depending on if we are reading or writing?


Back in the OpenMoko days we used the panel in normal 4-wire SPI mode with
the GPIO bitbang SPI master. The bitbang code in this driver also looks like
normal 4 wire.



Thanks for this hint!

Maybe using a standard 4-wire SPI simply works if we only write data and
make sure the panel is not sending anything...


According to the datasheet the the panel as a dedicated dout pin. Maybe you did 
not connect it in your design, which means you won't be able to read any data 
from the panel at all.


Also your custom bitbang code looks a bit strange:

gpio_set_value(data-dout_gpio, 1);
if (gpio_get_value(data-din_gpio) == 0)
return 1;

You set the state on the dout pin and then read the din pin, if both do not 
match you abort with an error. This suggest that, for whatever reason, you feed 
the dout pin back into the din pin in your design. Btw. this is also the only 
place where din is used in your driver.


- Lars
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] omapdss: Add new panel driver for Topolly td028ttec1 LCD.

2013-10-11 Thread Lars-Peter Clausen

On 10/11/2013 06:41 AM, Tomi Valkeinen wrote:

On 10/10/13 21:58, Lars-Peter Clausen wrote:


According to the datasheet the the panel as a dedicated dout pin. Maybe
you did not connect it in your design, which means you won't be able to
read any data from the panel at all.


I don't see a dedicated dout in the datasheet...
http://dl.wolfgang-draxinger.net/openmoko/doc/TD028TTEC1.pdf


Hm, ok, looks like the display controller used in the panel (the jbt6k74) has 
separate DIN and DOUT, but the panel only has one pin. But the datasheet for 
the panel is not exactly clear on whether it's DIN pin is both the DIN and DOUT 
pin from the controller or, just DIN and DOUT not being connected at all.





Also your custom bitbang code looks a bit strange:

 gpio_set_value(data-dout_gpio, 1);
 if (gpio_get_value(data-din_gpio) == 0)
 return 1;

You set the state on the dout pin and then read the din pin, if both do
not match you abort with an error. This suggest that, for whatever
reason, you feed the dout pin back into the din pin in your design. Btw.
this is also the only place where din is used in your driver.


Yes, he said the single Serial interface data input/output pin is
connected to both din and dout on the SoC. I guess the purpose of that
gpio_get_value() is to ensure that the panel is not pulling the line
when the SoC is writing to it. Not that I really understand how that can
work, but I'm not a HW guy =).


Hm, ok. I don't think the panel will ever actively drive the line. So in my 
opinion using either the McBSP SPI master or the GPIO bitbang SPI master should 
work just fine. You just wouldn't be able to read any register from the device. 
But the driver is not attempting to do this, so it should be fine.


- Lars
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] omapdss: Add new panel driver for Topolly td028ttec1 LCD.

2013-10-11 Thread Lars-Peter Clausen
On 10/11/2013 10:59 AM, Belisko Marek wrote:
 Hi Tomi,
 
 On Fri, Oct 11, 2013 at 10:17 AM, Tomi Valkeinen tomi.valkei...@ti.com 
 wrote:
 On 11/10/13 10:42, Dr. H. Nikolaus Schaller wrote:

 I am not sure if there is a SPI driver for a McBSP port [1]? And to make 
 that
 work (reliably) and tested it might need a lot of work for us. At least I 
 think
 such a change (e.g. setting up clock polarity etc.) is not done in some 
 minutes.
 And the only feedback we have from the panel is does not work/works. 
 I.e.
 if we are not lucky that it works immediately we have no real means to 
 debug.

 IMHO it also gives more flexibility to board designers to choose GPIOs 
 instead
 of enforcing some SPI interface by the driver (and encapsulate this arguable
 protocol in the driver). Maybe some board has 3 spare GPIOs but neither
 McBSPs nor McSPIs available.

 This has been an interesting thread, I've learnt a lot =).

 I still think the panel driver should not handle this, but there should
 be a separate spi bitbang driver for it.

 I understand you're not enthusiastic going that way, as the current
 version works for you. However, when using DT, we need to think how to
 represent the hardware in the device tree data, and it has to be right
 from the beginning.

 That's why I won't allow representing this panel as having 4 gpios in
 the DT data, because that is not correct. The panel has 3 pins. But
 then, the panel does allow reading, which could be implemented using 4
 gpios as you have done. This data should be in the spi-bitbang data, and
 the panel should just use the standard SPI framework.
 I disagree. There are different drivers which pass in platform data
 gpios (encoder-tfp410.c or encoder-tpd12s015.c)
 and those must be covered by DT then. I cannot see problem why to have
 for td028 panel 3 or 4 gpios defined in DT.

The problem is not representing it in the devicetree, but representing it
correctly. This is a SPI slave device, hence it should be presented in the
devicetree as a SPI slave device and not as a platform device with 4 GPIOs.

- Lars

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [alsa-devel] [PATCH RFC v3 0/8] Beaglebone-Black HDMI audio

2014-01-27 Thread Lars-Peter Clausen
On 01/27/2014 05:17 PM, Jyri Sarha wrote:
 On 01/27/2014 05:51 PM, Lars-Peter Clausen wrote:
 Hi,

 I think you should try to get this in sync with the work Jean-Francois is
 currently doing[1]. Having the HDMI transmitter register a CODEC device is
 definitely the right approach, compared to the rather 'interesting'
 constraints stuff you do in patch 4.

 
 Oh, Jean-Francois's patches are now out. I'll take those into use, but I am
 afraid it still does not save me from the constraint stuff.
 
 The most complex piece of code comes from limited sample-rate availability,
 which is coming Beaglebone-Black desing (available i2s master clock), not
 from the HDMI encoder itself. It does not help with the stereo only
 limitation either, because it comes from the one wire only audio data
 connection on BBB.
 
 Support for S16_LE could maybe be added if the tda998x specific codec would
 fiddle with CTS_N predivider-setting (K select) according to the used sample
 width. But it appears Cobox plays all the sample formats fine without this
 change, so the question is if playing around with CTS_N predivider would
 break already working support there (something to be tested).

The ASoC core will set the constraints for the audio format/rate to the
intersection of what is supported by the CODEC and the CPU DAI. So if the
limitation comes from the CPU DAI the constraints should be applied there
and not in the machine driver. Similar if the tda998x only supports 32 bit
samples it should advertise this and the compound card will only support 32
bit samples.

- Lars

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [alsa-devel] [PATCH RFC v3 0/8] Beaglebone-Black HDMI audio

2014-01-27 Thread Lars-Peter Clausen
Hi,

I think you should try to get this in sync with the work Jean-Francois is
currently doing[1]. Having the HDMI transmitter register a CODEC device is
definitely the right approach, compared to the rather 'interesting'
constraints stuff you do in patch 4.

- Lars

[1] http://lkml.org/lkml/2014/1/27/85


On 01/27/2014 04:37 PM, Jyri Sarha wrote:
 Changes since RFC v2 version of the patches:
 - Dropped out already applied:
   ASoC: hdmi-codec: Add devicetree binding with documentation
 - Addresses Mark's comments here:
   
 http://mailman.alsa-project.org/pipermail/alsa-devel/2013-December/070605.html
   - ASoC: davinci-evm: Add named clock reference to DT bindings
 - Get rid of unnecessary castings
 - Add mclk NULL checks
 - Use devm_clk_get()
 - Change clock name from ti,codec-clock to mclk
 - Address Mark's comments here:
   
 http://mailman.alsa-project.org/pipermail/alsa-devel/2013-December/070606.html
   - ASoC: davinci-evm: HDMI audio support for TDA998x trough McASP I2S bus
 - Get rid of unnecessary castings
 - Update commit message
   - Add: ASoC: davinci-mcasp: Set BCLK divider if McASP is BCLK master
 - Use snd_soc_params_to_bclk()
 
 Changes since the first RFC version of the patches:
 - Drop out already applied: 
   ASoC: hdmi-codec: Add SNDRV_PCM_FMTBIT_32_LE playback format
 - Change sound node's compatible property
   form: ti,am33xx-beaglebone-black to ti,am33xx-beaglebone-black-audio
 - Some minor style issue fixes from TI internal review
 
 Jyri Sarha (8):
   clk: add gpio controlled clock
   ASoC: davinci-evm: Add named clock reference to DT bindings
   ASoC: davinci-mcasp: Set BCLK divider if McASP is BCLK master
   ASoC: davinci-evm: HDMI audio support for TDA998x trough McASP I2S
 bus
   ASoC: davinci: HDMI audio build for AM33XX and TDA998x
   drm/tilcdc: Add I2C HDMI audio config for tda998x
   ARM: OMAP2+: omap2plus_defconfig: Enable tilcdc and TDA998X HDMI
 support
   ARM: OMAP2+: omap2plus_defconfig: Enable BeagleBone Black HDMI audio
 support
 
  .../devicetree/bindings/clock/gpio-clock.txt   |   21 ++
  .../bindings/sound/davinci-evm-audio.txt   |   13 +-
  arch/arm/configs/omap2plus_defconfig   |5 +
  drivers/clk/Makefile   |1 +
  drivers/clk/clk-gpio.c |  210 +++
  drivers/gpu/drm/tilcdc/tilcdc_slave.c  |   24 ++-
  include/linux/clk-provider.h   |   25 +++
  sound/soc/davinci/Kconfig  |   12 ++
  sound/soc/davinci/Makefile |1 +
  sound/soc/davinci/davinci-evm.c|  211 
 +++-
  sound/soc/davinci/davinci-mcasp.c  |   20 ++
  11 files changed, 534 insertions(+), 9 deletions(-)
  create mode 100644 Documentation/devicetree/bindings/clock/gpio-clock.txt
  create mode 100644 drivers/clk/clk-gpio.c
 

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [alsa-devel] [PATCH RFC v3 0/8] Beaglebone-Black HDMI audio

2014-01-27 Thread Lars-Peter Clausen

On 01/27/2014 08:40 PM, Jyri Sarha wrote:

On 01/27/2014 06:32 PM, Lars-Peter Clausen wrote:

On 01/27/2014 05:17 PM, Jyri Sarha wrote:

On 01/27/2014 05:51 PM, Lars-Peter Clausen wrote:

Hi,

I think you should try to get this in sync with the work Jean-Francois is
currently doing[1]. Having the HDMI transmitter register a CODEC device is
definitely the right approach, compared to the rather 'interesting'
constraints stuff you do in patch 4.



Oh, Jean-Francois's patches are now out. I'll take those into use, but I am
afraid it still does not save me from the constraint stuff.

The most complex piece of code comes from limited sample-rate availability,
which is coming Beaglebone-Black desing (available i2s master clock), not
from the HDMI encoder itself. It does not help with the stereo only
limitation either, because it comes from the one wire only audio data
connection on BBB.

Support for S16_LE could maybe be added if the tda998x specific codec would
fiddle with CTS_N predivider-setting (K select) according to the used sample
width. But it appears Cobox plays all the sample formats fine without this
change, so the question is if playing around with CTS_N predivider would
break already working support there (something to be tested).


The ASoC core will set the constraints for the audio format/rate to the
intersection of what is supported by the CODEC and the CPU DAI. So if the
limitation comes from the CPU DAI the constraints should be applied there
and not in the machine driver. Similar if the tda998x only supports 32 bit
samples it should advertise this and the compound card will only support 32
bit samples.



Yes. I know. But these limitations come from the run time setup of the audio
serial bus and those details are not available to the cpu dai at the register
time.

If more of the McASP configuration data would be moved to DT, instead of giving
it in set_sysclk, set_fmt, etc. calls it would be possible for the driver code
produce more accurate constraints at register time. However that would change
McASP driver a lot and would possibly break some of the legacy support.


There is nothing wrong with setting the constraints based on the parameters 
passed to set_sysclk or set_fmt, etc. In fact this is something that is often 
done by drivers.


- Lars

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [alsa-devel] [PATCH 14/18] ASoC: davinci: Add edma dmaengine platform driver

2014-03-13 Thread Lars-Peter Clausen

On 03/13/2014 10:18 AM, Peter Ujfalusi wrote:
[...]

+static const struct snd_pcm_hardware edma_pcm_hardware = {
+   .info   = SNDRV_PCM_INFO_MMAP |
+ SNDRV_PCM_INFO_MMAP_VALID |
+ SNDRV_PCM_INFO_BATCH |
+ SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME |
+ SNDRV_PCM_INFO_INTERLEAVED,
+   .buffer_bytes_max   = 128 * 1024,
+   .period_bytes_min   = 32,
+   .period_bytes_max   = 64 * 1024,
+   .periods_min= 2,
+   .periods_max= 19, /* Limit by edma dmaengine driver */
+};


The idea is that we can auto-discover all the things using the 
dma_slave_caps API. Too bad we removed the possibility to specify the 
maximum number of segments from the API. Maybe we need to add it back. Is 
the 19 a hard-limit or could it be worked around by software in the 
dmaengine driver?



+
+static const struct snd_dmaengine_pcm_config edma_dmaengine_pcm_config = {
+   .pcm_hardware = edma_pcm_hardware,
+   .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config,
+   .prealloc_buffer_size = 128 * 1024,


Unless there is a very good reason for exactly this size, just leave it 0 
and let the generic dmaengine driver use the default.



+};
+
+static const struct snd_dmaengine_pcm_config edma_compat_dmaengine_pcm_config 
= {
+   .pcm_hardware = edma_pcm_hardware,
+   .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config,
+   .compat_filter_fn = edma_filter_fn,
+   .prealloc_buffer_size = 128 * 1024,
+};


There is no need for different configs for DT and non-DT.


+
+int edma_pcm_platform_register(struct device *dev)
+{
+   if (dev-of_node)
+   return snd_dmaengine_pcm_register(dev,
+   edma_dmaengine_pcm_config,
+   SND_DMAENGINE_PCM_FLAG_NO_RESIDUE);


Since the edma dmaengine driver implements the slave cap API there is no 
need to manually specify SND_DMAENGINE_PCM_FLAG_NO_RESIDUE manually. But 
since the edma driver sets the granularity to 
DMA_RESIDUE_GRANULARITY_DESCRIPTOR in this case the generic dmaengine will 
not set SND_DMAENGINE_PCM_FLAG_NO_RESIDUE automatically since it assumes 
that the dmaengine driver is capable of properly reporting the DMA position.



+   else
+   return snd_dmaengine_pcm_register(dev,
+   edma_compat_dmaengine_pcm_config,
+   SND_DMAENGINE_PCM_FLAG_NO_RESIDUE |
+   SND_DMAENGINE_PCM_FLAG_NO_DT |
+   SND_DMAENGINE_PCM_FLAG_COMPAT);



If you set the flags to just SND_DMAENGINE_PCM_FLAG_COMPAT it will do the 
right thing in the generic dmaengine driver depending on whether 
dev-of_node is set or not.



There is also a devm_ version of snd_dmaengine_pcm_register() it probably 
makes sense to use it here.


--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [alsa-devel] [PATCH 14/18] ASoC: davinci: Add edma dmaengine platform driver

2014-03-13 Thread Lars-Peter Clausen

On 03/13/2014 12:56 PM, Peter Ujfalusi wrote:

On 03/13/2014 12:28 PM, Lars-Peter Clausen wrote:

On 03/13/2014 10:18 AM, Peter Ujfalusi wrote: [...]

+static const struct snd_pcm_hardware edma_pcm_hardware = { +.info
= SNDRV_PCM_INFO_MMAP | +  SNDRV_PCM_INFO_MMAP_VALID | +
SNDRV_PCM_INFO_BATCH | +  SNDRV_PCM_INFO_PAUSE |
SNDRV_PCM_INFO_RESUME | +  SNDRV_PCM_INFO_INTERLEAVED, +
.buffer_bytes_max= 128 * 1024, +.period_bytes_min= 32, +
.period_bytes_max= 64 * 1024, +.periods_min= 2, +
.periods_max= 19, /* Limit by edma dmaengine driver */ +};


The idea is that we can auto-discover all the things using the
dma_slave_caps API. Too bad we removed the possibility to specify the
maximum number of segments from the API. Maybe we need to add it back. Is
the 19 a hard-limit or could it be worked around by software in the
dmaengine driver?


The edma dmaengine driver will refuse to configure more than 20 paRAM slots (1
for the channel + 19 for the periods). Depending on the SoC we might have
different number of paRAM entries available. The intention with the limit was
to prevent cases when we run out of paRAM entires for other devices because
audio took most of them.


The reason why we removed the max_segments field from the slave_caps struct 
was because we though it should be possible to, even when the hardware only 
supports a fixed amount of segments, emulate support for more in software. 
If this is not the case with the edma, we need to bring this field back.


--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [alsa-devel] [PATCH 14/18] ASoC: davinci: Add edma dmaengine platform driver

2014-03-13 Thread Lars-Peter Clausen

On 03/13/2014 02:03 PM, Peter Ujfalusi wrote:

On 03/13/2014 12:28 PM, Lars-Peter Clausen wrote:

+int edma_pcm_platform_register(struct device *dev)
+{
+if (dev-of_node)
+return snd_dmaengine_pcm_register(dev,
+edma_dmaengine_pcm_config,
+SND_DMAENGINE_PCM_FLAG_NO_RESIDUE);


Since the edma dmaengine driver implements the slave cap API there is no need
to manually specify SND_DMAENGINE_PCM_FLAG_NO_RESIDUE manually. But since the
edma driver sets the granularity to DMA_RESIDUE_GRANULARITY_DESCRIPTOR in this
case the generic dmaengine will not set SND_DMAENGINE_PCM_FLAG_NO_RESIDUE
automatically since it assumes that the dmaengine driver is capable of
properly reporting the DMA position.


Hrm, I see. For eDMA I think we can support DMA_RESIDUE_GRANULARITY_SEGMENT
granularity. Since according to the documentation the _SEGMENT means that the
DMA position will be updated per periods, which is basically the same thing
what we are doing at the moment when the granularity is
DMA_RESIDUE_GRANULARITY_DESCRIPTOR.
 From ALSA point of view at least they are the same: neither of them can report
exact position, the DMA pointer jumps from period to period.

IMHO in the generic dmaengine PCM we should set the SNDRV_PCM_INFO_BATCH for
both cases.



Ups, sorry mixed up DMA_RESIDUE_GRANULARITY_SEGMENT and 
DMA_RESIDUE_GRANULARITY_DESCRIPTOR. You can just remove the 
SND_DMAENGINE_PCM_FLAG_NO_RESIDUE when registering the dmaengine PCM driver 
and everything will still work as expected.

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [alsa-devel] [PATCH 2/5] ASoC: Add HA (HEAD acoustics) DSP codec driver template

2014-04-28 Thread Lars-Peter Clausen

On 04/28/2014 02:17 PM, Stefan Roese wrote:

From: Jarkko Nikula jarkko.nik...@bitmer.com

This codec driver template represents an I2C controlled multichannel audio
codec that has many typical ASoC codec driver features like volume controls,
mixer stages, mux selection, output power control, in-codec audio routings,
codec bias management and DAI link configuration.

Updates from Stefan Roese, 2014-04-28:
Port the HA DSP codec driver to Linux v3.15-rc. This includes
support for DT based probing. No platform-data code is needed
any more, DT nodes are sufficient.

Signed-off-by: Jarkko Nikula jarkko.nik...@bitmer.com
Signed-off-by: Stefan Roese s...@denx.de
Cc: Thorsten Eisbein thorsten.eisb...@head-acoustics.de


Looks very good. Couple of bits inline.

[...]

+
+#include linux/module.h
+#include linux/moduleparam.h
+#include linux/init.h
+#include linux/delay.h
+#include linux/pm.h
+#include linux/i2c.h
+#include linux/platform_device.h
+#include linux/regmap.h
+#include linux/slab.h
+#include sound/core.h
+#include sound/pcm.h
+#include sound/pcm_params.h
+#include sound/soc.h
+#include sound/soc-dapm.h
+#include sound/tlv.h
+#include sound/initval.h


There seem to be a couple of includes here that are not really needed.


+
+#include ha-dsp.h

[...]

+static const char *ha_dsp_mode_texts[] = {Mode 1, Mode 2};


const char *const


+static SOC_ENUM_SINGLE_DECL(ha_dsp_mode_enum, HA_DSP_CTRL, 0,
+   ha_dsp_mode_texts);
+
+/* Monitor output mux selection */
+static const char *ha_dsp_monitor_texts[] = {Off, ADC, DAC};


const char *const


+static SOC_ENUM_SINGLE_DECL(ha_dsp_monitor_enum, HA_DSP_CTRL, 1,
+   ha_dsp_monitor_texts);
+

[...]

+static const struct snd_soc_dapm_widget ha_dsp_widgets[] = {
+   SND_SOC_DAPM_ADC(ADC, Capture, SND_SOC_NOPM, 0, 0),
+   SND_SOC_DAPM_DAC(DAC, Playback, SND_SOC_NOPM, 0, 0),
+
+   SND_SOC_DAPM_MIXER(OUT1 Mixer, SND_SOC_NOPM, 0, 0,
+  ha_dsp_out1_mixer_controls[0],
+  ARRAY_SIZE(ha_dsp_out1_mixer_controls)),


There is the SOC_MIXER_ARRAY() helper macro that you can use here and below.


+   SND_SOC_DAPM_MIXER(OUT2 Mixer, SND_SOC_NOPM, 0, 0,
+  ha_dsp_out2_mixer_controls[0],
+  ARRAY_SIZE(ha_dsp_out2_mixer_controls)),
+   SND_SOC_DAPM_MIXER(OUT3 Mixer, SND_SOC_NOPM, 0, 0,
+  ha_dsp_out3_mixer_controls[0],
+  ARRAY_SIZE(ha_dsp_out3_mixer_controls)),
+   SND_SOC_DAPM_MIXER(OUT4 Mixer, SND_SOC_NOPM, 0, 0,
+  ha_dsp_out4_mixer_controls[0],
+  ARRAY_SIZE(ha_dsp_out4_mixer_controls)),
+   SND_SOC_DAPM_MIXER(OUT5 Mixer, SND_SOC_NOPM, 0, 0,
+  ha_dsp_out5_mixer_controls[0],
+  ARRAY_SIZE(ha_dsp_out5_mixer_controls)),
+   SND_SOC_DAPM_MIXER(OUT6 Mixer, SND_SOC_NOPM, 0, 0,
+  ha_dsp_out6_mixer_controls[0],
+  ARRAY_SIZE(ha_dsp_out6_mixer_controls)),
+   SND_SOC_DAPM_MIXER(OUT7 Mixer, SND_SOC_NOPM, 0, 0,
+  ha_dsp_out7_mixer_controls[0],
+  ARRAY_SIZE(ha_dsp_out7_mixer_controls)),
+   SND_SOC_DAPM_MIXER(OUT8 Mixer, SND_SOC_NOPM, 0, 0,
+  ha_dsp_out8_mixer_controls[0],
+  ARRAY_SIZE(ha_dsp_out8_mixer_controls)),

[...]

+static int ha_dsp_hw_params(struct snd_pcm_substream *substream,
+   struct snd_pcm_hw_params *params,
+   struct snd_soc_dai *dai)
+{
+   struct snd_soc_pcm_runtime *rtd = substream-private_data;
+   struct snd_soc_codec *codec = rtd-codec;


A codec should never look at the pcm_runtime. The proper way to get a 
pointer to the codec in dai callbacks is dai-codec. Or just use dai-dev below.



+
+   dev_dbg(codec-dev, Sample format 0x%X\n, params_format(params));
+   dev_dbg(codec-dev, Channels %d\n, params_channels(params));
+   dev_dbg(codec-dev, Rate %d\n, params_rate(params));
+
+   return 0;
+}

[...]

+static int ha_dsp_set_bias_level(struct snd_soc_codec *codec,
+enum snd_soc_bias_level level)
+{
+   dev_dbg(codec-dev, Changing bias from %d to %d\n,
+   codec-dapm.bias_level, level);
+
+   switch (level) {
+   case SND_SOC_BIAS_ON:
+   break;
+   case SND_SOC_BIAS_PREPARE:
+   /* Set PLL on */
+   break;
+   case SND_SOC_BIAS_STANDBY:
+   /* Set power on, Set PLL off */
+   break;
+   case SND_SOC_BIAS_OFF:
+   /* Set power down */
+   break;
+   }
+   codec-dapm.bias_level = level;


If you don't do anything in set_bias_level, just don't implement the 
function. The default implementation if no callback is specified is to set 
the bias_level to the requested level.


Re: [PATCH] regmap: Allow read_reg_mask to be 0

2014-09-30 Thread Lars-Peter Clausen

On 09/30/2014 06:07 PM, Dan Murphy wrote:

There may be spi devices that do not require a
register read mask to read the registers.

Currently the code sets the read mask based on
a non-zero value passed in from the driver or if that
value is 0 sets the read mask to 0x80.


It only sets it to the bus default if both read_flag_mask and 
write_flag_mask are 0. The assumption is that both of them being zero is a 
invalid configuration and either of them (or both) have to be non-zero for 
proper operation, since otherwise the device can't tell the difference 
between a read and a write.


Do you have a device where both the read and the write mask is 0?

- Lars
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] regmap: Allow read_reg_mask to be 0

2014-09-30 Thread Lars-Peter Clausen

On 09/30/2014 11:18 PM, Dan Murphy wrote:

Lars

On 09/30/2014 04:03 PM, Lars-Peter Clausen wrote:

On 09/30/2014 06:07 PM, Dan Murphy wrote:

There may be spi devices that do not require a
register read mask to read the registers.

Currently the code sets the read mask based on
a non-zero value passed in from the driver or if that
value is 0 sets the read mask to 0x80.


It only sets it to the bus default if both read_flag_mask and write_flag_mask 
are 0. The assumption is that both of them being zero is a invalid 
configuration and either of them (or both) have to be non-zero for proper 
operation, since otherwise the device can't tell the difference between a read 
and a write.

Do you have a device where both the read and the write mask is 0?

- Lars


Yes I do have a device that the read/write mask are both 0.

The device, which is already in production, has a specific control register 
that sets either the reading or writing of the rest of the registers.

Here is the data sheet

http://www.ti.com/lit/ds/symlink/afe4403.pdf

See page 61 control0.

Driver is written for this part just want to get this lead patch in or maybe an 
alternate solution.


Looking at this the generic SPI regmap implementation might not necessarily 
be the best thing to use here and you are probably better of implementing 
either your own regmap bus or reg_read/reg_write callbacks that 
automatically set/clear the SPI_READ bit in the control register depending 
on the operation.


- Lars
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] regmap: Allow read_reg_mask to be 0

2014-10-01 Thread Lars-Peter Clausen

On 10/01/2014 01:39 PM, Dan Murphy wrote:

Lars

On 09/30/2014 04:29 PM, Lars-Peter Clausen wrote:

On 09/30/2014 11:18 PM, Dan Murphy wrote:

Lars

On 09/30/2014 04:03 PM, Lars-Peter Clausen wrote:

On 09/30/2014 06:07 PM, Dan Murphy wrote:

There may be spi devices that do not require a
register read mask to read the registers.

Currently the code sets the read mask based on
a non-zero value passed in from the driver or if that
value is 0 sets the read mask to 0x80.


It only sets it to the bus default if both read_flag_mask and write_flag_mask 
are 0. The assumption is that both of them being zero is a invalid 
configuration and either of them (or both) have to be non-zero for proper 
operation, since otherwise the device can't tell the difference between a read 
and a write.

Do you have a device where both the read and the write mask is 0?

- Lars


Yes I do have a device that the read/write mask are both 0.

The device, which is already in production, has a specific control register 
that sets either the reading or writing of the rest of the registers.

Here is the data sheet

http://www.ti.com/lit/ds/symlink/afe4403.pdf

See page 61 control0.

Driver is written for this part just want to get this lead patch in or maybe an 
alternate solution.


Looking at this the generic SPI regmap implementation might not necessarily be 
the best thing to use here and you are probably better of implementing either 
your own regmap bus or reg_read/reg_write callbacks that automatically 
set/clear the SPI_READ bit in the control register depending on the operation.

- Lars


I am not sure implementing a different SPI regmap implementation is really the 
best thing.
I am handling this control bit toggling in the peripheral driver.


This is the very thing that regmap is supposed to hide, the specifics of how 
the read or write access happens. regmap_read() is supposed to do a read, 
regmap_write() is supposed to do a write. If regmap_read() only does a write 
if you previously did a regmap_write(regmap, CTRL0, SPI_READ); then the 
semantics of the interface are broken. This means you can't use generic 
infrastructure and it will confuse people who read and review your code.


- Lars
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC] adp1653: Add device tree bindings for LED controller

2014-11-16 Thread Lars-Peter Clausen

On 11/16/2014 08:59 AM, Pavel Machek wrote:

[...]
+   adp1653: adp1653@30 {
+   compatible = ad,adp1653;


The Analog Devices vendor prefix is adi.

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 5/8] usb: musb: Change end point selection to use new IO access

2014-11-25 Thread Lars-Peter Clausen

On 11/25/2014 12:52 AM, Tony Lindgren wrote:

* Apelete Seketeli apel...@seketeli.net [141124 15:40]:

Hi Tony,

Thanks for the patch.

On Mon, Nov-24-2014 at 11:05:03 AM -0800, Tony Lindgren wrote:

This allows the endpoints to work when multiple MUSB glue
layers are built in.


Applied on top of 3.18-rc6 mainline and tested successfully on JZ4740.
Been able to use ethernet-over-usb to access the internet on
device. No issue as far as I'm concerned.


Great that's good to hear and thanks for testing.

Doing the DMA patches here right now.. For the DMA, I've set up
JZ4740 to use the MUSB_DMA_INVENTRA option by default, is that OK
or do you have some other DMA hardware on it?

If MUSB_DMA_INVENTRA does not work, and you don't have other DMA
hardware on it, we can pass MUSB_DMA_INVENTRA and leave the DMA
function pointers empty, and then the driver will bail out during
init unless the option for CONFIG_MUSB_PIO_ONLY is set.


Yea... so according to the datasheet there is no DMA support, or at least it 
is not documented in the datasheet's description of the USB core. There is a 
vendor driver for the core which has ifdefs to enable DMA which looks like 
MUSB_DMA_INVENTRA, but I think we never really go that to work too well. So 
the current configuration is to use only PIO.


- Lars

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH RFC (do not merge)] ASoC: davinci-mcasp: Set rule constraint if implicit bclk divider is used

2015-03-13 Thread Lars-Peter Clausen

On 03/13/2015 12:36 PM, Jyri Sarha wrote:
[...]

In theory this patch does exactly what it is supposed to. It only
allows a sample-rate and sample-format combination if the rate can be
produced with reasonable accuracy. Unfortunately the alsa-lib and
alsa-tools are not able use this information too well. If the requested
sample-rate and sample-format is not available the aplay/arecord
fails, even if plughw is selected, with:

pcm_params.c:170: snd1_pcm_hw_param_get_min: Assertion `!snd_interval_empty(i)' 
failed.

[...]

+
+   /*
+* If we rely on implicit BCLK divider setting we should
+* set constraints based on what we can provide.
+*/
+   if (mcasp-bclk_master  mcasp-bclk_div == 0  mcasp-sysclk_freq)
+   return snd_pcm_hw_rule_add(substream-runtime, 0,
+  SNDRV_PCM_HW_PARAM_RATE,
+  davinci_mcasp_hw_rule_rate,
+  mcasp,
+  SNDRV_PCM_HW_PARAM_FRAME_BITS,
+  SNDRV_PCM_HW_PARAM_CHANNELS, -1);
+


For things to work correctly you also need reverse rules restricting 
CHANNELS and FRAME_BITS based on the RATE. This might fix the issue you are 
seeing with the ALSA tools.


- Lars

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] gpiolib: irqchip: use different lockdep class for each gpio irqchip

2015-08-14 Thread Lars-Peter Clausen
On 08/14/2015 02:34 PM, Linus Walleij wrote:
[...]
 Every chip will get their own lock class on the heap.
 
 But I think it is a bit kludgy.
 
 Is it not possible to have  the lock key in struct gpio_chip
 be a real member instead of a pointer and get a per-chip
 lock that way?
 
 (...)
 struct lock_class_key lock_key;
 
 instead of:
 
 struct lock_class_key  *lock_key;
 
 - problem solved, without kludgy header defines?


Lock keys need to be in persistent memory since they have a unlimited life
time. Once registered it is expected to exist until the system is reset.

We recently fixed the same issue of nested locks in regmap. For reference
the discussion with had a look at different ways to solve this can be found
here[1] and the final patch series that went in here[2].

- Lars

[1] https://lkml.org/lkml/2015/6/25/144
[2] https://lkml.org/lkml/2015/7/8/43

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: Nokia N900 - audio TPA6130A2 problems

2015-07-25 Thread Lars-Peter Clausen

On 07/25/2015 12:28 PM, Pali Rohár wrote:

Hello,

sometimes after rebooting Nokia N900 initializing alsa audio fails.
Here output from dmesg log when it happen:

[6.925140] tpa6130a2 2-0060: Write failed
[6.929534] tpa6130a2 2-0060: Failed to initialize chip
[6.935272] tpa6130a2: probe of 2-0060 failed with error -121
[7.624237] rx51-audio n900-audio: Failed to add TPA6130A2 controls
[7.635101] rx51-audio n900-audio: ASoC: failed to init TLV320AIC34: -19
[7.645874] rx51-audio n900-audio: ASoC: failed to instantiate card -19
[7.665740] rx51-audio n900-audio: snd_soc_register_card failed (-19)
[8.063049] ALSA device list:
[8.070343]   No soundcards found.

Any idea what to do?


Looks like the chip is not responding. Try to add a small delay after 
powerup to give the device to be fully ready, something like the following:


--- a/sound/soc/codecs/tpa6130a2.c
+++ b/sound/soc/codecs/tpa6130a2.c
@@ -152,6 +152,8 @@ static int tpa6130a2_power(u8 power)
if (data-power_gpio = 0)
gpio_set_value(data-power_gpio, 1);

+   msleep(5);
+
data-power_state = 1;
ret = tpa6130a2_initialize();
if (ret  0) {

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3 2/3] iio:adc:palmas: add DT support

2015-10-19 Thread Lars-Peter Clausen
On 10/16/2015 02:53 PM, H. Nikolaus Schaller wrote:
[...]
> +Optional sub-nodes:
> +ti,channel0-current-microamp: Channel 0 current in uA.
> + Values are rounded to derive 0uA, 5uA, 15uA, 20uA.
> +ti,channel3-current-microamp: Channel 3 current in uA.
> + Valid are rounded to derive 0uA, 10uA, 400uA, 800uA.
> +ti,enable-extended-delay: Enable extended delay.

Those three above sound more like configuration policy, rather than hardware
description. What influence which values should be chosen?


--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html