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

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

Hi John,

On 23-01-26, John Watts wrote:
> 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;

...

> > >  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?

I meant that the content of novena_try_eeprom() can be part of the
probe() function :) Furthermore I mean that having to much sub-functions
isn't really helpful, but that is just my oppinion.

Regards,
  Marco