Messages in this thread | | | Date | Tue, 25 Jan 2022 13:08:58 +0100 | Subject | Re: [PATCH 6/8] nvmem: transformations: ethernet address offset support | From | Rafał Miłecki <> |
| |
On 28.12.2021 15:25, Michael Walle wrote: > An nvmem cell might just contain a base MAC address. To generate a > address of a specific interface, add a transformation to add an offset > to this base address. > > Add a generic implementation and the first user of it, namely the sl28 > vpd storage. > > Signed-off-by: Michael Walle <michael@walle.cc> > --- > drivers/nvmem/transformations.c | 45 +++++++++++++++++++++++++++++++++ > 1 file changed, 45 insertions(+) > > diff --git a/drivers/nvmem/transformations.c b/drivers/nvmem/transformations.c > index 61642a9feefb..15cd26da1f83 100644 > --- a/drivers/nvmem/transformations.c > +++ b/drivers/nvmem/transformations.c > @@ -12,7 +12,52 @@ struct nvmem_transformations { > nvmem_cell_post_process_t pp; > }; > > +/** > + * nvmem_transform_mac_address_offset() - Add an offset to a mac address cell > + * > + * A simple transformation which treats the index argument as an offset and add > + * it to a mac address. This is useful, if the nvmem cell stores a base > + * ethernet address. > + * > + * @index: nvmem cell index > + * @data: nvmem data > + * @bytes: length of the data > + * > + * Return: 0 or negative error code on failure. > + */ > +static int nvmem_transform_mac_address_offset(int index, unsigned int offset, > + void *data, size_t bytes) > +{ > + if (bytes != ETH_ALEN) > + return -EINVAL; > + > + if (index < 0) > + return -EINVAL; > + > + if (!is_valid_ether_addr(data)) > + return -EINVAL; > + > + eth_addr_add(data, index); > + > + return 0; > +} > + > +static int nvmem_kontron_sl28_vpd_pp(void *priv, const char *id, int index, > + unsigned int offset, void *data, > + size_t bytes) > +{ > + if (!id) > + return 0; > + > + if (!strcmp(id, "mac-address")) > + return nvmem_transform_mac_address_offset(index, offset, data, > + bytes); > + > + return 0; > +} > + > static const struct nvmem_transformations nvmem_transformations[] = { > + { .compatible = "kontron,sl28-vpd", .pp = nvmem_kontron_sl28_vpd_pp }, > {} > };
I think it's a rather bad solution that won't scale well at all.
You'll end up with a lot of NVMEM device specific strings and code in a NVMEM core.
You'll have a lot of duplicated code (many device specific functions calling e.g. nvmem_transform_mac_address_offset()).
I think it also ignores fact that one NVMEM device can be reused in multiple platforms / device models using different (e.g. vendor / device specific) cells.
What if we have: 1. Foo company using "kontron,sl28-vpd" with NVMEM cells: a. "mac-address" b. "mac-address-2" c. "mac-address-3" 2. Bar company using "kontron,sl28-vpd" with NVMEM cell: a. "mac-address"
In the first case you don't want any transformation.
If you consider using transformations for ASCII formats too then it causes another conflict issue. Consider two devices:
1. Foo company device with BIN format of MAC 2. Bar company device with ASCII format of MAC
Both may use exactly the same binding:
partition@0 { compatible = "nvmem-cells"; reg = <0x0 0x100000>; label = "bootloader";
#address-cells = <1>; #size-cells = <1>;
mac-address@100 { reg = <0x100 0x6>; }; };
how are you going to handle them with proposed implementation? You can't support both if you share "nvmem-cells" compatible string.
I think that what can solve those problems is assing "compatible" to NVMEM cells.
Let me think about details of that possible solution.
| |