From: Christopher Heiny <cheiny@synaptics.com>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Linux Input <linux-input@vger.kernel.org>,
Andrew Duggan <aduggan@synaptics.com>,
Vincent Huang <Vincent.huang@tw.synaptics.com>,
Vivian Ly <vly@synaptics.com>,
Daniel Rosenberg <daniel.rosenberg@synaptics.com>,
Jean Delvare <khali@linux-fr.org>,
Joerie de Gram <j.de.gram@gmail.com>,
Linus Walleij <linus.walleij@stericsson.com>,
Benjamin Tissoires <benjamin.tissoires@redhat.com>
Subject: Re: [PATCH] input: synaptics-rmi4 - Count IRQs before creating functions; save F01 container.
Date: Wed, 5 Feb 2014 17:28:24 -0800 [thread overview]
Message-ID: <52F2E538.1030502@synaptics.com> (raw)
In-Reply-To: <20140129225241.GA14709@core.coreip.homeip.net>
On 01/29/2014 02:52 PM, Dmitry Torokhov wrote:
> On Wed, Jan 29, 2014 at 10:30:48PM +0000, Christopher Heiny wrote:
>> Sorry for the delay on this. The mail problems from earlier this week continue to plague me.
>>
>> On 01/26/2014 10:36 PM, Dmitry Torokhov wrote:
>>> Hi Christopher,
>>>
>>> On Fri, Jan 10, 2014 at 01:53:34PM -0800, Christopher Heiny wrote:
>>>>
>>>> err_free_data:
>>>> + rmi_free_function_list(rmi_dev);
>>>> + if (gpio_is_valid(pdata->attn_gpio))
>>>> + gpio_free(pdata->attn_gpio);
>>>> + devm_kfree(&rmi_dev->dev, data->irq_status);
>>>> + devm_kfree(&rmi_dev->dev, data->current_irq_mask);
>>>> + devm_kfree(&rmi_dev->dev, data->irq_mask_store);
>>>> + devm_kfree(&rmi_dev->dev, data);
>>>
>>> It is rarely makes sense to explicitly free devm-managed data. In
>>> general I find that RMI code is very confused about when devm-managed
>>> resources are released (they only released after failed probe or remove,
>>> but we use devm_kzalloc to allocate function's interrupt masks during
>>> device creation and they will get freed only when parent unbinds, etc).
>>
>
>> Yeah, we've gotten a metric ton of confusing advice/recommendations
>> regarding devm_* (most of it offline from linux-input) and it shows.
>> At one point I was pretty much ready to just bag it all and write our
>> own garbage collecting storage manager, but figured that would be
>> unlikely to make it upstream :-)
>
> Yeah, some people see mistake screws for nails when they get a hold of a
> hammer. Managed resources are nice as long as you have clear
> understanding on what happens.
>
>>
>>> Given that you mentioned firmware flash in the work and I expect we'll
>>> be destroying and re-creating functions and other data structures at
>>> will I think we should limit use of devm APIs so that lifetime
>>> management is explicit and clear.
>>
>> Sounds good.
>>
>>> I tried adjusting the patch so that it works with the version of PDT
>>> cleanup patch I posted a few minutes ago, please let me know what you
>>> think.
>>
>> There's some comments below. After making those changes, I've applied this to my test tree, and it works well.
>>
>> I can send updated versions of your two patches, if you'd like.
>
> Yes, please, I always prefer applying something that was tested. You
> can also sent more of the pending stuff my way, no need to limit to 1
> patch at a time - I usually able to cherry-pick patches that I do not
> have questions about and we can work on ones that I need some
> clarification on. Just don't mailbomb me with 100 ;)
OK - we'll have some more on the way in a bit.
>>> +static int rmi_check_bootloader_mode(struct rmi_device *rmi_dev,
>>> + const struct pdt_entry *pdt)
>>> +{
>>> + int error;
>>> + u8 device_status;
>>> +
>>> + error = rmi_read(rmi_dev, pdt->data_base_addr + pdt->page_start,
>>> + &device_status);
>>
>> Since this is applied after your previous patch, then this statement should be:
>> error = rmi_read(rmi_dev, pdt->data_base_addr, &device_status);
>
> Hmm, I did not think I adjusted data_base_addr in PDT, I only stored the
> page start in there... The updated addresses as in function
> structures.
I went back and double checked. This was correct, it was page_start
that was bogus. Yesterday's patch fixed that.
prev parent reply other threads:[~2014-02-06 1:28 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-01-10 21:53 [PATCH] input: synaptics-rmi4 - Count IRQs before creating functions; save F01 container Christopher Heiny
2014-01-27 6:36 ` Dmitry Torokhov
2014-01-29 22:30 ` Christopher Heiny
2014-01-29 22:52 ` Dmitry Torokhov
2014-02-06 1:28 ` Christopher Heiny [this message]
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=52F2E538.1030502@synaptics.com \
--to=cheiny@synaptics.com \
--cc=Vincent.huang@tw.synaptics.com \
--cc=aduggan@synaptics.com \
--cc=benjamin.tissoires@redhat.com \
--cc=daniel.rosenberg@synaptics.com \
--cc=dmitry.torokhov@gmail.com \
--cc=j.de.gram@gmail.com \
--cc=khali@linux-fr.org \
--cc=linus.walleij@stericsson.com \
--cc=linux-input@vger.kernel.org \
--cc=vly@synaptics.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).