On Oct 12 2016 or thereabouts, Nick Dyer wrote:
> Hi Benjamin-
>
> Thanks for the review, I've answered in line.
>
> On Mon, Oct 10, 2016 at 03:48:07PM +0200, Benjamin Tissoires wrote:
> > On Sep 20 2016 or thereabouts, Nick Dyer wrote:
> > > Signed-off-by: Nick Dyer <nick@xxxxxxxxxxxxx>
> > > Tested-by: Chris Healy <cphealy@xxxxxxxxx>
> > > ---
> > > drivers/input/rmi4/Kconfig | 11 +
> > > drivers/input/rmi4/Makefile | 1 +
> > > drivers/input/rmi4/rmi_bus.c | 3 +
> > > drivers/input/rmi4/rmi_driver.c | 161 ++++++++++++++-
> > > drivers/input/rmi4/rmi_driver.h | 7 +
> > > drivers/input/rmi4/rmi_f01.c | 6 +
> > > drivers/input/rmi4/rmi_f34.c | 446 ++++++++++++++++++++++++++++++++++++++++
> > > include/linux/rmi.h | 1 +
> > > 8 files changed, 634 insertions(+), 2 deletions(-)
> > > create mode 100644 drivers/input/rmi4/rmi_f34.c
> [...]
> > > @@ -143,8 +154,11 @@ int rmi_process_interrupt_requests(struct rmi_device *rmi_dev)
> > > struct rmi_function *entry;
> > > int error;
> > >
> > > - if (!data)
> > > + mutex_lock(&data->irq_mutex);
> > > + if (!data || !data->irq_status || !data->f01_container) {
> > > + mutex_unlock(&data->irq_mutex);
> >
> > I have been now convinced that IRQ should be handled in core. It would
> > make everyone's life easier and I think means that we won't need these
> > checks given that IRQ could simply be disabled during FW update.
> >
> > I guess it's time to resurrect Bjorn's patch at
> > https://lkml.org/lkml/2016/5/9/1055
>
> We do use the interrupts in FW update in the current version. I haven't
> done the measurements, but my expectation would be that performance is
> slightly improved over polling, and it's more elegant.
>
> > > +#ifdef CONFIG_RMI4_F34
> > > +static int rmi_firmware_update(struct rmi_driver_data *data,
> > > + const struct firmware *fw)
> > > +{
> > > + struct device *dev = &data->rmi_dev->dev;
> > > + int ret;
> > > +
> > > + if (!data->f34_container) {
> > > + dev_warn(dev, "%s: No F34 present!\n",
> > > + __func__);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + ret = rmi_f34_check_supported(data->f34_container);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + /* Enter flash mode */
> > > + rmi_dbg(RMI_DEBUG_CORE, dev, "Enabling flash\n");
> > > + ret = rmi_f34_enable_flash(data->f34_container);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + /* Tear down functions and re-probe */
> > > + rmi_free_function_list(data->rmi_dev);
> > > +
> > > + ret = rmi_probe_interrupts(data);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = rmi_init_functions(data);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + if (!data->f01_bootloader_mode || !data->f34_container) {
> > > + dev_warn(dev, "%s: No F34 present or not in bootloader!\n",
> > > + __func__);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + /* Perform firmware update */
> > > + ret = rmi_f34_update_firmware(data->f34_container, fw);
> > > +
> > > + /* Re-probe */
> > > + rmi_dbg(RMI_DEBUG_CORE, dev, "Re-probing device\n");
> > > + rmi_free_function_list(data->rmi_dev);
> > > +
> > > + ret = rmi_scan_pdt(data->rmi_dev, NULL, rmi_initial_reset);
> > > + if (ret < 0)
> > > + dev_warn(dev, "RMI reset failed!\n");
> > > +
> > > + ret = rmi_probe_interrupts(data);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = rmi_init_functions(data);
> > > + if (ret)
> > > + return ret;
> >
> > You could basically export rmi_fn_reset() which would call
> > rmi_free_function_list(), rmi_scan_pdt (if initial reset),
> > rmi_probe_interrupts() and rmi_init_functions, and this would allow you
> > to have all this in f34.
>
> I see what you mean, and I do agree that it would be neater to have all
> of this in the f34 code.
>
> However, the problem is that when you call rmi_free_function_list(), the
> f34 driver and all the context information attached to it (struct
> rmi_function, struct f34_data, and any sysfs attributes in the f34
> directory) gets torn down, so you're kind of left without the branch you
> were sitting on.
>
> To get around that, I'd have to make f34 a special case anyway. Taking
> that into account, the current solution seemed neater to me. I could
> possibly cram a little more of it into rmi_f34.c, but I think the
> context has to be "struct rmi_driver_data".
If I understand correctly, rmi_firmware_update() is only called through
the sysfs. So how about you export the required functions from core you
are using and export 2 functions from rmi_f34 that will be a special
case: rmi_f34_create_sysfs() and rmi_f34_remove_sysfs() (or any better
names). You could just put your code in rmi_f34, provide noops
declarations if RMI_F34 is not set, and core will have only 2 calls to
rmi_f34.
BTW, I am thinking at carrying in my next RMI4 series your 1/2 patch. I
also want to take Bjorn and Andrew left patches so that we have a common
tree at some point. Any objections?
Of course, if you resubmit before me, feel free to carry over 1/2.
Cheers,
Benjamin
>
> Nick
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
