Hi Dmitry, On Thu, Feb 14, 2013 at 9:31 AM, Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> wrote: > On Wed, Feb 13, 2013 at 10:45:07PM -0800, Simon Glass wrote: >> >> >> >> +config KEYBOARD_CROS_EC >> >> + tristate "ChromeOS EC keyboard" >> >> + select INPUT_MATRIXKMAP >> >> + select MFD_CROS_EC >> > >> > Is this select safe? I.e. does MFD_CROS_EC depend on anything else? >> >> I'll remove it, since it isn't required, and it's true that it does >> need other things. > > Instead of droppign the dependency completely I think it should "depens > on MFD_CROS_EC" OK, done. > >> >> + >> >> +static void cros_ec_keyb_close(struct input_dev *dev) >> >> +{ >> >> + struct cros_ec_keyb *ckdev = input_get_drvdata(dev); >> >> + >> >> + blocking_notifier_chain_unregister(&ckdev->ec->event_notifier, >> >> + &ckdev->notifier); >> >> + blocking_notifier_chain_unregister(&ckdev->ec->wake_notifier, >> >> + &ckdev->wake_notifier); >> > >> > Why is this done via a notifier instead of regular resume method? >> >> Because we only call the notifer in resume when we were not waking on >> a keyboard event. We use it to flush the keyboard. It was a late >> change so there might be a better way, but this driver does not have a >> resume handler. > > Right and the question is why does not it have resume handler and why > you inventing your own resume infrastructure instead of using the > standard one. I will fix that. > >> >> + >> >> +static int cros_ec_keyb_probe(struct platform_device *pdev) >> >> +{ >> >> + struct cros_ec_device *ec = dev_get_drvdata(pdev->dev.parent); >> >> + struct device *dev = ec->dev; >> >> + struct cros_ec_keyb *ckdev = NULL; >> >> + struct input_dev *idev = NULL; >> >> + struct device_node *np; >> >> + int err; >> >> + >> >> + np = of_find_matching_node(NULL, cros_ec_kbc_of_match); >> > >> > And if we don't find it? >> >> Added error checking. >> >> > >> >> + >> >> + ckdev = kzalloc(sizeof(*ckdev), GFP_KERNEL); >> >> + if (!ckdev) { >> >> + dev_err(dev, "cannot allocate memory for ckdev\n"); >> >> + return -ENOMEM; >> >> + } >> >> + pdev->dev.of_node = np; >> > >> > Huh? I'd expect the platform device be fully set up (including DT data) >> > before the driver is called. >> >> This is a child of the mfd driver cros_ec, so I don't think that >> works. Or maybe I'm just not sure how to plumb it in so it is >> automatic. Or maybe I just need to add the id to the device info >> below? > > Who creates this device? Whoever does this should set up the > pdev->dev.of_node. This is not this driver's responsibility. > > And then you add the id to the table below and matching is done > automatically. OK I see - it comes from mfd and it is easy enough to make it do the right thing. Thanks for your comments and patience. I will send a new patch. Regards, Simon > > Thanks. > > -- > Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html