Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
On 6/22/2010 4:15 PM, Eric Miao wrote:
> On Tue, Jun 22, 2010 at 12:00 PM, Joonyoung Shim
> wrote:
>> On 6/22/2010 12:38 PM, Eric Miao wrote:
>>> On Tue, Jun 22, 2010 at 11:27 AM, Joonyoung Shim
>>> wrote:
On 6/22/2010 12:02 PM, Eric Miao wrote:
> On Tue, Jun 22, 2010 at 8:48 AM, Joonyoung Shim
> wrote:
>> On 6/21/2010 8:16 PM, Russell King - ARM Linux wrote:
>>> On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
wrote:
> On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
>> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim
>> wrote:
>>> +void __init samsung_keypad_set_platdata(struct
>>> samsung_keypad_platdata *pd)
>>> +{
>>> + � � � struct samsung_keypad_platdata *npd;
>>> +
>>> + � � � if (!pd) {
>>> + � � � � � � � printk(KERN_ERR "%s: no platform data\n", __func__);
>>> + � � � � � � � return;
>>> + � � � }
>>> +
>>> + � � � npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata),
>>> GFP_KERNEL);
>>> + � � � if (!npd)
>>> + � � � � � � � printk(KERN_ERR "%s: no memory for platform
>>> data\n", __func__);
>> This part of the code is actually duplicated again and again and
>> again
>> for each device, PXA and other legacy platforms are bad references
>> for
>> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are
>> three
>> major points:
>>
>> �1. A minimum 'struct pxa_device_desc' for a simple description of a
>> � � device (more than 90% of the devices can be described that way),
>> � � and avoid using a comparatively heavier weight platform_device,
>> � � which can be generated at run-time
>>
>> �2. pxa_register_device() to allocate and register the
>> platform_device
>> � � at run-time, along with the platform data
> It's a bad idea to make platform data be run-time discardable like
> this:
>
>>> +struct samsung_keypad_platdata {
>>> + � � � const struct matrix_keymap_data *keymap_data;
> What you end up with is some platform data structures which must be
> kept
> (those which have pointers to them from the platform data), and others
> (the platform data itself) which can be discarded at runtime.
>
> We know that the __initdata attributations cause lots of problems -
> they're frequently wrong. �Just see the constant hastle with __devinit
> et.al. �The same issue happens with __initdata as well.
>
> So why make things more complicated by allowing some platform data
> structures to be discardable and others not to be? �Is their small
> size (maybe 6 words for this one) really worth the hastle of getting
> __initdata attributations wrong (eg, on the keymap data?)
>
Russell,
The benefit I see is when multiple boards are compiled in, those
data not used can be automatically discarded.
>>> Yes, but only some of the data can be discarded. Continuing with the
>>> example in hand, while you can discard the six words which represent
>>> samsung_keypad_platdata, but the keymap_data can't be because that won't
>>> be re-allocated, which is probably a much larger data structure.
>>>
>> No. the keymap_data is possible too. The keypad driver allocates other
>> keymap area of input device and it is assigned from datas based on this
>> keymap_data.
>>
> This is a generic issue. Even if in your example, you can avoid this by
> re-allocation and re-assignment (ignore the performance issue for such
> behavior), the real question is the difficult to track all these down.
> Since
Right, it can occur difficulty of maintain. I wanted just to inform the
current fact.
> matrix_keypad_data is something out of your control (it was actually
> drafted by me and Dmitry if you are interested), and think about one day
> I changed it's definition, now you have to sync your driver and code every
> time to make sure the discarded data is not referenced.
>
if matrix_keypad_data is changed, i think the patchset should included
change of related other parts using it.
>>> That's reasonable but difficult in practice, every keypad driver using
>>> matrix_keypad_data could be doing things differently. That's what I'm
>> Just FYI, correct name is matrix_keymap_data and current all keypad
>> drivers using matrix_keymap_data use it to same way.
>>
>
> Note I was just thinking there is a potential issue as been pointed out that
> we can improve. And I'm not NACKing your perfect patch. Sorry if I made
> you think so.
>
Don't mind me. I appreciate yo
Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
On Tue, Jun 22, 2010 at 12:00 PM, Joonyoung Shim
wrote:
> On 6/22/2010 12:38 PM, Eric Miao wrote:
>> On Tue, Jun 22, 2010 at 11:27 AM, Joonyoung Shim
>> wrote:
>>> On 6/22/2010 12:02 PM, Eric Miao wrote:
On Tue, Jun 22, 2010 at 8:48 AM, Joonyoung Shim
wrote:
> On 6/21/2010 8:16 PM, Russell King - ARM Linux wrote:
>> On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
>>> On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
>>> wrote:
On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim
> wrote:
>> +void __init samsung_keypad_set_platdata(struct
>> samsung_keypad_platdata *pd)
>> +{
>> + � � � struct samsung_keypad_platdata *npd;
>> +
>> + � � � if (!pd) {
>> + � � � � � � � printk(KERN_ERR "%s: no platform data\n", __func__);
>> + � � � � � � � return;
>> + � � � }
>> +
>> + � � � npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata),
>> GFP_KERNEL);
>> + � � � if (!npd)
>> + � � � � � � � printk(KERN_ERR "%s: no memory for platform data\n",
>> __func__);
> This part of the code is actually duplicated again and again and again
> for each device, PXA and other legacy platforms are bad references for
> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are
> three
> major points:
>
> �1. A minimum 'struct pxa_device_desc' for a simple description of a
> � � device (more than 90% of the devices can be described that way),
> � � and avoid using a comparatively heavier weight platform_device,
> � � which can be generated at run-time
>
> �2. pxa_register_device() to allocate and register the platform_device
> � � at run-time, along with the platform data
It's a bad idea to make platform data be run-time discardable like
this:
>> +struct samsung_keypad_platdata {
>> + � � � const struct matrix_keymap_data *keymap_data;
What you end up with is some platform data structures which must be
kept
(those which have pointers to them from the platform data), and others
(the platform data itself) which can be discarded at runtime.
We know that the __initdata attributations cause lots of problems -
they're frequently wrong. �Just see the constant hastle with __devinit
et.al. �The same issue happens with __initdata as well.
So why make things more complicated by allowing some platform data
structures to be discardable and others not to be? �Is their small
size (maybe 6 words for this one) really worth the hastle of getting
__initdata attributations wrong (eg, on the keymap data?)
>>> Russell,
>>>
>>> The benefit I see is when multiple boards are compiled in, those
>>> data not used can be automatically discarded.
>> Yes, but only some of the data can be discarded. Continuing with the
>> example in hand, while you can discard the six words which represent
>> samsung_keypad_platdata, but the keymap_data can't be because that won't
>> be re-allocated, which is probably a much larger data structure.
>>
> No. the keymap_data is possible too. The keypad driver allocates other
> keymap area of input device and it is assigned from datas based on this
> keymap_data.
>
This is a generic issue. Even if in your example, you can avoid this by
re-allocation and re-assignment (ignore the performance issue for such
behavior), the real question is the difficult to track all these down.
Since
>>> Right, it can occur difficulty of maintain. I wanted just to inform the
>>> current fact.
>>>
matrix_keypad_data is something out of your control (it was actually
drafted by me and Dmitry if you are interested), and think about one day
I changed it's definition, now you have to sync your driver and code every
time to make sure the discarded data is not referenced.
>>> if matrix_keypad_data is changed, i think the patchset should included
>>> change of related other parts using it.
>>>
>>
>> That's reasonable but difficult in practice, every keypad driver using
>> matrix_keypad_data could be doing things differently. That's what I'm
>
> Just FYI, correct name is matrix_keymap_data and current all keypad
> drivers using matrix_keymap_data use it to same way.
>
Note I was just thinking there is a potential issue as been pointed out that
we can improve. And I'm not NACKing your perfect patch. Sorry if I made
you think so.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
On 6/22/2010 12:38 PM, Eric Miao wrote:
> On Tue, Jun 22, 2010 at 11:27 AM, Joonyoung Shim
> wrote:
>> On 6/22/2010 12:02 PM, Eric Miao wrote:
>>> On Tue, Jun 22, 2010 at 8:48 AM, Joonyoung Shim
>>> wrote:
On 6/21/2010 8:16 PM, Russell King - ARM Linux wrote:
> On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
>> On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
>> wrote:
>>> On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim
wrote:
> +void __init samsung_keypad_set_platdata(struct
> samsung_keypad_platdata *pd)
> +{
> + � � � struct samsung_keypad_platdata *npd;
> +
> + � � � if (!pd) {
> + � � � � � � � printk(KERN_ERR "%s: no platform data\n", __func__);
> + � � � � � � � return;
> + � � � }
> +
> + � � � npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata),
> GFP_KERNEL);
> + � � � if (!npd)
> + � � � � � � � printk(KERN_ERR "%s: no memory for platform data\n",
> __func__);
This part of the code is actually duplicated again and again and again
for each device, PXA and other legacy platforms are bad references for
this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
major points:
�1. A minimum 'struct pxa_device_desc' for a simple description of a
� � device (more than 90% of the devices can be described that way),
� � and avoid using a comparatively heavier weight platform_device,
� � which can be generated at run-time
�2. pxa_register_device() to allocate and register the platform_device
� � at run-time, along with the platform data
>>> It's a bad idea to make platform data be run-time discardable like this:
>>>
> +struct samsung_keypad_platdata {
> + � � � const struct matrix_keymap_data *keymap_data;
>>> What you end up with is some platform data structures which must be kept
>>> (those which have pointers to them from the platform data), and others
>>> (the platform data itself) which can be discarded at runtime.
>>>
>>> We know that the __initdata attributations cause lots of problems -
>>> they're frequently wrong. �Just see the constant hastle with __devinit
>>> et.al. �The same issue happens with __initdata as well.
>>>
>>> So why make things more complicated by allowing some platform data
>>> structures to be discardable and others not to be? �Is their small
>>> size (maybe 6 words for this one) really worth the hastle of getting
>>> __initdata attributations wrong (eg, on the keymap data?)
>>>
>> Russell,
>>
>> The benefit I see is when multiple boards are compiled in, those
>> data not used can be automatically discarded.
> Yes, but only some of the data can be discarded. Continuing with the
> example in hand, while you can discard the six words which represent
> samsung_keypad_platdata, but the keymap_data can't be because that won't
> be re-allocated, which is probably a much larger data structure.
>
No. the keymap_data is possible too. The keypad driver allocates other
keymap area of input device and it is assigned from datas based on this
keymap_data.
>>> This is a generic issue. Even if in your example, you can avoid this by
>>> re-allocation and re-assignment (ignore the performance issue for such
>>> behavior), the real question is the difficult to track all these down. Since
>> Right, it can occur difficulty of maintain. I wanted just to inform the
>> current fact.
>>
>>> matrix_keypad_data is something out of your control (it was actually
>>> drafted by me and Dmitry if you are interested), and think about one day
>>> I changed it's definition, now you have to sync your driver and code every
>>> time to make sure the discarded data is not referenced.
>>>
>> if matrix_keypad_data is changed, i think the patchset should included
>> change of related other parts using it.
>>
>
> That's reasonable but difficult in practice, every keypad driver using
> matrix_keypad_data could be doing things differently. That's what I'm
Just FYI, correct name is matrix_keymap_data and current all keypad
drivers using matrix_keymap_data use it to same way.
> concerned about. Things will be much easier for driver writers if he
> knows the data passed in will always be reference-able.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
On Tue, Jun 22, 2010 at 11:27 AM, Joonyoung Shim
wrote:
> On 6/22/2010 12:02 PM, Eric Miao wrote:
>> On Tue, Jun 22, 2010 at 8:48 AM, Joonyoung Shim
>> wrote:
>>> On 6/21/2010 8:16 PM, Russell King - ARM Linux wrote:
On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
> On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
> wrote:
>> On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
>>> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim
>>> wrote:
+void __init samsung_keypad_set_platdata(struct
samsung_keypad_platdata *pd)
+{
+ � � � struct samsung_keypad_platdata *npd;
+
+ � � � if (!pd) {
+ � � � � � � � printk(KERN_ERR "%s: no platform data\n", __func__);
+ � � � � � � � return;
+ � � � }
+
+ � � � npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata),
GFP_KERNEL);
+ � � � if (!npd)
+ � � � � � � � printk(KERN_ERR "%s: no memory for platform data\n",
__func__);
>>> This part of the code is actually duplicated again and again and again
>>> for each device, PXA and other legacy platforms are bad references for
>>> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
>>> major points:
>>>
>>> �1. A minimum 'struct pxa_device_desc' for a simple description of a
>>> � � device (more than 90% of the devices can be described that way),
>>> � � and avoid using a comparatively heavier weight platform_device,
>>> � � which can be generated at run-time
>>>
>>> �2. pxa_register_device() to allocate and register the platform_device
>>> � � at run-time, along with the platform data
>> It's a bad idea to make platform data be run-time discardable like this:
>>
+struct samsung_keypad_platdata {
+ � � � const struct matrix_keymap_data *keymap_data;
>> What you end up with is some platform data structures which must be kept
>> (those which have pointers to them from the platform data), and others
>> (the platform data itself) which can be discarded at runtime.
>>
>> We know that the __initdata attributations cause lots of problems -
>> they're frequently wrong. �Just see the constant hastle with __devinit
>> et.al. �The same issue happens with __initdata as well.
>>
>> So why make things more complicated by allowing some platform data
>> structures to be discardable and others not to be? �Is their small
>> size (maybe 6 words for this one) really worth the hastle of getting
>> __initdata attributations wrong (eg, on the keymap data?)
>>
> Russell,
>
> The benefit I see is when multiple boards are compiled in, those
> data not used can be automatically discarded.
Yes, but only some of the data can be discarded. Continuing with the
example in hand, while you can discard the six words which represent
samsung_keypad_platdata, but the keymap_data can't be because that won't
be re-allocated, which is probably a much larger data structure.
>>> No. the keymap_data is possible too. The keypad driver allocates other
>>> keymap area of input device and it is assigned from datas based on this
>>> keymap_data.
>>>
>>
>> This is a generic issue. Even if in your example, you can avoid this by
>> re-allocation and re-assignment (ignore the performance issue for such
>> behavior), the real question is the difficult to track all these down. Since
>
> Right, it can occur difficulty of maintain. I wanted just to inform the
> current fact.
>
>> matrix_keypad_data is something out of your control (it was actually
>> drafted by me and Dmitry if you are interested), and think about one day
>> I changed it's definition, now you have to sync your driver and code every
>> time to make sure the discarded data is not referenced.
>>
>
> if matrix_keypad_data is changed, i think the patchset should included
> change of related other parts using it.
>
That's reasonable but difficult in practice, every keypad driver using
matrix_keypad_data could be doing things differently. That's what I'm
concerned about. Things will be much easier for driver writers if he
knows the data passed in will always be reference-able.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
On 6/22/2010 12:02 PM, Eric Miao wrote:
> On Tue, Jun 22, 2010 at 8:48 AM, Joonyoung Shim
> wrote:
>> On 6/21/2010 8:16 PM, Russell King - ARM Linux wrote:
>>> On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
wrote:
> On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
>> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim
>> wrote:
>>> +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata
>>> *pd)
>>> +{
>>> + � � � struct samsung_keypad_platdata *npd;
>>> +
>>> + � � � if (!pd) {
>>> + � � � � � � � printk(KERN_ERR "%s: no platform data\n", __func__);
>>> + � � � � � � � return;
>>> + � � � }
>>> +
>>> + � � � npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata),
>>> GFP_KERNEL);
>>> + � � � if (!npd)
>>> + � � � � � � � printk(KERN_ERR "%s: no memory for platform data\n",
>>> __func__);
>> This part of the code is actually duplicated again and again and again
>> for each device, PXA and other legacy platforms are bad references for
>> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
>> major points:
>>
>> �1. A minimum 'struct pxa_device_desc' for a simple description of a
>> � � device (more than 90% of the devices can be described that way),
>> � � and avoid using a comparatively heavier weight platform_device,
>> � � which can be generated at run-time
>>
>> �2. pxa_register_device() to allocate and register the platform_device
>> � � at run-time, along with the platform data
> It's a bad idea to make platform data be run-time discardable like this:
>
>>> +struct samsung_keypad_platdata {
>>> + � � � const struct matrix_keymap_data *keymap_data;
> What you end up with is some platform data structures which must be kept
> (those which have pointers to them from the platform data), and others
> (the platform data itself) which can be discarded at runtime.
>
> We know that the __initdata attributations cause lots of problems -
> they're frequently wrong. �Just see the constant hastle with __devinit
> et.al. �The same issue happens with __initdata as well.
>
> So why make things more complicated by allowing some platform data
> structures to be discardable and others not to be? �Is their small
> size (maybe 6 words for this one) really worth the hastle of getting
> __initdata attributations wrong (eg, on the keymap data?)
>
Russell,
The benefit I see is when multiple boards are compiled in, those
data not used can be automatically discarded.
>>> Yes, but only some of the data can be discarded. Continuing with the
>>> example in hand, while you can discard the six words which represent
>>> samsung_keypad_platdata, but the keymap_data can't be because that won't
>>> be re-allocated, which is probably a much larger data structure.
>>>
>> No. the keymap_data is possible too. The keypad driver allocates other
>> keymap area of input device and it is assigned from datas based on this
>> keymap_data.
>>
>
> This is a generic issue. Even if in your example, you can avoid this by
> re-allocation and re-assignment (ignore the performance issue for such
> behavior), the real question is the difficult to track all these down. Since
Right, it can occur difficulty of maintain. I wanted just to inform the
current fact.
> matrix_keypad_data is something out of your control (it was actually
> drafted by me and Dmitry if you are interested), and think about one day
> I changed it's definition, now you have to sync your driver and code every
> time to make sure the discarded data is not referenced.
>
if matrix_keypad_data is changed, i think the patchset should included
change of related other parts using it.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
On Tue, Jun 22, 2010 at 8:48 AM, Joonyoung Shim wrote:
> On 6/21/2010 8:16 PM, Russell King - ARM Linux wrote:
>> On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
>>> On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
>>> wrote:
On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim
> wrote:
>> +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata
>> *pd)
>> +{
>> + � � � struct samsung_keypad_platdata *npd;
>> +
>> + � � � if (!pd) {
>> + � � � � � � � printk(KERN_ERR "%s: no platform data\n", __func__);
>> + � � � � � � � return;
>> + � � � }
>> +
>> + � � � npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata),
>> GFP_KERNEL);
>> + � � � if (!npd)
>> + � � � � � � � printk(KERN_ERR "%s: no memory for platform data\n",
>> __func__);
> This part of the code is actually duplicated again and again and again
> for each device, PXA and other legacy platforms are bad references for
> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
> major points:
>
> �1. A minimum 'struct pxa_device_desc' for a simple description of a
> � � device (more than 90% of the devices can be described that way),
> � � and avoid using a comparatively heavier weight platform_device,
> � � which can be generated at run-time
>
> �2. pxa_register_device() to allocate and register the platform_device
> � � at run-time, along with the platform data
It's a bad idea to make platform data be run-time discardable like this:
>> +struct samsung_keypad_platdata {
>> + � � � const struct matrix_keymap_data *keymap_data;
What you end up with is some platform data structures which must be kept
(those which have pointers to them from the platform data), and others
(the platform data itself) which can be discarded at runtime.
We know that the __initdata attributations cause lots of problems -
they're frequently wrong. �Just see the constant hastle with __devinit
et.al. �The same issue happens with __initdata as well.
So why make things more complicated by allowing some platform data
structures to be discardable and others not to be? �Is their small
size (maybe 6 words for this one) really worth the hastle of getting
__initdata attributations wrong (eg, on the keymap data?)
>>> Russell,
>>>
>>> The benefit I see is when multiple boards are compiled in, those
>>> data not used can be automatically discarded.
>>
>> Yes, but only some of the data can be discarded. Continuing with the
>> example in hand, while you can discard the six words which represent
>> samsung_keypad_platdata, but the keymap_data can't be because that won't
>> be re-allocated, which is probably a much larger data structure.
>>
>
> No. the keymap_data is possible too. The keypad driver allocates other
> keymap area of input device and it is assigned from datas based on this
> keymap_data.
>
This is a generic issue. Even if in your example, you can avoid this by
re-allocation and re-assignment (ignore the performance issue for such
behavior), the real question is the difficult to track all these down. Since
matrix_keypad_data is something out of your control (it was actually
drafted by me and Dmitry if you are interested), and think about one day
I changed it's definition, now you have to sync your driver and code every
time to make sure the discarded data is not referenced.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
On 6/21/2010 8:16 PM, Russell King - ARM Linux wrote:
> On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
>> On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
>> wrote:
>>> On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim
wrote:
> +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata
> *pd)
> +{
> + � � � struct samsung_keypad_platdata *npd;
> +
> + � � � if (!pd) {
> + � � � � � � � printk(KERN_ERR "%s: no platform data\n", __func__);
> + � � � � � � � return;
> + � � � }
> +
> + � � � npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata),
> GFP_KERNEL);
> + � � � if (!npd)
> + � � � � � � � printk(KERN_ERR "%s: no memory for platform data\n",
> __func__);
This part of the code is actually duplicated again and again and again
for each device, PXA and other legacy platforms are bad references for
this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
major points:
�1. A minimum 'struct pxa_device_desc' for a simple description of a
� � device (more than 90% of the devices can be described that way),
� � and avoid using a comparatively heavier weight platform_device,
� � which can be generated at run-time
�2. pxa_register_device() to allocate and register the platform_device
� � at run-time, along with the platform data
>>> It's a bad idea to make platform data be run-time discardable like this:
>>>
> +struct samsung_keypad_platdata {
> + � � � const struct matrix_keymap_data *keymap_data;
>>> What you end up with is some platform data structures which must be kept
>>> (those which have pointers to them from the platform data), and others
>>> (the platform data itself) which can be discarded at runtime.
>>>
>>> We know that the __initdata attributations cause lots of problems -
>>> they're frequently wrong. �Just see the constant hastle with __devinit
>>> et.al. �The same issue happens with __initdata as well.
>>>
>>> So why make things more complicated by allowing some platform data
>>> structures to be discardable and others not to be? �Is their small
>>> size (maybe 6 words for this one) really worth the hastle of getting
>>> __initdata attributations wrong (eg, on the keymap data?)
>>>
>> Russell,
>>
>> The benefit I see is when multiple boards are compiled in, those
>> data not used can be automatically discarded.
>
> Yes, but only some of the data can be discarded. Continuing with the
> example in hand, while you can discard the six words which represent
> samsung_keypad_platdata, but the keymap_data can't be because that won't
> be re-allocated, which is probably a much larger data structure.
>
No. the keymap_data is possible too. The keypad driver allocates other
keymap area of input device and it is assigned from datas based on this
keymap_data.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
Eric Miao wrote: > > >> and now in s5pc200 code, you can also register/re-use the s5pc100_keypad > >> device/driver, and everyone knows - OK, the IP on S5PC100 is now re-used > >> here. > >> > > > > This is naming issue and i know it was not desided yet. > > This keypad driver can support s3c64xx, s5pc100, s5pc110, s5pv210. > > > > I bet the first silicon this IP is deployed is s3c64xx, so my understanding > 's3c64xx-keypad.c' should be an acceptable name. And it's also reasonable, > a s3c64xx-keypad driver, and is later expanded to support subsequent silicons > without the file name being changed. Hi, Seems to be more suitable 'samsung-keypad', because can support S5PC100, S5PC110, and S5PV210 as well as S3C64XX. Already tested on SMDK6410(S3C6410), SMDKC100(S5PC100), Aquila(S5PC110), GONI(S5PC110) and SMDKV210(S5PV210) boards. Thanks. Best regards, Kgene. -- Kukjin Kim , Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
>> and now in s5pc200 code, you can also register/re-use the s5pc100_keypad >> device/driver, and everyone knows - OK, the IP on S5PC100 is now re-used >> here. >> > > This is naming issue and i know it was not desided yet. > This keypad driver can support s3c64xx, s5pc100, s5pc110, s5pv210. > I bet the first silicon this IP is deployed is s3c64xx, so my understanding 's3c64xx-keypad.c' should be an acceptable name. And it's also reasonable, a s3c64xx-keypad driver, and is later expanded to support subsequent silicons without the file name being changed. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
On Mon, Jun 21, 2010 at 7:16 PM, Russell King - ARM Linux
wrote:
> On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
>> On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
>> wrote:
>> > On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
>> >> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim
>> >> wrote:
>> >> > +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata
>> >> > *pd)
>> >> > +{
>> >> > + struct samsung_keypad_platdata *npd;
>> >> > +
>> >> > + if (!pd) {
>> >> > + printk(KERN_ERR "%s: no platform data\n", __func__);
>> >> > + return;
>> >> > + }
>> >> > +
>> >> > + npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata),
>> >> > GFP_KERNEL);
>> >> > + if (!npd)
>> >> > + printk(KERN_ERR "%s: no memory for platform data\n",
>> >> > __func__);
>> >>
>> >> This part of the code is actually duplicated again and again and again
>> >> for each device, PXA and other legacy platforms are bad references for
>> >> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
>> >> major points:
>> >>
>> >> 1. A minimum 'struct pxa_device_desc' for a simple description of a
>> >> device (more than 90% of the devices can be described that way),
>> >> and avoid using a comparatively heavier weight platform_device,
>> >> which can be generated at run-time
>> >>
>> >> 2. pxa_register_device() to allocate and register the platform_device
>> >> at run-time, along with the platform data
>> >
>> > It's a bad idea to make platform data be run-time discardable like this:
>> >
>> >> > +struct samsung_keypad_platdata {
>> >> > + const struct matrix_keymap_data *keymap_data;
>> >
>> > What you end up with is some platform data structures which must be kept
>> > (those which have pointers to them from the platform data), and others
>> > (the platform data itself) which can be discarded at runtime.
>> >
>> > We know that the __initdata attributations cause lots of problems -
>> > they're frequently wrong. Just see the constant hastle with __devinit
>> > et.al. The same issue happens with __initdata as well.
>> >
>> > So why make things more complicated by allowing some platform data
>> > structures to be discardable and others not to be? Is their small
>> > size (maybe 6 words for this one) really worth the hastle of getting
>> > __initdata attributations wrong (eg, on the keymap data?)
>> >
>>
>> Russell,
>>
>> The benefit I see is when multiple boards are compiled in, those
>> data not used can be automatically discarded.
>
> Yes, but only some of the data can be discarded. Continuing with the
> example in hand, while you can discard the six words which represent
> samsung_keypad_platdata, but the keymap_data can't be because that won't
> be re-allocated, which is probably a much larger data structure.
>
> So, samsung_keypad_platdata can be marked with __initdata, but the keymap
> data must not be - and this is where the confusion about what can be
> marked with __initdata starts - and eventually leads to the keymap data
> also being mistakenly marked __initdata.
Indeed. One ideal solution would be to make all the board code into
a module, yet since some of the code should be executed really
early and need to stay as built-in, looks like we need a way to
separate these two parts.
>
> Is saving six words (or so) worth the effort to track down stuff
> inappropriately marked with __initdata ? I'm sure there's bigger savings
> to be had in other parts of the kernel (such as shrinking the size of
> tcp hash tables, reducing the kernel message ring buffer, etc) if size is
> really a concern.
>
That's true, however, I think the size will become a bit more significant
if more and more board code is compiled into a single kernel.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
On 6/21/2010 6:05 PM, Eric Miao wrote:
> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim
> wrote:
>> This patch adds samsung keypad device definition for samsung cpus.
>>
>> Signed-off-by: Joonyoung Shim
>> Signed-off-by: Kyungmin Park
>> ---
>> arch/arm/plat-samsung/Kconfig|5 ++
>> arch/arm/plat-samsung/Makefile |1 +
>> arch/arm/plat-samsung/dev-keypad.c | 58
>> +
>
> Why need an individual file for a simple device? In the end, these files
> will flood over the plat-samsung/ directory. And now think this - in your
> new SoC design ,the keypad IP is replaced with a completely new one, does
> that mean a new dev-keypad-new1.c, dev-keypad-new2.c?
>
> I personally prefer a single devices.c for all the devices, if you
> orgnize well, shouldn't take up much code size in that single file.
>
I know, but will need more effort of other device as well as keypad to
do it.
>> arch/arm/plat-samsung/include/plat/devs.h|2 +
>> arch/arm/plat-samsung/include/plat/keypad.h | 59
>> ++
>> arch/arm/plat-samsung/include/plat/regs-keypad.h | 49 ++
>
> Will these registers be used elsewhere except within the keypad driver?
> If no, they might be better moved to the keypad driver itself, it makes
> the driver more self-contained and avoid the registers being accessed
> anywhere.
>
Right, these registers are used only in the keypad driver. I think it's
possible.
>> 6 files changed, 174 insertions(+), 0 deletions(-)
>> create mode 100644 arch/arm/plat-samsung/dev-keypad.c
>> create mode 100644 arch/arm/plat-samsung/include/plat/keypad.h
>> create mode 100644 arch/arm/plat-samsung/include/plat/regs-keypad.h
>>
>> diff --git a/arch/arm/plat-samsung/Kconfig b/arch/arm/plat-samsung/Kconfig
>> index 2753fb3..bd007e3 100644
>> --- a/arch/arm/plat-samsung/Kconfig
>> +++ b/arch/arm/plat-samsung/Kconfig
>> @@ -227,6 +227,11 @@ config SAMSUNG_DEV_TS
>>help
>>Common in platform device definitions for touchscreen device
>>
>> +config SAMSUNG_DEV_KEYPAD
>> + bool
>> + help
>> + Compile in platform device definitions for keypad
>> +
>> # DMA
>>
>> config S3C_DMA
>> diff --git a/arch/arm/plat-samsung/Makefile b/arch/arm/plat-samsung/Makefile
>> index b1d82cc..8269d80 100644
>> --- a/arch/arm/plat-samsung/Makefile
>> +++ b/arch/arm/plat-samsung/Makefile
>> @@ -48,6 +48,7 @@ obj-$(CONFIG_S3C_DEV_RTC) += dev-rtc.o
>>
>> obj-$(CONFIG_SAMSUNG_DEV_ADC) += dev-adc.o
>> obj-$(CONFIG_SAMSUNG_DEV_TS) += dev-ts.o
>> +obj-$(CONFIG_SAMSUNG_DEV_KEYPAD) += dev-keypad.o
>>
>> # DMA support
>>
>> diff --git a/arch/arm/plat-samsung/dev-keypad.c
>> b/arch/arm/plat-samsung/dev-keypad.c
>> new file mode 100644
>> index 000..679b444
>> --- /dev/null
>> +++ b/arch/arm/plat-samsung/dev-keypad.c
>> @@ -0,0 +1,58 @@
>> +/*
>> + * linux/arch/arm/plat-samsung/dev-keypad.c
>> + *
>> + * Copyright (C) 2010 Samsung Electronics Co.Ltd
>> + * Author: Joonyoung Shim
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License as published by the
>> + * Free Software Foundation; either version 2 of the License, or (at your
>> + * option) any later version.
>> + *
>> + */
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +static struct resource samsung_keypad_resources[] = {
>> + [0] = {
>> + .start = SAMSUNG_PA_KEYPAD,
>> + .end= SAMSUNG_PA_KEYPAD + 0x20 - 1,
>> + .flags = IORESOURCE_MEM,
>> + },
>> + [1] = {
>> + .start = IRQ_KEYPAD,
>> + .end= IRQ_KEYPAD,
>> + .flags = IORESOURCE_IRQ,
>> + },
>> +};
>> +
>> +struct platform_device samsung_device_keypad = {
>> + .name = "samsung-keypad",
>> + .id = -1,
>> + .num_resources = ARRAY_SIZE(samsung_keypad_resources),
>> + .resource = samsung_keypad_resources,
>> +};
>> +
>> +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
>> +{
>> + struct samsung_keypad_platdata *npd;
>> +
>> + if (!pd) {
>> + printk(KERN_ERR "%s: no platform data\n", __func__);
>> + return;
>> + }
>> +
>> + npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata),
>> GFP_KERNEL);
>> + if (!npd)
>> + printk(KERN_ERR "%s: no memory for platform data\n",
>> __func__);
>
> This part of the code is actually duplicated again and again and again
> for each device, PXA and other legacy platforms are bad references for
> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
> major points:
>
I know ben posted patches to remove duplicated codes such this.
http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg01474.
Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
> On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
> wrote:
> > On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
> >> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim
> >> wrote:
> >> > +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata
> >> > *pd)
> >> > +{
> >> > + struct samsung_keypad_platdata *npd;
> >> > +
> >> > + if (!pd) {
> >> > + printk(KERN_ERR "%s: no platform data\n", __func__);
> >> > + return;
> >> > + }
> >> > +
> >> > + npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata),
> >> > GFP_KERNEL);
> >> > + if (!npd)
> >> > + printk(KERN_ERR "%s: no memory for platform data\n",
> >> > __func__);
> >>
> >> This part of the code is actually duplicated again and again and again
> >> for each device, PXA and other legacy platforms are bad references for
> >> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
> >> major points:
> >>
> >> 1. A minimum 'struct pxa_device_desc' for a simple description of a
> >> device (more than 90% of the devices can be described that way),
> >> and avoid using a comparatively heavier weight platform_device,
> >> which can be generated at run-time
> >>
> >> 2. pxa_register_device() to allocate and register the platform_device
> >> at run-time, along with the platform data
> >
> > It's a bad idea to make platform data be run-time discardable like this:
> >
> >> > +struct samsung_keypad_platdata {
> >> > + const struct matrix_keymap_data *keymap_data;
> >
> > What you end up with is some platform data structures which must be kept
> > (those which have pointers to them from the platform data), and others
> > (the platform data itself) which can be discarded at runtime.
> >
> > We know that the __initdata attributations cause lots of problems -
> > they're frequently wrong. Just see the constant hastle with __devinit
> > et.al. The same issue happens with __initdata as well.
> >
> > So why make things more complicated by allowing some platform data
> > structures to be discardable and others not to be? Is their small
> > size (maybe 6 words for this one) really worth the hastle of getting
> > __initdata attributations wrong (eg, on the keymap data?)
> >
>
> Russell,
>
> The benefit I see is when multiple boards are compiled in, those
> data not used can be automatically discarded.
Yes, but only some of the data can be discarded. Continuing with the
example in hand, while you can discard the six words which represent
samsung_keypad_platdata, but the keymap_data can't be because that won't
be re-allocated, which is probably a much larger data structure.
So, samsung_keypad_platdata can be marked with __initdata, but the keymap
data must not be - and this is where the confusion about what can be
marked with __initdata starts - and eventually leads to the keymap data
also being mistakenly marked __initdata.
Is saving six words (or so) worth the effort to track down stuff
inappropriately marked with __initdata ? I'm sure there's bigger savings
to be had in other parts of the kernel (such as shrinking the size of
tcp hash tables, reducing the kernel message ring buffer, etc) if size is
really a concern.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
On Mon, Jun 21, 2010 at 5:29 PM, Marek Szyprowski
wrote:
> Hello,
>
> On Monday, June 21, 2010 11:06 AM Eric Miao wrote:
>
>> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim
>> wrote:
>> > This patch adds samsung keypad device definition for samsung cpus.
>> >
>> > Signed-off-by: Joonyoung Shim
>> > Signed-off-by: Kyungmin Park
>> > ---
>> > arch/arm/plat-samsung/Kconfig | 5 ++
>> > arch/arm/plat-samsung/Makefile | 1 +
>> > arch/arm/plat-samsung/dev-keypad.c | 58
>> +
>>
>> Why need an individual file for a simple device? In the end, these files
>> will flood over the plat-samsung/ directory. And now think this - in your
>> new SoC design ,the keypad IP is replaced with a completely new one, does
>> that mean a new dev-keypad-new1.c, dev-keypad-new2.c?
>>
>> I personally prefer a single devices.c for all the devices, if you
>> orgnize well, shouldn't take up much code size in that single file.
>
> A separate dev-abc.c files has been chosen by a platform maintainer. This
> has some advantages as some unused data can be easily not compiled into
> the kernel which is configured specially for a particular board.
This also can be done by something like below:
#if defined(CONFIG_S5P_KEYPAD) || defined(CONFIG_S5P_KEYPAD_MODULE)
struct platform_device s5p_device_keypad {
};
#endif
And if you want run-time discardable platform_device, you have to mark
this structure as __initdata, and duplicate that device when registering.
That's why in arch/arm/mach-mmp/, a light weight and descriptive
'struct pxa_device_desc' is introduced, so only those platform_devices
registered will be generated and registered.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
wrote:
> On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
>> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim
>> wrote:
>> > +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata
>> > *pd)
>> > +{
>> > + struct samsung_keypad_platdata *npd;
>> > +
>> > + if (!pd) {
>> > + printk(KERN_ERR "%s: no platform data\n", __func__);
>> > + return;
>> > + }
>> > +
>> > + npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata),
>> > GFP_KERNEL);
>> > + if (!npd)
>> > + printk(KERN_ERR "%s: no memory for platform data\n",
>> > __func__);
>>
>> This part of the code is actually duplicated again and again and again
>> for each device, PXA and other legacy platforms are bad references for
>> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
>> major points:
>>
>> 1. A minimum 'struct pxa_device_desc' for a simple description of a
>> device (more than 90% of the devices can be described that way),
>> and avoid using a comparatively heavier weight platform_device,
>> which can be generated at run-time
>>
>> 2. pxa_register_device() to allocate and register the platform_device
>> at run-time, along with the platform data
>
> It's a bad idea to make platform data be run-time discardable like this:
>
>> > +struct samsung_keypad_platdata {
>> > + const struct matrix_keymap_data *keymap_data;
>
> What you end up with is some platform data structures which must be kept
> (those which have pointers to them from the platform data), and others
> (the platform data itself) which can be discarded at runtime.
>
> We know that the __initdata attributations cause lots of problems -
> they're frequently wrong. Just see the constant hastle with __devinit
> et.al. The same issue happens with __initdata as well.
>
> So why make things more complicated by allowing some platform data
> structures to be discardable and others not to be? Is their small
> size (maybe 6 words for this one) really worth the hastle of getting
> __initdata attributations wrong (eg, on the keymap data?)
>
Russell,
The benefit I see is when multiple boards are compiled in, those
data not used can be automatically discarded.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
Hello, On Monday, June 21, 2010 11:06 AM Eric Miao wrote: > On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim > wrote: > > This patch adds samsung keypad device definition for samsung cpus. > > > > Signed-off-by: Joonyoung Shim > > Signed-off-by: Kyungmin Park > > --- > > arch/arm/plat-samsung/Kconfig|5 ++ > > arch/arm/plat-samsung/Makefile |1 + > > arch/arm/plat-samsung/dev-keypad.c | 58 > + > > Why need an individual file for a simple device? In the end, these files > will flood over the plat-samsung/ directory. And now think this - in your > new SoC design ,the keypad IP is replaced with a completely new one, does > that mean a new dev-keypad-new1.c, dev-keypad-new2.c? > > I personally prefer a single devices.c for all the devices, if you > orgnize well, shouldn't take up much code size in that single file. A separate dev-abc.c files has been chosen by a platform maintainer. This has some advantages as some unused data can be easily not compiled into the kernel which is configured specially for a particular board. Best regards -- Marek Szyprowski Samsung Poland R&D Center -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim
> wrote:
> > +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
> > +{
> > + struct samsung_keypad_platdata *npd;
> > +
> > + if (!pd) {
> > + printk(KERN_ERR "%s: no platform data\n", __func__);
> > + return;
> > + }
> > +
> > + npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata),
> > GFP_KERNEL);
> > + if (!npd)
> > + printk(KERN_ERR "%s: no memory for platform data\n",
> > __func__);
>
> This part of the code is actually duplicated again and again and again
> for each device, PXA and other legacy platforms are bad references for
> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
> major points:
>
> 1. A minimum 'struct pxa_device_desc' for a simple description of a
> device (more than 90% of the devices can be described that way),
> and avoid using a comparatively heavier weight platform_device,
> which can be generated at run-time
>
> 2. pxa_register_device() to allocate and register the platform_device
> at run-time, along with the platform data
It's a bad idea to make platform data be run-time discardable like this:
> > +struct samsung_keypad_platdata {
> > + const struct matrix_keymap_data *keymap_data;
What you end up with is some platform data structures which must be kept
(those which have pointers to them from the platform data), and others
(the platform data itself) which can be discarded at runtime.
We know that the __initdata attributations cause lots of problems -
they're frequently wrong. Just see the constant hastle with __devinit
et.al. The same issue happens with __initdata as well.
So why make things more complicated by allowing some platform data
structures to be discardable and others not to be? Is their small
size (maybe 6 words for this one) really worth the hastle of getting
__initdata attributations wrong (eg, on the keymap data?)
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim wrote:
> This patch adds samsung keypad device definition for samsung cpus.
>
> Signed-off-by: Joonyoung Shim
> Signed-off-by: Kyungmin Park
> ---
> arch/arm/plat-samsung/Kconfig | 5 ++
> arch/arm/plat-samsung/Makefile | 1 +
> arch/arm/plat-samsung/dev-keypad.c | 58 +
Why need an individual file for a simple device? In the end, these files
will flood over the plat-samsung/ directory. And now think this - in your
new SoC design ,the keypad IP is replaced with a completely new one, does
that mean a new dev-keypad-new1.c, dev-keypad-new2.c?
I personally prefer a single devices.c for all the devices, if you
orgnize well, shouldn't take up much code size in that single file.
> arch/arm/plat-samsung/include/plat/devs.h | 2 +
> arch/arm/plat-samsung/include/plat/keypad.h | 59
> ++
> arch/arm/plat-samsung/include/plat/regs-keypad.h | 49 ++
Will these registers be used elsewhere except within the keypad driver?
If no, they might be better moved to the keypad driver itself, it makes
the driver more self-contained and avoid the registers being accessed
anywhere.
> 6 files changed, 174 insertions(+), 0 deletions(-)
> create mode 100644 arch/arm/plat-samsung/dev-keypad.c
> create mode 100644 arch/arm/plat-samsung/include/plat/keypad.h
> create mode 100644 arch/arm/plat-samsung/include/plat/regs-keypad.h
>
> diff --git a/arch/arm/plat-samsung/Kconfig b/arch/arm/plat-samsung/Kconfig
> index 2753fb3..bd007e3 100644
> --- a/arch/arm/plat-samsung/Kconfig
> +++ b/arch/arm/plat-samsung/Kconfig
> @@ -227,6 +227,11 @@ config SAMSUNG_DEV_TS
> help
> Common in platform device definitions for touchscreen device
>
> +config SAMSUNG_DEV_KEYPAD
> + bool
> + help
> + Compile in platform device definitions for keypad
> +
> # DMA
>
> config S3C_DMA
> diff --git a/arch/arm/plat-samsung/Makefile b/arch/arm/plat-samsung/Makefile
> index b1d82cc..8269d80 100644
> --- a/arch/arm/plat-samsung/Makefile
> +++ b/arch/arm/plat-samsung/Makefile
> @@ -48,6 +48,7 @@ obj-$(CONFIG_S3C_DEV_RTC) += dev-rtc.o
>
> obj-$(CONFIG_SAMSUNG_DEV_ADC) += dev-adc.o
> obj-$(CONFIG_SAMSUNG_DEV_TS) += dev-ts.o
> +obj-$(CONFIG_SAMSUNG_DEV_KEYPAD) += dev-keypad.o
>
> # DMA support
>
> diff --git a/arch/arm/plat-samsung/dev-keypad.c
> b/arch/arm/plat-samsung/dev-keypad.c
> new file mode 100644
> index 000..679b444
> --- /dev/null
> +++ b/arch/arm/plat-samsung/dev-keypad.c
> @@ -0,0 +1,58 @@
> +/*
> + * linux/arch/arm/plat-samsung/dev-keypad.c
> + *
> + * Copyright (C) 2010 Samsung Electronics Co.Ltd
> + * Author: Joonyoung Shim
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
> + *
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +static struct resource samsung_keypad_resources[] = {
> + [0] = {
> + .start = SAMSUNG_PA_KEYPAD,
> + .end = SAMSUNG_PA_KEYPAD + 0x20 - 1,
> + .flags = IORESOURCE_MEM,
> + },
> + [1] = {
> + .start = IRQ_KEYPAD,
> + .end = IRQ_KEYPAD,
> + .flags = IORESOURCE_IRQ,
> + },
> +};
> +
> +struct platform_device samsung_device_keypad = {
> + .name = "samsung-keypad",
> + .id = -1,
> + .num_resources = ARRAY_SIZE(samsung_keypad_resources),
> + .resource = samsung_keypad_resources,
> +};
> +
> +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
> +{
> + struct samsung_keypad_platdata *npd;
> +
> + if (!pd) {
> + printk(KERN_ERR "%s: no platform data\n", __func__);
> + return;
> + }
> +
> + npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), GFP_KERNEL);
> + if (!npd)
> + printk(KERN_ERR "%s: no memory for platform data\n",
> __func__);
This part of the code is actually duplicated again and again and again
for each device, PXA and other legacy platforms are bad references for
this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
major points:
1. A minimum 'struct pxa_device_desc' for a simple description of a
device (more than 90% of the devices can be described that way),
and avoid using a comparatively heavier weight platform_device,
which can be generated at run-time
2. pxa_register_device() to allocate and register the platform_device
at run-time, along with the platform data
3. pxa168_add_*() as a consistent/clearly named API, and with type
checking for the platform_data type (it's a bit difficult to che
