| Message ID | 20231211165708.161808-3-biju.das.jz@bp.renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add polling support for DA9063 onkey driver | expand |
On 12/11/23 7:57 PM, Biju Das wrote: > Replace dev_err()->dev_err_probe() to simpilfy probe(). Simplify. :-) > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> [...] MBR, Sergey
Hi Biju, On Mon, Dec 11, 2023 at 5:57 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > Replace dev_err()->dev_err_probe() to simpilfy probe(). > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> Thanks for your patch! > --- a/drivers/input/misc/da9063_onkey.c > +++ b/drivers/input/misc/da9063_onkey.c > @@ -185,10 +185,9 @@ static int da9063_onkey_probe(struct platform_device *pdev) > > onkey = devm_kzalloc(&pdev->dev, sizeof(struct da9063_onkey), > GFP_KERNEL); > - if (!onkey) { > - dev_err(&pdev->dev, "Failed to allocate memory.\n"); > - return -ENOMEM; > - } > + if (!onkey) > + return dev_err_probe(&pdev->dev, -ENOMEM, > + "Failed to allocate memory.\n"); Please drop the error printing instead, the memory allocation core code already takes care of that in case of OOM. > > onkey->config = device_get_match_data(&pdev->dev); > if (!onkey->config) > @@ -197,19 +196,17 @@ static int da9063_onkey_probe(struct platform_device *pdev) > onkey->dev = &pdev->dev; > > onkey->regmap = dev_get_regmap(pdev->dev.parent, NULL); > - if (!onkey->regmap) { > - dev_err(&pdev->dev, "Parent regmap unavailable.\n"); > - return -ENXIO; > - } > + if (!onkey->regmap) > + return dev_err_probe(&pdev->dev, -ENXIO, > + "Parent regmap unavailable.\n"); > > onkey->key_power = !of_property_read_bool(pdev->dev.of_node, > "dlg,disable-key-power"); > > onkey->input = devm_input_allocate_device(&pdev->dev); > - if (!onkey->input) { > - dev_err(&pdev->dev, "Failed to allocated input device.\n"); > - return -ENOMEM; > - } > + if (!onkey->input) > + return dev_err_probe(&pdev->dev, -ENOMEM, > + "Failed to allocated input device.\n"); devm_input_allocate_device() only fails on OOM, so no need to print anything. > > onkey->input->name = onkey->config->name; > snprintf(onkey->phys, sizeof(onkey->phys), "%s/input0", > @@ -221,12 +218,9 @@ static int da9063_onkey_probe(struct platform_device *pdev) > > error = devm_delayed_work_autocancel(&pdev->dev, &onkey->work, > da9063_poll_on); > - if (error) { > - dev_err(&pdev->dev, > - "Failed to add cancel poll action: %d\n", > - error); > - return error; > - } > + if (error) > + return dev_err_probe(&pdev->dev, error, > + "Failed to add cancel poll action\n"); devm_delayed_work_autocancel() only fails on OOM, so no need to print anything. > > irq = platform_get_irq_byname(pdev, "ONKEY"); > if (irq < 0) > @@ -236,11 +230,9 @@ static int da9063_onkey_probe(struct platform_device *pdev) > NULL, da9063_onkey_irq_handler, > IRQF_TRIGGER_LOW | IRQF_ONESHOT, > "ONKEY", onkey); > - if (error) { > - dev_err(&pdev->dev, > - "Failed to request IRQ %d: %d\n", irq, error); > - return error; > - } > + if (error) > + return dev_err_probe(&pdev->dev, error, > + "Failed to request IRQ %d\n", irq); platform_get_irq_byname() already prints an error message on failure. > > error = dev_pm_set_wake_irq(&pdev->dev, irq); > if (error) > @@ -251,11 +243,9 @@ static int da9063_onkey_probe(struct platform_device *pdev) > device_init_wakeup(&pdev->dev, true); > > error = input_register_device(onkey->input); > - if (error) { > - dev_err(&pdev->dev, > - "Failed to register input device: %d\n", error); > - return error; > - } > + if (error) > + return dev_err_probe(&pdev->dev, error, > + "Failed to register input device\n"); Looks like all non-OOM failure paths in input_register_device() already print an error message, too... Gr{oetje,eeting}s, Geert
Hi Geert, Thanks for the feedback. > -----Original Message----- > From: Geert Uytterhoeven <geert@linux-m68k.org> > Sent: Tuesday, December 12, 2023 8:02 AM > Subject: Re: [PATCH 2/3] Input: da9063 - Use dev_err_probe() > > Hi Biju, > > On Mon, Dec 11, 2023 at 5:57 PM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > Replace dev_err()->dev_err_probe() to simpilfy probe(). > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Thanks for your patch! > > > --- a/drivers/input/misc/da9063_onkey.c > > +++ b/drivers/input/misc/da9063_onkey.c > > @@ -185,10 +185,9 @@ static int da9063_onkey_probe(struct > > platform_device *pdev) > > > > onkey = devm_kzalloc(&pdev->dev, sizeof(struct da9063_onkey), > > GFP_KERNEL); > > - if (!onkey) { > > - dev_err(&pdev->dev, "Failed to allocate memory.\n"); > > - return -ENOMEM; > > - } > > + if (!onkey) > > + return dev_err_probe(&pdev->dev, -ENOMEM, > > + "Failed to allocate memory.\n"); > > Please drop the error printing instead, the memory allocation core code > already takes care of that in case of OOM. OK. > > > > > onkey->config = device_get_match_data(&pdev->dev); > > if (!onkey->config) > > @@ -197,19 +196,17 @@ static int da9063_onkey_probe(struct > platform_device *pdev) > > onkey->dev = &pdev->dev; > > > > onkey->regmap = dev_get_regmap(pdev->dev.parent, NULL); > > - if (!onkey->regmap) { > > - dev_err(&pdev->dev, "Parent regmap unavailable.\n"); > > - return -ENXIO; > > - } > > + if (!onkey->regmap) > > + return dev_err_probe(&pdev->dev, -ENXIO, > > + "Parent regmap unavailable.\n"); > > > > onkey->key_power = !of_property_read_bool(pdev->dev.of_node, > > > > "dlg,disable-key-power"); > > > > onkey->input = devm_input_allocate_device(&pdev->dev); > > - if (!onkey->input) { > > - dev_err(&pdev->dev, "Failed to allocated input > device.\n"); > > - return -ENOMEM; > > - } > > + if (!onkey->input) > > + return dev_err_probe(&pdev->dev, -ENOMEM, > > + "Failed to allocated input > > + device.\n"); > > devm_input_allocate_device() only fails on OOM, so no need to print > anything. OK. > > > > > onkey->input->name = onkey->config->name; > > snprintf(onkey->phys, sizeof(onkey->phys), "%s/input0", @@ > > -221,12 +218,9 @@ static int da9063_onkey_probe(struct platform_device > > *pdev) > > > > error = devm_delayed_work_autocancel(&pdev->dev, &onkey->work, > > da9063_poll_on); > > - if (error) { > > - dev_err(&pdev->dev, > > - "Failed to add cancel poll action: %d\n", > > - error); > > - return error; > > - } > > + if (error) > > + return dev_err_probe(&pdev->dev, error, > > + "Failed to add cancel poll > > + action\n"); > > devm_delayed_work_autocancel() only fails on OOM, so no need to print > anything. OK. > > > > > irq = platform_get_irq_byname(pdev, "ONKEY"); > > if (irq < 0) > > @@ -236,11 +230,9 @@ static int da9063_onkey_probe(struct > platform_device *pdev) > > NULL, > da9063_onkey_irq_handler, > > IRQF_TRIGGER_LOW | > IRQF_ONESHOT, > > "ONKEY", onkey); > > - if (error) { > > - dev_err(&pdev->dev, > > - "Failed to request IRQ %d: %d\n", irq, error); > > - return error; > > - } > > + if (error) > > + return dev_err_probe(&pdev->dev, error, > > + "Failed to request IRQ %d\n", > > + irq); > > platform_get_irq_byname() already prints an error message on failure. I will change the print message as " Failed to allocate onkey irq"?? > > > > > error = dev_pm_set_wake_irq(&pdev->dev, irq); > > if (error) > > @@ -251,11 +243,9 @@ static int da9063_onkey_probe(struct > platform_device *pdev) > > device_init_wakeup(&pdev->dev, true); > > > > error = input_register_device(onkey->input); > > - if (error) { > > - dev_err(&pdev->dev, > > - "Failed to register input device: %d\n", error); > > - return error; > > - } > > + if (error) > > + return dev_err_probe(&pdev->dev, error, > > + "Failed to register input > > + device\n"); > > Looks like all non-OOM failure paths in input_register_device() already > print an error message, too... OK, I will send 1) separate patch for dropping unneeded prints related to OOM 2) A patch for replacing dev_err()->dev_err_probe() + Update error message for devm_request_threaded_irq() 3) separate patch for dropping print message for input_register_device(); Is it ok? Cheers, Biju
Hi Biju, On Tue, Dec 12, 2023 at 10:03 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > On Mon, Dec 11, 2023 at 5:57 PM Biju Das <biju.das.jz@bp.renesas.com> > > wrote: > > > Replace dev_err()->dev_err_probe() to simpilfy probe(). > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > OK, I will send > > 1) separate patch for dropping unneeded prints related to OOM > 2) A patch for replacing dev_err()->dev_err_probe() + Update error message for devm_request_threaded_irq() > 3) separate patch for dropping print message for input_register_device(); > > Is it ok? Personally, I'd be fine with just a single "cleanup error printing" patch. But Dmitry has the final say. Gr{oetje,eeting}s, Geert
On December 12, 2023 8:28:45 PM GMT+11:00, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >Hi Biju, > >On Tue, Dec 12, 2023 at 10:03 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: >> > From: Geert Uytterhoeven <geert@linux-m68k.org> >> > On Mon, Dec 11, 2023 at 5:57 PM Biju Das <biju.das.jz@bp.renesas.com> >> > wrote: >> > > Replace dev_err()->dev_err_probe() to simpilfy probe(). >> > > >> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > >> OK, I will send >> >> 1) separate patch for dropping unneeded prints related to OOM >> 2) A patch for replacing dev_err()->dev_err_probe() + Update error message for devm_request_threaded_irq() >> 3) separate patch for dropping print message for input_register_device(); >> >> Is it ok? > >Personally, I'd be fine with just a single "cleanup error printing" patch. >But Dmitry has the final say. I'm fine with a single patch unless you feel strongly about splitting it in 2. Thanks.
Hi Dmitry Torokhov, Thanks for the feedback. > -----Original Message----- > From: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Sent: Wednesday, December 13, 2023 8:49 PM > Subject: Re: [PATCH 2/3] Input: da9063 - Use dev_err_probe() > > On December 12, 2023 8:28:45 PM GMT+11:00, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: > >Hi Biju, > > > >On Tue, Dec 12, 2023 at 10:03 AM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > >> > From: Geert Uytterhoeven <geert@linux-m68k.org> On Mon, Dec 11, > >> > 2023 at 5:57 PM Biju Das <biju.das.jz@bp.renesas.com> > >> > wrote: > >> > > Replace dev_err()->dev_err_probe() to simpilfy probe(). > >> > > > >> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > >> OK, I will send > >> > >> 1) separate patch for dropping unneeded prints related to OOM > >> 2) A patch for replacing dev_err()->dev_err_probe() + Update error > >> message for devm_request_threaded_irq() > >> 3) separate patch for dropping print message for > >> input_register_device(); > >> > >> Is it ok? > > > >Personally, I'd be fine with just a single "cleanup error printing" > patch. > >But Dmitry has the final say. > > I'm fine with a single patch unless you feel strongly about splitting it > in 2. I will split into 2, 1) First patch for dropping redundant print messages. 2) Replace dev_err()->dev_err_probe() Cheers, Biju
diff --git a/drivers/input/misc/da9063_onkey.c b/drivers/input/misc/da9063_onkey.c index 9351ce0bb405..536220662b38 100644 --- a/drivers/input/misc/da9063_onkey.c +++ b/drivers/input/misc/da9063_onkey.c @@ -185,10 +185,9 @@ static int da9063_onkey_probe(struct platform_device *pdev) onkey = devm_kzalloc(&pdev->dev, sizeof(struct da9063_onkey), GFP_KERNEL); - if (!onkey) { - dev_err(&pdev->dev, "Failed to allocate memory.\n"); - return -ENOMEM; - } + if (!onkey) + return dev_err_probe(&pdev->dev, -ENOMEM, + "Failed to allocate memory.\n"); onkey->config = device_get_match_data(&pdev->dev); if (!onkey->config) @@ -197,19 +196,17 @@ static int da9063_onkey_probe(struct platform_device *pdev) onkey->dev = &pdev->dev; onkey->regmap = dev_get_regmap(pdev->dev.parent, NULL); - if (!onkey->regmap) { - dev_err(&pdev->dev, "Parent regmap unavailable.\n"); - return -ENXIO; - } + if (!onkey->regmap) + return dev_err_probe(&pdev->dev, -ENXIO, + "Parent regmap unavailable.\n"); onkey->key_power = !of_property_read_bool(pdev->dev.of_node, "dlg,disable-key-power"); onkey->input = devm_input_allocate_device(&pdev->dev); - if (!onkey->input) { - dev_err(&pdev->dev, "Failed to allocated input device.\n"); - return -ENOMEM; - } + if (!onkey->input) + return dev_err_probe(&pdev->dev, -ENOMEM, + "Failed to allocated input device.\n"); onkey->input->name = onkey->config->name; snprintf(onkey->phys, sizeof(onkey->phys), "%s/input0", @@ -221,12 +218,9 @@ static int da9063_onkey_probe(struct platform_device *pdev) error = devm_delayed_work_autocancel(&pdev->dev, &onkey->work, da9063_poll_on); - if (error) { - dev_err(&pdev->dev, - "Failed to add cancel poll action: %d\n", - error); - return error; - } + if (error) + return dev_err_probe(&pdev->dev, error, + "Failed to add cancel poll action\n"); irq = platform_get_irq_byname(pdev, "ONKEY"); if (irq < 0) @@ -236,11 +230,9 @@ static int da9063_onkey_probe(struct platform_device *pdev) NULL, da9063_onkey_irq_handler, IRQF_TRIGGER_LOW | IRQF_ONESHOT, "ONKEY", onkey); - if (error) { - dev_err(&pdev->dev, - "Failed to request IRQ %d: %d\n", irq, error); - return error; - } + if (error) + return dev_err_probe(&pdev->dev, error, + "Failed to request IRQ %d\n", irq); error = dev_pm_set_wake_irq(&pdev->dev, irq); if (error) @@ -251,11 +243,9 @@ static int da9063_onkey_probe(struct platform_device *pdev) device_init_wakeup(&pdev->dev, true); error = input_register_device(onkey->input); - if (error) { - dev_err(&pdev->dev, - "Failed to register input device: %d\n", error); - return error; - } + if (error) + return dev_err_probe(&pdev->dev, error, + "Failed to register input device\n"); return 0; }
Replace dev_err()->dev_err_probe() to simpilfy probe(). Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- drivers/input/misc/da9063_onkey.c | 46 ++++++++++++------------------- 1 file changed, 18 insertions(+), 28 deletions(-)