lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Thu, 2 Nov 2017 11:19:07 +0100
From:   Ludovic Desroches <ludovic.desroches@...rochip.com>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
CC:     Ludovic Desroches <ludovic.desroches@...rochip.com>,
        <linux-input@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <devicetree@...r.kernel.org>, <nicolas.ferre@...rochip.com>,
        <alexandre.belloni@...e-electrons.com>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 2/5] input: misc: introduce Atmel PTC driver

Hi Dmitry,

On Mon, Oct 30, 2017 at 04:30:41PM -0700, Dmitry Torokhov wrote:
> Hi Ludovic,
> 
> On Fri, Oct 20, 2017 at 03:31:18PM +0200, Ludovic Desroches wrote:
> > The Atmel Peripheral Touch Controller subsystem offers built-in hardware
> > for capacitive touch measurement on sensors that function as buttons,
> > sliders and wheels.
> > 
> > Two files are loaded when probing the driver:
> > - a firmware for the Pico Power Processor that computes raw data from
> >   the ADC front end to provide high level information as button touch or
> >   untouch, slider position, etc.
> > - a configuration file that describe the topology and the parameters of
> >   the sensors.
> 
> If I understand it correctly the configuration is loaded via a cha
> 

Sorry, what's a cha?

> > 
> > Signed-off-by: Ludovic Desroches <ludovic.desroches@...rochip.com>
> > ---
> >  drivers/input/misc/Kconfig     |  12 +
> >  drivers/input/misc/Makefile    |   1 +
> >  drivers/input/misc/atmel_ptc.c | 723 +++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 736 insertions(+)
> >  create mode 100644 drivers/input/misc/atmel_ptc.c
> > 
> > diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> > index 9f082a388388..76616b0425b5 100644
> > --- a/drivers/input/misc/Kconfig
> > +++ b/drivers/input/misc/Kconfig
> > @@ -96,6 +96,18 @@ config INPUT_ATMEL_CAPTOUCH
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called atmel_captouch.
> >  
> > +config INPUT_ATMEL_PTC
> > +	tristate "Atmel PTC Driver"
> > +	depends on OF || COMPILE_TEST
> > +	depends on SOC_SAMA5D2
> 
> Maybe "default SOC_SAMA5D2"?
> 
> 

I don't think it's a peripheral which will be used often.

> > +	help
> > +	  Say Y to enable support for the Atmel PTC Subsystem.
> > +
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called atmel_ptc.
> > +	  If you compile it as a built-in driver, you have to build the
> > +	  firmware into the kernel or to use an initrd.
> > +
> >  config INPUT_BMA150
> >  	tristate "BMA150/SMB380 acceleration sensor support"
> >  	depends on I2C
> > diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> > index 03fd4262ada9..f71972adc002 100644
> > --- a/drivers/input/misc/Makefile
> > +++ b/drivers/input/misc/Makefile
> > @@ -18,6 +18,7 @@ obj-$(CONFIG_INPUT_ARIZONA_HAPTICS)	+= arizona-haptics.o
> >  obj-$(CONFIG_INPUT_ATI_REMOTE2)		+= ati_remote2.o
> >  obj-$(CONFIG_INPUT_ATLAS_BTNS)		+= atlas_btns.o
> >  obj-$(CONFIG_INPUT_ATMEL_CAPTOUCH)	+= atmel_captouch.o
> > +obj-$(CONFIG_INPUT_ATMEL_PTC)		+= atmel_ptc.o
> >  obj-$(CONFIG_INPUT_BFIN_ROTARY)		+= bfin_rotary.o
> >  obj-$(CONFIG_INPUT_BMA150)		+= bma150.o
> >  obj-$(CONFIG_INPUT_CM109)		+= cm109.o
> > diff --git a/drivers/input/misc/atmel_ptc.c b/drivers/input/misc/atmel_ptc.c
> > new file mode 100644
> > index 000000000000..612eaede6072
> > --- /dev/null
> > +++ b/drivers/input/misc/atmel_ptc.c
> > @@ -0,0 +1,723 @@
> > +/*
> > + * Atmel PTC subsystem driver for SAMA5D2 devices and compatible.
> > + *
> > + * Copyright (C) 2017 Microchip,
> > + *               2017 Ludovic Desroches <ludovic.desroches@...rochip.com>
> > + *
> > + * This software is licensed under the terms of the GNU General Public
> > + * License version 2, as published by the Free Software Foundation, and
> > + * may be copied, distributed, and modified under those terms.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + */
> > +
> > +#include <linux/cdev.h>
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/firmware.h>
> > +#include <linux/input.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/types.h>
> > +#include <linux/uaccess.h>
> > +
> > +#define ATMEL_PTC_MAX_NODES	64
> > +#define ATMEL_PTC_MAX_SCROLLERS	4
> > +
> > +/* ----- PPP ----- */
> > +#define ATMEL_PPP_FIRMWARE_NAME	"atmel_ptc.bin"
> > +
> > +#define ATMEL_PPP_CONFIG	0x20
> > +#define ATMEL_PPP_CTRL		0x24
> > +#define ATMEL_PPP_CMD		0x28
> > +#define		ATMEL_PPP_CMD_STOP		0x1
> > +#define		ATMEL_PPP_CMD_RESET		0x2
> > +#define		ATMEL_PPP_CMD_RESTART		0x3
> > +#define		ATMEL_PPP_CMD_ABORT		0x4
> > +#define		ATMEL_PPP_CMD_RUN		0x5
> > +#define		ATMEL_PPP_CMD_RUN_LOCKED	0x6
> > +#define		ATMEL_PPP_CMD_RUN_OCD		0x7
> > +#define		ATMEL_PPP_CMD_UNLOCK		0x8
> > +#define		ATMEL_PPP_CMD_NMI		0x9
> > +#define		ATMEL_PPP_CMD_HOST_OCD_RESUME	0xB
> > +#define ATMEL_PPP_ISR		0x33
> > +#define		ATMEL_PPP_IRQ_MASK	GENMASK(7, 4)
> > +#define		ATMEL_PPP_IRQ0		BIT(4)
> > +#define		ATMEL_PPP_IRQ1		BIT(5)
> > +#define		ATMEL_PPP_IRQ2		BIT(6)
> > +#define		ATMEL_PPP_IRQ3		BIT(7)
> > +#define		ATMEL_PPP_NOTIFY_MASK	GENMASK(3, 0)
> > +#define		ATMEL_PPP_NOTIFY0	BIT(0)
> > +#define		ATMEL_PPP_NOTIFY1	BIT(1)
> > +#define		ATMEL_PPP_NOTIFY2	BIT(2)
> > +#define		ATMEL_PPP_NOTIFY3	BIT(3)
> > +#define ATMEL_PPP_IDR		0x34
> > +#define ATMEL_PPP_IER		0x35
> > +
> > +#define atmel_ppp_readb(ptc, reg)	readb_relaxed(ptc->ppp_regs + reg)
> > +#define atmel_ppp_writeb(ptc, reg, val)	writeb_relaxed(val, ptc->ppp_regs + reg)
> > +#define atmel_ppp_readl(ptc, reg)	readl_relaxed(ptc->ppp_regs + reg)
> > +#define atmel_ppp_writel(ptc, reg, val)	writel_relaxed(val, ptc->ppp_regs + reg)
> > +
> > +/* ----- QTM ----- */
> > +#define ATMEL_QTM_CONF_NAME		"atmel_ptc.conf"
> > +
> > +#define ATMEL_QTM_MB_OFFSET			0x4000
> > +#define ATMEL_QTM_MB_SIZE			0x1000
> > +
> > +#define ATMEL_QTM_MB_CMD_OFFSET			0x0
> > +#define		ATMEL_QTM_CMD_FIRM_VERSION		8
> > +#define		ATMEL_QTM_CMD_INIT			18
> > +#define		ATMEL_QTM_CMD_RUN			19
> > +#define		ATMEL_QTM_CMD_STOP			21
> > +#define		ATMEL_QTM_CMD_SET_ACQ_MODE_TIMER	24
> > +#define ATMEL_QTM_MB_NODE_GROUP_CONFIG_OFFSET	0x100
> > +#define ATMEL_QTM_MB_SCROLLER_CONFIG_OFFSET	0x81a
> > +#define		ATMEL_QTM_SCROLLER_TYPE_SLIDER		0x0
> > +#define		ATMEL_QTM_SCROLLER_TYPE_WHEEL		0x1
> > +#define ATMEL_QTM_MB_SCROLLER_DATA_OFFSET	0x842
> > +#define ATMEL_QTM_MB_TOUCH_EVENTS_OFFSET	0x880
> > +
> > +#define atmel_qtm_get_scroller_config(buf, id) \
> > +	memcpy(buf, \
> > +	       ptc->qtm_mb + ATMEL_QTM_MB_SCROLLER_CONFIG_OFFSET \
> > +	       + (id) * sizeof(struct atmel_qtm_scroller_config), \
> > +	       sizeof(struct atmel_qtm_scroller_config))
> 
> Macros are usually capitals. This also looks like obfuscation...
> 

I'll capitalize this macro and next ones.

> > +
> > +
> > +#define atmel_qtm_get_scroller_data(buf, id) \
> > +	memcpy(buf, \
> > +	       ptc->qtm_mb + ATMEL_QTM_MB_SCROLLER_DATA_OFFSET \
> > +	       + (id) * sizeof(struct atmel_qtm_scroller_data), \
> > +	       sizeof(struct atmel_qtm_scroller_data))
> > +
> > +#define get_scroller_resolution(scroller_config) \
> > +	(1 << (scroller_config.resol_deadband >> 4))
> > +
> > +struct atmel_qtm_cmd {
> > +	u16	id;
> > +	u16	addr;
> > +	u32	data;
> 
> I think these (and others where you talk to the hardware) need
> endianness annotation (__le16/__be16, __le32/__be32).
> 

Ok.

> > +} __packed;
> > +
> > +struct atmel_qtm_node_group_config {
> > +	u16	count;
> > +	u8	ptc_type;
> > +	u8	freq_option;
> > +	u8	calib_option;
> > +	u8	unused;
> > +} __packed;
> > +
> > +struct atmel_qtm_scroller_config {
> > +	u8	type;
> > +	u8	unused;
> > +	u16	key_start;
> > +	u8	key_count;
> > +	u8	resol_deadband;
> > +	u8	position_hysteresis;
> > +	u8	unused2;
> > +	u16	contact_min_threshold;
> > +} __packed;
> > +
> > +struct atmel_qtm_scroller_data {
> > +	u8	status;
> > +	u8	right_hyst;
> > +	u8	left_hyst;
> > +	u8	unused;
> > +	u16	raw_position;
> > +	u16	position;
> > +	u16	contact_size;
> > +} __packed;
> > +
> > +struct atmel_qtm_touch_events {
> > +	u32	key_event_id[2];
> > +	u32	key_enable_state[2];
> > +	u32	scroller_event_id;
> > +	u32	scroller_event_state;
> > +} __packed;
> > +
> > +struct atmel_ptc {
> > +	void __iomem		*ppp_regs;
> > +	void __iomem		*firmware;
> > +	int			irq;
> > +	u8			imr;
> > +	void __iomem		*qtm_mb;
> > +	struct clk		*clk_per;
> > +	struct clk		*clk_int_osc;
> > +	struct clk		*clk_slow;
> > +	struct device		*dev;
> > +	struct completion	ppp_ack;
> > +	unsigned int		button_keycode[ATMEL_PTC_MAX_NODES];
> > +	struct input_dev	*buttons_input;
> > +	struct input_dev	*scroller_input[ATMEL_PTC_MAX_SCROLLERS];
> > +	bool			buttons_registered;
> > +	bool			scroller_registered[ATMEL_PTC_MAX_SCROLLERS];
> > +	u32			button_event[ATMEL_PTC_MAX_NODES / 32];
> > +	u32			button_state[ATMEL_PTC_MAX_NODES / 32];
> > +	u32			scroller_event;
> > +	u32			scroller_state;
> > +};
> > +
> > +static void atmel_ppp_irq_enable(struct atmel_ptc *ptc, u8 mask)
> > +{
> > +	ptc->imr |= mask;
> > +	atmel_ppp_writeb(ptc, ATMEL_PPP_IER, mask & ATMEL_PPP_IRQ_MASK);
> > +}
> > +
> > +static void atmel_ppp_irq_disable(struct atmel_ptc *ptc, u8 mask)
> > +{
> > +	ptc->imr &= ~mask;
> > +	atmel_ppp_writeb(ptc, ATMEL_PPP_IDR, mask & ATMEL_PPP_IRQ_MASK);
> > +}
> > +
> > +static void atmel_ppp_notify(struct atmel_ptc *ptc, u8 mask)
> > +{
> > +	if (mask & ATMEL_PPP_NOTIFY_MASK) {
> > +		u8 notify = atmel_ppp_readb(ptc, ATMEL_PPP_ISR)
> > +			| (mask & ATMEL_PPP_NOTIFY_MASK);
> > +
> > +		atmel_ppp_writeb(ptc, ATMEL_PPP_ISR, notify);
> > +	}
> > +}
> > +
> > +static void atmel_ppp_irq_pending_clr(struct atmel_ptc *ptc, u8 mask)
> > +{
> > +	if (mask & ATMEL_PPP_IRQ_MASK) {
> > +		u8 irq = atmel_ppp_readb(ptc, ATMEL_PPP_ISR) & ~mask;
> > +
> > +		atmel_ppp_writeb(ptc, ATMEL_PPP_ISR, irq);
> > +	}
> > +}
> > +
> > +static void atmel_ppp_cmd_send(struct atmel_ptc *ptc, u32 cmd)
> > +{
> > +	atmel_ppp_writel(ptc, ATMEL_PPP_CMD, cmd);
> > +}
> > +
> > +static void atmel_ppp_irq_scroller_event(struct atmel_ptc *ptc)
> > +{
> > +	int i;
> > +
> > +	if (!ptc->scroller_event)
> > +		return;
> > +
> > +	for (i = 0; i < ATMEL_PTC_MAX_SCROLLERS; i++) {
> > +		u32 mask = 1 << i;
> 
> BIT(i)
> 
> But I wonder if you can't use for_each_set_bit() here.
> 

Probably, I'll have a look.

> > +		struct atmel_qtm_scroller_data scroller_data;
> > +		struct atmel_qtm_scroller_config scroller_config;
> > +
> > +		if (!(ptc->scroller_event & mask))
> > +			continue;
> > +
> > +		atmel_qtm_get_scroller_data(&scroller_data, i);
> > +		atmel_qtm_get_scroller_config(&scroller_config, i);
> 
> This ends up copying memory on each interrupt. Can we look up in the
> original buffer?
> 

Yes I can. I did this to get a snapshot and so consistent values within
those structures which may change in a continuous way.

> > +
> > +		if (scroller_config.type == ATMEL_QTM_SCROLLER_TYPE_WHEEL)
> > +			input_report_abs(ptc->scroller_input[i],
> > +					 ABS_WHEEL, scroller_data.position);
> 
> What endianness "position" is in?
> 

LE.

> > +		else
> > +			input_report_abs(ptc->scroller_input[i],
> > +					 ABS_X, scroller_data.position);
> > +
> > +		input_report_key(ptc->scroller_input[i], BTN_TOUCH,
> > +				 scroller_data.status & 0x1);
> > +		input_sync(ptc->scroller_input[i]);
> > +	}
> > +}
> > +
> > +static void atmel_ppp_irq_button_event(struct atmel_ptc *ptc)
> > +{
> > +	int i, j;
> > +
> > +	for (i = 0; i < ATMEL_PTC_MAX_NODES / 32; i++) {
> > +		if (!ptc->button_event[i])
> > +			continue;
> > +
> > +		for (j = 0; j < 32; j++) {
> > +			u32 mask = 1 << j;
> > +			u32 state = ptc->button_state[i] & mask;
> > +			unsigned int key_id = i * 32 + j;
> 
> I think you want to flatten these 2 loops with for_each_set_bit() as
> well.
> 

Yes.

> > +
> > +			if (!(ptc->button_event[i] & mask))
> > +				continue;
> > +
> > +			input_report_key(ptc->buttons_input,
> > +					 ptc->button_keycode[key_id], !!state);
> > +			input_sync(ptc->buttons_input);
> > +		}
> > +	}
> > +}
> > +
> > +static void atmel_ppp_irq_touch_event(struct atmel_ptc *ptc)
> > +{
> > +	atmel_ppp_irq_scroller_event(ptc);
> > +	atmel_ppp_irq_button_event(ptc);
> > +}
> > +
> > +static irqreturn_t atmel_ppp_irq_handler(int irq, void *data)
> > +{
> > +	struct atmel_ptc *ptc = data;
> > +	u32 isr = atmel_ppp_readb(ptc, ATMEL_PPP_ISR) & ptc->imr;
> > +
> > +	/* QTM CMD acknowledgment */
> > +	if (isr & ATMEL_PPP_IRQ0) {
> > +		atmel_ppp_irq_disable(ptc, ATMEL_PPP_IRQ0);
> > +		atmel_ppp_irq_pending_clr(ptc, ATMEL_PPP_IRQ0);
> > +		complete(&ptc->ppp_ack);
> > +	}
> > +	/* QTM touch event */
> > +	if (isr & ATMEL_PPP_IRQ1) {
> > +		struct atmel_qtm_touch_events touch_events;
> > +		int i;
> > +
> > +		memcpy(&touch_events,
> > +		       ptc->qtm_mb + ATMEL_QTM_MB_TOUCH_EVENTS_OFFSET,
> > +		       sizeof(touch_events));
> 
> Did you mean memcpy_fromio() here?
>

Probably, I have to checked. I remember having some weird behaviors with
first firmwares and memcpy, I tried several variants.

> > +
> > +		for (i = 0; i < ATMEL_PTC_MAX_NODES / 32; i++) {
> > +			ptc->button_event[i] = touch_events.key_event_id[i];
> > +			ptc->button_state[i] = touch_events.key_enable_state[i];
> > +		}
> > +		ptc->scroller_event = touch_events.scroller_event_id;
> > +		ptc->scroller_state = touch_events.scroller_event_state;
> > +
> > +		atmel_ppp_irq_pending_clr(ptc, ATMEL_PPP_IRQ1);
> > +
> > +		atmel_ppp_irq_touch_event(ptc);
> > +	}
> > +	/* Debug event */
> > +	if (isr & ATMEL_PPP_IRQ2)
> > +		atmel_ppp_irq_pending_clr(ptc, ATMEL_PPP_IRQ2);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +void atmel_qtm_cmd_send(struct atmel_ptc *ptc, struct atmel_qtm_cmd *cmd)
> > +{
> > +	int i, ret;
> > +
> > +	dev_dbg(ptc->dev, "%s: cmd=0x%x, addr=0x%x, data=0x%x\n",
> > +		__func__, cmd->id, cmd->addr, cmd->data);
> > +
> > +	memcpy(ptc->qtm_mb, cmd, sizeof(*cmd));
> 
> memcpy_toio()? It looks like it is needed elsewhere as well.
> 
> > +
> > +	/* Once command performed, we'll get an IRQ. */
> > +	atmel_ppp_irq_enable(ptc, ATMEL_PPP_IRQ0);
> > +	/* Notify PPP that we have sent a command. */
> > +	atmel_ppp_notify(ptc, ATMEL_PPP_NOTIFY0);
> > +	/* Wait for IRQ from PPP. */
> > +	wait_for_completion(&ptc->ppp_ack);
> > +
> > +	/*
> > +	 * Register input devices only when QTM is started since we need some
> > +	 * information from the QTM configuration.
> > +	 */
> 
> This is a bit weird... If we have configuration in device tree, what
> else do we need to do here? And if we need dynamic config anyway, then
> maybe we should not be using device tree?
> 

I agree. The resolution is not in the device tree, it will be redundant
with the configuration loaded.

I kept the DT part to get a link between the sensors and the code. If
there is another way to manage this, then I can rely only on the
configuration loaded.

> > +	if (cmd->id == ATMEL_QTM_CMD_RUN) {
> > +		if (ptc->buttons_input && !ptc->buttons_registered) {
> > +			ret = input_register_device(ptc->buttons_input);
> > +			if (ret)
> > +				dev_err(ptc->dev, "can't register input button device.\n");
> > +			else
> > +				ptc->buttons_registered = true;
> > +		}
> > +
> > +		for (i = 0; i < ATMEL_PTC_MAX_SCROLLERS; i++) {
> > +			struct input_dev *scroller = ptc->scroller_input[i];
> > +			struct atmel_qtm_scroller_config scroller_config;
> > +
> > +			if (!scroller || ptc->scroller_registered[i])
> > +				continue;
> > +
> > +			atmel_qtm_get_scroller_config(&scroller_config, i);
> > +
> > +			if (scroller_config.type ==
> > +			    ATMEL_QTM_SCROLLER_TYPE_SLIDER) {
> > +				unsigned int max = get_scroller_resolution(scroller_config);
> > +
> > +				input_set_abs_params(scroller, 0, 0, max, 0, 0);
> > +			}
> > +			ret = input_register_device(scroller);
> > +			if (ret)
> > +				dev_err(ptc->dev, "can't register input scroller device.\n");
> > +			else
> > +				ptc->scroller_registered[i] = true;
> > +		}
> > +	}
> > +
> > +	memcpy(cmd, ptc->qtm_mb, sizeof(*cmd));
> > +}
> > +
> > +static inline struct atmel_ptc *kobj_to_atmel_ptc(struct kobject *kobj)
> > +{
> > +	struct device *dev = kobj_to_dev(kobj);
> > +
> > +	return dev->driver_data;
> > +}
> > +
> > +static ssize_t atmel_qtm_mb_read(struct file *filp, struct kobject *kobj,
> > +				 struct bin_attribute *attr,
> > +				 char *buf, loff_t off, size_t count)
> > +{
> > +	struct atmel_ptc *ptc = kobj_to_atmel_ptc(kobj);
> > +	char *qtm_mb = (char *)ptc->qtm_mb;
> > +
> > +	dev_dbg(ptc->dev, "%s: off=0x%llx, count=%zu\n", __func__, off, count);
> > +
> > +	memcpy(buf, qtm_mb + off, count);
> > +
> > +	return count;
> > +}
> > +
> > +static ssize_t atmel_qtm_mb_write(struct file *filp, struct kobject *kobj,
> > +				  struct bin_attribute *attr,
> > +				  char *buf, loff_t off, size_t count)
> > +{
> > +	struct atmel_ptc *ptc = kobj_to_atmel_ptc(kobj);
> > +	char *qtm_mb = (char *)ptc->qtm_mb;
> > +
> > +	dev_dbg(ptc->dev, "%s: off=0x%llx, count=%zu\n", __func__, off, count);
> > +
> > +	if (off == 0 && count == sizeof(struct atmel_qtm_cmd))
> > +		atmel_qtm_cmd_send(ptc, (struct atmel_qtm_cmd *)buf);
> > +	else
> > +		memcpy(qtm_mb + off, buf, count);
> > +
> > +	return count;
> > +}
> > +
> > +static struct bin_attribute atmel_ptc_qtm_mb_attr = {
> > +	.attr = {
> > +		.name = "qtm_mb",
> > +		.mode = 0644,
> > +	},
> > +	.size = ATMEL_QTM_MB_SIZE,
> > +	.read = atmel_qtm_mb_read,
> > +	.write = atmel_qtm_mb_write,
> > +};
> > +
> > +/*
> > + * From QTM MB configuration, we can't retrieve all the information needed
> > + * to setup correctly input devices: buttons key codes and slider axis are
> > + * missing.
> > + */
> > +static int atmel_ptc_of_parse(struct atmel_ptc *ptc)
> > +{
> > +	struct device_node *sensor;
> > +	bool first_button = true;
> > +
> > +	/* Parse sensors. */
> > +	for_each_child_of_node(ptc->dev->of_node, sensor) {
> > +		if (!strcmp(sensor->name, "button")) {
> > +			u32 key_id, keycode;
> > +			struct input_dev *buttons = ptc->buttons_input;
> > +
> > +			if (of_property_read_u32(sensor, "reg", &key_id)) {
> > +				dev_err(ptc->dev, "reg is missing (%s)\n",
> > +					sensor->full_name);
> > +				return -EINVAL;
> > +			}
> > +
> > +			if (of_property_read_u32(sensor, "linux,keycode", &keycode)) {
> > +				dev_err(ptc->dev, "linux,keycode is missing (%s)\n",
> > +					sensor->full_name);
> > +				return -EINVAL;
> > +			}
> > +			ptc->button_keycode[key_id] = keycode;
> > +
> > +			/* All buttons are put together in a keyboard device. */
> > +			if (first_button) {
> > +				buttons = devm_input_allocate_device(ptc->dev);
> > +				if (!buttons)
> > +					return -ENOMEM;
> > +				buttons->name = "atmel_ptc_buttons";
> > +				buttons->dev.parent = ptc->dev;
> > +				buttons->keycode = ptc->button_keycode;
> > +				buttons->keycodesize = sizeof(ptc->button_keycode[0]);
> > +				buttons->keycodemax = ATMEL_PTC_MAX_NODES;
> > +				ptc->buttons_input = buttons;
> > +				first_button = false;
> > +			}
> > +
> > +			input_set_capability(buttons, EV_KEY, keycode);
> > +		} else if (!strcmp(sensor->name, "slider") ||
> > +			   !strcmp(sensor->name, "wheel")) {
> > +			u32 scroller_id;
> > +			struct input_dev *scroller;
> > +
> > +			if (of_property_read_u32(sensor, "reg", &scroller_id)) {
> > +				dev_err(ptc->dev, "reg is missing (%s)\n",
> > +					sensor->full_name);
> > +				return -EINVAL;
> > +			}
> > +
> > +			if (scroller_id > ATMEL_PTC_MAX_SCROLLERS - 1) {
> > +				dev_err(ptc->dev, "wrong scroller id (%s)\n",
> > +					sensor->full_name);
> > +				return -EINVAL;
> > +			}
> > +
> > +			scroller = devm_input_allocate_device(ptc->dev);
> > +			if (!scroller)
> > +				return -ENOMEM;
> > +
> > +			scroller->dev.parent = ptc->dev;
> > +			ptc->scroller_input[scroller_id] = scroller;
> > +
> > +			if (!strcmp(sensor->name, "slider")) {
> > +				scroller->name = "atmel_ptc_slider";
> > +				input_set_capability(scroller, EV_ABS, ABS_X);
> > +				input_set_capability(scroller, EV_KEY, BTN_TOUCH);
> > +			} else {
> > +				scroller->name = "atmel_ptc_wheel";
> > +				input_set_capability(scroller, EV_ABS, ABS_WHEEL);
> > +				input_set_capability(scroller, EV_KEY, BTN_TOUCH);
> > +			}
> > +		} else {
> > +			dev_err(ptc->dev, "%s is not supported\n", sensor->name);
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void atmel_qtm_conf_callback(const struct firmware *conf, void *context)
> > +{
> > +	struct atmel_ptc *ptc = context;
> > +	struct atmel_qtm_cmd qtm_cmd;
> > +	char *dst;
> > +	struct atmel_qtm_node_group_config node_group_config;
> > +
> > +	if (!conf) {
> > +		dev_err(ptc->dev, "cannot load QTM configuration, it has to be set manually.\n");
> > +		return;
> > +	}
> > +
> > +	atmel_ppp_irq_enable(ptc, ATMEL_PPP_IRQ1);
> > +	atmel_ppp_irq_disable(ptc, ATMEL_PPP_IRQ2 | ATMEL_PPP_IRQ3);
> > +
> > +	qtm_cmd.id = ATMEL_QTM_CMD_STOP;
> > +	atmel_qtm_cmd_send(ptc, &qtm_cmd);
> > +
> > +	/* Load QTM configuration. */
> > +	dst = (char *)ptc->qtm_mb + ATMEL_QTM_MB_NODE_GROUP_CONFIG_OFFSET;
> > +	/* memcpy doesn't work for an unknown reason. */
> > +	_memcpy_toio(dst, conf->data, conf->size);
> > +	release_firmware(conf);
> > +
> > +	if (atmel_ptc_of_parse(ptc))
> > +		dev_err(ptc->dev, "ptc_of_parse failed\n");
> > +
> > +	memcpy(&node_group_config,
> > +	       ptc->qtm_mb + ATMEL_QTM_MB_NODE_GROUP_CONFIG_OFFSET,
> > +	       sizeof(node_group_config));
> > +
> > +	/* Start QTM. */
> > +	qtm_cmd.id = ATMEL_QTM_CMD_INIT;
> > +	qtm_cmd.data = node_group_config.count;
> > +	atmel_qtm_cmd_send(ptc, &qtm_cmd);
> > +	qtm_cmd.id = ATMEL_QTM_CMD_SET_ACQ_MODE_TIMER;
> > +	qtm_cmd.data = 20;
> > +	atmel_qtm_cmd_send(ptc, &qtm_cmd);
> > +	qtm_cmd.id = ATMEL_QTM_CMD_RUN;
> > +	qtm_cmd.data = node_group_config.count;
> > +	atmel_qtm_cmd_send(ptc, &qtm_cmd);
> > +}
> > +
> > +static void atmel_ppp_fw_callback(const struct firmware *fw, void *context)
> > +{
> > +	struct atmel_ptc *ptc = context;
> > +	int ret;
> > +	struct atmel_qtm_cmd cmd;
> > +
> > +	if (!fw || !fw->size) {
> > +		dev_err(ptc->dev, "cannot load firmware.\n");
> > +		release_firmware(fw);
> > +		device_release_driver(ptc->dev);
> > +		return;
> > +	}
> > +
> > +	/* Command sequence to start from a clean state. */
> > +	atmel_ppp_cmd_send(ptc, ATMEL_PPP_CMD_ABORT);
> > +	atmel_ppp_irq_pending_clr(ptc, ATMEL_PPP_IRQ_MASK);
> > +	atmel_ppp_cmd_send(ptc, ATMEL_PPP_CMD_RESET);
> > +
> > +	memcpy(ptc->firmware, fw->data, fw->size);
> > +	release_firmware(fw);
> > +
> > +	atmel_ppp_cmd_send(ptc, ATMEL_PPP_CMD_RUN);
> > +
> > +	cmd.id = ATMEL_QTM_CMD_FIRM_VERSION;
> > +	atmel_qtm_cmd_send(ptc, &cmd);
> > +	dev_info(ptc->dev, "firmware version: %u\n", cmd.data);
> 
> dev_dbg().
> 

Ok.

> > +
> > +	/* PPP is running, it's time to load the QTM configuration. */
> > +	ret = request_firmware_nowait(THIS_MODULE, 1, ATMEL_QTM_CONF_NAME, ptc->dev,
> > +				      GFP_KERNEL, ptc, atmel_qtm_conf_callback);
> > +	if (ret)
> > +		dev_err(ptc->dev, "QTM configuration loading failed.\n");
> > +}
> > +
> > +static int atmel_ptc_probe(struct platform_device *pdev)
> > +{
> > +	struct atmel_ptc *ptc;
> > +	struct resource	*res;
> > +	void *shared_memory;
> > +	int ret;
> > +
> > +	ptc = devm_kzalloc(&pdev->dev, sizeof(*ptc), GFP_KERNEL);
> > +	if (!ptc)
> > +		return -ENOMEM;
> > +
> > +	platform_set_drvdata(pdev, ptc);
> > +	ptc->dev = &pdev->dev;
> > +	ptc->dev->driver_data = ptc;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!res)
> > +		return -ENODEV;
> > +
> > +	shared_memory = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(shared_memory))
> > +		return PTR_ERR(shared_memory);
> > +
> > +	ptc->firmware = shared_memory;
> > +	ptc->qtm_mb = shared_memory + ATMEL_QTM_MB_OFFSET;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > +	if (!res)
> > +		return -EINVAL;
> > +
> > +	ptc->ppp_regs = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(ptc->ppp_regs))
> > +		return PTR_ERR(ptc->ppp_regs);
> > +
> > +	ptc->irq = platform_get_irq(pdev, 0);
> > +	if (ptc->irq <= 0) {
> > +		if (!ptc->irq)
> > +			ptc->irq = -ENXIO;
> > +
> > +		return ptc->irq;
> > +	}
> > +
> > +	ptc->clk_per = devm_clk_get(&pdev->dev, "ptc_clk");
> > +	if (IS_ERR(ptc->clk_per))
> > +		return PTR_ERR(ptc->clk_per);
> > +
> > +	ptc->clk_int_osc = devm_clk_get(&pdev->dev, "ptc_int_osc");
> > +	if (IS_ERR(ptc->clk_int_osc))
> > +		return PTR_ERR(ptc->clk_int_osc);
> > +
> > +	ptc->clk_slow = devm_clk_get(&pdev->dev, "slow_clk");
> > +	if (IS_ERR(ptc->clk_slow))
> > +		return PTR_ERR(ptc->clk_slow);
> > +
> > +	ret = devm_request_irq(&pdev->dev, ptc->irq, atmel_ppp_irq_handler, 0,
> > +			       pdev->dev.driver->name, ptc);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = clk_prepare_enable(ptc->clk_int_osc);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = clk_prepare_enable(ptc->clk_per);
> > +	if (ret)
> > +		goto disable_clk_int_osc;
> > +
> > +	ret = clk_prepare_enable(ptc->clk_slow);
> > +	if (ret)
> > +		goto disable_clk_per;
> 
> You really want to enable clocks before you request IRQ. And since you
> mixing devm and non-devm APIs you need to be extra careful on teardown
> path to preserve order of releasing resources.
> devm_add_action_or_reset() is your friend here.
> 

I'll rework this part.

> Is your IRQ routine ready to be fired even though not all device
> structures are allocated/initialized?
> 

Probably not, I will defer it.

> > +
> > +	/* Needed to avoid unexpected behaviors. */
> > +	memset(ptc->firmware, 0, ATMEL_QTM_MB_OFFSET + sizeof(*ptc->qtm_mb));
> > +	ptc->imr = 0;
> > +	init_completion(&ptc->ppp_ack);
> > +
> > +	/*
> > +	 * Expose a file to give an access to the QTM mailbox to a user space
> > +	 * application in order to configure it or to send commands.
> > +	 */

Is it a good approach to offer a userspace access to the mailbox in this
way?

At the beginning, the mailbox should be accessed from userspace only for
debug purpose or to set the configuration manually.

Nowadays, we are developing tools to help the user to set the right
configuration. In the future, we may also provide tools to visualize in
'realtime' some values of the mailbox. I'm scared to miss some data and
to not use the right interface to reach these goals. What's your opinion?

> > +	ret = sysfs_create_bin_file(&pdev->dev.kobj, &atmel_ptc_qtm_mb_attr);
> > +	if (ret)
> > +		goto disable_clk_slow;
> 
> devm_device_add_group()?
>

Ok.

> > +
> > +	ret = request_firmware_nowait(THIS_MODULE, 1, ATMEL_PPP_FIRMWARE_NAME,
> > +				      ptc->dev, GFP_KERNEL, ptc,
> > +				      atmel_ppp_fw_callback);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "firmware loading failed\n");
> > +		ret = -EPROBE_DEFER;
> 
> Always defer? That is optimistic.
> 

Probably.

> > +		goto remove_qtm_mb;
> > +	}
> > +
> > +	return 0;
> > +
> > +remove_qtm_mb:
> > +	sysfs_remove_bin_file(&pdev->dev.kobj, &atmel_ptc_qtm_mb_attr);
> > +disable_clk_slow:
> > +	clk_disable_unprepare(ptc->clk_slow);
> > +disable_clk_per:
> > +	clk_disable_unprepare(ptc->clk_per);
> > +disable_clk_int_osc:
> > +	clk_disable_unprepare(ptc->clk_int_osc);
> > +
> > +	return ret;
> > +}
> > +
> > +static int atmel_ptc_remove(struct platform_device *pdev)
> > +{
> > +	struct atmel_ptc *ptc = platform_get_drvdata(pdev);
> > +	int i;
> > +
> 
> You need a completion in your fimrware loading callback and wait for it
> here, otherwise quick bind/unbind will crash the kernel.
> 
> 

I see, thanks.

> > +	if (ptc->buttons_registered)
> > +		input_unregister_device(ptc->buttons_input);
> > +
> > +	for (i = 0; i < ATMEL_PTC_MAX_SCROLLERS; i++) {
> > +		struct input_dev *scroller = ptc->scroller_input[i];
> > +
> > +		if (!scroller || !ptc->scroller_registered[i])
> > +			continue;
> > +		input_unregister_device(scroller);
> > +	}
> > +
> > +	sysfs_remove_bin_file(&pdev->dev.kobj, &atmel_ptc_qtm_mb_attr);
> > +	clk_disable_unprepare(ptc->clk_slow);
> > +	clk_disable_unprepare(ptc->clk_per);
> > +	clk_disable_unprepare(ptc->clk_int_osc);
> > +
> > +	return 0;
> > +}
> > +
> 
> #ifdef CONFIG_OF

I have depends on OF in Kconfig.

> > +static const struct of_device_id atmel_ptc_dt_match[] = {
> > +	{
> > +		.compatible = "atmel,sama5d2-ptc",
> > +	}, {
> > +		/* sentinel */
> > +	}
> > +};
> > +MODULE_DEVICE_TABLE(of, atmel_ptc_dt_match);
> #endif
> 
> > +
> > +static struct platform_driver atmel_ptc_driver = {
> > +	.probe = atmel_ptc_probe,
> > +	.remove = atmel_ptc_remove,
> > +	.driver = {
> > +		.name = "atmel_ptc",
> > +		.of_match_table = atmel_ptc_dt_match,
> 
> of_match_ptr() - you are allowing it to be compiled without OF.
> 

ditto but I can change it if you want.

> > +	},
> > +};
> > +module_platform_driver(atmel_ptc_driver)
> > +
> > +MODULE_AUTHOR("Ludovic Desroches <ludovic.desroches@...rochip.com>");
> > +MODULE_DESCRIPTION("Atmel PTC subsystem");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_FIRMWARE(ATMEL_PPP_FIRMWARE_NAME);
> > +MODULE_FIRMWARE(ATMEL_QTM_CONF_NAME);
> > -- 
> > 2.12.2
> > 
> 
> Thanks.

Thanks for your review.

Regards

Ludovic

Powered by blists - more mailing lists