Re: [PATCH 02/15] nvmem: microchip-otpc: rework to access packets based on tag

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Mon, Aug 4, 2025 at 12:03 PM Varshini Rajendran
<varshini.rajendran@microchip.com> wrote:
>
> Rework the driver to change the packet access technique based on the TAG
> instead of the currently in use "id".
>
> Since there is no way of knowing the OTP memory mapping in advance or the
> changes it can go through with time, the id based approach is not reliable.
> Accessing the packets based on the associated tags is a fail-proof
> approach. This method is aided by adding a table of contents to store the
> payload information which makes it easier to traverse through the OTP
> memory and read the data of the intended packet. The stride of the nvmem
> device is adjusted to 1 to support the TAG being treated as an offset.
> The only reliable way to recognize a packet without being aware of the
> flashed contents of the OTP memory is the TAG of the packet.

...

> +struct mchp_otpc_payload_info {
> +       u32 id;
> +       u32 packet_offset;
> +       u32 payload_length;
> +       u32 packet_type;
> +       u32 packet_tag;

Just wondering if these are okay to be in CPU endianess, i.o.w. is
this part of the protocol to HW, or is it simply Linux driver holding
values?

> +};

...

> +/**
> + * mchp_otpc_read() - Read the OTP packets and fill the buffer based on the TAG
> + *                   of the packet treated as the offset.
> + * @priv: Pointer to device structure.
> + * @off: offset of the OTP packet to be read. In this case, the TAG of the
> + *      corresponding packet.
> + * @val: Pointer to data buffer
> + * @bytes: length of the buffer
> + *
> + * A value of zero will be returned on success, a negative errno will be
> + * returned in error cases.

kernel-doc validator is not happy: Missing Return section.

>   */

...

> -       packet = mchp_otpc_id_to_packet(otpc, off / 4);
> +

Seems like a stray change (adding a blank line).

> +       packet = mchp_otpc_tag_to_packet(otpc, off);
>         if (!packet)
>                 return -EINVAL;

...

> +       writel_relaxed(0UL, otpc->base + MCHP_OTPC_AR);

Does UL give any value  / make any sense here?

...

> +       if (size == 0) {
> +               dev_err(otpc->dev, "Cannot access OTP memory !\n");
> +               if (!emul_enable)
> +                       dev_err(otpc->dev, "Boot packet not configured & Emulation mode not enabled !\n");

Stray space before the exclamation mark in both messages.

> +       }

-- 
With Best Regards,
Andy Shevchenko