Re: [PATCH] ARM: exynos_defconfig: Enable USB Video Class support

[Javier Martinez Canillas]

Hello Krzysztof,

On 09/09/2015 02:06 AM, Krzysztof Kozlowski wrote:
> On 08.09.2015 22:32, Javier Martinez Canillas wrote:
> 
> (...)
> 
>>  
>>> Let me rephrase my question into:
>>> 1. What is worth enabling in exynos_defconfig? USB devices? I would
>>> argue, except they are needed to boot.
>>
>> Ok, I understand your concern. The question is where we draw the line.
>>
>>> So maybe enable everything which Exynos boards have hard-wired? That
>>> would make some sense... but we're making kernel larger.
>>>
>>
>> In the case of this WebCam, it's not a typical USB device in the sense
>> that is built in the Chromebook and not something that's plugged on an
>> external USB port.
> 
> Right, that is the difference from regular USB devices.
> 
>>  
>>> 2. Maybe enable only what is a typical use case (including typical
>>> testing cases)? Then we would have to define what "typical" means. For
>>> example battery would be typical but camera would not.
>>>
>>
>> There are a lot of board specific drivers that we currently enable as
>> built-in like hwmon sensors or iio devices that are likely only present
>> on a single board or a family of boards.
>>
>> So then I think all those drivers should be changed as a module as well,
>> unless are critical for the board operation (i.e: thermal or fan drivers).
> 
> Actually I think we should not judge by number of board using given
> component but its usefulness in general exynos_defconfig case. Even when
> something is used on just one board but it is important for that board,
> then it should be built-in.
> 
> For example hwmon monitoring stuff to get information about board
> condition. Other example are leds on Odroid - to get visible condition
> of the board.
> 
> This don't have to be critical, but just important for testing.
> 
> Additionally such components can be accessed usually from limited
> user-space, e.g. system booted to console or SSH.
> 
> If using a component requires more complex user-space (e.g. any kind of
> window system), then probably already modules could be easily used. In
> such cases I would expect the boot is not from network but from MMC and
> there is a full-blown distro working.
> 

Ok, fair enough.
 
>>> 3. Argh, so maybe, if we agree that not everything is worth being
>>> enabled, that additional stuff could be build as module?
>>>
>>
>> Yes, I don't see anything wrong to enable more stuff as a module if
>> that will give more build / test coverage.
>>
>> The goal of kernelci is to add functional tests so besides testing
>> if a given kernel booted correctly, it's going to test if for example
>> USB enumeration is working and has no regressions. For that use case
>> is interesting to have support for the built-in USB devices like this
>> camera (either as built-in or as a module).
> 
> Okay, so we have some agreement that other stuff which is not important
> but still hard-wired on Exynos boards (built into the board), can be
> enabled as a module. So now we we have to draw the line which is
> "important enough" to built-in and which is not so it could be as module.
> 
>>From my point of view media in general (cameras, tuners etc.) should be
> put in the second category (module), especially that in usual to test
> them you would have to boot system to a full graphical mode. Can you
> test them from SSH connection? Maybe you could test DVB tuners by
> reading status of packets but still output won't be visible.
>

Ok, my take on it was that if is wired into the board, then it should be
supported out-of-the-box with a zImage build using exynos_defconfig but
I see your point and looks reasonable to me.

I'll wait a couple of days to see if others have more opinions and respin
the patch with these options enabled as a module.

Thanks a lot for your feedback!
 
> Any comments from other interested parties?
> 
> Best regards,
> Krzysztof
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
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 1/3] sysfs: Fix is_visible() support for binary attributes

[Emilio López]

On 09/09/15 01:12, Guenter Roeck wrote:

On 09/08/2015 08:58 PM, Greg KH wrote:

On Tue, Sep 08, 2015 at 06:10:16PM -0700, Guenter Roeck wrote:

Hi Emilio,

On 09/08/2015 05:51 PM, Emilio López wrote:

Hi Greg & Guenter,


[ ... ]


Unless I am missing something, this is not explained anywhere,
but it is
not entirely trivial to understand. I think it should be documented.


I agree. I couldn't find any mention of what this int was supposed
to be by looking at Documentation/ (is_visible is not even mentioned
:/) or include/linux/sysfs.h. Once we settle on something I'll
document it before sending a v2.


In the include file ? No strong preference, though.


By the way, I wrote a quick coccinelle script to match is_visible()
users which reference the index (included below), and it found
references to drivers which do not seem to use any binary
attributes, so I believe changing the index meaning shouldn't be an
issue.


Good.


I agree, make i the number of the bin attribute and that should solve
this issue.


No, that would conflict with the "normal" use of is_visible for
non-binary
attributes, and make the index all but useless, since the
is_visible function
would have to search through all the attributes anyway to figure
out which one
is being checked.


Yeah, using the same indexes would be somewhat pointless, although
not many seem to be using it anyway (only 14 files matched). Others
seem to be comparing the attr* instead. An alternative would be to
use negative indexes for binary attributes and positive indexes for
normal attributes.


... and I probably wrote or reviewed a significant percentage of
those ;-).

Using negative numbers for binary attributes is an interesting idea.
Kind of unusual, though. Greg, any thoughts on that ?


Ick, no, that's a mess, maybe we just could drop the index alltogether?



No, please don't. Having to manually compare dozens of index pointers
would be
even more of a mess.


So, what about keeping it the way it is in the patch, and documenting it 
thoroughly? Otherwise, we could introduce another "is_bin_visible" 
function to do this same thing but just on binary attributes, but I'd 
rather not add a new function pointer if possible.


Cheers,
Emilio
--
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 1/3] sysfs: Fix is_visible() support for binary attributes

[Guenter Roeck]

On 09/09/2015 06:14 AM, Emilio López wrote:

On 09/09/15 01:12, Guenter Roeck wrote:

On 09/08/2015 08:58 PM, Greg KH wrote:

On Tue, Sep 08, 2015 at 06:10:16PM -0700, Guenter Roeck wrote:

Hi Emilio,

On 09/08/2015 05:51 PM, Emilio López wrote:

Hi Greg & Guenter,


[ ... ]


Unless I am missing something, this is not explained anywhere,
but it is
not entirely trivial to understand. I think it should be documented.


I agree. I couldn't find any mention of what this int was supposed
to be by looking at Documentation/ (is_visible is not even mentioned
:/) or include/linux/sysfs.h. Once we settle on something I'll
document it before sending a v2.


In the include file ? No strong preference, though.


By the way, I wrote a quick coccinelle script to match is_visible()
users which reference the index (included below), and it found
references to drivers which do not seem to use any binary
attributes, so I believe changing the index meaning shouldn't be an
issue.


Good.


I agree, make i the number of the bin attribute and that should solve
this issue.


No, that would conflict with the "normal" use of is_visible for
non-binary
attributes, and make the index all but useless, since the
is_visible function
would have to search through all the attributes anyway to figure
out which one
is being checked.


Yeah, using the same indexes would be somewhat pointless, although
not many seem to be using it anyway (only 14 files matched). Others
seem to be comparing the attr* instead. An alternative would be to
use negative indexes for binary attributes and positive indexes for
normal attributes.


... and I probably wrote or reviewed a significant percentage of
those ;-).

Using negative numbers for binary attributes is an interesting idea.
Kind of unusual, though. Greg, any thoughts on that ?


Ick, no, that's a mess, maybe we just could drop the index alltogether?



No, please don't. Having to manually compare dozens of index pointers
would be
even more of a mess.


So, what about keeping it the way it is in the patch, and documenting it thoroughly? 
Otherwise, we could introduce another "is_bin_visible" function to do this same 
thing but just on binary attributes, but I'd rather not add a new function pointer if 
possible.



I would prefer to keep and document it.

Guenter

--
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

[PATCH] drm/exynos: Remove useless EXPORT_SYMBOL_GPLs

[Gustavo Padovan]

From: Daniel Kurtz 

All the user of these functions are inside exynos-drm driver and
you don't need to export the symbols for that case.

Signed-off-by: Daniel Kurtz 
Signed-off-by: Gustavo Padovan 
---
 drivers/gpu/drm/exynos/exynos_drm_core.c | 6 --
 drivers/gpu/drm/exynos/exynos_drm_g2d.c  | 3 ---
 2 files changed, 9 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_core.c 
b/drivers/gpu/drm/exynos/exynos_drm_core.c
index c68a6a2..7f55ba6 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_core.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_core.c
@@ -28,7 +28,6 @@ int exynos_drm_subdrv_register(struct exynos_drm_subdrv 
*subdrv)
 
return 0;
 }
-EXPORT_SYMBOL_GPL(exynos_drm_subdrv_register);
 
 int exynos_drm_subdrv_unregister(struct exynos_drm_subdrv *subdrv)
 {
@@ -39,7 +38,6 @@ int exynos_drm_subdrv_unregister(struct exynos_drm_subdrv 
*subdrv)
 
return 0;
 }
-EXPORT_SYMBOL_GPL(exynos_drm_subdrv_unregister);
 
 int exynos_drm_device_subdrv_probe(struct drm_device *dev)
 {
@@ -69,7 +67,6 @@ int exynos_drm_device_subdrv_probe(struct drm_device *dev)
 
return 0;
 }
-EXPORT_SYMBOL_GPL(exynos_drm_device_subdrv_probe);
 
 int exynos_drm_device_subdrv_remove(struct drm_device *dev)
 {
@@ -87,7 +84,6 @@ int exynos_drm_device_subdrv_remove(struct drm_device *dev)
 
return 0;
 }
-EXPORT_SYMBOL_GPL(exynos_drm_device_subdrv_remove);
 
 int exynos_drm_subdrv_open(struct drm_device *dev, struct drm_file *file)
 {
@@ -111,7 +107,6 @@ err:
}
return ret;
 }
-EXPORT_SYMBOL_GPL(exynos_drm_subdrv_open);
 
 void exynos_drm_subdrv_close(struct drm_device *dev, struct drm_file *file)
 {
@@ -122,4 +117,3 @@ void exynos_drm_subdrv_close(struct drm_device *dev, struct 
drm_file *file)
subdrv->close(dev, subdrv->dev, file);
}
 }
-EXPORT_SYMBOL_GPL(exynos_drm_subdrv_close);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_g2d.c 
b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
index 535b4ad..0a596f1 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_g2d.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
@@ -1090,7 +1090,6 @@ int exynos_g2d_get_ver_ioctl(struct drm_device *drm_dev, 
void *data,
 
return 0;
 }
-EXPORT_SYMBOL_GPL(exynos_g2d_get_ver_ioctl);
 
 int exynos_g2d_set_cmdlist_ioctl(struct drm_device *drm_dev, void *data,
 struct drm_file *file)
@@ -1261,7 +1260,6 @@ err:
g2d_put_cmdlist(g2d, node);
return ret;
 }
-EXPORT_SYMBOL_GPL(exynos_g2d_set_cmdlist_ioctl);
 
 int exynos_g2d_exec_ioctl(struct drm_device *drm_dev, void *data,
  struct drm_file *file)
@@ -1324,7 +1322,6 @@ int exynos_g2d_exec_ioctl(struct drm_device *drm_dev, 
void *data,
 out:
return 0;
 }
-EXPORT_SYMBOL_GPL(exynos_g2d_exec_ioctl);
 
 static int g2d_subdrv_probe(struct drm_device *drm_dev, struct device *dev)
 {
-- 
2.1.0

--
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: [RFC PATCH] clocksource: exynos_mct: use container_of() instead of this_cpu_ptr()

[Krzysztof Kozlowski]

On 04.09.2015 08:49, Alexey Klimov wrote:
> Since evt structure is embedded in per-CPU mevt structure it's
> definitely faster to use container_of() to get access to mevt
> if we have evt (for example as incoming function argument) instead
> of more expensive approach with this_cpu_ptr(_mct_tick).
> this_cpu_ptr() on per-CPU mevt structure leads to access to cp15
> to get cpu id and arithmetic operations.
> Container_of() is cheaper since it's just one asm instruction.
> This should work if used evt pointer is correct and owned by
> local mevt structure.
> 
> For example, before this patch set_state_shutdown() looks like:
> 
>  4a4: e92d4010push{r4, lr}
>  4a8: e3004000movwr4, #0
>  4ac: ebfebl  0 
>  4b0: e3003000movwr3, #0
>  4b4: e3404000movtr4, #0
>  4b8: e3403000movtr3, #0
>  4bc: e7933100ldr r3, [r3, r0, lsl #2]
>  4c0: e0844003add r4, r4, r3
>  4c4: e59400c0ldr r0, [r4, #192]  ; 0xc0
>  4c8: ebd4bl  420 
>  4cc: e3a0mov r0, #0
>  4d0: e8bd8010pop {r4, pc}
> 
> With this patch:
> 
>  4a4: e92d4010push{r4, lr}
>  4a8: e59000c0ldr r0, [r0, #192]  ; 0xc0
>  4ac: ebdbbl  420 
>  4b0: e3a0mov r0, #0
>  4b4: e8bd8010pop {r4, pc}
> 
> Also, for me size of exynos_mct.o decreased from 84588 bytes
> to 83956.
> 
> Signed-off-by: Alexey Klimov 
> ---
>  drivers/clocksource/exynos_mct.c | 12 
>  1 file changed, 8 insertions(+), 4 deletions(-)

Looks good and sensible. Why you called this RFC? You are not sure if
this is correct?

One minor nit-pick below, but I am fine without it anyway:

Reviewed-by: Krzysztof Kozlowski 


> 
> diff --git a/drivers/clocksource/exynos_mct.c 
> b/drivers/clocksource/exynos_mct.c
> index 029f96a..ff44082 100644
> --- a/drivers/clocksource/exynos_mct.c
> +++ b/drivers/clocksource/exynos_mct.c
> @@ -382,24 +382,28 @@ static void exynos4_mct_tick_start(unsigned long cycles,
>  static int exynos4_tick_set_next_event(unsigned long cycles,
>  struct clock_event_device *evt)
>  {
> - struct mct_clock_event_device *mevt = this_cpu_ptr(_mct_tick);
> + struct mct_clock_event_device *mevt;
>  
> + mevt = container_of(evt, struct mct_clock_event_device, evt);
>   exynos4_mct_tick_start(cycles, mevt);
> -

Actually I would prefer leaving the empty line here and add such in
function below. For me the code is more readable with
ending return separated by one line.

Best regards,
Krzysztof

>   return 0;
>  }
>  
>  static int set_state_shutdown(struct clock_event_device *evt)
>  {
> - exynos4_mct_tick_stop(this_cpu_ptr(_mct_tick));
> + struct mct_clock_event_device *mevt;
> +
> + mevt = container_of(evt, struct mct_clock_event_device, evt);
> + exynos4_mct_tick_stop(mevt);
>   return 0;
>  }
>  
>  static int set_state_periodic(struct clock_event_device *evt)
>  {
> - struct mct_clock_event_device *mevt = this_cpu_ptr(_mct_tick);
> + struct mct_clock_event_device *mevt;
>   unsigned long cycles_per_jiffy;
>  
> + mevt = container_of(evt, struct mct_clock_event_device, evt);
>   cycles_per_jiffy = (((unsigned long long)NSEC_PER_SEC / HZ * evt->mult)
>   >> evt->shift);
>   exynos4_mct_tick_stop(mevt);
> 

--
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 05/11] clk: samsung: exynos7: Corrects CMU_PERIC0 clocks names

[Krzysztof Kozlowski]

On 04.09.2015 20:37, Alim Akhtar wrote:
> This patch rename CMU_PERIC0 clocks names to match with user manual.
> And also adds missing gate clock for aclk_peric0_66.
> 
> Signed-off-by: Alim Akhtar 
> ---
>  drivers/clk/samsung/clk-exynos7.c   |   12 
>  include/dt-bindings/clock/exynos7-clk.h |3 ++-
>  2 files changed, 10 insertions(+), 5 deletions(-)
> 

Reviewed-by: Krzysztof Kozlowski 

Best regards,
Krzysztof

--
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 10/11] clk: samsung: exynos7: Add missing fixed_clks to cmu_info

[Krzysztof Kozlowski]

On 04.09.2015 20:37, Alim Akhtar wrote:
> FSYS0 fixed clocks are not added to fsys0_cmu_info, this make
> some of the usb clocks as orphan. This fixes the same.
> 
> Signed-off-by: Alim Akhtar 
> ---
>  drivers/clk/samsung/clk-exynos7.c |2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Krzysztof Kozlowski 

Best regards,
Krzysztof


--
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 09/11] clk: samsung: exynos7: Corrects CMU_FSYS1 clocks names

[Krzysztof Kozlowski]

On 04.09.2015 20:37, Alim Akhtar wrote:
> This patch rename CMU_FSYS1 clocks names to match with user manual.
> And also adds missing gate clock for aclk_fsys1_200.
> 
> Signed-off-by: Alim Akhtar 
> ---
>  drivers/clk/samsung/clk-exynos7.c   |   16 ++--
>  include/dt-bindings/clock/exynos7-clk.h |3 ++-
>  2 files changed, 12 insertions(+), 7 deletions(-)

Reviewed-by: Krzysztof Kozlowski 

Best regards,
Krzysztof


--
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 08/11] clk: samsung: exynos7: Corrects CMU_FSYS0 clocks names

[Krzysztof Kozlowski]

On 04.09.2015 20:37, Alim Akhtar wrote:
> This patch rename CMU_FSYS0 clocks names to match with user manual.
> And also adds missing gate clock for aclk_fsys0_200.
> 
> Signed-off-by: Alim Akhtar 
> ---
>  drivers/clk/samsung/clk-exynos7.c   |   24 ++--
>  include/dt-bindings/clock/exynos7-clk.h |3 ++-
>  2 files changed, 16 insertions(+), 11 deletions(-)
> 

Reviewed-by: Krzysztof Kozlowski 

Best regards,
Krzysztof


--
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

[PATCH 14/17] thermal: exynos: allow compile test

[Eduardo Valentin]

Adding COMPILE_TEST flag to exynos driver to facilitate
maintenance.

Cc: Lukasz Majewski 
Cc: Zhang Rui 
Cc: linux...@vger.kernel.org
Cc: linux-samsung-soc@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Eduardo Valentin 
---
 drivers/thermal/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 0fa5d20..9069cc7 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -345,7 +345,7 @@ source "drivers/thermal/ti-soc-thermal/Kconfig"
 endmenu
 
 menu "Samsung thermal drivers"
-depends on ARCH_EXYNOS
+depends on ARCH_EXYNOS || COMPILE_TEST
 source "drivers/thermal/samsung/Kconfig"
 endmenu
 
-- 
2.5.0

--
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 07/11] clk: samsung: exynos7: Corrects CMU_PERIS clocks names

[Krzysztof Kozlowski]

On 04.09.2015 20:37, Alim Akhtar wrote:
> This patch rename CMU_PERIS clocks names to match with user manual.
> And also adds missing gate clock for aclk_peris_66.
> 
> Signed-off-by: Alim Akhtar 
> ---
>  drivers/clk/samsung/clk-exynos7.c   |7 +--
>  include/dt-bindings/clock/exynos7-clk.h |3 ++-
>  2 files changed, 7 insertions(+), 3 deletions(-)

Reviewed-by: Krzysztof Kozlowski 

Best regards,
Krzysztof


--
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 06/11] clk: samsung: exynos7: Corrects CMU_PERIC1 clocks names

[Krzysztof Kozlowski]

On 04.09.2015 20:37, Alim Akhtar wrote:
> This patch rename CMU_PERIC1 clocks names to match with user manual.
> And also adds missing gate clock for aclk_peric1_66.
> 
> Signed-off-by: Alim Akhtar 
> ---
>  drivers/clk/samsung/clk-exynos7.c   |   38 
> ---
>  include/dt-bindings/clock/exynos7-clk.h |3 ++-
>  2 files changed, 22 insertions(+), 19 deletions(-)

Reviewed-by: Krzysztof Kozlowski 

Best regards,
Krzysztof


--
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 11/11] clk: samsung: exynos7: Add required clock tree for UFS

[Krzysztof Kozlowski]

On 04.09.2015 20:37, Alim Akhtar wrote:
> Adding required mux/div/gate clocks for UFS controller
> present on Exynos7.
> 
> Signed-off-by: Alim Akhtar 
> ---
>  drivers/clk/samsung/clk-exynos7.c   |  116 
> +++
>  include/dt-bindings/clock/exynos7-clk.h |   24 ++-
>  2 files changed, 138 insertions(+), 2 deletions(-)
> 

My previous comments are fixed, rest looks good:

Reviewed-by: Krzysztof Kozlowski 

Best regards,
Krzysztof


--
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 1/3] sysfs: Fix is_visible() support for binary attributes

[Greg KH]

On Wed, Sep 09, 2015 at 10:14:46AM -0300, Emilio López wrote:
> On 09/09/15 01:12, Guenter Roeck wrote:
> >On 09/08/2015 08:58 PM, Greg KH wrote:
> >>On Tue, Sep 08, 2015 at 06:10:16PM -0700, Guenter Roeck wrote:
> >>>Hi Emilio,
> >>>
> >>>On 09/08/2015 05:51 PM, Emilio López wrote:
> Hi Greg & Guenter,
> 
> >>>[ ... ]
> >>>
> >>>Unless I am missing something, this is not explained anywhere,
> >>>but it is
> >>>not entirely trivial to understand. I think it should be documented.
> 
> I agree. I couldn't find any mention of what this int was supposed
> to be by looking at Documentation/ (is_visible is not even mentioned
> :/) or include/linux/sysfs.h. Once we settle on something I'll
> document it before sending a v2.
> 
> >>>In the include file ? No strong preference, though.
> >>>
> By the way, I wrote a quick coccinelle script to match is_visible()
> users which reference the index (included below), and it found
> references to drivers which do not seem to use any binary
> attributes, so I believe changing the index meaning shouldn't be an
> issue.
> 
> >>>Good.
> >>>
> >>I agree, make i the number of the bin attribute and that should solve
> >>this issue.
> >>
> >No, that would conflict with the "normal" use of is_visible for
> >non-binary
> >attributes, and make the index all but useless, since the
> >is_visible function
> >would have to search through all the attributes anyway to figure
> >out which one
> >is being checked.
> 
> Yeah, using the same indexes would be somewhat pointless, although
> not many seem to be using it anyway (only 14 files matched). Others
> seem to be comparing the attr* instead. An alternative would be to
> use negative indexes for binary attributes and positive indexes for
> normal attributes.
> 
> >>>... and I probably wrote or reviewed a significant percentage of
> >>>those ;-).
> >>>
> >>>Using negative numbers for binary attributes is an interesting idea.
> >>>Kind of unusual, though. Greg, any thoughts on that ?
> >>
> >>Ick, no, that's a mess, maybe we just could drop the index alltogether?
> >>
> >
> >No, please don't. Having to manually compare dozens of index pointers
> >would be
> >even more of a mess.
> 
> So, what about keeping it the way it is in the patch, and documenting it
> thoroughly? Otherwise, we could introduce another "is_bin_visible" function
> to do this same thing but just on binary attributes, but I'd rather not add
> a new function pointer if possible.

is_bin_visiable makes sense to me instead of trying to overload the
index number in some crazy way.  There's no issue with adding another
function pointer.

thanks,

greg k-h
--
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