Re: [PATCH AUTOSEL 6.1 3/7] x86/hyperv: Use slow_virt_to_phys() in page transition hypervisor callback

[Pavel Machek]

Hi!

> In preparation for temporarily marking pages not present during a
> transition between encrypted and decrypted, use slow_virt_to_phys()
> in the hypervisor callback. As long as the PFN is correct,

This seems to be preparation for something we don't plan to do in
-stable. Please drop.

Best regards,
Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.


signature.asc
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 17/17] staging: nuc-led: update the TODOs

[Pavel Machek]

Hi!

> > > -  Need to validate the uAPI and document it before moving
> > >this driver out of staging.  
> > 
> > >  - Stabilize and document its sysfs interface.  
> >
> > Would you mind starting with this one?
> 
> Do you mean writing the ABI document for it? Surely I can do that,
> but I'm not sure where to put such document while it is on staging.

No need for formal ABI just yet, but it needs to be clear what the interface
is.

> > We should have existing APIs
> > for most of functionality described...
> 
> I tried to stay as close as possible to the existing API, but
> there are some things that required a different one.

I believe it should be possible to move _way_ closer to existing APIs.

> For instance, with WMI rev 0.64 and 1.0, any LED of the device
> can be programmed to be a power indicator.
> 
> When a LED is programmed this way, there are up to 3 (on rev 1.0) or up
> to 4 (on rev 0.64) different brightness level of the LED, and those
> are associated with a power status (like S0, S3, S5, "ready mode").

I'll need a description how this works.

>   /sys/class/leds/nuc::front1/
>   ├── blink_behavior
>   ├── blink_frequency

We have timer trigger for these.

>   ├── ethernet_type
>   ├── hdd_default
>   ├── indicator
>   ├── ready_mode_blink_behavior
>   ├── ready_mode_blink_frequency
>   ├── ready_mode_brightness
>   ├── s0_blink_behavior
>   ├── s0_blink_frequency
>   ├── s0_brightness
>   ├── s3_blink_behavior
>   ├── s3_blink_frequency
>   ├── s3_brightness
>   ├── s5_blink_behavior
>   ├── s5_blink_frequency
>   ├── s5_brightness

No. Take a look at triggers; for example hdd monitor should look very
much like existing disk trigger.

> On other words, the "indicator" tells what type of hardware event
> the LED is currently measuring:
> 
>   $ cat /sys/class/leds/nuc\:\:front1/indicator 
>   Power State  [HDD Activity]  Ethernet  WiFi  Software  Power Limit  
> Disable  

So this will use existing "trigger" infrastructure.

> That should likely be easier to discuss if any changes at the
> ABI would be needed. Before moving it out of staging, I would
> add another one under Documentation/ABI describing the meaning
> of each sysfs node.

Lets get reasonable ABI before moving it _into_ tree, staging or
otherwise.

Best regards,
Pavel
-- 
http://www.livejournal.com/~pavelmachek


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 17/17] staging: nuc-led: update the TODOs

[Pavel Machek]

Hi!

> -  Need to validate the uAPI and document it before moving
>this driver out of staging.

>  - Stabilize and document its sysfs interface.
   
Would you mind starting with this one? We should have existing APIs
for most of functionality described...

We really don't want to merge code with bad API, not even to staging.


Best regards,
Pavel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 10/10] errno.h: Provide EFSCORRUPTED for everybody

[Pavel Machek]

Hi!

> > There's currently 6 filesystems that have the same #define. Move it
> > into errno.h so it's defined in just one place.
> > 
> > Signed-off-by: Valdis Kletnieks 
> > Acked-by: Darrick J. Wong 
> > Reviewed-by: Jan Kara 
> > Acked-by: Theodore Ts'o 
> 
> >  fs/erofs/internal.h  | 2 --
> 
> >  fs/f2fs/f2fs.h   | 1 -
> 
> Acked-by: Chao Yu 

Are we still using EUCLEAN for something else than EFSCORRUPTED? Could
we perhaps change the glibc definiton to "your filesystem is
corrupted" in the long run?

Best regards,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 01/24] erofs: add on-disk layout

[Pavel Machek]

Hi!

> > No. gdb tells you what the actual offsets _are_.
> 
> Ok, reading your reply twice, I think we have different perspectives. I
> don't trust the comments.
> 
> The tool I had in mind is pahole that parses dwarf information about the
> structures, the same as gdb does. The actual value of the struct members
> is the thing that needs to be investigated in memory dumps or disk image
> dumps.
> 
> > > > The expected offset is somewhat valuable, but
> > > > perhaps the form is a bit off given the visual
> > > > run-in to the field types.
> > > > 
> > > > The extra work with this form is manipulating all
> > > > the offsets whenever a structure change occurs.
> > > 
> > > ... while this is error prone.
> > 
> > While the comment tells you what they _should be_.
> 
> That's exactly the source of confusion and bugs. For me an acceptable
> way of asserting that a value has certain offset is a build check, eg.
> like
> 
> BUILD_BUG_ON(strct my_superblock, magic, 16);

Yes, that would work, too. As would documentation file with the disk
structures.

Best regards,
Pavel
-- 
DENX Software Engineering GmbH,  Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 01/24] erofs: add on-disk layout

[Pavel Machek]

Hi!

> > > Rather than they didn't run "gdb" or "pahole" and change it by mistake.
> > 
> > I think Christoph is not right here.
> > 
> > Using external tools for validation is extra work
> > when necessary for understanding the code.
> 
> The advantage of using the external tools that the information about
> offsets is provably correct ...

No. gdb tells you what the actual offsets _are_.

> > The expected offset is somewhat valuable, but
> > perhaps the form is a bit off given the visual
> > run-in to the field types.
> > 
> > The extra work with this form is manipulating all
> > the offsets whenever a structure change occurs.
> 
> ... while this is error prone.

While the comment tells you what they _should be_.

Pavel
-- 
DENX Software Engineering GmbH,  Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 01/24] erofs: add on-disk layout

[Pavel Machek]

Hi!

> > +struct erofs_super_block {
> > +/*  0 */__le32 magic;   /* in the little endian */
> > +/*  4 */__le32 checksum;/* crc32c(super_block) */
> > +/*  8 */__le32 features;/* (aka. feature_compat) */
> > +/* 12 */__u8 blkszbits; /* support block_size == PAGE_SIZE only */
> 
> Please remove all the byte offset comments.  That is something that can
> easily be checked with gdb or pahole.

I don't think I agree. gdb will tell you byte offsets _on one
architecture_. But filesystem is supposed to be portable between them. 

> > +/* 64 */__u8 volume_name[16];   /* volume name */
> > +/* 80 */__le32 requirements;/* (aka. feature_incompat) */
> > +
> > +/* 84 */__u8 reserved2[44];
> > +} __packed; /* 128 bytes */
> 
> Please don't add __packed.  In this case I think you don't need it
> (but double check with pahole), but even if you would need it using
> proper padding fields and making sure all fields are naturally aligned
> will give you much better code generation on architectures that don't
> support native unaligned access.

This is on-disk structure, right?

drivers/staging/erofs/super.c:  struct erofs_super_block *layout;
drivers/staging/erofs/super.c:  layout = (struct erofs_super_block
*)((u8 *)bh->b_data

So __packed is right thing to do. If architecture accesses that
slowly, that's ungood, but different structures between architectures
would be really bad.

Best regards,
Pavel
-- 
DENX Software Engineering GmbH,  Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/9] staging: move greybus core out of staging

[Pavel Machek]

Hi!

> The Greybus code has long been "stable" but was living in the
> drivers/staging/ directory to see if there really was going to be
> devices using this protocol over the long-term.  With the success of
> millions of phones with this hardware and code in it, and the recent

So, what phones do have this, for example?

Does that mean that there's large choice of phones well supported by the
mainline?

Best regards,   Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7 08/24] erofs: add namei functions

[Pavel Machek]

Hi!

> > > + /*
> > > +  * on-disk error, let's only BUG_ON in the debugging mode.
> > > +  * otherwise, it will return 1 to just skip the invalid name
> > > +  * and go on (in consideration of the lookup performance).
> > > +  */
> > > + DBG_BUGON(qd->name > qd->end);
> > 
> > I believe you should check for errors in non-debug mode, too.
> 
> Thanks for your kindly reply!
> 
> The following is just my personal thought... If I am wrong, please
> kindly point out...
> 
> As you can see, this is a new prefixed string binary search algorithm
> which can provide similar performance with hashed approach (but no
> need to store hash value at all), so I really care about its lookup
> performance.
> 
> There is something needing to be concerned, is, whether namei() should
> report any potential on-disk issues or just return -ENOENT for these
> corrupted dirs, I think I tend to use the latter one.

-ENOENT is okay for corrupted directories, as long as corrupted
directories do not cause some kind of security bugs (memory
corruption, crashes, ...)


Best regards,
Pavel
-- 
DENX Software Engineering GmbH,  Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7 08/24] erofs: add namei functions

[Pavel Machek]

Hi!

> + /*
> +  * on-disk error, let's only BUG_ON in the debugging mode.
> +  * otherwise, it will return 1 to just skip the invalid name
> +  * and go on (in consideration of the lookup performance).
> +  */
> + DBG_BUGON(qd->name > qd->end);

I believe you should check for errors in non-debug mode, too.


> + if (unlikely(!ndirents)) {
> + DBG_BUGON(1);
> + kunmap_atomic(de);
> + put_page(page);
> + page = ERR_PTR(-EIO);
> + goto out;
> + }

-EUCLEAN is right error code for corrupted filesystem. (And you
 probably want to print something to the syslog, too).

Pavel
-- 
DENX Software Engineering GmbH,  Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 00/24] erofs: promote erofs from staging

[Pavel Machek]

Hi!

> >> Changelog from v1:
> >>  o resend the whole filesystem into a patchset suggested by Greg;
> >>  o code is more cleaner, especially for decompression frontend.
> >>
> >> --8<--
> >>
> >> Hi,
> >>
> >> EROFS file system has been in Linux-staging for about a year.
> >> It has been proved to be stable enough to move out of staging
> >> by 10+ millions of HUAWEI Android mobile phones on the market
> >> from EMUI 9.0.1, and it was promoted as one of the key features
> >> of EMUI 9.1 [1], including P30(pro).
> > 
> > Ok, maybe it is ready to be moved to kernel proper, but as git can
> > do moves, would it be better to do it as one commit?
> > 
> > Separate patches are still better for review, I guess.
> 
> Thanks for you reply. Either form is OK for me... The first step could
> be that I hope someone could kindly take some time to look into these
> patches... :)
> 
> The patch v2 is slightly different for the current code in the staging
> tree since I did some code cleanup these days (mainly renaming / moving,
> including rename unzip_vle.{c,h} to zdata.{c,h} and some confusing
> structure names and clean up internal.h...). No functional chance and I
> can submit cleanup patches to staging as well if doing moves by git...

I believe you should get those committed to staging/, yes. Then you
ask Al Viro to do pull the git move, I guess, and you follow his
instructions at that point...

FILESYSTEMS (VFS and infrastructure)
M:  Alexander Viro 
L:  linux-fsde...@vger.kernel.org

Best regards,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 00/24] erofs: promote erofs from staging

[Pavel Machek]

On Thu 2019-07-11 22:57:31, Gao Xiang wrote:
> Changelog from v1:
>  o resend the whole filesystem into a patchset suggested by Greg;
>  o code is more cleaner, especially for decompression frontend.
> 
> --8<--
> 
> Hi,
> 
> EROFS file system has been in Linux-staging for about a year.
> It has been proved to be stable enough to move out of staging
> by 10+ millions of HUAWEI Android mobile phones on the market
> from EMUI 9.0.1, and it was promoted as one of the key features
> of EMUI 9.1 [1], including P30(pro).

Ok, maybe it is ready to be moved to kernel proper, but as git can
do moves, would it be better to do it as one commit?

Separate patches are still better for review, I guess.
Pavel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4/4] rts5208: Fix usleep range is preferred over udelay

[Pavel Machek]

On Wed 2019-06-19 17:46:48, Lukas Schneider wrote:
> This patch fixes the issue reported by checkpatch:
> 
> CHECK: usleep_range is preferred over udelay;
> see Doucmentation/timers/timers-howto.txt
> 
> It's save to sleep here instead of using busy waiting,
> because we are not in an atomic context.

Is it good idea? How can the system really sleep for 50 usec?

 Pavel

> @@ -865,7 +865,7 @@ static int sd_change_phase(struct rtsx_chip *chip, u8 
> sample_point, u8 tune_dir)
>PHASE_CHANGE);
>   if (retval)
>   return retval;
> - udelay(50);
> + usleep_range(50, 60);
>   retval = rtsx_write_register(chip, SD_VP_CTL, 0xFF,
>PHASE_CHANGE |
>PHASE_NOT_RESET |
> @@ -877,14 +877,14 @@ static int sd_change_phase(struct rtsx_chip *chip, u8 
> sample_point, u8 tune_dir)
>CHANGE_CLK, CHANGE_CLK);
>   if (retval)
>   return retval;
> - udelay(50);
> + usleep_range(50, 60);
>   retval = rtsx_write_register(chip, SD_VP_CTL, 0xFF,
>PHASE_NOT_RESET |
>sample_point);
>   if (retval)
>   return retval;
>   }
> - udelay(100);
> + usleep_range(100, 110);
>  
>   rtsx_init_cmd(chip);
>   rtsx_add_cmd(chip, WRITE_REG_CMD, SD_DCMPS_CTL, DCMPS_CHANGE,
> @@ -918,7 +918,7 @@ static int sd_change_phase(struct rtsx_chip *chip, u8 
> sample_point, u8 tune_dir)
>   return retval;
>   }
>  
> - udelay(50);
> + usleep_range(50, 60);
>   }
>  
>   retval = rtsx_write_register(chip, SD_CFG1, SD_ASYNC_FIFO_NOT_RST, 0);
> @@ -1416,7 +1416,7 @@ static int sd_wait_data_idle(struct rtsx_chip *chip)
>   retval = STATUS_SUCCESS;
>   break;
>   }
> - udelay(100);
> + usleep_range(100, 110);
>   }
>   dev_dbg(rtsx_dev(chip), "SD_DATA_STATE: 0x%02x\n", val);
>  

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4] staging: olpc_dcon: olpc_dcon_xo_1.c: Switch to the gpio descriptor interface

[Pavel Machek]

Hi!

> Use the gpiod interface instead of the deprecated old non-descriptor
> interface in olpc_dcon_xo_1.c.
> 
> Signed-off-by: Nishad Kamdar 

You may want to cc: lkund...@v3.sk, he was doing great work on OLPC
lately...

Best regards,
Pavel

> ---
> Changes in v4:
>  - Move changelog after signed-off line.
> Changes in v3:
>  - Resolve a few compilation errors.
> Changes in v2:
>  - Resolve a few compilation errors.
>  - Add a level of indirection to read and write gpios.
> ---
>  drivers/staging/olpc_dcon/olpc_dcon_xo_1.c | 90 +++---
>  1 file changed, 47 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/staging/olpc_dcon/olpc_dcon_xo_1.c 
> b/drivers/staging/olpc_dcon/olpc_dcon_xo_1.c
> index ff145d493e1b..80b8d4153414 100644
> --- a/drivers/staging/olpc_dcon/olpc_dcon_xo_1.c
> +++ b/drivers/staging/olpc_dcon/olpc_dcon_xo_1.c
> @@ -11,35 +11,51 @@
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
>  #include 
> -#include 
> +#include 
>  #include 
> +#include 
>  #include 
>  
>  #include "olpc_dcon.h"
>  
> +enum dcon_gpios {
> + OLPC_DCON_STAT0,
> + OLPC_DCON_STAT1,
> + OLPC_DCON_IRQ,
> + OLPC_DCON_LOAD,
> + OLPC_DCON_BLANK,
> +};
> +
> +struct dcon_gpio {
> + const char *name;
> + unsigned long flags;
> +};
> +
> +static const struct dcon_gpio gpios_asis[] = {
> + [OLPC_DCON_STAT0] = { .name = "dcon_stat0", .flags = GPIOD_ASIS },
> + [OLPC_DCON_STAT1] = { .name = "dcon_stat1", .flags = GPIOD_ASIS },
> + [OLPC_DCON_IRQ] = { .name = "dcon_irq", .flags = GPIOD_ASIS },
> + [OLPC_DCON_LOAD] = { .name = "dcon_load", .flags = GPIOD_ASIS },
> + [OLPC_DCON_BLANK] = { .name = "dcon_blank", .flags = GPIOD_ASIS },
> +};
> +
> +struct gpio_desc *gpios[5];
> +
>  static int dcon_init_xo_1(struct dcon_priv *dcon)
>  {
>   unsigned char lob;
> -
> - if (gpio_request(OLPC_GPIO_DCON_STAT0, "OLPC-DCON")) {
> - pr_err("failed to request STAT0 GPIO\n");
> - return -EIO;
> - }
> - if (gpio_request(OLPC_GPIO_DCON_STAT1, "OLPC-DCON")) {
> - pr_err("failed to request STAT1 GPIO\n");
> - goto err_gp_stat1;
> - }
> - if (gpio_request(OLPC_GPIO_DCON_IRQ, "OLPC-DCON")) {
> - pr_err("failed to request IRQ GPIO\n");
> - goto err_gp_irq;
> - }
> - if (gpio_request(OLPC_GPIO_DCON_LOAD, "OLPC-DCON")) {
> - pr_err("failed to request LOAD GPIO\n");
> - goto err_gp_load;
> - }
> - if (gpio_request(OLPC_GPIO_DCON_BLANK, "OLPC-DCON")) {
> - pr_err("failed to request BLANK GPIO\n");
> - goto err_gp_blank;
> + int ret, i;
> + struct dcon_gpio *pin = _asis[0];
> +
> + for (i = 0; i < ARRAY_SIZE(gpios_asis); i++) {
> + gpios[i] = devm_gpiod_get(>client->dev, pin[i].name,
> +   pin[i].flags);
> + if (IS_ERR(gpios[i])) {
> + ret = PTR_ERR(gpios[i]);
> + pr_err("failed to request %s GPIO: %d\n", pin[i].name,
> +ret);
> + return ret;
> + }
>   }
>  
>   /* Turn off the event enable for GPIO7 just to be safe */
> @@ -61,12 +77,12 @@ static int dcon_init_xo_1(struct dcon_priv *dcon)
>   dcon->pending_src = dcon->curr_src;
>  
>   /* Set the directions for the GPIO pins */
> - gpio_direction_input(OLPC_GPIO_DCON_STAT0);
> - gpio_direction_input(OLPC_GPIO_DCON_STAT1);
> - gpio_direction_input(OLPC_GPIO_DCON_IRQ);
> - gpio_direction_input(OLPC_GPIO_DCON_BLANK);
> - gpio_direction_output(OLPC_GPIO_DCON_LOAD,
> -   dcon->curr_src == DCON_SOURCE_CPU);
> + gpiod_direction_input(gpios[OLPC_DCON_STAT0]);
> + gpiod_direction_input(gpios[OLPC_DCON_STAT1]);
> + gpiod_direction_input(gpios[OLPC_DCON_IRQ]);
> + gpiod_direction_input(gpios[OLPC_DCON_BLANK]);
> + gpiod_direction_output(gpios[OLPC_DCON_LOAD],
> +dcon->curr_src == DCON_SOURCE_CPU);
>  
>   /* Set up the interrupt mappings */
>  
> @@ -84,7 +100,7 @@ static int dcon_init_xo_1(struct dcon_priv *dcon)
>   /* Register the interrupt handler */
>   if (request_irq(DCON_IRQ, _interrupt, 0, "DCON", dcon)) {
>   pr_err("failed to request DCON's irq\n");
> - goto err_req_irq;
> + return -EIO;
>   }
>  
>   /* Clear INV_EN for GPIO7 (DCONIRQ) */
> @@ -125,18 +141,6 @@ static int dcon_init_xo_1(struct dcon_priv *dcon)
>   cs5535_gpio_set(OLPC_GPIO_DCON_BLANK, GPIO_EVENTS_ENABLE);
>  
>   return 0;
> -
> -err_req_irq:
> - gpio_free(OLPC_GPIO_DCON_BLANK);
> -err_gp_blank:
> - gpio_free(OLPC_GPIO_DCON_LOAD);
> -err_gp_load:
> - gpio_free(OLPC_GPIO_DCON_IRQ);
> -err_gp_irq:
> - gpio_free(OLPC_GPIO_DCON_STAT1);
> -err_gp_stat1:
> - gpio_free(OLPC_GPIO_DCON_STAT0);
> 

Re: [PATCH v1 7/8] PM / Hibernate: use pfn_to_online_page()

[Pavel Machek]

On Mon 2018-11-19 11:16:15, David Hildenbrand wrote:
> Let's use pfn_to_online_page() instead of pfn_to_page() when checking
> for saveable pages to not save/restore offline memory sections.
> 
> Cc: "Rafael J. Wysocki" 

Acked-by: Pavel Machek 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 06/15] Platform: OLPC: Add XO-1.75 EC driver

[Pavel Machek]

Hi!

> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > 
> > Easy to maintain when it's sorted.

No / it depends.

> > > +   channel = priv->rx_buf[0];
> > > +   byte = priv->rx_buf[1];
> > 
> > Maybe specific structures would fit better?
> > 
> > Like
> > 
> > struct olpc_ec_resp_hdr {
> >  u8 channel;
> >  u8 byte;
> > ...
> > }

Structures have padding and other nastyness...

> > > +   pm_wakeup_event(priv->pwrbtn->dev.parent,
> > > 1000);
> > 
> > Magic number.

Nothing wrong with magic numbers.

> > > +   args[0] = mask & 0xff;
> > > +   args[1] = (mask >> 8) & 0xff;
> > 
> > ...mask >> 0;
> > ...mask >> 8;

No, please.

> > __maybe_unused  instead of ugly #ifdef?
> > 
> > > +{
> > > +   struct platform_device *pdev = to_platform_device(dev);
> > > +   struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);
> > 
> > dev_get_drvdata() or how is it called?
> > 
> > > +   unsigned char hintargs[5];
> > 
> > struct olpc_ec_hint_cmd {
> > u8 ...
> > u32 ...
> > };
> > 
> > ?

No, unless you want to break the code. Or add __attribute__ packed and
deal with endianness.

Just no.

> > > +   static unsigned int suspend_count;
> > 
> > u32 I suppose.

You know, there's semantic difference between unsigned int and
u32. And this sounds like candidate for unsigned int.

> > > +   /* Enable all EC events while we're awake */
> > > +   olpc_xo175_ec_set_event_mask(0x);
> > 
> > #define EC_ALL_EVENTS GENMASK(15, 0)

Actually that's less readable. Just don't.

> > > +static const struct of_device_id olpc_xo175_ec_of_match[] = {
> > > +   { .compatible = "olpc,xo1.75-ec" },
> > > +   { },
> > 
> > No comma for terminators.

Comma is fine.
Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RFC 6/6] PM / Hibernate: exclude all PageOffline() pages

[Pavel Machek]

On Wed 2018-11-14 22:17:04, David Hildenbrand wrote:
> The content of pages that are marked PG_offline is not of interest
> (e.g. inflated by a balloon driver), let's skip these pages.
> 
> Cc: "Rafael J. Wysocki" 

Acked-by: Pavel Machek 

> diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
> index b0308a2c6000..01db1d13481a 100644
> --- a/kernel/power/snapshot.c
> +++ b/kernel/power/snapshot.c
> @@ -1222,7 +1222,7 @@ static struct page *saveable_highmem_page(struct zone 
> *zone, unsigned long pfn)
>   BUG_ON(!PageHighMem(page));
>  
>   if (swsusp_page_is_forbidden(page) ||  swsusp_page_is_free(page) ||
> - PageReserved(page))
> + PageReserved(page) || PageOffline(page))
>   return NULL;
>  
>   if (page_is_guard(page))
> @@ -1286,6 +1286,9 @@ static struct page *saveable_page(struct zone *zone, 
> unsigned long pfn)
>   if (swsusp_page_is_forbidden(page) || swsusp_page_is_free(page))
>   return NULL;
>  
> + if (PageOffline(page))
> + return NULL;
> +
>   if (PageReserved(page)
>   && (!kernel_page_present(page) || pfn_is_nosave(pfn)))
>   return NULL;

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 15/15] power: supply: olpc_battery: Add OLPC XO 1.75 support

[Pavel Machek]

On Wed 2018-10-10 19:23:00, Lubomir Rintel wrote:
> The battery and the protocol are essentially the same as OLPC XO 1.5,
> but the responses from the EC are LSB first.
> 
> Signed-off-by: Lubomir Rintel 

Acked-by: Pavel Machek 


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 14/15] power: supply: olpc_battery: Avoid using platform_info

[Pavel Machek]

On Wed 2018-10-10 19:22:59, Lubomir Rintel wrote:
> This wouldn't work on the DT-based ARM platform. Let's read the EC version
> directly from the EC driver instead.
> 
> This makes the driver no longer x86 specific.
> 
> Signed-off-by: Lubomir Rintel 

Acked-by: Pavel Machek 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 13/15] power: supply: olpc_battery: Move priv data to a struct

[Pavel Machek]

Hi!

> The global variables for private data are not too nice. I'd like some
> more, and that would clutter the global name space even further.
> 
> Signed-off-by: Lubomir Rintel 
> Reviewed-by: Andy Shevchenko 

Ok...

> - olpc_bat = power_supply_register(>dev, _bat_desc, NULL);
> - if (IS_ERR(olpc_bat)) {
> - ret = PTR_ERR(olpc_bat);
> - goto battery_failed;
> - }
> + data->olpc_bat = devm_power_supply_register(>dev, _bat_desc, 
> _cfg);
> + if (IS_ERR(data->olpc_bat))
> + return PTR_ERR(data->olpc_bat);
>  
> - ret = device_create_bin_file(_bat->dev, _bat_eeprom);
> + ret = device_create_bin_file(>olpc_bat->dev, _bat_eeprom);
>   if (ret)
> - goto eeprom_failed;
> + return ret;
>  
> - ret = device_create_file(_bat->dev, _bat_error);
> + ret = device_create_file(>olpc_bat->dev, _bat_error);
>   if (ret)
>   goto error_failed;
>  
>   if (olpc_ec_wakeup_available()) {
> - device_set_wakeup_capable(_ac->dev, true);
> - device_set_wakeup_capable(_bat->dev, true);
> + device_set_wakeup_capable(>olpc_ac->dev, true);
> + device_set_wakeup_capable(>olpc_bat->dev, true);
>   }
>  
>   return 0;
>  
>  error_failed:
> - device_remove_bin_file(_bat->dev, _bat_eeprom);
> -eeprom_failed:
> - power_supply_unregister(olpc_bat);
> -battery_failed:
> - power_supply_unregister(olpc_ac);
> + device_remove_bin_file(>olpc_bat->dev, _bat_eeprom);
>   return ret;
>  }

...but you are changing error handling here, which is not mentioned in
the changelog, and I'm nut sure you got it right.

Are you sure?

>  static int olpc_battery_remove(struct platform_device *pdev)
>  {
> - device_remove_file(_bat->dev, _bat_error);
> - device_remove_bin_file(_bat->dev, _bat_eeprom);
> - power_supply_unregister(olpc_bat);
> - power_supply_unregister(olpc_ac);
> + struct olpc_battery_data *data = platform_get_drvdata(pdev);
> +
> + device_remove_file(>olpc_bat->dev, _bat_error);
> + device_remove_bin_file(>olpc_bat->dev, _bat_eeprom);
>   return 0;
>  }

Here too.
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 12/15] power: supply: olpc_battery: Use DT to get battery version

[Pavel Machek]

On Wed 2018-10-10 19:22:57, Lubomir Rintel wrote:
> Avoid using the x86 OLPC platform specific call to get the board
> version. It won't work on FDT-based ARM MMP2 platform.
> 
> Signed-off-by: Lubomir Rintel 
> Reviewed-by: Andy Shevchenko 

Acked-by: Pavel Machek 

AFAICT, this should go earlier in the series; first, add support in
the code, then switch to new name in DTS.
Pavel

> ---
>  drivers/power/supply/olpc_battery.c | 10 +++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/power/supply/olpc_battery.c 
> b/drivers/power/supply/olpc_battery.c
> index 5a97e42a3547..540d44bf536f 100644
> --- a/drivers/power/supply/olpc_battery.c
> +++ b/drivers/power/supply/olpc_battery.c
> @@ -19,6 +19,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  
> @@ -622,11 +623,13 @@ static int olpc_battery_probe(struct platform_device 
> *pdev)
>   olpc_ac = power_supply_register(>dev, _ac_desc, NULL);
>   if (IS_ERR(olpc_ac))
>   return PTR_ERR(olpc_ac);
> -
> - if (olpc_board_at_least(olpc_board_pre(0xd0))) { /* XO-1.5 */
> + if (of_property_match_string(pdev->dev.of_node, "compatible",
> + "olpc,xo1.5-battery") >= 0) {
> + /* XO-1.5 */
>   olpc_bat_desc.properties = olpc_xo15_bat_props;
>   olpc_bat_desc.num_properties = ARRAY_SIZE(olpc_xo15_bat_props);
> - } else { /* XO-1 */
> + } else {
> + /* XO-1 */
>   olpc_bat_desc.properties = olpc_xo1_bat_props;
>   olpc_bat_desc.num_properties = ARRAY_SIZE(olpc_xo1_bat_props);
>   }
> @@ -672,6 +675,7 @@ static int olpc_battery_remove(struct platform_device 
> *pdev)
>  
>  static const struct of_device_id olpc_battery_ids[] = {
>   { .compatible = "olpc,xo1-battery" },
> + { .compatible = "olpc,xo1.5-battery" },
>   {}
>  };
>  MODULE_DEVICE_TABLE(of, olpc_battery_ids);

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 11/15] x86, olpc: Use a correct version when making up a battery node

[Pavel Machek]

On Wed 2018-10-10 19:22:56, Lubomir Rintel wrote:
> The XO-1 and XO-1.5 batteries apparently differ in an ability to report
> ambient temperature. Add a different compatible string to the 1.5
> battery.
> 
> Signed-off-by: Lubomir Rintel 

Acked-by: Pavel Machek 

> ---
>  arch/x86/platform/olpc/olpc_dt.c | 59 +++-
>  1 file changed, 42 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/x86/platform/olpc/olpc_dt.c 
> b/arch/x86/platform/olpc/olpc_dt.c
> index d6ee92986920..6e54e116b0c5 100644
> --- a/arch/x86/platform/olpc/olpc_dt.c
> +++ b/arch/x86/platform/olpc/olpc_dt.c
> @@ -218,10 +218,28 @@ static u32 __init olpc_dt_get_board_revision(void)
>   return be32_to_cpu(rev);
>  }
>  
> -void __init olpc_dt_fixup(void)
> +int olpc_dt_compatible_match(phandle node, const char *compat)
>  {
> - int r;
>   char buf[64];
> + int plen;
> + char *p;
> + int len;
> +
> + plen = olpc_dt_getproperty(node, "compatible", buf, sizeof(buf));
> + if (plen <= 0)
> + return 0;
> +
> + len = strlen(compat);
> + for (p = buf; p < buf + plen; p += strlen(p) + 1) {
> + if (strcmp(p, compat) == 0)
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +void __init olpc_dt_fixup(void)
> +{
>   phandle node;
>   u32 board_rev;
>  
> @@ -229,32 +247,33 @@ void __init olpc_dt_fixup(void)
>   if (!node)
>   return;
>  
> - /*
> -  * If the battery node has a compatible property, we are running a new
> -  * enough firmware and don't have fixups to make.
> -  */
> - r = olpc_dt_getproperty(node, "compatible", buf, sizeof(buf));
> - if (r > 0)
> - return;
> -
> - pr_info("PROM DT: Old firmware detected, applying fixes\n");
> -
> - /* Add olpc,xo1-battery compatible marker to battery node */
> - olpc_dt_interpret("\" /battery@0\" find-device"
> - " \" olpc,xo1-battery\" +compatible"
> - " device-end");
> -
>   board_rev = olpc_dt_get_board_revision();
>   if (!board_rev)
>   return;
>  
>   if (board_rev >= olpc_board_pre(0xd0)) {
> + if (olpc_dt_compatible_match(node, "olpc,xo1.5-battery"))
> + return;
> +
> + /* Add olpc,xo1.5-battery compatible marker to battery node */
> + olpc_dt_interpret("\" /battery@0\" find-device"
> + " \" olpc,xo1.5-battery\" +compatible"
> + " device-end");
> +
> + /* We're running a very old firmware if this is missing. */
> + if (olpc_dt_compatible_match(node, "olpc,xo1-battery"))
> + return;
> +
>   /* XO-1.5: add dcon device */
>   olpc_dt_interpret("\" /pci/display@1\" find-device"
>   " new-device"
>   " \" dcon\" device-name \" olpc,xo1-dcon\" +compatible"
>   " finish-device device-end");
>   } else {
> + /* We're running a very old firmware if this is missing. */
> + if (olpc_dt_compatible_match(node, "olpc,xo1-battery"))
> + return;
> +
>   /* XO-1: add dcon device, mark RTC as olpc,xo1-rtc */
>   olpc_dt_interpret("\" /pci/display@1,1\" find-device"
>   " new-device"
> @@ -264,6 +283,11 @@ void __init olpc_dt_fixup(void)
>   " \" olpc,xo1-rtc\" +compatible"
>   " device-end");
>   }
> +
> + /* Add olpc,xo1-battery compatible marker to battery node */
> + olpc_dt_interpret("\" /battery@0\" find-device"
> + " \" olpc,xo1-battery\" +compatible"
> + " device-end");
>  }
>  
>  void __init olpc_dt_build_devicetree(void)
> @@ -289,6 +313,7 @@ void __init olpc_dt_build_devicetree(void)
>  /* A list of DT node/bus matches that we want to expose as platform devices 
> */
>  static struct of_device_id __initdata of_ids[] = {
>   { .compatible = "olpc,xo1-battery" },
> + { .compatible = "olpc,xo1.5-battery" },
>   { .compatible = "olpc,xo1-dcon" },
>   { .compatible = "olpc,xo1-rtc" },
>   {},

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 10/15] dt-bindings: olpc_battery: Add XO-1.5 battery

[Pavel Machek]

On Wed 2018-10-10 19:22:55, Lubomir Rintel wrote:
> The XO-1 and XO-1.5 batteries apparently differ in an ability to report
> ambient temperature.
> 
> Signed-off-by: Lubomir Rintel 
> Reviewed-by: Rob Herring 

Acked-by: Pavel Machek 

Rob, can you apply?

Thanks,
Pavel
> ---
>  Documentation/devicetree/bindings/power/supply/olpc_battery.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/olpc_battery.txt 
> b/Documentation/devicetree/bindings/power/supply/olpc_battery.txt
> index c8901b3992d9..8d87d6b35a98 100644
> --- a/Documentation/devicetree/bindings/power/supply/olpc_battery.txt
> +++ b/Documentation/devicetree/bindings/power/supply/olpc_battery.txt
> @@ -2,4 +2,4 @@ OLPC battery
>  
>  
>  Required properties:
> -  - compatible : "olpc,xo1-battery"
> +  - compatible : "olpc,xo1-battery" or "olpc,xo1.5-battery"

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 08/15] Platform: OLPC: Move EC-specific functionality out from x86

[Pavel Machek]

On Wed 2018-10-10 19:22:53, Lubomir Rintel wrote:
> It is actually plaform independent. Move it to the olpc-ec driver from
> the X86 OLPC platform, so that it could be used by the ARM based laptops
> too.
> 
> Signed-off-by: Lubomir Rintel 

Acked-by: Pavel Machek 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 07/15] Platform: OLPC: Avoid a warning if the EC didn't register yet

[Pavel Machek]

On Wed 2018-10-10 19:22:52, Lubomir Rintel wrote:
> Just return ENODEV, so that whoever attempted to use the EC call can
> defer their work.
> 
> Signed-off-by: Lubomir Rintel 

Acked-by: Pavel Machek 

> ---
>  drivers/platform/olpc/olpc-ec.c | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/olpc/olpc-ec.c b/drivers/platform/olpc/olpc-ec.c
> index 35a21c66cd0d..342f5bb7f7a8 100644
> --- a/drivers/platform/olpc/olpc-ec.c
> +++ b/drivers/platform/olpc/olpc-ec.c
> @@ -116,8 +116,11 @@ int olpc_ec_cmd(u8 cmd, u8 *inbuf, size_t inlen, u8 
> *outbuf, size_t outlen)
>   struct olpc_ec_priv *ec = ec_priv;
>   struct ec_cmd_desc desc;
>  
> - /* Ensure a driver and ec hook have been registered */
> - if (WARN_ON(!ec_driver || !ec_driver->ec_cmd))
> + /* Driver not yet registered. */
> + if (!ec_driver)
> + return -ENODEV;
> +
> + if (WARN_ON(!ec_driver->ec_cmd))
>   return -ENODEV;
>  
>   if (!ec)

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 05/15] Platform: OLPC: Move OLPC config symbol out of x86 tree

[Pavel Machek]

On Wed 2018-10-10 19:22:50, Lubomir Rintel wrote:
> There are ARM OLPC machines that use mostly the same drivers, including
> EC infrastructure, DCON and Battery.
> 
> While at that, fix Kconfig to allow building this as a module.
> 
> Signed-off-by: Lubomir Rintel 

Acked-by: Pavel Machek 

> ---
>  arch/x86/Kconfig  | 11 ---
>  drivers/input/mouse/Kconfig   |  2 +-
>  drivers/platform/Kconfig  |  2 ++
>  drivers/platform/olpc/Kconfig | 11 +++
>  drivers/staging/olpc_dcon/Kconfig |  2 +-
>  5 files changed, 15 insertions(+), 13 deletions(-)
>  create mode 100644 drivers/platform/olpc/Kconfig
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 1a0be022f91d..be6af341a718 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -2729,17 +2729,6 @@ config SCx200HR_TIMER
> processor goes idle (as is done by the scheduler).  The
> other workaround is idle=poll boot option.
>  
> -config OLPC
> - bool "One Laptop Per Child support"
> - depends on !X86_PAE
> - select GPIOLIB
> - select OF
> - select OF_PROMTREE
> - select IRQ_DOMAIN
> - ---help---
> -   Add support for detecting the unique features of the OLPC
> -   XO hardware.
> -
>  config OLPC_XO1_PM
>   bool "OLPC XO-1 Power Management"
>   depends on OLPC && MFD_CS5535 && PM_SLEEP
> diff --git a/drivers/input/mouse/Kconfig b/drivers/input/mouse/Kconfig
> index 566a1e3aa504..58034892a4df 100644
> --- a/drivers/input/mouse/Kconfig
> +++ b/drivers/input/mouse/Kconfig
> @@ -165,7 +165,7 @@ config MOUSE_PS2_TOUCHKIT
>  
>  config MOUSE_PS2_OLPC
>   bool "OLPC PS/2 mouse protocol extension"
> - depends on MOUSE_PS2 && OLPC
> + depends on MOUSE_PS2 && X86 && OLPC
>   help
> Say Y here if you have an OLPC XO-1 laptop (with built-in
> PS/2 touchpad/tablet device).  The manufacturer calls the
> diff --git a/drivers/platform/Kconfig b/drivers/platform/Kconfig
> index d4c2e424a700..4313d73d3618 100644
> --- a/drivers/platform/Kconfig
> +++ b/drivers/platform/Kconfig
> @@ -10,3 +10,5 @@ source "drivers/platform/goldfish/Kconfig"
>  source "drivers/platform/chrome/Kconfig"
>  
>  source "drivers/platform/mellanox/Kconfig"
> +
> +source "drivers/platform/olpc/Kconfig"
> diff --git a/drivers/platform/olpc/Kconfig b/drivers/platform/olpc/Kconfig
> new file mode 100644
> index ..7b736c9e67ac
> --- /dev/null
> +++ b/drivers/platform/olpc/Kconfig
> @@ -0,0 +1,11 @@
> +config OLPC
> + tristate "One Laptop Per Child support"
> + depends on X86 || ARM || COMPILE_TEST
> + depends on !X86_PAE
> + select GPIOLIB
> + select OF
> + select OF_PROMTREE if X86
> + select IRQ_DOMAIN
> + help
> +   Add support for detecting the unique features of the OLPC
> +   XO hardware.
> diff --git a/drivers/staging/olpc_dcon/Kconfig 
> b/drivers/staging/olpc_dcon/Kconfig
> index c91a56f77bcb..07f9f1de8667 100644
> --- a/drivers/staging/olpc_dcon/Kconfig
> +++ b/drivers/staging/olpc_dcon/Kconfig
> @@ -1,6 +1,6 @@
>  config FB_OLPC_DCON
>   tristate "One Laptop Per Child Display CONtroller support"
> - depends on OLPC && FB
> + depends on X86 && OLPC && FB
>   depends on I2C
>   depends on (GPIO_CS5535 || GPIO_CS5535=n)
>   select BACKLIGHT_CLASS_DEVICE

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 04/15] Platform: OLPC: Remove an unused include

[Pavel Machek]

On Wed 2018-10-10 19:22:49, Lubomir Rintel wrote:
> Also, the header is x86 specific, while there are non-x86 OLPC machines.
> 
> Signed-off-by: Lubomir Rintel 

Acked-by: Pavel Machek 

> ---
>  drivers/platform/olpc/olpc-ec.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/platform/olpc/olpc-ec.c b/drivers/platform/olpc/olpc-ec.c
> index f99b183d5296..35a21c66cd0d 100644
> --- a/drivers/platform/olpc/olpc-ec.c
> +++ b/drivers/platform/olpc/olpc-ec.c
> @@ -15,7 +15,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  
>  struct ec_cmd_desc {
>   u8 cmd;

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 03/15] dt-bindings: olpc,xo1.75-ec: Add OLPC XO-1.75 EC bindings

[Pavel Machek]

On Wed 2018-10-10 19:22:48, Lubomir Rintel wrote:
> The OLPC XO-1.75 Embedded Controller is a SPI master that uses extra
> signals for handshaking. It needs to know when is the slave (Linux)
> side's TX FIFO ready for transfer (the ready-gpio signal on the SPI
> controller node) and when does it wish to respond with a command (the
> cmd-gpio property).
> 
> Signed-off-by: Lubomir Rintel 

Acked-by: Pavel Machek 


> ---
>  .../bindings/misc/olpc,xo1.75-ec.txt  | 24 +++
>  1 file changed, 24 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/misc/olpc,xo1.75-ec.txt
> 
> diff --git a/Documentation/devicetree/bindings/misc/olpc,xo1.75-ec.txt 
> b/Documentation/devicetree/bindings/misc/olpc,xo1.75-ec.txt
> new file mode 100644
> index ..14385fee65d2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/olpc,xo1.75-ec.txt
> @@ -0,0 +1,24 @@
> +OLPC XO-1.75 Embedded Controller
> +
> +Required properties:
> +- compatible: Should be "olpc,xo1.75-ec".
> +- cmd-gpio: gpio specifier of the CMD pin
> +
> +The embedded controller requires the SPI controller driver to signal 
> readiness
> +to receive a transfer (that is, when TX FIFO contains the response data) by
> +strobing the ACK pin with the ready signal. See the "ready-gpio" property of 
> the
> +SSP binding as documented in:
> +.
> +
> +Example:
> +  {
> + spi-slave;
> + status = "okay";
> + ready-gpio = < 125 GPIO_ACTIVE_HIGH>;
> +
> + slave {
> + compatible = "olpc,xo1.75-ec";
> + spi-cpha;
> + cmd-gpio = < 155 GPIO_ACTIVE_HIGH>;
> + };
> + };

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 02/15] Revert "platform/olpc: Make ec explicitly non-modular"

[Pavel Machek]

On Wed 2018-10-10 19:22:47, Lubomir Rintel wrote:
> It doesn't make sense to always have this built-in, e.g. on ARM
> multiplatform kernels. A better way to address the problem the original
> commit aimed to solve is to fix Kconfig.
> 
> This reverts commit f48d1496b8537d75776478c6942dd87f34d7f270.

This looks ok, but I don't see the Kconfig fix in the series. Is it
needed?
Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 01/15] power: supply: olpc_battery: correct the temperature units

[Pavel Machek]

On Wed 2018-10-10 19:22:46, Lubomir Rintel wrote:
> According to [1] and [2], the temperature values are in tenths of degree
> Celsius. Exposing the Celsius value makes the battery appear on fire:
> 
>   $ upower -i /org/freedesktop/UPower/devices/battery_olpc_battery
>   ...
>   temperature: 236.9 degrees C
> 
> Tested on OLPC XO-1 and OLPC XO-1.75 laptops.
> 
> [1] include/linux/power_supply.h
> [2] Documentation/power/power_supply_class.txt
> 
> Cc: sta...@vger.kernel.org
> Signed-off-by: Lubomir Rintel 

Acked-by: Pavel Machek 

> ---
>  drivers/power/supply/olpc_battery.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/power/supply/olpc_battery.c 
> b/drivers/power/supply/olpc_battery.c
> index 6da79ae14860..5a97e42a3547 100644
> --- a/drivers/power/supply/olpc_battery.c
> +++ b/drivers/power/supply/olpc_battery.c
> @@ -428,14 +428,14 @@ static int olpc_bat_get_property(struct power_supply 
> *psy,
>   if (ret)
>   return ret;
>  
> - val->intval = (s16)be16_to_cpu(ec_word) * 100 / 256;
> + val->intval = (s16)be16_to_cpu(ec_word) * 10 / 256;
>   break;
>   case POWER_SUPPLY_PROP_TEMP_AMBIENT:
>   ret = olpc_ec_cmd(EC_AMB_TEMP, NULL, 0, (void *)_word, 2);
>   if (ret)
>   return ret;
>  
> - val->intval = (int)be16_to_cpu(ec_word) * 100 / 256;
> + val->intval = (int)be16_to_cpu(ec_word) * 10 / 256;
>   break;
>   case POWER_SUPPLY_PROP_CHARGE_COUNTER:
>   ret = olpc_ec_cmd(EC_BAT_ACR, NULL, 0, (void *)_word, 2);

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] staging: greybus: light: Don't leak memory for no gain

[Pavel Machek]

Hi!

> On Tue, Jul 25, 2017 at 02:30:31PM +0200, Johan Hovold wrote:
> > [ +CC: Rui and Greg ]
> 
> Thanks Johan. I only got this because of you.

> > >   return ret;
> > >  }
> > 
> > And while it's fine to take this through linux-media, it would still be
> > good to keep the maintainers on CC.
> 
> Sakari, if you could resend the all series to the right lists and
> maintainers for proper review that would be great.
> 
> I did not get 0/2 and 2/2 patches.

0/2 and 2/2 were unrelated to the memory leak, IIRC. Let me google it
for you...

https://www.mail-archive.com/linux-media@vger.kernel.org/msg115840.html

This is memory leak and the driver is in staging. Acked-by or fixing
it yourself would be appropriate response, asking for resending of the
series... not quite so.

Best regards,

Pavel

> > On Tue, Jul 18, 2017 at 09:41:06PM +0300, Sakari Ailus wrote:
> > > Memory for struct v4l2_flash_config is allocated in
> > > gb_lights_light_v4l2_register() for no gain and yet the allocated memory 
> > > is
> > > leaked; the struct isn't used outside the function. Fix this.
> > > 
> > > Signed-off-by: Sakari Ailus 
> > > ---
> > >  drivers/staging/greybus/light.c | 17 ++---
> > >  1 file changed, 6 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/staging/greybus/light.c 
> > > b/drivers/staging/greybus/light.c
> > > index 129ceed39829..b25c117ec41a 100644
> > > --- a/drivers/staging/greybus/light.c
> > > +++ b/drivers/staging/greybus/light.c
> > > @@ -534,25 +534,21 @@ static int gb_lights_light_v4l2_register(struct 
> > > gb_light *light)
> > >  {
> > >   struct gb_connection *connection = get_conn_from_light(light);
> > >   struct device *dev = >bundle->dev;
> > > - struct v4l2_flash_config *sd_cfg;
> > > + struct v4l2_flash_config sd_cfg = { 0 };
> > >   struct led_classdev_flash *fled;
> > >   struct led_classdev *iled = NULL;
> > >   struct gb_channel *channel_torch, *channel_ind, *channel_flash;
> > >   int ret = 0;
> > >  
> > > - sd_cfg = kcalloc(1, sizeof(*sd_cfg), GFP_KERNEL);
> > > - if (!sd_cfg)
> > > - return -ENOMEM;
> > > -
> > >   channel_torch = get_channel_from_mode(light, GB_CHANNEL_MODE_TORCH);
> > >   if (channel_torch)
> > >   __gb_lights_channel_v4l2_config(_torch->intensity_uA,
> > > - _cfg->torch_intensity);
> > > + _cfg.torch_intensity);
> > >  
> > >   channel_ind = get_channel_from_mode(light, GB_CHANNEL_MODE_INDICATOR);
> > >   if (channel_ind) {
> > >   __gb_lights_channel_v4l2_config(_ind->intensity_uA,
> > > - _cfg->indicator_intensity);
> > > + _cfg.indicator_intensity);
> > >   iled = _ind->fled.led_cdev;
> > >   }
> > >  
> > > @@ -561,17 +557,17 @@ static int gb_lights_light_v4l2_register(struct 
> > > gb_light *light)
> > >  
> > >   fled = _flash->fled;
> > >  
> > > - snprintf(sd_cfg->dev_name, sizeof(sd_cfg->dev_name), "%s", light->name);
> > > + snprintf(sd_cfg.dev_name, sizeof(sd_cfg.dev_name), "%s", light->name);
> > >  
> > >   /* Set the possible values to faults, in our case all faults */
> > > - sd_cfg->flash_faults = LED_FAULT_OVER_VOLTAGE | LED_FAULT_TIMEOUT |
> > > + sd_cfg.flash_faults = LED_FAULT_OVER_VOLTAGE | LED_FAULT_TIMEOUT |
> > >   LED_FAULT_OVER_TEMPERATURE | LED_FAULT_SHORT_CIRCUIT |
> > >   LED_FAULT_OVER_CURRENT | LED_FAULT_INDICATOR |
> > >   LED_FAULT_UNDER_VOLTAGE | LED_FAULT_INPUT_VOLTAGE |
> > >   LED_FAULT_LED_OVER_TEMPERATURE;
> > >  
> > >   light->v4l2_flash = v4l2_flash_init(dev, NULL, fled, iled,
> > > - _flash_ops, sd_cfg);
> > > + _flash_ops, _cfg);
> > >   if (IS_ERR_OR_NULL(light->v4l2_flash)) {
> > >   ret = PTR_ERR(light->v4l2_flash);
> > >   goto out_free;
> > > @@ -580,7 +576,6 @@ static int gb_lights_light_v4l2_register(struct 
> > > gb_light *light)
> > >   return ret;
> > >  
> > >  out_free:
> > > - kfree(sd_cfg);
> > 
> > This looks a bit lazy, even if I just noticed that you repurpose this
> > error label (without renaming it) in you second patch.
> > 
> > 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: Shawn Guo: your attetion is needed here Re: [PATCH v8 00/34] i.MX Media Driver

[Pavel Machek]

On Tue 2017-06-20 08:05:05, Fabio Estevam wrote:
> On Tue, Jun 20, 2017 at 5:29 AM, Pavel Machek <pa...@ucw.cz> wrote:
> 
> > Hmm. I changed the subject to grab Shawn's attetion.
> >
> > But his acks should not be needed for forward progress. Yes, it would
> > be good, but he does not react -- so just reorder the series so that
> > dts changes come last, then apply the parts you can apply: driver can
> > go in.
> >
> > And actually... if maintainer does not respond at all, there are ways
> > to deal with that, too...
> 
> Shawn has already applied the dts part of the series and they show up
> in linux-next.

Aha, sorry about the noise. I see videomux parts being merged by
Mauro. Good :-).
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Shawn Guo: your attetion is needed here Re: [PATCH v8 00/34] i.MX Media Driver

[Pavel Machek]

Hi!

> >> But as Pavel pointed out, in fact we are missing many
> >> Acks still, for all of the dts source changes (patches
> >> 4-14), as well as really everything else (imx-media staging
> >> driver patches).
> > 
> > No Acks needed for the staging part. It's staging, so not held
> > to the same standards as non-staging parts. That doesn't mean
> > Acks aren't welcome, of course.
> 
> Acks are wanted for particular i.MX DTS changes including device
> tree binding descriptions.
> 
> Shawn, please bless the series.

Hmm. I changed the subject to grab Shawn's attetion.

But his acks should not be needed for forward progress. Yes, it would
be good, but he does not react -- so just reorder the series so that
dts changes come last, then apply the parts you can apply: driver can
go in.

And actually... if maintainer does not respond at all, there are ways
to deal with that, too...

Best regards,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v8 00/34] i.MX Media Driver

[Pavel Machek]

Hi!

> >> Other than that everything is ready AFAICT.
> >>
> > 
> > But as Pavel pointed out, in fact we are missing many
> > Acks still, for all of the dts source changes (patches
> > 4-14), as well as really everything else (imx-media staging
> > driver patches).
> 
> No Acks needed for the staging part. It's staging, so not held
> to the same standards as non-staging parts. That doesn't mean
> Acks aren't welcome, of course.
> 
> You don't need Greg's Ack for staging/media either, patches there
> go in via us (generally at least) and we handle those, not Greg.

Ok, good.

Can you just apply the patches? This is staging, they can be
reviewed/fixed there -- as expected for staging. They are already way
beyond staging quality, we don't want to get to "series v17"
here, and repeatedly sending them over the email does not really do
them any good.

Thanks,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v8 14/34] ARM: dts: imx6-sabreauto: add the ADV7180 video decoder

[Pavel Machek]

Hi!

> >>>Steve,
> >>>
> >>>You need to remove the fim node now that you've moved this to V4L2 
> >>>controls.
> >>>
> >>
> >>Yep, I caught this just after sending the v8 patchset. I'll send
> >>a v9 of this patch.
> >
> >This needs ack from devicetree people, then it can be merged. Can you
> >be a bit more forceful getting the ack?
> 
> OK, I need an Ack please, he said, in a forceful way. :)

I'd tune the force up a tiny bit more. This is not FreeBSD ;-). You
can read some emails from Linus for inspiration. Or drink few beers
and look at Al Viro's emails.

> In fact Ack's are needed for all the changes to dts sources,
> patches 4-14.

Actually, are they? Those should not need acks from device tree
people, just acks from ARM people, and those are easier to get... in
fact they should not need any acks, you should just send them to arm
people and get them merged.

1-4 is just a documentation, and you have acks there. (Except 2?)
That's ready to be merged, probably via the media tree? Just make it
clear you want it merged.

15,16 should be ready to. Media tree, too, I guess.

drivers/staging is greg. Advantage is these do _not_ need to go after
the dts changes. It is a driver. Actually I'd normally add dts support
after the driver. So you can push it now.

> >I don't think it makes sense to resubmit v9 before that. This is not a
> >rocket science.
> 
> I guess that makes sense, I'll wait for Ack's from all these patches
> before submitting the entire patchset as v9.

You may want to split the series, according to mainainters, or just
ask the maintainers to merge the relevant parts. I believe most of it
can be pushed now. 

Good luck,
Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v8 14/34] ARM: dts: imx6-sabreauto: add the ADV7180 video decoder

[Pavel Machek]

On Thu 2017-06-08 13:36:12, Steve Longerbeam wrote:
> 
> 
> On 06/08/2017 01:25 PM, Tim Harvey wrote:
> >
> >
> >Steve,
> >
> >You need to remove the fim node now that you've moved this to V4L2 controls.
> >
> 
> Yep, I caught this just after sending the v8 patchset. I'll send
> a v9 of this patch.

This needs ack from devicetree people, then it can be merged. Can you
be a bit more forceful getting the ack?

I don't think it makes sense to resubmit v9 before that. This is not a
rocket science.

Thanks,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver

[Pavel Machek]

Hi!

> > > According to the docs V4L2_CID_EXPOSURE_ABSOLUTE is in 100 usec units.
> > > 
> > >  OTOH V4L2_CID_EXPOSURE has no defined unit, so it's a better fit IMO.
> > > >Way more drivers appear to be using EXPOSURE than EXPOSURE_ABSOLUTE, too.
> > > 
> > > Done, switched to V4L2_CID_EXPOSURE. It's true, this control is not
> > > taking 100 usec units, so unit-less is better.
> > 
> > Thanks. If you know the units, it would be of course better to use
> > right units...
> 
> Steve: what's the unit in this case? Is it lines or something else?
> 
> Pavel: we do need to make sure the user space will be able to know the unit,
> too. It's rather a case with a number of controls: the unit is known but
> there's no API to convey it to the user.
> 
> The exposure is a bit special, too: granularity matters a lot on small
> values. On most other controls it does not.

Yeah. Basically problem with exposure is that the control is
logarithmic; by using linear scale we got too much resolution at long
times and too little resolution at short times.

(Plus, 100 usec ... n900 can do times _way_ shorter than that.)

Anyway, even u32 gives us enough range, but I so the linear/log
confusion does not matter. But it would be nicer if values were in 10
usec or usec, not 100 usec... 

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver

[Pavel Machek]

Hi!

> >>>+  /* Auto/manual exposure */
> >>>+  ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops,
> >>>+   V4L2_CID_EXPOSURE_AUTO,
> >>>+   V4L2_EXPOSURE_MANUAL, 0,
> >>>+   V4L2_EXPOSURE_AUTO);
> >>>+  ctrls->exposure = v4l2_ctrl_new_std(hdl, ops,
> >>>+  V4L2_CID_EXPOSURE_ABSOLUTE,
> >>>+  0, 65535, 1, 0);
> >>
> >>Is exposure_absolute supposed to be in microseconds...?
> >
> >Yes.
> 
> According to the docs V4L2_CID_EXPOSURE_ABSOLUTE is in 100 usec units.
> 
>  OTOH V4L2_CID_EXPOSURE has no defined unit, so it's a better fit IMO.
> >Way more drivers appear to be using EXPOSURE than EXPOSURE_ABSOLUTE, too.
> 
> Done, switched to V4L2_CID_EXPOSURE. It's true, this control is not
> taking 100 usec units, so unit-less is better.

Thanks. If you know the units, it would be of course better to use
right units...

Best regards,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

exposure vs. exposure_absolute was Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver

[Pavel Machek]

Hi!

> > > + /* Auto/manual exposure */
> > > + ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops,
> > > +  V4L2_CID_EXPOSURE_AUTO,
> > > +  V4L2_EXPOSURE_MANUAL, 0,
> > > +  V4L2_EXPOSURE_AUTO);
> > > + ctrls->exposure = v4l2_ctrl_new_std(hdl, ops,
> > > + V4L2_CID_EXPOSURE_ABSOLUTE,
> > > + 0, 65535, 1, 0);
> > 
> > Is exposure_absolute supposed to be in microseconds...?
> 
> Yes. OTOH V4L2_CID_EXPOSURE has no defined unit, so it's a better fit IMO.
> Way more drivers appear to be using EXPOSURE than EXPOSURE_ABSOLUTE, too.
> 
> Ideally we should have only one control for exposure.

No. N-o. No no no. NO! No. N-o. NONONO. No. NooOOO!

Sorry, no.

Userspace needs to know exposure times. It is not so important for a
webcam, but it is mandatory for digital camera. As it gets darker,
autogain wants to scale exposure to cca 1/100 sec, then it wants to
scale gain up to maximum, and only then it wants to continue scaling
exposure. (Threshold will be shorter in "sports" mode, perhaps
1/300sec?)

Plus, we want user to be able to manually set exposure parameters.

So... _this_ driver probably should use V4L2_CID_EXPOSURE. (If the
units are not known). But in general we'd prefer drivers using
V4L2_CID_EXPOSURE_ABSOLUTE. Your car has speedometer calibrated in
km/h or mph, not in "% of max", right?

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7 00/34] i.MX Media Driver

[Pavel Machek]

Hi!

> >If there's a need for this (there should not be, as the controls are exposed
> >to the user space through the sub-device nodes as the other drivers do), the
> >framework APIs need to be extended.
> 
> Right, this gets back to the media framework usability arguments. At least
> myself, Philipp, and Russell feel that automatic inheritance of a configured
> pipeline's controls to a video device adds to the usability.

For the record, usability can be pretty much fixed in v4l-utils... I
have patches that try ioctls on a list of fd's. Now we need a way to
find out which /dev/video* files belong to single camera. I believe
kernel already has required APIs, we just need to apply v4l-utils
patch to use them...

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver

[Pavel Machek]

Hi!

> +/* min/typical/max system clock (xclk) frequencies */
> +#define OV5640_XCLK_MIN  600
> +#define OV5640_XCLK_MAX 2400
> +
> +/*
> + * FIXME: there is no subdev API to set the MIPI CSI-2
> + * virtual channel yet, so this is hardcoded for now.
> + */
> +#define OV5640_MIPI_VC   1

Can the FIXME be fixed?

> +/*
> + * image size under 1280 * 960 are SUBSAMPLING

-> Image

> + * image size upper 1280 * 960 are SCALING

above?

> +/*
> + * FIXME: all of these register tables are likely filled with
> + * entries that set the register to their power-on default values,
> + * and which are otherwise not touched by this driver. Those entries
> + * should be identified and removed to speed register load time
> + * over i2c.
> + */

load->loading? Can the FIXME be fixed?

> + /* Auto/manual exposure */
> + ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops,
> +  V4L2_CID_EXPOSURE_AUTO,
> +  V4L2_EXPOSURE_MANUAL, 0,
> +  V4L2_EXPOSURE_AUTO);
> + ctrls->exposure = v4l2_ctrl_new_std(hdl, ops,
> + V4L2_CID_EXPOSURE_ABSOLUTE,
> + 0, 65535, 1, 0);

Is exposure_absolute supposed to be in microseconds...?


> + /* Auto/manual gain */
> + ctrls->auto_gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_AUTOGAIN,
> +  0, 1, 1, 1);
> + ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN,
> + 0, 1023, 1, 0);
> +
> + ctrls->saturation = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_SATURATION,
> +   0, 255, 1, 64);
> + ctrls->hue = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HUE,
> +0, 359, 1, 0);
> + ctrls->contrast = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_CONTRAST,
> + 0, 255, 1, 0);
> + ctrls->test_pattern =
> + v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_TEST_PATTERN,
> +  ARRAY_SIZE(test_pattern_menu) - 1,
> +  0, 0, test_pattern_menu);
> +

It is good to see sensor that has autogain/etc. I'm emulating them in
v4l-utils, and hardware that supports it is a good argument. 

Best regards,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 17/39] platform: add video-multiplexer subdevice driver

[Pavel Machek]

Hi!

> That self-referencing mux-controls property looks a bit superfluous:
> 
>   mux: video-multiplexer {
>   mux-controls = <>;
>   };
> 
> Other than that, I'm completely fine with splitting the compatible into
> something like video-mux-gpio and video-mux-mmio and reusing the
> mux-gpios property for video-mux-gpio.

Agreed, I overseen that.

> > You should be able to use code in drivers/mux as a library...
> 
> This is a good idea in principle, but this requires some rework of the
> mux subsystem, and that subsystem hasn't even landed yet. For now I'd
> like to focus on getting the DT bindings right.
> 
> I'd honestly prefer to not add this rework as a requirement for the i.MX
> media drivers to get into staging.

Hmm. staging/ normally accepts code with bigger design problems than
that.

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 17/39] platform: add video-multiplexer subdevice driver

[Pavel Machek]

Hi!

> > The MUX framework is already in linux-next. Could you use that instead of
> > adding new driver + bindings that are not compliant with the MUX framework?
> > I don't think it'd be much of a change in terms of code, using the MUX
> > framework appears quite simple.
> 
> It is not quite clear to me how to design the DT bindings for this. Just
> splitting the video-multiplexer driver from the mux-mmio / mux-gpio
> would make it necessary to keep the video-multiplexer node to describe
> the of-graph bindings. But then we have two different nodes in the DT
> that describe the same hardware:
> 
>   mux: mux {
>   compatible = "mux-gpio";
>   mux-gpios = < 0>, < 1>;
>   #mux-control-cells = <0>;
>   }
> 
>   video-multiplexer {
>   compatible = "video-multiplexer"
>   mux-controls = <>;
> 
>   ports {
>   /* ... */
>   }
>   }
> 
> It would feel more natural to have the ports in the mux node, but then
> how would the video-multiplexer driver be instanciated, and how would it
> get to the of-graph nodes?

Device tree representation and code used to implement the muxing
driver should be pretty independend, no? Yes, one piece of hardware
should have one entry in the device tree, so it should be something
like:


video-multiplexer {
compatible = "video-multiplexer-gpio"   
mux-gpios = < 0>, < 1>;
#mux-control-cells = <0>;

mux-controls = <>;
 
ports {
/* ... */
}
}

You should be able to use code in drivers/mux as a library...

Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 17/39] platform: add video-multiplexer subdevice driver

[Pavel Machek]

On Wed 2017-04-05 13:58:39, Lucas Stach wrote:
> Am Mittwoch, den 05.04.2017, 13:18 +0200 schrieb Pavel Machek:
> > Hi!
> > 
> > > + * video stream multiplexer controlled via gpio or syscon
> > > + *
> > > + * Copyright (C) 2013 Pengutronix, Sascha Hauer <ker...@pengutronix.de>
> > > + * Copyright (C) 2016 Pengutronix, Philipp Zabel <ker...@pengutronix.de>
> > 
> > This is actually quite interesting. Same email address for two
> > people...
> > 
> > Plus, I believe this wants to say that copyright is with Pengutronix,
> > not Sascha and Philipp. In that case you probably want to list
> > copyright and authors separately?
> > 
> Nope, copyright doesn't get transferred to the employer within the rules
> of the German "Urheberrecht", but stays at the original author of the
> code.

Ok, then I guess it should be

Copyright (C) 2013 Sascha Hauer
Work sponsored by Pengutronix, use <ker...@pengutronix.de> as contact address

or something? I know license change is not on the table, but I guess
it is good to get legal stuff right.
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 19/39] media: Add i.MX media core driver

[Pavel Machek]

Hi!

> +References
> +--
> +
> +[1] "i.MX 6Dual/6Quad Applications Processor Reference Manual"
> +[2] "i.MX 6Solo/6DualLite Applications Processor Reference Manual"

[2] is not present in the text above. [1] is there many times. What is
purpose of this section?
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 19/39] media: Add i.MX media core driver

[Pavel Machek]

Hi!


> +https://boundarydevices.com/products/nit6x_5mp

I'd use /product/ in url, as it redirects there.

> +https://boundarydevices.com/product/nit6x_5mp_mipi

..and for consistency.

> +The following example configures a direct conversion pipeline to capture
> +from the OV5640, transmitting on MIPI CSI-2 virtual channel 1. $sensorfmt
> +can be any format supported by the OV5640. $sensordim is the frame
> +dimension part of $sensorfmt (minus the mbus pixel code). $outputfmt can
> +be any format supported by the ipu1_ic_prpenc entity at its output pad:
> +
> +.. code-block:: none
> +
> +   # Setup links
> +   media-ctl -l "'ov5640 1-003c':0 -> 'imx6-mipi-csi2':0[1]"
> +   media-ctl -l "'imx6-mipi-csi2':2 -> 'ipu1_csi1':0[1]"
> +   media-ctl -l "'ipu1_csi1':1 -> 'ipu1_ic_prp':0[1]"
> +   media-ctl -l "'ipu1_ic_prp':1 -> 'ipu1_ic_prpenc':0[1]"
> +   media-ctl -l "'ipu1_ic_prpenc':1 -> 'ipu1_ic_prpenc capture':0[1]"
> +   # Configure pads
> +   media-ctl -V "'ov5640 1-003c':0 [fmt:$sensorfmt field:none]"
> +   media-ctl -V "'imx6-mipi-csi2':2 [fmt:$sensorfmt field:none]"
> +   media-ctl -V "'ipu1_csi1':1 [fmt:AYUV32/$sensordim field:none]"
> +   media-ctl -V "'ipu1_ic_prp':1 [fmt:AYUV32/$sensordim field:none]"
> +   media-ctl -V "'ipu1_ic_prpenc':1 [fmt:$outputfmt field:none]"
> +
> +Streaming can then begin on "ipu1_ic_prpenc capture" node. The v4l2-ctl
> +tool can be used to select any supported YUV or RGB pixelformat on the
> +capture device node.

Nothing for you to fix here, but we should really create some
library-like interface for media-ctl. 
Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 17/39] platform: add video-multiplexer subdevice driver

[Pavel Machek]

Hi!

> + * video stream multiplexer controlled via gpio or syscon
> + *
> + * Copyright (C) 2013 Pengutronix, Sascha Hauer 
> + * Copyright (C) 2016 Pengutronix, Philipp Zabel 

This is actually quite interesting. Same email address for two
people...

Plus, I believe this wants to say that copyright is with Pengutronix,
not Sascha and Philipp. In that case you probably want to list
copyright and authors separately?

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

script to setup pipeline was Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline

[Pavel Machek]

Hi!

> > I do agree with you that MC places a lot of burden on the user to
> > attain a lot of knowledge of the system's architecture.
> 
> Setting up the pipeline is not the hard part. One could write a
> script to do that. 

Can you try to write that script? I believe it would solve big part of
the problem.

> > And my other point is, I think most people who have a need to work with
> > the media framework on a particular platform will likely already be
> > quite familiar with that platform.
> 
> I disagree. The most popular platform device currently is Raspberry PI.
> 
> I doubt that almost all owners of RPi + camera module know anything
> about MC. They just use Raspberry's official driver with just provides
> the V4L2 interface.
> 
> I have a strong opinion that, for hardware like RPi, just the V4L2
> API is enough for more than 90% of the cases.

Maybe V4L2 API is enough for 90% of the users. But I don't believe
that means that we should provide compatibility. V4L2 API is not good
enough for complex devices, and if we can make RPi people fix
userspace... that's a good thing.

> > The media graph for imx6 is fairly self-explanatory in my opinion.
> > Yes that graph has to be generated, but just with a simple 'media-ctl
> > --print-dot', I don't see how that is difficult for the user.
> 
> Again, IMHO, the problem is not how to setup the pipeline, but, instead,
> the need to forward controls to the subdevices.
> 
> To use a camera, the user needs to set up a set of controls for the
> image to make sense (bright, contrast, focus, etc). If the driver
> doesn't forward those controls to the subdevs, an application like
> "camorama" won't actually work for real, as the user won't be able
> to adjust those parameters via GUI.

I believe this can be fixed in libv4l2.
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline

[Pavel Machek]

Hi!

> > Making use of the full potential of the hardware requires using a more
> > expressive interface. 
> 
> That's the core of the problem: most users don't need "full potential
> of the hardware". It is actually worse than that: several boards
> don't allow "full potential" of the SoC capabilities.

Well, in kernel we usually try to support "full hardware" potential.

And we are pretty sure users would like to take still photos, even if
common v4l2 applications can not do it.

> > That's what the kernel interface must provide. If
> > we decide to limit ourselves to a small sub-set of that potential on the
> > level of the kernel interface, we have made a wrong decision. It's as
> > simple as that. This is why the functionality (and which requires taking
> > a lot of policy decisions) belongs to the user space. We cannot have
> > multiple drivers providing multiple kernel interfaces for the same hardware.
> 
> I strongly disagree. Looking only at the hardware capabilities without
> having a solution to provide what the user wants is *wrong*.

Hardware manufacturers already did this kind of research for us. They
don't usually include features noone wants...

> Another case: the cx25821 hardware supports 12 video streams, 
> consuming almost all available bandwidth of an ePCI bus. Each video 
> stream connector can either be configured to be capture or output, in
> runtime. The hardware vendor chose to hardcode the driver to provide
> 8 inputs and 4 outputs. Their decision was based in the fact that
> the driver is already very complex, and it satisfies their customer's 
> needs. The cost/efforts of make the driver to be reconfigured in
> runtime were too high for almost no benefit.

Well, it is okay to provide 'limited' driver -- there's possibility to
fix the driver. But IMO it is not okay to provide 'limited' kernel
interface -- because if you try to fix it, you'll suddenly have
regressions.

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: outreachy

[Pavel Machek]

On Mon 2017-03-20 11:30:08, Greg KH wrote:
> On Mon, Mar 20, 2017 at 11:20:32AM +0100, Pavel Machek wrote:
> > Hi!
> > 
> > > > > Hah!  That's the joy of being a maintainer of a driver in staging.  
> > > > > Even
> > > > > if you filter out outreachy, you are going to get a lot of "basic
> > > > > mistakes" and other type patches cc:ed to you.
> > > > >
> > > > > I strongly suggest, that if you all don't like this type of stuff,
> > > > > either:
> > > > >   - work to get the code out of staging as soon as possible (i.e.
> > > > > send me coding style fixes for everything right now, and then
> > > > > fix up the rest of the stuff.)
> > > > >   - take yourself off the maintainer list for this code.
> > > > >
> > > > > It's your choice, outreachy right now is a lot of patches, but again,
> > > > > it's not going to keep you from getting the "basic" stuff sent to you
> > > > > in ways that is totally wrong.
> > > >
> > > > Could we get these trivial patches off the lkml? Yes, lkml already has
> > > > a lot of traffic, but no, this is not useful :-(.
> > > 
> > > The outreachy instructions say to use the -nol argument to
> > > get_maintainers, which would prevent them from being sent to any mailing
> > > list.  However others thought that all patches should be sent to mailing
> > > lists, and so I haven't enforced anything for people who have omitted
> > > -nol.  However I have tried to remove bcm maintainers from CC lists on
> > > replies and reminded people not to send you patches,
> > 
> > Wonderful :-(.
> > 
> > Can we at least make  those people put the word "outreachy" in the
> > subject so the emails are easier to delete?
> 
> It's easy for you to filter away as-is if you want to to by just looking
> at the cc: list for outreachy right now, don't make a new step for
> people to jump through because you don't want to be bothered.  You only
> have 10 more days of it if you want to just ignore any patch until
> then...

10 more days... I can survive 10 more days. I was afraid we would be
stuck with it "forever".
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: outreachy

[Pavel Machek]

Hi!

> > > Hah!  That's the joy of being a maintainer of a driver in staging.  Even
> > > if you filter out outreachy, you are going to get a lot of "basic
> > > mistakes" and other type patches cc:ed to you.
> > >
> > > I strongly suggest, that if you all don't like this type of stuff,
> > > either:
> > >   - work to get the code out of staging as soon as possible (i.e.
> > > send me coding style fixes for everything right now, and then
> > > fix up the rest of the stuff.)
> > >   - take yourself off the maintainer list for this code.
> > >
> > > It's your choice, outreachy right now is a lot of patches, but again,
> > > it's not going to keep you from getting the "basic" stuff sent to you
> > > in ways that is totally wrong.
> >
> > Could we get these trivial patches off the lkml? Yes, lkml already has
> > a lot of traffic, but no, this is not useful :-(.
> 
> The outreachy instructions say to use the -nol argument to
> get_maintainers, which would prevent them from being sent to any mailing
> list.  However others thought that all patches should be sent to mailing
> lists, and so I haven't enforced anything for people who have omitted
> -nol.  However I have tried to remove bcm maintainers from CC lists on
> replies and reminded people not to send you patches,

Wonderful :-(.

Can we at least make  those people put the word "outreachy" in the
subject so the emails are easier to delete?
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline

[Pavel Machek]

On Fri 2017-03-17 11:42:03, Russell King - ARM Linux wrote:
> On Tue, Mar 14, 2017 at 08:55:36AM +0100, Hans Verkuil wrote:
> > We're all very driver-development-driven, and userspace gets very little
> > attention in general. So before just throwing in the towel we should take
> > a good look at the reasons why there has been little or no development: is
> > it because of fundamental design defects, or because nobody paid attention
> > to it?
> > 
> > I strongly suspect it is the latter.
> > 
> > In addition, I suspect end-users of these complex devices don't really care
> > about a plugin: they want full control and won't typically use generic
> > applications. If they would need support for that, we'd have seen much more
> > interest. The main reason for having a plugin is to simplify testing and
> > if this is going to be used on cheap hobbyist devkits.
> 
> I think you're looking at it with a programmers hat on, not a users hat.
> 
> Are you really telling me that requiring users to 'su' to root, and then
> use media-ctl to manually configure the capture device is what most
> users "want" ?

If you want to help users, right way is to improve userland support. 

> Hasn't the way technology has moved towards graphical interfaces,
> particularly smart phones, taught us that the vast majority of users
> want is intuitive, easy to use interfaces, and not the command line
> with reams of documentation?

How is it relevant to _kernel_ interfaces?
Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: outreachy

[Pavel Machek]

Hi!

> 
> Hah!  That's the joy of being a maintainer of a driver in staging.  Even
> if you filter out outreachy, you are going to get a lot of "basic
> mistakes" and other type patches cc:ed to you.
> 
> I strongly suggest, that if you all don't like this type of stuff,
> either:
>   - work to get the code out of staging as soon as possible (i.e.
> send me coding style fixes for everything right now, and then
> fix up the rest of the stuff.)
>   - take yourself off the maintainer list for this code.
> 
> It's your choice, outreachy right now is a lot of patches, but again,
> it's not going to keep you from getting the "basic" stuff sent to you
> in ways that is totally wrong.

Could we get these trivial patches off the lkml? Yes, lkml already has
a lot of traffic, but no, this is not useful :-(.

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: media / v4l2-mc: wishlist for complex cameras (was Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline)

[Pavel Machek]

Hi!

> > > > mplayer is useful for testing... but that one already works (after you
> > > > setup the pipeline, and configure exposure/gain).
> > > > 
> > > > But thats useful for testing, not really for production. Image will be
> > > > out of focus and with wrong white balance.
> > > > 
> > > > What I would really like is an application to get still photos. For
> > > > taking pictures with manual settings we need
> > > > 
> > > > a) units for controls: user wants to focus on 1m, and take picture
> > > > with ISO200, 1/125 sec. We should also tell him that lens is f/5.6 and
> > > > focal length is 20mm with 5mm chip.
> > > > 
> > > > But... autofocus/autogain would really be good to have. Thus we need:
> > > > 
> > > > b) for each frame, we need exposure settings and focus position at
> > > > time frame was taken. Otherwise autofocus/autogain will be too
> > > > slow. At least focus position is going to be tricky -- either kernel
> > > > would have to compute focus position for us (not trivial) or we'd need
> > > > enough information to compute it in userspace.
> > > > 
> > > > There are more problems: hardware-accelerated preview is not trivial
> > > > to set up (and I'm unsure if it can be done in generic way). Still
> > > > photos application needs to switch resolutions between preview and
> > > > photo capture. Probably hardware-accelerated histograms are needed for
> > > > white balance, auto gain and auto focus, 
> > > > 
> > > > It seems like there's a _lot_ of stuff to be done before we have
> > > > useful support for complex cameras...  
> > > 
> > > Taking still pictures using a hardware-accelerated preview is
> > > a sophisticated use case. I don't know any userspace application
> > > that does that. Ok, several allow taking snapshots, by simply
> > > storing the image of the current frame.  
> > 
> > Well, there are applications that take still pictures. Android has
> > one. Maemo has another. Then there's fcam-dev. Its open source; with
> > modified kernel it is fully usable. I have version that runs on recent
> > nearly-mainline on N900. 
> 
> Hmm... it seems that FCam is specific for N900:
>   http://fcam.garage.maemo.org/
> 
> If so, then we have here just the opposite problem, if want it to be
> used as a generic application, as very likely it requires OMAP3-specific
> graph/subdevs.

Well... there's quick and great version on maemo.org. I do have local
version (still somehow N900-specific), but it no longer uses hardware
histogram/sharpness support. Should be almost generic.

> > So yes, I'd like solution for problems a) and b).

...but it has camera parameters hardcoded (problem a) and slow
(problem b). 

> > Question is if camera without autofocus is usable. I'd say "not
> > really".qv4l2
> 
> That actually depends on the sensor and how focus is adjusted.
> 
> I'm testing right now this camera module for RPi:
>https://www.raspberrypi.org/products/camera-module-v2/
> 
> I might be wrong, but this sensor doesn't seem to have auto-focus.
> Instead, it seems to use a wide-angle lens. So, except when the
> object is too close, the focus look OK.

Well, cameras without autofocus are somehow usable without
autofocus. But cameras with autofocus don't work too well without one.

> > If we really want to go that way (is not modifying library to access
> > the right files quite easy?), I believe non-confusing option would be
> > to have '/dev/video0 -- omap3 camera for legacy applications' which
> > would include all the controls.
> 
> Yeah, keeping /dev/video0 reserved for generic applications is something
> that could work. Not sure how easy would be to implement it.

Plus advanced applications would just ignore /dev/video0.. and not be confused.

> > > > > > You can get Nokia N900 on aliexpress. If not, they are still 
> > > > > > available
> > > > between people :-)  
> > > 
> > > I have one. Unfortunately, I never had a chance to use it, as the display
> > > stopped working one week after I get it.  
> > 
> > Well, I guess the easiest option is to just get another one :-).
> 
> :-)  Well, I guess very few units of N900 was sold in Brazil. Importing
> one is too expensive, due to taxes.

Try to ask at local mailing list. Those machines were quite common.

> > But otoh -- N900 is quite usable without the screen. 0x tool can
> > be used to boot the kernel, then you can use nfsroot and usb
> > networking. It also has serial port (over strange
> > connector). Connected over ssh over usb network is actually how I do
> > most of the v4l work.
> 
> If you pass me the pointers, I can try it when I have some time.

Ok, I guess I'll do that in private email.

> Anyway, I got myself an ISEE IGEPv2, with the expansion board:
>   https://www.isee.biz/products/igep-processor-boards/igepv2-dm3730
>   https://www.isee.biz/products/igep-expansion-boards/igepv2-expansion
> 
> The expansion board comes with a tvp5150 analog TV demod. So, with
> this device, I can simply connect it to a 

Re: media / v4l2-mc: wishlist for complex cameras (was Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline)

[Pavel Machek]

Hi!

> > Well, I believe first question is: what applications would we want to
> > run on complex devices? Will sending control from video to subdevs
> > actually help?
> 
> I would say: camorama, xawtv3, zbar, google talk, skype. If it runs
> with those, it will likely run with any other application.

I'll take a look when I'm at better internet access.

> > mplayer is useful for testing... but that one already works (after you
> > setup the pipeline, and configure exposure/gain).
> > 
> > But thats useful for testing, not really for production. Image will be
> > out of focus and with wrong white balance.
> > 
> > What I would really like is an application to get still photos. For
> > taking pictures with manual settings we need
> > 
> > a) units for controls: user wants to focus on 1m, and take picture
> > with ISO200, 1/125 sec. We should also tell him that lens is f/5.6 and
> > focal length is 20mm with 5mm chip.
> > 
> > But... autofocus/autogain would really be good to have. Thus we need:
> > 
> > b) for each frame, we need exposure settings and focus position at
> > time frame was taken. Otherwise autofocus/autogain will be too
> > slow. At least focus position is going to be tricky -- either kernel
> > would have to compute focus position for us (not trivial) or we'd need
> > enough information to compute it in userspace.
> > 
> > There are more problems: hardware-accelerated preview is not trivial
> > to set up (and I'm unsure if it can be done in generic way). Still
> > photos application needs to switch resolutions between preview and
> > photo capture. Probably hardware-accelerated histograms are needed for
> > white balance, auto gain and auto focus, 
> > 
> > It seems like there's a _lot_ of stuff to be done before we have
> > useful support for complex cameras...
> 
> Taking still pictures using a hardware-accelerated preview is
> a sophisticated use case. I don't know any userspace application
> that does that. Ok, several allow taking snapshots, by simply
> storing the image of the current frame.

Well, there are applications that take still pictures. Android has
one. Maemo has another. Then there's fcam-dev. Its open source; with
modified kernel it is fully usable. I have version that runs on recent
nearly-mainline on N900. 

So yes, I'd like solution for problems a) and b).

> > (And I'm not sure... when application such as skype is running, is
> > there some way to run autogain/autofocus/autowhitebalance? Is that
> > something we want to support?)
> 
> Autofocus no. Autogain/Autowhite can be done via libv4l, provided that
> it can access the device's controls via /dev/video devnode. Other
> applications may be using some other similar algorithms.
> 
> Ok, they don't use histograms provided by the SoC. So, they do it in
> software, with is slower. Still, it works fine when the light
> conditions don't change too fast.

I guess it is going to work well enough with higher CPU
usage. Question is if camera without autofocus is usable. I'd say "not
really".

> > I believe other question is: will not having same control on main
> > video device and subdevs be confusing? Does it actually help userspace
> > in any way? Yes, we can make controls accessible to old application,
> > but does it make them more useful? 
> 
> Yes. As I said, libv4l (and some apps) have logic inside to adjust
> the image via bright, contrast and white balance controls, using the
> video devnode. They don't talk subdev API. So, if those controls
> aren't exported, they won't be able to provide a good quality image.

Next question is if the libv4l will do the right thing if we just put
all controls to one node. For example on N900 you have exposure/gain
and brightness. But the brightness is applied at preview phase, so it
is "basically useless". You really need to adjust the image using the
exposure/gain.

> > > > In addition, I suspect end-users of these complex devices don't really 
> > > > care
> > > > about a plugin: they want full control and won't typically use generic
> > > > applications. If they would need support for that, we'd have seen much 
> > > > more
> > > > interest. The main reason for having a plugin is to simplify testing and
> > > > if this is going to be used on cheap hobbyist devkits.  
> > > 
> > > What are the needs for a cheap hobbyist devkit owner? Do we currently
> > > satisfy those needs? I'd say that having a functional driver when
> > > compiled without the subdev API, that implements the ioctl's/controls  
> > 
> > Having different interface based on config options... is just
> > weird. What about poor people (like me) trying to develop complex
> > applications?
> 
> Well, that could be done using other mechanisms, like a modprobe
> parameter or by switching the behaviour if a subdev interface is
> opened. I don't see much trouble on allowing accessing a control via
> both interfaces.

If we really want to go that way (is not modifying library to access
the right files quite easy?), I 

media / v4l2-mc: wishlist for complex cameras (was Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline)

[Pavel Machek]

Hi!

> > > Even if they were merged, if we keep the same mean time to develop a
> > > libv4l plugin, that would mean that a plugin for i.MX6 could take 2-3
> > > years to be developed.
> > > 
> > > There's a clear message on it:
> > >   - we shouldn't keep pushing for a solution via libv4l.  
> > 
> > Or:
> > 
> > - userspace plugin development had a very a low priority and
> >   never got the attention it needed.
> 
> The end result is the same: we can't count on it.
> 
> > 
> > I know that's *my* reason. I rarely if ever looked at it. I always assumed
> > Sakari and/or Laurent would look at it. If this reason is also valid for
> > Sakari and Laurent, then it is no wonder nothing has happened in all that
> > time.
> > 
> > We're all very driver-development-driven, and userspace gets very little
> > attention in general. So before just throwing in the towel we should take
> > a good look at the reasons why there has been little or no development: is
> > it because of fundamental design defects, or because nobody paid attention
> > to it?
> 
> No. We should look it the other way: basically, there are patches
> for i.MX6 driver that sends control from videonode to subdevs. 
> 
> If we nack apply it, who will write the userspace plugin? When
> such change will be merged upstream?

Well, I believe first question is: what applications would we want to
run on complex devices? Will sending control from video to subdevs
actually help?

mplayer is useful for testing... but that one already works (after you
setup the pipeline, and configure exposure/gain).

But thats useful for testing, not really for production. Image will be
out of focus and with wrong white balance.

What I would really like is an application to get still photos. For
taking pictures with manual settings we need

a) units for controls: user wants to focus on 1m, and take picture
with ISO200, 1/125 sec. We should also tell him that lens is f/5.6 and
focal length is 20mm with 5mm chip.

But... autofocus/autogain would really be good to have. Thus we need:

b) for each frame, we need exposure settings and focus position at
time frame was taken. Otherwise autofocus/autogain will be too
slow. At least focus position is going to be tricky -- either kernel
would have to compute focus position for us (not trivial) or we'd need
enough information to compute it in userspace.

There are more problems: hardware-accelerated preview is not trivial
to set up (and I'm unsure if it can be done in generic way). Still
photos application needs to switch resolutions between preview and
photo capture. Probably hardware-accelerated histograms are needed for
white balance, auto gain and auto focus, 

It seems like there's a _lot_ of stuff to be done before we have
useful support for complex cameras...

(And I'm not sure... when application such as skype is running, is
there some way to run autogain/autofocus/autowhitebalance? Is that
something we want to support?)

> If we don't have answers to any of the above questions, we should not
> nack it.
> 
> That's said, that doesn't prevent merging a libv4l plugin if/when
> someone can find time/interest to develop it.

I believe other question is: will not having same control on main
video device and subdevs be confusing? Does it actually help userspace
in any way? Yes, we can make controls accessible to old application,
but does it make them more useful? 

> > In addition, I suspect end-users of these complex devices don't really care
> > about a plugin: they want full control and won't typically use generic
> > applications. If they would need support for that, we'd have seen much more
> > interest. The main reason for having a plugin is to simplify testing and
> > if this is going to be used on cheap hobbyist devkits.
> 
> What are the needs for a cheap hobbyist devkit owner? Do we currently
> satisfy those needs? I'd say that having a functional driver when
> compiled without the subdev API, that implements the ioctl's/controls

Having different interface based on config options... is just
weird. What about poor people (like me) trying to develop complex
applications?

> [1] Yet, I might eventually do that for fun, an OMAP3 board with tvp5150
> just arrived here last week. It would be nice to have xawtv3 running on it :-)
> So, if I have a lot of spare time (with is very unlikely), I might eventually 
> do something for it to work.
> 
> > I know it took me a very long time before I had a working omap3.
> 
> My first OMAP3 board with working V4L2 source just arrived last week
> :-)

You can get Nokia N900 on aliexpress. If not, they are still available
between people :-).

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list

Re: [PATCH v5 15/39] [media] v4l2: add a frame interval error event

[Pavel Machek]

On Mon 2017-03-13 10:45:38, Russell King - ARM Linux wrote:
> On Mon, Mar 13, 2017 at 11:02:34AM +0100, Hans Verkuil wrote:
> > On 03/11/2017 07:14 PM, Steve Longerbeam wrote:
> > > The event must be user visible, otherwise the user has no indication
> > > the error, and can't correct it by stream restart.
> > 
> > In that case the driver can detect this and call vb2_queue_error. It's
> > what it is there for.
> > 
> > The event doesn't help you since only this driver has this issue. So nobody
> > will watch this event, unless it is sw specifically written for this SoC.
> > 
> > Much better to call vb2_queue_error to signal a fatal error (which this
> > apparently is) since there are more drivers that do this, and vivid supports
> > triggering this condition as well.
> 
> So today, I can fiddle around with the IMX219 registers to help gain
> an understanding of how this sensor works.  Several of the registers
> (such as the PLL setup [*]) require me to disable streaming on the
> sensor while changing them.
> 
> This is something I've done many times while testing various ideas,
> and is my primary way of figuring out and testing such things.
> 
> Whenever I resume streaming (provided I've let the sensor stop
> streaming at a frame boundary) it resumes as if nothing happened.  If I
> stop the sensor mid-frame, then I get the rolling issue that Steve
> reports, but once the top of the frame becomes aligned with the top of
> the capture, everything then becomes stable again as if nothing happened.
> 
> The side effect of what you're proposing is that when I disable streaming
> at the sensor by poking at its registers, rather than the capture just
> stopping, an error is going to be delivered to gstreamer, and gstreamer
> is going to exit, taking the entire capture process down.
> 
> This severely restricts the ability to be able to develop and test
> sensor drivers.

Well, but kernel should do what is best for production, not what is
best for driver debugging.

And yes, I guess you can have #ifdef or module parameter or something
switching for behaviour you prefer when you are debugging. But for
production, vb2_queue_error() seems to be the right solution.

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline

[Pavel Machek]

Hi!

> > Mid-layer is difficult... there are _hundreds_ of possible
> > pipeline setups. If it should live in kernel or in userspace is a
> > question... but I don't think having it in kernel helps in any way.
> 
> Mid-layer is difficult, because we either need to feed some
> library with knowledge for all kernel drivers or we need to improve
> the MC API to provide more details.
> 
> For example, several drivers used to expose entities via the
> generic MEDIA_ENT_T_DEVNODE to represent entities of different
> types. See, for example, entities 1, 5 and 7 (and others) at:
>
> > https://mchehab.fedorapeople.org/mc-next-gen/igepv2_omap3isp.png

Well... we provide enough information, so that device-specific code
does not have to be in kernel.

There are few types of ENT_T_DEVNODE there. "ISP CCP2" does not really
provide any functionality to the user. It just has to be there,
because the pipeline needs to be connected. "ISP Preview" provides
format conversion. "ISP Resizer" provides rescaling.

I'm not sure if it ever makes sense to use "ISP Preview
output". Normally you take data for display from "ISP Resizer
output". (Would there be some power advantage from that?)

> A device-specific code could either be hardcoding the entity number
> or checking for the entity strings to add some logic to setup
> controls on those "unknown" entities, a generic app won't be able 
> to do anything with them, as it doesn't know what function(s) such
> entity provide.

Generic app should know if it wants RGGB10 data, then it can use "ISP
CCDC output", or if it wants "cooked" data suitable for display, then
it wants to use "ISP Resizer output". Once application knows what
output it wants, there's just one path through the system. So being
able to tell the ENT_T_DEVNODEs apart does not seem to be critical.

OTOH, for useful camera application, different paths are needed at
different phases.
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline

[Pavel Machek]

On Sat 2017-03-11 23:14:56, Russell King - ARM Linux wrote:
> On Sat, Mar 11, 2017 at 08:25:49AM -0300, Mauro Carvalho Chehab wrote:
> > This situation is there since 2009. If I remember well, you tried to write
> > such generic plugin in the past, but never finished it, apparently because
> > it is too complex. Others tried too over the years. 
> > 
> > The last trial was done by Jacek, trying to cover just the exynos4 driver. 
> > Yet, even such limited scope plugin was not good enough, as it was never
> > merged upstream. Currently, there's no such plugins upstream.
> > 
> > If we can't even merge a plugin that solves it for just *one* driver,
> > I have no hope that we'll be able to do it for the generic case.
> 
> This is what really worries me right now about the current proposal for
> iMX6.  What's being proposed is to make the driver exclusively MC-based.
> 
> What that means is that existing applications are _not_ going to work
> until we have some answer for libv4l2, and from what you've said above,
> it seems that this has been attempted multiple times over the last _8_
> years, and each time it's failed.

Yeah. We need a mid-layer between legacy applications and MC
devices. Such layer does not exist in userspace or in kernel.

> Loading the problem onto the user in the hope that the user knows
> enough to properly configure it also doesn't work - who is going to
> educate the user about the various quirks of the hardware they're
> dealing with?

We have docs. Users can write shell scripts. Still, mid-layer would be
nice.

> So, the problem space we have here is absolutely huge, and merely
> having a plugin that activates when you open a /dev/video* node
> really doesn't solve it.
> 
> All in all, I really don't think "lets hope someone writes a v4l2
> plugin to solve it" is ever going to be successful.  I don't even
> see that there will ever be a userspace application that is anything
> more than a representation of the dot graphs that users can use to
> manually configure the capture system with system knowledge.
> 
> I think everyone needs to take a step back and think long and hard
> about this from the system usability perspective - I seriously
> doubt that we will ever see any kind of solution to this if we
> continue to progress with "we'll sort it in userspace some day."

Mid-layer is difficult... there are _hundreds_ of possible
pipeline setups. If it should live in kernel or in userspace is a
question... but I don't think having it in kernel helps in any way.

Best regards,
Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline

[Pavel Machek]

On Sun 2017-03-12 07:37:45, Russell King - ARM Linux wrote:
> On Sat, Mar 11, 2017 at 07:31:18PM -0800, Steve Longerbeam wrote:
> > 
> > 
> > On 03/11/2017 10:59 AM, Russell King - ARM Linux wrote:
> > >On Sat, Mar 11, 2017 at 10:54:55AM -0800, Steve Longerbeam wrote:
> > >>
> > >>
> > >>On 03/11/2017 10:45 AM, Russell King - ARM Linux wrote:
> > >>>I really don't think expecting the user to understand and configure
> > >>>the pipeline is a sane way forward.  Think about it - should the
> > >>>user need to know that, because they have a bayer-only CSI data
> > >>>source, that there is only one path possible, and if they try to
> > >>>configure a different path, then things will just error out?
> > >>>
> > >>>For the case of imx219 connected to iMX6, it really is as simple as
> > >>>"there is only one possible path" and all the complexity of the media
> > >>>interfaces/subdevs is completely unnecessary.  Every other block in
> > >>>the graph is just noise.
> > >>>
> > >>>The fact is that these dot graphs show a complex picture, but reality
> > >>>is somewhat different - there's only relatively few paths available
> > >>>depending on the connected source and the rest of the paths are
> > >>>completely useless.
> > >>>
> > >>
> > >>I totally disagree there. Raw bayer requires passthrough yes, but for
> > >>all other media bus formats on a mipi csi-2 bus, and all other media
> > >>bus formats on 8-bit parallel buses, the conersion pipelines can be
> > >>used for scaling, CSC, rotation, and motion-compensated de-interlacing.
> > >
> > >... which only makes sense _if_ your source can produce those formats.
> > >We don't actually disagree on that.
> > 
> > ...and there are lots of those sources! You should try getting out of
> > your imx219 shell some time, and have a look around! :)
> 
> If you think that, you are insulting me.  I've been thinking about this
> from the "big picture" point of view.  If you think I'm only thinking
> about this from only the bayer point of view, you're wrong.

Can you stop that insults nonsense?

> Given what Mauro has said, I'm convinced that the media controller stuff
> is a complete failure for usability, and adding further drivers using it
> is a mistake.

Hmm. But you did not present any alternative. Seems some hardware is
simply complex. So either we don't add complex drivers (_that_ would
be a mistake), or some userspace solution will need to be
done. Shell-script running media-ctl does not seem that hard.

> So, tell me how the user can possibly use iMX6 video capture without
> resorting to opening up a terminal and using media-ctl to manually
> configure the pipeline.  How is the user going to control the source
> device without using media-ctl to find the subdev node, and then using
> v4l2-ctl on it.  How is the user supposed to know which /dev/video*
> node they should be opening with their capture application?

Complex hardware sometimes requires userspace configuration. Running a
shell script on startup does not seem that hard.

And maybe we could do some kind of default setup in kernel, but that
does not really solve the problem.

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline

[Pavel Machek]

Hi!

> > > The rationale is that we should support the simplest use cases first.
> > > 
> > > In the case of the first MC-based driver (and several subsequent
> > > ones), the simplest use case required MC, as it was meant to suport
> > > a custom-made sophisticated application that required fine control
> > > on each component of the pipeline and to allow their advanced
> > > proprietary AAA userspace-based algorithms to work.  
> > 
> > The first MC based driver (omap3isp) supports what the hardware can do, it
> > does not support applications as such.
> 
> All media drivers support a subset of what the hardware can do. The
> question is if such subset covers the use cases or not.
> 
> The current MC-based drivers (except for uvc) took a patch to offer a
> more advanced API, to allow direct control to each IP module, as it was
> said, by the time we merged the OMAP3 driver, that, for the N9/N900 camera
> to work, it was mandatory to access the pipeline's individual components.
> 
> Such approach require that some userspace software will have knowledge
> about some hardware details, in order to setup pipelines and send controls
> to the right components. That makes really hard to have a generic user
> friendly application to use such devices.

Well. Even if you propagate controls to the right components, there's
still a lot application needs to know about the camera
subsystem. Focus lengths, for example. Speed of the focus
coil. Whether or not aperture controls are available. If they are not,
what is the fixed aperture.

Dunno. Knowing what control to apply on what subdevice does not look
like the hardest part of camera driver. Yes, it would be a tiny bit
easier if I would have just one device to deal with, but fcam-dev
has cca 2 lines of C++ code. 

> In the case of V4L2 controls, when there's no subdev API, the main
> driver (e. g. the driver that creates the /dev/video nodes) sends a
> multicast message to all bound I2C drivers. The driver(s) that need 
> them handle it. When the same control may be implemented on different
> drivers, the main driver sends a unicast message to just one
> driver[1].

Dunno. There's quite common to have two flashes. In that case, will
application control both at the same time?

> There's nothing wrong with this approach: it works, it is simpler,
> it is generic. So, if it covers most use cases, why not allowing it
> for usecases where a finer control is not a requirement?

Because the resulting interface is quite ugly?

> That's why I'm saying that I'm OK on merging any patch that would allow
> setting controls via the /dev/video interface on MC-based drivers when
> compiled without subdev API. I may also consider merging patches allowing

So.. userspace will now have to detect if subdev is available or not,
and access hardware in different ways?

> > The original plan was and continues to be sound, it's just that there have
> > always been too few hands to implement it. :-(
> 
> If there are no people to implement a plan, it doesn't matter how good
> the plan is, it won't work.

If the plan is good, someone will do it.
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline

[Pavel Machek]

Hi!

> > > Ok, perhaps supporting both subdev API and V4L2 API at the same
> > > time doesn't make much sense. We could disable one in favor of the
> > > other, either at compilation time or at runtime.  
> > 
> > Right. If the subdev API is disabled, then you have to inherit the subdev
> > controls in the bridge driver (how else would you be able to access them?).
> > And that's the usual case.
> > 
> > If you do have the subdev API enabled, AND you use the MC, then the
> > intention clearly is to give userspace full control and inheriting controls
> > no longer makes any sense (and is highly confusing IMHO).
> 
> I tend to agree with that.

Well, having different userspace interface according to config options
is strange. I believe the right solution is to make complex drivers
depend on CONFIG_VIDEO_V4L2_SUBDEV_API...

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline

[Pavel Machek]

Hi!

> >>I tend to agree with that.
> >
> >I agree as well.
> >
> >This is in line with how existing drivers behave, too.
> 
> 
> Well, sounds like there is consensus on this topic. I guess I'll
> go ahead and remove the control inheritance support. I suppose
> having a control appear in two places (subdev and video nodes) can
> be confusing.

I guess that's way to go. It is impossible to change userland APIs
once the patch is merged...
Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 22/39] media: Add userspace header file for i.MX

[Pavel Machek]

Hi!

> diff --git a/include/uapi/media/Kbuild b/include/uapi/media/Kbuild
> index aafaa5a..fa78958 100644
> --- a/include/uapi/media/Kbuild
> +++ b/include/uapi/media/Kbuild
> @@ -1 +1,2 @@
>  # UAPI Header export list
> +header-y += imx.h
> diff --git a/include/uapi/media/imx.h b/include/uapi/media/imx.h
> new file mode 100644
> index 000..f573de4
> --- /dev/null
> +++ b/include/uapi/media/imx.h
> @@ -0,0 +1,21 @@
> +/*
> + * Copyright (c) 2014-2015 Mentor Graphics Inc.
> + *
> + * 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
> + */
> +
> +#ifndef __UAPI_MEDIA_IMX_H__
> +#define __UAPI_MEDIA_IMX_H__
> +
> +enum imx_ctrl_id {
> + V4L2_CID_IMX_FIM_ENABLE = (V4L2_CID_USER_IMX_BASE + 0),
> + V4L2_CID_IMX_FIM_NUM,
> + V4L2_CID_IMX_FIM_TOLERANCE_MIN,
> + V4L2_CID_IMX_FIM_TOLERANCE_MAX,
> + V4L2_CID_IMX_FIM_NUM_SKIP,
> +};
> +

Should this #include something so that if userland includes it, it
will not get compile error?

Should there be some documentation of userland API?
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 15/39] [media] v4l2: add a frame interval error event

[Pavel Machek]

On Fri 2017-03-10 10:37:21, Steve Longerbeam wrote:
> Hi Hans,
> 
> On 03/10/2017 04:03 AM, Hans Verkuil wrote:
> >On 10/03/17 05:52, Steve Longerbeam wrote:
> >>Add a new FRAME_INTERVAL_ERROR event to signal that a video capture or
> >>output device has measured an interval between the reception or transmit
> >>completion of two consecutive frames of video that is outside the nominal
> >>frame interval by some tolerance value.
> >
> >Reading back what was said on this I agree with Sakari that this doesn't
> >belong here.
> >
> >Userspace can detect this just as easily (if not easier) with a timeout.
> >
> 
> 
> Unfortunately measuring frame intervals from userland is not accurate
> enough for i.MX6.
> 
> The issue here is that the IPUv3, specifically the CSI unit, can
> permanently lose vertical sync if there are truncated frames sent
> on the bt.656 bus. We have seen a single missing line of video cause
> loss of vertical sync. The only way to correct this is to shutdown
> the IPU capture hardware and restart, which can be accomplished
> simply by restarting streaming from userland.
> 
> There are no other indicators from the sensor about these short
> frame events (believe me, we've exhausted all avenues with the ADV718x).
> And the IPUv3 DMA engine has no status indicators for short frames
> either. So the only way to detect them is by measuring frame intervals.
> 
> The intervals have to be able to resolve a single line of missing video.
> With a PAL video source that requires better than 58 usec accuracy.
> 
> There is too much uncertainty to resolve this at user level. The
> driver is able to resolve this by measuring intervals between hardware
> interrupts as long as interrupt latency is reasonably low, and we
> have another method using the i.MX6 hardware input capture support
> that can measure these intervals very accurately with no errors
> introduced by interrupt latency.

Requiring < 58 usec interrupt latency for correct operation is a
little too optimistic, no?
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 07/39] ARM: dts: imx6qdl-sabrelite: remove erratum ERR006687 workaround

[Pavel Machek]

On Fri 2017-03-10 16:17:28, Fabio Estevam wrote:
> On Fri, Mar 10, 2017 at 3:59 PM, Troy Kisky
>  wrote:
> > On 3/9/2017 8:52 PM, Steve Longerbeam wrote:
> >> There is a pin conflict with GPIO_6. This pin functions as a power
> >> input pin to the OV5642 camera sensor, but ENET uses it as the h/w
> >> workaround for erratum ERR006687, to wake-up the ARM cores on normal
> >> RX and TX packet done events. So we need to remove the h/w workaround
> >> to support the OV5642. The result is that the CPUidle driver will no
> >> longer allow entering the deep idle states on the sabrelite.
> >>
> >> This is a partial revert of
> >>
> >> commit 6261c4c8f13e ("ARM: dts: imx6qdl-sabrelite: use GPIO_6 for FEC
> >>   interrupt.")
> >> commit a28eeb43ee57 ("ARM: dts: imx6: tag boards that have the HW 
> >> workaround
> >>   for ERR006687")
> >>
> >> Signed-off-by: Steve Longerbeam 
> >> ---
> >>  arch/arm/boot/dts/imx6qdl-sabrelite.dtsi | 4 
> >>  1 file changed, 4 deletions(-)
> >>
> >> diff --git a/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi 
> >> b/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi
> >> index 8413179..89dce27 100644
> >> --- a/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi
> >> +++ b/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi
> >> @@ -270,9 +270,6 @@
> >>   txd1-skew-ps = <0>;
> >>   txd2-skew-ps = <0>;
> >>   txd3-skew-ps = <0>;
> >
> > How about
> >
> > +#if !IS_ENABLED(CONFIG_VIDEO_OV5642)

dts is supposed to be hardware description.

> Or maybe just create a new device tree for using the camera, like
> imx6q-sabrelite-camera.dts.

And it should not depend on configuration. Hardware vendor should be
able to ship board with working device tree...

Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline

[Pavel Machek]

Hi!

> Argh! that is indeed a bug at libv4l (and maybe at gstreamer).
> 
> I guess that the always_needs_conversion logic was meant to be used to
> really odd proprietary formats, e. g:
>   
> /*  Vendor-specific formats   */
> #define V4L2_PIX_FMT_CPIA1v4l2_fourcc('C', 'P', 'I', 'A') /* cpia1 YUV */
> #define V4L2_PIX_FMT_WNVA v4l2_fourcc('W', 'N', 'V', 'A') /* Winnov hw 
> compress */
> #define V4L2_PIX_FMT_SN9C10X  v4l2_fourcc('S', '9', '1', '0') /* SN9C10x 
> compression */
> ...
> 
> I suspect that nobody uses libv4l2 with MC-based V4L2 devices. That's
> likely why nobody reported this bug before (that I know of).
> 
> In any case, for non-proprietary formats, the default should be to
> always offer both the emulated format and the original one.
> 
> I suspect that the enclosed patch should fix the issue with bayer formats.

...
> @@ -178,7 +178,7 @@ struct v4lconvert_data 
> *v4lconvert_create_with_dev_ops(int fd, void *dev_ops_pri
>   /* This keeps tracks of devices which have only formats for which apps
>  most likely will need conversion and we can thus safely add software
>  processing controls without a performance impact. */
> - int always_needs_conversion = 1;
> + int always_needs_conversion = 0;
>  
>   if (!data) {
>   fprintf(stderr, "libv4lconvert: error: out of memory!\n");
> @@ -208,8 +208,8 @@ struct v4lconvert_data 
> *v4lconvert_create_with_dev_ops(int fd, void *dev_ops_pri
>   if (j < ARRAY_SIZE(supported_src_pixfmts)) {
>   data->supported_src_formats |= 1ULL << j;
>   v4lconvert_get_framesizes(data, fmt.pixelformat, j);
> - if (!supported_src_pixfmts[j].needs_conversion)
> - always_needs_conversion = 0;
> + if (supported_src_pixfmts[j].needs_conversion)
> + always_needs_conversion = 1;
>   } else
>   always_needs_conversion = 0;
>   }

Is the else still needed? You changed default to 0...


Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 15/36] platform: add video-multiplexer subdevice driver

[Pavel Machek]

Hi!

> > Plus you might want to describe which port correspond to which gpio
> > state/bitfield values...
> > 
> > > +struct vidsw {
> > 
> > I knew it: it is secretely a switch! :-).
> 
> This driver started as a two-input gpio controlled bus switch.
> I changed the name when adding support for bitfield controlled
> multiplexers with more than two inputs.

We had discussion with Sakari / Rob whether gpio controlled thing is a
switch or a multiplexer :-).

> > > + if (!pad) {
> > > + /* Mirror the input side on the output side */
> > > + cfg->type = vidsw->endpoint[vidsw->active].bus_type;
> > > + if (cfg->type == V4L2_MBUS_PARALLEL ||
> > > + cfg->type == V4L2_MBUS_BT656)
> > > + cfg->flags = 
> > > vidsw->endpoint[vidsw->active].bus.parallel.flags;
> > > + }
> > 
> > Will this need support for other V4L2_MBUS_ values?
> 
> To support CSI-2 multiplexers, yes.

Can you stick switch () {  default: dev_err() } there, to help
future hackers?

Thank,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC 0/3] Add support for led triggers on phy link state change

[Pavel Machek]

Hi!

> Some drivers that include phy.h defined LED_OFF which conflicts with
> definition in leds.h. phy led support uses leds.h so the two namespaces are no
> longer isolated.
> The first two patches fix the two net drivers that declared enum constants 
> that
> conflict with enum constants in linux/leds.h.

Perhaps led patches should be cced to led mainainters?

LED SUBSYSTEM
M:  Richard Purdie 
M:  Jacek Anaszewski 
L:  linux-l...@vger.kernel.org

Pavel


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging/android: add Doc for SW_SYNC ioctl interface

[Pavel Machek]

On Thu 2016-08-11 13:45:53, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.pado...@collabora.co.uk>
> 
> This interface is hidden from kernel headers and it is intended for use
> only for testing. So testers would have to add the ioctl information
> internally. This is to prevent misuse of this feature.
> 
> v2: take in Eric suggestions for the Documentation
> 
> v3: really take in Eric suggestions
> 
> Signed-off-by: Gustavo Padovan <gustavo.pado...@collabora.co.uk>
> Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>

Acked-by: Pavel Machek <pa...@ucw.cz>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/7] de-stage SW_SYNC validation frawework

[Pavel Machek]

On Tue 2016-08-09 08:04:54, Daniel Vetter wrote:
> On Sun, Jul 24, 2016 at 05:00:31PM +0200, Pavel Machek wrote:
> > On Mon 2016-08-08 16:08:12, Gustavo Padovan wrote:
> > > 2016-08-07 Pavel Machek <pa...@ucw.cz>:
> > > 
> > > > On Sun 2016-07-24 15:21:11, Greg Kroah-Hartman wrote:
> > > > > On Mon, Jul 18, 2016 at 04:12:45PM -0300, Gustavo Padovan wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > Do you think there is time to get this in for 4.8?
> > > > > 
> > > > > No, it was too late on my end, due to travel and vacation, sorry.  
> > > > > I'll
> > > > > queue it up for 4.9-rc1.
> > > > 
> > > > Could we get some documentation what this does? Is it visilble to
> > > > userspace?
> > > 
> > > This interface is only intended for testing and validation, there are
> > > ioctls on the debugfs file that can be accessed by userspace but there
> > > isn't any exported kernel header with this info. The tester should know
> > > and add a internal header to be able to access it. We want to prevent
> > > people from misusing this feature by not advertising it nor providing
> > > documentation.
> > 
> > You are playing dangerous game here. debugfs is not normally considered 
> > stable,
> > but otoh... ioctls on debugfs?
> 
> It's not considered stable. The idea is that we also add the existing
> testcases to kselftest. It's purely a bit of interface to be able to drive
> run the test logic for real fences. What it really tests is the fence
> interface (which is public in the uapi headers and all that), but to be
> able to do that we need some (hw-independent way) to expose fences, which
> this provides.
> 
> Long term we might even do this as a proper interface (with some
> restrictions to make it safe and avoid userspace pulling the kernel over
> the table). And then rip out sw_sync entirely.
> 
> Imo there's no need at all for docs for this.

There's full directory of files, with absolutely zero comments/documentation. 
They
are quite hard to understand. Plus it has userland interface.

IMO comment should be added, explaining what it is testing, that interface is 
not considered
stable, and where the test lives.

I know what "fence" is in the cpu sense (mfence and friends), but I'm not sure 
what "real fence" is in this context.

Best regards,

Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/6] staging/android: remove doc from sw_sync

[Pavel Machek]

On Mon 2016-08-08 18:24:17, Gustavo Padovan wrote:
> From: Gustavo Padovan 
> 
> SW_SYNC should never be used by other pieces of the kernel apart from
> sync_debug as it is only a Sync File Validation Framework, so hide any
> info to avoid confuse this with a standard kernel internal API.

> Signed-off-by: Gustavo Padovan 

NAK.

It is unclear for what the code does, removing the docs is not going to help.

If it should not be used, document that it should not be used.. but not remove
the docs.

> -/**
> - * sync_timeline_signal() - signal a status change on a sync_timeline
> - * @obj: sync_timeline to signal
> - * @inc: num to increment on timeline->value
> - *
> - * A sync implementation should call this any time one of it's fences
> - * has signaled or has an error condition.
> - */
>  static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
>  {

And as the functions are static... there's little danger that someone will 
misuse them.


Pavel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/7] de-stage SW_SYNC validation frawework

[Pavel Machek]

On Mon 2016-08-08 16:08:12, Gustavo Padovan wrote:
> 2016-08-07 Pavel Machek <pa...@ucw.cz>:
> 
> > On Sun 2016-07-24 15:21:11, Greg Kroah-Hartman wrote:
> > > On Mon, Jul 18, 2016 at 04:12:45PM -0300, Gustavo Padovan wrote:
> > > > Hi,
> > > > 
> > > > Do you think there is time to get this in for 4.8?
> > > 
> > > No, it was too late on my end, due to travel and vacation, sorry.  I'll
> > > queue it up for 4.9-rc1.
> > 
> > Could we get some documentation what this does? Is it visilble to
> > userspace?
> 
> This interface is only intended for testing and validation, there are
> ioctls on the debugfs file that can be accessed by userspace but there
> isn't any exported kernel header with this info. The tester should know
> and add a internal header to be able to access it. We want to prevent
> people from misusing this feature by not advertising it nor providing
> documentation.

You are playing dangerous game here. debugfs is not normally considered stable,
but otoh... ioctls on debugfs?

Anyway, please provide some documentation. Kernel hackers need to know what 
this does.

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/7] de-stage SW_SYNC validation frawework

[Pavel Machek]

On Sun 2016-07-24 15:21:11, Greg Kroah-Hartman wrote:
> On Mon, Jul 18, 2016 at 04:12:45PM -0300, Gustavo Padovan wrote:
> > Hi,
> > 
> > Do you think there is time to get this in for 4.8?
> 
> No, it was too late on my end, due to travel and vacation, sorry.  I'll
> queue it up for 4.9-rc1.

Could we get some documentation what this does? Is it visilble to
userspace?

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/7] de-stage SW_SYNC validation frawework

[Pavel Machek]

Hi!

> From: Gustavo Padovan 
> 
> Hi Greg,
> 
> This is the last step in the Sync Framwork de-stage task. It

Typo: "fram_e_work"

> de-stage
> the SW_SYNC validation framework and the sync_debug info debugfs file.
> 
> The first 3 patches are clean up and improvements and the rest is preparation
> to de-stage and then finally the actual de-stage.

Could we get some kind of description what the sync framework does?
There are no useful comments in /sw_sync.c. There's no Documentation/
files. I don't know what this is supposed to do...

Pavel

> Please review,
> 
> Gustavo
> 
> ---
> Gustavo Padovan (7):
>   staging/android: move trace/sync.h to sync_trace.h
>   staging/android: remove doc from sw_sync
>   staging/android: display sync_pt name on debugfs
>   staging/android: do not let userspace trigger WARN_ON
>   staging/android: prepare sw_sync files for de-staging
>   dma-buf/sw_sync: de-stage SW_SYNC
>   staging/android: remove sync framework TODO
> 
>  drivers/dma-buf/Kconfig| 14 +++
>  drivers/dma-buf/Makefile   |  1 +
>  drivers/{staging/android => dma-buf}/sw_sync.c | 46 
> +++---
>  drivers/{staging/android => dma-buf}/sync_debug.c  |  7 ++--
>  drivers/{staging/android => dma-buf}/sync_debug.h  | 26 +---
>  .../android/trace/sync.h => dma-buf/sync_trace.h}  |  6 +--
>  drivers/staging/android/Kconfig| 13 --
>  drivers/staging/android/Makefile   |  1 -
>  drivers/staging/android/TODO   |  8 
>  9 files changed, 38 insertions(+), 84 deletions(-)
>  rename drivers/{staging/android => dma-buf}/sw_sync.c (84%)
>  rename drivers/{staging/android => dma-buf}/sync_debug.c (97%)
>  rename drivers/{staging/android => dma-buf}/sync_debug.h (72%)
>  rename drivers/{staging/android/trace/sync.h => dma-buf/sync_trace.h} (84%)
> 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/6] Intel Secure Guard Extensions

[Pavel Machek]

Hi!

> Good morning, I hope everyone's day is starting out well.

:-). Rainy day here.

> > > In the TL;DR department I would highly recommend that anyone
> > > interested in all of this read MIT's 170+ page review of the
> > > technology before jumping to any conclusions :-)
> 
> > Would you have links for 1-5?
> 
> First off my apologies to the list as I loathe personal inaccuracy,
> the MIT review paper is only 117 pages long.  I was typing the last
> e-mail at 0405 in the morning and was scrambling for the opportunity
> to get 50 minutes of sleep so my proofreading was sloppy... :-)

Thanks a lot for the links, I'd still say it was more accurate than
average for the lkml.

Best regards,
Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/6] Intel Secure Guard Extensions

[Pavel Machek]

Hi!

> We have been following and analyzing this technology since the first
> HASP paper was published detailing its development.  We have been

(1)

> 
> I told my associates the first time I reviewed this technology that
> SGX has the ability to be a bit of a Pandora's box and it seems to be
> following that course.

Can you elaborate on the Pandora's box? System administrator should be able to
disable SGX on the system, and use system to do anything that could be done with
the older CPUs, right?

> support data and application confidentiality and integrity in the face
> of an Iago threat environment, ie. a situation where a security

(2)

> Intel is obviously cognizant of the risk surrounding illicit uses of
> this technology since it clearly calls out that, by agreeing to have
> their key signed, a developer agrees to not implement nefarious or
> privacy invasive software.  Given the known issues that Certificate

Yeah, that's likely to work ... not :-(. "It is not spyware, it is just
collecting some anonymous statistics."

> domination and control.  They probably have enough on their hands with
> attempting to convert humanity to FPGA's and away from devices which
> are capable of maintaining a context of exection... :-)

Heh. FPGAs are not designed to replace CPUs anytime soon... And probably never.

> the Haven paper in which Microsoft Research discussed how SGX could be
> used to run unmodified Windows applications within an SGX TEE.

(3)

> I think Intel was somewhat sobered by the follow on paper in which
> Microsoft demonstrated that in an Iago environment an interloper was
> capable of determing with accuracy levels greater then 60% what was
> being done in an SGX TEE.  Matt Hoekstra was very quick to call out
> the need for the community to understand and develop side channel

(4)

> In the TL;DR department I would highly recommend that anyone
> interested in all of this read MIT's 170+ page review of the
> technology before jumping to any conclusions :-)

(5)

Would you have links for 1-5?

Thanks,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 06/12] staging/android: prepare sync_file for de-staging

[Pavel Machek]

Hi!


> -}
> -EXPORT_SYMBOL(sync_file_merge);
> -
>  static const char *android_fence_get_driver_name(struct fence *fence)
>  {
>   struct sync_timeline *parent = fence_parent(fence);

if this is meant to be used outside android, should it select some
better prefix than android_fence_?

Thanks,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 01/12] staging/android: remove redundant comments on sync_merge_data

[Pavel Machek]

On Wed 2016-04-27 13:27:08, Gustavo Padovan wrote:
> From: Gustavo Padovan 
> 
> struct sync_merge_data already have documentation on top of the
> struct definition. No need to duplicate it.
> 
> Signed-off-by: Gustavo Padovan 
> Reviewed-by: Maarten Lankhorst 

> @@ -33,8 +33,8 @@ struct sync_merge_data {
>  /**
>   * struct sync_fence_info - detailed fence information
>   * @obj_name:name of parent sync_timeline
> - * @driver_name: name of driver implementing the parent
> - * @status:  status of the fence 0:active 1:signaled <0:error
> +* @driver_name:  name of driver implementing the parent
> +* @status:   status of the fence 0:active 1:signaled <0:error

The whitespace (or mail client configuration?) looks wrong here.

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/6] Intel Secure Guard Extensions

[Pavel Machek]

Hi!
On Fri 2016-04-29 23:17:44, Jarkko Sakkinen wrote:
> On Tue, Apr 26, 2016 at 09:00:10PM +0200, Pavel Machek wrote:
> > On Mon 2016-04-25 20:34:07, Jarkko Sakkinen wrote:
> > > The firmware uses PRMRR registers to reserve an area of physical memory
> > > called Enclave Page Cache (EPC). There is a hardware unit in the
> > > processor called Memory Encryption Engine. The MEE encrypts and decrypts
> > > the EPC pages as they enter and leave the processor package.
> > 
> > What are non-evil use cases for this?
> 
> I'm not sure what you mean by non-evil.

Well, that's kind of hard to explain.

> > What are non-evil use cases for this?
> 
> Virtual TPMs for containers/guests would be one such use case.

..but as long as Intel has to sign each piece of software so that it
can use SGX, it is unsuitable for Linux kernel.

Best regards,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/6] Intel Secure Guard Extensions

[Pavel Machek]

Hi!

> > > Preventing cold boot attacks is really just icing on the cake.  The
> > > real point of this is to allow you to run an "enclave".  An SGX
> > > enclave has unencrypted code but gets access to a key that only it can
> > > access.  It could use that key to unwrap your ssh private key and sign
> > > with it without ever revealing the unwrapped key.  No one, not even
> > > root, can read enclave memory once the enclave is initialized and gets
> > > access to its personalized key.  The point of the memory encryption
> > > engine to to prevent even cold boot attacks from being used to read
> > > enclave memory.
> >
> > Ok, so the attacker can still access the "other" machine, but ok, key
> > is protected.
> >
> > But... that will mean that my ssh will need to be SGX-aware, and that
> > I will not be able to switch to AMD machine in future. ... or to other
> > Intel machine for that matter, right?
> 
> That's the whole point.  You could keep an unwrapped copy of the key
> offline so you could provision another machine if needed.
> 
> >
> > What new syscalls would be needed for ssh to get all this support?
> 
> This patchset or similar, plus some user code and an enclave to use.
> 
> Sadly, on current CPUs, you also need Intel to bless the enclave.  It
> looks like new CPUs might relax that requirement.

Umm. I'm afraid my evil meter just went over "smells evil" and "bit
evil" areas straight to "certainly looks evil".

> > > Replay Protected Memory Block.  It's a device that allows someone to
> > > write to it and confirm that the write happened and the old contents
> > > is no longer available.  You could use it to implement an enclave that
> > > checks a password for your disk but only allows you to try a certain
> > > number of times.
> >
> > Ookay... I guess I can get a fake Replay Protected Memory block, which
> > will confirm that write happened and not do anything from China, but
> > ok, if you put that memory on the CPU, you raise the bar to a "rather
> > difficult" (tm) level. Nice.
> 
> It's not so easy for the RPMB to leak things.  It would be much easier
> for it to simply not provide replay protection (i.e. more or less what
> the FBI asked from Apple: keep allowing guesses even though that
> shouldn't work).

Yup.

> > But that also means that when my CPU dies, I'll no longer be able to
> > access the encrypted data.
> 
> You could implement your own escrow policy and keep a copy in the
> safe.

And then Intel would have to bless my own escrow policy, which is,
realistically, not going to happen, right?

> > And, again, it means that quite complex new kernel-user interface will
> > be needed, right?
> 
> It's actually fairly straightforward, and the kernel part doesn't care
> what you use it for (the kernel part is the same for disk encryption
> and ssh, for example, except that disk encryption would care about
> replay protection, whereas ssh wouldn't).

So we end up with parts of kernel we can not change, and where we may
not even change the compiler. That means assembly. Hey, user, you have
freedom to this code, except it will not work. That was called TiVo
before. We'd have security-relevant parts of kernel where we could not
even fix a securit holes without Intel.

If anything, this is reason to switch to GPLv3.

I'm sorry. This is evil.

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/6] Intel Secure Guard Extensions

[Pavel Machek]

On Tue 2016-04-26 21:59:52, One Thousand Gnomes wrote:
> > But... that will mean that my ssh will need to be SGX-aware, and that
> > I will not be able to switch to AMD machine in future. ... or to other
> > Intel machine for that matter, right?
> 
> I'm not privy to AMD's CPU design plans.
> 
> However I think for the ssl/ssh case you'd use the same interfaces
> currently available for plugging in TPMs and dongles. It's a solved
> problem in the crypto libraries.
> 
> > What new syscalls would be needed for ssh to get all this support?
> 
> I don't see why you'd need new syscalls.

So the kernel will implement few selected crypto algorithms, similar
to what TPM would provide, using SGX, and then userspace no longer
needs to know about SGX?

Ok, I guess that's simple.

It also means it is boring, and the multiuser-game-of-the-day will not
be able to protect the (plain text) password from the cold boot
attack.

Nor will be emacs be able to protect in-memory copy of my diary from
cold boot attack.

So I guess yes, some new syscalls would be nice :-).
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/6] Intel Secure Guard Extensions

[Pavel Machek]

Hi!

> >> >> The firmware uses PRMRR registers to reserve an area of physical memory
> >> >> called Enclave Page Cache (EPC). There is a hardware unit in the
> >> >> processor called Memory Encryption Engine. The MEE encrypts and decrypts
> >> >> the EPC pages as they enter and leave the processor package.
> >> >
> >> > What are non-evil use cases for this?
> >>
> >> Storing your ssh private key encrypted such that even someone who
> >> completely compromises your system can't get the actual private key
> >
> > Well, if someone gets root on my system, he can get my ssh private
> > key right?
> >
> > So, you can use this to prevent "cold boot" attacks? (You know,
> > stealing machine, liquid nitrogen, moving DIMMs to different machine
> > to read them?) Ok. That's non-evil.
> 
> Preventing cold boot attacks is really just icing on the cake.  The
> real point of this is to allow you to run an "enclave".  An SGX
> enclave has unencrypted code but gets access to a key that only it can
> access.  It could use that key to unwrap your ssh private key and sign
> with it without ever revealing the unwrapped key.  No one, not even
> root, can read enclave memory once the enclave is initialized and gets
> access to its personalized key.  The point of the memory encryption
> engine to to prevent even cold boot attacks from being used to read
> enclave memory.

Ok, so the attacker can still access the "other" machine, but ok, key
is protected.

But... that will mean that my ssh will need to be SGX-aware, and that
I will not be able to switch to AMD machine in future. ... or to other
Intel machine for that matter, right?

What new syscalls would be needed for ssh to get all this support?

> > Is there reason not to enable this for whole RAM if the hw can do it?
> 
> The HW can't, at least not in the current implementation.  Also, the
> metadata has considerable overhead (no clue whether there's a
> performance hit, but there's certainly a memory usage hit).

:-(.

> >> out.  Using this in conjunction with an RPMB device to make it Rather
> >> Difficult (tm) for third parties to decrypt your disk even if you
> >> password has low entropy.  There are plenty more.
> >
> > I'm not sure what RPMB is, but I don't think you can make it too hard
> > to decrypt my disk if my password has low entropy. ... And I don't see
> > how encrypting RAM helps there.
> 
> Replay Protected Memory Block.  It's a device that allows someone to
> write to it and confirm that the write happened and the old contents
> is no longer available.  You could use it to implement an enclave that
> checks a password for your disk but only allows you to try a certain
> number of times.

Ookay... I guess I can get a fake Replay Protected Memory block, which
will confirm that write happened and not do anything from China, but
ok, if you put that memory on the CPU, you raise the bar to a "rather
difficult" (tm) level. Nice.

But that also means that when my CPU dies, I'll no longer be able to
access the encrypted data.

And, again, it means that quite complex new kernel-user interface will
be needed, right?
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/6] Intel Secure Guard Extensions

[Pavel Machek]

On Tue 2016-04-26 12:05:48, Andy Lutomirski wrote:
> On Tue, Apr 26, 2016 at 12:00 PM, Pavel Machek <pa...@ucw.cz> wrote:
> > On Mon 2016-04-25 20:34:07, Jarkko Sakkinen wrote:
> >> Intel(R) SGX is a set of CPU instructions that can be used by
> >> applications to set aside private regions of code and data.  The code
> >> outside the enclave is disallowed to access the memory inside the
> >> enclave by the CPU access control.
> >>
> >> The firmware uses PRMRR registers to reserve an area of physical memory
> >> called Enclave Page Cache (EPC). There is a hardware unit in the
> >> processor called Memory Encryption Engine. The MEE encrypts and decrypts
> >> the EPC pages as they enter and leave the processor package.
> >
> > What are non-evil use cases for this?
> 
> Storing your ssh private key encrypted such that even someone who
> completely compromises your system can't get the actual private key

Well, if someone gets root on my system, he can get my ssh private
key right?

So, you can use this to prevent "cold boot" attacks? (You know,
stealing machine, liquid nitrogen, moving DIMMs to different machine
to read them?) Ok. That's non-evil.

Is there reason not to enable this for whole RAM if the hw can do it? 

> out.  Using this in conjunction with an RPMB device to make it Rather
> Difficult (tm) for third parties to decrypt your disk even if you
> password has low entropy.  There are plenty more.

I'm not sure what RPMB is, but I don't think you can make it too hard
to decrypt my disk if my password has low entropy. ... And I don't see
how encrypting RAM helps there.

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v11 3/4] add FPGA manager core

[Pavel Machek]

Hi!

> > Of course, the maintainer gets the last word regardless of what anyone
> > else thinks.
> > 
> > Generally, minimal code is better.  Trying to future proof code is a
> > waste of time because you can't predict what will happen in the future.
> > It's way more likely that some pointer you never expected to be NULL
> > will be NULL instead of the few checked at the beginning of a function.
> > Adding useless code uses RAM and makes the function slower.  It's a bit
> > confusing for users as well because they will wonder when the NULL check
> > is used.  A lot of times this sort of error handling is a bit fake and
> > what I mean is that it looks correct but the system will just crash in a
> > later function.
> > 
> > Also especially with a simple NULL dereferences like this theoretical
> > one, it's better to just get the oops.  It kills the module but you get
> > a good message in the log and it's normally straight forward to debug.
> > 
> > We spent a surprising amount of time discussing useless code.  I made
> > someone redo a patch yesterday because they had incomplete error
> > handling for a situation which could never happen.
> 
> Thanks for the discussion.
> 
> Interesting.  The amount of code bloat here compiles down to about two
> machine instructions (in two places).  Actually a little more since I should
> be using IS_ERR_OR_NULL.  But the main question is whether I should do
> it at all.
> 
> The behaviour I should drive here is that the user will do their own error
> checking.  After they get a pointer to a FPGA manager using
> of_fpga_mgr_get(), they should check it and not assume that
> fpga_mgr_firmware_load() will do it for them, i.e.
> 
>   mgr = of_fpga_mgr_get(mgr_node);
>   if (IS_ERR(mgr))
>   return PTR_ERR(mgr);
>   fpga_mgr_firmware_load(mgr, flags, path);
> 
> I could take out these NULL pointer checks and it won't hurt anything unless
> someone is just using the functions badly, in which case: kablooey.

2 instructions is not that bad, do whatever is easier for you. These
patches received enough bikeshed painting.

Best regards,
Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v11 3/4] add FPGA manager core

[Pavel Machek]

Hi!

> > +++ b/drivers/fpga/fpga-mgr.c
> > @@ -0,0 +1,382 @@
> [..]
> > +int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, const char *buf,
> > + size_t count)
> > +{
> > +   struct device *dev = >dev;
> > +   int ret;
> > +
> > +   if (!mgr)
> > +   return -ENODEV;
> 
> This seems overly defensive.
> 
> [..]
> > +int fpga_mgr_firmware_load(struct fpga_manager *mgr, u32 flags,
> > +  const char *image_name)
> > +{
> > +   struct device *dev = >dev;
> > +   const struct firmware *fw;
> > +   int ret;
> > +
> > +   if (!mgr)
> > +   return -ENODEV;
> 
> Again; I'm of the opinion this is needlessly defensive.

Not only that, it can never happen. mgr is already dereferenced above.

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v10 6/8] staging: add bindings document for simple fpga bus

[Pavel Machek]

On Thu 2015-08-13 12:37:30, at...@opensource.altera.com wrote:
 From: Alan Tull at...@opensource.altera.com
 
 New bindings document for simple fpga bus.
 
 Signed-off-by: Alan Tull at...@opensource.altera.com

Acked-by: Pavel Machek pa...@ucw.cz

 + onchip_memory2_0: memory@0x0 {
 + device_type = memory;
 + compatible = ALTR,onchipmem-15.1;

I guess this should be altr, .


 + jtag_uart: serial@0x10002 {
 + compatible = altr,juart-15.1, 
 altr,juart-1.0;
 + reg = 0x0001 0x0002 
 0x0008;
 + interrupt-parent = intc;

Best regards,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v10 5/8] staging: usage documentation for simple fpga bus

[Pavel Machek]

On Thu 2015-08-13 12:37:29, at...@opensource.altera.com wrote:
 From: Alan Tull at...@opensource.altera.com
 
 Add a document spelling out usage of the simple fpga bus.
 
 Signed-off-by: Alan Tull at...@opensource.altera.com

Acked-by: Pavel Machek pa...@denx.de

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v10 3/8] add fpga manager core

[Pavel Machek]

On Thu 2015-08-13 12:37:27, at...@opensource.altera.com wrote:
 From: Alan Tull at...@opensource.altera.com
 
 API to support programming FPGA.

I'd do s/fpga/FPGA/ in the comments, too. Otherwise looks ok to me.


Acked-by: Pavel Machek pa...@denx.de

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v10 8/8] staging: add simple-fpga-bus

[Pavel Machek]

On Thu 2015-08-13 12:37:32, at...@opensource.altera.com wrote:
 From: Alan Tull at...@opensource.altera.com
 
 Add simple fpga bus.  This is a bus that configures an fpga and its
 bridges before populating the devices below it.  This is intended
 for use with device tree overlays.
 
 Note that FPGA bridges are seen as reset controllers so no special
 framework for FPGA bridges will need to be added.
 
 This supports fpga use where hardware blocks on a fpga will need
 drivers (as opposed to fpga used as an acceleration without drivers.)
 
 Signed-off-by: Alan Tull at...@opensource.altera.com

7,8: Acked-by: Pavel Machek pa...@denx.de

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v9 4/7] staging: fpga manager: add sysfs interface document

[Pavel Machek]

Hi!

 +What:/sys/class/fpga_manager/fpga/state
 +Date:July 2015
 +KernelVersion:   4.2
 +Contact: Alan Tull at...@opensource.altera.com
 +Description: Read fpga manager state as a string.

fpga-FPGA.

 + Valid states may vary by manufacturer; superset is:
 + * unknown   = can't determine state
 + * power off = FPGA power is off
 + * power up  = FPGA reports power is up
 + * reset = FPGA held in reset state
 + * firmware request  = firmware class request in progress
 + * firmware request error = firmware request failed
 + * write init= FPGA being prepared for programming
 + * write init error  = Error while preparing FPGA for
 +   programming
 + * write = FPGA ready to receive image data
 + * write error   = Error while programming
 + * write complete= Doing post programming steps
 + * write complete error  = Error while doing post programming
 + * operating = FPGA is programmed and operating

This will need some more details. firmware request is hardly a
hardware state, does it belong here? Is power off or on while firmware
is being requested? How does the fpga get into power up phase?
Normally, you'd only power it on to do something more with it...?

Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v9 4/7] staging: fpga manager: add sysfs interface document

[Pavel Machek]

On Fri 2015-07-24 07:39:15, atull wrote:
 On Fri, 24 Jul 2015, Pavel Machek wrote:
 
 Hi Pavel,
 
 Thanks for your your feedback in cleaning up these docs.
 
  Hi!
  
   +What:/sys/class/fpga_manager/fpga/state
   +Date:July 2015
   +KernelVersion:   4.2
   +Contact: Alan Tull at...@opensource.altera.com
   +Description: Read fpga manager state as a string.
  
  fpga-FPGA.
 
 Yep
 
  
   + Valid states may vary by manufacturer; superset is:
   + * unknown   = can't determine state
   + * power off = FPGA power is off
   + * power up  = FPGA reports power is up
   + * reset = FPGA held in reset state
   + * firmware request  = firmware class request in progress
   + * firmware request error = firmware request failed
   + * write init= FPGA being prepared for programming
   + * write init error  = Error while preparing FPGA for
   +   programming
   + * write = FPGA ready to receive image data
   + * write error   = Error while programming
   + * write complete= Doing post programming steps
   + * write complete error  = Error while doing post programming
   + * operating = FPGA is programmed and operating
  
 
 If I can make my intent clear, maybe we can figure out what will be most
 useful here.  
 
 The intent is to provide enough detail that if something goes wrong with
 the FPGA programming (something that the driver can't take care of) then 
 userspace can know that.  Such as if the firmware request fails, that 
 could be due to not being able to find the firmware file.
 
  This will need some more details. firmware request is hardly a
  hardware state, does it belong here? 
 
 This is a superset of FPGA states and fpga manager driver states as the
 fpga manager driver is walking through the steps to get the FPGA into
 a known operating state.  So it's a sequence, though some steps may get
 skipped. If there is an error, then userspace can know what step failed.
 
 Maybe this should be separated into fpga_state for hardware state and
 fpga_mgr_status (to report what step of progress the fpga manager driver
 is at during programming).  I want this to be useful and still not be 
 device (FPGA) specific.
 
  Is power off or on while firmware
  is being requested? 
 
 On.  It's a sequence.

Aha. Ok, so maybe noting that states normally go in the sequence (with
exception of various errors) would be enough?

Thanks,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v9 1/7] staging: usage documentation for FPGA manager core

[Pavel Machek]

On Fri 2015-07-17 10:51:11, at...@opensource.altera.com wrote:
 From: Alan Tull at...@opensource.altera.com
 
 Add a document on the new FPGA manager core.
 

 --- /dev/null
 +++ b/drivers/staging/fpga/Documentation/fpga-mgr.txt
 @@ -0,0 +1,117 @@
 +  FPGA Manager Core
 +
 +  Alan Tull 2015
 +
 +  Overview
 +  

The formatting is quite funny here. Add a newline after ---'s? Or
better format it the way other documents are formatted?

 +The FPGA manager core exports a set of functions for programming an image to 
 a

into?

 +FPGA.  All manufacturor specifics are hidden away in a low level driver.  The

manufacturer (more then one instance).

 +API is manufacturor agnostic.  Of course the FPGA image data itself is very
 +manufacturor specific but for our purposes it's just data in a buffer or file

, but

 +or something.  The FPGA manager core won't parse it or know anything about 
 it.

kill or know anything?

 +  Files
 +  -
 +drivers/staging/fpga/fpga-mgr.c
 +include/linux/fpga/fpga-mgr.h
 +

Kill this section, as it is going to change?

 +  The API Functions
 +  
 +The API that is exported is currently 6 functions:
 +
 +   int fpga_mgr_buf_load(struct fpga_manager *mgr,
 + u32 flags,
 + const char *buf,
 + size_t count);
 +
 +An FPGA image exists as a buffer in memory.  Load it into the FPGA.  The FPGA
 +ends up in operating mode or return a negative error code.

So 0 on success?

 +   int fpga_mgr_firmware_load(struct fpga_manager *mgr,
 +  u32 flags,
 +  const char *image_name);
 +
 +An FPGA image exists as a file that is on the firmware search path (see the

, that is in

 +firmware class documentation).  Load as above.
 +
 +   struct fpga_manager *of_fpga_mgr_get(struct device_node *node);
 +
 +Given a DT node, get a reference to a fpga manager.

fpga-FPGA, fix globally??

 +   void fpga_mgr_put(struct fpga_manager *mgr);
 +
 +Release the reference to the fpga manager.
 +
 +   int fpga_mgr_register(struct device *dev,
 + const char *name,
 + const struct fpga_manager_ops *mops,
 + void *priv);
 +   void fpga_mgr_unregister(struct device *dev);
 +
 +Register/unregister the lower level device specific driver.

low level.. And device specific - FPGA-specific ?


 +To add another fpga manager, look at the bottom part of socfpga.c for an
 +example, starting with the declaration of socfpga_fpga_ops.

Umm. You have good documentation below. Maybe you don't need to send
people to read sources...?

 +static const struct fpga_manager_ops socfpga_fpga_ops = {
 +   .write_init = socfpga_fpga_ops_configure_init,
 +   .write = socfpga_fpga_ops_configure_write,
 +   .write_complete = socfpga_fpga_ops_configure_complete,
 +   .state = socfpga_fpga_ops_state,
 +};
 +
 +You will want to create a platform driver that has a set of ops like that
 +and then register it with fpga_mgr_register in your probe function.  Your
 +ops will implement whatever device specific register writes needed and
 +will return negative error codes if things don't go well.
 +
 +The programming seqence is:

sequence.

 + 1. .write_init
 + 2. .write (may be called once or multiple times)
 + 3. .write_complete
 +
 +The .write_init function will prepare the FPGA to receive the image data.
 +
 +The .write function receives an image buffer or a chunk of the image and
 +writes it the FPGA.  The buffer may arrive as one chunk or a bunck
 of

bunch.

 +small chunks through this function being called multiple times.
 +
 +The .write_complete function is called after all the image has been written
 +to put the FPGA into operating mode.
 +
 +The .state function will read your hardware and return a code of type
 +enum fpga_mgr_states.  It doesn't result in a change in hardware state.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v9 3/7] staging: add bindings document for simple fpga bus

[Pavel Machek]

 +Optional properties:
 +- fpga-mgr : should contain a phandle to a fpga manager.

fpga-FPGA, globally.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v9 2/7] staging: usage documentation for simple fpga bus

[Pavel Machek]

On Fri 2015-07-17 10:51:12, at...@opensource.altera.com wrote:
 From: Alan Tull at...@opensource.altera.com
 
 Add a document spelling out usage of the simple fpga bus.

 +The DT overlay includes bindings (documented in bindings/simple-fpga-bus.txt)
 +that specify:
 + * Which fpga manager to use

fpga-FPGA, globally.

 + * Which image file to load
 + * Flags indicating whether this this image is for full reconfiguration or
 +   partial.
 + * a list of resets that should be released.  These enable the FPGA bridges.
 + * child nodes specifying the devices that will be added with appropriate
 +   compatible strings, etc.

Either all entries in the list should start with big letter or none
should. Also . at end of line should be consistent.

 +   Sequence
 +   
 + 1. Load the DT overlay.  One convenient way to do that is to use Pantelis'
 +handy configfs interface (more below).

Reader has no chance to know what Pantelis' configfs interface is, and
there's nothing below.

 + 2. The simple FPGA bus gets probed and will do the following:
 +a. call the fpga manager core to program the FPGA
 +b. release the FPGA bridges
 +c. call of_platform_populate resulting in device drivers getting probed.
 +

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] radio-bcm2048: remove unused var

[Pavel Machek]

On Tue 2015-04-28 09:03:41, Mauro Carvalho Chehab wrote:
 drivers/staging/media/bcm2048/radio-bcm2048.c: In function 
 'bcm2048_i2c_driver_probe':
 drivers/staging/media/bcm2048/radio-bcm2048.c:2596:11: warning: variable 
 'skip_release' set but not used [-Wunused-but-set-variable]
   int err, skip_release = 0;
^
 
 Signed-off-by: Mauro Carvalho Chehab mche...@osg.samsung.com

Acked-by: Pavel Machek pa...@ucw.cz

 diff --git a/drivers/staging/media/bcm2048/radio-bcm2048.c 
 b/drivers/staging/media/bcm2048/radio-bcm2048.c
 index e9d0691b21d3..5e11a78ceef3 100644
 --- a/drivers/staging/media/bcm2048/radio-bcm2048.c
 +++ b/drivers/staging/media/bcm2048/radio-bcm2048.c
 @@ -2593,7 +2593,7 @@ static int bcm2048_i2c_driver_probe(struct i2c_client 
 *client,
   const struct i2c_device_id *id)
  {
   struct bcm2048_device *bdev;
 - int err, skip_release = 0;
 + int err;
  
   bdev = kzalloc(sizeof(*bdev), GFP_KERNEL);
   if (!bdev) {
 @@ -2646,7 +2646,6 @@ free_sysfs:
   bcm2048_sysfs_unregister_properties(bdev, ARRAY_SIZE(attrs));
  free_registration:
   video_unregister_device(bdev-videodev);
 - skip_release = 1;
  free_irq:
   if (client-irq)
   free_irq(client-irq, bdev);

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 20/20] arm: mach-pxa: Decrement the power supply's device reference counter

[Pavel Machek]

On Fri 2015-02-27 09:20:01, Krzysztof Kozlowski wrote:
 Use power_supply_put() to decrement the power supply's device reference
 counter.
 
 Signed-off-by: Krzysztof Kozlowski k.kozlow...@samsung.com
 Reviewed-by: Bartlomiej Zolnierkiewicz b.zolnier...@samsung.com
 Reviewed-by: Sebastian Reichel s...@kernel.org
 Acked-by: Robert Jarzmik robert.jarz...@free.fr

Acked-by: Pavel Machek pa...@ucw.cz

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 00/20] power_supply: Allow safe usage of power supply

[Pavel Machek]

Hi!
 
 The patchset fixes invalid memory accesses in certain race scenarios by
 moving ownership of struct power_supply to the core. All drivers are
 modified.

Ok, who can apply the patches? Sebastian?

Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v8 2/4] fpga manager: add sysfs interface document

[Pavel Machek]

On Tue 2015-02-17 11:07:53, Rob Landley wrote:
 
 
 On 02/15/2015 04:40 PM, Pavel Machek wrote:
  On Wed 2015-01-21 13:27:00, Jason Gunthorpe wrote:
  On Wed, Jan 21, 2015 at 06:33:12PM +0200, Pantelis Antoniou wrote:
  My point is that the current firmware layer is overly cautious and
  FPGAs are very big. My current project on small Xilinx device has a
  10MB programming file. The biggest Xilinx device today has a max
  bitfile size around 122MB.
 
  So keeping that much memory pinned in the kernel when I can prove it
  is uncessary for my system (either because there is no suspend/resume
  possibility, or because I know the CPU can always access the
  filesytem) is very undesirable.
  
  Well, your current device aalso has 1GB RAM, no?
 
 Unnecessarily pinning 10% of your ram is a good solution?

Never said that. But I'd rather have _some_ API proposed, then try to
design in everthing including kitchen sink and do nothing.
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v8 2/4] fpga manager: add sysfs interface document

[Pavel Machek]

On Wed 2015-01-21 13:27:00, Jason Gunthorpe wrote:
 On Wed, Jan 21, 2015 at 06:33:12PM +0200, Pantelis Antoniou wrote:
  Hi Alan,
  
   On Jan 21, 2015, at 18:01 , One Thousand Gnomes 
   gno...@lxorguk.ukuu.org.uk wrote:
   
   On Thu, 15 Jan 2015 22:54:46 +0200
   Pantelis Antoniou pantelis.anton...@konsulko.com wrote:
   
   Hi Alan,
   
   On Jan 15, 2015, at 22:45 , One Thousand Gnomes 
   gno...@lxorguk.ukuu.org.uk wrote:
   
   On Thu, 15 Jan 2015 11:47:26 -0700
   Jason Gunthorpe jguntho...@obsidianresearch.com wrote:
   It is a novel idea, my concern would be that embedding the FPGA in the
   DT makes it permanent unswappable kernel memory.
   Not having the kernel hold the FPGA is best for many uses.
   
   If you have a filesysytem before the FPGA is set up then it belongs in
   the file system. As you presumably loaded the kernel from somewhere 
   there
   ought to be a file system (even an initrd).
   
   
   Request firmware does not imply keeping it around. You can always 
   re-request
   when reloading (although there’s a nasty big of caching that needs to be
   resolved with the firmware loader).
   
   Which comes down to the same thing. Unless you can prove that there is a
   path to recover the firmware file that does not have any dependancies
   upon the firmware executing (and those can be subtle and horrid at times)
   you need to keep it around for suspend/resume at least and potentially
   any unexpected error/reset.
   
  
  In that case the only safe place to put it is in the kernel image itself, 
  which
  is something the firmware loader already supports.
 
 My point is that the current firmware layer is overly cautious and
 FPGAs are very big. My current project on small Xilinx device has a
 10MB programming file. The biggest Xilinx device today has a max
 bitfile size around 122MB.
 
 So keeping that much memory pinned in the kernel when I can prove it
 is uncessary for my system (either because there is no suspend/resume
 possibility, or because I know the CPU can always access the
 filesytem) is very undesirable.

Well, your current device aalso has 1GB RAM, no?

 Other systems might have to take the ram hit.

I'd say the general case is store bitstream in RAM we can add
optimalizations later.

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel