Re: [PATCH 1/2] dt-bindings: nvmem: add byte-mask property for non-contiguous byte selection

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] dt-bindings: nvmem: byte-mask redefines reg size and accesses
  beyond reg
- [Low] dt-bindings: nvmem: missing hardware motivation for byte-mask in
  changelog
--

commit f8160f5bc1c29ed4ba48f6d458beb8ca96b09427
Author: Hongmei Xu <hongmei.xu@oss.qualcomm.com>

> dt-bindings: nvmem: add byte-mask property for non-contiguous byte selection
>
> Add the byte-mask property to the fixed-cell binding. This allows
> selecting non-contiguous physical bytes from an NVMEM region to form
> a logical cell value.
>
> Bit N set in byte-mask means physical byte N relative to the cell's
> base offset is included in the output; selected bytes are packed into
> the output buffer in ascending byte order. If fewer bytes are selected
> than the cell's logical size given by reg, the remaining output bytes
> are zero-filled. The number of set bits must not exceed the cell's
> logical size given by reg. byte-mask and bits are mutually exclusive.
>
> Signed-off-by: Hongmei Xu <hongmei.xu@oss.qualcomm.com>

[Severity: Low]
Could the commit message explain the hardware layout that motivates
byte-mask? It repeats the property rules from the schema, but does not
describe which SoC or NVMEM layout splits a logical cell across
non-contiguous physical bytes or why existing mechanisms such as multiple
reg entries or a device-specific layout are insufficient.

> diff --git a/Documentation/devicetree/bindings/nvmem/layouts/fixed-cell.yaml b/Documentation/devicetree/bindings/nvmem/layouts/fixed-cell.yaml
> index 38e3ad50ff4fb..cddda3f706917 100644
> --- a/Documentation/devicetree/bindings/nvmem/layouts/fixed-cell.yaml
> +++ b/Documentation/devicetree/bindings/nvmem/layouts/fixed-cell.yaml
> @@ -34,6 +34,21 @@ properties:
>          description:
>            Size in bit within the address range specified by reg.
>  
> +  byte-mask:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 1
> +    description: >
> +      Bitmask selecting which physical bytes within the NVMEM device region
> +      contribute to this cell's output. Bit N set means physical byte N
> +      relative to the cell's base offset is included; selected bytes are
> +      packed into the output buffer in ascending byte order. If fewer bytes
> +      are selected than the cell's logical size given by reg, the remaining
> +      output bytes are zero-filled. Byte positions are relative to the cell's
> +      base offset and limited to a 32-byte window. The highest set bit must
> +      not reference a byte beyond the device size. The number of set bits must
> +      not exceed the cell's logical size given by reg. Cannot be used
> +      together with bits.

[Severity: Medium]
Does byte-mask conflict with standard reg semantics by treating the reg
size as a logical output buffer size while reading physical bytes outside
the reg range?

Any non-contiguous selection of K bytes spans at least K + 1 physical
bytes (highest set bit H >= K). A Devicetree author must then either set
the reg size to K and access physical bytes up to offset + H outside the
declared reg range, hiding overlaps with sibling cells, or set the reg
size to the physical span M >= H + 1, which pads the output to M bytes and
breaks typed consumers like nvmem_cell_read_u32() that require the cell
size to match sizeof(*val).

Defining software zero-padding in reg also puts buffer formatting policy
into the hardware description, and the binding leaves write semantics for
cells with byte-mask undefined.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-nvmem-byte-mask-for-split-macaddr-v1-0-138f715f18c1@oss.qualcomm.com?part=1