Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] wifi: mt76: add support for providing precal in nvmem cells #765

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Ansuel
Copy link
Member

@Ansuel Ansuel commented Mar 26, 2023

Add support for providing precal in nvmem cells by adding nvmem cell as
an alternative source for mt76_get_of_eeprom().

Nvmem cells will follow standard nvmem cell definition and needs to be
called 'precal' to be correctly identified.


@nbd168 Hi, i need to be quite honest... this is not tested as i lack any device currently...

But anyway I notice upstream we have mediatek,mtd-eeprom but i really can't see why we need it instead of using the standard way of using nvmem cells.

On openwrt we are widely using them for ath10k precal and mac-address for pretty much everything and even this driver use them to set the mac address... So why not add support for eeprom?

I reordered the function and added support for it. What do you think? Also I decided to use precal for the cell name but this is totally arbritrary and comes from offs = is_mt7915(&dev->mt76) ? MT_EE_PRECAL : MT_EE_PRECAL_V2;

Also maybe on second look the load precal should be a secondary cell so name should be cal?

If this is accepted the final idea is to migrate everyone to nvmem implementation and flag the custom binding as deprecated.

Copy link

@rmilecki rmilecki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ansuel: I'd suggest to:

  1. Drop all #if defined(CONFIG_OF) and #if defined(CONFIG_NVMEM)
  2. Make mt76_get_of_eeprom() try all methods one by one and don't return until the last one fails

eeprom.c Outdated Show resolved Hide resolved
eeprom.c Outdated Show resolved Hide resolved
eeprom.c Show resolved Hide resolved
eeprom.c Outdated Show resolved Hide resolved
eeprom.c Outdated Show resolved Hide resolved
eeprom.c Outdated Show resolved Hide resolved
eeprom.c Outdated Show resolved Hide resolved
eeprom.c Outdated Show resolved Hide resolved
eeprom.c Outdated Show resolved Hide resolved
eeprom.c Outdated Show resolved Hide resolved
eeprom.c Outdated Show resolved Hide resolved
eeprom.c Show resolved Hide resolved
if (IS_ERR(data))
return PTR_ERR(data);

if (retlen < len) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should also fail for retlen > len. If NVMEM cell is bigger than expected EEPROM I'd say sth it just wrong there. I wouldn't advise copying just first N bytes of NVMEM cell as you do below.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh this is almost a pattern... In theory this should never happen... Should we enforce and match always the requested len?

And use != ?

eeprom.c Outdated Show resolved Hide resolved
@rmilecki
Copy link

rmilecki commented May 8, 2023

I fixed compilation errors and verified NVMEM access works (provides the same data as MTD one).

Tested-by: Rafał Miłecki <[email protected]>

In preparation for NVMEM support, split get_of_eeprom() in subfunction
to tidy the code and facilitate the addition of alternative method to
get eeprom data. No behaviour change intended.

While at it also drop OF ifdef checks as OF have stubs and calling
of_get_property would result in the same error returned.

Signed-off-by: Christian Marangi <[email protected]>
@Ansuel
Copy link
Member Author

Ansuel commented May 8, 2023

@rmilecki can you review another time? Think only the retlen is to understand aside from that that is ready...

And we need to understand for the name and the offset thing

@Ansuel
Copy link
Member Author

Ansuel commented May 23, 2023

@rmilecki @nbd168 any news with this? also should I submit this upstream?

@Ansuel
Copy link
Member Author

Ansuel commented Jun 14, 2023

@rmilecki @nbd168 sorry for retag but any news with this? maybe I should just propose upstream?

Add support for providing eeprom in nvmem cells by adding nvmem cell as
an alternative source for mt76_get_of_eeprom().

Nvmem cells will follow standard nvmem cell definition and needs to be
called eeprom' to be correctly identified.

Signed-off-by: Christian Marangi <[email protected]>
@dangowrt
Copy link
Member

dangowrt commented Jul 11, 2023

In case someone wants to test this on devices having the EEPROM stored inside a UBI volume or on a partition on the eMMC, I've written two simple read-only NVMEM providers for that:

dangowrt/linux@02ed591

dangowrt/linux@8229a57

@Ansuel
Copy link
Member Author

Ansuel commented Jul 11, 2023

@dangowrt btw I sent upstream a variant of this patch that has less patch delta, don't know if you notice it.

@dangowrt
Copy link
Member

@neheb
Copy link
Contributor

neheb commented Jul 26, 2023

@Ansuel this was merged.

@Ansuel
Copy link
Member Author

Ansuel commented Jul 26, 2023 via email

@neheb
Copy link
Contributor

neheb commented Jul 26, 2023

cd3dfe3

@Ansuel
Copy link
Member Author

Ansuel commented Jul 26, 2023 via email

@dangowrt
Copy link
Member

See torvalds/linux@5fdaeca
So actual upstream commit is here:
nbd168/wireless@5bef3a4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants