Re: Re: [PATCH 2/5] ASoC: OMAP4: omap-dmic: Initial support for OMAP DMIC
On Tuesday 22 November 2011 16:01:05 Mark Brown wrote:
> On Tue, Nov 22, 2011 at 04:01:57PM +0200, Peter Ujfalusi wrote:
> > + switch (params_rate(params)) {
> > + case 96000:
> > + case 192000:
> > + break;
>
> Why doesn't the driver need to tell the hardware what sample rate to run
> at?
Because the OMAP4 DMIC can only support on 96KHz...
Will be fixed.
>
> > + dmic_clk = clk_get(dmic->dev, "dmic_fck");
> > + if (IS_ERR(dmic_clk)) {
> > + dev_err(dmic->dev, "cant get dmic_fck\n");
> > + return -ENODEV;
> > + }
>
> Why aren't we getting and holding a reference to the clock over the
> entire lifetime of the driver?
We only need the reference for the dmic_fclk for reparenting which will happen
only once in most cases (or not at all, if pad_clks_ck is going to be used). I
don't think we should hold the reference for the dmic_fclk.
The clock handling is done via pm_runtime_get/put_sync().
> > + /* disable clock while reparenting */
> > + pm_runtime_put_sync(dmic->dev);
> > + ret = clk_set_parent(dmic_clk, parent_clk);
> > + pm_runtime_get_sync(dmic->dev);
>
> Since we're only allowing reclocking while idle shouldn't the clock
> already be disabled? Seems like it ought to be good for power if
> nothing else...
We enable the clocks at dai_startup for the DMIC (and disable them on
dai_shutdown). We can not reparent while the clocks are enabled.
This is the reason for this part.
> > +static int omap_dmic_set_clkdiv(struct snd_soc_dai *dai,
> > + int div_id, int div)
> > +{
>
> DMIC clocking is usually fairly simple so it seems surprising that the
> driver isn't able to figure this out for itself.
The clock towards the external digital mics are based on the DMIC_FCLK rate.
In case of DMIC_FCLK = 19.2MHz, 24.576MHz we can select between two dividers
which will result different clocks for the external microphones.
I would rather leave this decision to the machine driver which knows the
external components, and can pick the best divider for them.
--
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 35/65] OMAPDSS: APPLY: move spinlock outside the struct
On Tuesday 22 November 2011 02:51 PM, Tomi Valkeinen wrote:
dss_cache struct contains a spinlock used to protect the struct. A more
logical place for the spinlock is outside the struct that it is
protecting. So move it there.
Signed-off-by: Tomi Valkeinen
---
drivers/video/omap2/dss/apply.c | 22 --
1 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
index 23c723a..17639c0 100644
--- a/drivers/video/omap2/dss/apply.c
+++ b/drivers/video/omap2/dss/apply.c
@@ -89,13 +89,15 @@ struct mgr_priv_data {
};
static struct {
- spinlock_t lock;
struct ovl_priv_data ovl_priv_data_array[MAX_DSS_OVERLAYS];
struct mgr_priv_data mgr_priv_data_array[MAX_DSS_MANAGERS];
bool irq_enabled;
} dss_cache;
+/* protects dss_cache */
+static spinlock_t data_lock;
Minor comment: The name 'data_lock' doesn't tell much that its
protecting the dss_cache struct. Probably 'cache_lock' or
'priv_data_lock' or something like that may be more informative.
Archit
+
static struct ovl_priv_data *get_ovl_priv(struct omap_overlay *ovl)
{
return&dss_cache.ovl_priv_data_array[ovl->id];
@@ -108,7 +110,7 @@ static struct mgr_priv_data *get_mgr_priv(struct
omap_overlay_manager *mgr)
void dss_apply_init(void)
{
- spin_lock_init(&dss_cache.lock);
+ spin_lock_init(&data_lock);
}
static bool ovl_manual_update(struct omap_overlay *ovl)
@@ -149,10 +151,10 @@ int dss_mgr_wait_for_go(struct omap_overlay_manager *mgr)
unsigned long flags;
bool shadow_dirty, dirty;
- spin_lock_irqsave(&dss_cache.lock, flags);
+ spin_lock_irqsave(&data_lock, flags);
dirty = mp->dirty;
shadow_dirty = mp->shadow_dirty;
- spin_unlock_irqrestore(&dss_cache.lock, flags);
+ spin_unlock_irqrestore(&data_lock, flags);
if (!dirty&& !shadow_dirty) {
r = 0;
@@ -212,10 +214,10 @@ int dss_mgr_wait_for_go_ovl(struct omap_overlay *ovl)
unsigned long flags;
bool shadow_dirty, dirty;
- spin_lock_irqsave(&dss_cache.lock, flags);
+ spin_lock_irqsave(&data_lock, flags);
dirty = op->dirty;
shadow_dirty = op->shadow_dirty;
- spin_unlock_irqrestore(&dss_cache.lock, flags);
+ spin_unlock_irqrestore(&data_lock, flags);
if (!dirty&& !shadow_dirty) {
r = 0;
@@ -464,7 +466,7 @@ static void dss_apply_irq_handler(void *data, u32 mask)
for (i = 0; i< num_mgrs; i++)
mgr_busy[i] = dispc_mgr_go_busy(i);
- spin_lock(&dss_cache.lock);
+ spin_lock(&data_lock);
for (i = 0; i< num_ovls; ++i) {
ovl = omap_dss_get_overlay(i);
@@ -498,7 +500,7 @@ static void dss_apply_irq_handler(void *data, u32 mask)
dss_unregister_vsync_isr();
end:
- spin_unlock(&dss_cache.lock);
+ spin_unlock(&data_lock);
}
static int omap_dss_mgr_apply_ovl(struct omap_overlay *ovl)
@@ -620,7 +622,7 @@ int omap_dss_mgr_apply(struct omap_overlay_manager *mgr)
if (r)
return r;
- spin_lock_irqsave(&dss_cache.lock, flags);
+ spin_lock_irqsave(&data_lock, flags);
/* Configure overlays */
list_for_each_entry(ovl,&mgr->overlays, list)
@@ -641,7 +643,7 @@ int omap_dss_mgr_apply(struct omap_overlay_manager *mgr)
dss_write_regs();
}
- spin_unlock_irqrestore(&dss_cache.lock, flags);
+ spin_unlock_irqrestore(&data_lock, flags);
dispc_runtime_put();
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 35/65] OMAPDSS: APPLY: move spinlock outside the struct
On Wednesday 23 November 2011 02:55 PM, Archit Taneja wrote:
Minor comment: The name 'data_lock' doesn't tell much that its
protecting the dss_cache struct. Probably 'cache_lock' or
'priv_data_lock' or something like that may be more informative.
>
> Archit
Ah, just saw the next patch, you renamed dss_cache to dss_data, so
'data_lock' seems to make more sense now.
Archit
+
static struct ovl_priv_data *get_ovl_priv(struct omap_overlay *ovl)
{
return&dss_cache.ovl_priv_data_array[ovl->id];
@@ -108,7 +110,7 @@ static struct mgr_priv_data *get_mgr_priv(struct
omap_overlay_manager *mgr)
void dss_apply_init(void)
{
- spin_lock_init(&dss_cache.lock);
+ spin_lock_init(&data_lock);
}
static bool ovl_manual_update(struct omap_overlay *ovl)
@@ -149,10 +151,10 @@ int dss_mgr_wait_for_go(struct
omap_overlay_manager *mgr)
unsigned long flags;
bool shadow_dirty, dirty;
- spin_lock_irqsave(&dss_cache.lock, flags);
+ spin_lock_irqsave(&data_lock, flags);
dirty = mp->dirty;
shadow_dirty = mp->shadow_dirty;
- spin_unlock_irqrestore(&dss_cache.lock, flags);
+ spin_unlock_irqrestore(&data_lock, flags);
if (!dirty&& !shadow_dirty) {
r = 0;
@@ -212,10 +214,10 @@ int dss_mgr_wait_for_go_ovl(struct omap_overlay
*ovl)
unsigned long flags;
bool shadow_dirty, dirty;
- spin_lock_irqsave(&dss_cache.lock, flags);
+ spin_lock_irqsave(&data_lock, flags);
dirty = op->dirty;
shadow_dirty = op->shadow_dirty;
- spin_unlock_irqrestore(&dss_cache.lock, flags);
+ spin_unlock_irqrestore(&data_lock, flags);
if (!dirty&& !shadow_dirty) {
r = 0;
@@ -464,7 +466,7 @@ static void dss_apply_irq_handler(void *data, u32
mask)
for (i = 0; i< num_mgrs; i++)
mgr_busy[i] = dispc_mgr_go_busy(i);
- spin_lock(&dss_cache.lock);
+ spin_lock(&data_lock);
for (i = 0; i< num_ovls; ++i) {
ovl = omap_dss_get_overlay(i);
@@ -498,7 +500,7 @@ static void dss_apply_irq_handler(void *data, u32
mask)
dss_unregister_vsync_isr();
end:
- spin_unlock(&dss_cache.lock);
+ spin_unlock(&data_lock);
}
static int omap_dss_mgr_apply_ovl(struct omap_overlay *ovl)
@@ -620,7 +622,7 @@ int omap_dss_mgr_apply(struct omap_overlay_manager
*mgr)
if (r)
return r;
- spin_lock_irqsave(&dss_cache.lock, flags);
+ spin_lock_irqsave(&data_lock, flags);
/* Configure overlays */
list_for_each_entry(ovl,&mgr->overlays, list)
@@ -641,7 +643,7 @@ int omap_dss_mgr_apply(struct omap_overlay_manager
*mgr)
dss_write_regs();
}
- spin_unlock_irqrestore(&dss_cache.lock, flags);
+ spin_unlock_irqrestore(&data_lock, flags);
dispc_runtime_put();
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 40/65] OMAPDSS: APPLY: add mutex
Hi,
On Tuesday 22 November 2011 02:51 PM, Tomi Valkeinen wrote:
The functions in apply.c, called mostly via function pointers in overlay
and overlay_manager structs, will be divided into two groups. The other
group will not sleep and can be called from interrupts, and the other
group may sleep.
Small sentence issue above, both groups are called the 'other group'.
The idea is that the non-sleeping functions may only change certain
settings in overlays and managers, and those settings may only affect
the particular overlay/manager. For example, set the base address of the
overlay.
The blocking functions, however, will handle more complex configuration
changes. For example, when an overlay is enabled and fifo-merge feature
is used, we need to do the enable in multiple steps, waiting in between,
and the change affects multiple overlays and managers.
This patch adds the mutex which is used in the blocking functions to
have exclusive access to overlays and overlay managers.
Previously, when we changed the links between 'overlay->managers' and
'manager->devices', it wasn't protected by a lock. Why is it needed now?
As an example, suppose we are changing a manager's device to some other
display. Is this lock preventing someone else to get the older
'mgr->device' rather than the new one?
Archit
Signed-off-by: Tomi Valkeinen
---
drivers/video/omap2/dss/apply.c | 71 ++-
1 files changed, 62 insertions(+), 9 deletions(-)
diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
index b935264..fb6d3c2 100644
--- a/drivers/video/omap2/dss/apply.c
+++ b/drivers/video/omap2/dss/apply.c
@@ -97,6 +97,8 @@ static struct {
/* protects dss_data */
static spinlock_t data_lock;
+/* lock for blocking functions */
+static DEFINE_MUTEX(apply_lock);
static struct ovl_priv_data *get_ovl_priv(struct omap_overlay *ovl)
{
@@ -639,14 +641,22 @@ int omap_dss_mgr_apply(struct omap_overlay_manager *mgr)
void dss_mgr_enable(struct omap_overlay_manager *mgr)
{
+ mutex_lock(&apply_lock);
+
dispc_mgr_enable(mgr->id, true);
mgr->enabled = true;
+
+ mutex_unlock(&apply_lock);
}
void dss_mgr_disable(struct omap_overlay_manager *mgr)
{
+ mutex_lock(&apply_lock);
+
dispc_mgr_enable(mgr->id, false);
mgr->enabled = false;
+
+ mutex_unlock(&apply_lock);
}
int dss_mgr_set_info(struct omap_overlay_manager *mgr,
@@ -669,44 +679,65 @@ int dss_mgr_set_device(struct omap_overlay_manager *mgr,
{
int r;
+ mutex_lock(&apply_lock);
+
if (dssdev->manager) {
DSSERR("display '%s' already has a manager '%s'\n",
dssdev->name, dssdev->manager->name);
- return -EINVAL;
+ r = -EINVAL;
+ goto err;
}
if ((mgr->supported_displays& dssdev->type) == 0) {
DSSERR("display '%s' does not support manager '%s'\n",
dssdev->name, mgr->name);
- return -EINVAL;
+ r = -EINVAL;
+ goto err;
}
dssdev->manager = mgr;
mgr->device = dssdev;
mgr->device_changed = true;
+ mutex_unlock(&apply_lock);
+
return 0;
+err:
+ mutex_unlock(&apply_lock);
+ return r;
}
int dss_mgr_unset_device(struct omap_overlay_manager *mgr)
{
+ int r;
+
+ mutex_lock(&apply_lock);
+
if (!mgr->device) {
DSSERR("failed to unset display, display not set.\n");
- return -EINVAL;
+ r = -EINVAL;
+ goto err;
}
/*
* Don't allow currently enabled displays to have the overlay manager
* pulled out from underneath them
*/
- if (mgr->device->state != OMAP_DSS_DISPLAY_DISABLED)
- return -EINVAL;
+ if (mgr->device->state != OMAP_DSS_DISPLAY_DISABLED) {
+ r = -EINVAL;
+ goto err;
+ }
mgr->device->manager = NULL;
mgr->device = NULL;
mgr->device_changed = true;
+ mutex_unlock(&apply_lock);
+
return 0;
+err:
+ mutex_unlock(&apply_lock);
+ return r;
}
@@ -729,18 +760,24 @@ void dss_ovl_get_info(struct omap_overlay *ovl,
int dss_ovl_set_manager(struct omap_overlay *ovl,
struct omap_overlay_manager *mgr)
{
+ int r;
+
if (!mgr)
return -EINVAL;
+ mutex_lock(&apply_lock);
+
if (ovl->manager) {
DSSERR("overlay '%s' already has a manager '%s'\n",
ovl->name, ovl->manager->name);
- return -EINVAL;
+ r = -EINVAL;
+ goto err;
}
if (ovl->info.enabled) {
DSSERR("overlay has to be disabled to change the manager\n");
- return -EINVAL;
+ r = -EINVAL;
+
Re: [PATCH 25/26] OMAP: PM: convert the SmartReflex code into the AVS driver framework
On Tue, Nov 22, 2011 at 04:06:09PM +0100, jean.pi...@newoldbits.com wrote: > +config POWER_AVS_OMAP_V1 > + tristate "AVS support for the OMAP IP version 1" > + depends on ARCH_OMAP3 && PM > + help > + Say Y to enable AVS support on OMAP containing the version 1 of > + the SmartReflex IP. > + V1 is the 65nm version used in OMAP3430. > + > + Please note, that by default SmartReflex is only > + initialized. To enable the automatic voltage > + compensation for vdd mpu and vdd core from user space, > + user must write 1 to > + /debug/voltage/vdd_/smartreflex/autocomp, > + where X is mpu or core for OMAP3. > + Optionally autocompensation can be enabled in the kernel > + by default during system init via the enable_on_init flag > + which an be passed as platform data to the smartreflex driver. > + > +config POWER_AVS_OMAP_V2 > + tristate "AVS support for the OMAP IP version 2" > + depends on (ARCH_OMAP3 || ARCH_OMAP4) && PM > + help > + Say Y to enable AVS support on OMAP containing the version 2 of > + the SmartReflex IP. > + V2 is the update for the 45nm version of the IP used in OMAP3630 > + and OMAP4430 can't you read the revision register and decide this in runtime ? -- balbi signature.asc Description: Digital signature
Re: [PATCH 41/65] OMAPDSS: APPLY: add missing uses of spinlock
On Tuesday 22 November 2011 02:51 PM, Tomi Valkeinen wrote:
The functions in apply.c, called mostly via function pointers in overlay
and overlay_manager structs, will be divided into two groups. The other
group will not sleep and can be called from interrupts, and the other
group may sleep.
The idea is that the non-sleeping functions may only change certain
settings in overlays and managers, and those settings may only affect
the particular overlay/manager. For example, set the base address of the
overlay.
The blocking functions, however, will handle more complex configuration
changes. For example, when an overlay is enabled and fifo-merge feature
is used, we need to do the enable in multiple steps, waiting in between,
and the change affects multiple overlays and managers.
apply.c already contains a spinlock, which has been used to protect
(badly) the dss_data. This patch adds locks/unlocks of the spinlock to
the missing places, and the lock should now properly protect dss_data.
Signed-off-by: Tomi Valkeinen
---
drivers/video/omap2/dss/apply.c | 29 +
1 files changed, 29 insertions(+), 0 deletions(-)
diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
index fb6d3c2..9ad2a36 100644
--- a/drivers/video/omap2/dss/apply.c
+++ b/drivers/video/omap2/dss/apply.c
@@ -405,6 +405,9 @@ void dss_start_update(struct omap_overlay_manager *mgr)
struct mgr_priv_data *mp = get_mgr_priv(mgr);
struct ovl_priv_data *op;
struct omap_overlay *ovl;
+ unsigned long flags;
+
+ spin_lock_irqsave(&data_lock, flags);
mp->do_manual_update = true;
dss_write_regs();
@@ -418,6 +421,8 @@ void dss_start_update(struct omap_overlay_manager *mgr)
mp->shadow_dirty = false;
dispc_mgr_enable(mgr->id, true);
+
+ spin_unlock_irqrestore(&data_lock, flags);
}
static void dss_apply_irq_handler(void *data, u32 mask);
@@ -662,16 +667,28 @@ void dss_mgr_disable(struct omap_overlay_manager *mgr)
int dss_mgr_set_info(struct omap_overlay_manager *mgr,
struct omap_overlay_manager_info *info)
{
+ unsigned long flags;
+
+ spin_lock_irqsave(&data_lock, flags);
+
mgr->info = *info;
mgr->info_dirty = true;
+ spin_unlock_irqrestore(&data_lock, flags);
+
return 0;
}
void dss_mgr_get_info(struct omap_overlay_manager *mgr,
struct omap_overlay_manager_info *info)
{
+ unsigned long flags;
+
+ spin_lock_irqsave(&data_lock, flags);
+
*info = mgr->info;
+
+ spin_unlock_irqrestore(&data_lock, flags);
}
int dss_mgr_set_device(struct omap_overlay_manager *mgr,
@@ -745,16 +762,28 @@ err:
int dss_ovl_set_info(struct omap_overlay *ovl,
struct omap_overlay_info *info)
{
+ unsigned long flags;
+
+ spin_lock_irqsave(&data_lock, flags);
+
ovl->info = *info;
ovl->info_dirty = true;
+ spin_unlock_irqrestore(&data_lock, flags);
+
return 0;
}
void dss_ovl_get_info(struct omap_overlay *ovl,
struct omap_overlay_info *info)
{
+ unsigned long flags;
+
+ spin_lock_irqsave(&data_lock, flags);
+
*info = ovl->info;
+
+ spin_unlock_irqrestore(&data_lock, flags);
}
The get/set info functions for overlays and managers only modify the
omap_overlay_info or manager_info structs, these aren't really a part of
'dss_data', they only become a part of dss_data only when we call
mgr->apply().
So, are we protecting these functions so that 2 users of the same
overlay don't see incorrect info values?
Archit
int dss_ovl_set_manager(struct omap_overlay *ovl,
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] Introducing a generic AMP framework
On Wed, Nov 23, 2011 at 3:33 AM, Rusty Russell wrote: > That would imply that I'd done more than glance over them, and > unfortunately I haven't :( Np, thanks for glancing :) -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RESEND v2 2/2] regulator: TPS65910: VDD1/2 voltage selector count
Hi Afzal, On 23 November 2011 05:31, Mohammed, Afzal wrote: > Hi Liam, > > On Tue, Nov 08, 2011 at 21:26:39, Mark Brown wrote: >> On Tue, Nov 08, 2011 at 06:54:10PM +0530, Afzal Mohammed wrote: >> > Count of selector voltage is required for regulator_set_voltage >> > to work via set_voltage_sel. VDD1/2 currently have it as zero, >> > so regulator_set_voltage won't work for VDD1/2. >> > Update count (n_voltages) for VDD1/2. >> >> Acked-by: Mark Brown >> > > Can you please help this patch to get into mainline Kernel. > Without this VDD1 & 2 voltages cannot be varied on TPS65910. > > This patch applies cleanly to current mainline and is not > dependent on any other patch. > Will do, just been getting stuff back together after the kernel.org break in and a lot travel. Liam -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 42/65] OMAPDSS: DSI: call mgr_enable/disable for cmd mode displays
On Tuesday 22 November 2011 02:51 PM, Tomi Valkeinen wrote:
The current code uses dsi_video_mode_enable/disable functions to
enable/disable DISPC output for video mode displays. For command mode
displays we have no notion in the DISPC side of whether the panel is
enabled, except when a dss_start_update() call is made.
However, to properly maintain the DISPC state in apply.c, we need to
know if a manager used for a manual update display is currently in use.
This patch achieves that by changing dsi_video_mode_enable/disable to
dsi_enable/disable_video_output, which is called by both video and
command mode displays. For video mode displays it starts the actual
pixel stream, as it did before. For command mode displays it doesn't do
anything else than mark that the manager is currently in use.
dsi_video_mode_enable() doesn't only enable the DISPC output, it also
sends the long packet header to start video mode transfer.
I think it would be better if we had 2 separate functions, one which
starts/stops DSI video mode, and the other which enables/disables the
DISPC video port.
This way, a manual update panel would need to call only
dsi_enable/disable_video_output(which just enables or disables the
manager), whereas a video mode panel will need to call both.
This is just a suggestion though. It's probably okay to have both in the
same function too. We might have to separate them out later if we were
planning to standardise mipi dsi across SoCs.
Archit
Signed-off-by: Tomi Valkeinen
---
drivers/video/omap2/displays/panel-taal.c |6 ++
drivers/video/omap2/dss/apply.c |6 ++-
drivers/video/omap2/dss/dsi.c | 73 +++-
include/video/omapdss.h |4 +-
4 files changed, 51 insertions(+), 38 deletions(-)
diff --git a/drivers/video/omap2/displays/panel-taal.c
b/drivers/video/omap2/displays/panel-taal.c
index dd64bd1..00c5c61 100644
--- a/drivers/video/omap2/displays/panel-taal.c
+++ b/drivers/video/omap2/displays/panel-taal.c
@@ -1182,6 +1182,10 @@ static int taal_power_on(struct omap_dss_device *dssdev)
if (r)
goto err;
+ r = dsi_enable_video_output(dssdev, td->channel);
+ if (r)
+ goto err;
+
td->enabled = 1;
if (!td->intro_printed) {
@@ -1211,6 +1215,8 @@ static void taal_power_off(struct omap_dss_device *dssdev)
struct taal_data *td = dev_get_drvdata(&dssdev->dev);
int r;
+ dsi_disable_video_output(dssdev, td->channel);
+
r = taal_dcs_write_0(td, MIPI_DCS_SET_DISPLAY_OFF);
if (!r)
r = taal_sleep_in(td);
diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
index 9ad2a36..66f4c56 100644
--- a/drivers/video/omap2/dss/apply.c
+++ b/drivers/video/omap2/dss/apply.c
@@ -648,7 +648,8 @@ void dss_mgr_enable(struct omap_overlay_manager *mgr)
{
mutex_lock(&apply_lock);
- dispc_mgr_enable(mgr->id, true);
+ if (!mgr_manual_update(mgr))
+ dispc_mgr_enable(mgr->id, true);
mgr->enabled = true;
mutex_unlock(&apply_lock);
@@ -658,7 +659,8 @@ void dss_mgr_disable(struct omap_overlay_manager *mgr)
{
mutex_lock(&apply_lock);
- dispc_mgr_enable(mgr->id, false);
+ if (!mgr_manual_update(mgr))
+ dispc_mgr_enable(mgr->id, false);
mgr->enabled = false;
mutex_unlock(&apply_lock);
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index 08d3de90..a35f3fb 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -3939,65 +3939,70 @@ static void dsi_proto_timings(struct omap_dss_device
*dssdev)
}
}
-int dsi_video_mode_enable(struct omap_dss_device *dssdev, int channel)
+int dsi_enable_video_output(struct omap_dss_device *dssdev, int channel)
{
struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
int bpp = dsi_get_pixel_size(dssdev->panel.dsi_pix_fmt);
u8 data_type;
u16 word_count;
- switch (dssdev->panel.dsi_pix_fmt) {
- case OMAP_DSS_DSI_FMT_RGB888:
- data_type = MIPI_DSI_PACKED_PIXEL_STREAM_24;
- break;
- case OMAP_DSS_DSI_FMT_RGB666:
- data_type = MIPI_DSI_PIXEL_STREAM_3BYTE_18;
- break;
- case OMAP_DSS_DSI_FMT_RGB666_PACKED:
- data_type = MIPI_DSI_PACKED_PIXEL_STREAM_18;
- break;
- case OMAP_DSS_DSI_FMT_RGB565:
- data_type = MIPI_DSI_PACKED_PIXEL_STREAM_16;
- break;
- default:
- BUG();
- };
+ if (dssdev->panel.dsi_mode == OMAP_DSS_DSI_VIDEO_MODE) {
+ switch (dssdev->panel.dsi_pix_fmt) {
+ case OMAP_DSS_DSI_FMT_RGB888:
+ data_type = MIPI_DSI_PACKED_PIXEL_STREAM_24;
+ break;
+ case OMAP_DSS_DSI_FMT_RGB666:
+
Re: [PATCH 41/65] OMAPDSS: APPLY: add missing uses of spinlock
On Wed, 2011-11-23 at 15:26 +0530, Archit Taneja wrote:
> On Tuesday 22 November 2011 02:51 PM, Tomi Valkeinen wrote:
> > int dss_mgr_set_device(struct omap_overlay_manager *mgr,
> > @@ -745,16 +762,28 @@ err:
> > int dss_ovl_set_info(struct omap_overlay *ovl,
> > struct omap_overlay_info *info)
> > {
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&data_lock, flags);
> > +
> > ovl->info = *info;
> > ovl->info_dirty = true;
> >
> > + spin_unlock_irqrestore(&data_lock, flags);
> > +
> > return 0;
> > }
> >
> > void dss_ovl_get_info(struct omap_overlay *ovl,
> > struct omap_overlay_info *info)
> > {
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&data_lock, flags);
> > +
> > *info = ovl->info;
> > +
> > + spin_unlock_irqrestore(&data_lock, flags);
> > }
>
> The get/set info functions for overlays and managers only modify the
> omap_overlay_info or manager_info structs, these aren't really a part of
> 'dss_data', they only become a part of dss_data only when we call
> mgr->apply().
>
> So, are we protecting these functions so that 2 users of the same
> overlay don't see incorrect info values?
True, at this point the data_lock is a bit vague, and is protecting also
the info fields in omap_overlay and omap_overlay_manager.
A lock is needed, though, as otherwise the info struct may be only
partial. E.g. somebody calls set_info, which is half way copying the
values, and somebody else calls apply or get_info.
In the next patches the infos will be moved into the dss_data, and then
using dss_lock spin lock makes more sense.
Tomi
signature.asc
Description: This is a digitally signed message part
Re: [PATCH 40/65] OMAPDSS: APPLY: add mutex
On Wed, 2011-11-23 at 15:18 +0530, Archit Taneja wrote: > Hi, > > On Tuesday 22 November 2011 02:51 PM, Tomi Valkeinen wrote: > > The functions in apply.c, called mostly via function pointers in overlay > > and overlay_manager structs, will be divided into two groups. The other > > group will not sleep and can be called from interrupts, and the other > > group may sleep. > > Small sentence issue above, both groups are called the 'other group'. Thanks, fixed. > > > > The idea is that the non-sleeping functions may only change certain > > settings in overlays and managers, and those settings may only affect > > the particular overlay/manager. For example, set the base address of the > > overlay. > > > > The blocking functions, however, will handle more complex configuration > > changes. For example, when an overlay is enabled and fifo-merge feature > > is used, we need to do the enable in multiple steps, waiting in between, > > and the change affects multiple overlays and managers. > > > > This patch adds the mutex which is used in the blocking functions to > > have exclusive access to overlays and overlay managers. > > Previously, when we changed the links between 'overlay->managers' and > 'manager->devices', it wasn't protected by a lock. Why is it needed now? Previously many places were missing a lock =). > As an example, suppose we are changing a manager's device to some other > display. Is this lock preventing someone else to get the older > 'mgr->device' rather than the new one? Hmm. We need some lock there, that's for sure, as set/unset manager are changing the manager's list of overlays. However, it is also protected by the spinlock, so in that sense mutex is not necessary. I have to say I'm not sure if mutex is needed at this point. However, consider the end result when fifo-merge is used: dss_ovl_enable() will take the mutex, then it does the configuration in multiple steps, doing multiple spin_lock & spin_unlocks, waiting in between. If we had only a spinlock in set/unset_manager, the manager could be changed while dss_ovl_enable is doing the process of enabling the overlay. So I may have added mutexes or spinlocks a bit early in the series to some places, but I don't see any harm in that. It'd be rather difficult to try to find the exact spots where a lock becomes a requirement. Tomi signature.asc Description: This is a digitally signed message part
Re: [PATCH 25/26] OMAP: PM: convert the SmartReflex code into the AVS driver framework
Hi Felipe, On Wed, Nov 23, 2011 at 10:51 AM, Felipe Balbi wrote: > On Tue, Nov 22, 2011 at 04:06:09PM +0100, jean.pi...@newoldbits.com wrote: >> +config POWER_AVS_OMAP_V1 >> + tristate "AVS support for the OMAP IP version 1" >> + depends on ARCH_OMAP3 && PM >> + help >> + Say Y to enable AVS support on OMAP containing the version 1 of >> + the SmartReflex IP. >> + V1 is the 65nm version used in OMAP3430. >> + >> + Please note, that by default SmartReflex is only >> + initialized. To enable the automatic voltage >> + compensation for vdd mpu and vdd core from user space, >> + user must write 1 to >> + /debug/voltage/vdd_/smartreflex/autocomp, >> + where X is mpu or core for OMAP3. >> + Optionally autocompensation can be enabled in the kernel >> + by default during system init via the enable_on_init flag >> + which an be passed as platform data to the smartreflex driver. >> + >> +config POWER_AVS_OMAP_V2 >> + tristate "AVS support for the OMAP IP version 2" >> + depends on (ARCH_OMAP3 || ARCH_OMAP4) && PM >> + help >> + Say Y to enable AVS support on OMAP containing the version 2 of >> + the SmartReflex IP. >> + V2 is the update for the 45nm version of the IP used in OMAP3630 >> + and OMAP4430 > > can't you read the revision register and decide this in runtime ? Those Kconfig options are used to compile the v1 and/or v2 drivers. The init of v1 or v2 is decided at runtime, cf. the sr_init functions where the cpu revision is checked. Is this the correct check? > > -- > balbi > Thanks, Jean -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] Introducing a generic AMP framework
On Wed, Nov 23, 2011 at 5:25 AM, Saravana Kannan wrote: > Sorry for the rant, this naming just rubs me the wrong way. I definitely > appreciate the idea behind these patches. I don't share the same naming concerns you have (if any, then confusion with the bluetooth AMP patches and prefixes is more of a concern to me), but I don't care deeply about names. Feel free to offer a different name, though really 'amp' here only describes the general model and motivation and is rarely used throughout the code; we mostly either use 'remoteproc' or 'rpmsg', which respectively refer to the two frameworks that are being added (the former responsible for controlling the state of the remote processors, and the latter for communicating with them). Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 35/65] OMAPDSS: APPLY: move spinlock outside the struct
On 11/22/2011 11:21 AM, Tomi Valkeinen wrote: dss_cache struct contains a spinlock used to protect the struct. A more logical place for the spinlock is outside the struct that it is protecting. So move it there. a small question: isn't it clearer to keep lock inside struct, so it would be easier to read code? Say, if we meet spin_lock_irqsave(&dss_cache.lock, flags); in code we already aware of what struct being actually protected, and in case of external lock it's not that obvious -- regards, Sergey -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 42/65] OMAPDSS: DSI: call mgr_enable/disable for cmd mode displays
On Wed, 2011-11-23 at 15:40 +0530, Archit Taneja wrote: > On Tuesday 22 November 2011 02:51 PM, Tomi Valkeinen wrote: > > The current code uses dsi_video_mode_enable/disable functions to > > enable/disable DISPC output for video mode displays. For command mode > > displays we have no notion in the DISPC side of whether the panel is > > enabled, except when a dss_start_update() call is made. > > > > However, to properly maintain the DISPC state in apply.c, we need to > > know if a manager used for a manual update display is currently in use. > > > > This patch achieves that by changing dsi_video_mode_enable/disable to > > dsi_enable/disable_video_output, which is called by both video and > > command mode displays. For video mode displays it starts the actual > > pixel stream, as it did before. For command mode displays it doesn't do > > anything else than mark that the manager is currently in use. > > dsi_video_mode_enable() doesn't only enable the DISPC output, it also > sends the long packet header to start video mode transfer. > > I think it would be better if we had 2 separate functions, one which > starts/stops DSI video mode, and the other which enables/disables the > DISPC video port. > > This way, a manual update panel would need to call only > dsi_enable/disable_video_output(which just enables or disables the > manager), whereas a video mode panel will need to call both. > > This is just a suggestion though. It's probably okay to have both in the > same function too. We might have to separate them out later if we were > planning to standardise mipi dsi across SoCs. If you think from the panel driver's point of view, it doesn't know about DISPC. It just wants to enable the video stream (on video mode displays). If we had two functions, could only the first be used? I.e. is it possible to just enable the video mode transfer, without enabling DISPC? If not, I'm not sure what would be the use for two separate functions. And even if it can, I'm not sure what use it would be to enable only the video mode output without the actual pixel data from DISPC. It is true that the function in thsi patch is not the best one. For command mode display it's more about reserving the ovl manager for use than actually enabling it. Then again, how I thought the function's purpose was that it enables the DSI video output, but because for command mode there's need to trigger the actual frame transfer later, the function doesn't start the pixel feed for command mode displays. It just "prepares" the output. But even if we had the functions separated, the function called by video mode and command mode displays would be different, as for the former one it enables the pixel stream, and for latter one it just reserves the output. So, I'm fine with changing the function, but the reasoning for what functions we have and what they do should come from the panel driver's perspective, not because of OMAP DSS's HW details. Tomi signature.asc Description: This is a digitally signed message part
Re: [PATCH 35/65] OMAPDSS: APPLY: move spinlock outside the struct
On Wed, 2011-11-23 at 12:29 +0200, Sergey Kibrik wrote: > On 11/22/2011 11:21 AM, Tomi Valkeinen wrote: > > dss_cache struct contains a spinlock used to protect the struct. A more > > logical place for the spinlock is outside the struct that it is > > protecting. So move it there. > > a small question: isn't it clearer to keep lock inside struct, so it would be > easier to read code? Say, if we meet > > > spin_lock_irqsave(&dss_cache.lock, flags); > > in code we already aware of what struct being actually protected, and in case > of external lock it's not that obvious But if you meet code like: op = get_ovl_priv(ovl); You don't see that the data is inside the struct protected with the spinlock. So you still need to understand what it protects, and what the above function returns. But I see your point. I'm not sure which way is better. I thought it like this: the lock protects a struct, but if the lock is inside the struct, the lock would protect also itself. Which it doesn't. That said, I'm fine with both ways. It doesn't matter much. I didn't really look for any established patterns for this in the kernel code, but if everybody else have their locks inside the structs they protect, then obviously we should also. Tomi signature.asc Description: This is a digitally signed message part
Re: [RFC 1/3] pinctrl: add a driver for the OMAP pinmux
On 16:28 Tue 22 Nov , Stephen Warren wrote:
> Tony Lindgren wrote at Tuesday, November 22, 2011 10:54 AM:
> > * Linus Walleij [22 03:30]:
> > > On Tue, Nov 22, 2011 at 12:09 PM, Thomas Abraham
> > > wrote:
> > > > On 17 November 2011 19:27, Linus Walleij
> > > > wrote:
> > > >>
> > > >> Maybe I'm mistaken about the device tree ambitions, but
> > > >> I was sort of hoping that it would not contain too much
> > > >> custom magic numbers that need to be cross-referenced
> > > >> elsewhere ... or rather - the more understandable the device
> > > >> tree is, the more we win.
> > > >
> > > > Device tree is expected to describe the hardware. So to
> > > > cross-reference the hardware manual to understand device tree should
> > > > be fine I guess. For instance, GPIO numbers in dts would be written as
> > > > a numeric number and not a enum or other format. And by looking up the
> > > > manual, we understand the actual details of the GPIO pin.
> > > >
> > > > If dt-compiler had a option to support #define like in C, the numbers
> > > > could have been made more easier to understand. Like, 3 to mean
> > > > GPIO_PULL_UP in Exynos dts file.
> > >
> > > OK I think I get it now, so DT bindings shall really NOT be read
> > > by any of the pinctrl core, it is *supposed* to be all driver-specific.
> > > Then it makes perfect sense to have it as it is.
> >
> > Yes the driver nodes should describe in DT which pins to use:
> >
> > serial@1234 {
> > compatible = "8250";
> > reg = <0x1234 0x40>;
> > reg-shift = <2>;
> > interrupts = < 10 >;
> > pins = "uart1_rx", "uart1_tx";
> > };
>
> Sorry to jump in late here, but I wasn't aware of this thread.
>
> I don't necessarily agree with that. Describing the HW doesn't necessarily
> mean that each device needs to describe what pinmux pins it uses; one
> could quite easily have the pinmux describe what settings the various
> pins should have and which drivers will use those pins. That would map
> very well to the pinctrl subsystem's mapping table, and at least Tegra's
> existing pinmux tables, which are both just a big array of settings which
> end up getting provided to drivers.
>
> I'll try and track down the rest of this thread and catch up though...
I agreee here
as example on at91 I try to found a good way to let the macb driver to ask the
pin configuration
so in my mind i do not need put all pins in each board but in the dtsi and
then in the drivers just said
pins = "mii";
or
pins = "rmii";
or if I want to use the alternative config
pins = "mii_alt";
or
pins = "rmii_alt";
and then in the dtsi I describe the pin used for those configs
which is soc specifific not board
Best Regards,
J.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: [PATCH 2/5] ASoC: OMAP4: omap-dmic: Initial support for OMAP DMIC
On Wed, Nov 23, 2011 at 10:48:24AM +0200, Péter Ujfalusi wrote:
> On Tuesday 22 November 2011 16:01:05 Mark Brown wrote:
> > > + dmic_clk = clk_get(dmic->dev, "dmic_fck");
> > > + if (IS_ERR(dmic_clk)) {
> > > + dev_err(dmic->dev, "cant get dmic_fck\n");
> > > + return -ENODEV;
> > > + }
> > Why aren't we getting and holding a reference to the clock over the
> > entire lifetime of the driver?
> We only need the reference for the dmic_fclk for reparenting which will
> happen
> only once in most cases (or not at all, if pad_clks_ck is going to be used).
> I
> don't think we should hold the reference for the dmic_fclk.
> The clock handling is done via pm_runtime_get/put_sync().
This just seems like it's making the code needlessly complex - there's
no harm in holding the reference if you don't enable/disable the clock
and it makes this function much simpler.
> > > + /* disable clock while reparenting */
> > > + pm_runtime_put_sync(dmic->dev);
> > > + ret = clk_set_parent(dmic_clk, parent_clk);
> > > + pm_runtime_get_sync(dmic->dev);
> > Since we're only allowing reclocking while idle shouldn't the clock
> > already be disabled? Seems like it ought to be good for power if
> > nothing else...
> We enable the clocks at dai_startup for the DMIC (and disable them on
> dai_shutdown). We can not reparent while the clocks are enabled.
> This is the reason for this part.
That sounds like the enable is happening too early, then.
> > > +static int omap_dmic_set_clkdiv(struct snd_soc_dai *dai,
> > > + int div_id, int div)
> > > +{
> > DMIC clocking is usually fairly simple so it seems surprising that the
> > driver isn't able to figure this out for itself.
> The clock towards the external digital mics are based on the DMIC_FCLK rate.
> In case of DMIC_FCLK = 19.2MHz, 24.576MHz we can select between two dividers
> which will result different clocks for the external microphones.
> I would rather leave this decision to the machine driver which knows the
> external components, and can pick the best divider for them.
If that's what you're doing then it seems like the machine drivers
should be use set_sysclk() (or perhaps even the clk API) to set up the
rate they're looking for from the visible clock rather than fiddling
about with magic divider values. That way they can say exactly what
they want directly in terms of the result they're looking for.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 25/26] OMAP: PM: convert the SmartReflex code into the AVS driver framework
On Wed, Nov 23, 2011 at 11:22:42AM +0100, Jean Pihet wrote: > Hi Felipe, > > On Wed, Nov 23, 2011 at 10:51 AM, Felipe Balbi wrote: > > On Tue, Nov 22, 2011 at 04:06:09PM +0100, jean.pi...@newoldbits.com wrote: > >> +config POWER_AVS_OMAP_V1 > >> + tristate "AVS support for the OMAP IP version 1" > >> + depends on ARCH_OMAP3 && PM > >> + help > >> + Say Y to enable AVS support on OMAP containing the version 1 of > >> + the SmartReflex IP. > >> + V1 is the 65nm version used in OMAP3430. > >> + > >> + Please note, that by default SmartReflex is only > >> + initialized. To enable the automatic voltage > >> + compensation for vdd mpu and vdd core from user space, > >> + user must write 1 to > >> + /debug/voltage/vdd_/smartreflex/autocomp, > >> + where X is mpu or core for OMAP3. > >> + Optionally autocompensation can be enabled in the kernel > >> + by default during system init via the enable_on_init flag > >> + which an be passed as platform data to the smartreflex driver. > >> + > >> +config POWER_AVS_OMAP_V2 > >> + tristate "AVS support for the OMAP IP version 2" > >> + depends on (ARCH_OMAP3 || ARCH_OMAP4) && PM > >> + help > >> + Say Y to enable AVS support on OMAP containing the version 2 of > >> + the SmartReflex IP. > >> + V2 is the update for the 45nm version of the IP used in OMAP3630 > >> + and OMAP4430 > > > > can't you read the revision register and decide this in runtime ? > Those Kconfig options are used to compile the v1 and/or v2 drivers. > The init of v1 or v2 is decided at runtime, cf. the sr_init functions > where the cpu revision is checked. Is this the correct check? if you already decide in runtime the correct initialization to call, why do you add ifdefs ? It's not like you're adding that huge amount of code for v1 and v2, right ? -- balbi signature.asc Description: Digital signature
Re: [PATCH 42/65] OMAPDSS: DSI: call mgr_enable/disable for cmd mode displays
On Wednesday 23 November 2011 04:12 PM, Tomi Valkeinen wrote: On Wed, 2011-11-23 at 15:40 +0530, Archit Taneja wrote: On Tuesday 22 November 2011 02:51 PM, Tomi Valkeinen wrote: The current code uses dsi_video_mode_enable/disable functions to enable/disable DISPC output for video mode displays. For command mode displays we have no notion in the DISPC side of whether the panel is enabled, except when a dss_start_update() call is made. However, to properly maintain the DISPC state in apply.c, we need to know if a manager used for a manual update display is currently in use. This patch achieves that by changing dsi_video_mode_enable/disable to dsi_enable/disable_video_output, which is called by both video and command mode displays. For video mode displays it starts the actual pixel stream, as it did before. For command mode displays it doesn't do anything else than mark that the manager is currently in use. dsi_video_mode_enable() doesn't only enable the DISPC output, it also sends the long packet header to start video mode transfer. I think it would be better if we had 2 separate functions, one which starts/stops DSI video mode, and the other which enables/disables the DISPC video port. This way, a manual update panel would need to call only dsi_enable/disable_video_output(which just enables or disables the manager), whereas a video mode panel will need to call both. This is just a suggestion though. It's probably okay to have both in the same function too. We might have to separate them out later if we were planning to standardise mipi dsi across SoCs. If you think from the panel driver's point of view, it doesn't know about DISPC. It just wants to enable the video stream (on video mode displays). If we had two functions, could only the first be used? I.e. is it possible to just enable the video mode transfer, without enabling DISPC? If not, I'm not sure what would be the use for two separate functions. And even if it can, I'm not sure what use it would be to enable only the video mode output without the actual pixel data from DISPC. It is true that the function in thsi patch is not the best one. For command mode display it's more about reserving the ovl manager for use than actually enabling it. Then again, how I thought the function's purpose was that it enables the DSI video output, but because for command mode there's need to trigger the actual frame transfer later, the function doesn't start the pixel feed for command mode displays. It just "prepares" the output. But even if we had the functions separated, the function called by video mode and command mode displays would be different, as for the former one it enables the pixel stream, and for latter one it just reserves the output. So, I'm fine with changing the function, but the reasoning for what functions we have and what they do should come from the panel driver's perspective, not because of OMAP DSS's HW details. If you look from the panel driver's perspective, we shouldn't split this. I think it would be best to stuff the 'video mode enabling and manager enabling' functionality in omapdss_dsi_display_enable() itself, the panel driver shouldn't need to call a function separately to enable video mode for the panel. This way we would be more along the lines of the dpi driver, where dpi_display_enable() enables the manager in the end. I guess we can leave this the way you are doing it currently, and move this code into dsi_display_enable() code later on. Archit Tomi -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 42/65] OMAPDSS: DSI: call mgr_enable/disable for cmd mode displays
On Wed, 2011-11-23 at 16:38 +0530, Archit Taneja wrote: > > I think it would be best to stuff the 'video mode enabling and > manager > enabling' functionality in omapdss_dsi_display_enable() itself, the > panel driver shouldn't need to call a function separately to enable > video mode for the panel. This way we would be more along the lines > of > the dpi driver, where dpi_display_enable() enables the manager in the > end. But we need to configure the panel between enabling the DSI interface and enabling the video output, so we can't combine those two functions. For DPI things are simpler, as enabling the interface and the video output are more or less the same thing. Tomi signature.asc Description: This is a digitally signed message part
Re: [PATCH 25/26] OMAP: PM: convert the SmartReflex code into the AVS driver framework
Felipe, On Wed, Nov 23, 2011 at 12:04 PM, Felipe Balbi wrote: > On Wed, Nov 23, 2011 at 11:22:42AM +0100, Jean Pihet wrote: >> Hi Felipe, >> >> On Wed, Nov 23, 2011 at 10:51 AM, Felipe Balbi wrote: >> > On Tue, Nov 22, 2011 at 04:06:09PM +0100, jean.pi...@newoldbits.com wrote: >> >> +config POWER_AVS_OMAP_V1 >> >> + tristate "AVS support for the OMAP IP version 1" >> >> + depends on ARCH_OMAP3 && PM >> >> + help >> >> + Say Y to enable AVS support on OMAP containing the version 1 of >> >> + the SmartReflex IP. >> >> + V1 is the 65nm version used in OMAP3430. >> >> + >> >> + Please note, that by default SmartReflex is only >> >> + initialized. To enable the automatic voltage >> >> + compensation for vdd mpu and vdd core from user space, >> >> + user must write 1 to >> >> + /debug/voltage/vdd_/smartreflex/autocomp, >> >> + where X is mpu or core for OMAP3. >> >> + Optionally autocompensation can be enabled in the kernel >> >> + by default during system init via the enable_on_init flag >> >> + which an be passed as platform data to the smartreflex driver. >> >> + >> >> +config POWER_AVS_OMAP_V2 >> >> + tristate "AVS support for the OMAP IP version 2" >> >> + depends on (ARCH_OMAP3 || ARCH_OMAP4) && PM >> >> + help >> >> + Say Y to enable AVS support on OMAP containing the version 2 of >> >> + the SmartReflex IP. >> >> + V2 is the update for the 45nm version of the IP used in OMAP3630 >> >> + and OMAP4430 >> > >> > can't you read the revision register and decide this in runtime ? >> Those Kconfig options are used to compile the v1 and/or v2 drivers. >> The init of v1 or v2 is decided at runtime, cf. the sr_init functions >> where the cpu revision is checked. Is this the correct check? > > if you already decide in runtime the correct initialization to call, why > do you add ifdefs ? There is no #ifdef with CONFIG_POWER_AVS_OMAP_V[12], those options are used to compile or not the respective modules, cf. driver/power/avs/Makefile. > It's not like you're adding that huge amount of code > for v1 and v2, right ? That is correct, so both modules could be always compiled and init'ed at runtime depending on the chip revision. I am OK to change the code, please let me know what you think. > > -- > balbi > Thanks, Jean -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 25/26] OMAP: PM: convert the SmartReflex code into the AVS driver framework
Hi, On Wed, Nov 23, 2011 at 12:30:54PM +0100, Jean Pihet wrote: > >> On Wed, Nov 23, 2011 at 10:51 AM, Felipe Balbi wrote: > >> > On Tue, Nov 22, 2011 at 04:06:09PM +0100, jean.pi...@newoldbits.com > >> > wrote: > >> >> +config POWER_AVS_OMAP_V1 > >> >> + tristate "AVS support for the OMAP IP version 1" > >> >> + depends on ARCH_OMAP3 && PM > >> >> + help > >> >> + Say Y to enable AVS support on OMAP containing the version 1 of > >> >> + the SmartReflex IP. > >> >> + V1 is the 65nm version used in OMAP3430. > >> >> + > >> >> + Please note, that by default SmartReflex is only > >> >> + initialized. To enable the automatic voltage > >> >> + compensation for vdd mpu and vdd core from user space, > >> >> + user must write 1 to > >> >> + /debug/voltage/vdd_/smartreflex/autocomp, > >> >> + where X is mpu or core for OMAP3. > >> >> + Optionally autocompensation can be enabled in the kernel > >> >> + by default during system init via the enable_on_init flag > >> >> + which an be passed as platform data to the smartreflex driver. > >> >> + > >> >> +config POWER_AVS_OMAP_V2 > >> >> + tristate "AVS support for the OMAP IP version 2" > >> >> + depends on (ARCH_OMAP3 || ARCH_OMAP4) && PM > >> >> + help > >> >> + Say Y to enable AVS support on OMAP containing the version 2 of > >> >> + the SmartReflex IP. > >> >> + V2 is the update for the 45nm version of the IP used in OMAP3630 > >> >> + and OMAP4430 > >> > > >> > can't you read the revision register and decide this in runtime ? > >> Those Kconfig options are used to compile the v1 and/or v2 drivers. > >> The init of v1 or v2 is decided at runtime, cf. the sr_init functions > >> where the cpu revision is checked. Is this the correct check? > > > > if you already decide in runtime the correct initialization to call, why > > do you add ifdefs ? > There is no #ifdef with CONFIG_POWER_AVS_OMAP_V[12], those options are > used to compile or not the respective modules, cf. > driver/power/avs/Makefile. separate modules ? Can't that be combined into one driver only ? -- balbi signature.asc Description: Digital signature
Re: [PATCH 1/2] OMAPDSS: HDMI: Rid hw_params of extra argument
Hi, On Tue, 2011-11-22 at 20:53 +0530, Jassi Brar wrote: > Signed-off-by: Jassi Brar > --- Please write proper commit messages. For example, check http://who-t.blogspot.com/2009/12/on-commit-messages.html Tomi signature.asc Description: This is a digitally signed message part
Re: [PATCH 42/65] OMAPDSS: DSI: call mgr_enable/disable for cmd mode displays
On Wednesday 23 November 2011 04:57 PM, Tomi Valkeinen wrote: On Wed, 2011-11-23 at 16:38 +0530, Archit Taneja wrote: I think it would be best to stuff the 'video mode enabling and manager enabling' functionality in omapdss_dsi_display_enable() itself, the panel driver shouldn't need to call a function separately to enable video mode for the panel. This way we would be more along the lines of the dpi driver, where dpi_display_enable() enables the manager in the end. But we need to configure the panel between enabling the DSI interface and enabling the video output, so we can't combine those two functions. Oh okay, that's right, we can't start video mode before preparing the panel. Archit For DPI things are simpler, as enabling the interface and the video output are more or less the same thing. Tomi -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 24/24] gpio/omap: handle set_dataout reg capable IP on restore
On Mon, Nov 7, 2011 at 5:35 PM, DebBarma, Tarun Kanti
wrote:
> On Fri, Nov 4, 2011 at 10:23 PM, Kevin Hilman wrote:
>> Tarun Kanti DebBarma writes:
>>
>>> From: Nishanth Menon
>>>
>>> GPIO IP revisions such as those used in OMAP4 have a set_dataout
>>> while the previous revisions used a single dataout register.
>>> Depending on what is available restore the dataout settings
>>> to the right register.
>>>
>>> Signed-off-by: Nishanth Menon
>>> Signed-off-by: Tarun Kanti DebBarma
>>> Reviewed-by: Santosh Shilimkar
>>> ---
>>> drivers/gpio/gpio-omap.c | 9 +++--
>>> 1 files changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>>> index 4009446..3df7a98 100644
>>> --- a/drivers/gpio/gpio-omap.c
>>> +++ b/drivers/gpio/gpio-omap.c
>>> @@ -1073,7 +1073,7 @@ static int __devinit omap_gpio_probe(struct
>>> platform_device *pdev)
>>> bank->get_context_loss_count = pdata->get_context_loss_count;
>>> bank->regs = pdata->regs;
>>>
>>> - if (bank->regs->set_dataout && bank->regs->clr_dataout)
>>> + if (bank->regs->set_dataout)
>>
>> This change isn't right.
>>
>> The _set_gpio_dataout_reg function depends on the existence of
>> ->clr_dataout too.
> Ok, I will add the clr_dataout condtion as well.
>
>>
>>> bank->set_dataout = _set_gpio_dataout_reg;
>>> else
>>> bank->set_dataout = _set_gpio_dataout_mask;
>>> @@ -1351,7 +1351,12 @@ static void omap_gpio_restore_context(struct
>>> gpio_bank *bank)
>>> bank->base + bank->regs->risingdetect);
>>> __raw_writel(bank->context.fallingdetect,
>>> bank->base + bank->regs->fallingdetect);
>>> - __raw_writel(bank->context.dataout, bank->base + bank->regs->dataout);
>>> + if (bank->regs->set_dataout)
>>
>> Why the check again? The check has already been done in probe.
>>
>> Just use bank->set_dataout() here.
> Sure, i will make the change.
When I look at the signature of set_dataout(), it does not seem straight forward
to be used here. It expects (struct gpio_bank *bank, int gpio, int enable) to be
passed to it.
set_dataout (struct gpio_bank *bank, int gpio, int enable)
{
void __iomem *reg = bank->base;
u32 l = GPIO_BIT(bank, gpio);
...
if (enable)
...
else
...
__raw_writel(l, reg);
}
--
Tarun
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: OMAP: dmtimer: fix missing content/correction in low-power mode support
On Mon, Nov 21, 2011 at 10:12 PM, Ramirez Luna, Omar
wrote:
> Hi,
>
> On Tue, Nov 8, 2011 at 12:00 AM, Tarun Kanti DebBarma
> wrote:
>> Since omap_dm_timer_write_reg/__omap_dm_timer_write is now modified
>> to use timer->func_base OCP_CFG should not use this wrapper anymore.
>> Instead use __raw_writel() directly and use timer->io_base instead
>> to write to OCP_CFG.
>>
>> The timer->sys_stat is valid only if timer->revision is 1. In the
>> context restore function make this correction.
>>
>> Save the contexts and loss count when timer is stopped.
>> Also, disable the clock. Else, clock usecount would become imbalanced.
>>
>> Signed-off-by: Tarun Kanti DebBarma
> ...
>> diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
>> index af3b92b..f042c82 100644
>> --- a/arch/arm/plat-omap/dmtimer.c
>> +++ b/arch/arm/plat-omap/dmtimer.c
> ...
>> @@ -357,6 +357,21 @@ int omap_dm_timer_stop(struct omap_dm_timer *timer)
>>
>> __omap_dm_timer_stop(timer, timer->posted, rate);
>>
>> + if (timer->loses_context) {
>> + if (timer->get_context_loss_count)
>
> Maybe: if (timer->loses_context && timer->get_context_loss_count)
Sure.
>
>> + timer->ctx_loss_count =
>> + timer->get_context_loss_count(&timer->pdev->dev);
>
> could get rid of this weird one-line formatting
ok.
>
>> + }
>> +
>> + /*
>> + * Since the register values are computed and written within
>> + * __omap_dm_timer_stop, we need to use read to retrieve the
>> + * context.
>> + */
>> + timer->context.tclr =
>> + omap_dm_timer_read_reg(timer, OMAP_TIMER_CTRL_REG);
>> + timer->context.tisr = __raw_readl(timer->irq_stat);
>> + omap_dm_timer_disable(timer);
>
> FWIW, functionally it looks good to me, besides it fixes the broken
> dmtimer start/stop sequence from 3.2-rc1. Tested with tidspbridge on
> omap3.
Thanks.
>
> If needed:
>
> Tested-by: Omar Ramirez Luna
Sure. Thanks.
--
Tarun
>
> Regards,
>
> Omar
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
AWARD NOTICE
You have been awarded $100,000,000.00 among the 5 beneficiaries from the
Vittorio Foundation.Qualification numbers (P-333-7858,B-011-67)
Email:{vittoriofoundati...@hotmail.com}
Mr. Jennifer David
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: Re: [PATCH 2/5] ASoC: OMAP4: omap-dmic: Initial support for OMAP DMIC
On Wednesday 23 November 2011 10:58:07 Mark Brown wrote: > This just seems like it's making the code needlessly complex - there's > no harm in holding the reference if you don't enable/disable the clock > and it makes this function much simpler. OK. > > We enable the clocks at dai_startup for the DMIC (and disable them on > > dai_shutdown). We can not reparent while the clocks are enabled. > > This is the reason for this part. > > That sounds like the enable is happening too early, then. This only enables the clock for the DMIC block, the clock for the external DMIC will start at trigger time (and stop as well). In order to access to DMIC registers we need clocks, and the clocks are enabled for the duration we have capture stream open. I would think this is acceptable. > If that's what you're doing then it seems like the machine drivers > should be use set_sysclk() (or perhaps even the clk API) to set up the > rate they're looking for from the visible clock rather than fiddling > about with magic divider values. That way they can say exactly what > they want directly in terms of the result they're looking for. In OMAP4 DMIC the divider can not be chosen freely. The clock provided to the external microphones will depend on the selected DMIC_FCLK, and the divider. If I ask the machine driver to ask for specific speed for the external mic, the writer of the machine driver anyways have to look up the table from the TRM for the possible frequencies. So instead of providing magic divider it has to provide magic speed. I can do that if you prefer that way, but it just going to 'complicate' the driver a bit (well not that much, we just will have different way of looking up the register value for the divider, and it will be done in omap_dmic_set_dai_sysclk instead of omap_dmic_set_clkdiv). -- Péter -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/6] CBUS patches
Hi Tony, here are a few fixes for CBUS and friends plus an RFC of irq_domain conversion for retu.c. Untested, compile tested only. Felipe Balbi (6): cbus: fix compile breakage cbus: move the module_platform_driver where possible cbus: fix very old compile warning cbus: retu: move irq_chip to our context structure cbus: tahvo: move irq_chip to our context structure cbus: retu: implement irq_domain support drivers/cbus/cbus.c | 23 +--- drivers/cbus/retu-headset.c | 21 +++ drivers/cbus/retu-pwrbutton.c | 20 +++--- drivers/cbus/retu-rtc.c | 18 +++--- drivers/cbus/retu-wdt.c | 19 +++--- drivers/cbus/retu.c | 77 ++-- drivers/cbus/tahvo-usb.c | 21 ++- drivers/cbus/tahvo.c | 34 +++--- 8 files changed, 98 insertions(+), 135 deletions(-) -- 1.7.8.rc3 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/6] cbus: fix compile breakage
we need to include and Signed-off-by: Felipe Balbi --- drivers/cbus/cbus.c |2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/drivers/cbus/cbus.c b/drivers/cbus/cbus.c index 486254d..52eab4a 100644 --- a/drivers/cbus/cbus.c +++ b/drivers/cbus/cbus.c @@ -26,6 +26,8 @@ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */ +#include +#include #include #include #include -- 1.7.8.rc3 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/6] cbus: move the module_platform_driver where possible
this allows us to delete a bunch of boilerplate code
from the drivers. While at that, also add missing
MODULE_ALIAS().
Signed-off-by: Felipe Balbi
---
drivers/cbus/cbus.c | 19 +--
drivers/cbus/retu-headset.c | 21 ++---
drivers/cbus/retu-pwrbutton.c | 20 ++--
drivers/cbus/retu-rtc.c | 18 +-
drivers/cbus/retu-wdt.c | 19 +--
drivers/cbus/retu.c | 13 ++---
drivers/cbus/tahvo-usb.c | 21 +++--
drivers/cbus/tahvo.c | 13 ++---
8 files changed, 42 insertions(+), 102 deletions(-)
diff --git a/drivers/cbus/cbus.c b/drivers/cbus/cbus.c
index 52eab4a..c7ed881 100644
--- a/drivers/cbus/cbus.c
+++ b/drivers/cbus/cbus.c
@@ -247,7 +247,7 @@ int cbus_write_reg(struct device *child, unsigned dev,
unsigned reg,
}
EXPORT_SYMBOL(cbus_write_reg);
-static int __init cbus_bus_probe(struct platform_device *pdev)
+static int __devinit cbus_bus_probe(struct platform_device *pdev)
{
struct cbus_host *chost;
struct cbus_host_platform_data *pdata = pdev->dev.platform_data;
@@ -296,7 +296,7 @@ exit1:
return ret;
}
-static void __exit cbus_bus_remove(struct platform_device *pdev)
+static void __devexit cbus_bus_remove(struct platform_device *pdev)
{
struct cbus_host*chost = platform_get_drvdata(pdev);
@@ -308,23 +308,14 @@ static void __exit cbus_bus_remove(struct platform_device
*pdev)
}
static struct platform_driver cbus_driver = {
- .remove = __exit_p(cbus_bus_remove),
+ .probe = cbus_bus_probe,
+ .remove = __devexit_p(cbus_bus_remove),
.driver = {
.name = "cbus",
},
};
-static int __init cbus_bus_init(void)
-{
- return platform_driver_probe(&cbus_driver, cbus_bus_probe);
-}
-subsys_initcall(cbus_bus_init);
-
-static void __exit cbus_bus_exit(void)
-{
- platform_driver_unregister(&cbus_driver);
-}
-module_exit(cbus_bus_exit);
+module_platform_driver(cbus_driver);
MODULE_DESCRIPTION("CBUS serial protocol");
MODULE_LICENSE("GPL");
diff --git a/drivers/cbus/retu-headset.c b/drivers/cbus/retu-headset.c
index 3b8e138..576b0e6 100644
--- a/drivers/cbus/retu-headset.c
+++ b/drivers/cbus/retu-headset.c
@@ -222,7 +222,7 @@ static void retu_headset_detect_timer(unsigned long arg)
spin_unlock_irqrestore(&hs->lock, flags);
}
-static int __init retu_headset_probe(struct platform_device *pdev)
+static int __devinit retu_headset_probe(struct platform_device *pdev)
{
struct retu_headset *hs;
int irq;
@@ -291,7 +291,7 @@ err1:
return r;
}
-static int retu_headset_remove(struct platform_device *pdev)
+static int __devexit retu_headset_remove(struct platform_device *pdev)
{
struct retu_headset *hs = platform_get_drvdata(pdev);
@@ -333,7 +333,8 @@ static int retu_headset_resume(struct platform_device *pdev)
}
static struct platform_driver retu_headset_driver = {
- .remove = retu_headset_remove,
+ .probe = retu_headset_probe,
+ .remove = __devexit_p(retu_headset_remove),
.suspend= retu_headset_suspend,
.resume = retu_headset_resume,
.driver = {
@@ -341,19 +342,9 @@ static struct platform_driver retu_headset_driver = {
},
};
-static int __init retu_headset_init(void)
-{
- return platform_driver_probe(&retu_headset_driver, retu_headset_probe);
-}
-
-static void __exit retu_headset_exit(void)
-{
- platform_driver_unregister(&retu_headset_driver);
-}
-
-module_init(retu_headset_init);
-module_exit(retu_headset_exit);
+module_platform_driver(retu_headset_driver);
+MODULE_ALIAS("platform:retu-headset");
MODULE_DESCRIPTION("Retu/Vilma headset detection");
MODULE_LICENSE("GPL");
MODULE_AUTHOR("Juha Yrj�l�");
diff --git a/drivers/cbus/retu-pwrbutton.c b/drivers/cbus/retu-pwrbutton.c
index a5a3069..98ad005 100644
--- a/drivers/cbus/retu-pwrbutton.c
+++ b/drivers/cbus/retu-pwrbutton.c
@@ -72,7 +72,7 @@ static irqreturn_t retubutton_irq(int irq, void *_pwr)
return IRQ_HANDLED;
}
-static int __init retubutton_probe(struct platform_device *pdev)
+static int __devinit retubutton_probe(struct platform_device *pdev)
{
struct retu_pwrbutton *pwr;
int ret = 0;
@@ -127,7 +127,7 @@ err0:
return ret;
}
-static int __exit retubutton_remove(struct platform_device *pdev)
+static int __devexit retubutton_remove(struct platform_device *pdev)
{
struct retu_pwrbutton *pwr = platform_get_drvdata(pdev);
@@ -140,24 +140,16 @@ static int __exit retubutton_remove(struct
platform_device *pdev)
}
static struct platform_driver retu_pwrbutton_driver = {
- .remove = __exit_p(retubutton_remove),
+ .probe = retubutton_probe,
+ .remove = __de
[PATCH 4/6] cbus: retu: move irq_chip to our context structure
in theory, we could have many retu devices connected
to different CBUS buses. The only thing preventing
that is the poweroff() function pointer which we need
to set.
Signed-off-by: Felipe Balbi
---
drivers/cbus/retu.c | 22 +-
1 files changed, 13 insertions(+), 9 deletions(-)
diff --git a/drivers/cbus/retu.c b/drivers/cbus/retu.c
index 5bd88c6..25fa405 100644
--- a/drivers/cbus/retu.c
+++ b/drivers/cbus/retu.c
@@ -46,6 +46,8 @@ struct retu {
struct mutexmutex;
struct device *dev;
+ struct irq_chip irq_chip;
+
int irq_base;
int irq_end;
@@ -247,14 +249,6 @@ static void retu_bus_sync_unlock(struct irq_data *data)
mutex_unlock(&retu->mutex);
}
-static struct irq_chip retu_irq_chip = {
- .name = "retu",
- .irq_bus_lock = retu_bus_lock,
- .irq_bus_sync_unlock= retu_bus_sync_unlock,
- .irq_mask = retu_irq_mask,
- .irq_unmask = retu_irq_unmask,
-};
-
static inline void retu_irq_setup(int irq)
{
#ifdef CONFIG_ARM
@@ -272,7 +266,7 @@ static void retu_irq_init(struct retu *retu)
for (irq = base; irq < end; irq++) {
irq_set_chip_data(irq, retu);
- irq_set_chip(irq, &retu_irq_chip);
+ irq_set_chip(irq, &retu->irq_chip);
irq_set_nested_thread(irq, 1);
retu_irq_setup(irq);
}
@@ -409,6 +403,7 @@ static int retu_allocate_children(struct device *parent,
int irq_base)
*/
static int __devinit retu_probe(struct platform_device *pdev)
{
+ struct irq_chip *chip;
struct retu *retu;
int ret = -ENOMEM;
@@ -428,10 +423,19 @@ static int __devinit retu_probe(struct platform_device
*pdev)
goto err1;
}
+ chip = &retu->irq_chip;
+
+ chip->name = "retu",
+ chip->irq_bus_lock = retu_bus_lock,
+ chip->irq_bus_sync_unlock = retu_bus_sync_unlock,
+ chip->irq_mask = retu_irq_mask,
+ chip->irq_unmask = retu_irq_unmask,
+
retu->irq = platform_get_irq(pdev, 0);
retu->irq_base = ret;
retu->irq_end = ret + MAX_RETU_IRQ_HANDLERS;
retu->dev = &pdev->dev;
+
the_retu= retu;
mutex_init(&retu->mutex);
--
1.7.8.rc3
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/6] cbus: tahvo: move irq_chip to our context structure
in theory, we could have many tahvo devices connected
to different CBUS buses. Let's allow that to happen.
Signed-off-by: Felipe Balbi
---
drivers/cbus/tahvo.c | 21 -
1 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/drivers/cbus/tahvo.c b/drivers/cbus/tahvo.c
index 7f7c712..819111a 100644
--- a/drivers/cbus/tahvo.c
+++ b/drivers/cbus/tahvo.c
@@ -44,6 +44,8 @@ struct tahvo {
struct mutexmutex;
struct device *dev;
+ struct irq_chip irq_chip;
+
int irq_base;
int irq_end;
int irq;
@@ -199,14 +201,6 @@ static void tahvo_irq_unmask(struct irq_data *data)
tahvo->mask_pending = true;
}
-static struct irq_chip tahvo_irq_chip = {
- .name = "tahvo",
- .irq_bus_lock = tahvo_irq_bus_lock,
- .irq_bus_sync_unlock= tahvo_irq_bus_sync_unlock,
- .irq_mask = tahvo_irq_mask,
- .irq_unmask = tahvo_irq_unmask,
-};
-
static inline void tahvo_irq_setup(int irq)
{
#ifdef CONFIG_ARM
@@ -224,7 +218,7 @@ static void tahvo_irq_init(struct tahvo *tahvo)
for (irq = base; irq < end; irq++) {
irq_set_chip_data(irq, tahvo);
- irq_set_chip(irq, &tahvo_irq_chip);
+ irq_set_chip(irq, &tahvo->irq_chip);
irq_set_nested_thread(irq, 1);
tahvo_irq_setup(irq);
}
@@ -297,6 +291,7 @@ static int tahvo_allocate_children(struct device *parent,
int irq_base)
static int __devinit tahvo_probe(struct platform_device *pdev)
{
+ struct irq_chip *chip;
struct tahvo*tahvo;
int rev;
int ret;
@@ -321,6 +316,14 @@ static int __devinit tahvo_probe(struct platform_device
*pdev)
goto err1;
}
+ chip = &tahvo->irq_chip;
+
+ chip->name = "tahvo",
+ chip->irq_bus_lock = tahvo_irq_bus_lock,
+ chip->irq_bus_sync_unlock = tahvo_irq_bus_sync_unlock,
+ chip->irq_mask = tahvo_irq_mask,
+ chip->irq_unmask = tahvo_irq_unmask,
+
tahvo->irq_base = ret;
tahvo->irq_end = ret + MAX_TAHVO_IRQ_HANDLERS;
tahvo->dev = &pdev->dev;
--
1.7.8.rc3
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/6] cbus: fix very old compile warning
platform_driver.remove returns an 'int'.
Signed-off-by: Felipe Balbi
---
drivers/cbus/cbus.c |4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/drivers/cbus/cbus.c b/drivers/cbus/cbus.c
index c7ed881..45b01fd 100644
--- a/drivers/cbus/cbus.c
+++ b/drivers/cbus/cbus.c
@@ -296,7 +296,7 @@ exit1:
return ret;
}
-static void __devexit cbus_bus_remove(struct platform_device *pdev)
+static int __devexit cbus_bus_remove(struct platform_device *pdev)
{
struct cbus_host*chost = platform_get_drvdata(pdev);
@@ -305,6 +305,8 @@ static void __devexit cbus_bus_remove(struct
platform_device *pdev)
gpio_free(chost->clk_gpio);
kfree(chost);
+
+ return 0;
}
static struct platform_driver cbus_driver = {
--
1.7.8.rc3
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/PATCH 6/6] cbus: retu: implement irq_domain support
Then, when moving to devicetree, we can list
IRQs as 1, 2, 3. It's the only way to have a
sane devicetree actually.
Signed-off-by: Felipe Balbi
---
drivers/cbus/retu.c | 42 +++---
1 files changed, 27 insertions(+), 15 deletions(-)
diff --git a/drivers/cbus/retu.c b/drivers/cbus/retu.c
index 25fa405..f25e0a3 100644
--- a/drivers/cbus/retu.c
+++ b/drivers/cbus/retu.c
@@ -25,7 +25,7 @@
#include
#include
-
+#include
#include
#include
#include
@@ -46,6 +46,7 @@ struct retu {
struct mutexmutex;
struct device *dev;
+ struct irq_domain irq_domain;
struct irq_chip irq_chip;
int irq_base;
@@ -199,11 +200,9 @@ static irqreturn_t retu_irq_handler(int irq, void *_retu)
while (idr) {
unsigned long pending = __ffs(idr);
- unsigned intirq;
idr &= ~BIT(pending);
- irq = pending + retu->irq_base;
- handle_nested_irq(irq);
+ handle_nested_irq(pending);
}
return IRQ_HANDLED;
@@ -216,7 +215,7 @@ static void retu_irq_mask(struct irq_data *data)
struct retu *retu = irq_data_get_irq_chip_data(data);
int irq = data->irq;
- retu->mask |= (1 << (irq - retu->irq_base));
+ retu->mask |= (1 << irq);
retu->mask_pending = true;
}
@@ -225,7 +224,7 @@ static void retu_irq_unmask(struct irq_data *data)
struct retu *retu = irq_data_get_irq_chip_data(data);
int irq = data->irq;
- retu->mask &= ~(1 << (irq - retu->irq_base));
+ retu->mask &= ~(1 << irq);
retu->mask_pending = true;
}
@@ -260,6 +259,7 @@ static inline void retu_irq_setup(int irq)
static void retu_irq_init(struct retu *retu)
{
+ struct irq_domain *domain;
int base = retu->irq_base;
int end = retu->irq_end;
int irq;
@@ -270,10 +270,19 @@ static void retu_irq_init(struct retu *retu)
irq_set_nested_thread(irq, 1);
retu_irq_setup(irq);
}
+
+ /* IRQ domain setup */
+ domain = &retu->irq_domain;
+ domain->irq_base = base;
+ domain->nr_irq = MAX_RETU_IRQ_HANDLERS;
+ domain->hwirq_base = 1;
+
+ irq_domain_add(domain);
}
static void retu_irq_exit(struct retu *retu)
{
+ struct irq_domain *domain;
int base = retu->irq_base;
int end = retu->irq_end;
int irq;
@@ -285,6 +294,9 @@ static void retu_irq_exit(struct retu *retu)
irq_set_chip_and_handler(irq, NULL, NULL);
irq_set_chip_data(irq, NULL);
}
+
+ domain = &retu->irq_domain;
+ irq_domain_del(domain);
}
/* --
*/
@@ -326,7 +338,7 @@ static struct resource generic_resources[] = {
* @parent: parent device for this child
*/
static struct device *retu_allocate_child(char *name, struct device *parent,
- int irq_base, int irq1, int irq2, int num)
+ int irq1, int irq2, int num)
{
struct platform_device *pdev;
int status;
@@ -340,8 +352,8 @@ static struct device *retu_allocate_child(char *name,
struct device *parent,
pdev->dev.parent = parent;
if (num) {
- generic_resources[0].start = irq_base + irq1;
- generic_resources[1].start = irq_base + irq2;
+ generic_resources[0].start = irq1;
+ generic_resources[1].start = irq2;
status = platform_device_add_resources(pdev,
generic_resources, num);
@@ -368,26 +380,26 @@ err:
/**
* retu_allocate_children - Allocates Retu's children
*/
-static int retu_allocate_children(struct device *parent, int irq_base)
+static int retu_allocate_children(struct device *parent)
{
struct device *child;
- child = retu_allocate_child("retu-pwrbutton", parent, irq_base,
+ child = retu_allocate_child("retu-pwrbutton", parent,
RETU_INT_PWR, -1, 1);
if (!child)
return -ENOMEM;
- child = retu_allocate_child("retu-headset", parent, irq_base,
+ child = retu_allocate_child("retu-headset", parent,
RETU_INT_HOOK, -1, 1);
if (!child)
return -ENOMEM;
- child = retu_allocate_child("retu-rtc", parent, irq_base,
+ child = retu_allocate_child("retu-rtc", parent,
RETU_INT_RTCS, RETU_INT_RTCA, 2);
if (!child)
return -ENOMEM;
- child = retu_allocate_child("retu-wdt", parent, -1, -1, -1, 0);
+ child = retu_allocat
Re: Re: Re: [PATCH 2/5] ASoC: OMAP4: omap-dmic: Initial support for OMAP DMIC
On Wed, Nov 23, 2011 at 04:00:23PM +0200, Péter Ujfalusi wrote: > On Wednesday 23 November 2011 10:58:07 Mark Brown wrote: > > > We enable the clocks at dai_startup for the DMIC (and disable them on > > > dai_shutdown). We can not reparent while the clocks are enabled. > > > This is the reason for this part. > > That sounds like the enable is happening too early, then. > This only enables the clock for the DMIC block, the clock for the external > DMIC will start at trigger time (and stop as well). > In order to access to DMIC registers we need clocks, and the clocks are > enabled for the duration we have capture stream open. > I would think this is acceptable. Meh, I guess. It's hard to love code-wise. > > If that's what you're doing then it seems like the machine drivers > > should be use set_sysclk() (or perhaps even the clk API) to set up the > > rate they're looking for from the visible clock rather than fiddling > > about with magic divider values. That way they can say exactly what > > they want directly in terms of the result they're looking for. > In OMAP4 DMIC the divider can not be chosen freely. > The clock provided to the external microphones will depend on the selected > DMIC_FCLK, and the divider. > If I ask the machine driver to ask for specific speed for the external mic, > the writer of the machine driver anyways have to look up the table from the > TRM for the possible frequencies. So instead of providing magic divider it > has > to provide magic speed. Sure, but on the other hand it means that someone reading the machine driver can tell what's going on without going back to the magic table either. Having the rates in the code makes the code more legible and means that people can at least refer to the DMIC driver for a list of supported rates rather than having to find the TRM. I'd also guess that it's much more likely that people will remember clock rates they can set than divider tables but perhaps that's just me. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 1/3] pinctrl: add a driver for the OMAP pinmux
Op 22 nov. 2011, om 18:54 heeft Tony Lindgren het volgende geschreven:
> * Linus Walleij [22 03:30]:
>> On Tue, Nov 22, 2011 at 12:09 PM, Thomas Abraham
>> wrote:
>>> On 17 November 2011 19:27, Linus Walleij wrote:
Maybe I'm mistaken about the device tree ambitions, but
I was sort of hoping that it would not contain too much
custom magic numbers that need to be cross-referenced
elsewhere ... or rather - the more understandable the device
tree is, the more we win.
>>>
>>> Device tree is expected to describe the hardware. So to
>>> cross-reference the hardware manual to understand device tree should
>>> be fine I guess. For instance, GPIO numbers in dts would be written as
>>> a numeric number and not a enum or other format. And by looking up the
>>> manual, we understand the actual details of the GPIO pin.
>>>
>>> If dt-compiler had a option to support #define like in C, the numbers
>>> could have been made more easier to understand. Like, 3 to mean
>>> GPIO_PULL_UP in Exynos dts file.
>>
>> OK I think I get it now, so DT bindings shall really NOT be read
>> by any of the pinctrl core, it is *supposed* to be all driver-specific.
>> Then it makes perfect sense to have it as it is.
>
> Yes the driver nodes should describe in DT which pins to use:
>
>serial@1234 {
>compatible = "8250";
>reg = <0x1234 0x40>;
>reg-shift = <2>;
>interrupts = < 10 >;
> pins = "uart1_rx", "uart1_tx";
>};
>
> Note that we should use the actual signal names, not package specific
> pad names. This way they have a high likelyhood to work for new packages
> too by just mapping the signals to the new package.
How would this handle the situation where you can mux a signal to multiple
pins? IIRC omap3 and am335x can do funny stuff with the display pins like being
able to map the blue bits to different pinblocks.
regards,
Koen
signature.asc
Description: Message signed with OpenPGP using GPGMail
Re: Re: Re: Re: [PATCH 2/5] ASoC: OMAP4: omap-dmic: Initial support for OMAP DMIC
On Wednesday 23 November 2011 14:30:50 Mark Brown wrote: > Meh, I guess. It's hard to love code-wise. So you would prefer me to enable the OMAP DMIC's clocks at pcm_trigger:start time, and disable them on pcm_trigger:stop? I have seen cases when the driver did not received the pcm_trigger:stop prior to pcm_close call, so in that case a safety check in dai_shutdown is needed to be sure the dmic clocks are really disabled. I would still prefer to keep the dmic clocks enabled for the duration the stream is open. On the other hand if I had it in pcm_trigger, I don't need to play with the clocks when reparenting... > > > If that's what you're doing then it seems like the machine drivers > > > should be use set_sysclk() (or perhaps even the clk API) to set up > > > the > > > rate they're looking for from the visible clock rather than fiddling > > > about with magic divider values. That way they can say exactly what > > > they want directly in terms of the result they're looking for. > > > > In OMAP4 DMIC the divider can not be chosen freely. > > The clock provided to the external microphones will depend on the > > selected DMIC_FCLK, and the divider. > > If I ask the machine driver to ask for specific speed for the external > > mic, the writer of the machine driver anyways have to look up the table > > from the TRM for the possible frequencies. So instead of providing > > magic divider it has to provide magic speed. > > Sure, but on the other hand it means that someone reading the machine > driver can tell what's going on without going back to the magic table > either. Having the rates in the code makes the code more legible and > means that people can at least refer to the DMIC driver for a list of > supported rates rather than having to find the TRM. > > I'd also guess that it's much more likely that people will remember > clock rates they can set than divider tables but perhaps that's just me. Sure, I will change the driver accordingly. -- Péter -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: Re: Re: [PATCH 2/5] ASoC: OMAP4: omap-dmic: Initial support for OMAP DMIC
On Wed, Nov 23, 2011 at 05:24:41PM +0200, Péter Ujfalusi wrote: > On Wednesday 23 November 2011 14:30:50 Mark Brown wrote: > > Meh, I guess. It's hard to love code-wise. > So you would prefer me to enable the OMAP DMIC's clocks at pcm_trigger:start > time, and disable them on pcm_trigger:stop? I don't know that that's the best place, but it does feel like we ought to be able to do better than we are (and obviously if the clocks are required for writing to the registers that does change things a little). Perhaps there's not actually anything better given the restrictions, it's just that like I say it's hard to love the code. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] amp/remoteproc: add framework for controlling remote processors
Hi Stephen, Thanks for the review ! On Wed, Nov 23, 2011 at 5:27 AM, Stephen Boyd wrote: > How big are the images you're loading? I had to split the memcpy up into > megabyte chunks because I was running out of virtual memory to map in > two huge chunks (one for the firmware and one for the image's resting > place). This may work for now, but I think it will fail for limited > virtual memory environments. Unless CMA solves this problem? It definitely should. The images we specifically load aren't that big (~5MB) but some of the use cases we have do require around a whopping 100MB, and with CMA things just work (we do have to reserve a private CMA pool obviously, but it's a no brainer). > Also, this code assumes the firmware is actually as big as the elf > header says it is. It should be checking to make sure the elf_data is > actually big enough to copy from. Good point, thanks. > This is a bit odd. If I understand correctly, you load the firmware at > least twice. Once to see if the firmware has any virtio devices in the > resource table, and again when the image is actually loaded into RAM. Right; the firmware is loaded once at boot time, and then every time the remote processor is powered up. The former is required since we publish the remote services and their configurations (i.e. virtio devices, features and configurations) in the firmware, as a separate section (i.e. the "resource table"). We could use a separate file for this table, but we chose to couple it with the firmware in order to eliminate potential problems where the wrong resource table is loaded for a given firmware. It really makes sense since the resource table reflects the capabilities/requirements of the firmware. > Is > there any way to avoid loading it twice? I ask because we have something > like a 20Mb image that needs to be loaded and I'd like to avoid loading > it twice just to read a section out of it the first time. Sure, we could think of several approaches to optimize this, e.g. either keep the firmware loaded after boot (possibly to some limited period of time) or just allow platforms to provide their resource table as a separate file. Though frankly I'd just prefer to trust the page cache to eliminate any redundant overhead (at least when the rproc is powered up close enough to boot time), or just ignore this issue completely: since this overhead happens only once on boot, and users tend not to really power off their devices so much, i'm not sure how painful it really is ? > What do you do if dev goes away? Call rproc_unregister(), which blocks if request_firmware_nowait() didn't complete yet. > What do you think about making remoteproc into a bus? That's a really interesting idea, with some nice properties. But I'm not sure if the bus model fits our use cases: buses exclusively couple a single driver to any given hardware instance (i.e. device), and I think that a remote processor is more like an IOMMU: it's a hardware device that is being used by many drivers, and not exclusively driven by a single one. So we really end up having several (completely different) drivers that depend on this hardware block, and need some API to power it up, rather than having a single driver that manipulates it exclusively. > I imagine this function > would allocate a struct device and then rproc_register() would actually > hook it into the device layer. Then we could use the device we allocate > here for the firmware requests and we also have a nice namespace for all > remote processors in the system. Plus all the management code for > refcounting would come for free with the device layer (modulo the > rproc_boot() count). We can probably still do this even without turning remoteproc into a bus (i.e. by creating this device inside rproc_register()). I originally pondered whether to do this or not, as many other kernel frameworks do this too when their ->register() API is invoked, but I couldn't think of any strong advantages in doing so. Sounds like it does worth exploring thought, thanks for the comment. > What is RSC_VIRTIO_CFG? Sorry, that's an undocumented (and unused) resource entry that is going away. It was supposed to specify virtio device features for a given virtio device, but I'm removing it as part of the resource table overhaul I'm doing: once the resource entries will have a variable length, the virtio header will just be an inherent part of the virtio device resource entry. > For MSM I see two main pain points: > > * CMA is new and untested on MSM. I imagine in MSM's situation we would > carve out the memory chunks early on for each processor and then only > remove that memory if the processor is booted? I hope that just works > somehow. For ARM v6+, CMA just works. It's really simple and straight forward to use. > * rproc is tied to virtio fairly closely. Only for firmwares that supports virtio. If yours doesn't, you can probably just ignore the virtio parts (though I really recommend to think of
Re: [PATCH 0/7] Introducing a generic AMP framework
On Wed, Nov 23, 2011 at 12:27:31PM +0200, Ohad Ben-Cohen wrote: > On Wed, Nov 23, 2011 at 5:25 AM, Saravana Kannan > wrote: > > Sorry for the rant, this naming just rubs me the wrong way. I definitely > > appreciate the idea behind these patches. > I don't share the same naming concerns you have (if any, then > confusion with the bluetooth AMP patches and prefixes is more of a > concern to me), but I don't care deeply about names. I guess one very real potential for confusion here is the big/little stuff that ARM are pushing for next generation SoCs where a Linux image does actually run on muliple asymmetric cores. > Feel free to offer a different name, though really 'amp' here only > describes the general model and motivation and is rarely used > throughout the code; we mostly either use 'remoteproc' or 'rpmsg', > which respectively refer to the two frameworks that are being added > (the former responsible for controlling the state of the remote > processors, and the latter for communicating with them). How about using remoteproc then? -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 6/6] cbus: retu: implement irq_domain support
Hi,
On Wed, Nov 23, 2011 at 04:00:48PM +0200, Felipe Balbi wrote:
> Then, when moving to devicetree, we can list
> IRQs as 1, 2, 3. It's the only way to have a
> sane devicetree actually.
>
> Signed-off-by: Felipe Balbi
> ---
> drivers/cbus/retu.c | 42 +++---
> 1 files changed, 27 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/cbus/retu.c b/drivers/cbus/retu.c
> index 25fa405..f25e0a3 100644
> --- a/drivers/cbus/retu.c
> +++ b/drivers/cbus/retu.c
> @@ -25,7 +25,7 @@
>
> #include
> #include
> -
> +#include
> #include
> #include
> #include
> @@ -46,6 +46,7 @@ struct retu {
> struct mutexmutex;
> struct device *dev;
>
> + struct irq_domain irq_domain;
> struct irq_chip irq_chip;
>
> int irq_base;
> @@ -199,11 +200,9 @@ static irqreturn_t retu_irq_handler(int irq, void *_retu)
>
> while (idr) {
> unsigned long pending = __ffs(idr);
> - unsigned intirq;
>
> idr &= ~BIT(pending);
> - irq = pending + retu->irq_base;
> - handle_nested_irq(irq);
> + handle_nested_irq(pending);
This is particular, I'm not sure if it's correct. Not sure if we should
use hw IRQ number of linux IRQ number. A boot test should be enough to
verify though.
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH v3 5/5] clk: export tree topology and clk data via sysfs
Hi Mike, * Greg KH [22 10:51]: > On Tue, Nov 22, 2011 at 09:57:41AM -0800, Mike Turquette wrote: > > > Ah, comments like this warm my heart. > > > > > > Come on, no abusing the kobject code please, if have problems with how > > > the kernel core works, and it doesn't do things you want it to, then why > > > not change it to work properly for you, or at the least, ASK ME!!! > > > > Ok, I'm asking you now. There are two ways to solve this problem: > > > > 1) have kobject core create the lists linking the objects but defer > > allocations and any interactions with sysfs until later in the boot > > sequence, OR Please just make it all a loadable module using regular devices. That makes the development a lot easier and as a side effect also guarantees we're using standard interfaces. Sure most platforms using it want it compiled in at least for now, but in the long run it should be possible to load it when we get to initramfs. For the early usage of clocks.. > > 2) my code can create a list of clks (the same way that clkdev does) > > and defer kobject/sysfs stuff until later, which walks the list made > > during early-boot ..let's plan on getting rid of the early usage of clocks instead so you don't have the issue of deferring stuff. > > #1 is most closely aligned with the code I have here, #2 presents > > challenges that I haven't really though through. I know that OMAP > > uses the clk framework VERY early in it's boot sequence, but as long > > as the per-clk data is properly initialized then it should be OK. > > > > What do you think? We initialize clocksource/clockevent clocks early, but we can do that without clock fwk initially, then let a clockevent driver take over later on. That should solve your issues #1 and #2. > #3 - use debugfs and don't try to create a sysfs interface for the clock > structures :) Sounds like the debugfs is the way to go :) Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: HSI framework for linux-next
On Wed, Nov 09, 2011 at 08:51:40AM +1100, Stephen Rothwell wrote: > Hi Carlos, > > On Fri, 28 Oct 2011 12:27:46 +0300 Carlos Chinea > wrote: > > ... > > Right, so -rc1 has been released so I can consider this. Just a couple of > things: > > Linus (and the rest of us) is being a bit nmore paranoid about > where to fetch trees from. Public git hosters (like gitorious) can be > problematic since it is hard for us to verify the owners of trees > reliably. You either need to apply for a kernel.org account, or you will > need to add a signed tag to your tree before asking Linus to pull from > it. And that atg will have to be singned with a GPG key that can be > verified by Linus ... > > I am not sure how to translate that gitorious URI above into a > simple, publically fetchable URI (git://gitorious.org...). It has been two weeks. Did I miss any status updates? Here's the git fetchable URI: git://gitorious.org/kernel-hsi/kernel-hsi.git The existing tag is not signed though. -- Sebastian signature.asc Description: Digital signature
Re: [PATCH v5 6/7] arm: omap4: support pmu
On Mon, Oct 24, 2011 at 20:15, wrote:
> static struct platform_device* __init omap4_init_pmu(void)
> {
> int id = -1;
> @@ -420,6 +472,10 @@ static struct platform_device* __init
> omap4_init_pmu(void)
> return NULL;
> }
>
> + omap4_pmu_data.handle_irq = omap4_pmu_handler;
> + omap4_pmu_data.enable_irq = omap4_enable_cti;
> + omap4_pmu_data.disable_irq = omap4_disable_cti;
> +
> pd = omap_device_build_ss(dev_name, id, oh, 3, &omap4_pmu_data,
> sizeof(omap4_pmu_data),
> omap_pmu_latency,
> @@ -440,7 +496,9 @@ static void __init omap_init_pmu(void)
> pd = omap4_init_pmu();
> if (!pd)
> return;
> - omap_device_enable(pd);
> +
> + omap_device_enable(&od->pdev);
> + omap4_configure_pmu_irq();
This doesn't build, because there's no "od" in this function. I guess
you shouldn't be changing the omap_device_enable() call.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 5/5] clk: export tree topology and clk data via sysfs
On Wed, Nov 23, 2011 at 08:59:04AM -0800, Tony Lindgren wrote: > ..let's plan on getting rid of the early usage of clocks instead so > you don't have the issue of deferring stuff. No - we have too many platforms already using them early to do a change like this - and to do such a change will force them to invent some other way. That's just stupid. Keep the clk API as a fundamental thing which should be initialized early so we don't have to invent new clk APIs to get around its unavailability. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/4] regulator: map consumer regulator based on device tree
On Fri, Nov 18, 2011 at 04:47:20PM +0530, Rajendra Nayak wrote: > + struct device_node *regnode = NULL; > + char prop_name[32]; /* 32 is max size of property name */ There ought to be a #define for that though I can't see one right now - this can't be the only place where we need to do stuff like this. > + > + dev_dbg(dev, "Looking up %s-supply from device tree\n", supply); > + > + snprintf(prop_name, 32, "%s-supply", supply); sizeof(). -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 5/5] clk: export tree topology and clk data via sysfs
* Russell King - ARM Linux [23 09:31]: > On Wed, Nov 23, 2011 at 08:59:04AM -0800, Tony Lindgren wrote: > > ..let's plan on getting rid of the early usage of clocks instead so > > you don't have the issue of deferring stuff. > > No - we have too many platforms already using them early to do a change > like this - and to do such a change will force them to invent some other > way. That's just stupid. Not necessarily for all the systems. For omap2+ we should be able to boot the system initially with the perf counter as the clockevent and 32KiHz always running timer. Then a dmtimer using proper clockevent driver could take over. > Keep the clk API as a fundamental thing which should be initialized early > so we don't have to invent new clk APIs to get around its unavailability. What else are you aware of that is really needed early for clocks other than clockevent? We've already proven that SRAM init can be moved to happen later. The optional debug serial port is already enabled by the bootloader. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 0/4] Device tree support for regulators
On Fri, Nov 18, 2011 at 04:47:16PM +0530, Rajendra Nayak wrote: > For the first 2 patches (1/4 and 2/4) I have dropped > Acks from Mark, since they have changed to some extent > from the last post and retained the Acks recieved on the > last 2 patches (3/4 and 4/4) as they remain unchanged. Looks good and nobody complained yet so I've dropped this into -next to get it a bit of exposure. I'm reasonably happy that the DT people have already had a good chance to look at this stuff on previous iterations. Unless there's any major changes or we need to back it out later on for some other reason please send incremental patches. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 5/5] clk: export tree topology and clk data via sysfs
On Wed, Nov 23, 2011 at 10:55:19AM -0800, Tony Lindgren wrote: > * Russell King - ARM Linux [23 09:31]: > > Keep the clk API as a fundamental thing which should be initialized early > > so we don't have to invent new clk APIs to get around its unavailability. > What else are you aware of that is really needed early for clocks other > than clockevent? If nothing else we'd have to resolve all the device probe ordering issues (Grant's patches seem a bit stalled) and convert all the systems using the API to handle deferring probes until their clocks appear. Using an early initcall of some kind would help with that but it does seem like a lot of trouble and I'd expect it'll end up getting forced in on most systems anyway. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 5/5] clk: export tree topology and clk data via sysfs
* Mark Brown [23 10:34]: > On Wed, Nov 23, 2011 at 10:55:19AM -0800, Tony Lindgren wrote: > > * Russell King - ARM Linux [23 09:31]: > > > > Keep the clk API as a fundamental thing which should be initialized early > > > so we don't have to invent new clk APIs to get around its unavailability. > > > What else are you aware of that is really needed early for clocks other > > than clockevent? > > If nothing else we'd have to resolve all the device probe ordering > issues (Grant's patches seem a bit stalled) and convert all the systems > using the API to handle deferring probes until their clocks appear. > Using an early initcall of some kind would help with that but it does > seem like a lot of trouble and I'd expect it'll end up getting forced in > on most systems anyway. Yes the ordering depends on the system. In any case, initcalls are not super early compared to setup_timer. Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/6] OMAP: PM: register to the per-device PM QoS framework
Jean Pihet writes:
> On Thu, Nov 17, 2011 at 10:24 PM, Kevin Hilman wrote:
>> jean.pi...@newoldbits.com writes:
>>
>>> From: Jean Pihet
>>>
>>> Implement the devices wake-up latency constraints using the global
>>> device PM QoS notification handler which applies the constraints to the
>>> underlying layer by calling the corresponding function at hwmod level.
>>>
>>> Tested on OMAP3 Beagleboard and OMAP4 Pandaboard in RET/OFF using wake-up
>>> latency constraints on MPU, CORE and PER.
>>>
>>> Signed-off-by: Jean Pihet
> ...
>
>>> +/* Interface to the per-device PM QoS framework */
>>> +static int omap2_dev_pm_qos_handler(struct notifier_block *nb,
>>> + unsigned long new_value,
>>> + void *req)
>>> +{
>>> + struct omap_device *od;
>>> + struct omap_hwmod *oh;
>>> + struct platform_device *pdev;
>>> + struct dev_pm_qos_request *dev_pm_qos_req = req;
>>> +
>>> + pr_debug("OMAP PM CONSTRAINTS: req@0x%p, new_value=%lu\n",
>>
>> s/CONSTRAINTS/constraints/
>> another one below.
> Ok
>
>>
>>> + req, new_value);
>>> +
>>> + /* Look for the platform device for the constraint target device */
>>> + pdev = to_platform_device(dev_pm_qos_req->dev);
>>> +
>>> + /* Try to catch non platform devices */
>>
>> why?
> The constraints targets are the power domains, which you find by
> walking through the chain of structs dev, pdev, omap_device, hwmod and
> finally pwrdm.
OK
>>
>>> + if (pdev->name == NULL) {
>>> + pr_err("%s: Error: platform device for device %s not valid\n",
>>> + __func__, dev_name(dev_pm_qos_req->dev));
>>> + return -EINVAL;
>>> + }
>>> +
>>> + /* Find the associated omap_device for dev */
>>> + od = container_of(pdev, struct omap_device, pdev);
>>
>> What about devices that are valid platform_devices, but not omap_devices?
> Do you mean that od should be tested for NULL value?
First, it should be using the to_omap_device() helper function from
omap_device.h.
Until v3.2, it was based on container_of() so checking for NULL will not
help. However, as of v3.2, because I decoupled platform_device and
omap_device, checking for a NULL return from to_omap_device() will be a
good enough check.
Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-pm] [PATCH 4/6] OMAP3: cpuidle: next C-state decision depends on the PM QoS MPU and CORE constraints
Jean Pihet writes: > On Thu, Nov 17, 2011 at 10:29 PM, Kevin Hilman wrote: >> jean.pi...@newoldbits.com writes: >> >>> From: Jean Pihet >>> >>> The MPU latency figures for cpuidle include the MPU itself and also >>> the peripherals needed for the MPU to execute instructions (e.g. >>> main memory, caches, IRQ controller, MMU etc). On OMAP3 those >>> peripherals belong to the MPU and CORE power domains and so the >>> cpuidle C-states are a combination of MPU and CORE states. >>> >>> This patch implements the relation between the cpuidle and per- >>> device PM QoS frameworks in the OMAP3 specific idle callbacks. >>> >>> The chosen C-state shall satisfy the following conditions: >>> . the 'valid' field is enabled, >>> . it satisfies the enable_off_mode flag, >> >> Not directly related to this patch, but is there any reason to keep the >> 'enable_off_mode' flag after this series? > enable_off_mode could be removed completely after this series unless > there is a need to prevent OFF mode for debug reasons. Great. For debug reasons, we can just as easily set constraints to prevent off mode, so I would like to see it disappear. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 24/24] gpio/omap: handle set_dataout reg capable IP on restore
"DebBarma, Tarun Kanti" writes: > On Mon, Nov 7, 2011 at 5:35 PM, DebBarma, Tarun Kanti > wrote: >> On Fri, Nov 4, 2011 at 10:23 PM, Kevin Hilman wrote: >>> Tarun Kanti DebBarma writes: >>> From: Nishanth Menon GPIO IP revisions such as those used in OMAP4 have a set_dataout while the previous revisions used a single dataout register. Depending on what is available restore the dataout settings to the right register. Signed-off-by: Nishanth Menon Signed-off-by: Tarun Kanti DebBarma Reviewed-by: Santosh Shilimkar --- drivers/gpio/gpio-omap.c | 9 +++-- 1 files changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c index 4009446..3df7a98 100644 --- a/drivers/gpio/gpio-omap.c +++ b/drivers/gpio/gpio-omap.c @@ -1073,7 +1073,7 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev) bank->get_context_loss_count = pdata->get_context_loss_count; bank->regs = pdata->regs; - if (bank->regs->set_dataout && bank->regs->clr_dataout) + if (bank->regs->set_dataout) >>> >>> This change isn't right. >>> >>> The _set_gpio_dataout_reg function depends on the existence of >>> ->clr_dataout too. >> Ok, I will add the clr_dataout condtion as well. > >> >>> bank->set_dataout = _set_gpio_dataout_reg; else bank->set_dataout = _set_gpio_dataout_mask; @@ -1351,7 +1351,12 @@ static void omap_gpio_restore_context(struct gpio_bank *bank) bank->base + bank->regs->risingdetect); __raw_writel(bank->context.fallingdetect, bank->base + bank->regs->fallingdetect); - __raw_writel(bank->context.dataout, bank->base + bank->regs->dataout); + if (bank->regs->set_dataout) >>> >>> Why the check again? The check has already been done in probe. >>> >>> Just use bank->set_dataout() here. >> Sure, i will make the change. > > When I look at the signature of set_dataout(), it does not seem > straight forward to be used here. It expects (struct gpio_bank *bank, > int gpio, int enable) to be passed to it. IOW, it expects to only set 1 bit, where the context restore needs to set the value for the whole register. OK, then keep the original version, but make sure the if statement matches is checking for ->set_dataout and ->clr_dataout like the other one. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RESEND #3 1/7] OMAP3+: PM: SR: add suspend/resume handlers
"Menon, Nishanth" writes: > On Wed, Nov 16, 2011 at 18:02, Kevin Hilman wrote: >> Felipe Balbi writes: >> >>> From: Nishanth Menon >>> >>> SmartReflex should be disabled while entering low power mode due to >>> the following reasons: >> [...] >> >> Nishanth, in the end, didn't you decide to drop this patch? >> > > Yes, I did eventually, once we implemented DVFS for GPU, Ducati, HSI, > and other drivers, there was no real way to ensure sequence of suspend > sequencing even after moving this to suspend_noirq. some of the other > reasons: OK, thanks for the clarification. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] Introducing a generic AMP framework
On 11/23/2011 08:10 AM, Mark Brown wrote: On Wed, Nov 23, 2011 at 12:27:31PM +0200, Ohad Ben-Cohen wrote: On Wed, Nov 23, 2011 at 5:25 AM, Saravana Kannan wrote: Sorry for the rant, this naming just rubs me the wrong way. I definitely appreciate the idea behind these patches. I don't share the same naming concerns you have (if any, then confusion with the bluetooth AMP patches and prefixes is more of a concern to me), but I don't care deeply about names. I guess one very real potential for confusion here is the big/little stuff that ARM are pushing for next generation SoCs where a Linux image does actually run on muliple asymmetric cores. Feel free to offer a different name, though really 'amp' here only describes the general model and motivation and is rarely used throughout the code; we mostly either use 'remoteproc' or 'rpmsg', which respectively refer to the two frameworks that are being added (the former responsible for controlling the state of the remote processors, and the latter for communicating with them). How about using remoteproc then? remoteproc, peripheral proc, device proc, firmware proc (kinda lines up with request_firmware naming). Just throwing out names. Any one of these are okay with me. remoteproc would probably be the best fit since it's already used in the code and people are used to discussing about it. Thanks, Saravana -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/5] Documentation: common clk API
On Tue, Nov 22, 2011 at 6:03 PM, Saravana Kannan wrote:
> On 11/21/2011 05:40 PM, Mike Turquette wrote:
>> +Below is the common struct clk definition from include/linux.clk.h. It
>
> Typo
Will fix in V4.
>
>> +is modified slightly for brevity:
>> +
>> +struct clk {
>> + const char *name;
>> + const struct clk_hw_ops *ops;
>
> No strong opinion, but can we call this clk_ops for brevity?
I prefer clk_hw_ops, as it clearly delineates that these operations
know hardware-specific details.
>> +clk_prepare - does everything needed to get a clock ready to generate a
>> +proper signal which may include ungating the clk and actually generating
>> +that signal. clk_prepare MUST be called before clk_enable. This call
>> +holds the global prepare_mutex, which also prevents clk rates and
>> +topology from changing while held. This API is meant to be the "slow"
>> +part of a clk enable sequence, if applicable. This function must not be
>> +called in an interrupt context.
>
> in an atomic context?
Will fix in V4.
>> +clk_get_rate - Returns the cached rate for the clk. Does NOT query the
>> +hardware state. No lock is held.
>
> I wrote the stuff below and then realized that this ops is not really present
> in the code. Looks like stale doc. Can you please remove this? But I think
> the comments below do hold true for the actual clk_set_rate()/get_rate()
> implementation. I will try to repeat this as part of the actual code review.
Firstly this is a summary of the clk API in clk.h, not clk_hw_ops.
There isn't a hardware op for this since we just return clk->parent.
Secondly it is up to the caller to hold a lock. Any code calling
clk_get_rate might likely want to hold that lock anyways. I'll update
the comments to be explicit about this.
>
> I will be looking into the other patches in order, so, forgive me if I'm
> asking a question that has an obvious answer in the remaining patches.
>
> I think a lock needs to be taken for this call too. What prevents a clock set
> rate from getting called and modifying the cached rate variable while it's
> bring read. I don't think we should have a common code assume that read/write
> of longs are atomic.
>
>> +
>> +clk_get_parent - Returns the cached parent for the clk. Does NOT query
>> +the hardware state. No lock is held.
>
> Same question as above. Can we assume a read of a pointer variable is atomic?
> I'm not sure. I think this needs locking too.
Same answer as above. The caller must hold the lock. I'll update the comments.
>
>> +
>> +clk_set_rate - Attempts to change the clk rate to the one specified.
>> +Depending on a variety of common flags it may fail to maintain system
>> +stability or result in a variety of other clk rates changing.
>
> I'm not sure if this is intentional or if it's a mistake in phrasing it.
> IMHO, the rates of other clocks that are actually made available to device
> drivers should not be changed. This call might trigger rate changes inside
> the tree to accommodate the request from various children, but should not
> affect the rate of the leaf nodes.
>
> Can you please clarify the intention and/or the wording?
Setting the flag CLK_GATE_SET_RATE tells clk_set_rate not to change
the rate if the clk is enabled. This policy is not enforced
abritrarily: you don't have to set the flag on your clk. I'll update
the doc to make explicit mention of this flag.
>> +clk_set_rate deserves a special mention because it is more complex than
>> +the other operations. There are three key concepts to the common
>> +clk_set_rate implementation:
>> +
>> +1) recursively traversing up the clk tree and changing clk rates, one
>> +parent at a time, if each clk allows it
>> +2) failing to change rate if the clk is enabled and must only change
>> +rates while disabled
>
> Is this really true? Based on a quick glance at the other patches, it doesn't
> look it disallows set rate if the clock is enabled. Which is correct, because
> clock rates can be change while they are enabled (I'm referring to leaf
> clocks) as long as the hardware supports doing it correctly. So, this needs
> rewording? I'm guessing you are trying to say that you can't change the
> parent rate if it has multiple enabled child clocks running off of it and one
> of them wants to cause a parent rate change? I'm not sure if even that should
> be enforced in the common code -- doesn't look like you do either.
Same as my answer above. There is a flag which allows you to control
this behavior.
>
>> +2) using clk rate change notifiers to allow devices to handle dynamic
>
> Must be 3)
Haha good catch.
>>
>> +rate changes for clks which do support changing rates while enabled
>
> Again, I guess this applies to the other clock. Not the one for which
> clk_set_rate() is being called.
This applies to ANY clk which has the flag set and is called by
__clk_set_rate (which may be called many times in a recursive path).
>> +clk_set_rate(C, 26MHz);
>>
Re: [PATCH v3 5/5] clk: export tree topology and clk data via sysfs
On Tue, Nov 22, 2011 at 7:48 PM, Saravana Kannan wrote: > On 11/22/2011 11:13 AM, Greg KH wrote: >> >> On Tue, Nov 22, 2011 at 09:57:41AM -0800, Mike Turquette wrote: Ah, comments like this warm my heart. Come on, no abusing the kobject code please, if have problems with how the kernel core works, and it doesn't do things you want it to, then why not change it to work properly for you, or at the least, ASK ME!!! >>> >>> Ok, I'm asking you now. There are two ways to solve this problem: >>> >>> 1) have kobject core create the lists linking the objects but defer >>> allocations and any interactions with sysfs until later in the boot >>> sequence, OR >>> >>> 2) my code can create a list of clks (the same way that clkdev does) >>> and defer kobject/sysfs stuff until later, which walks the list made >>> during early-boot >>> >>> #1 is most closely aligned with the code I have here, #2 presents >>> challenges that I haven't really though through. I know that OMAP >>> uses the clk framework VERY early in it's boot sequence, but as long >>> as the per-clk data is properly initialized then it should be OK. >>> >>> What do you think? >> >> #3 - use debugfs and don't try to create a sysfs interface for the clock >> structures :) > > I would prefer debugfs too, but for my own selfish reasons. In our current I'll cook up debugfs code for the fwk and drop sysfs. Maybe not part of V4 (per Arnd's suggestion to focus on upstreamable bits), or maybe I will if I have time. Regards, Mike -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] omap fixes for v3.2-rc2
On Saturday 19 November 2011, Tony Lindgren wrote: > Please pull omap fixes from: > > git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap fixes > > Most of this pull request are fixes needed by Tomi for the > display driver clocking. > Hi Tony, Sorry for the delay on my side, I've only today looked at this series. The patches all look ok, as far as I can tell, but please rebase the series to make it easier to see what is going on: * At least one of the sub-branches is based on a random commit on Linus' tree. Please always base patches on an -rc for consistency! * Out of the 20 patches, not one is marked for 'stable' backports. I really want to make sure this time that all long-standing bugs get fixed in stable releases, so please go through the list and add 'Cc: sta...@kernel.org' to the ones you want to have backported. If all patches are actually addressing regressions, just tell me in the introductory mail next time. * Since a lot of patches address the dss, it would be nice to have a separate pull request for those. It's not really important, but I feel that it's easier to review stuff that is less mixed and splitting them out should be easy if you rebase the series anyway. Thanks for your patience, Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/5] clk: introduce the common clock framework
On Tue, Nov 22, 2011 at 7:12 PM, Saravana Kannan wrote:
> On 11/21/2011 05:40 PM, Mike Turquette wrote:
>> +void __clk_unprepare(struct clk *clk)
>> +{
>> + if (!clk)
>> + return;
>> +
>> + if (WARN_ON(clk->prepare_count == 0))
>> + return;
>> +
>> + if (--clk->prepare_count> 0)
>
> Space before ">"?
Will fix all spacing errors in v4.
>> +int __clk_enable(struct clk *clk)
>> +{
>> + int ret = 0;
>> +
>> + if (!clk)
>> + return 0;
>> +
>> + if (WARN_ON(clk->prepare_count == 0))
>> + return -ESHUTDOWN;
>
> EPERM? EBADFD?
This came from discussion out of Jeremy's original patches and I'm
inclined to keep it there.
>> +unsigned long clk_get_rate(struct clk *clk)
>> +{
>> + if (!clk)
>> + return 0;
>> +
>> + return clk->rate;
>
> I think you need to grab the prepare_mutex here too. Otherwise another call
> to clk_set_rate() could be changing clk->rate while it's being read here. It
> shouldn't be a problem in ARM where I think 32bit reads are atomic. But I'm
> not sure you can say the same for all archs.
Hmm, need to decide if code calling this will likely hold the mutex
itself. The comment can be updated to say "caller must hold
prepare_mutex", but that may be undo burden for a driver that just
wants to know a clk rate. Thoughts?
>> +/**
>> + * clk_recalc_rates
>> + * @clk: first clk in the subtree
>> + *
>> + * Walks the subtree of clks starting with clk and recalculates rates as
>> it
>> + * goes. Note that if a clk does not implement the recalc_rate operation
>> then
>> + * propagation of that subtree stops and all of that clks children will
>> not
>> + * have their rates updated. Caller must hold prepare_lock.
>
> May be call this functions __clk_recalc_rates() to match the naming
> convention of the other fns that don't grab locks?
Other functions have __private_func syntax for one of two reasons:
1) the outer function holds a lock, and sometimes we want to access
the inner function from some other code path that avoids nested
locking (e.g. clk_enable)
2) the outer function sets up some staging data for a recursive
mechanism to use (e.g. clk_set_rate)
clk_recalc_rates doesn't hold a lock nor does it have staging data, so
it would just end up looking like:
__clk_recalc_rates(struct clk *clk)
{
do the real work
}
clk_recalc_rates(struct clk *clk)
{
__clk_recalc_rates(clk)
}
There is no obvious gain for splitting the function.
>
>> + */
>> +static void clk_recalc_rates(struct clk *clk)
>> +{
>> + unsigned long old_rate;
>> + struct hlist_node *tmp;
>> + struct clk *child;
>> +
>> + old_rate = clk->rate;
>> +
>> + if (clk->ops->recalc_rate)
>> + clk->rate = clk->ops->recalc_rate(clk);
>
> Any reason you can't just do "else return" instead of the check below? That
> would be more straight-forward and also remove the need for old_rate.
old_rate doesn't protect against not having a .recalc_rate pointer.
It is an optimization for when a clks rate hasn't changed and there is
no reason to traverse the tree. In the case where there is no
.recalc_rate pointer then it serves the dual-purpose of bailing out
early.
>> +struct clk *__clk_set_rate(struct clk *clk, unsigned long rate)
>> +{
>> + struct clk *fail_clk = NULL;
>> + int ret;
>> + unsigned long old_rate = clk->rate;
>> + unsigned long new_rate;
>> + unsigned long parent_old_rate;
>> + unsigned long parent_new_rate = 0;
>> + struct clk *child;
>> + struct hlist_node *tmp;
>> +
>> + /* bail early if we can't change rate while clk is enabled */
>> + if ((clk->flags& CLK_GATE_SET_RATE)&& clk->enable_count)
>> + return clk;
>
> Spacing fix near & and &&.
Will fix in V4.
>
>> +
>> + /* find the new rate and see if parent rate should change too */
>> + WARN_ON(!clk->ops->round_rate);
>
> It looks like you don't consider absence of round_rate as an error (going by
> clk_round_rate()), so why warn on this? See below for additional comments.
>
>> + new_rate = clk->ops->round_rate(clk, rate,&parent_new_rate);
The above will cause a NULL ptr deref if you don't have a .round_rate
defined, so I'd say having .round_rate isn't very optional for
clk_set_rate support :-)
The question is, should it be? For very simple clocking schemes where
the PLL locking rates will never vary from board to board or the
oscillators won't get changed across products... I guess it's not
necessary. I can conditionally check for it before calling unless
others feel that .round_rate should be mandatory for .set_rate. Need
feedback on that.
>
> Should we split the "figuring out the parent rate" and "round rate"?
No. These two are inherently linked. If you set them independently
you will have some crazy code trying to make sure that the clk's rate
and the parent's rate that are being programmed make sense. Best to
co
Re: [GIT PULL] omap fixes for v3.2-rc2
* Arnd Bergmann [23 12:40]: > On Saturday 19 November 2011, Tony Lindgren wrote: > > Please pull omap fixes from: > > > > git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap fixes > > > > Most of this pull request are fixes needed by Tomi for the > > display driver clocking. > > > > Hi Tony, > > Sorry for the delay on my side, I've only today looked at this series. > The patches all look ok, as far as I can tell, but please rebase > the series to make it easier to see what is going on: > > * At least one of the sub-branches is based on a random commit on > Linus' tree. Please always base patches on an -rc for consistency! Yes sorry I made the same comment earlier to Benoit. That one patch is certainly based on a random commit.. Will cherry pick that one. The earlier patches are based on the earlier fixes (while waiting for them to get merged). So that's certainly not a random commit. Or at least was not at that time :) I can rebase those too anyways now that the earlier fixes are merged. > * Out of the 20 patches, not one is marked for 'stable' backports. > I really want to make sure this time that all long-standing bugs > get fixed in stable releases, so please go through the list and > add 'Cc: sta...@kernel.org' to the ones you want to have backported. > If all patches are actually addressing regressions, just tell me > in the introductory mail next time. It's mostly regressions and fixes on new features merged during the merge window. But looks like there's at least one patch that might make sense for stable too, will check them all. > * Since a lot of patches address the dss, it would be nice to have > a separate pull request for those. It's not really important, but > I feel that it's easier to review stuff that is less mixed and > splitting them out should be easy if you rebase the series anyway. OK will place those into fixes-dss branch. Cheers, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: HSI framework for linux-next
Hi Carlos, On Fri, 28 Oct 2011 12:27:46 +0300 Carlos Chinea wrote: > > I have been working in an HSI framework for linux: > > https://lkml.org/lkml/2011/6/10/280 > > The framework is in good shape and is currently being used for some > people so we would like it to see it integrated, if possible, for 3.3 > > Latest discussion for integration: > https://lkml.org/lkml/2011/10/20/177 > > Therefore, could you add the following tree to linux-next ? > g...@gitorious.org:kernel-hsi/kernel-hsi.git for-next I have added that tree to linux-next today using git://gitorious.org/kernel-hsi/kernel-hsi.git as the URL and just you as the contact in case if issues. Thanks for adding your subsystem tree as a participant of linux-next. As you may know, this is not a judgment of your code. The purpose of linux-next is for integration testing and to lower the impact of conflicts between subsystems in the next merge window. You will need to ensure that the patches/commits in your tree/series have been: * submitted under GPL v2 (or later) and include the Contributor's Signed-off-by, * posted to the relevant mailing list, * reviewed by you (or another maintainer of your subsystem tree), * successfully unit tested, and * destined for the current or next Linux merge window. Basically, this should be just what you would send to Linus (or ask him to fetch). It is allowed to be rebased if you deem it necessary. -- Cheers, Stephen Rothwell s...@canb.auug.org.au Legal Stuff: By participating in linux-next, your subsystem tree contributions are public and will be included in the linux-next trees. You may be sent e-mail messages indicating errors or other issues when the patches/commits from your subsystem tree are merged and tested in linux-next. These messages may also be cross-posted to the linux-next mailing list, the linux-kernel mailing list, etc. The linux-next tree project and IBM (my employer) make no warranties regarding the linux-next project, the testing procedures, the results, the e-mails, etc. If you don't agree to these ground rules, let me know and I'll remove your tree from participation in linux-next. pgpdkY8BeFUf4.pgp Description: PGP signature
Re: [GIT PULL] omap fixes for v3.2-rc2
On Wednesday 23 November 2011 14:03:58 Tony Lindgren wrote: > > The earlier patches are based on the earlier fixes (while waiting > for them to get merged). So that's certainly not a random commit. > Or at least was not at that time I can rebase those too anyways > now that the earlier fixes are merged. No need to do that unless you are rebasing them anyway. IMHO it's fine if you have all your bug fixes in one branch based on the previous bug fixes you sent, but it's of course also fine to start a fresh branch for each pull request. In general, I would recommend not rebasing when you have the choice, because that means your patches are not as well tested. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 5/5] clk: export tree topology and clk data via sysfs
On Wed, Nov 23, 2011 at 10:55:19AM -0800, Tony Lindgren wrote: > What else are you aware of that is really needed early for clocks other > than clockevent? TWD will lose its auto-calibration. Then there's various clock source and clock event implementations. These all call for the clk API to be up and running at init_early time. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] omap fixes for v3.2-rc2
* Arnd Bergmann [23 13:47]: > On Wednesday 23 November 2011 14:03:58 Tony Lindgren wrote: > > > > The earlier patches are based on the earlier fixes (while waiting > > for them to get merged). So that's certainly not a random commit. > > Or at least was not at that time I can rebase those too anyways > > now that the earlier fixes are merged. > > No need to do that unless you are rebasing them anyway. IMHO > it's fine if you have all your bug fixes in one branch based > on the previous bug fixes you sent, but it's of course also > fine to start a fresh branch for each pull request. > > In general, I would recommend not rebasing when you have the > choice, because that means your patches are not as well tested. Well I can keep only four of them because of the pull on a random commit. Looks like four of them apply to v3.1, will send them off to sta...@vger.kernel.org and send you two new pull requests. Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 25/26] OMAP: PM: convert the SmartReflex code into the AVS driver framework
...
> +int sr_configure_errgen(struct smartreflex *sr)
> +{
> + struct smartreflex_platform_data *pdata = sr->pdev->dev.platform_data;
> + u32 sr_config, sr_errconfig;
> +
> + if (IS_ERR_OR_NULL(sr))
> + return -EINVAL;
> +
> + if (!sr_calculate_clk_length(sr))
> + return -EINVAL;
> +
> + sr_config = sr->proto_sr_config;
> + sr_config |= SRCONFIG_ERRGEN_EN;
> + sr_write_reg(sr, SRCONFIG, sr_config);
> +
> + sr_errconfig = (pdata->err_weight << ERRCONFIG_ERRWEIGHT_SHIFT) |
> + (pdata->err_maxlimit << ERRCONFIG_ERRMAXLIMIT_SHIFT) |
> + (sr->err_minlimit << ERRCONFIG_ERRMINLIMIT_SHIFT);
> + sr_modify_reg(sr, sr->errconfig_offs, (SR_ERRWEIGHT_MASK |
> + SR_ERRMAXLIMIT_MASK | SR_ERRMINLIMIT_MASK),
> + sr_errconfig);
> +
> + /* Enabling the interrupts if the ERROR module is used */
> + sr_modify_reg(sr, sr->errconfig_offs,
> + sr->vpboundint_en, (sr->vpboundint_en | sr->vpboundint_st));
Nishanth Menon has a patch for the reversed order of parameters for
mask and bits to set in the above.
> +
> + return 0;
> +}
...
> +int __init sr_common_probe(struct platform_device *pdev,
> +struct smartreflex *sr)
> +{
> + struct smartreflex_platform_data *pdata = pdev->dev.platform_data;
> + struct resource *mem, *irq;
> + int ret = 0;
> +
> + sr->name = kasprintf(GFP_KERNEL, "sr_%s", pdata->name);
> + if (!sr->name) {
> + dev_err(&pdev->dev, "%s: Unable to alloc debugfs name\n",
> + __func__);
> + return -ENOMEM;
> + }
> +
> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!mem) {
> + dev_err(&pdev->dev, "%s: no mem resource\n", __func__);
> + return -ENODEV;
Need free(sr->name) ?
> + }
> +
> + mem = request_mem_region(mem->start, resource_size(mem),
> + dev_name(&pdev->dev));
> + if (!mem) {
> + dev_err(&pdev->dev, "%s: no mem region\n", __func__);
> + return -EBUSY;
> + }
> +
> + sr->base = ioremap(mem->start, resource_size(mem));
> + if (!sr->base) {
> + dev_err(&pdev->dev, "%s: ioremap fail\n", __func__);
> + ret = -ENOMEM;
> + goto err_release_region;
> + }
> +
> + irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> + if (!irq) {
> + dev_err(&pdev->dev, "%s: no IRQ resource defined\n", __func__);
> + ret = -ENODEV;
> + goto err_iounmap;
> + }
> + sr->irq = irq->start;
> +
> + ret = request_irq(sr->irq, sr->isr, 0, sr->name, (void *)sr);
> + if (ret) {
> + dev_err(&pdev->dev, "%s: could not register interrupt "
> + "handler\n", __func__);
> + goto err_iounmap;
> + }
> +
> + pdata->sr = sr;
> +
> + sr->pdev = pdev;
> + sr->voltdm = pdata->voltdm;
> +
> + sr->autocomp_active = false;
> +
> + pm_runtime_enable(&pdev->dev);
> + pm_runtime_irq_safe(&pdev->dev);
> +
> +#ifdef CONFIG_POWER_AVS_DEBUG
> + ret = sr_debugfs_setup(pdev, sr);
> + if (ret)
> + goto err_free_irq;
> +#endif
> +
> + dev_info(&pdev->dev, "%s: SmartReflex driver initialized\n", __func__);
> +
> + return ret;
> +
> +err_free_irq:
> + free_irq(sr->irq, (void *)sr);
> +err_iounmap:
> + iounmap(sr->base);
> +err_release_region:
> + release_mem_region(mem->start, resource_size(mem));
> +
> + return ret;
> +}
> +
> +int __devexit sr_remove(struct platform_device *pdev)
> +{
> + struct smartreflex_platform_data *pdata = pdev->dev.platform_data;
> + struct smartreflex *sr;
> + struct resource *mem;
> +
> + if (!pdata) {
> + dev_err(&pdev->dev, "%s: platform data missing\n", __func__);
> + return -EINVAL;
> + }
> +
> + sr = pdata->sr;
> + if (IS_ERR(sr)) {
Sometimes set to NULL, use IS_ERR_OR_NULL ?
> + dev_warn(&pdev->dev, "%s: smartreflex struct not found\n",
> + __func__);
> + return -EINVAL;
> + }
> +
> + if (sr->autocomp_active)
> + sr_stop_vddautocomp(sr);
> +#ifdef CONFIG_POWER_AVS_DEBUG
> + if (sr->dbg_dir)
> + debugfs_remove_recursive(sr->dbg_dir);
> +#endif
> + free_irq(sr->irq, (void *)sr);
> + iounmap(sr->base);
Need free(sr->name) ?
> + kfree(sr);
> + pdata->sr = NULL;
> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + release_mem_region(mem->start, resource_size(mem));
> +
> + return 0;
> +}
...
> +static irqreturn_t _interrupt(int irq, void *data)
> +{
> + struct smartreflex *sr = (struct smartreflex *)data;
> + u32 status = 0;
> +
> + /* Read the status bits */
> + sr_read_reg(sr, IRQSTATUS);
> +
> + /* Clear them by writing back */
> + sr_write_reg(sr, IRQSTATUS, status
[GIT PULL] omap fixes for v3.2-rc2, take 2
Hi Arnd, This one has the DSS patches left out, some patches sent to stable, and based on -rc2. Regards, Tony The following changes since commit cfcfc9eca2bcbd26a8e206baeb005b055dbf8e37: Linus Torvalds (1): Linux 3.2-rc2 are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap fixes Felipe Balbi (1): ARM: OMAP: smartreflex: fix IRQ handling bug Govindraj.R (1): ARM: OMAP2+: Fix Compilation error when omap_l3_noc built as module Kevin Hilman (2): ARM: OMAP3: CPUidle: include ARM: OMAP: PM: only register TWL with voltage layer when device is present Ming Lei (1): ARM: OMAP2: select ARM_AMBA if OMAP3_EMU is defined Thomas Weber (1): ARM: OMAP2+: Remove empty io.h Tony Lindgren (4): ARM: OMAP: Fix map_io for Amstrad E3 ARM: OMAP: Fix dpll_data compile error when omap2 only is selected ARM: OMAP: Fix reprogramming of dpll1 rate Merge branch 'fixes-v3.2-rc2' into fixes sricharan (1): ARM: OMAP: hwmod: Fix the addr space, irq, dma count APIs arch/arm/configs/omap1_defconfig|1 - arch/arm/mach-omap1/Kconfig |8 - arch/arm/mach-omap1/board-ams-delta.c | 10 -- arch/arm/mach-omap1/clock.h |3 +- arch/arm/mach-omap1/clock_data.c| 53 --- arch/arm/mach-omap1/devices.c |3 ++ arch/arm/mach-omap2/Kconfig |1 + arch/arm/mach-omap2/cpuidle34xx.c |1 + arch/arm/mach-omap2/omap_hwmod.c|6 ++-- arch/arm/mach-omap2/omap_l3_noc.c |2 +- arch/arm/mach-omap2/pm.c|6 +-- arch/arm/mach-omap2/smartreflex.c |2 +- arch/arm/mach-omap2/twl-common.c| 11 ++ arch/arm/mach-omap2/twl-common.h|3 ++ arch/arm/plat-omap/include/plat/clock.h |2 +- 15 files changed, 70 insertions(+), 42 deletions(-) delete mode 100644 arch/arm/mach-omap2/io.h -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[GIT PULL] DSS fixes for v3.2-rc2
Hi Arnd, This one has been separated out from the rest of the fixes. If some of these need to go to stable, then Tomi should do the rebasing and send them to sta...@vger.kernel.org as they don't apply cleanly on v3.1. Regards, Tony The following changes since commit cfcfc9eca2bcbd26a8e206baeb005b055dbf8e37: Linus Torvalds (1): Linux 3.2-rc2 are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap fixes-dss Archit Taneja (1): ARM: OMAP2PLUS: DSS: Ensure DSS works correctly if display is enabled in bootloader Tomi Valkeinen (9): ARM: OMAP2xxx: HWMOD: Fix DSS reset ARM: OMAP2xxx: HWMOD: fix DSS clock data ARM: OMAP3: HWMOD: Fix DSS reset ARM: OMAP3: HWMOD: fix DSS clock data ARM: OMAP4: HWMOD: remove extra clocks ARM: OMAP4: HWMOD: Add HWMOD_CONTROL_OPT_CLKS_IN_RESET for dss_core ARM: OMAP4: HWMOD: fix DSS clock data ARM: OMAP2/3: HWMOD: Add SYSS_HAS_RESET_STATUS for dss ARM: OMAP: HWMOD: Unify DSS resets for OMAPs Tony Lindgren (1): Merge branch 'hwmod_dss_fixes_3.2rc' of git://git.pwsan.com/linux-2.6 into fixes-dss arch/arm/mach-omap2/Makefile |5 +- arch/arm/mach-omap2/display.c | 159 arch/arm/mach-omap2/display.h | 29 arch/arm/mach-omap2/omap_hwmod_2420_data.c | 17 ++- arch/arm/mach-omap2/omap_hwmod_2430_data.c | 17 ++- .../mach-omap2/omap_hwmod_2xxx_3xxx_ipblock_data.c |5 +- arch/arm/mach-omap2/omap_hwmod_3xxx_data.c | 37 - arch/arm/mach-omap2/omap_hwmod_44xx_data.c | 24 ++-- arch/arm/mach-omap2/omap_hwmod_common_data.c |4 + arch/arm/mach-omap2/omap_hwmod_common_data.h |4 + arch/arm/plat-omap/include/plat/common.h |3 + include/video/omapdss.h|7 - 12 files changed, 276 insertions(+), 35 deletions(-) create mode 100644 arch/arm/mach-omap2/display.h -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] omap fixes for v3.2-rc2
* Tony Lindgren [23 14:36]: > * Arnd Bergmann [23 13:47]: > > On Wednesday 23 November 2011 14:03:58 Tony Lindgren wrote: > > > > > > The earlier patches are based on the earlier fixes (while waiting > > > for them to get merged). So that's certainly not a random commit. > > > Or at least was not at that time I can rebase those too anyways > > > now that the earlier fixes are merged. > > > > No need to do that unless you are rebasing them anyway. IMHO > > it's fine if you have all your bug fixes in one branch based > > on the previous bug fixes you sent, but it's of course also > > fine to start a fresh branch for each pull request. > > > > In general, I would recommend not rebasing when you have the > > choice, because that means your patches are not as well tested. > > Well I can keep only four of them because of the pull on a random > commit. Looks like four of them apply to v3.1, will send them > off to sta...@vger.kernel.org and send you two new pull requests. FYI, forgot to mention that I verified that merging in the two pull requests produces the same end result as the original pull request. Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 5/5] clk: export tree topology and clk data via sysfs
* Russell King - ARM Linux [23 14:07]: > On Wed, Nov 23, 2011 at 10:55:19AM -0800, Tony Lindgren wrote: > > What else are you aware of that is really needed early for clocks other > > than clockevent? > > TWD will lose its auto-calibration. Then there's various clock source > and clock event implementations. These all call for the clk API to be > up and running at init_early time. If we can't initialize those later on, then it sounds like some clocks and some parts of the clock fwk code needs to be static. However, we could still make big chunks of the clock framework into a regular device driver for allocating non-static clocks, debugfs interface and so on. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC 1/3] pinctrl: add a driver for the OMAP pinmux
> -Original Message-
> From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
> ow...@vger.kernel.org] On Behalf Of Koen Kooi
> Sent: Wednesday, November 23, 2011 8:51 PM
> To: Tony Lindgren
> Cc: Linus Walleij; Thomas Abraham; Nayak, Rajendra; linux-
> o...@vger.kernel.org; linaro-...@lists.linaro.org;
> linus.wall...@stericsson.com; linux-samsung-soc; devicetree-
> disc...@lists.ozlabs.org
> Subject: Re: [RFC 1/3] pinctrl: add a driver for the OMAP pinmux
>
>
> Op 22 nov. 2011, om 18:54 heeft Tony Lindgren het volgende geschreven:
>
> > * Linus Walleij [22 03:30]:
> >> On Tue, Nov 22, 2011 at 12:09 PM, Thomas Abraham
> >> wrote:
> >>> On 17 November 2011 19:27, Linus Walleij
> wrote:
>
> Maybe I'm mistaken about the device tree ambitions, but
> I was sort of hoping that it would not contain too much
> custom magic numbers that need to be cross-referenced
> elsewhere ... or rather - the more understandable the device
> tree is, the more we win.
> >>>
> >>> Device tree is expected to describe the hardware. So to
> >>> cross-reference the hardware manual to understand device tree should
> >>> be fine I guess. For instance, GPIO numbers in dts would be written as
> >>> a numeric number and not a enum or other format. And by looking up the
> >>> manual, we understand the actual details of the GPIO pin.
> >>>
> >>> If dt-compiler had a option to support #define like in C, the numbers
> >>> could have been made more easier to understand. Like, 3 to mean
> >>> GPIO_PULL_UP in Exynos dts file.
> >>
> >> OK I think I get it now, so DT bindings shall really NOT be read
> >> by any of the pinctrl core, it is *supposed* to be all driver-specific.
> >> Then it makes perfect sense to have it as it is.
> >
> > Yes the driver nodes should describe in DT which pins to use:
> >
> >serial@1234 {
> >compatible = "8250";
> >reg = <0x1234 0x40>;
> >reg-shift = <2>;
> >interrupts = < 10 >;
> > pins = "uart1_rx", "uart1_tx";
> >};
> >
> > Note that we should use the actual signal names, not package specific
> > pad names. This way they have a high likelyhood to work for new packages
> > too by just mapping the signals to the new package.
>
> How would this handle the situation where you can mux a signal to multiple
> pins? IIRC omap3 and am335x can do funny stuff with the display pins like
> being able to map the blue bits to different pinblocks.
>
That's quite not true, in case of omap3 pins are labeled as R0-R7, B0-B7 and
G0-G7; what changes is pixel format.
AM335x LCDC is completely different IP altogether and spec doesn't map
Colors to pins. It barely maps bit0 from memory to pinX.
Now you call it as a standard or legacy or may be due to SGX software,
the pixel format we use is BGR (as in memory).
It is completely depends on how you interface the pins to LCD (considering)
Software support/requirement.
Thanks,
Vaibhav
> regards,
>
> Koen
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 24/24] gpio/omap: handle set_dataout reg capable IP on restore
On Thu, Nov 24, 2011 at 1:24 AM, Kevin Hilman wrote: > "DebBarma, Tarun Kanti" writes: > >> On Mon, Nov 7, 2011 at 5:35 PM, DebBarma, Tarun Kanti >> wrote: >>> On Fri, Nov 4, 2011 at 10:23 PM, Kevin Hilman wrote: Tarun Kanti DebBarma writes: > From: Nishanth Menon > > GPIO IP revisions such as those used in OMAP4 have a set_dataout > while the previous revisions used a single dataout register. > Depending on what is available restore the dataout settings > to the right register. > > Signed-off-by: Nishanth Menon > Signed-off-by: Tarun Kanti DebBarma > Reviewed-by: Santosh Shilimkar > --- > drivers/gpio/gpio-omap.c | 9 +++-- > 1 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > index 4009446..3df7a98 100644 > --- a/drivers/gpio/gpio-omap.c > +++ b/drivers/gpio/gpio-omap.c > @@ -1073,7 +1073,7 @@ static int __devinit omap_gpio_probe(struct > platform_device *pdev) > bank->get_context_loss_count = pdata->get_context_loss_count; > bank->regs = pdata->regs; > > - if (bank->regs->set_dataout && bank->regs->clr_dataout) > + if (bank->regs->set_dataout) This change isn't right. The _set_gpio_dataout_reg function depends on the existence of ->clr_dataout too. >>> Ok, I will add the clr_dataout condtion as well. >> >>> > bank->set_dataout = _set_gpio_dataout_reg; > else > bank->set_dataout = _set_gpio_dataout_mask; > @@ -1351,7 +1351,12 @@ static void omap_gpio_restore_context(struct > gpio_bank *bank) > bank->base + bank->regs->risingdetect); > __raw_writel(bank->context.fallingdetect, > bank->base + bank->regs->fallingdetect); > - __raw_writel(bank->context.dataout, bank->base + > bank->regs->dataout); > + if (bank->regs->set_dataout) Why the check again? The check has already been done in probe. Just use bank->set_dataout() here. >>> Sure, i will make the change. >> >> When I look at the signature of set_dataout(), it does not seem >> straight forward to be used here. It expects (struct gpio_bank *bank, >> int gpio, int enable) to be passed to it. > > IOW, it expects to only set 1 bit, where the context restore needs to > set the value for the whole register. > > OK, then keep the original version, but make sure the if statement > matches is checking for ->set_dataout and ->clr_dataout like the other one. Right. I have done that. -- Tarun > > Kevin > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
