| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHp75VexBiRQ1SPYK6=N-vmCAC9DhtpNKanYXrFshSM9WZb+UA@mail.gmail.com>
Date: Wed, 30 May 2018 02:42:34 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Eddie James <eajames@...ux.vnet.ibm.com>
Cc: linux-i2c <linux-i2c@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
devicetree <devicetree@...r.kernel.org>,
Wolfram Sang <wsa@...-dreams.de>,
Rob Herring <robh+dt@...nel.org>,
Benjamin Herrenschmidt <benh@...nel.crashing.org>,
Joel Stanley <joel@....id.au>,
Mark Rutland <mark.rutland@....com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Edward A. James" <eajames@...ibm.com>
Subject: Re: [PATCH v7 2/7] drivers/i2c: Add FSI-attached I2C master algorithm
On Wed, May 30, 2018 at 1:24 AM, Eddie James <eajames@...ux.vnet.ibm.com> wrote:
> From: "Edward A. James" <eajames@...ibm.com>
>
> Add register definitions for FSI-attached I2C master and functions to
> access those registers over FSI. Add an FSI driver so that our I2C bus
> is probed up during an FSI scan.
This looks like reinventing a wheel in some ways.
See my comments below.
> +/*
> + * Copyright 2017 IBM Corporation
> + *
> + * Eddie James <eajames@...ibm.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
We are using SPDX identifiers. Can you?
> +/* Find left shift from first set bit in m */
> +#define MASK_TO_LSH(m) (__builtin_ffsll(m) - 1ULL)
Oh. What about GENMASK()?
> +/* Extract field m from v */
> +#define GETFIELD(m, v) (((v) & (m)) >> MASK_TO_LSH(m))
> +
> +/* Set field m of v to val */
> +#define SETFIELD(m, v, val) \
> + (((v) & ~(m)) | ((((typeof(v))(val)) << MASK_TO_LSH(m)) & (m)))
Oh, what about https://elixir.bootlin.com/linux/latest/source/include/linux/bitfield.h
?
> +#define I2C_CMD_WITH_START 0x80000000
> +#define I2C_CMD_WITH_ADDR 0x40000000
> +#define I2C_CMD_RD_CONT 0x20000000
> +#define I2C_CMD_WITH_STOP 0x10000000
> +#define I2C_CMD_FORCELAUNCH 0x08000000
BIT() ?
> +#define I2C_CMD_ADDR 0x00fe0000
> +#define I2C_CMD_READ 0x00010000
GENMASK()? Though precisely here it might be good to leave explicit values.
> +#define I2C_CMD_LEN 0x0000ffff
> +#define I2C_MODE_CLKDIV 0xffff0000
> +#define I2C_MODE_PORT 0x0000fc00
> +#define I2C_MODE_ENHANCED 0x00000008
> +#define I2C_MODE_DIAG 0x00000004
> +#define I2C_MODE_PACE_ALLOW 0x00000002
> +#define I2C_MODE_WRAP 0x00000001
What are they? Masks? Bit fields? Just plain numbers?
> +#define I2C_WATERMARK_HI 0x0000f000
> +#define I2C_WATERMARK_LO 0x000000f0
GENMASK() ?
> +#define I2C_INT_INV_CMD 0x00008000
> +#define I2C_INT_PARITY 0x00004000
> +#define I2C_INT_BE_OVERRUN 0x00002000
> +#define I2C_INT_BE_ACCESS 0x00001000
> +#define I2C_INT_LOST_ARB 0x00000800
> +#define I2C_INT_NACK 0x00000400
> +#define I2C_INT_DAT_REQ 0x00000200
> +#define I2C_INT_CMD_COMP 0x00000100
> +#define I2C_INT_STOP_ERR 0x00000080
> +#define I2C_INT_BUSY 0x00000040
> +#define I2C_INT_IDLE 0x00000020
BIT()
> +#define I2C_INT_ENABLE 0x0000ff80
> +#define I2C_INT_ERR 0x0000fcc0
> +#define I2C_STAT_INV_CMD 0x80000000
> +#define I2C_STAT_PARITY 0x40000000
> +#define I2C_STAT_BE_OVERRUN 0x20000000
> +#define I2C_STAT_BE_ACCESS 0x10000000
> +#define I2C_STAT_LOST_ARB 0x08000000
> +#define I2C_STAT_NACK 0x04000000
> +#define I2C_STAT_DAT_REQ 0x02000000
> +#define I2C_STAT_CMD_COMP 0x01000000
> +#define I2C_STAT_STOP_ERR 0x00800000
> +#define I2C_STAT_MAX_PORT 0x000f0000
> +#define I2C_STAT_ANY_INT 0x00008000
> +#define I2C_STAT_SCL_IN 0x00000800
> +#define I2C_STAT_SDA_IN 0x00000400
> +#define I2C_STAT_PORT_BUSY 0x00000200
> +#define I2C_STAT_SELF_BUSY 0x00000100
BIT()
> +#define I2C_STAT_FIFO_COUNT 0x000000ff
GENMASK()
> +
> +#define I2C_STAT_ERR 0xfc800000
> +#define I2C_STAT_ANY_RESP 0xff800000
> +#define I2C_ESTAT_FIFO_SZ 0xff000000
GENMASK()
> +#define I2C_ESTAT_SCL_IN_SY 0x00008000
> +#define I2C_ESTAT_SDA_IN_SY 0x00004000
> +#define I2C_ESTAT_S_SCL 0x00002000
> +#define I2C_ESTAT_S_SDA 0x00001000
> +#define I2C_ESTAT_M_SCL 0x00000800
> +#define I2C_ESTAT_M_SDA 0x00000400
> +#define I2C_ESTAT_HI_WATER 0x00000200
> +#define I2C_ESTAT_LO_WATER 0x00000100
> +#define I2C_ESTAT_PORT_BUSY 0x00000080
> +#define I2C_ESTAT_SELF_BUSY 0x00000040
BIT()
> +#define I2C_ESTAT_VERSION 0x0000001f
GENMASK()
> + __be32 data_be;
No need to have a suffix. If anything can go wrong we have a tool,
it's called sparse. It will catch out inappropriate use of __bitwise
types.
> + __be32 data_be = cpu_to_be32(*data);
cpu_to_be32p() IIUC?
> +static int fsi_i2c_dev_init(struct fsi_i2c_master *i2c)
> +{
> + int rc;
> + u32 mode = I2C_MODE_ENHANCED, extended_status, watermark = 0;
> + u32 interrupt = 0;
Redundant assignment.
> +
> + /* since we use polling, disable interrupts */
> + rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_INT_MASK, &interrupt);
> + if (rc)
> + return rc;
> + return rc;
Would be non-zero?
> +}
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists