Message-id: <alpine.LFD.2.00.1009241426120.13233@xanadu.home>
Date:	Fri, 24 Sep 2010 15:07:15 -0400 (EDT)
From:	Nicolas Pitre <nico@...xnic.net>
To:	Ash Hughes <ashley.hughes@...eyonder.co.uk>
Cc:	kernel@...tstofly.org,
	Russell King - ARM Linux <linux@....linux.org.uk>,
	linux-arm-kernel@...ts.infradead.org,
	lkml <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/1] ARM Orion: added Buffalo LS-CHL support

On Fri, 24 Sep 2010, Ash Hughes wrote:

> From: Ash Hughes <ashley.hughes@...eyonder.co.uk>
> Adds support for Buffalo Linkstation Live v3 (LS-CHL) NAS drives.
> Signed-off-by: Ash Hughes <ashley.hughes@...eyonder.co.uk>

Comments below.

> ---
> 
> Code has been in use for several months:
> http://forum.buffalo.nas-central.org/viewtopic.php?f=18&t=13566
> 
> diff -urBN linux-2.6.34-vanilla/arch/arm/mach-orion5x/Kconfig linux-2.6.34/arch/arm/mach-orion5x/Kconfig
> --- linux-2.6.34-vanilla/arch/arm/mach-orion5x/Kconfig	2010-05-16 22:17:36.000000000 +0100
> +++ linux-2.6.34/arch/arm/mach-orion5x/Kconfig	2010-06-11 13:46:45.665614309 +0100
> @@ -50,6 +50,13 @@
>  	  Buffalo Linkstation Pro/Live platform. Both v1 and
>  	  v2 devices are supported.
>  
> +config MACH_LINKSTATION_LSCHL
> +	bool "Buffalo Linkstation Live v3 (LS-CHL)"
> +	select I2C_BOARDINFO
> +	help
> +	  Say 'Y' here if you want your kernel to support the
> +	  Buffalo Linkstation Live v3 (LS-CHL) platform.
> +
>  config MACH_LINKSTATION_MINI
>  	bool "Buffalo Linkstation Mini"
>  	select I2C_BOARDINFO
> diff -urBN linux-2.6.34-vanilla/arch/arm/mach-orion5x/ls-chl-setup.c
> linux-2.6.34/arch/arm/mach-orion5x/ls-chl-setup.c
> --- linux-2.6.34-vanilla/arch/arm/mach-orion5x/ls-chl-setup.c	1970-01-01 01:00:00.000000000 +0100
> +++ linux-2.6.34/arch/arm/mach-orion5x/ls-chl-setup.c	2010-09-24 03:56:24.432077623 +0100
> @@ -0,0 +1,449 @@
> +/*
> + * arch/arm/mach-orion5x/ls-chl-setup.c
> + *
> + * Maintainer: Ash Hughes <ashley.hughes@...eyonder.co.uk>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2.  This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/proc_fs.h>

More on proc_fs below.

> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +#include <linux/pci.h>

Why do you need this include?

> +#include <linux/irq.h>
> +#include <linux/delay.h>

And this one?

> +#include <linux/mtd/physmap.h>
> +#include <linux/mv643xx_eth.h>
> +#include <linux/leds.h>
> +#include <linux/gpio_keys.h>
> +#include <linux/input.h>
> +#include <linux/i2c.h>
> +#include <linux/serial_reg.h>

And this one?

> +#include <linux/ata_platform.h>
> +#include <asm/mach-types.h>
> +#include <asm/gpio.h>

Please use linux/gpio.h instead.

> +#include <asm/mach/arch.h>
> +#include <asm/mach/pci.h>

Why this one?

> +#include "common.h"
> +#include "mpp.h"
> +#include "include/mach/system.h"

Please use <> for global includes.

> +#include <asm/uaccess.h>

We prefer to list global includes before local ones.

> +/*****************************************************************************
> + * Linkstation LS-CHL Info
> + ****************************************************************************/
> +
> +/*
> + * 256K NOR flash Device bus boot chip select
> + */
> +
> +#define LSCHL_NOR_BOOT_BASE	0xf4000000
> +#define LSCHL_NOR_BOOT_SIZE	SZ_256K
> +
> +/*****************************************************************************
> + * 256KB NOR Flash on BOOT Device
> + ****************************************************************************/
> +
> +static struct physmap_flash_data lschl_nor_flash_data = {
> +	.width		= 1,
> +};
> +
> +static struct resource lschl_nor_flash_resource = {
> +	.flags	= IORESOURCE_MEM,
> +	.start	= LSCHL_NOR_BOOT_BASE,
> +	.end	= LSCHL_NOR_BOOT_BASE + LSCHL_NOR_BOOT_SIZE - 1,
> +};
> +
> +static struct platform_device lschl_nor_flash = {
> +	.name			= "physmap-flash",
> +	.id			= 0,
> +	.dev		= {
> +		.platform_data	= &lschl_nor_flash_data,
> +	},
> +	.num_resources		= 1,
> +	.resource		= &lschl_nor_flash_resource,
> +};
> +
> +/*****************************************************************************
> + * Ethernet
> + ****************************************************************************/
> +
> +static struct mv643xx_eth_platform_data lschl_eth_data = {
> +	.phy_addr	= 8,
> +};

You should use the MV643XX_ETH_PHY_ADDR() macro.

[...]

> +#define LSCHL_GPIO_FAN_LOW	16
> +#define LSCHL_GPIO_FAN_HIGH	14
> +#define LSCHL_GPIO_FAN_LOCK	6
> +
> +#define BUFFALO_GPIO_FAN_STOP	0
> +#define BUFFALO_GPIO_FAN_SLOW	1
> +#define BUFFALO_GPIO_FAN_FAST	2
> +#define BUFFALO_GPIO_FAN_FULL	3
> +
> +#define BuffaloGpio_FanStop()	FanControlWrite(BUFFALO_GPIO_FAN_STOP)
> +#define BuffaloGpio_FanSlow()	FanControlWrite(BUFFALO_GPIO_FAN_SLOW)
> +#define BuffaloGpio_FanFast()	FanControlWrite(BUFFALO_GPIO_FAN_FAST)
> +#define BuffaloGpio_FanFull()	FanControlWrite(BUFFALO_GPIO_FAN_FULL)

The WindowsCamelCaseIdentifierStyle is usually frowned upon for 
Linux code.

> +void
> +FanControlWrite(unsigned int value)

Make this static.

> +{
> +	switch (value) {
> +	case BUFFALO_GPIO_FAN_STOP:
> +		gpio_set_value(LSCHL_GPIO_FAN_LOW, 1);
> +		gpio_set_value(LSCHL_GPIO_FAN_HIGH, 1);
> +		break;
> +	case BUFFALO_GPIO_FAN_SLOW:
> +		gpio_set_value(LSCHL_GPIO_FAN_LOW, 1);
> +		gpio_set_value(LSCHL_GPIO_FAN_HIGH, 0);
> +		break;
> +	case BUFFALO_GPIO_FAN_FAST:
> +		gpio_set_value(LSCHL_GPIO_FAN_LOW, 0);
> +		gpio_set_value(LSCHL_GPIO_FAN_HIGH, 1);
> +		break;
> +	case BUFFALO_GPIO_FAN_FULL:
> +		gpio_set_value(LSCHL_GPIO_FAN_LOW, 0);
> +		gpio_set_value(LSCHL_GPIO_FAN_HIGH, 0);
> +		break;
> +	default:
> +		break;
> +	} /* end of switch */
> +}
> +
> +unsigned int
> +BuffaloGpio_FanControlRead(void)

Ditto here (CamelCase and static).

> +{
> +
> +	int fan_low = gpio_get_value(LSCHL_GPIO_FAN_LOW);
> +	int fan_high = gpio_get_value(LSCHL_GPIO_FAN_HIGH);
> +
> +	if (!fan_low && !fan_high)
> +		return BUFFALO_GPIO_FAN_FULL;
> +	else if (!fan_low && fan_high)
> +		return BUFFALO_GPIO_FAN_FAST;
> +	else if (fan_low && !fan_high)
> +		return BUFFALO_GPIO_FAN_SLOW;
> +	else if (fan_low && fan_high)
> +		return BUFFALO_GPIO_FAN_STOP;
> +	return 0;
> +}

Now... both of those functions could be much simpler, similar to this:

static void fan_ctrl_write(unsigned int value)
{
	int fan_low = !(value & 1);
	int fan_high = !(value & 2);
	gpio_set_value(LSCHL_GPIO_FAN_LOW, fan_low);
	gpio_set_value(LSCHL_GPIO_FAN_HIGH, fan_high);
}

static unsigned int fan_ctrl_read(void)
{
	int fan_low = gpio_get_value(LSCHL_GPIO_FAN_LOW);
	int fan_high = gpio_get_value(LSCHL_GPIO_FAN_HIGH);
	return !fan_low | ((!fan_high) << 1);
}

> +static int
> +FanStatusRead(char *pg, char **st, off_t off, int length, int *eof, void *data)
> +{
> +
> +	int len = 0;
> +	off_t begin = 0;
> +
> +	int PinStat = gpio_get_value(LSCHL_GPIO_FAN_LOCK);
> +
> +	if (!PinStat)
> +		len = sprintf(pg, "Fine\n");
> +	else
> +		len = sprintf(pg, "Stop\n");
> +	*st = pg + (off - begin);
> +	len -= (off - begin);
> +	if (len > length)
> +		len = length;
> +	return len;
> +}
> +
> +
> +
> +static int
> +FanStatusWrite(struct file *fp, const char *buf, unsigned long cnt, void *d) {
> +	return cnt;
> +}
> +
> +static int
> +FanCtlWrite(struct file *filep, const char *buf, unsigned long cnt, void *data)
> +{
> +	int len, ret;
> +	char *ptr, tState[8];
> +
> +	if (cnt > 8)
> +		len = 8;
> +	else
> +		len = cnt;
> +
> +	ret = copy_from_user(tState, buf, len);
> +	if (ret < 0) {
> +		printk(KERN_ERR "%s: Setting fan speed failed\n", "ls-chl");
> +		return -EFAULT;
> +	}
> +
> +	ptr = strrchr(tState, '\n');
> +	if (ptr)
> +		*ptr = '\0';
> +
> +	if (strncmp(tState, "stop", strlen("stop")) == 0)
> +		BuffaloGpio_FanStop();
> +	else if (strncmp(tState, "slow", strlen("slow")) == 0)
> +		BuffaloGpio_FanSlow();
> +	else if (strncmp(tState, "fast", strlen("fast")) == 0)
> +		BuffaloGpio_FanFast();
> +	else if (strncmp(tState, "full", strlen("full")) == 0)
> +		BuffaloGpio_FanFull();
> +	return cnt;
> +}
> +
> +static int
> +FanCtlRead(char *pg, char **start, off_t off, int length, int *eof, void *data)
> +{
> +	int len = 0;
> +	off_t begin = 0;
> +
> +	unsigned int fan_status = BuffaloGpio_FanControlRead();
> +
> +	switch (fan_status) {
> +	case BUFFALO_GPIO_FAN_STOP:
> +		len += sprintf(pg, "stop\n");
> +		break;
> +	case BUFFALO_GPIO_FAN_SLOW:
> +		len += sprintf(pg, "slow\n");
> +		break;
> +	case BUFFALO_GPIO_FAN_FAST:
> +		len += sprintf(pg, "fast\n");
> +		break;
> +	case BUFFALO_GPIO_FAN_FULL:
> +		len += sprintf(pg, "full\n");
> +		break;
> +	default:
> +		break;
> +	} /* end of switch */
> +
> +	*start = pg + (off - begin);
> +	len -= (off - begin);
> +	if (len > length)
> +		len = length;
> +	return len;
> +}

Here I'd strongly suggest investigating the hwmon API which is the 
standard interface for fan control/status on Linux instead of creating 
an adhoc one.

> +static void __init lschl_init(void)
> +{
> +	/*
> +	 * Setup basic Orion functions. Need to be called early.
> +	 */
> +	struct proc_dir_entry *pde;
> +	orion5x_init();
> +
> +	orion5x_mpp_conf(lschl_mpp_modes);
> +
> +	/*
> +	 * Configure peripherals.
> +	 */
> +	orion5x_ehci0_init();
> +	orion5x_ehci1_init();
> +	orion5x_eth_init(&lschl_eth_data);
> +	orion5x_i2c_init();
> +	orion5x_sata_init(&lschl_sata_data);
> +	orion5x_uart0_init();
> +	orion5x_xor_init();
> +
> +	orion5x_setup_dev_boot_win(LSCHL_NOR_BOOT_BASE,
> +				   LSCHL_NOR_BOOT_SIZE);
> +	platform_device_register(&lschl_nor_flash);
> +
> +	platform_device_register(&lschl_leds);
> +
> +	platform_device_register(&lschl_button_device);
> +
> +	i2c_register_board_info(0, &lschl_i2c_rtc, 1);
> +
> +	pde = proc_mkdir("buffalo", 0);
> +	pde = proc_mkdir("buffalo/gpio", 0);
> +	pde = proc_mkdir("buffalo/gpio/fan", 0);
> +	pde = create_proc_entry("buffalo/gpio/fan/control", 0644, 0);
> +	pde->read_proc = FanCtlRead;
> +	pde->write_proc = FanCtlWrite;
> +	pde = create_proc_entry("buffalo/gpio/fan/lock", 0, 0);
> +	pde->read_proc = FanStatusRead;
> +	pde->write_proc = FanStatusWrite;

As I said, the hwmon interface would be a much better choice than a 
custom /proc interface.  But if you still prefer a lightweight 
interface, then a numeric sysfs entry would still be much better than 
/proc with that string parsing

> +	/* slow down fan */
> +	BuffaloGpio_FanSlow();
> +	/* usb power on */
> +	gpio_set_value(LSCHL_GPIO_USB_POWER, 1);
> +
> +	/* register power-off method */
> +	pm_power_off = lschl_power_off;
> +
> +	pr_info("%s: finished\n", __func__);
> +}
> +
> +#ifdef CONFIG_MACH_LINKSTATION_LSCHL

You don't need this #ifdef as this whole file is already conditionally 
compiled only when this config symbol is active.

> +MACHINE_START(LINKSTATION_LSCHL, "Buffalo Linkstation LiveV3 (LS-CHL)")
> +	/* Maintainer: Ash Hughes <ashley.hughes@...eyonder.co.uk> */
> +	.phys_io	= ORION5X_REGS_PHYS_BASE,
> +	.io_pg_offst	= ((ORION5X_REGS_VIRT_BASE) >> 18) & 0xFFFC,
> +	.boot_params	= 0x00000100,
> +	.init_machine	= lschl_init,
> +	.map_io		= orion5x_map_io,
> +	.init_irq	= orion5x_init_irq,
> +	.timer		= &orion5x_timer,
> +	.fixup		= tag_fixup_mem32,
> +MACHINE_END
> +#endif
> diff -urBN linux-2.6.34-vanilla/arch/arm/mach-orion5x/Makefile linux-2.6.34/arch/arm/mach-orion5x/Makefile
> --- linux-2.6.34-vanilla/arch/arm/mach-orion5x/Makefile	2010-05-16 22:17:36.000000000 +0100
> +++ linux-2.6.34/arch/arm/mach-orion5x/Makefile	2010-08-18 12:33:02.714982012 +0100
> @@ -21,3 +21,4 @@
>  obj-$(CONFIG_MACH_RD88F5181L_GE)	+= rd88f5181l-ge-setup.o
>  obj-$(CONFIG_MACH_RD88F5181L_FXO)	+= rd88f5181l-fxo-setup.o
>  obj-$(CONFIG_MACH_RD88F6183AP_GE)	+= rd88f6183ap-ge-setup.o
> +obj-$(CONFIG_MACH_LINKSTATION_LSCHL)   += ls-chl-setup.o
> 
> 


Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists