* Re: [PATCH v9 2/9] lib: vsprintf: export simple_strntoull() in a safe prototype
From: Jonathan Cameron @ 2026-04-12 15:05 UTC (permalink / raw)
To: Rodrigo Alencar
Cc: Andy Shevchenko, Petr Mladek, rodrigo.alencar, linux-kernel,
linux-iio, devicetree, linux-doc, David Lechner, Andy Shevchenko,
Lars-Peter Clausen, Michael Hennerich, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, Andrew Morton,
Steven Rostedt, Rasmus Villemoes, Sergey Senozhatsky, Shuah Khan
In-Reply-To: <mnz7d2zd27x6h2qa24rajgrbhkhsypybadkqz2fi43rg7bvjvj@oufys7xs25t4>
On Tue, 31 Mar 2026 14:01:04 +0100
Rodrigo Alencar <455.rodrigo.alencar@gmail.com> wrote:
> On 26/03/30 01:49PM, Rodrigo Alencar wrote:
> > On 26/03/27 03:17PM, Rodrigo Alencar wrote:
> > > On 26/03/27 12:21PM, Andy Shevchenko wrote:
> > > > On Fri, Mar 27, 2026 at 10:11:56AM +0000, Rodrigo Alencar wrote:
> > > > > On 26/03/27 11:17AM, Andy Shevchenko wrote:
> > > > > > On Fri, Mar 27, 2026 at 09:45:17AM +0100, Petr Mladek wrote:
> > > > > > > On Fri 2026-03-20 16:27:27, Rodrigo Alencar via B4 Relay wrote:
> >
> > ...
> >
> > > > > > Maybe we want to have kstrtof32() and kstrtof64() for these two cases?
> > > > > >
> > > > > > With that we will always consider the fraction part as 32- or 64-bit,
> > > > > > imply floor() on the fraction for the sake of simplicity and require
> > > > > > it to be NUL-terminated with possible trailing '\n'.
> > > > >
> > > > > I think this is a good idea, but calling it float or fixed point itself
> > > > > is a bit confusing as float often refers to the IEEE 754 standard and
> > > > > fixed point types is often expressed in Q-format.
> > > >
> > > > Yeah... I am lack of better naming.
> > >
> > > decimals is the name, but they are often represented as:
> > >
> > > DECIMAL = INT * 10^X + FRAC
> > >
> > > in a single 64-bit number, which would be fine for my end use case.
> > > However IIO decimal fixed point parsing is out there for quite some time a
> > > lot of drivers use that. The interface often relies on breaking parsed values
> > > into an integer array (for standard attributes int val and int val2 are expected).
> >
> > Thinking about this again and in IIO drivers we end up doing something like:
> >
> > val64 = (u64)val * MICRO + val2;
> >
> > so that drivers often work with scaled versions of the decimal value.
> > then, would it make sense to have a function that already outputs such value?
> > That would allow to have more freedom over the 64-bit split between integer
> > and fractional parts.
> > As a draft:
> >
> > static int _kstrtodec64(const char *s, unsigned int scale, u64 *res)
> > {
> > u64 _res = 0, _frac = 0;
> > unsigned int rv;
> >
> > if (*s != '.') {
> > rv = _parse_integer(s, 10, &_res);
> > if (rv & KSTRTOX_OVERFLOW)
> > return -ERANGE;
> > if (rv == 0)
> > return -EINVAL;
> > s += rv;
> > }
> >
> > if (*s == '.') {
> > s++;
> > rv = _parse_integer_limit(s, 10, &_frac, scale);
> > if (rv & KSTRTOX_OVERFLOW)
> > return -ERANGE;
> > if (rv == 0)
> > return -EINVAL;
> > s += rv;
> > if (rv < scale)
> > _frac *= int_pow(10, scale - rv);
> > while (isdigit(*s)) /* truncate */
> > s++;
> > }
> >
> > if (*s == '\n')
> > s++;
> > if (*s)
> > return -EINVAL;
> >
> > if (check_mul_overflow(_res, int_pow(10, scale), &_res) ||
> > check_add_overflow(_res, _frac, &_res))
> > return -ERANGE;
> >
> > *res = _res;
> > return 0;
> > }
> >
> > noinline
> > int kstrtoudec64(const char *s, unsigned int scale, u64 *res)
> > {
> > if (s[0] == '+')
> > s++;
> > return _kstrtodec64(s, scale, res);
> > }
> > EXPORT_SYMBOL(kstrtoudec64);
> >
> > noinline
> > int kstrtosdec64(const char *s, unsigned int scale, s64 *res)
> > {
> > u64 tmp;
> > int rv;
> >
> > if (s[0] == '-') {
> > rv = _kstrtodec64(s + 1, scale, &tmp);
> > if (rv < 0)
> > return rv;
> > if ((s64)-tmp > 0)
> > return -ERANGE;
> > *res = -tmp;
> > } else {
> > rv = kstrtoudec64(s, scale, &tmp);
> > if (rv < 0)
> > return rv;
> > if ((s64)tmp < 0)
> > return -ERANGE;
> > *res = tmp;
> > }
> > return 0;
> > }
> > EXPORT_SYMBOL(kstrtosdec64);
> >
> > e.g., kstrtosdec64() or kstrtoudec64() parses "3.1415" with scale 3 into 3141
>
> Hi Jonathan,
>
> developing more on that, I wouldn't need to create a iio_str_to_fixpoint64(),
> what do you think on new format types:
>
> #define IIO_VAL_DECIMAL64_1 101
> #define IIO_VAL_DECIMAL64_2 102
> #define IIO_VAL_DECIMAL64_3 103
> #define IIO_VAL_DECIMAL64_4 104
> #define IIO_VAL_DECIMAL64_5 105
> #define IIO_VAL_DECIMAL64_6 106
> #define IIO_VAL_DECIMAL64_7 107
> #define IIO_VAL_DECIMAL64_8 108
> #define IIO_VAL_DECIMAL64_9 109
> #define IIO_VAL_DECIMAL64_10 110
> #define IIO_VAL_DECIMAL64_11 111
> #define IIO_VAL_DECIMAL64_12 112
> #define IIO_VAL_DECIMAL64_13 113
> #define IIO_VAL_DECIMAL64_14 114
> #define IIO_VAL_DECIMAL64_15 115
Seems unlikely more than a few of these would ever be used.
If you want to keep the offsets define them in terms
of first one + whatever makes sense (maybe with a base value
to make that easier - then MICRO is BASE + 6 etc)
>
> #define IIO_VAL_DECIMAL64_MILLI IIO_VAL_DECIMAL64_3
> #define IIO_VAL_DECIMAL64_MICRO IIO_VAL_DECIMAL64_6
> #define IIO_VAL_DECIMAL64_NANO IIO_VAL_DECIMAL64_9
> #define IIO_VAL_DECIMAL64_PICO IIO_VAL_DECIMAL64_12
> #define IIO_VAL_DECIMAL64_FEMTO IIO_VAL_DECIMAL64_15
>
> which gets stored as 64-bit, and represent the decimal scaled value.
> That would also work for the PLL driver (using IIO_VAL_DECIMAL64_MICRO):
> - It supports frequency range from 1 to 26 GHz with micro Hz resolution
> - In the driver a 64-bit value: (val * MICRO + val2) is already created
> anyways.
> I would leverage something like kstrtodec64() in iio_write_channel_info().
>
> That way, I would drop the changes on the iio fixpoint parse, which I think
> it would do better with something like kstrntoull() to be able to handle that
> "dB" suffix.
>
> So for now, I may have the following approaches:
> - new kstrntoull() function: to have control over the parsing, whithout
> requiring NUL-termination, avoiding unecessary string scanning or copying.
> covered in v8.
> - expose a "safe" simple_strntoull(): minimal changes to vsprintf.c, this
> is covered by this patch series (v9), and it similar solution to kstrntoull().
> - new kstrtodec64() function: parse decimal numbers as 64-bit with NUL-termination.
> Might be covered in a v10, if it is a good idea.
>
> let me know your thoughts.
For string parsing I'm not a particular expert, so I only really care
about useability of the end result. Having types would work, but I'd kind
of want an 'odd ones' to stand out. Hence only defining them for parsing
cases we already support (but with 64 bit values) would be my preference
if you got ahead with this new approach.
Jonathan
>
^ permalink raw reply
* Re: [PATCH RFC v2 9/9] docs: iio: add documentation for ad9910 driver
From: Jonathan Cameron @ 2026-04-12 14:54 UTC (permalink / raw)
To: Rodrigo Alencar
Cc: Rodrigo Alencar via B4 Relay, rodrigo.alencar, linux-iio,
devicetree, linux-kernel, linux-doc, Lars-Peter Clausen,
Michael Hennerich, David Lechner, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Philipp Zabel, Jonathan Corbet,
Shuah Khan
In-Reply-To: <g57xnbhtvu25dvzy5b5pz76yzalpcjpnyclob6dy6j7fphxqle@5vww7psogbv3>
On Mon, 23 Mar 2026 11:58:53 +0000
Rodrigo Alencar <455.rodrigo.alencar@gmail.com> wrote:
> On 26/03/22 05:34PM, Jonathan Cameron wrote:
> > On Wed, 18 Mar 2026 17:56:09 +0000
> > Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@kernel.org> wrote:
> >
> > > From: Rodrigo Alencar <rodrigo.alencar@analog.com>
> > >
> > > Add documentation for the AD9910 DDS IIO driver, which describes channels,
> > > DDS modes, attributes and ABI usage examples.
> > >
> > > Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
>
> ...
>
> > > +Channel hierarchy
> > > +=================
> > > +
> > > +The driver exposes the following IIO output channels, each identified by a
> > > +unique channel number and a human-readable label:
> > > +
> > > +* ``out_altvoltage100``: ``phy``: Physical output: system clock and profile control
> > > +
> > > + * ``out_altvoltage110``: ``single_tone``: Single tone mode: per-profile
> > > + frequency, phase, amplitude
> > > +
> > > + * ``out_altvoltage120``: ``parallel_port``: Parallel port modulation: enable
> > > + and offset/scale parameters
> > > +
> > > + * ``out_altvoltage130``: ``digital_ramp_generator``: DRG control: enable,
> > > + destination, operating mode
> > > +
> > > + * ``out_altvoltage131``: ``digital_ramp_up``: DRG ramp-up parameters:
> > > + limits, step sizes, ramp rate
> > > + * ``out_altvoltage132``: ``digital_ramp_down``: DRG ramp-down parameters:
> > > + limits, step sizes, ramp rate
> > > +
> > > + * ``out_altvoltage140``: ``ram_control``: RAM playback: enable, destination,
> > > + operating mode, address range
> > > +
> > > + * ``out_altvoltage150``: ``output_shift_keying``: OSK: enable, amplitude
> > > + scale, ramp rate, auto/manual control
> > > +
> > > +The ``phy`` channel is the root of the hierarchy. Changing its
> > > +``sampling_frequency`` reconfigures the system clock (SYSCLK) which affects all
> > > +other channels. The ``profile`` attribute on this channel selects the active
> > > +hardware profile (0-7) used by the single tone and RAM channels.
> > I asked out this profile thing in one of the other patches. Key to me is
> > that how we write non active profiles? The most similar thing we've seen
> > in the past has been setting other frequencies for FSK or phases for PSK or
> > more mundane DC DAC output that are symbol based. (often an external signal)
>
> Yes, not allowing to configure a non-active profile at this point.
> Initially was not seeing this as a problem, but would you think different
> channels for each profiles should be created? e.g.:
>
> - out_altvoltage111 ... out_altvoltage118 for single tone profiles; and
> - out_altvoltage141 .. out_altvoltage148 for RAM profiles
>
> That would not remove the need for something like the profile attribute.
Yes. I'm thinking it would be something along those lines.
I think the tone and RAM profiles end up with totally different controls
so this should make it clear they aren't related.
>
> > For those we have added an additional index so we can see which symbol we
> > are changing parameters for. Here it might need to be done in the channel
> > numbering. I'm not sure.
> >
> > > +
> > > +All mode-specific channels (parallel port, DRG, RAM, OSK) have an ``enable``
> > > +attribute. The DRG and RAM channels additionally have ``destination`` and
> > > +``operating_mode`` attributes that configure which DDS core parameter is
> > > +modulated and how.
> >
> > I wonder if we flatten things out and have separate channels for each type
> > of modulation. Might lead to a more standard looking interfaces. We don't really
> > have a standard path to control one type of thing feeding another, whereas
> > we do have simple 'enable' interfaces.
>
> ...
>
> > > +RAM mode
> > > +--------
> > > +
> > > +The AD9910 contains a 1024 x 32-bit RAM that can be loaded with waveform data
> > > +and played back to modulate frequency, phase, amplitude, or polar (phase +
> > > +amplitude) parameters.
> > > +
> > > +RAM control channel attributes
> > > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > +
> > > +.. flat-table::
> > > + :header-rows: 1
> > > +
> > > + * - Attribute
> > > + - Unit
> > > + - Description
> > > +
> > > + * - ``out_altvoltage140_en``
> > > + - boolean
> > > + - Enable/disable RAM playback. Toggling swaps profile registers between
> > > + single tone and RAM configurations across all 8 profiles.
>
> ...
>
> > > + * - ``out_altvoltage140_address_start``
> > Do we need this flexibility to set the start? We needed a length, but
> > if we want different effective start can just load a different image.
>
> There is one image being loaded into the entire RAM and each profile may choose
> from wich sample it starts and ends its address ramp.
> The profiles can have those address ranges to overlap or not, then I suppose
> considering different images would just complicate things.
In the ABI docs discussion I raised the question on whether it makes more
sense to push this 'meta data' into the firmware image. Depends a bit
on how costly loading the image is vs just updating this properties and whether
there are significant real use cases where tweaking what is run from the
image apply. I can easily conjecture some. I just have no idea if they are
real!
Thanks,
Jonathan
>
> > > + - integer
> > > + - Start address for the active profile. Range [0, 1023]. Cannot be
> > > + changed while RAM mode is enabled. If set above current end address,
> > > + end address is automatically adjusted.
> > > +
>
> ...
>
^ permalink raw reply
* Re: [PATCH RFC v2 8/9] Documentation: ABI: testing: add docs for ad9910 sysfs entries
From: Jonathan Cameron @ 2026-04-12 14:51 UTC (permalink / raw)
To: Rodrigo Alencar
Cc: Rodrigo Alencar via B4 Relay, rodrigo.alencar, linux-iio,
devicetree, linux-kernel, linux-doc, Lars-Peter Clausen,
Michael Hennerich, David Lechner, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Philipp Zabel, Jonathan Corbet,
Shuah Khan
In-Reply-To: <mtqjtmsysz6ywvybeut6qzhee2o4qedwgvr5isbn4um7bwhjbe@sg2b7hwlszwd>
On Mon, 23 Mar 2026 11:36:08 +0000
Rodrigo Alencar <455.rodrigo.alencar@gmail.com> wrote:
> On 26/03/22 05:22PM, Jonathan Cameron wrote:
> > On Wed, 18 Mar 2026 17:56:08 +0000
> > Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@kernel.org> wrote:
> >
> > > From: Rodrigo Alencar <rodrigo.alencar@analog.com>
> > >
> > > Add ABI documentation file for the DDS AD9910 with sysfs entries to
> > > control Parallel Port, Digital Ramp Generator, RAM and OSK parameters.
> > >
> > > Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
> > > ---
>
> ...
>
> > > +What: /sys/bus/iio/devices/iio:deviceX/out_altvoltageY_profile
> > > +KernelVersion:
> > > +Contact: linux-iio@vger.kernel.org
> > > +Description:
> > > + Read/write the active profile index [0, 7] from/to the physical
> > > + channel. The AD9910 supports 8 profiles, each storing a complete
> > > + set of single tone (frequency, phase, amplitude) and RAM playback
> > > + parameters.
> >
> > This one is interesting. Can we treat them as symbols that we are picking
> > between? We have similar DAC ABIs for that already.
>
> The profile concept comes from the datasheet and defines sets of configuration
> for single tone and RAM control mode. I am not sure how we fit this idea into a
> "symbol"
Think of those tones as just different frequencies (the other stuff could be the same)
then this is FSK with 8 symbols.
>
> > Is this picking between them for purposes of configuration or setting which one is
> > in being output currently?
>
> Well, this is being used for configuration and activating, then, yes, you can only configure
> an active profile, but I was not seeing that as an issue. I suppose that simplifies the
> ABI a bit.
If you are only configuring the one that is active, short of a small overhead
of having to configure more than one property why have this at all?
Just leave it in a profile and have userspace reconfigure everything it wants to.
However, I see that in some of them modes below this is more complex
as the profiles are cycled through - for ram playback anyway.
It may make sense to separate the ram case from tone ones.
>
> ...
>
> > > +What: /sys/bus/iio/devices/iio:deviceX/out_altvoltageY_destination
> > > +KernelVersion:
> > > +Contact: linux-iio@vger.kernel.org
> > > +Description:
> > > + Read/write the digital ramp generator (DRG) or the RAM control
> > > + destination parameter. Determines which DDS core parameter is to
> > > + be modulated when the child mode channel is enabled.
> > > +
> > > + Available values can be read from the corresponding
> > > + out_altvoltageY_destination_available attribute.
> > > +
> > > + Valid values: "polar" (only for RAM control), "frequency", "phase"
> > > + and "amplitude"
> >
> > This is very device specific. Maybe we are better representing these as separate
> > channels each with their own controls for DRG. No problem if changing one changes
> > another.
>
> You mean removing this generic Y there? Indeed, there are separate configs for each one.
No, I mean having more channels. One for each of polar, frequency, phase and frequency for
each channel. Then enables for which channel is turned on. Might not work out,
but I'd like you to explore what problems that type of interface would bring.
The aim here is to add as little new ABI as possible as custom ABI is a real
pain for generic userspace.
>
> ...
>
> > > +What: /sys/bus/iio/devices/iio:deviceX/out_altvoltageY_operating_mode
> > > +KernelVersion:
> > > +Contact: linux-iio@vger.kernel.org
> > > +Description:
> > > + Read/write the DRG or RAM control operating mode. For the DRG
> > > + channel it controls the no-dwell behavior of the ramp.
> > > +
> > > + Available values can be read from the corresponding
> > > + out_altvoltageY_operating_mode_available attribute.
> > > +
> > > + Valid values for DRG channel:
> > > +
> > > + - "bidirectional": Normal ramp generation (ramp up then
> > > + down, dwelling at limits).
> >
> > Some sort of trapezium wave? Maybe this and continuous forms are combined
> > and we have a separate dwell time control?
>
> Yes, sort of. Dwell control is made by an external pin (DRCTL), often controlled
> by an FPGA to achieve certain required timings. I can say that software control
> is not really recommended, unless only a one-shot ramp is necessary.
So is it worth exposing this software control at all? Maybe just make it a firmware
description problem as to whether that DRCTL is connected or not.
>
> When adding the IIO backend support, extendend attributes will be added to
> support control of dwell times.
>
> >
> > > + - "ramp_down": No-dwell low; the ramp resets to upper
> > > + limit upon reaching the lower limit.
> > > + - "ramp_up": No-dwell high; the ramp resets to lower
> > > + limit upon reaching the upper limit.
> > > + - "bidirectional_continuous": Both no-dwell high and low;
> > > + the ramp continuously sweeps without dwelling.
> >
> > Triangle wave? bidirectional continuous is a rather confusing term so maybe
> > we should rethink this one.
>
> Mostly yes, but not only that. Sawtooth can be achieved as well by changing
> the step sizes, also other weird patterns can be achieved by toggling DRCTL pin.
Sawtooth is kind of a special triangle wave with one very steep side.
Wikipedia even has: "It can also be considered the extreme case of an asymmetric triangle wave"
https://en.wikipedia.org/wiki/Sawtooth_wave
> This mode is the most useful when one does not have an FPGA and want to save
> resources on controlling the DRCTL pin. That mode name comes from the datasheet,
> so I suppose it was fine.
Let us see if we can get more opinions on this. Whilst I can see the logic of
the datasheet naming, it's a bit obscure.
>
> > > +
> > > + Valid values for RAM control channel:
> > > +
> > > + - "direct_switch": start address defines fixed word to be used
> > > + by the selected profile.
> > > + - "ramp_up": One-shot ramp up through current profile's address
> > > + range.
> > > + - "bidirectional": Ramp up then down through PROFILE0 pin.
> >
> > Avoid specifics like this. Can we call external control pin or something like that?
> >
> > > + - "bidirectional_continuous": Continuous ramp up/down
> > > + through current profile's address range.
> > > + - "ramp_up_continuous": Continuous ramp up through
> > > + current profile's address range.
> >
> > I guess this goes back to start on finishing ramping up?
>
> Yes, any dwell time should be considered when loading the "waveform" into the RAM
>
> >
> > > + - "sequenced": Sequenced playback of RAM profiles up to
> > > + the active profile. Requires active profile > 0.
> > Is this just running through each profile one after another? (other than profile 0)?
>
> for this, this would be the actions:
> - configure all desired profiles (0 up to X)
> - Set the operating mode to sequenced (profile X would be active at this point)
> - Enable RAM mode
Ah. This reflects on the profile control above. As I mention there I'm not sure we
shouldn't separate the use of profile for tones (where it is symbol like) to that
for RAM addresses.
For the ram addresses I think I'd expose separate attributes for each (there aren't
that many) rather than a selector + controls.
Another option would be to push the control of this into the firmware files
(some sort of header). Whether that is sufficient would depend on the usecases
for the device. It would definitely make for an easier runtime configuration
though!
>
> When RAM mode is enabled, it would trigger the execution of profiles 0 up to X,
> in sequence, according to the configured address range and sample rate for each profile.
> When one profile ends the next starts until profile X finishes.
>
> > > + - "sequenced_continuous": Continuous sequenced playback
> > > + of RAM profiles up to the active profile. Requires
> > > + active profile > 0.
> > Similar to above, maybe separate out dwell time if that's the difference between
> > sequenced and sequenced_continuous.
>
> The difference here is that when Profile X finishes, Profile 0 starts again.
> So the previous one is kind of an one-shot mode of multiple profiles in sequence
They had fun designing this didn't they!
Anyhow, I think we'll need to work through a few more versions of this to get
as extensible an interface as possible.
>
> ...
>
^ permalink raw reply
* Re: [PATCH v8 2/3] spi: spacemit: introduce SpacemiT K1 SPI controller driver
From: Alex Elder @ 2026-04-12 14:23 UTC (permalink / raw)
To: Mark Brown, Guodong Xu
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Yixun Lan,
Alex Elder, Philipp Zabel, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, linux-spi, devicetree, linux-riscv,
spacemit, linux-kernel
In-Reply-To: <adkhJhEQIZHgIQpH@sirena.co.uk>
On 4/10/26 11:11 AM, Mark Brown wrote:
> On Fri, Apr 10, 2026 at 11:04:21PM -0400, Guodong Xu wrote:
>>
>> This patch introduces the driver for the SPI controller found in the
>> SpacemiT K1 SoC. Currently the driver supports master mode only.
>> The SPI hardware implements RX and TX FIFOs, 32 entries each, and
>> supports both PIO and DMA mode transfers.
Caveat: I haven't really looked closely at this code for
a few months, but I thought all issues had been addressed.
I was wrong... Guodong will be addressing your comments but
I wanted to weigh in.
>> +static struct dma_async_tx_descriptor *
>> +k1_spi_dma_prep(struct k1_spi_driver_data *drv_data,
>> + struct spi_transfer *transfer, bool tx)
>> +{
>> + phys_addr_t addr = drv_data->base_addr + SSP_DATAR;
>> + u32 burst_size = K1_SPI_THRESH * drv_data->bytes;
>> + struct dma_slave_config cfg = { };
>> + enum dma_transfer_direction dir;
>> + enum dma_slave_buswidth width;
>> + struct dma_chan *chan;
>> + struct sg_table *sgt;
>> +
>> + width = drv_data->bytes == 1 ? DMA_SLAVE_BUSWIDTH_1_BYTE :
>> + drv_data->bytes == 2 ? DMA_SLAVE_BUSWIDTH_2_BYTES :
>> + /* bytes == 4 */ DMA_SLAVE_BUSWIDTH_4_BYTES;
>
> Please use normal conditional statements (in this case a case statement)
> to keep the code legible.
>
>> +static irqreturn_t k1_spi_ssp_isr(int irq, void *dev_id)
>> +{
>> + struct k1_spi_driver_data *drv_data = dev_id;
>> + u32 val;
>
>> + /* Return immediately if we're not expecting any interrupts */
>> + if (!drv_data->transfer)
>> + return IRQ_NONE;
>
> That does't mean the hardware agrees!
You're right. We need to clear whatever caused the
interrupt it or we'll keep getting interrupted. This
obviously didn't happen during testing but thanks for
mentioning this.
>> + /* Get status and clear pending interrupts; all are handled below */
>> + val = readl(drv_data->base + SSP_STATUS);
>> + writel(val, drv_data->base + SSP_STATUS);
>
> Nothing after here can report IRQ_NONE, even if SSP_STATUS didn't flag
> anything. I'd just move the checks for transfer to when we're handling
> FIFOs and have the IRQ_NONE report be based on there being something set
> in the ISR.
Sounds good.
>> + /*
>> + * For SPI, bytes are transferred in both directions equally, and
>> + * RX always follows TX. Start by writing if there is anything to
>> + * write, then read. Once there's no more to read, we're done.
>> + */
>> + if (drv_data->tx_resid && (val & SSP_STATUS_TNF)) {
>> + /* If we finish writing, disable TX interrupts */
>> + if (k1_spi_write(drv_data, val)) {
>> + val = SSP_INT_EN_RX | SSP_INT_EN_ERROR;
>> + writel(val, drv_data->base + SSP_INT_EN);
>> + }
>> + }
>
> This overwrites val...
That's no good. We need to assign the interrupt status to a
different variable if things are going to be handled this way.
>
>> +
>> + /* We're not done unless we've read all that was requested */
>> + if (drv_data->rx_resid) {
>> + /* Read more if there FIFO is not empty */
>> + if (val & SSP_STATUS_RNE)
>> + if (k1_spi_read(drv_data, val))
>> + goto done;
>
> ...so the read won't see that there's data to read and we'll need
> another interrupt. I would suggest using a more meaingful name for the
> actual interrupt status.
Yes. I actually think you commented on this before, and I thought
I had addressed it but it's clear I did not.
Thanks for your review. Sorry for not fixing everything.
-Alex
^ permalink raw reply
* Re: [PATCH v2 13/13] arm64: defconfig: Enable I3C and SPD5118 hwmon
From: Krzysztof Kozlowski @ 2026-04-12 13:33 UTC (permalink / raw)
To: Guenter Roeck, Akhil R
Cc: Frank.Li, acpica-devel, alexandre.belloni, conor+dt, devicetree,
ebiggers, krzk+dt, lenb, linux-acpi, linux-hwmon, linux-i3c,
linux-kernel, miquel.raynal, p.zabel, rafael, robh, sakari.ailus,
wsa+renesas
In-Reply-To: <ef05d6fd-97d9-4795-9626-e69895e5df74@kernel.org>
On 12/04/2026 15:32, Krzysztof Kozlowski wrote:
> On 11/04/2026 09:20, Guenter Roeck wrote:
>> On 4/10/26 22:34, Akhil R wrote:
>> [ ... ]
>>>>>> And it
>>>>>> should bring me clear rule what I can or cannot remove from defconfig,
>>>>>> if in 2 years I come and start pruning it from symbols.
>>>
>>> I am still a little confused on what information would likely accept (and
>>> keep) these configs in the defconfig. Would updating the commit message
>>> as below work?
>>>
>>> "These configs enable the support for SPD5118 within the
>>> Small-Outline-Compression-Attached Memory Modules (SOCAMM) LPDDR5X found
>>> in the NVIDIA Vera CPUs. The Vera CPU uses ACPI and is part of platforms
>>> such as Vera Rubin."
>>>
>>
>> It is quite interesting that we argue about SPD5118 which is mandatory in
>> DDR5 systems. At the same time, CONFIG_IGB_HWMON, CONFIG_SENSORS_MACSMC_HWMON,
>> CONFIG_SENSORS_RASPBERRYPI_HWMON, and CONFIG_RTC_DRV_DS3232_HWMON _are_
>> enabled in arm64:defconfig. CONFIG_IGB_HWMON is even built-in.
>
> Why CONFIG_SENSORS_MACSMC_HWMON is weird? It is part of the soc using
> the defconfig?
>
> The author here has troubles bringing any arguments why his drivers
> should be defconfig and keeps asking what do I want to hear. If one
> cannot make an argument why a change is needed, then maybe the change
> should not be sent?
>
> It's the job of the author to convince why the community needs this
> change, unless it is obvious, ofc.
>
>>
>> It is kind of difficult to understand why those are more important than
>> the temperature sensor on DDR5 modules (or the temperature sensor on DDR4
>> modules, for that matter).
>
> No one discussed this. I have no clue what is SPD5118 and commit msg did
> not explain that. Did not even provide accurate user of that.
>
>>
>> I don't know what the policy for defconfig is, but just based on that it does
>> seem to lack consistency.
>
> No wonder... people write poor commits and send that to upstream. And
> when asked "why do we want this" they got stuck.
>
>>
>> A separate question is if it is time to enable I3C in default configurations.
>> I'd think so - more and more chip vendors support it, and presumably they would
>> not invest in it if there was no demand, but that is just my personal opinion.
>
> Isn't I3C needed for SPD5118. Otherwise I understand even less from this
> rationale - why I3C is being enabled here?
>
> And before author asks what do I want to here: no, it is author's job to
> convince me to accept I3C in defconfig. Not mine.
BTW, all this was asked at v1 and author did not improve the commit msg
beside giving quite broad/unspecific "Vera".
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 13/13] arm64: defconfig: Enable I3C and SPD5118 hwmon
From: Krzysztof Kozlowski @ 2026-04-12 13:32 UTC (permalink / raw)
To: Guenter Roeck, Akhil R
Cc: Frank.Li, acpica-devel, alexandre.belloni, conor+dt, devicetree,
ebiggers, krzk+dt, lenb, linux-acpi, linux-hwmon, linux-i3c,
linux-kernel, miquel.raynal, p.zabel, rafael, robh, sakari.ailus,
wsa+renesas
In-Reply-To: <d0f1f053-589a-4681-8c8f-8e4b5daec145@roeck-us.net>
On 11/04/2026 09:20, Guenter Roeck wrote:
> On 4/10/26 22:34, Akhil R wrote:
> [ ... ]
>>>>> And it
>>>>> should bring me clear rule what I can or cannot remove from defconfig,
>>>>> if in 2 years I come and start pruning it from symbols.
>>
>> I am still a little confused on what information would likely accept (and
>> keep) these configs in the defconfig. Would updating the commit message
>> as below work?
>>
>> "These configs enable the support for SPD5118 within the
>> Small-Outline-Compression-Attached Memory Modules (SOCAMM) LPDDR5X found
>> in the NVIDIA Vera CPUs. The Vera CPU uses ACPI and is part of platforms
>> such as Vera Rubin."
>>
>
> It is quite interesting that we argue about SPD5118 which is mandatory in
> DDR5 systems. At the same time, CONFIG_IGB_HWMON, CONFIG_SENSORS_MACSMC_HWMON,
> CONFIG_SENSORS_RASPBERRYPI_HWMON, and CONFIG_RTC_DRV_DS3232_HWMON _are_
> enabled in arm64:defconfig. CONFIG_IGB_HWMON is even built-in.
Why CONFIG_SENSORS_MACSMC_HWMON is weird? It is part of the soc using
the defconfig?
The author here has troubles bringing any arguments why his drivers
should be defconfig and keeps asking what do I want to hear. If one
cannot make an argument why a change is needed, then maybe the change
should not be sent?
It's the job of the author to convince why the community needs this
change, unless it is obvious, ofc.
>
> It is kind of difficult to understand why those are more important than
> the temperature sensor on DDR5 modules (or the temperature sensor on DDR4
> modules, for that matter).
No one discussed this. I have no clue what is SPD5118 and commit msg did
not explain that. Did not even provide accurate user of that.
>
> I don't know what the policy for defconfig is, but just based on that it does
> seem to lack consistency.
No wonder... people write poor commits and send that to upstream. And
when asked "why do we want this" they got stuck.
>
> A separate question is if it is time to enable I3C in default configurations.
> I'd think so - more and more chip vendors support it, and presumably they would
> not invest in it if there was no demand, but that is just my personal opinion.
Isn't I3C needed for SPD5118. Otherwise I understand even less from this
rationale - why I3C is being enabled here?
And before author asks what do I want to here: no, it is author's job to
convince me to accept I3C in defconfig. Not mine.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v7 2/2] dt-bindings: embedded-controller: Add synology microp devices
From: Krzysztof Kozlowski @ 2026-04-12 13:22 UTC (permalink / raw)
To: Markus Probst
Cc: Hans de Goede, Ilpo Järvinen, Bryan O'Donoghue,
Lee Jones, Pavel Machek, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Greg Kroah-Hartman, platform-driver-x86, linux-leds,
devicetree, linux-kernel, rust-for-linux
In-Reply-To: <485ab9e829e902e3f29172059be8c3203062d06b.camel@posteo.de>
On 12/04/2026 15:21, Markus Probst wrote:
> On Sun, 2026-04-12 at 10:26 +0200, Krzysztof Kozlowski wrote:
>> On Sat, Apr 11, 2026 at 05:27:35PM +0200, Markus Probst wrote:
>>> +properties:
>>> + compatible:
>>> + enum:
>>> + - synology,ds923p-microp
>>> + - synology,ds918p-microp
>>> + - synology,ds214play-microp
>>> + - synology,ds225p-microp
>>> + - synology,ds425p-microp
>>> + - synology,ds710p-microp
>>> + - synology,ds1010p-microp
>>> + - synology,ds723p-microp
>>> + - synology,ds1522p-microp
>>> + - synology,rs422p-microp
>>> + - synology,ds725p-microp
>>> + - synology,ds118-microp
>>> + - synology,ds124-microp
>>> + - synology,ds223-microp
>>> + - synology,ds223j-microp
>>> + - synology,ds1823xsp-microp
>>> + - synology,rs822p-microp
>>> + - synology,rs1221p-microp
>>> + - synology,rs1221rpp-microp
>>> + - synology,ds925p-microp
>>> + - synology,ds1525p-microp
>>> + - synology,ds1825p-microp
>>
>> Previous comment is not resolved. For example you stated that ds723p is
>> compatible with ds725p, so this should be expressed.
> Using this expression?
>
> properties:
> compatible:
> oneOf:
> - enum:
> - synology,ds923p-microp
> - synology,ds1522p-microp
> - enum:
> - synology,ds918p-microp
> - synology,ds415p-microp
> - const: synology,ds214play-microp
> ...
> ?
> If so shall there each be a description?
No, you changed nothing. You need fallbacks, please read example-schema
or DTS101 slides.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 13/13] arm64: defconfig: Enable I3C and SPD5118 hwmon
From: Krzysztof Kozlowski @ 2026-04-12 13:21 UTC (permalink / raw)
To: Akhil R
Cc: Frank.Li, acpica-devel, alexandre.belloni, conor+dt, devicetree,
ebiggers, krzk+dt, lenb, linux-acpi, linux-hwmon, linux-i3c,
linux-kernel, linux, miquel.raynal, p.zabel, rafael, robh,
sakari.ailus, wsa+renesas
In-Reply-To: <20260411053433.49655-1-akhilrajeev@nvidia.com>
On 11/04/2026 07:34, Akhil R wrote:
> On Fri, 10 Apr 2026 11:57:11 +0200, Krzysztof Kozlowski wrote:
>> On 10/04/2026 10:37, Akhil R wrote:
>>> On Fri, 10 Apr 2026 09:18:48 +0200, Krzysztof Kozlowski wrote:
>>>> On 10/04/2026 08:57, Guenter Roeck wrote:
>>>>> On 4/9/26 23:39, Krzysztof Kozlowski wrote:
>>>>>> On 09/04/2026 12:57, Akhil R wrote:
>>>>>>> Add I3C subsystem support, DesignWare I3C master controller, and
>>>>>>> SPD5118 hwmon sensor as modules to the defconfig and therefore
>>>>>>> enable the support for SPD5118 sensor on SOCAMM found in NVIDIA
>>>>>>> Vera platforms.
>>>>>>
>>>>>> git grep for "Vera" gave me zero results. Are you sure this is an
>>>>>> upstream platform? Please point the DTS using this.
>>>>>>
>>>>>
>>>>> I think this is an ACPI based system, or at least that is what Google search
>>>>> tells me.
>>>>
>>>> Thanks. Following Google Vera is either a "CPU" or entire architecture
>>>> (at least that's how they call it), so it does not have SPD5118 sensor.
>>>
>>> SOCAMM is a Memory Module. SPD5118, as it's Kconfig mentions, is a sensor
>>> found within such memory modules. I didn't quite get why would you state
>>> that the SOCAMM present in Vera architecture (or CPU) does not have
>>> SPD5118 in it.
>>
>> I said that CPU or entire architecture does not have it.
>>
>> Commit is pretty vague in helping me to figure out the things I asked
>> for in last email.
>>
>>
>>>
>>> Pasting the below from the Vera Rubin product page [1] -
>>> "NVIDIA Vera CPUs add enhanced serviceability with small-outline
>>> compression-attached memory modules (SOCAMM) LPDDR5X and in-system tests
>>> for the CPU cores."
>>>
>>> [1]: https://www.nvidia.com/en-us/data-center/technologies/rubin/
>>
>> So this is for Vera Rubin? For what is this exactly?
>
> SOCAMM is with the Vera CPU. Any Vera based platform would have this module.
> Vera Rubin is one such platform.
>
> SPD5118 is within the SOCAMM.
So just mention this, instead of giving imprecise "Vera".
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v7 2/2] dt-bindings: embedded-controller: Add synology microp devices
From: Markus Probst @ 2026-04-12 13:21 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Hans de Goede, Ilpo Järvinen, Bryan O'Donoghue,
Lee Jones, Pavel Machek, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Greg Kroah-Hartman, platform-driver-x86, linux-leds,
devicetree, linux-kernel, rust-for-linux
In-Reply-To: <20260412-cuddly-taipan-of-reputation-1cafe0@quoll>
[-- Attachment #1: Type: text/plain, Size: 2578 bytes --]
On Sun, 2026-04-12 at 10:26 +0200, Krzysztof Kozlowski wrote:
> On Sat, Apr 11, 2026 at 05:27:35PM +0200, Markus Probst wrote:
> > +properties:
> > + compatible:
> > + enum:
> > + - synology,ds923p-microp
> > + - synology,ds918p-microp
> > + - synology,ds214play-microp
> > + - synology,ds225p-microp
> > + - synology,ds425p-microp
> > + - synology,ds710p-microp
> > + - synology,ds1010p-microp
> > + - synology,ds723p-microp
> > + - synology,ds1522p-microp
> > + - synology,rs422p-microp
> > + - synology,ds725p-microp
> > + - synology,ds118-microp
> > + - synology,ds124-microp
> > + - synology,ds223-microp
> > + - synology,ds223j-microp
> > + - synology,ds1823xsp-microp
> > + - synology,rs822p-microp
> > + - synology,rs1221p-microp
> > + - synology,rs1221rpp-microp
> > + - synology,ds925p-microp
> > + - synology,ds1525p-microp
> > + - synology,ds1825p-microp
>
> Previous comment is not resolved. For example you stated that ds723p is
> compatible with ds725p, so this should be expressed.
Using this expression?
properties:
compatible:
oneOf:
- enum:
- synology,ds923p-microp
- synology,ds1522p-microp
- enum:
- synology,ds918p-microp
- synology,ds415p-microp
- const: synology,ds214play-microp
...
?
If so shall there each be a description?
Also ds723p and ds725p are not compatible. ds723p has a system current
sensor, ds725p does not. This will be relevant when implementing the
hwmon part of the driver.
>
> ds918p and ds415p as well. ds925p and several others you EXPLICITLY
> wrote they are compatible:
>
> "ds925p, ds1525p, ds1825p, ds1823xsp:
> - supports fan rpm report via an adt7475 chip and therefore does not
> have gpios for fan failure
> - no system current sensor"
Yes.
>
> Probably many more cases, I did not verify all of them.
>
> If there is going to be new version, please organize the patch
> documenting the compatible (DT bindings) before the patch using that
> compatible.
> See also: https://elixir.bootlin.com/linux/v6.14-rc6/source/Documentation/devicetree/bindings/submitting-patches.rst#L46
Ok.
Thanks
- Markus Probst
>
> > +
> > + fan-failure-gpios:
> > + description: GPIOs needed to determine which fans stopped working on a fan failure event.
> > + minItems: 2
> > + maxItems: 3
> > +
> > +required:
> > + - compatible
>
> Best regards,
> Krzysztof
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] mmc: dw_mmc: implement option for configuring DMA threshold
From: Shawn Lin @ 2026-04-12 11:15 UTC (permalink / raw)
To: Kaustabh Chakraborty, Ulf Hansson, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jaehoon Chung
Cc: shawn.lin, linux-mmc, devicetree, linux-kernel
In-Reply-To: <20260412-dwmmc-dma-thr-v1-2-75a2f658eee3@disroot.org>
Hi Kaustabh
在 2026/04/12 星期日 3:43, Kaustabh Chakraborty 写道:
> Some controllers, such as certain Exynos SDIO ones, are unable to
> perform DMA transfers of small amount of bytes properly. Following the
> device tree schema, implement the property to define the DMA transfer
> threshold (from a hard coded value of 16 bytes) so that lesser number of
> bytes can be transferred safely skipping DMA in such controllers. The
> value of 16 bytes stays as the default for controllers which do not
> define it.
>
> Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
> ---
> drivers/mmc/host/dw_mmc.c | 6 ++++--
> drivers/mmc/host/dw_mmc.h | 1 +
> 2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 20193ee7b73eb..0c0d269b5e033 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -40,7 +40,6 @@
> SDMMC_INT_RESP_ERR | SDMMC_INT_HLE)
> #define DW_MCI_ERROR_FLAGS (DW_MCI_DATA_ERROR_FLAGS | \
> DW_MCI_CMD_ERROR_FLAGS)
> -#define DW_MCI_DMA_THRESHOLD 16
>
> #define DW_MCI_FREQ_MAX 200000000 /* unit: HZ */
> #define DW_MCI_FREQ_MIN 100000 /* unit: HZ */
> @@ -821,7 +820,7 @@ static int dw_mci_pre_dma_transfer(struct dw_mci *host,
> * non-word-aligned buffers or lengths. Also, we don't bother
> * with all the DMA setup overhead for short transfers.
> */
> - if (data->blocks * data->blksz < DW_MCI_DMA_THRESHOLD)
> + if (data->blocks * data->blksz < host->dma_threshold)
> return -EINVAL;
>
> if (data->blksz & 3)
> @@ -3137,6 +3136,9 @@ static int dw_mci_parse_dt(struct dw_mci *host)
> if (!host->data_addr_override)
> device_property_read_u32(dev, "data-addr", &host->data_addr_override);
>
> + if (device_property_read_u32(dev, "dma-threshold-bytes", &host->dma_threshold) < 0)
> + host->dma_threshold = 16;
> +
> if (device_property_present(dev, "fifo-watermark-aligned"))
> host->wm_aligned = true;
>
> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
> index 42e58be74ce09..5cdd342d01b68 100644
> --- a/drivers/mmc/host/dw_mmc.h
> +++ b/drivers/mmc/host/dw_mmc.h
> @@ -163,6 +163,7 @@ struct dw_mci {
> void __iomem *regs;
> void __iomem *fifo_reg;
> u32 data_addr_override;
> + u32 dma_threshold;
In addition to fixing the issue mentioned in patch 1, please add a
comment for this new member.
> bool wm_aligned;
>
> struct scatterlist *sg;
>
^ permalink raw reply
* Re: [PATCH 3/8] firmware: meson: sm: Add thermal calibration SMC call
From: Krzysztof Kozlowski @ 2026-04-12 10:47 UTC (permalink / raw)
To: Ronald Claveau, Guillaume La Roque, Rafael J. Wysocki,
Daniel Lezcano, Zhang Rui, Lukasz Luba, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Neil Armstrong, Kevin Hilman,
Jerome Brunet, Martin Blumenstingl
Cc: linux-pm, linux-amlogic, devicetree, linux-kernel,
linux-arm-kernel
In-Reply-To: <20260410-add-thermal-t7-vim4-v1-3-19f2b8da74d7@aliel.fr>
On 10/04/2026 18:48, Ronald Claveau wrote:
> @@ -245,6 +246,14 @@ struct meson_sm_firmware *meson_sm_get(struct device_node *sm_node)
> }
> EXPORT_SYMBOL_GPL(meson_sm_get);
>
> +int meson_sm_get_thermal_calib(struct meson_sm_firmware *fw, u32 *trim_info,
Exported functions should have kerneldoc.
> + u32 tsensor_id)
> +{
> + return meson_sm_call(fw, SM_THERMAL_CALIB_READ, trim_info, tsensor_id,
> + 0, 0, 0, 0);
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 2/4] dt-bindings: iio: magnetometer: Add binding for QST QMC5883P
From: Hardik Phalet @ 2026-04-12 10:32 UTC (permalink / raw)
To: Krzysztof Kozlowski, Hardik Phalet
Cc: Greg Kroah-Hartman, Jonathan Cameron, David Lechner, Nuno Sá,
Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Brigham Campbell, Shuah Khan, linux-iio, devicetree, linux-kernel,
linux-staging
In-Reply-To: <20260410-deft-mottled-petrel-fa22bf@quoll>
On Fri Apr 10, 2026 at 1:36 PM IST, Krzysztof Kozlowski wrote:
> On Fri, Apr 10, 2026 at 09:55:24AM +0200, Krzysztof Kozlowski wrote:
>> On Thu, Apr 09, 2026 at 09:07:29PM +0000, Hardik Phalet wrote:
>> > Add the device tree binding document for the QST QMC5883P, a 3-axis
>> > anisotropic magneto-resistive (AMR) sensor with a 16-bit ADC that
>> > communicates over I2C. The binding exposes the required 'compatible'
>> > and 'reg' properties along with an optional 'vdd-supply' for the
>> > 2.5 V–3.6 V VDD rail.
>>
>> Drop last sentence. We can read the diff.
>>
>> ...
>>
>> > +properties:
>> > + compatible:
>> > + const: qst,qmc5883p
>> > +
>> > + reg:
>> > + maxItems: 1
>> > + description: I2C address of the device; the default address is 0x2c.
>> > +
>> > + vdd-supply:
>> > + description:
>> > + VDD power supply (2.5 V to 3.6 V). Powers all internal analog and
>> > + digital functional blocks.
>>
>> Supply should be required. Devices need them to operate.
>
> Ah, and since I expect new version, also:
>
> A nit, subject: drop second/last, redundant "binding for". The
> "dt-bindings" prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.17-rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
Noted. Thanks for sharing the link.
>
> Best regards,
> Krzysztof
Regards,
Hardik
^ permalink raw reply
* Re: [PATCH v2 2/4] dt-bindings: iio: magnetometer: Add binding for QST QMC5883P
From: Hardik Phalet @ 2026-04-12 10:31 UTC (permalink / raw)
To: Krzysztof Kozlowski, Hardik Phalet
Cc: Greg Kroah-Hartman, Jonathan Cameron, David Lechner, Nuno Sá,
Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Brigham Campbell, Shuah Khan, linux-iio, devicetree, linux-kernel,
linux-staging
In-Reply-To: <20260410-stimulating-happy-terrier-e82dcc@quoll>
On Fri Apr 10, 2026 at 1:25 PM IST, Krzysztof Kozlowski wrote:
> On Thu, Apr 09, 2026 at 09:07:29PM +0000, Hardik Phalet wrote:
>> Add the device tree binding document for the QST QMC5883P, a 3-axis
>> anisotropic magneto-resistive (AMR) sensor with a 16-bit ADC that
>> communicates over I2C. The binding exposes the required 'compatible'
>> and 'reg' properties along with an optional 'vdd-supply' for the
>> 2.5 V–3.6 V VDD rail.
>
> Drop last sentence. We can read the diff.
>
Noted.
> ...
>
>> +properties:
>> + compatible:
>> + const: qst,qmc5883p
>> +
>> + reg:
>> + maxItems: 1
>> + description: I2C address of the device; the default address is 0x2c.
>> +
>> + vdd-supply:
>> + description:
>> + VDD power supply (2.5 V to 3.6 V). Powers all internal analog and
>> + digital functional blocks.
>
> Supply should be required. Devices need them to operate.
My bad, I will fix this in the next series.
>
> Best regards,
> Krzysztof
Regards,
Hardik
^ permalink raw reply
* Re: [PATCH v2 1/4] dt-bindings: vendor-prefixes: Add QST Corporation
From: Hardik Phalet @ 2026-04-12 10:30 UTC (permalink / raw)
To: Krzysztof Kozlowski, Hardik Phalet
Cc: Greg Kroah-Hartman, Jonathan Cameron, David Lechner, Nuno Sá,
Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Brigham Campbell, Shuah Khan, linux-iio, devicetree, linux-kernel,
linux-staging
In-Reply-To: <20260410-watchful-magnificent-caracara-0cae3d@quoll>
On Fri Apr 10, 2026 at 1:22 PM IST, Krzysztof Kozlowski wrote:
> On Thu, Apr 09, 2026 at 09:07:20PM +0000, Hardik Phalet wrote:
>> + "^qst,.*":
>
> Website tells me qstcorp.com, so prefix is qstcorp. Unless it is
> different company, but then just explain that in commit msg (e.g.
> provide link to website).
No qstcorp.com is what I am referring to. I will change it in the next
commit.
>
> Best regards,
> Krzysztof
Regards,
Hardik
^ permalink raw reply
* Re: [PATCH v2 0/4] Add QST QMC5883P magnetometer driver
From: Hardik Phalet @ 2026-04-12 10:27 UTC (permalink / raw)
To: Andy Shevchenko, Hardik Phalet
Cc: Greg Kroah-Hartman, Jonathan Cameron, David Lechner, Nuno Sá,
Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Brigham Campbell, Shuah Khan, linux-iio, devicetree, linux-kernel,
linux-staging
In-Reply-To: <adh-PKgh5C0-iAah@ashevche-desk.local>
On Fri Apr 10, 2026 at 10:06 AM IST, Andy Shevchenko wrote:
> On Thu, Apr 09, 2026 at 09:07:11PM +0000, Hardik Phalet wrote:
>> Verification steps performed:
>> - Chip ID register (0x00) reads back 0x80 on probe, confirming the
>> correct device is present
>> - All three axes (in_magn_x_raw, in_magn_y_raw, in_magn_z_raw) return
>> non-zero, stable values when the board is held still and change
>> appropriately when the board is rotated
>> - in_magn_x_scale (and Y, Z) returns the expected fractional value for
>> the default ±8 G range (1/37500000)
>> - in_magn_sampling_frequency / _available, in_magn_oversampling_ratio /
>> _available, and downsampling_ratio / downsampling_ratio_available all
>> read and write correctly; the chip responds without error to each
>> valid setting
>> - Runtime PM: after 2 s of inactivity the device enters suspend mode
>> (MODE = 0x00 confirmed via i2cdump); the next sysfs read correctly
>> resumes the device and returns valid data
>> - System suspend/resume (echo mem > /sys/power/state) leaves the
>> driver in a consistent state; readings remain valid after resume
>> - dt_binding_check passes for patch 2/4
>> - Kernel builds cleanly with W=1 and no new warnings
>
> This driver is rather huge. There are mistakes you made in the process, though:
> - never send a new version for such a code (amount and complexity) earlier than
> a week; give others a chance to review
Noted, Andy. I will take care of it from now on.
> - do not put driver to staging, why?
There are some functionality missing. But it has since been made clear
to me that that is not a valid reason. I will move to `drivers/iio`
in the next patch series.
> - the investigation is rather poor about existence of the driver — make sure
> there is no compatible (by register layout) driver in IIO or even outside it
> (for ADCs it might appear as HWMON [drivers/hwmon] or INPUT [drivers/input]
> in some cases)
I will dig a bit deeper into this. But I could not find any thing
realted to the device I am working on (qmc5883p).
There is a device hmc5843_core.c that already handles HMC5883L. But the
register layout is different, field ranges, gain encoding etc. are
different and I think warrants a separate driver. Other than that I
could not find something related to device I am working on.
I will carefully go through HWMON and INPUT too, but do you have a
strategy to help me find somehting similar? It will be very useful.
Would `grep`-ing for 5883 and QST be a right starting point?
>
> --
> With Best Regards,
> Andy Shevchenko
Thank you for the review Andy, I appreciate your time!
Regards,
Hardik
-
^ permalink raw reply
* Re: [PATCH v2 0/4] Add QST QMC5883P magnetometer driver
From: Hardik Phalet @ 2026-04-12 10:05 UTC (permalink / raw)
To: David Lechner, Hardik Phalet, Greg Kroah-Hartman
Cc: Jonathan Cameron, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Brigham Campbell, Shuah Khan,
linux-iio, devicetree, linux-kernel, linux-staging
In-Reply-To: <ac8912f2-3bee-483f-91f1-72c5346792c6@baylibre.com>
On Sat Apr 11, 2026 at 12:56 AM IST, David Lechner wrote:
> On 4/9/26 4:07 PM, Hardik Phalet wrote:
>
> For a series this be, please wait at least a week for more feedback
> before submitting the next revision.
>
Noted.
>> This series adds initial Linux support for the QST QMC5883P, a 3-axis
>> anisotropic magneto-resistive (AMR) magnetometer with a 16-bit ADC that
>> communicates over I2C. To my knowledge there is no existing upstream
>> driver for this device.
>>
>> The driver supports:
>> - Raw magnetic field readings on X, Y, and Z axes
>> - Four selectable full-scale ranges (±2 G, ±8 G, ±12 G, ±30 G)
>> - Configurable output data rate (10, 50, 100, 200 Hz)
>> - Configurable oversampling ratio (1, 2, 4, 8)
>> - Configurable downsampling ratio (1, 2, 4, 8) via a custom sysfs
>
> What is the difference between oversampling and downsampling? I think
> we have used some filter attribute for downsampling/decimation in some
> other drivers so maybe that could be a good fit?
>
I mentioned my problem with it in my reply to your review for the third
patch in the series. Meanwhile, I will also have a look at how other
drivers are handling it.
>> attribute
>> - Runtime PM with a 2 s autosuspend delay
>> - System suspend/resume via pm_runtime_force_suspend/resume
>>
>> Regmap with an rbtree cache is used throughout. CTRL_1 and CTRL_2
>> bit fields are accessed via regmap_field to avoid read-modify-write
>> races. The STATUS register is marked precious so regmap never reads
>> it speculatively and clears the DRDY/OVFL bits unexpectedly.
>>
>> The init sequence on probe is: soft reset → wait 1 ms → deassert
>> reset → configure SET/RESET control → apply default ODR/OSR/DSR/RNG
>> → enter normal mode. This ordering was determined empirically on
>> hardware to produce reliable, non-zero axis readings.
>>
>> The driver is placed under drivers/staging/iio/magnetometer/ with a
>> TODO file tracking the remaining work before it can graduate:
>> - Triggered buffer support (iio_triggered_buffer_setup)
>> - DRDY interrupt support
>> - Self-test implementation
>
> These are not reasons to have the driver in staging. It is fine
> to have a driver that doesn't implement all functionality. We should
> be able to add those features without breaking anything.
Noted.
Regards,
Hardik
^ permalink raw reply
* Re: [PATCH 1/8] dt-bindings: thermal: amlogic: Add support for T7
From: Krzysztof Kozlowski @ 2026-04-12 9:58 UTC (permalink / raw)
To: Ronald Claveau
Cc: Guillaume La Roque, Rafael J. Wysocki, Daniel Lezcano, Zhang Rui,
Lukasz Luba, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl,
linux-pm, linux-amlogic, devicetree, linux-kernel,
linux-arm-kernel
In-Reply-To: <20260410-add-thermal-t7-vim4-v1-1-19f2b8da74d7@aliel.fr>
On Fri, Apr 10, 2026 at 06:48:02PM +0200, Ronald Claveau wrote:
> Add the amlogic,t7-thermal compatible for the Amlogic T7 thermal sensor.
>
> Unlike existing variants which use a phandle to the ao-secure syscon,
> the T7 relies on a secure monitor interface described by a phandle and
> a sensor index argument.
>
> Introduce the amlogic,secure-monitor property as a phandle-array and
> make amlogic,ao-secure or amlogic,secure-monitor conditionally required
> depending on the compatible.
>
> Signed-off-by: Ronald Claveau <linux-kernel-dev@aliel.fr>
> ---
> .../bindings/thermal/amlogic,thermal.yaml | 40 +++++++++++++++++++++-
> 1 file changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/thermal/amlogic,thermal.yaml b/Documentation/devicetree/bindings/thermal/amlogic,thermal.yaml
> index 70b273271754b..85ee73c6e1161 100644
> --- a/Documentation/devicetree/bindings/thermal/amlogic,thermal.yaml
> +++ b/Documentation/devicetree/bindings/thermal/amlogic,thermal.yaml
> @@ -22,6 +22,7 @@ properties:
> - amlogic,g12a-ddr-thermal
> - const: amlogic,g12a-thermal
> - const: amlogic,a1-cpu-thermal
> + - const: amlogic,t7-thermal
So these two entries are enum.
>
> reg:
> maxItems: 1
> @@ -42,12 +43,40 @@ properties:
> '#thermal-sensor-cells':
> const: 0
>
> + amlogic,secure-monitor:
> + description: phandle to the secure monitor
> + $ref: /schemas/types.yaml#/definitions/phandle-array
> + items:
> + - items:
> + - description: phandle to the secure monitor
> + - description: sensor index
For what exactly this sensor index is needed? commit msg explained me
nothing, instead repeated what you did. That's pointless, explain why
you did it.
> +
> required:
> - compatible
> - reg
> - interrupts
> - clocks
> - - amlogic,ao-secure
> +
> +allOf:
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - amlogic,g12a-cpu-thermal
> + - amlogic,g12a-ddr-thermal
Drop both, you need only fallback.
> + - amlogic,a1-cpu-thermal
And list is sorted alphabetically.
> + then:
> + required:
> + - amlogic,ao-secure
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 4/4] MAINTAINERS: Add entry for QST QMC5883P magnetometer driver
From: Hardik Phalet @ 2026-04-12 9:57 UTC (permalink / raw)
To: David Lechner, Hardik Phalet, Greg Kroah-Hartman
Cc: Jonathan Cameron, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Brigham Campbell, Shuah Khan,
linux-iio, devicetree, linux-kernel, linux-staging
In-Reply-To: <0fd8a01c-0ca9-4caf-946b-5e0e756e4cab@baylibre.com>
On Sat Apr 11, 2026 at 1:02 AM IST, David Lechner wrote:
> On 4/9/26 4:07 PM, Hardik Phalet wrote:
>> Add a MAINTAINERS entry for the QST QMC5883P staging IIO driver,
>> covering the driver source and its device tree binding.
>>
>> Signed-off-by: Hardik Phalet <hardik.phalet@pm.me>
>> ---
>> MAINTAINERS | 7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index a92290fffa16..d0b9bfceb283 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -20274,6 +20274,13 @@ F: Documentation/networking/device_drivers/ethernet/freescale/dpaa2/overview.rst
>> F: drivers/bus/fsl-mc/
>> F: include/uapi/linux/fsl_mc.h
>>
>> +QST QMC5883P MAGNETOMETER DRIVER
>> +M: Hardik Phalet <hardik.phalet@pm.me>
>> +L: linux-iio@vger.kernel.org
>> +S: Maintained
>> +F: Documentation/devicetree/bindings/iio/magnetometer/qst,qmc5883p.yaml
>> +F: drivers/staging/iio/magnetometer/
>> +
>> QT1010 MEDIA DRIVER
>> L: linux-media@vger.kernel.org
>> S: Orphan
>
> This should be split up and added in the patches that actually
> add the F: files. Most of it will go with the dt-bingings patch
> and the one line added later with the driver.
Makes sense, David. Will do in the next patch series.
Regards,
Hardik
^ permalink raw reply
* [PATCH RESEND 2/2] dt-bindings: trivial-devices: add atmel,atecc608b
From: Thorsten Blum @ 2026-04-12 9:56 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Guenter Roeck,
Jonathan Cameron, Frank Li, Cosmo Chou, Rodrigo Gobbi,
Wensheng Wang, Nuno Sá, Antoni Pokusinski, Eddie James,
Dixit Parmar, Pawel Dembicki
Cc: Thorsten Blum, Herbert Xu, linux-crypto, Krzysztof Kozlowski,
Conor Dooley, devicetree, linux-kernel
In-Reply-To: <20260412095642.120815-3-thorsten.blum@linux.dev>
Add entry for ATECC608B. Update the ATECC508A comment for consistency.
Acked-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
Resending to include
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: linux-crypto@vger.kernel.org
---
Documentation/devicetree/bindings/trivial-devices.yaml | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index a482aeadcd44..9da4c73b23cf 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -63,8 +63,10 @@ properties:
- arduino,unoq-mcu
# Temperature monitoring of Astera Labs PT5161L PCIe retimer
- asteralabs,pt5161l
- # i2c h/w elliptic curve crypto module
+ # ATECC508A - i2c h/w elliptic curve crypto module
- atmel,atecc508a
+ # ATECC608B - i2c h/w elliptic curve crypto module
+ - atmel,atecc608b
# ATSHA204 - i2c h/w symmetric crypto module
- atmel,atsha204
# ATSHA204A - i2c h/w symmetric crypto module
^ permalink raw reply related
* Re: [PATCH v2 3/4] staging: iio: magnetometer: Add QST QMC5883P driver
From: Hardik Phalet @ 2026-04-12 9:55 UTC (permalink / raw)
To: Greg Kroah-Hartman, Hardik Phalet
Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Brigham Campbell,
Shuah Khan, linux-iio, devicetree, linux-kernel, linux-staging
In-Reply-To: <2026041027-hatless-swinger-eae1@gregkh>
On Fri Apr 10, 2026 at 10:58 AM IST, Greg Kroah-Hartman wrote:
> On Thu, Apr 09, 2026 at 09:07:41PM +0000, Hardik Phalet wrote:
>> Add an IIO driver for the QST QMC5883P, a 3-axis anisotropic
>> magneto-resistive (AMR) magnetometer with a 16-bit ADC, communicating
>> over I2C. There is no existing upstream driver for this device.
>
> Sorry, but no new iio drivers should be added to staging. Take the time
> to do it right and put it into drivers/iio/ from the beginning.
> Otherwise this will just take more time and effort to get it into that
> location in the end (i.e. doing it right is simpler/faster.)
>
> thanks,
>
> greg k-h
Noted Greg, I will move it to /drivers/iio in the next version.
Thanks for the review. I appreciate your time.
Regards,
Hardik
^ permalink raw reply
* Re: [PATCH 2/4] soc: amlogic: clk-measure: Add A1 and T7 support
From: Krzysztof Kozlowski @ 2026-04-12 9:55 UTC (permalink / raw)
To: Jian Hu, Neil Armstrong, Jerome Brunet, Kevin Hilman,
Michael Turquette, Martin Blumenstingl, robh+dt, Rob Herring
Cc: devicetree, linux-amlogic, linux-kernel, linux-arm-kernel
In-Reply-To: <20260410100329.3167482-3-jian.hu@amlogic.com>
On 10/04/2026 12:03, Jian Hu wrote:
> Add support for the A1 and T7 SoC family in amlogic clk measure.
>
> Signed-off-by: Jian Hu <jian.hu@amlogic.com>
> ---
> drivers/soc/amlogic/meson-clk-measure.c | 272 ++++++++++++++++++++++++
> 1 file changed, 272 insertions(+)
>
> diff --git a/drivers/soc/amlogic/meson-clk-measure.c b/drivers/soc/amlogic/meson-clk-measure.c
> index d862e30a244e..083524671b76 100644
> --- a/drivers/soc/amlogic/meson-clk-measure.c
> +++ b/drivers/soc/amlogic/meson-clk-measure.c
> @@ -787,6 +787,258 @@ static const struct meson_msr_id clk_msr_s4[] = {
>
> };
>
> +static struct meson_msr_id clk_msr_a1[] = {
And existing code uses what sort of array? Seems you send us obsolete or
downstream code.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 3/4] staging: iio: magnetometer: Add QST QMC5883P driver
From: Hardik Phalet @ 2026-04-12 9:54 UTC (permalink / raw)
To: David Lechner, Hardik Phalet, Greg Kroah-Hartman
Cc: Jonathan Cameron, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Brigham Campbell, Shuah Khan,
linux-iio, devicetree, linux-kernel, linux-staging
In-Reply-To: <736964f9-1e93-47e7-80ca-1a89f239a353@baylibre.com>
On Sat Apr 11, 2026 at 1:32 AM IST, David Lechner wrote:
> On 4/9/26 4:07 PM, Hardik Phalet wrote:
>
> This is a little bit much to review all in one patch. Could be nice
> to split out power management to a separate patch.
>
> Oversampling/downsampling could be split to a separate patch or two
> as well.
>
Noted David. I will handle it in v3. I think downsampling can be another
patch.
>> +#define QMC5883P_REG_Y_MSB 0x04
>> +#define QMC58
>> 83P_REG_Z_LSB 0x05
>
> Were you manually editing the patch?
No. Maybe it's my email client (AERC). I will make sure the next version
is tight.
>
>> +#define QMC5883P_RSTCTRL_SET_RESET \
>> + 0x00 /* Set and reset on, i.e. the offset of device is renewed */
>> +#define QMC5883P_RSTCTRL_SET_ONLY 0x01 /* Set only on */
>> +#define QMC5883P_RSTCTRL_OFF 0x02 /* Set
>> and reset off */
>
> Or maybe you mail client mangled the patch? These wraps are
> happening in many places.
>
Yes. Exactly this.
>> + if (regval != QMC5883P_CHIP_ID)
>> + return dev_err_probe(data->dev, -ENODEV,
>
> We don't consider ID match an error. It has happened too many times
> that there is a compatible part with a different ID. This can just
> be dev_info() and return success.
>
Noted. Will handle in v3.
>> +static int qmc5883p_chip_init(struct qmc5883p_data *data)
>> +{
>> + int ret;
>> +
>> + ret = regmap_field_write(data->rf.sftrst, 1);
>> + if (ret)
>> + return ret;
>> +
>> + usleep_range(1000, 2000);
>
> Use fsleep() instead and add a comment explaining why this specific duration
> was selected.
>
Noted, will handle in v3.
>> + ret = regmap_field_write(data->rf.odr, QMC5883P_DEFAULT_ODR);
>> + if (ret)
>> + return ret;
>
> Since we just reset the chip, why do we need to set everything to a
> new default instead of using the chip's default? If there is a good
> reason, add a comment, otherwise we can leave this out.
>
These were simply more aligned with my usecase, so more of a development
artifact. Chip defaults are sufficient. I will leave this out.
>> +
>> + return regmap_field_write(data->rf.mode, QMC5883P_MODE_NORMAL);
>
> Does this start sampling? Seems like it could be out of place here.
>
Yes, normal mode starts sampling data. Your point makes sense, I will
remove it in the next version. Incorrect mental model, I apologise.
>> + ret = regmap_read_poll_timeout(data->regmap, QMC5883P_REG_STATUS,
>> + status, status & QMC5883P_STATUS_DRDY,
>> + QMC5883P_DRDY_POLL_US, 150000);
>
> Numbers with lots of 0s are easier to read as 150 * (MICRO / MILLI).
>
My bad, will handle in v3.
>> +static IIO_DEVICE_ATTR(downsampling_ratio, 0644, downsampling_ratio_show,
>> + downsampling_ratio_store, 0);
>> +static IIO_CONST_ATTR(downsampling_ratio_available, "1 2 4 8");
>
> As mentioned in the cover letter, we'd like to know more about what
> this actually does. If there is a good reason it doesn't fit with
> any existing filter attribute, then we'll need a patch to document
> the sysfs ABI as well.
>
In the device datasheet, OSR2("Down sampling ratio") is mentioned like this:
"Another filter is added for better noise performance; the depth can be
adjusted through OSR2". OSR2's defintion is called "down sampling ratio"
in a table. Nowhere else. I didn't know what attribute to map it to in
this case.
> Could save some dupilcation by making a macro that takes X/Y/Z
> as parameter. e.g.
>
> #define QMC5883P_CHAN(ch) \
> ... \
> .channel2 = IIO_MOD_##ch, \
> ... \
> .address = AXIS_##ch, \
> ...
>
Noted. Will do this in v3.
>> + ret = devm_regulator_get_enable_optional(dev, "vdd");
>> + if (ret && ret != -ENODEV)
>> + return dev_err_probe(dev, ret,
>> + "failed to get vdd regulator\n");
>> +
>> + /* Datasheet specifies up to 50 ms supply ramp + 250 us POR time. */
>> + fsleep(50000);
>> +
>> + i2c_set_clientdata(client, indio_dev);
>> +
>> + ret = qmc5883p_rf_init(data);
>
> Would be more logical to move this up right after regmap is declared.
>
Makes sense, I will move it up in v3.
>> + ret = devm_iio_device_register(dev, indio_dev);
>
> Usually, we just return directly here. This pretty much doesn't ever fail.
>
Okay. Will remove the dev_err_probe() in v3.
>> + ret = regmap_field_write(data->rf.mode, QMC5883P_MODE_NORMAL);
>> + if (ret)
>> + return ret;
>> +
>> + usleep_range(10000, 11000);
>
> Again, fsleep() and comment.
>
Noted.
>> +static const struct i2c_device_id qmc5883p_id[] = {
>> + { "qmc5883p", 0 },
>> + {},
>
> IIO style for this is:
>
> { }
>
> space between braces and no trailing comma.
>
Noted. Will take it up in v3.
Thanks for the in-depth review David. I really appreciate it.
Regards,
Hardik
^ permalink raw reply
* Re: [PATCH v2] dt-bindings: ARM: arm,vexpress-scc: convert to DT schema
From: Krzysztof Kozlowski @ 2026-04-12 9:50 UTC (permalink / raw)
To: Khushal Chitturi
Cc: robh, krzk+dt, conor+dt, liviu.dudau, sudeep.holla, lpieralisi,
pawel.moll, devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <20260411183355.8847-1-khushalchitturi@gmail.com>
On Sun, Apr 12, 2026 at 12:03:55AM +0530, Khushal Chitturi wrote:
> Convert the ARM Versatile Express Serial Configuration Controller
> bindings to DT schema.
>
> Signed-off-by: Khushal Chitturi <khushalchitturi@gmail.com>
> ---
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 20/21] dt-bindings: gpio: describe Waveshare GPIO controller
From: Krzysztof Kozlowski @ 2026-04-12 9:48 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Neil Armstrong, Jessica Zhang, David Airlie, Simona Vetter,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Cong Yang, Ondrej Jirman,
Javier Martinez Canillas, Jagan Teki, Liam Girdwood, Mark Brown,
Linus Walleij, Bartosz Golaszewski, dri-devel, devicetree,
linux-kernel, linux-gpio, Riccardo Mereu
In-Reply-To: <20260411-waveshare-dsi-touch-v2-20-75cdbeac5156@oss.qualcomm.com>
On Sat, Apr 11, 2026 at 03:10:40PM +0300, Dmitry Baryshkov wrote:
> The Waveshare DSI TOUCH family of panels has separate on-board GPIO
> controller, which controls power supplies to the panel and the touch
> screen and provides reset pins for both the panel and the touchscreen.
> Also it provides a simple PWM controller for panel backlight.
>
> Add bindings for these GPIO controllers. As overall integration might be
> not very obvious (and it differs significantly from the bindings used by
> the original drivers), provide complete example with the on-board
> regulators and the DSI panel.
>
> Tested-by: Riccardo Mereu <r.mereu@arduino.cc>
You cannot test a binding, it is not possible. Otherwise explain me how
did you copy it to the device and what sort of device runs YAML.
The tag was given here explicitly, so I really do not understand this. I
could imagine tags coming from a reply to the cover letter, but adding
tag here? That's just fake test.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 04/21] dt-bindings: display/panel: ilitek,ili9881c: describe Waveshare panel
From: Krzysztof Kozlowski @ 2026-04-12 9:46 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Neil Armstrong, Jessica Zhang, David Airlie, Simona Vetter,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Cong Yang, Ondrej Jirman,
Javier Martinez Canillas, Jagan Teki, Liam Girdwood, Mark Brown,
Linus Walleij, Bartosz Golaszewski, dri-devel, devicetree,
linux-kernel, linux-gpio
In-Reply-To: <20260411-waveshare-dsi-touch-v2-4-75cdbeac5156@oss.qualcomm.com>
On Sat, Apr 11, 2026 at 03:10:24PM +0300, Dmitry Baryshkov wrote:
> Describe Waveshare 7" DSI panel which uses ILI9881 as a panel
> controller. This panel requires two voltags supplies, so add separate
> iovcc supply.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> ---
> Documentation/devicetree/bindings/display/panel/ilitek,ili9881c.yaml | 2 ++
> 1 file changed, 2 insertions(+)
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Best regards,
Krzysztof
^ 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