Hi Dmitry, Many thanks for your kind review. All of your comments sound great to me; would just like to provide some background behind the first one before spinning a v7 (comments below). On Sun, Feb 17, 2019 at 10:40:54PM -0800, Dmitry Torokhov wrote: > Hi Jeff, > > On Sun, Feb 10, 2019 at 04:39:54PM -0600, 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 > > A few comments: > > - we have request_ihex_firmware that requests and validates biary > representation of ihex into the kernel. There is not really a good > reason to parse text ihex in kernel. > There is idex2bin in ./firmware to convert it. Agreed on all counts; using request_ihex_firmware was my original intent. However the vendor's ihex format is slightly nonstandard in that the checksum field is not calculated for every record, and the EOF record contains a nonzero address (0xFFFF), both of which cause ihex2bin to produce an error. This means the vendor's firmware would have to be passed through a second (custom) fixup tool before being converted yet again with ihex2bin. Therefore I (reluctantly) made the decision to handle the vendor's semi-custom ihex format in the driver directly. If that's a deal breaker, I'm happy to adopt request_ihex_firmware and contact the vendor to suggest that their configuration tool is updated to export true ihex files. However, there is no guarantee as to when/if that would happen, and it seems I would need to offer said fixup tool in the meantime (which I can certainly do). Let me know if that background changes your mind, or if you would still like to see this changed (either way works for me). > - I would recommend against doing this whole business of automatic > uprevving and updating firmware on probe. The controller mees to have > persistent firmware, so just check whether the device comes up in > bootloader or normal mode, and if it is in bootloader mode simply > forego creating and registering input device and wait for the > userspace to update firmware ad fix it up. > Not having to deal with firmware on probe will also allow compiling > the driver into the kernel (vs being a module) as firmware loading is > not normally available until after root filesystem is mounted. Agreed on all counts; I like that idea much better. > - firmware update is usually keyed off device model, not DT name. Agreed; I wasn't sold on my original method. My new plan is to let user space specify the name of the firmware, as the vendor's configuration tool appends some customer-specific revision information to the exported file's name that customers may opt to keep. > > +static int iqs5xx_read_burst(struct i2c_client *client, > > + u16 reg, u8 *val8, u8 len) > > If you make val8 void * you won't need to cast. Thanks for the tip; will do. > > + > > +static int iqs5xx_write_burst(struct i2c_client *client, > > + u16 reg, u8 *val8, u8 len) > > const void *. Thanks for the tip; will do. > > + error = devm_request_threaded_irq(&client->dev, client->irq, > > + NULL, iqs5xx_irq, > > + IRQF_ONESHOT | IRQF_TRIGGER_RISING, > > Please do not specify IRQ trigger, let it come from device tree, so only > use IRQF_ONESHOT. Agreed; will do. > By the way, I'd recommend using level interrupts so you never miss the > edge. Sounds good; I will update the example in the bindings doc to reflect. > Thanks. > > -- > Dmitry >
