Re: [PATCH v1 1/3] dt-bindings: i2c: Add Qualcomm I2C slave controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The binding conflates the I2C controller and the logical I2C slave device, violating the standard Linux I2C slave device tree model.
- [Low] The compatible string `qcom,i2c-slave` is overly generic and lacks an SoC-specific identifier.
--

commit cd018ae5e3845f1ec68c4429fb88f4e263862a30
Author: Viken Dadhaniya <viken.dadhaniya@oss.qualcomm.com>

dt-bindings: i2c: Add Qualcomm I2C slave controller

This commit introduces a device tree binding for the Qualcomm I2C slave
controller on the QDU1000. It defines properties for the slave address,
clocks, interrupts, and interconnects for a hardware block that operates
exclusively as an I2C slave device.

> diff --git a/Documentation/devicetree/bindings/i2c/qcom,i2c-slave.yaml b/Documentation/devicetree/bindings/i2c/qcom,i2c-slave.yaml
> new file mode 100644
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-slave.yaml
> @@ -0,0 +1,94 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i2c/qcom,i2c-slave.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm I2C Slave Controller
> +
> +maintainers:
> +  - Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
> +  - Viken Dadhaniya <viken.dadhaniya@oss.qualcomm.com>
> +
> +description:
> +  The Qualcomm I2C slave controller is a dedicated hardware IP found on
> +  Qualcomm SoCs that operates exclusively as an I2C slave device on the bus.
> +  It supports FIFO (PIO) mode for data transfer and does not function as an
> +  I2C master.
> +
> +properties:
> +  compatible:
> +    const: qcom,i2c-slave

[Severity: Low]
Is this compatible string overly generic? Including an SoC-specific identifier
like qcom,qdu1000-i2c-slave alongside a generic fallback is standard practice
to handle potential future hardware variants or quirks and avoid ABI breakage.

[ ... ]

> +  qcom,slave-addr:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      The 7-bit I2C slave address programmed into the controller's device
> +      address register. This controller operates exclusively as an I2C slave
> +      and has no child nodes, so the standard 'reg' property cannot be used
> +      to convey the I2C address (it is already used for the MMIO register
> +      range). The address is configurable at runtime via a hardware register.
> +    minimum: 0x08
> +    maximum: 0x77

[Severity: High]
Does this custom qcom,slave-addr property bypass the standard Linux I2C slave
device tree model? 

In standard I2C slave bindings, the address is specified via a child node
using the reg property with the I2C_OWN_SLAVE_ADDRESS flag. This allows
standard I2C slave backend drivers like slave-eeprom or slave-testunit to
attach correctly.

[ ... ]

> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - clock-names
> +  - qcom,slave-addr
> +  - interconnects
> +  - interconnect-names
> +
> +additionalProperties: false

[Severity: High]
By setting additionalProperties: false and implicitly forbidding #address-cells,
#size-cells, and child nodes, are we entirely locking out the standard I2C
slave framework? 

This design appears to conflate the I2C controller with the logical I2C slave
device, which could force driver implementations to bypass standard reg_slave
and unreg_slave APIs in favor of registering non-standard adapters.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260628-i2c-qcom-slave-v1-0-8b0a5c01f9f6@oss.qualcomm.com?part=1