[PATCH v4 1/6] FWU: Add FWU metadata access driver for MTD storage regions

[Michal Simek]

On 4/10/23 05:56, Jassi Brar wrote:
> On Wed, 29 Mar 2023 at 07:00, Michal Simek <michal.simek at amd.com> wrote:
>> On 3/27/23 23:15, jassisinghbrar at gmail.com wrote:
> 
>>> diff --git a/drivers/fwu-mdata/raw_mtd.c b/drivers/fwu-mdata/raw_mtd.c
>>> new file mode 100644
>>> index 0000000000..4b1a10073a
>>> --- /dev/null
>>> +++ b/drivers/fwu-mdata/raw_mtd.c
>>> @@ -0,0 +1,272 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>
>> Just a note: Did you choose GPL-2.0+ by purpose? Or it is just c&p?
>>
> just c&p. though isn't that the same as GPL-2.0-or-later ?

license choice is up to you. We normally use just gpl-2.0.

> 
> .......
>>> +
>>> +extern struct fwu_mtd_image_info fwu_mtd_images[];
>>
>> if there is a need to share it. It should go to header.
>>
> include/fwu,h  would be the header. Which is supposed to be very
> global, and doesn't seem very appropriate to mention private data
> shared between a driver and helper library.
> If people still insist, I will make the change.
> 
>> And fwu_mtd_images[] is not defined when only this patch is applied.
>> It means order of patches is not right. 1/6 and 2/6 should be swapped
>>
> I think 1 and 2 should be merged rather.

no issue with it.

> 
> ......
>>> +     if (!mtd_priv->mtd) {
>>> +             log_err("Failed to find mtd device by fwu-mdata-store\n");
>>> +             return -ENODEV;
>>> +     }
>>> +
>>> +     /* Get the offset of primary and seconday mdata */
>>
>>
>> typo
>>
> ok
> 
> ......
>>> +
>>> +static const struct udevice_id fwu_mdata_ids[] = {
>>> +     { .compatible = "u-boot,fwu-mdata-mtd" },
>>> +     { }
>>> +};
>>
>> As I said this DT binding should be approved first to make sure that we don't
>> need to fix DT binding in future. Just simply do it right from the begining.
>>
> Yes, I will cc Rob in the next submission (I only forgot last time).
> However, let us note that fwu-mdata-gpt.yaml isn't blessed either.
> I am not sure if there is any reason for the fwu node to even be in
> the dts for kernel. But sure it is good to have it eyeballed by the DT
> gods.

It doesn't really go to kernel.
Simon pushed options node directly to dt-schema
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/options/u-boot.yaml#L96
And that would be also location for this node too.

Thanks,
Michal