On Fri, 2017-10-13 at 18:02 +0200, Antonio Ospite wrote: > On Thu, 12 Oct 2017 17:04:42 +0200 > Marcin Niestroj <m.niestroj@xxxxxxxxxxxxxxxx> wrote: > > > Support was added based on Goodix GitHub repo [1]. There are two > > major > > differences between gt1151 and currently supported devices (gt9x): > > * CONFIG_DATA register has 0x8050 address instead of 0x8047, > > * config data checksum has 16-bit width instead of 8-bit. > > > > Also update goodix_i2c_test() function, so it reads ID register > > (which > > has the same address for all devices) instead of CONFIG_DATA > > (because > > its address is known only after reading ID of the device). > > > > [1] https://github.com/goodix/gt1x_driver_generic > > > > Signed-off-by: Marcin Niestroj <m.niestroj@xxxxxxxxxxxxxxxx> > > --- > > Patch was developed and tested on top of 4.14-rc4 using custom > > board. > > > > Just a suggestion, you could use a function pointer for the > device-specific checksum routines and have the check on the device id > only once in goodix_ts_probe(), see below. Function pointer is fine, but this is how I'd implement it: typedef enum { GOODIX_CONFIG_TYPE_GT911 = 0, GOODIX_CONFIG_TYPE_GT1151 } GoodixConfigType; static func_pointer config_funcs[] = { goodix_gt911_get_config, goodix_gt1151_get_config }; Store the GoodixConfigType in the device structure, and access the function pointer as: config_funcs[config_type] (... That's what I'd prefer, but whatever Dmitry prefers style-wise. This seems more extensible in case we have different functions needing similar changes in the future, allowing us to either extend the function pointer array, or use switch/case statements for smaller functions. Cheers -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
