* [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg
From: Russell King - ARM Linux @ 2012-10-25 13:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20121025154202.41f3cbba@endymion.delvare>
On Thu, Oct 25, 2012 at 03:42:02PM +0200, Jean Delvare wrote:
> On Thu, 25 Oct 2012 14:14:59 +0100, Russell King - ARM Linux wrote:
> > On Thu, Oct 25, 2012 at 02:57:48PM +0200, Jean Delvare wrote:
> > > Hi Felipe, Shubhrajyoti,
> > >
> > > On Mon, 22 Oct 2012 12:46:57 +0300, Felipe Balbi wrote:
> > > > From: Shubhrajyoti D <shubhrajyoti@ti.com>
> > > >
> > > > In case of a NACK, it's wise to tell our clients
> > > > drivers about how many bytes were actually transferred.
> > > >
> > > > Support this by adding an extra field to the struct
> > > > i2c_msg which gets incremented the amount of bytes
> > > > actually transferred.
> > > >
> > > > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> > > > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > > > ---
> > > > include/uapi/linux/i2c.h | 1 +
> > > > 1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
> > > > index 0e949cb..4b35c9b 100644
> > > > --- a/include/uapi/linux/i2c.h
> > > > +++ b/include/uapi/linux/i2c.h
> > > > @@ -77,6 +77,7 @@ struct i2c_msg {
> > > > #define I2C_M_NO_RD_ACK 0x0800 /* if I2C_FUNC_PROTOCOL_MANGLING */
> > > > #define I2C_M_RECV_LEN 0x0400 /* length will be first received byte */
> > > > __u16 len; /* msg length */
> > > > + __u16 transferred; /* actual bytes transferred */
> > > > __u8 *buf; /* pointer to msg data */
> > > > };
> > >
> > > On the principle I am fine with this, however you also need to define
> > > who should initialize this field, and to what value.
> >
> > You also miss one very very very big point. This will break every I2C
> > using userspace program out there unless it is rebuilt - this change will
> > require the exact right version of those userspace programs for the
> > kernel that they're being used on.
>
> How that? The extra field is added in a hole, so we don't change the
> struct size nor the relative positions of existing fields. Why would
> user-space care?
You know the layout of that struct for certain across all Linux supported
architectures, including some of the more obscure ones which may not
require pointers to be aligned?
^ permalink raw reply
* [GIT PULL] first set of ux500 for ARM SoC for v3.8
From: Arnd Bergmann @ 2012-10-25 13:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CACRpkdbSx-e4q+++=chjgyt0yD_v=JOP5OrQPoXb2-EPfNnrvg@mail.gmail.com>
On Thursday 25 October 2012, Linus Walleij wrote:
> On Thu, Oct 18, 2012 at 8:39 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> > Hi ARM SoC folks,
> >
> > here are two small commits for v3.8 that I'd like to make sure
> > land in ARM SoC before I begin with all the other stuff.
> >
> > Please pull them in!
>
> Ping on this.
Sorry for the delay, I just took over the task to pull in stuff from Olof
and we were both busy otherwise earlier.
I pulled this into a next/soc branch now. Wasn't completely sure which
branch to use but I figured that we couldn't make a good prediction of
the ordering yet, and we'll probably have a next/soc2 branch later anyway.
Please remember to write the pull request in a way that lets us reuse some
of the text for the merge changeset comment. Ideally in a signed tag
so git puts it in there automatically.
Arnd
^ permalink raw reply
* [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg
From: Jean Delvare @ 2012-10-25 13:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20121025131459.GG28061@n2100.arm.linux.org.uk>
On Thu, 25 Oct 2012 14:14:59 +0100, Russell King - ARM Linux wrote:
> On Thu, Oct 25, 2012 at 02:57:48PM +0200, Jean Delvare wrote:
> > Hi Felipe, Shubhrajyoti,
> >
> > On Mon, 22 Oct 2012 12:46:57 +0300, Felipe Balbi wrote:
> > > From: Shubhrajyoti D <shubhrajyoti@ti.com>
> > >
> > > In case of a NACK, it's wise to tell our clients
> > > drivers about how many bytes were actually transferred.
> > >
> > > Support this by adding an extra field to the struct
> > > i2c_msg which gets incremented the amount of bytes
> > > actually transferred.
> > >
> > > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> > > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > > ---
> > > include/uapi/linux/i2c.h | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
> > > index 0e949cb..4b35c9b 100644
> > > --- a/include/uapi/linux/i2c.h
> > > +++ b/include/uapi/linux/i2c.h
> > > @@ -77,6 +77,7 @@ struct i2c_msg {
> > > #define I2C_M_NO_RD_ACK 0x0800 /* if I2C_FUNC_PROTOCOL_MANGLING */
> > > #define I2C_M_RECV_LEN 0x0400 /* length will be first received byte */
> > > __u16 len; /* msg length */
> > > + __u16 transferred; /* actual bytes transferred */
> > > __u8 *buf; /* pointer to msg data */
> > > };
> >
> > On the principle I am fine with this, however you also need to define
> > who should initialize this field, and to what value.
>
> You also miss one very very very big point. This will break every I2C
> using userspace program out there unless it is rebuilt - this change will
> require the exact right version of those userspace programs for the
> kernel that they're being used on.
How that? The extra field is added in a hole, so we don't change the
struct size nor the relative positions of existing fields. Why would
user-space care?
--
Jean Delvare
^ permalink raw reply
* [PATCH 1/2] ARM: clk-imx27: Add missing clock for mx2-camera
From: Mauro Carvalho Chehab @ 2012-10-25 13:38 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1349473981-15084-2-git-send-email-fabio.estevam@freescale.com>
Hi F?bio,
Em Fri, 5 Oct 2012 18:53:01 -0300
Fabio Estevam <fabio.estevam@freescale.com> escreveu:
> During the clock conversion for mx27 the "per4_gate" clock was missed to get
> registered as a dependency of mx2-camera driver.
>
> In the old mx27 clock driver we used to have:
>
> DEFINE_CLOCK1(csi_clk, 0, NULL, 0, parent, &csi_clk1, &per4_clk);
>
> ,so does the same in the new clock driver.
>
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
> arch/arm/mach-imx/clk-imx27.c | 1 +
As this patch is for arch/arm, I'm understanding that it will be merged
via arm tree. So,
Acked-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/mach-imx/clk-imx27.c b/arch/arm/mach-imx/clk-imx27.c
> index 3b6b640..5ef0f08 100644
> --- a/arch/arm/mach-imx/clk-imx27.c
> +++ b/arch/arm/mach-imx/clk-imx27.c
> @@ -224,6 +224,7 @@ int __init mx27_clocks_init(unsigned long fref)
> clk_register_clkdev(clk[lcdc_ipg_gate], "ipg", "imx-fb.0");
> clk_register_clkdev(clk[lcdc_ahb_gate], "ahb", "imx-fb.0");
> clk_register_clkdev(clk[csi_ahb_gate], "ahb", "mx2-camera.0");
> + clk_register_clkdev(clk[per4_gate], "per", "mx2-camera.0");
> clk_register_clkdev(clk[usb_div], "per", "fsl-usb2-udc");
> clk_register_clkdev(clk[usb_ipg_gate], "ipg", "fsl-usb2-udc");
> clk_register_clkdev(clk[usb_ahb_gate], "ahb", "fsl-usb2-udc");
--
Regards,
Mauro
^ permalink raw reply
* [PATCH 2/2] arm: mvebu: adding SATA support: dt binding and config update
From: Jason Cooper @ 2012-10-25 13:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20121025152114.73e69c92@skate>
On Thu, Oct 25, 2012 at 03:21:14PM +0200, Thomas Petazzoni wrote:
> Jason,
>
> On Thu, 25 Oct 2012 09:18:18 -0400, Jason Cooper wrote:
>
> > > Jason, Andrew, do you want I split this patch as suggested by
> > > Thomas or are you fine with having one single patch?
> >
> > Yes, please make the defconfig changes a separate patch. Also, please
> > make sure only the minimum is enabled (eq RAID... isn't needed).
>
> I haven't looked in details at the driver, but is nr-ports = <foo> the
> right way of doing things? We may have platforms were port 0 is not
> used, but port 1 is used, and just a number of ports doesn't allow to
> express this.
Do you have an example of this?
thx,
Jason.
^ permalink raw reply
* [PATCH v3] ARM: mach-imx: Fix selection of ARCH_MXC
From: Arnd Bergmann @ 2012-10-25 13:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20121025025117.GC2464@S2101-09.ap.freescale.net>
On Thursday 25 October 2012, Shawn Guo wrote:
> On Fri, Oct 19, 2012 at 11:05:54PM +0800, Shawn Guo wrote:
> > On Thu, Oct 18, 2012 at 03:04:30PM -0300, Fabio Estevam wrote:
> > > Since commit c5a0d497(ARM: imx: enable multi-platform build),
> > > ARCH_MXC is selected by the following logic:
> > >
> > > config ARCH_MXC
> > > def_bool y if ARCH_MULTI_V4_V5 || ARCH_MULTI_V6_V7
> > >
> > > , which causes build error on vexpress_defconfig:
> > >
> > > arch/arm/mach-imx/hotplug.c:49: undefined reference to `imx_enable_cpu'
> > > arch/arm/mach-imx/platsmp.c:57: undefined reference to `imx_set_cpu_jump'
> > > arch/arm/mach-imx/platsmp.c:58: undefined reference to `imx_enable_cpu'
> > >
> > > Make ARCH_MXC a user selectable option, so that it does not get built
> > > by default on other defconfigs that select ARCH_MULTI_V4_V5 or ARCH_MULTI_V6_V7.
> > >
> > > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> >
> > Arnd, Olof,
> >
> > I just applied this patch on my imx/multi-platform. Can you please
> > pull it to update the branch in arm-soc? Thanks.
> >
> I just applied another fix [1] on my branch. Please pull both into
> arm-soc. Thanks.
Pulled into imx/multiplatform and next/cleanup now.
Thanks,
Arnd
^ permalink raw reply
* [PATCH 2/2] arm: mvebu: adding SATA support: dt binding and config update
From: Gregory CLEMENT @ 2012-10-25 13:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20121025152114.73e69c92@skate>
On 10/25/2012 03:21 PM, Thomas Petazzoni wrote:
> Jason,
>
> On Thu, 25 Oct 2012 09:18:18 -0400, Jason Cooper wrote:
>
>>> Jason, Andrew, do you want I split this patch as suggested by
>>> Thomas or are you fine with having one single patch?
>>
>> Yes, please make the defconfig changes a separate patch. Also, please
>> make sure only the minimum is enabled (eq RAID... isn't needed).
>
> I haven't looked in details at the driver, but is nr-ports = <foo> the
> right way of doing things? We may have platforms were port 0 is not
> used, but port 1 is used, and just a number of ports doesn't allow to
> express this.
>
> Shouldn't the DT property be
>
> ports = <0>, <1>
> ports = <1>
> ports = <1>, <3>
>
> In order to allow to more precisely enabled SATA ports? Or maybe the
> SATA ports cannot be enabled/disabled on a per-port basis, in which
> case I'm obviously wrong here.
The actual implementation of mv_sata.c doesn't work like this. You can
only pass the number of ports supported not the list of the port you
want to support. I've checked in the device tree binding documentation
_and_ also in the code.
>
> Best regards,
>
> Thomas
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply
* [PATCH] ARM: decompressor: clear SCTLR.A bit for v7 cores
From: Arnd Bergmann @ 2012-10-25 13:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <50893389.2090002@gmail.com>
On Thursday 25 October 2012, Rob Herring wrote:
> > On Thu, Oct 11, 2012 at 07:43:22AM -0500, Rob Herring wrote:
> >>
> >> With recent compilers and move to generic unaligned.h in commit d25c881
> >> (ARM: 7493/1: use generic unaligned.h), unaligned accesses will be used
> >> by the LZO decompressor on v7 cores. So we need to make sure unaligned
> >> accesses are allowed by clearing the SCTLR A bit.
> >
> > I just read this in http://gcc.gnu.org/gcc-4.7/changes.html:
> >
> > On ARM, when compiling for ARMv6 (but not ARMv6-M), ARMv7-A, ARMv7-R, or
> > ARMv7-M, the new option -munaligned-access is active by default, which for
> > some source codes generates code that accesses memory on unaligned addresses.
> > This will require the kernel of those systems to enable such accesses
> > (controlled by CP15 register c1, refer to ARM documentation). Alternatively
> > or for compatibility with kernels where unaligned accesses are not supported,
> > all code has to be compiled with -mno-unaligned-access. Linux/ARM in official
> > releases has automatically and unconditionally supported unaligned accesses
> > as emitted by GCC due to this option being active, since Linux version 2.6.28.
>
> I don't think there is such a thing as ARMv6-M.
There is, that would be the Cortex-M0/M0+/M1. You cannot run Linux on these.
Arnd
^ permalink raw reply
* [PATCH 3/5] pinctrl: dove: fix iomem and pdma clock
From: Jason Cooper @ 2012-10-25 13:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <508900A2.20005@gmail.com>
On Thu, Oct 25, 2012 at 11:04:34AM +0200, Sebastian Hesselbarth wrote:
> On 10/25/2012 09:04 AM, Linus Walleij wrote:
> >On Wed, Oct 24, 2012 at 4:25 PM, Andrew Lunn<andrew@lunn.ch> wrote:
> >
> >>Since 3.7 readl/writel require register addresses to be const void*
> >>instead of unsigned int. The register addresses are converted using
> >>IOMEM() and offsets are added instead of OR'ed.
> >>Also a workaround for the pdma clock is added, that is required as
> >>there is still no DT clock provider available on Dove.
> >
> >So essentially two unrelated patches squashed into one, and I
> >would apply the first one right off but now the clock change makes
> >me hesitate.
> >
> >> clk = devm_clk_get(&pdev->dev, NULL);
> >>+
> >>+ /* Currently there is no DT clock provider for pdma clock,
> >>+ this fallback ensures pdma clock is ticking */
> >
> >/*
> > * I prefer comment style like so because it's easier to read.
> > * Maybe it's a bit pedantic but bear with me.
> > */
> >
> >>+ if (IS_ERR(clk))
> >>+ clk = clk_get_sys("dove-pdma", NULL);
> >>+
> >
> >This is a horrible hack. But if the Marvell people agree about
> >it I will live with it.
>
> Unfortunately, it is. This is an chicken-egg-problem here, no
> DT clk-provider, no clocks properties..
>
> While writing pinctrl-dove I was planing to enable it after
> the clock provider but with Andrew pushing forward - for a good
> reason - there comes the trouble ;)
>
> I don't like the hack either but the clk-gate is there and
> if I don't enable it pinctrl-dove will hang the SoC.
>
> I have a clk-dove DT clk-provider in my pocket somewhere but
> didn't find the time to prepare it for posting..
Then let's split out the dove-pinctrl patches until you have that series
ready.
thx,
Jason.
^ permalink raw reply
* [PATCH] ARM: dts: mxs: add the "clock-names" for gpmi-nand
From: Arnd Bergmann @ 2012-10-25 13:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20121025071629.GD2464@S2101-09.ap.freescale.net>
On Thursday 25 October 2012, Shawn Guo wrote:
>
> Hi Arnd, Olof,
>
> Can you please apply this fix for -rc? Otherwise, please let me know
> if you prefer to get a pull request from me.
>
Applied to fixes branch, and added your Ack as well as the Review
tag from Marek.
Arnd
^ permalink raw reply
* [PATCH 4/4] OMAP: mtd: gpmc: add DT bindings for GPMC timings and NAND
From: Jon Hunter @ 2012-10-25 13:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <508909CC.9000000@gmail.com>
On 10/25/2012 04:43 AM, Daniel Mack wrote:
> Hi Jon,
>
> On 25.10.2012 03:53, Jon Hunter wrote:
>>
>> On 10/22/2012 02:55 PM, Daniel Mack wrote:
>
>>> +Example for an AM33xx board:
>>> +
>>> + gpmc: gpmc at 50000000 {
>>> + compatible = "ti,gpmc";
>>> + ti,hwmods = "gpmc";
>>> + reg = <0x50000000 0x1000000>;
>>
>> Nit-pick, that size is quite large for a register range. I recommend
>> looking at the HWMOD data file
>> (arch/arm/mach-omap2/omap_hwmod_33xx_data.c) and see how much space is
>> allocated for the registers (see structure am33xx_gpmc_addr_space).
>
> Yeah but reserving the entire memory as per the reference manual also
> prvents other from poking around in the same register space. Is there a
> scenario in which it would of disadvantage to reserve all that memory?
1. It chews up a large chunk of virtual memory space unnecessarily. For
devices not using HIGHMEM and wish to use say 512MB of RAM, virtual
memory space can be constrained.
2. We don't do that today probably because of #1.
Cheers
Jon
^ permalink raw reply
* [PATCH 2/2] arm: mvebu: adding SATA support: dt binding and config update
From: Thomas Petazzoni @ 2012-10-25 13:21 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20121025131818.GF18811@titan.lakedaemon.net>
Jason,
On Thu, 25 Oct 2012 09:18:18 -0400, Jason Cooper wrote:
> > Jason, Andrew, do you want I split this patch as suggested by
> > Thomas or are you fine with having one single patch?
>
> Yes, please make the defconfig changes a separate patch. Also, please
> make sure only the minimum is enabled (eq RAID... isn't needed).
I haven't looked in details at the driver, but is nr-ports = <foo> the
right way of doing things? We may have platforms were port 0 is not
used, but port 1 is used, and just a number of ports doesn't allow to
express this.
Shouldn't the DT property be
ports = <0>, <1>
ports = <1>
ports = <1>, <3>
In order to allow to more precisely enabled SATA ports? Or maybe the
SATA ports cannot be enabled/disabled on a per-port basis, in which
case I'm obviously wrong here.
Best regards,
Thomas
--
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply
* [PATCH v2] gpio/omap: fix off-mode bug: clear debounce clock enable mask on free/reset
From: Santosh Shilimkar @ 2012-10-25 13:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <50893A71.1030702@ti.com>
On Thursday 25 October 2012 06:41 PM, Jon Hunter wrote:
>
> On 10/25/2012 02:00 AM, Santosh Shilimkar wrote:
>> On Thursday 25 October 2012 04:24 AM, Jon Hunter wrote:
>>>
>>> On 10/24/2012 12:10 PM, Kevin Hilman wrote:
>>>> From: Kevin Hilman <khilman@ti.com>
>>>>
>>>> When a GPIO bank is freed or shutdown, ensure that the banks
>>>> dbck_enable_mask is cleared also. Otherwise, context restore on
>>>> subsequent off-mode transition will restore previous value from the
>>>> shadow copies (bank->context.debounce*) leading to mismatch state
>>>> between driver state and hardware state.
>>>>
>>>> This was discovered when board code was doing
>>>>
>>>> gpio_request_one()
>>>> gpio_set_debounce()
>>>> gpio_free()
>>>>
>>>> which was leaving the GPIO debounce settings in a confused state. If
>>>> that GPIO bank is subsequently used with off-mode enabled, bogus state
>>>> would be restored, leaving GPIO debounce enabled which then prevented
>>>> the CORE powerdomain from transitioning.
>>>>
>>>> To fix, ensure that bank->dbck_enable_mask is cleared when the bank
>>>> is freed/shutdown so debounce state doesn't persist.
>> The freed part is fine but I don't understand why it needs to be done
>> on _a_ gpio irq shutdown callback where IRQs related configuration
>> on that one GPIO needs to be cleared. De-bounce clock is surely not IRQ
>> related configuration.
>
> If we are freeing the IRQs related to gpio and resetting the gpio, then
> I don't see why we should not. We should not leave the debounce clock on
> if these gpios are no longer being used.
>
Sure which happens in free() anyways.
>>>> Special thanks to Grazvydas Ignotas for pointing out a bug in an
>>>> earlier version that would've disabled debounce on any runtime PM
>>>> transition.
>>>>
>>>> Reported-by: Paul Walmsley <paul@pwsan.com>
>>>> Cc: Igor Grinberg <grinberg@compulab.co.il>
>>>> Cc: Grazvydas Ignotas <notasas@gmail.com>
>>>> Signed-off-by: Kevin Hilman <khilman@ti.com>
>>>> ---
>>>> v2: only clear mask in free/shutdown, not in runtime PM paths,
>>>> clarified changelog
>>>> Applies on v3.7-rc2.
>>>>
>>>> drivers/gpio/gpio-omap.c | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>>>> index 94cbc84..113b167 100644
>>>> --- a/drivers/gpio/gpio-omap.c
>>>> +++ b/drivers/gpio/gpio-omap.c
>>>> @@ -539,6 +539,7 @@ static void _reset_gpio(struct gpio_bank *bank,
>>>> int gpio)
>>>> _set_gpio_irqenable(bank, gpio, 0);
>>>> _clear_gpio_irqstatus(bank, gpio);
>>>> _set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE);
>>>> + bank->dbck_enable_mask = 0;
>>>> }
>>>
>>> Does this need to be ...
>>>
>>> + bank->dbck_enable_mask &= ~(GPIO_BIT(bank, gpio));
>>> + _gpio_dbck_disable(bank);
>>>
>> Yes, its a per bank clock. There is an alternate. See below.
>>
>>> There could be more than one gpio using debounce and so we should only
>>> clear the appropriate bit. Also after clearing a bit we could see if we
>>> can disable the debounce clock too.
>>>
>> When I mentioned the clearing in gpio_free() path will do trick, I had
>> something like below in mind.
>>
>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>> index dee2856..8574105 100644
>> --- a/drivers/gpio/gpio-omap.c
>> +++ b/drivers/gpio/gpio-omap.c
>> @@ -629,8 +629,10 @@ static void omap_gpio_free(struct gpio_chip *chip,
>> unsigned offset)
>> * If this is the last gpio to be freed in the bank,
>> * disable the bank module.
>> */
>> - if (!bank->mod_usage)
>> + if (!bank->mod_usage) {
>> + bank->dbck_enable_mask = 0;
>> pm_runtime_put(bank->dev);
>> + }
>
> However, with this we could be leaving the debounce clock on longer than
> needed. I think we need to call _gpio_dbck_disable() each time we free a
> gpio and this function will determine if it can turn off the debounce
> clock.
The point is you can't disable the debounce clock if the back in use.
But I get your point. In case other in use GPIOs from bank doesn't
need debouce feature, we can turn off the debouce clock.
> In fact, there appears to be another bug in the current driver, that we
> do not clear the debounce_en register when freeing the gpio. Your patch
> addresses this, but I think that debounce should be disabled when a gpio
> is freed and not just when the last one is freed.
>
I agree. Just go ahead and spin the patch.
> Also, with the above change, can't we still run into the original
> problem? In other words, if a gpio is freed, but there is still another
> one active in the back that is not using debounce, then we could restore
> a incorrect debounce context because we have not clean-up the debounce
> settings?
>
Yep. Just mentioned above.
> So may be we need to add a _clear_gpio_debounce() function and
> call this when freeing a gpio.
>
Sounds good. This function can track the debounce back usage
and based on that do the deb-ounce clean-up.
Regards
santosh
^ permalink raw reply
* [PATCH 2/2] arm: mvebu: adding SATA support: dt binding and config update
From: Jason Cooper @ 2012-10-25 13:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5087F5B9.0@free-electrons.com>
On Wed, Oct 24, 2012 at 04:05:45PM +0200, Gregory CLEMENT wrote:
> On 10/24/2012 04:01 PM, Thomas Petazzoni wrote:
> > Hello,
> >
> > Shouldn't you split into one commit adding the SATA definition in
> > the .dtsi + doing the defconfig change (the "SoC" level modifications),
> > and then another commit for the .dts change? I don't really care
> > personally, it's really up to Jason/Andrew on this.
> >
> > Another comment below, though.
> >
> > On Wed, 24 Oct 2012 15:49:21 +0200, Gregory CLEMENT wrote:
> >
> >> diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
> >> index 94b4b9e..3f08233 100644
> >> --- a/arch/arm/boot/dts/armada-370-xp.dtsi
> >> +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
> >> @@ -69,6 +69,16 @@
> >> compatible = "marvell,armada-addr-decoding-controller";
> >> reg = <0xd0020000 0x258>;
> >> };
> >> +
> >> + sata at d00a0000 {
> >> + compatible = "marvell,orion-sata";
> >> + reg = <0xd00a0000 0x2400>;
> >> + interrupts = <55>;
> >> + nr-ports = <2>;
> >> + clocks = <&coreclk 0>;//, <&coreclk 0>;
> >
> > Alignment problem + remainings of tests or something like that.
>
> True I missed this one.
>
> Jason, Andrew, do you want I split this patch as suggested by Thomas or
> are you fine with having one single patch?
Yes, please make the defconfig changes a separate patch. Also, please
make sure only the minimum is enabled (eq RAID... isn't needed).
thx,
Jason.
^ permalink raw reply
* [PATCH 4/4] OMAP: mtd: gpmc: add DT bindings for GPMC timings and NAND
From: Jon Hunter @ 2012-10-25 13:16 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5088F19B.7050606@gmail.com>
On 10/25/2012 03:00 AM, Daniel Mack wrote:
> Hi Jon,
>
> many thanks for your time to look at this.
>
> On 25.10.2012 03:28, Jon Hunter wrote:
>> On 10/22/2012 02:55 PM, Daniel Mack wrote:
>>> diff --git a/Documentation/devicetree/bindings/bus/gpmc.txt b/Documentation/devicetree/bindings/bus/gpmc.txt
>>> new file mode 100644
>>> index 0000000..ef1c6e1
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/bus/gpmc.txt
>>> @@ -0,0 +1,59 @@
>>> +Device tree bindings for OMAP general purpose memory controllers (GPMC)
>>> +
>>> +The actual devices are instantiated from the child nodes of a GPMC node.
>>> +
>>> +Required properties:
>>> +
>>> + - compatible: Should be set to "ti,gpmc"
>>
>> Is this the only required property? I think that "reg" and "ti,hwmods"
>> are probably also required.
>
> Well yes, but at least "reg" is commonly omitted as it's part of a more
> "generic" set of properties. But ok, I can add these.
>
>> Also given that we are describing the hardware, I am wondering if the
>> number of chip-selects and wait signals should be defined here too. I
>> recall that different devices had different number of wait pins available.
>
> Hmm, that number is currently hard-coded in GPMC_CS_NUM. It would take
> some effort to make that dynamic but I agree that this would be a good
> thing to have. Afzal?
I believe that today all OMAP/AM devices have 8 chip-selects so probably
not a big deal. However, given we are moving to DT it would be nice to
move away from having such #defines for hardware related items.
Cheers
Jon
^ permalink raw reply
* [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg
From: Russell King - ARM Linux @ 2012-10-25 13:14 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20121025145748.760c218b@endymion.delvare>
On Thu, Oct 25, 2012 at 02:57:48PM +0200, Jean Delvare wrote:
> Hi Felipe, Shubhrajyoti,
>
> On Mon, 22 Oct 2012 12:46:57 +0300, Felipe Balbi wrote:
> > From: Shubhrajyoti D <shubhrajyoti@ti.com>
> >
> > In case of a NACK, it's wise to tell our clients
> > drivers about how many bytes were actually transferred.
> >
> > Support this by adding an extra field to the struct
> > i2c_msg which gets incremented the amount of bytes
> > actually transferred.
> >
> > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > ---
> > include/uapi/linux/i2c.h | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
> > index 0e949cb..4b35c9b 100644
> > --- a/include/uapi/linux/i2c.h
> > +++ b/include/uapi/linux/i2c.h
> > @@ -77,6 +77,7 @@ struct i2c_msg {
> > #define I2C_M_NO_RD_ACK 0x0800 /* if I2C_FUNC_PROTOCOL_MANGLING */
> > #define I2C_M_RECV_LEN 0x0400 /* length will be first received byte */
> > __u16 len; /* msg length */
> > + __u16 transferred; /* actual bytes transferred */
> > __u8 *buf; /* pointer to msg data */
> > };
>
> On the principle I am fine with this, however you also need to define
> who should initialize this field, and to what value.
You also miss one very very very big point. This will break every I2C
using userspace program out there unless it is rebuilt - this change will
require the exact right version of those userspace programs for the
kernel that they're being used on.
Now that we have the userspace API headers separated, this is now much
easier to detect: a patch which touches a uapi header needs much more
careful consideration than one which doesn't.
So no, strong NAK. This is not how we treat userspace.
If we want to change userspace API then we do it in a sane manner, where
we provide the new functionality in a way that it doesn't break existing
users. There's two ways to do this:
1. Leave the existing struct alone, introduce a new one with new ioctls.
Schedule the removal of the old interfaces for maybe 10 years time.
2. Rename the existing struct (eg struct old_i2c_msg), and create a new
struct called i2c_msg. Rename the existing ioctls to have OLD_ in
their names. Provide the existing ioctls under those names, and
make them print a warning once that userspace programs need updating.
Schedule the removal of the old interfaces for a shorter number of
years than (1);
Remember all those "old" syscalls we have... this is no different from
those.
^ permalink raw reply
* [PATCH 2/6] pinctrl: Update clock handling for the pinctrl-nomadik GPIO driver
From: Linus Walleij @ 2012-10-25 13:13 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CACRpkda2NQok7kzh0pFmZPYd4nky1kUw=67DZAYAJAXGuc1yzQ@mail.gmail.com>
On Thu, Oct 25, 2012 at 2:41 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Oct 24, 2012 at 4:45 PM, Lee Jones <lee.jones@linaro.org> wrote:
>
>> The clock framework has changed somewhat and it's now better to
>> invoke clock_prepare_enable() and clk_disable_unprepare() rather
>> than the legacy clk_enable() and clk_disable() calls. This patch
>> converts the Nomadik Pin Control driver to the new framework.
>>
>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>
> I was convinced that this is a good change but no regression,
> so applied to the devel branch for 3.8.
>
> I also removed the initial clk_prepare() so the reference count
> may actually go down to 0 for the GPIO block and the peripheral
> cluster eventually gets relaxed.
Famous last words!
The good news is that this actually works, and the refcount
*does* go down to zero and gate off entire peripheral
clusters.
However that was not good because something vital in
some peripheral cluster died and killed the system :-D
Lee, could to to track down the reason and fix it so the patch
can be applied?
The only thing you need to do is to remove the superfluous
clk_prepare() right after the devm_clk_get() that hogs each
peripheral cluster.
Probably some driver is needing a clk_get() or a clk_get_sys() is
needs to be added somewhere to bring up some vital cluster,
or there may be some out-of-tree driver needed to bring up the
cluster properly I have no clue... Maybe some cluster just
cannot be declocked like that.
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH v2] gpio/omap: fix off-mode bug: clear debounce clock enable mask on free/reset
From: Jon Hunter @ 2012-10-25 13:11 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5088E387.2050704@ti.com>
On 10/25/2012 02:00 AM, Santosh Shilimkar wrote:
> On Thursday 25 October 2012 04:24 AM, Jon Hunter wrote:
>>
>> On 10/24/2012 12:10 PM, Kevin Hilman wrote:
>>> From: Kevin Hilman <khilman@ti.com>
>>>
>>> When a GPIO bank is freed or shutdown, ensure that the banks
>>> dbck_enable_mask is cleared also. Otherwise, context restore on
>>> subsequent off-mode transition will restore previous value from the
>>> shadow copies (bank->context.debounce*) leading to mismatch state
>>> between driver state and hardware state.
>>>
>>> This was discovered when board code was doing
>>>
>>> gpio_request_one()
>>> gpio_set_debounce()
>>> gpio_free()
>>>
>>> which was leaving the GPIO debounce settings in a confused state. If
>>> that GPIO bank is subsequently used with off-mode enabled, bogus state
>>> would be restored, leaving GPIO debounce enabled which then prevented
>>> the CORE powerdomain from transitioning.
>>>
>>> To fix, ensure that bank->dbck_enable_mask is cleared when the bank
>>> is freed/shutdown so debounce state doesn't persist.
> The freed part is fine but I don't understand why it needs to be done
> on _a_ gpio irq shutdown callback where IRQs related configuration
> on that one GPIO needs to be cleared. De-bounce clock is surely not IRQ
> related configuration.
If we are freeing the IRQs related to gpio and resetting the gpio, then
I don't see why we should not. We should not leave the debounce clock on
if these gpios are no longer being used.
>>> Special thanks to Grazvydas Ignotas for pointing out a bug in an
>>> earlier version that would've disabled debounce on any runtime PM
>>> transition.
>>>
>>> Reported-by: Paul Walmsley <paul@pwsan.com>
>>> Cc: Igor Grinberg <grinberg@compulab.co.il>
>>> Cc: Grazvydas Ignotas <notasas@gmail.com>
>>> Signed-off-by: Kevin Hilman <khilman@ti.com>
>>> ---
>>> v2: only clear mask in free/shutdown, not in runtime PM paths,
>>> clarified changelog
>>> Applies on v3.7-rc2.
>>>
>>> drivers/gpio/gpio-omap.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>>> index 94cbc84..113b167 100644
>>> --- a/drivers/gpio/gpio-omap.c
>>> +++ b/drivers/gpio/gpio-omap.c
>>> @@ -539,6 +539,7 @@ static void _reset_gpio(struct gpio_bank *bank,
>>> int gpio)
>>> _set_gpio_irqenable(bank, gpio, 0);
>>> _clear_gpio_irqstatus(bank, gpio);
>>> _set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE);
>>> + bank->dbck_enable_mask = 0;
>>> }
>>
>> Does this need to be ...
>>
>> + bank->dbck_enable_mask &= ~(GPIO_BIT(bank, gpio));
>> + _gpio_dbck_disable(bank);
>>
> Yes, its a per bank clock. There is an alternate. See below.
>
>> There could be more than one gpio using debounce and so we should only
>> clear the appropriate bit. Also after clearing a bit we could see if we
>> can disable the debounce clock too.
>>
> When I mentioned the clearing in gpio_free() path will do trick, I had
> something like below in mind.
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index dee2856..8574105 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -629,8 +629,10 @@ static void omap_gpio_free(struct gpio_chip *chip,
> unsigned offset)
> * If this is the last gpio to be freed in the bank,
> * disable the bank module.
> */
> - if (!bank->mod_usage)
> + if (!bank->mod_usage) {
> + bank->dbck_enable_mask = 0;
> pm_runtime_put(bank->dev);
> + }
However, with this we could be leaving the debounce clock on longer than
needed. I think we need to call _gpio_dbck_disable() each time we free a
gpio and this function will determine if it can turn off the debounce
clock.
In fact, there appears to be another bug in the current driver, that we
do not clear the debounce_en register when freeing the gpio. Your patch
addresses this, but I think that debounce should be disabled when a gpio
is freed and not just when the last one is freed.
Also, with the above change, can't we still run into the original
problem? In other words, if a gpio is freed, but there is still another
one active in the back that is not using debounce, then we could restore
a incorrect debounce context because we have not clean-up the debounce
settings?
So may be we need to add a _clear_gpio_debounce() function and
call this when freeing a gpio.
Cheers
Jon
^ permalink raw reply
* [PATCH 2/6] pinctrl: Update clock handling for the pinctrl-nomadik GPIO driver
From: Lee Jones @ 2012-10-25 13:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CACRpkda2NQok7kzh0pFmZPYd4nky1kUw=67DZAYAJAXGuc1yzQ@mail.gmail.com>
On Thu, 25 Oct 2012, Linus Walleij wrote:
> On Wed, Oct 24, 2012 at 4:45 PM, Lee Jones <lee.jones@linaro.org> wrote:
>
> > The clock framework has changed somewhat and it's now better to
> > invoke clock_prepare_enable() and clk_disable_unprepare() rather
> > than the legacy clk_enable() and clk_disable() calls. This patch
> > converts the Nomadik Pin Control driver to the new framework.
> >
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
>
> I was convinced that this is a good change but no regression,
> so applied to the devel branch for 3.8.
>
> I also removed the initial clk_prepare() so the reference count
> may actually go down to 0 for the GPIO block and the peripheral
> cluster eventually gets relaxed.
Nice. Thanks Linus.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [PATCH v2 4/7] i2c: omap: in case of VERSION_2 read IRQSTATUS_RAW but write to IRQSTATUS
From: Santosh Shilimkar @ 2012-10-25 13:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20121025125249.GK21217@arwen.pp.htv.fi>
On Thursday 25 October 2012 06:22 PM, Felipe Balbi wrote:
> Hi,
>
> On Thu, Oct 25, 2012 at 06:23:57PM +0530, Santosh Shilimkar wrote:
>> On Thursday 25 October 2012 05:55 PM, Felipe Balbi wrote:
>>> on OMAP4+ we want to read IRQSTATUS_RAW register,
>>> instead of IRQSTATUS. The reason being that IRQSTATUS
>>> will only contain the bits which were enabled on
>>> IRQENABLE_SET and that will break when we need to
>>> poll for a certain bit which wasn't enabled as an
>>> IRQ source.
>>>
>>> One such case is after we finish converting to
>>> deferred stop bit, we will have to poll for ARDY
>>> bit before returning control for the client driver
>>> in order to prevent us from trying to start a
>>> transfer on a bus which is already busy.
>>>
>>> Note, however, that omap-i2c.c needs a big rework
>>> on register definition and register access. Such
>>> work will be done in a separate series of patches.
>>>
>>> Cc: Benoit Cousson <b-cousson@ti.com>
>>> Signed-off-by: Felipe Balbi <balbi@ti.com>
>>> ---
>>> drivers/i2c/busses/i2c-omap.c | 12 +++++++++++-
>>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
>>> index b004126..20f9ad6 100644
>>> --- a/drivers/i2c/busses/i2c-omap.c
>>> +++ b/drivers/i2c/busses/i2c-omap.c
>>> @@ -271,8 +271,18 @@ static inline void omap_i2c_write_reg(struct omap_i2c_dev *i2c_dev,
>>>
>>> static inline u16 omap_i2c_read_reg(struct omap_i2c_dev *i2c_dev, int reg)
>>> {
>>> - return __raw_readw(i2c_dev->base +
>>> + /* if we are OMAP_I2C_IP_VERSION_2, we need to read from
>>> + * IRQSTATUS_RAW, but write to IRQSTATUS
>>> + */
>>> + if ((i2c_dev->dtrev == OMAP_I2C_IP_VERSION_2) &&
>>> + (reg == OMAP_I2C_STAT_REG)) {
>> Doing this check on every I2C register read seems to
>> expensive to me. Can you not sort this in init with some offset
>> which can be 0 or non zero ? Sorry in case this is already dicussed.
>
> could be. I didn't go that route because I'm planning a complete rewrite
> of all register accesses. The way it's done today is completely broken
> and already expensive (with reg_shift and different map tables and so
> on).
>
> If it's really a big of a deal, I can try to find another way, maybe
> just adding omap_i2c_read_stat() and limit the version check just to
> I2C_STAT reads would do it for now...
>
Its a hot path since you read many I2C register reads, so getting
rid of that additional check will be good.
^ permalink raw reply
* [PATCH v2 5/7] i2c: omap: wait for transfer completion before sending STP bit
From: Santosh Shilimkar @ 2012-10-25 13:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1351167915-15079-6-git-send-email-balbi@ti.com>
On Thursday 25 October 2012 05:55 PM, Felipe Balbi wrote:
> Later patches will come adding support for
> reporting amount of bytes transferred so that
> client drivers can count how many bytes are
> left to transfer.
>
> This is useful mostly in case of NACKs when
> client driver wants to know exactly which
> byte got NACKed so it doesn't have to resend
> all bytes again.
>
> In order to make that work with OMAP's I2C
> controller, we need to prevent sending STP
> bit until message is transferred. The reason
> behind that is because OMAP_I2C_CNT_REG gets
> reset to original value after STP bit is
> shifted through I2C_SDA line and that would
> prevent us from reading the correct number of
> bytes left to transfer.
>
> The full programming model suggested by IP
> owner was the following:
>
> - start I2C transfer (without STP bit)
> - upon completion or NACK, read I2C_CNT register
> - write STP bit to I2C_CON register
> - wait for ARDY bit
>
> With this patch we're implementing all steps
> except step #2 which will come in a later
> patch adding such support.
>
Will this not break the bisect since CNT and
NACK, completion is added in later patch
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
Apart from above, rest of the change follow
the change log and looks fine tome. The
change is quite drastic so hopefully it
has gone through wider testing.
Regards
santosh
^ permalink raw reply
* [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg
From: Jean Delvare @ 2012-10-25 12:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1350899218-13624-8-git-send-email-balbi@ti.com>
Hi Felipe, Shubhrajyoti,
On Mon, 22 Oct 2012 12:46:57 +0300, Felipe Balbi wrote:
> From: Shubhrajyoti D <shubhrajyoti@ti.com>
>
> In case of a NACK, it's wise to tell our clients
> drivers about how many bytes were actually transferred.
>
> Support this by adding an extra field to the struct
> i2c_msg which gets incremented the amount of bytes
> actually transferred.
>
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
> include/uapi/linux/i2c.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
> index 0e949cb..4b35c9b 100644
> --- a/include/uapi/linux/i2c.h
> +++ b/include/uapi/linux/i2c.h
> @@ -77,6 +77,7 @@ struct i2c_msg {
> #define I2C_M_NO_RD_ACK 0x0800 /* if I2C_FUNC_PROTOCOL_MANGLING */
> #define I2C_M_RECV_LEN 0x0400 /* length will be first received byte */
> __u16 len; /* msg length */
> + __u16 transferred; /* actual bytes transferred */
> __u8 *buf; /* pointer to msg data */
> };
On the principle I am fine with this, however you also need to define
who should initialize this field, and to what value.
Not all i2c bus drivers will implement this functionality at first.
Some may even be unable to ever implement it. As soon as i2c chip
drivers start checking this value, you will hit combinations of chip
driver checking the value and bus driver not setting it. And it has to
work.
We have to decide whether it is enough to initialize "transferred" to
0, or if we need a more reliable way to distinguish between "0 bytes
transferred" and "the bus driver can't tell". What's your take on this?
If we need to distinguish, this could be done using a new I2C_FUNC_*
flag, or by initializing "transferred" to (__u16)(-1) instead of 0 for
example.
Then we have to decide whether initialization is done by the individual
drivers, or by i2c-core. By the drivers might be faster, but this may
also mean more code (and bugs more likely) than letting i2c-core handle
it. The exact balance probably depends on the answer to the previous
question (initializing a field to 0 is free in many cases.)
--
Jean Delvare
^ permalink raw reply
* [PATCH v2 4/7] i2c: omap: in case of VERSION_2 read IRQSTATUS_RAW but write to IRQSTATUS
From: Santosh Shilimkar @ 2012-10-25 12:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1351167915-15079-5-git-send-email-balbi@ti.com>
On Thursday 25 October 2012 05:55 PM, Felipe Balbi wrote:
> on OMAP4+ we want to read IRQSTATUS_RAW register,
> instead of IRQSTATUS. The reason being that IRQSTATUS
> will only contain the bits which were enabled on
> IRQENABLE_SET and that will break when we need to
> poll for a certain bit which wasn't enabled as an
> IRQ source.
>
> One such case is after we finish converting to
> deferred stop bit, we will have to poll for ARDY
> bit before returning control for the client driver
> in order to prevent us from trying to start a
> transfer on a bus which is already busy.
>
> Note, however, that omap-i2c.c needs a big rework
> on register definition and register access. Such
> work will be done in a separate series of patches.
>
> Cc: Benoit Cousson <b-cousson@ti.com>
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
> drivers/i2c/busses/i2c-omap.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index b004126..20f9ad6 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -271,8 +271,18 @@ static inline void omap_i2c_write_reg(struct omap_i2c_dev *i2c_dev,
>
> static inline u16 omap_i2c_read_reg(struct omap_i2c_dev *i2c_dev, int reg)
> {
> - return __raw_readw(i2c_dev->base +
> + /* if we are OMAP_I2C_IP_VERSION_2, we need to read from
> + * IRQSTATUS_RAW, but write to IRQSTATUS
> + */
> + if ((i2c_dev->dtrev == OMAP_I2C_IP_VERSION_2) &&
> + (reg == OMAP_I2C_STAT_REG)) {
Doing this check on every I2C register read seems to
expensive to me. Can you not sort this in init with some offset
which can be 0 or non zero ? Sorry in case this is already dicussed.
regards
santosh
^ permalink raw reply
* [PATCH v2 2/7] i2c: omap: reorder exit path of omap_i2c_xfer_msg()
From: Lothar Waßmann @ 2012-10-25 12:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <50893398.6070004@ti.com>
Hi,
Santosh Shilimkar writes:
> On Thursday 25 October 2012 05:55 PM, Felipe Balbi wrote:
> > just a cleanup patch trying to make exit path
> > more straightforward. No changes otherwise.
> >
> > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > ---
> > drivers/i2c/busses/i2c-omap.c | 26 +++++++++++++++++---------
> > 1 file changed, 17 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> > index c07d9c4..bea0277 100644
> > --- a/drivers/i2c/busses/i2c-omap.c
> > +++ b/drivers/i2c/busses/i2c-omap.c
> > @@ -505,6 +505,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> > {
> > struct omap_i2c_dev *dev = i2c_get_adapdata(adap);
> > unsigned long timeout;
> > + int ret;
[...]
> > + ret = -EREMOTEIO;
> > + goto err;
> > }
> > - return -EIO;
> > +
> > + return 0;
> With initialized value you can use
> return ret;
>
Doing it this way has the advantage, that if an additional error exit
is added it will generate an 'uninitialized variable' warning, if it
fails to set the return value.
Lothar Wa?mann
--
___________________________________________________________
Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996
www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________
^ permalink raw reply
* [PATCH v2 4/7] i2c: omap: in case of VERSION_2 read IRQSTATUS_RAW but write to IRQSTATUS
From: Felipe Balbi @ 2012-10-25 12:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <50893665.60604@ti.com>
Hi,
On Thu, Oct 25, 2012 at 06:23:57PM +0530, Santosh Shilimkar wrote:
> On Thursday 25 October 2012 05:55 PM, Felipe Balbi wrote:
> >on OMAP4+ we want to read IRQSTATUS_RAW register,
> >instead of IRQSTATUS. The reason being that IRQSTATUS
> >will only contain the bits which were enabled on
> >IRQENABLE_SET and that will break when we need to
> >poll for a certain bit which wasn't enabled as an
> >IRQ source.
> >
> >One such case is after we finish converting to
> >deferred stop bit, we will have to poll for ARDY
> >bit before returning control for the client driver
> >in order to prevent us from trying to start a
> >transfer on a bus which is already busy.
> >
> >Note, however, that omap-i2c.c needs a big rework
> >on register definition and register access. Such
> >work will be done in a separate series of patches.
> >
> >Cc: Benoit Cousson <b-cousson@ti.com>
> >Signed-off-by: Felipe Balbi <balbi@ti.com>
> >---
> > drivers/i2c/busses/i2c-omap.c | 12 +++++++++++-
> > 1 file changed, 11 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> >index b004126..20f9ad6 100644
> >--- a/drivers/i2c/busses/i2c-omap.c
> >+++ b/drivers/i2c/busses/i2c-omap.c
> >@@ -271,8 +271,18 @@ static inline void omap_i2c_write_reg(struct omap_i2c_dev *i2c_dev,
> >
> > static inline u16 omap_i2c_read_reg(struct omap_i2c_dev *i2c_dev, int reg)
> > {
> >- return __raw_readw(i2c_dev->base +
> >+ /* if we are OMAP_I2C_IP_VERSION_2, we need to read from
> >+ * IRQSTATUS_RAW, but write to IRQSTATUS
> >+ */
> >+ if ((i2c_dev->dtrev == OMAP_I2C_IP_VERSION_2) &&
> >+ (reg == OMAP_I2C_STAT_REG)) {
> Doing this check on every I2C register read seems to
> expensive to me. Can you not sort this in init with some offset
> which can be 0 or non zero ? Sorry in case this is already dicussed.
could be. I didn't go that route because I'm planning a complete rewrite
of all register accesses. The way it's done today is completely broken
and already expensive (with reg_shift and different map tables and so
on).
If it's really a big of a deal, I can try to find another way, maybe
just adding omap_i2c_read_stat() and limit the version check just to
I2C_STAT reads would do it for now...
--
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121025/d7a764f7/attachment.sig>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox