* [PATCH v4 6/6] iio: common: scmi_iio: convert to dev_err_probe()
From: Nuno Sa via B4 Relay @ 2024-03-28 16:22 UTC (permalink / raw)
To: linux-kernel, linux-iio, devicetree
Cc: Nuno Sá, Jonathan Cameron, Lars-Peter Clausen,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Olivier Moysan, Jyoti Bhayana
In-Reply-To: <20240328-ltc2983-misc-improv-v4-0-0cc428c07cd5@analog.com>
From: Nuno Sa <nuno.sa@analog.com>
Make use of dev_err_probe() and dev_errp_probe() to simplify error paths
during probe.
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
drivers/iio/common/scmi_sensors/scmi_iio.c | 45 +++++++++++++-----------------
1 file changed, 19 insertions(+), 26 deletions(-)
diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c
index 0c2caf3570db..30d58af02b4c 100644
--- a/drivers/iio/common/scmi_sensors/scmi_iio.c
+++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
@@ -626,12 +626,10 @@ scmi_alloc_iiodev(struct scmi_device *sdev,
SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE,
&sensor->sensor_info->id,
&sensor->sensor_update_nb);
- if (ret) {
- dev_err(&iiodev->dev,
- "Error in registering sensor update notifier for sensor %s err %d",
- sensor->sensor_info->name, ret);
- return ERR_PTR(ret);
- }
+ if (ret)
+ return dev_errp_probe(&iiodev->dev, ret,
+ "Error in registering sensor update notifier for sensor %s err %d",
+ sensor->sensor_info->name, ret);
scmi_iio_set_timestamp_channel(&iio_channels[i], i);
iiodev->channels = iio_channels;
@@ -653,10 +651,9 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev)
return -ENODEV;
sensor_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_SENSOR, &ph);
- if (IS_ERR(sensor_ops)) {
- dev_err(dev, "SCMI device has no sensor interface\n");
- return PTR_ERR(sensor_ops);
- }
+ if (IS_ERR(sensor_ops))
+ return dev_err_probe(dev, PTR_ERR(sensor_ops),
+ "SCMI device has no sensor interface\n");
nr_sensors = sensor_ops->count_get(ph);
if (!nr_sensors) {
@@ -667,8 +664,8 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev)
for (i = 0; i < nr_sensors; i++) {
sensor_info = sensor_ops->info_get(ph, i);
if (!sensor_info) {
- dev_err(dev, "SCMI sensor %d has missing info\n", i);
- return -EINVAL;
+ return dev_err_probe(dev, -EINVAL,
+ "SCMI sensor %d has missing info\n", i);
}
/* This driver only supports 3-axis accel and gyro, skipping other sensors */
@@ -683,29 +680,25 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev)
scmi_iio_dev = scmi_alloc_iiodev(sdev, sensor_ops, ph,
sensor_info);
if (IS_ERR(scmi_iio_dev)) {
- dev_err(dev,
- "failed to allocate IIO device for sensor %s: %ld\n",
- sensor_info->name, PTR_ERR(scmi_iio_dev));
- return PTR_ERR(scmi_iio_dev);
+ return dev_err_probe(dev, PTR_ERR(scmi_iio_dev),
+ "failed to allocate IIO device for sensor %s: %ld\n",
+ sensor_info->name, PTR_ERR(scmi_iio_dev));
}
err = devm_iio_kfifo_buffer_setup(&scmi_iio_dev->dev,
scmi_iio_dev,
&scmi_iio_buffer_ops);
if (err < 0) {
- dev_err(dev,
- "IIO buffer setup error at sensor %s: %d\n",
- sensor_info->name, err);
- return err;
+ return dev_err_probe(dev, err,
+ "IIO buffer setup error at sensor %s: %d\n",
+ sensor_info->name, err);
}
err = devm_iio_device_register(dev, scmi_iio_dev);
- if (err) {
- dev_err(dev,
- "IIO device registration failed at sensor %s: %d\n",
- sensor_info->name, err);
- return err;
- }
+ if (err)
+ return dev_err_probe(dev, err,
+ "IIO device registration failed at sensor %s: %d\n",
+ sensor_info->name, err);
}
return err;
}
--
2.44.0
^ permalink raw reply related
* [PATCH v4 4/6] iio: temperature: ltc2983: support vdd regulator
From: Nuno Sa via B4 Relay @ 2024-03-28 16:22 UTC (permalink / raw)
To: linux-kernel, linux-iio, devicetree
Cc: Nuno Sá, Jonathan Cameron, Lars-Peter Clausen,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Olivier Moysan, Jyoti Bhayana
In-Reply-To: <20240328-ltc2983-misc-improv-v4-0-0cc428c07cd5@analog.com>
From: Nuno Sa <nuno.sa@analog.com>
Add support for the power supply regulator.
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
drivers/iio/temperature/ltc2983.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c
index b4a8ca36458a..ff7f0829b575 100644
--- a/drivers/iio/temperature/ltc2983.c
+++ b/drivers/iio/temperature/ltc2983.c
@@ -18,6 +18,7 @@
#include <linux/module.h>
#include <linux/property.h>
#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
#include <linux/spi/spi.h>
#include <asm/byteorder.h>
@@ -1574,6 +1575,10 @@ static int ltc2983_probe(struct spi_device *spi)
if (ret)
return ret;
+ ret = devm_regulator_get_enable(&spi->dev, "vdd");
+ if (ret)
+ return ret;
+
gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
if (IS_ERR(gpio))
return PTR_ERR(gpio);
--
2.44.0
^ permalink raw reply related
* [PATCH v4 5/6] iio: backend: make use dev_errp_probe()
From: Nuno Sa via B4 Relay @ 2024-03-28 16:22 UTC (permalink / raw)
To: linux-kernel, linux-iio, devicetree
Cc: Nuno Sá, Jonathan Cameron, Lars-Peter Clausen,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Olivier Moysan, Jyoti Bhayana
In-Reply-To: <20240328-ltc2983-misc-improv-v4-0-0cc428c07cd5@analog.com>
From: Nuno Sa <nuno.sa@analog.com>
Using dev_errp_probe() to simplify the code.
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
drivers/iio/industrialio-backend.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c
index 2fea2bbbe47f..e0b08283d667 100644
--- a/drivers/iio/industrialio-backend.c
+++ b/drivers/iio/industrialio-backend.c
@@ -296,11 +296,9 @@ struct iio_backend *devm_iio_backend_get(struct device *dev, const char *name)
}
fwnode = fwnode_find_reference(dev_fwnode(dev), "io-backends", index);
- if (IS_ERR(fwnode)) {
- dev_err_probe(dev, PTR_ERR(fwnode),
- "Cannot get Firmware reference\n");
- return ERR_CAST(fwnode);
- }
+ if (IS_ERR(fwnode))
+ return dev_errp_probe(dev, PTR_ERR(fwnode),
+ "Cannot get Firmware reference\n");
guard(mutex)(&iio_back_lock);
list_for_each_entry(back, &iio_back_list, entry) {
--
2.44.0
^ permalink raw reply related
* [PATCH v4 3/6] dt-bindings: iio: temperature: ltc2983: document power supply
From: Nuno Sa via B4 Relay @ 2024-03-28 16:22 UTC (permalink / raw)
To: linux-kernel, linux-iio, devicetree
Cc: Nuno Sá, Jonathan Cameron, Lars-Peter Clausen,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Olivier Moysan, Jyoti Bhayana, Krzysztof Kozlowski
In-Reply-To: <20240328-ltc2983-misc-improv-v4-0-0cc428c07cd5@analog.com>
From: Nuno Sa <nuno.sa@analog.com>
Add a property for the VDD power supply regulator.
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
index dbb85135fd66..312febeeb3bb 100644
--- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
+++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
@@ -57,6 +57,8 @@ properties:
interrupts:
maxItems: 1
+ vdd-supply: true
+
adi,mux-delay-config-us:
description: |
Extra delay prior to each conversion, in addition to the internal 1ms
@@ -460,6 +462,7 @@ required:
- compatible
- reg
- interrupts
+ - vdd-supply
additionalProperties: false
@@ -489,6 +492,7 @@ examples:
#address-cells = <1>;
#size-cells = <0>;
+ vdd-supply = <&supply>;
interrupts = <20 IRQ_TYPE_EDGE_RISING>;
interrupt-parent = <&gpio>;
--
2.44.0
^ permalink raw reply related
* [PATCH v4 2/6] iio: temperature: ltc2983: convert to dev_err_probe()
From: Nuno Sa via B4 Relay @ 2024-03-28 16:22 UTC (permalink / raw)
To: linux-kernel, linux-iio, devicetree
Cc: Nuno Sá, Jonathan Cameron, Lars-Peter Clausen,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Olivier Moysan, Jyoti Bhayana
In-Reply-To: <20240328-ltc2983-misc-improv-v4-0-0cc428c07cd5@analog.com>
From: Nuno Sa <nuno.sa@analog.com>
Use dev_err_probe() in the probe() path. While at it, made some simple
improvements:
* Declare a struct device *dev helper. This also makes the style more
consistent (some places the helper was used and not in other places);
* Explicitly included the err.h and errno.h headers;
* Removed an useless else if();
* Removed some unnecessary line breaks.
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
drivers/iio/temperature/ltc2983.c | 255 +++++++++++++++++---------------------
1 file changed, 115 insertions(+), 140 deletions(-)
diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c
index 3c4524d57b8e..b4a8ca36458a 100644
--- a/drivers/iio/temperature/ltc2983.c
+++ b/drivers/iio/temperature/ltc2983.c
@@ -8,6 +8,8 @@
#include <linux/bitfield.h>
#include <linux/completion.h>
#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/errno.h>
#include <linux/kernel.h>
#include <linux/iio/iio.h>
#include <linux/interrupt.h>
@@ -657,10 +659,11 @@ ltc2983_thermocouple_new(const struct fwnode_handle *child, struct ltc2983_data
const struct ltc2983_sensor *sensor)
{
struct ltc2983_thermocouple *thermo;
+ struct device *dev = &st->spi->dev;
u32 oc_current;
int ret;
- thermo = devm_kzalloc(&st->spi->dev, sizeof(*thermo), GFP_KERNEL);
+ thermo = devm_kzalloc(dev, sizeof(*thermo), GFP_KERNEL);
if (!thermo)
return ERR_PTR(-ENOMEM);
@@ -687,21 +690,19 @@ ltc2983_thermocouple_new(const struct fwnode_handle *child, struct ltc2983_data
LTC2983_THERMOCOUPLE_OC_CURR(3);
break;
default:
- dev_err(&st->spi->dev,
- "Invalid open circuit current:%u", oc_current);
- return ERR_PTR(-EINVAL);
+ return dev_errp_probe(dev, -EINVAL,
+ "Invalid open circuit current:%u",
+ oc_current);
}
thermo->sensor_config |= LTC2983_THERMOCOUPLE_OC_CHECK(1);
}
/* validate channel index */
if (!(thermo->sensor_config & LTC2983_THERMOCOUPLE_DIFF_MASK) &&
- sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
- dev_err(&st->spi->dev,
- "Invalid chann:%d for differential thermocouple",
- sensor->chan);
- return ERR_PTR(-EINVAL);
- }
+ sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN)
+ return dev_errp_probe(dev, -EINVAL,
+ "Invalid chann:%d for differential thermocouple",
+ sensor->chan);
struct fwnode_handle *ref __free(fwnode_handle) =
fwnode_find_reference(child, "adi,cold-junction-handle", 0);
@@ -709,14 +710,13 @@ ltc2983_thermocouple_new(const struct fwnode_handle *child, struct ltc2983_data
ref = NULL;
} else {
ret = fwnode_property_read_u32(ref, "reg", &thermo->cold_junction_chan);
- if (ret) {
+ if (ret)
/*
* This would be catched later but we can just return
* the error right away.
*/
- dev_err(&st->spi->dev, "Property reg must be given\n");
- return ERR_PTR(ret);
- }
+ return dev_errp_probe(dev, ret,
+ "Property reg must be given\n");
}
/* check custom sensor */
@@ -752,16 +752,14 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct ltc2983_data *st,
struct fwnode_handle *ref __free(fwnode_handle) =
fwnode_find_reference(child, "adi,rsense-handle", 0);
- if (IS_ERR(ref)) {
- dev_err(dev, "Property adi,rsense-handle missing or invalid");
- return ERR_CAST(ref);
- }
+ if (IS_ERR(ref))
+ return dev_errp_probe(dev, PTR_ERR(ref),
+ "Property adi,rsense-handle missing or invalid");
ret = fwnode_property_read_u32(ref, "reg", &rtd->r_sense_chan);
- if (ret) {
- dev_err(dev, "Property reg must be given\n");
- return ERR_PTR(ret);
- }
+ if (ret)
+ return dev_errp_probe(dev, ret,
+ "Property reg must be given\n");
ret = fwnode_property_read_u32(child, "adi,number-of-wires", &n_wires);
if (!ret) {
@@ -780,19 +778,19 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct ltc2983_data *st,
rtd->sensor_config = LTC2983_RTD_N_WIRES(3);
break;
default:
- dev_err(dev, "Invalid number of wires:%u\n", n_wires);
- return ERR_PTR(-EINVAL);
+ return dev_errp_probe(dev, -EINVAL,
+ "Invalid number of wires:%u\n",
+ n_wires);
}
}
if (fwnode_property_read_bool(child, "adi,rsense-share")) {
/* Current rotation is only available with rsense sharing */
if (fwnode_property_read_bool(child, "adi,current-rotate")) {
- if (n_wires == 2 || n_wires == 3) {
- dev_err(dev,
- "Rotation not allowed for 2/3 Wire RTDs");
- return ERR_PTR(-EINVAL);
- }
+ if (n_wires == 2 || n_wires == 3)
+ return dev_errp_probe(dev, -EINVAL,
+ "Rotation not allowed for 2/3 Wire RTDs");
+
rtd->sensor_config |= LTC2983_RTD_C_ROTATE(1);
} else {
rtd->sensor_config |= LTC2983_RTD_R_SHARE(1);
@@ -815,29 +813,22 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct ltc2983_data *st,
if (((rtd->sensor_config & LTC2983_RTD_KELVIN_R_SENSE_MASK)
== LTC2983_RTD_KELVIN_R_SENSE_MASK) &&
- (rtd->r_sense_chan <= min)) {
+ (rtd->r_sense_chan <= min))
/* kelvin rsense*/
- dev_err(dev,
- "Invalid rsense chann:%d to use in kelvin rsense",
- rtd->r_sense_chan);
+ return dev_errp_probe(dev, -EINVAL,
+ "Invalid rsense chann:%d to use in kelvin rsense",
+ rtd->r_sense_chan);
- return ERR_PTR(-EINVAL);
- }
-
- if (sensor->chan < min || sensor->chan > max) {
- dev_err(dev, "Invalid chann:%d for the rtd config",
- sensor->chan);
-
- return ERR_PTR(-EINVAL);
- }
+ if (sensor->chan < min || sensor->chan > max)
+ return dev_errp_probe(dev, -EINVAL,
+ "Invalid chann:%d for the rtd config",
+ sensor->chan);
} else {
/* same as differential case */
- if (sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
- dev_err(&st->spi->dev,
- "Invalid chann:%d for RTD", sensor->chan);
-
- return ERR_PTR(-EINVAL);
- }
+ if (sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN)
+ return dev_errp_probe(dev, -EINVAL,
+ "Invalid chann:%d for RTD",
+ sensor->chan);
}
/* check custom sensor */
@@ -885,10 +876,9 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct ltc2983_data *st,
rtd->excitation_current = 0x08;
break;
default:
- dev_err(&st->spi->dev,
- "Invalid value for excitation current(%u)",
- excitation_current);
- return ERR_PTR(-EINVAL);
+ return dev_errp_probe(dev, -EINVAL,
+ "Invalid value for excitation current(%u)",
+ excitation_current);
}
}
@@ -912,16 +902,14 @@ ltc2983_thermistor_new(const struct fwnode_handle *child, struct ltc2983_data *s
struct fwnode_handle *ref __free(fwnode_handle) =
fwnode_find_reference(child, "adi,rsense-handle", 0);
- if (IS_ERR(ref)) {
- dev_err(dev, "Property adi,rsense-handle missing or invalid");
- return ERR_CAST(ref);
- }
+ if (IS_ERR(ref))
+ return dev_errp_probe(dev, PTR_ERR(ref),
+ "Property adi,rsense-handle missing or invalid");
ret = fwnode_property_read_u32(ref, "reg", &thermistor->r_sense_chan);
- if (ret) {
- dev_err(dev, "rsense channel must be configured...\n");
- return ERR_PTR(ret);
- }
+ if (ret)
+ return dev_errp_probe(dev, ret,
+ "rsense channel must be configured...\n");
if (fwnode_property_read_bool(child, "adi,single-ended")) {
thermistor->sensor_config = LTC2983_THERMISTOR_SGL(1);
@@ -936,12 +924,10 @@ ltc2983_thermistor_new(const struct fwnode_handle *child, struct ltc2983_data *s
}
/* validate channel index */
if (!(thermistor->sensor_config & LTC2983_THERMISTOR_DIFF_MASK) &&
- sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
- dev_err(&st->spi->dev,
- "Invalid chann:%d for differential thermistor",
- sensor->chan);
- return ERR_PTR(-EINVAL);
- }
+ sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN)
+ return dev_errp_probe(dev, -EINVAL,
+ "Invalid chann:%d for differential thermistor",
+ sensor->chan);
/* check custom sensor */
if (sensor->type >= LTC2983_SENSOR_THERMISTOR_STEINHART) {
@@ -980,12 +966,10 @@ ltc2983_thermistor_new(const struct fwnode_handle *child, struct ltc2983_data *s
switch (excitation_current) {
case 0:
/* auto range */
- if (sensor->type >=
- LTC2983_SENSOR_THERMISTOR_STEINHART) {
- dev_err(&st->spi->dev,
- "Auto Range not allowed for custom sensors\n");
- return ERR_PTR(-EINVAL);
- }
+ if (sensor->type >= LTC2983_SENSOR_THERMISTOR_STEINHART)
+ return dev_errp_probe(dev, -EINVAL,
+ "Auto Range not allowed for custom sensors\n");
+
thermistor->excitation_current = 0x0c;
break;
case 250:
@@ -1022,10 +1006,9 @@ ltc2983_thermistor_new(const struct fwnode_handle *child, struct ltc2983_data *s
thermistor->excitation_current = 0x0b;
break;
default:
- dev_err(&st->spi->dev,
- "Invalid value for excitation current(%u)",
- excitation_current);
- return ERR_PTR(-EINVAL);
+ return dev_errp_probe(dev, -EINVAL,
+ "Invalid value for excitation current(%u)",
+ excitation_current);
}
}
@@ -1036,11 +1019,12 @@ static struct ltc2983_sensor *
ltc2983_diode_new(const struct fwnode_handle *child, const struct ltc2983_data *st,
const struct ltc2983_sensor *sensor)
{
+ struct device *dev = &st->spi->dev;
struct ltc2983_diode *diode;
u32 temp = 0, excitation_current = 0;
int ret;
- diode = devm_kzalloc(&st->spi->dev, sizeof(*diode), GFP_KERNEL);
+ diode = devm_kzalloc(dev, sizeof(*diode), GFP_KERNEL);
if (!diode)
return ERR_PTR(-ENOMEM);
@@ -1055,12 +1039,11 @@ ltc2983_diode_new(const struct fwnode_handle *child, const struct ltc2983_data *
/* validate channel index */
if (!(diode->sensor_config & LTC2983_DIODE_DIFF_MASK) &&
- sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
- dev_err(&st->spi->dev,
- "Invalid chann:%d for differential thermistor",
- sensor->chan);
- return ERR_PTR(-EINVAL);
- }
+ sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN)
+ return dev_errp_probe(dev, -EINVAL,
+ "Invalid chann:%d for differential thermistor",
+ sensor->chan);
+
/* set common parameters */
diode->sensor.fault_handler = ltc2983_common_fault_handler;
diode->sensor.assign_chan = ltc2983_diode_assign_chan;
@@ -1082,10 +1065,9 @@ ltc2983_diode_new(const struct fwnode_handle *child, const struct ltc2983_data *
diode->excitation_current = 0x03;
break;
default:
- dev_err(&st->spi->dev,
- "Invalid value for excitation current(%u)",
- excitation_current);
- return ERR_PTR(-EINVAL);
+ return dev_errp_probe(dev, -EINVAL,
+ "Invalid value for excitation current(%u)",
+ excitation_current);
}
}
@@ -1101,26 +1083,26 @@ static struct ltc2983_sensor *ltc2983_r_sense_new(struct fwnode_handle *child,
struct ltc2983_data *st,
const struct ltc2983_sensor *sensor)
{
+ struct device *dev = &st->spi->dev;
struct ltc2983_rsense *rsense;
int ret;
u32 temp;
- rsense = devm_kzalloc(&st->spi->dev, sizeof(*rsense), GFP_KERNEL);
+ rsense = devm_kzalloc(dev, sizeof(*rsense), GFP_KERNEL);
if (!rsense)
return ERR_PTR(-ENOMEM);
/* validate channel index */
- if (sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
- dev_err(&st->spi->dev, "Invalid chann:%d for r_sense",
- sensor->chan);
- return ERR_PTR(-EINVAL);
- }
+ if (sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN)
+ return dev_errp_probe(dev, -EINVAL,
+ "Invalid chann:%d for r_sense",
+ sensor->chan);
ret = fwnode_property_read_u32(child, "adi,rsense-val-milli-ohms", &temp);
- if (ret) {
- dev_err(&st->spi->dev, "Property adi,rsense-val-milli-ohms missing\n");
- return ERR_PTR(-EINVAL);
- }
+ if (ret)
+ return dev_errp_probe(dev, -EINVAL,
+ "Property adi,rsense-val-milli-ohms missing\n");
+
/*
* Times 1000 because we have milli-ohms and __convert_to_raw
* expects scales of 1000000 which are used for all other
@@ -1139,21 +1121,21 @@ static struct ltc2983_sensor *ltc2983_adc_new(struct fwnode_handle *child,
struct ltc2983_data *st,
const struct ltc2983_sensor *sensor)
{
+ struct device *dev = &st->spi->dev;
struct ltc2983_adc *adc;
- adc = devm_kzalloc(&st->spi->dev, sizeof(*adc), GFP_KERNEL);
+ adc = devm_kzalloc(dev, sizeof(*adc), GFP_KERNEL);
if (!adc)
return ERR_PTR(-ENOMEM);
if (fwnode_property_read_bool(child, "adi,single-ended"))
adc->single_ended = true;
- if (!adc->single_ended &&
- sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
- dev_err(&st->spi->dev, "Invalid chan:%d for differential adc\n",
- sensor->chan);
- return ERR_PTR(-EINVAL);
- }
+ if (!adc->single_ended && sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN)
+ return dev_errp_probe(dev, -EINVAL,
+ "Invalid chan:%d for differential adc\n",
+ sensor->chan);
+
/* set common parameters */
adc->sensor.assign_chan = ltc2983_adc_assign_chan;
adc->sensor.fault_handler = ltc2983_common_fault_handler;
@@ -1165,21 +1147,20 @@ static struct ltc2983_sensor *ltc2983_temp_new(struct fwnode_handle *child,
struct ltc2983_data *st,
const struct ltc2983_sensor *sensor)
{
+ struct device *dev = &st->spi->dev;
struct ltc2983_temp *temp;
- temp = devm_kzalloc(&st->spi->dev, sizeof(*temp), GFP_KERNEL);
+ temp = devm_kzalloc(dev, sizeof(*temp), GFP_KERNEL);
if (!temp)
return ERR_PTR(-ENOMEM);
if (fwnode_property_read_bool(child, "adi,single-ended"))
temp->single_ended = true;
- if (!temp->single_ended &&
- sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
- dev_err(&st->spi->dev, "Invalid chan:%d for differential temp\n",
- sensor->chan);
- return ERR_PTR(-EINVAL);
- }
+ if (!temp->single_ended && sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN)
+ return dev_errp_probe(dev, -EINVAL,
+ "Invalid chan:%d for differential temp\n",
+ sensor->chan);
temp->custom = __ltc2983_custom_sensor_new(st, child, "adi,custom-temp",
false, 4096, true);
@@ -1329,10 +1310,9 @@ static int ltc2983_parse_fw(struct ltc2983_data *st)
device_property_read_u32(dev, "adi,filter-notch-freq", &st->filter_notch_freq);
st->num_channels = device_get_child_node_count(dev);
- if (!st->num_channels) {
- dev_err(&st->spi->dev, "At least one channel must be given!");
- return -EINVAL;
- }
+ if (!st->num_channels)
+ return dev_err_probe(dev, -EINVAL,
+ "At least one channel must be given!");
st->sensors = devm_kcalloc(dev, st->num_channels, sizeof(*st->sensors),
GFP_KERNEL);
@@ -1419,6 +1399,7 @@ static int ltc2983_eeprom_cmd(struct ltc2983_data *st, unsigned int cmd,
unsigned int wait_time, unsigned int status_reg,
unsigned long status_fail_mask)
{
+ struct device *dev = &st->spi->dev;
unsigned long time;
unsigned int val;
int ret;
@@ -1437,19 +1418,16 @@ static int ltc2983_eeprom_cmd(struct ltc2983_data *st, unsigned int cmd,
time = wait_for_completion_timeout(&st->completion,
msecs_to_jiffies(wait_time));
- if (!time) {
- dev_err(&st->spi->dev, "EEPROM command timed out\n");
- return -ETIMEDOUT;
- }
+ if (!time)
+ return dev_err_probe(dev, -ETIMEDOUT, "EEPROM command timed out\n");
ret = regmap_read(st->regmap, status_reg, &val);
if (ret)
return ret;
- if (val & status_fail_mask) {
- dev_err(&st->spi->dev, "EEPROM command failed: 0x%02X\n", val);
- return -EINVAL;
- }
+ if (val & status_fail_mask)
+ return dev_err_probe(dev, -EINVAL,
+ "EEPROM command failed: 0x%02X\n", val);
return 0;
}
@@ -1457,16 +1435,15 @@ static int ltc2983_eeprom_cmd(struct ltc2983_data *st, unsigned int cmd,
static int ltc2983_setup(struct ltc2983_data *st, bool assign_iio)
{
u32 iio_chan_t = 0, iio_chan_v = 0, chan, iio_idx = 0, status;
+ struct device *dev = &st->spi->dev;
int ret;
/* make sure the device is up: start bit (7) is 0 and done bit (6) is 1 */
ret = regmap_read_poll_timeout(st->regmap, LTC2983_STATUS_REG, status,
LTC2983_STATUS_UP(status) == 1, 25000,
25000 * 10);
- if (ret) {
- dev_err(&st->spi->dev, "Device startup timed out\n");
- return ret;
- }
+ if (ret)
+ return dev_err_probe(dev, ret, "Device startup timed out\n");
ret = regmap_update_bits(st->regmap, LTC2983_GLOBAL_CONFIG_REG,
LTC2983_NOTCH_FREQ_MASK,
@@ -1566,12 +1543,13 @@ static const struct iio_info ltc2983_iio_info = {
static int ltc2983_probe(struct spi_device *spi)
{
+ struct device *dev = &spi->dev;
struct ltc2983_data *st;
struct iio_dev *indio_dev;
struct gpio_desc *gpio;
int ret;
- indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
if (!indio_dev)
return -ENOMEM;
@@ -1582,10 +1560,9 @@ static int ltc2983_probe(struct spi_device *spi)
return -ENODEV;
st->regmap = devm_regmap_init_spi(spi, <c2983_regmap_config);
- if (IS_ERR(st->regmap)) {
- dev_err(&spi->dev, "Failed to initialize regmap\n");
- return PTR_ERR(st->regmap);
- }
+ if (IS_ERR(st->regmap))
+ return dev_err_probe(dev, PTR_ERR(st->regmap),
+ "Failed to initialize regmap\n");
mutex_init(&st->lock);
init_completion(&st->completion);
@@ -1597,7 +1574,7 @@ static int ltc2983_probe(struct spi_device *spi)
if (ret)
return ret;
- gpio = devm_gpiod_get_optional(&st->spi->dev, "reset", GPIOD_OUT_HIGH);
+ gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
if (IS_ERR(gpio))
return PTR_ERR(gpio);
@@ -1607,7 +1584,7 @@ static int ltc2983_probe(struct spi_device *spi)
gpiod_set_value_cansleep(gpio, 0);
}
- st->iio_chan = devm_kzalloc(&spi->dev,
+ st->iio_chan = devm_kzalloc(dev,
st->iio_channels * sizeof(*st->iio_chan),
GFP_KERNEL);
if (!st->iio_chan)
@@ -1617,12 +1594,10 @@ static int ltc2983_probe(struct spi_device *spi)
if (ret)
return ret;
- ret = devm_request_irq(&spi->dev, spi->irq, ltc2983_irq_handler,
+ ret = devm_request_irq(dev, spi->irq, ltc2983_irq_handler,
IRQF_TRIGGER_RISING, st->info->name, st);
- if (ret) {
- dev_err(&spi->dev, "failed to request an irq, %d", ret);
- return ret;
- }
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to request an irq\n");
if (st->info->has_eeprom) {
ret = ltc2983_eeprom_cmd(st, LTC2983_EEPROM_WRITE_CMD,
@@ -1639,7 +1614,7 @@ static int ltc2983_probe(struct spi_device *spi)
indio_dev->modes = INDIO_DIRECT_MODE;
indio_dev->info = <c2983_iio_info;
- return devm_iio_device_register(&spi->dev, indio_dev);
+ return devm_iio_device_register(dev, indio_dev);
}
static int ltc2983_resume(struct device *dev)
--
2.44.0
^ permalink raw reply related
* [PATCH v4 1/6] printk: add new dev_errp_probe() helper
From: Nuno Sa via B4 Relay @ 2024-03-28 16:22 UTC (permalink / raw)
To: linux-kernel, linux-iio, devicetree
Cc: Nuno Sá, Jonathan Cameron, Lars-Peter Clausen,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Olivier Moysan, Jyoti Bhayana
In-Reply-To: <20240328-ltc2983-misc-improv-v4-0-0cc428c07cd5@analog.com>
From: Nuno Sa <nuno.sa@analog.com>
This is similar to dev_err_probe() but for cases where an ERR_PTR() is
to be returned simplifying patterns like:
dev_err_probe(dev, ret, ...);
return ERR_PTR(ret)
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
include/linux/dev_printk.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/include/linux/dev_printk.h b/include/linux/dev_printk.h
index 6bfe70decc9f..64484d092a77 100644
--- a/include/linux/dev_printk.h
+++ b/include/linux/dev_printk.h
@@ -276,4 +276,9 @@ do { \
__printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
+/* Simple helper for dev_err_probe() when ERR_PTR() is to be returned. */
+#define dev_errp_probe(dev, ___err, fmt, ...) ({ \
+ ERR_PTR(dev_err_probe(dev, ___err, fmt, ##__VA_ARGS__)); \
+})
+
#endif /* _DEVICE_PRINTK_H_ */
--
2.44.0
^ permalink raw reply related
* [PATCH v4 0/6] iio: temperature: ltc2983: small improvements
From: Nuno Sa via B4 Relay @ 2024-03-28 16:22 UTC (permalink / raw)
To: linux-kernel, linux-iio, devicetree
Cc: Nuno Sá, Jonathan Cameron, Lars-Peter Clausen,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Olivier Moysan, Jyoti Bhayana, Krzysztof Kozlowski
The v4 introduces an new dev_errp_probe() helper to deal with cases
where we want to return error pointers. The refactor in the IIO ltc2983
is an heavy user of the pattern and was the main motivation for this.
Also added two new patches so we have three users of the new
dev_errp_probe() helper.
---
Changes in v4:
- Link to v3: https://lore.kernel.org/r/20240301-ltc2983-misc-improv-v3-0-c09516ac0efc@analog.com
- Patch 1
* New patch
- Patch 2
* Use dev_errp_probe() instead of local variant
- Patch 5
* New patch
- Patch 6
* New patch
---
Nuno Sa (6):
printk: add new dev_errp_probe() helper
iio: temperature: ltc2983: convert to dev_err_probe()
dt-bindings: iio: temperature: ltc2983: document power supply
iio: temperature: ltc2983: support vdd regulator
iio: backend: make use dev_errp_probe()
iio: common: scmi_iio: convert to dev_err_probe()
.../bindings/iio/temperature/adi,ltc2983.yaml | 4 +
drivers/iio/common/scmi_sensors/scmi_iio.c | 45 ++--
drivers/iio/industrialio-backend.c | 8 +-
drivers/iio/temperature/ltc2983.c | 260 ++++++++++-----------
include/linux/dev_printk.h | 5 +
5 files changed, 151 insertions(+), 171 deletions(-)
---
base-commit: 27eea4778db8268cd6dc80a5b853c599bd3099f1
change-id: 20240227-ltc2983-misc-improv-d9c4a3819b1f
--
Thanks!
- Nuno Sá
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: arm64: marvell: add solidrun cn9130 clearfog boards
From: Josua Mayer @ 2024-03-28 16:22 UTC (permalink / raw)
To: Andrew Lunn, Gregory Clement, Sebastian Hesselbarth, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: Yazan Shhady, linux-arm-kernel@lists.infradead.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <20240321-cn9130-som-v1-1-711127a409ae@solid-run.com>
Hi all,
Am 21.03.24 um 22:47 schrieb Josua Mayer:
> Add bindings for SolidRun Clearfog boards, using a new SoM based on
> CN9130 SoC.
> The carrier boards are identical to the older Armada 388 based Clearfog
> boards. For consistency the carrier part of compatible strings are
> copied, including the established "-a1" suffix.
>
> Signed-off-by: Josua Mayer <josua@solid-run.com>
> ---
> .../devicetree/bindings/arm/marvell/armada-7k-8k.yaml | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/marvell/armada-7k-8k.yaml b/Documentation/devicetree/bindings/arm/marvell/armada-7k-8k.yaml
> index 16d2e132d3d1..36bdfd1bedd9 100644
> --- a/Documentation/devicetree/bindings/arm/marvell/armada-7k-8k.yaml
> +++ b/Documentation/devicetree/bindings/arm/marvell/armada-7k-8k.yaml
> @@ -82,4 +82,16 @@ properties:
> - const: marvell,armada-ap807-quad
> - const: marvell,armada-ap807
>
> + - description:
> + SolidRun CN9130 clearfog family single-board computers
> + items:
vvv
> + - enum:
> + - solidrun,clearfog-base-a1
> + - solidrun,clearfog-pro-a1
> + - const: solidrun,clearfog-a1
^^^
After some thought, this no longer makes any sense to me.
Even identical carrier board combined with different SoC
will have different characteristics.
Any reason to repeat armada 388 board compatibles?
Otherwise I may choose only solidrun,cn9130-clearfog-base/pro.
> + - const: solidrun,cn9130-sr-som
> + - const: marvell,cn9130
> + - const: marvell,armada-ap807-quad
> + - const: marvell,armada-ap807
> +
> additionalProperties: true
>
^ permalink raw reply
* Re: [PATCH] arm64: dts: qcom: Add support for Samsung Galaxy Z Fold5
From: Krzysztof Kozlowski @ 2024-03-28 16:19 UTC (permalink / raw)
To: serdeliuk, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20240328-arm64-dts-add-support-for-samsung-galaxy-zfold5-v1-1-9434150d0465@yahoo.com>
On 28/03/2024 15:57, Alexandru Marc Serdeliuc via B4 Relay wrote:
> From: Alexandru Marc Serdeliuc <serdeliuk@yahoo.com>
>
> Add support for Samsung Galaxy Z Fold5 (q5q) foldable phone
>
> Currently working features:
> - Framebuffer
> - UFS
> - i2c
> - Buttons
>
> Signed-off-by: Alexandru Marc Serdeliuc <serdeliuk@yahoo.com>
> ---
> arch/arm64/boot/dts/qcom/Makefile | 1 +
> arch/arm64/boot/dts/qcom/sm8550-samsung-q5q.dts | 616 ++++++++++++++++++++++++
> 2 files changed, 617 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
> index 7d40ec5e7d21..a7503fd35b6c 100644
> --- a/arch/arm64/boot/dts/qcom/Makefile
> +++ b/arch/arm64/boot/dts/qcom/Makefile
> @@ -241,6 +241,7 @@ dtb-$(CONFIG_ARCH_QCOM) += sm8450-sony-xperia-nagara-pdx224.dtb
> dtb-$(CONFIG_ARCH_QCOM) += sm8550-hdk.dtb
> dtb-$(CONFIG_ARCH_QCOM) += sm8550-mtp.dtb
> dtb-$(CONFIG_ARCH_QCOM) += sm8550-qrd.dtb
> +dtb-$(CONFIG_ARCH_QCOM) += sm8550-samsung-q5q.dtb
> dtb-$(CONFIG_ARCH_QCOM) += sm8650-mtp.dtb
> dtb-$(CONFIG_ARCH_QCOM) += sm8650-qrd.dtb
> dtb-$(CONFIG_ARCH_QCOM) += x1e80100-crd.dtb
> diff --git a/arch/arm64/boot/dts/qcom/sm8550-samsung-q5q.dts b/arch/arm64/boot/dts/qcom/sm8550-samsung-q5q.dts
> new file mode 100644
> index 000000000000..ac8392022a7f
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sm8550-samsung-q5q.dts
> @@ -0,0 +1,616 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +/*
> + * Copyright (c) 2024, Alexandru Marc Serdeliuc <serdeliuk@yahoo.com>
> + * Copyright (c) 2024, David Wronek <david@mainlining.org>
> + * Copyright (c) 2022, Linaro Limited
> + */
> +
> +/dts-v1/;
> +
> +#include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
> +#include <dt-bindings/regulator/qcom,rpmh-regulator.h>
> +#include "sm8550.dtsi"
> +#include "pm8550.dtsi"
> +#include "pm8550vs.dtsi"
> +#include "pmk8550.dtsi"
> +
> +/ {
> + model = "Samsung Galaxy Z Fold5";
> + compatible = "samsung,q5q", "qcom,sm8550";
> + #address-cells = <0x02>;
> + #size-cells = <0x02>;
> + chassis-type = "handset";
> +
> + aliases {
> + serial0 = &uart7;
> + };
> +
> + chosen {
> + #address-cells = <2>;
> + #size-cells = <2>;
> + ranges;
> +
> + bootargs = "console=tty0 clk_ignore_unused pd_ignore_unused";
Console should be chosed via stdout. The "unused" are not parts of
hardware description, so drop entire bootargs.
> +
> + framebuffer: framebuffer@b8000000 {
> + compatible = "simple-framebuffer";
> + reg = <0x0 0xb8000000 0x0 0x2b00000>;
> + width = <2176>;
> + height = <1812>;
> + stride = <(2176 * 4)>;
> + format = "a8r8g8b8";
> + };
> + };
> +
> + gpio-keys {
> + compatible = "gpio-keys";
> + pinctrl-0 = <&volume_up_n>;
> + pinctrl-names = "default";
> +
> + key-volume-up {
> + label = "Volume Up";
> + linux,code = <KEY_VOLUMEUP>;
> + gpios = <&pm8550_gpios 6 GPIO_ACTIVE_LOW>;
> + debounce-interval = <15>;
> + linux,can-disable;
> + wakeup-source;
> + };
> + };
> +
> + vph_pwr: vph-pwr-regulator {
> + compatible = "regulator-fixed";
> + regulator-name = "vph_pwr";
> + regulator-min-microvolt = <3700000>;
> + regulator-max-microvolt = <3700000>;
> + regulator-always-on;
> + regulator-boot-on;
> + };
> +
> + reserved-memory {
> + chipinfo_region@81cf4000 {
Underscores are not allowed, use hyphens.
It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).
> + reg = <0x0 0x81cf4000 0x0 0x1000>;
> + no-map;
> + };
> +
> + kaslr_region@b01ff000 {
> + reg = <0x0 0xb01ff000 0x0 0x1000>;
> + no-map;
> + };
> +
> + uh_guest_region {
It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).
> + reg = <0x0 0xb1000000 0x0 0x3000000>;
> + no-map;
> + };
> +
> + uh_heap_region {
> + reg = <0x0 0xb0200000 0x0 0x40000>;
> + no-map;
> + };
> +
> +/*
> + * The bootloader will only keep display hardware enabled
> + * if this memory region is named exactly 'splash_region'
> + */
Missing indentation
> + splash_region {
> + reg = <0x0 0xb8000000 0x0 0x2b00000>;
> + no-map;
> + };
> + }; // end reserved-memory
Drop such comments. There is no such code in any mainline DTS. Please
use mainline DTS as your starting point.
You now duplicated a lot of issues which we solved already. That's not
how you upstream your board.
...
> + }; // end regulators-0
Really, drop the comment.
...
> +
> +&pcie0 {
> + status = "okay";
Status is the last property.
> + wake-gpios = <&tlmm 96 GPIO_ACTIVE_HIGH>;
> + perst-gpios = <&tlmm 94 GPIO_ACTIVE_LOW>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&pcie0_default_state>;
Reverse these two.
> +};
> +
> +&pcie0_phy {
> + status = "okay";
> + vdda-phy-supply = <&vreg_l1e_0p88>;
> + vdda-pll-supply = <&vreg_l3e_1p2>;
> +};
> +
> +&pcie1 {
> + status = "okay";
> + wake-gpios = <&tlmm 99 GPIO_ACTIVE_HIGH>;
> + perst-gpios = <&tlmm 97 GPIO_ACTIVE_LOW>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&pcie1_default_state>;
> +};
> +
> +&pcie1_phy {
> + vdda-phy-supply = <&vreg_l3c_0p91>;
> + 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;
> + };
> +};
> +
> +&qupv3_id_0 {
> + status = "okay";
> +};
> +
> +&remoteproc_adsp {
> + status = "okay";
> + firmware-name = "qcom/sm8550/adsp.mbn",
> + "qcom/sm8550/adsp_dtb.mbn";
> +};
> +
> +&remoteproc_cdsp {
> + status = "okay";
> + firmware-name = "qcom/sm8550/cdsp.mbn",
> + "qcom/sm8550/cdsp_dtb.mbn";
> +};
> +
> +&remoteproc_mpss {
> + status = "okay";
> + firmware-name = "qcom/sm8550/modem.mbn",
> + "qcom/sm8550/modem_dtb.mbn";
> +};
> +
> +&sleep_clk {
> + clock-frequency = <32000>;
> +};
> +
> +&tlmm {
> + gpio-reserved-ranges = <36 4>, <50 2>;
> +};
> +
> +&ufs_mem_hc {
> + status = "okay";
> + reset-gpios = <&tlmm 210 GPIO_ACTIVE_LOW>;
> + vcc-supply = <&vreg_l17b_2p5>;
> + vcc-max-microamp = <1300000>;
> + vccq-supply = <&vreg_l1g_1p2>;
> + vccq-max-microamp = <1200000>;
> + vdd-hba-supply = <&vreg_l3g_1p2>;
> +};
> +
> +&ufs_mem_phy {
> + status = "okay";
> + vdda-phy-supply = <&vreg_l1d_0p88>;
> + vdda-pll-supply = <&vreg_l3e_1p2>;
> +};
> +
> +&xo_board {
> + clock-frequency = <76800000>;
> +};
> +
> +&dispcc {
> + status = "disabled";
> +};
> +
> +&pon_pwrkey {
Does not look ordered by name. Keep alphanetical order of node
overrides. Just like all other DTS does, which you should take as
starting point.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH net-next v6 17/17] net: pse-pd: Add TI TPS23881 PSE controller driver
From: Andrew Lunn @ 2024-03-28 16:17 UTC (permalink / raw)
To: Kory Maincent
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jonathan Corbet, Luis Chamberlain, Russ Weight,
Greg Kroah-Hartman, Rafael J. Wysocki, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Oleksij Rempel, Mark Brown,
Frank Rowand, Heiner Kallweit, Russell King, Thomas Petazzoni,
netdev, linux-kernel, linux-doc, devicetree, Dent Project
In-Reply-To: <20240326-feature_poe-v6-17-c1011b6ea1cb@bootlin.com>
> +static int
> +tps23881_get_unused_chan(struct tps23881_port_matrix port_matrix[TPS23881_MAX_CHANS],
> + int port_cnt)
> +{
> + bool used;
> + int i, j;
> +
> + for (i = 0; i < TPS23881_MAX_CHANS; i++) {
> + used = false;
> +
> + for (j = 0; j < port_cnt; j++) {
> + if (port_matrix[j].hw_chan[0] == i) {
> + used = true;
> + break;
> + }
> +
> + if (port_matrix[j].is_4p &&
> + port_matrix[j].hw_chan[1] == i) {
> + used = true;
> + break;
> + }
> + }
> +
> + if (!used)
> + return i;
> + }
> +
> + return -1;
nitpick: Return -ENODEV.
> + while (cnt_4ch_grp1 < 4) {
> + ret = tps23881_get_unused_chan(tmp_port_matrix, port_cnt);
> + if (ret < 0) {
> + pr_err("tps23881: port matrix issue, no chan available\n");
> + return -ENODEV;
and then just returns ret.
> +static int
> +tps23881_set_ports_conf(struct tps23881_priv *priv,
> + struct tps23881_port_matrix port_matrix[TPS23881_MAX_CHANS])
> +{
> + struct i2c_client *client = priv->client;
> + int i, ret;
> + u16 val;
> +
> + /* Set operating mode */
> + ret = i2c_smbus_write_word_data(client, TPS23881_REG_OP_MODE, 0xaaaa);
Could you add some #defines here? This is semiauto i think?
> + if (ret)
> + return ret;
> +
> + /* Disable DC disconnect */
> + ret = i2c_smbus_write_word_data(client, TPS23881_REG_DIS_EN, 0x0);
> + if (ret)
> + return ret;
> +
> + /* Set port power allocation */
> + val = 0;
> + for (i = 0; i < TPS23881_MAX_CHANS; i++) {
> + if (!port_matrix[i].exist)
> + continue;
> +
> + if (port_matrix[i].is_4p)
> + val |= 0xf << ((port_matrix[i].lgcl_chan[0] / 2) * 4);
> + else
> + val |= 0x3 << ((port_matrix[i].lgcl_chan[0] / 2) * 4);
> + }
> + ret = i2c_smbus_write_word_data(client, TPS23881_REG_PORT_POWER, val);
> + if (ret)
> + return ret;
> +
> + /* Enable detection and classification */
> + val = 0;
> + for (i = 0; i < TPS23881_MAX_CHANS; i++) {
> + if (!port_matrix[i].exist)
> + continue;
> +
> + val |= BIT(port_matrix[i].lgcl_chan[0]) |
> + BIT(port_matrix[i].lgcl_chan[0] + 4);
> + if (port_matrix[i].is_4p)
> + val |= BIT(port_matrix[i].lgcl_chan[1]) |
> + BIT(port_matrix[i].lgcl_chan[1] + 4);
> + }
> + ret = i2c_smbus_write_word_data(client, TPS23881_REG_DET_CLA_EN, 0xffff);
This looks odd. You calculate val, and then don't use it?
Andrew
^ permalink raw reply
* Re: [PATCH] dt-bindings: arm: qcom: Add Samsung Galaxy Z Fold5
From: Krzysztof Kozlowski @ 2024-03-28 16:16 UTC (permalink / raw)
To: serdeliuk, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20240328-dt-bindings-arm-qcom-add-support-for-samsung-galaxy-zfold5-v1-1-cb612e3ade18@yahoo.com>
On 28/03/2024 15:31, Alexandru Marc Serdeliuc via B4 Relay wrote:
> From: Alexandru Marc Serdeliuc <serdeliuk@yahoo.com>
>
> This documents Samsung Galaxy Z Fold5 (samsung,q5q)
> which is a foldable phone by Samsung based on the sm8550 SoC.
>
> Signed-off-by: Alexandru Marc Serdeliuc <serdeliuk@yahoo.com>
> ---
> This documents Samsung Galaxy Z Fold5 (samsung,q5q)
> which is a foldable phone by Samsung based on the sm8550 SoC.
Please always send DTS and its binding together.
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
This is an automated instruction, just in case, because many review tags
are being ignored. If you know the process, you can skip it (please do
not feel offended by me posting it here - no bad intentions intended).
If you do not know the process, here is a short explanation:
Please add Acked-by/Reviewed-by/Tested-by tags when posting new
versions, under or above your Signed-off-by tag. Tag is "received", when
provided in a message replied to you on the mailing list. Tools like b4
can help here. However, there's no need to repost patches *only* to add
the tags. The upstream maintainer will do that for tags received on the
version they apply.
https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 17/23] dt-bindings: media: imx258: Rename to include vendor prefix
From: Luigi311 @ 2024-03-28 16:15 UTC (permalink / raw)
To: Conor Dooley, Kieran Bingham
Cc: Conor Dooley, git, linux-media, dave.stevenson, jacopo.mondi,
mchehab, robh, krzysztof.kozlowski+dt, conor+dt, shawnguo,
s.hauer, kernel, festevam, sakari.ailus, devicetree, imx,
linux-arm-kernel, linux-kernel
In-Reply-To: <20240328-prideful-unopposed-e29fcee74c29@wendy>
On 3/28/24 04:15, Conor Dooley wrote:
> On Thu, Mar 28, 2024 at 08:52:01AM +0000, Kieran Bingham wrote:
>> Quoting git@luigi311.com (2024-03-28 00:57:34)
>>> On 3/27/24 17:47, Conor Dooley wrote:
>>>> On Wed, Mar 27, 2024 at 05:17:03PM -0600, git@luigi311.com wrote:
>>>>> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
>>>>>
>>>>> imx258.yaml doesn't include the vendor prefix of sony, so
>>>>> rename to add it.
>>>>> Update the id entry and MAINTAINERS to match.
>>>>>
>>>>> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>>>>> Acked-by: Conor Dooley <conor.dooley@microchip.com>
>>>>
>>>> This is a v1 with my ack, something has gone awry here. It's also
>>>> missing your signoff. Did you pick up someone else's series?
>>>
>>> Yes, this is a continuation of Dave's work. I contacted him directly,
>>> and he mentioned that he is unable to submit a v2 any time soon and
>>> was open to someone else continuing it in his stead.
>
> Ah okay. Unfortunately I see so many binding patches pass by that I
> sometimes forget about what I already reviewed, and I did not
> remember this one at all.
No worries I'm not surprised since i see constant things submitted
to upstream and v1 was actually sent a year ago so there would be no
shot that i would remember it either.
>
>>> This is my first
>>> time submitting a patch via a mailing list, so I'm not sure if I'm
>>> missing something, but I only added my sign off for anything that
>>> actually included work from my side and not just bringing his patch
>>> forward to this patch series.
>
> Right. The rules are that you need to add it when you send someone's
> work, like chain of custody type of thing.
Ohh i see, ok ill go ahead and add my sign off to all the patches then
>
>> Your cover letter states v2, but the individual patches do not.
>>
>> Add the '-v2' (or, rather, next it will be '-v3') to git format-patch
>> when you save your series and it will add the version to each patch. You
>> can also add '-s' to that command I believe to add your SoB to each
>> patch.
>
> or a rebase will do it with --signoff:
> https://git-scm.com/docs/git-rebase#Documentation/git-rebase.txt---signoff
Perfect thanks for the information you two! Ill be sure to use those
for the next revision.
>
> Cheers,
> Conor.
^ permalink raw reply
* Re: [PATCH 08/10] iio: backend: add new functionality
From: Jonathan Cameron @ 2024-03-28 15:59 UTC (permalink / raw)
To: Nuno Sá
Cc: Nuno Sa via B4 Relay, nuno.sa, linux-iio, devicetree,
Dragos Bogdan, Lars-Peter Clausen, Michael Hennerich, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Olivier Moysan
In-Reply-To: <cec2ac9c67aeae7c59434a86713f35461d171c04.camel@gmail.com>
On Thu, 28 Mar 2024 16:42:38 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:
> On Thu, 2024-03-28 at 15:16 +0000, Jonathan Cameron wrote:
> > On Thu, 28 Mar 2024 14:22:32 +0100
> > Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
> >
> > > From: Nuno Sa <nuno.sa@analog.com>
> > >
> > > This adds the needed backend ops for supporting a backend inerfacing
> > > with an high speed dac. The new ops are:
> > >
> > > * data_source_set();
> > > * set_sampling_freq();
> > > * extend_chan_spec();
> > > * ext_info_set();
> > > * ext_info_get().
> > >
> > > Also to note the new helpers that are meant to be used by the backends
> > > + return 0;
> > > + /*
> > > + * !\NOTE: this will break as soon as we have multiple backends on one
> > > + * frontend and all of them extend channels. In that case, the core
> > > + * backend code has no way to get the correct backend given the
> > > + * iio device.
> > > + *
> > > + * One solution for this could be introducing a new backend
> > > + * dedicated callback in struct iio_info so we can callback into the
> > > + * frontend so it can give us the right backend given a chan_spec.
> > > + */
> >
> > Hmm. This is indeed messy. Could we associate it with the buffer as presuably
> > a front end with multiple backends is using multiple IIO buffers?
> >
>
> Hmm, the assumption of having multiple buffers seems plausible to me but considering
> the example we have in hands it would be cumbersome to get the backend. Considering
> iio_backend_ext_info_get(), how could we get the backend if it was associated to one
> of the IIO buffers? I think we would need more "intrusive" changes to make that work
> or do you have something in mind=
Nope. Just trying to get my head around the associations. I hadn't thought about
how to make that visible in the code. Probably a callabck anyway.
>
> > As you say a dance via the front end would work fine.
>
> I'm happy you're also open for a proper solution already. I mention this in the
> cover. My idea was something like (consider the iio_backend_ext_info_get()):
>
> if (!indio_dev->info->get_iio_backend())
> return -EOPNOTSUPP;
>
> back = indio_dev->info->get_iio_backend(indio_dev, chan_spec);
>
> It would be nice to have some "default/generic" implementation for cases where we
> only have one backend per frontend so that the frontend would not need to define the
> callback.
Agreed - either a default that means if the callback isn't provided we get the
single backend or if that proves fiddly at least a standard callback we can
use in all such cases.
>
> >
> >
> > > + iio_device_set_drvdata(indio_dev, back);
> > > +
> > > + /* Don't allow backends to get creative and force their own handlers */
> > > + for (ext_info = chan->ext_info; ext_info->name; ext_info++) {
> > > + if (ext_info->read != iio_backend_ext_info_get)
> > > + return -EINVAL;
> > > + if (ext_info->write != iio_backend_ext_info_set)
> > > + return -EINVAL;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(iio_backend_extend_chan_spec, IIO_BACKEND);
> >
> > > diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h
> > > index a6d79381866e..09ff2f8f9fd8 100644
> > > --- a/include/linux/iio/backend.h
> > > +++ b/include/linux/iio/backend.h
> > > @@ -4,6 +4,7 @@
> > >
> > > #include <linux/types.h>
> > >
> > > +struct iio_chan_spec;
> > > struct fwnode_handle;
> > > struct iio_backend;
> > > struct device;
> > > @@ -15,6 +16,26 @@ enum iio_backend_data_type {
> > > IIO_BACKEND_DATA_TYPE_MAX
> > > };
> > >
> > > +enum iio_backend_data_source {
> > > + IIO_BACKEND_INTERNAL_CW,
> >
> > CW? Either expand out what ever that is in definition of add a comment
> > at least.
>
> Continuous wave :)
Spell that out.
>
> >
> > > + IIO_BACKEND_EXTERNAL,
> > What does external mean in this case?
>
> In this particular case comes from a DMA source (IP). I thought external to be more
> generic but if you prefer, I can do something like IIO_BACKEND_DMA?
So from another IP block? For that to be reasonably 'generic' we'd need a way
to known where it was coming from.
Now I remember advantage of reviewing on weekends - fewer replies during the reviews :)
Jonathan
^ permalink raw reply
* Re: [PATCH net-next v6 16/17] dt-bindings: net: pse-pd: Add bindings for TPS23881 PSE controller
From: Andrew Lunn @ 2024-03-28 15:58 UTC (permalink / raw)
To: Kory Maincent
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jonathan Corbet, Luis Chamberlain, Russ Weight,
Greg Kroah-Hartman, Rafael J. Wysocki, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Oleksij Rempel, Mark Brown,
Frank Rowand, Heiner Kallweit, Russell King, Thomas Petazzoni,
netdev, linux-kernel, linux-doc, devicetree, Dent Project
In-Reply-To: <20240326-feature_poe-v6-16-c1011b6ea1cb@bootlin.com>
On Tue, Mar 26, 2024 at 03:04:53PM +0100, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
>
> Add the TPS23881 I2C Power Sourcing Equipment controller device tree
> bindings documentation.
>
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH net-next v6 15/17] net: pse-pd: Add PD692x0 PSE controller driver
From: Andrew Lunn @ 2024-03-28 15:57 UTC (permalink / raw)
To: Kory Maincent
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jonathan Corbet, Luis Chamberlain, Russ Weight,
Greg Kroah-Hartman, Rafael J. Wysocki, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Oleksij Rempel, Mark Brown,
Frank Rowand, Heiner Kallweit, Russell King, Thomas Petazzoni,
netdev, linux-kernel, linux-doc, devicetree, Dent Project
In-Reply-To: <20240326-feature_poe-v6-15-c1011b6ea1cb@bootlin.com>
On Tue, Mar 26, 2024 at 03:04:52PM +0100, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
>
> Add a new driver for the PD692x0 I2C Power Sourcing Equipment controller.
> This driver only support i2c communication for now.
>
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH 01/10] iio: buffer: add helper for setting direction
From: Jonathan Cameron @ 2024-03-28 15:54 UTC (permalink / raw)
To: Nuno Sá
Cc: Nuno Sa via B4 Relay, nuno.sa, linux-iio, devicetree,
Dragos Bogdan, Lars-Peter Clausen, Michael Hennerich, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Olivier Moysan
In-Reply-To: <d30c43f7f9d2db5b9c0e779d99f029da1a751636.camel@gmail.com>
On Thu, 28 Mar 2024 16:18:04 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:
> On Thu, 2024-03-28 at 14:36 +0000, Jonathan Cameron wrote:
> > On Thu, 28 Mar 2024 14:22:25 +0100
> > Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
> >
> > > From: Nuno Sa <nuno.sa@analog.com>
> > >
> > > Simple helper for setting the buffer direction when it's allocated using
> > > iio_dmaengine_buffer_alloc().
> > >
> > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > I wonder if we should align with the approach for triggered-buffers with and _ext
> > form of the registration function that takes a direction. It seems odd to allocate
> > one then change the direction.
> >
>
> I agree it feels odd but I did not wanted to include buffer_impl.h in places that
> should not have it :)
>
> This patchseries adds the direction to devm_iio_dmaengine_buffer_setup(). Maybe what
> we need is to have a non devm variant iio_dmaengine_buffer_setup() and turn
> iio_dmaengine_buffer_alloc() static again. Maybe that would make things a bit more
> consistent. In fact looking closer into that file, I would get rid of:
>
> devm_iio_dmaengine_buffer_alloc() and __devm_iio_dmaengine_buffer_free()
>
> and have:
>
> devm_iio_dmaengine_buffer_setup() and iio_dmaengine_buffer_setup() that make use of
> iio_dmaengine_buffer_free() and iio_dmaengine_buffer_alloc().
>
> I think it would make more sense like the above. Thoughts?
Sounds reasonable to me
Jonathan
>
> - Nuno Sá
>
^ permalink raw reply
* Re: [PATCH 10/10] iio: dac: support the ad9739a RF DAC
From: Jonathan Cameron @ 2024-03-28 15:51 UTC (permalink / raw)
To: Nuno Sa via B4 Relay
Cc: nuno.sa, linux-iio, devicetree, Dragos Bogdan, Lars-Peter Clausen,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Olivier Moysan
In-Reply-To: <20240328-iio-backend-axi-dac-v1-10-afc808b3fde3@analog.com>
On Thu, 28 Mar 2024 14:22:34 +0100
Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
> From: Nuno Sa <nuno.sa@analog.com>
>
> The AD9739A is a 14-bit, 2.5 GSPS high performance RF DACs that are capable
> of synthesizing wideband signals from dc up to 3 GHz.
DC perhaps
>
> A dual-port, source synchronous, LVDS interface simplifies the digital
> interface with existing FGPA/ASIC technology. On-chip controllers are used
> to manage external and internal clock domain variations over temperature to
> ensure reliable data transfer from the host to the DAC core.
>
> Co-developed-by: Dragos Bogdan <dragos.bogdan@analog.com>
> Signed-off-by: Dragos Bogdan <dragos.bogdan@analog.com>
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
Hi Nuno,
A few questions / comments inline but on the whole looking good to me.
Jonathan
> ---
> Documentation/ABI/testing/sysfs-bus-iio-ad9739a | 17 +
> MAINTAINERS | 1 +
> drivers/iio/dac/Kconfig | 16 +
> drivers/iio/dac/Makefile | 1 +
> drivers/iio/dac/ad9739a.c | 445 ++++++++++++++++++++++++
> 5 files changed, 480 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-ad9739a b/Documentation/ABI/testing/sysfs-bus-iio-ad9739a
> new file mode 100644
> index 000000000000..8a8a5cd10386
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-ad9739a
> @@ -0,0 +1,17 @@
> +What: /sys/bus/iio/devices/iio:deviceX/out_voltageY_operating_mode
> +KernelVersion: 6.9
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + Dac operating mode. One of the following modes can be selected:
DAC operating mode. ...
> + * normal: This is DAC normal mode.
> + * mixed-mode: In this mode the output is effectively chopped at the
Spaces and tabs mixed...
> + DAC sample rate. This has the effect of reducing the
> + power of the fundamental signal while increasing the
> + power of the images centered around the DAC sample rate,
> + thus improving the output power of these images.
Any idea why it is called mixed mode? Name doesn't suggest to me what the Docs say
this does.
> +
> +What: /sys/bus/iio/devices/iio:deviceX/out_voltageY_operating_mode_available
> +KernelVersion: 6.9
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + Available operating modes.
> M: Antoniu Miclaus <antoniu.miclaus@analog.com>
> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index 7c0a8caa9a34..ee0d9798d8b4 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -131,6 +131,22 @@ config AD5624R_SPI
> Say yes here to build support for Analog Devices AD5624R, AD5644R and
> AD5664R converters (DAC). This driver uses the common SPI interface.
>
> +config AD9739A
> + tristate "Analog Devices AD9739A RF DAC spi driver"
> + depends on SPI
> + select REGMAP_SPI
> + select IIO_BACKEND
> + help
> + Say yes here to build support for Analog Devices AD9739A Digital-to
> + Analog Converter.
> +
> + The driver requires the assistance of the AXI DAC IP core to operate,
Maybe a depends on || COMPILE_TEST to increase build coverage (compared to
a hard depends on)
> + since SPI is used for configuration only, while data has to be
> + streamed into memory via DMA.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called ad9739a.
> +
> diff --git a/drivers/iio/dac/ad9739a.c b/drivers/iio/dac/ad9739a.c
> new file mode 100644
> index 000000000000..46431fa345a5
> --- /dev/null
> +++ b/drivers/iio/dac/ad9739a.c
> +
> +enum {
> + AD9739A_NORMAL_MODE,
> + AD9739A_MIXED_MODE = 2,
Push these next to the relevant registers and more conventional defines.
Not seeing why the enum helps much here.
> +};
> +
> +static int ad9739a_oper_mode_get(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan)
> +{
> + struct ad9739a_state *st = iio_priv(indio_dev);
> + u32 mode;
> + int ret;
> +
> + ret = regmap_read(st->regmap, AD9739A_REG_DEC_CNT, &mode);
> + if (ret)
> + return ret;
> +
> + mode = FIELD_GET(AD9739A_DAC_DEC, mode);
> + /* sanity check we get valid values from the HW */
> + if (mode != AD9739A_NORMAL_MODE && mode != AD9739A_MIXED_MODE)
> + return -EIO;
> + if (!mode)
> + return AD9739A_NORMAL_MODE;
> +
> + return AD9739A_MIXED_MODE - 1;
As below. I'd like to see a mapping function, or lookup table or similar
rather than handling this conversion in code.
> +}
> +
> +static int ad9739a_oper_mode_set(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan, u32 mode)
> +{
> + struct ad9739a_state *st = iio_priv(indio_dev);
> +
> + if (mode == AD9739A_MIXED_MODE - 1)
> + mode = AD9739A_MIXED_MODE;
Why? Feels like a comment is needed. Or a more obvious conversion function.
> +
> + return regmap_update_bits(st->regmap, AD9739A_REG_DEC_CNT,
> + AD9739A_DAC_DEC, mode);
> +}
> +
> +static int ad9739a_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct ad9739a_state *st = iio_priv(indio_dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *val = st->sample_rate;
> + *val2 = 0;
> + return IIO_VAL_INT_64;
Big numbers :)
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +
> +/*
> + * Recommended values (as per datasheet) for the dac clk common mode voltage
> + * and Mu controller. Look at table 29.
> + */
> +static const struct reg_sequence ad9739a_clk_mu_ctrl[] = {
> + /* DAC clk common mode voltage */
> + {AD9739A_REG_CROSS_CNT1, 0x0f},
{ AD9739A_REG_CROSS_CNT1, 0x0f },
etc is more readable in my opinion so is always my preference in IIO.
> + {AD9739A_REG_CROSS_CNT2, 0x0f},
> + /* Mu controller configuration */
> + {AD9739A_REG_PHS_DET, 0x30},
> + {AD9739A_REG_MU_DUTY, 0x80},
> + {AD9739A_REG_MU_CNT2, 0x44},
> + {AD9739A_REG_MU_CNT3, 0x6c},
> +};
> +
> +static int ad9739a_init(struct device *dev, const struct ad9739a_state *st)
> +{
> + unsigned int i = 0, lock, fsc;
> + u32 fsc_raw;
> + int ret;
> +
> + ret = regmap_multi_reg_write(st->regmap, ad9739a_clk_mu_ctrl,
> + ARRAY_SIZE(ad9739a_clk_mu_ctrl));
> + if (ret)
> + return ret;
> +
> + /*
> + * Try to get the MU lock. Repeat the below steps AD9739A_LOCK_N_TRIES
> + * (as specified by the datasheet) until we get the lock.
> + */
> + do {
> + ret = regmap_write(st->regmap, AD9739A_REG_MU_CNT4,
> + AD9739A_MU_CNT4_DEFAULT);
> + if (ret)
> + return ret;
> +
> + /* Enable the Mu controller search and track mode. */
MU for consistency
> + ret = regmap_set_bits(st->regmap, AD9739A_REG_MU_CNT1,
> + AD9739A_MU_EN_MASK);
> + if (ret)
> + return ret;
> +
> + /* Ensure the DLL loop is locked */
> + ret = regmap_read_poll_timeout(st->regmap, AD9739A_REG_MU_STAT1,
> + lock, lock & AD9739A_MU_LOCK_MASK,
> + 0, 1000);
if (ret < 0 && ret != -ETIMEOUT)
return ret;
i.e. deal with error codes that don't meant it timed out.
> + } while (ret && ++i < AD9739A_LOCK_N_TRIES);
> +
> + if (i == AD9739A_LOCK_N_TRIES)
> + return dev_err_probe(dev, ret, "Mu lock timeout\n");
> +
> + /* Receiver tracking and lock. Same deal as the Mu controller */
MU or Mu. Either fine but be consistent in comments. I have no idea what this is
so can't say which is better.
> + i = 0;
> + do {
> + ret = regmap_update_bits(st->regmap, AD9739A_REG_LVDS_REC_CNT4,
> + AD9739A_FINE_DEL_SKW_MASK,
> + FIELD_PREP(AD9739A_FINE_DEL_SKW_MASK, 2));
> + if (ret)
> + return ret;
> +
> + /* Disable the receiver and the loop. */
> + ret = regmap_write(st->regmap, AD9739A_REG_LVDS_REC_CNT1, 0);
> + if (ret)
> + return ret;
> +
> + /*
> + * Re-enable the loop so it falls out of lock and begins the
> + * search/track routine again.
> + */
> + ret = regmap_set_bits(st->regmap, AD9739A_REG_LVDS_REC_CNT1,
> + AD9739A_RCVR_LOOP_EN_MASK);
> + if (ret)
> + return ret;
> +
> + /* Ensure the DLL loop is locked */
> + ret = regmap_read_poll_timeout(st->regmap,
> + AD9739A_REG_LVDS_REC_STAT9, lock,
> + lock == AD9739A_RCVR_TRACK_AND_LOCK,
> + 0, 1000);
As above, consider other error codes than -ETIMEOUT;
> + } while (ret && ++i < AD9739A_LOCK_N_TRIES);
> +
> + if (i == AD9739A_LOCK_N_TRIES)
> + return dev_err_probe(dev, ret, "Receiver lock timeout\n");
> +
> + ret = device_property_read_u32(dev, "adi,full-scale-microamp", &fsc);
> + if (ret && ret == -EINVAL)
> + return 0;
> + if (ret)
> + return ret;
> + if (!in_range(fsc, AD9739A_FSC_MIN, AD9739A_FSC_RANGE))
> + return dev_err_probe(dev, -EINVAL,
> + "Invalid full scale current(%u) [%u %u]\n",
> + fsc, AD9739A_FSC_MIN, AD9739A_FSC_MAX);
> + /*
> + * IOUTFS is given by
> + * Ioutfs = 0.0226 * FSC + 8.58
> + * and is given in mA. Hence we'll have to multiply by 10 * MILLI in
> + * order to get rid of the fractional.
> + */
> + fsc_raw = DIV_ROUND_CLOSEST(fsc * 10 - 85800, 226);
> +
> + ret = regmap_write(st->regmap, AD9739A_REG_FSC_1, fsc_raw & 0xff);
> + if (ret)
> + return ret;
> +
> + return regmap_update_bits(st->regmap, AD9739A_REG_FSC_1,
> + AD9739A_FSC_MSB, fsc_raw >> 8);
> +}
> +
> +static int ad9739a_probe(struct spi_device *spi)
> +{
> + struct device *dev = &spi->dev;
> + struct iio_dev *indio_dev;
> + struct ad9739a_state *st;
> + unsigned int id;
> + struct clk *clk;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> +
> + clk = devm_clk_get_enabled(dev, NULL);
> + if (IS_ERR(clk))
> + return dev_err_probe(dev, PTR_ERR(clk), "Could not get clkin\n");
> +
> + st->sample_rate = clk_get_rate(clk);
> + if (!in_range(st->sample_rate, AD9739A_MIN_DAC_CLK,
> + AD9739A_DAC_CLK_RANGE))
> + return dev_err_probe(dev, -EINVAL,
> + "Invalid dac clk range(%lu) [%lu %lu]\n",
> + st->sample_rate, AD9739A_MIN_DAC_CLK,
> + AD9739A_MAX_DAC_CLK);
> +
> + st->regmap = devm_regmap_init_spi(spi, &ad9739a_regmap_config);
> + if (IS_ERR(st->regmap))
> + return PTR_ERR(st->regmap);
> +
> + ret = regmap_read(st->regmap, AD9739A_REG_ID, &id);
> + if (ret)
> + return ret;
> +
> + if (id != AD9739A_ID)
> + return dev_err_probe(dev, -ENODEV, "Unrecognized CHIP_ID 0x%X",
> + id);
Do we have to give up here? Could it be a compatible future part?
If so we should fallback on what firmware told us it was + perhaps a
dev_info() to say we don't recognise the ID register value.
> +
> + ret = ad9739a_reset(dev, st);
> + if (ret)
> + return ret;
> +
> + ret = ad9739a_init(dev, st);
> + if (ret)
> + return ret;
> +
> + st->back = devm_iio_backend_get(dev, NULL);
> + if (IS_ERR(st->back))
> + return PTR_ERR(st->back);
> +
> + ret = devm_iio_backend_request_buffer(dev, st->back, indio_dev);
> + if (ret)
> + return ret;
> +
> + ret = iio_backend_extend_chan_spec(indio_dev, st->back,
> + &ad9739a_channels[0]);
> + if (ret)
> + return ret;
> +
> + ret = iio_backend_set_sampling_freq(st->back, 0, st->sample_rate);
> + if (ret)
> + return ret;
> +
> + ret = devm_iio_backend_enable(dev, st->back);
> + if (ret)
> + return ret;
> +
> + indio_dev->name = "ad9739a";
> + indio_dev->info = &ad9739a_info;
> + indio_dev->channels = ad9739a_channels;
> + indio_dev->num_channels = ARRAY_SIZE(ad9739a_channels);
> + indio_dev->setup_ops = &ad9739a_buffer_setup_ops;
> +
> + return devm_iio_device_register(&spi->dev, indio_dev);
> +}
^ permalink raw reply
* Re: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support
From: Cristian Marussi @ 2024-03-28 15:47 UTC (permalink / raw)
To: Peng Fan (OSS)
Cc: Sudeep Holla, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Linus Walleij, Dan Carpenter, linux-arm-kernel, linux-kernel,
devicetree, linux-gpio, Peng Fan, Oleksii Moisieiev
In-Reply-To: <20240323-pinctrl-scmi-v6-3-a895243257c0@nxp.com>
On Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
>
> Add basic implementation of the SCMI v3.2 pincontrol protocol.
>
Hi,
a few more comments down below...
> Co-developed-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> drivers/firmware/arm_scmi/Makefile | 1 +
> drivers/firmware/arm_scmi/driver.c | 2 +
> drivers/firmware/arm_scmi/pinctrl.c | 921 ++++++++++++++++++++++++++++++++++
> drivers/firmware/arm_scmi/protocols.h | 1 +
> include/linux/scmi_protocol.h | 75 +++
> 5 files changed, 1000 insertions(+)
>
> diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
> index a7bc4796519c..8e3874ff1544 100644
> --- a/drivers/firmware/arm_scmi/Makefile
> +++ b/drivers/firmware/arm_scmi/Makefile
> @@ -11,6 +11,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o
> scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
> scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
> scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
> +scmi-protocols-y += pinctrl.o
> scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)
>
> obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 415e6f510057..ac2d4b19727c 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -3142,6 +3142,7 @@ static int __init scmi_driver_init(void)
> scmi_voltage_register();
> scmi_system_register();
> scmi_powercap_register();
> + scmi_pinctrl_register();
>
> return platform_driver_register(&scmi_driver);
> }
> @@ -3159,6 +3160,7 @@ static void __exit scmi_driver_exit(void)
> scmi_voltage_unregister();
> scmi_system_unregister();
> scmi_powercap_unregister();
> + scmi_pinctrl_unregister();
>
> scmi_transports_exit();
>
> diff --git a/drivers/firmware/arm_scmi/pinctrl.c b/drivers/firmware/arm_scmi/pinctrl.c
> new file mode 100644
> index 000000000000..87d9b89cab13
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/pinctrl.c
> @@ -0,0 +1,921 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * System Control and Management Interface (SCMI) Pinctrl Protocol
> + *
> + * Copyright (C) 2024 EPAM
> + * Copyright 2024 NXP
> + */
> +
> +#include <linux/module.h>
> +#include <linux/scmi_protocol.h>
> +#include <linux/slab.h>
> +
> +#include "common.h"
> +#include "protocols.h"
> +
> +/* Updated only after ALL the mandatory features for that version are merged */
> +#define SCMI_PROTOCOL_SUPPORTED_VERSION 0x0
> +
AFAICS, the only missing things are PINCTRL_SET_PERMISSIONS (optional command)
and the multiple-configs on SETTINGS_GET, but this latter is something really
that we have to ask for in the request AND we did not as of now since we dont
need it...so I would say to bump the version to 0x10000 just to avoid needless
warning as soon as a server supporting Pinctrl is met.
> +#define GET_GROUPS_NR(x) le32_get_bits((x), GENMASK(31, 16))
> +#define GET_PINS_NR(x) le32_get_bits((x), GENMASK(15, 0))
> +#define GET_FUNCTIONS_NR(x) le32_get_bits((x), GENMASK(15, 0))
> +
> +#define EXT_NAME_FLAG(x) le32_get_bits((x), BIT(31))
> +#define NUM_ELEMS(x) le32_get_bits((x), GENMASK(15, 0))
> +
> +#define REMAINING(x) le32_get_bits((x), GENMASK(31, 16))
> +#define RETURNED(x) le32_get_bits((x), GENMASK(11, 0))
> +
> +#define CONFIG_FLAG_MASK GENMASK(19, 18)
> +#define SELECTOR_MASK GENMASK(17, 16)
> +#define SKIP_CONFIGS_MASK GENMASK(15, 8)
> +#define CONFIG_TYPE_MASK GENMASK(7, 0)
> +
> +enum scmi_pinctrl_protocol_cmd {
> + PINCTRL_ATTRIBUTES = 0x3,
> + PINCTRL_LIST_ASSOCIATIONS = 0x4,
> + PINCTRL_SETTINGS_GET = 0x5,
> + PINCTRL_SETTINGS_CONFIGURE = 0x6,
> + PINCTRL_REQUEST = 0x7,
> + PINCTRL_RELEASE = 0x8,
> + PINCTRL_NAME_GET = 0x9,
> + PINCTRL_SET_PERMISSIONS = 0xa
> +};
> +
> +struct scmi_msg_settings_conf {
> + __le32 identifier;
> + __le32 function_id;
> + __le32 attributes;
> + __le32 configs[];
> +};
> +
> +struct scmi_msg_settings_get {
> + __le32 identifier;
> + __le32 attributes;
> +};
> +
> +struct scmi_resp_settings_get {
> + __le32 function_selected;
> + __le32 num_configs;
> + __le32 configs[];
> +};
> +
> +struct scmi_msg_pinctrl_protocol_attributes {
> + __le32 attributes_low;
> + __le32 attributes_high;
> +};
> +
> +struct scmi_msg_pinctrl_attributes {
> + __le32 identifier;
> + __le32 flags;
> +};
> +
> +struct scmi_resp_pinctrl_attributes {
> + __le32 attributes;
> + u8 name[SCMI_SHORT_NAME_MAX_SIZE];
> +};
> +
> +struct scmi_msg_pinctrl_list_assoc {
> + __le32 identifier;
> + __le32 flags;
> + __le32 index;
> +};
> +
> +struct scmi_resp_pinctrl_list_assoc {
> + __le32 flags;
> + __le16 array[];
> +};
> +
> +struct scmi_msg_func_set {
> + __le32 identifier;
> + __le32 function_id;
> + __le32 flags;
> +};
> +
As said by Dan...drop this.
> +struct scmi_msg_request {
> + __le32 identifier;
> + __le32 flags;
> +};
> +
> +struct scmi_group_info {
> + char name[SCMI_MAX_STR_SIZE];
> + bool present;
> + u32 *group_pins;
> + u32 nr_pins;
> +};
> +
> +struct scmi_function_info {
> + char name[SCMI_MAX_STR_SIZE];
> + bool present;
> + u32 *groups;
> + u32 nr_groups;
> +};
> +
> +struct scmi_pin_info {
> + char name[SCMI_MAX_STR_SIZE];
> + bool present;
> +};
> +
> +struct scmi_pinctrl_info {
> + u32 version;
> + int nr_groups;
> + int nr_functions;
> + int nr_pins;
> + struct scmi_group_info *groups;
> + struct scmi_function_info *functions;
> + struct scmi_pin_info *pins;
> +};
> +
> +static int scmi_pinctrl_attributes_get(const struct scmi_protocol_handle *ph,
> + struct scmi_pinctrl_info *pi)
> +{
> + int ret;
> + struct scmi_xfer *t;
> + struct scmi_msg_pinctrl_protocol_attributes *attr;
> +
> + ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0, sizeof(*attr),
> + &t);
> + if (ret)
> + return ret;
> +
> + attr = t->rx.buf;
> +
> + ret = ph->xops->do_xfer(ph, t);
> + if (!ret) {
> + pi->nr_functions = GET_FUNCTIONS_NR(attr->attributes_high);
> + pi->nr_groups = GET_GROUPS_NR(attr->attributes_low);
> + pi->nr_pins = GET_PINS_NR(attr->attributes_low);
I was thinking, does make sense to allow a nr_pins == 0 setup to probe
successfully ? Becasuse is legit for the platform to return zero groups or
zero functions BUT zero pins is just useless (spec does not say
anything)
Maybe you could just put a dev_warn() here on if (nr_pins == 0) and bail
out with -EINVAL...
On the other side looking at the zero groups/function case, that is
plausible and handled properly by the driver since a 0-bytes
devm_kcalloc will return ZERO_SIZE_PTR (not NULL) and all the remaining
references to pinfo->groups and pinfo->functions are guarded by a check
on selector >= nr_groups (or >= nr_functions), and by scmi_pinctrl_validate_id()
so the zero grouyps/fuctions scenarios should be safely handled.
> + }
> +
> + ph->xops->xfer_put(ph, t);
> + return ret;
> +}
> +
> +static int scmi_pinctrl_count_get(const struct scmi_protocol_handle *ph,
> + enum scmi_pinctrl_selector_type type)
> +{
> + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> +
> + switch (type) {
> + case PIN_TYPE:
> + return pi->nr_pins;
> + case GROUP_TYPE:
> + return pi->nr_groups;
> + case FUNCTION_TYPE:
> + return pi->nr_functions;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int scmi_pinctrl_validate_id(const struct scmi_protocol_handle *ph,
> + u32 identifier,
> + enum scmi_pinctrl_selector_type type)
> +{
> + int value;
> +
> + value = scmi_pinctrl_count_get(ph, type);
> + if (value < 0)
> + return value;
> +
> + if (identifier >= value)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int scmi_pinctrl_attributes(const struct scmi_protocol_handle *ph,
> + enum scmi_pinctrl_selector_type type,
> + u32 selector, char *name,
> + u32 *n_elems)
> +{
> + int ret;
> + struct scmi_xfer *t;
> + struct scmi_msg_pinctrl_attributes *tx;
> + struct scmi_resp_pinctrl_attributes *rx;
> + u32 ext_name_flag;
what about a bool
> +
> + if (!name)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_validate_id(ph, selector, type);
> + if (ret)
> + return ret;
> +
> + ret = ph->xops->xfer_get_init(ph, PINCTRL_ATTRIBUTES, sizeof(*tx),
> + sizeof(*rx), &t);
> + if (ret)
> + return ret;
> +
> + tx = t->tx.buf;
> + rx = t->rx.buf;
> + tx->identifier = cpu_to_le32(selector);
> + tx->flags = cpu_to_le32(type);
> +
> + ret = ph->xops->do_xfer(ph, t);
> + if (!ret) {
> + if (n_elems)
> + *n_elems = NUM_ELEMS(rx->attributes);
> +
> + strscpy(name, rx->name, SCMI_SHORT_NAME_MAX_SIZE);
> +
> + ext_name_flag = EXT_NAME_FLAG(rx->attributes);
> + } else
> + ext_name_flag = 0;
and you dont need this else branch to set ext_name_flag to false, since down
below you will check ext_flag ONLY if !ret, so it will have surely been
set if the do_xfer did not fail.
> +
> + ph->xops->xfer_put(ph, t);
> +
> + /*
> + * If supported overwrite short name with the extended one;
> + * on error just carry on and use already provided short name.
> + */
> + if (!ret && ext_name_flag)
> + ph->hops->extended_name_get(ph, PINCTRL_NAME_GET, selector,
> + (u32 *)&type, name,
> + SCMI_MAX_STR_SIZE);
> + return ret;
> +}
> +
> +struct scmi_pinctrl_ipriv {
> + u32 selector;
> + enum scmi_pinctrl_selector_type type;
> + u32 *array;
> +};
> +
> +static void iter_pinctrl_assoc_prepare_message(void *message,
> + u32 desc_index,
> + const void *priv)
> +{
> + struct scmi_msg_pinctrl_list_assoc *msg = message;
> + const struct scmi_pinctrl_ipriv *p = priv;
> +
> + msg->identifier = cpu_to_le32(p->selector);
> + msg->flags = cpu_to_le32(p->type);
> + /* Set the number of OPPs to be skipped/already read */
OPP ? .. maybe drop this comment that was cut/pasted from somewhere else :D
> + msg->index = cpu_to_le32(desc_index);
> +}
> +
> +static int iter_pinctrl_assoc_update_state(struct scmi_iterator_state *st,
> + const void *response, void *priv)
> +{
> + const struct scmi_resp_pinctrl_list_assoc *r = response;
> +
> + st->num_returned = RETURNED(r->flags);
> + st->num_remaining = REMAINING(r->flags);
> +
> + return 0;
> +}
> +
> +static int
> +iter_pinctrl_assoc_process_response(const struct scmi_protocol_handle *ph,
> + const void *response,
> + struct scmi_iterator_state *st, void *priv)
> +{
> + const struct scmi_resp_pinctrl_list_assoc *r = response;
> + struct scmi_pinctrl_ipriv *p = priv;
> +
> + p->array[st->desc_index + st->loop_idx] =
> + le16_to_cpu(r->array[st->loop_idx]);
> +
> + return 0;
> +}
> +
> +static int scmi_pinctrl_list_associations(const struct scmi_protocol_handle *ph,
> + u32 selector,
> + enum scmi_pinctrl_selector_type type,
> + u16 size, u32 *array)
> +{
> + int ret;
> + void *iter;
> + struct scmi_iterator_ops ops = {
> + .prepare_message = iter_pinctrl_assoc_prepare_message,
> + .update_state = iter_pinctrl_assoc_update_state,
> + .process_response = iter_pinctrl_assoc_process_response,
> + };
> + struct scmi_pinctrl_ipriv ipriv = {
> + .selector = selector,
> + .type = type,
> + .array = array,
> + };
> +
> + if (!array || !size || type == PIN_TYPE)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_validate_id(ph, selector, type);
> + if (ret)
> + return ret;
> +
> + iter = ph->hops->iter_response_init(ph, &ops, size,
> + PINCTRL_LIST_ASSOCIATIONS,
> + sizeof(struct scmi_msg_pinctrl_list_assoc),
> + &ipriv);
> +
> + if (IS_ERR(iter))
> + return PTR_ERR(iter);
> +
> + return ph->hops->iter_response_run(iter);
> +}
> +
> +struct scmi_settings_get_ipriv {
> + u32 selector;
> + enum scmi_pinctrl_selector_type type;
> + u32 flag;
> + enum scmi_pinctrl_conf_type *config_types;
> + u32 *config_values;
> +};
> +
> +static void
> +iter_pinctrl_settings_get_prepare_message(void *message, u32 desc_index,
> + const void *priv)
> +{
> + struct scmi_msg_settings_get *msg = message;
> + const struct scmi_settings_get_ipriv *p = priv;
> + u32 attributes;
> +
> + attributes = FIELD_PREP(CONFIG_FLAG_MASK, p->flag) |
> + FIELD_PREP(SELECTOR_MASK, p->type);
> +
> + if (p->flag == 1)
A boolean like .get_all would be more clear..see down below why you dont need
a flag 0|1|2
> + attributes |= FIELD_PREP(SKIP_CONFIGS_MASK, desc_index);
> + else if (!p->flag)
> + attributes |= FIELD_PREP(CONFIG_TYPE_MASK, p->config_types[0]);
> +
> + msg->attributes = cpu_to_le32(attributes);
> + msg->identifier = cpu_to_le32(p->selector);
> +}
> +
> +static int
> +iter_pinctrl_settings_get_update_state(struct scmi_iterator_state *st,
> + const void *response, void *priv)
> +{
> + const struct scmi_resp_settings_get *r = response;
> + struct scmi_settings_get_ipriv *p = priv;
> +
> + if (p->flag == 1) {
Ditto... see below the explanation
> + st->num_returned = le32_get_bits(r->num_configs, GENMASK(7, 0));
> + st->num_remaining = le32_get_bits(r->num_configs,
> + GENMASK(31, 24));
> + } else {
> + st->num_returned = 1;
> + st->num_remaining = 0;
> + }
> +
> + return 0;
> +}
> +
> +static int
> +iter_pinctrl_settings_get_process_response(const struct scmi_protocol_handle *ph,
> + const void *response,
> + struct scmi_iterator_state *st,
> + void *priv)
> +{
> + const struct scmi_resp_settings_get *r = response;
> + struct scmi_settings_get_ipriv *p = priv;
> +
> + if (!p->flag) {
> + if (p->config_types[0] !=
> + le32_get_bits(r->configs[st->loop_idx * 2], GENMASK(7, 0)))
> + return -EINVAL;
> + } else if (p->flag == 1) {
> + p->config_types[st->desc_index + st->loop_idx] =
> + le32_get_bits(r->configs[st->loop_idx * 2],
> + GENMASK(7, 0));
> + } else if (p->flag == 2) {
> + return 0;
> + }
Unneeded...see down below for explanation
> +
> + p->config_values[st->desc_index + st->loop_idx] =
> + le32_to_cpu(r->configs[st->loop_idx * 2 + 1]);
> +
> + return 0;
> +}
> +
> +static int
> +scmi_pinctrl_settings_get(const struct scmi_protocol_handle *ph, u32 selector,
> + enum scmi_pinctrl_selector_type type,
> + enum scmi_pinctrl_conf_type config_type,
> + u32 *config_value)
> +{
> + int ret;
> + void *iter;
> + struct scmi_iterator_ops ops = {
> + .prepare_message = iter_pinctrl_settings_get_prepare_message,
> + .update_state = iter_pinctrl_settings_get_update_state,
> + .process_response = iter_pinctrl_settings_get_process_response,
> + };
> + struct scmi_settings_get_ipriv ipriv = {
> + .selector = selector,
> + .type = type,
> + .flag = 0,
> + .config_types = &config_type,
> + .config_values = config_value,
> + };
> +
So this function is used to retrieve configs; as of now, just one, then
it could be extended to fetch all the configs, and for this it uses the
iterators helpers, BUT it is not and will not be used to just fetch the
selected_function with flag_2 (even though is always provided), since in
that case you wont get back a multi-part SCMI response and so there is no
need to use iterators...
IOW... no need here to handle flag_2 scenario and as a consequence I would
change the ipriv flag to be be a boolean .get_all, like it was, since it is
more readable (and so you wont need to add any comment..)
In the future could make sense to add here also a *selected_function output
param to this function since you will always get it back for free when
retrieving configs ... BUT for now is just not needed really...no users
for this case till now...
...when the time will come that we will need a function_selected_get to
be issued without retrieveing also the configs I would add a distinct
routine that crafts properly a SETTINGS_GET with flag_2 without worrying
about multi-part responses (and with no need for iterators support)
Trying to handle all in here just complicates stuff...
> + if (!config_value || type == FUNCTION_TYPE)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_validate_id(ph, selector, type);
> + if (ret)
> + return ret;
> +
> + iter = ph->hops->iter_response_init(ph, &ops, 1, PINCTRL_SETTINGS_GET,
> + sizeof(struct scmi_msg_settings_get),
> + &ipriv);
> +
> + if (IS_ERR(iter))
> + return PTR_ERR(iter);
> +
> + return ph->hops->iter_response_run(iter);
> +}
> +
> +static int
> +scmi_pinctrl_settings_conf(const struct scmi_protocol_handle *ph,
> + u32 selector,
> + enum scmi_pinctrl_selector_type type,
> + u32 nr_configs,
> + enum scmi_pinctrl_conf_type *config_type,
> + u32 *config_value)
> +{
> + struct scmi_xfer *t;
> + struct scmi_msg_settings_conf *tx;
> + u32 attributes;
> + int ret, i;
> + u32 configs_in_chunk, conf_num = 0;
> + u32 chunk;
> + int max_msg_size = ph->hops->get_max_msg_size(ph);
> +
> + if (!config_type || !config_value || type == FUNCTION_TYPE)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_validate_id(ph, selector, type);
> + if (ret)
> + return ret;
> +
> + configs_in_chunk = (max_msg_size - sizeof(*tx)) / (sizeof(__le32) * 2);
> + while (conf_num < nr_configs) {
> + chunk = (nr_configs - conf_num > configs_in_chunk) ?
> + configs_in_chunk : nr_configs - conf_num;
> +
> + ret = ph->xops->xfer_get_init(ph, PINCTRL_SETTINGS_CONFIGURE,
> + sizeof(*tx) +
> + chunk * 2 * sizeof(__le32),
> + 0, &t);
> + if (ret)
> + return ret;
for consistency I would
break;
like below and you will exit always from the last return ret;
> +
> + tx = t->tx.buf;
> + tx->identifier = cpu_to_le32(selector);
> + attributes = FIELD_PREP(GENMASK(1, 0), type) |
> + FIELD_PREP(GENMASK(9, 2), chunk);
> + tx->attributes = cpu_to_le32(attributes);
> +
> + for (i = 0; i < chunk; i++) {
> + tx->configs[i * 2] =
> + cpu_to_le32(config_type[conf_num + i]);
> + tx->configs[i * 2 + 1] =
> + cpu_to_le32(config_value[conf_num + i]);
> + }
> +
> + ret = ph->xops->do_xfer(ph, t);
> +
> + ph->xops->xfer_put(ph, t);
> +
> + if (ret)
> + break;
> +
> + conf_num += chunk;
> + }
> +
> + return ret;
> +}
> +
> +static int scmi_pinctrl_function_select(const struct scmi_protocol_handle *ph,
> + u32 group,
> + enum scmi_pinctrl_selector_type type,
> + u32 function_id)
> +{
> + int ret;
> + struct scmi_xfer *t;
> + struct scmi_msg_settings_conf *tx;
> + u32 attributes;
> +
> + ret = scmi_pinctrl_validate_id(ph, group, type);
> + if (ret)
> + return ret;
> +
> + ret = ph->xops->xfer_get_init(ph, PINCTRL_SETTINGS_CONFIGURE,
> + sizeof(*tx), 0, &t);
> + if (ret)
> + return ret;
> +
> + tx = t->tx.buf;
> + tx->identifier = cpu_to_le32(group);
> + tx->function_id = cpu_to_le32(function_id);
> + attributes = FIELD_PREP(GENMASK(1, 0), type) | BIT(10);
> + tx->attributes = cpu_to_le32(attributes);
> +
> + ret = ph->xops->do_xfer(ph, t);
> + ph->xops->xfer_put(ph, t);
> +
> + return ret;
> +}
> +
> +static int scmi_pinctrl_request(const struct scmi_protocol_handle *ph,
> + u32 identifier,
> + enum scmi_pinctrl_selector_type type)
> +{
> + int ret;
> + struct scmi_xfer *t;
> + struct scmi_msg_request *tx;
> +
> + if (type == FUNCTION_TYPE)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_validate_id(ph, identifier, type);
> + if (ret)
> + return ret;
> +
> + ret = ph->xops->xfer_get_init(ph, PINCTRL_REQUEST, sizeof(*tx), 0, &t);
> + if (ret)
> + return ret;
> +
> + tx = t->tx.buf;
> + tx->identifier = cpu_to_le32(identifier);
> + tx->flags = cpu_to_le32(type);
> +
> + ret = ph->xops->do_xfer(ph, t);
> + ph->xops->xfer_put(ph, t);
> +
> + return ret;
> +}
> +
..this function ...
> +static int scmi_pinctrl_pin_request(const struct scmi_protocol_handle *ph,
> + u32 pin)
> +{
> + return scmi_pinctrl_request(ph, pin, PIN_TYPE);
> +}
> +
> +static int scmi_pinctrl_free(const struct scmi_protocol_handle *ph,
> + u32 identifier,
> + enum scmi_pinctrl_selector_type type)
> +{
> + int ret;
> + struct scmi_xfer *t;
> + struct scmi_msg_request *tx;
> +
> + if (type == FUNCTION_TYPE)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_validate_id(ph, identifier, type);
> + if (ret)
> + return ret;
> +
> + ret = ph->xops->xfer_get_init(ph, PINCTRL_RELEASE, sizeof(*tx), 0, &t);
> + if (ret)
> + return ret;
> +
> + tx = t->tx.buf;
> + tx->identifier = cpu_to_le32(identifier);
> + tx->flags = cpu_to_le32(type);
> +
> + ret = ph->xops->do_xfer(ph, t);
> + ph->xops->xfer_put(ph, t);
> +
> + return ret;
> +}
> +
...and this are completely identical, beside the used command msg_id...please make
it a common workhorse function by adding a param for the command...
> +static int scmi_pinctrl_pin_free(const struct scmi_protocol_handle *ph, u32 pin)
> +{
> + return scmi_pinctrl_free(ph, pin, PIN_TYPE);
> +}
> +
...and convert these _request/_free functions into a pair odf simple wrapper invoking
the common workhorse...
> +static int scmi_pinctrl_get_group_info(const struct scmi_protocol_handle *ph,
> + u32 selector,
> + struct scmi_group_info *group)
> +{
> + int ret;
> +
> + if (!group)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_attributes(ph, GROUP_TYPE, selector,
> + group->name,
> + &group->nr_pins);
> + if (ret)
> + return ret;
> +
> + if (!group->nr_pins) {
> + dev_err(ph->dev, "Group %d has 0 elements", selector);
> + return -ENODATA;
> + }
> +
> + group->group_pins = kmalloc_array(group->nr_pins,
> + sizeof(*group->group_pins),
> + GFP_KERNEL);
> + if (!group->group_pins)
> + return -ENOMEM;
> +
> + ret = scmi_pinctrl_list_associations(ph, selector, GROUP_TYPE,
> + group->nr_pins, group->group_pins);
> + if (ret) {
> + kfree(group->group_pins);
> + return ret;
> + }
> +
> + group->present = true;
> + return 0;
> +}
> +
> +static int scmi_pinctrl_get_group_name(const struct scmi_protocol_handle *ph,
> + u32 selector, const char **name)
> +{
> + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> +
> + if (!name)
> + return -EINVAL;
> +
> + if (selector >= pi->nr_groups)
> + return -EINVAL;
> +
> + if (!pi->groups[selector].present) {
> + int ret;
> +
> + ret = scmi_pinctrl_get_group_info(ph, selector,
> + &pi->groups[selector]);
> + if (ret)
> + return ret;
> + }
> +
> + *name = pi->groups[selector].name;
> +
> + return 0;
> +}
> +
> +static int scmi_pinctrl_group_pins_get(const struct scmi_protocol_handle *ph,
> + u32 selector, const u32 **pins,
> + u32 *nr_pins)
> +{
> + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> +
> + if (!pins || !nr_pins)
> + return -EINVAL;
> +
> + if (selector >= pi->nr_groups)
> + return -EINVAL;
> +
> + if (!pi->groups[selector].present) {
> + int ret;
> +
> + ret = scmi_pinctrl_get_group_info(ph, selector,
> + &pi->groups[selector]);
> + if (ret)
> + return ret;
> + }
> +
> + *pins = pi->groups[selector].group_pins;
> + *nr_pins = pi->groups[selector].nr_pins;
> +
> + return 0;
> +}
> +
> +static int scmi_pinctrl_get_function_info(const struct scmi_protocol_handle *ph,
> + u32 selector,
> + struct scmi_function_info *func)
> +{
> + int ret;
> +
> + if (!func)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_attributes(ph, FUNCTION_TYPE, selector,
> + func->name,
> + &func->nr_groups);
> + if (ret)
> + return ret;
> +
> + if (!func->nr_groups) {
> + dev_err(ph->dev, "Function %d has 0 elements", selector);
> + return -ENODATA;
> + }
> +
> + func->groups = kmalloc_array(func->nr_groups, sizeof(*func->groups),
> + GFP_KERNEL);
> + if (!func->groups)
> + return -ENOMEM;
> +
> + ret = scmi_pinctrl_list_associations(ph, selector, FUNCTION_TYPE,
> + func->nr_groups, func->groups);
> + if (ret) {
> + kfree(func->groups);
> + return ret;
> + }
> +
> + func->present = true;
> + return 0;
> +}
> +
> +static int scmi_pinctrl_get_function_name(const struct scmi_protocol_handle *ph,
> + u32 selector, const char **name)
> +{
> + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> +
> + if (!name)
> + return -EINVAL;
> +
> + if (selector >= pi->nr_functions)
> + return -EINVAL;
> +
> + if (!pi->functions[selector].present) {
> + int ret;
> +
> + ret = scmi_pinctrl_get_function_info(ph, selector,
> + &pi->functions[selector]);
> + if (ret)
> + return ret;
> + }
> +
> + *name = pi->functions[selector].name;
> + return 0;
> +}
> +
> +static int
> +scmi_pinctrl_function_groups_get(const struct scmi_protocol_handle *ph,
> + u32 selector, u32 *nr_groups,
> + const u32 **groups)
> +{
> + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> +
> + if (!groups || !nr_groups)
> + return -EINVAL;
> +
> + if (selector >= pi->nr_functions)
> + return -EINVAL;
> +
> + if (!pi->functions[selector].present) {
> + int ret;
> +
> + ret = scmi_pinctrl_get_function_info(ph, selector,
> + &pi->functions[selector]);
> + if (ret)
> + return ret;
> + }
> +
> + *groups = pi->functions[selector].groups;
> + *nr_groups = pi->functions[selector].nr_groups;
> +
> + return 0;
> +}
> +
> +static int scmi_pinctrl_mux_set(const struct scmi_protocol_handle *ph,
> + u32 selector, u32 group)
> +{
> + return scmi_pinctrl_function_select(ph, group, GROUP_TYPE, selector);
> +}
> +
> +static int scmi_pinctrl_get_pin_info(const struct scmi_protocol_handle *ph,
> + u32 selector, struct scmi_pin_info *pin)
> +{
> + int ret;
> +
> + if (!pin)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_attributes(ph, PIN_TYPE, selector,
> + pin->name, NULL);
> + if (ret)
> + return ret;
> +
> + pin->present = true;
> + return 0;
> +}
> +
> +static int scmi_pinctrl_get_pin_name(const struct scmi_protocol_handle *ph,
> + u32 selector, const char **name)
> +{
> + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> +
> + if (!name)
> + return -EINVAL;
> +
> + if (selector >= pi->nr_pins)
> + return -EINVAL;
> +
> + if (!pi->pins[selector].present) {
> + int ret;
> +
> + ret = scmi_pinctrl_get_pin_info(ph, selector,
> + &pi->pins[selector]);
> + if (ret)
> + return ret;
> + }
> +
> + *name = pi->pins[selector].name;
> +
> + return 0;
> +}
> +
> +static int scmi_pinctrl_name_get(const struct scmi_protocol_handle *ph,
> + u32 selector,
> + enum scmi_pinctrl_selector_type type,
> + const char **name)
> +{
> + switch (type) {
> + case PIN_TYPE:
> + return scmi_pinctrl_get_pin_name(ph, selector, name);
> + case GROUP_TYPE:
> + return scmi_pinctrl_get_group_name(ph, selector, name);
> + case FUNCTION_TYPE:
> + return scmi_pinctrl_get_function_name(ph, selector, name);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static const struct scmi_pinctrl_proto_ops pinctrl_proto_ops = {
> + .count_get = scmi_pinctrl_count_get,
> + .name_get = scmi_pinctrl_name_get,
> + .group_pins_get = scmi_pinctrl_group_pins_get,
> + .function_groups_get = scmi_pinctrl_function_groups_get,
> + .mux_set = scmi_pinctrl_mux_set,
> + .settings_get = scmi_pinctrl_settings_get,
> + .settings_conf = scmi_pinctrl_settings_conf,
> + .pin_request = scmi_pinctrl_pin_request,
> + .pin_free = scmi_pinctrl_pin_free,
> +};
> +
> +static int scmi_pinctrl_protocol_init(const struct scmi_protocol_handle *ph)
> +{
> + int ret;
> + u32 version;
> + struct scmi_pinctrl_info *pinfo;
> +
> + ret = ph->xops->version_get(ph, &version);
> + if (ret)
> + return ret;
> +
> + dev_dbg(ph->dev, "Pinctrl Version %d.%d\n",
> + PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
> +
> + pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL);
> + if (!pinfo)
> + return -ENOMEM;
> +
> + ret = scmi_pinctrl_attributes_get(ph, pinfo);
> + if (ret)
> + return ret;
..as a I was saying is nr_pins == 0 the scmi_pinctrl_attributes_get
could return -EINVAL here and bail out....not sure that a running setup
with zero pins has any values (even for testing...) BUT, as said above,
I wuld certainly add a dev_warn in scmi_pinctrl_attributes_get() when
nr_pins == 0
Thanks,
Cristian
^ permalink raw reply
* Re: [PATCH 08/10] iio: backend: add new functionality
From: Nuno Sá @ 2024-03-28 15:42 UTC (permalink / raw)
To: Jonathan Cameron, Nuno Sa via B4 Relay
Cc: nuno.sa, linux-iio, devicetree, Dragos Bogdan, Lars-Peter Clausen,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Olivier Moysan
In-Reply-To: <20240328151632.298bd95f@jic23-huawei>
On Thu, 2024-03-28 at 15:16 +0000, Jonathan Cameron wrote:
> On Thu, 28 Mar 2024 14:22:32 +0100
> Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
>
> > From: Nuno Sa <nuno.sa@analog.com>
> >
> > This adds the needed backend ops for supporting a backend inerfacing
> > with an high speed dac. The new ops are:
> >
> > * data_source_set();
> > * set_sampling_freq();
> > * extend_chan_spec();
> > * ext_info_set();
> > * ext_info_get().
> >
> > Also to note the new helpers that are meant to be used by the backends
> > when extending an IIO channel (adding extended info):
> >
> > * iio_backend_ext_info_set();
> > * iio_backend_ext_info_get().
> >
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> I'm pretty flexible on this so far as I think we are still learning how front
> ends and backends should interact. Maybe we'll figure that out in the medium
> term and rework this stuff. For now it looks fine. A few minor things inline.
> >
> > +/**
> > + * iio_backend_ext_info_get - IIO ext_info read callback
> > + * @indio_dev: IIO device
> > + * @private: Data private to the driver
> > + * @chan: IIO channel
> > + * @buf: Buffer where to place the attribute data
> > + *
> > + * This helper is intended to be used by backends that extend an IIO channel
> > + * (trough iio_backend_extend_chan_spec()) with extended info. In that case,
> > + * backends are not supposed to give their own callbacks (as they would not
> > + * a way to get te backend from indio_dev). This is the getter.
>
> te->the?
Yes and some more typos :).
>
>
> > +/**
> > + * iio_backend_extend_chan_spec - Extend an IIO channel
> > + * @indio_dev: IIO device
> > + * @back: Backend device
> > + * @chan: IIO channel
> > + *
> > + * Some backends may have their own functionalities and hence capable of
> > + * extending a frontend's channel.
> > + *
> > + * RETURNS:
> > + * 0 on success, negative error number on failure.
> > + */
> > +int iio_backend_extend_chan_spec(struct iio_dev *indio_dev,
> > + struct iio_backend *back,
> > + struct iio_chan_spec *chan)
> > +{
> > + const struct iio_chan_spec_ext_info *ext_info = chan->ext_info;
> This is getting confusing. So this one is the front end value of ext_info?
> Name it as such frontend_ext_info
Yes, it's the frontend pointer. Just to enforce the below constrain. Will rename as
suggested.
>
> > + int ret;
> > +
> > + ret = iio_backend_op_call(back, extend_chan_spec, chan);
> > + if (ret)
> > + return ret;
> > + /*
> > + * Let's keep things simple for now. Don't allow to overwrite the
> > + * frontend's extended info. If ever needed, we can support appending
> > + * it.
> > + */
> > + if (ext_info && chan->ext_info != ext_info)
> > + return -EOPNOTSUPP;
> > + if (!chan->ext_info)
>
> This is checking if the backend added anything? Perhaps a comment on that
> as we don't need a backend_ext_info local variable...
Yes, but just regarding ext_info as that is the only thing we're handling below
(doing some sanity checks). Note that (as you said above) I'm keeping things a bit
"open" in that the backend is open to extend whatever it wants. With time we may
learn better what do we want constrain or not.
>
> > + return 0;
> > + /*
> > + * !\NOTE: this will break as soon as we have multiple backends on one
> > + * frontend and all of them extend channels. In that case, the core
> > + * backend code has no way to get the correct backend given the
> > + * iio device.
> > + *
> > + * One solution for this could be introducing a new backend
> > + * dedicated callback in struct iio_info so we can callback into the
> > + * frontend so it can give us the right backend given a chan_spec.
> > + */
>
> Hmm. This is indeed messy. Could we associate it with the buffer as presuably
> a front end with multiple backends is using multiple IIO buffers?
>
Hmm, the assumption of having multiple buffers seems plausible to me but considering
the example we have in hands it would be cumbersome to get the backend. Considering
iio_backend_ext_info_get(), how could we get the backend if it was associated to one
of the IIO buffers? I think we would need more "intrusive" changes to make that work
or do you have something in mind=
> As you say a dance via the front end would work fine.
I'm happy you're also open for a proper solution already. I mention this in the
cover. My idea was something like (consider the iio_backend_ext_info_get()):
if (!indio_dev->info->get_iio_backend())
return -EOPNOTSUPP;
back = indio_dev->info->get_iio_backend(indio_dev, chan_spec);
It would be nice to have some "default/generic" implementation for cases where we
only have one backend per frontend so that the frontend would not need to define the
callback.
>
>
> > + iio_device_set_drvdata(indio_dev, back);
> > +
> > + /* Don't allow backends to get creative and force their own handlers */
> > + for (ext_info = chan->ext_info; ext_info->name; ext_info++) {
> > + if (ext_info->read != iio_backend_ext_info_get)
> > + return -EINVAL;
> > + if (ext_info->write != iio_backend_ext_info_set)
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(iio_backend_extend_chan_spec, IIO_BACKEND);
>
> > diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h
> > index a6d79381866e..09ff2f8f9fd8 100644
> > --- a/include/linux/iio/backend.h
> > +++ b/include/linux/iio/backend.h
> > @@ -4,6 +4,7 @@
> >
> > #include <linux/types.h>
> >
> > +struct iio_chan_spec;
> > struct fwnode_handle;
> > struct iio_backend;
> > struct device;
> > @@ -15,6 +16,26 @@ enum iio_backend_data_type {
> > IIO_BACKEND_DATA_TYPE_MAX
> > };
> >
> > +enum iio_backend_data_source {
> > + IIO_BACKEND_INTERNAL_CW,
>
> CW? Either expand out what ever that is in definition of add a comment
> at least.
Continuous wave :)
>
> > + IIO_BACKEND_EXTERNAL,
> What does external mean in this case?
In this particular case comes from a DMA source (IP). I thought external to be more
generic but if you prefer, I can do something like IIO_BACKEND_DMA?
> > + IIO_BACKEND_DATA_SOURCE_MAX
> > +};
> > +
> > +/**
> > + * IIO_BACKEND_EX_INFO - Helper for an IIO extended channel attribute
> > + * @_name: Attribute name
> > + * @_shared: Whether the attribute is shared between all channels
> > + * @_what: Data private to the driver
> > + */
> > +#define IIO_BACKEND_EX_INFO(_name, _shared, _what) { \
> > + .name = (_name), \
> > + .shared = (_shared), \
> > + .read = iio_backend_ext_info_get, \
> > + .write = iio_backend_ext_info_set, \
> > + .private = (_what), \
> > +}
> > +
> > /**
> > * struct iio_backend_data_fmt - Backend data format
> > * @type: Data type.
> > @@ -35,8 +56,13 @@ struct iio_backend_data_fmt {
> > * @chan_enable: Enable one channel.
> > * @chan_disable: Disable one channel.
> > * @data_format_set: Configure the data format for a specific channel.
> > + * @data_source_set: Configure the data source for a specific channel.
> > + * @set_sample_rate: Configure the sampling rate for a specific channel.
> > * @request_buffer: Request an IIO buffer.
> > * @free_buffer: Free an IIO buffer.
> > + * @extend_chan_spec: Extend an IIO channel.
> > + * @ext_info_set: Extended info setter.
> > + * @ext_info_get: Extended info getter.
> > **/
> > struct iio_backend_ops {
> > int (*enable)(struct iio_backend *back);
> > @@ -45,10 +71,21 @@ struct iio_backend_ops {
> > int (*chan_disable)(struct iio_backend *back, unsigned int chan);
> > int (*data_format_set)(struct iio_backend *back, unsigned int chan,
> > const struct iio_backend_data_fmt *data);
> > + int (*data_source_set)(struct iio_backend *back, unsigned int chan,
> > + enum iio_backend_data_source data);
> > + int (*set_sample_rate)(struct iio_backend *back, unsigned int chan,
> > + u64 sample_rate);
>
> Name the parameter that so we know the units. _hz?
yes. And u64 to not fall in the CCF problem for 32bits :)
- Nuno Sá
^ permalink raw reply
* Re: [PATCH] dt-bindings: arm: qcom: Add Samsung Galaxy Z Fold5
From: Alexandru Serdeliuc @ 2024-03-28 15:42 UTC (permalink / raw)
To: Konrad Dybcio, Bjorn Andersson, Rob Herring, Krzysztof Kozlowski,
Conor Dooley
Cc: linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <6b77633e-c501-4488-9b10-1881cfbf6f2c@linaro.org>
Is there anything I need to do to sent them as a series? Or only modify
all 3 files on a new branch and send them as a new patch?
The situation can be replicated easy, modify the following two files
Documentation/devicetree/bindings/arm/qcom.yaml
arch/arm64/boot/dts/qcom/Makefile
Then generate the pach and check, will show this
$ ./scripts/checkpatch.pl
/tmp/tosend2/0001-arm64-dts-qcom-add-support-for-samsung-galaxy-z-fold5.eml
WARNING: Missing commit description - Add an appropriate one
WARNING: DT binding docs and includes should be a separate patch. See:
Documentation/devicetree/bindings/submitting-patches.rst
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#59:
new file mode 100644
total: 0 errors, 3 warnings, 630 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or
--fix-inplace.
/tmp/tosend2/0001-arm64-dts-qcom-add-support-for-samsung-galaxy-z-fold5.eml
has style problems, please review.
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
On 28/3/24 16:34, Konrad Dybcio wrote:
> On 28.03.2024 4:10 PM, Alexandru Serdeliuc wrote:
>> Hi Konrad,
>>
>> Thanks, I unfortunately sent the patch 2 prior seeing your reply.
>>
>> The warning was this one which says that i need to send the mods separately in two patches:
>>
>>>>> WARNING: DT binding docs and includes should be a separate patch. See: Documentation/devicetree/bindings/submitting-patches.rst
> Hm, if they were two separate patches, this is a false-positive. Could you
> push the branch somewhere, so that we can report it to checkpatch maintainers?
>
>>
>> I suppose that me sending two separate patches was not good, how i can fix this?
> Please pick them both onto a single branch and send together as a series,
> with a revision bump (v2) and mention that you made no changes other than
> combining the two in the cover letter.
>
> Konrad
^ permalink raw reply
* Re: [PATCH] dt-bindings: arm: qcom: Add Samsung Galaxy Z Fold5
From: Alexandru Serdeliuc @ 2024-03-28 15:10 UTC (permalink / raw)
To: Konrad Dybcio, Bjorn Andersson, Rob Herring, Krzysztof Kozlowski,
Conor Dooley
Cc: linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <8e09b779-a18f-46b4-926c-40e2a5782d85@linaro.org>
Hi Konrad,
Thanks, I unfortunately sent the patch 2 prior seeing your reply.
The warning was this one which says that i need to send the mods
separately in two patches:
>>>
>>>WARNING: DT binding docs and includes should be a separate patch.
See: Documentation/devicetree/bindings/submitting-patches.rst
>>>
I suppose that me sending two separate patches was not good, how i can
fix this?
Best regards,
Marc
On 28/3/24 15:47, Konrad Dybcio wrote:
> On 28.03.2024 3:42 PM, Alexandru Serdeliuc wrote:
>> Hi Konrad,
>>
>>
>> Thanks, yes, I am new to b4 and sending patches, in a few minutes I will add the second patch.
>>
>> That actually add the device tree, but without the previous patch it showed me a warning, and with both patches provided another warning that i need to split them in two.
> Oh no, you should send them together! Could you please paste the warning so that
> we can work out the issue?
>
> Konrad
^ permalink raw reply
* Re: [PATCH v8 19/20] virt: geniezone: Add tracing support for hyp call and vcpu exit_reason
From: Steven Rostedt @ 2024-03-28 15:40 UTC (permalink / raw)
To: Yi-De Wu
Cc: Yingshiuan Pan, Ze-Yu Wang, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jonathan Corbet, Catalin Marinas, Wihl Deacon,
Masami Hiramatsu, Mathieu Desnoyers, Richard Cochran,
Matthias Brugger, AngeloGioacchino Del Regno, devicetree,
linux-kernel, linux-doc, linux-arm-kernel, linux-trace-kernel,
netdev, linux-mediatek, David Bradil, Trilok Soni, Jade Shih,
Ivan Tseng, My Chuang, Shawn Hsiao, PeiLun Suei, Liju Chen,
Willix Yeh, Kevenny Hsieh
In-Reply-To: <20231228105147.13752-20-yi-de.wu@mediatek.com>
On Thu, 28 Dec 2023 18:51:46 +0800
Yi-De Wu <yi-de.wu@mediatek.com> wrote:
> Add tracepoints for hypervisor calls and VCPU exit reasons in GenieZone
> driver. It aids performance debugging by providing more information
> about hypervisor operations and VCPU behavior.
>
> Command Usage:
> echo geniezone:* >> /sys/kernel/tracing/set_event
> echo 1 > /sys/kernel/tracing/tracing_on
> echo 0 > /sys/kernel/tracing/tracing_on
> cat /sys/kernel/tracing/trace
>
> For example:
> crosvm_vcpu0-4838 [004] ..... 76053.536034: mtk_hypcall_enter: id=0xbb001005
> crosvm_vcpu0-4838 [004] ..... 76053.540039: mtk_hypcall_leave: id=0xbb001005 invalid=0
> crosvm_vcpu0-4838 [004] ..... 76053.540040: mtk_vcpu_exit: vcpu exit_reason=0x92920003
Cleaning out patchwork, I noticed this patch.
You can make the above more informative by having it output:
crosvm_vcpu0-4838 [004] ..... 76053.540040: mtk_vcpu_exit: vcpu exit_reason=IRQ
>
> This example tracks a hypervisor function call by an ID (`0xbb001005`)
> from initiation to termination, which is supported (invalid=0). A vCPU
> exit is triggered by an Interrupt Request (IRQ) (exit reason: 0x92920003).
>
> /* VM exit reason */
> enum {
> GZVM_EXIT_UNKNOWN = 0x92920000,
> GZVM_EXIT_MMIO = 0x92920001,
> GZVM_EXIT_HYPERCALL = 0x92920002,
> GZVM_EXIT_IRQ = 0x92920003,
> GZVM_EXIT_EXCEPTION = 0x92920004,
> GZVM_EXIT_DEBUG = 0x92920005,
> GZVM_EXIT_FAIL_ENTRY = 0x92920006,
> GZVM_EXIT_INTERNAL_ERROR = 0x92920007,
> GZVM_EXIT_SYSTEM_EVENT = 0x92920008,
> GZVM_EXIT_SHUTDOWN = 0x92920009,
> GZVM_EXIT_GZ = 0x9292000a,
> };
>
> Signed-off-by: Liju-clr Chen <liju-clr.chen@mediatek.com>
> Signed-off-by: Yi-De Wu <yi-de.wu@mediatek.com>
> ---
> arch/arm64/geniezone/vm.c | 5 +++
> drivers/virt/geniezone/gzvm_vcpu.c | 3 ++
> include/trace/events/geniezone.h | 54 ++++++++++++++++++++++++++++++
> 3 files changed, 62 insertions(+)
> create mode 100644 include/trace/events/geniezone.h
>
> diff --git a/arch/arm64/geniezone/vm.c b/arch/arm64/geniezone/vm.c
> index a9d264bbb3b1..5667643251b5 100644
> --- a/arch/arm64/geniezone/vm.c
> +++ b/arch/arm64/geniezone/vm.c
> @@ -7,6 +7,8 @@
> #include <linux/err.h>
> #include <linux/uaccess.h>
>
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/geniezone.h>
> #include <linux/gzvm.h>
> #include <linux/gzvm_drv.h>
> #include "gzvm_arch_common.h"
> @@ -33,7 +35,10 @@ int gzvm_hypcall_wrapper(unsigned long a0, unsigned long a1,
> unsigned long a6, unsigned long a7,
> struct arm_smccc_res *res)
> {
> + trace_mtk_hypcall_enter(a0);
> arm_smccc_hvc(a0, a1, a2, a3, a4, a5, a6, a7, res);
> + trace_mtk_hypcall_leave(a0, (res->a0 != ERR_NOT_SUPPORTED) ? 0 : 1);
> +
> return gzvm_err_to_errno(res->a0);
> }
>
> diff --git a/drivers/virt/geniezone/gzvm_vcpu.c b/drivers/virt/geniezone/gzvm_vcpu.c
> index 86c690749277..138ec064596b 100644
> --- a/drivers/virt/geniezone/gzvm_vcpu.c
> +++ b/drivers/virt/geniezone/gzvm_vcpu.c
> @@ -10,6 +10,8 @@
> #include <linux/mm.h>
> #include <linux/platform_device.h>
> #include <linux/slab.h>
> +
> +#include <trace/events/geniezone.h>
> #include <linux/gzvm_drv.h>
>
> /* maximum size needed for holding an integer */
> @@ -103,6 +105,7 @@ static long gzvm_vcpu_run(struct gzvm_vcpu *vcpu, void __user *argp)
>
> while (!need_userspace && !signal_pending(current)) {
> gzvm_arch_vcpu_run(vcpu, &exit_reason);
> + trace_mtk_vcpu_exit(exit_reason);
>
> switch (exit_reason) {
> case GZVM_EXIT_MMIO:
> diff --git a/include/trace/events/geniezone.h b/include/trace/events/geniezone.h
> new file mode 100644
> index 000000000000..1fa44f9c4b3c
> --- /dev/null
> +++ b/include/trace/events/geniezone.h
> @@ -0,0 +1,54 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2023 MediaTek Inc.
> + */
> +
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM geniezone
> +
> +#define _TRACE_GENIEZONE_H
> +
> +#include <linux/tracepoint.h>
#define GZVM_EXIT_REASONS \
EM(UNKNOWN) \
EM(MMIO) \
EM(HYPERCALL) \
EM(IRQ) \
EM(EXCEPTION) \
EM(DEBUG) \
EM(FAIL_ENTRY) \
EM(INTERNAL_ERROR) \
EM(SYSTEM_EVENT) \
EM(SHUTDOWN) \
EMe(GZ)
#undef EM
#undef EMe
#define EM(a) TRACE_DEFINE_ENUM(GZVM_EXIT_##a);
#define EMe(a) TRACE_DEFINE_ENUM(GZVM_EXIT_##a);
GZVM_EXIT_REASONS
#undef EM
#undef EMe
#define EM(a) { GZVM_EXIT_##a, #a },
#define EMe(a) { GZVM_EXIT_##a, #a }
> +
> +TRACE_EVENT(mtk_hypcall_enter,
> + TP_PROTO(unsigned long id),
> +
> + TP_ARGS(id),
> +
> + TP_STRUCT__entry(__field(unsigned long, id)),
> +
> + TP_fast_assign(__entry->id = id;),
> +
> + TP_printk("id=0x%lx", __entry->id)
> +);
> +
> +TRACE_EVENT(mtk_hypcall_leave,
> + TP_PROTO(unsigned long id, unsigned long invalid),
> +
> + TP_ARGS(id, invalid),
> +
> + TP_STRUCT__entry(__field(unsigned long, id)
> + __field(unsigned long, invalid)
> + ),
> +
> + TP_fast_assign(__entry->id = id;
> + __entry->invalid = invalid;
> + ),
> +
> + TP_printk("id=0x%lx invalid=%lu", __entry->id, __entry->invalid)
> +);
> +
> +TRACE_EVENT(mtk_vcpu_exit,
> + TP_PROTO(unsigned long exit_reason),
> +
> + TP_ARGS(exit_reason),
> +
> + TP_STRUCT__entry(__field(unsigned long, exit_reason)),
> +
> + TP_fast_assign(__entry->exit_reason = exit_reason;),
> +
> + TP_printk("vcpu exit_reason=0x%lx", __entry->exit_reason)
TP_printk("vcpu exit_reason=0x%lx",
__print_symbolic(__entry->exit_reason, GZVM_EXIT_REASONS))
And instead of having the cryptic enum values printed, you will have human
readable reasons.
-- Steve
> +);
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>
^ permalink raw reply
* Re: [PATCH 09/10] iio: dac: add support for AXI DAC IP core
From: Jonathan Cameron @ 2024-03-28 15:35 UTC (permalink / raw)
To: Nuno Sa via B4 Relay
Cc: nuno.sa, linux-iio, devicetree, Dragos Bogdan, Lars-Peter Clausen,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Olivier Moysan
In-Reply-To: <20240328-iio-backend-axi-dac-v1-9-afc808b3fde3@analog.com>
On Thu, 28 Mar 2024 14:22:33 +0100
Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
> From: Nuno Sa <nuno.sa@analog.com>
>
> Support the Analog Devices Generic AXI DAC IP core. The IP core is used
> for interfacing with digital-to-analog (DAC) converters that require either
> a high-speed serial interface (JESD204B/C) or a source synchronous parallel
> interface (LVDS/CMOS). Typically (for such devices) SPI will be used for
> configuration only, while this IP core handles the streaming of data into
> memory via DMA.
>
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
A few minor things inline, but mostly seems fine to me.
Jonathan
...
> diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c
> new file mode 100644
> index 000000000000..0022ecb4e4bb
> --- /dev/null
> +++ b/drivers/iio/dac/adi-axi-dac.c
> +
> +enum {
> + AXI_DAC_FREQ_TONE_1,
> + AXI_DAC_FREQ_TONE_2,
> + AXI_DAC_SCALE_TONE_1,
> + AXI_DAC_SCALE_TONE_2,
> + AXI_DAC_PHASE_TONE_1,
> + AXI_DAC_PHASE_TONE_2,
> +};
> +
> +static int __axi_dac_frequency_get(struct axi_dac_state *st, unsigned int chan,
> + unsigned int tone, unsigned int *freq)
> +{
> + u32 reg, raw;
> + int ret;
> +
> + if (!st->dac_clk) {
> + dev_err(st->dev, "Sampling rate is 0...\n");
> + return -EINVAL;
> + }
> +
> + if (tone == AXI_DAC_FREQ_TONE_1)
Given this is matching 2 out of enum with other values, it would be more
locally readable as a switch statement with an error returning default.
Then we wouldn't need to look at the caller.
Or at the caller convert from the enum to 0,1 for all these functions.
> + reg = AXI_DAC_REG_CHAN_CNTRL_2(chan);
> + else
> + reg = AXI_DAC_REG_CHAN_CNTRL_4(chan);
> +
> + ret = regmap_read(st->regmap, reg, &raw);
> + if (ret)
> + return ret;
> +
> + raw = FIELD_GET(AXI_DAC_FREQUENCY, raw);
> + *freq = DIV_ROUND_CLOSEST_ULL(raw * st->dac_clk, BIT(16));
> +
> + return 0;
> +}
...
> +static int axi_dac_scale_set(struct axi_dac_state *st,
> + const struct iio_chan_spec *chan,
> + const char *buf, size_t len, unsigned int tone)
> +{
> + int integer, frac, scale;
> + u32 raw = 0, reg;
> + int ret;
> +
> + ret = iio_str_to_fixpoint(buf, 100000, &integer, &frac);
> + if (ret)
> + return ret;
> +
> + scale = integer * MEGA + frac;
> + if (scale <= -2 * (int)MEGA || scale >= 2 * (int)MEGA)
> + return -EINVAL;
> +
> + /* format is 1.1.14 (sign, integer and fractional bits) */
> + if (scale < 0) {
> + raw = FIELD_PREP(AXI_DAC_SCALE_SIGN, 1);
> + scale *= -1;
> + }
> +
> + raw |= div_u64((u64)scale * AXI_DAC_SCALE_INT, MEGA);
> +
> + if (tone == AXI_DAC_SCALE_TONE_1)
> + reg = AXI_DAC_REG_CHAN_CNTRL_1(chan->channel);
> + else
> + reg = AXI_DAC_REG_CHAN_CNTRL_3(chan->channel);
> +
> + guard(mutex)(&st->lock);
> + ret = regmap_write(st->regmap, reg, raw);
> + if (ret)
> + return ret;
> +
> + /* synchronize channels */
> + ret = regmap_set_bits(st->regmap, AXI_DAC_REG_CNTRL_1, AXI_DAC_SYNC);
> + if (ret)
> + return ret;
> +
> + return len;
> +}
> +
> +static int axi_dac_phase_set(struct axi_dac_state *st,
> + const struct iio_chan_spec *chan,
> + const char *buf, size_t len, unsigned int tone)
> +{
> + int integer, frac, phase;
> + u32 raw, reg;
> + int ret;
> +
> + ret = iio_str_to_fixpoint(buf, 100000, &integer, &frac);
> + if (ret)
> + return ret;
> +
> + phase = integer * MEGA + frac;
> + if (phase < 0 || phase > AXI_DAC_2_PI_MEGA)
> + return -EINVAL;
> +
> + raw = DIV_ROUND_CLOSEST_ULL((u64)phase * U16_MAX, AXI_DAC_2_PI_MEGA);
> +
> + if (tone == AXI_DAC_PHASE_TONE_1)
Preference for a switch so it's clear there are only 2 choices.
> + reg = AXI_DAC_REG_CHAN_CNTRL_2(chan->channel);
> + else
> + reg = AXI_DAC_REG_CHAN_CNTRL_4(chan->channel);
> +
> + guard(mutex)(&st->lock);
> + ret = regmap_update_bits(st->regmap, reg, AXI_DAC_PHASE,
> + FIELD_PREP(AXI_DAC_PHASE, raw));
> + if (ret)
> + return ret;
> +
> + /* synchronize channels */
> + ret = regmap_set_bits(st->regmap, AXI_DAC_REG_CNTRL_1, AXI_DAC_SYNC);
> + if (ret)
> + return ret;
> +
> + return len;
> +}
> +
> +static int axi_dac_ext_info_set(struct iio_backend *back, uintptr_t private,
> + const struct iio_chan_spec *chan,
> + const char *buf, size_t len)
> +{
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> + switch (private) {
> + case AXI_DAC_FREQ_TONE_1:
> + case AXI_DAC_FREQ_TONE_2:
Same as the get path - convert to which tone here so that the enum becomes
a tone index for the functions called and the mapping to that single enum
is kept clear of the lower level code.
> + return axi_dac_frequency_set(st, chan, buf, len, private);
> + case AXI_DAC_SCALE_TONE_1:
> + case AXI_DAC_SCALE_TONE_2:
> + return axi_dac_scale_set(st, chan, buf, len, private);
> + case AXI_DAC_PHASE_TONE_1:
> + case AXI_DAC_PHASE_TONE_2:
> + return axi_dac_phase_set(st, chan, buf, len, private);
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static int axi_dac_ext_info_get(struct iio_backend *back, uintptr_t private,
> + const struct iio_chan_spec *chan, char *buf)
> +{
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> + switch (private) {
> + case AXI_DAC_FREQ_TONE_1:
> + case AXI_DAC_FREQ_TONE_2:
> + return axi_dac_frequency_get(st, chan, buf, private);
I'd break out private as an unsigned int here and then - AXI_DAC_FREQ_TONE_1
so that it is just which tone for all the calls made from here.
Similar for the following ones.
> + case AXI_DAC_SCALE_TONE_1:
> + case AXI_DAC_SCALE_TONE_2:
> + return axi_dac_scale_get(st, chan, buf, private);
> + case AXI_DAC_PHASE_TONE_1:
> + case AXI_DAC_PHASE_TONE_2:
> + return axi_dac_phase_get(st, chan, buf, private);
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static const struct iio_chan_spec_ext_info axi_dac_ext_info[] = {
> + IIO_BACKEND_EX_INFO("frequency0", IIO_SEPARATE, AXI_DAC_FREQ_TONE_1),
> + IIO_BACKEND_EX_INFO("frequency1", IIO_SEPARATE, AXI_DAC_FREQ_TONE_2),
> + IIO_BACKEND_EX_INFO("scale0", IIO_SEPARATE, AXI_DAC_SCALE_TONE_1),
> + IIO_BACKEND_EX_INFO("scale1", IIO_SEPARATE, AXI_DAC_SCALE_TONE_2),
> + IIO_BACKEND_EX_INFO("phase0", IIO_SEPARATE, AXI_DAC_PHASE_TONE_1),
> + IIO_BACKEND_EX_INFO("phase1", IIO_SEPARATE, AXI_DAC_PHASE_TONE_2),
> + {}
> +};
> +
> +static int axi_dac_extend_chan(struct iio_backend *back,
> + struct iio_chan_spec *chan)
> +{
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> + if (chan->type != IIO_ALTVOLTAGE)
> + return -EINVAL;
> + if (st->reg_config & AXI_DDS_DISABLE)
> + /* nothing to extend */
> + return 0;
> +
> + chan->ext_info = axi_dac_ext_info;
> +
> + return 0;
> +}
> +static int axi_dac_set_sample_rate(struct iio_backend *back, unsigned int chan,
> + u64 sample_rate)
> +{
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> + unsigned int freq;
> + int ret, tone;
> +
> + if (!sample_rate)
> + return -EINVAL;
> + if (st->reg_config & AXI_DDS_DISABLE)
> + /* nothing to care if DDS is disabled */
Rephrase this. Is the point that the sample rate has no meaning without DDS or
that it has meaning but nothing to do here?
> + return 0;
> +
> + guard(mutex)(&st->lock);
> + /*
> + * If dac_clk is 0 then this must be the first time we're being notified
> + * about the interface sample rate. Hence, just update our internal
> + * variable and bail... If it's not 0, then we get the current DDS
> + * frequency (for the old rate) and update the registers for the new
> + * sample rate.
> + */
> + if (!st->dac_clk) {
> + st->dac_clk = sample_rate;
> + return 0;
> + }
> +
> + for (tone = 0; tone <= AXI_DAC_FREQ_TONE_2; tone++) {
> + ret = __axi_dac_frequency_get(st, chan, tone, &freq);
> + if (ret)
> + return ret;
> +
> + ret = __axi_dac_frequency_set(st, chan, sample_rate, tone, freq);
> + if (ret)
> + return ret;
> + }
> +
> + st->dac_clk = sample_rate;
> +
> + return 0;
> +}
^ permalink raw reply
* Re: [PATCH] dt-bindings: arm: qcom: Add Samsung Galaxy Z Fold5
From: Konrad Dybcio @ 2024-03-28 15:34 UTC (permalink / raw)
To: Alexandru Serdeliuc, Bjorn Andersson, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20620ab0-5024-439e-943b-ab12d35a60d8@yahoo.com>
On 28.03.2024 4:10 PM, Alexandru Serdeliuc wrote:
> Hi Konrad,
>
> Thanks, I unfortunately sent the patch 2 prior seeing your reply.
>
> The warning was this one which says that i need to send the mods separately in two patches:
>
>>>>
>
>>>>WARNING: DT binding docs and includes should be a separate patch. See: Documentation/devicetree/bindings/submitting-patches.rst
Hm, if they were two separate patches, this is a false-positive. Could you
push the branch somewhere, so that we can report it to checkpatch maintainers?
>
>>>>
>
>
> I suppose that me sending two separate patches was not good, how i can fix this?
Please pick them both onto a single branch and send together as a series,
with a revision bump (v2) and mention that you made no changes other than
combining the two in the cover letter.
Konrad
^ permalink raw reply
* Re: [net-next,v4 0/2] ravb: Support describing the MDIO bus
From: Jakub Kicinski @ 2024-03-28 15:28 UTC (permalink / raw)
To: Niklas Söderlund
Cc: Sergey Shtylyov, David S. Miller, Eric Dumazet, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Geert Uytterhoeven, Claudiu Beznea, Yoshihiro Shimoda, Biju Das,
netdev, devicetree, linux-renesas-soc
In-Reply-To: <20240328094546.GI1108818@ragnatech.se>
On Thu, 28 Mar 2024 10:45:46 +0100 Niklas Söderlund wrote:
> This series was marked as Deferred in patchwork. I just wonder why that
> is? Patch 1/2 touches bindings so it could go thru the Renesas tree but
> patch 2/2 touches the driver and depends on 1/2. Should not this whole
> series go thru net-next?
I don't see why either. Looks ready to apply, TBH.
pw-bot: under-review
I'll get to applying later today, hopefully.
^ 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