Re: Re: [PATCH 2/2] misc: add driver support for MAX8997 MUIC

Re: Re: [PATCH 2/2] misc: add driver support for MAX8997 MUIC

[MyungJoo Ham]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=euc-kr, Size: 1250 bytes --]

Sender : Andrew Morton<akpm@linux-foundation.org>
Date : 2011-11-29 08:48 (GMT+09:00)
Title : Re: [PATCH 2/2] misc: add driver support for MAX8997 MUIC
> 
> On Thu, 24 Nov 2011 18:12:18 +0900
> Donggeun Kim wrote:
> >
> > ...
> >
> > +static irqreturn_t max8997_muic_irq_handler(int irq, void *data)
> > +{
> > + struct max8997_muic_info *info = data;
> > +
> > + dev_dbg(info->dev, "irq:%d ", irq);
> > + info->irq = irq;
> > +
> > + schedule_work(&info->irq_work);
> > +
> > + return IRQ_HANDLED;
> > +}
> 
> The code does schedule_work() but I see no flush_work().  Is it
> possible for the work to still be pending after device shutdown, or
> even after rmmod?
> 

Yes, it is possible that the work is still pending or running and use data that are freed by max8997_muic_remove().

To be safe with the last-minute interrupt handling, we may need to add flush_work or cancel_work_sync at max8997_muic_remove().

> 

 MyungJoo Ham (ÇÔ¸íÁÖ)

Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: +82-10-6714-2858 / office: +82-31-279-8033
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: Re: [PATCH 2/2] misc: add driver support for MAX8997 MUIC

[MyungJoo Ham]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=euc-kr, Size: 2239 bytes --]

> On Monday 12 December 2011, Samuel Ortiz wrote:
> > On Thu, Nov 24, 2011 at 06:12:18PM +0900, Donggeun Kim wrote:
> > > The MUIC function in MAX8997 device can be used as
> > > a USB port detector and switch.
> > > This patch supports the MUIC feature of MAX8997.
> > 
> > I'm queueing this one to my for-next branch as it depends on your previous MFD
> > patch.
> > I'm also cc'ing Arnd in case he has some objections.
> 
> Thanks for the notification!
> 
> First of all, every new user space interface including new sysfs files
> and uevent messages needs documentation in the Documentation/ABI
> directory.
> 
> The part that is not clear to me is how this new driver fits in
> with the first fsa9480 driver and the currently discussed extcon
> subsystem, apparently all written by the same team at Samsung.
> Is this misc driver still useful when the extcon framework gets
> merged?

When extcom gets merged, this driver should be updated. It looks like allowing the interrupt handler to update extcon state would be enough. Bascially, this is all this driver is required for extcon to work. (and the prvious author of extcon--"extended" switch class at that time--was the author of this patch.)

Anyway, this patch has just told me that I'll need to add more sysfs entries for extcon to show the list of cable names and/or to show the status for each of them, soon or later.

> 
> I very much believe that we should avoid introducing user interfaces
> in a driver specific way when we have multiple pieces of hardware
> that try to do the same thing. The extcon subsystem seems to handle
> this correctly on the high-level (I have not done a detailed
> review yet but I trust that it will be ok), so I don't understand
> why we want another driver for the MAX8997 MUIC.

This sysfs entry is a bit confusing to me (that userspace may update this CONTROL register). I'll consult Donggeun when he arrives and will write again about this one. If it is not writable, this is supported by extcon framework, so this sysfs entry will disappear adapting extcon.


Cheers!
MyungJoo
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: Re: [PATCH 2/2] misc: add driver support for MAX8997 MUIC

[함명주]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=euc-kr, Size: 2882 bytes --]

Samuel Ortiz<sameo@linux.intel.com> wrote:
> Hi Arnd,
> On Thu, Dec 15, 2011 at 03:34:01PM +0000, Arnd Bergmann wrote:
> > On Thursday 15 December 2011, MyungJoo Ham wrote:
> > > Arnd Bergmann wrote:
> > > > On Monday 12 December 2011, Samuel Ortiz wrote:
> > > > The part that is not clear to me is how this new driver fits in
> > > > with the first fsa9480 driver and the currently discussed extcon
> > > > subsystem, apparently all written by the same team at Samsung.
> > > > Is this misc driver still useful when the extcon framework gets
> > > > merged?
> > > 
> > > When extcom gets merged, this driver should be updated. It looks like allowing
> > > the interrupt handler to update extcon state would be enough. Bascially, this
> > > is all this driver is required for extcon to work. (and the prvious author of
> > > extcon--"extended" switch class at that time--was the author of this patch.)
> > > 
> > > Anyway, this patch has just told me that I'll need to add more sysfs entries
> > > for extcon to show the list of cable names and/or to show the status for each
> > > of them, soon or later.
> > 
> > I think it would be better not to merge this driver until the user interface
> > in the extcon framework has been finalized. I don't mind the driver, but I
> > want to prevent user interfaces from appearing in the kernel that we
> > intend to change anyway.
> > 
> > How about dropping the driver from the mfd tree, and adding it into the
> > extcon series for the next posting?
> Sounds good to me, I'll drop it.
> 
> Cheers,
> Samuel.
> 

To wait for extcon get merged and keep the MAX8997-MUIC without any sysfs interfaces or extcon feature, the following patch has been submitted yesterday:

http://lkml.org/lkml/2011/12/15/123

[PATCH] misc: max8997-muic: remove sysfs attribute
> The 'switch' sysfs attribute is used for
> showing and setting the switch state of the device.
> However, setting it from the userspace focibly is not appropriate.
> And showing the state can be handled from
> other places (e.g. external connector) in a unified manner.
> This patch removes the sysfs attribute.
> Plus, uevent messages for interrupts are also removed.
> 
> The initial max8997-muic driver is merged at:
> git://git.kernel.org/pub/scm/linux/kernel/git/sameo/mfd-2.6.git for-next
> 
> Signed-off-by: Donggeun Kim <dg77.kim@samsung.com>
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> Acked-by: Greg Kroah-Hartman <gregkh@suse.de>

We can send the patch (with extcon support) later anyway if it's going to be dropped for now.

And, this MAX8997-MUIC will be the example driver for extcon located at /drivers/extcon/.


Cheers!
MyungJoo.ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥