Hi Dmitry, Thanks for the feedback. > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe() > > On Sat, Jul 22, 2023 at 05:51:17PM +0000, Biju Das wrote: > > Hi Dmitry Torokhov, > > > > Thanks for the feedback. > > > > > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe() > > > > > > On Wed, Jul 19, 2023 at 06:43:47AM +0000, Biju Das wrote: > > > > Hi Dmitry Torokhov, > > > > > > > > Thanks for the feedback. > > > > > > > > > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe() > > > > > > > > > > On Mon, Jul 17, 2023 at 06:45:27PM +0000, Biju Das wrote: > > > > > > Hi Dmitry, > > > > > > > > > > > > > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify > > > > > > > probe() > > > > > > > > > > > > > > On Mon, Jul 17, 2023 at 07:15:50PM +0100, Mark Brown wrote: > > > > > > > > On Mon, Jul 17, 2023 at 04:35:02PM +0000, Biju Das wrote: > > > > > > > > > > > > > > > > > The .device_get_match_data callbacks are missing for I2C > > > > > > > > > and SPI bus > > > > > > > subsystems. > > > > > > > > > Can you please throw some lights on this? > > > > > > > > > > > > > > > > It's the first time I've ever heard of that callback, I > > > > > > > > don't know why whoever added it wouldn't have done those > > > > > > > > buses in particular or if it just didn't happen. Try > > > > > > > > adding it and if it works send > > > > > the patches? > > > > > > > > > > > > > > I think there is a disconnect. Right now > > > > > > > device_get_match_data callbacks are part of > > > > > > > fwnode_operations. I was proposing to add another optional > > > > > > > device_get_match_data callback to 'struct > > > bus_type' > > > > > > > to allow individual buses control how match data is handled, > > > > > > > before (or after) jumping into the fwnode-backed > > > > > > > device_get_match_data > > > > > callbacks. > > > > > > > > > > > > That is what implemented here [1] and [2] right? > > > > > > > > > > > > > > > > > > First it check for fwnode-backed device_get_match_data > > > > > > callbacks and Fallback is bus-type based match. > > > > > > > > > > > > Looks like you are proposing to unify [1] and [2] and you want > > > > > > the logic to be other way around. ie, first bus-type match, > > > > > > then fwnode-backed callbacks? > > > > > > > > > > > > > > > > I do not have a strong preference for the ordering, i.e. I think > > > > > it is perfectly fine to do the generic fwnode-based lookup and > > > > > if there is no match have bus method called as a fallback, > > > > > > > > That involves a bit of work. > > > > > > > > const void *device_get_match_data(const struct device *dev); > > > > > > > > const struct i2c_device_id *i2c_match_id(const struct > > > > i2c_device_id > > > *id, > > > > const struct i2c_client > > > > *client); > > > > > > > > const struct spi_device_id *spi_get_device_id(const struct > > > > spi_device *sdev); > > > > > > > > Basically, the bus-client driver(such as exc3000) needs to pass > > > > struct device and device_get_match_data after generic fwnode-based > > > > lookup, needs to find the bus type based on struct device and call > > > > a new generic > > > > void* bus_get_match_data(void*) callback, so that each bus > > > > interface can do a match. > > > > > > Yes, something like this (which does not seem that involved to > me...): > > > > Looks it will work. > > > > But there is some 2 additional checks in core code, every driver which > is not bus type need to go through this checks. > > > > Also in Bus specific callback, there are 2 additional checks. > > > > So, performance wise [1] is better. > > I do not believe this is a concern whatsoever: majority of > architectures/boards have been converted to ACPI/DT, which are being > matched first as they are now, so the fallback to bus-specific matching > against bus-specific device ID tables will be very infrequent. > Additionally, device_get_match_data() is predominantly called from > driver probe paths, so we need not be concerned with it being used with > class devices or other kinds of devices not associated with a bus. Looks like most of the i2c client driver uses similar handling for ACPI/DT and ID tables. If that is the case, it is good to have this proposed change which will simplify most of the drivers listed in [1] [1] https://elixir.bootlin.com/linux/latest/A/ident/i2c_match_id Eg: drivers/hwmon/pmbus/ibm-cffps.c enum versions vs = cffps_unknown; const void *md = of_device_get_match_data(&client->dev); const struct i2c_device_id *id; if (md) { vs = (enum versions)md; } else { id = i2c_match_id(ibm_cffps_id, client); if (id) vs = (enum versions)id->driver_data; } The above code can be converted to vs = (enum versions)device_get_match_data(&client->dev); > > > > > Moreover, we need to avoid code duplication with [1] > > > > [1] > > If and when my proposed solution gets into the kernel we can drop > i2c_get_match_data() altogether. Agreed. Will wait for other people's view on this topic. Cheers, Biju
