Re: [PATCH 3/6] bus: mvebu: add mbus driver

[Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>]

On 02/07/2014 07:58 AM, Sascha Hauer wrote:
> Hi Sebastian,
>
> On Wed, Feb 05, 2014 at 11:40:06PM +0100, Sebastian Hesselbarth wrote:
>> + *   device accesses. This API is mvebu_mbus_add_window_by_id(),
>> + *   mvebu_mbus_add_window_remap_by_id() and
>> + *   mvebu_mbus_del_window().
>> + *
>> + * - Provides a debugfs interface in /sys/kernel/debug/mvebu-mbus/ to
>> + *   see the list of CPU -> SDRAM windows and their configuration
>> + *   (file 'sdram') and the list of CPU -> devices windows and their
>> + *   configuration (file 'devices').
>> + */
>
> I'm inclined to say that you should use dev_* functions for debugging,
> but I see the code is very close to the kernel and you don't have a
> struct device_d pointer everywhere. Up to you to use dev_*, but if not
> please add a #define pr_fmt here.

Sascha,

uhh, you got me. I really just copied it over without looking at the
code at all. I'll give the driver some closer look and will see where
I can bareabox-ify it. I'll send a v2 of this very patch then.

>> +
>> +#include <common.h>
>> +#include <init.h>
>> +#include <io.h>
>> +#include <of.h>
>> +#include <of_address.h>
>> +#include <linux/mbus.h>
>> +
>
> [...]
>
>> +
>> +static const struct mvebu_mbus_soc_data orion5x_2win_mbus_data = {
>> +	.num_wins            = 8,
>> +	.num_remappable_wins = 2,
>> +	.win_cfg_offset      = orion5x_mbus_win_offset,
>> +	.setup_cpu_target    = mvebu_mbus_default_setup_cpu_target,
>> +};
>> +
>> +static const struct mvebu_mbus_soc_data mv78xx0_mbus_data = {
>> +	.num_wins            = 14,
>> +	.num_remappable_wins = 8,
>> +	.win_cfg_offset      = mv78xx0_mbus_win_offset,
>> +	.setup_cpu_target    = mvebu_mbus_default_setup_cpu_target,
>> +};
>
> Don't you end up with unused variable warnings here if not all SoCs are
> enabled? Maybe add some __maybe_unused here.

Ok.

Sebastian

>> +
>> +static struct of_device_id mvebu_mbus_dt_ids[] = {
>> +#if defined(CONFIG_ARCH_ARMADA_370) || defined(CONFIG_ARCH_ARMADA_XP)
>> +	{ .compatible = "marvell,armada370-mbus",
>> +	  .data = (u32)&armada_370_xp_mbus_data, },
>> +	{ .compatible = "marvell,armadaxp-mbus",
>> +	  .data = (u32)&armada_370_xp_mbus_data, },
>> +#endif
>> +#if defined(CONFIG_ARCH_DOVE)
>> +	{ .compatible = "marvell,dove-mbus",
>> +	  .data = (u32)&dove_mbus_data, },
>> +#endif


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox