* Re: [PATCH 01/12] iio: imu: inv_icm42600: add core of new inv_icm42600 driver
From: Jean-Baptiste Maneyrol @ 2020-05-18 14:14 UTC (permalink / raw)
To: Jonathan Cameron
Cc: jic23@kernel.org, robh+dt@kernel.org, robh@kernel.org,
mchehab+huawei@kernel.org, davem@davemloft.net,
gregkh@linuxfoundation.org, linux-iio@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <20200508142820.000057f8@Huawei.com>
Hi Jonathan,
thanks for the feedbacks, I'm sorry but I will not be able to have a correct email formatting to respond you inline.
No problem with all the comments. For iio_device_get_drvdata, it would make more sense to use a const struct iio_dev * as argument. I am obliged to do the pointer conversion since iio_get_mount_matrix requires the use of a const struct iio_dev *.
For resume/suspend, I will add commentaries to explain what it is really doing and for which purpose. Sensor states save and restore will remain in this patch, since it makes more sense to have it as a core functionnality, as much as gyro/accel turn on/off.
Thanks.
JB
From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org> on behalf of Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Sent: Friday, May 8, 2020 15:28
To: Jean-Baptiste Maneyrol <JManeyrol@invensense.com>
Cc: jic23@kernel.org <jic23@kernel.org>; robh+dt@kernel.org <robh+dt@kernel.org>; robh@kernel.org <robh@kernel.org>; mchehab+huawei@kernel.org <mchehab+huawei@kernel.org>; davem@davemloft.net <davem@davemloft.net>; gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>;
linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; devicetree@vger.kernel.org <devicetree@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 01/12] iio: imu: inv_icm42600: add core of new inv_icm42600 driver
CAUTION: This email originated from outside of the organization. Please make sure the sender is who they say they are and do not click links or open attachments unless you recognize the sender and know the content is safe.
On Thu, 7 May 2020 16:42:11 +0200
Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:
> Core component of a new driver for InvenSense ICM-426xx devices.
> It includes registers definition, main probe/setup, and device
> utility functions.
>
> ICM-426xx devices are latest generation of 6-axis IMU,
> gyroscope+accelerometer and temperature sensor. This device
> includes a 2K FIFO, supports I2C/I3C/SPI, and provides
> intelligent motion features like pedometer, tilt detection,
> and tap detection.
>
> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
Hi Jean-Baptiste,
A few minor things inline.
Thanks,
Jonathan
> ---
> drivers/iio/imu/inv_icm42600/inv_icm42600.h | 372 +++++++++++
> .../iio/imu/inv_icm42600/inv_icm42600_core.c | 618 ++++++++++++++++++
> 2 files changed, 990 insertions(+)
> create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600.h
> create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
>
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> new file mode 100644
> index 000000000000..8da4c8249aed
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> @@ -0,0 +1,372 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2020 Invensense, Inc.
> + */
> +
> +#ifndef INV_ICM42600_H_
> +#define INV_ICM42600_H_
> +
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
> +#include <linux/regmap.h>
> +#include <linux/mutex.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/pm.h>
> +#include <linux/iio/iio.h>
> +
> +enum inv_icm42600_chip {
> + INV_CHIP_ICM42600,
> + INV_CHIP_ICM42602,
> + INV_CHIP_ICM42605,
> + INV_CHIP_ICM42622,
> + INV_CHIP_NB,
> +};
> +
> +/* serial bus slew rates */
> +enum inv_icm42600_slew_rate {
> + INV_ICM42600_SLEW_RATE_20_60NS,
> + INV_ICM42600_SLEW_RATE_12_36NS,
> + INV_ICM42600_SLEW_RATE_6_18NS,
> + INV_ICM42600_SLEW_RATE_4_12NS,
> + INV_ICM42600_SLEW_RATE_2_6NS,
> + INV_ICM42600_SLEW_RATE_INF_2NS,
> +};
> +
> +enum inv_icm42600_sensor_mode {
> + INV_ICM42600_SENSOR_MODE_OFF,
> + INV_ICM42600_SENSOR_MODE_STANDBY,
> + INV_ICM42600_SENSOR_MODE_LOW_POWER,
> + INV_ICM42600_SENSOR_MODE_LOW_NOISE,
> + INV_ICM42600_SENSOR_MODE_NB,
> +};
> +
> +/* gyroscope fullscale values */
> +enum inv_icm42600_gyro_fs {
> + INV_ICM42600_GYRO_FS_2000DPS,
> + INV_ICM42600_GYRO_FS_1000DPS,
> + INV_ICM42600_GYRO_FS_500DPS,
> + INV_ICM42600_GYRO_FS_250DPS,
> + INV_ICM42600_GYRO_FS_125DPS,
> + INV_ICM42600_GYRO_FS_62_5DPS,
> + INV_ICM42600_GYRO_FS_31_25DPS,
> + INV_ICM42600_GYRO_FS_15_625DPS,
> + INV_ICM42600_GYRO_FS_NB,
> +};
> +
> +/* accelerometer fullscale values */
> +enum inv_icm42600_accel_fs {
> + INV_ICM42600_ACCEL_FS_16G,
> + INV_ICM42600_ACCEL_FS_8G,
> + INV_ICM42600_ACCEL_FS_4G,
> + INV_ICM42600_ACCEL_FS_2G,
> + INV_ICM42600_ACCEL_FS_NB,
> +};
> +
> +/* ODR suffixed by LN or LP are Low-Noise or Low-Power mode only */
> +enum inv_icm42600_odr {
> + INV_ICM42600_ODR_8KHZ_LN = 3,
> + INV_ICM42600_ODR_4KHZ_LN,
> + INV_ICM42600_ODR_2KHZ_LN,
> + INV_ICM42600_ODR_1KHZ_LN,
> + INV_ICM42600_ODR_200HZ,
> + INV_ICM42600_ODR_100HZ,
> + INV_ICM42600_ODR_50HZ,
> + INV_ICM42600_ODR_25HZ,
> + INV_ICM42600_ODR_12_5HZ,
> + INV_ICM42600_ODR_6_25HZ_LP,
> + INV_ICM42600_ODR_3_125HZ_LP,
> + INV_ICM42600_ODR_1_5625HZ_LP,
> + INV_ICM42600_ODR_500HZ,
> + INV_ICM42600_ODR_NB,
> +};
> +
> +enum inv_icm42600_filter {
> + /* Low-Noise mode sensor data filter (3rd order filter by default) */
> + INV_ICM42600_FILTER_BW_ODR_DIV_2,
> +
> + /* Low-Power mode sensor data filter (averaging) */
> + INV_ICM42600_FILTER_AVG_1X = 1,
> + INV_ICM42600_FILTER_AVG_16X = 6,
> +};
> +
> +struct inv_icm42600_sensor_conf {
> + int mode;
> + int fs;
> + int odr;
> + int filter;
> +};
> +#define INV_ICM42600_SENSOR_CONF_INIT {-1, -1, -1, -1}
> +
> +struct inv_icm42600_conf {
> + struct inv_icm42600_sensor_conf gyro;
> + struct inv_icm42600_sensor_conf accel;
> + bool temp_en;
> +};
> +
> +struct inv_icm42600_suspended {
> + enum inv_icm42600_sensor_mode gyro;
> + enum inv_icm42600_sensor_mode accel;
> + bool temp;
> +};
> +
> +/*
/**
It's valid kernel doc so lets mark it as such.
> + * struct inv_icm42600_state - driver state variables
> + * @lock: chip access lock.
Nice to be a bit more specific on that. What about the chip needs
a lock at this level as opposed to bus locks etc?
> + * @chip: chip identifier.
> + * @name: chip name.
> + * @map: regmap pointer.
> + * @vdd_supply: VDD voltage regulator for the chip.
> + * @vddio_supply: I/O voltage regulator for the chip.
> + * @orientation: sensor chip orientation relative to main hardware.
> + * @conf: chip sensors configurations.
> + * @suspended: suspended sensors configuration.
> + */
> +struct inv_icm42600_state {
> + struct mutex lock;
> + enum inv_icm42600_chip chip;
> + const char *name;
> + struct regmap *map;
> + struct regulator *vdd_supply;
> + struct regulator *vddio_supply;
> + struct iio_mount_matrix orientation;
> + struct inv_icm42600_conf conf;
> + struct inv_icm42600_suspended suspended;
> +};
> +
> +/* Virtual register addresses: @bank on MSB (4 upper bits), @address on LSB */
> +
> +/* Bank selection register, available in all banks */
> +#define INV_ICM42600_REG_BANK_SEL 0x76
> +#define INV_ICM42600_BANK_SEL_MASK GENMASK(2, 0)
> +
> +/* User bank 0 (MSB 0x00) */
> +#define INV_ICM42600_REG_DEVICE_CONFIG 0x0011
> +#define INV_ICM42600_DEVICE_CONFIG_SOFT_RESET BIT(0)
> +
> +#define INV_ICM42600_REG_DRIVE_CONFIG 0x0013
> +#define INV_ICM42600_DRIVE_CONFIG_I2C_MASK GENMASK(5, 3)
> +#define INV_ICM42600_DRIVE_CONFIG_I2C(_rate) \
> + FIELD_PREP(INV_ICM42600_DRIVE_CONFIG_I2C_MASK, (_rate))
> +#define INV_ICM42600_DRIVE_CONFIG_SPI_MASK GENMASK(2, 0)
> +#define INV_ICM42600_DRIVE_CONFIG_SPI(_rate) \
> + FIELD_PREP(INV_ICM42600_DRIVE_CONFIG_SPI_MASK, (_rate))
> +
> +#define INV_ICM42600_REG_INT_CONFIG 0x0014
> +#define INV_ICM42600_INT_CONFIG_INT2_LATCHED BIT(5)
> +#define INV_ICM42600_INT_CONFIG_INT2_PUSH_PULL BIT(4)
> +#define INV_ICM42600_INT_CONFIG_INT2_ACTIVE_HIGH BIT(3)
> +#define INV_ICM42600_INT_CONFIG_INT2_ACTIVE_LOW 0x00
> +#define INV_ICM42600_INT_CONFIG_INT1_LATCHED BIT(2)
> +#define INV_ICM42600_INT_CONFIG_INT1_PUSH_PULL BIT(1)
> +#define INV_ICM42600_INT_CONFIG_INT1_ACTIVE_HIGH BIT(0)
> +#define INV_ICM42600_INT_CONFIG_INT1_ACTIVE_LOW 0x00
> +
> +#define INV_ICM42600_REG_FIFO_CONFIG 0x0016
> +#define INV_ICM42600_FIFO_CONFIG_MASK GENMASK(7, 6)
> +#define INV_ICM42600_FIFO_CONFIG_BYPASS \
> + FIELD_PREP(INV_ICM42600_FIFO_CONFIG_MASK, 0)
> +#define INV_ICM42600_FIFO_CONFIG_STREAM \
> + FIELD_PREP(INV_ICM42600_FIFO_CONFIG_MASK, 1)
> +#define INV_ICM42600_FIFO_CONFIG_STOP_ON_FULL \
> + FIELD_PREP(INV_ICM42600_FIFO_CONFIG_MASK, 2)
> +
> +/* all sensor data are 16 bits (2 registers wide) in big-endian */
> +#define INV_ICM42600_REG_TEMP_DATA 0x001D
> +#define INV_ICM42600_REG_ACCEL_DATA_X 0x001F
> +#define INV_ICM42600_REG_ACCEL_DATA_Y 0x0021
> +#define INV_ICM42600_REG_ACCEL_DATA_Z 0x0023
> +#define INV_ICM42600_REG_GYRO_DATA_X 0x0025
> +#define INV_ICM42600_REG_GYRO_DATA_Y 0x0027
> +#define INV_ICM42600_REG_GYRO_DATA_Z 0x0029
> +#define INV_ICM42600_DATA_INVALID -32768
> +
> +#define INV_ICM42600_REG_INT_STATUS 0x002D
> +#define INV_ICM42600_INT_STATUS_UI_FSYNC BIT(6)
> +#define INV_ICM42600_INT_STATUS_PLL_RDY BIT(5)
> +#define INV_ICM42600_INT_STATUS_RESET_DONE BIT(4)
> +#define INV_ICM42600_INT_STATUS_DATA_RDY BIT(3)
> +#define INV_ICM42600_INT_STATUS_FIFO_THS BIT(2)
> +#define INV_ICM42600_INT_STATUS_FIFO_FULL BIT(1)
> +#define INV_ICM42600_INT_STATUS_AGC_RDY BIT(0)
> +
> +/*
> + * FIFO access registers
> + * FIFO count is 16 bits (2 registers) big-endian
> + * FIFO data is a continuous read register to read FIFO content
> + */
> +#define INV_ICM42600_REG_FIFO_COUNT 0x002E
> +#define INV_ICM42600_REG_FIFO_DATA 0x0030
> +
> +#define INV_ICM42600_REG_SIGNAL_PATH_RESET 0x004B
> +#define INV_ICM42600_SIGNAL_PATH_RESET_DMP_INIT_EN BIT(6)
> +#define INV_ICM42600_SIGNAL_PATH_RESET_DMP_MEM_RESET BIT(5)
> +#define INV_ICM42600_SIGNAL_PATH_RESET_RESET BIT(3)
> +#define INV_ICM42600_SIGNAL_PATH_RESET_TMST_STROBE BIT(2)
> +#define INV_ICM42600_SIGNAL_PATH_RESET_FIFO_FLUSH BIT(1)
> +
> +/* default configuration: all data big-endian and fifo count in bytes */
> +#define INV_ICM42600_REG_INTF_CONFIG0 0x004C
> +#define INV_ICM42600_INTF_CONFIG0_FIFO_HOLD_LAST_DATA BIT(7)
> +#define INV_ICM42600_INTF_CONFIG0_FIFO_COUNT_REC BIT(6)
> +#define INV_ICM42600_INTF_CONFIG0_FIFO_COUNT_ENDIAN BIT(5)
> +#define INV_ICM42600_INTF_CONFIG0_SENSOR_DATA_ENDIAN BIT(4)
> +#define INV_ICM42600_INTF_CONFIG0_UI_SIFS_CFG_MASK GENMASK(1, 0)
> +#define INV_ICM42600_INTF_CONFIG0_UI_SIFS_CFG_SPI_DIS \
> + FIELD_PREP(INV_ICM42600_INTF_CONFIG0_UI_SIFS_CFG_MASK, 2)
> +#define INV_ICM42600_INTF_CONFIG0_UI_SIFS_CFG_I2C_DIS \
> + FIELD_PREP(INV_ICM42600_INTF_CONFIG0_UI_SIFS_CFG_MASK, 3)
> +
> +#define INV_ICM42600_REG_INTF_CONFIG1 0x004D
> +#define INV_ICM42600_INTF_CONFIG1_ACCEL_LP_CLK_RC BIT(3)
> +
> +#define INV_ICM42600_REG_PWR_MGMT0 0x004E
> +#define INV_ICM42600_PWR_MGMT0_TEMP_DIS BIT(5)
> +#define INV_ICM42600_PWR_MGMT0_IDLE BIT(4)
> +#define INV_ICM42600_PWR_MGMT0_GYRO(_mode) \
> + FIELD_PREP(GENMASK(3, 2), (_mode))
> +#define INV_ICM42600_PWR_MGMT0_ACCEL(_mode) \
> + FIELD_PREP(GENMASK(1, 0), (_mode))
> +
> +#define INV_ICM42600_REG_GYRO_CONFIG0 0x004F
> +#define INV_ICM42600_GYRO_CONFIG0_FS(_fs) \
> + FIELD_PREP(GENMASK(7, 5), (_fs))
> +#define INV_ICM42600_GYRO_CONFIG0_ODR(_odr) \
> + FIELD_PREP(GENMASK(3, 0), (_odr))
> +
> +#define INV_ICM42600_REG_ACCEL_CONFIG0 0x0050
> +#define INV_ICM42600_ACCEL_CONFIG0_FS(_fs) \
> + FIELD_PREP(GENMASK(7, 5), (_fs))
> +#define INV_ICM42600_ACCEL_CONFIG0_ODR(_odr) \
> + FIELD_PREP(GENMASK(3, 0), (_odr))
> +
> +#define INV_ICM42600_REG_GYRO_ACCEL_CONFIG0 0x0052
> +#define INV_ICM42600_GYRO_ACCEL_CONFIG0_ACCEL_FILT(_f) \
> + FIELD_PREP(GENMASK(7, 4), (_f))
> +#define INV_ICM42600_GYRO_ACCEL_CONFIG0_GYRO_FILT(_f) \
> + FIELD_PREP(GENMASK(3, 0), (_f))
> +
> +#define INV_ICM42600_REG_TMST_CONFIG 0x0054
> +#define INV_ICM42600_TMST_CONFIG_MASK GENMASK(4, 0)
> +#define INV_ICM42600_TMST_CONFIG_TMST_TO_REGS_EN BIT(4)
> +#define INV_ICM42600_TMST_CONFIG_TMST_RES_16US BIT(3)
> +#define INV_ICM42600_TMST_CONFIG_TMST_DELTA_EN BIT(2)
> +#define INV_ICM42600_TMST_CONFIG_TMST_FSYNC_EN BIT(1)
> +#define INV_ICM42600_TMST_CONFIG_TMST_EN BIT(0)
> +
> +#define INV_ICM42600_REG_FIFO_CONFIG1 0x005F
> +#define INV_ICM42600_FIFO_CONFIG1_RESUME_PARTIAL_RD BIT(6)
> +#define INV_ICM42600_FIFO_CONFIG1_WM_GT_TH BIT(5)
> +#define INV_ICM42600_FIFO_CONFIG1_TMST_FSYNC_EN BIT(3)
> +#define INV_ICM42600_FIFO_CONFIG1_TEMP_EN BIT(2)
> +#define INV_ICM42600_FIFO_CONFIG1_GYRO_EN BIT(1)
> +#define INV_ICM42600_FIFO_CONFIG1_ACCEL_EN BIT(0)
> +
> +/* FIFO watermark is 16 bits (2 registers wide) in little-endian */
> +#define INV_ICM42600_REG_FIFO_WATERMARK 0x0060
> +#define INV_ICM42600_FIFO_WATERMARK_VAL(_wm) \
> + cpu_to_le16((_wm) & GENMASK(11, 0))
> +/* FIFO is 2048 bytes, let 12 samples for reading latency */
> +#define INV_ICM42600_FIFO_WATERMARK_MAX (2048 - 12 * 16)
> +
> +#define INV_ICM42600_REG_INT_CONFIG1 0x0064
> +#define INV_ICM42600_INT_CONFIG1_TPULSE_DURATION BIT(6)
> +#define INV_ICM42600_INT_CONFIG1_TDEASSERT_DISABLE BIT(5)
> +#define INV_ICM42600_INT_CONFIG1_ASYNC_RESET BIT(4)
> +
> +#define INV_ICM42600_REG_INT_SOURCE0 0x0065
> +#define INV_ICM42600_INT_SOURCE0_UI_FSYNC_INT1_EN BIT(6)
> +#define INV_ICM42600_INT_SOURCE0_PLL_RDY_INT1_EN BIT(5)
> +#define INV_ICM42600_INT_SOURCE0_RESET_DONE_INT1_EN BIT(4)
> +#define INV_ICM42600_INT_SOURCE0_UI_DRDY_INT1_EN BIT(3)
> +#define INV_ICM42600_INT_SOURCE0_FIFO_THS_INT1_EN BIT(2)
> +#define INV_ICM42600_INT_SOURCE0_FIFO_FULL_INT1_EN BIT(1)
> +#define INV_ICM42600_INT_SOURCE0_UI_AGC_RDY_INT1_EN BIT(0)
> +
> +#define INV_ICM42600_REG_WHOAMI 0x0075
> +#define INV_ICM42600_WHOAMI_ICM42600 0x40
> +#define INV_ICM42600_WHOAMI_ICM42602 0x41
> +#define INV_ICM42600_WHOAMI_ICM42605 0x42
> +#define INV_ICM42600_WHOAMI_ICM42622 0x46
> +
> +/* User bank 1 (MSB 0x10) */
> +#define INV_ICM42600_REG_SENSOR_CONFIG0 0x1003
> +#define INV_ICM42600_SENSOR_CONFIG0_ZG_DISABLE BIT(5)
> +#define INV_ICM42600_SENSOR_CONFIG0_YG_DISABLE BIT(4)
> +#define INV_ICM42600_SENSOR_CONFIG0_XG_DISABLE BIT(3)
> +#define INV_ICM42600_SENSOR_CONFIG0_ZA_DISABLE BIT(2)
> +#define INV_ICM42600_SENSOR_CONFIG0_YA_DISABLE BIT(1)
> +#define INV_ICM42600_SENSOR_CONFIG0_XA_DISABLE BIT(0)
> +
> +/* Timestamp value is 20 bits (3 registers) in little-endian */
> +#define INV_ICM42600_REG_TMSTVAL 0x1062
> +#define INV_ICM42600_TMSTVAL_MASK GENMASK(19, 0)
> +
> +#define INV_ICM42600_REG_INTF_CONFIG4 0x107A
> +#define INV_ICM42600_INTF_CONFIG4_I3C_BUS_ONLY BIT(6)
> +#define INV_ICM42600_INTF_CONFIG4_SPI_AP_4WIRE BIT(1)
> +
> +#define INV_ICM42600_REG_INTF_CONFIG6 0x107C
> +#define INV_ICM42600_INTF_CONFIG6_MASK GENMASK(4, 0)
> +#define INV_ICM42600_INTF_CONFIG6_I3C_EN BIT(4)
> +#define INV_ICM42600_INTF_CONFIG6_I3C_IBI_BYTE_EN BIT(3)
> +#define INV_ICM42600_INTF_CONFIG6_I3C_IBI_EN BIT(2)
> +#define INV_ICM42600_INTF_CONFIG6_I3C_DDR_EN BIT(1)
> +#define INV_ICM42600_INTF_CONFIG6_I3C_SDR_EN BIT(0)
> +
> +/* User bank 4 (MSB 0x40) */
> +#define INV_ICM42600_REG_INT_SOURCE8 0x404F
> +#define INV_ICM42600_INT_SOURCE8_FSYNC_IBI_EN BIT(5)
> +#define INV_ICM42600_INT_SOURCE8_PLL_RDY_IBI_EN BIT(4)
> +#define INV_ICM42600_INT_SOURCE8_UI_DRDY_IBI_EN BIT(3)
> +#define INV_ICM42600_INT_SOURCE8_FIFO_THS_IBI_EN BIT(2)
> +#define INV_ICM42600_INT_SOURCE8_FIFO_FULL_IBI_EN BIT(1)
> +#define INV_ICM42600_INT_SOURCE8_AGC_RDY_IBI_EN BIT(0)
> +
> +#define INV_ICM42600_REG_OFFSET_USER0 0x4077
> +#define INV_ICM42600_REG_OFFSET_USER1 0x4078
> +#define INV_ICM42600_REG_OFFSET_USER2 0x4079
> +#define INV_ICM42600_REG_OFFSET_USER3 0x407A
> +#define INV_ICM42600_REG_OFFSET_USER4 0x407B
> +#define INV_ICM42600_REG_OFFSET_USER5 0x407C
> +#define INV_ICM42600_REG_OFFSET_USER6 0x407D
> +#define INV_ICM42600_REG_OFFSET_USER7 0x407E
> +#define INV_ICM42600_REG_OFFSET_USER8 0x407F
> +
> +/* Sleep times required by the driver */
> +#define INV_ICM42600_POWER_UP_TIME_MS 100
> +#define INV_ICM42600_RESET_TIME_MS 1
> +#define INV_ICM42600_ACCEL_STARTUP_TIME_MS 20
> +#define INV_ICM42600_GYRO_STARTUP_TIME_MS 60
> +#define INV_ICM42600_GYRO_STOP_TIME_MS 150
> +#define INV_ICM42600_TEMP_STARTUP_TIME_MS 14
> +#define INV_ICM42600_SUSPEND_DELAY_MS 2000
> +
> +typedef int (*inv_icm42600_bus_setup)(struct inv_icm42600_state *);
> +
> +extern const struct regmap_config inv_icm42600_regmap_config;
> +extern const struct dev_pm_ops inv_icm42600_pm_ops;
> +
> +const struct iio_mount_matrix *
> +inv_icm42600_get_mount_matrix(const struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan);
> +
> +uint32_t inv_icm42600_odr_to_period(enum inv_icm42600_odr odr);
> +
> +int inv_icm42600_set_accel_conf(struct inv_icm42600_state *st,
> + struct inv_icm42600_sensor_conf *conf,
> + unsigned int *sleep);
> +
> +int inv_icm42600_set_gyro_conf(struct inv_icm42600_state *st,
> + struct inv_icm42600_sensor_conf *conf,
> + unsigned int *sleep);
> +
> +int inv_icm42600_set_temp_conf(struct inv_icm42600_state *st, bool enable,
> + unsigned int *sleep);
> +
> +int inv_icm42600_debugfs_reg(struct iio_dev *indio_dev, unsigned int reg,
> + unsigned int writeval, unsigned int *readval);
> +
> +int inv_icm42600_core_probe(struct regmap *regmap, int chip,
> + inv_icm42600_bus_setup bus_setup);
> +
> +#endif
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> new file mode 100644
> index 000000000000..35bdf4f9d31e
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> @@ -0,0 +1,618 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2020 Invensense, Inc.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/iio/iio.h>
> +
> +#include "inv_icm42600.h"
> +
> +static const struct regmap_range_cfg inv_icm42600_regmap_ranges[] = {
> + {
> + .name = "user banks",
> + .range_min = 0x0000,
> + .range_max = 0x4FFF,
> + .selector_reg = INV_ICM42600_REG_BANK_SEL,
> + .selector_mask = INV_ICM42600_BANK_SEL_MASK,
> + .selector_shift = 0,
> + .window_start = 0,
> + .window_len = 0x1000,
> + },
> +};
> +
> +const struct regmap_config inv_icm42600_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = 0x4FFF,
> + .ranges = inv_icm42600_regmap_ranges,
> + .num_ranges = ARRAY_SIZE(inv_icm42600_regmap_ranges),
> +};
> +EXPORT_SYMBOL_GPL(inv_icm42600_regmap_config);
> +
> +struct inv_icm42600_hw {
> + uint8_t whoami;
> + const char *name;
> + const struct inv_icm42600_conf *conf;
> +};
> +
> +/* chip initial default configuration */
> +static const struct inv_icm42600_conf inv_icm42600_default_conf = {
> + .gyro = {
> + .mode = INV_ICM42600_SENSOR_MODE_OFF,
> + .fs = INV_ICM42600_GYRO_FS_2000DPS,
> + .odr = INV_ICM42600_ODR_50HZ,
> + .filter = INV_ICM42600_FILTER_BW_ODR_DIV_2,
> + },
> + .accel = {
> + .mode = INV_ICM42600_SENSOR_MODE_OFF,
> + .fs = INV_ICM42600_ACCEL_FS_16G,
> + .odr = INV_ICM42600_ODR_50HZ,
> + .filter = INV_ICM42600_FILTER_BW_ODR_DIV_2,
> + },
> + .temp_en = false,
> +};
> +
> +static const struct inv_icm42600_hw inv_icm42600_hw[] = {
> + [INV_CHIP_ICM42600] = {
> + .whoami = INV_ICM42600_WHOAMI_ICM42600,
> + .name = "icm42600",
> + .conf = &inv_icm42600_default_conf,
> + },
> + [INV_CHIP_ICM42602] = {
> + .whoami = INV_ICM42600_WHOAMI_ICM42602,
> + .name = "icm42602",
> + .conf = &inv_icm42600_default_conf,
> + },
> + [INV_CHIP_ICM42605] = {
> + .whoami = INV_ICM42600_WHOAMI_ICM42605,
> + .name = "icm42605",
> + .conf = &inv_icm42600_default_conf,
> + },
> + [INV_CHIP_ICM42622] = {
> + .whoami = INV_ICM42600_WHOAMI_ICM42622,
> + .name = "icm42622",
> + .conf = &inv_icm42600_default_conf,
> + },
> +};
> +
> +const struct iio_mount_matrix *
> +inv_icm42600_get_mount_matrix(const struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan)
> +{
> + const struct inv_icm42600_state *st =
> + iio_device_get_drvdata((struct iio_dev *)indio_dev);
Interesting... iio_device_get_drvdata is never going to modify
the struct iio_dev. Should we just change that to take a
const struct iio_dev * ?
> +
> + return &st->orientation;
> +}
> +
> +uint32_t inv_icm42600_odr_to_period(enum inv_icm42600_odr odr)
> +{
> + static uint32_t odr_periods[INV_ICM42600_ODR_NB] = {
> + /* reserved values */
> + 0, 0, 0,
> + /* 8kHz */
> + 125000,
> + /* 4kHz */
> + 250000,
> + /* 2kHz */
> + 500000,
> + /* 1kHz */
> + 1000000,
> + /* 200Hz */
> + 5000000,
> + /* 100Hz */
> + 10000000,
> + /* 50Hz */
> + 20000000,
> + /* 25Hz */
> + 40000000,
> + /* 12.5Hz */
> + 80000000,
> + /* 6.25Hz */
> + 160000000,
> + /* 3.125Hz */
> + 320000000,
> + /* 1.5625Hz */
> + 640000000,
> + /* 500Hz */
> + 2000000,
> + };
> +
> + return odr_periods[odr];
> +}
> +
> +static int inv_icm42600_set_pwr_mgmt0(struct inv_icm42600_state *st,
> + enum inv_icm42600_sensor_mode gyro,
> + enum inv_icm42600_sensor_mode accel,
> + bool temp, unsigned int *sleep)
msleep or similar that indicates the units of the sleep time.
> +{
> + enum inv_icm42600_sensor_mode oldgyro = st->conf.gyro.mode;
> + enum inv_icm42600_sensor_mode oldaccel = st->conf.accel.mode;
> + bool oldtemp = st->conf.temp_en;
> + unsigned int sleepval;
> + unsigned int val;
> + int ret;
> +
> + /* if nothing changed, exit */
> + if (gyro == oldgyro && accel == oldaccel && temp == oldtemp)
> + return 0;
> +
> + val = INV_ICM42600_PWR_MGMT0_GYRO(gyro) |
> + INV_ICM42600_PWR_MGMT0_ACCEL(accel);
> + if (!temp)
> + val |= INV_ICM42600_PWR_MGMT0_TEMP_DIS;
> + dev_dbg(regmap_get_device(st->map), "pwr_mgmt0: %#02x\n", val);
I wonder if you have a little too much in the way of debug prints.
These are internal to the code and so could only be wrong due to a local
bug. Once you've finished writing the driver I'd hope we won't need these!
> + ret = regmap_write(st->map, INV_ICM42600_REG_PWR_MGMT0, val);
> + if (ret)
> + return ret;
> +
> + st->conf.gyro.mode = gyro;
> + st->conf.accel.mode = accel;
> + st->conf.temp_en = temp;
> +
> + /* compute required wait time for sensors to stabilize */
> + sleepval = 0;
> + /* temperature stabilization time */
> + if (temp && !oldtemp) {
> + if (sleepval < INV_ICM42600_TEMP_STARTUP_TIME_MS)
> + sleepval = INV_ICM42600_TEMP_STARTUP_TIME_MS;
> + }
> + /* accel startup time */
> + if (accel != oldaccel && oldaccel == INV_ICM42600_SENSOR_MODE_OFF) {
> + /* block any register write for at least 200 µs */
> + usleep_range(200, 300);
> + if (sleepval < INV_ICM42600_ACCEL_STARTUP_TIME_MS)
> + sleepval = INV_ICM42600_ACCEL_STARTUP_TIME_MS;
> + }
> + if (gyro != oldgyro) {
> + /* gyro startup time */
> + if (oldgyro == INV_ICM42600_SENSOR_MODE_OFF) {
> + /* block any register write for at least 200 µs */
> + usleep_range(200, 300);
> + if (sleepval < INV_ICM42600_GYRO_STARTUP_TIME_MS)
> + sleepval = INV_ICM42600_GYRO_STARTUP_TIME_MS;
> + /* gyro stop time */
> + } else if (gyro == INV_ICM42600_SENSOR_MODE_OFF) {
> + if (sleepval < INV_ICM42600_GYRO_STOP_TIME_MS)
> + sleepval = INV_ICM42600_GYRO_STOP_TIME_MS;
> + }
> + }
> +
> + /* deferred sleep value if sleep pointer is provided or direct sleep */
> + if (sleep)
> + *sleep = sleepval;
> + else if (sleepval)
> + msleep(sleepval);
> +
> + return 0;
> +}
> +
> +int inv_icm42600_set_accel_conf(struct inv_icm42600_state *st,
> + struct inv_icm42600_sensor_conf *conf,
> + unsigned int *sleep)
> +{
> + struct inv_icm42600_sensor_conf *oldconf = &st->conf.accel;
> + unsigned int val;
> + int ret;
> +
> + /* Sanitize missing values with current values */
> + if (conf->mode < 0)
> + conf->mode = oldconf->mode;
> + if (conf->fs < 0)
> + conf->fs = oldconf->fs;
> + if (conf->odr < 0)
> + conf->odr = oldconf->odr;
> + if (conf->filter < 0)
> + conf->filter = oldconf->filter;
> +
> + /* set ACCEL_CONFIG0 register (accel fullscale & odr) */
> + if (conf->fs != oldconf->fs || conf->odr != oldconf->odr) {
> + val = INV_ICM42600_ACCEL_CONFIG0_FS(conf->fs) |
> + INV_ICM42600_ACCEL_CONFIG0_ODR(conf->odr);
> + dev_dbg(regmap_get_device(st->map), "accel_config0: %#02x\n", val);
> + ret = regmap_write(st->map, INV_ICM42600_REG_ACCEL_CONFIG0, val);
> + if (ret)
> + return ret;
> + oldconf->fs = conf->fs;
> + oldconf->odr = conf->odr;
> + }
> +
> + /* set GYRO_ACCEL_CONFIG0 register (accel filter) */
> + if (conf->filter != oldconf->filter) {
> + val = INV_ICM42600_GYRO_ACCEL_CONFIG0_ACCEL_FILT(conf->filter) |
> + INV_ICM42600_GYRO_ACCEL_CONFIG0_GYRO_FILT(st->conf.gyro.filter);
> + dev_dbg(regmap_get_device(st->map), "gyro_accel_config0: %#02x\n", val);
> + ret = regmap_write(st->map, INV_ICM42600_REG_GYRO_ACCEL_CONFIG0, val);
> + if (ret)
> + return ret;
> + oldconf->filter = conf->filter;
> + }
> +
> + /* set PWR_MGMT0 register (accel sensor mode) */
> + return inv_icm42600_set_pwr_mgmt0(st, st->conf.gyro.mode, conf->mode,
> + st->conf.temp_en, sleep);
> +}
> +
> +int inv_icm42600_set_gyro_conf(struct inv_icm42600_state *st,
> + struct inv_icm42600_sensor_conf *conf,
> + unsigned int *sleep)
> +{
> + struct inv_icm42600_sensor_conf *oldconf = &st->conf.gyro;
> + unsigned int val;
> + int ret;
> +
> + /* sanitize missing values with current values */
> + if (conf->mode < 0)
> + conf->mode = oldconf->mode;
> + if (conf->fs < 0)
> + conf->fs = oldconf->fs;
> + if (conf->odr < 0)
> + conf->odr = oldconf->odr;
> + if (conf->filter < 0)
> + conf->filter = oldconf->filter;
> +
> + /* set GYRO_CONFIG0 register (gyro fullscale & odr) */
> + if (conf->fs != oldconf->fs || conf->odr != oldconf->odr) {
> + val = INV_ICM42600_GYRO_CONFIG0_FS(conf->fs) |
> + INV_ICM42600_GYRO_CONFIG0_ODR(conf->odr);
> + dev_dbg(regmap_get_device(st->map), "gyro_config0: %#02x\n", val);
> + ret = regmap_write(st->map, INV_ICM42600_REG_GYRO_CONFIG0, val);
> + if (ret)
> + return ret;
> + oldconf->fs = conf->fs;
> + oldconf->odr = conf->odr;
> + }
> +
> + /* set GYRO_ACCEL_CONFIG0 register (gyro filter) */
> + if (conf->filter != oldconf->filter) {
> + val = INV_ICM42600_GYRO_ACCEL_CONFIG0_ACCEL_FILT(st->conf.accel.filter) |
> + INV_ICM42600_GYRO_ACCEL_CONFIG0_GYRO_FILT(conf->filter);
> + dev_dbg(regmap_get_device(st->map), "gyro_accel_config0: %#02x\n", val);
> + ret = regmap_write(st->map, INV_ICM42600_REG_GYRO_ACCEL_CONFIG0, val);
> + if (ret)
> + return ret;
> + oldconf->filter = conf->filter;
> + }
> +
> + /* set PWR_MGMT0 register (gyro sensor mode) */
> + return inv_icm42600_set_pwr_mgmt0(st, conf->mode, st->conf.accel.mode,
> + st->conf.temp_en, sleep);
> +
> + return 0;
> +}
> +
> +int inv_icm42600_set_temp_conf(struct inv_icm42600_state *st, bool enable,
> + unsigned int *sleep)
> +{
> + return inv_icm42600_set_pwr_mgmt0(st, st->conf.gyro.mode,
> + st->conf.accel.mode, enable,
> + sleep);
> +}
> +
> +int inv_icm42600_debugfs_reg(struct iio_dev *indio_dev, unsigned int reg,
> + unsigned int writeval, unsigned int *readval)
> +{
> + struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> + int ret;
> +
> + mutex_lock(&st->lock);
> +
> + if (readval)
> + ret = regmap_read(st->map, reg, readval);
> + else
> + ret = regmap_write(st->map, reg, writeval);
> +
> + mutex_unlock(&st->lock);
> +
> + return ret;
> +}
> +
> +static int inv_icm42600_set_conf(struct inv_icm42600_state *st,
> + const struct inv_icm42600_conf *conf)
> +{
> + unsigned int val;
> + int ret;
> +
> + /* set PWR_MGMT0 register (gyro & accel sensor mode, temp enabled) */
> + val = INV_ICM42600_PWR_MGMT0_GYRO(conf->gyro.mode) |
> + INV_ICM42600_PWR_MGMT0_ACCEL(conf->accel.mode);
> + if (!conf->temp_en)
> + val |= INV_ICM42600_PWR_MGMT0_TEMP_DIS;
> + ret = regmap_write(st->map, INV_ICM42600_REG_PWR_MGMT0, val);
> + if (ret)
> + return ret;
> +
> + /* set GYRO_CONFIG0 register (gyro fullscale & odr) */
> + val = INV_ICM42600_GYRO_CONFIG0_FS(conf->gyro.fs) |
> + INV_ICM42600_GYRO_CONFIG0_ODR(conf->gyro.odr);
> + ret = regmap_write(st->map, INV_ICM42600_REG_GYRO_CONFIG0, val);
> + if (ret)
> + return ret;
> +
> + /* set ACCEL_CONFIG0 register (accel fullscale & odr) */
> + val = INV_ICM42600_ACCEL_CONFIG0_FS(conf->accel.fs) |
> + INV_ICM42600_ACCEL_CONFIG0_ODR(conf->accel.odr);
> + ret = regmap_write(st->map, INV_ICM42600_REG_ACCEL_CONFIG0, val);
> + if (ret)
> + return ret;
> +
> + /* set GYRO_ACCEL_CONFIG0 register (gyro & accel filters) */
> + val = INV_ICM42600_GYRO_ACCEL_CONFIG0_ACCEL_FILT(conf->accel.filter) |
> + INV_ICM42600_GYRO_ACCEL_CONFIG0_GYRO_FILT(conf->gyro.filter);
> + ret = regmap_write(st->map, INV_ICM42600_REG_GYRO_ACCEL_CONFIG0, val);
> + if (ret)
> + return ret;
> +
> + /* update internal conf */
> + st->conf = *conf;
> +
> + return 0;
> +}
> +
> +/**
> + * inv_icm42600_setup() - check and setup chip.
If doing kernel-doc (which is good) you should do it all.
So document the parameters as well.
It's worth running the kernel-doc script over any file where
you put some and fixing up any warnings / errors.
> + */
> +static int inv_icm42600_setup(struct inv_icm42600_state *st,
> + inv_icm42600_bus_setup bus_setup)
> +{
> + const struct inv_icm42600_hw *hw = &inv_icm42600_hw[st->chip];
> + const struct device *dev = regmap_get_device(st->map);
> + unsigned int mask, val;
> + int ret;
> +
> + /* check chip self-identification value */
> + ret = regmap_read(st->map, INV_ICM42600_REG_WHOAMI, &val);
> + if (ret)
> + return ret;
> + if (val != hw->whoami) {
> + dev_err(dev, "invalid whoami %#02x expected %#02x (%s)\n",
> + val, hw->whoami, hw->name);
> + return -ENODEV;
> + }
> + dev_info(dev, "found %s (%#02x)\n", hw->name, hw->whoami);
Hmm. I'm never that keen on this sort of log noise. Why do you need it
except for initial debug?
> + st->name = hw->name;
> +
> + /* reset to make sure previous state are not there */
> + ret = regmap_write(st->map, INV_ICM42600_REG_DEVICE_CONFIG,
> + INV_ICM42600_DEVICE_CONFIG_SOFT_RESET);
> + if (ret)
> + return ret;
> + msleep(INV_ICM42600_RESET_TIME_MS);
blank line here to separate two logical blocks of code.
Slightly helps readability.
> + ret = regmap_read(st->map, INV_ICM42600_REG_INT_STATUS, &val);
> + if (ret)
> + return ret;
> + if (!(val & INV_ICM42600_INT_STATUS_RESET_DONE)) {
> + dev_err(dev, "reset error, reset done bit not set\n");
> + return -ENODEV;
> + }
> +
> + /* set chip bus configuration */
> + ret = bus_setup(st);
> + if (ret)
> + return ret;
> +
> + /* sensor data in big-endian (default) */
> + mask = INV_ICM42600_INTF_CONFIG0_SENSOR_DATA_ENDIAN;
> + val = INV_ICM42600_INTF_CONFIG0_SENSOR_DATA_ENDIAN;
> + ret = regmap_update_bits(st->map, INV_ICM42600_REG_INTF_CONFIG0,
> + mask, val);
Long line, but I'd rather you just didn't bother will local variables
in cases like this where you just set them to a constant.
Take the 80 chars thing as guidance not a rule :)
> + if (ret)
> + return ret;
> +
> + return inv_icm42600_set_conf(st, hw->conf);
> +}
> +
> +static int inv_icm42600_enable_regulator_vddio(struct inv_icm42600_state *st)
> +{
> + int ret;
> +
> + ret = regulator_enable(st->vddio_supply);
> + if (ret)
> + return ret;
> +
> + /* wait a little for supply ramp */
> + usleep_range(3000, 4000);
> +
> + return 0;
> +}
> +
> +static void inv_icm42600_disable_regulators(void *_data)
> +{
> + struct inv_icm42600_state *st = _data;
> + const struct device *dev = regmap_get_device(st->map);
> + int ret;
> +
> + ret = regulator_disable(st->vddio_supply);
> + if (ret)
> + dev_err(dev, "failed to disable vddio error %d\n", ret);
> +
> + ret = regulator_disable(st->vdd_supply);
> + if (ret)
> + dev_err(dev, "failed to disable vdd error %d\n", ret);
> +}
> +
> +static void inv_icm42600_disable_pm(void *_data)
> +{
> + struct device *dev = _data;
> +
> + pm_runtime_put_sync(dev);
> + pm_runtime_disable(dev);
> +}
> +
> +int inv_icm42600_core_probe(struct regmap *regmap, int chip,
> + inv_icm42600_bus_setup bus_setup)
> +{
> + struct device *dev = regmap_get_device(regmap);
> + struct inv_icm42600_state *st;
> + int ret;
> +
> + BUILD_BUG_ON(ARRAY_SIZE(inv_icm42600_hw) != INV_CHIP_NB);
Why not just give the array an explicit size when you define it above?
I guess it would in theory be possible to not instantiate all of the array
but relying on different size of a variable length array seems less than
ideal.
> + if (chip < 0 || chip >= INV_CHIP_NB) {
> + dev_err(dev, "invalid chip = %d\n", chip);
> + return -ENODEV;
> + }
> +
> + st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
> + if (!st)
> + return -ENOMEM;
nitpick: blank line here.
> + dev_set_drvdata(dev, st);
> + mutex_init(&st->lock);
> + st->chip = chip;
> + st->map = regmap;
> +
> + ret = iio_read_mount_matrix(dev, "mount-matrix", &st->orientation);
> + if (ret) {
> + dev_err(dev, "failed to retrieve mounting matrix %d\n", ret);
> + return ret;
> + }
> +
> + st->vdd_supply = devm_regulator_get(dev, "vdd");
> + if (IS_ERR(st->vdd_supply))
> + return PTR_ERR(st->vdd_supply);
> +
> + st->vddio_supply = devm_regulator_get(dev, "vddio");
> + if (IS_ERR(st->vddio_supply))
> + return PTR_ERR(st->vddio_supply);
> +
> + ret = regulator_enable(st->vdd_supply);
> + if (ret)
> + return ret;
> + msleep(INV_ICM42600_POWER_UP_TIME_MS);
> +
> + ret = inv_icm42600_enable_regulator_vddio(st);
> + if (ret) {
> + regulator_disable(st->vdd_supply);
> + return ret;
> + }
> +
> + ret = devm_add_action_or_reset(dev, inv_icm42600_disable_regulators,
> + st);
I'd prefer to see two devm_add_action_or_reset calls. One for each regulator.
That means you don't have to do the extra disable logic above which is
a bit fragile in amongst a whole load of device managed calls.
> + if (ret)
> + return ret;
> +
> + /* setup chip registers */
> + ret = inv_icm42600_setup(st, bus_setup);
> + if (ret)
> + return ret;
> +
> + /* setup runtime power management */
> + ret = pm_runtime_set_active(dev);
> + if (ret)
> + return ret;
> + pm_runtime_get_noresume(dev);
> + pm_runtime_enable(dev);
> + pm_runtime_set_autosuspend_delay(dev, INV_ICM42600_SUSPEND_DELAY_MS);
> + pm_runtime_use_autosuspend(dev);
> + pm_runtime_put(dev);
> +
> + return devm_add_action_or_reset(dev, inv_icm42600_disable_pm, dev);
> +}
> +EXPORT_SYMBOL_GPL(inv_icm42600_core_probe);
> +
> +static int __maybe_unused inv_icm42600_suspend(struct device *dev)
> +{
> + struct inv_icm42600_state *st = dev_get_drvdata(dev);
> + int ret;
> +
> + mutex_lock(&st->lock);
> +
> + st->suspended.gyro = st->conf.gyro.mode;
> + st->suspended.accel = st->conf.accel.mode;
> + st->suspended.temp = st->conf.temp_en;
> + if (pm_runtime_suspended(dev)) {
> + ret = 0;
> + goto out_unlock;
> + }
> +
> + ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
> + INV_ICM42600_SENSOR_MODE_OFF, false,
> + NULL);
> + if (ret)
> + goto out_unlock;
> +
> + regulator_disable(st->vddio_supply);
> +
> +out_unlock:
> + mutex_unlock(&st->lock);
> + return ret;
> +}
> +
> +static int __maybe_unused inv_icm42600_resume(struct device *dev)
> +{
> + struct inv_icm42600_state *st = dev_get_drvdata(dev);
> + int ret;
> +
> + mutex_lock(&st->lock);
> +
> + ret = inv_icm42600_enable_regulator_vddio(st);
> + if (ret)
> + goto out_unlock;
> +
> + pm_runtime_disable(dev);
> + pm_runtime_set_active(dev);
> + pm_runtime_enable(dev);
> +
> + /* restore sensors state */
> + ret = inv_icm42600_set_pwr_mgmt0(st, st->suspended.gyro,
> + st->suspended.accel,
> + st->suspended.temp, NULL);
> + if (ret)
> + goto out_unlock;
You may need this later, but for now it's a bit comic so ideally introduce
it only when needed.
> +
> +out_unlock:
> + mutex_unlock(&st->lock);
> + return ret;
> +}
> +
> +static int __maybe_unused inv_icm42600_runtime_suspend(struct device *dev)
> +{
> + struct inv_icm42600_state *st = dev_get_drvdata(dev);
> + int ret;
> +
> + mutex_lock(&st->lock);
> +
> + /* disable all sensors */
> + ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
> + INV_ICM42600_SENSOR_MODE_OFF, false,
> + NULL);
> + if (ret)
> + goto error_unlock;
> +
> + regulator_disable(st->vddio_supply);
> +
> +error_unlock:
> + mutex_unlock(&st->lock);
> + return ret;
> +}
> +
> +static int __maybe_unused inv_icm42600_runtime_resume(struct device *dev)
> +{
> + struct inv_icm42600_state *st = dev_get_drvdata(dev);
> + int ret;
> +
> + mutex_lock(&st->lock);
> +
Why don't we need to reenable all the sensors we disabled in runtime suspend?
I can guess why we might not, but a comment here to explain would save on
possible confusion..
> + ret = inv_icm42600_enable_regulator_vddio(st);
> +
> + mutex_unlock(&st->lock);
> + return ret;
> +}
> +
> +const struct dev_pm_ops inv_icm42600_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(inv_icm42600_suspend, inv_icm42600_resume)
> + SET_RUNTIME_PM_OPS(inv_icm42600_runtime_suspend,
> + inv_icm42600_runtime_resume, NULL)
> +};
> +EXPORT_SYMBOL_GPL(inv_icm42600_pm_ops);
> +
> +MODULE_AUTHOR("InvenSense, Inc.");
> +MODULE_DESCRIPTION("InvenSense ICM-426xx device driver");
> +MODULE_LICENSE("GPL");
^ permalink raw reply
* Re: [V6, 1/2] media: dt-bindings: media: i2c: Document DW9768 bindings
From: Tomasz Figa @ 2020-05-18 14:12 UTC (permalink / raw)
To: Dongchun Zhu
Cc: Linus Walleij, Bartosz Golaszewski, Mauro Carvalho Chehab,
Andy Shevchenko, Rob Herring, Mark Rutland, Sakari Ailus,
Nicolas Boichat, Matthias Brugger, Cao Bing Bu, srv_heupstream,
moderated list:ARM/Mediatek SoC support,
list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>,,
Sj Huang, Linux Media Mailing List, linux-devicetree, Louis Kuo,
Shengnan Wang (王圣男)
In-Reply-To: <20200518132731.20855-2-dongchun.zhu@mediatek.com>
Hi Dongchun,
On Mon, May 18, 2020 at 3:29 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
>
> Add DeviceTree binding documentation for Dongwoon Anatech DW9768 voice
> coil actuator.
Thanks for the patch. Please see my comments below.
>
> Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
This version includes significant changes, so the reviewed-by tag
shouldn't have been carried out.
> ---
> .../bindings/media/i2c/dongwoon,dw9768.yaml | 105 +++++++++++++++++++++
> MAINTAINERS | 7 ++
> 2 files changed, 112 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml b/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
> new file mode 100644
> index 0000000..b909e83
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
> @@ -0,0 +1,105 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright (c) 2020 MediaTek Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/dongwoon,dw9768.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Dongwoon Anatech DW9768 Voice Coil Motor (VCM) Lens Device Tree Bindings
> +
> +maintainers:
> + - Dongchun Zhu <dongchun.zhu@mediatek.com>
> +
> +description: |-
> + The Dongwoon DW9768 is a single 10-bit digital-to-analog (DAC) converter
> + with 100 mA output current sink capability. VCM current is controlled with
> + a linear mode driver. The DAC is controlled via a 2-wire (I2C-compatible)
> + serial interface that operates at clock rates up to 1MHz. This chip
> + integrates Advanced Actuator Control (AAC) technology and is intended for
> + driving voice coil lenses in camera modules.
> +
> +properties:
> + compatible:
> + enum:
> + # for DW9768 VCM
> + - dongwoon,dw9768
> + # for GT9769 VCM
> + - giantec,gt9769
> +
> + reg:
> + maxItems: 1
> +
> + vin-supply:
> + description:
> + Definition of the regulator used as I2C I/O interface power supply.
> +
> + vdd-supply:
> + description:
> + Definition of the regulator used as VCM chip power supply.
> +
> + dongwoon,aac-mode:
> + description:
> + Indication of AAC mode select.
> + allOf:
> + - $ref: "/schemas/types.yaml#/definitions/uint32"
> + - enum:
> + - 0 # Direct (default)
> + - 1 # AAC2 (operation time# 0.48 x Tvib)
> + - 2 # AAC3 (operation time# 0.70 x Tvib)
> + - 3 # AAC4 (operation time# 0.75 x Tvib)
> + - 4 # Reserved
> + - 5 # AAC8 (operation time# 1.13 x Tvib)
> + - 6 # Reserved
> + - 7 # Reserved
I'll ultimately leave it to DT maintainers, but is there any reason to
define the reserved values?
> +
> + dongwoon,aac-timing:
> + description:
> + Indication of AAC Timing count, unit of 0.1 milliseconds.
> + Valid values vary from 0 to 63 (default 32).
> + allOf:
> + - $ref: "/schemas/types.yaml#/definitions/uint32"
> +
> + dongwoon,clock-dividing-rate:
> + description:
> + Indication of VCM internal clock dividing rate select, as one multiple
> + factor to calculate VCM ring periodic time Tvib.
> + allOf:
> + - $ref: "/schemas/types.yaml#/definitions/uint32"
> + - enum:
> + - 0 # Dividing Rate - 2
> + - 1 # Dividing Rate - 1 (default)
> + - 2 # Dividing Rate - 1/2
> + - 3 # Dividing Rate - 1/4
> + - 4 # Dividing Rate - 8
> + - 5 # Dividing Rate - 4
> + - 6 # Dividing Rate - Reserved
> + - 7 # Dividing Rate - Reserved
Ditto.
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH v3 10/15] net: ethernet: mtk-eth-mac: new driver
From: Bartosz Golaszewski @ 2020-05-18 14:07 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Jonathan Corbet, Rob Herring, David S . Miller, Matthias Brugger,
John Crispin, Sean Wang, Mark Lee, Jakub Kicinski, Fabien Parent,
Heiner Kallweit, Edwin Peer, DTML, linux-kernel@vger.kernel.org,
Networking, Linux ARM, moderated list:ARM/Mediatek SoC...,
Stephane Le Provost, Pedro Tsai, Andrew Perepech,
Bartosz Golaszewski
In-Reply-To: <CAK8P3a0XgJtZNKePZUUpzADO25-JZKyDiVHFS_yuHRXTjvjDwg@mail.gmail.com>
pt., 15 maj 2020 o 15:32 Arnd Bergmann <arnd@arndb.de> napisał(a):
>
> On Thu, May 14, 2020 at 10:00 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > +static int mtk_mac_ring_pop_tail(struct mtk_mac_ring *ring,
> > + struct mtk_mac_ring_desc_data *desc_data)
>
> I took another look at this function because of your comment on the locking
> the descriptor updates, which seemed suspicious as the device side does not
> actually use the locks to access them
>
> > +{
> > + struct mtk_mac_ring_desc *desc = &ring->descs[ring->tail];
> > + unsigned int status;
> > +
> > + /* Let the device release the descriptor. */
> > + dma_rmb();
> > + status = desc->status;
> > + if (!(status & MTK_MAC_DESC_BIT_COWN))
> > + return -1;
>
> The dma_rmb() seems odd here, as I don't see which prior read
> is being protected by this.
>
> > + desc_data->len = status & MTK_MAC_DESC_MSK_LEN;
> > + desc_data->flags = status & ~MTK_MAC_DESC_MSK_LEN;
> > + desc_data->dma_addr = ring->dma_addrs[ring->tail];
> > + desc_data->skb = ring->skbs[ring->tail];
> > +
> > + desc->data_ptr = 0;
> > + desc->status = MTK_MAC_DESC_BIT_COWN;
> > + if (status & MTK_MAC_DESC_BIT_EOR)
> > + desc->status |= MTK_MAC_DESC_BIT_EOR;
> > +
> > + /* Flush writes to descriptor memory. */
> > + dma_wmb();
>
> The comment and the barrier here seem odd as well. I would have expected
> a barrier after the update to the data pointer, and only a single store
> but no read of the status flag instead of the read-modify-write,
> something like
>
> desc->data_ptr = 0;
> dma_wmb(); /* make pointer update visible before status update */
> desc->status = MTK_MAC_DESC_BIT_COWN | (status & MTK_MAC_DESC_BIT_EOR);
>
> > + ring->tail = (ring->tail + 1) % MTK_MAC_RING_NUM_DESCS;
> > + ring->count--;
>
> I would get rid of the 'count' here, as it duplicates the information
> that is already known from the difference between head and tail, and you
> can't update it atomically without holding a lock around the access to
> the ring. The way I'd do this is to have the head and tail pointers
> in separate cache lines, and then use READ_ONCE/WRITE_ONCE
> and smp barriers to access them, with each one updated on one
> thread but read by the other.
>
Your previous solution seems much more reliable though. For instance
in the above: when we're doing the TX cleanup (we got the TX ready
irq, we're iterating over descriptors until we know there are no more
packets scheduled (count == 0) or we encounter one that's still owned
by DMA), a parallel TX path can schedule new packets to be sent and I
don't see how we can atomically check the count (understood as a
difference between tail and head) and run a new iteration (where we'd
modify the head or tail) without risking the other path getting in the
way. We'd have to always check the descriptor.
I experimented a bit with this and couldn't come up with anything that
would pass any stress test.
On the other hand: spin_lock_bh() works fine and I like your approach
from the previous e-mail - except for the work for updating stats as
we could potentially lose some stats when we're updating in process
context with RX/TX paths running in parallel in napi context but that
would be rare enough to overlook it.
I hope v4 will be good enough even with spinlocks. :)
Bart
^ permalink raw reply
* Re: [PATCH v3 7/7] clocksource: mips-gic-timer: Set limitations on clocksource/sched-clocks usage
From: Daniel Lezcano @ 2020-05-18 13:59 UTC (permalink / raw)
To: Serge Semin
Cc: Serge Semin, Thomas Bogendoerfer, Thomas Gleixner, Alexey Malahov,
Paul Burton, Ralf Baechle, Alessandro Zummo, Alexandre Belloni,
Arnd Bergmann, Rob Herring, linux-mips, linux-rtc, devicetree,
Vincenzo Frascino, linux-kernel
In-Reply-To: <20200516121647.g6jua35kkihmw5r6@mobilestation>
On 16/05/2020 14:16, Serge Semin wrote:
> Hello Daniel,
>
> Thanks for your comment. My response is below.
>
> On Fri, May 15, 2020 at 07:10:04PM +0200, Daniel Lezcano wrote:
>> On Thu, May 07, 2020 at 12:41:07AM +0300, Serge Semin wrote:
>>> Currently neither clocksource nor scheduler clock kernel framework
>>> support the clocks with variable frequency. Needless to say how many
>>> problems may cause the sudden base clocks frequency change. In a
>>> simplest case the system time will either slow down or speed up.
>>> Since on CM2.5 and earlier MIPS GIC timer is synchronously clocked
>>> with CPU we must set some limitations on using it for these frameworks
>>> if CPU frequency may change. First of all it's not safe to have the
>>> MIPS GIC used for scheduler timings. So we shouldn't proceed with
>>> the clocks registration in the sched-subsystem. Secondly we must
>>> significantly decrease the MIPS GIC clocksource rating. This will let
>>> the system to use it only as a last resort.
>>>
>>> Note CM3.x-based systems may also experience the problems with MIPS GIC
>>> if the CPU-frequency change is activated for the whole CPU cluster
>>> instead of using the individual CPC core clocks divider.
>>
>> May be there is no alternative but the code looks a bit hacksih. Isn't possible
>> to do something with the sched_mark_unstable?
>>
>> Or just not use the timer at all ?
>
> Not using the timer might be better, but not that good alternative either
> especially in our case due to very slow external timer. Me and Thomas
> Bogendoerfer discussed the similar commit I've provided to the csrc-r4k driver
> available on MIPS:
> https://lkml.org/lkml/2020/5/11/576
>
> To cut it short, you are right. The solution with using clocksource_mark_unstable()
> is better alternative spied up in x86 tsc implementation. I'll use a similar
> approach here and submit the updated patch in v3.
>
> Could you please proceed with the rest of the series review? I'd like to send
> the next version with as many comments taken into account as possible. The
> patchset has been submitted a while ago, but except Rob noone have had any
> comments.(
For me other patches are ok.
I can apply patches 1, 2, 4, 5, 6
Will remain patches 3 et 7
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
^ permalink raw reply
* Re: [PATCH v4 00/12] PCI: aardvark: Fix support for Turris MOX and Compex wifi cards
From: Marek Behun @ 2020-05-18 13:50 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Pali Rohár, Gregory CLEMENT, Andrew Murray, Bjorn Helgaas,
Thomas Petazzoni, Jason Cooper, Andrew Lunn,
Sebastian Hesselbarth, Rob Herring, Remi Pommarel,
Tomasz Maciej Nowak, Xogium, devicetree, linux-kernel, linux-pci
In-Reply-To: <20200518134614.GA31554@e121166-lin.cambridge.arm.com>
On Mon, 18 May 2020 14:46:14 +0100
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> On Mon, May 18, 2020 at 12:30:04PM +0200, Pali Rohár wrote:
> > On Sunday 17 May 2020 17:57:02 Gregory CLEMENT wrote:
> > > Hello,
> > >
> > > > Hello,
> > > >
> > > > On Thu, 30 Apr 2020 10:06:13 +0200
> > > > Pali Rohár <pali@kernel.org> wrote:
> > > >
> > > >> Marek Behún (5):
> > > >> PCI: aardvark: Improve link training
> > > >> PCI: aardvark: Add PHY support
> > > >> dt-bindings: PCI: aardvark: Describe new properties
> > > >> arm64: dts: marvell: armada-37xx: Set pcie_reset_pin to gpio function
> > > >> arm64: dts: marvell: armada-37xx: Move PCIe comphy handle property
> > > >>
> > > >> Pali Rohár (7):
> > > >> PCI: aardvark: Train link immediately after enabling training
> > > >> PCI: aardvark: Don't blindly enable ASPM L0s and don't write to
> > > >> read-only register
> > > >> PCI: of: Zero max-link-speed value is invalid
> > > >> PCI: aardvark: Issue PERST via GPIO
> > > >> PCI: aardvark: Add FIXME comment for PCIE_CORE_CMD_STATUS_REG access
> > > >> PCI: aardvark: Replace custom macros by standard linux/pci_regs.h
> > > >> macros
> > > >> arm64: dts: marvell: armada-37xx: Move PCIe max-link-speed property
> > > >
> > > > Thanks a lot for this work. For a number of reasons, I'm less involved
> > > > in Marvell platform support in Linux, but I reviewed your series and
> > > > followed the discussions around it, and I'm happy to give my:
> > > >
> > > > Acked-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> > >
> > > With this acked-by for the series, the reviewed-by from Rob on the
> > > binding and the tested-by, I am pretty confident so I applied the
> > > patches 10, 11 and 12 on mvebu/dt64.
> > >
> > > Thanks,
> > >
> > > Gregory
> >
> > Thank you!
> >
> > Lorenzo, would you now take remaining patches?
>
> Yes - even though I have reservations about patch (5) and the
> problem is related to a complete lack of programming model for
> these host controllers and a clear separation between what's
> done in the OS vs bootloader, PERST handling in this host
> bridge is *really* a mess.
>
> I applied 1-9 to pci/aardvark.
>
> Lorenzo
Hooray, thanks, Lorenzo (and everyone else).
^ permalink raw reply
* Re: [RESEND PATCH v8 0/3] Add Intel ComboPhy driver
From: Kishon Vijay Abraham I @ 2020-05-18 13:49 UTC (permalink / raw)
To: Dilip Kota, linux-kernel, vkoul, devicetree
Cc: robh, andriy.shevchenko, cheol.yong.kim, chuanhua.lei, qi-ming.wu,
yixin.zhu
In-Reply-To: <cover.1589530082.git.eswara.kota@linux.intel.com>
Dilip,
On 5/15/2020 1:43 PM, Dilip Kota wrote:
> This patch series adds Intel ComboPhy driver, respective yaml schemas
>
> Changes on v8:
> As per PHY Maintainer's request add description in comments for doing
> register access through register map framework.
>
> Changes on v7:
> As per System control driver maintainer's inputs remove
> fwnode_to_regmap() definition and use device_node_get_regmap()
Can you fix this warning and resend the patch?
drivers/phy/intel/phy-intel-combo.c:229:6: warning: ‘cb_mode’ may be used
uninitialized in this function [-Wmaybe-uninitialized]
ret = regmap_write(cbphy->hsiocfg, REG_COMBO_MODE(cbphy->bid), cb_mode);
~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/phy/intel/phy-intel-combo.c:204:24: note: ‘cb_mode’ was declared here
enum intel_combo_mode cb_mode;
^~~~~~~
Thanks
Kishon
>
> Changes on v6:
> Rebase patches on the latest maintainer's branch
> https://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git/?h=phy-for-5.7
> Dilip Kota (3):
> dt-bindings: phy: Add PHY_TYPE_XPCS definition
> dt-bindings: phy: Add YAML schemas for Intel ComboPhy
> phy: intel: Add driver support for ComboPhy
>
> .../devicetree/bindings/phy/intel,combo-phy.yaml | 101 ++++
> drivers/phy/intel/Kconfig | 14 +
> drivers/phy/intel/Makefile | 1 +
> drivers/phy/intel/phy-intel-combo.c | 632 +++++++++++++++++++++
> include/dt-bindings/phy/phy.h | 1 +
> 5 files changed, 749 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
> create mode 100644 drivers/phy/intel/phy-intel-combo.c
>
^ permalink raw reply
* Re: [PATCH v2 18/20] mips: csrc-r4k: Decrease r4k-clocksource rating if CPU_FREQ enabled
From: Serge Semin @ 2020-05-18 13:48 UTC (permalink / raw)
To: Thomas Bogendoerfer
Cc: Serge Semin, Alexey Malahov, Paul Burton, Ralf Baechle,
Greg Kroah-Hartman, Arnd Bergmann, Rob Herring, linux-pm,
devicetree, Vincenzo Frascino, Thomas Gleixner, linux-mips,
linux-kernel
In-Reply-To: <20200515210647.GA22922@alpha.franken.de>
On Fri, May 15, 2020 at 11:06:47PM +0200, Thomas Bogendoerfer wrote:
> On Fri, May 15, 2020 at 10:48:27AM +0300, Serge Semin wrote:
> > Thomas,
> > Could you take a look at my comment below so I could proceed with the
> > patchset v3 development?
>
> I can't help, but using r4k clocksource with changing frequency is
> probaly only usefull as a random generator. So IMHO the only two
> options are disabling it or implement what arch/x86/kernel/tsc.c does.
>
> Thomas.
Thomas, could you proceed with the rest of the patches review?
├─>[PATCH v2 16/20] bus: cdmm: Add MIPS R5 arch support
├─>[PATCH v2 15/20] mips: cdmm: Add mti,mips-cdmm dtb node support
├─>[PATCH v2 13/20] mips: early_printk_8250: Use offset-sized IO-mem accessors
├─>[PATCH v2 12/20] mips: MAAR: Add XPA mode support
├─>[PATCH v2 10/20] mips: Add CONFIG/CONFIG6/Cause reg fields macro
└─>[PATCH v2 09/20] mips: Add CP0 Write Merge config support
It would be great if I fixed more comments in the next patchset version.
-Sergey
>
> --
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea. [ RFC1925, 2.3 ]
^ permalink raw reply
* Re: [PATCH v4 00/12] PCI: aardvark: Fix support for Turris MOX and Compex wifi cards
From: Lorenzo Pieralisi @ 2020-05-18 13:46 UTC (permalink / raw)
To: Pali Rohár
Cc: Gregory CLEMENT, Andrew Murray, Bjorn Helgaas, Thomas Petazzoni,
Jason Cooper, Andrew Lunn, Sebastian Hesselbarth, Rob Herring,
Remi Pommarel, Marek Behún, Tomasz Maciej Nowak, Xogium,
devicetree, linux-kernel, linux-pci
In-Reply-To: <20200518103004.6tydnad3apkfn77y@pali>
On Mon, May 18, 2020 at 12:30:04PM +0200, Pali Rohár wrote:
> On Sunday 17 May 2020 17:57:02 Gregory CLEMENT wrote:
> > Hello,
> >
> > > Hello,
> > >
> > > On Thu, 30 Apr 2020 10:06:13 +0200
> > > Pali Rohár <pali@kernel.org> wrote:
> > >
> > >> Marek Behún (5):
> > >> PCI: aardvark: Improve link training
> > >> PCI: aardvark: Add PHY support
> > >> dt-bindings: PCI: aardvark: Describe new properties
> > >> arm64: dts: marvell: armada-37xx: Set pcie_reset_pin to gpio function
> > >> arm64: dts: marvell: armada-37xx: Move PCIe comphy handle property
> > >>
> > >> Pali Rohár (7):
> > >> PCI: aardvark: Train link immediately after enabling training
> > >> PCI: aardvark: Don't blindly enable ASPM L0s and don't write to
> > >> read-only register
> > >> PCI: of: Zero max-link-speed value is invalid
> > >> PCI: aardvark: Issue PERST via GPIO
> > >> PCI: aardvark: Add FIXME comment for PCIE_CORE_CMD_STATUS_REG access
> > >> PCI: aardvark: Replace custom macros by standard linux/pci_regs.h
> > >> macros
> > >> arm64: dts: marvell: armada-37xx: Move PCIe max-link-speed property
> > >
> > > Thanks a lot for this work. For a number of reasons, I'm less involved
> > > in Marvell platform support in Linux, but I reviewed your series and
> > > followed the discussions around it, and I'm happy to give my:
> > >
> > > Acked-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> >
> > With this acked-by for the series, the reviewed-by from Rob on the
> > binding and the tested-by, I am pretty confident so I applied the
> > patches 10, 11 and 12 on mvebu/dt64.
> >
> > Thanks,
> >
> > Gregory
>
> Thank you!
>
> Lorenzo, would you now take remaining patches?
Yes - even though I have reservations about patch (5) and the
problem is related to a complete lack of programming model for
these host controllers and a clear separation between what's
done in the OS vs bootloader, PERST handling in this host
bridge is *really* a mess.
I applied 1-9 to pci/aardvark.
Lorenzo
^ permalink raw reply
* Re: [PATCH v2 10/19] spi: dw: Use DMA max burst to set the request thresholds
From: Serge Semin @ 2020-05-18 13:43 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Serge Semin, Andy Shevchenko, Mark Brown, Alexey Malahov,
Thomas Bogendoerfer, Paul Burton, Ralf Baechle, Arnd Bergmann,
Allison Randal, Gareth Williams, Rob Herring, linux-mips,
devicetree, Georgy Vlasov, Ramil Zaripov, Jarkko Nikula,
Thomas Gleixner, Wan Ahmad Zainie, Linus Walleij, Clement Leger,
linux-spi, Linux Kernel Mailing List
In-Reply-To: <CAHp75VeMcv-hQViCANQARiNh0LwmugsDWk=MF1c5E3t7z5h02Q@mail.gmail.com>
On Mon, May 18, 2020 at 04:25:20PM +0300, Andy Shevchenko wrote:
> On Mon, May 18, 2020 at 3:53 PM Serge Semin
> <Sergey.Semin@baikalelectronics.ru> wrote:
> > On Mon, May 18, 2020 at 02:03:43PM +0300, Andy Shevchenko wrote:
> > > On Sat, May 16, 2020 at 11:01:33PM +0300, Serge Semin wrote:
> > > > On Fri, May 15, 2020 at 05:38:42PM +0300, Andy Shevchenko wrote:
> > > > > On Fri, May 15, 2020 at 01:47:49PM +0300, Serge Semin wrote:
>
> ...
>
> > > > > > struct dma_chan *txchan;
> > > > > > + u32 txburst;
> > > > > > struct dma_chan *rxchan;
> > > > > > + u32 rxburst;
> > > > >
> > > > > Leave u32 together, it may be optimal on 64-bit architectures where ABIs require padding.
> > > >
> > > > It's not like anyone cared about padding in this structure in the first place)
> > >
> > > I think I have been caring (to some extend).
> >
> > Well, If you have then instead of asking to rearrange just two members (which
> > by the way finely grouped by the Tx-Rx affiliation) why not sending a
> > patch, which would refactor the whole structure so to be optimal for the x64
> > platforms? I don't really see why this gets very important for you seeing
> > Mark is Ok with this. My current commit follows the common driver design
> > including the DW SSI data members grouping. On the second thought I'll leave
> > it as is then.
>
> Again same issue here. What is really easy to do for you here, will
> become a burden and additional churn to anybody else.
> So, why not to minimize it in the first place? Same with comma in
> another patch. Sorry, I really don't get it.
If comma is more or less understandable (though adding it is absolutely
redundant there and doesn't worth even a bit of time spending for the
discussion), here you consider the patch from padding point of view.
The driver developer didn't care about it, but did care about grouping the
members in a corresponding way. The padding burden will be there anyway and
should be fixed for the whole structure in an additional patch. Until then
the way of grouping should be preserved.
-Sergey
>
> --
> With Best Regards,
> Andy Shevchenko
^ permalink raw reply
* [PATCH 3/3] iio: magnetometer: ak8975: Add gpio reset support
From: Jonathan Albrieux @ 2020-05-18 13:36 UTC (permalink / raw)
To: linux-kernel
Cc: Jonathan Albrieux, Andy Shevchenko,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Greg Kroah-Hartman, Hartmut Knaack, Jonathan Cameron,
Lars-Peter Clausen, Linus Walleij,
open list:IIO SUBSYSTEM AND DRIVERS, Peter Meerwald-Stadler,
Steve Winslow, Thomas Gleixner, Jonathan Cameron
In-Reply-To: <20200518133645.19127-1-jonathan.albrieux@gmail.com>
Set reset gpio to high on ak8975_power_on, set reset gpio to low on
ak8975_power_off.
Without setting it to high on ak8975_power_on driver's probe fails
on ak8975_who_i_am while checking for device identity for AK09911 chip
AK09911 has a reset gpio to handle register's reset. If reset gpio is
set to low it will trigger the reset. AK09911 datasheed says that if not
used reset pin should be connected to VID and this patch emulates this
situation
Signed-off-by: Jonathan Albrieux <jonathan.albrieux@gmail.com>
---
drivers/iio/magnetometer/ak8975.c | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
index 3c881541ae72..c1ba5cd2cb6c 100644
--- a/drivers/iio/magnetometer/ak8975.c
+++ b/drivers/iio/magnetometer/ak8975.c
@@ -358,6 +358,7 @@ struct ak8975_data {
u8 asa[3];
long raw_to_gauss[3];
struct gpio_desc *eoc_gpiod;
+ struct gpio_desc *reset_gpiod;
int eoc_irq;
wait_queue_head_t data_ready_queue;
unsigned long flags;
@@ -384,10 +385,13 @@ static int ak8975_power_on(const struct ak8975_data *data)
"Failed to enable specified Vid supply\n");
return ret;
}
+
+ gpiod_set_value_cansleep(data->reset_gpiod, 0);
+
/*
- * According to the datasheet the power supply rise time i 200us
+ * According to the datasheet the power supply rise time is 200us
* and the minimum wait time before mode setting is 100us, in
- * total 300 us. Add some margin and say minimum 500us here.
+ * total 300us. Add some margin and say minimum 500us here.
*/
usleep_range(500, 1000);
return 0;
@@ -396,6 +400,8 @@ static int ak8975_power_on(const struct ak8975_data *data)
/* Disable attached power regulator if any. */
static void ak8975_power_off(const struct ak8975_data *data)
{
+ gpiod_set_value_cansleep(data->reset_gpiod, 1);
+
regulator_disable(data->vid);
regulator_disable(data->vdd);
}
@@ -839,6 +845,7 @@ static int ak8975_probe(struct i2c_client *client,
struct ak8975_data *data;
struct iio_dev *indio_dev;
struct gpio_desc *eoc_gpiod;
+ struct gpio_desc *reset_gpiod;
const void *match;
unsigned int i;
int err;
@@ -856,6 +863,15 @@ static int ak8975_probe(struct i2c_client *client,
if (eoc_gpiod)
gpiod_set_consumer_name(eoc_gpiod, "ak_8975");
+ /*
+ * If reset pin is provided then will be set to high on power on
+ * and to low on power off according to AK09911 datasheet
+ */
+ reset_gpiod = devm_gpiod_get_optional(&client->dev,
+ "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(reset_gpiod))
+ return PTR_ERR(reset_gpiod);
+
/* Register with IIO */
indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
if (indio_dev == NULL)
@@ -866,6 +882,7 @@ static int ak8975_probe(struct i2c_client *client,
data->client = client;
data->eoc_gpiod = eoc_gpiod;
+ data->reset_gpiod = reset_gpiod;
data->eoc_irq = 0;
err = iio_read_mount_matrix(&client->dev, "mount-matrix", &data->orientation);
--
2.17.1
^ permalink raw reply related
* [PATCH 2/3] dt-bindings: iio: magnetometer: ak8975: add gpio reset support
From: Jonathan Albrieux @ 2020-05-18 13:36 UTC (permalink / raw)
To: linux-kernel
Cc: Jonathan Albrieux, Andy Shevchenko,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Greg Kroah-Hartman, Hartmut Knaack, Jonathan Cameron,
Lars-Peter Clausen, Linus Walleij,
open list:IIO SUBSYSTEM AND DRIVERS, Peter Meerwald-Stadler,
Steve Winslow, Thomas Gleixner, Jonathan Cameron, Rob Herring
In-Reply-To: <20200518133645.19127-1-jonathan.albrieux@gmail.com>
Add reset-gpio support.
AK09911 has a reset gpio to handle register's reset. If reset gpio is
set to low it will trigger the reset. AK09911 datasheed says that if not
used reset pin should be connected to VID and this patch emulates this
situation
Without setting it to high on ak8975_power_on driver's probe fails
on ak8975_who_i_am while checking for device identity for AK09911 chip
Signed-off-by: Jonathan Albrieux <jonathan.albrieux@gmail.com>
---
.../devicetree/bindings/iio/magnetometer/ak8975.yaml | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/iio/magnetometer/ak8975.yaml b/Documentation/devicetree/bindings/iio/magnetometer/ak8975.yaml
index 86e3efa693a8..a82c0ff5d098 100644
--- a/Documentation/devicetree/bindings/iio/magnetometer/ak8975.yaml
+++ b/Documentation/devicetree/bindings/iio/magnetometer/ak8975.yaml
@@ -37,6 +37,9 @@ properties:
mount-matrix:
description: an optional 3x3 mounting rotation matrix
+ reset-gpio:
+ description: an optional pin needed for AK09911 to set the reset state
+
required:
- compatible
- reg
@@ -53,6 +56,7 @@ examples:
reg = <0x0c>;
gpios = <&gpj0 7 0>;
vdd-supply = <&ldo_3v3_gnss>;
+ reset-gpio = <&msmgpio 111 1>;
mount-matrix = "-0.984807753012208", /* x0 */
"0", /* y0 */
"-0.173648177666930", /* z0 */
--
2.17.1
^ permalink raw reply related
* [PATCH 1/3] dt-bindings: iio: magnetometer: ak8975: convert txt format to yaml
From: Jonathan Albrieux @ 2020-05-18 13:36 UTC (permalink / raw)
To: linux-kernel
Cc: Jonathan Albrieux, Andy Shevchenko,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Greg Kroah-Hartman, Hartmut Knaack, Jonathan Cameron,
Lars-Peter Clausen, Linus Walleij,
open list:IIO SUBSYSTEM AND DRIVERS, Peter Meerwald-Stadler,
Steve Winslow, Thomas Gleixner, Jonathan Cameron, Rob Herring
In-Reply-To: <20200518133645.19127-1-jonathan.albrieux@gmail.com>
Converts documentation from txt format to yaml
Signed-off-by: Jonathan Albrieux <jonathan.albrieux@gmail.com>
---
.../bindings/iio/magnetometer/ak8975.txt | 30 ---------
.../bindings/iio/magnetometer/ak8975.yaml | 66 +++++++++++++++++++
2 files changed, 66 insertions(+), 30 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt
create mode 100644 Documentation/devicetree/bindings/iio/magnetometer/ak8975.yaml
diff --git a/Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt b/Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt
deleted file mode 100644
index aa67ceb0d4e0..000000000000
--- a/Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt
+++ /dev/null
@@ -1,30 +0,0 @@
-* AsahiKASEI AK8975 magnetometer sensor
-
-Required properties:
-
- - compatible : should be "asahi-kasei,ak8975"
- - reg : the I2C address of the magnetometer
-
-Optional properties:
-
- - gpios : should be device tree identifier of the magnetometer DRDY pin
- - vdd-supply: an optional regulator that needs to be on to provide VDD
- - mount-matrix: an optional 3x3 mounting rotation matrix
-
-Example:
-
-ak8975@c {
- compatible = "asahi-kasei,ak8975";
- reg = <0x0c>;
- gpios = <&gpj0 7 0>;
- vdd-supply = <&ldo_3v3_gnss>;
- mount-matrix = "-0.984807753012208", /* x0 */
- "0", /* y0 */
- "-0.173648177666930", /* z0 */
- "0", /* x1 */
- "-1", /* y1 */
- "0", /* z1 */
- "-0.173648177666930", /* x2 */
- "0", /* y2 */
- "0.984807753012208"; /* z2 */
-};
diff --git a/Documentation/devicetree/bindings/iio/magnetometer/ak8975.yaml b/Documentation/devicetree/bindings/iio/magnetometer/ak8975.yaml
new file mode 100644
index 000000000000..86e3efa693a8
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/magnetometer/ak8975.yaml
@@ -0,0 +1,66 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/magnetometer/ak8975.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AsahiKASEI AK8975 magnetometer sensor
+
+maintainers:
+ - can't find a mantainer, author is Laxman Dewangan <ldewangan@nvidia.com>
+
+properties:
+ compatible:
+ enum:
+ - "asahi-kasei,ak8975"
+ - "ak8975"
+ - "asahi-kasei,ak8963"
+ - "ak8963"
+ - "asahi-kasei,ak09911"
+ - "ak09911"
+ - "asahi-kasei,ak09912"
+ - "ak09912"
+
+ reg:
+ maxItems: 1
+ description: the I2C address of the magnetometer
+
+ gpios:
+ description: should be device tree identifier of the magnetometer DRDY pin
+
+ vdd-supply:
+ maxItems: 1
+ description: |
+ an optional regulator that needs to be on to provide VDD power to
+ the sensor.
+
+ mount-matrix:
+ description: an optional 3x3 mounting rotation matrix
+
+required:
+ - compatible
+ - reg
+
+examples:
+ - |
+ i2c@78b7000 {
+ reg = <0x78b6000 0x600>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ak8975@c {
+ compatible = "asahi-kasei,ak8975";
+ reg = <0x0c>;
+ gpios = <&gpj0 7 0>;
+ vdd-supply = <&ldo_3v3_gnss>;
+ mount-matrix = "-0.984807753012208", /* x0 */
+ "0", /* y0 */
+ "-0.173648177666930", /* z0 */
+ "0", /* x1 */
+ "-1", /* y1 */
+ "0", /* z1 */
+ "-0.173648177666930", /* x2 */
+ "0", /* y2 */
+ "0.984807753012208"; /* z2 */
+ };
+ };
--
2.17.1
^ permalink raw reply related
* [PATCH 0/3] iio: magnetometer: ak8975: Add gpio reset support
From: Jonathan Albrieux @ 2020-05-18 13:36 UTC (permalink / raw)
To: linux-kernel
Cc: Jonathan Albrieux, Andy Shevchenko,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Greg Kroah-Hartman, Hartmut Knaack, Jonathan Cameron,
Lars-Peter Clausen, Linus Walleij,
open list:IIO SUBSYSTEM AND DRIVERS, Peter Meerwald-Stadler,
Steve Winslow, Thomas Gleixner
Convert documentation from txt format to yaml. Add documentation about
reset-gpio.
Set reset gpio to high on ak8975_power_on, set reset gpio to low on
ak8975_power_off.
Without setting it to high on ak8975_power_on driver's probe fails
on ak8975_who_i_am while checking for device identity for AK09911 chip
AK09911 has a reset gpio to handle register's reset. If reset gpio is
set to low it will trigger the reset. AK09911 datasheed says that if not
used reset pin should be connected to VID and this patch emulates this
situation
Jonathan Albrieux (3):
dt-bindings: iio: magnetometer: ak8975: convert txt format to yaml
dt-bindings: iio: magnetometer: ak8975: add gpio reset support
iio: magnetometer: ak8975: Add gpio reset support
.../bindings/iio/magnetometer/ak8975.txt | 30 --------
.../bindings/iio/magnetometer/ak8975.yaml | 70 +++++++++++++++++++
drivers/iio/magnetometer/ak8975.c | 21 +++++-
3 files changed, 89 insertions(+), 32 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt
create mode 100644 Documentation/devicetree/bindings/iio/magnetometer/ak8975.yaml
--
2.17.1
^ permalink raw reply
* [PATCH 4/4] iio: imu: bmi160: added mount-matrix support
From: Jonathan Albrieux @ 2020-05-18 13:33 UTC (permalink / raw)
To: linux-kernel
Cc: Jonathan Albrieux,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Hartmut Knaack, Lars-Peter Clausen,
open list:IIO SUBSYSTEM AND DRIVERS, Peter Meerwald-Stadler,
Jonathan Cameron
In-Reply-To: <20200518133358.18978-1-jonathan.albrieux@gmail.com>
Add mount-matrix binding support. As chip could have different orientations
a mount matrix support is needed to correctly translate these differences
Signed-off-by: Jonathan Albrieux <jonathan.albrieux@gmail.com>
---
drivers/iio/imu/bmi160/bmi160.h | 1 +
drivers/iio/imu/bmi160/bmi160_core.c | 20 ++++++++++++++++++++
2 files changed, 21 insertions(+)
diff --git a/drivers/iio/imu/bmi160/bmi160.h b/drivers/iio/imu/bmi160/bmi160.h
index 923c3b274fde..a82e040bd109 100644
--- a/drivers/iio/imu/bmi160/bmi160.h
+++ b/drivers/iio/imu/bmi160/bmi160.h
@@ -9,6 +9,7 @@ struct bmi160_data {
struct regmap *regmap;
struct iio_trigger *trig;
struct regulator_bulk_data supplies[2];
+ struct iio_mount_matrix orientation;
};
extern const struct regmap_config bmi160_regmap_config;
diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
index 9bbe0d8e6720..78c8ca962359 100644
--- a/drivers/iio/imu/bmi160/bmi160_core.c
+++ b/drivers/iio/imu/bmi160/bmi160_core.c
@@ -110,6 +110,7 @@
.storagebits = 16, \
.endianness = IIO_LE, \
}, \
+ .ext_info = bmi160_ext_info, \
}
/* scan indexes follow DATA register order */
@@ -265,6 +266,20 @@ static const struct bmi160_odr_item bmi160_odr_table[] = {
},
};
+static const struct iio_mount_matrix *
+bmi160_get_mount_matrix(const struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan)
+{
+ struct bmi160_data *data = iio_priv(indio_dev);
+
+ return &data->orientation;
+}
+
+static const struct iio_chan_spec_ext_info bmi160_ext_info[] = {
+ IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, bmi160_get_mount_matrix),
+ { }
+};
+
static const struct iio_chan_spec bmi160_channels[] = {
BMI160_CHANNEL(IIO_ACCEL, X, BMI160_SCAN_ACCEL_X),
BMI160_CHANNEL(IIO_ACCEL, Y, BMI160_SCAN_ACCEL_Y),
@@ -840,6 +855,11 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
return ret;
}
+ ret = iio_read_mount_matrix(dev, "mount-matrix",
+ &data->orientation);
+ if (ret)
+ return ret;
+
ret = bmi160_chip_init(data, use_spi);
if (ret)
return ret;
--
2.17.1
^ permalink raw reply related
* [PATCH 3/4] iio: imu: bmi160: added regulator support
From: Jonathan Albrieux @ 2020-05-18 13:33 UTC (permalink / raw)
To: linux-kernel
Cc: Jonathan Albrieux,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Hartmut Knaack, Lars-Peter Clausen,
open list:IIO SUBSYSTEM AND DRIVERS, Peter Meerwald-Stadler,
Jonathan Cameron
In-Reply-To: <20200518133358.18978-1-jonathan.albrieux@gmail.com>
---
drivers/iio/imu/bmi160/bmi160.h | 2 ++
drivers/iio/imu/bmi160/bmi160_core.c | 27 ++++++++++++++++++++++++++-
2 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/imu/bmi160/bmi160.h b/drivers/iio/imu/bmi160/bmi160.h
index 621f5309d735..923c3b274fde 100644
--- a/drivers/iio/imu/bmi160/bmi160.h
+++ b/drivers/iio/imu/bmi160/bmi160.h
@@ -3,10 +3,12 @@
#define BMI160_H_
#include <linux/iio/iio.h>
+#include <linux/regulator/consumer.h>
struct bmi160_data {
struct regmap *regmap;
struct iio_trigger *trig;
+ struct regulator_bulk_data supplies[2];
};
extern const struct regmap_config bmi160_regmap_config;
diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
index 6af65d6f1d28..9bbe0d8e6720 100644
--- a/drivers/iio/imu/bmi160/bmi160_core.c
+++ b/drivers/iio/imu/bmi160/bmi160_core.c
@@ -15,6 +15,7 @@
#include <linux/delay.h>
#include <linux/irq.h>
#include <linux/of_irq.h>
+#include <linux/regulator/consumer.h>
#include <linux/iio/iio.h>
#include <linux/iio/triggered_buffer.h>
@@ -709,6 +710,12 @@ static int bmi160_chip_init(struct bmi160_data *data, bool use_spi)
unsigned int val;
struct device *dev = regmap_get_device(data->regmap);
+ ret = regulator_bulk_enable(ARRAY_SIZE(data->supplies), data->supplies);
+ if (ret) {
+ dev_err(dev, "Failed to enable regulators: %d\n", ret);
+ return ret;
+ }
+
ret = regmap_write(data->regmap, BMI160_REG_CMD, BMI160_CMD_SOFTRESET);
if (ret)
return ret;
@@ -793,9 +800,17 @@ int bmi160_probe_trigger(struct iio_dev *indio_dev, int irq, u32 irq_type)
static void bmi160_chip_uninit(void *data)
{
struct bmi160_data *bmi_data = data;
+ struct device *dev = regmap_get_device(bmi_data->regmap);
+ int ret;
bmi160_set_mode(bmi_data, BMI160_GYRO, false);
bmi160_set_mode(bmi_data, BMI160_ACCEL, false);
+
+ ret = regulator_bulk_disable(ARRAY_SIZE(bmi_data->supplies),
+ bmi_data->supplies);
+ if (ret) {
+ dev_err(dev, "Failed to disable regulators: %d\n", ret);
+ }
}
int bmi160_core_probe(struct device *dev, struct regmap *regmap,
@@ -815,6 +830,16 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
dev_set_drvdata(dev, indio_dev);
data->regmap = regmap;
+ data->supplies[0].supply = "vdd";
+ data->supplies[1].supply = "vddio";
+ ret = devm_regulator_bulk_get(dev,
+ ARRAY_SIZE(data->supplies),
+ data->supplies);
+ if (ret) {
+ dev_err(dev, "Failed to get regulators: %d\n", ret);
+ return ret;
+ }
+
ret = bmi160_chip_init(data, use_spi);
if (ret)
return ret;
@@ -853,6 +878,6 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
}
EXPORT_SYMBOL_GPL(bmi160_core_probe);
-MODULE_AUTHOR("Daniel Baluta <daniel.baluta@intel.com");
+MODULE_AUTHOR("Daniel Baluta <daniel.baluta@intel.com>");
MODULE_DESCRIPTION("Bosch BMI160 driver");
MODULE_LICENSE("GPL v2");
--
2.17.1
^ permalink raw reply related
* [PATCH 2/4] dt-bindings: iio: imu: bmi160: add regulators and mount-matrix
From: Jonathan Albrieux @ 2020-05-18 13:33 UTC (permalink / raw)
To: linux-kernel
Cc: Jonathan Albrieux,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Hartmut Knaack, Lars-Peter Clausen,
open list:IIO SUBSYSTEM AND DRIVERS, Peter Meerwald-Stadler,
Jonathan Cameron, Rob Herring
In-Reply-To: <20200518133358.18978-1-jonathan.albrieux@gmail.com>
Add vdd-supply and vddio-supply support.
Add mount-matrix support.
Signed-off-by: Jonathan Albrieux <jonathan.albrieux@gmail.com>
---
.../devicetree/bindings/iio/imu/bmi160.yaml | 21 +++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/Documentation/devicetree/bindings/iio/imu/bmi160.yaml b/Documentation/devicetree/bindings/iio/imu/bmi160.yaml
index 6b464ce5ed0b..5b13af7a209f 100644
--- a/Documentation/devicetree/bindings/iio/imu/bmi160.yaml
+++ b/Documentation/devicetree/bindings/iio/imu/bmi160.yaml
@@ -46,6 +46,21 @@ properties:
set if the specified interrupt pin should be configured as
open drain. If not set, defaults to push-pull.
+ vdd-supply:
+ maxItems: 1
+ description: |
+ an optional regulator that needs to be on to provide VDD power to
+ the sensor.
+
+ vddio-supply:
+ maxItems: 1
+ description: |
+ an optional regulator that needs to be on to provide the VDD IO power to
+ the sensor.
+
+ mount-matrix:
+ description: an optional 3x3 mounting rotation matrix
+
required:
- compatible
- reg
@@ -61,9 +76,15 @@ examples:
bmi160@68 {
compatible = "bosch,bmi160";
reg = <0x68>;
+ vdd-supply = <&pm8916_l17>;
+ vddio-supply = <&pm8916_l6>;
interrupt-parent = <&gpio4>;
interrupts = <12 1>;
interrupt-names = "INT1";
+ mount-matrix = "0", "1", "0",
+ "-1", "0", "0",
+ "0", "0", "1";
+ };
};
- |
// Example for SPI
--
2.17.1
^ permalink raw reply related
* [PATCH 1/4] dt-bindings: iio: imu: bmi160: convert txt format to yaml
From: Jonathan Albrieux @ 2020-05-18 13:33 UTC (permalink / raw)
To: linux-kernel
Cc: Jonathan Albrieux,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Hartmut Knaack, Lars-Peter Clausen,
open list:IIO SUBSYSTEM AND DRIVERS, Peter Meerwald-Stadler,
Jonathan Cameron, Rob Herring
In-Reply-To: <20200518133358.18978-1-jonathan.albrieux@gmail.com>
Converts documentation from txt format to yaml
Signed-off-by: Jonathan Albrieux <jonathan.albrieux@gmail.com>
---
.../devicetree/bindings/iio/imu/bmi160.txt | 37 --------
.../devicetree/bindings/iio/imu/bmi160.yaml | 84 +++++++++++++++++++
2 files changed, 84 insertions(+), 37 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/iio/imu/bmi160.txt
create mode 100644 Documentation/devicetree/bindings/iio/imu/bmi160.yaml
diff --git a/Documentation/devicetree/bindings/iio/imu/bmi160.txt b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
deleted file mode 100644
index 900c169de00f..000000000000
--- a/Documentation/devicetree/bindings/iio/imu/bmi160.txt
+++ /dev/null
@@ -1,37 +0,0 @@
-Bosch BMI160 - Inertial Measurement Unit with Accelerometer, Gyroscope
-and externally connectable Magnetometer
-
-https://www.bosch-sensortec.com/bst/products/all_products/bmi160
-
-Required properties:
- - compatible : should be "bosch,bmi160"
- - reg : the I2C address or SPI chip select number of the sensor
- - spi-max-frequency : set maximum clock frequency (only for SPI)
-
-Optional properties:
- - interrupts : interrupt mapping for IRQ
- - interrupt-names : set to "INT1" if INT1 pin should be used as interrupt
- input, set to "INT2" if INT2 pin should be used instead
- - drive-open-drain : set if the specified interrupt pin should be configured as
- open drain. If not set, defaults to push-pull.
-
-Examples:
-
-bmi160@68 {
- compatible = "bosch,bmi160";
- reg = <0x68>;
-
- interrupt-parent = <&gpio4>;
- interrupts = <12 IRQ_TYPE_EDGE_RISING>;
- interrupt-names = "INT1";
-};
-
-bmi160@0 {
- compatible = "bosch,bmi160";
- reg = <0>;
- spi-max-frequency = <10000000>;
-
- interrupt-parent = <&gpio2>;
- interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
- interrupt-names = "INT2";
-};
diff --git a/Documentation/devicetree/bindings/iio/imu/bmi160.yaml b/Documentation/devicetree/bindings/iio/imu/bmi160.yaml
new file mode 100644
index 000000000000..6b464ce5ed0b
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/imu/bmi160.yaml
@@ -0,0 +1,84 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/imu/bmi160.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Bosch BMI160
+
+maintainers:
+ - can't find a mantainer, author is Daniel Baluta <daniel.baluta@intel.com>
+
+description: |
+ Inertial Measurement Unit with Accelerometer, Gyroscope and externally
+ connectable Magnetometer
+ https://www.bosch-sensortec.com/bst/products/all_products/bmi160
+
+properties:
+ compatible:
+ const: bosch,bmi160
+
+ reg:
+ maxItems: 1
+ description: the I2C address or SPI chip select number of the sensor
+
+ spi-max-frequency:
+ maxItems: 1
+ description: set maximum clock frequency (required only for SPI)
+
+ interrupts:
+ maxItems: 1
+ description: interrupt mapping for IRQ
+
+ interrupt-names:
+ minItems: 1
+ maxItems: 1
+ items:
+ enum:
+ - INT1
+ - INT2
+ description: |
+ set to "INT1" if INT1 pin should be used as interrupt input, set
+ to "INT2" if INT2 pin should be used instead
+
+ drive-open-drain:
+ description: |
+ set if the specified interrupt pin should be configured as
+ open drain. If not set, defaults to push-pull.
+
+required:
+ - compatible
+ - reg
+
+examples:
+ - |
+ // Example for I2C
+ i2c@78b7000 {
+ reg = <0x78b6000 0x600>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ bmi160@68 {
+ compatible = "bosch,bmi160";
+ reg = <0x68>;
+ interrupt-parent = <&gpio4>;
+ interrupts = <12 1>;
+ interrupt-names = "INT1";
+ };
+ - |
+ // Example for SPI
+ spi@78b7000 {
+ reg = <0x78b7000 0x600>,
+ <0x7884000 0x23000>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ bmi160@0 {
+ compatible = "bosch,bmi160";
+ reg = <0>;
+ spi-max-frequency = <10000000>;
+ interrupt-parent = <&gpio2>;
+ interrupts = <12 1>;
+ interrupt-names = "INT2";
+ };
+ };
--
2.17.1
^ permalink raw reply related
* [PATCH 0/4] iio: imu: bmi160: added regulator and mount-matrix support
From: Jonathan Albrieux @ 2020-05-18 13:33 UTC (permalink / raw)
To: linux-kernel
Cc: Jonathan Albrieux,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Hartmut Knaack, Lars-Peter Clausen,
open list:IIO SUBSYSTEM AND DRIVERS, Peter Meerwald-Stadler
Convert txt format documentation to yaml.
Add documentation about vdd-supply, vddio-supply and mount-matrix.
Add vdd-supply and vddio-supply support. Without this support, vdd and vddio
should be set to always-on in device tree.
Add mount-matrix binding support. As chip could have different orientations
a mount matrix support is needed to correctly translate these differences
Jonathan Albrieux (4):
dt-bindings: iio: imu: bmi160: convert txt format to yaml
dt-bindings: iio: imu: bmi160: add regulators and mount-matrix
iio: imu: bmi160: added regulator support
iio: imu: bmi160: added mount-matrix support
.../devicetree/bindings/iio/imu/bmi160.txt | 37 ------
.../devicetree/bindings/iio/imu/bmi160.yaml | 105 ++++++++++++++++++
drivers/iio/imu/bmi160/bmi160.h | 3 +
drivers/iio/imu/bmi160/bmi160_core.c | 47 +++++++-
4 files changed, 154 insertions(+), 38 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/iio/imu/bmi160.txt
create mode 100644 Documentation/devicetree/bindings/iio/imu/bmi160.yaml
--
2.17.1
^ permalink raw reply
* [V6, 2/2] media: i2c: dw9768: Add DW9768 VCM driver
From: Dongchun Zhu @ 2020-05-18 13:27 UTC (permalink / raw)
To: linus.walleij, bgolaszewski, mchehab, andriy.shevchenko, robh+dt,
mark.rutland, sakari.ailus, drinkcat, tfiga, matthias.bgg,
bingbu.cao
Cc: srv_heupstream, linux-mediatek, linux-arm-kernel, sj.huang,
linux-media, devicetree, louis.kuo, shengnan.wang, dongchun.zhu
In-Reply-To: <20200518132731.20855-1-dongchun.zhu@mediatek.com>
Add a V4L2 sub-device driver for DW9768 voice coil motor, providing
control to set the desired focus via IIC serial interface.
Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
---
MAINTAINERS | 1 +
drivers/media/i2c/Kconfig | 13 ++
drivers/media/i2c/Makefile | 1 +
drivers/media/i2c/dw9768.c | 515 +++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 530 insertions(+)
create mode 100644 drivers/media/i2c/dw9768.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 8d72c41..c92dc99 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5157,6 +5157,7 @@ L: linux-media@vger.kernel.org
S: Maintained
T: git git://linuxtv.org/media_tree.git
F: Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
+F: drivers/media/i2c/dw9768.c
DONGWOON DW9807 LENS VOICE COIL DRIVER
M: Sakari Ailus <sakari.ailus@linux.intel.com>
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 125d596..afdf994 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -1040,6 +1040,19 @@ config VIDEO_DW9714
capability. This is designed for linear control of
voice coil motors, controlled via I2C serial interface.
+config VIDEO_DW9768
+ tristate "DW9768 lens voice coil support"
+ depends on I2C && VIDEO_V4L2
+ depends on PM
+ select MEDIA_CONTROLLER
+ select VIDEO_V4L2_SUBDEV_API
+ select V4L2_FWNODE
+ help
+ This is a driver for the DW9768 camera lens voice coil.
+ DW9768 is a 10 bit DAC with 100mA output current sink
+ capability. This is designed for linear control of
+ voice coil motors, controlled via I2C serial interface.
+
config VIDEO_DW9807_VCM
tristate "DW9807 lens voice coil support"
depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 77bf7d0..4057476 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_VIDEO_SAA6752HS) += saa6752hs.o
obj-$(CONFIG_VIDEO_AD5820) += ad5820.o
obj-$(CONFIG_VIDEO_AK7375) += ak7375.o
obj-$(CONFIG_VIDEO_DW9714) += dw9714.o
+obj-$(CONFIG_VIDEO_DW9768) += dw9768.o
obj-$(CONFIG_VIDEO_DW9807_VCM) += dw9807-vcm.o
obj-$(CONFIG_VIDEO_ADV7170) += adv7170.o
obj-$(CONFIG_VIDEO_ADV7175) += adv7175.o
diff --git a/drivers/media/i2c/dw9768.c b/drivers/media/i2c/dw9768.c
new file mode 100644
index 0000000..a0cc9f3
--- /dev/null
+++ b/drivers/media/i2c/dw9768.c
@@ -0,0 +1,515 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2020 MediaTek Inc.
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
+#include <media/v4l2-async.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-fwnode.h>
+#include <media/v4l2-subdev.h>
+
+#define DW9768_NAME "dw9768"
+#define DW9768_MAX_FOCUS_POS (1024 - 1)
+/*
+ * This sets the minimum granularity for the focus positions.
+ * A value of 1 gives maximum accuracy for a desired focus position
+ */
+#define DW9768_FOCUS_STEPS 1
+
+/*
+ * Ring control and Power control register
+ * Bit[1] RING_EN
+ * 0: Direct mode
+ * 1: AAC mode (ringing control mode)
+ * Bit[0] PD
+ * 0: Normal operation mode
+ * 1: Power down mode
+ * DW9768 requires waiting time of Topr after PD reset takes place.
+ */
+#define DW9768_RING_PD_CONTROL_REG 0x02
+#define DW9768_PD_MODE_OFF 0x00
+#define DW9768_PD_MODE_EN BIT(0)
+#define DW9768_AAC_MODE_EN BIT(1)
+
+/*
+ * DW9768 separates two registers to control the VCM position.
+ * One for MSB value, another is LSB value.
+ * DAC_MSB: D[9:8] (ADD: 0x03)
+ * DAC_LSB: D[7:0] (ADD: 0x04)
+ * D[9:0] DAC data input: positive output current = D[9:0] / 1023 * 100[mA]
+ */
+#define DW9768_MSB_ADDR 0x03
+#define DW9768_LSB_ADDR 0x04
+#define DW9768_STATUS_ADDR 0x05
+
+/*
+ * AAC mode control & prescale register
+ * Bit[7:5] Namely AC[2:0], decide the VCM mode and operation time.
+ * 000 Direct(default)
+ * 001 AAC2 0.48xTvib
+ * 010 AAC3 0.70xTvib
+ * 011 AAC4 0.75xTvib
+ * 100 Reserved
+ * 101 AAC8 1.13xTvib
+ * 110 Reserved
+ * 111 Reserved
+ * Bit[2:0] Namely PRESC[2:0], set the internal clock dividing rate as follow.
+ * 000 2
+ * 001 1(default)
+ * 010 1/2
+ * 011 1/4
+ * 100 8
+ * 101 4
+ * 110 Reserved
+ * 111 Reserved
+ */
+#define DW9768_AAC_PRESC_REG 0x06
+#define DW9768_AAC_MODE_SEL_MASK GENMASK(7, 5)
+#define DW9768_CLOCK_PRE_SCALE_SEL_MASK GENMASK(2, 0)
+
+/*
+ * VCM period of vibration register
+ * Bit[5:0] Defined as VCM rising periodic time (Tvib) together with PRESC[2:0]
+ * Tvib = (6.3ms + AACT[5:0] * 0.1ms) * Dividing Rate
+ * Dividing Rate is the internal clock dividing rate that is defined at
+ * PRESCALE register (ADD: 0x06)
+ */
+#define DW9768_AAC_TIME_REG 0x07
+
+/*
+ * DW9768 requires waiting time (delay time) of t_OPR after power-up,
+ * or in the case of PD reset taking place.
+ */
+#define DW9768_T_OPR_US 1000
+
+/*
+ * This acts as the minimum granularity of lens movement.
+ * Keep this value power of 2, so the control steps can be
+ * uniformly adjusted for gradual lens movement, with desired
+ * number of control steps.
+ */
+#define DW9768_MOVE_STEPS 16
+
+/*
+ * DW9768_AAC_PRESC_REG & DW9768_AAC_TIME_REG determine VCM operation time.
+ * If DW9768_AAC_PRESC_REG is 0x41, and DW9768_AAC_TIME_REG is 0x39, VCM mode
+ * would be AAC3, Operation Time would be 0.70xTvib, that is 8.40ms.
+ */
+#define DW9768_MOVE_DELAY_US 8400
+#define DW9768_STABLE_TIME_US 20000
+
+static const char * const dw9768_supply_names[] = {
+ "vin", /* I2C I/O interface power */
+ "vdd", /* VCM power */
+};
+
+/* dw9768 device structure */
+struct dw9768 {
+ struct regulator_bulk_data supplies[ARRAY_SIZE(dw9768_supply_names)];
+ struct v4l2_ctrl_handler ctrls;
+ struct v4l2_ctrl *focus;
+ struct v4l2_subdev sd;
+
+ u32 aac_mode;
+ u32 aac_timing;
+ u32 clock_dividing_rate;
+ bool aac_mode_control_enable;
+ bool aact_cnt_select_enable;
+ bool clock_dividing_rate_select_enable;
+};
+
+static inline struct dw9768 *sd_to_dw9768(struct v4l2_subdev *subdev)
+{
+ return container_of(subdev, struct dw9768, sd);
+}
+
+struct regval_list {
+ u8 reg_num;
+ u8 value;
+};
+
+static int dw9768_read_smbus(struct dw9768 *dw9768, unsigned char reg,
+ unsigned char *val)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
+ int ret;
+
+ ret = i2c_smbus_read_byte_data(client, reg);
+
+ if (ret < 0)
+ return ret;
+
+ *val = (unsigned char)ret;
+
+ return 0;
+}
+
+static int dw9768_mod_reg(struct dw9768 *dw9768, u8 reg, u8 mask, u8 val)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
+ u8 readval;
+ int ret;
+
+ ret = dw9768_read_smbus(dw9768, reg, &readval);
+ if (ret)
+ return ret;
+
+ val = (readval & ~mask) | (val & mask);
+
+ return i2c_smbus_write_byte_data(client, reg, val);
+}
+
+static int dw9768_set_dac(struct dw9768 *dw9768, u16 val)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
+
+ /* Write VCM position to registers */
+ return i2c_smbus_write_word_swapped(client, DW9768_MSB_ADDR, val);
+}
+
+static int dw9768_init(struct dw9768 *dw9768)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
+ int ret, val;
+
+ /* Reset DW9768_RING_PD_CONTROL_REG to default status 0x00 */
+ ret = i2c_smbus_write_byte_data(client, DW9768_RING_PD_CONTROL_REG,
+ DW9768_PD_MODE_OFF);
+ if (ret < 0)
+ return ret;
+
+ /*
+ * DW9769 requires waiting delay time of t_OPR
+ * after PD reset takes place.
+ */
+ usleep_range(DW9768_T_OPR_US, DW9768_T_OPR_US + 100);
+
+ /* Set DW9768_RING_PD_CONTROL_REG to DW9768_AAC_MODE_EN(0x01) */
+ ret = i2c_smbus_write_byte_data(client, DW9768_RING_PD_CONTROL_REG,
+ DW9768_AAC_MODE_EN);
+ if (ret < 0)
+ return ret;
+
+ /* Set AAC mode according to DT property */
+ if (dw9768->aac_mode_control_enable) {
+ ret = dw9768_mod_reg(dw9768, DW9768_AAC_PRESC_REG,
+ DW9768_AAC_MODE_SEL_MASK,
+ dw9768->aac_mode << 5);
+ if (ret < 0)
+ return ret;
+ }
+
+ /* Set Clock Dividing Rate factor according to DT property */
+ if (dw9768->clock_dividing_rate_select_enable) {
+ ret = dw9768_mod_reg(dw9768, DW9768_AAC_PRESC_REG,
+ DW9768_CLOCK_PRE_SCALE_SEL_MASK,
+ dw9768->clock_dividing_rate);
+ if (ret < 0)
+ return ret;
+ }
+
+ /* Set AAC Timing according to DT property */
+ if (dw9768->aact_cnt_select_enable) {
+ ret = i2c_smbus_write_byte_data(client, DW9768_AAC_TIME_REG,
+ dw9768->aac_timing);
+ if (ret < 0)
+ return ret;
+ }
+
+ for (val = dw9768->focus->val % DW9768_MOVE_STEPS;
+ val <= dw9768->focus->val;
+ val += DW9768_MOVE_STEPS) {
+ ret = dw9768_set_dac(dw9768, val);
+ if (ret) {
+ dev_err(&client->dev, "%s I2C failure: %d",
+ __func__, ret);
+ return ret;
+ }
+ usleep_range(DW9768_MOVE_DELAY_US,
+ DW9768_MOVE_DELAY_US + 1000);
+ }
+
+ return 0;
+}
+
+static int dw9768_release(struct dw9768 *dw9768)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
+ int ret, val;
+
+ val = round_down(dw9768->focus->val, DW9768_MOVE_STEPS);
+ for ( ; val >= 0; val -= DW9768_MOVE_STEPS) {
+ ret = dw9768_set_dac(dw9768, val);
+ if (ret) {
+ dev_err(&client->dev, "I2C write fail: %d", ret);
+ return ret;
+ }
+ usleep_range(DW9768_MOVE_DELAY_US, DW9768_MOVE_DELAY_US + 1000);
+ }
+
+ /*
+ * Wait for the motor to stabilize after the last movement
+ * to prevent the motor from shaking.
+ */
+ usleep_range(DW9768_STABLE_TIME_US - DW9768_MOVE_DELAY_US,
+ DW9768_STABLE_TIME_US - DW9768_MOVE_DELAY_US + 1000);
+
+ ret = i2c_smbus_write_byte_data(client, DW9768_RING_PD_CONTROL_REG,
+ DW9768_PD_MODE_EN);
+ if (ret < 0)
+ return ret;
+
+ /*
+ * DW9769 requires waiting delay time of t_OPR
+ * after PD reset takes place.
+ */
+ usleep_range(DW9768_T_OPR_US, DW9768_T_OPR_US + 100);
+
+ return 0;
+}
+
+static int dw9768_runtime_suspend(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct dw9768 *dw9768 = sd_to_dw9768(sd);
+
+ dw9768_release(dw9768);
+ regulator_bulk_disable(ARRAY_SIZE(dw9768_supply_names),
+ dw9768->supplies);
+
+ return 0;
+}
+
+static int dw9768_runtime_resume(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct dw9768 *dw9768 = sd_to_dw9768(sd);
+ int ret;
+
+ ret = regulator_bulk_enable(ARRAY_SIZE(dw9768_supply_names),
+ dw9768->supplies);
+ if (ret < 0) {
+ dev_err(dev, "failed to enable regulators\n");
+ return ret;
+ }
+
+ /*
+ * The datasheet refers to t_OPR that needs to be waited before sending
+ * I2C commands after power-up.
+ */
+ usleep_range(DW9768_T_OPR_US, DW9768_T_OPR_US + 100);
+
+ ret = dw9768_init(dw9768);
+ if (ret < 0)
+ goto disable_regulator;
+
+ return 0;
+
+disable_regulator:
+ regulator_bulk_disable(ARRAY_SIZE(dw9768_supply_names),
+ dw9768->supplies);
+
+ return ret;
+}
+
+static int dw9768_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+ struct dw9768 *dw9768 = container_of(ctrl->handler,
+ struct dw9768, ctrls);
+
+ if (ctrl->id == V4L2_CID_FOCUS_ABSOLUTE)
+ return dw9768_set_dac(dw9768, ctrl->val);
+
+ return 0;
+}
+
+static const struct v4l2_ctrl_ops dw9768_ctrl_ops = {
+ .s_ctrl = dw9768_set_ctrl,
+};
+
+static int dw9768_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+ int ret;
+
+ ret = pm_runtime_get_sync(sd->dev);
+ if (ret < 0) {
+ pm_runtime_put_noidle(sd->dev);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int dw9768_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+ pm_runtime_put(sd->dev);
+
+ return 0;
+}
+
+static const struct v4l2_subdev_internal_ops dw9768_int_ops = {
+ .open = dw9768_open,
+ .close = dw9768_close,
+};
+
+static const struct v4l2_subdev_ops dw9768_ops = { };
+
+static int dw9768_init_controls(struct dw9768 *dw9768)
+{
+ struct v4l2_ctrl_handler *hdl = &dw9768->ctrls;
+ const struct v4l2_ctrl_ops *ops = &dw9768_ctrl_ops;
+
+ v4l2_ctrl_handler_init(hdl, 1);
+
+ dw9768->focus = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_FOCUS_ABSOLUTE, 0,
+ DW9768_MAX_FOCUS_POS,
+ DW9768_FOCUS_STEPS, 0);
+
+ if (hdl->error)
+ return hdl->error;
+
+ dw9768->sd.ctrl_handler = hdl;
+
+ return 0;
+}
+
+static int dw9768_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct dw9768 *dw9768;
+ unsigned int aac_mode_select;
+ unsigned int aac_timing_select;
+ unsigned int clock_dividing_rate_select;
+ unsigned int i;
+ int ret;
+
+ dw9768 = devm_kzalloc(dev, sizeof(*dw9768), GFP_KERNEL);
+ if (!dw9768)
+ return -ENOMEM;
+
+ v4l2_i2c_subdev_init(&dw9768->sd, client, &dw9768_ops);
+ dw9768->aac_mode_control_enable = false;
+ dw9768->aact_cnt_select_enable = false;
+ dw9768->clock_dividing_rate_select_enable = false;
+
+ /* Optional indication of AAC mode select */
+ ret = fwnode_property_read_u32(dev_fwnode(dev), "dongwoon,aac-mode",
+ &aac_mode_select);
+
+ if (!ret) {
+ dw9768->aac_mode_control_enable = true;
+ dw9768->aac_mode = aac_mode_select;
+ }
+
+ /* Optional indication of VCM internal clock dividing rate select */
+ ret = fwnode_property_read_u32(dev_fwnode(dev),
+ "dongwoon,clock-dividing-rate",
+ &clock_dividing_rate_select);
+
+ if (!ret) {
+ dw9768->clock_dividing_rate_select_enable = true;
+ dw9768->clock_dividing_rate = clock_dividing_rate_select;
+ }
+
+ /* Optional indication of AAC Timing */
+ ret = fwnode_property_read_u32(dev_fwnode(dev), "dongwoon,aac-timing",
+ &aac_timing_select);
+
+ if (!ret) {
+ dw9768->aact_cnt_select_enable = true;
+ dw9768->aac_timing = aac_timing_select;
+ }
+
+ for (i = 0; i < ARRAY_SIZE(dw9768_supply_names); i++)
+ dw9768->supplies[i].supply = dw9768_supply_names[i];
+
+ ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(dw9768_supply_names),
+ dw9768->supplies);
+ if (ret) {
+ dev_err(dev, "failed to get regulators\n");
+ return ret;
+ }
+
+ ret = dw9768_init_controls(dw9768);
+ if (ret)
+ goto entity_cleanup;
+
+ dw9768->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+ dw9768->sd.internal_ops = &dw9768_int_ops;
+
+ ret = media_entity_pads_init(&dw9768->sd.entity, 0, NULL);
+ if (ret < 0)
+ goto entity_cleanup;
+
+ dw9768->sd.entity.function = MEDIA_ENT_F_LENS;
+
+ pm_runtime_enable(dev);
+ if (!pm_runtime_enabled(dev)) {
+ ret = dw9768_runtime_resume(dev);
+ if (ret < 0) {
+ dev_err(dev, "failed to power on: %d\n", ret);
+ goto entity_cleanup;
+ }
+ }
+
+ ret = v4l2_async_register_subdev(&dw9768->sd);
+ if (ret < 0)
+ goto entity_cleanup;
+
+ return 0;
+
+entity_cleanup:
+ v4l2_ctrl_handler_free(&dw9768->ctrls);
+ media_entity_cleanup(&dw9768->sd.entity);
+ return ret;
+}
+
+static int dw9768_remove(struct i2c_client *client)
+{
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct dw9768 *dw9768 = sd_to_dw9768(sd);
+
+ pm_runtime_disable(&client->dev);
+ v4l2_async_unregister_subdev(&dw9768->sd);
+ v4l2_ctrl_handler_free(&dw9768->ctrls);
+ media_entity_cleanup(&dw9768->sd.entity);
+ if (!pm_runtime_status_suspended(&client->dev))
+ dw9768_runtime_suspend(&client->dev);
+ pm_runtime_set_suspended(&client->dev);
+
+ return 0;
+}
+
+static const struct of_device_id dw9768_of_table[] = {
+ { .compatible = "dongwoon,dw9768" },
+ { .compatible = "giantec,gt9769" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, dw9768_of_table);
+
+static const struct dev_pm_ops dw9768_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+ pm_runtime_force_resume)
+ SET_RUNTIME_PM_OPS(dw9768_runtime_suspend, dw9768_runtime_resume, NULL)
+};
+
+static struct i2c_driver dw9768_i2c_driver = {
+ .driver = {
+ .name = DW9768_NAME,
+ .pm = &dw9768_pm_ops,
+ .of_match_table = dw9768_of_table,
+ },
+ .probe_new = dw9768_probe,
+ .remove = dw9768_remove,
+};
+module_i2c_driver(dw9768_i2c_driver);
+
+MODULE_AUTHOR("Dongchun Zhu <dongchun.zhu@mediatek.com>");
+MODULE_DESCRIPTION("DW9768 VCM driver");
+MODULE_LICENSE("GPL v2");
--
2.9.2
^ permalink raw reply related
* [V6, 1/2] media: dt-bindings: media: i2c: Document DW9768 bindings
From: Dongchun Zhu @ 2020-05-18 13:27 UTC (permalink / raw)
To: linus.walleij, bgolaszewski, mchehab, andriy.shevchenko, robh+dt,
mark.rutland, sakari.ailus, drinkcat, tfiga, matthias.bgg,
bingbu.cao
Cc: srv_heupstream, linux-mediatek, linux-arm-kernel, sj.huang,
linux-media, devicetree, louis.kuo, shengnan.wang, dongchun.zhu
In-Reply-To: <20200518132731.20855-1-dongchun.zhu@mediatek.com>
Add DeviceTree binding documentation for Dongwoon Anatech DW9768 voice
coil actuator.
Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
.../bindings/media/i2c/dongwoon,dw9768.yaml | 105 +++++++++++++++++++++
MAINTAINERS | 7 ++
2 files changed, 112 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
diff --git a/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml b/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
new file mode 100644
index 0000000..b909e83
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
@@ -0,0 +1,105 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright (c) 2020 MediaTek Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/dongwoon,dw9768.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Dongwoon Anatech DW9768 Voice Coil Motor (VCM) Lens Device Tree Bindings
+
+maintainers:
+ - Dongchun Zhu <dongchun.zhu@mediatek.com>
+
+description: |-
+ The Dongwoon DW9768 is a single 10-bit digital-to-analog (DAC) converter
+ with 100 mA output current sink capability. VCM current is controlled with
+ a linear mode driver. The DAC is controlled via a 2-wire (I2C-compatible)
+ serial interface that operates at clock rates up to 1MHz. This chip
+ integrates Advanced Actuator Control (AAC) technology and is intended for
+ driving voice coil lenses in camera modules.
+
+properties:
+ compatible:
+ enum:
+ # for DW9768 VCM
+ - dongwoon,dw9768
+ # for GT9769 VCM
+ - giantec,gt9769
+
+ reg:
+ maxItems: 1
+
+ vin-supply:
+ description:
+ Definition of the regulator used as I2C I/O interface power supply.
+
+ vdd-supply:
+ description:
+ Definition of the regulator used as VCM chip power supply.
+
+ dongwoon,aac-mode:
+ description:
+ Indication of AAC mode select.
+ allOf:
+ - $ref: "/schemas/types.yaml#/definitions/uint32"
+ - enum:
+ - 0 # Direct (default)
+ - 1 # AAC2 (operation time# 0.48 x Tvib)
+ - 2 # AAC3 (operation time# 0.70 x Tvib)
+ - 3 # AAC4 (operation time# 0.75 x Tvib)
+ - 4 # Reserved
+ - 5 # AAC8 (operation time# 1.13 x Tvib)
+ - 6 # Reserved
+ - 7 # Reserved
+
+ dongwoon,aac-timing:
+ description:
+ Indication of AAC Timing count, unit of 0.1 milliseconds.
+ Valid values vary from 0 to 63 (default 32).
+ allOf:
+ - $ref: "/schemas/types.yaml#/definitions/uint32"
+
+ dongwoon,clock-dividing-rate:
+ description:
+ Indication of VCM internal clock dividing rate select, as one multiple
+ factor to calculate VCM ring periodic time Tvib.
+ allOf:
+ - $ref: "/schemas/types.yaml#/definitions/uint32"
+ - enum:
+ - 0 # Dividing Rate - 2
+ - 1 # Dividing Rate - 1 (default)
+ - 2 # Dividing Rate - 1/2
+ - 3 # Dividing Rate - 1/4
+ - 4 # Dividing Rate - 8
+ - 5 # Dividing Rate - 4
+ - 6 # Dividing Rate - Reserved
+ - 7 # Dividing Rate - Reserved
+
+required:
+ - compatible
+ - reg
+ - vin-supply
+ - vdd-supply
+
+additionalProperties: false
+
+examples:
+ - |
+
+ i2c {
+ clock-frequency = <400000>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ dw9768: camera-lens@c {
+ compatible = "dongwoon,dw9768";
+ reg = <0x0c>;
+
+ vin-supply = <&mt6358_vcamio_reg>;
+ vdd-supply = <&mt6358_vcama2_reg>;
+ dongwoon,aac-mode = <2>;
+ dongwoon,aac-timing = <57>;
+ };
+ };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index e64e5db..8d72c41 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5151,6 +5151,13 @@ T: git git://linuxtv.org/media_tree.git
F: Documentation/devicetree/bindings/media/i2c/dongwoon,dw9714.txt
F: drivers/media/i2c/dw9714.c
+DONGWOON DW9768 LENS VOICE COIL DRIVER
+M: Dongchun Zhu <dongchun.zhu@mediatek.com>
+L: linux-media@vger.kernel.org
+S: Maintained
+T: git git://linuxtv.org/media_tree.git
+F: Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
+
DONGWOON DW9807 LENS VOICE COIL DRIVER
M: Sakari Ailus <sakari.ailus@linux.intel.com>
L: linux-media@vger.kernel.org
--
2.9.2
^ permalink raw reply related
* [V6, 0/2] media: i2c: Add support for DW9768 VCM driver
From: Dongchun Zhu @ 2020-05-18 13:27 UTC (permalink / raw)
To: linus.walleij, bgolaszewski, mchehab, andriy.shevchenko, robh+dt,
mark.rutland, sakari.ailus, drinkcat, tfiga, matthias.bgg,
bingbu.cao
Cc: srv_heupstream, linux-mediatek, linux-arm-kernel, sj.huang,
linux-media, devicetree, louis.kuo, shengnan.wang, dongchun.zhu
Hello,
This series adds DT bindings in YAML and V4L2 sub-device driver for DW9768
lens voice coil motor(VCM), which is a 10-bit DAC with 100mA output current
sink capability from Dongwoon.
The driver is designed for linear control of voice coil motor,
and controlled via IIC serial interface to set the desired focus.
It controls the position with 10-bit DAC data D[9:0] and seperates
two 8-bit regs to control the VCM position as belows.
DAC_MSB: D[9:8](ADDR: 0x03):
+---+---+---+---+---+---+---+---+
|---|---|---|---|---|---|D09|D08|
+---+---+---+---+---+---+---+---+
DAC_LSB: D[7:0](ADDR: 0x04):
+---+---+---+---+---+---+---+---+
|D07|D06|D05|D04|D03|D02|D01|D00|
+---+---+---+---+---+---+---+---+
This driver supports:
- set DW9768 to standby mode once suspend and turn it back to active if resume
- set the desired focus via V4L2_CID_FOCUS_ABSOLUTE ctrl
Previous versions of this patch-set can be found here:
v5: https://lore.kernel.org/linux-media/20200502161727.30463-1-dongchun.zhu@mediatek.com/
v4: https://lore.kernel.org/linux-media/20200330123634.363-1-dongchun.zhu@mediatek.com/
v3: https://lore.kernel.org/linux-media/20200228155958.20657-1-dongchun.zhu@mediatek.com/
v2: https://lore.kernel.org/linux-media/20190905072142.14606-1-dongchun.zhu@mediatek.com/
v1: https://lore.kernel.org/linux-media/20190708100641.2702-1-dongchun.zhu@mediatek.com/
Mainly changes of v6 are addressing comments from Rob, Sakari, Tomasz.
Compared to v5:
- Add a second compatible string for the Giantec device
- Document optional properties: "dongwoon,aac-mode", "dongwoon,aac-timing" and
"dongwoon,clock-dividing-rate" for lens specific reg settings
- Adjust Kconfig to match the current media tree master branch
- Use container_of() directly to replace of defining macro function
Please help review.
Thanks.
Dongchun Zhu (2):
media: dt-bindings: media: i2c: Document DW9768 bindings
media: i2c: dw9768: Add DW9768 VCM driver
.../bindings/media/i2c/dongwoon,dw9768.yaml | 105 +++++
MAINTAINERS | 8 +
drivers/media/i2c/Kconfig | 13 +
drivers/media/i2c/Makefile | 1 +
drivers/media/i2c/dw9768.c | 515 +++++++++++++++++++++
5 files changed, 642 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
create mode 100644 drivers/media/i2c/dw9768.c
--
2.9.2
^ permalink raw reply
* Re: [PATCH v2 10/19] spi: dw: Use DMA max burst to set the request thresholds
From: Andy Shevchenko @ 2020-05-18 13:25 UTC (permalink / raw)
To: Serge Semin
Cc: Andy Shevchenko, Serge Semin, Mark Brown, Alexey Malahov,
Thomas Bogendoerfer, Paul Burton, Ralf Baechle, Arnd Bergmann,
Allison Randal, Gareth Williams, Rob Herring, linux-mips,
devicetree, Georgy Vlasov, Ramil Zaripov, Jarkko Nikula,
Thomas Gleixner, Wan Ahmad Zainie, Linus Walleij, Clement Leger,
linux-spi, Linux Kernel Mailing List
In-Reply-To: <20200518125253.r4fpr4mjflclqpym@mobilestation>
On Mon, May 18, 2020 at 3:53 PM Serge Semin
<Sergey.Semin@baikalelectronics.ru> wrote:
> On Mon, May 18, 2020 at 02:03:43PM +0300, Andy Shevchenko wrote:
> > On Sat, May 16, 2020 at 11:01:33PM +0300, Serge Semin wrote:
> > > On Fri, May 15, 2020 at 05:38:42PM +0300, Andy Shevchenko wrote:
> > > > On Fri, May 15, 2020 at 01:47:49PM +0300, Serge Semin wrote:
...
> > > > > struct dma_chan *txchan;
> > > > > + u32 txburst;
> > > > > struct dma_chan *rxchan;
> > > > > + u32 rxburst;
> > > >
> > > > Leave u32 together, it may be optimal on 64-bit architectures where ABIs require padding.
> > >
> > > It's not like anyone cared about padding in this structure in the first place)
> >
> > I think I have been caring (to some extend).
>
> Well, If you have then instead of asking to rearrange just two members (which
> by the way finely grouped by the Tx-Rx affiliation) why not sending a
> patch, which would refactor the whole structure so to be optimal for the x64
> platforms? I don't really see why this gets very important for you seeing
> Mark is Ok with this. My current commit follows the common driver design
> including the DW SSI data members grouping. On the second thought I'll leave
> it as is then.
Again same issue here. What is really easy to do for you here, will
become a burden and additional churn to anybody else.
So, why not to minimize it in the first place? Same with comma in
another patch. Sorry, I really don't get it.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH 17/17] ARM: dts: r8a7742: Add RWDT node
From: Lad, Prabhakar @ 2020-05-18 13:23 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Geert Uytterhoeven, Lad Prabhakar, Jens Axboe, Rob Herring,
Wolfram Sang, Ulf Hansson, David S. Miller, Wim Van Sebroeck,
Guenter Roeck, linux-ide,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux Kernel Mailing List, Linux I2C, Linux MMC List, netdev,
Linux-Renesas, Linux Watchdog Mailing List
In-Reply-To: <9ab946d2-1076-ed92-0a48-9a95d798d291@cogentembedded.com>
Hi Sergei,
On Mon, May 18, 2020 at 2:17 PM Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
>
> Hello!
>
> On 18.05.2020 15:27, Lad, Prabhakar wrote:
>
> >>> Add a device node for the Watchdog Timer (RWDT) controller on the Renesas
> >>> RZ/G1H (r8a7742) SoC.
> >>>
> >>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >>> Reviewed-by: Marian-Cristian Rotariu <marian-cristian.rotariu.rb@bp.renesas.com>
> >>
> >> Thanks for your patch!
> >>
> >>> --- a/arch/arm/boot/dts/r8a7742.dtsi
> >>> +++ b/arch/arm/boot/dts/r8a7742.dtsi
> >>> @@ -201,6 +201,16 @@
> >>> #size-cells = <2>;
> >>> ranges;
> >>>
> >>> + rwdt: watchdog@e6020000 {
> >>> + compatible = "renesas,r8a7742-wdt",
> >>> + "renesas,rcar-gen2-wdt";
> >>> + reg = <0 0xe6020000 0 0x0c>;
> >>> + clocks = <&cpg CPG_MOD 402>;
> >>> + power-domains = <&sysc R8A7742_PD_ALWAYS_ON>;
> >>> + resets = <&cpg 402>;
> >>> + status = "disabled";
> >>
> >> Missing "interrupts" property.
> >>
> > "interrupts" property isn't used by rwdt driver and can be dropped
> > from bindings file.
>
> DT describes the hardware, not its driver's abilities.
>
Agreed will add, I had followed it on similar lines of r8a7743/44.
Cheers,
--Prabhakar
^ permalink raw reply
* Re: [PATCH v2 06/19] spi: dw: Discard static DW DMA slave structures
From: Andy Shevchenko @ 2020-05-18 13:22 UTC (permalink / raw)
To: Serge Semin
Cc: Serge Semin, Andy Shevchenko, Mark Brown, Georgy Vlasov,
Ramil Zaripov, Alexey Malahov, Thomas Bogendoerfer, Paul Burton,
Ralf Baechle, Allison Randal, Gareth Williams, Arnd Bergmann,
Rob Herring, linux-mips, devicetree, Wan Ahmad Zainie,
Jarkko Nikula, Thomas Gleixner, wuxu.wu, Clement Leger, linux-spi,
Linux Kernel Mailing List
In-Reply-To: <20200518123242.xoosc4pcj7heo4he@mobilestation>
On Mon, May 18, 2020 at 3:32 PM Serge Semin
<Sergey.Semin@baikalelectronics.ru> wrote:
> On Mon, May 18, 2020 at 02:38:13PM +0300, Andy Shevchenko wrote:
> > On Mon, May 18, 2020 at 2:01 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Sat, May 16, 2020 at 05:20:30PM +0300, Serge Semin wrote:
...
> > > I'm not talking about stack, it's fine for me, what I'm talking about is *how*
> > > they are being initialized. Read my message again carefully, please.
> >
> > And to avoid additional churn around this, the purpose is to show what
> > Dreq number is in use actually for these transfers (that's why 0
> > assignment is explicitly there and no counterpart Dreq filled).
>
> Sorry, but nothing persuasive so far. We still can remove their static definition
> and explicitly assign zero or one to the src and dst request IDs on stack. I'll do
> this in v3, since I have to send one anyway.
Right, I'm completely fine with it, thanks!
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v7 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
From: Andy Shevchenko @ 2020-05-18 13:20 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Ramuthevar, Vadivel MuruganX, kbuild test robot,
Linux Kernel Mailing List, open list:MEMORY TECHNOLOGY...,
devicetree, kbuild-all, Miquel Raynal, Richard Weinberger,
Vignesh R, Brendan Higgins, Thomas Gleixner, Boris Brezillon,
Anders Roxell, masonccyang
In-Reply-To: <CAK8P3a25GbMwbtvkxgmuGss6nEfAW4_vVbOXPxOYuDOaU_zcjA@mail.gmail.com>
On Mon, May 18, 2020 at 2:57 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Mon, May 18, 2020 at 1:43 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Mon, May 18, 2020 at 2:39 PM Ramuthevar, Vadivel MuruganX
> > <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
> > > On 15/5/2020 10:30 pm, Arnd Bergmann wrote:
> > > > On Fri, May 15, 2020 at 4:25 PM Andy Shevchenko
> > > > <andy.shevchenko@gmail.com> wrote:
> > > >> On Fri, May 15, 2020 at 4:48 PM kbuild test robot <lkp@intel.com> wrote:
> >
> > > > iowrite_be32() is the correct way to store word into a big-endian mmio register,
> > > > if that is the intention here.
> > > Thank you for suggestions to use iowrite32be(), it suits exactly.
> >
> > Can you before doing this comment what is the real intention here?
> >
> > And note, if you are going to use iowrite*() / ioread*() in one place,
> > you will probably need to replace all of the read*() / write*() to
> > respective io* API.
>
> The way that ioread/iowrite are defined, they are required to be a superset
> of what readl/writel do and can take __iomem pointers from either
> ioremap() or ioport_map()/pci_iomap() style mappings, while readl/writel
> are only required to work with ioremap().
>
> There is no technical requirement to stick to one set or the other for
> ioremap(), but the overhead of ioread/iowrite is also small enough
> that it generally does not hurt.
Right, my suggestion is solely for consistency. It would be a bit
weird to see readl() along with ioread32() in the same driver (in case
there are no differentiated callbacks specifically for different type
of IP).
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox