| Message ID | 20240921181939.392517-1-vassilisamir@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | iio: core: remove iio_validate_own_trigger() function | expand |
On 9/21/24 11:19, Vasileios Amoiridis wrote: > The iio_validate_own_trigger() function was added in this commit [1] but it is > the same with the below function called iio_trigger_validate_own_device(). The > bodies of the functions can be found in [2], [3]. > > [1]: https://lore.kernel.org/all/51cd3e3e74a6addf8d333f4a109fb9c5a11086ee.1683541225.git.mazziesaccount@gmail.com/ > [2]: https://elixir.bootlin.com/linux/v6.11/source/drivers/iio/industrialio-trigger.c#L732 > [3]: https://elixir.bootlin.com/linux/v6.11/source/drivers/iio/industrialio-trigger.c#L752 The signature of the two functions are different, the order of the parameters is switched. So you can't just swap them out for the `validate_trigger` callback since the signature is not compatible. But maybe you can update the implementation of one of the functions to calling the other function.
On Sat, Sep 21, 2024 at 12:23:39PM -0700, Lars-Peter Clausen wrote: > On 9/21/24 11:19, Vasileios Amoiridis wrote: > > The iio_validate_own_trigger() function was added in this commit [1] but it is > > the same with the below function called iio_trigger_validate_own_device(). The > > bodies of the functions can be found in [2], [3]. > > > > [1]: https://lore.kernel.org/all/51cd3e3e74a6addf8d333f4a109fb9c5a11086ee.1683541225.git.mazziesaccount@gmail.com/ > > [2]: https://elixir.bootlin.com/linux/v6.11/source/drivers/iio/industrialio-trigger.c#L732 > > [3]: https://elixir.bootlin.com/linux/v6.11/source/drivers/iio/industrialio-trigger.c#L752 > > The signature of the two functions are different, the order of the > parameters is switched. So you can't just swap them out for the > `validate_trigger` callback since the signature is not compatible. But maybe > you can update the implementation of one of the functions to calling the > other function. > Hi Lars, Hmm, I see what you mean. Still though, do you think that we could do some cleaning here? I can see 3 approaches: 1) One of the 2 functions calls the other internally and nothing else has to change. 1) The default iio_validate_own_trigger() is used only by 2 out of many drivers who use the .validate_trigger call. If this is deprecated, many function signatures will need to change (swap the args) and then rename the rest. The default iio_trigger_validate_own_device() is used in almost all of the drivers apart from 3 * gyro/st_gyro_core.c * common/st_sensors/st_sensors_trigger.c * trigger/stm32-lptimer-trigger.c So it will be less noise to change the iio_trigger_validate_own_device() in the sense that the signature of 3 functions will need to change (swap args) and then rename the rest. 1 is by far the less noisy as only a couple lines need to change but we still endup with 2 functions doing the same thing. 2 and 3 require more noise but we end up having 1 implementation which looks cleaner. What would you say? Cheers, Vasilis
On 9/21/24 23:07, Vasileios Amoiridis wrote: > On Sat, Sep 21, 2024 at 12:23:39PM -0700, Lars-Peter Clausen wrote: >> On 9/21/24 11:19, Vasileios Amoiridis wrote: >>> The iio_validate_own_trigger() function was added in this commit [1] but it is >>> the same with the below function called iio_trigger_validate_own_device(). The >>> bodies of the functions can be found in [2], [3]. >>> >>> [1]: https://lore.kernel.org/all/51cd3e3e74a6addf8d333f4a109fb9c5a11086ee.1683541225.git.mazziesaccount@gmail.com/ >>> [2]: https://elixir.bootlin.com/linux/v6.11/source/drivers/iio/industrialio-trigger.c#L732 >>> [3]: https://elixir.bootlin.com/linux/v6.11/source/drivers/iio/industrialio-trigger.c#L752 >> >> The signature of the two functions are different, the order of the >> parameters is switched. So you can't just swap them out for the >> `validate_trigger` callback since the signature is not compatible. But maybe >> you can update the implementation of one of the functions to calling the >> other function. >> > > Hi Lars, > > Hmm, I see what you mean. Still though, do you think that we could do some > cleaning here? I can see 3 approaches: > > 1) One of the 2 functions calls the other internally and nothing else has > to change. I would go with this. Changing the signatures to be the same would be (in my, not always humble enough, opinion) wrong. The different order of parameters reflects the different idea. One checks if device for trigger is the right one, the other checks if the trigger for the device is the right one. Thus, the order of parameters should be different. Calling the same implementation internally is fine with me. Maybe Jonathan will share his opinion when recovers from all the plumbing in Vienna ;) Yours, -- Matti
On Sun, Sep 22, 2024 at 12:44:15PM +0300, Matti Vaittinen wrote: > On 9/21/24 23:07, Vasileios Amoiridis wrote: > > On Sat, Sep 21, 2024 at 12:23:39PM -0700, Lars-Peter Clausen wrote: > > > On 9/21/24 11:19, Vasileios Amoiridis wrote: > > > > The iio_validate_own_trigger() function was added in this commit [1] but it is > > > > the same with the below function called iio_trigger_validate_own_device(). The > > > > bodies of the functions can be found in [2], [3]. > > > > > > > > [1]: https://lore.kernel.org/all/51cd3e3e74a6addf8d333f4a109fb9c5a11086ee.1683541225.git.mazziesaccount@gmail.com/ > > > > [2]: https://elixir.bootlin.com/linux/v6.11/source/drivers/iio/industrialio-trigger.c#L732 > > > > [3]: https://elixir.bootlin.com/linux/v6.11/source/drivers/iio/industrialio-trigger.c#L752 > > > > > > The signature of the two functions are different, the order of the > > > parameters is switched. So you can't just swap them out for the > > > `validate_trigger` callback since the signature is not compatible. But maybe > > > you can update the implementation of one of the functions to calling the > > > other function. > > > > > > > Hi Lars, > > > > Hmm, I see what you mean. Still though, do you think that we could do some > > cleaning here? I can see 3 approaches: > > > > 1) One of the 2 functions calls the other internally and nothing else has > > to change. > > I would go with this. Changing the signatures to be the same would be (in > my, not always humble enough, opinion) wrong. The different order of > parameters reflects the different idea. One checks if device for trigger is > the right one, the other checks if the trigger for the device is the right > one. Thus, the order of parameters should be different. > > Calling the same implementation internally is fine with me. Maybe Jonathan > will share his opinion when recovers from all the plumbing in Vienna ;) > > Yours, > -- Matti > > -- > Matti Vaittinen > Linux kernel developer at ROHM Semiconductors > Oulu Finland > Hi Matti! Thanks for your comment! Well, I still think in my eyes is better to have one function do one thing instead of multiple. Also, I didn't think of this argument with the order of arguments, it makes sense. My experience is quite limited to how things should be in such a large project so I trust your opinion. I would still like to see what Jonathan has to say on this though, maybe he had some reasoning behind!!! Have a nice day! Cheers, Vasilis