| 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
On Sun, 22 Sep 2024 13:07:21 +0200 Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > 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!!! > No to changing the signatures. It removes the difference in meaning of the callbacks even though they happen to have the same implementation in this very simple (and common case). In the trigger first one, that is the subject. We are asking the question 'is this trigger ok being used for this device'. In the other the device is the subject and we asking the question 'is this device ok to use this trigger' When we are checking the combination you have here, sure they become the same thing but there are devices where it matters that the trigger is not used to drive other devices (typically because it's a hardware line that goes nowhere else, so no interrupts etc) but other triggers can be used to drive this device (often by software triggering the scan). We have the opposite case as well but that's often a shortcut when it just happens to be really complex to get the trigger to reset (often requires reading all the data or similar) - that condition can almost always be relaxed but sometimes it's a lot of code for a niche case. So fine to change the implementation of one of these checks on tightly coupled device and trigger to call the other but don't touch the callback signatures as to that breaks the logical parameter ordering. Jonathan > Have a nice day! > > Cheers, > Vasilis
On Sat, Sep 28, 2024 at 03:55:19PM +0100, Jonathan Cameron wrote: > On Sun, 22 Sep 2024 13:07:21 +0200 > Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > > > 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!!! > > > No to changing the signatures. It removes the difference > in meaning of the callbacks even though they happen to have > the same implementation in this very simple (and common case). > > In the trigger first one, that is the subject. We are asking the > question 'is this trigger ok being used for this device'. > In the other the device is the subject and we asking the > question 'is this device ok to use this trigger' > > When we are checking the combination you have here, sure they > become the same thing but there are devices where it > matters that the trigger is not used to drive other devices > (typically because it's a hardware line that goes nowhere > else, so no interrupts etc) but other triggers can be used > to drive this device (often by software triggering the scan). > We have the opposite case as well but that's often > a shortcut when it just happens to be really complex to get > the trigger to reset (often requires reading all the data > or similar) - that condition can almost always be relaxed > but sometimes it's a lot of code for a niche case. > > So fine to change the implementation of one of these > checks on tightly coupled device and trigger to call the other > but don't touch the callback signatures as to that breaks the > logical parameter ordering. > > Jonathan > Hi Jonathan, Thank you very much for the explanation, it makes total sense. No need to change everything I think, it is a very small thing and maybe even better like how it is now from what I understand. Cheers, Vasilis > > Have a nice day! > > > > Cheers, > > Vasilis >