Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966508AbbBCTQM (ORCPT ); Tue, 3 Feb 2015 14:16:12 -0500 Received: from smtp38.i.mail.ru ([94.100.177.98]:36807 "EHLO smtp38.i.mail.ru" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S966254AbbBCTQI convert rfc822-to-8bit (ORCPT ); Tue, 3 Feb 2015 14:16:08 -0500 X-Greylist: delayed 75818 seconds by postgrey-1.27 at vger.kernel.org; Tue, 03 Feb 2015 14:16:07 EST Date: Tue, 3 Feb 2015 22:14:32 +0300 From: Roman Volkov To: Mark Rutland Cc: Roman Volkov , Dmitry Torokhov , Rob Herring , Pawel Moll , Ian Campbell , Kumar Gala , "grant.likely@linaro.org" , Hans de Goede , Jiri Kosina , Wolfram Sang , "linux-input@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "devicetree@vger.kernel.org" , Tony Prisk Subject: Re: [PATCH 5/5] i8042: Add i8042_dt.h glue for DT support Message-ID: <20150203221432.662461d8@v1ron-s7> In-Reply-To: <20150203115249.GB30866@leverpostej> References: <1422913730-12663-1-git-send-email-v1ron@v1ros.org> <1422913730-12663-5-git-send-email-v1ron@v1ros.org> <20150203115249.GB30866@leverpostej> X-Mailer: Claws Mail 3.11.1 (GTK+ 2.24.25; x86_64-unknown-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT X-Spam: Not detected X-Mras: Ok Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5085 Lines: 173 В Tue, 3 Feb 2015 11:52:50 +0000 Mark Rutland пишет: > On Mon, Feb 02, 2015 at 09:48:50PM +0000, Roman Volkov wrote: > > This header file designed to be similar to other glue layers found > > for i8042. The difference is that interrupt numbers, device address, > > and other information should be retrieved from the device tree. > > > > Signed-off-by: Tony Prisk > > Signed-off-by: Roman Volkov > > --- > > drivers/input/serio/i8042-dt.h | 112 > > +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 112 > > insertions(+) create mode 100644 drivers/input/serio/i8042-dt.h > > > > diff --git a/drivers/input/serio/i8042-dt.h > > b/drivers/input/serio/i8042-dt.h new file mode 100644 > > index 0000000..0d1a344 > > --- /dev/null > > +++ b/drivers/input/serio/i8042-dt.h > > @@ -0,0 +1,112 @@ > > +#ifndef _I8042_DT_H > > +#define _I8042_DT_H > > + > > +#include > > +#include > > +#include > > + > > +/* > > + * This program is free software; you can redistribute it and/or > > modify it > > + * under the terms of the GNU General Public License version 2 as > > published by > > + * the Free Software Foundation. > > + */ > > + > > +static void __iomem *i8042_base; > > +static unsigned int i8042_command_reg; > > +static unsigned int i8042_status_reg; > > +static unsigned int i8042_data_reg; > > +#define I8042_COMMAND_REG i8042_command_reg > > +#define I8042_STATUS_REG i8042_status_reg > > +#define I8042_DATA_REG i8042_data_reg > > + > > +/* > > + * Names. > > + */ > > + > > +static const char *i8042_kbd_phys_desc; > > +static const char *i8042_aux_phys_desc; > > +static const char *i8042_mux_phys_desc; > > +#define I8042_KBD_PHYS_DESC i8042_kbd_phys_desc > > +#define I8042_AUX_PHYS_DESC i8042_aux_phys_desc > > +#define I8042_MUX_PHYS_DESC i8042_mux_phys_desc > > + > > +/* > > + * IRQs. > > + */ > > +static int i8042_kbd_irq; > > +static int i8042_aux_irq; > > +#define I8042_KBD_IRQ i8042_kbd_irq > > +#define I8042_AUX_IRQ i8042_aux_irq > > That's a lot of static values. Surely nothing physically prevents the > use of multiple i8042 chips? > > > + > > +static inline int i8042_read_data(void) > > +{ > > + return readb(i8042_base + i8042_data_reg); > > +} > > + > > +static inline int i8042_read_status(void) > > +{ > > + return readb(i8042_base + i8042_status_reg); > > +} > > + > > +static inline void i8042_write_data(int val) > > +{ > > + writeb(val, i8042_base + i8042_data_reg); > > +} > > + > > +static inline void i8042_write_command(int val) > > +{ > > + writeb(val, i8042_base + i8042_command_reg); > > +} > > + > > +static inline int i8042_platform_init(struct platform_device *pdev) > > +{ > > + struct device_node *np = pdev->dev.of_node; > > + int status; > > + > > + i8042_base = of_iomap(np, 0); > > + if (!i8042_base) > > + return -ENOMEM; > > + > > + status = of_property_read_u32(np, "command-reg", > > &i8042_command_reg); > > + if (status) > > + return status; > > + > > + status = of_property_read_u32(np, "status-reg", > > &i8042_status_reg); > > + if (status) > > + return status; > > + > > + status = of_property_read_u32(np, "data-reg", > > &i8042_data_reg); > > + if (status) > > + return status; > > You should probably validate that these are within the range provided > in the reg property. > > You also need to clean up if you fail. It looks like here and below we > leak the i8042_base mapping if we decide to fail. > > > + > > + i8042_kbd_irq = irq_of_parse_and_map(np, 0); > > + i8042_aux_irq = irq_of_parse_and_map(np, 1); > > You can use platform_get_irq(pdev, N) here, the IRQ will already have > been parsed by the core. > > > + status = of_property_read_string(np, "linux,kbd_phys_desc", > > + > > &i8042_kbd_phys_desc); > > + if (status) > > + i8042_kbd_phys_desc = "i8042/serio0"; > > + > > + status = of_property_read_string(np, "linux,aux_phys_desc", > > + > > &i8042_aux_phys_desc); > > + if (status) > > + i8042_aux_phys_desc = "i8042/serio1"; > > + > > + status = of_property_read_string(np, "linux,mux_phys_desc", > > + > > &i8042_mux_phys_desc); > > + if (status) > > + i8042_mux_phys_desc = "i8042/serio%d"; > > User-provided values as format strings? Not a good idea. > > What exactly are these used for? > > > + > > + if (of_get_property(np, "init-reset", NULL)) > > + i8042_reset = true; > > Use of_property_read_bool. > > Thanks, > Mark. Mark, thanks for the good review. The current i8042 driver likely does not support multiple controllers. Maybe Dmitry will comment something regarding this and overall idea. I will check the case when DTS contains multiple i8042-compatible nodes. Expected behavior is that only the first node will be parsed and next nodes are silently ignored. Regards, Roman. Regards, Roman. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/