* Re: [PATCH 1/2] iio: dac: dac8163: Add driver for DAC8163
From: Andy Shevchenko @ 2026-06-23 19:39 UTC (permalink / raw)
To: Lukas Metz
Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-kernel,
linux-iio, devicetree
In-Reply-To: <20260623-dac8163-work-v1-1-5b508158faa0@gmx.net>
On Tue, Jun 23, 2026 at 06:07:27PM +0200, Lukas Metz wrote:
> The DAC756x, DAC816x, and DAC856x devices are low-power, voltage-output,
> dual-channel, 12-, 14-, and 16-bit digital-to-analog converters (DACs),
> respectively. These devices include a 2.5-V, 4-ppm/°C internal
> reference, giving a full-scale output voltage range of 2.5 V or 5 V.
> +config TI_DAC8163
> + tristate "Texas Instruments 12/14/16-bit 2-channel DAC driver"
> + depends on SPI_MASTER
> + help
> + Driver for the Texas Instruments digital-to-analog converter
> + family dacxx6x compatible with the variants DAC7562,
> + DAC7563, DAC8162, DAC8163, DAC8562 and DAC8563.
This list doesn't scale. Put one part number per line as
- DAC7... (channel bits)
- DAC8... (...)
- ...
> + If compiled as a module, it will be called ti-dac8163.
...
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>
> +#include <linux/of.h>
No, for this header in new drivers. Can't we use device and/or fwnode property
APIs instead?
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +#include <linux/iio/iio.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/units.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/printk.h>
? Usually one wants dev_printk.h.
> +#include <linux/bitfield.h>
Keep the list sorted, and also follow the IWYU principle.
...
> +#define COMMAND_SET(x, y) (FIELD_PREP(COMMAND_MASK, (x)) | \
> + FIELD_PREP(ADDRESS_MASK, (y)))
This is ugly indentation. Compare to
#define COMMAND_SET(x, y) \
(FIELD_PREP(COMMAND_MASK, (x)) | FIELD_PREP(ADDRESS_MASK, (y)))
...
> +enum dacxx6x_ldac_modes {
> + LDAC_MODE_ACTIVE = 0,
> + LDAC_MODE_INACTIVE = 1
Leave trailing commas in the non-terminator entries here and there.
> +};
...
> +enum dacxx6x_supported_device_ids {
> + ID_DAC7562,
> + ID_DAC7563,
> + ID_DAC8162,
> + ID_DAC8163,
> + ID_DAC8562,
> + ID_DAC8563
> +};
This is solely used to make indexed array with chip_info. Instead kill this
enum and use separate structures.
...
> +struct dacxx6x_state {
> + struct spi_device *spi;
Why not regmap?
> + struct regulator *vref;
> + struct gpio_desc *loaddacs;
> +
> + bool internal_ref;
> + int vref_uv;
_uV
> + unsigned int cached[2];
> +
> + /*
> + * Lock to protect the state of the device from potential concurrent
> + * write accesses from userspace.
> + */
> + struct mutex lock;
> +};
...
> +#define DACXX6X_CHAN(id, resolution) \
> + { \
> + .type = IIO_VOLTAGE, .channel = (id), .output = 1, \
> + .indexed = 1, .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
Do not put two or more things on the same line, it's unreadable.
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> + .scan_type = { .realbits = (resolution), \
> + .shift = 16 - (resolution) }, \
Make these to be 4 lines.
> + }
...
> +static const struct dacxx6x_chip_info dacxx6x_chip_info_table[6] = {
Drop the number in the square brackets, let compiler do that job. But see
above, this has to be just 6 different data structures.
> +};
...
> +static int dacxx6x_write_reg(struct dacxx6x_state *st, int reg, int addr,
> + unsigned int val)
> +{
> + u8 tx[3];
Are you sure about this? How would it work with DMA?
> + tx[0] = COMMAND_SET(reg, addr);
> + tx[1] = (val >> 8) & 0xff;
> + tx[2] = val & 0xff;
Use proper unaligned setters: put_unaligned_be16() from linux/unaligned.h.
> + return spi_write(st->spi, tx, sizeof(tx));
> +}
...
> +static int dacxx6x_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int val,
> + int val2, long mask)
Split logically:
static int dacxx6x_write_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int val, int val2, long mask)
> +{
> + struct dacxx6x_state *st = iio_priv(indio_dev);
> + struct device *dev = &st->spi->dev;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + dev_dbg(dev, "%s: val=%d val2=%d\n", __func__, val, val2);
No. Is it RFC? PoC? Or production-ready? If not the latter, come when it will
be production-ready.
> + if (val2 != 0)
> + return -EINVAL;
> + if (val < 0 || val >= BIT(chan->scan_type.realbits))
> + return -EINVAL;
Why not ERANGE or something like this?
> +
> + mutex_lock(&st->lock);
Why not guard()()?
> + int ret = dacxx6x_write_reg(st, CMD_WRITE_UPDATE, chan->channel,
> + (unsigned int)val
> + << chan->scan_type.shift);
Awful indentation. Please, check
> + if (!ret)
No, use regular pattern: Check for error first.
> + st->cached[chan->channel] = val;
> + mutex_unlock(&st->lock);
> + return ret;
> + default:
> + return -EINVAL;
> + }
> +}
...
> +static int dacxx6x_probe(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev;
> + struct dacxx6x_state *st;
> + const struct dacxx6x_chip_info *info;
> + int ret;
Reversed xmas tree order, please.
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> + st->spi = spi;
> + spi_set_drvdata(spi, indio_dev);
Is it being used?
> + st->loaddacs = devm_gpiod_get_optional(&spi->dev, "ti,loaddacs",
> + GPIOD_OUT_LOW);
In the above you used temporary variable for struct device, why not here?
> + if (IS_ERR(st->loaddacs))
> + return PTR_ERR(st->loaddacs);
> +
> + st->internal_ref =
> + device_property_read_bool(&spi->dev, "ti,internal-ref");
And here (and it becomes a single line as well).
> + if (!st->internal_ref) {
> + st->vref = devm_regulator_get(&spi->dev, "vref");
> + if (IS_ERR(st->vref))
> + return PTR_ERR(st->vref);
> +
> + ret = regulator_enable(st->vref);
> + if (ret < 0)
> + return ret;
> + }
Can't you get just voltage and be done with it for now?
> + mutex_init(&st->lock);
devm_mutex_init.
> + if (st->internal_ref) {
> + st->vref_uv = 2500000; /* 2.5V internal reference */
> + } else {
> + st->vref_uv = regulator_get_voltage(st->vref);
> + if (st->vref_uv < 0) {
> + ret = st->vref_uv;
> + goto err;
> + }
> + }
> +
> + gpiod_set_value(st->loaddacs, 0);
> +
> + ret = dacxx6x_write_reg(st, CMD_LDAC_MODE, 0,
> + FIELD_PREP(LDAC_MODE_CHANNEL_A_MASK, LDAC_MODE_INACTIVE) |
> + FIELD_PREP(LDAC_MODE_CHANNEL_B_MASK, LDAC_MODE_INACTIVE));
> +
Stray blank line.
> + if (ret < 0)
> + goto err;
> +
> + ret = dacxx6x_write_reg(st, CMD_SEL_REFERENCE, 0,
> + FIELD_PREP(VOLTAGE_REFERENCE_MASK, st->internal_ref));
> +
Ditto.
> + if (ret < 0)
> + goto err;
> +
> + info = spi_get_device_match_data(spi);
We need to check for NULL here due to driver_override.
> + indio_dev->name = info->name;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->info = &dacxx6x_iio_info;
> + indio_dev->channels = info->channels;
> + indio_dev->num_channels = 2;
> +
> + ret = iio_device_register(indio_dev);
> + if (ret)
> + goto err;
> +
> + return 0;
> +err:
> + if (!st->internal_ref)
> + regulator_disable(st->vref);
> + mutex_destroy(&st->lock);
> + return ret;
> +}
...
> +static void dacxx6x_remove(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev = spi_get_drvdata(spi);
> + struct dacxx6x_state *st = iio_priv(indio_dev);
> +
> + iio_device_unregister(indio_dev);
> + mutex_destroy(&st->lock);
> + if (!st->internal_ref)
> + regulator_disable(st->vref);
> +}
Finish devm conversion and drop this function.
...
> +#define DACXX6X_COMPATIBLE(of_compatible, id) \
> + { \
> + .compatible = of_compatible, \
> + .data = &dacxx6x_chip_info_table[id] \
> + }
No, just use directly, so drop this macro.
> +static const struct of_device_id dacxx6x_of_match[] = {
> + DACXX6X_COMPATIBLE("ti,dac7562", ID_DAC7562),
> + DACXX6X_COMPATIBLE("ti,dac7563", ID_DAC7563),
> + DACXX6X_COMPATIBLE("ti,dac8162", ID_DAC8162),
> + DACXX6X_COMPATIBLE("ti,dac8163", ID_DAC8163),
> + DACXX6X_COMPATIBLE("ti,dac8562", ID_DAC8562),
> + DACXX6X_COMPATIBLE("ti,dac8563", ID_DAC8563),
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, dacxx6x_of_match);
> +
> +static const struct spi_device_id dacxx6x_id_table[] = {
> + { "dac7562", (kernel_ulong_t)&dacxx6x_chip_info_table[ID_DAC7562] },
> + { "dac7563", (kernel_ulong_t)&dacxx6x_chip_info_table[ID_DAC7563] },
> + { "dac8162", (kernel_ulong_t)&dacxx6x_chip_info_table[ID_DAC8162] },
> + { "dac8163", (kernel_ulong_t)&dacxx6x_chip_info_table[ID_DAC8163] },
> + { "dac8562", (kernel_ulong_t)&dacxx6x_chip_info_table[ID_DAC8562] },
> + { "dac8563", (kernel_ulong_t)&dacxx6x_chip_info_table[ID_DAC8563] },
> + {}
> +};
> +
Unneeded blank line.
> +MODULE_DEVICE_TABLE(spi, dacxx6x_id_table);
> +
> +static struct spi_driver dacxx6x_driver = {
> + .driver = {
> + .name = "ti-dacxx6x",
> + .of_match_table = dacxx6x_of_match,
> + },
> + .probe = dacxx6x_probe,
> + .remove = dacxx6x_remove,
> + .id_table = dacxx6x_id_table,
> +};
> +
Ditto.
> +module_spi_driver(dacxx6x_driver);
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2 3/3] iio: magnetometer: st_magn: honour st,fullscale-milligauss DT property
From: Jonathan Cameron @ 2026-06-23 19:29 UTC (permalink / raw)
To: Herman van Hazendonk
Cc: David Lechner, Nuno Sá, Andy Shevchenko, Nathan Chancellor,
Nick Desaulniers, Bill Wendling, Justin Stitt, Denis Ciocca,
Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Denis Ciocca, Linus Walleij, linux-iio,
linux-kernel, llvm, devicetree
In-Reply-To: <20260616-submit-iio-lsm303dlh-magn-fixes-v2-3-063edcf74e60@herrie.org>
On Tue, 16 Jun 2026 15:02:06 +0200
Herman van Hazendonk <github.com@herrie.org> wrote:
> The ST magnetometer core's common probe hardcodes fs_avl[0] -- the
> highest-sensitivity full-scale supported by the chip -- as the
> starting range. For the LSM303DLH that is +/-1.3 G; for the
> LSM303DLHC and LSM303DLM it is +/-2 G; for the LIS3MDL it is +/-4 G.
>
> That is the right default for "minimal noise floor at a desk", but
> it leaves no margin for boards that pick up appreciable DC bias from
> nearby PCB structures. On the HP TouchPad (apq8060 / tenderloin) the
> LSM303DLH magnetometer is mounted close enough to the surrounding
> power planes that X reads back as the chip's 0xF000 overflow
> sentinel (== -4096 raw, the value the chip publishes when the ADC
> saturates) on every sample at the chip-default range, while Y and Z
> fall well within the +/-1.3 G window.
>
> Parse the st,fullscale-milligauss device-tree property (documented
> separately in dt-bindings/iio/st,st-sensors.yaml) in the
> magnetometer common probe to select the initial fs_avl entry by its
> mg value. The DT binding pins the accepted value set per compatible
> via allOf/if-then enum clauses, so a malformed mg value fails
> dt_binding_check rather than reaching the driver. Sensors with a
> fixed full-scale (fs.addr == 0: LSM303AGR, LIS2MDL, IIS2MDC) have no
> register to switch and the property is rejected outright for them
> in the binding; the parse block is additionally gated on fs.addr as
> defence in depth against stale DTBs.
>
> Per-sensor mg ranges are listed in st_magn_sensors_settings[]. For
> LSM303DLH and LSM303DLHC/DLM the valid values are 1300, 1900, 2500,
> 4000, 4700, 5600 and 8100; for LIS3MDL, LSM9DS1-magn and LSM303C-magn
> they are 4000, 8000, 12000, 16000.
>
> Empirical scale sweep on the HP TouchPad confirmed that on this
> board any fs_avl >= 1 produces non-saturated X readings:
>
> scale (0.001 G/LSB) | X raw Y raw Z raw
> --------------------+-------------------------------
> 1.100 | -4096 44 46 (X saturated)
> 0.855 | -547 37 37 (clean)
> 0.670 | -433 94 103 (clean)
> 0.450 | -266 44 71 (clean)
> 0.400 | -235 34 65 (clean)
> 0.330 | -196 27 56 (clean)
> 0.230 | -145 15 40 (clean)
>
> 2500 mg is the natural choice for tenderloin: comfortably outside
> the saturation regime while keeping useful precision for compass
> applications.
>
> Assisted-by: Claude:claude-opus-4-7 sparse smatch clang-analyzer coccinelle checkpatch
> Assisted-by: Sashiko:claude-opus-4-7
Hmm. First time I remember seeing Sashiko credited like this. Seems like pretty much
every patch series of any complexity would end up crediting sashiko.
Out of curiosity were you just looking at reports, or were you running it locally to
help with development?
One thing inline.
> Signed-off-by: Herman van Hazendonk <github.com@herrie.org>
> ---
> drivers/iio/magnetometer/st_magn_core.c | 32 ++++++++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
>
> diff --git a/drivers/iio/magnetometer/st_magn_core.c b/drivers/iio/magnetometer/st_magn_core.c
> index ef348d316c00..6f369e8dddea 100644
> --- a/drivers/iio/magnetometer/st_magn_core.c
> +++ b/drivers/iio/magnetometer/st_magn_core.c
> @@ -10,6 +10,7 @@
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/mutex.h>
> +#include <linux/property.h>
> #include <linux/sysfs.h>
> #include <linux/iio/iio.h>
> #include <linux/iio/sysfs.h>
> @@ -608,6 +609,7 @@ int st_magn_common_probe(struct iio_dev *indio_dev)
> struct st_sensor_data *mdata = iio_priv(indio_dev);
> struct device *parent = indio_dev->dev.parent;
> struct st_sensors_platform_data *pdata = dev_get_platdata(parent);
> + const char *propname;
> int err;
>
> indio_dev->modes = INDIO_DIRECT_MODE;
> @@ -628,6 +630,36 @@ int st_magn_common_probe(struct iio_dev *indio_dev)
> mdata->current_fullscale = &mdata->sensor_settings->fs.fs_avl[0];
> mdata->odr = mdata->sensor_settings->odr.odr_avl[0].hz;
>
> + /*
> + * Skip fixed-FS chips (fs.addr == 0): no register to switch.
> + * The binding rejects the property on these compatibles too;
> + * the gate guards stale DTBs.
Isn't it optional? If so they aren't necessarily stale, people
may have relied on the default - which I now notice isn't specified
in the dt-binding (just replied to that patch).
> + */
> + propname = "st,fullscale-milligauss";
> + if (mdata->sensor_settings->fs.addr &&
> + device_property_present(parent, propname)) {
^ permalink raw reply
* Re: [PATCH v2 2/3] dt-bindings: iio: st,st-sensors: add st,fullscale-milligauss
From: Jonathan Cameron @ 2026-06-23 19:26 UTC (permalink / raw)
To: Herman van Hazendonk
Cc: David Lechner, Nuno Sá, Andy Shevchenko, Nathan Chancellor,
Nick Desaulniers, Bill Wendling, Justin Stitt, Denis Ciocca,
Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Denis Ciocca, Linus Walleij, linux-iio,
linux-kernel, llvm, devicetree
In-Reply-To: <20260616-submit-iio-lsm303dlh-magn-fixes-v2-2-063edcf74e60@herrie.org>
On Tue, 16 Jun 2026 15:02:05 +0200
Herman van Hazendonk <github.com@herrie.org> wrote:
> Add an optional st,fullscale-milligauss property that selects the
> initial magnetometer full-scale range at probe time, expressed in
> milligauss.
>
> The motivating case is the LSM303DLH magnetometer on the HP TouchPad
> (apq8060 / tenderloin) where the kernel's chip-default +/-1.3 G range
> saturates the X axis to the chip's 0xF000 overflow sentinel out of
> probe, because the chip is mounted close to surrounding power planes
> and picks up enough DC bias to exceed the smallest range.
>
> The chip is not wedged by the saturation: a sysfs write of a wider
> range to in_magn_x_scale recovers it on the next conversion, and a
> UDEV rule on add of the IIO device is a viable steady-state
> workaround. What the DT property buys is the probe-time window: the
> in-tree consumers we use (sensorfw's iio-sensors-adaptor and the
> geomagnetic / orientation services on top of it) start polling
> in_magn_x_raw essentially as soon as the device node appears and
> treat the 0xF000 sentinel as a legitimate sample. Until a UDEV rule
> fires and commits the wider range, every read returns the stuck
> sentinel, and on slow-boot paths the consumer may have already
> cached a bogus calibration baseline by the time UDEV catches up.
>
> st,fullscale-milligauss lets the device tree declare a wider
> initial range up-front so the correct range is in effect before any
> IIO consumer can open the device, and keeps the board-specific
> magnetometer calibration alongside the rest of the hardware
> description rather than splitting it between DTS and per-distro
> UDEV rules.
>
> The full property name is spelled out rather than abbreviated to
> "-mg" because this same binding file already covers ST accelerometers
> where the conventional shorthand "mg" reads as milli-g (acceleration);
> the explicit "-milligauss" suffix makes the unit unambiguous if a
> similar tunable is ever introduced for the accel/gyro/pressure
> families.
>
> The property is scoped to magnetometer compatibles via allOf/if-then
> clauses, with per-family enum lists of the accepted milligauss
> values (1300..8100 for LSM303DLH/DLHC/DLM, 4000..16000 for
> LIS3MDL/LSM9DS1/LSM303C). LSM303AGR / LIS2MDL / IIS2MDC have a
> single fixed full-scale (15000 mg) with no register to switch
> ranges, so the property is rejected outright for them. DTSes that
> misspell the value, place the property on an accelerometer /
> gyroscope / pressure node, or set it on a fixed-FS magnetometer
> fail dt_binding_check rather than emitting a runtime warning.
>
> The property is purely additive: if absent, drivers fall back to
> their existing chip default. No existing in-tree DTS is affected.
>
> Assisted-by: Claude:claude-opus-4-7 dt_binding_check checkpatch
> Assisted-by: Sashiko:claude-opus-4-7
> Signed-off-by: Herman van Hazendonk <github.com@herrie.org>
> ---
> .../devicetree/bindings/iio/st,st-sensors.yaml | 71 ++++++++++++++++++++++
> 1 file changed, 71 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iio/st,st-sensors.yaml b/Documentation/devicetree/bindings/iio/st,st-sensors.yaml
> index a1a958215cdb..f0805604c849 100644
> --- a/Documentation/devicetree/bindings/iio/st,st-sensors.yaml
> +++ b/Documentation/devicetree/bindings/iio/st,st-sensors.yaml
> @@ -126,6 +126,13 @@ properties:
> mount-matrix:
> description: an optional 3x3 mounting rotation matrix.
>
> + st,fullscale-milligauss:
> + description:
> + Initial magnetometer full-scale at probe time, in milligauss.
> + Per-chip allowed values are enumerated in the allOf clauses
> + below.
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> allOf:
> - if:
> properties:
> @@ -163,6 +170,70 @@ allOf:
> maxItems: 1
> st,drdy-int-pin: false
>
> + # Per-chip enum lists for st,fullscale-milligauss. Out-of-range
> + # values fail dt_binding_check instead of being demoted to a
> + # runtime warning by the driver.
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - st,lsm303dlh-magn
> + - st,lsm303dlhc-magn
> + - st,lsm303dlm-magn
> + then:
> + properties:
> + st,fullscale-milligauss:
> + enum: [1300, 1900, 2500, 4000, 4700, 5600, 8100]
Can we also add default: to these entries to document what happens
when nothing is provided?
Thanks,
Jonathan
^ permalink raw reply
* Re: [PATCH v2 1/3] iio: common: st_sensors: honour channel endianness in read_axis_data
From: Jonathan Cameron @ 2026-06-23 19:23 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Herman van Hazendonk, David Lechner, Nuno Sá,
Andy Shevchenko, Nathan Chancellor, Nick Desaulniers,
Bill Wendling, Justin Stitt, Denis Ciocca, Lars-Peter Clausen,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Denis Ciocca,
Linus Walleij, linux-iio, linux-kernel, llvm, devicetree, stable
In-Reply-To: <ajJwzsIMxX3Ew1x_@ashevche-desk.local>
On Wed, 17 Jun 2026 13:02:54 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> On Tue, Jun 16, 2026 at 03:02:04PM +0200, Herman van Hazendonk wrote:
> > st_sensors_read_axis_data() unconditionally decoded multi-byte
> > results with get_unaligned_le16() / get_unaligned_le24() regardless
> > of the channel's declared scan_type.endianness.
> >
> > For every ST sensor that has used this helper since it was introduced
> > this happened to be fine because the ST IMU/accel/gyro/pressure
> > families publish their data registers as little-endian and the
> > channel specs in those drivers declare IIO_LE accordingly.
> >
> > The LSM303DLH magnetometer however publishes its X/Y/Z output as a
> > pair of big-endian bytes (the H register sits at the lower address,
> > 0x03/0x05/0x07, and the L register immediately after), and its
> > channel specs in st_magn_core.c correctly declare IIO_BE -- but
> > read_axis_data() ignored that and decoded as little-endian, swapping
> > the high and low bytes of every magnetometer sample. The LSM303DLHC
> > and LSM303DLM share the same st_magn_16bit_channels (IIO_BE) and
> > were therefore byte-swapped by the same bug; users of those parts
> > will see different in_magn_*_raw values after this fix lands.
> >
> > The bug is most visible on a stationary chip: in earth's field the
> > true X reading is small and the high byte sits at 0x00, so swapping
> > the bytes pins sysfs X at exactly the low byte's pattern (e.g. 0x00F0
> > = 240). Y and Z still appear "to vary" because their magnitudes are
> > larger and the noise in the low byte produces big swings in the
> > swapped high byte:
> >
> > before (LSM303DLH flat, sysfs in_magn_*_raw):
> > X=240 (stuck), Y= 12032..23296, Z=-16128..-9728
> >
> > after (direct i2c-dev big-endian decode, same chip same orientation):
> > X≈-4096, Y≈210, Z≈80 (sensible values reflecting earth's
> > ambient field at low gauss range)
> >
> > Fix read_axis_data() to dispatch on ch->scan_type.endianness and
> > call get_unaligned_be16() / get_unaligned_be24() when the channel
> > declares IIO_BE. Existing IIO_LE consumers (st_accel, st_gyro,
> > st_pressure, st_lsm6dsx and others) are unaffected because their
> > channel specs already declare IIO_LE and the LE path is unchanged.
> >
> > While restructuring the branches, replace the previously implicit
> > silent-success-with-uninitialised-*data fall-through for
> > byte_for_channel outside 1..3 with an explicit return -EINVAL. No
> > in-tree ST sensor publishes such a channel, but the new behaviour
> > is strictly safer than handing userspace garbage.
>
> Sounds like inevitable change in ABI, but worth doing it.
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Applied to the fixes-togreg branch of iio.git. I'll wait until after
rc1 to send a pull request though (and rebase on rc1 in the meantime).
The other patches may have to wait, I haven't checked yet!
Jonathan
>
^ permalink raw reply
* Re:[PATCH 1/2] dt-bindings: hwmon: chipcap2: Add label property
From: Flaviu Nistor @ 2026-06-23 19:22 UTC (permalink / raw)
To: Guenter Roeck
Cc: Javier Carrasco, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Jonathan Corbet, Shuah Khan, Flaviu Nistor, linux-hwmon,
linux-kernel, devicetree, linux-doc
In-Reply-To: <b6d34af3-0c4a-4870-a240-f7873621d2ce@roeck-us.net>
On 6/23/26 9:58 PM CET, Guenter Roeck wrote:
>On 6/23/26 11:16, Flaviu Nistor wrote:
>> On Mon Jun 22, 2026 at 7:29 PM CEST, Javier Carrasco wrote:
>>> On Mon Jun 22, 2026 at 2:21 PM CEST, Flaviu Nistor wrote:
>>>> Add support for an optional label property similar to other hwmon devices
>>>> This allows, in case of boards with multiple CHIPCAP2 sensors, to assign
>>>> distinct names to each instance.
>>>>
>>>> Signed-off-by: Flaviu Nistor <flaviu.nistor@gmail.com>
>>>> ---
>>>> .../devicetree/bindings/hwmon/amphenol,chipcap2.yaml | 5 +++++
>>>> 1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/hwmon/amphenol,chipcap2.ya=
>>> ml b/Documentation/devicetree/bindings/hwmon/amphenol,chipcap2.yaml
>>>> index 17351fdbefce..f00b5a4b14dd 100644
>>>> --- a/Documentation/devicetree/bindings/hwmon/amphenol,chipcap2.yaml
>>>> +++ b/Documentation/devicetree/bindings/hwmon/amphenol,chipcap2.yaml
>>>> @@ -33,6 +33,10 @@ properties:
>>>> reg:
>>>> maxItems: 1
>>>>
>>>> + label:
>>>> + description:
>>>> + A descriptive name for this channel, like "ambient" or "psu".
>>>> +
>>>> interrupts:
>>>> items:
>>>> - description: measurement ready indicator
>>>> @@ -72,6 +76,7 @@ examples:
>>>> <5 IRQ_TYPE_EDGE_RISING>,
>>>> <6 IRQ_TYPE_EDGE_RISING>;
>>>> interrupt-names =3D "ready", "low", "high";
>>>> + label =3D "somelabel";
>>>> vdd-supply =3D <®_vdd>;
>>>> };
>>> };
>>>
>>> Hello Falviu, thank you for your patch.
>>>
>>
>> Hello Javier, thanks for your reply.
>>
>>> Should we not add a reference to hwmon-common.yaml (with
>>> unevelautedProperties instead of additionalProperties), as label is
>>> defined there? I believe that Krzysztof Kozlowski did something similar
>>> for the shunt-resistor-micro-ohms property. Could we follow suit here?
>>>
>>
>> This is a good question and I am happy you asked. I also thought a lot
>> about this and the reason I decided to go for this approach is that by using
>> $ref: hwmon-common.yaml#, I would have to change additionalProperties: false
>> to unevaluatedProperties: false, which will evaluate in case it is used, also
>> shunt-resistor-micro-ohms property which does not apply to this sensor. At
>> least this is my understanding, but of course I can be wrong (I see lm75 binding
>> also uses $ref: hwmon-common.yaml# but shunt-resistor-micro-ohms does not apply).
>>
>
>Where does the idea come from that shunt-resistor-micro-ohms would be mandatory ?
>That would make hwmon-common.yaml unusable for most chips.
I think this is a misunderstanding since I never had the intention to imply that
shunt-resistor-micro-ohms would be mandatory, but rather I observed that if I used
$ref: hwmon-common.yaml#, property shunt-resistor-micro-ohms can be added (no need to,
but still possible) in the example section and the dt_binding_check will pass.
Since hwmon-common.yaml is already there I will change the binding in a v2 and use it.
Best regards,
Flaviu
^ permalink raw reply
* Re: [PATCH 2/2] dt-bindings: iio: dac: Add DAC8163
From: David Lechner @ 2026-06-23 19:17 UTC (permalink / raw)
To: Lukas Metz, Jonathan Cameron, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: linux-kernel, linux-iio, devicetree
In-Reply-To: <20260623-dac8163-work-v1-2-5b508158faa0@gmx.net>
It is more logical to put the dt-bindings patch first in the series
before the driver that makes use of it.
On 6/23/26 11:07 AM, Lukas Metz wrote:
> Add device tree binding for the Texas Instruments DAC8163 family
> including the DAC7562, DAC7563, DAC8162, DAC8163, DAC8562 and DAC8563.
>
> Signed-off-by: Lukas Metz <lukas.metz@gmx.net>
> ---
> .../devicetree/bindings/iio/dac/ti,dac8163.yaml | 75 ++++++++++++++++++++++
> MAINTAINERS | 1 +
> 2 files changed, 76 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iio/dac/ti,dac8163.yaml b/Documentation/devicetree/bindings/iio/dac/ti,dac8163.yaml
> new file mode 100644
> index 000000000000..bb4bad389323
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/dac/ti,dac8163.yaml
> @@ -0,0 +1,75 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/dac/ti,dac8163.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments DAC8163 family of DACs
> +
> +description:
> + The DAC756x, DAC816x, and DAC856x devices are low-power, voltage-output,
> + dual-channel, 12-, 14-, and 16-bit digital-to-analog converters (DACs),
> + respectively. These devices include a 2.5-V, 4-ppm/°C internal
> + reference, giving a full-scale output voltage range of 2.5 V or 5 V.
> +
> +maintainers:
> + - Lukas Metz <lukas.metz@gmx.net>
> +
> +properties:
> + compatible:
> + enum:
> + - ti,dac7562
> + - ti,dac7563
> + - ti,dac8162
> + - ti,dac8163
> + - ti,dac8562
> + - ti,dac8563
> +
> + reg:
> + maxItems: 1
> +
There are a couple of more SPI properties needed since this is not a "normal"
SPI device. We can only write and not read because there is no D_OUT pin. So
spi-rx-bus-width:
items:
- const: 0
will describe this.
There is also no chip select, but now that I am looking, there doesn't seem to
be anything to describe this.
> + ti,loaddacs-gpios:
This should match the actual pin name. And we don't need vendor prefix on gpios.
So `ldac-gpios`.
We also want the binding to be complete even if the driver doesn't all of it, so
`clear-gpios` and `sync-gpios` probably make sense too.
Usually for SPI devices we also add:
spi-max-frequency:
maximum: ...
> + description:
> + Pin needs to be asserted permanently when updating the DAC synchronously.
> + maxItems: 1
> +
> + vref-supply:
> + description:
> + Reference voltage for scaling if an external reference is used.
I prefer to use the actual pin name, i.e. vrefin-supply.
And for the description, it is the actual external reference voltage
supply, not just the voltage.
And we also need `avdd-supply: true`.
> +
> + ti,internal-ref:
> + type: boolean
> + description:
> + Flag if the internal reference is used (external otherwise).
Usually, we don't bother with a property like this since it is redundant.
If an external reference supply is given, then it gets used, otherwise
the internal reference is used.
> +
> +required:
> + - compatible
> + - reg
avdd-supply will be required too
> +
> +allOf:
> + - $ref: /schemas/spi/spi-peripheral-props.yaml#
> + - if:
> + not:
> + required:
> + - ti,internal-ref
> + then:
> + required:
> + - vref-supply
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/gpio/gpio.h>
> + spi {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + dac@1 {
> + compatible = "ti,dac8163";
> + reg = <0x1>; /* CS1 */
These chips don't appear to have a chip select pin, so this comment
doesn't make sense to me. More logical would be to just use dac@0
and reg = <0>; since it should just be ignored.
> + ti,loaddacs-gpios = <&gpiog 8 GPIO_ACTIVE_HIGH>;
The pin is marked active low in the datasheet, so I would expect
this to be GPIO_ACTIVE_LOW.
> + ti,internal-ref; /* internal reference used*/
> + };
> + };
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e82cc28e1bc3..5512f5eaab44 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -26398,6 +26398,7 @@ TI DAC8163 DAC DRIVER
> M: Lukas Metz <lukas.metz@gmx.net>
> L: linux-iio@vger.kernel.org
> S: Maintained
> +F: Documentation/devicetree/bindings/iio/dac/ti,dac8163.yaml
> F: drivers/iio/dac/ti-dac8163.c
>
> TI DATA TRANSFORM AND HASHING ENGINE (DTHE) V2 CRYPTO DRIVER
>
^ permalink raw reply
* Re: [PATCH v3 3/3] iio: magnetometer: add driver for QST QMC5883L Sensor
From: Jonathan Cameron @ 2026-06-23 19:16 UTC (permalink / raw)
To: Siratul Islam
Cc: robh, krzk+dt, conor+dt, dlechner, nuno.sa, andy, linux-iio,
devicetree, linux-kernel
In-Reply-To: <20260619104524.10172-4-siratul.islam@linux.dev>
On Fri, 19 Jun 2026 16:45:06 +0600
Siratul Islam <siratul.islam@linux.dev> wrote:
> Add driver for the QST QMC5883L 3-Axis Magnetic Sensor
> connected via i2c.
>
> Signed-off-by: Siratul Islam <siratul.islam@linux.dev>
I'm going to apply this now but given rc1 is still some way off plenty
of time for more feedback or tags!
Applied to the testing branch of iio.git with a tiny tweak.
Thanks,
Jonathan
> diff --git a/drivers/iio/magnetometer/qmc5883l.c b/drivers/iio/magnetometer/qmc5883l.c
> new file mode 100644
> index 000000000000..b7fff3b63ca4
> --- /dev/null
> +++ b/drivers/iio/magnetometer/qmc5883l.c
> @@ -0,0 +1,517 @@
> +// SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> +/*
> + * Support for QST QMC5883L 3-Axis Magnetic Sensor on I2C bus.
> + *
> + * Copyright (C) 2026 Siratul Islam <siratul.islam@linux.dev>
> + *
> + * Datasheet available at
> + * <https://www.qstcorp.com/upload/pdf/202512/13-52-04%20QMC5883L%20Datasheet%20Rev.%20B.pdf>
> + *
My pet trivial complaint. Blank line adds nothing useful. I'll remove it whilst applying ;)
> + */
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: hwmon: chipcap2: Add label property
From: Guenter Roeck @ 2026-06-23 18:58 UTC (permalink / raw)
To: Flaviu Nistor, Javier Carrasco
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet,
Shuah Khan, linux-hwmon, linux-kernel, devicetree, linux-doc
In-Reply-To: <20260623181625.5697-1-flaviu.nistor@gmail.com>
On 6/23/26 11:16, Flaviu Nistor wrote:
> On Mon Jun 22, 2026 at 7:29 PM CEST, Javier Carrasco wrote:
>> On Mon Jun 22, 2026 at 2:21 PM CEST, Flaviu Nistor wrote:
>>> Add support for an optional label property similar to other hwmon devices
>>> This allows, in case of boards with multiple CHIPCAP2 sensors, to assign
>>> distinct names to each instance.
>>>
>>> Signed-off-by: Flaviu Nistor <flaviu.nistor@gmail.com>
>>> ---
>>> .../devicetree/bindings/hwmon/amphenol,chipcap2.yaml | 5 +++++
>>> 1 file changed, 5 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/hwmon/amphenol,chipcap2.ya=
>> ml b/Documentation/devicetree/bindings/hwmon/amphenol,chipcap2.yaml
>>> index 17351fdbefce..f00b5a4b14dd 100644
>>> --- a/Documentation/devicetree/bindings/hwmon/amphenol,chipcap2.yaml
>>> +++ b/Documentation/devicetree/bindings/hwmon/amphenol,chipcap2.yaml
>>> @@ -33,6 +33,10 @@ properties:
>>> reg:
>>> maxItems: 1
>>>
>>> + label:
>>> + description:
>>> + A descriptive name for this channel, like "ambient" or "psu".
>>> +
>>> interrupts:
>>> items:
>>> - description: measurement ready indicator
>>> @@ -72,6 +76,7 @@ examples:
>>> <5 IRQ_TYPE_EDGE_RISING>,
>>> <6 IRQ_TYPE_EDGE_RISING>;
>>> interrupt-names =3D "ready", "low", "high";
>>> + label =3D "somelabel";
>>> vdd-supply =3D <®_vdd>;
>>> };
>> };
>>
>> Hello Falviu, thank you for your patch.
>>
>
> Hello Javier, thanks for your reply.
>
>> Should we not add a reference to hwmon-common.yaml (with
>> unevelautedProperties instead of additionalProperties), as label is
>> defined there? I believe that Krzysztof Kozlowski did something similar
>> for the shunt-resistor-micro-ohms property. Could we follow suit here?
>>
>
> This is a good question and I am happy you asked. I also thought a lot
> about this and the reason I decided to go for this approach is that by using
> $ref: hwmon-common.yaml#, I would have to change additionalProperties: false
> to unevaluatedProperties: false, which will evaluate in case it is used, also
> shunt-resistor-micro-ohms property which does not apply to this sensor. At
> least this is my understanding, but of course I can be wrong (I see lm75 binding
> also uses $ref: hwmon-common.yaml# but shunt-resistor-micro-ohms does not apply).
>
Where does the idea come from that shunt-resistor-micro-ohms would be mandatory ?
That would make hwmon-common.yaml unusable for most chips.
Guenter
>> I am also not a big fan of a name like "somelabel", and a more
>> meaningful name from a "real" example would look better. I know that
>> some examples have already used "somelabel" as an example, but others
>> have used more meaningful names too.
>>
>
> I will have to send a v2 since for the label property description I used
> "channel" instead of "sensor" (detected by Sashiko AI review), so I can
> use in the example section a more meaningful name like "Room" if no other
> suggestion.
>
>> Best regards,
>> Javier Carrasco
>
> Best regards,
> Flaviu Nistor
>
^ permalink raw reply
* Re: [PATCH 1/2] iio: dac: dac8163: Add driver for DAC8163
From: Siratul Islam @ 2026-06-23 18:56 UTC (permalink / raw)
To: lukas.metz
Cc: andy, conor+dt, devicetree, dlechner, jic23, krzk+dt, linux-iio,
linux-kernel, nuno.sa, robh
In-Reply-To: <20260623-dac8163-work-v1-1-5b508158faa0@gmx.net>
On Tue, 2026-06-23 at 18:07 +0200, Lukas Metz wrote:
> The DAC756x, DAC816x, and DAC856x devices are low-power, voltage-output,
> dual-channel, 12-, 14-, and 16-bit digital-to-analog converters (DACs),
> respectively. These devices include a 2.5-V, 4-ppm/°C internal
> reference, giving a full-scale output voltage range of 2.5 V or 5 V.
>
> Signed-off-by: Lukas Metz <lukas.metz@gmx.net>
> ---
> MAINTAINERS | 6 +
> drivers/iio/dac/Kconfig | 10 ++
> drivers/iio/dac/Makefile | 1 +
> drivers/iio/dac/ti-dac8163.c | 339 +++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 356 insertions(+)
Hi! I took a quick look, and probably missed a lot of stuff. But here are my thoughts.
> diff --git a/MAINTAINERS b/MAINTAINERS
...
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DACxx6x IIO driver (SPI)
> + */
A link to the datasheet here would be nice.
> +
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>
> +#include <linux/of.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +#include <linux/iio/iio.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/units.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/printk.h>
> +#include <linux/bitfield.h>
Sort the includes alphabetically. And include what you use. "mod_devicetable.h" is missing for example.
While at it, separate the core headers from "<linux/iio/iio.h>". add ir in a sepatate line.
> +
> +#define COMMAND_MASK GENMASK(6, 3)
> +#define ADDRESS_MASK GENMASK(2, 0)
> +
> +#define COMMAND_SET(x, y) (FIELD_PREP(COMMAND_MASK, (x)) | \
> + FIELD_PREP(ADDRESS_MASK, (y)))
I'd align the FIELD_PREPs to make it look better. It may also fit in a single line.
> +
> +#define CMD_WRITE_INPUT_REG 0x0
> +#define CMD_UPDATE_DAC 0x1
> +#define CMD_WRITE_UPDATE_ALL 0x2
> +#define CMD_WRITE_UPDATE 0x3
> +#define CMD_SET_PWR_MODE 0x4
> +#define CMD_SOFT_RST 0x5
> +
> +#define CMD_LDAC_MODE 0x6
> +#define LDAC_MODE_CHANNEL_A_MASK BIT(0)
> +#define LDAC_MODE_CHANNEL_B_MASK BIT(1)
> +
> +#define CMD_SEL_REFERENCE 0x7
> +#define VOLTAGE_REFERENCE_MASK BIT(0)
> +
Group the CMD values together, also all these values would look better aligned.
> +enum dacxx6x_ldac_modes {
> + LDAC_MODE_ACTIVE = 0,
> + LDAC_MODE_INACTIVE = 1
> +};
A trailing comma would be nice.
> +
> +enum dacxx6x_voltage_reference {
> + VOLTAGE_REFERENCE_EXTERNAL = 0,
> + VOLTAGE_REFERENCE_INTERNAL = 1
> +};
Same here
> +
> +enum dacxx6x_supported_device_ids {
> + ID_DAC7562,
> + ID_DAC7563,
> + ID_DAC8162,
> + ID_DAC8163,
> + ID_DAC8562,
> + ID_DAC8563
> +};
> +
Here too.
>
> +struct dacxx6x_state {
Since the filename is dac8163.c, how about naming the functions/structs/other symbols that as well instead of dacxx6x?
> + struct spi_device *spi;
> +
How about use regmap?
> + struct regulator *vref;
> + struct gpio_desc *loaddacs;
> +
> + bool internal_ref;
> + int vref_uv;
> +
> + unsigned int cached[2];
> +
> + /*
> + * Lock to protect the state of the device from potential concurrent
> + * write accesses from userspace.
> + */
> + struct mutex lock;
> +};
> +
> +struct dacxx6x_chip_info {
> + const char *name;
> + const struct iio_chan_spec channels[2];
> +};
> +
> +#define DACXX6X_CHAN(id, resolution) \
> + { \
> + .type = IIO_VOLTAGE, .channel = (id), .output = 1, \
> + .indexed = 1, .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> + .scan_type = { .realbits = (resolution), \
> + .shift = 16 - (resolution) }, \
> + }
> +
> +static const struct dacxx6x_chip_info dacxx6x_chip_info_table[6] = {
> + [ID_DAC7562] = {
> + .name = "dac7562",
> + .channels = {
> + DACXX6X_CHAN(0, 12),
> + DACXX6X_CHAN(1, 12),
> + }
> + },
> + [ID_DAC7563] = {
> + .name = "dac7563",
> + .channels = {
> + DACXX6X_CHAN(0, 12),
> + DACXX6X_CHAN(1, 12),
> + }
> + },
> + [ID_DAC8162] = {
> + .name = "dac8162",
> + .channels = {
> + DACXX6X_CHAN(0, 14),
> + DACXX6X_CHAN(1, 14),
> + }
> + },
> + [ID_DAC8163] = {
> + .name = "dac8163",
> + .channels = {
> + DACXX6X_CHAN(0, 14),
> + DACXX6X_CHAN(1, 14),
> + }
> + },
> + [ID_DAC8562] = {
> + .name = "dac8562",
> + .channels = {
> + DACXX6X_CHAN(0, 16),
> + DACXX6X_CHAN(1, 16),
> + }
> + },
> + [ID_DAC8563] = {
> + .name = "dac8563",
> + .channels = {
> + DACXX6X_CHAN(0, 16),
> + DACXX6X_CHAN(1, 16),
> + }
> + },
> +};
> +
> +static int dacxx6x_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val,
> + int *val2, long mask)
> +{
> + struct dacxx6x_state *st;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + st = iio_priv(indio_dev);
Could this be assigned before the switch?
> + mutex_lock(&st->lock);
> + *val = st->cached[chan->channel];
> + mutex_unlock(&st->lock);
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> + st = iio_priv(indio_dev);
> + *val = st->vref_uv / MILLI; /* vref in mV */
> + *val2 = chan->scan_type.realbits;
> + return IIO_VAL_FRACTIONAL_LOG2;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int dacxx6x_write_reg(struct dacxx6x_state *st, int reg, int addr,
> + unsigned int val)
> +{
> + u8 tx[3];
> +
> + tx[0] = COMMAND_SET(reg, addr);
> + tx[1] = (val >> 8) & 0xff;
How about put_unaligned_be16?
>
> + tx[2] = val & 0xff;
> +
> + return spi_write(st->spi, tx, sizeof(tx));
> +}
> +
> +static int dacxx6x_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int val,
> + int val2, long mask)
> +{
> + struct dacxx6x_state *st = iio_priv(indio_dev);
> + struct device *dev = &st->spi->dev;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + dev_dbg(dev, "%s: val=%d val2=%d\n", __func__, val, val2);
> + if (val2 != 0)
> + return -EINVAL;
> +
> + if (val < 0 || val >= BIT(chan->scan_type.realbits))
> + return -EINVAL;
> +
> + mutex_lock(&st->lock);
> + int ret = dacxx6x_write_reg(st, CMD_WRITE_UPDATE, chan->channel,
> + (unsigned int)val
> + << chan->scan_type.shift);
This case should be enclosed { }. Also, Use guard() from "cleanup.h" instead of manual mutex lock/unlock. Here and in
other places.
> +
> + if (!ret)
> + st->cached[chan->channel] = val;
> + mutex_unlock(&st->lock);
> + return ret;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static const struct iio_info dacxx6x_iio_info = {
> + .write_raw = dacxx6x_write_raw,
> + .read_raw = dacxx6x_read_raw
Trailing comma here
> +};
> +
> +static int dacxx6x_probe(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev;
> + struct dacxx6x_state *st;
> + const struct dacxx6x_chip_info *info;
> + int ret;
Sort these in a reverse christmas tree order.
> +
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> + st->spi = spi;
> + spi_set_drvdata(spi, indio_dev);
> +
> + st->loaddacs = devm_gpiod_get_optional(&spi->dev, "ti,loaddacs",
> + GPIOD_OUT_LOW);
Vendor prefixes are not needed here.
> + if (IS_ERR(st->loaddacs))
> + return PTR_ERR(st->loaddacs);
> +
> + st->internal_ref =
> + device_property_read_bool(&spi->dev, "ti,internal-ref");
> +
> + if (!st->internal_ref) {
> + st->vref = devm_regulator_get(&spi->dev, "vref");
> + if (IS_ERR(st->vref))
> + return PTR_ERR(st->vref);
Maybe use return dev_err_probe?
> +
> + ret = regulator_enable(st->vref);
> + if (ret < 0)
> + return ret;
> + }
> +
> + mutex_init(&st->lock);
use devm_mutex_init.
> +
> + if (st->internal_ref) {
> + st->vref_uv = 2500000; /* 2.5V internal reference */
A note on where this value came from or why this was chosen, or a reference to datasheet would be better.
> + } else {
> + st->vref_uv = regulator_get_voltage(st->vref);
> + if (st->vref_uv < 0) {
> + ret = st->vref_uv;
> + goto err;
> + }
> + }
> +
You have a CMD_SOFT_RST defined but not used. Should this be used to reset before doing any configuration?
> + gpiod_set_value(st->loaddacs, 0);
> +
> + ret = dacxx6x_write_reg(st, CMD_LDAC_MODE, 0,
> + FIELD_PREP(LDAC_MODE_CHANNEL_A_MASK, LDAC_MODE_INACTIVE) |
> + FIELD_PREP(LDAC_MODE_CHANNEL_B_MASK, LDAC_MODE_INACTIVE));
> +
> + if (ret < 0)
> + goto err;
> +
> + ret = dacxx6x_write_reg(st, CMD_SEL_REFERENCE, 0,
> + FIELD_PREP(VOLTAGE_REFERENCE_MASK, st->internal_ref));
> +
> + if (ret < 0)
> + goto err;
> +
> + info = spi_get_device_match_data(spi);
> +
> + indio_dev->name = info->name;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->info = &dacxx6x_iio_info;
> + indio_dev->channels = info->channels;
> + indio_dev->num_channels = 2;
use ARRAY_SIZE(info->channels) and include linux/array_size.h
> +
> + ret = iio_device_register(indio_dev);
Use devm_iio_device_register
> + if (ret)
> + goto err;
> +
> + return 0;
> +
> +err:
> + if (!st->internal_ref)
> + regulator_disable(st->vref);
> + mutex_destroy(&st->lock);
> + return ret;
> +}
> +
> +static void dacxx6x_remove(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev = spi_get_drvdata(spi);
> + struct dacxx6x_state *st = iio_priv(indio_dev);
> +
> + iio_device_unregister(indio_dev);
Using devm would help here too. No need to unregister manually
> + mutex_destroy(&st->lock);
> + if (!st->internal_ref)
> + regulator_disable(st->vref);
> +}
> +
> +#define DACXX6X_COMPATIBLE(of_compatible, id) \
> + { \
> + .compatible = of_compatible, \
> + .data = &dacxx6x_chip_info_table[id] \
> + }
> +
> +static const struct of_device_id dacxx6x_of_match[] = {
> + DACXX6X_COMPATIBLE("ti,dac7562", ID_DAC7562),
> + DACXX6X_COMPATIBLE("ti,dac7563", ID_DAC7563),
> + DACXX6X_COMPATIBLE("ti,dac8162", ID_DAC8162),
> + DACXX6X_COMPATIBLE("ti,dac8163", ID_DAC8163),
> + DACXX6X_COMPATIBLE("ti,dac8562", ID_DAC8562),
> + DACXX6X_COMPATIBLE("ti,dac8563", ID_DAC8563),
> + {}
{} should have a space "{ }"
> +};
> +MODULE_DEVICE_TABLE(of, dacxx6x_of_match);
> +
> +static const struct spi_device_id dacxx6x_id_table[] = {
> + { "dac7562", (kernel_ulong_t)&dacxx6x_chip_info_table[ID_DAC7562] },
> + { "dac7563", (kernel_ulong_t)&dacxx6x_chip_info_table[ID_DAC7563] },
> + { "dac8162", (kernel_ulong_t)&dacxx6x_chip_info_table[ID_DAC8162] },
> + { "dac8163", (kernel_ulong_t)&dacxx6x_chip_info_table[ID_DAC8163] },
> + { "dac8562", (kernel_ulong_t)&dacxx6x_chip_info_table[ID_DAC8562] },
> + { "dac8563", (kernel_ulong_t)&dacxx6x_chip_info_table[ID_DAC8563] },
> + {}
Same here
> +};
> +
> +MODULE_DEVICE_TABLE(spi, dacxx6x_id_table);
> +
> +static struct spi_driver dacxx6x_driver = {
> + .driver = {
> + .name = "ti-dacxx6x",
Name doesn't need vendor prefix.
> + .of_match_table = dacxx6x_of_match,
> + },
> + .probe = dacxx6x_probe,
> + .remove = dacxx6x_remove,
> + .id_table = dacxx6x_id_table,
> +};
> +
No space here.
> +module_spi_driver(dacxx6x_driver);
> +
> +MODULE_AUTHOR("Lukas Metz <lukas.metz@gmx.net>");
> +MODULE_DESCRIPTION("Texas Instruments 12/14/16-bit 2-channel DAC driver");
> +MODULE_LICENSE("GPL");
Thanks
Sirat
^ permalink raw reply
* Re: [PATCH v4 12/12] iio: dac: ad5686: add gain control support
From: Jonathan Cameron @ 2026-06-23 18:51 UTC (permalink / raw)
To: Rodrigo Alencar via B4 Relay
Cc: rodrigo.alencar, Michael Auchter, linux, linux-iio, devicetree,
linux-kernel, linux-hardening, Michael Hennerich, David Lechner,
Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Philipp Zabel, Kees Cook, Gustavo A. R. Silva
In-Reply-To: <20260623-ad5686-new-features-v4-12-28962a57db0f@analog.com>
On Tue, 23 Jun 2026 11:55:52 +0100
Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@kernel.org> wrote:
> From: Rodrigo Alencar <rodrigo.alencar@analog.com>
>
> Most of the supported devices rely on a GAIN pin to control a 2x
> multiplier applied to the output voltage. Other devices, e.g. the
> single-channel ones, provides a gain control through a bit field in
> the control register. Some designs might have the GAIN pin hardwired
> to VDD/VLOGIC or GND, which would have no "gain-gpios" device property,
> being able to set "adi,range-double" if it is hardwired to VDD. The
> vref_mv field is moved down in the struct ad5686_state, so that the
> overall size increase is reduced.
>
> Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
The rest of this series looks good to me. Let's see what others think.
Take a quick look at the sashiko feedback (much of it is invalid
stuff about the supplies being added) just to check there is nothing
there. There be16 one might be real though I'm not sure the channels
are actually marked as IIO_BE - maybe I'm missing something,
Thanks,
Jonathan
^ permalink raw reply
* Re: [PATCH 0/2] Add driver for DAC8163:
From: David Lechner @ 2026-06-23 18:50 UTC (permalink / raw)
To: Andy Shevchenko, Lukas Metz
Cc: Jonathan Cameron, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-iio,
devicetree
In-Reply-To: <ajrR1jlkggGq_rrj@ashevche-desk.local>
On 6/23/26 1:35 PM, Andy Shevchenko wrote:
> On Tue, Jun 23, 2026 at 06:07:26PM +0200, Lukas Metz wrote:
>> This series adds an IIO driver for the Texas Instruments DAC7562, DAC7563,
>> DAC8162, DAC8163, DAC8562, and DAC8563 dual-channel voltage-output DACs.
>>
>> These devices are pin-compatible 12-, 14-, and 16-bit variants sharing the
>> same 24-bit SPI command interface. Each device provides two independently
>> addressable output channels and includes a 2.5 V, 4 ppm/°C internal
>> reference that can be enabled via device tree, or an external reference
>> supplied through a regulator. The register and command structure differs
>> from already existing drivers which makes adding a new driver a
>> reasonable choice in my opinion.
>>
>> The driver supports:
>> - All six device variants via a shared chip info table
>> - DAC updates in synchronous mode
>> - Configurable internal or external voltage reference
>> - Optional LDAC GPIO which has to be asserted permanently when using
>> synchronous updates.
>> - IIO_CHAN_INFO_RAW and IIO_CHAN_INFO_SCALE attributes per channel
>>
>> Datasheet (DAC8163):
>> https://www.ti.com/lit/gpn/dac8163
>
> Why do we need a brand new driver? Do we have an existing one that may be
> expanded to support this HW? (Note, not all existing drivers are under IIO
> folder, some of them might be found in hwmon, input, or drivers/misc.)
I thought the statement above is clear that there are not any compatible
drivers already. And I would not expect a DAC to have a driver in hwmon
or input since it is an output device.
>
>> The driver was tested with a DAC8163 on a custom STM32MP157F board with
>> external reference enabled.
>
^ permalink raw reply
* Re: [PATCH v4 3/3] iio: adc: Add ti-ads1110 support to ti-ads1100 driver
From: Jakub Szczudło @ 2026-06-23 18:49 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-iio, andy, antoniu.miclaus, conor+dt, devicetree, dlechner,
duje, jic23, jishnu.prakash, jorge.marques, joshua.crofts1,
krzk+dt, linusw, linux-kernel, marcelo.schmitt, mazziesaccount,
mike.looijmans, nuno.sa, robh, sakari.ailus, wens
In-Reply-To: <ajpQn8EVOx3ssxG6@ashevche-desk.local>
wt., 23 cze 2026 o 11:23 Andy Shevchenko <andriy.shevchenko@intel.com>
napisał(a):
>
> On Tue, Jun 23, 2026 at 12:15:50AM +0200, Jakub Szczudlo wrote:
> > Add ADS1110 support that have faster datarate than ADS1100, it also uses
> > internal voltage reference of 2.048V for measurement.
>
> ...
>
> > config TI_ADS1100
> > - tristate "Texas Instruments ADS1100 and ADS1000 ADC"
> > + tristate "Texas Instruments ADS1100 and similar single channel I2C ADC"
> > depends on I2C
> > help
> > - If you say yes here you get support for Texas Instruments ADS1100 and
> > - ADS1000 ADC chips.
> > + If you say yes here you get support TI ADS1100 and similar single
> > + channel I2C Analog to Digital Converters.
>
> User won't know what similar are really supported. The rule of thumb is to add
> the list of supported here as
>
> - ADS1000 (...perhaps some very short spec info...)
> - ADS1100 (...perhaps some very short spec info...)
>
>
> > This driver can also be built as a module. If so, the module will be
> > called ti-ads1100.
>
> ...
>
> > +static int ads1100_get_vref_milivolts(struct ads1100_data *data)
> > +{
> > + if (data->ads_config->has_internal_vref_only)
> > + return ADS1110_INTERNAL_REF_mV;
> > +
> > + return regulator_get_voltage(data->reg_vdd) / MILLI;
>
> For now we used "(MICRO / MILLI)" instead of "MILLI", to show the unit
> conversion.
>
> > +}
>
> ...
>
> > if (ret < 0) {
> > dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> > return ret;
>
> > + } else if (ret < 2) {
>
> Redundant 'else'.
>
> > + dev_err(&data->client->dev, "Short I2C read\n");
> > + return -EIO;
> > }
>
> ...
>
> > - microvolts = regulator_get_voltage(data->reg_vdd);
> > + microvolts = ads1100_get_vref_milivolts(data) * (MICRO / MILLI);
>
> See above, here you correctly used the existing pattern, the above is
> inconsistent and needs to be addressed.
>
> ...
>
> > + model = i2c_get_match_data(client);
> > + if (!model)
> > + return dev_err_probe(dev, -EINVAL,
> > + "Can't get device data from firmware\n");
> > +
> > + data->ads_config = (struct ads1100_config *)model;
>
> You can't drop const like this. If you need to apply modification,
> use devm_kmemdup(). Otherwise it won't work correctly if you have two different
> sensors of the same driver in the system.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
thanks for review, I will wait few days and start working on v5
best regards,
Jakub Szczudlo
^ permalink raw reply
* Re: [PATCH] dt-bindings: arm: qcom,ids: Add SoC ID for Snapdragon SDA 850
From: sashiko-bot @ 2026-06-23 18:48 UTC (permalink / raw)
To: David Heidelberg via B4 Relay; +Cc: robh, conor+dt, devicetree
In-Reply-To: <20260623-sda850-v1-1-ddd8e62c85d8@ixit.cz>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing SoC ID mapping in the qcom-socinfo driver causes incorrect sysfs machine attribute.
--
commit 40f8c20a8f811ef4d99535abc1be00a3d96ed7b4
Author: David Heidelberg <david@ixit.cz>
dt-bindings: arm: qcom,ids: Add SoC ID for Snapdragon SDA 850
This commit adds the numerical SoC ID (348) for the Qualcomm Snapdragon
SDA850 to the device tree bindings header.
> diff --git a/include/dt-bindings/arm/qcom,ids.h b/include/dt-bindings/arm/qcom,ids.h
> index 1af73c0ad41cd..4b3062f57eecd 100644
> --- a/include/dt-bindings/arm/qcom,ids.h
> +++ b/include/dt-bindings/arm/qcom,ids.h
> @@ -184,6 +184,7 @@
> #define QCOM_ID_IPQ8078 344
> #define QCOM_ID_SDM636 345
> #define QCOM_ID_SDA636 346
> +#define QCOM_ID_SDA850 348
[Severity: Medium]
Should this ID also be added to the soc_id[] array in
drivers/soc/qcom/socinfo.c?
Without adding qcom_board_id(SDA850) to the array, socinfo_machine()
will return NULL when the qcom-socinfo driver probes a Snapdragon SDA850
device, because it fails to map the SMEM build ID (348) to a string.
As a result, soc_device_register() will fall back to using
of_machine_read_model(), which exposes the device tree's board model
string in the /sys/devices/soc0/machine sysfs attribute rather than
the expected SoC name.
Does this regression break user-space tools that rely on the
Qualcomm-specific ABI expectation for the machine attribute?
> #define QCOM_ID_SDM632 349
> #define QCOM_ID_SDA632 350
> #define QCOM_ID_SDA450 351
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260623-sda850-v1-1-ddd8e62c85d8@ixit.cz?part=1
^ permalink raw reply
* [PATCH] dt-bindings: arm: qcom,ids: Add SoC ID for Snapdragon SDA 850
From: David Heidelberg via B4 Relay @ 2026-06-23 18:41 UTC (permalink / raw)
To: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
Conor Dooley
Cc: linux-arm-msm, devicetree, linux-kernel, David Heidelberg
From: David Heidelberg <david@ixit.cz>
Add SoC ID for Qualcomm Snapdragon SDA850.
Signed-off-by: David Heidelberg <david@ixit.cz>
---
Will be used by Comma three.
---
include/dt-bindings/arm/qcom,ids.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/dt-bindings/arm/qcom,ids.h b/include/dt-bindings/arm/qcom,ids.h
index 1af73c0ad41cd..4b3062f57eecd 100644
--- a/include/dt-bindings/arm/qcom,ids.h
+++ b/include/dt-bindings/arm/qcom,ids.h
@@ -179,16 +179,17 @@
#define QCOM_ID_SDM450 338
#define QCOM_ID_SM8150 339
#define QCOM_ID_SDA845 341
#define QCOM_ID_IPQ8072 342
#define QCOM_ID_IPQ8076 343
#define QCOM_ID_IPQ8078 344
#define QCOM_ID_SDM636 345
#define QCOM_ID_SDA636 346
+#define QCOM_ID_SDA850 348
#define QCOM_ID_SDM632 349
#define QCOM_ID_SDA632 350
#define QCOM_ID_SDA450 351
#define QCOM_ID_SDM439 353
#define QCOM_ID_SDM429 354
#define QCOM_ID_SM8250 356
#define QCOM_ID_SA8155 362
#define QCOM_ID_SDA439 363
---
base-commit: 948efecf22e49aa4bf55bb73ec79a0ddcfd38571
change-id: 20260623-sda850-6b5c1e44662c
Best regards,
--
David Heidelberg <david@ixit.cz>
^ permalink raw reply related
* Re: [PATCH 0/2] Add driver for DAC8163:
From: Andy Shevchenko @ 2026-06-23 18:35 UTC (permalink / raw)
To: Lukas Metz
Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-kernel,
linux-iio, devicetree
In-Reply-To: <20260623-dac8163-work-v1-0-5b508158faa0@gmx.net>
On Tue, Jun 23, 2026 at 06:07:26PM +0200, Lukas Metz wrote:
> This series adds an IIO driver for the Texas Instruments DAC7562, DAC7563,
> DAC8162, DAC8163, DAC8562, and DAC8563 dual-channel voltage-output DACs.
>
> These devices are pin-compatible 12-, 14-, and 16-bit variants sharing the
> same 24-bit SPI command interface. Each device provides two independently
> addressable output channels and includes a 2.5 V, 4 ppm/°C internal
> reference that can be enabled via device tree, or an external reference
> supplied through a regulator. The register and command structure differs
> from already existing drivers which makes adding a new driver a
> reasonable choice in my opinion.
>
> The driver supports:
> - All six device variants via a shared chip info table
> - DAC updates in synchronous mode
> - Configurable internal or external voltage reference
> - Optional LDAC GPIO which has to be asserted permanently when using
> synchronous updates.
> - IIO_CHAN_INFO_RAW and IIO_CHAN_INFO_SCALE attributes per channel
>
> Datasheet (DAC8163):
> https://www.ti.com/lit/gpn/dac8163
Why do we need a brand new driver? Do we have an existing one that may be
expanded to support this HW? (Note, not all existing drivers are under IIO
folder, some of them might be found in hwmon, input, or drivers/misc.)
> The driver was tested with a DAC8163 on a custom STM32MP157F board with
> external reference enabled.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2 0/4] Introduce HONOR MagicBook Art 14 device tree
From: Valentin Manea @ 2026-06-23 18:27 UTC (permalink / raw)
To: mail
Cc: andersson, conor+dt, devicetree, konradybcio, krzk+dt,
linux-arm-msm, linux-kernel, robh
In-Reply-To: <20260515172926.16597-1-mail@etehtsea.me>
Hi
> I'm using it with an external display connected to the HDMI port as > a daily driver for quite some time now. > > eDP support is broken
(screen is eigher grey or grey with noise), > thus, keeping it disabled
for now.
The reason for the eDP problems is that the firmware enables DSC for
the panel at boot, but the msm driver does not yet support it. There are
some patches floating around to add the functionality but they seem to
have some problems. In the repo with my name that you quoted I have
integrated them for testing, but as they are they already have been
rejected for mainline, so I think we just need to wait for qualcomm or
linaro to finish their work.
Now I have a simpler solution for this problem, I just made a copy of the
one of the eDP panel drivers and do a force disable of DSC on resume,
which seems to work perfectly fine, there is enough bandwidth to
drive the panel even at 120Hz even with DSC disabled. You can find my
patch in the quoted repository in the qcom-x1e-7.0 branch.
If this is an acceptable solution I will cleanup the patch more and
post it here. Perhaps the other solution is for the msm driver to
disable DSC explicitly for panels that advertise it, because it might
be enabled by bootloaders/firmware.
Valentin
^ permalink raw reply
* Re: [PATCH v2 05/12] nvmem: microchip-otpc: add tag-based packet lookup
From: Andy Shevchenko @ 2026-06-23 18:31 UTC (permalink / raw)
To: Varshini Rajendran
Cc: ehristev, jic23, dlechner, nuno.sa, andy, robh, krzk+dt, conor+dt,
nicolas.ferre, alexandre.belloni, claudiu.beznea, srini,
linux-iio, devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <20260623105944.128840-6-varshini.rajendran@microchip.com>
On Tue, Jun 23, 2026 at 04:29:37PM +0530, Varshini Rajendran wrote:
> Add support for accessing OTP packets by their 4-byte ASCII tag while
> preserving backward compatibility with the existing ID-based lookup.
>
> The OTP memory layout can vary across devices and may change over time,
> making the packet ID approach unreliable when the memory map is not
> known in advance. The packet tag provides a reliable way to identify
> and access packets without prior knowledge of the OTP memory layout.
>
> Two offset encoding are now supported:
> 1. Legacy ID-based: offset = OTP_PKT(id) = id * 4
> Used in DT as: reg = <OTP_PKT(1) 76>;
> 2. TAG-based: offset = 4-byte ASCII packet tag
> Used in DT as: reg = <0x41435354 0x4c>; (tag "ACST")
>
> The driver resolves offsets matching valid legacy selectors (multiples
> of 4 within the packet count) through ID lookup, falling back to tag
> lookup for other values. This ensures existing device trees continue
> to work while enabling new tag-based access.
>
> During probe, packet meta data including the tag is read and cached.
> The driver also validates OTP memory accessibility and emulation mode
> status. When the boot packet is not configured, emulation mode allows
> access to the other packets. When both are not available an
> informational message is logged.
>
> The stride of the nvmem memory is set to 1 in order to support tag based
> offsets, comment in the header file is updated accordingly.
...
> #define MCHP_OTPC_SIZE (11 * 1024)
Side note: At some point maybe (11 * SZ_1K) with help of sizes.h?
...
> +/**
> + * mchp_otpc_resolve_packet() - resolve offset to packet
> + * @otpc: OTPC private data
> + * @off: NVMEM offset (legacy ID-based or TAG-based)
> + *
> + * Legacy offsets (multiples of 4 within valid ID range) are resolved
> + * through ID lookup. Other offsets are treated as 4-byte ASCII tags.
> + *
> + * Return: pointer to packet if found, NULL otherwise
> + */
> +static struct mchp_otpc_packet *mchp_otpc_resolve_packet(struct mchp_otpc *otpc,
> + u32 off)
> +{
> + /*
> + * Legacy id based packet access: offset = id * 4
> + * Inside the driver we use continuous unsigned integer numbers
> + * for packet id, thus divide off by 4 before passing it to
> + * mchp_otpc_id_to_packet().
> + */
> +
> + if (!(off % 4) && (off / 4) < otpc->npackets)
> + return mchp_otpc_id_to_packet(otpc, off / 4);
Hmm... I was thinking about something like temporary variables for these two.
Note, in some cases the compiler may issue a single instruction when it sees
both together. That's why in many GPIO drivers we use something like
unsigned int offset = foo / 8;
unsigned int shift = foo % 8;
or similar.
> + /*
> + * TAG-based packet access: offset is a 4-byte ASCII tag
> + */
> + return mchp_otpc_tag_to_packet(otpc, off);
> +}
...
> {
> struct nvmem_device *nvmem;
> struct mchp_otpc *otpc;
> - u32 size;
> + u32 size, tmp;
> int ret;
> + bool emul_enable;
Perhaps keep the reversed xmas tree order?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2 03/12] iio: adc: at91-sama5d2_adc: adapt the driver for sama7d65
From: Andy Shevchenko @ 2026-06-23 18:22 UTC (permalink / raw)
To: Varshini Rajendran
Cc: ehristev, jic23, dlechner, nuno.sa, andy, robh, krzk+dt, conor+dt,
nicolas.ferre, alexandre.belloni, claudiu.beznea, srini,
linux-iio, devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <20260623105944.128840-4-varshini.rajendran@microchip.com>
On Tue, Jun 23, 2026 at 04:29:35PM +0530, Varshini Rajendran wrote:
> Add support for sama7d65 ADC. The differences are highlighted with the
> compatible. The calibration data layout is the main difference.
Do you need to update a Kconfig help text?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re:[PATCH 1/2] dt-bindings: hwmon: chipcap2: Add label property
From: Flaviu Nistor @ 2026-06-23 18:16 UTC (permalink / raw)
To: Javier Carrasco
Cc: Guenter Roeck, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Jonathan Corbet, Shuah Khan, Flaviu Nistor, linux-hwmon,
linux-kernel, devicetree, linux-doc
In-Reply-To: <DJFPYCV2FXW7.1BFG9DURPZRCC@gmail.com>
On Mon Jun 22, 2026 at 7:29 PM CEST, Javier Carrasco wrote:
>On Mon Jun 22, 2026 at 2:21 PM CEST, Flaviu Nistor wrote:
>> Add support for an optional label property similar to other hwmon devices
>> This allows, in case of boards with multiple CHIPCAP2 sensors, to assign
>> distinct names to each instance.
>>
>> Signed-off-by: Flaviu Nistor <flaviu.nistor@gmail.com>
>> ---
>> .../devicetree/bindings/hwmon/amphenol,chipcap2.yaml | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/amphenol,chipcap2.ya=
>ml b/Documentation/devicetree/bindings/hwmon/amphenol,chipcap2.yaml
>> index 17351fdbefce..f00b5a4b14dd 100644
>> --- a/Documentation/devicetree/bindings/hwmon/amphenol,chipcap2.yaml
>> +++ b/Documentation/devicetree/bindings/hwmon/amphenol,chipcap2.yaml
>> @@ -33,6 +33,10 @@ properties:
>> reg:
>> maxItems: 1
>>
>> + label:
>> + description:
>> + A descriptive name for this channel, like "ambient" or "psu".
>> +
>> interrupts:
>> items:
>> - description: measurement ready indicator
>> @@ -72,6 +76,7 @@ examples:
>> <5 IRQ_TYPE_EDGE_RISING>,
>> <6 IRQ_TYPE_EDGE_RISING>;
>> interrupt-names =3D "ready", "low", "high";
>> + label =3D "somelabel";
>> vdd-supply =3D <®_vdd>;
>> };
> };
>
>Hello Falviu, thank you for your patch.
>
Hello Javier, thanks for your reply.
>Should we not add a reference to hwmon-common.yaml (with
>unevelautedProperties instead of additionalProperties), as label is
>defined there? I believe that Krzysztof Kozlowski did something similar
>for the shunt-resistor-micro-ohms property. Could we follow suit here?
>
This is a good question and I am happy you asked. I also thought a lot
about this and the reason I decided to go for this approach is that by using
$ref: hwmon-common.yaml#, I would have to change additionalProperties: false
to unevaluatedProperties: false, which will evaluate in case it is used, also
shunt-resistor-micro-ohms property which does not apply to this sensor. At
least this is my understanding, but of course I can be wrong (I see lm75 binding
also uses $ref: hwmon-common.yaml# but shunt-resistor-micro-ohms does not apply).
>I am also not a big fan of a name like "somelabel", and a more
>meaningful name from a "real" example would look better. I know that
>some examples have already used "somelabel" as an example, but others
>have used more meaningful names too.
>
I will have to send a v2 since for the label property description I used
"channel" instead of "sensor" (detected by Sashiko AI review), so I can
use in the example section a more meaningful name like "Room" if no other
suggestion.
>Best regards,
>Javier Carrasco
Best regards,
Flaviu Nistor
^ permalink raw reply
* Re: [PATCH v2 02/12] iio: adc: at91-sama5d2_adc: rework temp calibration layout handling
From: Andy Shevchenko @ 2026-06-23 18:15 UTC (permalink / raw)
To: Varshini Rajendran
Cc: ehristev, jic23, dlechner, nuno.sa, andy, robh, krzk+dt, conor+dt,
nicolas.ferre, alexandre.belloni, claudiu.beznea, srini,
linux-iio, devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <20260623105944.128840-3-varshini.rajendran@microchip.com>
On Tue, Jun 23, 2026 at 04:29:34PM +0530, Varshini Rajendran wrote:
> Extend support to handle different temperature calibration layouts.
>
> Add a temperature calibration data layout structure to describe indexes
> of the factors P1, P4, P6, tag, minimum length of the packet and the
> scaling factors for P1 (mul, div) which are SoC-specific instead of the
> older non scalable id structure. This helps handle the differences in the
> same function flow and prepare the calibration data to be applied. Add
> additional condition to validate the calibration data read from the
> NVMEM cell using the TAG of the packet.
>
> Use cleanup helpers for NVMEM data buffer wherever applicable.
> +#define AT91_TEMP_CALIB_TAG_ACST 0x41435354
This seems like FourCC that can be represented in ASCII (I suppose "ACST").
Can you add a short comment on top to decode that?
/* ...blablabla... in ASCII "ACST" */
...
> struct at91_adc_platform {
> const struct at91_adc_reg_layout *layout;
> @@ -481,6 +504,7 @@ struct at91_adc_platform {
> unsigned int chan_realbits;
> unsigned int temp_chan;
> bool temp_sensor;
> + const struct at91_adc_temp_calib_layout *temp_calib_layout;
> };
Is this the best placement in accordance with `pahole` tool?
...
> - u32 *buf;
> + u32 *buf __free(kfree) = NULL;
This looks like a separate change. Why is it in this patch?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH V13 5/9] iio: imu: inv_icm42607: Add PM support for icm42607
From: Chris Morgan @ 2026-06-23 18:08 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Chris Morgan, linux-iio, andy, nuno.sa, dlechner,
jean-baptiste.maneyrol, linux-rockchip, devicetree, heiko,
conor+dt, krzk+dt, robh, andriy.shevchenko
In-Reply-To: <ajnwk_zWevY14mEO@wintermute.localhost.fail>
On Mon, Jun 22, 2026 at 09:34:10PM -0500, Chris Morgan wrote:
> On Sun, Jun 21, 2026 at 06:19:48PM +0100, Jonathan Cameron wrote:
> > On Mon, 15 Jun 2026 12:25:48 -0500
> > Chris Morgan <macroalpha82@gmail.com> wrote:
> >
> > > From: Chris Morgan <macromorgan@hotmail.com>
> > >
> > > Add power management support for the ICM42607 device driver.
> > >
> > > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> > A few things from taking a look at the sashiko report:
> > https://sashiko.dev/#/patchset/20260615172554.160910-1-macroalpha82%40gmail.com
> >
> > > ---
> > > drivers/iio/imu/inv_icm42607/inv_icm42607.h | 18 +++
> > > .../iio/imu/inv_icm42607/inv_icm42607_core.c | 139 ++++++++++++++++++
> > > .../iio/imu/inv_icm42607/inv_icm42607_i2c.c | 1 +
> > > .../iio/imu/inv_icm42607/inv_icm42607_spi.c | 1 +
> > > 4 files changed, 159 insertions(+)
> > >
> > > diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607.h b/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> > > index a6a58571935f..4f4f541027dc 100644
> > > --- a/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> > > +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> >
> > > @@ -334,11 +345,18 @@ struct inv_icm42607_state {
> > > #define INV_ICM42607_GYRO_STOP_TIME_MS 45
> > > #define INV_ICM42607_TEMP_STARTUP_TIME_MS 77
> > >
> > > +/*
> > > + * Suspend delay assumed from other icm42600 series device, not
> > > + * documented in datasheet.
> > > + */
> > > +#define INV_ICM42607_SUSPEND_DELAY_MS (2 * USEC_PER_MSEC)
> >
> > Sashiko had a valid comment on this. MSEC_PER_SEC seems more
> > appropriate given this is 2 seconds in milli seconds.
> >
> > > +
> > > typedef int (*inv_icm42607_bus_setup)(struct inv_icm42607_state *);
> > >
> > > extern const struct regmap_config inv_icm42607_regmap_config;
> > > extern const struct inv_icm42607_hw inv_icm42607_hw_data;
> > > extern const struct inv_icm42607_hw inv_icm42607p_hw_data;
> > > +extern const struct dev_pm_ops inv_icm42607_pm_ops;
> > >
> > > int inv_icm42607_core_probe(struct regmap *regmap,
> > > const struct inv_icm42607_hw *hw,
> > > diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> > > index 4b8e19091786..64f5d263de4f 100644
> > > --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> > > +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> > > @@ -4,6 +4,7 @@
> > > */
> > >
> > > #include <linux/bitfield.h>
> > > +#include <linux/cleanup.h>
> > > #include <linux/delay.h>
> > > #include <linux/dev_printk.h>
> > > #include <linux/device/devres.h>
> > > @@ -11,6 +12,7 @@
> > > #include <linux/iio/iio.h>
> > > #include <linux/module.h>
> > > #include <linux/mutex.h>
> > > +#include <linux/pm_runtime.h>
> > > #include <linux/regmap.h>
> > > #include <linux/regulator/consumer.h>
> > > #include <linux/time.h>
> > > @@ -103,6 +105,63 @@ const struct inv_icm42607_hw inv_icm42607p_hw_data = {
> > > };
> > > EXPORT_SYMBOL_NS_GPL(inv_icm42607p_hw_data, "IIO_ICM42607");
> > >
> > > +static int inv_icm42607_set_pwr_mgmt0(struct inv_icm42607_state *st,
> > > + enum inv_icm42607_sensor_mode gyro,
> > > + enum inv_icm42607_sensor_mode accel,
> > > + bool temp, unsigned int *sleep_ms)
> > > +{
> > > + enum inv_icm42607_sensor_mode oldaccel = st->conf.accel.mode;
> > > + enum inv_icm42607_sensor_mode oldgyro = st->conf.gyro.mode;
> > > + bool oldtemp = st->conf.temp_en;
> > > + unsigned int sleepval_ms;
> > > + unsigned int val;
> > > + int ret;
> > > +
> > > + if (gyro == oldgyro && accel == oldaccel && temp == oldtemp)
> > > + return 0;
> > > +
> > > + /*
> > > + * Datasheet on page 14.26 says we need to ensure the gyro sensor is on
> > > + * for a minimum of 45ms. So if we transition from an on state to an
> > > + * off state wait 45ms to ensure a sufficient pause before power off.
> >
> > Sashiko commented on this.. I think what we could do with adding to the
> > comment is what the path is that didn't pass through this function which would
> > ensure we have been on for 30 of this msecs already.
>
> I'm going to track whatever time the gyro started, and then if less
> than 45ms has elapsed just pause the remaining amount of time.
>
> >
> > > + */
> > > + if (!gyro && oldgyro)
> > > + fsleep(INV_ICM42607_GYRO_STOP_TIME_MS * USEC_PER_MSEC);
> > > +
> > > + val = FIELD_PREP(INV_ICM42607_PWR_MGMT0_GYRO_MODE_MASK, gyro);
> > > + val |= FIELD_PREP(INV_ICM42607_PWR_MGMT0_ACCEL_MODE_MASK, accel);
> > > + ret = regmap_write(st->map, INV_ICM42607_REG_PWR_MGMT0, val);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + st->conf.gyro.mode = gyro;
> > > + st->conf.accel.mode = accel;
> > > + st->conf.temp_en = temp;
> > > +
> > > + /*
> > > + * If a state change occurs from off to on, sleep for the startup
> > > + * time of the sensor, unless a sleep_ms is specified. Since more
> > > + * than one sensor can be transitioned from off to on, select the
> > > + * maximum time from each of the sensors changing from off to on.
> > > + */
> > > + sleepval_ms = 0;
> > > + if (temp && !oldtemp)
> > > + sleepval_ms = max(sleepval_ms, INV_ICM42607_TEMP_STARTUP_TIME_MS);
> > > +
> > > + if (accel && !oldaccel)
> > > + sleepval_ms = max(sleepval_ms, INV_ICM42607_ACCEL_STARTUP_TIME_MS);
> > > +
> > > + if (gyro && !oldgyro)
> > > + sleepval_ms = max(sleepval_ms, INV_ICM42607_GYRO_STARTUP_TIME_MS);
> > > +
> > > + if (sleep_ms)
> > > + *sleep_ms = sleepval_ms;
> > > + else if (sleepval_ms)
> > > + fsleep(sleepval_ms * USEC_PER_MSEC);
> > > +
> > > + return 0;
> > > +}
> >
> > >
> > > int inv_icm42607_core_probe(struct regmap *regmap,
> > > @@ -236,6 +305,8 @@ int inv_icm42607_core_probe(struct regmap *regmap,
> > > if (!st)
> > > return -ENOMEM;
> > >
> > > + dev_set_drvdata(dev, st);
> > > +
> > > ret = devm_mutex_init(dev, &st->lock);
> > > if (ret)
> > > return ret;
> > > @@ -271,10 +342,78 @@ int inv_icm42607_core_probe(struct regmap *regmap,
> > > if (ret)
> > > return ret;
> > >
> > > + ret = devm_pm_runtime_set_active_enabled(dev);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + pm_runtime_set_autosuspend_delay(dev, INV_ICM42607_SUSPEND_DELAY_MS);
> > > + pm_runtime_use_autosuspend(dev);
> > Sashiko does put out some stuff here. Please take a look and work out or
> > test if it is right (I think not but haven't checked that carefully!)
> > From a quick look I think that the auto disabling of autosuspend does a
> > rpm_idle() that should result in it suspending...
> >
>
> I see a few other drivers adding one more call to
> devm_pm_runtime_enable() so I'm going to see how that works out.
Obviously that didn't work, but digging in much deeper into the PM code
I see that devm_pm_runtime_set_active_enabled() returns
devm_pm_runtime_enable(), which sets devm_add_action_or_reset() on
pm_runtime_disable_action(), which calls
pm_runtime_dont_use_autosuspend() and pm_runtime_disable(). Shouldn't
this work, or am I missing something? Basically when the driver
detaches shouldn't this chain end up calling
pm_runtime_dont_use_autosuspend()?
Thank you,
Chris
>
> >
> > > +
> > > return 0;
> > > }
> > > EXPORT_SYMBOL_NS_GPL(inv_icm42607_core_probe, "IIO_ICM42607");
>
> Thank you,
> Chris
^ permalink raw reply
* Re: [PATCH 5/5] arm64: dts: qcom: hamoa: Extend QMPPHY description for USB4
From: Dmitry Baryshkov @ 2026-06-23 17:58 UTC (permalink / raw)
To: Konrad Dybcio
Cc: Vinod Koul, Neil Armstrong, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Bjorn Andersson, linux-kernel, linux-phy,
linux-arm-msm, devicetree, usb4-upstream, Raghavendra Thoorpu,
Mika Westerberg, Sven Peter, Konrad Dybcio
In-Reply-To: <20260518-topic-usb4phy-v1-5-71d827c49dca@oss.qualcomm.com>
On Mon, May 18, 2026 at 12:29:52PM +0200, Konrad Dybcio wrote:
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>
> The USB4 part of the QMPPHY requires that one more GCC clock (P2RR2P -
> PHY-to-Router, Router-to-PHY) is enabled for the PHY to initialize
> successfully. Describe that.
>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> ---
> arch/arm64/boot/dts/qcom/hamoa.dtsi | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH 4/5] phy: qualcomm: qmp-combo: Add USB4/TBT3 configuration data for Hamoa
From: Dmitry Baryshkov @ 2026-06-23 17:58 UTC (permalink / raw)
To: Konrad Dybcio
Cc: Vinod Koul, Neil Armstrong, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Bjorn Andersson, linux-kernel, linux-phy,
linux-arm-msm, devicetree, usb4-upstream, Raghavendra Thoorpu,
Mika Westerberg, Sven Peter, Konrad Dybcio
In-Reply-To: <20260518-topic-usb4phy-v1-4-71d827c49dca@oss.qualcomm.com>
On Mon, May 18, 2026 at 12:29:51PM +0200, Konrad Dybcio wrote:
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>
> Add the offsets and configuration tables to support USB4 and
> Thunderbolt 3 operation on the USB4-capable PHYs found on Hamoa chips.
>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> ---
> drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 200 ++++++++++++++++++++-
> drivers/phy/qualcomm/phy-qcom-qmp-pcs-aon-v6.h | 2 +
> drivers/phy/qualcomm/phy-qcom-qmp-pcs-usb-v6.h | 15 ++
> .../phy/qualcomm/phy-qcom-qmp-qserdes-txrx-v6_n4.h | 45 +++++
> 4 files changed, 256 insertions(+), 6 deletions(-)
>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v4 2/3] iio: health: add MAX86150 ECG and PPG biosensor driver
From: sashiko-bot @ 2026-06-23 17:57 UTC (permalink / raw)
To: Md Shofiqul Islam; +Cc: devicetree, robh, conor+dt
In-Reply-To: <20260623174600.17100-3-shofiqtest@gmail.com>
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The driver fails to assign `indio_dev->trig`, causing `iio_trigger_validate_own_device` to unconditionally reject the trigger.
- [High] Using a hard IRQ handler for the IIO trigger without interrupt masking causes a system-locking interrupt storm for level-triggered interrupts.
- [Medium] Timestamp reconstruction assigns inaccurate timestamps into the past when the hardware FIFO overflows because it ignores the `OVF_COUNTER` dropped sample count.
- [Medium] The I2C DMA buffer `fifo_raw` shares a cacheline with the pushed CPU buffer `buf`, violating DMA API constraints.
- [Medium] A blind sleep in `read_raw` risks reading an empty FIFO if the internal oscillator or device wake-up is slower than nominal.
--
commit 44deffd1bb30e8d4384d605ba5d156fb29678ad5
Author: Md Shofiqul Islam <shofiqtest@gmail.com>
iio: health: add MAX86150 ECG and PPG biosensor driver
The MAX86150 (Maxim/Analog Devices) combines two PPG optical channels
(Red/IR LED) and one ECG biopotential channel in a single I2C device with
a 32-entry hardware FIFO.
> diff --git a/drivers/iio/health/max86150.c b/drivers/iio/health/max86150.c
> new file mode 100644
> index 0000000000000..0140c93bd4ec9
> --- /dev/null
> +++ b/drivers/iio/health/max86150.c
[ ... ]
> +struct max86150_data {
> + struct regmap *regmap;
> + struct device *dev;
> + struct iio_trigger *trig;
> + u32 sample_period_ns;
> + u8 fifo_raw[MAX86150_SAMPLE_BYTES] __aligned(ARCH_DMA_MINALIGN);
> + s32 buf[6] __aligned(8);
> +};
[Severity: Medium]
Since fifo_raw is 9 bytes long, does the __aligned macro only align the
start of the array, leaving the buf array packed into the same DMA cacheline?
Mapping an unpadded buffer that shares a cacheline with CPU-accessed data
might violate kernel DMA mapping constraints and trigger
CONFIG_DMA_API_DEBUG warnings.
[ ... ]
> +static int max86150_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
[ ... ]
> + /* Wait for one complete sample period at 100 Hz (<= 10 ms) */
> + usleep_range(11000, 13000);
> +
> + ret = max86150_read_one_sample(data, &ppg_red, &ppg_ir, &ecg);
[Severity: Medium]
Is it possible for the hardware read to execute while the FIFO is still
empty if the internal oscillator or device wake-up is slower than nominal?
Would it be safer to use regmap_read_poll_timeout on the PPG_RDY status
bit to ensure a sample is actually ready?
[ ... ]
> +static irqreturn_t max86150_trigger_handler(int irq, void *p)
> +{
[ ... ]
> + /*
> + * Anchor timestamps to the A_FULL IRQ capture time: sample index
> + * (MAX86150_A_FULL_SAMPLES - 1) corresponds to pf->timestamp.
> + * Samples that accumulated between the IRQ and handler execution
> + * receive timestamps in the future relative to the IRQ, eliminating
> + * load-dependent jitter in multi-sample drains.
> + */
> + for (i = 0; i < n_avail; i++) {
> + s64 ts = pf->timestamp +
> + (s64)(i - (MAX86150_A_FULL_SAMPLES - 1)) *
> + data->sample_period_ns;
[Severity: Medium]
When the FIFO overflows (ovf > 0), the hardware overwrites the oldest
samples. Does this calculation assign inaccurate timestamps into the past
because it ignores the OVF_COUNTER dropped sample count?
The remaining 32 samples are chronologically shifted forward by ovf relative
to the original A_FULL IRQ event.
[ ... ]
> +static int max86150_probe(struct i2c_client *client)
> +{
[ ... ]
> + /*
> + * If the device tree provides an interrupt, set up a hardware
> + * trigger so userspace can use the FIFO almost-full signal to
> + * drive capture without polling.
> + */
> + if (client->irq > 0) {
> + unsigned long irq_trig;
> +
> + data->trig = devm_iio_trigger_alloc(&client->dev,
> + "%s-dev%d",
> + indio_dev->name,
> + iio_device_id(indio_dev));
> + if (!data->trig)
> + return -ENOMEM;
> +
> + data->trig->ops = &max86150_trigger_ops;
> + iio_trigger_set_drvdata(data->trig, indio_dev);
[Severity: High]
Does the driver fail to assign indio_dev->trig = data->trig after
allocating the trigger?
Since validate_trigger is set to iio_trigger_validate_own_device, it looks
like missing this assignment will cause any attempt to write to the
current_trigger sysfs node to unconditionally fail with -EINVAL.
> +
> + /*
> + * Honour the interrupt trigger type from the device tree.
> + * Fall back to falling-edge if the DT does not specify one.
> + */
> + irq_trig = irq_get_trigger_type(client->irq);
> + if (!irq_trig)
> + irq_trig = IRQF_TRIGGER_FALLING;
> +
> + ret = devm_request_irq(&client->dev, client->irq,
> + iio_trigger_generic_data_rdy_poll,
> + irq_trig,
> + "max86150", data->trig);
[Severity: High]
Will using a hard IRQ handler for the IIO trigger without interrupt masking
cause an interrupt storm for level-triggered interrupts?
For level-triggered interrupts, the hard handler wakes the consumer thread
and returns IRQ_HANDLED without clearing the physical interrupt via I2C.
The IRQ core then unmasks the still-asserted interrupt line, causing it to
immediately fire again and preventing the threaded consumer from ever
running to clear the source.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260623174600.17100-1-shofiqtest@gmail.com?part=2
^ permalink raw reply
* Re: [PATCH 3/5] phy: qualcomm: qmp-combo: Add preliminary USB4 support
From: Dmitry Baryshkov @ 2026-06-23 17:56 UTC (permalink / raw)
To: Konrad Dybcio
Cc: Konrad Dybcio, Vinod Koul, Neil Armstrong, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, linux-kernel,
linux-phy, linux-arm-msm, devicetree, usb4-upstream,
Raghavendra Thoorpu, Mika Westerberg, Sven Peter
In-Reply-To: <3972248c-acfc-4b31-8c99-69bfdba34b8c@oss.qualcomm.com>
On Tue, Jun 16, 2026 at 01:44:48PM +0200, Konrad Dybcio wrote:
> On 5/28/26 10:00 AM, Dmitry Baryshkov wrote:
> > On Fri, May 22, 2026 at 02:05:14PM +0200, Konrad Dybcio wrote:
> >> On 5/20/26 5:06 PM, Dmitry Baryshkov wrote:
> >>> On Tue, May 19, 2026 at 10:12:06AM +0200, Konrad Dybcio wrote:
> >>>> On 5/18/26 5:38 PM, Dmitry Baryshkov wrote:
> >>>>> On Mon, May 18, 2026 at 04:15:16PM +0200, Konrad Dybcio wrote:
> >>>>>> On 5/18/26 3:57 PM, Dmitry Baryshkov wrote:
> >>>>>>> On Mon, May 18, 2026 at 12:29:50PM +0200, Konrad Dybcio wrote:
> >>>>>>>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> >>>>>>>>
> >>>>>>>> Some Combo PHYs (so far only on SC8280XP, X1E80100 and Glymur), come in
> >>>>>>>> a flavor called USB43DP, which as the name implies, features USB4, USB3
> >>>>>>>> and DP signal processing capabilities. In that architecture, USB3 and
> >>>>>>>> USB4 PHYs share the same USB_PLL while featuring separate logic spaces.
> >>>>>>>> The DP part is roughly the same as on the instances without USB4.
> >>>>>>>>
> >>>>>>>> The USB4 and USB3/DP operation modes of the PHY are mutually exclusive.
> >>>>>>>> Only one USB protocol (and flavor of pipe clock) can be active at a
> >>>>>>>> given moment (not to be confused with USB3 not being able to be
> >>>>>>>> tunneled as USB4 packets - that of course remains possible).
> >>>>>>>> The DP PLL is still used for clocking tunneled DP links. It may be
> >>>>>>>> turned off to save power when no tunnels are active, but that's left as
> >>>>>>>> a TODO item for now.
> >>>>>>>>
> >>>>>>>> Due to the nature of USB4, the Type-C handling happens entirely inside
> >>>>>>>> the Host Router, and as such the QMPPHY's mux_set() function is
> >>>>>>>> nullified for the period when USB4 PHY remains active. This is strictly
> >>>>>>>> necessary, as the Host Router driver is going to excercise manual
> >>>>>>>> control over the USB4 PHY's power state, which is needed by the suspend
> >>>>>>>> and resume flows. Failure to control that synchronously with other
> >>>>>>>> parts of the code results in a SoC crash by unlocked access.
> >>>>>>>>
> >>>>>>>> Because of that, a new struct phy is spawned to expose the USB4 mode,
> >>>>>>>> along with a .set_mode callback to allow toggling between USB4 and TBT3
> >>>>>>>> submodes.
> >>>>>>>>
> >>>>>>>> Thunderbolt 3, having a number of differences vs USB4, requires a
> >>>>>>>> couple specific overrides, pertaining to electrical characteristics,
> >>>>>>>> which are easily accommodated for.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> >>>>>>>> ---
> >>>>>>>> drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 392 ++++++++++++++++++++++++------
> >>>>>>>> 1 file changed, 322 insertions(+), 70 deletions(-)
> >>>>>>>>
> >>>>>>>
> >>>>>>> Overall it looks good. The major question (after looking at TODOs), do
> >>>>>>> we need a separate submode for USB+DP / TBT+DP?
> >>>>>>
> >>>>>> The problem space is as follows:
> >>>>>>
> >>>>>> After a TBT (collectively TBT3+ and USB4) link has been established and
> >>>>>> we have a link partner, we may (based on the HW capabilities and user
> >>>>>> config, such as kernel params but not only) start or stop a DP tunnel at
> >>>>>> runtime. On Qualcomm hardware, the PHY is kept in USB4 mode and its DP
> >>>>>> AUX lines are not used (instead, the encapsulated DP AUX packets are r/w
> >>>>>> entirely within the USB4 subsystem via a pair of FIFOs that Linux sees
> >>>>>> as a separate DP AUX host)
> >>>>>
> >>>>> So far so good. But I still don't grok if having a DP-over-USB4 is a
> >>>>> separate submode or not. I.e. I see code (and TODOs) to detect and
> >>>>> handle DP going on and off. Would it be better if we specify that
> >>>>> explicitly?
> >>>>
> >>>> I really don't want to end up in a situation like we have with:
> >>>>
> >>>> $ rg _USB include/linux/phy/phy.h
> >>>> 29: PHY_MODE_USB_HOST,
> >>>> 30: PHY_MODE_USB_HOST_LS,
> >>>> 31: PHY_MODE_USB_HOST_FS,
> >>>> 32: PHY_MODE_USB_HOST_HS,
> >>>> 33: PHY_MODE_USB_HOST_SS,
> >>>> 34: PHY_MODE_USB_DEVICE,
> >>>> 35: PHY_MODE_USB_DEVICE_LS,
> >>>> 36: PHY_MODE_USB_DEVICE_FS,
> >>>> 37: PHY_MODE_USB_DEVICE_HS,
> >>>> 38: PHY_MODE_USB_DEVICE_SS,
> >>>> 39: PHY_MODE_USB_OTG,
> >>>>
> >>>>>> Then, on hamoa/glymur specifically, any of the 3 USB4-capable DP hosts
> >>>>>> can be muxed to either of the 2 DPIN ports on any of the 3 USB4 routers
> >>>>>> (and each of these routers is hardwired to one of the PHYs).
> >>>>>>
> >>>>>> To underline, we have 3 DP producers and 6 consumers. If there's e.g. a
> >>>>>> super high-res display at one of the physical ports, or a long
> >>>>>> daisy-chain, we may need to use 2 DPTXes to service 1 receptacle. Then,
> >>>>>> we would only need one of the PHYs (associated with the router that's
> >>>>>> wired to that port) to provide a DP clock.
> >>>>>>
> >>>>>> This, along with the normal (logical or physical) present/absent status
> >>>>>> can change at runtime. My plan is to use phy_set_opts(dp_tunelling=true)
> >>>>>> or something along those lines to toggle that bit as necessary
> >>>>>
> >>>>> I don't see phy_set_opts(). So maybe a submode then...
> >>>>
> >>>> Sorry, I misremembered the name. The function is phy_configure(), and it
> >>>> takes a union phy_configure_opts, hence the confusion
> >>>
> >>> So, phy_configure() will be called for the DP PHY to set the DP opts,
> >>> but how do you plan to determine if DP is on or not? Or do you plan to
> >>> add phy_tbt_configure_opts ?
> >>>
> >>> Another obvious option would be to set the flag if DP PHY is being tuned
> >>> on / off. I don't know if that fulfills your needs.
> >>
> >> Either this or tbt_configure_opts. We still have the muxing question to
> >> chew through.
> >>
> >> The bottom line is that all AUX traffic happens between the "AUX adapters"
> >> within USB4SS, talking over thunderbolt to other AUX adapters on the LTTPRs
> >> and the far-end device (and anything inbetween in a chained topology) meaning
> >> we only need to engage the DP host itself (and therefore the PHY) after we've
> >> already performed the capability negotiations
> >
> > I hope you mean USB link capabilities. DP host still needs to ping LTTPRs
> > and read all the DP properties on its own. I don't think we want to leak
> > that to the other layers.
>
> I must crush your hopes.
>
> There's some preliminary TBT-layer setup (handled by the tbt driver in
> Linux), followed by the expected DPCD (and alike) r/w accesses, which on
> our hw must happen through the DP adapters housed inside USB4SS (again,
> because the DPTX's auxbus is NOPed out). Think of it as just another
> i2c_aux provider.
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
--
With best wishes
Dmitry
^ 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