public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
  • * Re: [PATCH] Add a mfd driver to support Advantech EIO-IS200 series EC.
           [not found] ` <ebc6d86e627242a785f75f88623108c6@advantech.com.tw>
           [not found]   ` <3fbfadad386d475f8ecd0dfbc0d91fea@advantech.com.tw>
    @ 2023-08-18 11:22   ` Lee Jones
      2023-08-18 11:24     ` Lee Jones
      1 sibling, 1 reply; 3+ messages in thread
    From: Lee Jones @ 2023-08-18 11:22 UTC (permalink / raw)
      To: Wenkai.Chung; +Cc: 'linux-kernel@vger.kernel.org', Susi.Driver
    
    With respect to the subject, please use this command for examples:
    
      git log --oneline -- drivers/mfd
    
    On Thu, 27 Jul 2023, Wenkai.Chung wrote:
    
    > Add a mfd driver to support Advantech EIO-IS200 series EC.
    
    Please drop all references to "mfd".  It's not a real thing.
    
    Please tell us all about your driver here.  A one line description is
    not appropriate for a 600 line commit.
    
    > Signed-off-by: wenkai.chung <wenkai.chung@advantech.com.tw>
    > ---
    >  drivers/mfd/Kconfig         |  13 +
    >  drivers/mfd/Makefile        |   1 +
    >  drivers/mfd/eiois200.h      | 146 +++++++++++
    >  drivers/mfd/eiois200_core.c | 496 ++++++++++++++++++++++++++++++++++++
    >  4 files changed, 656 insertions(+)
    >  create mode 100644 drivers/mfd/eiois200.h  create mode 100644 drivers/mfd/eiois200_core.c
    > 
    > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index 6f5b259a6d6a..ca792a077da9 100644
    > --- a/drivers/mfd/Kconfig
    > +++ b/drivers/mfd/Kconfig
    > @@ -154,6 +154,19 @@ config MFD_ATMEL_HLCDC
    >  	  This driver provides common support for accessing the device,
    >  	  additional drivers must be enabled in order to use the
    >  	  functionality of the device.
    > +	  
    > + config MFD_EIOIS200
    > +	tristate "Advantech EIO-IS200 Embedded Controller core driver"
    > +	depends on X86 && m
    
    Depends on m?
    
    > +	default m
    
    Are you sure you want this?
    
    > +	select MFD_CORE
    > +	help
    > +	  Support for the EIO-IS200 controller.
    
    Which is ...
    
    > +	  This driver provides common support for accessing the device,
    > +	  additional drivers must be enabled in order to use the functionality
    > +	  of the device.
    
    What will the module be called?
    
    What other devices does this h/w support?
    
    > +
    
    You don't need 2 spaces here.
    
    >  
    >  config MFD_ATMEL_SMC
    >  	bool
    > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index f3d1f1dc73b5..59e911054688 100644
    > --- a/drivers/mfd/Makefile
    > +++ b/drivers/mfd/Makefile
    > @@ -276,6 +276,7 @@ obj-$(CONFIG_MFD_INTEL_M10_BMC_PMCI)   += intel-m10-bmc-pmci.o
    >  
    >  obj-$(CONFIG_MFD_ATC260X)	+= atc260x-core.o
    >  obj-$(CONFIG_MFD_ATC260X_I2C)	+= atc260x-i2c.o
    > +obj-$(CONFIG_MFD_EIOIS200)	+= eiois200_core.o
    >  
    >  rsmu-i2c-objs			:= rsmu_core.o rsmu_i2c.o
    >  rsmu-spi-objs			:= rsmu_core.o rsmu_spi.o
    > diff --git a/drivers/mfd/eiois200.h b/drivers/mfd/eiois200.h new file mode 100644 index 000000000000..24a448d70d00
    > --- /dev/null
    > +++ b/drivers/mfd/eiois200.h
    > @@ -0,0 +1,146 @@
    > +/* SPDX-License-Identifier: GPL-2.0-only */
    > +
    > +#ifndef _MFD_EIOIS200_H_
    > +#define _MFD_EIOIS200_H_
    
    '\n'
    
    > +#include <linux/stddef.h>
    
    What are you using this for?
    
    > +#include <linux/io.h>
    > +#include <linux/regmap.h>
    > +
    > +// Definition
    
    No C++ style comments.
    
    > +#define EIOIS200_CHIPID1				0x20
    > +#define EIOIS200_CHIPID2				0x21
    > +#define EIOIS200_CHIPVER				0x22
    > +#define EIOIS200_SIOCTRL				0x23
    > +#define EIOIS200_SIOCTRL_SIOEN				BIT(0)
    > +#define EIOIS200_SIOCTRL_SWRST				BIT(1)
    > +#define EIOIS200_IRQCTRL				0x70
    > +#define EIOIS200_CHIP_ID				0x9610
    > +#define EIO201_211_CHIP_ID				0x9620
    
    CHIP_ID or CHIPID, please be consistent.
    
    > +#define EIOIS200_ICCODE					0x10
    > +#define EIO201_ICCODE					0x20
    > +#define EIO211_ICCODE					0x21
    > +
    > +// LPC PNP
    > +#define EIOIS200_PNP_INDEX				0x299
    > +#define EIOIS200_PNP_DATA				0x29A
    > +#define EIOIS200_SUB_PNP_INDEX				0x499
    > +#define EIOIS200_SUB_PNP_DATA				0x49A
    > +#define EIOIS200_EXT_MODE_ENTER				0x87
    > +#define EIOIS200_EXT_MODE_EXIT				0xAA
    > +
    > +// LPC LDN
    > +#define EIOIS200_LDN					0x07
    > +#define EIOIS200_LDN_PMC0				0x0C
    > +#define EIOIS200_LDN_PMC1				0x0D
    > +
    > +// PMC common registers
    > +#define EIOIS200_LDAR					0x30
    > +#define EIOIS200_LDAR_LDACT				BIT(0)
    > +#define EIOIS200_IOBA0H					0x60
    > +#define EIOIS200_IOBA0L					0x61
    > +#define EIOIS200_IOBA1H					0x62
    > +#define EIOIS200_IOBA1L					0x63
    > +#define EIOIS200_PMC_STATUS_IBF				BIT(1)
    > +#define EIOIS200_PMC_STATUS_OBF				BIT(0)
    > +#define EIOIS200_PMC_PORT				0x2F0
    > +
    > +// PMC (Power management channel) command list
    
    (Power Management Channel)
    
    Why aren't the abbreviations above expanded out as well?
    
    > +#define EIOIS200_PMC_CMD_WDT_WRITE			0x2A
    > +#define EIOIS200_PMC_CMD_WDT_READ			0x2B
    > +#define EIOIS200_PMC_CMD_WDT_START			0x2C
    > +#define EIOIS200_PMC_CMD_WDT_STOP			0x2D
    > +#define EIOIS200_PMC_CMD_WDT_TRIG			0x2E
    > +#define EIOIS200_PMC_CMD_ACPIRAM_READ			0x31
    > +#define EIOIS200_PMC_CMD_CFG_SAVE			0x56
    > +
    > +// New PMC command list
    > +#define EIOIS200_NEW_PMC_CMD_DOC_FW_READ		0x03
    > +#define EIOIS200_NEW_PMC_CMD_SYSTEM_READ		0x55
    > +#define EIOIS200_NEW_PMC_CMD_WDT_WRITE			0x2A
    > +#define EIOIS200_NEW_PMC_CMD_WDT_READ			0x2B
    > +
    > +// OLD PMC
    > +#define EIOIS200_PMC_NO_INDEX				0xFF
    > +
    > +// ACPI RAM Address Table
    > +#define EIOIS200_ACPIRAM_VERSIONSECTION	(0xFA)
    > +#define EIOIS200_ACPIRAM_ICVENDOR	(EIOIS200_ACPIRAM_VERSIONSECTION + 0x00)
    > +#define EIOIS200_ACPIRAM_ICCODE		(EIOIS200_ACPIRAM_VERSIONSECTION + 0x01)
    > +#define EIOIS200_ACPIRAM_CODEBASE	(EIOIS200_ACPIRAM_VERSIONSECTION + 0x02)
    > +
    > +/* Firmware **/
    > +#define EIOIS200_F_SUB_NEW_CODE_BASE	BIT(6)	// Identity second EC code base version
    > +#define EIOIS200_F_SUB_CHANGED		BIT(7)	// Setting second EC changed
    > +#define EIOIS200_F_NEW_CODE_BASE	BIT(8)	// Identity code base version
    > +#define EIOIS200_F_CHANGED		BIT(9)	// Setting changed
    > +#define EIOIS200_F_SUB_CHIP_EXIST	BIT(30)	// Second EIO-IS200 exist
    > +#define EIOIS200_F_CHIP_EXIST		BIT(31)	// EIO-IS200 exist
    > +
    > +/* Others **/
    
    What comment style is this?
    
    > +#define EC_NUM	2
    > +
    > +struct _pmc_port {
    > +	union {
    > +		u16 cmd;
    > +		u16 status;
    > +	};
    > +	u16 data;
    > +};
    > +
    > +struct _eiois200_dev_port {
    > +	u16 idx;
    > +	u16 data;
    > +};
    > +
    > +struct _pmc_op {
    > +	u8 cmd;
    > +	u8 index;	// use 0xFF to identify PMC command with index or not.
    > +	u8 offset;
    > +	u8 len;
    > +	u8 *data;
    > +};
    > +
    > +struct _pmc_new_op {
    > +	u8  cmd;
    > +	u8  control;
    > +	u8  device_id;
    > +	u8  size;
    > +	u8  *payload;
    > +};
    > +
    > +enum eiois200_rw_operation {
    > +	OPERATION_READ,
    > +	OPERATION_WRITE,
    > +};
    > +
    > +struct _eiois200_dev {
    > +	u32 flag;
    > +
    > +	struct _pmc_port  pmc;
    > +	struct _pmc_port  pmc1;
    > +
    > +	struct mutex eiois200_io_mutex;		// mutex lock for eiois200 io access
    > +};
    > +
    > +/* exported symbol */
    > +//extern struct _eiois200_dev *eiois200_dev; int 
    
    No commented out code.
    
    > +eiois200_core_pmc_operation(const struct _pmc_port *pmc,
    > +				const struct _pmc_op *operation,
    > +				enum eiois200_rw_operation rw);
    > +
    > +int eiois200_core_new_pmc_operation(const struct _pmc_port *pmc,
    > +				    const struct _pmc_new_op *operation);
    > +
    > +int eiois200_core_pmc_wait_ibf(const struct _pmc_port *pmc); int 
    
    Does this even compile?
    
    > +eiois200_core_pmc_wait_obf(const struct _pmc_port *pmc);
    > +
    > +#define WAIT_IBF(pmc)		eiois200_core_pmc_wait_ibf(pmc)
    > +#define WAIT_OBF(pmc)		eiois200_core_pmc_wait_obf(pmc)
    > +#define NEW_CODE_BASE		(eiois200_dev->flag & EIOIS200_F_NEW_CODE_BASE)
    > +
    > +#ifdef pr_fmt
    > +#undef pr_fmt
    > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt #endif
    
    Why do you need this?
    
    > +#endif
    > diff --git a/drivers/mfd/eiois200_core.c b/drivers/mfd/eiois200_core.c new file mode 100644 index 000000000000..4be6c8651b6a
    > --- /dev/null
    > +++ b/drivers/mfd/eiois200_core.c
    > @@ -0,0 +1,496 @@
    > +// SPDX-License-Identifier: GPL-2.0-only
    > +/*
    > + * MFD driver for Advantech EIO-IS200 Embedded controller.
    > + *
    > + * This driver provides PMC commands interface for subdrivers.
    > + *
    > + * Copyright (C) 2023 Advantech Corporation. All rights reserved.
    > + */
    > +
    > +#include <linux/uaccess.h>
    > +#include <linux/mfd/core.h>
    > +#include <linux/mutex.h>
    > +#include <linux/delay.h>
    > +#include <linux/isa.h>
    
    Alphabetical.
    
    > +#include "eiois200.h"
    > +
    > +static int timeout = 500;
    
    Why 500?  Is this ms or us?
    
    > +module_param(timeout, int, 0);
    > +MODULE_PARM_DESC(timeout,
    > +		 "IO exponential increased timeout value. (default="
    > +		 __MODULE_STRING(timeout) ")");
    
    Do you really need to override this value?
    
    > +struct _eiois200_dev_port eio_pnp_port[EC_NUM] = {
    > +	{EIOIS200_PNP_INDEX,	 EIOIS200_PNP_DATA    },
    > +	{EIOIS200_SUB_PNP_INDEX, EIOIS200_SUB_PNP_DATA} };
    
    Formatting.
    
    > +struct _eiois200_dev *eiois200_dev;
    
    Why the '_'?
    
    > +struct regmap *regmap_is200;
    
    Why do these need to be global?
    
    > +static struct mfd_cell susi_mfd_devs[] = {
    > +	{ .name = "eiois200_wdt"},
    > +};
    
    Where are the other devices?
    
    > +/* For regmap */
    
    You don't need this.
    
    > +static int reg_read(void *context, unsigned int reg, unsigned int *val) 
    
    Whitespace at the end of the line.
    
    > +{
    > +	*val = inb(reg);
    
    This looks odd.  Surely this basic use-case is catered for elsewhere?
    
    > +	return 0;
    > +}
    > +
    > +static int reg_write(void *context, unsigned int reg, unsigned int val) 
    > +{
    > +	outb(val, reg);
    > +
    > +	return 0;
    > +}
    > +
    > +struct regmap_range is200_rang[] = {
    
    range*
    
    > +	{EIOIS200_PNP_INDEX,	 EIOIS200_PNP_DATA	 },
    
    Space after the '{'
    
    > +	{EIOIS200_SUB_PNP_INDEX, EIOIS200_SUB_PNP_DATA	 },
    > +	{EIOIS200_PMC_PORT,	 EIOIS200_PMC_PORT + 0x0F},
    > +};
    > +
    > +static const struct regmap_access_table volatile_regs = {
    > +	.yes_ranges = is200_rang,
    > +	.n_yes_ranges = ARRAY_SIZE(is200_rang), };
    > +
    > +static const struct regmap_config pnp_regmap_config = {
    > +	.reg_bits	= 16,
    > +	.val_bits	= 8,
    > +	.volatile_table = &volatile_regs,
    > +
    > +	.reg_write	= reg_write,
    > +	.reg_read	= reg_read,
    > +};
    > +
    > +/* For EIO-IS200 pnp io port access */
    > +static int is200_pnp_in(const struct _eiois200_dev_port *port, u8 idx) 
    
    Please do a check for whitespace issues everywhere - I see lots.
    
    > +{
    > +	int ret;
    > +
    > +	regmap_write(regmap_is200, port->idx, idx);
    > +	regmap_read(regmap_is200, port->data, &ret);
    
    It's not a ret - please use 'value'.
    
    > +
    > +	return ret;
    > +}
    > +
    > +static void is200_pnp_out(const struct _eiois200_dev_port *port, u8 
    > +idx, u8 data) {
    
    This is an odd place to linewrap - please also tab out to the '('.
    
    > +	regmap_write(regmap_is200, port->idx, idx);
    > +	regmap_write(regmap_is200, port->data, data); }
    > +
    > +static void is200_ext_entry(const struct _eiois200_dev_port *port) {
    > +	regmap_write(regmap_is200, port->idx, EIOIS200_EXT_MODE_ENTER);
    > +	regmap_write(regmap_is200, port->idx, EIOIS200_EXT_MODE_ENTER); }
    
    Formatting - and throughout.
    
    > +static void eio_ext_leave(const struct _eiois200_dev_port *port) {
    > +	regmap_write(regmap_is200, port->idx, EIOIS200_EXT_MODE_EXIT); }
    > +
    > +/* EIO-IS200 io port access function for pmc command */ static int 
    
    The return type should not be up there.
    
    [...]
    
    I'm going to stop here for now.  Lots to do.
    
    Please make sure you read the documents pointed to from here:
    
      https://docs.kernel.org/process/5.Posting.html
    
    And run and fix issue alluded to by scripts/checkpatch.pl before reposting.
    
    -- 
    Lee Jones [李琼斯]
    
    ^ permalink raw reply	[flat|nested] 3+ messages in thread

  • end of thread, other threads:[~2023-08-18 11:25 UTC | newest]
    
    Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
    -- links below jump to the message on this page --
         [not found] <20230727053655.92525-1-wenkai.chung@advantech.com.tw>
         [not found] ` <ebc6d86e627242a785f75f88623108c6@advantech.com.tw>
         [not found]   ` <3fbfadad386d475f8ecd0dfbc0d91fea@advantech.com.tw>
    2023-08-15 16:59     ` [PATCH] Add a mfd driver to support Advantech EIO-IS200 series EC Lee Jones
    2023-08-18 11:22   ` Lee Jones
    2023-08-18 11:24     ` Lee Jones
    

    This is a public inbox, see mirroring instructions
    for how to clone and mirror all data and code used for this inbox