question about

question about

[Kevin Wilson]

Hi,
Any idea which methods adds the following entries:
/sys/devices/system/clocksource/clocksource0
/sys/devices/system/clocksource/clocksource0/available_clocksource

?

Best,
KevinW

question about

[Moritz Fischer]

On Sun, Mar 24, 2013 at 1:29 PM, Kevin Wilson <wkevils@gmail.com> wrote:

Kevin,

> Any idea which methods adds the following entries:
> /sys/devices/system/clocksource/clocksource0
> /sys/devices/system/clocksource/clocksource0/available_clocksource

I'm not an expert, but a quick grep for 'available_clocksource' led me
to the kernel/time/clocksource.c file.

Hope that helps you out,

- Moritz

question about

[Rami Rosen]

Hi,
In init_clocksource_sysfs() method,
device_register() adds the folder:
	/sys/devices/system/clocksource/clocksource0,
whereas subsys_system_register() adds the parent folder
(/sys/devices/system/clocksource)
see:
http://lxr.free-electrons.com/source/kernel/time/clocksource.c#L902


rgs,
Rami Rosen
http://ramirose.wix.com/ramirosen

On Mon, Mar 25, 2013 at 4:57 AM, Moritz Fischer
<moritz.fischer@ettus.com> wrote:
> On Sun, Mar 24, 2013 at 1:29 PM, Kevin Wilson <wkevils@gmail.com> wrote:
>
> Kevin,
>
>> Any idea which methods adds the following entries:
>> /sys/devices/system/clocksource/clocksource0
>> /sys/devices/system/clocksource/clocksource0/available_clocksource
>
> I'm not an expert, but a quick grep for 'available_clocksource' led me
> to the kernel/time/clocksource.c file.
>
> Hope that helps you out,
>
> - Moritz
>
> _______________________________________________
> Kernelnewbies mailing list
> Kernelnewbies at kernelnewbies.org
> http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Question about

[Vladimir Zapolskiy]

Hello,

I need to set a GPIO (active high) output high on boot, which enables a
power rail supplying some external devices.

I have a question regarding "regulator-boot-on" and "enable-active-high"
fixed regulator device tree properties (actually AFAIU it applies to
gpio regulator as well, by the way, which one is proper to use in my
situation?)

Here is what we have from the code:

  [...]
  constraints->boot_on = of_property_read_bool(np, "regulator-boot-on");
  [...]
  if (init_data->constraints.boot_on)
          config->enabled_at_boot = true;
  [...]
  config->enable_high = of_property_read_bool(np, "enable-active-high");
  [...]
  cfg.ena_gpio_invert = !config->enable_high;
  if (config->enabled_at_boot) {
          if (config->enable_high)
                  cfg.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH;
          else
                  cfg.ena_gpio_flags |= GPIOF_OUT_INIT_LOW;
  } else {
          if (config->enable_high)
                  cfg.ena_gpio_flags |= GPIOF_OUT_INIT_LOW;
          else
                 cfg.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH;
  }
  [...]
  ret = gpio_request_one(config->ena_gpio,
                         GPIOF_DIR_OUT | config->ena_gpio_flags,
                         rdev_get_name(rdev));
  [...]
  /* Enable GPIO at initial use */
  if (pin->enable_count == 0)
          gpiod_set_value_cansleep(pin->gpiod,
                                  !pin->ena_gpio_invert);
  [...]


If we simplify the matter by assuming GPIOF_OUT_INIT_LOW is inverted
GPIOF_OUT_INIT_HIGH and vice versa, then it is easy to compute by
running over the variants that GPIO output value is set HIGH (regardless
of GPIO active low status) if and only if "regulator-boot-on" is
provided and "enable-active-high" has no effect at all.

This fact confuses me, because from the general regulator and fixed
regulator device tree bindings documentation I get:

  [...]
  - regulator-boot-on: bootloader/firmware enabled regulator
  [...]
  - enable-active-high: Polarity of GPIO is Active high
  If this property is missing, the default assumed is Active low.
  [...]

According to the documentation I'd assume that "regulator-boot-on" does
not touch gpio output value setting (so, if it is controlled by
bootloader or firmware, then it might be out of Linux kernel control).
Also my impression of "enable-active-high" property is that is should
have some effect on the GPIO output value (but it is not, see above),
and actually I don't quite understand why this property exists - there
is a high chance that "enable-active-high" and the real GPIO polarity do
not coincide, it should be more reliable to get GPIO flags of a
particular GPIO right in the regulator driver/framework.

Let's consider two possible configurations:

| regulator-boot-on | enable-active-high | GPIO polarity | GPIO output |
+-------------------+--------------------+---------------+-------------+
|        no         |         yes        |  active high  |    low      |
|        no         |          no        |  active low   |   high      |

I'd rather think that both resulting GPIO outputs are incorrect or
better to say do not correspond to my perception of "regulator-boot-on"
and "enable-active-high" DTS properties described in the documentation,
however above "enable-active-high" and actual GPIO polarity are the same
(when they are not, it is another open topic for discussion).

Do I miss something or have a mistake? Is there a problem in the
implemented logic?

Should documentation be updated to reflect "regulator-boot-on" role that
a regulator is re-enabled by the kernel?

Should "enable-active-high" be replaced by getting GPIO flags directly?

Thank you in advance.

--
With best wishes,
Vladimir

Question about

[Vladimir Zapolskiy]

Hello,

I need to set a GPIO (active high) output high on boot, which enables a
power rail supplying some external devices.

I have a question regarding "regulator-boot-on" and "enable-active-high"
fixed regulator device tree properties (actually AFAIU it applies to
gpio regulator as well, by the way, which one is proper to use in my
situation?)

Here is what we have from the code:

  [...]
  constraints->boot_on = of_property_read_bool(np, "regulator-boot-on");
  [...]
  if (init_data->constraints.boot_on)
          config->enabled_at_boot = true;
  [...]
  config->enable_high = of_property_read_bool(np, "enable-active-high");
  [...]
  cfg.ena_gpio_invert = !config->enable_high;
  if (config->enabled_at_boot) {
          if (config->enable_high)
                  cfg.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH;
          else
                  cfg.ena_gpio_flags |= GPIOF_OUT_INIT_LOW;
  } else {
          if (config->enable_high)
                  cfg.ena_gpio_flags |= GPIOF_OUT_INIT_LOW;
          else
                 cfg.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH;
  }
  [...]
  ret = gpio_request_one(config->ena_gpio,
                         GPIOF_DIR_OUT | config->ena_gpio_flags,
                         rdev_get_name(rdev));
  [...]
  /* Enable GPIO at initial use */
  if (pin->enable_count == 0)
          gpiod_set_value_cansleep(pin->gpiod,
                                  !pin->ena_gpio_invert);
  [...]


If we simplify the matter by assuming GPIOF_OUT_INIT_LOW is inverted
GPIOF_OUT_INIT_HIGH and vice versa, then it is easy to compute by
running over the variants that GPIO output value is set HIGH (regardless
of GPIO active low status) if and only if "regulator-boot-on" is
provided and "enable-active-high" has no effect at all.

This fact confuses me, because from the general regulator and fixed
regulator device tree bindings documentation I get:

  [...]
  - regulator-boot-on: bootloader/firmware enabled regulator
  [...]
  - enable-active-high: Polarity of GPIO is Active high
  If this property is missing, the default assumed is Active low.
  [...]

According to the documentation I'd assume that "regulator-boot-on" does
not touch gpio output value setting (so, if it is controlled by
bootloader or firmware, then it might be out of Linux kernel control).
Also my impression of "enable-active-high" property is that is should
have some effect on the GPIO output value (but it is not, see above),
and actually I don't quite understand why this property exists - there
is a high chance that "enable-active-high" and the real GPIO polarity do
not coincide, it should be more reliable to get GPIO flags of a
particular GPIO right in the regulator driver/framework.

Let's consider two possible configurations:

| regulator-boot-on | enable-active-high | GPIO polarity | GPIO output |
+-------------------+--------------------+---------------+-------------+
|        no         |         yes        |  active high  |    low      |
|        no         |          no        |  active low   |   high      |

I'd rather think that both resulting GPIO outputs are incorrect or
better to say do not correspond to my perception of "regulator-boot-on"
and "enable-active-high" DTS properties described in the documentation,
however above "enable-active-high" and actual GPIO polarity are the same
(when they are not, it is another open topic for discussion).

Do I miss something or have a mistake? Is there a problem in the
implemented logic?

Should documentation be updated to reflect "regulator-boot-on" role that
a regulator is re-enabled by the kernel?

Should "enable-active-high" be replaced by getting GPIO flags directly?

Thank you in advance.

--
With best wishes,
Vladimir

RE: question about

[Haojian Zhuang]

[-- Attachment #1: Type: text/plain, Size: 1436 bytes --]

Thanks for your good catch. We needn't this checking at here. Now I format this patch.

Best Regards
Haojian

-----Original Message-----
From: Dan Carpenter [mailto:error27@gmail.com] 
Sent: 2010年5月28日 7:46 PM
To: Haojian Zhuang
Cc: kernel-janitors@vger.kernel.org
Subject: question about

Hi,

I was going through some Smatch stuff and it complains about something
in drivers/leds/leds-88pm860x.c  Could you take a look at it?  I'm not
sure what was intended there.

drivers/leds/leds-88pm860x.c +228 __check_device(6) warn: unsigned 'p->flags' is never less than zero.
   222  static int __check_device(struct pm860x_led_pdata *pdata, char *name)
   223  {
   224          struct pm860x_led_pdata *p = pdata;
   225          int ret = -EINVAL;
   226
   227          while (p && p->id) {
   228                  if ((p->id != PM8606_ID_LED) || (p->flags < 0))
                                                         ^^^^^^^^^^^^

	p->flags is an unsigned int so this condition is never true.

   229                          break;
   230
   231                  if (!strncmp(name, pm860x_led_name[p->flags],
   232                          MFD_NAME_SIZE)) {
   233                          ret = (int)p->flags;
   234                          break;
   235                  }
   236                  p++;
   237          }
   238          return ret;
   239  }

regards,
dan carpenter

[-- Attachment #2: 0001-leds-remove-redundant-checking.patch --]
[-- Type: application/octet-stream, Size: 922 bytes --]

From 59b2a1391d531bf06143ad5fc7b86490fead5065 Mon Sep 17 00:00:00 2001
From: Haojian Zhuang <haojian.zhuang@marvell.com>
Date: Fri, 28 May 2010 21:36:47 +0800
Subject: [PATCH] leds: remove redundant checking

flags variable is declared as unsigned. So we needn't to check whether it's
negative value.

Signed-off-by: Haojian Zhuang <haojian.zhuang@marvell.com>
---
 drivers/leds/leds-88pm860x.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/leds/leds-88pm860x.c b/drivers/leds/leds-88pm860x.c
index da146da..5decc1c 100644
--- a/drivers/leds/leds-88pm860x.c
+++ b/drivers/leds/leds-88pm860x.c
@@ -160,7 +160,7 @@ static int __check_device(struct pm860x_led_pdata *pdata, char *name)
 	int ret = -EINVAL;
 
 	while (p && p->id) {
-		if ((p->id != PM8606_ID_LED) || (p->flags < 0))
+		if (p->id != PM8606_ID_LED)
 			break;
 
 		if (!strncmp(name, pm860x_led_name[p->flags],
-- 
1.5.6.5

question about

[Dan Carpenter]

Hi,

I was going through some Smatch stuff and it complains about something
in drivers/leds/leds-88pm860x.c  Could you take a look at it?  I'm not
sure what was intended there.

drivers/leds/leds-88pm860x.c +228 __check_device(6) warn: unsigned 'p->flags' is never less than zero.
   222  static int __check_device(struct pm860x_led_pdata *pdata, char *name)
   223  {
   224          struct pm860x_led_pdata *p = pdata;
   225          int ret = -EINVAL;
   226
   227          while (p && p->id) {
   228                  if ((p->id != PM8606_ID_LED) || (p->flags < 0))
                                                         ^^^^^^^^^^^^

	p->flags is an unsigned int so this condition is never true.

   229                          break;
   230
   231                  if (!strncmp(name, pm860x_led_name[p->flags],
   232                          MFD_NAME_SIZE)) {
   233                          ret = (int)p->flags;
   234                          break;
   235                  }
   236                  p++;
   237          }
   238          return ret;
   239  }

regards,
dan carpenter