On Fri, Jan 29, 2021 at 2:47 PM carlis zhangxuezhi3@gmail.com wrote:
On Fri, 29 Jan 2021 12:23:08 +0200 Andy Shevchenko andy.shevchenko@gmail.com wrote:
On Fri, Jan 29, 2021 at 7:01 AM carlis zhangxuezhi3@gmail.com wrote:
On Thu, 28 Jan 2021 16:33:02 +0200 Andy Shevchenko andy.shevchenko@gmail.com wrote:
On Thu, Jan 28, 2021 at 2:58 PM Carlis zhangxuezhi3@gmail.com wrote:
...
Please, go again thru my comments and comments from others and carefully address all of them everywhere in your contribution. If you have questions, ask them in reply in the corresponding context.
...
/**
- init_tearing_effect_line() - init tearing effect line
For example, above was commented on and hasn't been addressed here.
hi,here i can not get you.....
The above is a blank line which is redundant. It also applied to the other function in the code.
- @par: FBTFT parameter object
- Return: 0 on success, < 0 if error occurred.
*/ static int init_tearing_effect_line(struct fbtft_par *par) { struct device *dev = par->info->device; struct gpio_desc *te; int rc;
te = gpiod_get_optional(dev, "te", GPIOD_IN); if (IS_ERR(te)) return dev_err_probe(dev, PTR_ERR(te), "Failed to request te GPIO\n");if (te) {This one is not like I suggested.
Why? My thinking is that if the TE is not configured and NULL is returned, the initialization can still proceed.....
I have suggested to bail out immediately. It will reduce an indentation level on the below code.
par->irq_te = gpiod_to_irq(te); gpiod_put(te);if (par->irq_te) {This is wrong.
Why? i have read gpiod_to_irq code, if an error occurs, a negative value is returned, and zero is not possible,so I need this value to determine if TE IRQ is configured
It returns two possible cases: - error code (when negative) - Linux IRQ number otherwise
You check makes a no-op since in either variant it will proceed to the request of IRQ, which is wrong in an error case.
rc = devm_request_irq(dev, par->irq_te, panel_te_handler, IRQF_TRIGGER_RISING, "TE_GPIO", par);Try to use less LOCs.
if (rc) return dev_err_probe(dev, rc, "TEIRQ request failed.\n");
disable_irq_nosync(par->irq_te); init_completion(&par->panel_te);} else { return dev_err_probe(dev, par->irq_te,"gpiod to TE IRQ failed.\n"); }
Again, it is not what had been suggested.
}