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.
