* [PATCH net-next 5/8] net: mdio: realtek-rtl9300: Add c45 over c22 mitigation
From: Markus Stockhausen @ 2026-06-13 11:29 UTC (permalink / raw)
To: andrew, hkallweit1, linux, davem, edumazet, kuba, pabeni, netdev,
chris.packham, daniel, robh, krzk+dt, conor+dt, devicetree
Cc: Markus Stockhausen
In-Reply-To: <20260613112946.1071411-1-markus.stockhausen@gmx.de>
When reading the PHY state on c22 based buses the hardware polling unit
reads the EEE status with a sequence similar to this:
...
phy_write(phy, 31, 0x0);
phy_write(phy, 13, 0x7); /* c22 over c45 MDIO_AN_EEE_ADV */
phy_write(phy, 14, 0x3c);
phy_write(phy, 13, 0x8007);
phy_read(phy, 14);
phy_write(phy, 13, 0x7); /* c22 over c45 MDIO_AN_EEE_LPABLE */
phy_write(phy, 14, 0x3d);
phy_write(phy, 13, 0x8007);
...
If the Linux kernel wants to do the same in mmd_phy_read() via a call to
mmd_phy_indirect() this most likely fails. The commands are issued in a
straight sequence but between two of them the hardware polling might run
a status check for the same PHY. This effectively breaks the kernel access
and makes use of c45 over c22 unusable.
Detailed analysis shows that for RTL838x, RTL839x and RTL931x polling
can be safely deactivated during operation. The MAC layer will continue
to show the last known state. RTL839x is an exception from this. As soon
as polling is disabled the MAC link status register shows "port down".
Enhance the driver to detect this register 13/14/13/14 access sequence.
Before the first access to register 13 of a PHY disable polling for the
corresponding port. Reenable polling as soon as the sequence is finished
or any other unexpected input is detected. Some details about the stop
and start timing:
- The stopping is issued inflight while the polling engine is working.
After it is finished no new polling for the port will be issued (tested
with only one port with active polling).
- Reenabling the polling engine happens within ~25us after the last
command of the MMD sequence. This is mostly due to MMIO overhead.
Technically speaking, add a simple state machine that increments a
per-port MMD counter for each successful step of the sequence. When the
first command starts (counter=1) stop polling. When the last command
finishes (counter=4) or unexpected data is sent start polling.
Additionally:
- Add a global "initialization done" tracker that stops the mechanism
from kicking in during bus probing.
- Add a global "link flapping" option that allows to disable the state
tracker for the to-be-added RTL839x series completely.
Signed-off-by: Markus Stockhausen <markus.stockhausen@gmx.de>
---
drivers/net/mdio/mdio-realtek-rtl9300.c | 62 ++++++++++++++++++++++++-
1 file changed, 60 insertions(+), 2 deletions(-)
diff --git a/drivers/net/mdio/mdio-realtek-rtl9300.c b/drivers/net/mdio/mdio-realtek-rtl9300.c
index a7fd075947b6..e206ee3e2b1c 100644
--- a/drivers/net/mdio/mdio-realtek-rtl9300.c
+++ b/drivers/net/mdio/mdio-realtek-rtl9300.c
@@ -196,10 +196,12 @@ struct otto_emdio_priv {
struct mutex lock; /* protect HW access */
DECLARE_BITMAP(valid_ports, MAX_PORTS);
u16 page[MAX_PORTS];
+ u8 mmd_state[MAX_PORTS];
u8 smi_bus[MAX_PORTS];
u8 smi_addr[MAX_PORTS];
bool smi_bus_is_c45[MAX_SMI_BUSSES];
struct mii_bus *bus[MAX_SMI_BUSSES];
+ bool init_done;
};
struct otto_emdio_info {
@@ -209,6 +211,7 @@ struct otto_emdio_info {
u32 cmd_read;
u32 cmd_write;
struct otto_emdio_cmd_regs cmd_regs;
+ bool link_flap;
u8 num_buses;
u8 num_ports;
u16 num_pages;
@@ -254,6 +257,43 @@ static int otto_emdio_set_port_polling(struct otto_emdio_priv *priv, int port, b
BIT(port % 32), active);
}
+static int otto_emdio_mmd_prefix(struct otto_emdio_priv *priv, int port, int regnum)
+{
+ u8 newstate, *state = &priv->mmd_state[port];
+ int expected, ret = 0;
+
+ if (!priv->init_done)
+ return 0;
+ /*
+ * Disabled polling might produce link flapping and false notification interrupts on the
+ * MAC layer. In this case disable c45 over c22 MMD access because chances are high that
+ * the register 13/14/13/14 sequence is intercepted by a parallel hardware access. As
+ * a workaround the PHY must provide its own mmd read/write() callbacks and redirect to
+ * normal c22 registers. See rtlgen_read_mmd().
+ */
+ if (priv->info->link_flap)
+ return (regnum == MII_MMD_DATA || regnum == MII_MMD_CTRL) ? -EIO : 0;
+
+ expected = (*state & 1) ? MII_MMD_DATA : MII_MMD_CTRL;
+ newstate = regnum == expected ? *state + 1 : 0;
+
+ if (newstate == 1 || newstate < *state)
+ ret = otto_emdio_set_port_polling(priv, port, !newstate);
+ *state = newstate;
+
+ return ret;
+}
+
+static int otto_emdio_mmd_postfix(struct otto_emdio_priv *priv, int port, int regnum)
+{
+ if (priv->mmd_state[port] != 4)
+ return 0;
+
+ priv->mmd_state[port] = 0;
+
+ return otto_emdio_set_port_polling(priv, port, true);
+}
+
static int otto_emdio_run_cmd(struct mii_bus *bus, u32 cmd,
struct otto_emdio_cmd_regs *cmd_data)
{
@@ -463,7 +503,15 @@ static int otto_emdio_read_c22(struct mii_bus *bus, int phy_id, int regnum)
if (regnum == 31)
return priv->page[port];
+ ret = otto_emdio_mmd_prefix(priv, port, regnum);
+ if (ret)
+ return ret;
+
ret = priv->info->read_c22(bus, port, regnum, &value);
+ if (ret)
+ return ret;
+
+ ret = otto_emdio_mmd_postfix(priv, port, regnum);
}
return ret ? ret : value;
@@ -472,7 +520,7 @@ static int otto_emdio_read_c22(struct mii_bus *bus, int phy_id, int regnum)
static int otto_emdio_write_c22(struct mii_bus *bus, int phy_id, int regnum, u16 value)
{
struct otto_emdio_priv *priv = otto_emdio_bus_to_priv(bus);
- int port;
+ int port, ret;
port = otto_emdio_phy_to_port(bus, phy_id);
if (port < 0)
@@ -487,7 +535,15 @@ static int otto_emdio_write_c22(struct mii_bus *bus, int phy_id, int regnum, u16
return 0;
}
- return priv->info->write_c22(bus, port, regnum, value);
+ ret = otto_emdio_mmd_prefix(priv, port, regnum);
+ if (ret)
+ return ret;
+
+ ret = priv->info->write_c22(bus, port, regnum, value);
+ if (ret)
+ return ret;
+
+ return otto_emdio_mmd_postfix(priv, port, regnum);
}
}
@@ -794,6 +850,8 @@ static int otto_emdio_probe(struct platform_device *pdev)
return err;
}
+ priv->init_done = true;
+
return 0;
}
--
2.54.0
^ permalink raw reply related
* [PATCH net-next 6/8] net: mdio: realtek-rtl9300: Increase MDIO timeout
From: Markus Stockhausen @ 2026-06-13 11:29 UTC (permalink / raw)
To: andrew, hkallweit1, linux, davem, edumazet, kuba, pabeni, netdev,
chris.packham, daniel, robh, krzk+dt, conor+dt, devicetree
Cc: Markus Stockhausen
In-Reply-To: <20260613112946.1071411-1-markus.stockhausen@gmx.de>
Access to the Realtek Otto ethernet MDIO bus must wait for a free slot
between two hardware polls. The polling sequence consists of at least
17 commands on the RTL8380 devices. This delay can be nicely seen when
disabling polling completely. The following times are measured from the
last register write that sets the command bit until hardware responds
with the command finished bit set.
- average c22 read with polling enabled on all ports: ~380us
- average c22 read with polling enabled on one port: ~380us
- average c22 read with polling completely disabled: ~180us
With a default MDIO bus frequency of 2.5Mhz the bare hardware runtime
for a single command (32 bit preamble + 32 bit data) is ~25us. So the
hardware adds quite some overhead. On top of this comes the fact that
especially the RTL838x devices are low on resources (500Mhz 4Kec core
with 16K cache).
Analysis on a RTL838x device with 28 ports gives PHY access timeouts
during one of three boots while waiting for command completion. This is
currently set to 1ms. From the above explanation one can see that there
is not much headroom left.
Increase the timeout to 5ms.
Signed-off-by: Markus Stockhausen <markus.stockhausen@gmx.de>
---
drivers/net/mdio/mdio-realtek-rtl9300.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/mdio/mdio-realtek-rtl9300.c b/drivers/net/mdio/mdio-realtek-rtl9300.c
index e206ee3e2b1c..244af5fdeaf3 100644
--- a/drivers/net/mdio/mdio-realtek-rtl9300.c
+++ b/drivers/net/mdio/mdio-realtek-rtl9300.c
@@ -302,9 +302,9 @@ static int otto_emdio_run_cmd(struct mii_bus *bus, u32 cmd,
u32 cmdstate;
int ret;
- /* Defensive pre check just in case something goes horrible wrong */
+ /* Defensive pre check just in case something goes horribly wrong */
ret = regmap_read_poll_timeout(priv->regmap, info->cmd_regs.c22_data,
- cmdstate, !(cmdstate & PHY_CTRL_CMD), 10, 1000);
+ cmdstate, !(cmdstate & PHY_CTRL_CMD), 10, 5000);
if (ret)
return ret;
@@ -344,7 +344,7 @@ static int otto_emdio_run_cmd(struct mii_bus *bus, u32 cmd,
return ret;
ret = regmap_read_poll_timeout(priv->regmap, info->cmd_regs.c22_data,
- cmdstate, !(cmdstate & PHY_CTRL_CMD), 10, 1000);
+ cmdstate, !(cmdstate & PHY_CTRL_CMD), 10, 5000);
if (ret)
return ret;
--
2.54.0
^ permalink raw reply related
* [PATCH net-next 8/8] net: mdio: realtek-rtl9300: Add support for RTL839x
From: Markus Stockhausen @ 2026-06-13 11:29 UTC (permalink / raw)
To: andrew, hkallweit1, linux, davem, edumazet, kuba, pabeni, netdev,
chris.packham, daniel, robh, krzk+dt, conor+dt, devicetree
Cc: Markus Stockhausen
In-Reply-To: <20260613112946.1071411-1-markus.stockhausen@gmx.de>
The MDIO driver has been prepared for multiple device support. Add all
required bits for the RTL839x (aka cypress) series. This is straightforward
but some things are worth mentioning.
- The device has a lot in common with the RTL931x series. 8192 (Realtek)
pages and 7 MMIO registers
- There are two SMI buses for 1G PHYs. Neither the bus nor address map
register exists.
- The MAC layer shows link flapping when temporarily deactivating the
hardware polling for one port. Mark this in the info structure.
- The hardware has not much to configure. So the setup_controller()
function is not needed.
Signed-off-by: Markus Stockhausen <markus.stockhausen@gmx.de>
---
drivers/net/mdio/mdio-realtek-rtl9300.c | 104 ++++++++++++++++++++++++
1 file changed, 104 insertions(+)
diff --git a/drivers/net/mdio/mdio-realtek-rtl9300.c b/drivers/net/mdio/mdio-realtek-rtl9300.c
index d9ff0b0aecbb..2ab1aeb85eed 100644
--- a/drivers/net/mdio/mdio-realtek-rtl9300.c
+++ b/drivers/net/mdio/mdio-realtek-rtl9300.c
@@ -139,6 +139,29 @@
#define RTL8380_SMI_POLL_CTRL 0xa17c
#define RTL8380_SMI_PORT0_5_ADDR_CTRL 0xa1c8
+#define RTL8390_NUM_BUSES 2
+#define RTL8390_NUM_PAGES 8192
+#define RTL8390_NUM_PORTS 52
+#define RTL8390_BCAST_PHYID_CTRL 0x03ec
+#define RTL8390_PHYREG_ACCESS_CTRL 0x03dc
+#define RTL8390_PHY_CTRL_REG_ADDR GENMASK(9, 5)
+#define RTL8390_PHY_CTRL_PARK_PAGE GENMASK(27, 23)
+#define RTL8390_PHY_CTRL_MAIN_PAGE GENMASK(22, 10)
+#define RTL8390_PHY_CTRL_FAIL BIT(1)
+#define RTL8390_PHY_CTRL_WRITE BIT(3)
+#define RTL8390_PHY_CTRL_READ 0
+#define RTL8390_PHY_CTRL_TYPE_C45 BIT(2)
+#define RTL8390_PHY_CTRL_TYPE_C22 0
+#define RTL8390_PHYREG_CTRL 0x03e0
+#define RTL8390_PHY_CTRL_EXT_PAGE GENMASK(8, 0)
+#define RTL8390_PHYREG_DATA_CTRL 0x03f0
+#define RTL8390_PHY_CTRL_INDATA GENMASK(31, 16)
+#define RTL8390_PHY_CTRL_DATA GENMASK(15, 0)
+#define RTL8390_PHYREG_MMD_CTRL 0x03f4
+#define RTL8390_PHYREG_PORT_CTRL_LOW 0x03e4
+#define RTL8390_PHYREG_PORT_CTRL_HIGH 0x03e8
+#define RTL8390_SMI_PORT_POLLING_CTRL 0x03fc
+
#define RTL9300_NUM_BUSES 4
#define RTL9300_NUM_PAGES 4096
#define RTL9300_NUM_PORTS 28
@@ -457,6 +480,62 @@ static int otto_emdio_8380_write_c45(struct mii_bus *bus, int port,
return otto_emdio_write_cmd(bus, RTL8380_PHY_CTRL_TYPE_C45, &cmd_data);
}
+static int otto_emdio_8390_read_c22(struct mii_bus *bus, int port, int regnum, u32 *value)
+{
+ struct otto_emdio_priv *priv = otto_emdio_bus_to_priv(bus);
+ struct otto_emdio_cmd_regs cmd_data = {
+ .c22_data = FIELD_PREP(RTL8390_PHY_CTRL_REG_ADDR, regnum) |
+ FIELD_PREP(RTL8390_PHY_CTRL_MAIN_PAGE, priv->page[port]),
+ .ext_page = FIELD_PREP(RTL8390_PHY_CTRL_EXT_PAGE, 0x1ff),
+ .io_data = FIELD_PREP(RTL8390_PHY_CTRL_INDATA, port),
+ };
+
+ return otto_emdio_read_cmd(bus, RTL8390_PHY_CTRL_TYPE_C22, &cmd_data,
+ RTL8390_PHY_CTRL_DATA, value);
+}
+
+static int otto_emdio_8390_write_c22(struct mii_bus *bus, int port, int regnum, u16 value)
+{
+ struct otto_emdio_priv *priv = otto_emdio_bus_to_priv(bus);
+ struct otto_emdio_cmd_regs cmd_data = {
+ .c22_data = FIELD_PREP(RTL8390_PHY_CTRL_REG_ADDR, regnum) |
+ FIELD_PREP(RTL8390_PHY_CTRL_MAIN_PAGE, priv->page[port]),
+ .ext_page = FIELD_PREP(RTL8390_PHY_CTRL_EXT_PAGE, 0x1ff),
+ .io_data = FIELD_PREP(RTL8390_PHY_CTRL_INDATA, value),
+ .port_mask_high = (u32)(BIT_ULL(port) >> 32),
+ .port_mask_low = (u32)(BIT_ULL(port)),
+ };
+
+ return otto_emdio_write_cmd(bus, RTL8390_PHY_CTRL_TYPE_C22, &cmd_data);
+}
+
+static int otto_emdio_8390_read_c45(struct mii_bus *bus, int port,
+ int dev_addr, int regnum, u32 *value)
+{
+ struct otto_emdio_cmd_regs cmd_data = {
+ .c45_data = FIELD_PREP(PHY_CTRL_MMD_DEVAD, dev_addr) |
+ FIELD_PREP(PHY_CTRL_MMD_REG, regnum),
+ .io_data = FIELD_PREP(RTL8390_PHY_CTRL_INDATA, port),
+ };
+
+ return otto_emdio_read_cmd(bus, RTL8390_PHY_CTRL_TYPE_C45, &cmd_data,
+ RTL8390_PHY_CTRL_DATA, value);
+}
+
+static int otto_emdio_8390_write_c45(struct mii_bus *bus, int port,
+ int dev_addr, int regnum, u16 value)
+{
+ struct otto_emdio_cmd_regs cmd_data = {
+ .c45_data = FIELD_PREP(PHY_CTRL_MMD_DEVAD, dev_addr) |
+ FIELD_PREP(PHY_CTRL_MMD_REG, regnum),
+ .io_data = FIELD_PREP(RTL8390_PHY_CTRL_INDATA, value),
+ .port_mask_high = (u32)(BIT_ULL(port) >> 32),
+ .port_mask_low = (u32)(BIT_ULL(port)),
+ };
+
+ return otto_emdio_write_cmd(bus, RTL8390_PHY_CTRL_TYPE_C45, &cmd_data);
+}
+
static int otto_emdio_9300_read_c22(struct mii_bus *bus, int port, int regnum, u32 *value)
{
struct otto_emdio_priv *priv = otto_emdio_bus_to_priv(bus);
@@ -963,6 +1042,30 @@ static const struct otto_emdio_info otto_emdio_8380_info = {
.write_c45 = otto_emdio_8380_write_c45,
};
+static const struct otto_emdio_info otto_emdio_8390_info = {
+ .cmd_fail = RTL8390_PHY_CTRL_FAIL,
+ .cmd_read = RTL8390_PHY_CTRL_READ,
+ .cmd_write = RTL8390_PHY_CTRL_WRITE,
+ .cmd_regs = {
+ .broadcast = RTL8390_BCAST_PHYID_CTRL,
+ .c22_data = RTL8390_PHYREG_ACCESS_CTRL,
+ .c45_data = RTL8390_PHYREG_MMD_CTRL,
+ .ext_page = RTL8390_PHYREG_CTRL,
+ .io_data = RTL8390_PHYREG_DATA_CTRL,
+ .port_mask_low = RTL8390_PHYREG_PORT_CTRL_LOW,
+ .port_mask_high = RTL8390_PHYREG_PORT_CTRL_HIGH,
+ },
+ .link_flap = true,
+ .num_buses = RTL8390_NUM_BUSES,
+ .num_pages = RTL8390_NUM_PAGES,
+ .num_ports = RTL8390_NUM_PORTS,
+ .poll_ctrl = RTL8390_SMI_PORT_POLLING_CTRL,
+ .read_c22 = otto_emdio_8390_read_c22,
+ .read_c45 = otto_emdio_8390_read_c45,
+ .write_c22 = otto_emdio_8390_write_c22,
+ .write_c45 = otto_emdio_8390_write_c45,
+};
+
static const struct otto_emdio_info otto_emdio_9300_info = {
.addr_map_base = RTL9300_SMI_PORT0_5_ADDR_CTRL,
.bus_map_base = RTL9300_SMI_PORT0_15_POLLING_SEL,
@@ -1014,6 +1117,7 @@ static const struct otto_emdio_info otto_emdio_9310_info = {
static const struct of_device_id otto_emdio_ids[] = {
{ .compatible = "realtek,rtl8380-mdio", .data = &otto_emdio_8380_info },
+ { .compatible = "realtek,rtl8391-mdio", .data = &otto_emdio_8390_info },
{ .compatible = "realtek,rtl9301-mdio", .data = &otto_emdio_9300_info },
{ .compatible = "realtek,rtl9311-mdio", .data = &otto_emdio_9310_info },
{}
--
2.54.0
^ permalink raw reply related
* Re: [PATCH 3/3] iio: magnetometer: add driver for QST QMC5883L Sensor
From: Jonathan Cameron @ 2026-06-13 11:33 UTC (permalink / raw)
To: Sirat
Cc: robh, krzk+dt, conor+dt, dlechner, nuno.sa, andy, linux-iio,
devicetree, linux-kernel
In-Reply-To: <CANn+LWJoSiCY8cTBNZd9w0H-b-D6C7rH2j8QmB57eTKO0g-x_A@mail.gmail.com>
On Sat, 13 Jun 2026 14:47:25 +0600
Sirat <email@sirat.me> wrote:
> On Fri, Jun 12, 2026 at 11:02 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Fri, 12 Jun 2026 18:45:27 +0600
> > Siratul Islam <email@sirat.me> wrote:
> >
> > > Add driver for the QST QMC5883L 3-Axis Magnetic Sensor
> > > connected via i2c.
> > >
> > > Signed-off-by: Siratul Islam <email@sirat.me>
> >
> > Trying to avoid repeating stuff Joshua already covered.
> > Various comments inline.
> >
> > Thanks,
> >
> > Jonathan
> >
> > > diff --git a/drivers/iio/magnetometer/qmc5883l.c b/drivers/iio/magnetometer/qmc5883l.c
> > > new file mode 100644
> > > index 000000000000..055e51570635
> > > --- /dev/null
> > > +++ b/drivers/iio/magnetometer/qmc5883l.c
> ...
> > > +#define QMC5883L_OSR_512 FIELD_PREP_CONST(QMC5883L_OSR_MASK, 0x00)
> > > +#define QMC5883L_OSR_256 FIELD_PREP_CONST(QMC5883L_OSR_MASK, 0x01)
> > > +#define QMC5883L_OSR_128 FIELD_PREP_CONST(QMC5883L_OSR_MASK, 0x02)
> > > +#define QMC5883L_OSR_64 FIELD_PREP_CONST(QMC5883L_OSR_MASK, 0x03)
> > These are used for matching - normally we'd add defines for the filed value and
> > then use FIELD_GET() to extract it for matching.
> > e.g.
> > #define QMC5883L_OSR_512 0x0
> > #define QMC5883L_OSR_256 0x1
> > rather these.
> >
> I thought using _CONST would be better for compile time checks and not
> needing to use FIELD_GET everywhere. The values 0x01, 0x02 don't have
> a lot of meaning in a vacuum. But of course you would have more
> experience about it and I'd love to know why we shouldn't use
It is mainly about readability at the point of use.
ret = regmap_update_bits(data->regmap,
QMC5883L_OSR_MASK,
FIELD_PREP(QMC5883L_OSR_MASK, osr));
makes it clear to the reader that it is a field assignment and that correct
adjustment of bits has been applied (shifting, masking etc)
ret = regmap_update_bits(data->regmap,
QMC5883L_REG_CTRL1,
QMC5883L_OSR_MASK, osr);
means that to check that the right field is being assigned we have to go
find where osr is set and up to the defines just to check that everything
is as expected.
+ the other strong argument is convention. Certainly in IIO and perhaps
more generally we all field values are the content of the field, not the
shifted variant.
As you note, the compile time checks are lost, but I'm not sure they
bring a lot of value in this particular case where it's copy typed stuff
from the datasheet.
> > > +
> ...
>
> > > +static const int qmc5883l_rng_avail[] = {
> > > + 0, QMC5883L_SCALE_2G, /* 2G */
> >
> > I'm not sure the defines really help. Perhaps push the value down here
> > and then look it up from this array when matching.
> >
> But then the array will be serving more than 1 purpose? kind of like a
> side effect I think.
The array is of the value that are available - so using them to say 'this
one' to me seems fine. If you were to do the common trick of having those as a
2d array (needs a cast for read_avail) then
static const int qmc5883l_rng_avail[][2] = { // or [2][2] if you prefer
[QMC5883L_RNG_2G] = { 0, 83333 },
[QMC5883L_RNG_8G] = { 0, 333333 },
};
At the other place it is looked up it becomes something like:
*val = qmc5773l_rng_avail[data->range][0]
*val2 = qmc5773l_rng_avail[data->range][1];
possibly rename it to
static const int qmc5883l_scales[2][];
as it is not used both to express what is available and for use
as a lookup. It was always the available scales rather than ranges
anyway.
> >
> ...
> > > +
> > > + /* DRDY pin no used in this version of the driver */
> > > + ret = regmap_write(data->regmap, QMC5883L_REG_CTRL2,
> > > + QMC5883L_INT_DISABLE);
> > I don't mind if these sorts of cases go a little over 80 chars as sometimes
> > it helps readability.
> >
> I try to disable clang-format for cases like this. It's an oversight on my side.
> >
> > Does it really reset with interrupts on? That's odd. Mind you the
> > INT_ENB sounds like it would be an enable but as you have named it here
> > it is actually a disable so all bets are off when it comes to sensible ;)
> >
> The datasheet says "The interrupt can be disabled by set 0AH[0] = 1"
> which I took as it's enabled by default.
> I explicitly disable it just to be safe.
That's fair enough, if very odd given that's a typical path to annoying
races. Might be worth just testing the hardware to check the default
> >
> ...
> > > +static const struct of_device_id qmc5883l_match[] = {
> > > + { .compatible = "qstcorp,qmc5883l" },
> > > + { },
> >
> > As below, no comma.
> >
> I did it so that clang-format doesn't wrap it all in 1 line. I forgot
> to remove the comma before sending the patch. Sorry.
> > > +};
> >
>
> Thanks
> Sirat
^ permalink raw reply
* Re: [PATCH 4/4] arm64: dts: allwinner: add Radxa Cubie A7S
From: Jernej Škrabec @ 2026-06-13 11:37 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Chen-Yu Tsai,
Samuel Holland, Maxime Ripard, Ulf Hansson, enzo.adriano.code
Cc: devicetree, linux-arm-kernel, linux-sunxi, linux-kernel,
linux-mmc, Enzo Adriano
In-Reply-To: <20260613-a733-dts-v1-public-ready-v1-4-7787c94681db@gmail.com>
Dne sobota, 13. junij 2026 ob 11:42:16 Srednjeevropski poletni čas je Enzo Adriano via B4 Relay napisal(a):
> From: Enzo Adriano <enzo.adriano.code@gmail.com>
>
> Add the Radxa Cubie A7S board description with serial console and SD card
> boot support.
>
> Ethernet remains disabled until the GMAC210 wrapper, clocks, resets,
> MDIO, PHY reset, PHY power, and link behavior are proven.
>
> Signed-off-by: Enzo Adriano <enzo.adriano.code@gmail.com>
> ---
> arch/arm64/boot/dts/allwinner/Makefile | 1 +
> .../boot/dts/allwinner/sun60i-a733-cubie-a7s.dts | 48 ++++++++++++++++++++++
> 2 files changed, 49 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/allwinner/Makefile b/arch/arm64/boot/dts/allwinner/Makefile
> index d116864b6c2b..824cc35152db 100644
> --- a/arch/arm64/boot/dts/allwinner/Makefile
> +++ b/arch/arm64/boot/dts/allwinner/Makefile
> @@ -56,6 +56,7 @@ dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-2024.dtb
> dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-h.dtb
> dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-plus.dtb
> dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-sp.dtb
> +dtb-$(CONFIG_ARCH_SUNXI) += sun60i-a733-cubie-a7s.dtb
> dtb-$(CONFIG_ARCH_SUNXI) += sun55i-a527-cubie-a5e.dtb
> dtb-$(CONFIG_ARCH_SUNXI) += sun55i-h728-x96qpro+.dtb
> dtb-$(CONFIG_ARCH_SUNXI) += sun55i-t527-avaota-a1.dtb
> diff --git a/arch/arm64/boot/dts/allwinner/sun60i-a733-cubie-a7s.dts b/arch/arm64/boot/dts/allwinner/sun60i-a733-cubie-a7s.dts
> new file mode 100644
> index 000000000000..453761a96323
> --- /dev/null
> +++ b/arch/arm64/boot/dts/allwinner/sun60i-a733-cubie-a7s.dts
> @@ -0,0 +1,48 @@
> +// SPDX-License-Identifier: (GPL-2.0-only OR MIT)
> +
> +/dts-v1/;
> +
> +#include "sun60i-a733.dtsi"
> +
> +/ {
> + model = "Radxa Cubie A7S";
> + compatible = "radxa,cubie-a7s", "allwinner,sun60i-a733";
> +
> + aliases {
> + serial0 = &uart0;
> + mmc0 = &mmc0;
> + };
> +
> + chosen {
> + stdout-path = "serial0:115200n8";
> + };
> +
> + reg_vcc3v3: vcc3v3 {
> + compatible = "regulator-fixed";
> + regulator-name = "vcc-3v3";
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-always-on;
> + };
> +};
> +
> +&mmc0 {
> + vmmc-supply = <®_vcc3v3>;
> + bus-width = <4>;
> + no-mmc;
> + no-sdio;
> + status = "okay";
> +};
> +
> +&pio {
> + uart0_pb9_pb10_pins: uart0-pb9-pb10-pins {
> + pins = "PB9", "PB10";
> + function = "uart0";
> + };
Besides sashiko bot comments, these pins should go to main A733 DTSI,
like it's done for other SoCs.
In any case, it's a bit early for DT. At least clocks should land before.
Best regards,
Jernej
> +};
> +
> +&uart0 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&uart0_pb9_pb10_pins>;
> + status = "okay";
> +};
>
>
^ permalink raw reply
* Re: [PATCH 3/4] arm64: dts: allwinner: add Allwinner A733 SoC
From: sashiko-bot @ 2026-06-13 11:50 UTC (permalink / raw)
To: Enzo Adriano via B4 Relay; +Cc: robh, devicetree, conor+dt, linux-sunxi
In-Reply-To: <20260613-a733-dts-v1-public-ready-v1-3-7787c94681db@gmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] The patch references missing header files for CCU clock and reset bindings, which will cause a build failure.
- [Low] New compatible strings for the CCU and pinctrl are used without being documented in the YAML schemas.
--
commit 08b3c222d5fc2b42b4c8ea3dd602ebcc113d5545
Author: Enzo Adriano <enzo.adriano.code@gmail.com>
arm64: dts: allwinner: add Allwinner A733 SoC
Add the initial A733 SoC description with CPUs, timers, interrupt
controller, clocks, pinctrl, UART0, and MMC0.
Keep peripherals disabled by default. Board DTS files can enable only the
devices that are proven on their hardware.
> diff --git a/arch/arm64/boot/dts/allwinner/sun60i-a733.dtsi b/arch/arm64/boot/dts/allwinner/sun60i-a733.dtsi
> new file mode 100644
> --- /dev/null
> +++ b/arch/arm64/boot/dts/allwinner/sun60i-a733.dtsi
> @@ -0,0 +1,198 @@
> +// SPDX-License-Identifier: (GPL-2.0-only OR MIT)
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/clock/sun60i-a733-ccu.h>
> +#include <dt-bindings/reset/sun60i-a733-ccu.h>
[Severity: Low]
Are these header files introduced in a different patch? Compiling this device
tree will fail at the preprocessor stage because these headers are missing
from the baseline tree and aren't added in this patch.
[ ... ]
> + pio: pinctrl@2000000 {
> + compatible = "allwinner,sun60i-a733-pinctrl";
[Severity: Low]
Are these new compatible strings documented in the dt-binding YAML schemas?
Adding new compatibles without their corresponding binding documentation will
trigger make dtbs_check warnings.
> + reg = <0x02000000 0x600>;
[ ... ]
> + ccu: clock-controller@2002000 {
> + compatible = "allwinner,sun60i-a733-ccu";
[Severity: Low]
Will this compatible string also need to be added to the YAML schemas to
avoid validation warnings?
> + reg = <0x02002000 0x2000>;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260613-a733-dts-v1-public-ready-v1-0-7787c94681db@gmail.com?part=3
^ permalink raw reply
* Re: [PATCH v4 6/7] arm64: dts: allwinner: a100: reserve RAM for ATF
From: Alexander Sverdlin @ 2026-06-13 12:23 UTC (permalink / raw)
To: Jernej Škrabec, linux-arm-kernel, linux-sunxi
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Chen-Yu Tsai,
Samuel Holland, Hans de Goede, Dmitry Torokhov, Andre Przywara,
Jun Yan, Lukas Schmid, J. Neuschäfer, Eric Biggers,
Michal Simek, Luca Weiss, Sven Peter, Maxime Ripard, devicetree,
linux-kernel, linux-input
In-Reply-To: <FE7Vh4yfTmGMM24i18Wwwg@gmail.com>
Hi Jernej,
On Sat, 2026-06-13 at 11:38 +0200, Jernej Škrabec wrote:
> > > > Add reserved-memory node carving out Trusted Firmware-A region spanning
> > > > fixed 256K from physical address 0x40000000. Even though Allwinner ATF
> > > > itself passes the address range in the fdt to U-Boot, U-Boot currently
> > > > only reserves this memory internally, but doesn't carve out the region
> > > > in the fdt passed to Linux.
> > > >
> > > > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
> > >
> > > NAK. It is job of boot procedure to properly inject TF-A reserved node.
> > > Any issue should be fixed there.
> >
> >
> > like in commit 0d17c865118881609ea7e381c7cadbb7979cc596
> > ("arm64: dts: allwinner: Add Allwinner H616 .dtsi file")
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> >
> > ? ;-)
>
> I was against that too.
>
> It's time to be fixed in bootloader.
I agree, I was wondering about this shortcut as well... I'll implement
some sort of generic reserved memory handling in U-Boot.
--
Alexander Sverdlin.
^ permalink raw reply
* Re: [PATCH v5 2/3] pwm: rp1: Add RP1 PWM controller driver
From: Julian Braha @ 2026-06-13 12:27 UTC (permalink / raw)
To: Andrea della Porta, Uwe Kleine-König, linux-pwm, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Florian Fainelli,
Broadcom internal kernel review list, devicetree,
linux-rpi-kernel, linux-arm-kernel, linux-kernel, Naushir Patuck,
Stanimir Varbanov, mbrugger
In-Reply-To: <f8dd46a553351adaf9d29fbba9f98e803b672fe7.1780670224.git.andrea.porta@suse.com>
On 6/12/26 15:01, Andrea della Porta wrote:
> +config PWM_RASPBERRYPI_RP1
> + tristate "RP1 PWM support"
> + depends on MISC_RP1 || COMPILE_TEST
> + depends on HAS_IOMEM
> + select REGMAP_MMIO
> + select MFD_SYSCON
> + help
> + PWM framework driver for Raspberry Pi RP1 controller.
> +
Hi Andrea,
Selecting REGMAP_MMIO is unnecessary here since you're already selecting
MFD_SYSCON.
- Julian Braha
^ permalink raw reply
* [PATCH v3 0/3] clk: samsung: exynos990: Fix PERIS gate clock parents and add TMU_SUB
From: Denzeel Oliva @ 2026-06-13 12:36 UTC (permalink / raw)
To: Krzysztof Kozlowski, Sylwester Nawrocki, Chanwoo Choi,
Peter Griffin, Alim Akhtar, Michael Turquette, Stephen Boyd,
Brian Masney, Rob Herring, Conor Dooley
Cc: linux-samsung-soc, linux-clk, devicetree, linux-arm-kernel,
linux-kernel, Denzeel Oliva
Fix several PERIS CMU clock parent mismatches and add the missing
TMU_SUB_PCLK gate clock. The dt-bindings patch adds the new clock
ID. The second patch adds the TMU_SUB_PCLK gate. The third patch
corrects eight gate clock parents and reorders the GIC mux parents.
Signed-off-by: Denzeel Oliva <wachiturroxd150@gmail.com>
---
v2 -> v3:
- Split TMU_SUB gate addition from parent fixes into separate
patches (Krzysztof)
- Now three patches: dt-bindings, add gate, fix parents
v2: https://lore.kernel.org/r/20260613-exynos990-peris-fix-v2-v2-0-3dff7ade75b3@gmail.com
v1: https://lore.kernel.org/r/20260528-exynos990-peris-fix-v1-1-5b65aa7def2d@gmail.com
---
Denzeel Oliva (3):
dt-bindings: clock: exynos990: Add CLK_GOUT_PERIS_TMU_SUB_PCLK
clk: samsung: exynos990: Add PERIS TMU_SUB_PCLK gate
clk: samsung: exynos990: Fix PERIS gate clock parents
drivers/clk/samsung/clk-exynos990.c | 24 ++++++++++++++----------
include/dt-bindings/clock/samsung,exynos990.h | 1 +
2 files changed, 15 insertions(+), 10 deletions(-)
---
base-commit: c425609d6ac4012c8bbf01ec2e10e801b1923a7b
change-id: 20260613-exynos990-peris-fix-v3-fac19b879206
Best regards,
--
Denzeel Oliva <wachiturroxd150@gmail.com>
^ permalink raw reply
* [PATCH v3 1/3] dt-bindings: clock: exynos990: Add CLK_GOUT_PERIS_TMU_SUB_PCLK
From: Denzeel Oliva @ 2026-06-13 12:36 UTC (permalink / raw)
To: Krzysztof Kozlowski, Sylwester Nawrocki, Chanwoo Choi,
Peter Griffin, Alim Akhtar, Michael Turquette, Stephen Boyd,
Brian Masney, Rob Herring, Conor Dooley
Cc: linux-samsung-soc, linux-clk, devicetree, linux-arm-kernel,
linux-kernel, Denzeel Oliva
In-Reply-To: <20260613-exynos990-peris-fix-v3-v3-0-2b230db78ae4@gmail.com>
Add the missing TMU_SUB_PCLK clock ID for the Exynos990 PERIS CMU.
Signed-off-by: Denzeel Oliva <wachiturroxd150@gmail.com>
---
include/dt-bindings/clock/samsung,exynos990.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/dt-bindings/clock/samsung,exynos990.h b/include/dt-bindings/clock/samsung,exynos990.h
index 47540307cb52..c06f591d9d90 100644
--- a/include/dt-bindings/clock/samsung,exynos990.h
+++ b/include/dt-bindings/clock/samsung,exynos990.h
@@ -434,5 +434,6 @@
#define CLK_GOUT_PERIS_TMU_TOP_PCLK 17
#define CLK_GOUT_PERIS_OTP_CON_BIRA_OSCCLK 18
#define CLK_GOUT_PERIS_OTP_CON_TOP_OSCCLK 19
+#define CLK_GOUT_PERIS_TMU_SUB_PCLK 20
#endif
--
2.54.0
^ permalink raw reply related
* [PATCH v3 2/3] clk: samsung: exynos990: Add PERIS TMU_SUB_PCLK gate
From: Denzeel Oliva @ 2026-06-13 12:36 UTC (permalink / raw)
To: Krzysztof Kozlowski, Sylwester Nawrocki, Chanwoo Choi,
Peter Griffin, Alim Akhtar, Michael Turquette, Stephen Boyd,
Brian Masney, Rob Herring, Conor Dooley
Cc: linux-samsung-soc, linux-clk, devicetree, linux-arm-kernel,
linux-kernel, Denzeel Oliva
In-Reply-To: <20260613-exynos990-peris-fix-v3-v3-0-2b230db78ae4@gmail.com>
Add the missing CLK_GOUT_PERIS_TMU_SUB_PCLK gate clock for the Thermal
Management Unit sub-block and update CLKS_NR_PERIS accordingly.
Signed-off-by: Denzeel Oliva <wachiturroxd150@gmail.com>
---
drivers/clk/samsung/clk-exynos990.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/samsung/clk-exynos990.c b/drivers/clk/samsung/clk-exynos990.c
index 4385c3b76dd6..ee3566b8e57c 100644
--- a/drivers/clk/samsung/clk-exynos990.c
+++ b/drivers/clk/samsung/clk-exynos990.c
@@ -21,7 +21,7 @@
#define CLKS_NR_HSI0 (CLK_GOUT_HSI0_LHS_ACEL_D_HSI0_CLK + 1)
#define CLKS_NR_PERIC0 (CLK_GOUT_PERIC0_SYSREG_PCLK + 1)
#define CLKS_NR_PERIC1 (CLK_GOUT_PERIC1_XIU_P_ACLK + 1)
-#define CLKS_NR_PERIS (CLK_GOUT_PERIS_OTP_CON_TOP_OSCCLK + 1)
+#define CLKS_NR_PERIS (CLK_GOUT_PERIS_TMU_SUB_PCLK + 1)
/* ---- CMU_TOP ------------------------------------------------------------- */
@@ -2619,6 +2619,10 @@ static const struct samsung_gate_clock peris_gate_clks[] __initconst = {
"gout_peris_d_tzpc_peris_pclk", "mout_peris_bus_user",
CLK_CON_GAT_GOUT_BLK_PERIS_UID_D_TZPC_PERIS_IPCLKPORT_PCLK,
21, 0, 0),
+ GATE(CLK_GOUT_PERIS_TMU_SUB_PCLK,
+ "gout_peris_tmu_sub_pclk", "mout_peris_bus_user",
+ CLK_CON_GAT_GOUT_BLK_PERIS_UID_TMU_SUB_IPCLKPORT_PCLK,
+ 21, 0, 0),
GATE(CLK_GOUT_PERIS_TMU_TOP_PCLK,
"gout_peris_tmu_top_pclk", "mout_peris_clk_peris_gic",
CLK_CON_GAT_GOUT_BLK_PERIS_UID_TMU_TOP_IPCLKPORT_PCLK,
--
2.54.0
^ permalink raw reply related
* [PATCH v3 3/3] clk: samsung: exynos990: Fix PERIS gate clock parents
From: Denzeel Oliva @ 2026-06-13 12:36 UTC (permalink / raw)
To: Krzysztof Kozlowski, Sylwester Nawrocki, Chanwoo Choi,
Peter Griffin, Alim Akhtar, Michael Turquette, Stephen Boyd,
Brian Masney, Rob Herring, Conor Dooley
Cc: linux-samsung-soc, linux-clk, devicetree, linux-arm-kernel,
linux-kernel, Denzeel Oliva
In-Reply-To: <20260613-exynos990-peris-fix-v3-v3-0-2b230db78ae4@gmail.com>
Correct eight PERIS gate clock parents to match the hardware clock
tree and reorder the GIC mux parents so mout_peris_bus_user is the
default source.
Signed-off-by: Denzeel Oliva <wachiturroxd150@gmail.com>
---
drivers/clk/samsung/clk-exynos990.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/clk/samsung/clk-exynos990.c b/drivers/clk/samsung/clk-exynos990.c
index ee3566b8e57c..df5928833b23 100644
--- a/drivers/clk/samsung/clk-exynos990.c
+++ b/drivers/clk/samsung/clk-exynos990.c
@@ -2551,7 +2551,7 @@ static const unsigned long peris_clk_regs[] __initconst = {
/* Parent clock list for CMU_PERIS muxes */
PNAME(mout_peris_bus_user_p) = { "oscclk", "mout_cmu_peris_bus" };
-PNAME(mout_peris_clk_peris_gic_p) = { "oscclk", "mout_peris_bus_user" };
+PNAME(mout_peris_clk_peris_gic_p) = { "mout_peris_bus_user", "oscclk" };
static const struct samsung_mux_clock peris_mux_clks[] __initconst = {
MUX(CLK_MOUT_PERIS_BUS_USER, "mout_peris_bus_user",
@@ -2584,15 +2584,15 @@ static const struct samsung_gate_clock peris_gate_clks[] __initconst = {
CLK_CON_GAT_GOUT_BLK_PERIS_UID_RSTNSYNC_CLK_PERIS_BUSP_IPCLKPORT_CLK,
21, 0, 0),
GATE(CLK_GOUT_PERIS_CLK_PERIS_OSCCLK_CLK,
- "gout_peris_clk_peris_oscclk_clk", "mout_peris_bus_user",
+ "gout_peris_clk_peris_oscclk_clk", "oscclk",
CLK_CON_GAT_CLK_BLK_PERIS_UID_RSTNSYNC_CLK_PERIS_OSCCLK_IPCLKPORT_CLK,
21, 0, 0),
GATE(CLK_GOUT_PERIS_CLK_PERIS_GIC_CLK,
- "gout_peris_clk_peris_gic_clk", "mout_peris_bus_user",
+ "gout_peris_clk_peris_gic_clk", "mout_peris_clk_peris_gic",
CLK_CON_GAT_GOUT_BLK_PERIS_UID_RSTNSYNC_CLK_PERIS_GIC_IPCLKPORT_CLK,
21, 0, 0),
GATE(CLK_GOUT_PERIS_AD_AXI_P_PERIS_ACLKM,
- "gout_peris_ad_axi_p_peris_aclkm", "mout_peris_bus_user",
+ "gout_peris_ad_axi_p_peris_aclkm", "mout_peris_clk_peris_gic",
CLK_CON_GAT_GOUT_BLK_PERIS_UID_AD_AXI_P_PERIS_IPCLKPORT_ACLKM,
21, CLK_IGNORE_UNUSED, 0),
GATE(CLK_GOUT_PERIS_OTP_CON_BIRA_PCLK,
@@ -2600,19 +2600,19 @@ static const struct samsung_gate_clock peris_gate_clks[] __initconst = {
CLK_CON_GAT_GOUT_BLK_PERIS_UID_OTP_CON_BIRA_IPCLKPORT_PCLK,
21, 0, 0),
GATE(CLK_GOUT_PERIS_GIC_CLK,
- "gout_peris_gic_clk", "mout_peris_bus_user",
+ "gout_peris_gic_clk", "mout_peris_clk_peris_gic",
CLK_CON_GAT_GOUT_BLK_PERIS_UID_GIC_IPCLKPORT_CLK,
21, CLK_IS_CRITICAL, 0),
GATE(CLK_GOUT_PERIS_LHM_AXI_P_PERIS_CLK,
- "gout_peris_lhm_axi_p_peris_clk", "oscclk",
+ "gout_peris_lhm_axi_p_peris_clk", "mout_peris_bus_user",
CLK_CON_GAT_GOUT_BLK_PERIS_UID_LHM_AXI_P_PERIS_IPCLKPORT_I_CLK,
21, CLK_IGNORE_UNUSED, 0),
GATE(CLK_GOUT_PERIS_MCT_PCLK,
- "gout_peris_mct_pclk", "mout_peris_clk_peris_gic",
+ "gout_peris_mct_pclk", "mout_peris_bus_user",
CLK_CON_GAT_GOUT_BLK_PERIS_UID_MCT_IPCLKPORT_PCLK,
21, 0, 0),
GATE(CLK_GOUT_PERIS_OTP_CON_TOP_PCLK,
- "gout_peris_otp_con_top_pclk", "mout_peris_clk_peris_gic",
+ "gout_peris_otp_con_top_pclk", "mout_peris_bus_user",
CLK_CON_GAT_GOUT_BLK_PERIS_UID_OTP_CON_TOP_IPCLKPORT_PCLK,
21, 0, 0),
GATE(CLK_GOUT_PERIS_D_TZPC_PERIS_PCLK,
@@ -2624,7 +2624,7 @@ static const struct samsung_gate_clock peris_gate_clks[] __initconst = {
CLK_CON_GAT_GOUT_BLK_PERIS_UID_TMU_SUB_IPCLKPORT_PCLK,
21, 0, 0),
GATE(CLK_GOUT_PERIS_TMU_TOP_PCLK,
- "gout_peris_tmu_top_pclk", "mout_peris_clk_peris_gic",
+ "gout_peris_tmu_top_pclk", "mout_peris_bus_user",
CLK_CON_GAT_GOUT_BLK_PERIS_UID_TMU_TOP_IPCLKPORT_PCLK,
21, 0, 0),
GATE(CLK_GOUT_PERIS_OTP_CON_BIRA_OSCCLK,
--
2.54.0
^ permalink raw reply related
* Re: [PATCH v3 3/3] clk: samsung: exynos990: Fix PERIS gate clock parents
From: sashiko-bot @ 2026-06-13 12:41 UTC (permalink / raw)
To: Denzeel Oliva; +Cc: robh, conor+dt, devicetree
In-Reply-To: <20260613-exynos990-peris-fix-v3-v3-3-2b230db78ae4@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Reordering a clock parent array (PNAME) to change the default clock parent falsifies the hardware register mapping.
--
commit 053886a8fb5adfd1285c3768f792e89fdc6baa21
Author: Denzeel Oliva <wachiturroxd150@gmail.com>
clk: samsung: exynos990: Fix PERIS gate clock parents
This commit corrects eight PERIS gate clock parents to match the hardware
clock tree. It also reorders the GIC mux parents to make mout_peris_bus_user
the default source.
> diff --git a/drivers/clk/samsung/clk-exynos990.c b/drivers/clk/samsung/clk-exynos990.c
> index ee3566b8e57c9..df5928833b232 100644
> --- a/drivers/clk/samsung/clk-exynos990.c
> +++ b/drivers/clk/samsung/clk-exynos990.c
> @@ -2551,7 +2551,7 @@ static const unsigned long peris_clk_regs[] __initconst = {
>
> /* Parent clock list for CMU_PERIS muxes */
> PNAME(mout_peris_bus_user_p) = { "oscclk", "mout_cmu_peris_bus" };
> -PNAME(mout_peris_clk_peris_gic_p) = { "oscclk", "mout_peris_bus_user" };
> +PNAME(mout_peris_clk_peris_gic_p) = { "mout_peris_bus_user", "oscclk" };
[Severity: High]
Does reordering the mout_peris_clk_peris_gic_p array break the hardware
register mapping?
In the Exynos clock framework, the indices of the PNAME array correspond
directly to the hardware multiplexer register bit values (where index 0 maps
to register value 0). Reordering the array to change the default parent could
cause the framework to write the wrong index to the mux register when
clk_set_parent() is called.
Should the default clock parent be configured using the assigned-clocks and
assigned-clock-parents properties in the Device Tree instead of modifying the
driver's hardware description?
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260613-exynos990-peris-fix-v3-v3-0-2b230db78ae4@gmail.com?part=3
^ permalink raw reply
* Re: [PATCH v2 02/16] device property: Add fwnode_graph_get_next_port_endpoint()
From: Andy Shevchenko @ 2026-06-13 13:00 UTC (permalink / raw)
To: Chen-Yu Tsai
Cc: Bartosz Golaszewski, Greg Kroah-Hartman, Daniel Scally,
Heikki Krogerus, Sakari Ailus, Rafael J. Wysocki,
Danilo Krummrich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, AngeloGioacchino Del Regno, Alan Stern,
linux-acpi, driver-core, linux-pm, linux-usb, devicetree,
linux-mediatek, linux-arm-kernel, linux-kernel,
Manivannan Sadhasivam
In-Reply-To: <CAGXv+5EHHWfiWgqPpf-RMKoSQLc2cd9OA4Z36SNoL5C53kVh2g@mail.gmail.com>
On Fri, Jun 12, 2026 at 04:20:18PM +0900, Chen-Yu Tsai wrote:
> On Wed, Jun 10, 2026 at 11:08 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, Jun 10, 2026 at 04:40:36PM +0800, Chen-Yu Tsai wrote:
...
> > > +struct fwnode_handle *fwnode_graph_get_next_port_endpoint(const struct fwnode_handle *port,
> > > + struct fwnode_handle *prev)
> > > +{
> > > + struct fwnode_handle *ep;
> >
> > Unused?
> >
> > > + while (1) {
> >
> > This is usually harder to read and follow. It's like "pay much attention on
> > the code", but here no rocket science, no code to really pay attention to.
> >
> > > + prev = fwnode_get_next_child_node(port, prev);
> > > + if (!prev)
> > > + break;
> > > +
> > > + if (WARN(!fwnode_name_eq(prev, "endpoint"),
> > > + "non endpoint node is used (%pfw)", prev))
> > > + continue;
> > > +
> > > + break;
> > > + }
> > > +
> > > + return prev;
> > > +}
> >
> > So, this can be rewritten as
> >
> > ep = prev;
> > do {
> > ep = fwnode_get_next_child_node(port, ep);
> > if (fwnode_name_eq(ep, "endpoint"))
> > break;
> >
> > WARN_ON(ep, ...);
> > } while (ep);
> >
> > return ep;
> >
> > But also big question why? to WARN*(). There is no use in the entire
> > property.c.
>
> Will drop. This function was lifted from drivers/of/property.c then
> adapted to the fwnode APIs, so it still has the structure of its
> origin. With the WARN() gone, rewriting it as do {} while() becomes:
>
> do {
> prev = fwnode_get_next_child_node(port, prev);
> if (prev && fwnode_name_eq(prev, "endpoint"))
'prev &&' is not needed.
> break;
> } while (prev);
>
> return prev;
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: hwmon: pmbus: ti,lm25066: add current limit properties
From: Guenter Roeck @ 2026-06-13 13:05 UTC (permalink / raw)
To: Conor Dooley
Cc: Potin Lai, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Zev Weiss, linux-hwmon, devicetree, linux-kernel, Cosmo Chou,
Mike Hsieh, Potin Lai
In-Reply-To: <20260612-sibling-battalion-453d3d239aaa@spud>
On 6/12/26 14:13, Conor Dooley wrote:
> On Fri, Jun 12, 2026 at 10:19:14AM -0700, Guenter Roeck wrote:
>> On 6/12/26 09:12, Conor Dooley wrote:
>>> On Fri, Jun 12, 2026 at 05:10:38PM +0800, Potin Lai wrote:
>>>> On Fri, Jun 12, 2026 at 1:27 AM Conor Dooley <conor@kernel.org> wrote:
>>>>>
>>>>> On Thu, Jun 11, 2026 at 05:58:44PM +0800, Potin Lai wrote:
>>>>>> Add mutually exclusive 'ti,cl-smbus-high' and 'ti,cl-smbus-low' boolean
>>>>>> properties to configure the device's Current Limit (CL) behavior using
>>>>>> SMBus settings instead of physical pins.
>>>>>>
>>>>>> Signed-off-by: Potin Lai <potin.lai.pt@gmail.com>
>>>>>> ---
>>>>>> .../devicetree/bindings/hwmon/pmbus/ti,lm25066.yaml | 20 ++++++++++++++++++++
>>>>>> 1 file changed, 20 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/ti,lm25066.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/ti,lm25066.yaml
>>>>>> index a20f140dc79a..95ea7c26dec2 100644
>>>>>> --- a/Documentation/devicetree/bindings/hwmon/pmbus/ti,lm25066.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/hwmon/pmbus/ti,lm25066.yaml
>>>>>> @@ -46,6 +46,26 @@ properties:
>>>>>>
>>>>>> additionalProperties: false
>>>>>>
>>>>>> + ti,cl-smbus-high:
>>>>>> + description: |
>>>>>> + Configure the Current Limit (CL) to use the SMBus high setting.
>>>>>> + type: boolean
>>>>>> +
>>>>>> + ti,cl-smbus-low:
>>>>>> + description: |
>>>>>> + Configure the Current Limit (CL) to use the SMBus low setting.
>>>>>> + type: boolean
>>>>>
>>>>> What's smbus specific about this? If the pin was connected to a GPIO,
>>>>> you'd then need to have different properties or use these ones with an
>>>>> inaccurate name.
>>>>>
>>>>
>>>> The "smbus" in the property name was originally meant to indicate
>>>> that the setting is configured via the internal register over the SMBus (I2C)
>>>> interface, rather than physical pins.
>>>
>>> Right, but if you do it via the physical pins using a gpio, you still
>>> need a way to say what limit is. The status quo only works if the limit
>>> pin is tied high or low.
>>>
>>
>> The physical pin is supposed to be connected to ground or left floating.
>> It seems unlikely that anyone would ever have the idea of connecting it
>> to a GPIO pin, and doing so would for sure mess up the driver because
>> its state is only read in the probe function. The configuration here
>
> Well yeah, "obviously" if someone wanted to use a GPIO the driver would
> have to change to handle that - but probably not that much since it'd be
> a static setting that could be done at probe.
>
> I get that it may be unlikely, but it seems like a reasonable thing that
> someone might want to do, and renaming the property to not exclude that
> usecase seems to be "free".
>
It is not only unlikely, it would be risky and potentially result in
undefined behavior. The pin is supposed to be static. It is undefined
if the hardware evaluates it once after power-up, after "power good"
was detected (if the specific chip supports it), or continuously.
Making the pin run-time configurable would be a risky endeavor with
zero gain since the value can be set by software configuration,
while at the same time adding complexity to the hardware.
Guenter
^ permalink raw reply
* Re: [PATCH v5 5/9] block: implement NVMEM provider
From: sashiko-bot @ 2026-06-13 13:21 UTC (permalink / raw)
To: Loic Poulain; +Cc: devicetree, robh, conor+dt
In-Reply-To: <20260612-block-as-nvmem-v5-5-95e0b30fff90@oss.qualcomm.com>
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] NVMEM provider opens the block device with exclusive write restriction, causing conflicts on mounted systems.
- [High] Hardcoded 4GB size limit incorrectly rejects modern eMMC devices.
- [High] Integer overflow in size assignment exposes massive out-of-bounds size.
- [High] Stale `dev_t` reference can cause data leaks across block device hotplugs (ABA problem).
- [High] Using `__free(fput)` defers block device claim release, creating race conditions.
--
commit 459b659c960efdc8a9e2177c5bc4f832e7ee5730
Author: Daniel Golle <daniel@makrotopia.org>
block: implement NVMEM provider
On embedded devices using an eMMC it is common that one or more partitions
on the eMMC are used to store MAC addresses and Wi-Fi calibration EEPROM
data. Allow referencing the partition in device tree for the kernel and
Wi-Fi drivers accessing it via the NVMEM layer.
> diff --git a/block/blk-nvmem.c b/block/blk-nvmem.c
> new file mode 100644
> index 0000000000000..c005f059d9fe5
> --- /dev/null
> +++ b/block/blk-nvmem.c
> @@ -0,0 +1,109 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
[ ... ]
> +static int blk_nvmem_reg_read(void *priv, unsigned int from, void *val, size_t bytes)
> +{
> + blk_mode_t mode = BLK_OPEN_READ | BLK_OPEN_RESTRICT_WRITES;
[Severity: High]
Since the NVMEM provider claims the whole disk (disk->part0), will opening it
with BLK_OPEN_RESTRICT_WRITES and an exclusive holder (priv) return -EBUSY if
any partition on the disk is already mounted or opened for writing?
> + dev_t devt = (dev_t)(uintptr_t)priv;
> + size_t bytes_left = bytes;
> + loff_t pos = from;
> + int ret = 0;
> +
> + struct file *bdev_file __free(fput) = bdev_file_open_by_dev(devt, mode, priv, NULL);
[Severity: High]
Does using __free(fput) defer the release of the block device claim? Because
fput() schedules the file release asynchronously, could another subsystem
trying to mount or claim the device immediately after an NVMEM read
sporadically fail with -EBUSY?
Should this use bdev_fput() instead to synchronously yield the claim before
the rest of the file release?
> + if (IS_ERR(bdev_file))
> + return PTR_ERR(bdev_file);
[ ... ]
> +void blk_nvmem_add(struct block_device *bdev)
> +{
[ ... ]
> + /*
> + * skip block device too large to be represented as NVMEM devices,
> + * the NVMEM reg_read callback uses an unsigned int offset
> + */
> + if (bdev_nr_bytes(bdev) > UINT_MAX) {
[Severity: High]
Since the NVMEM provider is registered against the whole disk rather than the
individual NVMEM partition, does this 4GB limit incorrectly reject most modern
eMMC devices?
> + dev_warn(dev, "block device too large to be an NVMEM provider\n");
> + return;
> + }
> +
> + config.id = NVMEM_DEVID_NONE;
> + config.dev = dev;
> + config.name = dev_name(dev);
> + config.owner = THIS_MODULE;
> + config.priv = (void *)(uintptr_t)dev->devt;
[Severity: High]
Can storing the ephemeral dev_t identifier without holding a pinned block
device reference cause an ABA problem?
If the block device is removed, blk_nvmem_del() calls nvmem_unregister()
which drops a reference but does not synchronously wait for consumers to
close. If a new device is plugged in and reuses the same dev_t, could a
consumer reading from the still-alive NVMEM device inadvertently access the
unrelated new block device?
> + config.reg_read = blk_nvmem_reg_read;
> + config.size = bdev_nr_bytes(bdev);
[Severity: High]
Can this assignment cause an integer overflow?
The config.size field in struct nvmem_config is a signed 32-bit int, so disk
sizes between 2GB and 4GB will overflow to a negative value. When
nvmem_register() sign-extends this into its 64-bit size_t field, won't it
result in a massive out-of-bounds size being exposed via sysfs?
> + config.word_size = 1;
> + config.stride = 1;
> + config.read_only = true;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-block-as-nvmem-v5-0-95e0b30fff90@oss.qualcomm.com?part=5
^ permalink raw reply
* Re: [PATCH v5 1/9] block: partitions: of: Skip child nodes without reg property
From: sashiko-bot @ 2026-06-13 13:21 UTC (permalink / raw)
To: Loic Poulain; +Cc: robh, devicetree, conor+dt
In-Reply-To: <20260612-block-as-nvmem-v5-1-95e0b30fff90@oss.qualcomm.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [High] The device node reference for `partitions_np` is acquired but leaked on both the success and incompatibility early-return paths.
- [High] Potential NULL pointer dereference in `strscpy()` if a partition node lacks both `label` and `name` properties.
- [High] Malicious device tree properties `#address-cells` and `#size-cells` can cause integer underflow, bypassing the length check and leading to massive out-of-bounds reads.
--
commit a6b306502d7065c56b83fd73a46be12067fb56e3
Author: Loic Poulain <loic.poulain@oss.qualcomm.com>
block: partitions: of: Skip child nodes without reg property
Child nodes of a fixed-partitions node are not necessarily partition
entries, for example an nvmem-layout node has no reg property. The
current code passes a NULL reg pointer and uninitialized len to the
length check, which can result in a kernel panic or silent failure to
register any partitions.
> diff --git a/block/partitions/of.c b/block/partitions/of.c
> index c22b606610981..534e02a9d85f6 100644
> --- a/block/partitions/of.c
> +++ b/block/partitions/of.c
> @@ -15,6 +15,10 @@ static int validate_of_partition(struct device_node *np, int slot)
> int a_cells = of_n_addr_cells(np);
> int s_cells = of_n_size_cells(np);
>
> + /* Skip nodes without a reg property (e.g. nvmem-layout) */
> + if (!reg)
> + return 1;
> +
> /* Make sure reg len match the expected addr and size cells */
> if (len / sizeof(*reg) != a_cells + s_cells)
> return -EINVAL;
[Severity: High]
This is a pre-existing issue, but could malicious device tree properties
cause an integer underflow here?
The functions of_n_addr_cells() and of_n_size_cells() return signed integers.
If a device tree specifies a large unsigned value like 0xffffffff, these
functions will return -1.
The validation check above can be bypassed if the sum of negative and positive
cell counts equals the length ratio.
When the negative size is later passed to of_read_number(reg, a_cells) in
add_of_partition(), the internal while (size--) loop underflows, causing a
massive out-of-bounds read and panic.
[ ... ]
> @@ -97,9 +102,10 @@ int of_partition(struct parsed_partitions *state)
> break;
> }
>
> - add_of_partition(state, slot, np);
> -
> - slot++;
> + if (of_property_present(np, "reg")) {
> + add_of_partition(state, slot, np);
[Severity: High]
This isn't a bug introduced by this patch, but does add_of_partition() have a
NULL pointer dereference risk?
Inside add_of_partition(), partname is populated by checking for "label"
and falling back to "name". If a node omits both properties, of_get_property()
returns NULL:
block/partitions/of.c:add_of_partition() {
...
partname = of_get_property(np, "label", &len);
if (!partname)
partname = of_get_property(np, "name", &len);
strscpy(info->volname, partname, sizeof(info->volname));
...
}
Will passing a NULL partname to strscpy() cause a kernel panic?
> + slot++;
> + }
> }
>
> seq_buf_puts(&state->pp_buf, "\n");
[Severity: High]
This is a pre-existing issue, but does this function leak the partitions_np
device node reference?
A reference is acquired via of_node_get(ddev->of_node) at the start of
of_partition().
If the node is not compatible with "fixed-partitions", the function returns 0
early without calling of_node_put(partitions_np).
Similarly, on this successful parsing path, it returns 1 without releasing
the reference.
Could this lead to memory exhaustion or block the release of dynamically
allocated device nodes every time partitions are rescanned?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-block-as-nvmem-v5-0-95e0b30fff90@oss.qualcomm.com?part=1
^ permalink raw reply
* Re: [PATCH v5 9/9] arm64: dts: qcom: arduino-imola: Describe NVMEM layout for WiFi/BT addresses
From: sashiko-bot @ 2026-06-13 13:21 UTC (permalink / raw)
To: Loic Poulain; +Cc: devicetree, robh, conor+dt
In-Reply-To: <20260612-block-as-nvmem-v5-9-95e0b30fff90@oss.qualcomm.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Confused Deputy / Use-After-Free via dev_t reuse in block NVMEM provider.
--
commit 459b659c960efdc8a9e2177c5bc4f832e7ee5730
Author: Daniel Golle <daniel@makrotopia.org>
block: implement NVMEM provider
On embedded devices using an eMMC it is common that one or more partitions
on the eMMC are used to store MAC addresses and Wi-Fi calibration EEPROM
data. Allow referencing the partition in device tree for the kernel and
Wi-Fi drivers accessing it via the NVMEM layer. For now, NVMEM is only
registered for the whole disk block device.
> diff --git a/block/blk-nvmem.c b/block/blk-nvmem.c
> new file mode 100644
> index 0000000000000..c005f059d9fe5
> --- /dev/null
> +++ b/block/blk-nvmem.c
[ ... ]
> +static int blk_nvmem_reg_read(void *priv, unsigned int from, void *val, size_t bytes)
> +{
> + blk_mode_t mode = BLK_OPEN_READ | BLK_OPEN_RESTRICT_WRITES;
> + dev_t devt = (dev_t)(uintptr_t)priv;
> + size_t bytes_left = bytes;
> + loff_t pos = from;
> + int ret = 0;
> +
> + struct file *bdev_file __free(fput) = bdev_file_open_by_dev(devt, mode, priv, NULL);
[Severity: High]
Does this code allow for a cross-device data leak if the block device is
removed and its dev_t is reassigned?
If an NVMEM consumer (such as a WiFi driver or a process holding an open
sysfs file) retains an active reference to the NVMEM cell, the nvmem_device
remains alive even after blk_nvmem_del() calls nvmem_unregister() because
nvmem_unregister() only decrements the registration reference count.
If the block layer then frees the removed device's dev_t, it could be
immediately reassigned to a newly plugged block device (e.g. a USB stick).
If the consumer later reads the NVMEM cell, wouldn't bdev_file_open_by_dev()
successfully open the new device using the reused dev_t, allowing the
consumer to unknowingly read data from the unrelated device?
> + if (IS_ERR(bdev_file))
> + return PTR_ERR(bdev_file);
> +
[ ... ]
> +void blk_nvmem_add(struct block_device *bdev)
> +{
> + struct device *dev = &bdev->bd_device;
> + struct nvmem_config config = {};
[ ... ]
> + config.id = NVMEM_DEVID_NONE;
> + config.dev = dev;
> + config.name = dev_name(dev);
> + config.owner = THIS_MODULE;
> + config.priv = (void *)(uintptr_t)dev->devt;
[Severity: High]
Is it safe to store the dev_t here as the priv pointer?
Since the dev_t identifier can be reassigned by the block layer once the
device is removed, storing it this way might lead to the isolation issue
mentioned above if an NVMEM consumer holds a reference past the device's
removal.
> + config.reg_read = blk_nvmem_reg_read;
> + config.size = bdev_nr_bytes(bdev);
> + config.word_size = 1;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-block-as-nvmem-v5-0-95e0b30fff90@oss.qualcomm.com?part=9
^ permalink raw reply
* Re: [PATCH 4/4] arm64: dts: allwinner: add Radxa Cubie A7S
From: Enzo @ 2026-06-13 13:29 UTC (permalink / raw)
To: Jernej Škrabec
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Chen-Yu Tsai,
Samuel Holland, Maxime Ripard, Ulf Hansson, devicetree,
linux-arm-kernel, linux-sunxi, linux-kernel, linux-mmc
In-Reply-To: <yDDIdJcZRCmL2YwI2Wv_ng@gmail.com>
Hi Jernej,
Thanks for taking a look.
> Besides sashiko bot comments, these pins should go to main A733 DTSI,
> like it's done for other SoCs.
>
> In any case, it's a bit early for DT. At least clocks should land before.
Agreed on both points. I'll move the UART0 pin definition into the main
A733 DTSI for the next revision, following the other Allwinner SoCs.
I will also hold off on sending a v2 until the A733 clock prerequisites
are in a better state, unless maintainers prefer a different ordering.
For now I'll keep this series as a checkpoint and continue tracking/testing
the RTC, clock and pinctrl prerequisite work.
Best regards,
Enzo
On Sat, Jun 13, 2026 at 7:37 AM Jernej Škrabec <jernej.skrabec@gmail.com> wrote:
>
> Dne sobota, 13. junij 2026 ob 11:42:16 Srednjeevropski poletni čas je Enzo Adriano via B4 Relay napisal(a):
> > From: Enzo Adriano <enzo.adriano.code@gmail.com>
> >
> > Add the Radxa Cubie A7S board description with serial console and SD card
> > boot support.
> >
> > Ethernet remains disabled until the GMAC210 wrapper, clocks, resets,
> > MDIO, PHY reset, PHY power, and link behavior are proven.
> >
> > Signed-off-by: Enzo Adriano <enzo.adriano.code@gmail.com>
> > ---
> > arch/arm64/boot/dts/allwinner/Makefile | 1 +
> > .../boot/dts/allwinner/sun60i-a733-cubie-a7s.dts | 48 ++++++++++++++++++++++
> > 2 files changed, 49 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/allwinner/Makefile b/arch/arm64/boot/dts/allwinner/Makefile
> > index d116864b6c2b..824cc35152db 100644
> > --- a/arch/arm64/boot/dts/allwinner/Makefile
> > +++ b/arch/arm64/boot/dts/allwinner/Makefile
> > @@ -56,6 +56,7 @@ dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-2024.dtb
> > dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-h.dtb
> > dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-plus.dtb
> > dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-sp.dtb
> > +dtb-$(CONFIG_ARCH_SUNXI) += sun60i-a733-cubie-a7s.dtb
> > dtb-$(CONFIG_ARCH_SUNXI) += sun55i-a527-cubie-a5e.dtb
> > dtb-$(CONFIG_ARCH_SUNXI) += sun55i-h728-x96qpro+.dtb
> > dtb-$(CONFIG_ARCH_SUNXI) += sun55i-t527-avaota-a1.dtb
> > diff --git a/arch/arm64/boot/dts/allwinner/sun60i-a733-cubie-a7s.dts b/arch/arm64/boot/dts/allwinner/sun60i-a733-cubie-a7s.dts
> > new file mode 100644
> > index 000000000000..453761a96323
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/allwinner/sun60i-a733-cubie-a7s.dts
> > @@ -0,0 +1,48 @@
> > +// SPDX-License-Identifier: (GPL-2.0-only OR MIT)
> > +
> > +/dts-v1/;
> > +
> > +#include "sun60i-a733.dtsi"
> > +
> > +/ {
> > + model = "Radxa Cubie A7S";
> > + compatible = "radxa,cubie-a7s", "allwinner,sun60i-a733";
> > +
> > + aliases {
> > + serial0 = &uart0;
> > + mmc0 = &mmc0;
> > + };
> > +
> > + chosen {
> > + stdout-path = "serial0:115200n8";
> > + };
> > +
> > + reg_vcc3v3: vcc3v3 {
> > + compatible = "regulator-fixed";
> > + regulator-name = "vcc-3v3";
> > + regulator-min-microvolt = <3300000>;
> > + regulator-max-microvolt = <3300000>;
> > + regulator-always-on;
> > + };
> > +};
> > +
> > +&mmc0 {
> > + vmmc-supply = <®_vcc3v3>;
> > + bus-width = <4>;
> > + no-mmc;
> > + no-sdio;
> > + status = "okay";
> > +};
> > +
> > +&pio {
> > + uart0_pb9_pb10_pins: uart0-pb9-pb10-pins {
> > + pins = "PB9", "PB10";
> > + function = "uart0";
> > + };
>
> Besides sashiko bot comments, these pins should go to main A733 DTSI,
> like it's done for other SoCs.
>
> In any case, it's a bit early for DT. At least clocks should land before.
>
> Best regards,
> Jernej
>
> > +};
> > +
> > +&uart0 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&uart0_pb9_pb10_pins>;
> > + status = "okay";
> > +};
> >
> >
>
>
>
>
^ permalink raw reply
* Re: [PATCH net-next v2 2/2] net: pse-pd: add Realtek/Broadcom PSE MCU driver
From: sashiko-bot @ 2026-06-13 13:30 UTC (permalink / raw)
To: Jonas Jelonek; +Cc: devicetree, conor+dt, robh
In-Reply-To: <20260612132944.460646-3-jelonek.jonas@gmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The PoE power supply regulator is enabled after the PSE system is globally enabled, which can trigger hardware faults.
- [Medium] A race condition in the UART transport between `rtpse_uart_send()` and `rtpse_uart_receive()` can cause spurious transaction failures (-EIO) for new commands.
--
commit ee055c1d6f6679a77708ffd42882e725da96d11d
Author: Jonas Jelonek <jelonek.jonas@gmail.com>
net: pse-pd: add Realtek/Broadcom PSE MCU driver
A range of PoE switches use a small microcontroller on the PCB to front
the actual PSE silicon. The host CPU talks to that MCU over I2C/SMBus or
UART using a fixed 12-byte request/response protocol.
> diff --git a/drivers/net/pse-pd/realtek-pse-core.c b/drivers/net/pse-pd/realtek-pse-core.c
> --- /dev/null
> +++ b/drivers/net/pse-pd/realtek-pse-core.c
[ ... ]
> +int rtpse_register(struct rtpse_ctrl *pse)
> +{
[ ... ]
> + if (!info.system_enable) {
> + ret = rtpse_set_global_state(pse, true);
> + /* Dialects without a global-state concept (e.g. BCM) return
> + * -EOPNOTSUPP; treat that as "no separate enable required".
> + */
> + if (ret && ret != -EOPNOTSUPP)
> + return dev_err_probe(pse->dev, ret,
> + "failed to enable PSE system\n");
> + }
> +
> + ret = regulator_enable(pse->poe_supply);
[Severity: High]
Does this enable sequence trigger hardware faults? If rtpse_set_global_state()
is called before regulator_enable(), the PSE chips might actively sense and
deliver power before their main power rail is stable. This could cause the
hardware to detect an Under-Voltage Lockout (UVLO) fault.
Should the poe_supply be enabled and allowed to stabilize before globally
enabling the system?
> diff --git a/drivers/net/pse-pd/realtek-pse-uart.c b/drivers/net/pse-pd/realtek-pse-uart.c
> --- /dev/null
> +++ b/drivers/net/pse-pd/realtek-pse-uart.c
[ ... ]
> +static size_t rtpse_uart_receive(struct serdev_device *serdev,
> + const u8 *buf, size_t count)
> +{
[ ... ]
> + scoped_guard(spinlock_irqsave, &ctx->rx_lock) {
> + take = min(count, sizeof(ctx->rx_buf) - ctx->rx_len);
> + if (take) {
> + memcpy(ctx->rx_buf + ctx->rx_len, buf, take);
> + ctx->rx_len += take;
> + done = (ctx->rx_len == sizeof(ctx->rx_buf));
> + }
> + }
> + if (done)
> + complete(&ctx->rx_done);
> +
> + /* consume all to avoid desync/misalignment */
> + return count;
> +}
[ ... ]
> +static int rtpse_uart_send(struct rtpse_ctrl *pse, const struct rtpse_mcu_msg *req)
> +{
> + struct rtpse_uart *ctx = to_rtpse_uart(pse);
> + int written;
> +
> + /* clear any leftover rx state before transmitting */
> + reinit_completion(&ctx->rx_done);
> + scoped_guard(spinlock_irqsave, &ctx->rx_lock)
> + ctx->rx_len = 0;
[Severity: Medium]
Can this sequence cause spurious transaction failures for new commands?
In rtpse_uart_send(), reinit_completion() is called outside the rx_lock
before clearing rx_len. Concurrently, in rtpse_uart_receive(), complete()
is also called outside the spinlock.
If a delayed UART RX interrupt from a previous timed-out transaction fires
just as a new transaction starts, could the interrupt handler fill rx_buf
and call complete() after the new send reinitializes the completion, but
before it clears rx_len?
This might cause the new transaction to return immediately from
wait_for_completion_timeout() and falsely fail with -EIO due to rx_len
being evaluated as incomplete.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612132944.460646-1-jelonek.jonas@gmail.com?part=2
^ permalink raw reply
* Re: [PATCH 2/5] iio: adc: Add ti-ads1262 driver
From: Jonathan Cameron @ 2026-06-13 13:45 UTC (permalink / raw)
To: Kurt Borja
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Linus Walleij,
Bartosz Golaszewski, David Lechner, Nuno Sá, Andy Shevchenko,
linux-iio, devicetree, linux-kernel, linux-gpio
In-Reply-To: <20260612-ads126x-v1-2-894c788d03ed@gmail.com>
On Fri, 12 Jun 2026 17:46:20 -0500
Kurt Borja <kuurtb@gmail.com> wrote:
> Add ti-ads1262 driver for TI ADS1262 and ADS1263 ADCs with initial
> support for the following features:
>
> - Power management
> - IIO direct and buffer modes
> - Channel hot-reloading
> - Internal or external oscillator
> - Internal or external voltage reference
> - Filter configuration
> - Sensor bias configuration
> - IDAC configuration
> - Level-shift voltage configuration
> - Auxiliary ADC interoperability considerations
>
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> ---
> MAINTAINERS | 1 +
> drivers/iio/adc/Kconfig | 13 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/ti-ads1262.c | 1816 ++++++++++++++++++++++++++++++++++++++++++
That is rather too big. I think you'll have to work out how to split this
up into more manageable chunks. Staying under a 1000 (preferably a lot less)
per patch makes it much easier for people to review.
Given the complexity of the device this might be one that has to go
in as several series, building up functionality as we go.
I'll ignore all the DT stuff as sounds like that may radically change and
just take a fairly superficial first look at this.
Jonathan
> diff --git a/drivers/iio/adc/ti-ads1262.c b/drivers/iio/adc/ti-ads1262.c
> new file mode 100644
> index 000000000000..fd1911cf65ac
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads1262.c
> @@ -0,0 +1,1816 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Texas Instruments ADS1262 ADC driver
> + *
> + * Copyright (C) 2025 Kurt Borja <kuurtb@gmail.com>
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/align.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitmap.h>
> +#include <linux/bitops.h>
> +#include <linux/cleanup.h>
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/compiler_attributes.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/lockdep.h>
Fairly unusual to see that header in a driver.
What's it here for?
> +#include <linux/math.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/mutex.h>
> +#include <linux/overflow.h>
> +#include <linux/property.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/time.h>
> +#include <linux/types.h>
> +#include <linux/units.h>
> +/* IDACMAG constants */
> +#define ADS1262_IDACMAG_OFF 0
> +#define ADS1262_IDACMAG_COUNT 11
> +
> +/* REFMUX constants */
Naming is good enough I'm not sure I'd bother with the comments
to say what these are.
On option is to just group them with the register they are about
and using extra indenting to visually separate them from the register
#define ADS1262_REFMUX_REG 0xxx
#define ADS1262_REFMUX_RMUXP_MASK GENMASK(5, 3)
#define ADS1262_RMUX_INTER 0
#define ADS1262_RMUX_AIN0_AIN1 1
#define ADS1262_RMUX_AIN2_AIN3 2
#define ADS1262_RMUX_AIN4_AIN5 3
#define ADS1262_RMUX_AVDD_AVSS 4
#define ADS1262_RMUX_COUNT 5
However, if you are going to have a terminating entry, an anonymous enum might be better
with that just as the last item.
#define ADS1262_REFMUX_RMUXN_MASK GENMASK(2, 0)
> +#define ADS1262_RMUX_INTER 0
> +#define ADS1262_RMUX_AIN0_AIN1 1
> +#define ADS1262_RMUX_AIN2_AIN3 2
> +#define ADS1262_RMUX_AIN4_AIN5 3
> +#define ADS1262_RMUX_AVDD_AVSS 4
> +#define ADS1262_RMUX_COUNT 5
> +
> +struct ads1262_channel {
As a general rule we tend to avoid bitfields because of all the problems
with how loose the C spec is on how these actually get laid out.
I'd just have this as a suitable 32 bit value and then have
defines for masks within that.
> + /* MODE0 */
> + u8 conv_delay:4;
> + u8 chop_mode:1;
> + u8 idac_rot_mode:1;
> + u8 runmode:1;
> + u8 rev_vref_pol:1;
> +
> + /* MODE1 */
> + u8 sbias_magnitude:3;
> + u8 sbias_polarity:1;
> + u8 sbias_connection:1;
> + u8 filter:3;
> +
> + /* MODE2 */
> + u8 data_rate:4;
> + u8 gain:3;
> + u8 pga_bypass:1;
> +
> + /* INPMUX */
> + u8 negative_input:4;
> + u8 positive_input:4;
> +};
> +
> +struct ads1262 {
> + struct spi_device *spi;
> + struct regmap *regmap;
> + struct iio_dev *indio_dev;
> + struct iio_trigger *trig;
> + struct gpio_desc *reset_gpiod;
> + struct gpio_desc *start_gpiod;
> +
> + void *scan_buffer;
I think this is always accessed as a __be32. If so just type it as that.
> + size_t scan_sz;
> +
> + /* Protects channel state */
> + struct mutex chan_lock;
> + u32 vref_uV;
> + unsigned int num_channels;
> + struct ads1262_channel *channels;
> + struct completion drdy;
> + struct spi_message msg;
> + struct spi_transfer xfer;
> +
> + /* Protects transfer buffers and concurrent SPI transfers */
> + struct mutex xfer_lock;
> +
> + u8 tx[6] __aligned(IIO_DMA_MINALIGN);
> + union {
> + u8 rx[6];
> + struct {
> + __be32 data;
> + } __packed shift_reg;
> + struct {
> + u8 dummy;
> + __be32 data;
I guess Sashiko already moaned about unaligned accesses (I'm too lazy to check :)
You may have to always memcpy this to a local variable rather than accessing
it directly.
> + } __packed holding_reg;
> + };
> +};
> +
> +static int ads1262_dev_power_on(struct ads1262 *st)
> +{
> + int ret;
> +
> + ret = gpiod_set_value_cansleep(st->reset_gpiod, 0);
> + if (ret)
> + return ret;
> +
> + fsleep(9 * USEC_PER_MSEC);
> +
> + return 0;
> +}
> +
> +static int ads1262_dev_power_off(struct ads1262 *st)
> +{
> + int ret;
> +
> + ret = gpiod_set_value_cansleep(st->reset_gpiod, 1);
> + if (ret)
> + return ret;
> +
> + fsleep(9 * USEC_PER_MSEC);
> +
> + return 0;
> +}
Given how similar these two are (assuming they don't get more complex
later) perhaps just combine them into one function, or use a shared function
plus trivial wrappers.
> +
> +static int ads1262_dev_cmd(struct ads1262 *st, u8 opcode)
> +{
> + guard(mutex)(&st->xfer_lock);
> +
> + st->tx[0] = opcode;
> +
> + return spi_write(st->spi, &st->tx[0], sizeof(st->tx[0]));
maybe use
return spi_write_then_read(st->spi, opcode, sizeof(opcode), NULL, 0);
Might as well use the helpers that bounce buffer in cases like this rather
than rolling our own.
> +}
> +static int ads1262_channel_hot_reload(struct ads1262 *st,
> + const struct iio_chan_spec *chan)
> +{
> + unsigned int weight;
> + unsigned long i;
> + int ret;
> +
> + /*
> + * Hot reloading is only required on buffer mode and if only one channel
> + * is enabled.
> + */
> + if (!iio_device_try_claim_buffer_mode(st->indio_dev))
> + return 0;
Look at IIO_ACQUIRE_DIRECT_MODE()
That should let you simplify the error paths in here.
I see you used it elsewhere.
> +
> + weight = bitmap_weight(st->indio_dev->active_scan_mask,
> + iio_get_masklength(st->indio_dev));
> + if (weight != 1) {
> + iio_device_release_direct(st->indio_dev);
> + return 0;
> + }
> +
> + i = find_first_bit(st->indio_dev->active_scan_mask,
> + iio_get_masklength(st->indio_dev));
> + if (i != chan->scan_index) {
> + iio_device_release_direct(st->indio_dev);
> + return 0;
> + }
> +
> + /*
> + * The device automatically hot reloads the channel after writing to
> + * the configuration registers.
> + */
> + ret = ads1262_channel_enable(st, &st->channels[chan->scan_index]);
> +
> + iio_device_release_direct(st->indio_dev);
> +
> + return ret;
> +}
> +
> +static int ads1262_channel_read(struct ads1262 *st,
> + struct ads1262_channel *chan_data,
> + __be32 *val)
Trivial: Probably move val to line above.
> +{
> + struct device *dev = &st->spi->dev;
> + u8 runmode;
> + int ret;
> +
> + PM_RUNTIME_ACQUIRE_AUTOSUSPEND(dev, pm);
> + if (PM_RUNTIME_ACQUIRE_ERR(&pm))
> + return -ENXIO;
> +
> + IIO_DEV_ACQUIRE_DIRECT_MODE(st->indio_dev, claim);
> + if (IIO_DEV_ACQUIRE_FAILED(claim))
> + return -EBUSY;
> +
> + /*
> + * When a channel has chop mode or IDAC rotation mode, the first
> + * conversion is always withheld so the datasheet suggests using the
> + * CONTINUOUS mode and briefly starting and stopping conversions to
> + * achieve the same effect (Section 9.4.1.2).
> + */
> + if (chan_data->chop_mode || chan_data->idac_rot_mode)
> + runmode = ADS1262_RUNMODE_CONTINUOUS;
> + else
> + runmode = ADS1262_RUNMODE_PULSE;
> +
> + ads1262_channel_set_runmode(st, chan_data, runmode);
> +
> + ret = ads1262_channel_enable(st, chan_data);
> + if (ret)
> + return ret;
> +
> + ret = ads1262_dev_start_one(st, runmode);
> + if (ret)
> + return ret;
> +
> + ads1262_wait_for_conversion(st);
> +
> + return ads1262_dev_read_data_command(st, ADS1262_OPCODE_RDATA1, val);
> +}
> +
> +static int ads1262_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val,
> + int *val2, long mask)
> +{
> + struct ads1262 *st = iio_priv(indio_dev);
> + struct ads1262_channel *chan_data;
> + u8 mode, realbits;
> + __be32 raw;
> + u32 cnv;
> + int ret;
> +
> + chan_data = &st->channels[chan->scan_index];
> + realbits = chan->scan_type.realbits;
I'd be tempted to do these two as part of declarations.
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = ads1262_channel_read(st, chan_data, &raw);
> + if (ret)
> + return ret;
> +
> + cnv = be32_to_cpu(raw);
> + *val = sign_extend32(cnv, realbits - 1);
Might as well combine these lines and avoid need for local cnv.
*VAL = sign_extend32(be32_to_cpu(raw), realbits - 1);
> +
> + return IIO_VAL_INT;
> +
> + case IIO_CHAN_INFO_SCALE:
> + u64 divd, divr, tmp, rem;
> +
> + mutex_lock(&st->chan_lock);
> + divd = st->vref_uV;
> + divr = BIT_ULL(chan_data->gain + realbits - 1) * 1000;
> + mutex_unlock(&st->chan_lock);
> +
> + tmp = div64_u64(divd * 1000000000ULL, divr);
> + *val = div64_u64_rem(tmp, 1000000000ULL, &rem);
Use the NANO define probably to avoid need to zero count when reviewing.
> + *val2 = rem;
> +
> + return IIO_VAL_INT_PLUS_NANO;
> +
> +static int ads1262_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> + struct ads1262 *st = iio_priv(indio_dev);
> + struct device *dev = &st->spi->dev;
> +
> + ads1262_dev_stop(st);
> + pm_runtime_put_autosuspend(dev);
> + spi_unoptimize_message(&st->msg);
> + kfree(st->scan_buffer);
> + st->scan_buffer = NULL;
> + st->scan_sz = 0;
Are either scan_buffer or scan_sz ever checked? If not I would not
bother clearing them as their state doesn't matter when we aren't
in buffered mode. Maybe there is a small argument that clearing them
might help with debug but I don't see that as a worth the implication that
something might be gated on these.
> +
> + return 0;
> +}
> +
> +static bool ads1262_validate_scan_mask(struct iio_dev *indio_dev,
> + const unsigned long *scan_mask)
> +{
> + struct ads1262 *st = iio_priv(indio_dev);
> + struct device *dev = &st->spi->dev;
> +
> + if (iio_trigger_using_own(indio_dev)) {
> + dev_err(dev, "The %s trigger only supports one active channel\n",
> + st->trig->name);
Given this can be trivially triggered from userspace - if you want to have
a print here, use a rate limited version.
> + return iio_validate_scan_mask_onehot(indio_dev, scan_mask);
> + }
> +
> + return true;
> +}
> +
> +static int ads1262_fill_buffer_mult(struct ads1262 *st)
> +{
> + __be32 val, *scan_buffer = st->scan_buffer;
Avoid mixing pointer and no point, or anything with assignments
as it makes the code harder to read.
> + unsigned int chan;
> + int i = -1;
> + int ret;
> +
> + /*
> + * This routine enables and reads channels in a full-duplex fashion.
> + *
> + * When a channel is enabled, the previous conversion is clocked out of
> + * the shift data register on the same transfer (Section 9.4.7.1). This
> + * allows for low latency software sequencing but forbids any SPI
> + * activity happen in between or data corruption may occur, hence the
> + * need to take the xfer_lock for the whole operation.
> + */
Just to check: Is SPI traffic on the same bus to a different device fine?
If not you'd need spi_bus_lock(). If it is fine then reword this to talk about
communications with this device just to avoid confusion.
> +
> + guard(mutex)(&st->xfer_lock);
> +
> + iio_for_each_active_channel(st->indio_dev, chan) {
> + ret = ads1262_channel_enable_and_read(st, &st->channels[chan],
> + &val);
It's fine to go a little past 80 chars on a line if it helps readabiilty.
The harder limit is 100 chars though.
> + if (ret)
> + return ret;
> +
> + if (i > -1)
> + scan_buffer[i] = val;
> + i++;
> +
> + ads1262_wait_for_conversion(st);
> + }
> +
> + return ads1262_dev_read_data_direct(st, &scan_buffer[i]);
> +}
> +static int ads1262_read_chip_name(struct ads1262 *st, char **name)
> +{
> + struct device *dev = &st->spi->dev;
> + u8 dev_id;
> + unsigned int val;
> + int ret;
> +
> + ret = regmap_read(st->regmap, ADS1262_ID_REG, &val);
> + if (ret)
> + return ret;
> +
> + dev_id = FIELD_GET(ADS1262_DEV_ID_MASK, val);
> +
> + switch (dev_id) {
> + case ADS1262_DEV_ID_ADS1262:
> + *name = "ads1262";
Given, at somepoint I would guess you'll want to support the auxiliary adc
on the 1263, I'd start with a struct chip_info (with the name in there)
and pick that rather than just the name here.
> + break;
> + case ADS1262_DEV_ID_ADS1263:
Not particularly important but common practice to just change the prefix
for anything device specific.
case ADS1263_DEV_ID
> + *name = "ads1263";
> + break;
> + default:
> + *name = "ads1262";
Given we'll ultimately want fallback compatibles to work and so allow
for firmware to specify which device to fallback to, this should really be
using the guidance from firmware to select rather than always guessing
the 1262 variant. That is safe though given the 'subset' nature so this
doesn't matter as much as it normally does.
> + dev_dbg(dev, "Failed to identify device with ID 0x%x\n", val);
> + }
> +
> + return 0;
> +}
>
> +
> +static int ads1262_dev_configure(struct ads1262 *st)
> +{
> + struct device *dev = &st->spi->dev;
> + u32 idac_pins[2], idac_mags[2];
> + u8 val;
> + int ret;
> +
> + ret = ads1262_dev_reset(st);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to reset device\n");
> +
> + ret = regmap_update_bits(st->regmap, ADS1262_INTERFACE_REG,
> + ADS1262_INTERFACE_STATUS_MASK |
> + ADS1262_INTERFACE_CRC_MASK, 0);
regmap_clear_bits()
> + if (ret)
> + return ret;
> +
> + if (device_property_present(dev, "ti,vbias")) {
> + ret = regmap_update_bits(st->regmap, ADS1262_POWER_REG,
> + ADS1262_POWER_VBIAS_MASK,
> + ADS1262_POWER_VBIAS_MASK);
regmap_set_bits()
> + if (ret)
> + return ret;
> + }
> +
> +static int ads1262_gpio_setup(struct ads1262 *st)
> +{
> + struct device *dev = &st->spi->dev;
> + struct gpio_desc *gpiod;
> + const char *con_id;
> +
> + con_id = "start";
> + gpiod = devm_gpiod_get_optional(dev, con_id, GPIOD_OUT_LOW);
> + if (IS_ERR(gpiod))
> + return dev_err_probe(dev, PTR_ERR(gpiod),
> + "Failed to get %s GPIO\n", con_id);
Whilst not duplicating the string con_id is nice, it does break grepping for error
messages. So I'd just duplicate it.
> + st->start_gpiod = gpiod;
> +
> + con_id = "reset";
> + gpiod = devm_gpiod_get_optional(dev, con_id, GPIOD_OUT_HIGH);
> + if (IS_ERR(gpiod))
> + return dev_err_probe(dev, PTR_ERR(gpiod),
> + "Failed to get %s GPIO\n", con_id);
> + st->reset_gpiod = gpiod;
> +
> + return 0;
> +}
> +
> +static int ads1262_regulator_setup(struct ads1262 *st)
> +{
> + struct device *dev = &st->spi->dev;
> + const char *reg_id, *prop;
> + u32 mux[2] = {};
> + int val, ret;
> +
> + reg_id = "dvdd";
> + ret = devm_regulator_get_enable(dev, reg_id);
> + if (ret)
> + goto err_regulator_get;
> +
> + reg_id = "avdd";
> + ret = devm_regulator_get_enable(dev, reg_id);
> + if (ret)
> + goto err_regulator_get;
> +
> + prop = "ti,neg-refmux";
> + device_property_read_u32(dev, prop, &mux[0]);
> + if (mux[0] >= ADS1262_RMUX_COUNT)
> + return dev_err_probe(dev, -ENXIO, " %s out of range\n", prop);
> +
> + prop = "ti,pos-refmux";
> + device_property_read_u32(dev, prop, &mux[1]);
> + if (mux[1] >= ADS1262_RMUX_COUNT)
> + return dev_err_probe(dev, -ENXIO, " %s out of range\n", prop);
> +
> + if (mux[0] == ADS1262_RMUX_INTER && mux[1] == ADS1262_RMUX_INTER) {
> + /* The internal voltage reference is 2.5 V */
> + st->vref_uV = 2500000;
> + return 0;
> + }
> +
> + val = FIELD_PREP(ADS1262_REFMUX_RMUXN_MASK, mux[0]);
> + val |= FIELD_PREP(ADS1262_REFMUX_RMUXP_MASK, mux[1]);
> + ret = regmap_update_bits(st->regmap, ADS1262_REFMUX_REG,
> + ADS1262_REFMUX_RMUXN_MASK |
> + ADS1262_REFMUX_RMUXP_MASK, val);
> + if (ret)
> + return ret;
> +
> + reg_id = "vref";
> + st->vref_uV = devm_regulator_get_enable_read_voltage(dev, reg_id);
> + if (st->vref_uV < 0)
> + goto err_regulator_get;
> +
> + return 0;
> +
> +err_regulator_get:
> + return dev_err_probe(dev, ret, "Failed to get regulator %s\n", reg_id);
I'd be tempted to just duplicate the print in the interests of simpler code flow.
Would avoid the need for a local variable to stash 'last one I tried'.
> +}
> +
> +static const struct reg_default ads1262_reg_defaults[] = {
> + { ADS1262_POWER_REG, 0x11 },
Is it sensible to specify these in terms of the fields that make them up?
Can make it easier to see what the default state actually means.
Sometimes it is just too complex, so we don't bother.
> + { ADS1262_INTERFACE_REG, 0x05 },
> + { ADS1262_MODE0_REG, 0x00 },
> + { ADS1262_MODE1_REG, 0x80 },
> + { ADS1262_MODE2_REG, 0x04 },
> + { ADS1262_INPMUX_REG, 0x01 },
> + { ADS1262_OFCAL0_REG, 0x00 },
> + { ADS1262_OFCAL1_REG, 0x00 },
> + { ADS1262_OFCAL2_REG, 0x00 },
> + { ADS1262_FSCAL0_REG, 0x00 },
> + { ADS1262_FSCAL1_REG, 0x00 },
> + { ADS1262_FSCAL2_REG, 0x40 },
> + { ADS1262_IDACMUX_REG, 0xBB },
> + { ADS1262_IDACMAG_REG, 0x00 },
> + { ADS1262_REFMUX_REG, 0x00 },
> + { ADS1262_TDACP_REG, 0x00 },
> + { ADS1262_TDACN_REG, 0x00 },
> + { ADS1262_GPIOCON_REG, 0x00 },
> + { ADS1262_GPIODIR_REG, 0x00 },
> + { ADS1262_GPIODAT_REG, 0x00 },
> + { ADS1262_ADC2CFG_REG, 0x00 },
> + { ADS1262_ADC2MUX_REG, 0x01 },
> + { ADS1262_ADC2OFC0_REG, 0x00 },
> + { ADS1262_ADC2OFC1_REG, 0x00 },
> + { ADS1262_ADC2FSC0_REG, 0x00 },
> + { ADS1262_ADC2FSC1_REG, 0x40 },
> +};
> +static int ads1262_runtime_suspend(struct device *dev)
> +{
> + struct ads1262 *st = dev_get_drvdata(dev);
> +
> + if (!st->reset_gpiod)
> + return 0;
> +
> + regcache_cache_only(st->regmap, true);
> +
> + return ads1262_dev_power_off(st);
> +}
> +
> +static int ads1262_runtime_resume(struct device *dev)
> +{
> + struct ads1262 *st = dev_get_drvdata(dev);
> + int ret;
> +
> + if (!st->reset_gpiod)
> + return 0;
> +
> + ret = ads1262_dev_power_on(st);
> + if (ret)
> + return ret;
> +
> + regcache_cache_only(st->regmap, false);
> + regcache_mark_dirty(st->regmap);
> +
> + return regcache_sync(st->regmap);
> +}
> +
> +DEFINE_RUNTIME_DEV_PM_OPS(ads1262_runtime_pm, ads1262_runtime_suspend,
> + ads1262_runtime_resume, NULL);
> +
> +static const struct of_device_id ads1262_of_match[] = {
> + { .compatible = "ti,ads1262" },
> + { .compatible = "ti,ads1263" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, ads1262_of_match);
> +
> +static const struct spi_device_id ads1262_spi_match[] = {
> + { "ads1262" },
> + { "ads1263" },
Named initializers please. Uwe is driving through updating
all the existing drivers to use them (various reasons to do that
but simplest one is it's inconsistent with what we have always done
for of_device_id!)
> + { }
> +};
> +MODULE_DEVICE_TABLE(spi, ads1262_spi_match);
> +
> +static struct spi_driver ads1262_spi_driver = {
> + .driver = {
> + .name = "ads1262",
> + .of_match_table = ads1262_of_match,
> + .pm = pm_ptr(&ads1262_runtime_pm),
> + },
> + .probe = ads1262_spi_probe,
> + .id_table = ads1262_spi_match,
> +};
> +module_spi_driver(ads1262_spi_driver);
> +
> +MODULE_DESCRIPTION("Texas Instruments ADS1262 ADC driver");
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Kurt Borja <kuurtb@gmail.com>");
>
^ permalink raw reply
* Re: [PATCH 4/5] iio: adc: ti-ads1262: Add calibration support
From: Jonathan Cameron @ 2026-06-13 13:50 UTC (permalink / raw)
To: Kurt Borja
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Linus Walleij,
Bartosz Golaszewski, David Lechner, Nuno Sá, Andy Shevchenko,
linux-iio, devicetree, linux-kernel, linux-gpio
In-Reply-To: <20260612-ads126x-v1-4-894c788d03ed@gmail.com>
On Fri, 12 Jun 2026 17:46:22 -0500
Kurt Borja <kuurtb@gmail.com> wrote:
> Add channel calibration support.
>
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> ---
> drivers/iio/adc/ti-ads1262.c | 70 +++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 69 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/adc/ti-ads1262.c b/drivers/iio/adc/ti-ads1262.c
> index 6d5f22836ad8..b33505e7fdc7 100644
> --- a/drivers/iio/adc/ti-ads1262.c
> +++ b/drivers/iio/adc/ti-ads1262.c
> @@ -217,6 +217,10 @@
> #define ADS1262_RMUX_AVDD_AVSS 4
> #define ADS1262_RMUX_COUNT 5
>
> +/* The calibration word is signed 24 bits value */
> +#define ADS1262_CALIB_WORD_MAX ((int)(GENMASK(22, 0)))
> +#define ADS1262_CALIB_WORD_MIN (-ADS1262_CALIB_WORD_MAX - 1)
> +
> struct ads1262_channel {
> /* MODE0 */
> u8 conv_delay:4;
> @@ -453,6 +457,32 @@ static int ads1262_dev_start_one(struct ads1262 *st, u8 runmode)
> return 0;
> }
>
> +static int ads1262_read_calib(struct ads1262 *st, unsigned int reg, u32 *val)
> +{
> + __le32 lval;
> + int ret;
> +
> + /*
> + * The calibration word is a signed 24 bit LSB-first value.
Single line comment. However, it's also fairly clear from the code, so maybe
no comment at all.
> + */
> + ret = regmap_bulk_read(st->regmap, reg, &lval, 3);
Read it into a u8 [3] and use get_unaligned_le24()
That avoids us having to think too much about the bits that aren't
initialized and static analysis / compilers having to figure out
they don't matter. I general it is easier to understand.
> + if (ret)
> + return ret;
> + *val = sign_extend32(le32_to_cpu(lval), 23);
> +
> + return 0;
> +}
> +
> +static int ads1262_write_calib(struct ads1262 *st, unsigned int reg, u32 val)
> +{
> + __le32 lval = cpu_to_le32(val);
> +
> + /*
> + * The calibration word is a signed 24 bit LSB-first value.
> + */
> + return regmap_bulk_write(st->regmap, reg, &lval, 3);
Similar with a u8 [3] for the '__le24' storage.
> +}
> +
^ permalink raw reply
* Re: [PATCH 2/5] iio: adc: Add ti-ads1262 driver
From: Jonathan Cameron @ 2026-06-13 14:06 UTC (permalink / raw)
To: Kurt Borja
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Linus Walleij,
Bartosz Golaszewski, David Lechner, Nuno Sá, Andy Shevchenko,
linux-iio, devicetree, linux-kernel, linux-gpio
In-Reply-To: <20260613144544.0594a7f0@jic23-huawei>
>
> > +static int ads1262_channel_hot_reload(struct ads1262 *st,
> > + const struct iio_chan_spec *chan)
> > +{
> > + unsigned int weight;
> > + unsigned long i;
> > + int ret;
> > +
> > + /*
> > + * Hot reloading is only required on buffer mode and if only one channel
> > + * is enabled.
> > + */
> > + if (!iio_device_try_claim_buffer_mode(st->indio_dev))
> > + return 0;
>
> Look at IIO_ACQUIRE_DIRECT_MODE()
obviously that comment is garbage - so please ignore!
>
> That should let you simplify the error paths in here.
> I see you used it elsewhere.
^ permalink raw reply
* Re: [PATCH 5/5] iio: adc: Add ti-ads1263-adc2 driver
From: Jonathan Cameron @ 2026-06-13 14:10 UTC (permalink / raw)
To: Kurt Borja
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Linus Walleij,
Bartosz Golaszewski, David Lechner, Nuno Sá, Andy Shevchenko,
linux-iio, devicetree, linux-kernel, linux-gpio
In-Reply-To: <20260612-ads126x-v1-5-894c788d03ed@gmail.com>
On Fri, 12 Jun 2026 17:46:23 -0500
Kurt Borja <kuurtb@gmail.com> wrote:
> The TI ADS1263 includes an auxiliary, 24-bit, delta-sigma ADC (ADC2).
> ADC2 operation is independent of ADC1, with independent selections of
> input channel, reference voltage, sample rate, and channel gain
>
> Add support for this ADC as an independent IIO device, through the
> auxiliary bus API.
A few things inline.
>
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> diff --git a/drivers/iio/adc/ti-ads1262.c b/drivers/iio/adc/ti-ads1262.c
> index b33505e7fdc7..1a4b2f934d43 100644
> --- a/drivers/iio/adc/ti-ads1262.c
> +++ b/drivers/iio/adc/ti-ads1262.c
> +static int ads1262_aux_device_setup(struct ads1262 *st)
> +{
> + struct device *dev = &st->spi->dev;
> + struct ads1263_adc2_channel *chans;
> + struct auxiliary_device *adev;
> + struct ads1263_adc2_ctx *ctx;
> + struct fwnode_handle *node;
> + int id, ret;
> +
> + node = device_get_named_child_node(dev, "adc");
> + if (!node)
> + return 0;
> +
> + ctx = kzalloc_obj(*ctx);
> + if (!ctx) {
> + ret = -ENOMEM;
> + goto out_node_put;
> + }
> +
> + id = ida_alloc(&ads1262_ida, GFP_KERNEL);
> + if (id < 0) {
> + ret = id;
> + goto out_free_adc2;
> + }
> +
> + chans = kcalloc(st->num_channels, sizeof(*chans), GFP_KERNEL);
> + if (!chans) {
> + ret = -ENOMEM;
> + goto out_free_id;
> + }
> +
> + for (unsigned int i = 0; i < st->num_channels; i++) {
> + chans[i].negative_input = st->channels[i].negative_input;
> + chans[i].positive_input = st->channels[i].positive_input;
> + }
> +
> + ctx->chip = st;
> + ctx->num_channels = st->num_channels;
> + ctx->channels = chans;
> + ctx->enable = ads1263_adc2_enable;
> + ctx->start = ads1263_adc2_start;
> + ctx->stop = ads1263_adc2_stop;
> + ctx->read = ads1263_adc2_read;
> + mutex_init(&ctx->chan_lock);
devm_mutex_init()
> +
> + adev = &ctx->adev;
> + adev->name = "ads1263_adc2";
> + adev->id = id;
> + adev->dev.release = ads1262_aux_device_release;
> + adev->dev.parent = dev;
> + device_set_node(&adev->dev, no_free_ptr(node));
> +
> + ret = auxiliary_device_init(adev);
> + if (ret)
> + goto out_free_channels;
> +
> + ret = auxiliary_device_add(adev);
> + if (ret) {
> + auxiliary_device_uninit(adev);
> + return ret;
> + }
> +
> + return devm_add_action_or_reset(dev, ads1262_aux_device_destroy, adev);
> +
> +out_free_channels:
> + kfree(chans);
> +out_free_id:
> + ida_free(&ads1262_ida, id);
> +out_free_adc2:
> + kfree(ctx);
> +out_node_put:
> + fwnode_handle_put(node);
> +
> + return ret;
> +}
> diff --git a/drivers/iio/adc/ti-ads1262.h b/drivers/iio/adc/ti-ads1262.h
> new file mode 100644
> index 000000000000..98697d771da3
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads1262.h
> @@ -0,0 +1,39 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Texas Instruments ADS1262 ADC driver
> + *
> + * Copyright (C) 2025 Kurt Borja <kuurtb@gmail.com>
> + */
> +
> +#ifndef _ADS1262_H_
> +#define _ADS1262_H_
> +
> +#include <linux/auxiliary_bus.h>
> +#include <linux/types.h>
> +
> +struct ads1263_adc2_channel {
As with earlier patch avoid bitfields where layout matters.
Better to just use defines for the field makss.
> + /* ADC2CFG */
> + u8 gain:3;
> + u8 refmux:3;
> + u8 data_rate:2;
> +
> + /* ADC2MUX */
> + u8 negative_input:4;
> + u8 positive_input:4;
> +};
> +
> +struct ads1263_adc2_ctx {
> + struct auxiliary_device adev;
> + struct ads1262 *chip;
> + /* Protects channel state */
> + struct mutex chan_lock;
> + struct ads1263_adc2_channel *channels;
> + unsigned int num_channels;
> + int (*enable)(struct ads1263_adc2_ctx *ctx,
> + const struct ads1263_adc2_channel *chan);
> + int (*start)(struct ads1263_adc2_ctx *ctx);
> + int (*stop)(struct ads1263_adc2_ctx *ctx);
> + int (*read)(struct ads1263_adc2_ctx *ctx, __be32 *val);
I'm not sure I see this loose coupling as that useful. I'd just export the
functions from the other module and add them to this header.
Maybe I'm missing why you need this complexity.
> +};
> +
> +#endif
> diff --git a/drivers/iio/adc/ti-ads1263-adc2.c b/drivers/iio/adc/ti-ads1263-adc2.c
> new file mode 100644
> index 000000000000..d21f08bbd9ee
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads1263-adc2.c
> +
> +static int ads1263_adc2_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct ads1263_adc2 *st = iio_priv(indio_dev);
> + struct ads1263_adc2_ctx *ctx = st->ctx;
> + struct ads1263_adc2_channel *chan_data;
> + u8 realbits;
> + __be32 raw;
> + u32 cnv;
> + int ret;
> +
> + chan_data = &st->ctx->channels[chan->scan_index];
> + realbits = chan->scan_type.realbits;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = ads1263_adc2_channel_read(indio_dev, chan_data, &raw);
> + if (ret)
> + return ret;
> +
> + cnv = be32_to_cpu(raw);
> + cnv >>= chan->scan_type.shift;
Is that really an unaligned be24? Might be better to handle it as such.
> + *val = sign_extend32(cnv, realbits - 1);
> +
> + return IIO_VAL_INT;
> +
> + case IIO_CHAN_INFO_SCALE:
> + u64 divd, divr, tmp, rem;
> +
> + mutex_lock(&ctx->chan_lock);
> + divd = st->vref_uV;
> + divr = BIT_ULL(chan_data->gain + realbits - 1) * 1000;
> + mutex_unlock(&ctx->chan_lock);
> +
> + tmp = div64_u64(divd * 1000000000ULL, divr);
> + *val = div64_u64_rem(tmp, 1000000000ULL, &rem);
NANO
> + *val2 = rem;
> +static int
This is oddly different to formatting of most other functions.
static int ads1263_adc2_read_avail(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
const int **vals, int *type,
int *length, long mask)
Seems to be under 80 chars + for this sort of thing, going a bit
over is fine anyway.
> +ads1263_adc2_read_avail(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, const int **vals,
> + int *type, int *length, long mask)
> +{
> + switch (mask) {
> + case IIO_CHAN_INFO_HARDWAREGAIN:
> + *type = IIO_VAL_INT;
> + *vals = ads1263_adc2_gain_avail;
> + *length = ARRAY_SIZE(ads1263_adc2_gain_avail);
> + return IIO_AVAIL_LIST;
> +
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *type = IIO_VAL_INT;
> + *vals = ads1263_adc2_data_rate_avail;
> + *length = ARRAY_SIZE(ads1263_adc2_data_rate_avail);
> + return IIO_AVAIL_LIST;
> +
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static int ads1263_adc2_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct ads1263_adc2 *st = iio_priv(indio_dev);
> + struct ads1263_adc2_ctx *ctx = st->ctx;
> + struct ads1263_adc2_channel *chan_data;
> + unsigned int i;
> +
> + chan_data = &ctx->channels[chan->scan_index];
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_HARDWAREGAIN:
> + for (i = 0; i < ARRAY_SIZE(ads1263_adc2_gain_avail); i++) {
> + if (val == ads1263_adc2_gain_avail[i])
> + break;
> + }
> + if (i == ARRAY_SIZE(ads1263_adc2_gain_avail))
> + return -EINVAL;
> +
> + mutex_lock(&ctx->chan_lock);
> + chan_data->gain = i;
> + mutex_unlock(&ctx->chan_lock);
> +
> + return 0;
> +
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + for (i = 0; i < ARRAY_SIZE(ads1263_adc2_data_rate_avail); i++) {
> + if (val == ads1263_adc2_data_rate_avail[i])
> + break;
> + }
> + if (i == ARRAY_SIZE(ads1263_adc2_data_rate_avail))
> + return -EINVAL;
> +
> + mutex_lock(&ctx->chan_lock);
Slightly nicer to add scope got each cases block and then do just guard() for these
> + chan_data->data_rate = i;
> + mutex_unlock(&ctx->chan_lock);
> +
> + return 0;
> +
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + return ads1263_adc2_channel_hot_reload(st, chan);
> +}
> +
> +static int ads1263_adc2_channels_setup(struct iio_dev *indio_dev)
> +{
> + struct ads1263_adc2 *st = iio_priv(indio_dev);
> + struct device *dev = &st->ctx->adev.dev;
> + struct ads1263_adc2_ctx *ctx = st->ctx;
> + struct iio_chan_spec *chns;
> + unsigned int i;
> +
> + /* Account for the timestamp channel */
> + chns = devm_kcalloc(dev, ctx->num_channels + 1, sizeof(*chns),
> + GFP_KERNEL);
> + if (!chns)
> + return -ENOMEM;
> +
> + for (i = 0; i < ctx->num_channels; i++) {
> + guard(mutex)(&ctx->chan_lock);
> +
> + ctx->channels[i].refmux = st->refmux;
> +
> + chns[i] = ads1263_adc2_iio_voltage_template;
Rather than using a template like this I'd just set it all here using
a designated initializer. Means there is one place to see all the fields.
chns[i] = (struct iio_chan_spec) {
.type = IIO_VOLTAGE,
.indexed = true,
.differential = true, //not sure why this wasn't in your template.
.channel = ctx->channels[i].positive_input;
.channel2 = ctx->channels[i].negative_input;
.scan_index = i,
.scan_type = {
.format = IIO_SCAN_FORMAT_SIGNED_INT,
.realbits = 24,
.storagebits = 32,
.shift = 8,
.endianness = IIO_BE,
},
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
BIT(IIO_CHAN_INFO_SCALE) |
BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
BIT(IIO_CHAN_INFO_SAMP_FREQ),
.info_mask_shared_by_all_available =
BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
BIT(IIO_CHAN_INFO_SAMP_FREQ),
}
> + chns[i].scan_index = i;
> + chns[i].channel = ctx->channels[i].positive_input;
> + chns[i].channel2 = ctx->channels[i].negative_input;
> + chns[i].differential = true;
> + }
> +
> + chns[i] = (struct iio_chan_spec)
> + IIO_CHAN_SOFT_TIMESTAMP(ctx->num_channels - 1);
That macro has recently become a designated intializer so
chns[i] = IIO_CHAN_SOFT_TIMESTAMP(ctx->num_channels - 1);
> + chns[i].scan_index = i;
Isn't this just overwriting the ctx->num_channels - 1 we just
passed in above?
> +
> + indio_dev->num_channels = ctx->num_channels + 1;
> + indio_dev->channels = chns;
> +
> + return 0;
> +}
> +
> +static int ads1263_adc2_probe(struct auxiliary_device *auxdev,
> + const struct auxiliary_device_id *id)
> +{
> + struct ads1263_adc2_ctx *ctx =
> + container_of(auxdev, struct ads1263_adc2_ctx, adev);
> + struct device *dev = &auxdev->dev;
> + struct iio_dev *indio_dev;
> + struct ads1263_adc2 *st;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> + st->ctx = ctx;
> + st->indio_dev = indio_dev;
> +
> + ret = ads1263_adc2_regulator_setup(st);
> + if (ret)
> + return ret;
> +
> + indio_dev->name = (char *)id->driver_data;
See below.
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->info = &ads1263_adc2_iio_info;
> + ret = ads1263_adc2_channels_setup(indio_dev);
> + if (ret)
> + return ret;
> +
> + ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> + iio_pollfunc_store_time,
> + ads1263_adc2_trigger_handler,
> + &ads1263_adc2_buffer_ops);
> + if (ret)
> + return ret;
> +
> + return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static const struct auxiliary_device_id ads1263_adc2_auxiliary_match[] = {
> + { .name = "ti_ads1262.ads1263_adc2",
> + .driver_data = (kernel_ulong_t)"ads1263_adc2" },
{
.name = "ti_ads1262.ads1263_adc2",
.driver_data = (kernel_ulong_t)"ads1263_adc2",
Though I really don't like forcing that cast in there and it should be irrelevant
given there is only one entry in this table. Should be fine to just hard code that
where used. If you need this later, wrap it in a structure.
},
> + { }
> +};
> +MODULE_DEVICE_TABLE(auxiliary, ads1263_adc2_auxiliary_match);
^ permalink raw reply
* Re: [PATCH v9 5/9] ARM: dts: sun8i: v3s: Add nodes for MIPI CSI-2 support
From: Paul Kocialkowski @ 2026-06-13 14:11 UTC (permalink / raw)
To: Chen-Yu Tsai; +Cc: linux-sunxi, conor+dt, robh, devicetree, sashiko-reviews
In-Reply-To: <ahWqcWy_6QuNIOi-@shepard>
[-- Attachment #1: Type: text/plain, Size: 3109 bytes --]
Hi Chen-Yu,
Le Tue 26 May 26, 16:13, Paul Kocialkowski a écrit :
> > > > + reg = <0x01cb2000 0x1000>;
> > > > + clocks = <&ccu CLK_BUS_CSI>,
> > > > + <&ccu CLK_MIPI_CSI>;
> > > > + clock-names = "bus", "mod";
> > > > + resets = <&ccu RST_BUS_CSI>;
> > >
> > > Will this node fail dtbs_check schema validation? The existing device tree
> > > binding schema for "allwinner,sun6i-a31-mipi-dphy" requires an interrupts
> > > property, which appears to be missing here.
> >
> > Yeah, this is definitely going to fail validation.
>
> To be very clear about this, the interrupt is absolutely not required for
> proper operation of the unit and there's a chance we might eventually see
> a SoC that doesn't have it wired.
>
> I could just hook the interrupt for now and we could make it optional if ever
> needed, but I could also mark is as optional now if you prefer.
>
> > > If the V3s hardware physically lacks a D-PHY interrupt, would it be better
> > > to add the V3s-specific compatible string and update the binding schema
> > > to make the interrupts property conditionally optional for the V3s?
> >
> > FWIW the vendor kernel has some code for the D-PHY's interrupt bits [1].
> > Could you add the interrupt (I'm guessing the MIPI-CSI interrupt?) and
> > give it a quick test?
>
> Thanks for digging this up! It really looks like it's just used for debug
> purposes.
>
> I have previously used the mipi csi-2 interrupt for the mipi csi-2 controller
> and never for the d-phy. There's a chance it's the same interrupt that is wired
> to both units (like it is for isp/csi).
>
> I'll give it a try when I get back home. If it doesn't trigger, it probbaly
> means it's not wired to the d-phy and should really be made optional.
I just tested it: the interrupt is wired to both the mipi csi-2 and d-phy
units and triggers when interrupt enable bits are set on the d-phy.
So I will just add it to the d-phy node and keep the property marked as
mandatory in the binding.
All the best,
Paul
> > Thanks
> > ChenYu
> >
> > [1] https://github.com/gediz/v3s-linux-sdk/blob/master/linux-3.4/drivers/media/video/sunxi-vfe/mipi_csi/dphy/dphy_reg.c
> >
> >
> > > > + allwinner,direction = "rx";
> > > > + status = "disabled";
> > > > + #phy-cells = <0>;
> > > > + };
> > > > +
> > >
> > > --
> > > Sashiko AI review · https://sashiko.dev/#/patchset/20260518153339.619947-1-paulk@sys-base.io?part=5
> > >
>
> --
> Paul Kocialkowski,
>
> Independent contractor - sys-base - https://www.sys-base.io/
> Free software developer - https://www.paulk.fr/
>
> Expert in multimedia, graphics and embedded hardware support with Linux.
--
Paul Kocialkowski,
Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/
Expert in multimedia, graphics and embedded hardware support with Linux.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ 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