From: Ryan Mallon <ryan@bluewatersys.com>
To: Alexey Charkov <alchark@gmail.com>
Cc: "Russell King - ARM Linux" <linux@arm.linux.org.uk>,
linux-arm-kernel@lists.infradead.org,
vt8500-wm8505-linux-kernel@googlegroups.com,
"Eric Miao" <eric.y.miao@gmail.com>,
"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
"Albin Tonnerre" <albin.tonnerre@free-electrons.com>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/6 v10] ARM: Add basic architecture support for VIA/WonderMedia 85xx SoC's
Date: Thu, 23 Dec 2010 11:32:17 +1300 [thread overview]
Message-ID: <4D127C71.5040301@bluewatersys.com> (raw)
In-Reply-To: <AANLkTinZEhJ8aUe63yc_BNdifn_5g8U=iStwwXEU-+V9@mail.gmail.com>
On 12/23/2010 11:02 AM, Alexey Charkov wrote:
> 2010/12/23 Ryan Mallon <ryan@bluewatersys.com>:
>> On 12/23/2010 10:18 AM, Alexey Charkov wrote:
>>> This adds support for the family of Systems-on-Chip produced initially
>>> by VIA and now its subsidiary WonderMedia that have recently become
>>> widespread in lower-end Chinese ARM-based tablets and netbooks.
>>>
>>> Support is included for both VT8500 and WM8505, selectable by a
>>> configuration switch at kernel build time.
>>>
>>> Included are basic machine initialization files, register and
>>> interrupt definitions, support for the on-chip interrupt controller,
>>> high-precision OS timer, GPIO lines, necessary macros for early debug,
>>> pulse-width-modulated outputs control, as well as platform device
>>> configurations for the specific drivers implemented elsewhere.
>>>
>>> Signed-off-by: Alexey Charkov <alchark@gmail.com>
>>> ---
>>>
>>> Welcome the jubilee tenth revision of this patch ;-)
>>>
>>> I've tried to incorporate the suggestions by Ryan and Arnd, hope that
>>> there is nothing left out. There was a massive reorganization of code
>>> to remove less-than-obvious magic with MMIO registers and irqs being
>>> held in huge structs, now they are again macro definitions. Those
>>> macros are, however, only included in single isolated files, and
>>> actual values to use are chosen at runtime by calling the respective
>>> routines at machine initialization. There are also stylistic changes
>>> all around, where Ryan suggested.
>>>
>>> As a result, i8042 should again be adjusted a bit to reflect the new
>>> place to find respective register/irq definitions, that one will be
>>> sent in the respective thread shortly.
>>>
>>> Best regards,
>>> Alexey
>>
>> <snip>
>>
>>> --- /dev/null
>>> +++ b/arch/arm/mach-vt8500/devices-vt8500.c
>>> @@ -0,0 +1,117 @@
>>> +/* linux/arch/arm/mach-vt8500/devices-vt8500.c
>>> + *
>>> + * Copyright (C) 2010 Alexey Charkov <alchark@gmail.com>
>>> + *
>>> + * This software is licensed under the terms of the GNU General Public
>>> + * License version 2, as published by the Free Software Foundation, and
>>> + * may be copied, distributed, and modified under those terms.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + *
>>> + */
>>> +
>>> +#include <linux/platform_device.h>
>>> +
>>> +#include <mach/vt8500_regs.h>
>>> +#include <mach/vt8500_irqs.h>
>>> +#include <mach/i8042.h>
>>> +#include "devices.h"
>>> +
>>> +static inline struct resource WMT_MMIO_RES(u32 start, u32 size)
>>> +{
>>> + struct resource tmp = {
>>> + .flags = IORESOURCE_MEM,
>>> + .start = start,
>>> + .end = start + size - 1,
>>> + };
>>> +
>>> + return tmp;
>>> +}
>>
>> These functions can be marked __init (though I guess they already are if
>> marked inline?). They should also have lower case names since they are
>> proper functions.
>
> As inline functions are unrolled into the caller at compile time, and
> the caller is __init, I would expect their code to be freed after init
> as well. I could be wrong, though :)
If the compiler decided not to inline them for whatever reason they
would stick around. They should also probably be marked __init to make
it clear that they are not permanent functions.
>> Do these functions generate warnings about returning temporary values
>> off the stack? If so, they could be rewritten as:
>>
>
> Should those be compile-time or run-time? I did not see any.
I thought you might get compile time warnings. Returning things off the
stack in general is error prone. I think passing a pointer to the
resource into the function (as below) maybe a bit better.
>> static __init void wmt_mmio_res(struct resource *res,
>> u32 start, u32 size)
>> {
>> res->flags = IORESOURCE_MEM;
>> res->start = start;
>> res->end = start + size - 1;
>> }
>>
>> You could also make these functions static inline in devices.h to avoid
>> having to define them for each board.
>>
>
> Agreed.
>
>>> +
>>> +static inline struct resource WMT_IRQ_RES(int irq)
>>> +{
>>> + struct resource tmp = {
>>> + .flags = IORESOURCE_IRQ,
>>> + .start = irq,
>>> + .end = irq,
>>> + };
>>> +
>>> + return tmp;
>>> +}
>>> +
>>> +#define WMT_RES_ADD(__device, __resource, __num) \
>>> +if (platform_device_add_resources(__device, __resource, __num)) \
>>> + pr_err("Failed to assign resources to __device##\n");
>>
>> This could be written as a proper function. The resource add is unlikely
>> to fail. Maybe keep the warning but don't worry about printing the
>> device name?
>>
>
> There is memory allocation inside platform_device_add_resources, so
> probably there is scope for failure. I could add unlikely(), though.
I meant more in terms of losing the device name in the error which seems
to be the sole point of the macro. i.e it could be rewritten as:
static void wmt_add_resource(struct platform_device *pdev,
const struct resource *res,
unsigned num)
{
if (platform_device_add_resources(pdev, res, num))
pr_err("Failed to add device resource\n");
}
Because the add resource failing would indicate some serious problem you
may even want to BUG().
~Ryan
--
Bluewater Systems Ltd - ARM Technology Solution Centre
Ryan Mallon 5 Amuri Park, 404 Barbadoes St
ryan@bluewatersys.com PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com New Zealand
Phone: +64 3 3779127 Freecall: Australia 1800 148 751
Fax: +64 3 3779135 USA 1800 261 2934
next prev parent reply other threads:[~2010-12-22 22:30 UTC|newest]
Thread overview: 91+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-11-07 16:28 [PATCH 1/6 v2] ARM: Add basic architecture support for VIA/WonderMedia 85xx SoC's Alexey Charkov
2010-11-07 16:28 ` [PATCH 2/6 v2] serial: Add support for UART on VIA VT8500 and compatibles Alexey Charkov
2010-11-07 23:08 ` Alan Cox
2010-11-08 0:58 ` [PATCH 2/6 v3] " Alexey Charkov
2010-11-08 10:46 ` Alan Cox
2010-11-08 17:33 ` [PATCH 2/6 v4] " Alexey Charkov
2010-11-07 16:28 ` [PATCH 3/6 v2] input: Add support for VIA VT8500 and compatibles in i8042 Alexey Charkov
2010-11-12 22:54 ` Alexey Charkov
2010-11-12 23:30 ` Dmitry Torokhov
2010-11-13 0:00 ` Alexey Charkov
2010-12-22 21:41 ` [PATCH 3/6 v3] " Alexey Charkov
2010-11-07 16:28 ` [PATCH 4/6 v2] usb: Add support for VIA VT8500 and compatibles in EHCI HCD Alexey Charkov
2010-11-07 16:28 ` [PATCH 5/6 v2] rtc: Add support for the RTC in VIA VT8500 and compatibles Alexey Charkov
2010-11-12 22:53 ` Alexey Charkov
2010-11-13 12:14 ` Lars-Peter Clausen
2010-11-13 23:56 ` [PATCH 5/6 v3] " Alexey Charkov
2010-11-14 15:50 ` Lars-Peter Clausen
2010-11-14 17:00 ` [PATCH 5/6 v4] " Alexey Charkov
2010-11-23 19:17 ` Alexey Charkov
2010-11-24 19:23 ` Lars-Peter Clausen
2010-11-24 22:47 ` [PATCH 5/6 v5] " Alexey Charkov
2010-11-07 16:28 ` [PATCH 6/6 v2] ARM: Add support for the display controllers in VT8500 and WM8505 Alexey Charkov
2010-11-08 4:17 ` Paul Mundt
2010-11-08 12:56 ` Alexey Charkov
2010-11-08 14:14 ` [PATCH 6/6 v3] " Alexey Charkov
2010-11-08 20:43 ` Paul Mundt
2010-11-08 21:15 ` Alexey Charkov
2010-11-08 21:30 ` Paul Mundt
2010-11-08 23:42 ` [PATCH 6/6 v4] " Alexey Charkov
2010-11-08 23:54 ` Paul Mundt
2010-11-09 0:03 ` Alexey Charkov
2010-11-09 7:36 ` Guennadi Liakhovetski
2010-11-09 9:39 ` Paul Mundt
2010-11-09 9:49 ` Alexey Charkov
2010-11-09 10:33 ` [PATCH 6/6 v3] " Russell King - ARM Linux
2010-11-09 10:51 ` Paul Mundt
2010-11-09 11:04 ` Russell King - ARM Linux
2010-11-09 13:02 ` Geert Uytterhoeven
2010-11-09 13:33 ` Arnd Bergmann
2010-11-09 16:20 ` Paul Mundt
2010-11-08 8:47 ` [PATCH 6/6 v2] " Arnd Bergmann
2010-11-09 10:23 ` Alexey Charkov
2010-11-09 15:03 ` Arnd Bergmann
2010-11-07 16:57 ` [PATCH 1/6 v2] ARM: Add basic architecture support for VIA/WonderMedia 85xx SoC's Russell King - ARM Linux
2010-11-07 17:08 ` Alexey Charkov
2010-11-07 17:17 ` Russell King - ARM Linux
2010-11-07 18:25 ` [PATCH 1/6 v3] " Alexey Charkov
2010-11-08 17:19 ` [PATCH 1/6 v4] " Alexey Charkov
2010-11-10 15:16 ` saeed bishara
2010-11-10 15:18 ` Russell King - ARM Linux
2010-11-10 15:20 ` saeed bishara
2010-11-11 21:23 ` [PATCH 1/6 v5] " Alexey Charkov
2010-11-11 23:49 ` Russell King - ARM Linux
2010-11-12 16:54 ` [PATCH 1/6 v6] " Alexey Charkov
2010-11-12 20:14 ` Alexey Charkov
2010-11-23 19:50 ` [PATCH 1/6 v7] " Alexey Charkov
2010-12-19 17:40 ` [PATCH 1/6 v8] " Alexey Charkov
2010-12-20 18:15 ` Arnd Bergmann
2010-12-20 19:15 ` Russell King - ARM Linux
2010-12-20 19:26 ` Alexey Charkov
2010-12-20 19:54 ` [PATCH 1/6 v9] " Alexey Charkov
2010-12-20 20:50 ` Ryan Mallon
2010-12-20 21:48 ` Alexey Charkov
2010-12-20 22:23 ` Ryan Mallon
2010-12-20 23:00 ` Alexey Charkov
2010-12-20 23:22 ` Ryan Mallon
2010-12-20 23:49 ` Alexey Charkov
2010-12-21 0:09 ` Alexey Charkov
2010-12-21 2:32 ` Ryan Mallon
2010-12-21 10:00 ` Alexey Charkov
2010-12-21 12:05 ` Arnd Bergmann
2010-12-21 19:39 ` Ryan Mallon
2010-12-22 21:18 ` [PATCH 1/6 v10] " Alexey Charkov
2010-12-22 21:52 ` Ryan Mallon
2010-12-22 22:02 ` Alexey Charkov
2010-12-22 22:32 ` Ryan Mallon [this message]
2010-12-22 22:25 ` [PATCH 1/6 v11] " Alexey Charkov
2010-12-22 22:59 ` Ryan Mallon
2010-12-22 23:38 ` [PATCH 1/6 v12] " Alexey Charkov
2010-12-28 14:52 ` Alexey Charkov
2011-01-15 0:53 ` Alexey Charkov
2011-01-15 0:58 ` Russell King - ARM Linux
2011-01-15 1:13 ` Alexey Charkov
2011-01-21 10:43 ` Russell King - ARM Linux
2011-07-06 12:34 ` [PATCH 1/6 v8] " Russell King - ARM Linux
2011-07-07 7:13 ` Alexey Charkov
2011-07-07 7:54 ` Arnd Bergmann
2011-07-07 7:59 ` Russell King - ARM Linux
2011-07-07 8:05 ` Arnd Bergmann
2010-11-07 17:00 ` [PATCH 1/6 v2] " Russell King - ARM Linux
2010-11-07 17:16 ` Alexey Charkov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4D127C71.5040301@bluewatersys.com \
--to=ryan@bluewatersys.com \
--cc=albin.tonnerre@free-electrons.com \
--cc=alchark@gmail.com \
--cc=eric.y.miao@gmail.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@arm.linux.org.uk \
--cc=u.kleine-koenig@pengutronix.de \
--cc=vt8500-wm8505-linux-kernel@googlegroups.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).