Re: [PATCH v3 2/3] iio:adc:palmas: add DT support
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
Re: [PATCH] gpiolib: irqchip: use different lockdep class for each gpio irqchip
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
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 RFC (do not merge)] ASoC: davinci-mcasp: Set rule constraint if implicit bclk divider is used
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 5/8] usb: musb: Change end point selection to use new IO access
On 11/25/2014 12:52 AM, Tony Lindgren wrote: * Apelete Seketeli [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: [RFC] adp1653: Add device tree bindings for LED controller
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] regmap: Allow read_reg_mask to be 0
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: [PATCH] regmap: Allow read_reg_mask to be 0
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
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: [alsa-devel] [PATCH 2/5] ASoC: Add HA (HEAD acoustics) DSP codec driver template
On 04/28/2014 02:17 PM, Stefan Roese wrote:
From: Jarkko Nikula
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
Signed-off-by: Stefan Roese
Cc: Thorsten Eisbein
Looks very good. Couple of bits inline.
[...]
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
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.
+
+ return 0;
+}
+
+static struct snd_soc_dai_ops ha_dsp_dai_ops = {
const
+ .hw_params = ha_dsp_hw_params,
+ .set_fmt= ha_dsp_set_dai_fmt,
+};
+
+static struct snd_soc_dai_driver ha_dsp_dai = {
+ .name = "ha-ds
Re: [alsa-devel] [PATCH 14/18] ASoC: davinci: Add edma dmaengine platform driver
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 14/18] ASoC: davinci: Add edma dmaengine platform driver
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
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 RFC v3 0/8] Beaglebone-Black HDMI audio
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 RFC v3 0/8] Beaglebone-Black HDMI audio
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
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: [PATCH] omapdss: Add new panel driver for Topolly td028ttec1 LCD.
On 10/11/2013 10:59 AM, Belisko Marek wrote: > Hi Tomi, > > On Fri, Oct 11, 2013 at 10:17 AM, Tomi Valkeinen > 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: [PATCH] omapdss: Add new panel driver for Topolly td028ttec1 LCD.
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.
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.
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] dma: edma: add device_slave_caps() support
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" 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
---
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] dma: edma: add device_slave_caps() support
On 07/24/2013 10:28 AM, Fernandes, Joel wrote:
>
> On Jul 24, 2013, at 3:23 AM, "Lars-Peter Clausen" 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
>>>>> ---
>>>>> 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
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
>>> ---
>>> 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
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
> ---
> 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 v5 2/2] iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver
On 07/18/2013 10:36 AM, Oleksandr Kozaruk wrote:
> Hello Lars,
>
> On Wed, Jul 17, 2013 at 9:04 PM, Lars-Peter Clausen 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
Re: [PATCH v5 2/2] iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver
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
> Signed-off-by: Graeme Gregory
> Signed-off-by: Oleksandr Kozaruk
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 */
> +
Re: [PATCH v5 1/2] ARM: dts: twl: Add GPADC data to device tree
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
>> ---
>> 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
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 v3 2/2] iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver
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:
>> >
Re: [PATCH v3 2/2] iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver
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
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
Re: am335x: TSC & ADC reworking including DT pieces, take 4
On 06/11/2013 06:27 PM, Jonathan Cameron wrote: > > > Samuel Ortiz 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: am335x: TSC & ADC reworking including DT pieces, take 4
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
[PATCH] OMAPDSS: nec-nl8048 panel: Use dev_pm_ops
Use dev_pm_ops instead of the deprecated legacy suspend/resume callbacks.
Signed-off-by: Lars-Peter Clausen
---
.../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
[PATCH] ASoC: omap: Call omap_mcbsp_set_threshold() from mcbsp hw_params
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
---
Only compile tested, but I don't see why it should not work
Signed-off-by: Lars-Peter Clausen
---
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 */
Re: [PATCH v3 4/8] MFD: ti_am335x_tscadc: add device tree binding information
Hi,
On 01/18/2013 11:48 AM, Patil, Rachna wrote:
> From: "Patil, Rachna"
>
> Signed-off-by: Patil, Rachna
> ---
> .../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
Re: [PATCH] gpio: New driver for GPO emulation using PWM generators
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] gpio: New driver for GPO emulation using PWM generators
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. 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 1/3] ti_adc: Update with IIO map interface
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 1/3] ti_adc: Update with IIO map interface
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
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
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
> ---
> 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
> #include
> #include
> -#include
> #include
> +#include
> +#include
>
> #include
> #include
> @@ -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] ti_tscadc: Match mfd sub devices to regmap interface
On 10/31/2012 05:41 AM, Russ Dill wrote:
> On Wed, Oct 31, 2012 at 8:55 AM, 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.
>>
>> Signed-off-by: Pantelis Antoniou
>> ---
>> 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
>> #include
>> #include
>> +#include
>>
>> +#include
>> #include
>> #include
>>
>> @@ -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] ti_tscadc: Update with IIO map interface & deal with partial activation
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
> ---
> 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
> #include
> #include
> -#include
> #include
> +#include
> +#include
>
> #include
> #include
> @@ -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);
> + }
>
> /* ADC C
Re: [PATCH] ti_tscadc: Match mfd sub devices to regmap interface
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
> ---
> 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 v2 03/15] dmaengine: Add no_wakeup parameter to dmaengine_prep_dma_cyclic()
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 v2 03/15] dmaengine: Add no_wakeup parameter to dmaengine_prep_dma_cyclic()
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] Don't clobber access methods when !regmap
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
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: [alsa-devel] [RFC 1/3] ASoC: dmaengine: Don't use runtime private data for dmaengine data
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: [alsa-devel] [RFC 1/3] ASoC: dmaengine: Don't use runtime private data for dmaengine data
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
> Signed-off-by: Russell King
> ---
> 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: [PATCH] PWM: Add support for configuring polarity of PWM
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: [PATCH] PWM: Add support for configuring polarity of PWM
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] drivers/rtc/rtc-twl.c: fix threaded IRQ to use IRQF_ONESHOT
On 07/10/2012 12:15 AM, Andrew Morton wrote:
> On Fri, 6 Jul 2012 09:33:54 -0700
> Kevin Hilman 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
>> ---
>> 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 5/5] bq27x00 - don't report power-supply change so often.
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 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
>> ---
>>
>> 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
[PATCH 09/11] video: Remove redundant spi driver bus initialization
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:
//
@@
identifier _driver;
@@
struct spi_driver _driver = {
.driver = {
- .bus = &spi_bus_type,
},
};
//
Signed-off-by: Lars-Peter Clausen
Cc: Tomi Valkeinen
Cc: Florian Tobias Schandinat
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: [alsa-devel] [PATCH] ASoC: omap: convert per-board modules to platform drivers
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
Re: [PATCH] mmc: fix handling NULL returned from regulator_get()
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
>> ---
>>
>> 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 10/14] PM / MIPS: Use struct syscore_ops instead of sysdevs for PM
On 04/17/2011 11:12 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki
>
> 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
For the jz4740 part:
Acked-and-tested-by: Lars-Peter Clausen
> ---
> 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
> #include
> #include
> -#include
> +#include
>
> #include
> #include
> @@ -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
Re: [PATCH 1/7] pwm: Add pwm core driver
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
