Hi Tom, Overall I think this is much better than the older out of tree platform driver - thanks for cleaning it up.
One general comment - it would be good to run the patches through ./scripts/checkpatch.pl & fix up any warnings it gives unless you have a good reason to disagree with them. On Tue, Mar 05, 2019 at 06:28:42AM +0800, Yifeng Li wrote: > Lemote Yeeloong is a laptop powered by Loongson 2F MIPS processor, > primarily a demo platform for hobbyists and developers. It uses an > ENE KB3310B Embedded Controller with customized firmware to implement > hardware and power management. > > A monolithic platform driver code for those functionality has existed > out-of-tree for many years. This commit creates a MFD driver for the EC > chip on Yeeloong laptop to isolate EC-related code from core MIPS code, > and serves as the foundation of various subdrivers. > > My original attempt was to create a regmap for subdrivers to access the > EC, unfortunately, the board files in Linux/MIPS still needs to access > the EC directly for power management. Unless we find a better home for > those code, we simply export the EC-related functions. I'm not sure I follow why the power management code prevents use of regmap? Are you talking about the wakeup_loongson() function? Perhaps it would make sense for the suspend code to be part of one of the possible subdrivers you mention. The lemote-2f seems to be the only system that provides an implementation of wakeup_loongson() so perhaps a driver could instead just register its own struct platform_suspend_ops & avoid the need for code in arch/mips to care about the EC. > diff --git a/drivers/mfd/yeeloong_kb3310b.c b/drivers/mfd/yeeloong_kb3310b.c > new file mode 100644 > index 000000000000..64d353a83122 > --- /dev/null > +++ b/drivers/mfd/yeeloong_kb3310b.c > @@ -0,0 +1,207 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > + > +/* > + * MFD driver for ENE KB3310B embedded controller on Lemote Yeeloong laptops > + * > + * Copyright (C) 2008 Lemote Inc. > + * Author: liujl <[email protected]>, 2008-04-20 > + * > + * Copyright (C) 2018 Yifeng Li > + * Author: Yifeng Li <[email protected]> > + * > + * This is a MFD driver for the ENE KB3310B Embedded Controller for Lemote > + * Yeeloong laptops to provide utility functions to access the chip from > + * subdrivers, and handle events and interrupts in board files. This is a > + * special-purpose driver, and it's only used on Lemote Yeeloong laptops, > + * and is a mandatory dependency. > + * > + * My original attempt was to create a regmap for subdrivers to access the > + * EC, unfortunately, the board files in Linux/MIPS still needs to access > + * the EC directly for power management. Unless we find a better home for > + * those code, we simply export the EC-related functions. > + */ > + > +#include <linux/export.h> > +#include <linux/platform_device.h> > +#include <linux/mfd/core.h> > +#include <linux/io.h> > +#include <linux/delay.h> > + > +#include <linux/mfd/yeeloong_kb3310b.h> > + > +#define DRV_NAME "yeeloong_kb3310b: " Defining pr_fmt() would be cleaner - you wouldn't need to manually include DRV_NAME in your messages later. > + > +/***************************************************************************** > + * Most drivers, such as battery or backlight drivers, uses the I/O ports to > + * access the Index Registers to obtain hardware status and information from > + * EC chip. > + > ****************************************************************************/ Nit: the lines of asterisks aren't part of the kernel's general comment style & I think it would looks cleaner to remove them. > +static struct kb3310b_chip *kb3310b_fwinfo; > + > +static const struct mfd_cell kb3310b_cells[] = { > + { > + .name = "yeeloong_sci" > + }, > + { > + .name = "yeeloong_hwmon" > + }, > + { > + .name = "yeeloong_battery" > + }, > + { > + .name = "yeeloong_backlight" > + }, > + { > + .name = "yeeloong_lcd" > + }, > + { > + .name = "yeeloong_hotkey" > + }, Nit: I think it'd look cleaner if you remove the newlines within each array entry, eg: { .name = "yeeloong_sci" }, { .name = "yeelong_hwmon" }, ... > +}; > + > +static DEFINE_SPINLOCK(kb3310b_index_lock); > + > +u8 kb3310b_read(u16 reg) > +{ > + unsigned long flags; > + u8 val; > + > + spin_lock_irqsave(&kb3310b_index_lock, flags); > + > + outb((reg & 0xff00) >> 8, KB3310B_IO_PORT_HIGH); > + outb((reg & 0x00ff), KB3310B_IO_PORT_LOW); > + val = inb(KB3310B_IO_PORT_DATA); > + > + spin_unlock_irqrestore(&kb3310b_index_lock, flags); > + > + return val; > +} > +EXPORT_SYMBOL_GPL(kb3310b_read); > + > +void kb3310b_write(u16 reg, u8 val) > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&kb3310b_index_lock, flags); > + > + outb((reg & 0xff00) >> 8, KB3310B_IO_PORT_HIGH); > + outb((reg & 0x00ff), KB3310B_IO_PORT_LOW); > + outb(val, KB3310B_IO_PORT_DATA); > + inb(KB3310B_IO_PORT_DATA); /* flush pending writes */ > + > + spin_unlock_irqrestore(&kb3310b_index_lock, flags); > +} > +EXPORT_SYMBOL_GPL(kb3310b_write); If you do switch to using regmap then the above could be neatly represented as a bus - see struct regmap_bus, and various users of it throughout drivers/. regmap would even handle the locking for you. > + > +bool kb3310b_fw_earlier(char *version) > +{ > + return (strncasecmp(kb3310b_fwinfo->version, > + version, KB3310B_VERSION_LEN) < 0); > +} > +EXPORT_SYMBOL_GPL(kb3310b_fw_earlier); > + > +static int kb3310b_probe(struct platform_device *pdev) > +{ > + kb3310b_fwinfo = dev_get_platdata(&pdev->dev); > + pr_info(DRV_NAME "firmware version %s", kb3310b_fwinfo->version); > + > + return devm_mfd_add_devices(&pdev->dev, -1, kb3310b_cells, > + ARRAY_SIZE(kb3310b_cells), NULL, 0, NULL); > +} > + > +static struct platform_driver kb3310b_driver = { > + .driver = { > + .name = "yeeloong_kb3310b", > + }, > + .probe = kb3310b_probe, > +}; > +builtin_platform_driver(kb3310b_driver); > + > +/***************************************************************************** > + * For interrupt handling and power management, the EC chip is also needed to > + * be queried from the board file at arch/mips/loongson64, through a separate > + * command port. > + > *****************************************************************************/ As before, I think it'd be cleaner without the lines of asterisks. Probably without the empty line before the variable too. > + > +static DEFINE_SPINLOCK(kb3310b_command_lock); Since this is only used in kb3310b_query_seq() could you just declare it (still static) inside that function? Thanks, Paul > + > +/* > + * This function is used for EC command writes and corresponding status > queries. > + */ > +int kb3310b_query_seq(unsigned char cmd) > +{ > + int timeout; > + unsigned char status; > + unsigned long flags; > + > + spin_lock_irqsave(&kb3310b_command_lock, flags); > + > + /* make chip goto reset mode */ > + udelay(KB3310B_REG_UDELAY); > + outb(cmd, KB3310B_CMD_PORT); > + udelay(KB3310B_REG_UDELAY); > + > + /* check if the command is received by EC */ > + timeout = KB3310B_CMD_TIMEOUT; > + status = inb(KB3310B_STS_PORT); > + while (timeout-- && (status & (1 << 1))) { > + status = inb(KB3310B_STS_PORT); > + udelay(KB3310B_REG_UDELAY); > + } > + > + spin_unlock_irqrestore(&kb3310b_command_lock, flags); > + > + if (timeout <= 0) { > + pr_err(DRV_NAME > + "(%x/NA) failed to issue command %d, no response!\n", > + timeout, cmd); > + return -EINVAL; > + } > + > + pr_info(DRV_NAME > + "(%x/%x) issued command %d, status: 0x%x\n", > + timeout, KB3310B_CMD_TIMEOUT - timeout, > + cmd, status); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(kb3310b_query_seq); > + > +/* > + * Send query command to EC to get the proper event number. > + */ > +int kb3310b_query_event_num(void) > +{ > + return kb3310b_query_seq(KB3310B_CMD_GET_EVENT_NUM); > +} > +EXPORT_SYMBOL_GPL(kb3310b_query_event_num); > + > +/* > + * Get event number from EC. > + * > + * NOTE: This routine must follow the query_event_num function in the > + * interrupt. > + */ > +int kb3310b_get_event_num(void) > +{ > + int timeout = 100; > + unsigned char value; > + unsigned char status; > + > + udelay(KB3310B_REG_UDELAY); > + status = inb(KB3310B_STS_PORT); > + udelay(KB3310B_REG_UDELAY); > + while (timeout-- && !(status & (1 << 0))) { > + status = inb(KB3310B_STS_PORT); > + udelay(KB3310B_REG_UDELAY); > + } > + if (timeout <= 0) { > + pr_info("%s: get event number timeout.\n", __func__); > + return -EINVAL; > + } > + value = inb(KB3310B_DAT_PORT); > + udelay(KB3310B_REG_UDELAY); > + > + return value; > +} > +EXPORT_SYMBOL_GPL(kb3310b_get_event_num); > diff --git a/include/linux/mfd/yeeloong_kb3310b.h > b/include/linux/mfd/yeeloong_kb3310b.h > new file mode 100644 > index 000000000000..1f16ba2579bc > --- /dev/null > +++ b/include/linux/mfd/yeeloong_kb3310b.h > @@ -0,0 +1,211 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > + > +/* > + * MFD driver for ENE KB3310B embedded controller on Lemote Yeeloong laptops > + * > + * Copyright (C) 2008 Lemote Inc. > + * Author: liujl <[email protected]>, 2008-04-20 > + * > + * Copyright (C) 2018 Yifeng Li > + * Author: Yifeng Li <[email protected]> > + */ > + > +#ifndef __LINUX_MFD_YEELOONG_KB3310B_H > +#define __LINUX_MFD_YEELOONG_KB3310B_H > + > +extern u8 kb3310b_read(u16 reg); > +extern void kb3310b_write(u16 reg, u8 val); > +extern bool kb3310b_fw_earlier(char *version); > +extern int kb3310b_query_seq(unsigned char cmd); > +extern int kb3310b_query_event_num(void); > +extern int kb3310b_get_event_num(void); > + > +typedef int (*sci_handler) (int status); > + > +extern int yeeloong_sci_register_notify(struct notifier_block *nb); > +extern int yeeloong_sci_unregister_notify(struct notifier_block *nb); > + > +#define KB3310B_VERSION_LEN 8 > + > +struct kb3310b_chip { > + char version[KB3310B_VERSION_LEN]; > +}; > + > +#define KB3310B_SCI_IRQ_NUM 0x0A > + > +/* > + * The following registers are determined by the EC index configuration. > + * 1, fill the PORT_HIGH as EC register high part. > + * 2, fill the PORT_LOW as EC register low part. > + * 3, fill the PORT_DATA as EC register write data or get the data from it. > + */ > +#define KB3310B_IO_PORT_HIGH 0x0381 > +#define KB3310B_IO_PORT_LOW 0x0382 > +#define KB3310B_IO_PORT_DATA 0x0383 > + > +/* > + * EC delay time is 500us for register and status access > + */ > +#define KB3310B_REG_UDELAY 500 > +#define KB3310B_CMD_TIMEOUT 0x1000 > + > +/* > + * EC access port for SCI communication > + */ > +#define KB3310B_CMD_PORT 0x66 > +#define KB3310B_STS_PORT 0x66 > +#define KB3310B_DAT_PORT 0x62 > +#define KB3310B_CMD_INIT_IDLE_MODE 0xdd > +#define KB3310B_CMD_EXIT_IDLE_MODE 0xdf > +#define KB3310B_CMD_INIT_RESET_MODE 0xd8 > +#define KB3310B_CMD_REBOOT_SYSTEM 0x8c > +#define KB3310B_CMD_GET_EVENT_NUM 0x84 > +#define KB3310B_CMD_PROGRAM_PIECE 0xda > + > +/* temperature & fan registers */ > +#define KB3310B_REG_TEMPERATURE_VALUE 0xF458 > +#define KB3310B_REG_FAN_AUTO_MAN_SWITCH 0xF459 > +#define KB3310B_BIT_FAN_AUTO 0 > +#define KB3310B_BIT_FAN_MANUAL 1 > +#define KB3310B_REG_FAN_CONTROL 0xF4D2 > +#define KB3310B_BIT_FAN_CONTROL_ON (1 << 0) > +#define KB3310B_BIT_FAN_CONTROL_OFF (0 << 0) > +#define KB3310B_REG_FAN_STATUS 0xF4DA > +#define KB3310B_BIT_FAN_STATUS_ON (1 << 0) > +#define KB3310B_BIT_FAN_STATUS_OFF (0 << 0) > +#define KB3310B_REG_FAN_SPEED_HIGH 0xFE22 > +#define KB3310B_REG_FAN_SPEED_LOW 0xFE23 > +#define KB3310B_REG_FAN_SPEED_LEVEL 0xF4CC > + > +/* fan speed divider */ > +#define KB3310B_FAN_SPEED_DIVIDER 480000 /* (60*1000*1000/62.5/2)*/ > + > +/* battery registers */ > +#define KB3310B_REG_BAT_DESIGN_CAP_HIGH 0xF77D > +#define KB3310B_REG_BAT_DESIGN_CAP_LOW 0xF77E > +#define KB3310B_REG_BAT_FULLCHG_CAP_HIGH 0xF780 > +#define KB3310B_REG_BAT_FULLCHG_CAP_LOW 0xF781 > +#define KB3310B_REG_BAT_DESIGN_VOL_HIGH 0xF782 > +#define KB3310B_REG_BAT_DESIGN_VOL_LOW 0xF783 > +#define KB3310B_REG_BAT_CURRENT_HIGH 0xF784 > +#define KB3310B_REG_BAT_CURRENT_LOW 0xF785 > +#define KB3310B_REG_BAT_VOLTAGE_HIGH 0xF786 > +#define KB3310B_REG_BAT_VOLTAGE_LOW 0xF787 > +#define KB3310B_REG_BAT_TEMPERATURE_HIGH 0xF788 > +#define KB3310B_REG_BAT_TEMPERATURE_LOW 0xF789 > +#define KB3310B_REG_BAT_RELATIVE_CAP_HIGH 0xF492 > +#define KB3310B_REG_BAT_RELATIVE_CAP_LOW 0xF493 > +#define KB3310B_REG_BAT_VENDOR 0xF4C4 > +#define KB3310B_FLAG_BAT_VENDOR_SANYO 0x01 > +#define KB3310B_FLAG_BAT_VENDOR_SIMPLO 0x02 > +#define KB3310B_REG_BAT_CELL_COUNT 0xF4C6 > +#define KB3310B_FLAG_BAT_CELL_3S1P 0x03 > +#define KB3310B_FLAG_BAT_CELL_3S2P 0x06 > +#define KB3310B_REG_BAT_CHARGE 0xF4A2 > +#define KB3310B_FLAG_BAT_CHARGE_DISCHARGE 0x01 > +#define KB3310B_FLAG_BAT_CHARGE_CHARGE 0x02 > +#define KB3310B_FLAG_BAT_CHARGE_ACPOWER 0x00 > +#define KB3310B_REG_BAT_STATUS 0xF4B0 > +#define KB3310B_BIT_BAT_STATUS_LOW (1 << 5) > +#define KB3310B_BIT_BAT_STATUS_DESTROY (1 << 2) > +#define KB3310B_BIT_BAT_STATUS_FULL (1 << 1) > +#define KB3310B_BIT_BAT_STATUS_IN (1 << 0) > +#define KB3310B_REG_BAT_CHARGE_STATUS 0xF4B1 > +#define KB3310B_BIT_BAT_CHARGE_STATUS_OVERTEMP (1 << 2) > +#define KB3310B_BIT_BAT_CHARGE_STATUS_PRECHG (1 << 1) > +#define KB3310B_REG_BAT_STATE 0xF482 > +#define KB3310B_BIT_BAT_STATE_CHARGING (1 << 1) > +#define KB3310B_BIT_BAT_STATE_DISCHARGING (1 << 0) > +#define KB3310B_REG_BAT_POWER 0xF440 > +#define KB3310B_BIT_BAT_POWER_S3 (1 << 2) > +#define KB3310B_BIT_BAT_POWER_ON (1 << 1) > +#define KB3310B_BIT_BAT_POWER_ACIN (1 << 0) > + > +/* other registers */ > + > +/* Audio: rd/wr */ > +#define KB3310B_REG_AUDIO_VOLUME 0xF46C > +#define KB3310B_REG_AUDIO_MUTE 0xF4E7 > +#define KB3310B_REG_AUDIO_BEEP 0xF4D0 > + > +/* USB port power or not: rd/wr */ > +#define KB3310B_REG_USB0_FLAG 0xF461 > +#define KB3310B_REG_USB1_FLAG 0xF462 > +#define KB3310B_REG_USB2_FLAG 0xF463 > +#define KB3310B_BIT_USB_FLAG_ON 1 > +#define KB3310B_BIT_USB_FLAG_OFF 0 > + > +/* LID */ > +#define KB3310B_REG_LID_DETECT 0xF4BD > +#define KB3310B_BIT_LID_DETECT_ON 1 > +#define KB3310B_BIT_LID_DETECT_OFF 0 > + > +/* CRT */ > +#define KB3310B_REG_CRT_DETECT 0xF4AD > +#define KB3310B_BIT_CRT_DETECT_PLUG 1 > +#define KB3310B_BIT_CRT_DETECT_UNPLUG 0 > + > +/* LCD backlight brightness adjust: 9 levels */ > +#define KB3310B_REG_DISPLAY_BRIGHTNESS 0xF4F5 > + > +/* Black screen status */ > +#define KB3310B_REG_DISPLAY_LCD 0xF79F > +#define KB3310B_BIT_DISPLAY_LCD_ON 1 > +#define KB3310B_BIT_DISPLAY_LCD_OFF 0 > + > +/* LCD backlight control: off/restore */ > +#define KB3310B_REG_BACKLIGHT_CTRL 0xF7BD > +#define KB3310B_BIT_BACKLIGHT_ON 1 > +#define KB3310B_BIT_BACKLIGHT_OFF 0 > + > +/* Reset the machine auto-clear: rd/wr */ > +#define KB3310B_REG_RESET 0xF4EC > +#define KB3310B_BIT_RESET_ON 1 > + > +/* Light the LED: rd/wr */ > +#define KB3310B_REG_LED 0xF4C8 > +#define KB3310B_BIT_LED_RED_POWER (1 << 0) > +#define KB3310B_BIT_LED_ORANGE_POWER (1 << 1) > +#define KB3310B_BIT_LED_GREEN_CHARGE (1 << 2) > +#define KB3310B_BIT_LED_RED_CHARGE (1 << 3) > +#define KB3310B_BIT_LED_NUMLOCK (1 << 4) > + > +/* Test LED mode, all LED on/off */ > +#define KB3310B_REG_LED_TEST 0xF4C2 > +#define KB3310B_BIT_LED_TEST_IN 1 > +#define KB3310B_BIT_LED_TEST_OUT 0 > + > +/* Camera on/off */ > +#define KB3310B_REG_CAMERA_STATUS 0xF46A > +#define KB3310B_BIT_CAMERA_STATUS_ON 1 > +#define KB3310B_BIT_CAMERA_STATUS_OFF 0 > +#define KB3310B_REG_CAMERA_CONTROL 0xF7B7 > +#define KB3310B_BIT_CAMERA_CONTROL_OFF 0 > +#define KB3310B_BIT_CAMERA_CONTROL_ON 1 > + > +/* WLAN Status */ > +#define KB3310B_REG_WLAN 0xF4FA > +#define KB3310B_BIT_WLAN_ON 1 > +#define KB3310B_BIT_WLAN_OFF 0 > + > +/* SCI Event Number from EC */ > +enum { > + KB3310B_EVENT_START = 0x22, > + KB3310B_EVENT_LID = 0x23, /* LID open/close */ > + KB3310B_EVENT_DISPLAY_TOGGLE, /* Fn+F3 for display switch */ > + KB3310B_EVENT_SLEEP, /* Fn+F1 for entering sleep mode */ > + KB3310B_EVENT_OVERTEMP, /* Over-temperature occurred */ > + KB3310B_EVENT_CRT_DETECT, /* CRT is connected */ > + KB3310B_EVENT_CAMERA, /* Camera on/off */ > + KB3310B_EVENT_USB_OC2, /* USB2 Overcurrent occurred */ > + KB3310B_EVENT_USB_OC0, /* USB0 Overcurrent occurred */ > + KB3310B_EVENT_BLACK_SCREEN, /* Turn on/off backlight */ > + KB3310B_EVENT_AUDIO_MUTE, /* Mute on/off */ > + KB3310B_EVENT_DISPLAY_BRIGHTNESS, /* LCD backlight brightness adjust */ > + KB3310B_EVENT_AC_BAT, /* AC & Battery relative issue */ > + KB3310B_EVENT_AUDIO_VOLUME, /* Volume adjust */ > + KB3310B_EVENT_WLAN, /* WLAN on/off */ > + KB3310B_EVENT_END > +}; > + > +#endif /* !__LINUX_MFD_YEELOONG_KB3310B_H */ > -- > 2.20.1 >
