Re: [PATCH V3 00/18] ARM: OMAP2+: GPMC clean-up and DT update

2013-03-16 Thread Ezequiel Garcia
On Fri, Mar 15, 2013 at 10:20:58AM -0500, Jon Hunter wrote:
> While adding device-tree support for NOR memories, it became apparent
> that there is no common way for configuring various GPMC settings for
> devices that interface to the GPMC. These settings include bus-width,
> synchronous/asynchronous mode, burst settings, wait monitoring etc.
> Therefore, to simplify the GPMC code and add device-tree support for
> NOR, it was first necessary to consolidate how these settings are
> programmed.
> 
> Series based upon Mark Jackson's patch [1] and Ezequiel Garcia GPMC
> clean-up series [2]. Entire series available here [3].
> 
> V3 changes:
> - Rebased on RMK's IS_ERR_VALUE() patch and removed usage of
>   IS_ERR_VALUE() from series.
> - Fixed BUG in NAND code introduced in V2 by making gpmc_settings
>   structure a local variable (I forgot to initialise structure to zero).
> - Added fix from Javier to correct return value from gpmc_probe_nor().
> 
> V2 changes:
> - Made gpmc_settings structure local in gpmc_nand_init().
> - Use resource_size() API in probe_nor().
> - Add kernel-doc for gpmc_cs_program_settings() function.
> - Use of_platform_device_create() to register NOR devices in probe_nor().
> - Add support for GPMC address-address-data multiplexing which required
>   changing GPMC DT property "gpmc,mux-add-data" to store a 32-bit value
>   and changing mux_add_data member of gpmc_settings to be a 32-bit type
>   instead of bool.
> - Add detection for incorrect GPMC chip-select base addresses.
> - Cleaned-up code in gpmc_mem_init() and changed gpmc_mem_init() so that
>   it would not return an error when the GPMC driver is being probed.
> 
> V1 changes:
> - Clean-up/simplification of ONENAND initialisation code.
> - Add a new GPMC structure to unify storage of various GPMC settings
>   (that are non-timing related) for client devices and add a new
>   function to program these settings in a common way.
> - Migrate initialisation code for existing flash, usb and networking
>   devices to use the new structure and function for GPMC settings.
> - Add device-tree support for NOR flash memories.
> - Add additional GPMC timing parameters to GPMC device-tree binding.
> - Update GPMC NAND and ONENAND device-tree support to retrieve GPMC
>   settings from device-tree.
> 
> Testing includes:
> - Boot testing on OMAP2420 H4, OMAP3430 SDP and OMAP4430 SDP with
>   and without device-tree present.
> - OMAP2420 H4 board has NOR flash and OMAP3430 SDP has NOR, NAND
>   and ONENAND flash. So verified that flash is detected on boot
>   as expected. Note additional patches [4] are required for OMAP2420
>   H4 and OMAP3430 SDP dts files in order to enable flash memory
>   support.
> - All of the above boards use GPMC for interfacing to a networking
>   chip and so verified that networking is working wit this series.
>   However, please note that networking is not currently supported
>   on these boards when booting with DT and so networking is only
>   tested without DT.
> 
> [1] http://permalink.gmane.org/gmane.linux.ports.arm.omap/94765
> [2] http://comments.gmane.org/gmane.linux.ports.arm.omap/93784
> [3] https://github.com/jonhunter/linux/tree/omap-gpmc-for-v3.10
> [4] https://github.com/jonhunter/linux/tree/omap-dt-for-v3.10
> 
> Javier Martinez Canillas (1):
>   ARM: OMAP2+: return -ENODEV if GPMC child device creation fails
> 
> Jon Hunter (17):
>   ARM: OMAP2+: Simplify code configuring ONENAND devices
>   ARM: OMAP2+: Add variable to store number of GPMC waitpins
>   ARM: OMAP2+: Add structure for storing GPMC settings
>   ARM: OMAP2+: Add function for configuring GPMC settings
>   ARM: OMAP2+: Convert ONENAND to use gpmc_cs_program_settings()
>   ARM: OMAP2+: Convert NAND to use gpmc_cs_program_settings()
>   ARM: OMAP2+: Convert SMC91x to use gpmc_cs_program_settings()
>   ARM: OMAP2+: Convert TUSB to use gpmc_cs_program_settings()
>   ARM: OMAP2+: Don't configure of chip-select options in
> gpmc_cs_configure()
>   ARM: OMAP2+: Add function to read GPMC settings from device-tree
>   ARM: OMAP2+: Add device-tree support for NOR flash
>   ARM: OMAP2+: Add additional GPMC timing parameters
>   ARM: OMAP2+: Convert NAND to retrieve GPMC settings from DT
>   ARM: OMAP2+: Convert ONENAND to retrieve GPMC settings from DT
>   ARM: OMAP2+: Detect incorrectly aligned GPMC base address
>   ARM: OMAP2+: Remove unnecesssary GPMC definitions and variable
>   ARM: OMAP2+: Allow GPMC probe to complete even if CS mapping fails
> 
>  Documentation/devicetree/bindings/bus/ti-gpmc.txt  |   48 +-
>  Documentation/devicetree/bindings/mtd/gpmc-nor.txt |   98 
>  .../devicetree/bindings/mtd/gpmc-onenand.txt   |3 +
>  arch/arm/mach-omap2/gpmc-nand.c|   39 +-
>  arch/arm/mach-omap2/gpmc-onenand.c |  110 ++--
>  arch/arm/mach-omap2/gpmc-smc91x.c  |   30 +-
>  arch/arm/mach-omap2/gpmc.c |  524 
> +++-
>  arch/arm/mach-omap2/gpmc.h 

Re: [PATCH V3 10/18] ARM: OMAP2+: Add function to read GPMC settings from device-tree

2013-03-16 Thread Ezequiel Garcia
Hi Jon,

I have some tiny nitpicks...

On Fri, Mar 15, 2013 at 10:21:08AM -0500, Jon Hunter wrote:
> Adds a function to read the various GPMC chip-select settings from
> device-tree and store them in the gpmc_settings structure.
> 
> Update the GPMC device-tree binding documentation to describe these
> options.
> 
> Signed-off-by: Jon Hunter 
> ---
>  Documentation/devicetree/bindings/bus/ti-gpmc.txt |   23 ++
>  arch/arm/mach-omap2/gpmc.c|   49 
> +
>  arch/arm/mach-omap2/gpmc.h|2 +
>  3 files changed, 74 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/bus/ti-gpmc.txt 
> b/Documentation/devicetree/bindings/bus/ti-gpmc.txt
> index 5ddb2e9..6fde1cf 100644
> --- a/Documentation/devicetree/bindings/bus/ti-gpmc.txt
> +++ b/Documentation/devicetree/bindings/bus/ti-gpmc.txt
> @@ -65,6 +65,29 @@ The following are only applicable to OMAP3+ and AM335x:
>   - gpmc,wr-access
>   - gpmc,wr-data-mux-bus
>  
> +GPMC chip-select settings properties for child nodes. All are optional.
> +
> +- gpmc,burst-length  Page/burst length. Must be 4, 8 or 16.
> +- gpmc,burst-wrapEnables wrap bursting
> +- gpmc,burst-readEnables read page/burst mode
> +- gpmc,burst-write   Enables write page/burst mode
> +- gpmc,device-nand   Device is NAND
> +- gpmc,device-width  Total width of device(s) connected to a GPMC
> + chip-select in bytes. The GPMC supports 8-bit
> + and 16-bit devices and so this property must be
> + 1 or 2.
> +- gpmc,mux-add-data  Address and data multiplexing configuration.
> + Valid values are 1 for address-address-data
> + multiplexing mode and 2 for address-data
> + multiplexing mode.
> +- gpmc,sync-read Enables synchronous read. Defaults to asynchronous
> + is this is not set.
> +- gpmc,sync-writeEnables synchronous writes. Defaults to asynchronous
> + is this is not set.
> +- gpmc,wait-pin  Wait-pin used by client. Must be less than
> + "gpmc,num-waitpins".
> +- gpmc,wait-on-read  Enables wait monitoring on reads.
> +- gpmc,wait-on-write Enables wait monitoring on writes.
>  
>  Example for an AM33xx board:
>  
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index 3ec1937..1e7eef3 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -1190,6 +1190,55 @@ static struct of_device_id gpmc_dt_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(of, gpmc_dt_ids);
>  
> +/**
> + * gpmc_read_settings_dt - read gpmc settings from device-tree
> + * @np:  pointer to device-tree node for a gpmc child device
> + * @p:   pointer to gpmc settings structure
> + *
> + * Reads the GPMC settings for a GPMC child device from device-tree and
> + * stores them in the GPMC settings structure passed. The GPMC settings
> + * structure is initialise to zero by this function and so any previously

s/initialise/initialized ?

> + * stored settings will be clearer.

s/clearer/cleared ?

I'm not an english native speaker,
so please bare with me if I'm wrong on these...

-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
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 V4] ARM: dts: add minimal DT support for DevKit8000.

2013-03-16 Thread Anil Kumar
Hi Benoit,

On Thu, Mar 7, 2013 at 12:21 PM, Benoit Cousson  wrote:
> Hi,
>
> On 03/06/2013 06:53 PM, Tony Lindgren wrote:
>> * Anil Kumar  [130305 18:40]:
>>> Hi Tony,
>>>
> From: linux-arm-kernel [mailto:linux-arm-kernel-
> boun...@lists.infradead.org] On Behalf Of Anil Kumar
> Sent: Wednesday, February 27, 2013 8:03 AM
> To: devicetree-disc...@lists.ozlabs.org; linux-omap@vger.kernel.org;
> linux-ker...@vger.kernel.org; linux-arm-ker...@lists.infradead.org
> Cc: li...@arm.linux.org.uk; Cousson, Benoit; Tony Lindgren; Grant
> Likely; Anil Kumar; tho...@tomweber.eu
> Subject: Re: FW: [PATCH V4] ARM: dts: add minimal DT support for
> DevKit8000.
>
> Hi,
>>
>> DevKit8000 is a beagle board clone from Timll, sold by
>> armkits.com. The DevKit8000 has RS232 serial port, LCD, DVI-D,
>> S-Video, Ethernet, SD/MMC, keyboard, camera, SPI, I2C, USB and
>> JTAG interface.
>>
>> This patch adds the basic DT support for devkit8000. At this time,
> Information
>> of twl4030 (PMIC), MMC1, I2C1 and leds are added.
>>
>> Signed-off-by: Anil Kumar 
>> Tested-by: Thomas Weber 
>
> Gentle Ping. As there are no review comments on this patch,
> Could you please pull this patch ?
>>>
>>> Gentle Ping.
>>>
>>> This patch also got Reviewed-by: Manish Badarkhe 
>>> and as patch "ARM: dts: omap3-devkit8000: Enable audio support"  has 
>>> already got
>>> "Acked-by: Peter Ujfalusi " but it is on the
>>> top of this patch.
>>> So, Could you please pull this patch in one of your omap branch? or
>>> you want me to
>>> do rebase this patch on top of v3.9-rc1 ?
>>
>> Let's wait for Benoit to queue it as he has a bunch of .dts changes already
>> applied. Too bad we missed v3.9 merge window for those, but hopefully
>> we can get them queued early for v3.10.
>
> Yep, sorry for having missed 3.9, I was a little bit sick at the wrong
> moment :-(
>
> I'm starting queuing the pending patches, and should have enough to push
> to you just after Linaro Connect.

I think you missed below patch which is needed for gpmc nand to work fine.

Jon Hunter:-
  ARM: OMAP2+: Fix-up gpmc merge error

I have re based my changes on top of your repository to make pull
easier for you.
I hope you will pull these changes for 3.10.

The following changes since Commit a7f7881de9c0b58de3b3aea8f01a8aef906d4ade

are available in the git repository at:

git://gitorious.org/devkit8000-for_3-10/devkit8000-for_3-10.git
branch for_3.10/dts

for you to fetch changes up to Commit 975abc8b2e7ec4ff7324d726c9775958945ccc5e

Anil Kumar:
ARM: dts: add minimal DT support for DevKit8000
ARM: dts: omap3-devkit8000: Enable audio support
ARM: dts: omap3-devkit8000: add nand dt node

Thanks,
Anil
--
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 02/18] ARM: OMAP2+: Add variable to store number of GPMC waitpins

2013-03-16 Thread Ezequiel Garcia
Hi Jon,

On Fri, Mar 15, 2013 at 10:21:00AM -0500, Jon Hunter wrote:
> The GPMC has wait-pin signals that can be assigned to a chip-select
> to monitor the ready signal of an external device. Add a variable to
> indicate the total number of wait-pins for a given device. This will
> allow us to detect if the wait-pin being selected is valid or not.
> 
> When booting with device-tree read the number of wait-pins from the
> device-tree blob. When device-tree is not used set the number of
> wait-pins to 4 which is valid for OMAP2-5 devices. Newer devices
> that have less wait-pins (such as AM335x) only support booting with
> device-tree and so hard-coding the wait-pin number when not using
> device-tree is fine.
> 
> Signed-off-by: Jon Hunter 
> ---
>  arch/arm/mach-omap2/gpmc.c |   16 
>  1 file changed, 16 insertions(+)
> 
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index ef655d9..88a261c 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -108,6 +108,8 @@
>  #define  GPMC_HAS_WR_ACCESS  0x1
>  #define  GPMC_HAS_WR_DATA_MUX_BUS0x2
>  
> +#define GPMC_NR_WAITPINS 4
> +
>  /* XXX: Only NAND irq has been considered,currently these are the only ones 
> used
>   */
>  #define  GPMC_NR_IRQ 2
> @@ -153,6 +155,7 @@ static struct resourcegpmc_cs_mem[GPMC_CS_NUM];
>  static DEFINE_SPINLOCK(gpmc_mem_lock);
>  /* Define chip-selects as reserved by default until probe completes */
>  static unsigned int gpmc_cs_map = ((1 << GPMC_CS_NUM) - 1);
> +static unsigned int gpmc_nr_waitpins;
>  static struct device *gpmc_dev;
>  static int gpmc_irq;
>  static resource_size_t phys_base, mem_size;
> @@ -1297,6 +1300,13 @@ static int gpmc_probe_dt(struct platform_device *pdev)
>   if (!of_id)
>   return 0;
>  
> + ret = of_property_read_u32(pdev->dev.of_node, "gpmc,num-waitpins",
> +&gpmc_nr_waitpins);
> + if (ret < 0) {
> + pr_err("%s: number of wait pins not found!\n", __func__);
> + return ret;
> + }
> +
>   for_each_node_by_name(child, "nand") {
>   ret = gpmc_probe_nand_child(pdev, child);
>   if (ret < 0) {
> @@ -1372,6 +1382,12 @@ static int gpmc_probe(struct platform_device *pdev)
>   if (gpmc_setup_irq() < 0)
>   dev_warn(gpmc_dev, "gpmc_setup_irq failed\n");
>  
> + /* Now the GPMC is initialised, unreserve the chip-selects */
> + gpmc_cs_map = 0;

The above seems to be a remanent of another patch.
I think you already sent that one, right?

-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
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


commit 6797b4f causes boot hangs with nfsroot

2013-03-16 Thread Paul Walmsley

Hi Jon,

Looks like commit 6797b4fe0e554ce71f47038fd929c9ca929a9f3c ("ARM: OMAP2+: 
Prevent potential crash if GPMC probe fails") causes hangs on boot.  I 
suspect it may only occur when the root filesystem is on NFS over 
GPMC-based Ethernet.

Here's the current state of affairs at v3.9-rc1 on the two boards that 
show the problem:

http://www.pwsan.com/omap/testlogs/test_v3.9-rc1/20130312100243/boot/2430sdp/2430sdp_log.txt
http://www.pwsan.com/omap/testlogs/test_v3.9-rc1/20130312100243/boot/37xxevm/37xxevm_log.txt

and here are the bootlogs after reverting 6797b4f:

http://www.pwsan.com/omap/testlogs/trace_boot_hang_3.9-rc/20130316132833/boot/2430sdp/2430sdp_log.txt
http://www.pwsan.com/omap/testlogs/trace_boot_hang_3.9-rc/20130316132833/boot/37xxevm/37xxevm_log.txt

... where the boards at least boot to userspace.  I don't know what's up 
with the NFS-related lock problems.  Those look like yet another bug that 
is presumably not OMAP-specific :-(

Care to take a look at the commit 6797b4f bug?


- Paul
--
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 00/50] staging: omap-thermal: several code refactoring

2013-03-16 Thread Greg KH
On Sat, Mar 16, 2013 at 08:46:03AM -0400, Eduardo Valentin wrote:
> Hello Dan,
> 
> On 16-03-2013 05:05, Dan Carpenter wrote:
> >I've reviewed this set.
> >
> >I hate to make people redo whole patchset sets, and I hate
> >re-reviewing code.  Obviously, I don't really like the bunny hop
> >patches and I'm trying to discourage that going forward.  ;P  But
> >I wouldn't say it's a "Redo the whole thing" kind of problem.
> >
> >Could just resend patch 33 and 47?  You should probably be able to
> >redo those without changing the rest.
> 
> I could of course change them if the comment is better clarified. As
> I mentioned as reply to one of your comments, those changes are
> following what is suggested in CodingStyle file.
> 
> I can of course send a diff on top of 33, to fix the introduce bug.
> 
> For 47, I'm not sure the comment is fully applicable.

As I've taken all of these already (sorry Dan, I was fast and I didn't
review them as well as you did), you will have to just send incremental
patches on top of the whole series in order for me to be able to apply
them.

thanks,

greg k-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: [PATCH 47/50] staging: omap-thermal: switch mutex to spinlock inside omap-bandgap

2013-03-16 Thread Dan Carpenter
On Sat, Mar 16, 2013 at 08:41:30AM -0400, Eduardo Valentin wrote:
> On 16-03-2013 04:59, Dan Carpenter wrote:
> >On Fri, Mar 15, 2013 at 09:00:35AM -0400, Eduardo Valentin wrote:
> >>@@ -502,9 +504,9 @@ int _omap_bandgap_write_threshold(struct omap_bandgap 
> >>*bg_ptr, int id, int val,
> >>if (ret < 0)
> >>goto exit;
> >>
> >>-   mutex_lock(&bg_ptr->bg_mutex);
> >>+   spin_lock(&bg_ptr->lock);
> >
> >These need to disable interrupts because we take the spin lock in
> >the IRQ handler.
> 
> This IRQ gets masked at the IRQ controller level when served
> (ONE_SHOT). Not sure if your comment is applicable in this case..

Yes.  It still applies.  We need to disable IRQs from the current
CPU while we are holding a spin_lock which they will need.

regards,
dan carpenter

--
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 07/50] staging: omap-thermal: introduce RMW_BITS macro

2013-03-16 Thread Dan Carpenter
On Sat, Mar 16, 2013 at 08:36:51AM -0400, Eduardo Valentin wrote:
> >But that said, I don't care for the RMW_BITS() very much as a long
> >term thing.  If we just used pointers instead of passing the offset
> >into the bg_ptr->conf->sensors[] array then everything would be a
> >lot cleaner.
> >
> >In other words, instead of this:
> >
> >static u32 omap_bandgap_read_temp(struct omap_bandgap *bg_ptr, int id)
> >
> >We would have:
> >
> >static u32 omap_bandgap_read_temp(struct omap_bandgap *bg_ptr,
> >   struct temp_sensor_registers *tsr)
> 
> I see. That will require a change in the whole driver though. If you
> see, the driver as of today uses the former approach, not only for
> read_temp or rmw_bits.

Yep.

> 
> >
> >If you have the pointer then it's easy write RMW_BITS() as a
> >function.
> >
> >static void rmw_bits(struct omap_bandgap *bg_ptr, u32 reg, u32 mask, u32 val)
> >{
> > u32 r;
> >
> > r = omap_bandgap_readl(bg_ptr, reg);
> > r &= ~mask;
> > r |= val << __ffs(mask);
> > omap_bandgap_writel(bg_ptr, r, reg);
> >}
> >
> >It's called like:
> >
> > rmw_bits(bg_ptr, tsr->bgap_mask_ctrl, tsr->mask_freeze_mask, 1);
> 
> This is nice, but it will require fetching tsr from .conf before
> every call o rmw_bits. And for that you need the sensor index.
> 

No.  I'm suggesting that you re-write the driver to pass the tsr
pointer directly instead of the index.

regards,
dan carpenter

--
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 33/50] staging: omap-thermal: refactor APIs handling threshold values

2013-03-16 Thread Dan Carpenter
On Sat, Mar 16, 2013 at 08:49:20AM -0400, Eduardo Valentin wrote:
> On 16-03-2013 04:39, Dan Carpenter wrote:
> >On Fri, Mar 15, 2013 at 09:00:21AM -0400, Eduardo Valentin wrote:
> >>if (ret) {
> >>dev_err(bg_ptr->dev, "failed to read thot\n");
> >>-   return -EIO;
> >>+   ret = -EIO;
> >>+   goto exit;
> >>}
> >>
> >>-   *thot = temp;
> >>+   *val = temp;
> >>
> >>+exit:
> >>return 0;
> >>  }
> >
> >
> >The bunny hop has introduced a bug and this always returns success.
> 
> What was the bug introduced?
> 
> I will send a patch to fix the return value.

Returning the wrong value *is* the bug.

regards,
dan carpenter

--
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 09/50] staging: omap-thermal: make a omap_bandgap_power with only one exit point

2013-03-16 Thread Dan Carpenter
On Sat, Mar 16, 2013 at 08:39:11AM -0400, Eduardo Valentin wrote:
> Hey Dan,
> 
> On 15-03-2013 17:22, Dan Carpenter wrote:
> >On Fri, Mar 15, 2013 at 08:59:57AM -0400, Eduardo Valentin wrote:
> >>Change the way the omap_bandgap_power is written so that it has only
> >>one exit entry (Documentation/CodingStyle).
> >>
> >
> >It's only if there is an unlock or something that you should do
> >this.  Otherwise the pointless bunny hop is misleading and annoying.
> 
> Well, if that is the case the Chapter 7 needs to be rewritten, don't
> you think? The way it is stated, it is clear that it is a design
> decision to use it for keeping only one exit point (quoting):
> 
> "Albeit deprecated by some people, the equivalent of the goto statement is
> used frequently by compilers in form of the unconditional jump instruction.
> 
> The goto statement comes in handy when a function exits from multiple
> locations and some common work such as cleanup has to be done.
> 
> The rationale is:
> 
> - unconditional statements are easier to understand and follow
> - nesting is reduced
> - errors by not updating individual exit points when making
> modifications are prevented
> - saves the compiler work to optimize redundant code away ;)"
> 
> I believe this patch falls into at least three of the above rationale.

There isn't any cleanup work done.  That's what I was explaining
about.  When I see a goto I expect cleanup work to be done, but in
this case it was misleading.

Where you would do the one exit point is when there is cleanup:

ret = function_one();
if (ret)
goto unlock;

ret = function_two();
if (ret)
goto unlock;


That's better than:
ret = function_one();
if (ret) {
mutex_unlock();
return ret;
}

ret = function_two();
if (ret) {
mutex_unlock();
return ret;
}

On the one hand, I think the documentation should be updated because
obviously people get confused by it.  But on the other hand, to me
the documentation is already pretty clear.  There is no style
guideline that says every function should only have a single return.
That would be silly.

regards,
dan carpenter

--
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 33/50] staging: omap-thermal: refactor APIs handling threshold values

2013-03-16 Thread Eduardo Valentin

On 16-03-2013 04:39, Dan Carpenter wrote:

On Fri, Mar 15, 2013 at 09:00:21AM -0400, Eduardo Valentin wrote:

if (ret) {
dev_err(bg_ptr->dev, "failed to read thot\n");
-   return -EIO;
+   ret = -EIO;
+   goto exit;
}

-   *thot = temp;
+   *val = temp;

+exit:
return 0;
  }



The bunny hop has introduced a bug and this always returns success.


What was the bug introduced?

I will send a patch to fix the return value.



regards,
dan carpenter





--
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 00/50] staging: omap-thermal: several code refactoring

2013-03-16 Thread Eduardo Valentin

Hello Dan,

On 16-03-2013 05:05, Dan Carpenter wrote:

I've reviewed this set.

I hate to make people redo whole patchset sets, and I hate
re-reviewing code.  Obviously, I don't really like the bunny hop
patches and I'm trying to discourage that going forward.  ;P  But
I wouldn't say it's a "Redo the whole thing" kind of problem.

Could just resend patch 33 and 47?  You should probably be able to
redo those without changing the rest.


I could of course change them if the comment is better clarified. As I 
mentioned as reply to one of your comments, those changes are following 
what is suggested in CodingStyle file.


I can of course send a diff on top of 33, to fix the introduce bug.

For 47, I'm not sure the comment is fully applicable.



regards,
dan carpenter





--
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 47/50] staging: omap-thermal: switch mutex to spinlock inside omap-bandgap

2013-03-16 Thread Eduardo Valentin

On 16-03-2013 04:59, Dan Carpenter wrote:

On Fri, Mar 15, 2013 at 09:00:35AM -0400, Eduardo Valentin wrote:

Because there is a need to lock inside IRQ handler, this patch
changes the locking mechanism inside the omap-bandgap.[c,h] to
spinlocks. Now this lock is used to protect omap_bandgap struct
during APIs exposed (possibly used in sysfs handling functions)
and inside the ALERT IRQ handler.

Because there are registers shared among the sensors, this lock
is global, not per sensor.

Signed-off-by: Eduardo Valentin 
---
  drivers/staging/omap-thermal/TODO   |1 -
  drivers/staging/omap-thermal/omap-bandgap.c |   18 ++
  drivers/staging/omap-thermal/omap-bandgap.h |4 ++--
  3 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/omap-thermal/TODO 
b/drivers/staging/omap-thermal/TODO
index 77b761b..0f24e9b 100644
--- a/drivers/staging/omap-thermal/TODO
+++ b/drivers/staging/omap-thermal/TODO
@@ -1,7 +1,6 @@
  List of TODOs (by Eduardo Valentin)

  on omap-bandgap.c:
-- Rework locking
  - Improve driver code by adding usage of regmap-mmio
  - Test every exposed API to userland
  - Add support to hwmon
diff --git a/drivers/staging/omap-thermal/omap-bandgap.c 
b/drivers/staging/omap-thermal/omap-bandgap.c
index 4b631fd..846ced6 100644
--- a/drivers/staging/omap-thermal/omap-bandgap.c
+++ b/drivers/staging/omap-thermal/omap-bandgap.c
@@ -33,7 +33,7 @@
  #include 
  #include 
  #include 
-#include 
+#include 
  #include 
  #include 
  #include 
@@ -170,6 +170,7 @@ static irqreturn_t omap_bandgap_talert_irq_handler(int irq, 
void *data)
u32 t_hot = 0, t_cold = 0, ctrl;
int i;

+   spin_lock(&bg_ptr->lock);
for (i = 0; i < bg_ptr->conf->sensor_count; i++) {
tsr = bg_ptr->conf->sensors[i].registers;
ctrl = omap_bandgap_readl(bg_ptr, tsr->bgap_status);
@@ -208,6 +209,7 @@ static irqreturn_t omap_bandgap_talert_irq_handler(int irq, 
void *data)
if (bg_ptr->conf->report_temperature)
bg_ptr->conf->report_temperature(bg_ptr, i);
}
+   spin_unlock(&bg_ptr->lock);

return IRQ_HANDLED;
  }
@@ -502,9 +504,9 @@ int _omap_bandgap_write_threshold(struct omap_bandgap 
*bg_ptr, int id, int val,
if (ret < 0)
goto exit;

-   mutex_lock(&bg_ptr->bg_mutex);
+   spin_lock(&bg_ptr->lock);


These need to disable interrupts because we take the spin lock in
the IRQ handler.


This IRQ gets masked at the IRQ controller level when served (ONE_SHOT). 
Not sure if your comment is applicable in this case..




regards,
dan carpenter






--
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 09/50] staging: omap-thermal: make a omap_bandgap_power with only one exit point

2013-03-16 Thread Eduardo Valentin

Hey Dan,

On 15-03-2013 17:22, Dan Carpenter wrote:

On Fri, Mar 15, 2013 at 08:59:57AM -0400, Eduardo Valentin wrote:

Change the way the omap_bandgap_power is written so that it has only
one exit entry (Documentation/CodingStyle).



It's only if there is an unlock or something that you should do
this.  Otherwise the pointless bunny hop is misleading and annoying.


Well, if that is the case the Chapter 7 needs to be rewritten, don't you 
think? The way it is stated, it is clear that it is a design decision to 
use it for keeping only one exit point (quoting):


"Albeit deprecated by some people, the equivalent of the goto statement is
used frequently by compilers in form of the unconditional jump instruction.

The goto statement comes in handy when a function exits from multiple
locations and some common work such as cleanup has to be done.

The rationale is:

- unconditional statements are easier to understand and follow
- nesting is reduced
- errors by not updating individual exit points when making
modifications are prevented
- saves the compiler work to optimize redundant code away ;)"

I believe this patch falls into at least three of the above rationale.




regards,
dan carpenter





--
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 07/50] staging: omap-thermal: introduce RMW_BITS macro

2013-03-16 Thread Eduardo Valentin

Hello Dan,

On 15-03-2013 17:09, Dan Carpenter wrote:

On Fri, Mar 15, 2013 at 08:59:55AM -0400, Eduardo Valentin wrote:

This patch introduce a macro to read, update, write bitfields.
It will be specific to bandgap data structures.

Signed-off-by: Eduardo Valentin 
---
  drivers/staging/omap-thermal/omap-bandgap.c |  178 +++
  1 files changed, 46 insertions(+), 132 deletions(-)

diff --git a/drivers/staging/omap-thermal/omap-bandgap.c 
b/drivers/staging/omap-thermal/omap-bandgap.c
index 9f2d7cc..1c1b905 100644
--- a/drivers/staging/omap-thermal/omap-bandgap.c
+++ b/drivers/staging/omap-thermal/omap-bandgap.c
@@ -52,25 +52,29 @@ static void omap_bandgap_writel(struct omap_bandgap 
*bg_ptr, u32 val, u32 reg)
writel(val, bg_ptr->base + reg);
  }

+/* update bits, value will be shifted */
+#define RMW_BITS(bg_ptr, id, reg, mask, val)   \
+do {   \
+   struct temp_sensor_registers *t;\
+   u32 r;  \
+   \
+   t = bg_ptr->conf->sensors[(id)].registers;\
+   r = omap_bandgap_readl(bg_ptr, t->reg);  \
+   r &= ~t->mask;   \
+   r |= (val) << __ffs(t->mask);  \
+   omap_bandgap_writel(bg_ptr, r, t->reg);  \
+} while (0)
+
  static int omap_bandgap_power(struct omap_bandgap *bg_ptr, bool on)
  {
-   struct temp_sensor_registers *tsr;
int i;
-   u32 ctrl;

if (!OMAP_BANDGAP_HAS(bg_ptr, POWER_SWITCH))
return 0;

-   for (i = 0; i < bg_ptr->conf->sensor_count; i++) {
-   tsr = bg_ptr->conf->sensors[i].registers;
-   ctrl = omap_bandgap_readl(bg_ptr, tsr->temp_sensor_ctrl);
-   ctrl &= ~tsr->bgap_tempsoff_mask;
+   for (i = 0; i < bg_ptr->conf->sensor_count; i++)
/* active on 0 */
-   ctrl |= !on << __ffs(tsr->bgap_tempsoff_mask);
-
-   /* write BGAP_TEMPSOFF should be reset to 0 */
-   omap_bandgap_writel(bg_ptr, ctrl, tsr->temp_sensor_ctrl);
-   }
+   RMW_BITS(bg_ptr, i, temp_sensor_ctrl, bgap_tempsoff_mask, !on);

return 0;
  }
@@ -78,15 +82,13 @@ static int omap_bandgap_power(struct omap_bandgap *bg_ptr, 
bool on)
  static u32 omap_bandgap_read_temp(struct omap_bandgap *bg_ptr, int id)
  {


This patch is fine and it makes it cleaner than before.

But that said, I don't care for the RMW_BITS() very much as a long
term thing.  If we just used pointers instead of passing the offset
into the bg_ptr->conf->sensors[] array then everything would be a
lot cleaner.

In other words, instead of this:

static u32 omap_bandgap_read_temp(struct omap_bandgap *bg_ptr, int id)

We would have:

static u32 omap_bandgap_read_temp(struct omap_bandgap *bg_ptr,
  struct temp_sensor_registers *tsr)


I see. That will require a change in the whole driver though. If you 
see, the driver as of today uses the former approach, not only for 
read_temp or rmw_bits.




If you have the pointer then it's easy write RMW_BITS() as a
function.

static void rmw_bits(struct omap_bandgap *bg_ptr, u32 reg, u32 mask, u32 val)
{
u32 r;

r = omap_bandgap_readl(bg_ptr, reg);
r &= ~mask;
r |= val << __ffs(mask);
omap_bandgap_writel(bg_ptr, r, reg);
}

It's called like:

rmw_bits(bg_ptr, tsr->bgap_mask_ctrl, tsr->mask_freeze_mask, 1);


This is nice, but it will require fetching tsr from .conf before every 
call o rmw_bits. And for that you need the sensor index.




regards,
dan carpenter


Thanks for your time reviewing this patch and suggesting improvements.








--
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: Getting kernel uImage build issue on omap2+

2013-03-16 Thread Anil Kumar
Hi Javier,

On Sat, Mar 16, 2013 at 2:53 PM, Javier Martinez Canillas
 wrote:
> On Sat, Mar 16, 2013 at 5:44 AM, Anil Kumar  wrote:
>> Hi,
>>
>> I am getting kernel uImage build issue on omap2+ log[1]
>>
>> Taken kernel branch "for_3.10/dts" from
>> https://git.kernel.org/pub/scm/linux/kernel/git/bcousson/linux-omap-dt.git
>>
>> Taking reference from
>> https://kernel.googlesource.com/pub/scm/linux/kernel/git/tmlind/linux-omap/+/omap-for-v3.9/multiplatform-enable-signed-v2
>>
>> Am I missing some thing ?
>>
>> [1]
>> anil@anil-laptop:~/Anil/omap3/bcousson$  mkimage -A arm -O linux -T
>> kernel -C none -a 0x80008000 -e 0x80008000 -n "Linux" -d
>> zImage-omap2plus uImage-omap2plus
>> mkimage: Can't open zImage-omap2plus: No such file or directory
>> anil@anil-laptop:~/Anil/omap3/bcousson$
>>
>> Thanks,
>> Anil
>>
>
> Hi Anil,
>
> It seems that Tony's email assumed that you generated a bunch of
> zImages for different platforms and then naming them zImage-$platform.
>
> e.g:
>
> $ make ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- omap2plus_defconfig
> $ make -j 4 ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- zImage modules
> zImage-omap2plus
> $ cp cp arch/arm/boot/zImage zImage-omap2plus
>
> and then you can use the command in [1]:
>
> $  mkimage -A arm -O linux -T kernel -C none -a 0x80008000 -e
> 0x80008000 -n "Linux" -d zImage-omap2plus uImage-omap2plus
>
> anyways, the problem is that zImage-omap2plus does not exist and you
> have to use the zImage generated by "make zImage". What I usually do
> is just:
>
> $ mkimage -A arm -O linux -T kernel -C none -a 0x80008000 -e
> 0x80008000 -n "Linux" -d arch/arm/boot/zImage uImage-omap2plus
>
> and then copy uImage-omap2plus as uImage on either my board MMC/SD or
> Flash memory.

Thanks, It solved the issue
Anil
--
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] OF: Export all DT proc update functions

2013-03-16 Thread Grant Likely
On Fri,  4 Jan 2013 21:31:07 +0200, Pantelis Antoniou 
 wrote:
> There are other users for the proc DT functions.
> Export them.
> 
> Signed-off-by: Pantelis Antoniou 

Hi Pantelis.

Patches 1 & 2 look good. No comments there.

This patch bothers me. The manipulation of the proc entries is part and
parcel of adding and removing nodes. The real problem seems to be that
the node addition/removal APIs need to be made usable by the overlay
code.

g.
--
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] OF: Export all DT proc update functions

2013-03-16 Thread Grant Likely
On Fri,  4 Jan 2013 21:31:07 +0200, Pantelis Antoniou 
 wrote:
> There are other users for the proc DT functions.
> Export them.
> 
> Signed-off-by: Pantelis Antoniou 

Actually, I cannot find the user of this patch. Why is it needed?

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


Re: [PATCH 5/6] OF: Introduce Device Tree resolve support.

2013-03-16 Thread Grant Likely
On Wed, 23 Jan 2013 12:58:02 +0200, Pantelis Antoniou 
 wrote:
> Hi David,
> 
> On Jan 23, 2013, at 6:40 AM, David Gibson wrote:
> > Ok.  Nonetheless it's not hard to avoid a recursive approach here.
> 
> How can I find the maximum phandle value of a subtree without using recursion.
> Note that the whole function is just 6 lines long.

It's a failure in the existing kernel DT data structures. We need a hash
lookup for the phandles to eliminate the search entirely. Then you'd be
able to allocated new phandles on the fly easily and resolve phandles
without searching the whole tree (which has always been horrible).

That said, I'd like to punt on the whole phandle resolution thing. The
DT overlay support can be merged without the phandle resolution support
if the core rejects any overlays with phandle collisions.

g.
--
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: Getting kernel uImage build issue on omap2+

2013-03-16 Thread Javier Martinez Canillas
On Sat, Mar 16, 2013 at 5:44 AM, Anil Kumar  wrote:
> Hi,
>
> I am getting kernel uImage build issue on omap2+ log[1]
>
> Taken kernel branch "for_3.10/dts" from
> https://git.kernel.org/pub/scm/linux/kernel/git/bcousson/linux-omap-dt.git
>
> Taking reference from
> https://kernel.googlesource.com/pub/scm/linux/kernel/git/tmlind/linux-omap/+/omap-for-v3.9/multiplatform-enable-signed-v2
>
> Am I missing some thing ?
>
> [1]
> anil@anil-laptop:~/Anil/omap3/bcousson$  mkimage -A arm -O linux -T
> kernel -C none -a 0x80008000 -e 0x80008000 -n "Linux" -d
> zImage-omap2plus uImage-omap2plus
> mkimage: Can't open zImage-omap2plus: No such file or directory
> anil@anil-laptop:~/Anil/omap3/bcousson$
>
> Thanks,
> Anil
>

Hi Anil,

It seems that Tony's email assumed that you generated a bunch of
zImages for different platforms and then naming them zImage-$platform.

e.g:

$ make ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- omap2plus_defconfig
$ make -j 4 ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- zImage modules
zImage-omap2plus
$ cp cp arch/arm/boot/zImage zImage-omap2plus

and then you can use the command in [1]:

$  mkimage -A arm -O linux -T kernel -C none -a 0x80008000 -e
0x80008000 -n "Linux" -d zImage-omap2plus uImage-omap2plus

anyways, the problem is that zImage-omap2plus does not exist and you
have to use the zImage generated by "make zImage". What I usually do
is just:

$ mkimage -A arm -O linux -T kernel -C none -a 0x80008000 -e
0x80008000 -n "Linux" -d arch/arm/boot/zImage uImage-omap2plus

and then copy uImage-omap2plus as uImage on either my board MMC/SD or
Flash memory.

Also, if you find this inconvenient, you can add CONFIG_CMD_BOOTZ to
your board header config file in U-Boot so it can boot a zImage
directly instead of an uImage. Not sure when did was introduced on
U-Boot but should be available on any decent-ish version.

Hope it helps,
Javier
--
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 00/50] staging: omap-thermal: several code refactoring

2013-03-16 Thread Dan Carpenter
I've reviewed this set.

I hate to make people redo whole patchset sets, and I hate
re-reviewing code.  Obviously, I don't really like the bunny hop
patches and I'm trying to discourage that going forward.  ;P  But
I wouldn't say it's a "Redo the whole thing" kind of problem.

Could just resend patch 33 and 47?  You should probably be able to
redo those without changing the rest.

regards,
dan carpenter

--
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 47/50] staging: omap-thermal: switch mutex to spinlock inside omap-bandgap

2013-03-16 Thread Dan Carpenter
On Fri, Mar 15, 2013 at 09:00:35AM -0400, Eduardo Valentin wrote:
> Because there is a need to lock inside IRQ handler, this patch
> changes the locking mechanism inside the omap-bandgap.[c,h] to
> spinlocks. Now this lock is used to protect omap_bandgap struct
> during APIs exposed (possibly used in sysfs handling functions)
> and inside the ALERT IRQ handler.
> 
> Because there are registers shared among the sensors, this lock
> is global, not per sensor.
> 
> Signed-off-by: Eduardo Valentin 
> ---
>  drivers/staging/omap-thermal/TODO   |1 -
>  drivers/staging/omap-thermal/omap-bandgap.c |   18 ++
>  drivers/staging/omap-thermal/omap-bandgap.h |4 ++--
>  3 files changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/omap-thermal/TODO 
> b/drivers/staging/omap-thermal/TODO
> index 77b761b..0f24e9b 100644
> --- a/drivers/staging/omap-thermal/TODO
> +++ b/drivers/staging/omap-thermal/TODO
> @@ -1,7 +1,6 @@
>  List of TODOs (by Eduardo Valentin)
>  
>  on omap-bandgap.c:
> -- Rework locking
>  - Improve driver code by adding usage of regmap-mmio
>  - Test every exposed API to userland
>  - Add support to hwmon
> diff --git a/drivers/staging/omap-thermal/omap-bandgap.c 
> b/drivers/staging/omap-thermal/omap-bandgap.c
> index 4b631fd..846ced6 100644
> --- a/drivers/staging/omap-thermal/omap-bandgap.c
> +++ b/drivers/staging/omap-thermal/omap-bandgap.c
> @@ -33,7 +33,7 @@
>  #include 
>  #include 
>  #include 
> -#include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -170,6 +170,7 @@ static irqreturn_t omap_bandgap_talert_irq_handler(int 
> irq, void *data)
>   u32 t_hot = 0, t_cold = 0, ctrl;
>   int i;
>  
> + spin_lock(&bg_ptr->lock);
>   for (i = 0; i < bg_ptr->conf->sensor_count; i++) {
>   tsr = bg_ptr->conf->sensors[i].registers;
>   ctrl = omap_bandgap_readl(bg_ptr, tsr->bgap_status);
> @@ -208,6 +209,7 @@ static irqreturn_t omap_bandgap_talert_irq_handler(int 
> irq, void *data)
>   if (bg_ptr->conf->report_temperature)
>   bg_ptr->conf->report_temperature(bg_ptr, i);
>   }
> + spin_unlock(&bg_ptr->lock);
>  
>   return IRQ_HANDLED;
>  }
> @@ -502,9 +504,9 @@ int _omap_bandgap_write_threshold(struct omap_bandgap 
> *bg_ptr, int id, int val,
>   if (ret < 0)
>   goto exit;
>  
> - mutex_lock(&bg_ptr->bg_mutex);
> + spin_lock(&bg_ptr->lock);

These need to disable interrupts because we take the spin lock in
the IRQ handler.

regards,
dan carpenter


--
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 33/50] staging: omap-thermal: refactor APIs handling threshold values

2013-03-16 Thread Dan Carpenter
On Fri, Mar 15, 2013 at 09:00:21AM -0400, Eduardo Valentin wrote:
>   if (ret) {
>   dev_err(bg_ptr->dev, "failed to read thot\n");
> - return -EIO;
> + ret = -EIO;
> + goto exit;
>   }
>  
> - *thot = temp;
> + *val = temp;
>  
> +exit:
>   return 0;
>  }


The bunny hop has introduced a bug and this always returns success.

regards,
dan carpenter

--
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/50] staging: omap-thermal: rewrite omap_bandgap_mcelsius_to_adc on kernel coding style

2013-03-16 Thread Dan Carpenter
On Fri, Mar 15, 2013 at 09:00:13AM -0400, Eduardo Valentin wrote:
> Follow Documentation/CodingStyle while doing omap_bandgap_mcelsius_to_adc
> 

I have the same response for all these bunny hop patches.

When you're reading the code and you see a goto then you assume that
it's there for a reason so it's misleading when it doesn't do
anything.  It's simpler if the code is simple.

regards,
dan carpenter

--
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 22/50] staging: omap-thermal: rewrite omap_bandgap_adc_to_mcelsius on kernel coding style

2013-03-16 Thread Dan Carpenter
On Fri, Mar 15, 2013 at 09:00:10AM -0400, Eduardo Valentin wrote:
> Follow Documentation/CodingStyle while doing  omap_bandgap_adc_to_mcelsius.

Someone should probably fix CodingStyle to be more clear.  That's
not what was intended at all...  :/

regards,
dan carpenter

--
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: [net PATCH 1/1] drivers: net: ethernet: ti: davinci_emac: fix usage of cpdma_check_free_tx_desc()

2013-03-16 Thread Prabhakar Lad
Hi Mugunthan

Thanks for the patch!

On Fri, Mar 15, 2013 at 7:40 PM, Mugunthan V N  wrote:
> Fix which was done in the following commit in cpsw driver has
> to be taken forward to davinci emac driver as well.
>
> commit d35162f89b8f00537d7b240b76d2d0e8b8d29aa0
> Author: Daniel Mack 
> Date:   Tue Mar 12 06:31:19 2013 +
>
> net: ethernet: cpsw: fix usage of cpdma_check_free_tx_desc()
>
> Commit fae50823d0 ("net: ethernet: davinci_cpdma: Add boundary for rx
> and tx descriptors") introduced a function to check the current
> allocation state of tx packets. The return value is taken into account
> to stop the netqork queue on the adapter in case there are no free
> slots.
>
> However, cpdma_check_free_tx_desc() returns 'true' if there is room in
> the bitmap, not 'false', so the usage of the function is wrong.
>
> Reported-by: Prabhakar Lad 
> Signed-off-by: Mugunthan V N 

 Tested-by: Lad, Prabhakar 

Cheers,
--Prabhakar Lad
http://www.linkedin.com/pub/prabhakar-lad/19/92b/955

> ---
>  drivers/net/ethernet/ti/davinci_emac.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/ti/davinci_emac.c 
> b/drivers/net/ethernet/ti/davinci_emac.c
> index 52c0536..ae1b77a 100644
> --- a/drivers/net/ethernet/ti/davinci_emac.c
> +++ b/drivers/net/ethernet/ti/davinci_emac.c
> @@ -1102,7 +1102,7 @@ static int emac_dev_xmit(struct sk_buff *skb, struct 
> net_device *ndev)
> /* If there is no more tx desc left free then we need to
>  * tell the kernel to stop sending us tx frames.
>  */
> -   if (unlikely(cpdma_check_free_tx_desc(priv->txchan)))
> +   if (unlikely(!cpdma_check_free_tx_desc(priv->txchan)))
> netif_stop_queue(ndev);
>
> return NETDEV_TX_OK;
> --
> 1.7.9.5
>
> --
> 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
--
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