Re: [PATCH] of: of_net: add support to parse ASCII encoded mac-addresses

[Marco Felsch <m.felsch@pengutronix.de>]

Hi Ahmad,

On 23-08-08, Ahmad Fatoum wrote:
> Hello Marco,
> 
> On 07.08.23 19:07, Marco Felsch wrote:
> > Some vendors like Polyhex store the MAC address ASCII encoded instead of
> > using the plain 6-byte MAC address. This commit adds the support to
> > decode the 12-byte ASCII encoded MAC addresses.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> 
> FYI, the upstream device tree binding for this is NVMEM layout, which was only
> recently added to Linux and for which barebox has no support yet.

I know that, thanks for the info :) I thought that this is no "layout"
it's just the mac-address stored in ASCII instead of plain 6-byte
storage.

> I can understand that porting NVMEM layouts, just to get a MAC address assigned
> might not be an attractive proposition, but I don't think that adding a new
> barebox-specific binding is the right way here.

Me neither therefore I dropped the barebox specific binding and did just
do some heuristic.

> I'd suggest, you get the nvmem cell in board code and assign it there.
> There's readily available API for that. If you are interested in a
> generic solution, NVMEM layouts are the way to go IMO.

Thought about that too but went this way because it's much less code
than doing it in the board code. Also it allows to share the code with
others.

As said, I don't think that this is a layout. Of course there are more
ASCII strings to store the production test result but this is not
relevant. I really need to check which is more effort
board-code vs. layout-support if you think that this is layout.

> > ---
> >  drivers/of/of_net.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> > 
> > diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
> > index 75a24073da51..4e74986cdda8 100644
> > --- a/drivers/of/of_net.c
> > +++ b/drivers/of/of_net.c
> > @@ -79,6 +79,8 @@ static int of_get_mac_addr(struct device_node *np, const char *name, u8 *addr)
> >  	return -ENODEV;
> >  }
> >  
> > +#define ETH_ALEN_ASCII	12
> > +
> >  int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr)
> >  {
> >  	struct nvmem_cell *cell;
> > @@ -98,6 +100,23 @@ int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr)
> >  	if (IS_ERR(mac))
> >  		return PTR_ERR(mac);
> >  
> > +	if (len == ETH_ALEN_ASCII) {
> > +		u8 *mac_new;
> > +		int ret;
> > +
> > +		mac_new = kzalloc(sizeof("xx:xx:xx:xx:xx:xx"), GFP_KERNEL);
> > +		ret = hex2bin(mac_new, mac, ETH_ALEN);
> 
> Why not parse into a fixed size local buffer and then copy it? Would save
> you the extra allocation.

I went this way to keep the below free logic the same.

Regards,
  Marco