| Message ID | 1392269277-16391-3-git-send-email-dmitry.torokhov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/12/2014 09:27 PM, Dmitry Torokhov wrote: > Do not write configuration data in probe(), we have config() for that. Then we should call config() in rmi_function_probe() to ensure that any platform data or device tree configuration settings get written to the device. Thinking about that, it looks like it's not fatal if the config write fails in that situation. The device might not function as intended, but you can hopefully get some use out of it (for instance, a phone's touchscreen sensitivity might be wacky, but the user will still be able to dial tech support). For example: static int rmi_function_probe(struct device *dev) { struct rmi_function *fn = to_rmi_function(dev); struct rmi_function_handler *handler = to_rmi_function_handler(dev->driver); int error; if (handler->probe) { error = handler->probe(fn); if (error) return error; } if (handler->config) { error = handler->config(fn); if (error) dev_warn(dev, "Driver config failed.\n"); } return 0; } > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/input/rmi4/rmi_f01.c | 18 ------------------ > 1 file changed, 18 deletions(-) > > diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c > index 897d8ac..976aba3 100644 > --- a/drivers/input/rmi4/rmi_f01.c > +++ b/drivers/input/rmi4/rmi_f01.c > @@ -303,12 +303,6 @@ static int rmi_f01_initialize(struct rmi_function *fn) > if (pdata->power_management.doze_interval) { > data->device_control.doze_interval = > pdata->power_management.doze_interval; > - error = rmi_write(rmi_dev, data->doze_interval_addr, > - data->device_control.doze_interval); > - if (error < 0) { > - dev_err(&fn->dev, "Failed to configure F01 doze interval register.\n"); > - return error; > - } > } else { > error = rmi_read(rmi_dev, data->doze_interval_addr, > &data->device_control.doze_interval); > @@ -324,12 +318,6 @@ static int rmi_f01_initialize(struct rmi_function *fn) > if (pdata->power_management.wakeup_threshold) { > data->device_control.wakeup_threshold = > pdata->power_management.wakeup_threshold; > - error = rmi_write(rmi_dev, data->wakeup_threshold_addr, > - data->device_control.wakeup_threshold); > - if (error < 0) { > - dev_err(&fn->dev, "Failed to configure F01 wakeup threshold register.\n"); > - return error; > - } > } else { > error = rmi_read(rmi_dev, data->wakeup_threshold_addr, > &data->device_control.wakeup_threshold); > @@ -347,12 +335,6 @@ static int rmi_f01_initialize(struct rmi_function *fn) > if (pdata->power_management.doze_holdoff) { > data->device_control.doze_holdoff = > pdata->power_management.doze_holdoff; > - error = rmi_write(rmi_dev, data->doze_holdoff_addr, > - data->device_control.doze_holdoff); > - if (error < 0) { > - dev_err(&fn->dev, "Failed to configure F01 doze holdoff register.\n"); > - return error; > - } > } else { > error = rmi_read(rmi_dev, data->doze_holdoff_addr, > &data->device_control.doze_holdoff); >
On Thu, Feb 13, 2014 at 11:23:44AM -0800, Christopher Heiny wrote: > On 02/12/2014 09:27 PM, Dmitry Torokhov wrote: > >Do not write configuration data in probe(), we have config() for that. > > Then we should call config() in rmi_function_probe() to ensure that > any platform data or device tree configuration settings get written > to the device. Well, yes, we may elect to update device configuration in probe, but then we should not be doing that 2nd time in ->config(). We shoudl pick either one or another. Thanks.
On 02/13/2014 01:54 PM, Dmitry Torokhov wrote: > On Thu, Feb 13, 2014 at 11:23:44AM -0800, Christopher Heiny wrote: >> >On 02/12/2014 09:27 PM, Dmitry Torokhov wrote: >>> > >Do not write configuration data in probe(), we have config() for that. >> > >> >Then we should call config() in rmi_function_probe() to ensure that >> >any platform data or device tree configuration settings get written >> >to the device. > > Well, yes, we may elect to update device configuration in probe, but > then we should not be doing that 2nd time in ->config(). We shoudl pick > either one or another. But as the code currently stands, config() is only called when a device reset is detected, not during initialization. So if there's platform specific configuration data that needs to be written to a function, it won't get written until after a device reset occurs, which might never happen. So either we need to write that data (or call config()) in each function's probe(), or in rmi_function_probe(). -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 14, 2014 at 03:00:43PM -0800, Christopher Heiny wrote: > On 02/13/2014 01:54 PM, Dmitry Torokhov wrote: > >On Thu, Feb 13, 2014 at 11:23:44AM -0800, Christopher Heiny wrote: > >>>On 02/12/2014 09:27 PM, Dmitry Torokhov wrote: > >>>> >Do not write configuration data in probe(), we have config() for that. > >>> > >>>Then we should call config() in rmi_function_probe() to ensure that > >>>any platform data or device tree configuration settings get written > >>>to the device. > > > >Well, yes, we may elect to update device configuration in probe, but > >then we should not be doing that 2nd time in ->config(). We shoudl pick > >either one or another. > > But as the code currently stands, config() is only called when a > device reset is detected, not during initialization. So if there's > platform specific configuration data that needs to be written to a > function, it won't get written until after a device reset occurs, > which might never happen. So either we need to write that data (or > call config()) in each function's probe(), or in > rmi_function_probe(). Ah, I missed the fact that we do not normally call ->config() unless device was reset. BTW, if device was reset, shouldn't we tear down everything and then reenumerate all functions?
On 02/17/2014 11:23 AM, Dmitry Torokhov wrote: > On Fri, Feb 14, 2014 at 03:00:43PM -0800, Christopher Heiny wrote: >> On 02/13/2014 01:54 PM, Dmitry Torokhov wrote: >>> On Thu, Feb 13, 2014 at 11:23:44AM -0800, Christopher Heiny wrote: >>>>> On 02/12/2014 09:27 PM, Dmitry Torokhov wrote: >>>>>>> Do not write configuration data in probe(), we have config() for that. >>>>> >>>>> Then we should call config() in rmi_function_probe() to ensure that >>>>> any platform data or device tree configuration settings get written >>>>> to the device. >>> >>> Well, yes, we may elect to update device configuration in probe, but >>> then we should not be doing that 2nd time in ->config(). We shoudl pick >>> either one or another. >> >> But as the code currently stands, config() is only called when a >> device reset is detected, not during initialization. So if there's >> platform specific configuration data that needs to be written to a >> function, it won't get written until after a device reset occurs, >> which might never happen. So either we need to write that data (or >> call config()) in each function's probe(), or in >> rmi_function_probe(). > > Ah, I missed the fact that we do not normally call ->config() unless > device was reset. BTW, if device was reset, shouldn't we tear down > everything and then reenumerate all functions? That's only required if the reset is a result of the device being reflashed. Since we're dropping support for user-space control of reflash, and will instead use an in-driver reflash, we know which resets are the result of a reflash and which aren't. The reflash code will do the following sequence: - tell the core to tear down the functions - perform the reflash operation - reset the device - tell the core to re-enumerate the functions Chris -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/12/2014 09:27 PM, Dmitry Torokhov wrote: > Do not write configuration data in probe(), we have config() for that. I've just submitted a patch to correctly call config() after probe(). So this becomes... Signed-off-by: Christopher Heiny <cheiny@synaptics.com> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/input/rmi4/rmi_f01.c | 18 ------------------ > 1 file changed, 18 deletions(-) > > diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c > index 897d8ac..976aba3 100644 > --- a/drivers/input/rmi4/rmi_f01.c > +++ b/drivers/input/rmi4/rmi_f01.c > @@ -303,12 +303,6 @@ static int rmi_f01_initialize(struct rmi_function *fn) > if (pdata->power_management.doze_interval) { > data->device_control.doze_interval = > pdata->power_management.doze_interval; > - error = rmi_write(rmi_dev, data->doze_interval_addr, > - data->device_control.doze_interval); > - if (error < 0) { > - dev_err(&fn->dev, "Failed to configure F01 doze interval register.\n"); > - return error; > - } > } else { > error = rmi_read(rmi_dev, data->doze_interval_addr, > &data->device_control.doze_interval); > @@ -324,12 +318,6 @@ static int rmi_f01_initialize(struct rmi_function *fn) > if (pdata->power_management.wakeup_threshold) { > data->device_control.wakeup_threshold = > pdata->power_management.wakeup_threshold; > - error = rmi_write(rmi_dev, data->wakeup_threshold_addr, > - data->device_control.wakeup_threshold); > - if (error < 0) { > - dev_err(&fn->dev, "Failed to configure F01 wakeup threshold register.\n"); > - return error; > - } > } else { > error = rmi_read(rmi_dev, data->wakeup_threshold_addr, > &data->device_control.wakeup_threshold); > @@ -347,12 +335,6 @@ static int rmi_f01_initialize(struct rmi_function *fn) > if (pdata->power_management.doze_holdoff) { > data->device_control.doze_holdoff = > pdata->power_management.doze_holdoff; > - error = rmi_write(rmi_dev, data->doze_holdoff_addr, > - data->device_control.doze_holdoff); > - if (error < 0) { > - dev_err(&fn->dev, "Failed to configure F01 doze holdoff register.\n"); > - return error; > - } > } else { > error = rmi_read(rmi_dev, data->doze_holdoff_addr, > &data->device_control.doze_holdoff); >
diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c index 897d8ac..976aba3 100644 --- a/drivers/input/rmi4/rmi_f01.c +++ b/drivers/input/rmi4/rmi_f01.c @@ -303,12 +303,6 @@ static int rmi_f01_initialize(struct rmi_function *fn) if (pdata->power_management.doze_interval) { data->device_control.doze_interval = pdata->power_management.doze_interval; - error = rmi_write(rmi_dev, data->doze_interval_addr, - data->device_control.doze_interval); - if (error < 0) { - dev_err(&fn->dev, "Failed to configure F01 doze interval register.\n"); - return error; - } } else { error = rmi_read(rmi_dev, data->doze_interval_addr, &data->device_control.doze_interval); @@ -324,12 +318,6 @@ static int rmi_f01_initialize(struct rmi_function *fn) if (pdata->power_management.wakeup_threshold) { data->device_control.wakeup_threshold = pdata->power_management.wakeup_threshold; - error = rmi_write(rmi_dev, data->wakeup_threshold_addr, - data->device_control.wakeup_threshold); - if (error < 0) { - dev_err(&fn->dev, "Failed to configure F01 wakeup threshold register.\n"); - return error; - } } else { error = rmi_read(rmi_dev, data->wakeup_threshold_addr, &data->device_control.wakeup_threshold); @@ -347,12 +335,6 @@ static int rmi_f01_initialize(struct rmi_function *fn) if (pdata->power_management.doze_holdoff) { data->device_control.doze_holdoff = pdata->power_management.doze_holdoff; - error = rmi_write(rmi_dev, data->doze_holdoff_addr, - data->device_control.doze_holdoff); - if (error < 0) { - dev_err(&fn->dev, "Failed to configure F01 doze holdoff register.\n"); - return error; - } } else { error = rmi_read(rmi_dev, data->doze_holdoff_addr, &data->device_control.doze_holdoff);
Do not write configuration data in probe(), we have config() for that. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/input/rmi4/rmi_f01.c | 18 ------------------ 1 file changed, 18 deletions(-)