Hi all, On Mon, 2019-12-16 at 07:49 +0000, Ardelean, Alexandru wrote: > On Sun, 2019-12-15 at 16:18 +0000, Jonathan Cameron wrote: > > > > On Fri, 13 Dec 2019 18:03:08 +0200 > > Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx> wrote: > > > > > This change is done in preparation of adding an `struct > > > adis_timeout` > > > type. > > > Some ADIS drivers support multiple drivers, with various > > > combinations > > > of > > > timeouts. Creating static tables for each driver is possible, but > > > that > > > also > > > creates quite a lot of duplication. > > > > > > Signed-off-by: Nuno Sá <nuno.sa@xxxxxxxxxx> > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx> > > > > There are considerable advantages to using constant structures, > > (security - not that relevant here probably, XiP, general > > readability) > > > > So to take a series like this I want to see evidence that it makes > > a significant difference. So far you just have cases where we end > > up > > with a worse result. More code, harder to read... > > > > Hence it will take a lot to persuade me to take this series without > > the follow up patches where I assume significant advantages are > > seen. > > > > Well, we've have some discussion about this, and how to do it. > There are several alternatives. > > Some of the ideas were: > 1. Keep the static data and clone it + populate the adis_timeout data > as > needed during probe [based on each device's chip-info] > 2. Rework all the chip-info data to include the adis_data types/info > > 2. may require more work ; 1. require fewer patches > > This implementation [in this series] is 1. but without keeping the > static > data and template. > I guess the idea was to reduce memory usage [by keeping the static > data]. I > admit the memory usage is not that big. > > I'll take a look at this again, and see if 2. can work more nicely. > It might be that 1. would be the end-result, but who knows? > > Thanks > Alex I guess we also need to prepare/send the following patches to show Jonathan why we need to dynamically allocate the data structure in some drivers. In the end is because some devices require different timeouts (handled by the adis core library) than the others and, in some cases these differences are quite significative. It was even happening that in some cases, we were not sleeping enough time (eg: after a reset command). In the next patches, a timeout structure is included that needs to be filled for each device. Alex, maybe we should include more patches in this series to show the "big picture" and then we can discuss if this is the best approach. Nuno Sá
