Hi Heikki, On 30/11/2023 12:54, Heikki Krogerus wrote: > Hi Roger, > >>> Why not just match against the structures themselves? >>> >>> if (tps->data == &tps25750_data) >>> ... >> >> Then you need to declare tps25750_data and friends at the top of the file? >> >> A better approach might be to have type agnostic quirk flags for the special >> behavior required for different types. This way, multiple devices can share >> the same quirk if needed. >> >> e.g. >> NEEDS_POWER_UP instead of TIPD_TYPE_APPLE_CD321X >> SKIP_VID_READ instead of TIPD_TYPE_TI_TPS25750X >> INIT_ON_RESUME instead of TIPD_TYPE_TI_TPS25750X >> >> Also rename cd321x_switch_power_state() to tps6598x_switch_power_state(). > > No. Functions like that isolate cd321x specific functionality into an > actual "function" just like they should. > > Quirk flags mean that if something breaks, it will almost always break > for everybody (there is no real isolation with quirk flags), and when > things are fixed and when features are added, we are forced to always > "dance" around those quirk flags - you always have to consider them. > > Platform/device type checks are just as bad IMO, but in one way they > are better than quirk flags. There is no question about what a > platform check is checking, but quirk flags can so easily become > incomprehensible (just what exactly does it mean when you say > NEEDS_POWER_UP, SKIP_VID_READ and so on (you would need to document > those quirks, which is waste of effort, and in reality nobody will do). > > In case of tipd/code.c, it should be converted into a library that > only has the common/shared functionality. CD321, TPS2579x, TPS6598x > and what ever there is, then will have a glue driver that handles > everything that specific for their controller type. Do you mean that you want to treat the 3 devices as different incompatible devices so each one has a separate driver which warrants for a different DT binding for each and also Kconfig symbol? > > Before this driver is reorganised like that (any volunteers?), we'll > have the PD controller type checks, but quirk flags we will not have. > > In general, you should only use quirk flags if there is no other > way to move forward - they are the last resort. They are dangerous, > and even in the best case they reduce the maintenability of the code. > > thanks, > -- cheers, -roger
