Re: [PATCH v2 1/7] mfd: yeeloong_kb3310b: support KB3310B EC for Lemote Yeeloong laptops.

From: Paul Burton
Date: Tue Mar 05 2019 - 18:51:04 EST

Next message: John Hubbard: "Re: [PATCH] mm/hmm: fix unused variable warnings"
Previous message: Steven Rostedt: "Re: [PATCH 1/2] tracing: kdb: Fix ftdump to not sleep"
In reply to: Yifeng Li: "[PATCH v2 1/7] mfd: yeeloong_kb3310b: support KB3310B EC for Lemote Yeeloong laptops."
Next in thread: Tom Li: "Re: [PATCH v2 1/7] mfd: yeeloong_kb3310b: support KB3310B EC for Lemote Yeeloong laptops."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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 <liujl@xxxxxxxxxx>, 2008-04-20
> + *
> + * Copyright (C) 2018 Yifeng Li
> + * Author: Yifeng Li <tomli@xxxxxxxx>
> + *
> + * 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 <liujl@xxxxxxxxxx>, 2008-04-20
> + *
> + * Copyright (C) 2018 Yifeng Li
> + * Author: Yifeng Li <tomli@xxxxxxxx>
> + */
> +
> +#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
>

Next message: John Hubbard: "Re: [PATCH] mm/hmm: fix unused variable warnings"
Previous message: Steven Rostedt: "Re: [PATCH 1/2] tracing: kdb: Fix ftdump to not sleep"
In reply to: Yifeng Li: "[PATCH v2 1/7] mfd: yeeloong_kb3310b: support KB3310B EC for Lemote Yeeloong laptops."
Next in thread: Tom Li: "Re: [PATCH v2 1/7] mfd: yeeloong_kb3310b: support KB3310B EC for Lemote Yeeloong laptops."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]