On Fri, 24 Oct 2025 10:26:16 +0100 Karunika Choo <[email protected]> wrote:
> On 24/10/2025 07:43, Boris Brezillon wrote: > > On Tue, 14 Oct 2025 10:43:30 +0100 > > Karunika Choo <[email protected]> wrote: > > > >> Add a framework to support architecture-specific features. This allows > >> other parts of the driver to adjust their behaviour based on the feature > >> bits enabled for a given architecture. > > > > I'm not convinced we need this just yet. AFAICT, the only feature flag > > being added in this patchset is PANTHOR_HW_FEATURE_PWR_CONTROL, and > > most of this is abstracted away with function pointers already. The > > only part that tests this FEATURE_PWR_CONTROL flag is the > > initialization, which could very much be abstracted away with a > > function pointer (NULL meaning no PWR block present). There might be > > other use cases you're planning to use this for, so I'd like to hear > > about them to make my final opinion on that. > > > > I see your point — the intent here is mainly to have the feature flag > reflect hardware-level changes. In this series, for example, it > corresponds to the addition of the new PWR_CONTROL block. Yes, but those are not really optional features. Those are functional changes that are usually done on major version changes. But let's say it was something done on a minor version change, it's still something that I think would be better off abstracted using a vtable of some sort, and have this vtable forked everytime a version changes requires something new. > > Another use case would be arch v11, where a new PRFCNT_FEATURES register > was introduced. In that case, we might want to adjust the > counters_per_block [1] value depending on that register’s value. Again, it looks like a property that can be determined at init time. For v10 it'd be hardcoded to X, and on v11+, you'd extract that from PERFCNT_FEATURES. I'm really not a huge fan of this feature flag pattern because it's very easy to forget to add/propagate one flag when adding support for new HW/flags. So I'd much rather rely on ">= X.Y" version checks in the init path, and for anything more involved or happening in some hot path, function based pointer specialization. > > I would also expect this mechanism to remain useful for future hardware > revisions, as it provides a clean way to describe architectural > differences without scattering version-specific checks throughout the > code, while still being lighter-weight than function pointers. Well, that's questionable. What I usually see in practice is the following pattern spreading over the code base: if (SUPPORTS(OBSCURE_FEATURE_NAME)) { // do stuff that are not obviously related to the // feature flag name } whereas, if we're having a model where the specialization is done high enough, you'd just end up with functions calling more specialized helpers: void do_something_for_v12() { hw_block_a_do_y() hw_block_b_do_x() ... }
