* [PATCH v7 7/8] iio: accel: adxl345: Add comment to probe
From: Lothar Rubusch @ 2024-04-01 19:49 UTC (permalink / raw)
To: lars, Michael.Hennerich, jic23, robh+dt, krzysztof.kozlowski+dt,
conor+dt
Cc: linux-iio, devicetree, linux-kernel, eraretuya, l.rubusch
In-Reply-To: <20240401194906.56810-1-l.rubusch@gmail.com>
Add a comment to the probe() function to describe the passed function
pointer argument in particular.
Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
drivers/iio/accel/adxl345_core.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 5d0f3243e..006ce66c0 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -168,6 +168,16 @@ static void adxl345_powerdown(void *regmap)
regmap_write(regmap, ADXL345_REG_POWER_CTL, ADXL345_POWER_CTL_STANDBY);
}
+/**
+ * adxl345_core_probe() - probe and setup for the adxl345 accelerometer,
+ * also covers the adlx375 accelerometer
+ * @dev: Driver model representation of the device
+ * @regmap: Regmap instance for the device
+ * @setup: Setup routine to be executed right before the standard device
+ * setup
+ *
+ * Return: 0 on success, negative errno on error
+ */
int adxl345_core_probe(struct device *dev, struct regmap *regmap,
int (*setup)(struct device*, struct regmap*))
{
--
2.25.1
^ permalink raw reply related
* [PATCH v7 6/8] iio: accel: adxl345: Reorder probe initialization
From: Lothar Rubusch @ 2024-04-01 19:49 UTC (permalink / raw)
To: lars, Michael.Hennerich, jic23, robh+dt, krzysztof.kozlowski+dt,
conor+dt
Cc: linux-iio, devicetree, linux-kernel, eraretuya, l.rubusch
In-Reply-To: <20240401194906.56810-1-l.rubusch@gmail.com>
Bring indio_dev, setup() and data initialization to begin of the probe()
function to increase readability. Access members through data
pointer to assure implicitely the driver's data instance is correctly
initialized and functional.
Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
drivers/iio/accel/adxl345_core.c | 40 ++++++++++++++++----------------
1 file changed, 20 insertions(+), 20 deletions(-)
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 8d4a66d8c..5d0f3243e 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -180,14 +180,30 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
ADXL345_DATA_FORMAT_SELF_TEST);
int ret;
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ data = iio_priv(indio_dev);
+ data->regmap = regmap;
+ data->info = device_get_match_data(dev);
+ if (!data->info)
+ return -ENODEV;
+
+ indio_dev->name = data->info->name;
+ indio_dev->info = &adxl345_info;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ indio_dev->channels = adxl345_channels;
+ indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
+
if (setup) {
/* Perform optional initial bus specific configuration */
- ret = setup(dev, regmap);
+ ret = setup(dev, data->regmap);
if (ret)
return ret;
/* Enable full-resolution mode */
- ret = regmap_update_bits(regmap, ADXL345_REG_DATA_FORMAT,
+ ret = regmap_update_bits(data->regmap, ADXL345_REG_DATA_FORMAT,
data_format_mask,
ADXL345_DATA_FORMAT_FULL_RES);
if (ret)
@@ -196,14 +212,14 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
} else {
/* Enable full-resolution mode (init all data_format bits) */
- ret = regmap_write(regmap, ADXL345_REG_DATA_FORMAT,
+ ret = regmap_write(data->regmap, ADXL345_REG_DATA_FORMAT,
ADXL345_DATA_FORMAT_FULL_RES);
if (ret)
return dev_err_probe(dev, ret,
"Failed to set data range\n");
}
- ret = regmap_read(regmap, ADXL345_REG_DEVID, ®val);
+ ret = regmap_read(data->regmap, ADXL345_REG_DEVID, ®val);
if (ret < 0)
return dev_err_probe(dev, ret, "Error reading device ID\n");
@@ -211,22 +227,6 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
return dev_err_probe(dev, -ENODEV, "Invalid device ID: %x, expected %x\n",
regval, ADXL345_DEVID);
- indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
- if (!indio_dev)
- return -ENOMEM;
-
- data = iio_priv(indio_dev);
- data->regmap = regmap;
- data->info = device_get_match_data(dev);
- if (!data->info)
- return -ENODEV;
-
- indio_dev->name = data->info->name;
- indio_dev->info = &adxl345_info;
- indio_dev->modes = INDIO_DIRECT_MODE;
- indio_dev->channels = adxl345_channels;
- indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
-
/* Enable measurement mode */
ret = adxl345_powerup(data->regmap);
if (ret < 0)
--
2.25.1
^ permalink raw reply related
* [PATCH v7 5/8] iio: accel: adxl345: Pass function pointer to core
From: Lothar Rubusch @ 2024-04-01 19:49 UTC (permalink / raw)
To: lars, Michael.Hennerich, jic23, robh+dt, krzysztof.kozlowski+dt,
conor+dt
Cc: linux-iio, devicetree, linux-kernel, eraretuya, l.rubusch
In-Reply-To: <20240401194906.56810-1-l.rubusch@gmail.com>
Provide a way for bus specific pre-configuration by adding a function
pointer argument to the driver core's probe() function, and keep
the driver core implementation bus independent.
In case NULL was passed, a regmap_write() shall initialize all bits of
the data_format register, else regmap_update() is used. In this way
spi and i2c are covered.
Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
drivers/iio/accel/adxl345.h | 3 ++-
drivers/iio/accel/adxl345_core.c | 32 +++++++++++++++++++++++++-------
drivers/iio/accel/adxl345_i2c.c | 2 +-
drivers/iio/accel/adxl345_spi.c | 2 +-
4 files changed, 29 insertions(+), 10 deletions(-)
diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
index 732820d34..e859c01d4 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -60,6 +60,7 @@ struct adxl345_chip_info {
int uscale;
};
-int adxl345_core_probe(struct device *dev, struct regmap *regmap);
+int adxl345_core_probe(struct device *dev, struct regmap *regmap,
+ int (*setup)(struct device*, struct regmap*));
#endif /* _ADXL345_H_ */
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index f875a6275..8d4a66d8c 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -168,7 +168,8 @@ static void adxl345_powerdown(void *regmap)
regmap_write(regmap, ADXL345_REG_POWER_CTL, ADXL345_POWER_CTL_STANDBY);
}
-int adxl345_core_probe(struct device *dev, struct regmap *regmap)
+int adxl345_core_probe(struct device *dev, struct regmap *regmap,
+ int (*setup)(struct device*, struct regmap*))
{
struct adxl345_data *data;
struct iio_dev *indio_dev;
@@ -179,6 +180,29 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap)
ADXL345_DATA_FORMAT_SELF_TEST);
int ret;
+ if (setup) {
+ /* Perform optional initial bus specific configuration */
+ ret = setup(dev, regmap);
+ if (ret)
+ return ret;
+
+ /* Enable full-resolution mode */
+ ret = regmap_update_bits(regmap, ADXL345_REG_DATA_FORMAT,
+ data_format_mask,
+ ADXL345_DATA_FORMAT_FULL_RES);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Failed to set data range\n");
+
+ } else {
+ /* Enable full-resolution mode (init all data_format bits) */
+ ret = regmap_write(regmap, ADXL345_REG_DATA_FORMAT,
+ ADXL345_DATA_FORMAT_FULL_RES);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Failed to set data range\n");
+ }
+
ret = regmap_read(regmap, ADXL345_REG_DEVID, ®val);
if (ret < 0)
return dev_err_probe(dev, ret, "Error reading device ID\n");
@@ -203,12 +227,6 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap)
indio_dev->channels = adxl345_channels;
indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
- /* Enable full-resolution mode */
- ret = regmap_update_bits(regmap, ADXL345_REG_DATA_FORMAT,
- data_format_mask, ADXL345_DATA_FORMAT_FULL_RES);
- if (ret)
- return dev_err_probe(dev, ret, "Failed to set data range\n");
-
/* Enable measurement mode */
ret = adxl345_powerup(data->regmap);
if (ret < 0)
diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
index a3084b0a8..4065b8f7c 100644
--- a/drivers/iio/accel/adxl345_i2c.c
+++ b/drivers/iio/accel/adxl345_i2c.c
@@ -27,7 +27,7 @@ static int adxl345_i2c_probe(struct i2c_client *client)
if (IS_ERR(regmap))
return dev_err_probe(&client->dev, PTR_ERR(regmap), "Error initializing regmap\n");
- return adxl345_core_probe(&client->dev, regmap);
+ return adxl345_core_probe(&client->dev, regmap, NULL);
}
static const struct adxl345_chip_info adxl345_i2c_info = {
diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
index 93ca349f1..1c0513bd3 100644
--- a/drivers/iio/accel/adxl345_spi.c
+++ b/drivers/iio/accel/adxl345_spi.c
@@ -33,7 +33,7 @@ static int adxl345_spi_probe(struct spi_device *spi)
if (IS_ERR(regmap))
return dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing regmap\n");
- return adxl345_core_probe(&spi->dev, regmap);
+ return adxl345_core_probe(&spi->dev, regmap, NULL);
}
static const struct adxl345_chip_info adxl345_spi_info = {
--
2.25.1
^ permalink raw reply related
* [PATCH v7 4/8] dt-bindings: iio: accel: adxl345: Add spi-3wire
From: Lothar Rubusch @ 2024-04-01 19:49 UTC (permalink / raw)
To: lars, Michael.Hennerich, jic23, robh+dt, krzysztof.kozlowski+dt,
conor+dt
Cc: linux-iio, devicetree, linux-kernel, eraretuya, l.rubusch,
Krzysztof Kozlowski
In-Reply-To: <20240401194906.56810-1-l.rubusch@gmail.com>
Add spi-3wire because the device allows to be configured for spi 3-wire
communication.
Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
index 07cacc3f6..280ed479e 100644
--- a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
+++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
@@ -32,6 +32,8 @@ properties:
spi-cpol: true
+ spi-3wire: true
+
interrupts:
maxItems: 1
--
2.25.1
^ permalink raw reply related
* [PATCH v7 3/8] iio: accel: adxl345: Move defines to header
From: Lothar Rubusch @ 2024-04-01 19:49 UTC (permalink / raw)
To: lars, Michael.Hennerich, jic23, robh+dt, krzysztof.kozlowski+dt,
conor+dt
Cc: linux-iio, devicetree, linux-kernel, eraretuya, l.rubusch
In-Reply-To: <20240401194906.56810-1-l.rubusch@gmail.com>
Move defines from core to the header file. Keep the defines block together
in one location.
Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
drivers/iio/accel/adxl345.h | 32 ++++++++++++++++++++++++++++++++
drivers/iio/accel/adxl345_core.c | 32 --------------------------------
2 files changed, 32 insertions(+), 32 deletions(-)
diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
index 284bd387c..732820d34 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -8,6 +8,38 @@
#ifndef _ADXL345_H_
#define _ADXL345_H_
+#define ADXL345_REG_DEVID 0x00
+#define ADXL345_REG_OFSX 0x1E
+#define ADXL345_REG_OFSY 0x1F
+#define ADXL345_REG_OFSZ 0x20
+#define ADXL345_REG_OFS_AXIS(index) (ADXL345_REG_OFSX + (index))
+#define ADXL345_REG_BW_RATE 0x2C
+#define ADXL345_REG_POWER_CTL 0x2D
+#define ADXL345_REG_DATA_FORMAT 0x31
+#define ADXL345_REG_DATAX0 0x32
+#define ADXL345_REG_DATAY0 0x34
+#define ADXL345_REG_DATAZ0 0x36
+#define ADXL345_REG_DATA_AXIS(index) \
+ (ADXL345_REG_DATAX0 + (index) * sizeof(__le16))
+
+#define ADXL345_BW_RATE GENMASK(3, 0)
+#define ADXL345_BASE_RATE_NANO_HZ 97656250LL
+
+#define ADXL345_POWER_CTL_MEASURE BIT(3)
+#define ADXL345_POWER_CTL_STANDBY 0x00
+
+#define ADXL345_DATA_FORMAT_RANGE GENMASK(1, 0) /* Set the g range */
+#define ADXL345_DATA_FORMAT_JUSTIFY BIT(2) /* Left-justified (MSB) mode */
+#define ADXL345_DATA_FORMAT_FULL_RES BIT(3) /* Up to 13-bits resolution */
+#define ADXL345_DATA_FORMAT_SELF_TEST BIT(7) /* Enable a self test */
+
+#define ADXL345_DATA_FORMAT_2G 0
+#define ADXL345_DATA_FORMAT_4G 1
+#define ADXL345_DATA_FORMAT_8G 2
+#define ADXL345_DATA_FORMAT_16G 3
+
+#define ADXL345_DEVID 0xE5
+
/*
* In full-resolution mode, scale factor is maintained at ~4 mg/LSB
* in all g ranges.
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index e4afc6d2a..f875a6275 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -17,38 +17,6 @@
#include "adxl345.h"
-#define ADXL345_REG_DEVID 0x00
-#define ADXL345_REG_OFSX 0x1e
-#define ADXL345_REG_OFSY 0x1f
-#define ADXL345_REG_OFSZ 0x20
-#define ADXL345_REG_OFS_AXIS(index) (ADXL345_REG_OFSX + (index))
-#define ADXL345_REG_BW_RATE 0x2C
-#define ADXL345_REG_POWER_CTL 0x2D
-#define ADXL345_REG_DATA_FORMAT 0x31
-#define ADXL345_REG_DATAX0 0x32
-#define ADXL345_REG_DATAY0 0x34
-#define ADXL345_REG_DATAZ0 0x36
-#define ADXL345_REG_DATA_AXIS(index) \
- (ADXL345_REG_DATAX0 + (index) * sizeof(__le16))
-
-#define ADXL345_BW_RATE GENMASK(3, 0)
-#define ADXL345_BASE_RATE_NANO_HZ 97656250LL
-
-#define ADXL345_POWER_CTL_MEASURE BIT(3)
-#define ADXL345_POWER_CTL_STANDBY 0x00
-
-#define ADXL345_DATA_FORMAT_RANGE GENMASK(1, 0) /* Set the g range */
-#define ADXL345_DATA_FORMAT_JUSTIFY BIT(2) /* Left-justified (MSB) mode */
-#define ADXL345_DATA_FORMAT_FULL_RES BIT(3) /* Up to 13-bits resolution */
-#define ADXL345_DATA_FORMAT_SELF_TEST BIT(7) /* Enable a self test */
-
-#define ADXL345_DATA_FORMAT_2G 0
-#define ADXL345_DATA_FORMAT_4G 1
-#define ADXL345_DATA_FORMAT_8G 2
-#define ADXL345_DATA_FORMAT_16G 3
-
-#define ADXL345_DEVID 0xE5
-
struct adxl345_data {
const struct adxl345_chip_info *info;
struct regmap *regmap;
--
2.25.1
^ permalink raw reply related
* [PATCH v7 2/8] iio: accel: adxl345: Group bus configuration
From: Lothar Rubusch @ 2024-04-01 19:49 UTC (permalink / raw)
To: lars, Michael.Hennerich, jic23, robh+dt, krzysztof.kozlowski+dt,
conor+dt
Cc: linux-iio, devicetree, linux-kernel, eraretuya, l.rubusch
In-Reply-To: <20240401194906.56810-1-l.rubusch@gmail.com>
Group the indio_dev initialization and bus configuration for improved
readability.
Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
drivers/iio/accel/adxl345_core.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index ff89215e9..e4afc6d2a 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -229,18 +229,18 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap)
if (!data->info)
return -ENODEV;
- /* Enable full-resolution mode */
- ret = regmap_update_bits(regmap, ADXL345_REG_DATA_FORMAT,
- data_format_mask, ADXL345_DATA_FORMAT_FULL_RES);
- if (ret)
- return dev_err_probe(dev, ret, "Failed to set data range\n");
-
indio_dev->name = data->info->name;
indio_dev->info = &adxl345_info;
indio_dev->modes = INDIO_DIRECT_MODE;
indio_dev->channels = adxl345_channels;
indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
+ /* Enable full-resolution mode */
+ ret = regmap_update_bits(regmap, ADXL345_REG_DATA_FORMAT,
+ data_format_mask, ADXL345_DATA_FORMAT_FULL_RES);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to set data range\n");
+
/* Enable measurement mode */
ret = adxl345_powerup(data->regmap);
if (ret < 0)
--
2.25.1
^ permalink raw reply related
* [PATCH v7 1/8] iio: accel: adxl345: Make data_range obsolete
From: Lothar Rubusch @ 2024-04-01 19:48 UTC (permalink / raw)
To: lars, Michael.Hennerich, jic23, robh+dt, krzysztof.kozlowski+dt,
conor+dt
Cc: linux-iio, devicetree, linux-kernel, eraretuya, l.rubusch
In-Reply-To: <20240401194906.56810-1-l.rubusch@gmail.com>
Replace write() data_format by regmap_update_bits() to keep bus specific
pre-configuration which might have happened before on the same register.
The bus specific bits in data_format register then need to be masked out,
Remove the data_range field from the struct adxl345_data, because it is
not used anymore.
Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
drivers/iio/accel/adxl345_core.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 8bd30a23e..ff89215e9 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -37,7 +37,11 @@
#define ADXL345_POWER_CTL_MEASURE BIT(3)
#define ADXL345_POWER_CTL_STANDBY 0x00
-#define ADXL345_DATA_FORMAT_FULL_RES BIT(3) /* Up to 13-bits resolution */
+#define ADXL345_DATA_FORMAT_RANGE GENMASK(1, 0) /* Set the g range */
+#define ADXL345_DATA_FORMAT_JUSTIFY BIT(2) /* Left-justified (MSB) mode */
+#define ADXL345_DATA_FORMAT_FULL_RES BIT(3) /* Up to 13-bits resolution */
+#define ADXL345_DATA_FORMAT_SELF_TEST BIT(7) /* Enable a self test */
+
#define ADXL345_DATA_FORMAT_2G 0
#define ADXL345_DATA_FORMAT_4G 1
#define ADXL345_DATA_FORMAT_8G 2
@@ -48,7 +52,6 @@
struct adxl345_data {
const struct adxl345_chip_info *info;
struct regmap *regmap;
- u8 data_range;
};
#define ADXL345_CHANNEL(index, axis) { \
@@ -202,6 +205,10 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap)
struct adxl345_data *data;
struct iio_dev *indio_dev;
u32 regval;
+ unsigned int data_format_mask = (ADXL345_DATA_FORMAT_RANGE |
+ ADXL345_DATA_FORMAT_JUSTIFY |
+ ADXL345_DATA_FORMAT_FULL_RES |
+ ADXL345_DATA_FORMAT_SELF_TEST);
int ret;
ret = regmap_read(regmap, ADXL345_REG_DEVID, ®val);
@@ -218,15 +225,14 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap)
data = iio_priv(indio_dev);
data->regmap = regmap;
- /* Enable full-resolution mode */
- data->data_range = ADXL345_DATA_FORMAT_FULL_RES;
data->info = device_get_match_data(dev);
if (!data->info)
return -ENODEV;
- ret = regmap_write(data->regmap, ADXL345_REG_DATA_FORMAT,
- data->data_range);
- if (ret < 0)
+ /* Enable full-resolution mode */
+ ret = regmap_update_bits(regmap, ADXL345_REG_DATA_FORMAT,
+ data_format_mask, ADXL345_DATA_FORMAT_FULL_RES);
+ if (ret)
return dev_err_probe(dev, ret, "Failed to set data range\n");
indio_dev->name = data->info->name;
--
2.25.1
^ permalink raw reply related
* [PATCH v7 0/8] iio: accel: adxl345: Add spi-3wire feature
From: Lothar Rubusch @ 2024-04-01 19:48 UTC (permalink / raw)
To: lars, Michael.Hennerich, jic23, robh+dt, krzysztof.kozlowski+dt,
conor+dt
Cc: linux-iio, devicetree, linux-kernel, eraretuya, l.rubusch
Pass a function setup() as pointer from SPI/I2C specific modules to the
core module. Implement setup() to pass the spi-3wire bus option, if
declared in the device-tree.
In the core module then update data_format register configuration bits
instead of overwriting it. The changes allow to remove a data_range field.
Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
V1 -> V2: Split into spi-3wire and refactoring
V2 -> V3: Split further, focus on needed changesets
V3 -> V4: Drop "Remove single info instances";
split "Group bus configuration" into separat
comment patch; reorder patch set
V4 -> V5: Refrase comments; Align comments to 75; rebuild FORMAT_MASK by
available flags; fix indention
V5 -> V6: Remove FORMAT_MASK by a local variable on call site;
Refrase comments;
Remove unneeded include
V6 -> V7: Restructure optional passing the setup() to core's probe()
Guarantee that initially a regmap_write() was called to init
all bits to a defined state
- When a setup() e.g. for 3wire is passed, then call
regmap_write() inside the setup(). In the following
core's probe() has to call regmap_update()
- When NULL is passed, then call regmap_write() in core's
probe()
- Refactoring: remove obvious comments and simplify code
Lothar Rubusch (8):
iio: accel: adxl345: Make data_range obsolete
iio: accel: adxl345: Group bus configuration
iio: accel: adxl345: Move defines to header
dt-bindings: iio: accel: adxl345: Add spi-3wire
iio: accel: adxl345: Pass function pointer to core
iio: accel: adxl345: Reorder probe initialization
iio: accel: adxl345: Add comment to probe
iio: accel: adxl345: Add spi-3wire option
.../bindings/iio/accel/adi,adxl345.yaml | 2 +
drivers/iio/accel/adxl345.h | 36 +++++++-
drivers/iio/accel/adxl345_core.c | 92 ++++++++++---------
drivers/iio/accel/adxl345_i2c.c | 2 +-
drivers/iio/accel/adxl345_spi.c | 10 +-
5 files changed, 94 insertions(+), 48 deletions(-)
--
2.25.1
^ permalink raw reply
* Re: [PATCH 6/6] iio: adc: ad7173: Add support for AD411x devices
From: David Lechner @ 2024-04-01 19:45 UTC (permalink / raw)
To: dumitru.ceclan
Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-iio,
devicetree, linux-kernel, Dumitru Ceclan
In-Reply-To: <20240401-ad4111-v1-6-34618a9cc502@analog.com>
On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
<devnull+dumitru.ceclan.analog.com@kernel.org> wrote:
>
> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
>
> Add support for AD4111/AD4112/AD4114/AD4115/AD4116.
>
> The AD411X family encompasses a series of low power, low noise, 24-bit,
> sigma-delta analog-to-digital converters that offer a versatile range of
> specifications.
>
> This family of ADCs integrates an analog front end suitable for processing
> both fully differential and single-ended, bipolar voltage inputs
> addressing a wide array of industrial and instrumentation requirements.
>
> - All ADCs have inputs with a precision voltage divider with a division
> ratio of 10.
> - AD4116 has 5 low level inputs without a voltage divider.
> - AD4111 and AD4112 support current inputs (0 mA to 20 mA) using a 50ohm
> shunt resistor.
>
> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> ---
> drivers/iio/adc/ad7173.c | 224 ++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 210 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> index 9526585e6929..ac32bd7dbd1e 100644
> --- a/drivers/iio/adc/ad7173.c
> +++ b/drivers/iio/adc/ad7173.c
> @@ -1,8 +1,9 @@
> // SPDX-License-Identifier: GPL-2.0+
> /*
> - * AD717x family SPI ADC driver
> + * AD717x and AD411x family SPI ADC driver
> *
> * Supported devices:
> + * AD4111/AD4112/AD4114/AD4115/AD4116
> * AD7172-2/AD7172-4/AD7173-8/AD7175-2
> * AD7175-8/AD7176-2/AD7177-2
> *
> @@ -72,6 +73,11 @@
> #define AD7175_2_ID 0x0cd0
> #define AD7172_4_ID 0x2050
> #define AD7173_ID 0x30d0
> +#define AD4111_ID 0x30d0
> +#define AD4112_ID 0x30d0
> +#define AD4114_ID 0x30d0
It might make it a bit more obvious that not all chips have a unique
ID if we rename AD7173_ID to AD7173_AD4111_AD4112_AD4114_ID rather
than introducing multiple macros with the same value.
Or leave it as AD7173_ID to keep it short and add a comment where it
is used with 411x chips in ad7173_device_info[].
> +#define AD4116_ID 0x34d0
> +#define AD4115_ID 0x38d0
> #define AD7175_8_ID 0x3cd0
> #define AD7177_ID 0x4fd0
> #define AD7173_ID_MASK GENMASK(15, 4)
> @@ -120,11 +126,20 @@
> #define AD7173_VOLTAGE_INT_REF_uV 2500000
> #define AD7173_TEMP_SENSIIVITY_uV_per_C 477
> #define AD7177_ODR_START_VALUE 0x07
> +#define AD4111_SHUNT_RESISTOR_OHM 50
> +#define AD4111_DIVIDER_RATIO 10
> +#define AD411X_VCOM_INPUT 0X10
> +#define AD4111_CURRENT_CHAN_CUTOFF 16
>
> #define AD7173_FILTER_ODR0_MASK GENMASK(5, 0)
> #define AD7173_MAX_CONFIGS 8
>
> enum ad7173_ids {
> + ID_AD4111,
> + ID_AD4112,
> + ID_AD4114,
> + ID_AD4115,
> + ID_AD4116,
> ID_AD7172_2,
> ID_AD7172_4,
> ID_AD7173_8,
> @@ -134,16 +149,26 @@ enum ad7173_ids {
> ID_AD7177_2,
> };
>
> +enum ad4111_current_channels {
> + AD4111_CURRENT_IN0P_IN0N,
> + AD4111_CURRENT_IN1P_IN1N,
> + AD4111_CURRENT_IN2P_IN2N,
> + AD4111_CURRENT_IN3P_IN3N,
> +};
> +
> struct ad7173_device_info {
> const unsigned int *sinc5_data_rates;
> unsigned int num_sinc5_data_rates;
> unsigned int odr_start_value;
> + unsigned int num_inputs_with_divider;
> unsigned int num_channels;
> unsigned int num_configs;
> unsigned int num_inputs;
Probably a good idea to change num_inputs to num_voltage_inputs so it
isn't confused with the total number of inputs.
Similarly num_voltage_inputs_with_divider instead of num_inputs_with_divider.
Also could use a comment to make it clear if num_voltage_inputs
includes num_voltage_inputs_with_divider or not. And that it doesn't
include VINCOM.
Probably also need some flag here to differentiate ADCINxx voltage
inputs on AD4116.
> unsigned int clock;
> unsigned int id;
> char *name;
> + bool has_current_inputs;
Maybe more future-proof to have num_current_inputs instead of bool?
> + bool has_vcom;
For consistency: has_vcom_input
> bool has_temp;
> bool has_input_buf;
> bool has_int_ref;
> @@ -189,6 +214,24 @@ struct ad7173_state {
> #endif
> };
>
> +static unsigned int ad4115_sinc5_data_rates[] = {
> + 24845000, 24845000, 20725000, 20725000, /* 0-3 */
> + 15564000, 13841000, 10390000, 10390000, /* 4-7 */
> + 4994000, 2499000, 1000000, 500000, /* 8-11 */
> + 395500, 200000, 100000, 59890, /* 12-15 */
> + 49920, 20000, 16660, 10000, /* 16-19 */
> + 5000, 2500, 2500, /* 20-22 */
> +};
> +
> +static unsigned int ad4116_sinc5_data_rates[] = {
> + 12422360, 12422360, 12422360, 12422360, /* 0-3 */
> + 10362690, 10362690, 7782100, 6290530, /* 4-7 */
> + 5194800, 2496900, 1007600, 499900, /* 8-11 */
> + 390600, 200300, 100000, 59750, /* 12-15 */
> + 49840, 20000, 16650, 10000, /* 16-19 */
> + 5000, 2500, 1250, /* 20-22 */
> +};
> +
> static const unsigned int ad7173_sinc5_data_rates[] = {
> 6211000, 6211000, 6211000, 6211000, 6211000, 6211000, 5181000, 4444000, /* 0-7 */
> 3115000, 2597000, 1007000, 503800, 381000, 200300, 100500, 59520, /* 8-15 */
> @@ -204,7 +247,91 @@ static const unsigned int ad7175_sinc5_data_rates[] = {
> 5000, /* 20 */
> };
>
> +static unsigned int ad4111_current_channel_config[] = {
> + [AD4111_CURRENT_IN0P_IN0N] = 0x1E8,
> + [AD4111_CURRENT_IN1P_IN1N] = 0x1C9,
> + [AD4111_CURRENT_IN2P_IN2N] = 0x1AA,
> + [AD4111_CURRENT_IN3P_IN3N] = 0x18B,
> +};
As mentioned in the DT bindings review, it would make more sense to
just use the datasheet numbers for the current input channels in the
diff-channels DT property, then we don't need this lookup table.
> +
> static const struct ad7173_device_info ad7173_device_info[] = {
> + [ID_AD4111] = {
> + .id = AD4111_ID,
> + .num_inputs_with_divider = 8,
> + .num_channels = 16,
> + .num_configs = 8,
> + .num_inputs = 8,
> + .num_gpios = 2,
> + .has_temp = true,
> + .has_vcom = true,
> + .has_input_buf = true,
> + .has_current_inputs = true,
> + .has_int_ref = true,
> + .clock = 2 * HZ_PER_MHZ,
> + .sinc5_data_rates = ad7173_sinc5_data_rates,
> + .num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
> + },
> + [ID_AD4112] = {
> + .id = AD4112_ID,
> + .num_inputs_with_divider = 8,
> + .num_channels = 16,
> + .num_configs = 8,
> + .num_inputs = 8,
> + .num_gpios = 2,
> + .has_vcom = true,
> + .has_temp = true,
> + .has_input_buf = true,
> + .has_current_inputs = true,
> + .has_int_ref = true,
> + .clock = 2 * HZ_PER_MHZ,
> + .sinc5_data_rates = ad7173_sinc5_data_rates,
> + .num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
> + },
> + [ID_AD4114] = {
> + .id = AD4114_ID,
> + .num_inputs_with_divider = 16,
> + .num_channels = 16,
> + .num_configs = 8,
> + .num_inputs = 16,
> + .num_gpios = 4,
> + .has_vcom = true,
> + .has_temp = true,
> + .has_input_buf = true,
> + .has_int_ref = true,
> + .clock = 2 * HZ_PER_MHZ,
> + .sinc5_data_rates = ad7173_sinc5_data_rates,
> + .num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
> + },
> + [ID_AD4115] = {
> + .id = AD4115_ID,
> + .num_inputs_with_divider = 16,
> + .num_channels = 16,
> + .num_configs = 8,
> + .num_inputs = 16,
> + .num_gpios = 4,
> + .has_vcom = true,
> + .has_temp = true,
> + .has_input_buf = true,
> + .has_int_ref = true,
> + .clock = 8 * HZ_PER_MHZ,
> + .sinc5_data_rates = ad4115_sinc5_data_rates,
> + .num_sinc5_data_rates = ARRAY_SIZE(ad4115_sinc5_data_rates),
> + },
> + [ID_AD4116] = {
> + .id = AD4116_ID,
> + .num_inputs_with_divider = 11,
> + .num_channels = 16,
> + .num_configs = 8,
> + .num_inputs = 16,
> + .num_gpios = 4,
> + .has_vcom = true,
> + .has_temp = true,
> + .has_input_buf = true,
> + .has_int_ref = true,
> + .clock = 4 * HZ_PER_MHZ,
> + .sinc5_data_rates = ad4116_sinc5_data_rates,
> + .num_sinc5_data_rates = ARRAY_SIZE(ad4116_sinc5_data_rates),
> + },
> [ID_AD7172_2] = {
> .name = "ad7172-2",
> .id = AD7172_2_ID,
> @@ -670,18 +797,34 @@ static int ad7173_read_raw(struct iio_dev *indio_dev,
>
> return IIO_VAL_INT;
> case IIO_CHAN_INFO_SCALE:
> - if (chan->type == IIO_TEMP) {
> +
> + switch (chan->type) {
> + case IIO_TEMP:
> temp = AD7173_VOLTAGE_INT_REF_uV * MILLI;
> temp /= AD7173_TEMP_SENSIIVITY_uV_per_C;
> *val = temp;
> *val2 = chan->scan_type.realbits;
> - } else {
> + break;
can we just return here instead of break?
> + case IIO_VOLTAGE:
> *val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel);
> *val2 = chan->scan_type.realbits - !!(ch->cfg.bipolar);
> +
> + if (chan->channel < st->info->num_inputs_with_divider)
> + *val *= AD4111_DIVIDER_RATIO;
> + break;
same: return here
> + case IIO_CURRENT:
> + *val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel);
> + *val /= AD4111_SHUNT_RESISTOR_OHM;
> + *val2 = chan->scan_type.realbits - !!(ch->cfg.bipolar);
Static analysis tools like to complain about using bool as int.
Probably more clear to write it as (ch->cfg.bipolar ? 1 : 0) anyway.
> + break;
return here
> + default:
> + return -EINVAL;
> }
> return IIO_VAL_FRACTIONAL_LOG2;
> case IIO_CHAN_INFO_OFFSET:
> - if (chan->type == IIO_TEMP) {
> +
> + switch (chan->type) {
> + case IIO_TEMP:
> /* 0 Kelvin -> raw sample */
> temp = -ABSOLUTE_ZERO_MILLICELSIUS;
> temp *= AD7173_TEMP_SENSIIVITY_uV_per_C;
> @@ -690,8 +833,13 @@ static int ad7173_read_raw(struct iio_dev *indio_dev,
> AD7173_VOLTAGE_INT_REF_uV *
> MILLI);
> *val = -temp;
> - } else {
> + break;
return ...
> + case IIO_VOLTAGE:
> + case IIO_CURRENT:
> *val = -BIT(chan->scan_type.realbits - 1);
Expecting a special case here, at least when ADCIN15 is configured for
pseudo-differential inputs.
> + break;
return ...
> + default:
> + return -EINVAL;
> }
> return IIO_VAL_INT;
> case IIO_CHAN_INFO_SAMP_FREQ:
> @@ -909,6 +1057,24 @@ static int ad7173_register_clk_provider(struct iio_dev *indio_dev)
> &st->int_clk_hw);
> }
>
> +static int ad4111_validate_current_ain(struct ad7173_state *st,
> + unsigned int ain[2])
> +{
> + struct device *dev = &st->sd.spi->dev;
> +
> + if (!st->info->has_current_inputs)
> + return dev_err_probe(dev, -EINVAL,
> + "Reg values equal to or higher than %d are restricted to models with current channels.\n",
> + AD4111_CURRENT_CHAN_CUTOFF);
> +
> + if (ain[1] != 0 && ain[0] >= ARRAY_SIZE(ad4111_current_channel_config))
> + return dev_err_probe(dev, -EINVAL,
> + "For current channel diff-channels must be <[0-%d],0>\n",
> + ARRAY_SIZE(ad4111_current_channel_config) - 1);
> +
> + return 0;
> +}
> +
> static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
> unsigned int ain[2])
> {
> @@ -951,7 +1117,7 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
> struct device *dev = indio_dev->dev.parent;
> struct iio_chan_spec *chan_arr, *chan;
> unsigned int ain[2], chan_index = 0;
> - int ref_sel, ret, num_channels;
> + int ref_sel, ret, reg, num_channels;
>
> num_channels = device_get_child_node_count(dev);
>
> @@ -1004,10 +1170,20 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
> if (ret)
> return ret;
>
> - ret = ad7173_validate_voltage_ain_inputs(st, ain);
> + ret = fwnode_property_read_u32(child, "reg", ®);
> if (ret)
> return ret;
>
> + if (reg >= AD4111_CURRENT_CHAN_CUTOFF) {
As mentioned in the DT bindings review, using reg to determine if an a
channel is a current input or voltage input seems wrong/fragile. We
should be able to use the has_current_inputs flag and the input
numbers instead to determine the type of input.
> + ret = ad4111_validate_current_ain(st, ain);
> + if (ret)
> + return ret;
> + } else {
> + ret = ad7173_validate_voltage_ain_inputs(st, ain);
> + if (ret)
> + return ret;
> + }
> +
> ret = fwnode_property_match_property_string(child,
> "adi,reference-select",
> ad7173_ref_sel_str,
> @@ -1028,15 +1204,22 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
> *chan = ad7173_channel_template;
> chan->address = chan_index;
> chan->scan_index = chan_index;
> - chan->channel = ain[0];
> - chan->channel2 = ain[1];
> - chan->differential = true;
>
> - chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]);
> + if (reg >= AD4111_CURRENT_CHAN_CUTOFF) {
> + chan->type = IIO_CURRENT;
> + chan->channel = ain[0];
> + chan_st_priv->ain = ad4111_current_channel_config[ain[0]];
> + } else {
> + chan->channel = ain[0];
> + chan->channel2 = ain[1];
> + chan->differential = true;
Expecting chan->differential = false when ADCIN15 is configured for
pseudo-differential inputs.
Also, perhaps missed in previous reviews, I would expect
chan->differential = false when channels are used as single-ended.
> +
> + chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]);
> + chan_st_priv->cfg.input_buf = st->info->has_input_buf;
> + }
> +
> chan_st_priv->chan_reg = chan_index;
> - chan_st_priv->cfg.input_buf = st->info->has_input_buf;
> chan_st_priv->cfg.odr = 0;
> -
> chan_st_priv->cfg.bipolar = fwnode_property_read_bool(child, "bipolar");
> if (chan_st_priv->cfg.bipolar)
> chan->info_mask_separate |= BIT(IIO_CHAN_INFO_OFFSET);
> @@ -1167,6 +1350,14 @@ static int ad7173_probe(struct spi_device *spi)
> }
>
> static const struct of_device_id ad7173_of_match[] = {
> + { .compatible = "ad4111",
> + .data = &ad7173_device_info[ID_AD4111]},
> + { .compatible = "ad4112",
> + .data = &ad7173_device_info[ID_AD4112]},
> + { .compatible = "ad4114",
> + .data = &ad7173_device_info[ID_AD4114]},
> + { .compatible = "ad4115",
> + .data = &ad7173_device_info[ID_AD4115]},
> { .compatible = "adi,ad7172-2",
> .data = &ad7173_device_info[ID_AD7172_2]},
> { .compatible = "adi,ad7172-4",
> @@ -1186,6 +1377,11 @@ static const struct of_device_id ad7173_of_match[] = {
> MODULE_DEVICE_TABLE(of, ad7173_of_match);
>
> static const struct spi_device_id ad7173_id_table[] = {
> + { "ad4111", (kernel_ulong_t)&ad7173_device_info[ID_AD4111]},
> + { "ad4112", (kernel_ulong_t)&ad7173_device_info[ID_AD4112]},
> + { "ad4114", (kernel_ulong_t)&ad7173_device_info[ID_AD4114]},
> + { "ad4115", (kernel_ulong_t)&ad7173_device_info[ID_AD4115]},
> + { "ad4116", (kernel_ulong_t)&ad7173_device_info[ID_AD4116]},
> { "ad7172-2", (kernel_ulong_t)&ad7173_device_info[ID_AD7172_2]},
> { "ad7172-4", (kernel_ulong_t)&ad7173_device_info[ID_AD7172_4]},
> { "ad7173-8", (kernel_ulong_t)&ad7173_device_info[ID_AD7173_8]},
> @@ -1210,5 +1406,5 @@ module_spi_driver(ad7173_driver);
> MODULE_IMPORT_NS(IIO_AD_SIGMA_DELTA);
> MODULE_AUTHOR("Lars-Peter Clausen <lars@metafo.de>");
> MODULE_AUTHOR("Dumitru Ceclan <dumitru.ceclan@analog.com>");
> -MODULE_DESCRIPTION("Analog Devices AD7172/AD7173/AD7175/AD7176 ADC driver");
> +MODULE_DESCRIPTION("Analog Devices AD717x and AD411x ADC driver");
> MODULE_LICENSE("GPL");
>
> --
> 2.43.0
>
>
^ permalink raw reply
* Re: [PATCH 5/6] iio: adc: ad7173: Remove index from temp channel
From: David Lechner @ 2024-04-01 19:40 UTC (permalink / raw)
To: dumitru.ceclan
Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-iio,
devicetree, linux-kernel, Dumitru Ceclan
In-Reply-To: <20240401-ad4111-v1-5-34618a9cc502@analog.com>
On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
<devnull+dumitru.ceclan.analog.com@kernel.org> wrote:
>
> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
>
> Temperature channel is unique per device, index is not needed.
>
> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> ---
This breaks userspace, so the commit message should explain why it is
safe to do this (e.g. driver hasn't reached mainline yet, so won't
break existing users since there are none).
^ permalink raw reply
* Re: [PATCH 4/6] iio: adc: ad7173: refactor ain and vref selection
From: David Lechner @ 2024-04-01 19:40 UTC (permalink / raw)
To: dumitru.ceclan
Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-iio,
devicetree, linux-kernel, Dumitru Ceclan
In-Reply-To: <20240401-ad4111-v1-4-34618a9cc502@analog.com>
On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
<devnull+dumitru.ceclan.analog.com@kernel.org> wrote:
>
> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
>
> Move validation of analog inputs and reference voltage selection to
> separate functions.
>
> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> ---
Same as my comment on PATCH 3/6. We would like to know why this change
is being made.
^ permalink raw reply
* Re: [PATCH 3/6] iio: adc: ad7173: refactor channel configuration parsing
From: David Lechner @ 2024-04-01 19:39 UTC (permalink / raw)
To: dumitru.ceclan
Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-iio,
devicetree, linux-kernel, Dumitru Ceclan
In-Reply-To: <20240401-ad4111-v1-3-34618a9cc502@analog.com>
On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
<devnull+dumitru.ceclan.analog.com@kernel.org> wrote:
>
> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
>
> Move configurations regarding number of channels from
> *_fw_parse_device_config to *_fw_parse_channel_config.
>
> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> ---
Commit messages need to explain _why_ the change is being made [1]. It
is not obvious to me why this needs to be moved.
[1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format
^ permalink raw reply
* Re: [PATCH 2/6] iio: adc: ad7173: fix buffers enablement for ad7176-2
From: David Lechner @ 2024-04-01 19:38 UTC (permalink / raw)
To: dumitru.ceclan
Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-iio,
devicetree, linux-kernel, Dumitru Ceclan
In-Reply-To: <20240401-ad4111-v1-2-34618a9cc502@analog.com>
On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
<devnull+dumitru.ceclan.analog.com@kernel.org> wrote:
>
> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
>
> AD7176-2 does not feature input buffers, enable buffers only on
> supported models.
>
> Fixes: cff259bf7274 ("iio: adc: ad7173: fix buffers enablement for ad7176-2")
> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> ---
To be consistent with has_temp, maybe add `.has_input_buf = false,` to
ID_AD7176_2.
But either way:
Reviewed-by: David Lechner <dlechner@baylibre.com>
^ permalink raw reply
* Re: [PATCH 1/6] dt-bindings: adc: ad7173: add support for ad411x
From: David Lechner @ 2024-04-01 19:37 UTC (permalink / raw)
To: dumitru.ceclan
Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-iio,
devicetree, linux-kernel, Dumitru Ceclan
In-Reply-To: <20240401-ad4111-v1-1-34618a9cc502@analog.com>
On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
<devnull+dumitru.ceclan.analog.com@kernel.org> wrote:
>
> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
>
> Add support for: AD4111, AD4112, AD4114, AD4115, AD4116.
>
> AD411x family ADCs support a VCOM pin, dedicated for single-ended usage.
> AD4111/AD4112 support current channels, usage is implemented by
> specifying channel reg values bigger than 15.
>
> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> ---
> .../devicetree/bindings/iio/adc/adi,ad7173.yaml | 59 +++++++++++++++++++++-
> 1 file changed, 57 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> index ea6cfcd0aff4..bba2de0a52f3 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> @@ -19,7 +19,18 @@ description: |
> primarily for measurement of signals close to DC but also delivers
> outstanding performance with input bandwidths out to ~10kHz.
>
> + Analog Devices AD411x ADC's:
> + The AD411X family encompasses a series of low power, low noise, 24-bit,
> + sigma-delta analog-to-digital converters that offer a versatile range of
> + specifications. They integrate an analog front end suitable for processing
> + fully differential/single-ended and bipolar voltage inputs.
> +
> Datasheets for supported chips:
> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4111.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4112.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4114.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4115.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4116.pdf
> https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf
> https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-4.pdf
> https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf
> @@ -31,6 +42,11 @@ description: |
> properties:
> compatible:
> enum:
> + - adi,ad4111
> + - adi,ad4112
> + - adi,ad4114
> + - adi,ad4115
> + - adi,ad4116
> - adi,ad7172-2
> - adi,ad7172-4
> - adi,ad7173-8
> @@ -125,10 +141,19 @@ patternProperties:
>
> properties:
> reg:
> + description:
> + Reg values 16-19 are only permitted for ad4111/ad4112 current channels.
> minimum: 0
> - maximum: 15
> + maximum: 19
This looks wrong. Isn't reg describing the number of logical channels
(# of channel config registers)?
After reviewing the driver, I see that > 16 is used as a way of
flagging current inputs, but still seems like the wrong way to do it.
See suggestion below.
>
> diff-channels:
> + description:
> + For using current channels specify only the positive channel.
> + (IIN2+, IIN2−) -> diff-channels = <2 0>
I find this a bit confusing since 2 is already VIN2 and 0 is already
VIN0. I think it would make more sense to assign unique channel
numbers individually to the negative and positive current inputs.
Also, I think it makes sense to use the same numbers that the
registers in the datasheet use (8 - 11 for negative and 12 to 15 for
positive).
So: (IIN2+, IIN2−) -> diff-channels = <13 10>
> +
> + Family AD411x supports a dedicated VCOM voltage input.
> + To select it set the second channel to 16.
> + (VIN2, VCOM) -> diff-channels = <2 16>
The 411x datasheets call this pin VINCOM so calling it VCOM here is a
bit confusing.
Also, do we need to add a vincom-supply to get this voltage? Or is it
safe to assume it is always connected to AVSS? The datasheet seems to
indicate that the latter is the case. But then it also has this
special case (at least for AD4116, didn't check all datasheets)
"VIN10, VINCOM (single-ended or differential pair)". If it can be used
as part of a fully differential input, we probably need some extra
flag to indicate that case.
Similarly, do we need special handling for ADCIN15 on AD4116? It has a
"(pseudo differential or differential pair)" notation that other
inputs don't. In other words, it is more like VINCOM than it is to the
other ADCINxx pins. So we probably need an adcin15-supply for this pin
to properly get the right channel configuration. I.e. the logic in the
IIO driver would be if adcin15-supply is present, any channels that
use this input are pseudo-differential, otherwise any channels that
use it are fully differential.
> items:
> minimum: 0
> maximum: 31
> @@ -166,7 +191,6 @@ allOf:
> - $ref: /schemas/spi/spi-peripheral-props.yaml#
>
> # Only ad7172-4, ad7173-8 and ad7175-8 support vref2
> - # Other models have [0-3] channel registers
Did you forget to remove
reg:
maximum: 3
from this if statement that this comment is referring to?
> - if:
> properties:
> compatible:
> @@ -187,6 +211,37 @@ allOf:
> - vref
> - refout-avss
> - avdd
> +
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - adi,ad4114
> + - adi,ad4115
> + - adi,ad4116
> + - adi,ad7173-8
> + - adi,ad7175-8
> + then:
> + patternProperties:
> + "^channel@[0-9a-f]$":
> + properties:
> + reg:
> + maximum: 15
As with the previous reg comment, this if statement should not be
needed since maximum should not be changed to 19.
> +
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - adi,ad7172-2
> + - adi,ad7175-2
> + - adi,ad7176-2
> + - adi,ad7177-2
> + then:
> + patternProperties:
> + "^channel@[0-9a-f]$":
> + properties:
> reg:
> maximum: 3
It looks to me like AD7172-4 actually has 8 possible channels rather
than 16. So it would need a special condition as well. But that is a
bug in the previous bindings and should therefore be fixed in a
separate patch.
^ permalink raw reply
* Re: [PATCH 2/2] arm64: dts: qcom: sc7180: Fix UFS PHY clocks
From: Dmitry Baryshkov @ 2024-04-01 19:28 UTC (permalink / raw)
To: Danila Tikhonov
Cc: andersson, konrad.dybcio, vkoul, kishon, robh,
krzysztof.kozlowski+dt, conor+dt, cros-qcom-dts-watchers,
manivannan.sadhasivam, davidwronek, linux-arm-msm, linux-phy,
devicetree, linux-kernel
In-Reply-To: <20240401182240.55282-3-danila@jiaxyga.com>
On Mon, 1 Apr 2024 at 21:23, Danila Tikhonov <danila@jiaxyga.com> wrote:
>
> QMP PHY used in SC7180 requires 3 clocks:
>
> * ref - 19.2MHz reference clock from RPMh
> * ref_aux - Auxiliary reference clock from GCC
> * qref - QREF clock from GCC
>
> While at it, let's move 'clocks' property before 'clock-names' to match
> the style used commonly.
>
> Signed-off-by: Danila Tikhonov <danila@jiaxyga.com>
> ---
> arch/arm64/boot/dts/qcom/sc7180.dtsi | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v6 RESEND 4/4] arm64: dts: qcom: aim300: add AIM300 AIoT
From: Dmitry Baryshkov @ 2024-04-01 19:22 UTC (permalink / raw)
To: Tengfei Fan
Cc: andersson, konrad.dybcio, robh, krzysztof.kozlowski+dt, conor+dt,
keescook, tony.luck, gpiccoli, linux-arm-msm, devicetree,
linux-kernel, linux-hardening, kernel, Qiang Yu, Ziyue Zhang
In-Reply-To: <20240401093843.2591147-5-quic_tengfan@quicinc.com>
On Mon, 1 Apr 2024 at 12:40, Tengfei Fan <quic_tengfan@quicinc.com> wrote:
>
> Add AIM300 AIoT Carrier board DTS support, including usb, UART, PCIe,
> I2C functions support.
> Here is a diagram of AIM300 AIoT Carrie Board and SoM
> +--------------------------------------------------+
> | AIM300 AIOT Carrie Board |
> | |
> | +-----------------+ |
> |power----->| Fixed regulator |---------+ |
> | +-----------------+ | |
> | | |
> | v VPH_PWR |
> | +----------------------------------------------+ |
> | | AIM300 SOM | | |
> | | |VPH_PWR | |
> | | v | |
> | | +-------+ +--------+ +------+ | |
> | | | UFS | | QCS8550| |PMIC | | |
> | | +-------+ +--------+ +------+ | |
> | | | |
> | +----------------------------------------------+ |
> | |
> | +----+ +------+ |
> | |USB | | UART | |
> | +----+ +------+ |
> +--------------------------------------------------+
>
> Co-developed-by: Qiang Yu <quic_qianyu@quicinc.com>
> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
> Co-developed-by: Ziyue Zhang <quic_ziyuzhan@quicinc.com>
> Signed-off-by: Ziyue Zhang <quic_ziyuzhan@quicinc.com>
> Signed-off-by: Tengfei Fan <quic_tengfan@quicinc.com>
> ---
> arch/arm64/boot/dts/qcom/Makefile | 1 +
> .../boot/dts/qcom/qcs8550-aim300-aiot.dts | 384 ++++++++++++++++++
> 2 files changed, 385 insertions(+)
> create mode 100644 arch/arm64/boot/dts/qcom/qcs8550-aim300-aiot.dts
>
> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
> index 7d40ec5e7d21..02d9bc3bfce7 100644
> --- a/arch/arm64/boot/dts/qcom/Makefile
> +++ b/arch/arm64/boot/dts/qcom/Makefile
> @@ -96,6 +96,7 @@ dtb-$(CONFIG_ARCH_QCOM) += qcm6490-idp.dtb
> dtb-$(CONFIG_ARCH_QCOM) += qcs404-evb-1000.dtb
> dtb-$(CONFIG_ARCH_QCOM) += qcs404-evb-4000.dtb
> dtb-$(CONFIG_ARCH_QCOM) += qcs6490-rb3gen2.dtb
> +dtb-$(CONFIG_ARCH_QCOM) += qcs8550-aim300-aiot.dtb
> dtb-$(CONFIG_ARCH_QCOM) += qdu1000-idp.dtb
> dtb-$(CONFIG_ARCH_QCOM) += qrb2210-rb1.dtb
> dtb-$(CONFIG_ARCH_QCOM) += qrb4210-rb2.dtb
> diff --git a/arch/arm64/boot/dts/qcom/qcs8550-aim300-aiot.dts b/arch/arm64/boot/dts/qcom/qcs8550-aim300-aiot.dts
> new file mode 100644
> index 000000000000..8188766c3d84
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/qcs8550-aim300-aiot.dts
> @@ -0,0 +1,384 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +/*
> + * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +/dts-v1/;
> +
> +#include <dt-bindings/leds/common.h>
> +#include "qcs8550-aim300.dtsi"
> +#include "pm8010.dtsi"
> +#include "pmr735d_a.dtsi"
> +#include "pmr735d_b.dtsi"
> +
> +/ {
> + model = "Qualcomm Technologies, Inc. QCS8550 AIM300 AIOT";
> + compatible = "qcom,qcs8550-aim300-aiot", "qcom,qcs8550-aim300", "qcom,qcs8550",
> + "qcom,sm8550";
> +
> + aliases {
> + serial0 = &uart7;
> + };
> +
> + chosen {
> + stdout-path = "serial0:115200n8";
> + };
> +
> + gpio-keys {
> + compatible = "gpio-keys";
> +
> + pinctrl-0 = <&volume_up_n>;
> + pinctrl-names = "default";
> +
> + key-volume-up {
> + label = "Volume Up";
> + debounce-interval = <15>;
> + gpios = <&pm8550_gpios 6 GPIO_ACTIVE_LOW>;
> + linux,code = <KEY_VOLUMEUP>;
> + linux,can-disable;
> + wakeup-source;
> + };
> + };
> +
> + pmic-glink {
> + compatible = "qcom,sm8550-pmic-glink", "qcom,pmic-glink";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + orientation-gpios = <&tlmm 11 GPIO_ACTIVE_HIGH>;
> +
> + connector@0 {
> + compatible = "usb-c-connector";
> + reg = <0>;
> + power-role = "dual";
> + data-role = "dual";
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> +
> + pmic_glink_hs_in: endpoint {
> + remote-endpoint = <&usb_1_dwc3_hs>;
> + };
> + };
> +
> + port@1 {
> + reg = <1>;
> +
> + pmic_glink_ss_in: endpoint {
> + remote-endpoint = <&redriver_ss_out>;
> + };
> + };
> +
> + port@2 {
> + reg = <2>;
> +
> + pmic_glink_sbu: endpoint {
> + remote-endpoint = <&fsa4480_sbu_mux>;
> + };
> + };
> + };
> + };
> + };
> +
> + vph_pwr: regulator-vph-pwr {
> + compatible = "regulator-fixed";
> + regulator-name = "vph_pwr";
> + regulator-min-microvolt = <3700000>;
> + regulator-max-microvolt = <3700000>;
> +
> + regulator-always-on;
> + regulator-boot-on;
> + };
> +};
> +
> +&apps_rsc {
> + regulators-0 {
> + vdd-bob1-supply = <&vph_pwr>;
> + vdd-bob2-supply = <&vph_pwr>;
> + };
> +
> + regulators-3 {
> + vdd-s4-supply = <&vph_pwr>;
> + vdd-s5-supply = <&vph_pwr>;
> + };
> +
> + regulators-4 {
> + vdd-s4-supply = <&vph_pwr>;
> + };
> +
> + regulators-5 {
> + vdd-s1-supply = <&vph_pwr>;
> + vdd-s2-supply = <&vph_pwr>;
> + vdd-s3-supply = <&vph_pwr>;
> + vdd-s4-supply = <&vph_pwr>;
> + vdd-s5-supply = <&vph_pwr>;
> + vdd-s6-supply = <&vph_pwr>;
> + };
> +};
> +
> +&i2c_hub_2 {
> + status = "okay";
> +
> + typec-mux@42 {
> + compatible = "fcs,fsa4480";
> + reg = <0x42>;
> +
> + vcc-supply = <&vreg_bob1>;
> +
> + mode-switch;
> + orientation-switch;
> +
> + port {
> + fsa4480_sbu_mux: endpoint {
> + remote-endpoint = <&pmic_glink_sbu>;
> + };
> + };
> + };
> +
> + typec-retimer@1c {
> + compatible = "onnn,nb7vpq904m";
> + reg = <0x1c>;
> +
> + vcc-supply = <&vreg_l15b_1p8>;
> +
> + orientation-switch;
> + retimer-switch;
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> +
> + redriver_ss_out: endpoint {
> + remote-endpoint = <&pmic_glink_ss_in>;
> + };
> + };
> +
> + port@1 {
> + reg = <1>;
> +
> + redriver_ss_in: endpoint {
> + data-lanes = <3 2 1 0>;
> + remote-endpoint = <&usb_dp_qmpphy_out>;
> + };
> + };
> + };
> + };
> +};
> +
> +&mdss_dsi0 {
> + vdda-supply = <&vreg_l3e_1p2>;
Is this wired on the carrier board or on the AIC300 SoM?
> + status = "okay";
> +
> + panel@0 {
> + compatible = "visionox,vtdr6130";
> + reg = <0>;
> +
> + pinctrl-0 = <&dsi_active>, <&te_active>;
> + pinctrl-1 = <&dsi_suspend>, <&te_suspend>;
> + pinctrl-names = "default", "sleep";
> +
> + reset-gpios = <&tlmm 133 GPIO_ACTIVE_LOW>;
> +
> + vci-supply = <&vreg_l13b_3p0>;
> + vdd-supply = <&vreg_l11b_1p2>;
> + vddio-supply = <&vreg_l12b_1p8>;
> +
> + port {
> + panel0_in: endpoint {
> + remote-endpoint = <&mdss_dsi0_out>;
> + };
> + };
> + };
> +};
> +
> +&mdss_dsi0_out {
> + remote-endpoint = <&panel0_in>;
> + data-lanes = <0 1 2 3>;
> +};
> +
> +&mdss_dsi0_phy {
> + vdds-supply = <&vreg_l1e_0p88>;
This too
> + status = "okay";
> +};
> +
> +&pcie0 {
> + perst-gpios = <&tlmm 94 GPIO_ACTIVE_LOW>;
> + wake-gpios = <&tlmm 96 GPIO_ACTIVE_HIGH>;
And this
> +
> + pinctrl-0 = <&pcie0_default_state>;
> + pinctrl-names = "default";
> +
> + status = "okay";
> +};
> +
> +&pcie0_phy {
> + vdda-phy-supply = <&vreg_l1e_0p88>;
> + vdda-pll-supply = <&vreg_l3e_1p2>;
You guess the question. I think I'll stop here. Please review your
changes here, which are really specific to the carrier board and which
apply to the SoM.
> +
> + status = "okay";
> +};
> +
> +&pcie_1_phy_aux_clk {
> + clock-frequency = <1000>;
> +};
> +
> +&pcie1 {
> + perst-gpios = <&tlmm 97 GPIO_ACTIVE_LOW>;
> + wake-gpios = <&tlmm 99 GPIO_ACTIVE_HIGH>;
> +
> + pinctrl-0 = <&pcie1_default_state>;
> + pinctrl-names = "default";
> +
> + status = "okay";
> +};
> +
> +&pcie1_phy {
> + vdda-phy-supply = <&vreg_l3c_0p9>;
> + vdda-pll-supply = <&vreg_l3e_1p2>;
> + vdda-qref-supply = <&vreg_l1e_0p88>;
> +
> + status = "okay";
> +};
> +
> +&pm8550_gpios {
> + volume_up_n: volume-up-n-state {
> + pins = "gpio6";
> + function = "normal";
> + power-source = <1>;
> + bias-pull-up;
> + input-enable;
> + };
> +};
> +
> +&pm8550b_eusb2_repeater {
> + vdd18-supply = <&vreg_l15b_1p8>;
> + vdd3-supply = <&vreg_l5b_3p1>;
> +};
> +
> +
> +&pon_pwrkey {
> + status = "okay";
> +};
> +
> +&pon_resin {
> + linux,code = <KEY_VOLUMEDOWN>;
> +
> + status = "okay";
> +};
> +
> +&qupv3_id_0 {
> + status = "okay";
> +};
> +
> +&remoteproc_adsp {
> + firmware-name = "qcom/qcs8550/adsp.mbn",
> + "qcom/qcs8550/adsp_dtbs.elf";
> + status = "okay";
> +};
> +
> +&remoteproc_cdsp {
> + firmware-name = "qcom/qcs8550/cdsp.mbn",
> + "qcom/qcs8550/cdsp_dtbs.elf";
> + status = "okay";
> +};
> +
> +&sleep_clk {
> + clock-frequency = <32000>;
> +};
> +
> +&swr1 {
> + status = "okay";
> +};
> +
> +&swr2 {
> + status = "okay";
> +};
> +
> +&tlmm {
> + gpio-reserved-ranges = <32 8>;
> +
> + dsi_active: dsi-active-state {
> + pins = "gpio133";
> + function = "gpio";
> + drive-strength = <8>;
> + bias-disable;
> + };
> +
> + dsi_suspend: dsi-suspend-state {
> + pins = "gpio133";
> + function = "gpio";
> + drive-strength = <2>;
> + bias-pull-down;
> + };
> +
> + te_active: te-active-state {
> + pins = "gpio86";
> + function = "mdp_vsync";
> + drive-strength = <2>;
> + bias-pull-down;
> + };
> +
> + te_suspend: te-suspend-state {
> + pins = "gpio86";
> + function = "mdp_vsync";
> + drive-strength = <2>;
> + bias-pull-down;
> + };
> +};
> +
> +&uart7 {
> + status = "okay";
> +};
> +
> +&usb_1 {
> + status = "okay";
> +};
> +
> +&usb_1_dwc3 {
> + dr_mode = "otg";
> + usb-role-switch;
> +};
> +
> +&usb_1_dwc3_hs {
> + remote-endpoint = <&pmic_glink_hs_in>;
> +};
> +
> +&usb_1_dwc3_ss {
> + remote-endpoint = <&usb_dp_qmpphy_usb_ss_in>;
> +};
> +
> +&usb_1_hsphy {
> + phys = <&pm8550b_eusb2_repeater>;
> +
> + vdd-supply = <&vreg_l1e_0p88>;
> + vdda12-supply = <&vreg_l3e_1p2>;
> +
> + status = "okay";
> +};
> +
> +&usb_dp_qmpphy {
> + vdda-phy-supply = <&vreg_l3e_1p2>;
> + vdda-pll-supply = <&vreg_l3f_0p88>;
> +
> + orientation-switch;
> +
> + status = "okay";
> +};
> +
> +&usb_dp_qmpphy_out {
> + remote-endpoint = <&redriver_ss_in>;
> +};
> +
> +&usb_dp_qmpphy_usb_ss_in {
> + remote-endpoint = <&usb_1_dwc3_ss>;
> +};
> +
> +&xo_board {
> + clock-frequency = <76800000>;
> +};
> --
> 2.25.1
>
>
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v6 RESEND 2/4] arm64: dts: qcom: qcs8550: introduce qcs8550 dtsi
From: Dmitry Baryshkov @ 2024-04-01 19:13 UTC (permalink / raw)
To: Tengfei Fan
Cc: andersson, konrad.dybcio, robh, krzysztof.kozlowski+dt, conor+dt,
keescook, tony.luck, gpiccoli, linux-arm-msm, devicetree,
linux-kernel, linux-hardening, kernel
In-Reply-To: <20240401093843.2591147-3-quic_tengfan@quicinc.com>
On Mon, 1 Apr 2024 at 12:40, Tengfei Fan <quic_tengfan@quicinc.com> wrote:
>
> QCS8550 is derived from SM8550. The differnece between SM8550 and
> QCS8550 is QCS8550 doesn't have modem RF system. QCS8550 is mainly used
> in IoT scenarios.
> QCS8550 firmware has different memory map with SM8550 firmware. The
> memory map will be runtime added through bootloader.
> There are 3 types of reserved memory regions here:
> 1. Firmware related regions which aren't shared with kernel.
> The device tree source in kernel doesn't need to have node to indicate
> the firmware related reserved information. OS bootloader conveys the
> information by update device tree in runtime.
> This will be described as: UEFI saves the physical address of the
> UEFI System Table to dts file's chosen node. Kernel read this table and
> add reserved memory regions to efi config table. Current reserved memory
> region may have reserved region which was not yet used, release note of
> the firmware have such kind of information.
> 2. Firmware related memory regions which are shared with Kernel
> Each region has a specific node with specific label name for later
> phandle reference from other driver dt node.
> 3. PIL regions.
> PIL regions will be reserved and then assigned to subsystem firmware
> later.
> Here is a reserved memory map for this platform:
> 0x100000000 +------------------+
> | |
> | Firmware Related |
> | |
> 0xd4d00000 +------------------+
> | |
> | Kernel Available |
> | |
> 0xa7000000 +------------------+
> | |
> | PIL Region |
> | |
> 0x8a800000 +------------------+
> | |
> | Firmware Related |
> | |
> 0x80000000 +------------------+
> Note that:
> 0xa7000000..0xA8000000 is used by bootloader, when kernel boot up,
> it is available for kernel usage. This region is not suggested to be
> used by kernel features like ramoops, suspend resume etc.
>
> Signed-off-by: Tengfei Fan <quic_tengfan@quicinc.com>
> ---
> arch/arm64/boot/dts/qcom/qcs8550.dtsi | 169 ++++++++++++++++++++++++++
> 1 file changed, 169 insertions(+)
> create mode 100644 arch/arm64/boot/dts/qcom/qcs8550.dtsi
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Minor nit below.
>
> diff --git a/arch/arm64/boot/dts/qcom/qcs8550.dtsi b/arch/arm64/boot/dts/qcom/qcs8550.dtsi
> new file mode 100644
> index 000000000000..a3ebf3d4e16d
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/qcs8550.dtsi
> @@ -0,0 +1,169 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +/*
> + * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include "sm8550.dtsi"
> +
> +/delete-node/ &reserved_memory;
> +
> +/ {
> + reserved_memory: reserved-memory {
> + #address-cells = <2>;
> + #size-cells = <2>;
> + ranges;
> +
> +
> + /* These are 3 types of reserved memory regions here:
> + * 1. Firmware related regions which aren't shared with kernel.
> + * The device tree source in kernel doesn't need to have node to
> + * indicate the firmware related reserved information. OS bootloader
> + * conveys the information by update device tree in runtime.
> + * This will be described as: UEFI saves the physical address of
> + * the UEFI System Table to dts file's chosen node. Kernel read this
> + * table and add reserved memory regions to efi config table. Current
> + * reserved memory region may have reserved region which was not yet
> + * used, release note of the firmware have such kind of information.
> + * 2. Firmware related memory regions which are shared with Kernel.
> + * Each region has a specific node with specific label name for
> + * later phandle reference from other driver dt node.
> + * 3. PIL regions.
> + * PIL regions will be reserved and then assigned to subsystem
> + * firmware later.
> + * Here is a reserved memory map for this platform:
> + * 0x100000000 +------------------+
> + * | |
> + * | Firmware Related |
> + * | |
> + * 0xd4d00000 +------------------+
> + * | |
> + * | Kernel Available |
> + * | |
> + * 0xa7000000 +------------------+
> + * | |
> + * | PIL Region |
> + * | |
> + * 0x8a800000 +------------------+
> + * | |
> + * | Firmware Related |
> + * | |
> + * 0x80000000 +------------------+
> + * Note that:
> + * 0xa7000000..0xA8000000 is used by bootloader, when kernel boot up,
> + * it is available for kernel usage. This region is not suggested to
> + * be used by kernel features like ramoops, suspend resume etc.
> + */
> +
> + /*
> + * Firmware related regions, bootlader will possible reserve parts of
> + * region from 0x80000000..0x8a800000.
> + */
> + aop_image_mem: aop-image-region@81c00000 {
> + reg = <0x0 0x81c00000 0x0 0x60000>;
> + no-map;
> + };
> +
> + aop_cmd_db_mem: aop-cmd-db-region@81c60000 {
> + compatible = "qcom,cmd-db";
> + reg = <0x0 0x81c60000 0x0 0x20000>;
> + no-map;
> + };
> +
> + aop_config_mem: aop-config-region@81c80000 {
> + no-map;
> + reg = <0x0 0x81c80000 0x0 0x20000>;
> + };
> +
> + smem_mem: smem-region@81d00000 {
> + compatible = "qcom,smem";
> + reg = <0x0 0x81d00000 0x0 0x200000>;
> + hwlocks = <&tcsr_mutex 3>;
> + no-map;
> + };
> +
> + adsp_mhi_mem: adsp-mhi-region@81f00000 {
> + reg = <0x0 0x81f00000 0x0 0x20000>;
> + no-map;
> + };
> +
> + /* PIL region */
> + mpss_mem: mpss-region@8a800000 {
> + reg = <0x0 0x8a800000 0x0 0x10800000>;
> + no-map;
> + };
> +
> + q6_mpss_dtb_mem: q6-mpss-dtb-region@9b000000 {
> + reg = <0x0 0x9b000000 0x0 0x80000>;
> + no-map;
> + };
> +
> + ipa_fw_mem: ipa-fw-region@9b080000 {
> + reg = <0x0 0x9b080000 0x0 0x10000>;
> + no-map;
> + };
> +
> + ipa_gsi_mem: ipa-gsi-region@9b090000 {
> + reg = <0x0 0x9b090000 0x0 0xa000>;
> + no-map;
> + };
> +
> + gpu_micro_code_mem: gpu-micro-code-region@9b09a000 {
> + reg = <0x0 0x9b09a000 0x0 0x2000>;
> + no-map;
> + };
> +
> + spss_region_mem: spss-region@9b100000 {
> + reg = <0x0 0x9b100000 0x0 0x180000>;
> + no-map;
> + };
> +
> + spu_secure_shared_memory_mem: spu-secure-shared-memory-region@9b280000 {
> + reg = <0x0 0x9b280000 0x0 0x80000>;
> + no-map;
> + };
> +
> + camera_mem: camera-region@9b300000 {
> + reg = <0x0 0x9b300000 0x0 0x800000>;
> + no-map;
> + };
> +
> + video_mem: video-region@9bb00000 {
> + reg = <0x0 0x9bb00000 0x0 0x700000>;
> + no-map;
> + };
> +
> + cvp_mem: cvp-region@9c200000 {
> + reg = <0x0 0x9c200000 0x0 0x700000>;
> + no-map;
> + };
> +
> + cdsp_mem: cdsp-region@9c900000 {
> + reg = <0x0 0x9c900000 0x0 0x2000000>;
> + no-map;
> + };
> +
> + q6_cdsp_dtb_mem: q6-cdsp-dtb-region@9e900000 {
> + reg = <0x0 0x9e900000 0x0 0x80000>;
> + no-map;
> + };
> +
> + q6_adsp_dtb_mem: q6-adsp-dtb-region@9e980000 {
> + reg = <0x0 0x9e980000 0x0 0x80000>;
> + no-map;
> + };
> +
> + adspslpi_mem: adspslpi-region@9ea00000 {
> + reg = <0x0 0x9ea00000 0x0 0x4080000>;
> + no-map;
> + };
> +
> + /*
> + * Firmware related regions, bootlader will possible reserve parts of
Nit: bootloader will possibly...
> + * region from 0xd8000000..0x100000000.
> + */
> + mpss_dsm_mem: mpss_dsm_region@d4d00000 {
> + reg = <0x0 0xd4d00000 0x0 0x3300000>;
> + no-map;
> + };
> + };
> +};
> --
> 2.25.1
>
>
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH 3/3] drm: panel: Add LG sw43408 panel driver
From: Dmitry Baryshkov @ 2024-04-01 19:11 UTC (permalink / raw)
To: Marijn Suijten
Cc: Sumit Semwal, Caleb Connolly, Neil Armstrong, Jessica Zhang,
Sam Ravnborg, David Airlie, Daniel Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, dri-devel, devicetree,
linux-kernel, linux-arm-msm, Vinod Koul, Caleb Connolly
In-Reply-To: <fn3r4ykwxvgf4ujmpevpsrcwmwzpjl5bhcp6ekyebowgf4rpz3@fyxcwjgn6abg>
On Mon, 1 Apr 2024 at 13:29, Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> On 2024-03-30 16:37:08, Dmitry Baryshkov wrote:
> > On Sat, 30 Mar 2024 at 12:27, Marijn Suijten
> > <marijn.suijten@somainline.org> wrote:
> > >
> > > On 2024-03-30 05:59:30, Dmitry Baryshkov wrote:
> > > > From: Sumit Semwal <sumit.semwal@linaro.org>
> > > >
> > > > LG SW43408 is 1080x2160, 4-lane MIPI-DSI panel, used in some Pixel3
> > > > phones.
> > > >
> > > > Whatever init sequence we have for this panel isn't capable of
> > > > initialising it completely, toggling the reset gpio ever causes the
> > > > panel to die. Until this is resolved we avoid resetting the panel. The
> > >
> > > Are you sure it is avoided? This patch seems to be toggling reset_gpio in
> > > sw43408_prepare()?
> > >
> > > > disable/unprepare functions only put the panel to sleep mode and
> > > > disable the backlight.
> > > >
> > > > Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>
> > > > [vinod: Add DSC support]
> > > > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > > > [caleb: cleanup and support turning off the panel]
> > > > Signed-off-by: Caleb Connolly <caleb@connolly.tech>
> > > > [DB: partially rewrote the driver and fixed DSC programming]
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > ---
> > > > MAINTAINERS | 8 +
> > > > drivers/gpu/drm/panel/Kconfig | 11 ++
> > > > drivers/gpu/drm/panel/Makefile | 1 +
> > > > drivers/gpu/drm/panel/panel-lg-sw43408.c | 322 +++++++++++++++++++++++++++++++
> > > > 4 files changed, 342 insertions(+)
> > > >
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index 4b511a55101c..f4cf7ee97376 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -6755,6 +6755,14 @@ S: Maintained
> > > > F: Documentation/devicetree/bindings/display/panel/jadard,jd9365da-h3.yaml
> > > > F: drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c
> > > >
> > > > +DRM DRIVER FOR LG SW43408 PANELS
> > > > +M: Sumit Semwal <sumit.semwal@linaro.org>
> > > > +M: Caleb Connolly <caleb.connolly@linaro.org>
> > > > +S: Maintained
> > > > +T: git git://anongit.freedesktop.org/drm/drm-misc
> > > > +F: Documentation/devicetree/bindings/display/panel/lg,sw43408.yaml
> > > > +F: drivers/gpu/drm/panel/panel-lg-sw43408.c
> > > > +
> > > > DRM DRIVER FOR LOGICVC DISPLAY CONTROLLER
> > > > M: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > S: Supported
> > > > diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> > > > index d037b3b8b999..f94c702735cb 100644
> > > > --- a/drivers/gpu/drm/panel/Kconfig
> > > > +++ b/drivers/gpu/drm/panel/Kconfig
> > > > @@ -335,6 +335,17 @@ config DRM_PANEL_LG_LG4573
> > > > Say Y here if you want to enable support for LG4573 RGB panel.
> > > > To compile this driver as a module, choose M here.
> > > >
> > > > +config DRM_PANEL_LG_SW43408
> > > > + tristate "LG SW43408 panel"
> > > > + depends on OF
> > > > + depends on DRM_MIPI_DSI
> > > > + depends on BACKLIGHT_CLASS_DEVICE
> > > > + help
> > > > + Say Y here if you want to enable support for LG sw43408 panel.
> > > > + The panel has a 1080x2160 resolution and uses
> > > > + 24 bit RGB per pixel. It provides a MIPI DSI interface to
> > > > + the host and has a built-in LED backlight.
> > > > +
> > > > config DRM_PANEL_MAGNACHIP_D53E6EA8966
> > > > tristate "Magnachip D53E6EA8966 DSI panel"
> > > > depends on OF && SPI
> > > > diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> > > > index f156d7fa0bcc..a75687d13caf 100644
> > > > --- a/drivers/gpu/drm/panel/Makefile
> > > > +++ b/drivers/gpu/drm/panel/Makefile
> > > > @@ -34,6 +34,7 @@ obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK050H3146W) += panel-leadtek-ltk050h3146w.o
> > > > obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK500HD1829) += panel-leadtek-ltk500hd1829.o
> > > > obj-$(CONFIG_DRM_PANEL_LG_LB035Q02) += panel-lg-lb035q02.o
> > > > obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o
> > > > +obj-$(CONFIG_DRM_PANEL_LG_SW43408) += panel-lg-sw43408.o
> > > > obj-$(CONFIG_DRM_PANEL_MAGNACHIP_D53E6EA8966) += panel-magnachip-d53e6ea8966.o
> > > > obj-$(CONFIG_DRM_PANEL_NEC_NL8048HL11) += panel-nec-nl8048hl11.o
> > > > obj-$(CONFIG_DRM_PANEL_NEWVISION_NV3051D) += panel-newvision-nv3051d.o
> > > > diff --git a/drivers/gpu/drm/panel/panel-lg-sw43408.c b/drivers/gpu/drm/panel/panel-lg-sw43408.c
> > > > new file mode 100644
> > > > index 000000000000..365d25e14d54
> > > > --- /dev/null
> > > > +++ b/drivers/gpu/drm/panel/panel-lg-sw43408.c
> > > > @@ -0,0 +1,322 @@
> > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > +/*
> > > > + * Copyright (C) 2019-2024 Linaro Ltd
> > > > + * Author: Sumit Semwal <sumit.semwal@linaro.org>
> > > > + * Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > + */
> > > > +
> > > > +#include <linux/backlight.h>
> > > > +#include <linux/delay.h>
> > > > +#include <linux/gpio/consumer.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/of.h>
> > > > +#include <linux/regulator/consumer.h>
> > > > +
> > > > +#include <video/mipi_display.h>
> > > > +
> > > > +#include <drm/drm_mipi_dsi.h>
> > > > +#include <drm/drm_panel.h>
> > > > +#include <drm/drm_probe_helper.h>
> > > > +#include <drm/display/drm_dsc.h>
> > > > +#include <drm/display/drm_dsc_helper.h>
> > > > +
> > > > +#define NUM_SUPPLIES 2
> > > > +
> > > > +struct sw43408_panel {
> > > > + struct drm_panel base;
> > > > + struct mipi_dsi_device *link;
> > > > +
> > > > + const struct drm_display_mode *mode;
> > > > +
> > > > + struct regulator_bulk_data supplies[NUM_SUPPLIES];
> > > > +
> > > > + struct gpio_desc *reset_gpio;
> > > > +};
> > > > +
> > > > +static inline struct sw43408_panel *to_panel_info(struct drm_panel *panel)
> > > > +{
> > > > + return container_of(panel, struct sw43408_panel, base);
> > > > +}
> > > > +
> > > > +static int sw43408_unprepare(struct drm_panel *panel)
> > > > +{
> > > > + struct sw43408_panel *ctx = to_panel_info(panel);
> > > > + int ret;
> > > > +
> > > > + ret = mipi_dsi_dcs_set_display_off(ctx->link);
> > > > + if (ret < 0)
> > > > + dev_err(panel->dev, "set_display_off cmd failed ret = %d\n", ret);
> > > > +
> > > > + ret = mipi_dsi_dcs_enter_sleep_mode(ctx->link);
> > > > + if (ret < 0)
> > > > + dev_err(panel->dev, "enter_sleep cmd failed ret = %d\n", ret);
> > > > +
> > > > + msleep(100);
> > > > +
> > > > + gpiod_set_value(ctx->reset_gpio, 1);
> > > > +
> > > > + return regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> > > > +}
> > > > +
> > > > +static int sw43408_program(struct drm_panel *panel)
> > > > +{
> > > > + struct sw43408_panel *ctx = to_panel_info(panel);
> > > > + struct drm_dsc_picture_parameter_set pps;
> > > > + u8 dsc_en = 0x11;
> > >
> > > Yeah, this is completely strange. Bit 0, 0x1, is to enable DSC which is
> > > normal. 0x10 however, which is bit 4, selects PPS table 2. Do you ever set
> > > pps_identifier in struct drm_dsc_picture_parameter_set to 2? Or is the table
> > > that you send below bogus and/or not used? Maybe the Driver IC on the other
> > > end of the DSI link has a default PPS table with identifier 2 that works out of
> > > the box?
> >
> > Note, MIPI standard also requires two bytes argument. I suspect that
> > LG didn't fully follow the standard here.
>
> Have you read this command from downstream DTS, or have you tried sending 2
> bytes and seen the panel breaking? The second byte is marked as reserved and
> should be equal to 0; if the Driver IC is okay with sending either 1 or 2 bytes
> I'd strive to stick with the defined length of 2 bytes for this DCS.
>
> Have you played around with the PPS table? What if you change
> drm_dsc_picture_paremeter_set::pps_identifier to the second table, will the
> panel stop working as expected again? This could indicate that the PPS that is
> sent is incorrect (even though the information in the original DSC config was
> enough to set up the DPU and DSI correctly).
>
> According to the DSI spec it is allowed to have a pre-stored/pre-programmed
> PPS table, which could be used here making the current call to
> mipi_dsi_picture_parameter_set() useless and "confusing"?
Ok, some short summary of my tests.
Skipping PPS doesn't work at all, so there is no default.
Adding a second zero byte doesn't seem to change anything. Dropping
the 0x1 bit ('enable') doesn't seem to change anything.
If I send COMPRESSION_MODE before sending the PPS, various combinations work.
If I send COMPRESSION_MODE after sending the PPS, the follow combos work:
pps_identifier = 0x0, COMPRESSION_MODE = 0x11
pps_identifier = 0x1, COMPRESSION_MODE = 0x21
>
> > Basically that's the reason why I went for the _raw function instead
> > of adding PPS and codec arguments to the existing function.
> >
> > >
> > > > + mipi_dsi_dcs_write_seq(ctx->link, MIPI_DCS_SET_GAMMA_CURVE, 0x02);
> > > > +
> > > > + mipi_dsi_dcs_set_tear_on(ctx->link, MIPI_DSI_DCS_TEAR_MODE_VBLANK);
> > > > +
> > > > + mipi_dsi_dcs_write_seq(ctx->link, 0x53, 0x0c, 0x30);
> > > > + mipi_dsi_dcs_write_seq(ctx->link, 0x55, 0x00, 0x70, 0xdf, 0x00, 0x70, 0xdf);
> > > > + mipi_dsi_dcs_write_seq(ctx->link, 0xf7, 0x01, 0x49, 0x0c);
> > > > +
> > > > + mipi_dsi_dcs_exit_sleep_mode(ctx->link);
> > > > +
> > > > + msleep(135);
> > > > +
> > > > + mipi_dsi_compression_mode_raw(ctx->link, &dsc_en, 1);
> > >
> > > Even though I think we should change this function to describe the known
> > > bit layout of command 0x7 per the VESA DSI spec, for now replace 1 with
> > > sizeof(dsc_en)?
> >
> > If dsc_en were an array, it would have been a proper thing. Maybe I
> > should change it to the array to remove confusion.
>
> It should work even with a single byte, just to clarify to readers that the 3rd
> argument is the byte-size of the input.
>
> > > > + mipi_dsi_dcs_write_seq(ctx->link, 0xb0, 0xac);
> > > > + mipi_dsi_dcs_write_seq(ctx->link, 0xe5,
> > > > + 0x00, 0x3a, 0x00, 0x3a, 0x00, 0x0e, 0x10);
> > > > + mipi_dsi_dcs_write_seq(ctx->link, 0xb5,
> > > > + 0x75, 0x60, 0x2d, 0x5d, 0x80, 0x00, 0x0a, 0x0b,
> > > > + 0x00, 0x05, 0x0b, 0x00, 0x80, 0x0d, 0x0e, 0x40,
> > > > + 0x00, 0x0c, 0x00, 0x16, 0x00, 0xb8, 0x00, 0x80,
> > > > + 0x0d, 0x0e, 0x40, 0x00, 0x0c, 0x00, 0x16, 0x00,
> > > > + 0xb8, 0x00, 0x81, 0x00, 0x03, 0x03, 0x03, 0x01,
> > > > + 0x01);
> > > > + msleep(85);
> > > > + mipi_dsi_dcs_write_seq(ctx->link, 0xcd,
> > > > + 0x00, 0x00, 0x00, 0x19, 0x19, 0x19, 0x19, 0x19,
> > > > + 0x19, 0x19, 0x19, 0x19, 0x19, 0x19, 0x19, 0x19,
> > > > + 0x16, 0x16);
> > > > + mipi_dsi_dcs_write_seq(ctx->link, 0xcb, 0x80, 0x5c, 0x07, 0x03, 0x28);
> > > > + mipi_dsi_dcs_write_seq(ctx->link, 0xc0, 0x02, 0x02, 0x0f);
> > > > + mipi_dsi_dcs_write_seq(ctx->link, 0x55, 0x04, 0x61, 0xdb, 0x04, 0x70, 0xdb);
> > > > + mipi_dsi_dcs_write_seq(ctx->link, 0xb0, 0xca);
> > > > +
> > > > + mipi_dsi_dcs_set_display_on(ctx->link);
> > >
> > > Any specific reason to not have the (un)blanking sequence in the enable/disable
> > > callbacks and leaving display configuration in (un)prepare?
> >
> > We are back to the question on when it's fine to send the commands. I
> > think the current agreement is to send everything in the
> > prepare/unprepare, because of some strange hosts.
>
> For my panel drivers I'm sticking with having `post-on` commands (from
> downstream) in `enable/disable`, which is typically only `set_display_on`. In
> hopes of proposing a `prepare_atomic()` some time to allow mode selection.
>
> In a short test on recent -next I am once again allowed to send DSI commands in
> both .disable and .unprepare, making both functions a "clean" inverse of .enable
> and .prepare respectively.
The world isn't limited to the MSM hosts.
>
> > > > + msleep(50);
> > > > +
> > > > + ctx->link->mode_flags &= ~MIPI_DSI_MODE_LPM;
> > > > +
> > > > + drm_dsc_pps_payload_pack(&pps, ctx->link->dsc);
> > > > + mipi_dsi_picture_parameter_set(ctx->link, &pps);
> > >
> > > I'm always surprised why this is sent _after_ turning the display on (unblanking
> > > it). Wouldn't that cause unnecessary corruption?
> >
> > No idea. I followed the dowsntream command sequences here. Most likely
> > the panel is not fully on until it receives the full frame to be
> > displayed.
>
> According to the DSI spec a PPS update is allowed to happen every frame, and
> (for cmdmode panels) will take effect after the next TE trigger. Unsure if a TE
> event happens before the first frame, otherwise this may start taking effect
> on the second frame onwards only.
>
> If there's no corruption on the first frame there might be a pre-programmed PPS
> table in slot 2, supporting the theory above.
--
With best wishes
Dmitry
^ permalink raw reply
* [PATCH v3] media: dt-bindings: ovti,ov2680: Document more properties
From: Fabio Estevam @ 2024-04-01 19:05 UTC (permalink / raw)
To: sakari.ailus
Cc: rmfrfs, laurent.pinchart, hansg, robh, krzysztof.kozlowski+dt,
conor+dt, linux-media, devicetree, Fabio Estevam
From: Fabio Estevam <festevam@denx.de>
OV2680 has a single data lane MIPI interface.
Document the clock-lanes and data-lanes properties to avoid
the following dt-schema warning:
imx7s-warp.dtb: camera@36: port:endpoint: Unevaluated properties are not allowed ('clock-lanes', 'data-lanes' were unexpected)
from schema $id: http://devicetree.org/schemas/media/i2c/ovti,ov2680.yaml#
While at it, also document the link-frequencies property as recommended
by the following document:
https://www.kernel.org/doc/html/v6.9-rc1/driver-api/media/camera-sensor.html#handling-clocks
Signed-off-by: Fabio Estevam <festevam@denx.de>
---
Changes since v2:
- Use additionalProperties: false (Laurent).
- Mark link-frequencies as mandatory. (Laurent).
.../bindings/media/i2c/ovti,ov2680.yaml | 25 ++++++++++++++++++-
1 file changed, 24 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov2680.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov2680.yaml
index cf456f8d9ddc..6ae7d4457536 100644
--- a/Documentation/devicetree/bindings/media/i2c/ovti,ov2680.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov2680.yaml
@@ -50,9 +50,29 @@ properties:
Definition of the regulator used as digital power supply.
port:
- $ref: /schemas/graph.yaml#/properties/port
description:
A node containing an output port node.
+ $ref: /schemas/graph.yaml#/$defs/port-base
+ additionalProperties: false
+
+ properties:
+ endpoint:
+ $ref: /schemas/media/video-interfaces.yaml#
+ additionalProperties: false
+
+ properties:
+ clock-lanes:
+ const: 0
+
+ data-lanes:
+ const: 1
+
+ link-frequencies: true
+
+ remote-endpoint: true
+
+ required:
+ - link-frequencies
required:
- compatible
@@ -89,6 +109,9 @@ examples:
port {
ov2680_to_mipi: endpoint {
remote-endpoint = <&mipi_from_sensor>;
+ clock-lanes = <0>;
+ data-lanes = <1>;
+ link-frequencies = /bits/ 64 <330000000>;
};
};
};
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v2 0/7] arm64: dts: qcom: fix description of the Type-C signals
From: Dmitry Baryshkov @ 2024-04-01 18:48 UTC (permalink / raw)
To: Rob Herring
Cc: Krzysztof Kozlowski, Luca Weiss, Conor Dooley, Bjorn Andersson,
Bryan O'Donoghue, linux-arm-msm, devicetree, linux-kernel,
Konrad Dybcio
In-Reply-To: <171198916314.1093638.15006189720750656914.robh@kernel.org>
On Mon, 1 Apr 2024 at 19:36, Rob Herring <robh@kernel.org> wrote:
>
>
> On Sun, 31 Mar 2024 06:48:50 +0300, Dmitry Baryshkov wrote:
> > Rename the HS link between usb-c-connector and the DWC3 USB controller.
> > Add missing graph connection between the QMP PHY and DWC3 USB
> > controller.
> >
> > Reported-by: Luca Weiss <luca.weiss@fairphone.com>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> > Changes in v2:
> > - Fixed other platforms in addition to sm8250 (Bryan)
> > - Link to v1: https://lore.kernel.org/r/20240322-typec-fix-sm8250-v1-0-1ac22b333ea9@linaro.org
> >
> > ---
> > Dmitry Baryshkov (7):
> > arm64: dts: qcom: sm8250: describe HS signals properly
> > arm64: dts: qcom: sm8250: add a link between DWC3 and QMP PHY
> > arm64: dts: qcom: sc8180x: switch USB+DP QMP PHYs to new bindings
> > arm64: dts: qcom: sc8180x: describe USB signals properly
> > arm64: dts: qcom: sc8280xp: describe USB signals properly
> > arm64: dts: qcom: x1e80100: describe USB signals properly
> > arm64: dts: qcom: sm8150-hdk: rename Type-C HS endpoints
> >
> > arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 8 +-
> > .../arm64/boot/dts/qcom/sc8180x-lenovo-flex-5g.dts | 16 +-
> > arch/arm64/boot/dts/qcom/sc8180x-primus.dts | 20 +--
> > arch/arm64/boot/dts/qcom/sc8180x.dtsi | 164 ++++++++++-----------
> > arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 20 +--
> > .../dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 20 +--
> > arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 54 ++++++-
> > arch/arm64/boot/dts/qcom/sm8150-hdk.dts | 4 +-
> > .../boot/dts/qcom/sm8250-xiaomi-elish-common.dtsi | 8 +-
> > arch/arm64/boot/dts/qcom/sm8250.dtsi | 24 ++-
> > arch/arm64/boot/dts/qcom/x1e80100.dtsi | 149 ++++++++++++++++++-
> > 11 files changed, 340 insertions(+), 147 deletions(-)
> > ---
> > base-commit: 13ee4a7161b6fd938aef6688ff43b163f6d83e37
> > change-id: 20240322-typec-fix-sm8250-33c47a03a056
> >
> > Best regards,
> > --
> > Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >
> >
> >
>
>
> My bot found new DTB warnings on the .dts files added or changed in this
> series.
>
> Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
> are fixed by another series. Ultimately, it is up to the platform
> maintainer whether these warnings are acceptable or not. No need to reply
> unless the platform maintainer has comments.
>
> If you already ran DT checks and didn't see these error(s), then
> make sure dt-schema is up to date:
>
> pip3 install dtschema --upgrade
>
>
> New warnings running 'make CHECK_DTBS=y qcom/qrb5165-rb5.dtb qcom/sc8180x-lenovo-flex-5g.dtb qcom/sc8180x-primus.dtb qcom/sc8280xp-crd.dtb qcom/sc8280xp-lenovo-thinkpad-x13s.dtb qcom/sm8150-hdk.dtb' for 20240331-typec-fix-sm8250-v2-0-857acb6bd88e@linaro.org:
>
> arch/arm64/boot/dts/qcom/sc8180x-lenovo-flex-5g.dtb: clock-controller@af00000: clocks: [[41, 0], [42], [95, 1], [95, 2], [99, 1], [99, 2], [125, 0], [125, 1]] is too long
> from schema $id: http://devicetree.org/schemas/clock/qcom,dispcc-sm8x50.yaml#
> arch/arm64/boot/dts/qcom/sc8180x-primus.dtb: clock-controller@af00000: clocks: [[41, 0], [42], [97, 1], [97, 2], [101, 1], [101, 2], [127, 0], [127, 1]] is too long
> from schema $id: http://devicetree.org/schemas/clock/qcom,dispcc-sm8x50.yaml#
I don' t think it's new, it just had PHY indices changed. But let's
fix it anyway.
>
>
>
>
>
--
With best wishes
Dmitry
^ permalink raw reply
* [PATCH 0/2] phy: qcom-qmp-ufs: Fix PHY QMP clocks for SC7180
From: Danila Tikhonov @ 2024-04-01 18:22 UTC (permalink / raw)
To: andersson, konrad.dybcio, vkoul, kishon, robh,
krzysztof.kozlowski+dt, conor+dt, cros-qcom-dts-watchers,
manivannan.sadhasivam, davidwronek
Cc: linux-arm-msm, linux-phy, devicetree, linux-kernel,
Danila Tikhonov
This series of patches is based on the series from Manivannan:
https://lore.kernel.org/all/20240131-ufs-phy-clock-v3-0-58a49d2f4605@linaro.org/
Patch from David adding a UFS nodes for SC7180(SM7125):
https://lore.kernel.org/all/20240121-sm7125-upstream-v4-6-f7d1212c8ebb@gmail.com/
The patch submitted by David and a series of patches submitted by Manivannan
were both applied at approximately the same time. As a result, David's patch
did not include this change.
To: Bjorn Andersson <andersson@kernel.org>
To: Konrad Dybcio <konrad.dybcio@linaro.org>
To: Vinod Koul <vkoul@kernel.org>
To: Kishon Vijay Abraham I <kishon@kernel.org>
To: Rob Herring <robh@kernel.org>
To: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
To: Conor Dooley <conor+dt@kernel.org>
To: cros-qcom-dts-watchers@chromium.org
To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To: David Wronek <davidwronek@gmail.com>
Cc: linux-arm-msm@vger.kernel.org
Cc: linux-phy@lists.infradead.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Danila Tikhonov <danila@jiaxyga.com>
Danila Tikhonov (2):
dt-bindings: phy: qmp-ufs: Fix PHY clocks for SC7180
arm64: dts: qcom: sc7180: Fix UFS PHY clocks
.../bindings/phy/qcom,sc8280xp-qmp-ufs-phy.yaml | 1 +
arch/arm64/boot/dts/qcom/sc7180.dtsi | 9 ++++++---
2 files changed, 7 insertions(+), 3 deletions(-)
--
2.44.0
^ permalink raw reply
* [PATCH 2/2] arm64: dts: qcom: sc7180: Fix UFS PHY clocks
From: Danila Tikhonov @ 2024-04-01 18:22 UTC (permalink / raw)
To: andersson, konrad.dybcio, vkoul, kishon, robh,
krzysztof.kozlowski+dt, conor+dt, cros-qcom-dts-watchers,
manivannan.sadhasivam, davidwronek
Cc: linux-arm-msm, linux-phy, devicetree, linux-kernel,
Danila Tikhonov
In-Reply-To: <20240401182240.55282-1-danila@jiaxyga.com>
QMP PHY used in SC7180 requires 3 clocks:
* ref - 19.2MHz reference clock from RPMh
* ref_aux - Auxiliary reference clock from GCC
* qref - QREF clock from GCC
While at it, let's move 'clocks' property before 'clock-names' to match
the style used commonly.
Signed-off-by: Danila Tikhonov <danila@jiaxyga.com>
---
arch/arm64/boot/dts/qcom/sc7180.dtsi | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
index 2b481e20ae38..5c9ec8047f00 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -1585,9 +1585,12 @@ ufs_mem_phy: phy@1d87000 {
compatible = "qcom,sc7180-qmp-ufs-phy",
"qcom,sm7150-qmp-ufs-phy";
reg = <0 0x01d87000 0 0x1000>;
- clocks = <&gcc GCC_UFS_MEM_CLKREF_CLK>,
- <&gcc GCC_UFS_PHY_PHY_AUX_CLK>;
- clock-names = "ref", "ref_aux";
+ clocks = <&rpmhcc RPMH_CXO_CLK>,
+ <&gcc GCC_UFS_PHY_PHY_AUX_CLK>,
+ <&gcc GCC_UFS_MEM_CLKREF_CLK>;
+ clock-names = "ref",
+ "ref_aux",
+ "qref";
power-domains = <&gcc UFS_PHY_GDSC>;
resets = <&ufs_mem_hc 0>;
reset-names = "ufsphy";
--
2.44.0
^ permalink raw reply related
* [PATCH 1/2] dt-bindings: phy: qmp-ufs: Fix PHY clocks for SC7180
From: Danila Tikhonov @ 2024-04-01 18:22 UTC (permalink / raw)
To: andersson, konrad.dybcio, vkoul, kishon, robh,
krzysztof.kozlowski+dt, conor+dt, cros-qcom-dts-watchers,
manivannan.sadhasivam, davidwronek
Cc: linux-arm-msm, linux-phy, devicetree, linux-kernel,
Danila Tikhonov
In-Reply-To: <20240401182240.55282-1-danila@jiaxyga.com>
QMP UFS PHY used in SC7180 requires 3 clocks:
* ref - 19.2MHz reference clock from RPMh
* ref_aux - Auxiliary reference clock from GCC
* qref - QREF clock from GCC
This change obviously breaks the ABI, but it is inevitable since the
clock topology needs to be accurately described in the binding.
Signed-off-by: Danila Tikhonov <danila@jiaxyga.com>
---
.../devicetree/bindings/phy/qcom,sc8280xp-qmp-ufs-phy.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-ufs-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-ufs-phy.yaml
index 91a6cc38ff7f..a79fde9a8cdf 100644
--- a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-ufs-phy.yaml
+++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-ufs-phy.yaml
@@ -86,6 +86,7 @@ allOf:
enum:
- qcom,msm8998-qmp-ufs-phy
- qcom,sa8775p-qmp-ufs-phy
+ - qcom,sc7180-qmp-ufs-phy
- qcom,sc7280-qmp-ufs-phy
- qcom,sc8180x-qmp-ufs-phy
- qcom,sc8280xp-qmp-ufs-phy
--
2.44.0
^ permalink raw reply related
* Re: [PATCH v2 27/27] kselftest/riscv: kselftest for user mode cfi
From: Deepak Gupta @ 2024-04-01 17:55 UTC (permalink / raw)
To: Muhammad Usama Anjum
Cc: paul.walmsley, rick.p.edgecombe, broonie, Szabolcs.Nagy,
kito.cheng, keescook, ajones, conor.dooley, cleger, atishp, alex,
bjorn, alexghiti, samuel.holland, conor, linux-doc, linux-riscv,
linux-kernel, devicetree, linux-mm, linux-arch, linux-kselftest,
corbet, tech-j-ext, palmer, aou, robh+dt, krzysztof.kozlowski+dt,
oleg, akpm, arnd, ebiederm, Liam.Howlett, vbabka, lstoakes, shuah,
brauner, andy.chiu, jerry.shih, hankuan.chen, greentime.hu, evan,
xiao.w.wang, charlie, apatel, mchitale, dbarboza, sameo,
shikemeng, willy, vincent.chen, guoren, samitolvanen,
songshuaishuai, gerg, heiko, bhe, jeeheng.sia, cyy, maskray,
ancientmodern4, mathis.salmen, cuiyunhui, bgray, mpe, baruch, alx,
david, catalin.marinas, revest, josh, shr, deller, omosnace,
ojeda, jhubbard
In-Reply-To: <CAKC1njQj7GfkdE1HJD54utkoPqJXyqMeoXOxa6ActqZ-fSDuKQ@mail.gmail.com>
On Mon, Apr 1, 2024 at 10:34 AM Deepak Gupta <debug@rivosinc.com> wrote:
>
> On Mon, Apr 1, 2024 at 2:48 AM Muhammad Usama Anjum
> <usama.anjum@collabora.com> wrote:
> >
> > >>> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
> > >>> ---
> > >>> tools/testing/selftests/riscv/Makefile | 2 +-
> > >>> tools/testing/selftests/riscv/cfi/Makefile | 10 +
> > >>> .../testing/selftests/riscv/cfi/cfi_rv_test.h | 85 ++++
> > >>> .../selftests/riscv/cfi/riscv_cfi_test.c | 91 +++++
> > >>> .../testing/selftests/riscv/cfi/shadowstack.c | 376 ++++++++++++++++++
> > >>> .../testing/selftests/riscv/cfi/shadowstack.h | 39 ++
> > >> Please add generated binaries in the .gitignore files.
> > >
> > > hmm...
> > > I don't see binary as part of the patch. Which file are you referring
> > > to here being binary?
> > shadowstack would be generated by the build. Create a .gitignore file and
> > add it there. For example, look at
> > tools/testing/selftests/riscv/vector/.gitignore to understand.
>
> It's `shadowstack.c` (a C source file) and not a binary file.
Nevermind. I think what you want me to do is add a rule in `.gitignore`.
I was thinking otherwise (that somehow you're seeing a binary file in
patch set).
Thanks. Will do that in the next iteration.
^ permalink raw reply
* Re: [PATCH] ASoC: dt-bindings: mt2701-wm8960: Convert to dtschema
From: Kartik Agarwala @ 2024-04-01 17:38 UTC (permalink / raw)
To: Rob Herring
Cc: lgirdwood, broonie, krzysztof.kozlowski+dt, conor+dt,
matthias.bgg, angelogioacchino.delregno, linux-sound, devicetree,
linux-kernel, linux-arm-kernel, linux-mediatek
In-Reply-To: <20240401151414.GA706943-robh@kernel.org>
On 4/1/24 8:44 PM, Rob Herring wrote:
> On Mon, Apr 01, 2024 at 10:05:05AM +0530, Kartik Agarwala wrote:
>> + A list of the connections between audio components. Each entry is a
>> + pair of strings, the first being the connection's sink, the second
>> + being the connection's source.
>> +
>> + mediatek,audio-codec:
>> + $ref: /schemas/types.yaml#/definitions/phandle
>> + description: The phandle of the WM8960 audio codec.
>> +
>> + pinctrl-names:
>> + const: default
>> +
>> + pinctrl-0: true
>
> You can drop pinctrl properties. Those are implicitly supported.
Hi,
Thanks for the review!
Just to clarify, the removal of pinctrl properties should only apply
to this section and not to the required properties or the example,
is that correct?
Regards,
Kartik Agarwala
^ 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