Re: [PATCH] i2c: s3c2410: Don't enable PM runtime on the adapter device
On 04/16/2015 12:33 PM, Sylwester Nawrocki wrote: On 16/04/15 12:10, Charles Keepax wrote: Commit 523c5b89640e (i2c: Remove support for legacy PM) removed the PM ops from the bus type, which causes the pm operations on the s3c2410 adapter device to fail (-ENOSUPP in rpm_callback). The adapter device doesn't get bound to a driver and as such can't have its own pm_runtime callbacks. Previously this was fine as the bus callbacks would have been used, but now this can cause devices which use PM runtime and are attached over I2C to fail to resume. This commit fixes this issue by just doing the PM operations directly on the I2C device, rather than the adapter device in the driver and adding some stub callbacks for runtime suspend and resume. Signed-off-by: Charles Keepax ckee...@opensource.wolfsonmicro.com --- drivers/i2c/busses/i2c-s3c2410.c | 21 - 1 files changed, 16 insertions(+), 5 deletions(-) @@ -1253,7 +1253,6 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev) platform_set_drvdata(pdev, i2c); Wouldn't adding pm_runtime_no_callbacks(pdev-dev); here let us avoid the runtime resume/suspend stubs? Or just do pm_runtime_no_callbacks on the adapter device. Preferably in the I2C core. That should solve the issue without requiring any other changes. - Lars -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc 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 1/3] ASoC: samsung: Add machine driver for Trats2
Hi,
A few small comments inline.
On 01/05/2015 12:25 PM, Inha Song wrote:
--- /dev/null
+++ b/sound/soc/samsung/trats2_wm1811.c
@@ -0,0 +1,216 @@
[...]
+#include linux/of.h
+#include linux/module.h
+#include linux/clk.h
+#include sound/soc.h
+#include sound/pcm_params.h
+#include i2s.h
+#include i2s-regs.h
You probably don't need i2s-regs.h
+#include ../codecs/wm8994.h
+
+struct trats2_machine_priv {
+ struct clk *clk_mclk;
+};
+
+static struct trats2_machine_priv trats2_wm1811_priv;
+
+static const struct snd_kcontrol_new trats2_controls[] = {
+ SOC_DAPM_PIN_SWITCH(SPK),
+};
+
+const struct snd_soc_dapm_widget trats2_dapm_widgets[] = {
static
+ SND_SOC_DAPM_SPK(SPK, NULL),
+};
+
+static int trats2_aif1_hw_params(struct snd_pcm_substream *substream,
+struct snd_pcm_hw_params *params)
+{
+ struct snd_soc_pcm_runtime *rtd = substream-private_data;
+ struct snd_soc_dai *cpu_dai = rtd-cpu_dai;
+ struct snd_soc_dai *codec_dai = rtd-codec_dai;
+ struct trats2_machine_priv *priv = snd_soc_card_get_drvdata(rtd-card);
+ unsigned int sysclk_rate;
+ unsigned int mclk_rate =
+ (unsigned int)clk_get_rate(priv-clk_mclk);
+ int ret;
+
+ /* Set the codec DAI configuration to Master*/
+ ret = snd_soc_dai_set_fmt(codec_dai, SND_SOC_DAIFMT_I2S
+ | SND_SOC_DAIFMT_NB_NF
+ | SND_SOC_DAIFMT_CBM_CFM);
+ if (ret 0) {
+ dev_err(codec_dai-dev,
+ Failed to set codec dai format: %d\n, ret);
+ return ret;
+ }
+
+ /* Set the cpu DAI configuration to Slave */
+ ret = snd_soc_dai_set_fmt(cpu_dai, SND_SOC_DAIFMT_I2S
+ | SND_SOC_DAIFMT_NB_NF
+ | SND_SOC_DAIFMT_CBM_CFM);
+ if (ret 0) {
+ dev_err(cpu_dai-dev,
+ Failed to set cpu dai format: %d\n, ret);
+ return ret;
+ }
Use the dai_fmt field in the dai_link struct to setup the DAI link format.
This will configure both the CPU and the CODEC DAI with the specified
format, no need to do it manually.
[...]
+static struct snd_soc_ops trats2_aif1_ops = {
const
+ .hw_params = trats2_aif1_hw_params,
+};
+
+static int trats2_init_paiftx(struct snd_soc_pcm_runtime *rtd)
+{
+ struct snd_soc_codec *codec = rtd-codec;
+ struct snd_soc_card *card = rtd-card;
+ struct trats2_machine_priv *priv = snd_soc_card_get_drvdata(card);
+ int ret;
+
+ ret = clk_prepare_enable(priv-clk_mclk);
Maybe just do this in the platform device probe handler and you should have
a matching disable call somewhere.
+ if (ret) {
+ dev_err(codec-dev, Failed to enable mclk: %d\n, ret);
+ return ret;
+ }
+
+ return 0;
+}
[...]
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dmaengine: pl330: Set residue in tx_status callback
On 12/03/2014 05:47 AM, Padma Venkat wrote:
Hi Lars,
[snip]
+
+ ret = dma_cookie_status(chan, cookie, txstate);
+ if (ret == DMA_COMPLETE || !txstate)
+ return ret;
+
+ used = txstate-used;
+
+ spin_lock_irqsave(pch-lock, flags);
+ sar = readl(regs + SA(thrd-id));
+ dar = readl(regs + DA(thrd-id));
+
+ list_for_each_entry(desc, pch-work_list, node) {
+ if (desc-status == BUSY) {
+ current_c = desc-txd.cookie;
+ if (first) {
+ first_c = desc-txd.cookie;
+ first = false;
+ }
+
+ if (first_c current_c)
+ residue += desc-px.bytes;
+ else {
+ if (desc-rqcfg.src_inc
pl330_src_addr_in_desc(desc, sar)) {
+ residue += desc-px.bytes;
+ residue -= sar - desc-px.src_addr;
+ } else if (desc-rqcfg.dst_inc
pl330_dst_addr_in_desc(desc, dar))
{
+ residue += desc-px.bytes;
+ residue -= dar - desc-px.dst_addr;
+ }
+ }
+ } else if (desc-status == PREP)
+ residue += desc-px.bytes;
+
+ if (desc-txd.cookie == used)
+ break;
+ }
+ spin_unlock_irqrestore(pch-lock, flags);
+ dma_set_residue(txstate, residue);
+ return ret;
}
[snip]
Any comment on this patch?
Well it doesn't break audio, but I don't think it has the correct haviour
for all cases yet.
OK. Any way of testing other cases like scatter-gather and memcopy. I
verified memcopy in dmatest but it seems not doing anything with
residue bytes.
E.g. I think your current patch fails if more than one transfer has been
submitted. In that case you'll count the bytes for all of them rather than
just the one requested.
Again, the semantics are that it should return the progress of the transfer
for which the allocation function returned the cookie that is passe to this
May be my understanding is wrong. For clarification..In the
snd_dmaengine_pcm_pointer it is subtracting the residue bytes from the
total buffer bytes not from period bytes. So how it expects
the progress of the transfer of the passed cookie which just holds period bytes?
The issue that makes implementing this correctly for the pl330 driver
complicated is that the driver allocates one cookie per segment/period, but
the external API works with one cookie per transfer. All those cookies that
are assigned to the segments/periods that are not the first in a transfer
are not externally visible.
dmaengine_prep_slave_sg() and friends create a transfer and return
descriptor for this transfer with a single cookie. If that cookie is passed
to dmaengine_tx_status() the function is supposed to report the progress on
that one transfer that was previously allocated and returned that cookie
number. residue is the total number of bytes that are still left to to be
processed for that transfer.
I've started reworking the pl330 driver to only have a single cookie per
transfer, instead of one per period/segment. This makes implementing the
residue reporting a lot easier. I never finished that work though, but I can
try to see if I can revive it next week.
- Lars
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dmaengine: pl330: Set residue in tx_status callback
On 12/02/2014 06:38 AM, Padma Venkat wrote:
Hi Vinod/Lars,
On 11/26/14, Padmavathi Venna padm...@samsung.com wrote:
Fill txstate.residue with the amount of bytes remaining in the current
transfer if the transfer is not complete. This will be of particular
use to i2s DMA transfers, providing more accurate hw_ptr values to ASoC.
I had taken the code from Dylan Reid dgr...@chromium.org patch from the
below link and modified according to the current dmaengine framework.
http://comments.gmane.org/gmane.linux.kernel.samsung-soc/23007
Cc: Dylan Reid dgr...@chromium.org
Signed-off-by: Padmavathi Venna padm...@samsung.com
---
This patch has been tested for audio playback on exynos5420 peach-pit.
drivers/dma/pl330.c | 67
+-
1 files changed, 65 insertions(+), 2 deletions(-)
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index b7493d2..db880ae 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2182,11 +2182,74 @@ static void pl330_free_chan_resources(struct
dma_chan *chan)
pm_runtime_put_autosuspend(pch-dmac-ddma.dev);
}
+static inline int
+pl330_src_addr_in_desc(struct dma_pl330_desc *desc, unsigned int sar)
+{
+ return ((desc-px.src_addr = sar)
+ (sar = (desc-px.src_addr + desc-px.bytes)));
+}
+
+static inline int
+pl330_dst_addr_in_desc(struct dma_pl330_desc *desc, unsigned int dar)
+{
+ return ((desc-px.dst_addr = dar)
+ (dar = (desc-px.dst_addr + desc-px.bytes)));
+}
+
static enum dma_status
pl330_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
struct dma_tx_state *txstate)
{
- return dma_cookie_status(chan, cookie, txstate);
+ dma_addr_t sar, dar;
+ struct dma_pl330_chan *pch = to_pchan(chan);
+ void __iomem *regs = pch-dmac-base;
+ struct pl330_thread *thrd = pch-thread;
+ struct dma_pl330_desc *desc;
+ unsigned int residue = 0;
+ unsigned long flags;
+ bool first = true;
+ dma_cookie_t first_c, current_c;
+ dma_cookie_t used;
+ enum dma_status ret;
+
+ ret = dma_cookie_status(chan, cookie, txstate);
+ if (ret == DMA_COMPLETE || !txstate)
+ return ret;
+
+ used = txstate-used;
+
+ spin_lock_irqsave(pch-lock, flags);
+ sar = readl(regs + SA(thrd-id));
+ dar = readl(regs + DA(thrd-id));
+
+ list_for_each_entry(desc, pch-work_list, node) {
+ if (desc-status == BUSY) {
+ current_c = desc-txd.cookie;
+ if (first) {
+ first_c = desc-txd.cookie;
+ first = false;
+ }
+
+ if (first_c current_c)
+ residue += desc-px.bytes;
+ else {
+ if (desc-rqcfg.src_inc
pl330_src_addr_in_desc(desc, sar)) {
+ residue += desc-px.bytes;
+ residue -= sar - desc-px.src_addr;
+ } else if (desc-rqcfg.dst_inc
pl330_dst_addr_in_desc(desc, dar)) {
+ residue += desc-px.bytes;
+ residue -= dar - desc-px.dst_addr;
+ }
+ }
+ } else if (desc-status == PREP)
+ residue += desc-px.bytes;
+
+ if (desc-txd.cookie == used)
+ break;
+ }
+ spin_unlock_irqrestore(pch-lock, flags);
+ dma_set_residue(txstate, residue);
+ return ret;
}
static void pl330_issue_pending(struct dma_chan *chan)
@@ -2631,7 +2694,7 @@ static int pl330_dma_device_slave_caps(struct dma_chan
*dchan,
caps-directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
caps-cmd_pause = false;
caps-cmd_terminate = true;
- caps-residue_granularity = DMA_RESIDUE_GRANULARITY_DESCRIPTOR;
+ caps-residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
return 0;
}
Any comment on this patch?
Well it doesn't break audio, but I don't think it has the correct haviour
for all cases yet.
Again, the semantics are that it should return the progress of the transfer
for which the allocation function returned the cookie that is passe to this
function. You have to consider that there might be multiple different
descriptors submitted and in the work list, not just the one we want to know
the status of. The big problem with the pl330 driver is that the current
structure of the driver makes it not so easy to implement the residue
reporting correctly.
- Lars
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc 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/2] ASoC: samsung: Add machine driver for odroidx2
On 07/04/2014 01:04 PM, Sylwester Nawrocki wrote: On 22/05/14 20:53, Mark Brown wrote: + ret = snd_soc_dai_set_fmt(codec_dai, SND_SOC_DAIFMT_I2S + | SND_SOC_DAIFMT_NB_NF + | SND_SOC_DAIFMT_CBM_CFM); + if (ret 0) + return ret; + + ret = snd_soc_dai_set_fmt(cpu_dai, SND_SOC_DAIFMT_I2S + | SND_SOC_DAIFMT_NB_NF + | SND_SOC_DAIFMT_CBM_CFM); + if (ret 0) + return ret; These are constant, set these in the dai_link. set_fmt also sets master/slave mode of the I2S DAI, after I moved this into the cpu_dai link data structure after suspend/resume cycle the I2S IP block is not being properly re-configured. Should the format setting be added in resume_post callback, or is there any other preferred way ? Similarly the syclk settings are being lost over suspend/resume cycle and nothing restores them. The I2S driver should save and restore it's register during suspend/resume. - Lars -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc 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/2] ASoC: samsung: Add machine driver for odroidx2
On 07/04/2014 01:24 PM, Sylwester Nawrocki wrote: On 04/07/14 13:10, Lars-Peter Clausen wrote: On 07/04/2014 01:04 PM, Sylwester Nawrocki wrote: On 22/05/14 20:53, Mark Brown wrote: + ret = snd_soc_dai_set_fmt(codec_dai, SND_SOC_DAIFMT_I2S + | SND_SOC_DAIFMT_NB_NF + | SND_SOC_DAIFMT_CBM_CFM); + if (ret 0) + return ret; + + ret = snd_soc_dai_set_fmt(cpu_dai, SND_SOC_DAIFMT_I2S + | SND_SOC_DAIFMT_NB_NF + | SND_SOC_DAIFMT_CBM_CFM); + if (ret 0) + return ret; These are constant, set these in the dai_link. set_fmt also sets master/slave mode of the I2S DAI, after I moved this into the cpu_dai link data structure after suspend/resume cycle the I2S IP block is not being properly re-configured. Should the format setting be added in resume_post callback, or is there any other preferred way ? Similarly the syclk settings are being lost over suspend/resume cycle and nothing restores them. The I2S driver should save and restore it's register during suspend/resume. OK, thanks. I checked the I2S driver does it, but only when dai-active flags is set [1]. However, during system resume this flag is not set (in situation where playback or capture wasn't active right before system suspend). Is the dai-active test wrong in that driver then ? I'm not sure if it could just be removed. Yes, just remove the check. - Lars -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc 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 1/2] [RFC] ASoC: samsung: move s3c24xx over to dmaengine
On 06/05/2014 03:20 PM, Arnd Bergmann wrote: The s3c24xx sound support keeps giving randconfig build errors, this is a new attempt to solve it by moving the driver over to use dmaengine exclusively. The removal of the legacy DMA support and the addition of the s3c24xx_dma_filter pointer are fairly obvious here. The part I'm not completely sure about is the removal of the s3c2410_dma_ctrl(..., S3C2410_DMAOP_STARTED) and dma_data-ops-started() calls. My understanding is that these are only required for drivers that do not support cyclic transfers, which the new dma engine driver now does, so we can simply remove them. This would also fix at least one bug in the ac97 driver on newer machines, which currently gives us a NULL pointer dereference from trying to call dma_data-ops-started(). Any insights about this, or testing would be very welcome. There is already a very similar patch for this, see: http://mailman.alsa-project.org/pipermail/alsa-devel/2014-June/077327.html I think that one is scheduled to be merged. - Lars -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc 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] Fix for possible null pointer dereference in dma.c
On 05/20/2014 09:16 PM, Tomasz Figa wrote: Hi Rickard, On 20.05.2014 21:12, Rickard Strandqvist wrote: Hi Tomasz What I based my patch on is really because of this line: if (substream) snd_pcm_period_elapsed(substream); Boojin Kim thought that this was needed, if this is true anymore..? I have not been able to immerse myself so much in all patches. I'm working on about 100 similar patches. To me having NULL as either data argument of buffer done callback or private_data would be a serious driver bug and IMHO it's better to let it crash with a NULL pointer dereference to let someone notice than mask it by adding a condition. Still, I'm not too experienced with ALSA and ASoC, so I might be wrong. Mark, what do you think about this? Given that there is a patch[1] which removes the whole file I think we can stop the discussion about this patch here. But for the record, substream will never ever be NULL in this function. prtd might be though if the DMA completion callback races against the closing of the PCM stream. [1] http://mailman.alsa-project.org/pipermail/alsa-devel/2014-May/076860.html -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc 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/3] ASoC: dmaengine: Support custom channel names
On 10/23/2013 01:30 PM, Mark Brown wrote: On Tue, Oct 22, 2013 at 01:45:17PM +0200, Lars-Peter Clausen wrote: On 10/19/2013 06:43 PM, Mark Brown wrote: Some devices have more than just simple TX and RX DMA channels, for example modern Samsung I2S IPs support a secondary transmit DMA stream which is mixed into the primary stream during playback. Allow such devices to specify the names of the channels to be requested in their dma_data. As shortly discussed yesterday, I think the general idea is fine. But it might be better to have the names available at PCM creation time, since this allows us to e.g. do proper probe referral and will also have the code take the same path in the DT case, no matter if it uses the default names or not. I agree, but I'm thinking that the way to do this is to get the entire struct provided earlier so that the compat drivers get to use this stuff too. Is there any great reason not to do that? No, that should be fine. I've been thinking about this before as well. We probably need something like a snd_soc_register_component_with_dai_data() or similar. That assign the DAI data on creation. - Lars -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc 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/3] ASoC: dmaengine: Support custom channel names
On 10/19/2013 06:43 PM, Mark Brown wrote: From: Mark Brown broo...@linaro.org Some devices have more than just simple TX and RX DMA channels, for example modern Samsung I2S IPs support a secondary transmit DMA stream which is mixed into the primary stream during playback. Allow such devices to specify the names of the channels to be requested in their dma_data. Signed-off-by: Mark Brown broo...@linaro.org As shortly discussed yesterday, I think the general idea is fine. But it might be better to have the names available at PCM creation time, since this allows us to e.g. do proper probe referral and will also have the code take the same path in the DT case, no matter if it uses the default names or not. - Lars -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc 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] iio: exynos_adc: use wait_for_completion_timeout instead of interruptible
On 10/11/2013 10:23 AM, Naveen Krishna Chatradhi wrote:
This patch does the following
1. use wait_for_completion_timeout instead of
wait_for_completion_interruptible_timeout
2. Reset software if a timeout happens.
3. Also reduce the timeout to 100milli secs
It is always good to have a description of why a chance should be done in
the commit message. It is obvious what the patch does by just looking at the
changed lines, it is not that obvious though why those lines had to be changed.
Note: submitted for review at https://patchwork.kernel.org/patch/2279591/
Signed-off-by: Naveen Krishna Chatradhi ch.nav...@samsung.com
Cc: Doug Anderson diand...@chromium.org
Cc: Lars-Peter Clausen l...@metafoo.de
---
Changes since v1:
As per discussion at
http://marc.info/?l=linux-kernelm=136517637228869w=3
This patch does the following
1. use wait_for_completion_timeout instead of
wait_for_completion_interruptible_timeout
2. Reset software if a timeout happens.
3. Also reduce the timeout to 100milli secs
Changes since v2:
None.
Rebased and reposting.
---
drivers/iio/adc/exynos_adc.c | 66
+++---
1 file changed, 36 insertions(+), 30 deletions(-)
diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index d25b262..f18ed7e 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -82,7 +82,7 @@ enum adc_version {
#define ADC_CON_EN_START (1u 0)
#define ADC_DATX_MASK0xFFF
-#define EXYNOS_ADC_TIMEOUT (msecs_to_jiffies(1000))
+#define EXYNOS_ADC_TIMEOUT (msecs_to_jiffies(100))
struct exynos_adc {
void __iomem*regs;
@@ -112,6 +112,30 @@ static inline unsigned int exynos_adc_get_version(struct
platform_device *pdev)
return (unsigned int)match-data;
}
+static void exynos_adc_hw_init(struct exynos_adc *info)
+{
+ u32 con1, con2;
+
+ if (info-version == ADC_V2) {
+ con1 = ADC_V2_CON1_SOFT_RESET;
+ writel(con1, ADC_V2_CON1(info-regs));
+
+ con2 = ADC_V2_CON2_OSEL | ADC_V2_CON2_ESEL |
+ ADC_V2_CON2_HIGHF | ADC_V2_CON2_C_TIME(0);
+ writel(con2, ADC_V2_CON2(info-regs));
+
+ /* Enable interrupts */
+ writel(1, ADC_V2_INT_EN(info-regs));
+ } else {
+ /* set default prescaler values and Enable prescaler */
+ con1 = ADC_V1_CON_PRSCLV(49) | ADC_V1_CON_PRSCEN;
+
+ /* Enable 12-bit ADC resolution */
+ con1 |= ADC_V1_CON_RES;
+ writel(con1, ADC_V1_CON(info-regs));
+ }
+}
+
static int exynos_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int *val,
@@ -121,6 +145,7 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
struct exynos_adc *info = iio_priv(indio_dev);
unsigned long timeout;
u32 con1, con2;
+ int ret;
if (mask != IIO_CHAN_INFO_RAW)
return -EINVAL;
@@ -145,16 +170,21 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
ADC_V1_CON(info-regs));
}
- timeout = wait_for_completion_interruptible_timeout
+ timeout = wait_for_completion_timeout
(info-completion, EXYNOS_ADC_TIMEOUT);
Nitpick: It would be nice to put the info-completion on the same line as
the wait_for_completion...
+ if (timeout == 0) {
+ dev_warn(indio_dev-dev, Conversion timed out reseting\n);
+ exynos_adc_hw_init(info);
+ ret = -ETIMEDOUT;
+ } else {
+ *val = info-value;
+ ret = IIO_VAL_INT;
+ }
*val = info-value;
This line above should probably be removed.
mutex_unlock(indio_dev-mlock);
- if (timeout == 0)
- return -ETIMEDOUT;
-
- return IIO_VAL_INT;
+ return ret;
}
static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
@@ -226,30 +256,6 @@ static int exynos_adc_remove_devices(struct device *dev,
void *c)
return 0;
}
-static void exynos_adc_hw_init(struct exynos_adc *info)
-{
- u32 con1, con2;
-
- if (info-version == ADC_V2) {
- con1 = ADC_V2_CON1_SOFT_RESET;
- writel(con1, ADC_V2_CON1(info-regs));
-
- con2 = ADC_V2_CON2_OSEL | ADC_V2_CON2_ESEL |
- ADC_V2_CON2_HIGHF | ADC_V2_CON2_C_TIME(0);
- writel(con2, ADC_V2_CON2(info-regs));
-
- /* Enable interrupts */
- writel(1, ADC_V2_INT_EN(info-regs));
- } else {
- /* set default prescaler values and Enable prescaler */
- con1 = ADC_V1_CON_PRSCLV(49) | ADC_V1_CON_PRSCEN;
-
- /* Enable 12-bit ADC resolution */
- con1 |= ADC_V1_CON_RES;
- writel(con1, ADC_V1_CON(info-regs
Re: [alsa-devel] [PATCH 18/30] ASoC: samsung: move plat/ headers to local directory
On 04/11/2013 07:19 PM, Mark Brown wrote: On Thu, Apr 11, 2013 at 07:08:42PM +0200, Arnd Bergmann wrote: On Thursday 11 April 2013, Mark Brown wrote: This doesn't apply to my topic/samsung branch, can you please regenerate it against that or let me know what to apply it against? This one should work. Unfortunately I now found during testing that the s3c24xx sound support has a few build errors at the moment, but this patch should not add any new ones: Applied (after hand editing the commit message), thanks. make[5]: *** [sound/soc/samsung/i2s.o] Error 1 /git/arm-soc/sound/soc/samsung/neo1973_wm8753.c:25:24: fatal error: mach/gta02.h: No such file or directory #include mach/gta02.h Hrm, someone killed GTA02 support? That's sad... if that's really the case we could kill the machine driver but not tonight as I'm running late... I think the file was moved, we should pull over the audio relevant GPIO definitions into the ASoC board driver. - Lars -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc 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 20/30] ASoC: samsung: convert to dmaengine API
On 04/11/2013 04:47 PM, Arnd Bergmann wrote: On Thursday 11 April 2013, Mark Brown wrote: On Thu, Apr 11, 2013 at 02:05:02AM +0200, Arnd Bergmann wrote: In order to build the exynos kernel with CONFIG_ARCH_MULTIPLATFORM, we must convert all users of the Samsung private DMA interface to the generic dmaengine API. This version of the patch adds the generic dmaengine API as an alternative to the existing samsung specific one. Once all the older platforms provide support for the common dmaengine interfaces, we can remove the old code. There's generic ASoC dmaengine code which should be used instead of open coding this. Lars-Peter Clausen and Lee Jones have been working on making this a totally generic driver, right now it's a library. Ok, I see. I'll drop this patch from my series then and will let someone else handle this driver in 3.11. We can probably live without sound support in 3.10 when running a multiplatform kernel, and it will keep working for exynos-only kernels without the patch. I actually had a look at how the Samsung PCM driver a couple of days back, but I didn't fully grasp how things work with the secondary TX channel for the i2s driver and to make it work with the generic dmaengine PCM driver. The code handling this in the i2s driver seems to be rather messy with lots of ifs and elses. Also things would have would be a lot easier if the dt bindings had used two subnodes each with their own 'dmas' property. - Lars -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC: PATCH 2/2] iio: adc: exynos_adc: Handle timeout and race conditions
On 04/03/2013 07:06 PM, Doug Anderson wrote: Lars, On Sat, Mar 16, 2013 at 7:41 AM, Lars-Peter Clausen l...@metafoo.de wrote: I think you still need the mutex for serialization, otherwise the requests would just cancel each other out. Btw. what happens if you start a conversion while another is still in progress? Is it possible to abort a conversion? I was thinking that the spinlock would just replace the mutex for the purposes of serialization. Since we sleep inside the protected section we need to use a mutex. I stepped back a bit, though, and I'm wondering if we're over-thinking things. The timeout case should certainly be handled properly (thanks for pointing it out), but getting a timeout is really not expected and adding a lot of extra overhead to handle it elegantly seems a bit much? Specifically, the mutex means that we have one user of the ADC at a time, and ADC conversion has nothing variable about it. The user manual that I have access to talks about 12-bit conversion happening in 1 microsecond with a 5MHz input clock or 5 microseconds with a 1MHz input clock. Even if someone has clocks configured very differently, it would be hard to imagine a conversion actually taking a full second. ...so that means that if the timeout actually fires then something else fairly drastic has gone wrong. It's _very_ unlikely that the IRQ will still go off for this conversion sometime in the future. It's not the timeout case I'm worried about, but the case where the transfer is interrupted by the user. Even though it is rather unlikely for the problem to occur we should still try to avoid it, this is one of these annoying heisenbugs that happen once in a while and nobody is able to reproduce them. To me, total modifications to what's landed already ought to be: * Change timeout to long (from unsigned long) * Make sure we return errors (negative results) from wait_for_completion_interruptible_timeout() properly. * If we get back a value of 0 from wait_for_completion_interruptible_timeout() then we should print a warning and attempt machinations to reset the ADC. Without ever seeing real-world situtations that would cause a real timeout these machinations would be a bit of a guess (is resetting the adc useful when it's more likely that someone accidentally messed with the clock tree or power gated the ADC?)... ...or perhaps a warning and a TODO in the code would be enough? Thoughts? I think most of this is already implemented and Naveen sent a patch to reset the controller in case of a timeout, which is a good idea and works fine, but you still should handle the case where the user aborted the transfer. Just resetting the core should work as well in that case. - Lars -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC: PATCH 2/2] iio: adc: exynos_adc: Handle timeout and race conditions
On 04/05/2013 04:56 PM, Doug Anderson wrote: Lars, On Fri, Apr 5, 2013 at 1:53 AM, Lars-Peter Clausen l...@metafoo.de wrote: Since we sleep inside the protected section we need to use a mutex. Ah, good point. It's not the timeout case I'm worried about, but the case where the transfer is interrupted by the user. Even though it is rather unlikely for the problem to occur we should still try to avoid it, this is one of these annoying heisenbugs that happen once in a while and nobody is able to reproduce them. Yes, of course. Then we can also get extra confidence that the reset logic works well by stressing out this case... :) This makes me think, though. Given how fast we expect the ADC transaction to finish, would there be any benefit to making the wait non-interruptible and then shortening the timeout a whole lot. If we shortened to 1ms then we're really not non-interruptible for very long and there's less chance of subtle bugs in the way that reset works. Yes, that could also work. - Lars -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC: PATCH 2/2] iio: adc: exynos_adc: Handle timeout and race conditions
On 03/16/2013 01:37 AM, Doug Anderson wrote: On Fri, Mar 15, 2013 at 2:53 PM, Lars-Peter Clausen l...@metafoo.de wrote: What exactly is the spinlock protecting against here? Concurrent runs of exynos_adc_isr? This is probably not issue in the first place. What you want to protect against is that completion is completed between the call to INIT_COMPLETION() and the start of a new conversion. So the sections that need to be under the spinlock are the complete call here and the point from INIT_COMPLETION until the transfer is started in exynos_read_raw(). Make sure to use spin_lock_irq there. ...and at that point I _think_ you won't also need the mutex. A reasonable way to test to see if you've got this all correct would be to: * Start two processes that are reading from different ADCs that will report very different values (maybe add a device tree node for adc1 or adc7 and use those since they're not really connected to thermistors?). * Have your two processes read as fast as they can. This could just be while true; do cat /sys/class/hwmon/hwmon0/device/temp1_input; done * Decrease your timeout and maybe(?) sprinkle some random udelays in the irq handler so that the timeouts happen sometimes but not others. * Periodically cancel one of the readers with Ctrl-C If all is working well then you should always get back the right value from the right reader (and get no crashes). I think you still need the mutex for serialization, otherwise the requests would just cancel each other out. Btw. what happens if you start a conversion while another is still in progress? Is it possible to abort a conversion? - Lars -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC: PATCH 2/2] iio: adc: exynos_adc: Handle timeout and race conditions
On 03/15/2013 05:26 PM, Naveen Krishna Chatradhi wrote:
This patch does the following
1. Handle the return values of wait_for_completion_interruptible_timeout
2. Add spin locks to avoid race conditions during ISR.
Signed-off-by: Naveen Krishna Chatradhi ch.nav...@samsung.com
Cc: Doug Anderson diand...@chromium.org
Cc: Lars-Peter Clausen l...@metafoo.de
---
Discussion thread for this patch can be found at
http://www.gossamer-threads.com/lists/linux/kernel/1693284?page=last
I've not seen any reference to spin lock usage in IIO.
Kindly, suggest me if there is a better way to avoid the race.
Thanks,
Naveen
drivers/iio/adc/exynos_adc.c | 14 --
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index ed6fdd7..4de28ae 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -91,6 +91,7 @@ struct exynos_adc {
struct completion completion;
+ spinlock_t reg_lock;
u32 value;
unsigned intversion;
};
@@ -117,7 +118,7 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
long mask)
{
struct exynos_adc *info = iio_priv(indio_dev);
- unsigned long timeout;
+ long timeout;
u32 con1, con2;
if (mask != IIO_CHAN_INFO_RAW)
@@ -143,15 +144,19 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
ADC_V1_CON(info-regs));
}
+ INIT_COMPLETION(info-completion);
+
This needs to happen before you start the transfer.
timeout = wait_for_completion_interruptible_timeout
(info-completion, EXYNOS_ADC_TIMEOUT);
+
*val = info-value;
mutex_unlock(indio_dev-mlock);
if (timeout == 0)
return -ETIMEDOUT;
-
+ else if (timeout 0)
+ return timeout;
return IIO_VAL_INT;
}
@@ -159,6 +164,8 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
{
struct exynos_adc *info = (struct exynos_adc *)dev_id;
+ spin_lock(info-reg_lock);
+
/* Read value */
info-value = readl(ADC_V1_DATX(info-regs))
ADC_DATX_MASK;
@@ -170,6 +177,8 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
complete(info-completion);
+ spin_unlock(info-reg_lock);
+
What exactly is the spinlock protecting against here? Concurrent runs of
exynos_adc_isr? This is probably not issue in the first place.
What you want to protect against is that completion is completed between the
call to INIT_COMPLETION() and the start of a new conversion. So the sections
that need to be under the spinlock are the complete call here and the point
from INIT_COMPLETION until the transfer is started in exynos_read_raw(). Make
sure to use spin_lock_irq there.
return IRQ_HANDLED;
}
@@ -327,6 +336,7 @@ static int exynos_adc_probe(struct platform_device *pdev)
else
indio_dev-num_channels = MAX_ADC_V2_CHANNELS;
+ spin_lock_init(info-reg_lock);
ret = iio_device_register(indio_dev);
if (ret)
goto err_irq;
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc 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] iio: adc: exynos5_adc: fix compilation warnings
On 03/13/2013 07:01 PM, Doug Anderson wrote: Naveen, On Tue, Mar 12, 2013 at 9:48 PM, Naveen Krishna Chatradhi ch.nav...@samsung.com wrote: Doug, There was a comment from Lars regarding the match not being NULL, if driver depends on CONFIG_OF. So, i've removed the NULL check in v2 of this patch. https://patchwork.kernel.org/patch/841/ I'm checking the return value of get_version() for -ve values before assigning to info-version. So, i left the (unsigned int) unchanged. Hmmm, I guess this was the point that confused me. I went back and agree with Lars--it can't be NULL. ...but that means that exynos_adc_get_version() can't return an error, so why are we checking for an error? Agreed. Adding the dependency on OF in Kconfig should be all that is needed. - Lars -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc 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] iio: adc: exynos5_adc: fix compilation warnings
On 03/13/2013 07:23 PM, Doug Anderson wrote: Lars, On Wed, Mar 13, 2013 at 11:11 AM, Lars-Peter Clausen l...@metafoo.de wrote: Agreed. Adding the dependency on OF in Kconfig should be all that is needed. I think changing the timeout from 'unsigned long' to 'long' is also legit (to match the actual type returned) and a good idea. -Doug Yes, but that's a different issue and to be honest I didn't even realize that the patch was trying to fix this as well. In my opinion it's best to split this up into two patches one which fixes the OF dependency. The other fixing the timeout issue. Cause there is more to it than just changing the type. wait_for_completion_interruptible_timeout() may return 1) 0, if there was a timeout, waiting for the completion 2) 0, if the completion was completeted, the returned value the remaining time. 3) 0, If it was interrupt while waiting for the completion The code currently only handles 1) and 2), but it also needs to handle 3). Since the completion has not been completed in case 3. E.g. something like this should work: if (timeout == 0) return -ETIMEDOUT; else if(timeout 0) return timeout; return 0; - Lars -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc 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] iio: adc: exynos5_adc: fix compilation warnings
On 03/13/2013 07:35 PM, Lars-Peter Clausen wrote: On 03/13/2013 07:23 PM, Doug Anderson wrote: Lars, On Wed, Mar 13, 2013 at 11:11 AM, Lars-Peter Clausen l...@metafoo.de wrote: Agreed. Adding the dependency on OF in Kconfig should be all that is needed. I think changing the timeout from 'unsigned long' to 'long' is also legit (to match the actual type returned) and a good idea. -Doug Yes, but that's a different issue and to be honest I didn't even realize that the patch was trying to fix this as well. In my opinion it's best to split this up into two patches one which fixes the OF dependency. The other fixing the timeout issue. Cause there is more to it than just changing the type. wait_for_completion_interruptible_timeout() may return 1) 0, if there was a timeout, waiting for the completion 2) 0, if the completion was completeted, the returned value the remaining time. 3) 0, If it was interrupt while waiting for the completion The code currently only handles 1) and 2), but it also needs to handle 3). Since the completion has not been completed in case 3. E.g. something like this should work: if (timeout == 0) return -ETIMEDOUT; else if(timeout 0) return timeout; return 0; I just saw, there is another issue related to this. The driver should call INIT_COMPLETION(info-completion) before starting the conversion. Otherwise there may be a problem if we got interrupted while waiting for the interrupt. E.g. imagine the following sequence. 1) Start conversion 2) Wait for completion 3) Abort waiting 4) Interrupt kicks in and marks the completion as done Now if another conversion is started the completion will already be completed and wait_for_completion_interruptible_timeout() will return right away without waiting for the conversion to be finished. - Lars -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] iio: adc: exynos5_adc: fix compilation warnings
On 03/06/2013 05:11 AM, Naveen Krishna Chatradhi wrote:
From: Naveen Krishna Ch ch.nav...@samsung.com
Fixes the compilation warnings and potential NULL pointer
dereferencing pointed out by Dan Carpenter.
I'd say that's a rather un-potential NULL pointer dereferencing, but if it
makes the static checkers happy, why not. Since the same match table is used to
match the device, probe won't be called unless there is a match, so
of_match_node() will never return NULL in this case.
Signed-off-by: Naveen Krishna Ch ch.nav...@samsung.com
---
drivers/iio/adc/exynos_adc.c | 24 +---
1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index ed6fdd7..67d07aa 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -92,7 +92,7 @@ struct exynos_adc {
struct completion completion;
u32 value;
- unsigned intversion;
+ unsigned intversion;
};
static const struct of_device_id exynos_adc_match[] = {
@@ -102,12 +102,15 @@ static const struct of_device_id exynos_adc_match[] = {
};
MODULE_DEVICE_TABLE(of, exynos_adc_match);
-static inline unsigned int exynos_adc_get_version(struct platform_device
*pdev)
+static inline int exynos_adc_get_version(struct platform_device *pdev)
{
const struct of_device_id *match;
match = of_match_node(exynos_adc_match, pdev-dev.of_node);
- return (unsigned int)match-data;
+ if (!match)
+ return -ENODEV;
+
+ return (int)match-data;
}
static int exynos_read_raw(struct iio_dev *indio_dev,
@@ -117,7 +120,7 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
long mask)
{
struct exynos_adc *info = iio_priv(indio_dev);
- unsigned long timeout;
+ int timeout;
u32 con1, con2;
if (mask != IIO_CHAN_INFO_RAW)
@@ -255,7 +258,7 @@ static int exynos_adc_probe(struct platform_device *pdev)
struct iio_dev *indio_dev = NULL;
struct resource *mem;
int ret = -ENODEV;
- int irq;
+ int irq, version;
if (!np)
return ret;
@@ -268,6 +271,15 @@ static int exynos_adc_probe(struct platform_device *pdev)
info = iio_priv(indio_dev);
+ version = exynos_adc_get_version(pdev);
+ if (version 0) {
+ dev_err(pdev-dev, no matching of_node, err = %d\n, version);
+ ret = version;
+ goto err_iio;
+ }
+
+ info-version = version;
+
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
info-regs = devm_request_and_ioremap(pdev-dev, mem);
@@ -311,8 +323,6 @@ static int exynos_adc_probe(struct platform_device *pdev)
goto err_irq;
}
- info-version = exynos_adc_get_version(pdev);
-
platform_set_drvdata(pdev, indio_dev);
indio_dev-name = dev_name(pdev-dev);
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7] iio: adc: add exynos adc driver under iio framwork
On 02/15/2013 07:56 AM, Naveen Krishna Chatradhi wrote:
This patch adds New driver to support:
1. Supports ADC IF found on EXYNOS4412/EXYNOS5250
and future SoCs from Samsung
2. Add ADC driver under iio/adc framework
3. Also adds the Documentation for device tree bindings
Signed-off-by: Naveen Krishna Chatradhi ch.nav...@samsung.com
Looks good.
Reviewed-by: Lars-Peter Clausen l...@metafoo.de
One minor thing though, there are a couple of places where you break a line
into multiple lines, even though the line fits easily inside the 80 chars
per line limit. In my opinion this doesn't help the legibility of the code.
E.g.:
+ info-value = readl(ADC_V1_DATX(info-regs))
+ ADC_DATX_MASK;
There is no need to respin the patch just for this, but if you happen to
make another version of the patch, that's something to consider.
---
Changes since v1:
1. Fixed comments from Lars
2. Added support for ADC on EXYNOS5410
Changes since v2:
1. Changed the instance name for (struct iio_dev *) to indio_dev
2. Changed devm_request_irq to request_irq
Few doubts regarding the mappings and child device handling.
Kindly, suggest me better methods.
Changes since v3:
1. Added clk_prepare_disable and regulator_disable calls in _remove()
2. Moved init_completion before irq_request
3. Added NULL pointer check for devm_request_and_ioremap() return value.
4. Use number of channels as per the ADC version
5. Change the define ADC_V1_CHANNEL to ADC_CHANNEL
6. Update the Documentation to include EXYNOS5410 compatible
Changes since v4:
1. if devm_request_and_ioremap() failes, free iio_device before returning
Changes since v5:
1. Fixed comments from Olof (ADC hardware version handling)
2. Rebased on top of comming OF framework for IIO by Guenter Roeck.
Changes since v6:
1. Addressed comments from Lars-Peter Clausen
btw. these kind of change logs are not really helpful, it would be better to
list the actual changes made.
.../bindings/arm/samsung/exynos5-adc.txt | 42 ++
drivers/iio/adc/Kconfig|7 +
drivers/iio/adc/Makefile |1 +
drivers/iio/adc/exynos_adc.c | 438
4 files changed, 488 insertions(+)
.../devicetree/bindings/arm/samsung/exynos-adc.txt | 52 +++
drivers/iio/adc/Kconfig|7 +
drivers/iio/adc/Makefile |1 +
drivers/iio/adc/exynos_adc.c | 440
4 files changed, 500 insertions(+)
create mode 100644
Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
create mode 100644 drivers/iio/adc/exynos_adc.c
diff --git a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
new file mode 100644
index 000..f686378
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
@@ -0,0 +1,52 @@
+Samsung Exynos Analog to Digital Converter bindings
+
+This devicetree binding are for the new adc driver written fori
+Exynos4 and upward SoCs from Samsung.
+
+New driver handles the following
+1. Supports ADC IF found on EXYNOS4412/EXYNOS5250
+ and future SoCs from Samsung
+2. Add ADC driver under iio/adc framework
+3. Also adds the Documentation for device tree bindings
+
+Required properties:
+- compatible:Must be samsung,exynos-adc-v1
+ for exynos4412/5250 controllers.
+ Must be samsung,exynos-adc-v2 for
+ future controllers.
+- reg: Contains ADC register address range (base
address and
+ length).
+- interrupts:Contains the interrupt information for the
timer. The
+ format is being dependent on which interrupt controller
+ the Samsung device uses.
+- #io-channel-cells = 1; As ADC has multiple outputs
+
+Note: child nodes can be added for auto probing from device tree.
+
+Example: adding device info in dtsi file
+
+adc: adc@12D1 {
+ compatible = samsung,exynos-adc-v1;
+ reg = 0x12D1 0x100;
+ interrupts = 0 106 0;
+ #io-channel-cells = 1;
+ io-channel-ranges;
+};
+
+
+Example: Adding child nodes in dts file
+
+adc@12D1 {
+
+ /* NTC thermistor is a hwmon device */
+ ncp15wb473@0 {
+ compatible = ntc,ncp15wb473;
+ pullup-uV = 180;
+ pullup-ohm = 47000;
+ pulldown-ohm = 0;
+ io-channels = adc 4;
+ };
+};
+
+Note: Does not apply to ADC driver under arch/arm/plat-samsung/
+Note: The child node can be added under the adc node or seperately.
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index e372257..04311f8 100644
Re: [PATCH v7] iio: adc: add exynos adc driver under iio framwork
On 02/15/2013 02:17 PM, Naveen Krishna Ch wrote: On 15 February 2013 18:43, Lars-Peter Clausen l...@metafoo.de wrote: On 02/15/2013 07:56 AM, Naveen Krishna Chatradhi wrote: This patch adds New driver to support: 1. Supports ADC IF found on EXYNOS4412/EXYNOS5250 and future SoCs from Samsung 2. Add ADC driver under iio/adc framework 3. Also adds the Documentation for device tree bindings Signed-off-by: Naveen Krishna Chatradhi ch.nav...@samsung.com Looks good. Reviewed-by: Lars-Peter Clausen l...@metafoo.de One minor thing though, there are a couple of places where you break a line into multiple lines, even though the line fits easily inside the 80 chars per line limit. In my opinion this doesn't help the legibility of the code. E.g.: + info-value = readl(ADC_V1_DATX(info-regs)) + ADC_DATX_MASK; There is no need to respin the patch just for this, but if you happen to make another version of the patch, that's something to consider. --- Changes since v1: 1. Fixed comments from Lars 2. Added support for ADC on EXYNOS5410 Changes since v2: 1. Changed the instance name for (struct iio_dev *) to indio_dev 2. Changed devm_request_irq to request_irq Few doubts regarding the mappings and child device handling. Kindly, suggest me better methods. Changes since v3: 1. Added clk_prepare_disable and regulator_disable calls in _remove() 2. Moved init_completion before irq_request 3. Added NULL pointer check for devm_request_and_ioremap() return value. 4. Use number of channels as per the ADC version 5. Change the define ADC_V1_CHANNEL to ADC_CHANNEL 6. Update the Documentation to include EXYNOS5410 compatible Changes since v4: 1. if devm_request_and_ioremap() failes, free iio_device before returning Changes since v5: 1. Fixed comments from Olof (ADC hardware version handling) 2. Rebased on top of comming OF framework for IIO by Guenter Roeck. Changes since v6: 1. Addressed comments from Lars-Peter Clausen btw. these kind of change logs are not really helpful, it would be better to list the actual changes made. Hello Lars, No other changes from my side. But, I can send another version. Do you want me to list the latest change alone instead of the whole change list ? Hi, No need to resend the patch, this is just something to consider for the future. A changelog entry which reads like Addressed Jon Does comments is not really useful since most people will probably not know or not longer remember all the details of those comments, instead a nice list of all the changes which have been made is much better. E.g.: Changes since v6: * Fixed debugfs_read_reg * Introduced timeout when waiting for the data ready IRQ * Slightly reformatted exynos_read_raw for better legibility - Lars -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: iio: adc: add exynos5 adc driver under iio framwork
On 02/13/2013 02:16 PM, Naveen Krishna Ch wrote:
Please ignore the unfinished previous mail.
On 13 February 2013 08:18, Naveen Krishna Ch naveenkrishna...@gmail.com
wrote:
On 13 February 2013 02:37, Guenter Roeck li...@roeck-us.net wrote:
On Wed, Jan 23, 2013 at 04:58:06AM -, Naveen Krishna Chatradhi wrote:
This patch add an ADC IP found on EXYNOS5 series socs from Samsung.
Also adds the Documentation for device tree bindings.
Signed-off-by: Naveen Krishna Chatradhi ch.nav...@samsung.com
---
Changes since v1:
1. Fixed comments from Lars
2. Added support for ADC on EXYNOS5410
Changes since v2:
1. Changed the instance name for (struct iio_dev *) to indio_dev
2. Changed devm_request_irq to request_irq
Few doubts regarding the mappings and child device handling.
Kindly, suggest me better methods.
.../bindings/arm/samsung/exynos5-adc.txt | 37 ++
drivers/iio/adc/Kconfig|7 +
drivers/iio/adc/Makefile |1 +
drivers/iio/adc/exynos5_adc.c | 464
4 files changed, 509 insertions(+)
create mode 100644
Documentation/devicetree/bindings/arm/samsung/exynos5-adc.txt
create mode 100644 drivers/iio/adc/exynos5_adc.c
diff --git a/Documentation/devicetree/bindings/arm/samsung/exynos5-adc.txt
b/Documentation/devicetree/bindings/arm/samsung/exynos5-adc.txt
new file mode 100644
index 000..9a5b515
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/samsung/exynos5-adc.txt
@@ -0,0 +1,37 @@
+Samsung Exynos5 Analog to Digital Converter bindings
+
+Required properties:
+- compatible:Must be samsung,exynos5250-adc for
exynos5250 controllers.
+- reg: Contains ADC register address range (base
address and
+ length).
+- interrupts:Contains the interrupt information for the
timer. The
+ format is being dependent on which interrupt
controller
+ the Samsung device uses.
+
+Note: child nodes can be added for auto probing from device tree.
+
+Example: adding device info in dtsi file
+
+adc@12D1 {
+ compatible = samsung,exynos5250-adc;
+ reg = 0x12D1 0x100;
+ interrupts = 0 106 0;
+ #address-cells = 1;
+ #size-cells = 1;
+ ranges;
+};
+
+
+Example: Adding child nodes in dts file
+
+adc@12D1 {
+
+ /* NTC thermistor is a hwmon device */
+ ncp15wb473@0 {
+ compatible = ntc,ncp15wb473;
+ reg = 0x0;
+ pullup-uV = 180;
+ pullup-ohm = 47000;
+ pulldown-ohm = 0;
+ };
+};
How about:
adc: adc@12D1 {
compatible = samsung,exynos5250-adc;
reg = 0x12D1 0x100;
interrupts = 0 106 0;
#io-channel-cells = 1;
};
...
ncp15wb473@0 {
compatible = ntc,ncp15wb473;
reg = 0x0; /* is this needed ? */
io-channels = adc 0;
io-channel-names = adc;
pullup-uV = 180; /* uV or uv ? */
pullup-ohm = 47000;
pulldown-ohm = 0;
};
The ncp15wb473 driver would then use either iio_channel_get_all() to get
the iio
channel list or, if it only supports one adc channel per instance,
iio_channel_get().
In that context, it would probably make sense to rework the ntc_thermistor
driver to support both DT as well as direct instantiation using access
functions
and platform data (as it does today).
Also see https://patchwork.kernel.org/patch/2112171/.
Hello Guenter,
I've rebase my adc driver on top of your (OF for IIO patch)
My setup is like the below one. kindly, help me find the right device
tree node params
One ADC controller with 8 channels,
4 NTC thermistors are connected to channel 3, 4, 5 and 6 of ADC respectively
ADC ch - 0
ADC ch - 1
ADC ch - 2
ADC ch - 3 --NTC
ADC ch - 4 --NTC
ADC ch - 5 --NTC
ADC ch - 6 --NTC
ADC ch - 7
I've started off with something like this.
adc0: adc@12D1 {
compatible = samsung,exynos5250-adc;
reg = 0x12D1 0x100;
interrupts = 0 106 0;
#io-channel-cells = 1;
};
adc0: adc@12D1 {
This is a copy paste error, right? you only have one dt node for the adc?
vdd-supply = buck5_reg;
ncp15wb473@0 {
compatible = ntc,ncp15wb473;
io-channels = adc0 3;
io-channel-names = adc3;
pullup-uV = 180;
pullup-ohm = 47000;
pulldown-ohm = 0;
id = 3;
};
Re: iio: adc: add exynos5 adc driver under iio framwork
On 02/13/2013 02:53 PM, Naveen Krishna Ch wrote: [...] ADC driver will use of_platform_populate() to populate the child nodes (ntc thermistors in my case) I've modified the NTC driver to support DT. in probe chan = iio_channel_get(pdev-dev, adcX); and using id field to use respective ADC channel to do the raw_read() The beauty of the interface is that the consumer doesn't need to know the number of the channel it is using. This is already fully described in the io-channels property. Since you only have one channel per consumer just use iio_chanel_get(pdev-dev, NULL) Right this helped me get the channels properly. I've a doubt in the driver posted at https://lkml.org/lkml/2013/1/24/2 i don't need to use this anymore right use iio_map_array_register() Right. Thats so simple then.. Thanks Yes, if you are using devicetree you don't need to use iio_map_array_register() anymore :) - Lars -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] iio: adc: add exynos5 adc driver under iio framwork
On 01/24/2013 01:42 AM, Doug Anderson wrote:
Lars,
On Wed, Jan 23, 2013 at 4:52 AM, Lars-Peter Clausen l...@metafoo.de wrote:
Few doubts regarding the mappings and child device handling.
Kindly, suggest me better methods.
The patch looks mostly good now. As for the mappings, the problem is that we
currently do not have any device tree bindings for IIO. So a proper solution
would be to add dt bindings for IIO.
Can you explain more how you think this would work? It seems like
just having child nodes as platform drivers would be OK (to me) and
having them instantiated with of_platform_populate() seems reasonable.
...but the one thing that is missing is a way for children to get
access to the channel properly.
The children have access to the ADC struct device (it is their
parent device) and have a channel number (their reg field), but
there is no API call to map this to a struct iio_channel. Is that
all that's needed in this case? ...or are you envisioning something
more?
Well, the idea is that the consumer doesn't need to know the channel number.
That's what the mapping takes care of. It basically creates a consumer alias
for the channel. When requesting the channel the consumer always uses the
same name.
iio_channel_get(dev_name(pdev-dev), voltage);
And the mapping contains an entry which maps the tuple of device name and
channel name to a real IIO channel which as been registered by a IIO device.
Note if there is only one channel you can also just use NULL for the channel
name. This is similar to how clocks are managed with the clk framework.
Now for the dt bindings I think we should stick to something similar to what
the clk framework does.
E.g.
adc: adc@12D1 {
#io-channel-cells = 1;
io-channel-output-names = adc1, adc2, ...;
ncp15wb473@0 {
compatible = ntc,ncp15wb473;
...
io-channels = adc 0; // First ADC channel
io-channel-names = voltage;
};
ncp15wb473@0 {
compatible = ntc,ncp15wb473;
...
io-channels = adc 1; // Second ADC channel
io-channel-names = voltage;
}
};
io-channel-output-names and io-channel-names can be optional. In the case
where there is only one channel it's not really necessary to use
io-channel-names.
- Lars
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] iio: adc: add exynos5 adc driver under iio framwork
On 01/24/2013 03:20 PM, Naveen Krishna Ch wrote:
On 24 January 2013 15:24, Lars-Peter Clausen l...@metafoo.de wrote:
On 01/24/2013 01:42 AM, Doug Anderson wrote:
Lars,
On Wed, Jan 23, 2013 at 4:52 AM, Lars-Peter Clausen l...@metafoo.de wrote:
Few doubts regarding the mappings and child device handling.
Kindly, suggest me better methods.
The patch looks mostly good now. As for the mappings, the problem is that
we
currently do not have any device tree bindings for IIO. So a proper
solution
would be to add dt bindings for IIO.
Can you explain more how you think this would work? It seems like
just having child nodes as platform drivers would be OK (to me) and
having them instantiated with of_platform_populate() seems reasonable.
...but the one thing that is missing is a way for children to get
access to the channel properly.
The children have access to the ADC struct device (it is their
parent device) and have a channel number (their reg field), but
there is no API call to map this to a struct iio_channel. Is that
all that's needed in this case? ...or are you envisioning something
more?
Well, the idea is that the consumer doesn't need to know the channel number.
That's what the mapping takes care of. It basically creates a consumer alias
for the channel. When requesting the channel the consumer always uses the
same name.
iio_channel_get(dev_name(pdev-dev), voltage);
And the mapping contains an entry which maps the tuple of device name and
channel name to a real IIO channel which as been registered by a IIO device.
Note if there is only one channel you can also just use NULL for the channel
name. This is similar to how clocks are managed with the clk framework.
Now for the dt bindings I think we should stick to something similar to what
the clk framework does.
E.g.
adc: adc@12D1 {
#io-channel-cells = 1;
io-channel-output-names = adc1, adc2, ...;
ncp15wb473@0 {
compatible = ntc,ncp15wb473;
...
io-channels = adc 0; // First ADC channel
io-channel-names = voltage;
};
ncp15wb473@0 {
compatible = ntc,ncp15wb473;
...
io-channels = adc 1; // Second ADC channel
io-channel-names = voltage;
}
};
Hello Lars,
I've a doubt here
How do i find the child dev_name for iio_map_array_register();
cause the child devices are probed during of_platform_populate();
and during the probe the child calls
iio_channel_get(dev_name(pdev-dev), voltage);
The child device nodes are ncp15wb473.0 and ncp15wb473.1 in this case.
But, this may change.
Hi,
I think we should handle the devicetree channel mapping in the IIO core and
not in the individual drivers. If we handle it in the core we do not need to
create a iio_map and won't need to know the name of the consumer.
You'd basically need to check whether the device requesting the IIO channel
has a of_node. If it has, check if it has an io-channels attribute. If it
also has an io-channels attribute lookup the IIO device by the phandle and
create a iio_channel for the nth channel of that device.
If either the device has no of_node or no io-channels attribute fallback to
using the iio_map based lookup method.
This would require one API change though, iio_channel_get would need to take
a device instead of the device name so it has access to both the device name
and the device node. Jonathan: any particular reason why you choose to let
iio_channel_get the device name instead of the device itself?
- Lars
Kindly, help.
Assume we have a device tree like this
adc@12D1 {
#io-channel-cells = 1;
io-channel-output-names = adc0, adc1, adc2,
adc3, adc4, adc5,
adc6, adc7, adc8, adc9;
ncp15wb473@0 {
compatible = ntc,ncp15wb473;
reg = 0x0;
io-channel-names = voltage;
pullup-uV = 180;
pullup-ohm = 47000;
pulldown-ohm = 0;
};
ncp15wb473@1 {
compatible = ntc,ncp15wb473;
reg = 0x1;
io-channel-names = voltage;
pullup-uV = 180;
pullup-ohm = 47000;
pulldown-ohm = 0;
};
};
io-channel-output-names and io-channel-names can be optional. In the case
where there is only one channel it's not really necessary to use
io-channel-names.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] iio: adc: add exynos5 adc driver under iio framwork
On 01/24/2013 05:12 PM, Doug Anderson wrote:
Lars,
Thank you for your comments / thoughts...
Hi,
On Thu, Jan 24, 2013 at 1:54 AM, Lars-Peter Clausen l...@metafoo.de wrote:
adc: adc@12D1 {
#io-channel-cells = 1;
io-channel-output-names = adc1, adc2, ...;
ncp15wb473@0 {
compatible = ntc,ncp15wb473;
...
io-channels = adc 0; // First ADC channel
I'm not an expert, but I think the typical way is:
* No need to include a handle to adc. It's logically our parent. In
a similar way i2c devices don't specify their parent bus--they are
just listed under it.
* The 0 should be specified with reg = 0
The relationship between the IIO sensor device and the consumer device is
not always a parent child relationship. In this case it makes sense to have
the ADC as the parent for the thermistors. But for other cases this may not
be true. E.g. take a touchscreen or power monitoring platform device which
uses the IIO device to do measurements.
To implement this I'd imagine that we'll need a new API call, right?
In this case the thermistor driver won't know the name of the channel.
It can find the ADC (the struct device and probably other things) and
knows a channel index. Am I understanding properly?
This can be done by adding a new api call, but it would be best if both dt
and non-dt based consumers can use the same function. I outlined one
possible solution how this can be done in the previous mail to Naveen.
- Lars
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] iio: adc: add exynos5 adc driver under iio framwork
On 01/24/2013 08:15 PM, Tomasz Figa wrote:
Hi,
On Thursday 24 of January 2013 19:19:57 Lars-Peter Clausen wrote:
On 01/24/2013 05:12 PM, Doug Anderson wrote:
Lars,
Thank you for your comments / thoughts...
Hi,
On Thu, Jan 24, 2013 at 1:54 AM, Lars-Peter Clausen l...@metafoo.de
wrote:
adc: adc@12D1 {
#io-channel-cells = 1;
io-channel-output-names = adc1, adc2, ...;
ncp15wb473@0 {
compatible = ntc,ncp15wb473;
...
io-channels = adc 0; // First ADC channel
I'm not an expert, but I think the typical way is:
* No need to include a handle to adc. It's logically our parent. In
a similar way i2c devices don't specify their parent bus--they are
just listed under it.
* The 0 should be specified with reg = 0
The relationship between the IIO sensor device and the consumer device
is not always a parent child relationship. In this case it makes sense
to have the ADC as the parent for the thermistors. But for other cases
this may not be true. E.g. take a touchscreen or power monitoring
platform device which uses the IIO device to do measurements.
The policy is to use children with reg property only inside a node
representing a bus controller through which the child device is being
accessed (like I2C, SPI).
I would see IIO bindings similar to what we have with GPIOs, interrupts or
regulators, so io-channels = iio-controller channel seems fine (or
rather iio-channels) with the node under appropriate parent.
IIO is a very Linux specific term, the device tree bindings should be as OS
agnostic as possible, so io-channels is probably the better term.
To implement this I'd imagine that we'll need a new API call, right?
In this case the thermistor driver won't know the name of the channel.
It can find the ADC (the struct device and probably other things) and
knows a channel index. Am I understanding properly?
This can be done by adding a new api call, but it would be best if both
dt and non-dt based consumers can use the same function. I outlined one
possible solution how this can be done in the previous mail to Naveen.
In case of the solution I mentioned, implementation would be almost
identical to what is done with GPIOs (see drivers/gpio/gpiolib-of.c).
Although similar to the GPIO bindings, the clk bindings are in my opinion an
even better example.
- Lars
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] iio: adc: add exynos5 adc driver under iio framwork
Hi,
On 01/21/2013 02:37 PM, Naveen Krishna Chatradhi wrote:
This patch add an ADC IP found on EXYNOS5 series socs from Samsung.
Also adds the Documentation for device tree bindings.
[...]
diff --git a/drivers/iio/adc/exynos5_adc.c b/drivers/iio/adc/exynos5_adc.c
new file mode 100644
index 000..cd33ea2
--- /dev/null
+++ b/drivers/iio/adc/exynos5_adc.c
@@ -0,0 +1,412 @@
+/*
+ * exynos5_adc.c - Support for ADC in EXYNOS5 SoCs
+ *
+ * 8-channel, 10/12-bit ADC
+ *
+ * Copyright (C) 2013 Naveen Krishna Chatradhi ch.nav...@samsung.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include linux/module.h
+#include linux/platform_device.h
+#include linux/interrupt.h
+#include linux/delay.h
+#include linux/kernel.h
+#include linux/slab.h
+#include linux/io.h
+#include linux/clk.h
+#include linux/completion.h
+#include linux/of.h
+#include linux/of_irq.h
+#include linux/regulator/consumer.h
+#include linux/of_platform.h
+
+#include linux/iio/iio.h
+#include linux/iio/machine.h
+#include linux/iio/driver.h
+
+/* Samsung ADC registers definitions */
+#define EXYNOS_ADC_CON(x)((x) + 0x00)
+#define EXYNOS_ADC_DLY(x)((x) + 0x08)
+#define EXYNOS_ADC_DATX(x) ((x) + 0x0C)
+#define EXYNOS_ADC_INTCLR(x) ((x) + 0x18)
+#define EXYNOS_ADC_MUX(x)((x) + 0x1c)
+
+/* Bit definitions for EXYNOS_ADC_MUX: */
+#define ADC_RES (1u 16)
+#define ADC_ECFLG(1u 15)
+#define ADC_PRSCEN (1u 14)
+#define ADC_PRSCLV(x)(((x) 0xFF) 6)
+#define ADC_PRSCVLMASK (0xFF 6)
+#define ADC_STANDBY (1u 2)
+#define ADC_READ_START (1u 1)
+#define ADC_EN_START (1u 0)
You are a bit inconsistent with your prefixes, sometimes you use EXYNOS_ADC,
sometimes just ADC, it would be better to use the same prefix all the time.
+
+#define ADC_DATX_MASK0xFFF
+
+struct exynos5_adc {
+ void __iomem*regs;
+ struct clk *clk;
+ unsigned intirq;
+ struct regulator*vdd;
+
+ struct completion completion;
+
+ struct iio_map *map;
+ u32 value;
+ u32 prescale;
+};
+
+static const struct of_device_id exynos5_adc_match[] = {
+ { .compatible = samsung,exynos5250-adc },
+ {},
+};
+MODULE_DEVICE_TABLE(of, exynos5_adc_match);
+
+/* default maps used by iio consumer (ex: ntc-thermistor driver) */
+static struct iio_map exynos5_adc_iio_maps[] = {
+ {
+ .consumer_dev_name = 0.ncp15wb473,
+ .consumer_channel = adc_user3,
+ .adc_channel_label = adc3,
+ },
+ {},
+};
Hm... this should not be in the driver itself.
+
+static int exynos5_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val,
+ int *val2,
+ long mask)
+{
+ struct exynos5_adc *info = iio_priv(indio_dev);
+ u32 con;
+
+ if (mask == IIO_CHAN_INFO_RAW) {
+ mutex_lock(indio_dev-mlock);
+
+ /* Select the channel to be used */
+ writel(chan-address, EXYNOS_ADC_MUX(info-regs));
+ /* Trigger conversion */
+ con = readl(EXYNOS_ADC_CON(info-regs));
+ writel(con | ADC_EN_START, EXYNOS_ADC_CON(info-regs));
+
+ wait_for_completion(info-completion);
+ *val = info-value;
+
+ mutex_unlock(indio_dev-mlock);
+
+ return IIO_VAL_INT;
+ }
+
+ return -EINVAL;
+}
+
+static irqreturn_t exynos5_adc_isr(int irq, void *dev_id)
+{
+ struct exynos5_adc *info = (struct exynos5_adc *)dev_id;
+
+ /* Read value and clear irq */
+ info-value = readl(EXYNOS_ADC_DATX(info-regs))
+ ADC_DATX_MASK;
+ writel(0, EXYNOS_ADC_INTCLR(info-regs));
+
+ complete(info-completion);
+
+ return IRQ_HANDLED;
+}
+
+static int exynos5_adc_reg_access(struct iio_dev *indio_dev,
+ unsigned reg, unsigned writeval,
+ unsigned *readval)
+{
+ struct exynos5_adc *info = iio_priv(indio_dev);
+ u32 ret;
+
+
Re: [PATCH] backlight: lcd: add driver for raster-type lcd's with gpio controlled panel reset
On 01/07/2012 07:23 PM, Russell King - ARM Linux wrote: On Thu, Jan 05, 2012 at 09:12:26PM +0530, Thomas Abraham wrote: Add a lcd panel driver for simple raster-type lcd's which uses a gpio controlled panel reset. The driver controls the nRESET line of the panel using a gpio connected from the host system. The Vcc supply to the panel is (optionally) controlled using a voltage regulator. This driver excludes support for lcd panels that use a serial command interface or direct memory mapped IO interface. I'm trying to work out what kind of LCD panel this is for. I assume not the panels which would be connected to a SoC, which have a parallel interface to a frame buffer device (LCD controller)? If this is for these kinds of LCD panels, how are you handling the timing required for active panels - some of which must not be powered up without the LCD controller first being setup and enabled, and must be powered down before the LCD controller is disabled. I've seen this requirement with panels connected to ARM Ltd's development boards, and also some SoCs. This is handled by the LCD framework. Or at least should be, see this patchset http://www.spinics.net/lists/linux-fbdev/msg04503.html - Lars -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] backlight: lcd: add driver for raster-type lcd's with gpio controlled panel reset
[...]
diff --git a/Documentation/devicetree/bindings/lcd/lcd-pwrctrl.txt
b/Documentation/devicetree/bindings/lcd/lcd-pwrctrl.txt
new file mode 100644
index 000..941e2ff
--- /dev/null
+++ b/Documentation/devicetree/bindings/lcd/lcd-pwrctrl.txt
@@ -0,0 +1,39 @@
+* Power controller for simple lcd panels
+
+Some LCD panels provide a simple control interface for the host system. The
+control mechanism would include a nRESET line connected to a gpio of the host
+system and a Vcc supply line which the host can optionally be controlled
using
+a voltage regulator. Such simple panels do not support serial command
+interface (such as i2c or spi) or memory-mapped-io interface.
+
+Required properties:
+- compatible: should be 'lcd,powerctrl'
+- gpios: The GPIO number of the host system used to control the nRESET line.
+ The format of the gpio specifier depends on the gpio controller of the
+ host system.
+
+Optional properties:
+- lcd,pwrctrl-nreset-gpio-invert: When the nRESET line is asserted low, the
+ lcd panel is reset and stays in reset mode as long as the nRESET line is
+ asserted low. This is the default behaviour of most lcd panels. If a lcd
+ panel requires the nRESET line to be asserted high for panel reset, then
+ this property is used.
Maybe use OF_GPIO_ACTIVE_LOW here instead. That would make active high the
default but be a bit more consistent.
+- lcd,pwrctrl-min-uV: If a regulator controls the Vcc voltage of the lcd
panel,
+ this property specifies the minimum voltage the regulator should supply.
+ The value of this property should in in micro-volts.
+- lcd,pwrctrl-max-uV: If a regulator controls the Vcc voltage of the lcd
panel,
+ this property specifies the maximum voltage the regulator should limit to
+ on the Vcc line. The value of this property should in in micro-volts.
The min and max voltage should rather be specified through the regulator
constraints.
+- vcc-lcd-supply: phandle of the regulator that controls the vcc supply to
+ the lcd panel.
+
[...]
diff --git a/drivers/video/backlight/lcd_pwrctrl.c
b/drivers/video/backlight/lcd_pwrctrl.c
new file mode 100644
index 000..6f3110b
--- /dev/null
+++ b/drivers/video/backlight/lcd_pwrctrl.c
@@ -0,0 +1,231 @@
[...]
+static int lcd_pwrctrl_set_power(struct lcd_device *lcd, int power)
+{
+ struct lcd_pwrctrl *lp = lcd_get_data(lcd);
+ struct lcd_pwrctrl_data *pd = lp-pdata;
+ int lcd_enable, lcd_reset;
+
+ lcd_enable = (power == FB_BLANK_POWERDOWN || lp-suspended) ? 0 : 1;
+ lcd_reset = (pd-invert) ? !lcd_enable : lcd_enable;
+
+ if (IS_ERR(lp-regulator))
+ goto no_regulator;
I wouldn't use a goto here.
+
+ if (lcd_enable) {
+ if ((pd-min_uV || pd-max_uV)
+ regulator_set_voltage(lp-regulator,
+ pd-min_uV, pd-max_uV))
+ dev_info(lp-dev,
+ regulator voltage set failed\n);
+ if (regulator_enable(lp-regulator))
+ dev_info(lp-dev, failed to enable regulator\n);
+ } else {
+ regulator_disable(lp-regulator);
+ }
I think you have to make sure that the regulator enable and disable calls are
balanced.
+
+ no_regulator:
+ gpio_direction_output(lp-pdata-gpio, lcd_reset);
+ lp-power = power;
+ return 0;
+}
+
[...]
+
+#ifdef CONFIG_OF
I think you can remove all the CONFIG_OF ifdefs, the of API should stub itself
out.
+static void __devinit lcd_pwrctrl_parse_dt(struct device *dev,
+ struct lcd_pwrctrl_data *pdata)
+{
+ struct device_node *np = dev-of_node;
+
+ pdata-gpio = of_get_gpio(np, 0);
+ if (of_get_property(np, lcd,pwrctrl-nreset-gpio-invert, NULL))
+ pdata-invert = true;
+ of_property_read_u32(np, lcd,pwrctrl-min-uV, pdata-min_uV);
+ of_property_read_u32(np, lcd,pwrctrl-max-uV, pdata-max_uV);
+}
+#endif
+
+static int __devinit lcd_pwrctrl_probe(struct platform_device *pdev)
+{
+ struct lcd_pwrctrl *lp;
+ struct lcd_pwrctrl_data *pdata = pdev-dev.platform_data;
+ struct device *dev = pdev-dev;
+ int err;
+
+#ifdef CONFIG_OF
+ if (dev-of_node) {
+ pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata) {
+ dev_err(dev, memory allocation for pdata failed\n);
+ return -ENOMEM;
+ }
+ lcd_pwrctrl_parse_dt(dev, pdata);
+ }
+#endif
+
+ if (!pdata) {
+ dev_err(dev, platform data not available\n);
+ return -EINVAL;
+ }
+
+ err = gpio_request(pdata-gpio, LCD-nRESET);
+ if (err) {
+ dev_err(dev, gpio [%d] request failed\n, pdata-gpio);
+ return err;
+ }
+
+ lp = devm_kzalloc(dev, sizeof(struct lcd_pwrctrl),
Re: [RFC][PATCH 0/4] lcd: platform-lcd: Add lcd panel and device tree support
On 01/03/2012 12:54 PM, Thomas Abraham wrote: Hi Lars, On 3 January 2012 14:36, Lars-Peter Clausen l...@metafoo.de wrote: On 01/02/2012 06:54 AM, Thomas Abraham wrote: The platform-lcd driver depends on platform-specific callbacks to setup the lcd panel. These callbacks are supplied using driver's platform data. But for adding device tree support for platform-lcd driver, providing such callbacks is not possible (without using auxdata). Since the callbacks are usually lcd panel specific, it is possible to include the lcd panel specific setup and control functionality in the platform-lcd driver itself, thereby eliminating the need for supplying platform specific callbacks to the driver. The platform-lcd driver can include support for multiple lcd panels. This patchset removes the need for platform data for platform-lcd driver and adds support which can be used to implement lcd panel specific functionality in the driver. As an example, the support for Hydis hv070wsa lcd panel is added to the platform-lcd driver which is then used on the Exynos4 based Origen board. This currently breaks build for other users of platform-lcd driver. Those can be fixed if this approach is acceptable. The whole approach looks rather backwards to me. The exact purpose of the platform_lcd driver is to redirect the lcd driver callbacks to board code. So by removing this support you not only break all the existing driver but also create a driver which does nothing. Then you add another layer of abstraction to implement custom drivers in this driver. A better approach in my opinion is to simply implement these drivers as first level LCD drivers. So leave the platform-lcd driver as it is and just add a gpio (or maybe regulator) lcd driver instead. The existing platform-lcd driver mostly depends on the platform callbacks for lcd panel power controls. Looking at the current users of platform-lcd driver, a majority of the users of platform-lcd driver use a gpio to enable/disable the display/power. The gpio is controlled by the platform callbacks which the platform-lcd driver calls. Hence, it is possible to extend the platform-lcd driver to include the functionality of managing the gpio for lcd control. The platform code is only expected to provide a gpio number to the platform-lcd driver. This allows consolidation of the all the different platform callbacks that use a gpio for simple enable/disable of the lcd display. But there are other users of platform-lcd that do lot more than just control a single gpio. For such cases, it is possible to reuse the existing infrastructure of platform-lcd driver and extend it to handle such lcd panel specific functionality. The main advantages that I see here is the consolidation of platform specific callbacks into the driver which inturn allows adding device tree support without depending on platform data which have pointers to platform specific functions. In the next version of this patchset, it will be ensured that no platform breaks due to this change. Consolidation of the different implementations which use a GPIO to control the LCD state is a good idea. But as I wrote above in this series you added more or less another layer of abstraction. Namely introducing the platform-lcd driver as a intermediate layer between the actual driver and the LCD framework. But the layer is so thin that all it does is to call the plat_lcd_driver_data callback from the lcd_ops callback. There is really no point in doing this since you can setup the lcd_ops callbacks directly. Also following your approach we would end up with a bunch of unrelated LCD drivers in the platform-lcd driver module. The platform-lcd driver is so generic that basically any LCD driver could be implemented on-top of it, but this does not mean it has to. As said before, writing a plain LCD driver which implements the GPIO handling and leaving the platform-lcd driver as it is, is in my opinion a better approach. -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC][PATCH 0/4] lcd: platform-lcd: Add lcd panel and device tree support
On 01/03/2012 06:07 PM, Thomas Abraham wrote: On 3 January 2012 17:56, Lars-Peter Clausen l...@metafoo.de wrote: On 01/03/2012 12:54 PM, Thomas Abraham wrote: Hi Lars, On 3 January 2012 14:36, Lars-Peter Clausen l...@metafoo.de wrote: On 01/02/2012 06:54 AM, Thomas Abraham wrote: The platform-lcd driver depends on platform-specific callbacks to setup the lcd panel. These callbacks are supplied using driver's platform data. But for adding device tree support for platform-lcd driver, providing such callbacks is not possible (without using auxdata). Since the callbacks are usually lcd panel specific, it is possible to include the lcd panel specific setup and control functionality in the platform-lcd driver itself, thereby eliminating the need for supplying platform specific callbacks to the driver. The platform-lcd driver can include support for multiple lcd panels. This patchset removes the need for platform data for platform-lcd driver and adds support which can be used to implement lcd panel specific functionality in the driver. As an example, the support for Hydis hv070wsa lcd panel is added to the platform-lcd driver which is then used on the Exynos4 based Origen board. This currently breaks build for other users of platform-lcd driver. Those can be fixed if this approach is acceptable. The whole approach looks rather backwards to me. The exact purpose of the platform_lcd driver is to redirect the lcd driver callbacks to board code. So by removing this support you not only break all the existing driver but also create a driver which does nothing. Then you add another layer of abstraction to implement custom drivers in this driver. A better approach in my opinion is to simply implement these drivers as first level LCD drivers. So leave the platform-lcd driver as it is and just add a gpio (or maybe regulator) lcd driver instead. The existing platform-lcd driver mostly depends on the platform callbacks for lcd panel power controls. Looking at the current users of platform-lcd driver, a majority of the users of platform-lcd driver use a gpio to enable/disable the display/power. The gpio is controlled by the platform callbacks which the platform-lcd driver calls. Hence, it is possible to extend the platform-lcd driver to include the functionality of managing the gpio for lcd control. The platform code is only expected to provide a gpio number to the platform-lcd driver. This allows consolidation of the all the different platform callbacks that use a gpio for simple enable/disable of the lcd display. But there are other users of platform-lcd that do lot more than just control a single gpio. For such cases, it is possible to reuse the existing infrastructure of platform-lcd driver and extend it to handle such lcd panel specific functionality. The main advantages that I see here is the consolidation of platform specific callbacks into the driver which inturn allows adding device tree support without depending on platform data which have pointers to platform specific functions. In the next version of this patchset, it will be ensured that no platform breaks due to this change. Consolidation of the different implementations which use a GPIO to control the LCD state is a good idea. But as I wrote above in this series you added more or less another layer of abstraction. Namely introducing the platform-lcd driver as a intermediate layer between the actual driver and the LCD framework. But the layer is so thin that all it does is to call the plat_lcd_driver_data callback from the lcd_ops callback. There is really no point in doing this since you can setup the lcd_ops callbacks directly. Also following your approach we would end up with a bunch of unrelated LCD drivers in the platform-lcd driver module. The platform-lcd driver is so generic that basically any LCD driver could be implemented on-top of it, but this does not mean it has to. Yes, this will lead to including support for multiple lcd panels in the platform-lcd driver. But, we get to reuse most of the infrastruture in the platform-lcd driver such as module init/cleanup, suspend/resume, probe and lcd driver registration. There is a plan to include regulator support in the platform-lcd driver as well. If we go for independent drivers for all existing users of platform-lcd driver, then this common code in platform-lcd driver will have to be duplicated in multiple new drivers. Most of the infrastructure code is driver boiler-plate code. And you'll add about the same amount of code due to your additional layer redirection. You'll still have a probe and remove function per driver. You'll have to define a set of plat_lcd_driver_data per driver. You'll have all these ugly ifdefs. An exception is the suspend/resume handling. But this does not look like it is something which is specific to simple displays, more complex ones or non-platform driver lcd drivers might want
Re: [PATCH RESEND v3] ARM: s3c2442: Setup gpio {set,get}_pull callbacks
On 11/30/2010 07:18 AM, Kukjin Kim wrote:
Vasily Khoruzhick wrote:
Currently the {set,get}_pull callbacks of the s3c24xx_gpiocfg_default
structure
are initalized via s3c_gpio_{get,set}pull_1up. This results in a linker
error when compiling kernel for s3c2442:
arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f4): undefined reference to
`s3c_gpio_getpull_1up'
arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f8): undefined reference to
`s3c_gpio_setpull_1up'
Yeah, happened build error when selected only S3C2442.
The s3c2442 has pulldowns instead of pullups compared to the s3c2440.
The method of controlling them is the same though.
So this patch modifies the existing s3c_gpio_{get,set}pull_1up helper
functions
to take an additional parameter deciding whether the pin has a pullup or
pulldown.
The s3c_gpio_{get,set}pull_1{down,up} functions then wrap that functions
passing
either S3C_GPIO_PULL_UP or S3C_GPIO_PULL_DOWN.
Furthermore this patch sets up the s3c24xx_gpiocfg_default.{get,set}_pull
fields
in the s3c2442 cpu init function to the new pulldown helper functions.
Based on patch from Lars-Peter Clausen l...@metafoo.de
Signed-off-by: Vasily Khoruzhick anars...@gmail.com
---
v2: adapt patch for 2.6.37-rc1
v3: restore default pull callbacks, add default pull callbacks for s3c2442
arch/arm/mach-s3c2440/Kconfig |1 +
arch/arm/mach-s3c2440/s3c2442.c|7 +++
arch/arm/plat-s3c24xx/gpiolib.c|9 +++-
arch/arm/plat-samsung/gpio-config.c| 44
--
-
.../plat-samsung/include/plat/gpio-cfg-helpers.h | 11 +
5 files changed, 63 insertions(+), 9 deletions(-)
diff --git a/arch/arm/mach-s3c2440/Kconfig b/arch/arm/mach-s3c2440/Kconfig
index ff024a6..478fae0 100644
--- a/arch/arm/mach-s3c2440/Kconfig
+++ b/arch/arm/mach-s3c2440/Kconfig
@@ -18,6 +18,7 @@ config CPU_S3C2440
config CPU_S3C2442
bool
select CPU_ARM920T
+select S3C_GPIO_PULL_DOWN
select S3C2410_CLOCK
select S3C2410_GPIO
select S3C2410_PM if PM
diff --git a/arch/arm/mach-s3c2440/s3c2442.c b/arch/arm/mach-
s3c2440/s3c2442.c
index 188ad1e..0dbc91c 100644
--- a/arch/arm/mach-s3c2440/s3c2442.c
+++ b/arch/arm/mach-s3c2440/s3c2442.c
@@ -32,6 +32,7 @@
#include linux/interrupt.h
#include linux/ioport.h
#include linux/mutex.h
+#include linux/gpio.h
#include linux/clk.h
#include linux/io.h
@@ -44,6 +45,10 @@
#include plat/clock.h
#include plat/cpu.h
+#include plat/gpio-core.h
+#include plat/gpio-cfg.h
+#include plat/gpio-cfg-helpers.h
+
/* S3C2442 extended clock support */
static unsigned long s3c2442_camif_upll_round(struct clk *clk,
@@ -160,6 +165,8 @@ static struct sys_device s3c2442_sysdev = {
int __init s3c2442_init(void)
{
printk(S3C2442: Initialising architecture\n);
To add empty line would be helpful to read here.
+s3c24xx_gpiocfg_default.set_pull = s3c_gpio_setpull_1down;
+s3c24xx_gpiocfg_default.get_pull = s3c_gpio_getpull_1down;
Right now, this is ok to me...but I think we need to sort this out with
other S3C SoCs.
return sysdev_register(s3c2442_sysdev);
}
diff --git a/arch/arm/plat-s3c24xx/gpiolib.c b/arch/arm/plat-
s3c24xx/gpiolib.c
index 24c6f5a..b805dab 100644
--- a/arch/arm/plat-s3c24xx/gpiolib.c
+++ b/arch/arm/plat-s3c24xx/gpiolib.c
@@ -82,8 +82,13 @@ static struct s3c_gpio_cfg s3c24xx_gpiocfg_banka = {
struct s3c_gpio_cfg s3c24xx_gpiocfg_default = {
.set_config = s3c_gpio_setcfg_s3c24xx,
.get_config = s3c_gpio_getcfg_s3c24xx,
-.set_pull = s3c_gpio_setpull_1up,
-.get_pull = s3c_gpio_getpull_1up,
+#if defined(CONFIG_S3C_GPIO_PULL_UP)
+.set_pull = s3c_gpio_setpull_1up,
+.get_pull = s3c_gpio_getpull_1up,
^^
Why do you use white space instead of tab?
Original code used tab in there.
+#elif defined(CONFIG_S3C_GPIO_PULL_DOWN)
+.set_pull = s3c_gpio_setpull_1down,
+.get_pull = s3c_gpio_getpull_1down,
+#endif
Yeah, this is needed for avoiding build error with only S3C2442.
But please remember, now, its default is CONFIG_S3C_GPIO_PULL_UP when used
s3c2410_defconfig even though selected S3C_GPIO_PULL_DOWN together.
I will thinking about better method for single binary kernel of S3C24XX. As
you know, current your method can not support it.
};
struct s3c_gpio_chip s3c24xx_gpios[] = {
diff --git a/arch/arm/plat-samsung/gpio-config.c b/arch/arm/plat-
samsung/gpio-config.c
index b732b77..ac7f13f 100644
--- a/arch/arm/plat-samsung/gpio-config.c
+++ b/arch/arm/plat-samsung/gpio-config.c
@@ -280,16 +280,17 @@ s3c_gpio_pull_t s3c_gpio_getpull_updown(struct
s3c_gpio_chip *chip,
}
#endif
-#ifdef CONFIG_S3C_GPIO_PULL_UP
-int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
- unsigned int off, s3c_gpio_pull_t pull)
+#if defined(CONFIG_S3C_GPIO_PULL_UP
Re: [PATCH RESEND v3] ARM: s3c2442: Setup gpio {set,get}_pull callbacks
On 11/30/2010 01:01 PM, Vasily Khoruzhick wrote:
On Tuesday 30 November 2010 13:53:43 Lars-Peter Clausen wrote:
Hmm...how about s3c_gpio_setpull_1updown(...)?
And actually, not used 3rd argument, pull now.
I prefer follwoing.
You need the 4th arguemnt, because the s3c2440 only supports pullups and
the s3c2442 only supports pulldowns. So you want to return -EINVAL if
somebody tries to set a pullup on a s3c2442 based board.
Your proposed solution would return 0 and set a pulldown instead.
Well, at least it allows single-binary kernel for s3c24xx to exist. I think
it's OK, as setting pull{up,down} bit for any non S3C_GPIO_PULL_NONE arg
preserves semantics for all SoCs (s3c2410/s3c2440/s3c2442) by cost of not
handling errors. Anyway, who wants to call cfgpull with S3C_GPIO_PULL_DOWN on
s3c2410/s3c2440 or with S3C_GPIO_PULL_UP on s3c2442?
Regards
Vasily
Hi
While this might work for setting the pullup, what to you want to return in
get_pull?
The reason why s3c24xx_gpiocfg_default needs to have {get,set}_pull set at
compile
time is that the board init code is called before the cpu init code. Which is
in my
opinion a bit odd and should be fixed instead.
If it is not fixed for whatever reason we could fallback to using some sort of
cpu_is_s3c2442() ? S3C_GPIO_PULL_UP : S3C_GPIO_PULL_DOWN
- Lars
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RESEND v3] ARM: s3c2442: Setup gpio {set,get}_pull callbacks
On 11/30/2010 03:33 PM, Vasily Khoruzhick wrote:
On Tuesday 30 November 2010 15:12:37 Lars-Peter Clausen wrote:
Hi
While this might work for setting the pullup, what to you want to return in
get_pull?
Some custom value like S3C_GPIO_PULL_ENABLED?
Well, or we could use a custom setter and getter functions for configuring the
pull-ups/downs.
The reason why s3c24xx_gpiocfg_default needs to have {get,set}_pull set at
compile time is that the board init code is called before the cpu init
code. Which is in my opinion a bit odd and should be fixed instead.
That's because cpu init code is arch_initcall. Kernel calls mdesc-
init_machine before any arch_initcall function, not sure if it can be fixed
without massive rework of existing code.
Both the cpu init code and the machine init code are run at arch_initcall.
If it is not fixed for whatever reason we could fallback to using some sort
of cpu_is_s3c2442() ? S3C_GPIO_PULL_UP : S3C_GPIO_PULL_DOWN
AFAIK, Ben does not like runtime CPUtype checks
I prefer it over broken code. And you would only need it until the cpu init
functions
have been run.
You would add a wrapper like:
s3c24xx_set_pull(...)
{
if (cpu_is_s3c2442())
s3c_gpio_setpull_1down(...)
else
s3c_gpio_setpull_1up(...)
}
And then set s3c24xx_gpiocfg_default.{set,get}_pull to those at compile time.
And
once the cpu init code runs replace them with either s3c_gpio_setpull_1down or
s3c_gpio_setpull_1up depending on the cpu.
- Lars
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] ARM: s3c244x: Fix mess with gpio {set,get}_pull callbacks
On 11/30/2010 08:46 PM, Vasily Khoruzhick wrote:
Currently the {set,get}_pull callbacks of the s3c24xx_gpiocfg_default
structure
are initalized via s3c_gpio_{get,set}pull_1up. This results in a linker
error when only CONFIG_CPU_S3C2442 is selected:
arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f4): undefined reference to
`s3c_gpio_getpull_1up'
arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f8): undefined reference to
`s3c_gpio_setpull_1up'
The s3c2442 has pulldowns instead of pullups compared to the s3c2440.
The method of controlling them is the same though.
So this patch modifies the existing s3c_gpio_{get,set}pull_1up helper
functions
to take an additional parameter deciding whether the pin has a pullup or
pulldown.
The s3c_gpio_{get,set}pull_1{down,up} functions then wrap that functions
passing
either S3C_GPIO_PULL_UP or S3C_GPIO_PULL_DOWN.
Furthermore this patch sets up the s3c24xx_gpiocfg_default.{get,set}_pull
fields
in the s3c244{0,2}_map_io function to the new pulldown helper functions.
Based on patch from Lars-Peter Clausen l...@metafoo.de
Signed-off-by: Vasily Khoruzhick anars...@gmail.com
---
v2: adapt patch for 2.6.37-rc1
v3: restore default pull callbacks, add default pull callbacks for s3c2442
v4: remove default pull callbacks, set them in per-soc map_io function
instead.
arch/arm/mach-s3c2440/Kconfig |1 +
arch/arm/mach-s3c2440/s3c2440.c| 11 -
arch/arm/mach-s3c2440/s3c2442.c| 14 ++
arch/arm/plat-s3c24xx/cpu.c|8 ++--
arch/arm/plat-s3c24xx/gpiolib.c|2 -
arch/arm/plat-s3c24xx/include/plat/s3c244x.h |7 +++-
arch/arm/plat-samsung/gpio-config.c| 45
.../plat-samsung/include/plat/gpio-cfg-helpers.h | 11 +
8 files changed, 80 insertions(+), 19 deletions(-)
...
+static int s3c_gpio_setpull_1(struct s3c_gpio_chip *chip,
+ unsigned int off, s3c_gpio_pull_t pull,
+ s3c_gpio_pull_t updown)
{
void __iomem *reg = chip-base + 0x08;
u32 pup = __raw_readl(reg);
- pup = __raw_readl(reg);
-
- if (pup == S3C_GPIO_PULL_UP)
+ if (pup == updown)
This should be pull == updown, otherwise looks fine to me
pup = ~(1 off);
else if (pup == S3C_GPIO_PULL_NONE)
pup |= (1 off);
@@ -300,17 +299,45 @@ int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
return 0;
}
- Lars
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: s3c2442: gta02: Fix usage gpio bank j pin definitions
Ben Dooks wrote: On 24/09/10 19:24, Lars-Peter Clausen wrote: The gta02 header file still uses the old S3C2410_GPJx defines instead of the S3C2410_GPJ(x) macro. Since the S3C2410_GPJx defines have already been removed this causes a build failure. This patches fixes the issue by doing a s,S3C2410_GPJ([\d]+),S3C2410_GPJ(\1),g on the file. Is this currently causing a build error? If so, please add the snippet of the build log. Otherwise it'll get queued for the next merge. CC sound/soc/s3c24xx/neo1973_gta02_wm8753.o sound/soc/s3c24xx/neo1973_gta02_wm8753.c: In function 'lm4853_set_spk': sound/soc/s3c24xx/neo1973_gta02_wm8753.c:244: error: 'S3C2440_GPJ2' undeclared (first use in this function) sound/soc/s3c24xx/neo1973_gta02_wm8753.c:244: error: (Each undeclared identifier is reported only once sound/soc/s3c24xx/neo1973_gta02_wm8753.c:244: error: for each function it appears in.) sound/soc/s3c24xx/neo1973_gta02_wm8753.c: In function 'lm4853_event': sound/soc/s3c24xx/neo1973_gta02_wm8753.c:265: error: 'S3C2440_GPJ1' undeclared (first use in this function) sound/soc/s3c24xx/neo1973_gta02_wm8753.c:265: error: 'value' undeclared (first use in this function) sound/soc/s3c24xx/neo1973_gta02_wm8753.c: In function 'neo1973_gta02_init': sound/soc/s3c24xx/neo1973_gta02_wm8753.c:458: error: 'S3C2440_GPJ2' undeclared (first use in this function) sound/soc/s3c24xx/neo1973_gta02_wm8753.c:464: error: 'GTA02_GPIO_AMP_HP_IN' undeclared (first use in this function) sound/soc/s3c24xx/neo1973_gta02_wm8753.c:470: error: 'S3C2440_GPJ1' undeclared (first use in this function) sound/soc/s3c24xx/neo1973_gta02_wm8753.c: In function 'neo1973_gta02_exit': sound/soc/s3c24xx/neo1973_gta02_wm8753.c:498: error: 'S3C2440_GPJ2' undeclared (first use in this function) sound/soc/s3c24xx/neo1973_gta02_wm8753.c:499: error: 'S3C2440_GPJ1' undeclared (first use in this function) -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] ARM: s3c24xx: Set ARCH_NR_GPIOS according to the selected SoC types.
Currently the code in gpiolib.c tries to register GPIO BANKA to BANKM.
ARCH_NR_GPIOS on the other hand is set only to 256, which would be the
equivalent of BANKA to BANKH. Thus the registration of all other banks will
fail.
This patch fixes this by setting S3C_GPIO_END according to the selected SoC
types. S3C_GPIO_END is set to the maximum of the number of GPIOs over all
selected SoC types. Thus it is ensured that memory is not wasted if support for
SoCs with higher GPIO numbers is not built-in.
When registering the banks it is made sure that banks which are outside of that
range are not even tried to be registered. Otherwise there would a problem with
configs where CONFIG_S3C24XX_GPIO_EXTRA is set to a non zero value.
Signed-off-by: Lars-Peter Clausen l...@metafoo.de
---
arch/arm/mach-s3c2410/include/mach/gpio.h | 25 -
arch/arm/plat-s3c24xx/gpiolib.c |2 ++
2 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/arch/arm/mach-s3c2410/include/mach/gpio.h
b/arch/arm/mach-s3c2410/include/mach/gpio.h
index b649bf2..80dfe25 100644
--- a/arch/arm/mach-s3c2410/include/mach/gpio.h
+++ b/arch/arm/mach-s3c2410/include/mach/gpio.h
@@ -16,22 +16,21 @@
#define gpio_cansleep __gpio_cansleep
#define gpio_to_irq__gpio_to_irq
-/* some boards require extra gpio capacity to support external
- * devices that need GPIO.
- */
+#include mach/gpio-fns.h
+#include mach/gpio-nrs.h
-#ifdef CONFIG_CPU_S3C244X
-#define ARCH_NR_GPIOS (32 * 9 + CONFIG_S3C24XX_GPIO_EXTRA)
+#if defined(CPU_S3C2416) || defined(CPU_S3C2443)
+#define S3C_GPIO_END S3C2410_GPM(S3C2410_GPIO_M_NR)
+#elif defined(CONFIG_CPU_S3C244X)
+#define S3C_GPIO_END S3C2410_GPJ(S3C2410_GPIO_J_NR)
#else
-#define ARCH_NR_GPIOS (256 + CONFIG_S3C24XX_GPIO_EXTRA)
+#define S3C_GPIO_END S3C2410_GPH(S3C2410_GPIO_H_NR)
#endif
+/* some boards require extra gpio capacity to support external
+ * devices that need GPIO.
+ */
+#define ARCH_NR_GPIOS (S3C_GPIO_END + CONFIG_S3C24XX_GPIO_EXTRA)
+
#include asm-generic/gpio.h
-#include mach/gpio-nrs.h
-#include mach/gpio-fns.h
-#ifdef CONFIG_CPU_S3C24XX
-#define S3C_GPIO_END (S3C2410_GPIO_BANKJ + 32)
-#else
-#define S3C_GPIO_END (S3C2410_GPIO_BANKH + 32)
-#endif
diff --git a/arch/arm/plat-s3c24xx/gpiolib.c b/arch/arm/plat-s3c24xx/gpiolib.c
index 4c0896f..65b6126 100644
--- a/arch/arm/plat-s3c24xx/gpiolib.c
+++ b/arch/arm/plat-s3c24xx/gpiolib.c
@@ -221,6 +221,8 @@ static __init int s3c24xx_gpiolib_init(void)
int gpn;
for (gpn = 0; gpn ARRAY_SIZE(s3c24xx_gpios); gpn++, chip++) {
+ if (chip-chip.base = S3C_GPIO_END)
+ break;
if (!chip-config)
chip-config = s3c24xx_gpiocfg_default;
--
1.5.6.5
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] ARM: s3c2442: Setup gpio {set,get}_pull callbacks
Currently the {set,get}_pull callbacks of the s3c24xx_gpiocfg_default structure
are not initalized for the s3c2442 cpu type. This results in a NULL-pointer
deref upon calling s3c_gpio_setpull.
The s3c2442 has pulldowns instead of pullups compared to the s3c2440.
The method of controlling them is the same though.
So this patch modifies the existing s3c_gpio_{get,set}pull_1up helper functions
to take an additional parameter deciding whether the pin has a pullup or
pulldown.
The s3c_gpio_{get,set}pull_1{down,up} functions then wrap that functions passing
either S3C_GPIO_PULL_UP or S3C_GPIO_PULL_DOWN.
Furthermore this patch sets up the s3c24xx_gpiocfg_default.{get,set}_pull fields
in the s3c2442 cpu init function to the new pulldown helper functions.
Signed-off-by: Lars-Peter Clausen l...@metafoo.de
---
arch/arm/mach-s3c2440/Kconfig |1 +
arch/arm/mach-s3c2440/s3c2442.c|7 +++
arch/arm/plat-samsung/gpio-config.c| 43 ---
.../plat-samsung/include/plat/gpio-cfg-helpers.h | 11 +
4 files changed, 55 insertions(+), 7 deletions(-)
diff --git a/arch/arm/mach-s3c2440/Kconfig b/arch/arm/mach-s3c2440/Kconfig
index cd8e7de..136ebd0 100644
--- a/arch/arm/mach-s3c2440/Kconfig
+++ b/arch/arm/mach-s3c2440/Kconfig
@@ -20,6 +20,7 @@ config CPU_S3C2442
bool
depends on ARCH_S3C2410
select CPU_ARM920T
+ select S3C_GPIO_PULL_DOWN
select S3C2410_CLOCK
select S3C2410_GPIO
select S3C2410_PM if PM
diff --git a/arch/arm/mach-s3c2440/s3c2442.c b/arch/arm/mach-s3c2440/s3c2442.c
index 188ad1e..0dbc91c 100644
--- a/arch/arm/mach-s3c2440/s3c2442.c
+++ b/arch/arm/mach-s3c2440/s3c2442.c
@@ -32,6 +32,7 @@
#include linux/interrupt.h
#include linux/ioport.h
#include linux/mutex.h
+#include linux/gpio.h
#include linux/clk.h
#include linux/io.h
@@ -44,6 +45,10 @@
#include plat/clock.h
#include plat/cpu.h
+#include plat/gpio-core.h
+#include plat/gpio-cfg.h
+#include plat/gpio-cfg-helpers.h
+
/* S3C2442 extended clock support */
static unsigned long s3c2442_camif_upll_round(struct clk *clk,
@@ -160,6 +165,8 @@ static struct sys_device s3c2442_sysdev = {
int __init s3c2442_init(void)
{
printk(S3C2442: Initialising architecture\n);
+ s3c24xx_gpiocfg_default.set_pull = s3c_gpio_setpull_1down;
+ s3c24xx_gpiocfg_default.get_pull = s3c_gpio_getpull_1down;
return sysdev_register(s3c2442_sysdev);
}
diff --git a/arch/arm/plat-samsung/gpio-config.c
b/arch/arm/plat-samsung/gpio-config.c
index 57b68a5..1abc81e 100644
--- a/arch/arm/plat-samsung/gpio-config.c
+++ b/arch/arm/plat-samsung/gpio-config.c
@@ -230,16 +230,16 @@ s3c_gpio_pull_t s3c_gpio_getpull_updown(struct
s3c_gpio_chip *chip,
}
#endif
-#ifdef CONFIG_S3C_GPIO_PULL_UP
-int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
-unsigned int off, s3c_gpio_pull_t pull)
+#if defined(CONFIG_S3C_GPIO_PULL_UP) || defined(CONFIG_S3C_GPIO_PULL_DOWN)
+static int s3c_gpio_setpull_1(struct s3c_gpio_chip *chip,
+unsigned int off, s3c_gpio_pull_t pull,
s3c_gpio_pull_t updown)
{
void __iomem *reg = chip-base + 0x08;
u32 pup = __raw_readl(reg);
pup = __raw_readl(reg);
- if (pup == S3C_GPIO_PULL_UP)
+ if (pup == updown)
pup = ~(1 off);
else if (pup == S3C_GPIO_PULL_NONE)
pup |= (1 off);
@@ -250,17 +250,46 @@ int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
return 0;
}
-s3c_gpio_pull_t s3c_gpio_getpull_1up(struct s3c_gpio_chip *chip,
-unsigned int off)
+static s3c_gpio_pull_t s3c_gpio_getpull_1(struct s3c_gpio_chip *chip,
+unsigned int off, s3c_gpio_pull_t updown)
{
void __iomem *reg = chip-base + 0x08;
u32 pup = __raw_readl(reg);
pup = (1 off);
- return pup ? S3C_GPIO_PULL_NONE : S3C_GPIO_PULL_UP;
+ return pup ? S3C_GPIO_PULL_NONE : updown;
+}
+#endif /* defined(CONFIG_S3C_GPIO_PULL_UP) ||
defined(CONFIG_S3C_GPIO_PULL_DOWN) */
+
+#ifdef CONFIG_S3C_GPIO_PULL_UP
+s3c_gpio_pull_t s3c_gpio_getpull_1up(struct s3c_gpio_chip *chip,
+unsigned int off)
+{
+ return s3c_gpio_getpull_1(chip, off, S3C_GPIO_PULL_UP);
+}
+
+int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
+unsigned int off, s3c_gpio_pull_t pull)
+{
+ return s3c_gpio_setpull_1(chip, off, pull, S3C_GPIO_PULL_UP);
}
#endif /* CONFIG_S3C_GPIO_PULL_UP */
+#ifdef CONFIG_S3C_GPIO_PULL_DOWN
+s3c_gpio_pull_t s3c_gpio_getpull_1down(struct s3c_gpio_chip *chip,
+unsigned int off)
+{
+ return s3c_gpio_getpull_1(chip, off, S3C_GPIO_PULL_DOWN);
+}
+
+int s3c_gpio_setpull_1down(struct s3c_gpio_chip *chip,
+unsigned int off, s3c_gpio_pull_t pull)
+{
+ return s3c_gpio_setpull_1
