| Message ID | 1469472228-58912-1-git-send-email-briannorris@chromium.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Mon, Jul 25, 2016 at 11:43:48AM -0700, Brian Norris wrote: > From: Douglas Anderson <dianders@chromium.org> > > In cros_ec_keyb we stored "dev" in "struct cros_ec_keyb", but this was > the EC's dev pointer and not the keyboard's. Let's clean this up to > make it the keyboard's dev pointer. This could be useful in future > patches but also has the nice effect of changing a few printouts to > include the name of the keyboard device instead of the EC device, which > seems more technically correct. What is the difference in the output? Could you show old vs new example? > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > Signed-off-by: Brian Norris <briannorris@chromium.org> > --- > drivers/input/keyboard/cros_ec_keyb.c | 19 +++++++++---------- > 1 file changed, 9 insertions(+), 10 deletions(-) > > diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c > index 6e48616a3a88..ab1b50152b86 100644 > --- a/drivers/input/keyboard/cros_ec_keyb.c > +++ b/drivers/input/keyboard/cros_ec_keyb.c > @@ -186,7 +186,7 @@ static irqreturn_t cros_ec_keyb_irq(int irq, void *data) > if (ret >= 0) > cros_ec_keyb_process(ckdev, kb_state, ret); > else > - dev_err(ec->dev, "failed to get keyboard state: %d\n", ret); > + dev_err(ckdev->dev, "failed to get keyboard state: %d\n", ret); > > return IRQ_HANDLED; > } > @@ -236,7 +236,7 @@ static void cros_ec_keyb_compute_valid_keys(struct cros_ec_keyb *ckdev) > 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 device *dev = &pdev->dev; > struct cros_ec_keyb *ckdev; > struct input_dev *idev; > struct device_node *np; > @@ -246,23 +246,22 @@ static int cros_ec_keyb_probe(struct platform_device *pdev) > if (!np) > return -ENODEV; > > - ckdev = devm_kzalloc(&pdev->dev, sizeof(*ckdev), GFP_KERNEL); > + ckdev = devm_kzalloc(dev, sizeof(*ckdev), GFP_KERNEL); > if (!ckdev) > return -ENOMEM; > - err = matrix_keypad_parse_of_params(&pdev->dev, &ckdev->rows, > - &ckdev->cols); > + err = matrix_keypad_parse_of_params(dev, &ckdev->rows, &ckdev->cols); > if (err) > return err; > > - ckdev->valid_keys = devm_kzalloc(&pdev->dev, ckdev->cols, GFP_KERNEL); > + ckdev->valid_keys = devm_kzalloc(dev, ckdev->cols, GFP_KERNEL); > if (!ckdev->valid_keys) > return -ENOMEM; > > - ckdev->old_kb_state = devm_kzalloc(&pdev->dev, ckdev->cols, GFP_KERNEL); > + ckdev->old_kb_state = devm_kzalloc(dev, ckdev->cols, GFP_KERNEL); > if (!ckdev->old_kb_state) > return -ENOMEM; > > - idev = devm_input_allocate_device(&pdev->dev); > + idev = devm_input_allocate_device(dev); > if (!idev) > return -ENOMEM; > > @@ -273,7 +272,7 @@ static int cros_ec_keyb_probe(struct platform_device *pdev) > > ckdev->ec = ec; > ckdev->dev = dev; > - dev_set_drvdata(&pdev->dev, ckdev); > + dev_set_drvdata(dev, ckdev); > > idev->name = CROS_EC_DEV_NAME; > idev->phys = ec->phys_name; > @@ -282,7 +281,7 @@ static int cros_ec_keyb_probe(struct platform_device *pdev) > idev->id.bustype = BUS_VIRTUAL; > idev->id.version = 1; > idev->id.product = 0; > - idev->dev.parent = &pdev->dev; > + idev->dev.parent = dev; > idev->open = cros_ec_keyb_open; > idev->close = cros_ec_keyb_close; > > -- > 2.8.0.rc3.226.g39d4020 >
On Mon, Jul 25, 2016 at 01:31:51PM -0700, Dmitry Torokhov wrote: > On Mon, Jul 25, 2016 at 11:43:48AM -0700, Brian Norris wrote: > > From: Douglas Anderson <dianders@chromium.org> > > > > In cros_ec_keyb we stored "dev" in "struct cros_ec_keyb", but this was > > the EC's dev pointer and not the keyboard's. Let's clean this up to > > make it the keyboard's dev pointer. This could be useful in future > > patches but also has the nice effect of changing a few printouts to > > include the name of the keyboard device instead of the EC device, which > > seems more technically correct. > > What is the difference in the output? Could you show old vs new example? For instance, on a veyron Chromebook, I see this on bootup with v4.7: # dmesg | grep -e cros-ec -e keyb [ 0.539153] cros-ec-spi spi0.0: Chrome EC device registered [ 1.224505] cros-ec-spi spi0.0: valid_keys[00] = 0x14 [ 1.224509] cros-ec-spi spi0.0: valid_keys[01] = 0xff [ 1.224514] cros-ec-spi spi0.0: valid_keys[02] = 0xff [ 1.224518] cros-ec-spi spi0.0: valid_keys[03] = 0xff [ 1.224521] cros-ec-spi spi0.0: valid_keys[04] = 0xff [ 1.224525] cros-ec-spi spi0.0: valid_keys[05] = 0xf5 [ 1.224529] cros-ec-spi spi0.0: valid_keys[06] = 0xff [ 1.224533] cros-ec-spi spi0.0: valid_keys[07] = 0xa4 [ 1.224536] cros-ec-spi spi0.0: valid_keys[08] = 0xff [ 1.224540] cros-ec-spi spi0.0: valid_keys[09] = 0xfe [ 1.224544] cros-ec-spi spi0.0: valid_keys[10] = 0x55 [ 1.224548] cros-ec-spi spi0.0: valid_keys[11] = 0xfa [ 1.224551] cros-ec-spi spi0.0: valid_keys[12] = 0xca [ 1.224632] input: cros_ec as /devices/platform/ff110000.spi/spi_master/spi0/spi0.0/ff110000.spi:ec@0:keyboard-controller/input/input0 and this, with this patch applied: # dmesg | grep -e cros-ec -e keyb [ 0.539437] cros-ec-spi spi0.0: Chrome EC device registered [ 1.224648] cros-ec-keyb ff110000.spi:ec@0:keyboard-controller: valid_keys[00] = 0x14 [ 1.224653] cros-ec-keyb ff110000.spi:ec@0:keyboard-controller: valid_keys[01] = 0xff [ 1.224657] cros-ec-keyb ff110000.spi:ec@0:keyboard-controller: valid_keys[02] = 0xff [ 1.224662] cros-ec-keyb ff110000.spi:ec@0:keyboard-controller: valid_keys[03] = 0xff [ 1.224666] cros-ec-keyb ff110000.spi:ec@0:keyboard-controller: valid_keys[04] = 0xff [ 1.224670] cros-ec-keyb ff110000.spi:ec@0:keyboard-controller: valid_keys[05] = 0xf5 [ 1.224674] cros-ec-keyb ff110000.spi:ec@0:keyboard-controller: valid_keys[06] = 0xff [ 1.224678] cros-ec-keyb ff110000.spi:ec@0:keyboard-controller: valid_keys[07] = 0xa4 [ 1.224682] cros-ec-keyb ff110000.spi:ec@0:keyboard-controller: valid_keys[08] = 0xff [ 1.224686] cros-ec-keyb ff110000.spi:ec@0:keyboard-controller: valid_keys[09] = 0xfe [ 1.224690] cros-ec-keyb ff110000.spi:ec@0:keyboard-controller: valid_keys[10] = 0x55 [ 1.224694] cros-ec-keyb ff110000.spi:ec@0:keyboard-controller: valid_keys[11] = 0xfa [ 1.224698] cros-ec-keyb ff110000.spi:ec@0:keyboard-controller: valid_keys[12] = 0xca [ 1.224779] input: cros_ec as /devices/platform/ff110000.spi/spi_master/spi0/spi0.0/ff110000.spi:ec@0:keyboard-controller/input/input (both are done with an added '#define DEBUG', since that's easier than fiddling with dynamic debug prints) I think it's important to see the "cros-ec-keyb" driver name, and the specific device name is a nice bonus. Do you want to paste something like this into the commit description, or should I resend? Rebards, Brian -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 25, 2016 at 01:46:05PM -0700, Brian Norris wrote: > On Mon, Jul 25, 2016 at 01:31:51PM -0700, Dmitry Torokhov wrote: > > On Mon, Jul 25, 2016 at 11:43:48AM -0700, Brian Norris wrote: > > > From: Douglas Anderson <dianders@chromium.org> > > > > > > In cros_ec_keyb we stored "dev" in "struct cros_ec_keyb", but this was > > > the EC's dev pointer and not the keyboard's. Let's clean this up to > > > make it the keyboard's dev pointer. This could be useful in future > > > patches but also has the nice effect of changing a few printouts to > > > include the name of the keyboard device instead of the EC device, which > > > seems more technically correct. > > > > What is the difference in the output? Could you show old vs new example? > > For instance, on a veyron Chromebook, I see this on bootup with v4.7: > > # dmesg | grep -e cros-ec -e keyb > [ 0.539153] cros-ec-spi spi0.0: Chrome EC device registered > [ 1.224505] cros-ec-spi spi0.0: valid_keys[00] = 0x14 > [ 1.224509] cros-ec-spi spi0.0: valid_keys[01] = 0xff > [ 1.224514] cros-ec-spi spi0.0: valid_keys[02] = 0xff > [ 1.224518] cros-ec-spi spi0.0: valid_keys[03] = 0xff > [ 1.224521] cros-ec-spi spi0.0: valid_keys[04] = 0xff > [ 1.224525] cros-ec-spi spi0.0: valid_keys[05] = 0xf5 > [ 1.224529] cros-ec-spi spi0.0: valid_keys[06] = 0xff > [ 1.224533] cros-ec-spi spi0.0: valid_keys[07] = 0xa4 > [ 1.224536] cros-ec-spi spi0.0: valid_keys[08] = 0xff > [ 1.224540] cros-ec-spi spi0.0: valid_keys[09] = 0xfe > [ 1.224544] cros-ec-spi spi0.0: valid_keys[10] = 0x55 > [ 1.224548] cros-ec-spi spi0.0: valid_keys[11] = 0xfa > [ 1.224551] cros-ec-spi spi0.0: valid_keys[12] = 0xca > [ 1.224632] input: cros_ec as /devices/platform/ff110000.spi/spi_master/spi0/spi0.0/ff110000.spi:ec@0:keyboard-controller/input/input0 > > and this, with this patch applied: > > # dmesg | grep -e cros-ec -e keyb > [ 0.539437] cros-ec-spi spi0.0: Chrome EC device registered > [ 1.224648] cros-ec-keyb ff110000.spi:ec@0:keyboard-controller: valid_keys[00] = 0x14 > [ 1.224653] cros-ec-keyb ff110000.spi:ec@0:keyboard-controller: valid_keys[01] = 0xff > [ 1.224657] cros-ec-keyb ff110000.spi:ec@0:keyboard-controller: valid_keys[02] = 0xff > [ 1.224662] cros-ec-keyb ff110000.spi:ec@0:keyboard-controller: valid_keys[03] = 0xff > [ 1.224666] cros-ec-keyb ff110000.spi:ec@0:keyboard-controller: valid_keys[04] = 0xff > [ 1.224670] cros-ec-keyb ff110000.spi:ec@0:keyboard-controller: valid_keys[05] = 0xf5 > [ 1.224674] cros-ec-keyb ff110000.spi:ec@0:keyboard-controller: valid_keys[06] = 0xff > [ 1.224678] cros-ec-keyb ff110000.spi:ec@0:keyboard-controller: valid_keys[07] = 0xa4 > [ 1.224682] cros-ec-keyb ff110000.spi:ec@0:keyboard-controller: valid_keys[08] = 0xff > [ 1.224686] cros-ec-keyb ff110000.spi:ec@0:keyboard-controller: valid_keys[09] = 0xfe > [ 1.224690] cros-ec-keyb ff110000.spi:ec@0:keyboard-controller: valid_keys[10] = 0x55 > [ 1.224694] cros-ec-keyb ff110000.spi:ec@0:keyboard-controller: valid_keys[11] = 0xfa > [ 1.224698] cros-ec-keyb ff110000.spi:ec@0:keyboard-controller: valid_keys[12] = 0xca > [ 1.224779] input: cros_ec as /devices/platform/ff110000.spi/spi_master/spi0/spi0.0/ff110000.spi:ec@0:keyboard-controller/input/input > > (both are done with an added '#define DEBUG', since that's easier than fiddling > with dynamic debug prints) > > I think it's important to see the "cros-ec-keyb" driver name, and the > specific device name is a nice bonus. > > Do you want to paste something like this into the commit description, or > should I resend? Yes, I munged the commit message a tiny bit, thank you.
diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c index 6e48616a3a88..ab1b50152b86 100644 --- a/drivers/input/keyboard/cros_ec_keyb.c +++ b/drivers/input/keyboard/cros_ec_keyb.c @@ -186,7 +186,7 @@ static irqreturn_t cros_ec_keyb_irq(int irq, void *data) if (ret >= 0) cros_ec_keyb_process(ckdev, kb_state, ret); else - dev_err(ec->dev, "failed to get keyboard state: %d\n", ret); + dev_err(ckdev->dev, "failed to get keyboard state: %d\n", ret); return IRQ_HANDLED; } @@ -236,7 +236,7 @@ static void cros_ec_keyb_compute_valid_keys(struct cros_ec_keyb *ckdev) 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 device *dev = &pdev->dev; struct cros_ec_keyb *ckdev; struct input_dev *idev; struct device_node *np; @@ -246,23 +246,22 @@ static int cros_ec_keyb_probe(struct platform_device *pdev) if (!np) return -ENODEV; - ckdev = devm_kzalloc(&pdev->dev, sizeof(*ckdev), GFP_KERNEL); + ckdev = devm_kzalloc(dev, sizeof(*ckdev), GFP_KERNEL); if (!ckdev) return -ENOMEM; - err = matrix_keypad_parse_of_params(&pdev->dev, &ckdev->rows, - &ckdev->cols); + err = matrix_keypad_parse_of_params(dev, &ckdev->rows, &ckdev->cols); if (err) return err; - ckdev->valid_keys = devm_kzalloc(&pdev->dev, ckdev->cols, GFP_KERNEL); + ckdev->valid_keys = devm_kzalloc(dev, ckdev->cols, GFP_KERNEL); if (!ckdev->valid_keys) return -ENOMEM; - ckdev->old_kb_state = devm_kzalloc(&pdev->dev, ckdev->cols, GFP_KERNEL); + ckdev->old_kb_state = devm_kzalloc(dev, ckdev->cols, GFP_KERNEL); if (!ckdev->old_kb_state) return -ENOMEM; - idev = devm_input_allocate_device(&pdev->dev); + idev = devm_input_allocate_device(dev); if (!idev) return -ENOMEM; @@ -273,7 +272,7 @@ static int cros_ec_keyb_probe(struct platform_device *pdev) ckdev->ec = ec; ckdev->dev = dev; - dev_set_drvdata(&pdev->dev, ckdev); + dev_set_drvdata(dev, ckdev); idev->name = CROS_EC_DEV_NAME; idev->phys = ec->phys_name; @@ -282,7 +281,7 @@ static int cros_ec_keyb_probe(struct platform_device *pdev) idev->id.bustype = BUS_VIRTUAL; idev->id.version = 1; idev->id.product = 0; - idev->dev.parent = &pdev->dev; + idev->dev.parent = dev; idev->open = cros_ec_keyb_open; idev->close = cros_ec_keyb_close;