Re: [PATCH v2 3/4] ARM: novena: Read Ethernet MAC address from EEPROM

[John Watts <contact@jookia.org>]

On Wed, Jan 25, 2023 at 09:07:14PM +0100, Marco Felsch wrote:
> > +	rc = read_file_2("/dev/eeprom0", &read, &eeprom, max);
> 
> You never free the eeprom buffer.

Argh, I explicitly remember writing down that I need to do this then promptly
got distracted when searching for the correct free function to use.

> > +
> > +	if (rc < 0 && rc != -EFBIG) {
> > +		pr_err("Unable to read Novena EEPROM: %s\n", strerror(-rc));
> > +		return NULL;
> > +	} else if (read != max) {
> > +		pr_err("Short read from Novena EEPROM?\n");
> > +		return NULL;
> > +	} else {
> > +		return eeprom;
> > +	}
> 
> Last else can be dropped and instad:
> 
> 	return eeprom;
> > +}

Okay.

> > +
> > +static bool novena_check_eeprom(struct novena_eeprom *eeprom)
> > +{
> > +	char *sig = eeprom->signature;
> > +	size_t size = sizeof(eeprom->signature);
> > +
> > +	if (memcmp("Novena", sig, size) != 0) {
> > +		pr_err("Unknown Novena EEPROM signature\n");
> > +		return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +static void novena_set_mac(struct novena_eeprom *eeprom)
> > +{
> > +	struct device_node *dnode;
> > +
> > +	dnode = of_find_node_by_alias(of_get_root_node(), "ethernet0");
> > +	if (dnode)
> > +		of_eth_register_ethaddr(dnode, eeprom->mac);
> > +	else
> > +		pr_err("Unable to find ethernet node\n");
> > +}
> > +
> > +static void novena_try_eeprom(void)
> > +{
> > +	struct novena_eeprom *eeprom = novena_read_eeprom();
> > +
> > +	if (!eeprom || !novena_check_eeprom(eeprom))
> > +		return;
> > +
> > +	novena_set_mac(eeprom);
> > +}
> >  
> >  static int novena_probe(struct device *dev)
> >  {
> > +	novena_try_eeprom();
> 
> It's not wrong to move it into a sub-function but IMO at least this
> function can be part of the probe() function. Also I would do the
> device_ensured_probe function here and just have one function which I
> would call: novena_set_mac(). This way it's a bit easier to follow the
> code. But that's just my oppinion.

I'm not quite sure what you mean, could you provide an example?

> > +	eeprom: eeprom@56 {
> > +		compatible = "24c512";
> > +		reg = <0x56>;
> > +		pagesize = <128>;
> > +		status = "okay";
> 
> status can be dropped, "okay" is the default for the dtc.

Gotcha.

> 
> Regards,
>   Marco

John.