[PATCH v2 1/2] Input: Add device_enable handler to DaVinci Keyscan platform data

[PATCH v2 1/2] Input: Add device_enable handler to DaVinci Keyscan platform data

[miguel.aguilar]

From: Miguel Aguilar <miguel.aguilar@ridgerun.com>

Add a function pointer in the platform data of the DaVinci Keyscan driver
called device_enabled, in order to perform board specific actions when
the device is initialized, like setup the PINMUX configuration.

Signed-off-by: Miguel Aguilar <miguel.aguilar@ridgerun.com>
---
 arch/arm/mach-davinci/include/mach/keyscan.h |    1 +
 drivers/input/keyboard/davinci_keyscan.c     |    8 ++++++++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-davinci/include/mach/keyscan.h b/arch/arm/mach-davinci/include/mach/keyscan.h
index b4e21a2..7a560e0 100644
--- a/arch/arm/mach-davinci/include/mach/keyscan.h
+++ b/arch/arm/mach-davinci/include/mach/keyscan.h
@@ -29,6 +29,7 @@ enum davinci_matrix_types {
 };
 
 struct davinci_ks_platform_data {
+	int		(*device_enable)(struct device *dev);
 	unsigned short	*keymap;
 	u32		keymapsize;
 	u8		rep:1;
diff --git a/drivers/input/keyboard/davinci_keyscan.c b/drivers/input/keyboard/davinci_keyscan.c
index 6e52d85..d410d7a 100644
--- a/drivers/input/keyboard/davinci_keyscan.c
+++ b/drivers/input/keyboard/davinci_keyscan.c
@@ -174,6 +174,14 @@ static int __init davinci_ks_probe(struct platform_device *pdev)
 	struct davinci_ks_platform_data *pdata = pdev->dev.platform_data;
 	int error, i;
 
+	if (pdata->device_enable) {
+		error = pdata->device_enable(dev);
+		if (error < 0) {
+			dev_dbg(dev, "device enable function failed\n");
+			return error;
+		}
+	}
+
 	if (!pdata->keymap) {
 		dev_dbg(dev, "no keymap from pdata\n");
 		return -EINVAL;
-- 
1.6.0.4

Re: [PATCH v2 1/2] Input: Add device_enable handler to DaVinci Keyscan platform data

[Dmitry Torokhov]

On Fri, Nov 13, 2009 at 01:43:54PM -0600, miguel.aguilar@ridgerun.com wrote:
> From: Miguel Aguilar <miguel.aguilar@ridgerun.com>
> 
> Add a function pointer in the platform data of the DaVinci Keyscan driver
> called device_enabled, in order to perform board specific actions when
> the device is initialized, like setup the PINMUX configuration.
> 
> Signed-off-by: Miguel Aguilar <miguel.aguilar@ridgerun.com>
> ---
>  arch/arm/mach-davinci/include/mach/keyscan.h |    1 +
>  drivers/input/keyboard/davinci_keyscan.c     |    8 ++++++++
>  2 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-davinci/include/mach/keyscan.h b/arch/arm/mach-davinci/include/mach/keyscan.h
> index b4e21a2..7a560e0 100644
> --- a/arch/arm/mach-davinci/include/mach/keyscan.h
> +++ b/arch/arm/mach-davinci/include/mach/keyscan.h
> @@ -29,6 +29,7 @@ enum davinci_matrix_types {
>  };
>  
>  struct davinci_ks_platform_data {
> +	int		(*device_enable)(struct device *dev);
>  	unsigned short	*keymap;
>  	u32		keymapsize;
>  	u8		rep:1;
> diff --git a/drivers/input/keyboard/davinci_keyscan.c b/drivers/input/keyboard/davinci_keyscan.c
> index 6e52d85..d410d7a 100644
> --- a/drivers/input/keyboard/davinci_keyscan.c
> +++ b/drivers/input/keyboard/davinci_keyscan.c
> @@ -174,6 +174,14 @@ static int __init davinci_ks_probe(struct platform_device *pdev)
>  	struct davinci_ks_platform_data *pdata = pdev->dev.platform_data;
>  	int error, i;
>  
> +	if (pdata->device_enable) {
> +		error = pdata->device_enable(dev);
> +		if (error < 0) {
> +			dev_dbg(dev, "device enable function failed\n");
> +			return error;
> +		}
> +	}
> +

Hi Miguel,

Does this need to live in the driver? Why can't platform code do this
for us?

Thanks.

-- 
Dmitry

Re: [PATCH v2 1/2] Input: Add device_enable handler to DaVinci Keyscan platform data

[Miguel Aguilar]

Hi Dmitry,

>>  
>> +	if (pdata->device_enable) {
>> +		error = pdata->device_enable(dev);
>> +		if (error < 0) {
>> +			dev_dbg(dev, "device enable function failed\n");
>> +			return error;
>> +		}
>> +	}
>> +
> 
> Hi Miguel,
> 
> Does this need to live in the driver? Why can't platform code do this
> for us?
> 
> Thanks.
> 

The reason to invoke the device_enable function in the driver is because in the 
testing process of the key scan driver a issue was found when the key scan is 
built as a module.

There is a specific PINMUX configuration that only should be set if the key scan 
driver is loaded in the kernel to avoid pin conflicts. So when the key scan is 
built as a module the board file (or platform code) doesn't know if the key scan 
is loaded or not, so that's why the driver is the one who must invoke the 
device_enable function in the probe function.

Thanks,

Miguel Aguilar

Re: [PATCH v2 1/2] Input: Add device_enable handler to DaVinci Keyscan platform data

[Dmitry Torokhov]

On Thu, Nov 19, 2009 at 10:32:21AM -0600, Miguel Aguilar wrote:
> Hi Dmitry,
>
>>>  +	if (pdata->device_enable) {
>>> +		error = pdata->device_enable(dev);
>>> +		if (error < 0) {
>>> +			dev_dbg(dev, "device enable function failed\n");
>>> +			return error;
>>> +		}
>>> +	}
>>> +
>>
>> Hi Miguel,
>>
>> Does this need to live in the driver? Why can't platform code do this
>> for us?
>>
>> Thanks.
>>
>
> The reason to invoke the device_enable function in the driver is because 
> in the testing process of the key scan driver a issue was found when the 
> key scan is built as a module.
>
> There is a specific PINMUX configuration that only should be set if the 
> key scan driver is loaded in the kernel to avoid pin conflicts. So when 
> the key scan is built as a module the board file (or platform code) 
> doesn't know if the key scan is loaded or not, so that's why the driver 
> is the one who must invoke the device_enable function in the probe 
> function.
>

I see... What happens if PINMUX is set but there isn't a driver? Also,
what happens when you unload the module? Don't you need a similar call
to disable PINMUX configuration?

-- 
Dmitry

Re: [PATCH v2 1/2] Input: Add device_enable handler to DaVinci Keyscan platform data

[Miguel Aguilar]

Dmitry Torokhov wrote:
> On Thu, Nov 19, 2009 at 10:32:21AM -0600, Miguel Aguilar wrote:
>> Hi Dmitry,
>>
>>>>  +	if (pdata->device_enable) {
>>>> +		error = pdata->device_enable(dev);
>>>> +		if (error < 0) {
>>>> +			dev_dbg(dev, "device enable function failed\n");
>>>> +			return error;
>>>> +		}
>>>> +	}
>>>> +
>>> Hi Miguel,
>>>
>>> Does this need to live in the driver? Why can't platform code do this
>>> for us?
>>>
>>> Thanks.
>>>
>> The reason to invoke the device_enable function in the driver is because 
>> in the testing process of the key scan driver a issue was found when the 
>> key scan is built as a module.
>>
>> There is a specific PINMUX configuration that only should be set if the 
>> key scan driver is loaded in the kernel to avoid pin conflicts. So when 
>> the key scan is built as a module the board file (or platform code) 
>> doesn't know if the key scan is loaded or not, so that's why the driver 
>> is the one who must invoke the device_enable function in the probe 
>> function.
>>
> 
> I see... What happens if PINMUX is set but there isn't a driver? Also,
> what happens when you unload the module? Don't you need a similar call
> to disable PINMUX configuration?
> 

Dmitry,

If PINMUX is set and there isn't the driver, the PINMUX configuration can 
overwrite the PINMUX configuration needed by other drivers which actually are 
loaded. This situation happens in the DM365 EVM, because the hardware design 
there are hardware modules that can't coexist, so handling the PINMUX 
configuration calling a function pointer is a safe way to ensure that the PINMUX 
is set only when the module is loaded.

Disable PINMUX configuration doesn't make sense, because there isn't a thing 
such disable PINMUX configuration, there are a lot of possible PINMUX 
combinations, and there is no combination that means disable, and also change 
the PINMUX configuration to a different state would be risky.

Is the responsibility of the platform code or of the driver in the 
initialization, like in the case of the key scan, to set the needed PINMUX 
configuration for running the module properly.

Thanks,
Miguel Aguilar

Re: [PATCH v2 1/2] Input: Add device_enable handler to DaVinci Keyscan platform data

[Dmitry Torokhov]

On Thu, Nov 19, 2009 at 11:54:35AM -0600, Miguel Aguilar wrote:
> Dmitry Torokhov wrote:
>> On Thu, Nov 19, 2009 at 10:32:21AM -0600, Miguel Aguilar wrote:
>>> Hi Dmitry,
>>>
>>>>>  +	if (pdata->device_enable) {
>>>>> +		error = pdata->device_enable(dev);
>>>>> +		if (error < 0) {
>>>>> +			dev_dbg(dev, "device enable function failed\n");
>>>>> +			return error;
>>>>> +		}
>>>>> +	}
>>>>> +
>>>> Hi Miguel,
>>>>
>>>> Does this need to live in the driver? Why can't platform code do this
>>>> for us?
>>>>
>>>> Thanks.
>>>>
>>> The reason to invoke the device_enable function in the driver is 
>>> because in the testing process of the key scan driver a issue was 
>>> found when the key scan is built as a module.
>>>
>>> There is a specific PINMUX configuration that only should be set if 
>>> the key scan driver is loaded in the kernel to avoid pin conflicts. 
>>> So when the key scan is built as a module the board file (or platform 
>>> code) doesn't know if the key scan is loaded or not, so that's why 
>>> the driver is the one who must invoke the device_enable function in 
>>> the probe function.
>>>
>>
>> I see... What happens if PINMUX is set but there isn't a driver? Also,
>> what happens when you unload the module? Don't you need a similar call
>> to disable PINMUX configuration?
>>
>
> Dmitry,
>
> If PINMUX is set and there isn't the driver, the PINMUX configuration can 
> overwrite the PINMUX configuration needed by other drivers which actually 
> are loaded. This situation happens in the DM365 EVM, because the hardware 
> design there are hardware modules that can't coexist, so handling the 
> PINMUX configuration calling a function pointer is a safe way to ensure 
> that the PINMUX is set only when the module is loaded.
>

How do you ensure that only one of these drivers is loaded at a time?
Or is the set of supported devices is board-specific (in which case the
board code should have an idea how to properly set the configuration for
the devices that are supported on that board)?

> Disable PINMUX configuration doesn't make sense, because there isn't a 
> thing such disable PINMUX configuration, there are a lot of possible 
> PINMUX combinations, and there is no combination that means disable, and 
> also change the PINMUX configuration to a different state would be risky.
>

I see.

-- 
Dmitry

Re: [PATCH v2 1/2] Input: Add device_enable handler to DaVinci Keyscan platform data

[Miguel Aguilar]

Dmitry Torokhov wrote:
> On Thu, Nov 19, 2009 at 11:54:35AM -0600, Miguel Aguilar wrote:
>> Dmitry Torokhov wrote:
>>> On Thu, Nov 19, 2009 at 10:32:21AM -0600, Miguel Aguilar wrote:
>>>> Hi Dmitry,
>>>>
>>>>>>  +	if (pdata->device_enable) {
>>>>>> +		error = pdata->device_enable(dev);
>>>>>> +		if (error < 0) {
>>>>>> +			dev_dbg(dev, "device enable function failed\n");
>>>>>> +			return error;
>>>>>> +		}
>>>>>> +	}
>>>>>> +
>>>>> Hi Miguel,
>>>>>
>>>>> Does this need to live in the driver? Why can't platform code do this
>>>>> for us?
>>>>>
>>>>> Thanks.
>>>>>
>>>> The reason to invoke the device_enable function in the driver is 
>>>> because in the testing process of the key scan driver a issue was 
>>>> found when the key scan is built as a module.
>>>>
>>>> There is a specific PINMUX configuration that only should be set if 
>>>> the key scan driver is loaded in the kernel to avoid pin conflicts. 
>>>> So when the key scan is built as a module the board file (or platform 
>>>> code) doesn't know if the key scan is loaded or not, so that's why 
>>>> the driver is the one who must invoke the device_enable function in 
>>>> the probe function.
>>>>
>>> I see... What happens if PINMUX is set but there isn't a driver? Also,
>>> what happens when you unload the module? Don't you need a similar call
>>> to disable PINMUX configuration?
>>>
>> Dmitry,
>>
>> If PINMUX is set and there isn't the driver, the PINMUX configuration can 
>> overwrite the PINMUX configuration needed by other drivers which actually 
>> are loaded. This situation happens in the DM365 EVM, because the hardware 
>> design there are hardware modules that can't coexist, so handling the 
>> PINMUX configuration calling a function pointer is a safe way to ensure 
>> that the PINMUX is set only when the module is loaded.
>>
> 
> How do you ensure that only one of these drivers is loaded at a time?
> Or is the set of supported devices is board-specific (in which case the
> board code should have an idea how to properly set the configuration for
> the devices that are supported on that board)?
The DM365 EVM has other device that can't coexist with key scan, so this is a 
particular situation. There no way to ensure that one of this modules is loaded 
at time, but using device_enable handler is a proper way to ensure that a driver 
will be loaded properly, the board code can't assume which driver is going to be 
loaded.

I think Sneha, can bring you more details about this particular case.
> 
>> Disable PINMUX configuration doesn't make sense, because there isn't a 
>> thing such disable PINMUX configuration, there are a lot of possible 
>> PINMUX combinations, and there is no combination that means disable, and 
>> also change the PINMUX configuration to a different state would be risky.
>>
> 
> I see.
> 

RE: [PATCH v2 1/2] Input: Add device_enable handler to DaVinci Keyscan platform data

[Narnakaje, Snehaprabha]

[...]

> > How do you ensure that only one of these drivers is loaded at a time?
> > Or is the set of supported devices is board-specific (in which case the
> > board code should have an idea how to properly set the configuration for
> > the devices that are supported on that board)?
> The DM365 EVM has other device that can't coexist with key scan, so this
> is a
> particular situation. There no way to ensure that one of this modules is
> loaded
> at time, but using device_enable handler is a proper way to ensure that a
> driver
> will be loaded properly, the board code can't assume which driver is going
> to be
> loaded.
> 
> I think Sneha, can bring you more details about this particular case.

Ideally PINMUX settings and any other EVM/board related initializations should be done in the board-setup files, when the platform_data for the driver is registered. Our PINMUX configuration APIs do not really have resource management capabilities to remember what drivers/peripherals have taken the resource/bits.

As Miguel mentioned, we have PINMUX conflict between Keyscan lines and the EMIF address lines used by CPLD. PINMUX2 register can only select one of configurations. CPLD is required for the video capture driver which should be enabled all the time in a system. So Miguel in his initial patch called the keyscan_init function within the #ifdef CONFIG_KEYSCAN construct. The keyscan_init function handled the PINMUX configuration for keyscan and also registered the platform_device. The general guideline in the board-setup files is that we should always let platform_device registered. The driver is enabled/disabled anyway using the CONFIG option.

The default configuration for the board does not enable Keyscan. One can test/use kescyan driver by enabling the CONFIG option, but CPLD (video capture input selection, to be specific) configuration will not be available. I would say, it is a limitation in the board/EVM design. Other board designs can have CPLD use different address lines, in which case the CONFIG option can be enabled in the default configuration.

Thanks
Sneha

Re: [PATCH v2 1/2] Input: Add device_enable handler to DaVinci Keyscan platform data

[Miguel Aguilar]

Hi Dmitry,

Is this patch ready to be pushed based on the justification that we bring about it?.

Thanks,
Miguel Aguilar

Narnakaje, Snehaprabha wrote:
> [...]
> 
>>> How do you ensure that only one of these drivers is loaded at a time?
>>> Or is the set of supported devices is board-specific (in which case the
>>> board code should have an idea how to properly set the configuration for
>>> the devices that are supported on that board)?
>> The DM365 EVM has other device that can't coexist with key scan, so this
>> is a
>> particular situation. There no way to ensure that one of this modules is
>> loaded
>> at time, but using device_enable handler is a proper way to ensure that a
>> driver
>> will be loaded properly, the board code can't assume which driver is going
>> to be
>> loaded.
>>
>> I think Sneha, can bring you more details about this particular case.
> 
> Ideally PINMUX settings and any other EVM/board related initializations should be done in the board-setup files, when the platform_data for the driver is registered. Our PINMUX configuration APIs do not really have resource management capabilities to remember what drivers/peripherals have taken the resource/bits.
> 
> As Miguel mentioned, we have PINMUX conflict between Keyscan lines and the EMIF address lines used by CPLD. PINMUX2 register can only select one of configurations. CPLD is required for the video capture driver which should be enabled all the time in a system. So Miguel in his initial patch called the keyscan_init function within the #ifdef CONFIG_KEYSCAN construct. The keyscan_init function handled the PINMUX configuration for keyscan and also registered the platform_device. The general guideline in the board-setup files is that we should always let platform_device registered. The driver is enabled/disabled anyway using the CONFIG option.
> 
> The default configuration for the board does not enable Keyscan. One can test/use kescyan driver by enabling the CONFIG option, but CPLD (video capture input selection, to be specific) configuration will not be available. I would say, it is a limitation in the board/EVM design. Other board designs can have CPLD use different address lines, in which case the CONFIG option can be enabled in the default configuration.
> 
> Thanks
> Sneha
> 

RE: [PATCH v2 1/2] Input: Add device_enable handler to DaVinci Keyscan platform data

[Steve Chen]

On Thu, 2009-11-19 at 15:33 -0600, Narnakaje, Snehaprabha wrote:
> [...]
> 
> > > How do you ensure that only one of these drivers is loaded at a time?
> > > Or is the set of supported devices is board-specific (in which case the
> > > board code should have an idea how to properly set the configuration for
> > > the devices that are supported on that board)?
> > The DM365 EVM has other device that can't coexist with key scan, so this
> > is a
> > particular situation. There no way to ensure that one of this modules is
> > loaded
> > at time, but using device_enable handler is a proper way to ensure that a
> > driver
> > will be loaded properly, the board code can't assume which driver is going
> > to be
> > loaded.
> > 
> > I think Sneha, can bring you more details about this particular
> case.
> 
> Ideally PINMUX settings and any other EVM/board related
> initializations should be done in the board-setup files, when the
> platform_data for the driver is registered. Our PINMUX configuration
> APIs do not really have resource management capabilities to remember
> what drivers/peripherals have taken the resource/bits.
> 
> As Miguel mentioned, we have PINMUX conflict between Keyscan lines and
> the EMIF address lines used by CPLD. PINMUX2 register can only select
> one of configurations. CPLD is required for the video capture driver
> which should be enabled all the time in a system. So Miguel in his
> initial patch called the keyscan_init function within the #ifdef
> CONFIG_KEYSCAN construct. The keyscan_init function handled the PINMUX
> configuration for keyscan and also registered the platform_device. The
> general guideline in the board-setup files is that we should always
> let platform_device registered. The driver is enabled/disabled anyway
> using the CONFIG option.
> 
> The default configuration for the board does not enable Keyscan. One
> can test/use kescyan driver by enabling the CONFIG option, but CPLD
> (video capture input selection, to be specific) configuration will not
> be available. I would say, it is a limitation in the board/EVM design.
> Other board designs can have CPLD use different address lines, in
> which case the CONFIG option can be enabled in the default
> configuration.
> 
> Thanks
> Sneha
> 

During DA830/OMAP-L137 development in MVL 5, automatic pinmux detection
was added.  This allows pinmux configuration to change dynamically as
modules are loaded/unloaded.  When pinmux conflict is detected, driver
load would fail and the user is notified via error message with the
exact pins that caused the failure.

The pinmux conflict detection was tied to the clock interface, and the
patch was rejected by the community for that reason.  If you think it
would be helpful, you are welcome to look at the MVL Pro 5.0 code and
the patch submitted in the DaVinci mailing list archive.

Regards,

Steve

Re: [PATCH v2 1/2] Input: Add device_enable handler to DaVinci Keyscan platform data

[Kevin Hilman]

<miguel.aguilar@ridgerun.com> writes:

> From: Miguel Aguilar <miguel.aguilar@ridgerun.com>
>
> Add a function pointer in the platform data of the DaVinci Keyscan driver
> called device_enabled, in order to perform board specific actions when
> the device is initialized, like setup the PINMUX configuration.
>
> Signed-off-by: Miguel Aguilar <miguel.aguilar@ridgerun.com>

Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>

Dmitry,

Will you be queueing this driver (and this patch) for 2.6.34?  I
thought you had accepted the original driver, but I don't see it in
the master or next branch of your input tree at:

  git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git

Thanks or clarifying.

Kevin


> ---
>  arch/arm/mach-davinci/include/mach/keyscan.h |    1 +
>  drivers/input/keyboard/davinci_keyscan.c     |    8 ++++++++
>  2 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-davinci/include/mach/keyscan.h b/arch/arm/mach-davinci/include/mach/keyscan.h
> index b4e21a2..7a560e0 100644
> --- a/arch/arm/mach-davinci/include/mach/keyscan.h
> +++ b/arch/arm/mach-davinci/include/mach/keyscan.h
> @@ -29,6 +29,7 @@ enum davinci_matrix_types {
>  };
>  
>  struct davinci_ks_platform_data {
> +	int		(*device_enable)(struct device *dev);
>  	unsigned short	*keymap;
>  	u32		keymapsize;
>  	u8		rep:1;
> diff --git a/drivers/input/keyboard/davinci_keyscan.c b/drivers/input/keyboard/davinci_keyscan.c
> index 6e52d85..d410d7a 100644
> --- a/drivers/input/keyboard/davinci_keyscan.c
> +++ b/drivers/input/keyboard/davinci_keyscan.c
> @@ -174,6 +174,14 @@ static int __init davinci_ks_probe(struct platform_device *pdev)
>  	struct davinci_ks_platform_data *pdata = pdev->dev.platform_data;
>  	int error, i;
>  
> +	if (pdata->device_enable) {
> +		error = pdata->device_enable(dev);
> +		if (error < 0) {
> +			dev_dbg(dev, "device enable function failed\n");
> +			return error;
> +		}
> +	}
> +
>  	if (!pdata->keymap) {
>  		dev_dbg(dev, "no keymap from pdata\n");
>  		return -EINVAL;
> -- 
> 1.6.0.4
>
>
> _______________________________________________
> Davinci-linux-open-source mailing list
> Davinci-linux-open-source@linux.davincidsp.com
> http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source

Re: [PATCH v2 1/2] Input: Add device_enable handler to DaVinci Keyscan platform data

[Dmitry Torokhov]

On Mon, Dec 07, 2009 at 04:24:59PM -0800, Kevin Hilman wrote:
> <miguel.aguilar@ridgerun.com> writes:
> 
> > From: Miguel Aguilar <miguel.aguilar@ridgerun.com>
> >
> > Add a function pointer in the platform data of the DaVinci Keyscan driver
> > called device_enabled, in order to perform board specific actions when
> > the device is initialized, like setup the PINMUX configuration.
> >
> > Signed-off-by: Miguel Aguilar <miguel.aguilar@ridgerun.com>
> 
> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> 
> Dmitry,
> 
> Will you be queueing this driver (and this patch) for 2.6.34?  I
> thought you had accepted the original driver, but I don't see it in
> the master or next branch of your input tree at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git

The driver is there, commit bc09dcadc1a3da87d58aa70ebc8e9441205be75c on
'next' branch (I don't really use master branch).  It was committed back
in October, probably that's why you don't see it?

As far as the patch goes - I got an impression from email sent by Steve
that there are ways to do automatic PINMUX detection and thus this patch
was not needed. Is this ture? If not I am stull unsure what happens if
you unload the driver. How do you restore the old configuration so that
the device you took over from can start working again? Maybe pinmux
should be controlled via sysfs attribute (in board code) so that user
can switch on-fly between the devices?

Thanks.

-- 
Dmitry

Re: [PATCH v2 1/2] Input: Add device_enable handler to DaVinci Keyscan platform data

[Kevin Hilman]

Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:

> On Mon, Dec 07, 2009 at 04:24:59PM -0800, Kevin Hilman wrote:
>> <miguel.aguilar@ridgerun.com> writes:
>> 
>> > From: Miguel Aguilar <miguel.aguilar@ridgerun.com>
>> >
>> > Add a function pointer in the platform data of the DaVinci Keyscan driver
>> > called device_enabled, in order to perform board specific actions when
>> > the device is initialized, like setup the PINMUX configuration.
>> >
>> > Signed-off-by: Miguel Aguilar <miguel.aguilar@ridgerun.com>
>> 
>> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
>> 
>> Dmitry,
>> 
>> Will you be queueing this driver (and this patch) for 2.6.34?  I
>> thought you had accepted the original driver, but I don't see it in
>> the master or next branch of your input tree at:
>> 
>>   git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git
>
> The driver is there, commit bc09dcadc1a3da87d58aa70ebc8e9441205be75c on
> 'next' branch (I don't really use master branch).  It was committed back
> in October, probably that's why you don't see it?
>
> As far as the patch goes - I got an impression from email sent by
> Steve that there are ways to do automatic PINMUX detection and thus
> this patch was not needed.  Is this ture?

The method Steve was referring to was done for MontaVista
internal/product kernels but was rejected for upstream (by me) because
of the way it was implemented (by tying mux settings to clock
settings.)

> If not I am stull unsure what happens if you unload the driver. How
> do you restore the old configuration so that the device you took
> over from can start working again? Maybe pinmux should be controlled
> via sysfs attribute (in board code) so that user can switch on-fly
> between the devices?

The way we currently handle MUXing in davinci, you don't need to do
anything after the driver unloads.  Any other user of these pins will
mux them as needed.

If really necessary, we could do an equivalent 'device_disable' hook
but there would be empty as it wouldn't be needed.

So, speaking as maintainer of the DaVinci support, I'm in favor of this
approach from Miguel.

Thanks,


Kevin

Re: [PATCH v2 1/2] Input: Add device_enable handler to DaVinci Keyscan platform data

[Kevin Hilman]

Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org> writes:

> Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
>
>> On Mon, Dec 07, 2009 at 04:24:59PM -0800, Kevin Hilman wrote:
>>> <miguel.aguilar-9uBrGCPFOa1Wk0Htik3J/w@public.gmane.org> writes:
>>> 
>>> > From: Miguel Aguilar <miguel.aguilar-9uBrGCPFOa1Wk0Htik3J/w@public.gmane.org>
>>> >
>>> > Add a function pointer in the platform data of the DaVinci Keyscan driver
>>> > called device_enabled, in order to perform board specific actions when
>>> > the device is initialized, like setup the PINMUX configuration.
>>> >
>>> > Signed-off-by: Miguel Aguilar <miguel.aguilar-9uBrGCPFOa1Wk0Htik3J/w@public.gmane.org>
>>> 
>>> Signed-off-by: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
>>> 
>>> Dmitry,
>>> 
>>> Will you be queueing this driver (and this patch) for 2.6.34?  I
>>> thought you had accepted the original driver, but I don't see it in
>>> the master or next branch of your input tree at:
>>> 
>>>   git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git
>>
>> The driver is there, commit bc09dcadc1a3da87d58aa70ebc8e9441205be75c on
>> 'next' branch (I don't really use master branch).  It was committed back
>> in October, probably that's why you don't see it?
>>
>> As far as the patch goes - I got an impression from email sent by
>> Steve that there are ways to do automatic PINMUX detection and thus
>> this patch was not needed.  Is this ture?
>
> The method Steve was referring to was done for MontaVista
> internal/product kernels but was rejected for upstream (by me) because
> of the way it was implemented (by tying mux settings to clock
> settings.)
>
>> If not I am stull unsure what happens if you unload the driver. How
>> do you restore the old configuration so that the device you took
>> over from can start working again? Maybe pinmux should be controlled
>> via sysfs attribute (in board code) so that user can switch on-fly
>> between the devices?
>
> The way we currently handle MUXing in davinci, you don't need to do
> anything after the driver unloads.  Any other user of these pins will
> mux them as needed.
>
> If really necessary, we could do an equivalent 'device_disable' hook
> but there would be empty as it wouldn't be needed.
>
> So, speaking as maintainer of the DaVinci support, I'm in favor of this
> approach from Miguel.

Dmitry,

Unless there are further objectsions, could you please queue this
patch for 2.6.34 with my signoff?

Thanks,

Kevin

Re: [PATCH v2 1/2] Input: Add device_enable handler to DaVinci Keyscan platform data

[Dmitry Torokhov]

On Tue, Jan 05, 2010 at 04:21:11PM -0800, Kevin Hilman wrote:
> Kevin Hilman <khilman@deeprootsystems.com> writes:
> 
> > Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
> >
> >> On Mon, Dec 07, 2009 at 04:24:59PM -0800, Kevin Hilman wrote:
> >>> <miguel.aguilar@ridgerun.com> writes:
> >>> 
> >>> > From: Miguel Aguilar <miguel.aguilar@ridgerun.com>
> >>> >
> >>> > Add a function pointer in the platform data of the DaVinci Keyscan driver
> >>> > called device_enabled, in order to perform board specific actions when
> >>> > the device is initialized, like setup the PINMUX configuration.
> >>> >
> >>> > Signed-off-by: Miguel Aguilar <miguel.aguilar@ridgerun.com>
> >>> 
> >>> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> >>> 
> >>> Dmitry,
> >>> 
> >>> Will you be queueing this driver (and this patch) for 2.6.34?  I
> >>> thought you had accepted the original driver, but I don't see it in
> >>> the master or next branch of your input tree at:
> >>> 
> >>>   git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git
> >>
> >> The driver is there, commit bc09dcadc1a3da87d58aa70ebc8e9441205be75c on
> >> 'next' branch (I don't really use master branch).  It was committed back
> >> in October, probably that's why you don't see it?
> >>
> >> As far as the patch goes - I got an impression from email sent by
> >> Steve that there are ways to do automatic PINMUX detection and thus
> >> this patch was not needed.  Is this ture?
> >
> > The method Steve was referring to was done for MontaVista
> > internal/product kernels but was rejected for upstream (by me) because
> > of the way it was implemented (by tying mux settings to clock
> > settings.)
> >
> >> If not I am stull unsure what happens if you unload the driver. How
> >> do you restore the old configuration so that the device you took
> >> over from can start working again? Maybe pinmux should be controlled
> >> via sysfs attribute (in board code) so that user can switch on-fly
> >> between the devices?
> >
> > The way we currently handle MUXing in davinci, you don't need to do
> > anything after the driver unloads.  Any other user of these pins will
> > mux them as needed.
> >
> > If really necessary, we could do an equivalent 'device_disable' hook
> > but there would be empty as it wouldn't be needed.
> >
> > So, speaking as maintainer of the DaVinci support, I'm in favor of this
> > approach from Miguel.
> 
> Dmitry,
> 
> Unless there are further objectsions, could you please queue this
> patch for 2.6.34 with my signoff?
> 

Applied to for-linus, I do not see any reason in holding this patch
till .34. Sorry for the delay.

-- 
Dmitry

Re: [PATCH v2 1/2] Input: Add device_enable handler to DaVinci Keyscan platform data

[Kevin Hilman]

Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:

> On Tue, Jan 05, 2010 at 04:21:11PM -0800, Kevin Hilman wrote:
>> Kevin Hilman <khilman@deeprootsystems.com> writes:
>> 
>> > Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
>> >
>> >> On Mon, Dec 07, 2009 at 04:24:59PM -0800, Kevin Hilman wrote:
>> >>> <miguel.aguilar@ridgerun.com> writes:
>> >>> 
>> >>> > From: Miguel Aguilar <miguel.aguilar@ridgerun.com>
>> >>> >
>> >>> > Add a function pointer in the platform data of the DaVinci Keyscan driver
>> >>> > called device_enabled, in order to perform board specific actions when
>> >>> > the device is initialized, like setup the PINMUX configuration.
>> >>> >
>> >>> > Signed-off-by: Miguel Aguilar <miguel.aguilar@ridgerun.com>
>> >>> 
>> >>> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
>> >>> 
>> >>> Dmitry,
>> >>> 
>> >>> Will you be queueing this driver (and this patch) for 2.6.34?  I
>> >>> thought you had accepted the original driver, but I don't see it in
>> >>> the master or next branch of your input tree at:
>> >>> 
>> >>>   git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git
>> >>
>> >> The driver is there, commit bc09dcadc1a3da87d58aa70ebc8e9441205be75c on
>> >> 'next' branch (I don't really use master branch).  It was committed back
>> >> in October, probably that's why you don't see it?
>> >>
>> >> As far as the patch goes - I got an impression from email sent by
>> >> Steve that there are ways to do automatic PINMUX detection and thus
>> >> this patch was not needed.  Is this ture?
>> >
>> > The method Steve was referring to was done for MontaVista
>> > internal/product kernels but was rejected for upstream (by me) because
>> > of the way it was implemented (by tying mux settings to clock
>> > settings.)
>> >
>> >> If not I am stull unsure what happens if you unload the driver. How
>> >> do you restore the old configuration so that the device you took
>> >> over from can start working again? Maybe pinmux should be controlled
>> >> via sysfs attribute (in board code) so that user can switch on-fly
>> >> between the devices?
>> >
>> > The way we currently handle MUXing in davinci, you don't need to do
>> > anything after the driver unloads.  Any other user of these pins will
>> > mux them as needed.
>> >
>> > If really necessary, we could do an equivalent 'device_disable' hook
>> > but there would be empty as it wouldn't be needed.
>> >
>> > So, speaking as maintainer of the DaVinci support, I'm in favor of this
>> > approach from Miguel.
>> 
>> Dmitry,
>> 
>> Unless there are further objectsions, could you please queue this
>> patch for 2.6.34 with my signoff?
>> 
>
> Applied to for-linus, I do not see any reason in holding this patch
> till .34. Sorry for the delay.
>

Excellent.  Thanks.

I'll submit the platform specific code for -rc4 as well.

Kevin