From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753294AbcIAHPU (ORCPT ); Thu, 1 Sep 2016 03:15:20 -0400 Received: from down.free-electrons.com ([37.187.137.238]:59014 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750813AbcIAHPS (ORCPT ); Thu, 1 Sep 2016 03:15:18 -0400 Subject: Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall To: Jonathan Cameron , Guenter Roeck References: <1469519027-11387-1-git-send-email-quentin.schulz@free-electrons.com> <5277864.qlyRKbC1Zy@ws-stein> <2324970.FJcTUYNoO2@ws-stein> <20160726160410.GE30628@roeck-us.net> <20160815170730.GB32260@roeck-us.net> Cc: Alexander Stein , linux-kernel@vger.kernel.org, jdelvare@suse.com, knaack.h@gmx.de, lars@metafoo.de, pmeerw@pmeerw.net, maxime.ripard@free-electrons.com, wens@csie.org, lee.jones@linaro.org, linux-hwmon@vger.kernel.org, linux-iio@vger.kernel.org, linux-arm-kernel@lists.infradead.org, thomas.petazzoni@free-electrons.com, antoine.tenart@free-electrons.com From: Quentin Schulz Message-ID: <3164bade-8022-8ad1-e327-f183128a3e88@free-electrons.com> Date: Thu, 1 Sep 2016 09:15:04 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 15/08/2016 23:35, Jonathan Cameron wrote: > > > On 15 August 2016 18:07:30 BST, Guenter Roeck wrote: >> On Mon, Aug 15, 2016 at 04:40:21PM +0100, Jonathan Cameron wrote: >>> On 26/07/16 17:04, Guenter Roeck wrote: >>>> On Tue, Jul 26, 2016 at 12:00:33PM +0200, Alexander Stein wrote: >>>>> On Tuesday 26 July 2016 11:33:59, Quentin Schulz wrote: >>>>>> On 26/07/2016 11:05, Alexander Stein wrote: >>>>>>> On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote: >>>>>>>> On 26/07/2016 10:21, Alexander Stein wrote: >>>>>>>>> On Tuesday 26 July 2016 09:43:44, Quentin Schulz wrote: >>>>>>>>>> iio_channel_get_all returns -ENODEV when it cannot find >> either phandles >>>>>>>>>> and >>>>>>>>>> properties in the Device Tree or channels whose >> consumer_dev_name >>>>>>>>>> matches >>>>>>>>>> iio_hwmon in iio_map_list. The iio_map_list is filled in by >> iio drivers >>>>>>>>>> which might be probed after iio_hwmon. >>>>>>>>> >>>>>>>>> Would it work if iio_channel_get_all returning ENODEV is used >> for >>>>>>>>> returning >>>>>>>>> EPROBE_DEFER in iio_channel_get_all? Using late initcalls for >>>>>>>>> driver/device >>>>>>>>> dependencies seems not right for me at this place. >>>>>>>> >>>>>>>> Then what if the iio_channel_get_all is called outside of the >> probe of a >>>>>>>> driver? We'll have to change the error code, things we are >> apparently >>>>>>>> trying to avoid (see v2 patches' discussions). >>>>>>> >>>>>>> Maybe I didn't express my idea enough. I don't want to change >> the behavior >>>>>>> of iio_channel_get_all at all. Just the result evaluation of >>>>>>> iio_channel_get_all in iio_hwmon_probe. I have something link >> the patch >>>>>>> below in mind. >>>>>>> >>>>>>> Best regards, >>>>>>> Alexander >>>>>>> --- >>>>>>> diff --git a/drivers/hwmon/iio_hwmon.c >> b/drivers/hwmon/iio_hwmon.c >>>>>>> index b550ba5..e32d150 100644 >>>>>>> --- a/drivers/hwmon/iio_hwmon.c >>>>>>> +++ b/drivers/hwmon/iio_hwmon.c >>>>>>> @@ -73,8 +73,12 @@ static int iio_hwmon_probe(struct >> platform_device >>>>>>> *pdev) >>>>>>> >>>>>>> name = dev->of_node->name; >>>>>>> >>>>>>> channels = iio_channel_get_all(dev); >>>>>>> >>>>>>> - if (IS_ERR(channels)) >>>>>>> - return PTR_ERR(channels); >>>>>>> + if (IS_ERR(channels)) { >>>>>>> + if (PTR_ERR(channels) == -ENODEV) >>>>>>> + return -EPROBE_DEFER; >>>>>>> + else >>>>>>> + return PTR_ERR(channels); >>>>>>> + } >>>>>>> >>>>>>> st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL); >>>>>>> if (st == NULL) { >>>>>> >>>>>> Indeed, I misunderstood what you told me. >>>>>> >>>>>> Actually, the patch you proposed is part of my v1 >>>>>> (https://lkml.org/lkml/2016/6/28/203) and v2 >>>>>> (https://lkml.org/lkml/2016/7/15/215). >>>>>> Jonathan and Guenter didn't really like the idea of changing the >> -ENODEV >>>>>> in -EPROBE_DEFER. >>>>> >>>>> Thanks for the links. >>>>> >>>>>> What I thought you were proposing was to change the -ENODEV >> return code >>>>>> inside iio_channel_get_all. This cannot be an option since the >> function >>>>>> might be called outside of a probe (it is not yet, but might be >> in the >>>>>> future?). >>>>> >>>>> AFAICS this is a helper function not knowing about device probing >> itself. And >>>>> it should stay at that. >>>>> >>>>>> Of what I understood, two possibilities are then possible >> (proposed >>>>>> either by Guenter or Jonathan): either rework the iio framework >> to >>>>>> register iio map array earlier or to use late_initcall instead of >> init >>>>>> for the driver consuming the iio channels. >>>>> >>>>> Interestingly using this problem would not arise due to module >> dependencies. >>>>> But using late_initcall would mean this needs to be done on any >> driver using >>>>> iio channels? I would rather keep those consumers simple. >>>>> >>>> Me too, but that would imply a solution in iio. The change you >> propose above >>>> isn't exactly simple either, and would also be needed in each >> consumer driver. >>>> >>>> Just for the record, I dislike the late_initcall solution as well, >> but I prefer >>>> it over blindly converting ENODEV to EPROBE_DEFER. >>> I'm falling on the other side on this one right now. Though I'd be >> tempted >>> to renaming the function to something like >> iio_channel_get_all_or_defer >>> to make it explicit that it can result in deferred probing. >>> >> Would this new function return -EPROBE_DEFER instead of -ENODEV ? > Yes. Though whether it really adds much over doing that in drivers isn't clear. > > Hmm. Needs more thought... Either we do the exact same "hack" as in the v2[1] in what you call iio_channel_get_all_or_defer or we duplicate the code from iio_channel_get_all in iio_channel_get_all_or_defer. Both do not seem right to me but I really dislike the late_initcall method. With this method we can only have one level of "channel dependency". This means if we ever create a new driver which depends on channels from the driver using late_initcall, we will also have to use late_initcall and we can't be sure the new driver will always be probed after the driver he depends on. [1] https://lkml.org/lkml/2016/7/15/215 Quentin >> >> Thanks, >> Guenter >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-iio" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >