[v8,1/2] dt-bindings: input: touchscreen: iqs5xx: Add bindings

This patch adds binding documentation for the Azoteq IQS550/572/525
family of trackpad/touchscreen controllers.

Signed-off-by: Jeff LaBundy <jeff@labundy.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes in v8:
  - Added Reviewed-by trailer

Changes in v7:
  - Removed the azoteq,exp-ver-major and azoteq,exp-ver-minor properties to
    match the driver's updated device identification scheme
  - Updated the example node to use a level-sensitive interrupt rather than
    an edge-sensitive interrupt
  - Removed previous Reviewed-by trailer as this file has since changed

Changes in v6:
  - None

Changes in v5:
  - None

Changes in v4:
  - None

Changes in v3:
  - Added Reviewed-by trailer

Changes in v2:
  - Separated each valid "compatible" property with a line break
  - Specified the polarity of the RDY and NRST pins
  - Replaced duplicate definitions of common touchscreen properties with a
    reference to touchscreen.txt
  - Specified the example node as "touchscreen@74"

 .../bindings/input/touchscreen/iqs5xx.txt          | 80 ++++++++++++++++++++++
 .../devicetree/bindings/vendor-prefixes.txt        |  1 +
 2 files changed, 81 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/iqs5xx.txt

--
2.7.4

Hi Dmitry,

Just wanted to check in and see if you're satisfied with this patch
set based on my solutions to your feedback from February (v7) as well
as some final fortifications I opted to make earlier this month (v8).

On Sun, Apr 07, 2019 at 12:01:12AM -0500, Jeff LaBundy wrote:
> This patch adds support for the Azoteq IQS550/572/525 family of
> trackpad/touchscreen controllers.
> 
> The driver has been tested with an IQS550EV02 evaluation board. A
> demonstration of the driver's capabilities is available here:
> 
> https://youtu.be/sRNNx4XZBts
> 
> Signed-off-by: Jeff LaBundy <jeff@labundy.com>
> ---
> Changes in v8:
>   - Updated iqs5xx_fw_file_write to disable the interrupt before resetting the
>     device, and reset the device into normal mode if the firmware update fails
>   - Updated iqs5xx_dev_init to validate the device's bootloader status
>   - Updated iqs5xx_irq to return IRQ_NONE instead of IRQ_HANDLED if the device
>     unexpectedly asserts the RDY output during bootloader mode
> 
> Changes in v7:
>   - Changed the 'val' pointer type in iqs5xx_read_burst and iqs5xx_write_burst
>     to void * and const void *, respectively so that callers need not cast
>   - Overhauled the device initialization process to simply enter the bootloader
>     (for subsequent firmware updates from user space) rather than automatically
>     updating firmware in response to version mismatches
>   - Withheld input device allocation and registration until the device has been
>     successfully identified (or corrected firmware pushed from user space)
>   - Added comments in iqs5xx_fw_file_parse to explain why request_ihex_firmware
>     is not used
>   - Delayed the return from iqs5xx_irq until the device releases its RDY output
>     to allow the use of level-sensitive interrupts
>   - Removed IRQF_TRIGGER_RISING to preserve the type specified by device tree

Thanks,
Jeff L.

Hi Jeff,

On Sun, Apr 07, 2019 at 12:01:12AM -0500, Jeff LaBundy wrote:
> +static void iqs5xx_reset(struct i2c_client *client)
> +{
> +	struct iqs5xx_private *iqs5xx = i2c_get_clientdata(client);
> +
> +	gpiod_set_value_cansleep(iqs5xx->reset_gpio, 0);
> +	usleep_range(200, 300);
> +
> +	gpiod_set_value_cansleep(iqs5xx->reset_gpio, 1);

I believe we need to switch these statements around:

	gpiod_set_value_cansleep(iqs5xx->reset_gpio, 1);
	usleep_range(200, 300);
	gpiod_set_value_cansleep(iqs5xx->reset_gpio, 0);

so that you activate reset line, wait, and then release it. GPIOD deals
with logical signals, with 1 being active and 0 beig inactive. If reset
line is active low (it typically us) then it shoudl be described as such
in DTS and then gpiod API will take care of converting "1" logical
active to "0" actual value being output.

> +
> +static irqreturn_t iqs5xx_irq(int irq, void *data)
> +{
> +	struct iqs5xx_private *iqs5xx = (struct iqs5xx_private *)data;
> +	struct iqs5xx_touch_data *touch_data;
> +	struct i2c_client *client = iqs5xx->client;
> +	struct input_dev *input = iqs5xx->input;
> +	int error, i;
> +	u8 buf[sizeof(*touch_data) * IQS5XX_NUM_CONTACTS];

Given that iqs5xx_touch_data is packed, can't we do

	struct iqs5xx_touch_data touch_data[IQS5XX_NUM_CONTACTS];

instead?

No need to resubmit if you agree, I can make changes on my side before
applying.

I also noticed that you are overusing ARRAY_SIZE(): in several cases you
used it instead of sizeof() for supplying size of a buffer to transfer
functions. While the result will not change, from logical POW you are
not interested in number of elements in an array there, you want the
total size of data structure that just happens to be an array.

Thanks.

Hi Dmitry,

Many thanks for your kind review. Agreed on all counts and a minor v9 will
follow shortly. Responses below.

On Sun, Apr 28, 2019 at 10:31:36AM -0700, Dmitry Torokhov wrote:
> Hi Jeff,
> 
> On Sun, Apr 07, 2019 at 12:01:12AM -0500, Jeff LaBundy wrote:
> > +static void iqs5xx_reset(struct i2c_client *client)
> > +{
> > +	struct iqs5xx_private *iqs5xx = i2c_get_clientdata(client);
> > +
> > +	gpiod_set_value_cansleep(iqs5xx->reset_gpio, 0);
> > +	usleep_range(200, 300);
> > +
> > +	gpiod_set_value_cansleep(iqs5xx->reset_gpio, 1);
> 
> I believe we need to switch these statements around:
> 
> 	gpiod_set_value_cansleep(iqs5xx->reset_gpio, 1);
> 	usleep_range(200, 300);
> 	gpiod_set_value_cansleep(iqs5xx->reset_gpio, 0);
> 
> so that you activate reset line, wait, and then release it. GPIOD deals
> with logical signals, with 1 being active and 0 beig inactive. If reset
> line is active low (it typically us) then it shoudl be described as such
> in DTS and then gpiod API will take care of converting "1" logical
> active to "0" actual value being output.

D'oh! Great catch; looks like I was getting away with this because my dts
was swapped too. I'll fix this here as well as the bindings doc too.

> > +
> > +static irqreturn_t iqs5xx_irq(int irq, void *data)
> > +{
> > +	struct iqs5xx_private *iqs5xx = (struct iqs5xx_private *)data;
> > +	struct iqs5xx_touch_data *touch_data;
> > +	struct i2c_client *client = iqs5xx->client;
> > +	struct input_dev *input = iqs5xx->input;
> > +	int error, i;
> > +	u8 buf[sizeof(*touch_data) * IQS5XX_NUM_CONTACTS];
> 
> Given that iqs5xx_touch_data is packed, can't we do
> 
> 	struct iqs5xx_touch_data touch_data[IQS5XX_NUM_CONTACTS];
> 
> instead?

Absolutely; I've fixed this and updated the previous references to 'buf'
accordingly (much simpler now).

> No need to resubmit if you agree, I can make changes on my side before
> applying.

Thanks for the offer, but since I need to fix the reset polarity in the
bindings doc as well, I'll resubmit so that you don't have to go to any
trouble on account of my silly errors.

> I also noticed that you are overusing ARRAY_SIZE(): in several cases you
> used it instead of sizeof() for supplying size of a buffer to transfer
> functions. While the result will not change, from logical POW you are
> not interested in number of elements in an array there, you want the
> total size of data structure that just happens to be an array.

Thanks for spotting that; there was an instance of that in iqs5xx_irq which
has been taken care of as part of the above fix.

I still use ARRAY_SIZE in calls to i2c_transfer; that's because we need to
pass the number of i2c_msg elements and not the size of a buffer. However,
there were two instances where the msg.len member and associated memcpy or
memcmp should have used sizeof.

I'll make another pass through it then update the change list accordingly.

> Thanks.
> 
> -- 
> Dmitry

Thanks,
Jeff L.