From: Alexandru Ardelean <email@example.com> To: Hans de Goede <firstname.lastname@example.org> Cc: Alexandru Ardelean <email@example.com>, firstname.lastname@example.org, LKML <email@example.com>, linux-iio <firstname.lastname@example.org>, email@example.com, firstname.lastname@example.org, Jonathan Cameron <email@example.com>, firstname.lastname@example.org Subject: Re: [PATCH 00/10] platform/x86: toshiba_acpi: move acpi add/remove to device-managed routines Date: Tue, 30 Mar 2021 12:22:04 +0300 [thread overview] Message-ID: <CA+U=Dsq-YDBzSEsRBsTOkzf=1yyfB4esTJMvc6rKWTPff=i1TQ@mail.gmail.com> (raw) In-Reply-To: <email@example.com> On Tue, Mar 30, 2021 at 11:21 AM Hans de Goede <firstname.lastname@example.org> wrote: > > Hi Alexadru, Jonathan, > > On 3/24/21 1:55 PM, Alexandru Ardelean wrote: > > This changeset tries to do a conversion of the toshiba_acpi driver to use > > only device-managed routines. The driver registers as a singleton, so no > > more than one device can be registered at a time. > > > > My main intent here is to try to convert the iio_device_alloc() and > > iio_device_register() to their devm_ variants. > > > > Usually, when converting a registration call to device-managed variant, the > > init order must be preserved. And the deregistration order must be a mirror > > of the registration (in reverse order). > > > > This change tries to do that, by using devm_ variants where available and > > devm_add_action_or_reset() where this isn't possible. > > Some deregistration ordering is changed, because it wasn't exactly > > mirroring (in reverse) the init order. > > > > For the IIO subsystem, the toshiba_acpi driver is the only user of > > iio_device_alloc(). If this changeset is accepted (after discussion), I > > will propose to remove the iio_device_alloc() function. > > > > While I admit this may look like an overzealous effort to use devm_ > > everywhere (in IIO at least), for me it's a fun/interesting excercise. > > Alexadru, thank you for the patches. > > Jonathan, thank you for the reviews. > > To be honest I'm currently inclined to not accept / merge these patches, > this is based on 2 assumptions from me, which might be wrong. let me explain. > > If I understand things correctly, the main reason for this rework of > the toshiba_acpi code is to move iio_device_alloc() and iio_device_register() > to their devm_ variants, converting the last users in the tree ? yes well, we still have plenty of users iio_device_alloc() / iio_device_register() inside drivers/iio but the toshipa_acpi driver is the more quirky user of these functions [treewide] i wanted to jump on those simpler IIO cases, but i thought i would leave those to new contributors [for a while]; the complexity of those conversions is good enough to get some people started to contribute changes that are a bit more useful than checkpatch fixes, comment fixes [etc]; [personally] i feel that these devm_ conversions are complex enough to maybe get people wanting to dig more into some kernel design stuff > > This would allow these 2 iio functions to then be e.g. marked as static / > private helpers inside the iio core, so that all new users can only use > the devm_ versions. But if I'm reading Jonathan's reaction correctly then > Jonathan is not planning to do that because they might still be useful > in some cases. > > Jonathan have I correctly understood that you don't plan to make any > changes to the iio_device_alloc() and iio_device_register() functions > even if this gets merged ? > > Which brings me to my next assumption, Alexandru, I don't read anything > about testing anywhere. So I'm currently under the assumption that > you don't have any hardware using the toshiba_acpi driver and that this > is thus untested ? yes, i don't have any hw to test this > > The not being tested state is my main reason for not wanting to merge > this. The toshiba_acpi driver likely does not have a whole lot of users, > so the chances of someone running release candidates or even just the > latest kernels on hardware which uses it are small. This means that if > we accidentally introduce a bug with this series it might not get caught > until say lots of people start upgrading to Ubuntu 22.04 which is > the first Ubuntu kernel with your changes; and then at least one of the > hit users needs to have the skills to find us and get in contact about that. > > TL;DR: we might break stuff and if we do it might be a long time until we > find out we did and then we have been shipping broken code for ages... ack well, i don't insist in pushing this series; another thought was to just send bits of this set, which are simple enough to consider even on their own; maybe i'll look for a toshiba laptop with support for this stuff; i'll see thanks :) Alex > > Regards, > > Hans > > > > > > > > > Alexandru Ardelean (10): > > platform/x86: toshiba_acpi: bind life-time of toshiba_acpi_dev to > > parent > > platform/x86: toshiba_acpi: use devm_add_action_or_reset() for > > singleton clear > > platform/x86: toshiba_acpi: bind registration of miscdev object to > > parent > > platform/x86: toshiba_acpi: use device-managed functions for input > > device > > platform/x86: toshiba_acpi: register backlight with device-managed > > variant > > platform/x86: toshiba_acpi: use devm_led_classdev_register() for LEDs > > platform/x86: toshiba_acpi: use device-managed functions for > > accelerometer > > platform/x86: toshiba_acpi: use device-managed for wwan_rfkill > > management > > platform/x86: toshiba_acpi: use device-managed for sysfs removal > > platform/x86: toshiba_acpi: bind proc entries creation to parent > > > > drivers/platform/x86/toshiba_acpi.c | 249 +++++++++++++++++----------- > > 1 file changed, 150 insertions(+), 99 deletions(-) > > >
next prev parent reply other threads:[~2021-03-30 9:22 UTC|newest] Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-03-24 12:55 Alexandru Ardelean 2021-03-24 12:55 ` [PATCH 01/10] platform/x86: toshiba_acpi: bind life-time of toshiba_acpi_dev to parent Alexandru Ardelean 2021-03-29 14:30 ` Jonathan Cameron 2021-03-30 6:49 ` Alexandru Ardelean 2021-03-24 12:55 ` [PATCH 02/10] platform/x86: toshiba_acpi: use devm_add_action_or_reset() for singleton clear Alexandru Ardelean 2021-03-29 14:27 ` Jonathan Cameron 2021-03-24 12:55 ` [PATCH 03/10] platform/x86: toshiba_acpi: bind registration of miscdev object to parent Alexandru Ardelean 2021-03-29 14:33 ` Jonathan Cameron 2021-03-24 12:55 ` [PATCH 04/10] platform/x86: toshiba_acpi: use device-managed functions for input device Alexandru Ardelean 2021-03-29 14:48 ` Jonathan Cameron 2021-03-24 12:55 ` [PATCH 05/10] platform/x86: toshiba_acpi: register backlight with device-managed variant Alexandru Ardelean 2021-03-29 14:50 ` Jonathan Cameron 2021-03-24 12:55 ` [PATCH 06/10] platform/x86: toshiba_acpi: use devm_led_classdev_register() for LEDs Alexandru Ardelean 2021-03-29 14:52 ` Jonathan Cameron 2021-03-24 12:55 ` [PATCH 07/10] platform/x86: toshiba_acpi: use device-managed functions for accelerometer Alexandru Ardelean 2021-03-29 14:54 ` Jonathan Cameron 2021-03-24 12:55 ` [PATCH 08/10] platform/x86: toshiba_acpi: use device-managed for wwan_rfkill management Alexandru Ardelean 2021-03-29 14:57 ` Jonathan Cameron 2021-03-24 12:55 ` [PATCH 09/10] platform/x86: toshiba_acpi: use device-managed for sysfs removal Alexandru Ardelean 2021-03-29 15:09 ` Jonathan Cameron 2021-03-24 12:55 ` [PATCH 10/10] platform/x86: toshiba_acpi: bind proc entries creation to parent Alexandru Ardelean 2021-03-29 15:10 ` Jonathan Cameron 2021-03-29 12:38 ` [PATCH 00/10] platform/x86: toshiba_acpi: move acpi add/remove to device-managed routines Jonathan Cameron 2021-03-29 14:01 ` Alexandru Ardelean 2021-03-30 8:20 ` Hans de Goede 2021-03-30 9:22 ` Alexandru Ardelean [this message] 2021-03-30 9:27 ` Hans de Goede
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CA+U=Dsq-YDBzSEsRBsTOkzf=1yyfB4esTJMvc6rKWTPff=i1TQ@mail.gmail.com' \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --subject='Re: [PATCH 00/10] platform/x86: toshiba_acpi: move acpi add/remove to device-managed routines' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).