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
