Re: [PATCH v3 14/16] mmc: tmio: move TMIO_MASK_{READOP,WRITEOP} handling to correct place

[Masahiro Yamada]

2018-02-08 6:47 GMT+09:00 Wolfram Sang :
> On Thu, Jan 18, 2018 at 01:28:14AM +0900, Masahiro Yamada wrote:
>> As far as I tested the IP on UniPhier SoCs, TMIO_STAT_{RXRDY,TXRQ}
>> are asserted for DMA mode as well as for PIO.  I need to disable the
>> those IRQs in dma_ops->start hook, otherwise the DMA transfer fails
>> with the following error message:
>>   PIO IRQ in DMA mode!
>>
>> Renesas chips are the same cases since I see their dma_ops->start
>> hooks explicitly clear TMIO_STAT_{RXRDY,TXRQ} (with nice comment!).
>>
>> If we do this sanity check in TMIO MMC core, RXRDY/TXRQ handling
>> should be entirely moved to the core.  tmio_mmc_cmd_irq() will
>> be a suitable place to disable them.
>>
>> The probe function sets TMIO_MASK_{READOP,WRITEOP} but this is odd.
>>
>> /* Unmask the IRQs we want to know about */
>> if (!_host->chan_rx)
>> irq_mask |= TMIO_MASK_READOP;
>> if (!_host->chan_tx)
>> irq_mask |= TMIO_MASK_WRITEOP;
>>
>> At this point, _host->{chan_rx,chan_tx} are _always_ NULL because
>> tmio_mmc_request_dma() is called after this code.  Consequently,
>> TMIO_MASK_{READOP,WRITEOP} are set here whether DMA is used or not.
>> Remove this pointless code.
>>
>> Signed-off-by: Masahiro Yamada 
>
> I need to stop reviewing here because I'd need the applied version for
> checking. I hope Ulf can give me a base tomorrow.
>
> Or Yamada-san, do you meanwhile have a git repo somewhere?
>

Patch 1-6 were pulled merged in this MW.

Patch 7-16 are cleanly applicable onto Linus' tree.
(commit 581e400ff935d34 as of writing)



-- 
Best Regards
Masahiro Yamada

Re: [PATCH v3 12/16] mmc: tmio: support IP-builtin card detection logic

[Masahiro Yamada]

2018-02-08 4:34 GMT+09:00 Wolfram Sang :
> On Thu, Jan 18, 2018 at 01:28:12AM +0900, Masahiro Yamada wrote:
>> A card detect GPIO is set up only for platforms with "cd-gpios"
>> DT property or TMIO_MMC_USE_GPIO_CD flag.  However, the driver
>> core always uses mmc_gpio_get_cd, which just fails with -ENOSYS
>> if ctx->cd_gpio is unset.
>>
>> The bit 5 of the status register provides the current signal level
>> of the CD line.  Allow to use it if the GPIO is unused.
>>
>> Signed-off-by: Masahiro Yamada 
>
> Reviewed-by: Wolfram Sang 
>
>> @@ -1095,7 +1103,7 @@ static const struct mmc_host_ops tmio_mmc_ops = {
>
> I just wonder why the diff-tool puts 'const' in the definition. There is
> no const in my version here. And there shouldn't be because we modify
> the struct in this patch.
>

Which kernel version are you seeing?

The following patch was applied and it is in Linus'tree.



commit c055fc75c1757b220108489038cfe60496b13865
Author: Masahiro Yamada 
Date:   Sat Nov 25 01:24:41 2017 +0900

mmc: tmio: move mmc_host_ops to struct tmio_mmc_host from static data

Currently, tmio_mmc_ops is static data and tmio_mmc_host_probe()
updates some hooks in the static data.  This is a problem when
two or more instances call tmio_mmc_host_probe() and each of them
requests to use its own card_busy/start_signal_voltage_switch.

We can borrow a solution from sdhci_alloc_host().  Copy the whole
ops structure to host->mmc_host_ops, then override the hooks in
malloc'ed data.  Constify tmio_mmc_ops since it is now a template
ops used by default.

Signed-off-by: Masahiro Yamada 
Reviewed-by: Wolfram Sang 
Tested-by: Wolfram Sang 
Signed-off-by: Ulf Hansson 



-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/2] drm: adv7511: Add support for i2c_new_secondary_device

[Kieran Bingham]

Hi Archit,

On 07/02/18 12:33, Kieran Bingham wrote:
> Hi Archit,
> 
> Thank you for your review,
> 



>>>   unsigned int val;
>>>   int ret;
>>>   @@ -1153,24 +1151,35 @@ static int adv7511_probe(struct i2c_client *i2c,
>>> const struct i2c_device_id *id)
>>>   if (ret)
>>>   goto uninit_regulators;
>>>   -    regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, 
>>> edid_i2c_addr);
>>> -    regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
>>> - main_i2c_addr - 0xa);
> 
> 
> Packet address here is written as an offset of -0x0a, which gives 0x2f or 
> 0x33 ... ?
> 
> I think these current offsets are platform specific to a specific platform :)

I appear to be using platform specific maths specific to a non-conforming
platform or universe :-)

Sorry for the incorrect assertion here. - I mixed up 8bit and 7bit values.

The offsets are valid (when calculated correctly) - however - as per my reply to
Laurent - I believe the patch which determined the offsets was using the wrong
base :).

Also - due to a hardware issue on the ADV7511 - the Wheat (as the only known
user of two chips on the same bus) will need to be updated to ensure the current
assignments don't conflict.

--
Kieran

Re: [PATCH 2/2] drm: adv7511: Add support for i2c_new_secondary_device

[Kieran Bingham]

Hi Laurent,

On 07/02/18 21:59, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Wednesday, 7 February 2018 17:14:09 EET Kieran Bingham wrote:
>> On 29/01/18 10:26, Laurent Pinchart wrote:
>>> On Monday, 22 January 2018 14:50:00 EET Kieran Bingham wrote:
 The ADV7511 has four 256-byte maps that can be accessed via the main I²C
 ports. Each map has it own I²C address and acts as a standard slave
 device on the I²C bus.

 Allow a device tree node to override the default addresses so that
 address conflicts with other devices on the same bus may be resolved at
 the board description level.

 Signed-off-by: Kieran Bingham 
 ---

  .../bindings/display/bridge/adi,adv7511.txt| 10 +-
>>>
>>> I don't mind personally, but device tree maintainers usually ask for DT
>>> bindings changes to be split to a separate patch.
>>>
  drivers/gpu/drm/bridge/adv7511/adv7511.h   |  4 +++
  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c   | 36 ++-
  3 files changed, 37 insertions(+), 13 deletions(-)

 diff --git
 a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
 b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
 index 0047b1394c70..f6bb9f6d3f48 100644
 --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
 +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt

 @@ -70,6 +70,9 @@ Optional properties:
rather than generate its own timings for HDMI output.
  
  - clocks: from common clock binding: reference to the CEC clock.
  - clock-names: from common clock binding: must be "cec".

 +- reg-names : Names of maps with programmable addresses.
 +  It can contain any map needing a non-default address.
 +  Possible maps names are : "main", "edid", "cec", "packet"
>>>
>>> Is the reg-names property (and the additional maps) mandatory or optional
>>> ? If mandatory you should also update the existing DT sources that use
>>> those bindings.
>>
>> They are currently optional. I do prefer it that way - but perhaps due to an
>> issue mentioned below we might need to make this driver mandatory ?
>>
>>> If optional you should define which I2C addresses will be used when
>>> the maps are not specified (and in that case I think we should go for
>>> the addresses listed as default in the datasheet, which correspond to the
>>> current driver implementation when the main address is 0x3d/0x7a).
>>
>> The current addresses do not correspond to the datasheet, even when the
>> implementation main address is set to 0x3d.
> 
> Don't they ? The driver currently uses the following (8-bit) I2C addresses:
> 
> EDID:   main + 4  = 0x7e (0x3f)
> Packet: main - 10 = 0x70 (0x38)
> CEC:main - 2  = 0x78 (0x3c)
> 
> Those are the default addresses according to section 4.1 of the ADV7511W 
> programming guide (rev. B), and they match the ones you set in this patch.

Sorry - I was clearly subtracting 8bit values from a 7bit address in my failed
assertion, to both you and Archit.



>> Thus, in my opinion - they are currently 'wrong' - but perhaps changing them
>> is considered breakage too.
>>
>> A particular issue will arise here too - as on this device - when the device
>> is in low-power mode (after probe, before use) - the maps will respond on
>> their *hardware default* addresses (the ones implemented in this patch),
>> and thus consume that address on the I2C bus regardless of their settings
>> in the driver.
> 
> We've discussed this previously and I share you concern. Just to make sure I 
> remember correctly, did all the secondary maps reset to their default 
> addresses, or just the EDID map ?


The following responds on the bus when programmed at alternative addresses, and
in low power mode. The responses are all 0, but that's still going to conflict
with other hardware if it tries to use the 'un-used' addresses.

Packet (0x38),
Main (0x39),
Fixed (set to 0 by software, but shows up at 0x3e)
and EDID (0x3f).

So actually it's only the CEC which don't respond when in "low-power-mode".


As far as I can see, (git grep  -B3 adi,adv75) - The r8a7792-wheat.dts is the
only instance of a device using 0x3d, which means that Sergei's patch changed
the behaviour of all the existing devices before that.

Thus - this patch could be seen to be a 'correction' back to the original
behaviour for all devices except the Wheat, and possibly devices added after
Sergei's patch went in.

However - by my understanding, - any device which has only one ADV75(3,1)+
should use the hardware defined addresses (the hardware defaults will be
conflicting on the bus anyway, thus should be assigned to the ADV7511)

Any platform which uses *two* ADV7511 devices on the same bus should actually
set *both* devices to use entirely separate addresses - or they will still
conflict with each other.

Now - if my understanding is correct 

Re: [RFC v4 26/26] ARM: dts: iwg22m: Add watchdog support to SoM dtsi

[Wolfram Sang]

On Wed, Jan 31, 2018 at 06:24:30PM +, Fabrizio Castro wrote:
> This patch enables the watchdog from within the iwg20m SoM dtsi.
> 
> Signed-off-by: Fabrizio Castro 
> Signed-off-by: Ramesh Shanmugasundaram 
> 
> Reviewed-by: Geert Uytterhoeven 

Reviewed-by: Wolfram Sang 



signature.asc
Description: PGP signature

Re: [RFC v4 25/26] ARM: dts: iwg20m: Add watchdog support to SoM dtsi

[Wolfram Sang]

On Wed, Jan 31, 2018 at 06:24:29PM +, Fabrizio Castro wrote:
> This patch enables the watchdog from within the iwg20m SoM dtsi.
> 
> Signed-off-by: Fabrizio Castro 
> Signed-off-by: Ramesh Shanmugasundaram 
> 
> Reviewed-by: Geert Uytterhoeven 

Reviewed-by: Wolfram Sang 



signature.asc
Description: PGP signature

Re: [RFC v4 24/26] ARM: dts: r8a7794: Add watchdog support to SoC dtsi

[Wolfram Sang]

On Wed, Jan 31, 2018 at 06:24:28PM +, Fabrizio Castro wrote:
> This commit adds watchdog support to the r8a7794 dtsi.
> 
> Signed-off-by: Fabrizio Castro 
> Signed-off-by: Ramesh Shanmugasundaram 
> 
> Reviewed-by: Geert Uytterhoeven 

Acked-by: Wolfram Sang 



signature.asc
Description: PGP signature

Re: [RFC v4 23/26] ARM: dts: r8a7791: Add watchdog support to SoC dtsi

[Wolfram Sang]

On Wed, Jan 31, 2018 at 06:24:27PM +, Fabrizio Castro wrote:
> This commit adds watchdog support to the r8a7791 dtsi.
> 
> Signed-off-by: Fabrizio Castro 
> Signed-off-by: Ramesh Shanmugasundaram 
> 
> Reviewed-by: Geert Uytterhoeven 

Acked-by: Wolfram Sang 



signature.asc
Description: PGP signature

Re: [RFC v4 22/26] ARM: dts: r8a7790: Add watchdog support to SoC dtsi

[Wolfram Sang]

On Wed, Jan 31, 2018 at 06:24:26PM +, Fabrizio Castro wrote:
> This commit adds watchdog support to the r8a7790 dtsi.
> 
> Signed-off-by: Fabrizio Castro 
> Signed-off-by: Ramesh Shanmugasundaram 
> 
> Reviewed-by: Geert Uytterhoeven 

Acked-by: Wolfram Sang 



signature.asc
Description: PGP signature

Re: [RFC v4 21/26] ARM: dts: r8a7745: Add watchdog support to SoC dtsi

[Wolfram Sang]

On Wed, Jan 31, 2018 at 06:24:25PM +, Fabrizio Castro wrote:
> This patch adds watchdog support to the r8a7745 SoC dtsi.
> 
> Signed-off-by: Fabrizio Castro 
> Signed-off-by: Ramesh Shanmugasundaram 
> 
> Reviewed-by: Geert Uytterhoeven 

Acked-by: Wolfram Sang 



signature.asc
Description: PGP signature

Re: [RFC v4 20/26] ARM: dts: r8a7743: Add watchdog support to SoC dtsi

[Wolfram Sang]

On Wed, Jan 31, 2018 at 06:24:24PM +, Fabrizio Castro wrote:
> This patch adds watchdog support to the r8a7743 SoC dtsi.
> 
> Signed-off-by: Fabrizio Castro 
> Signed-off-by: Ramesh Shanmugasundaram 
> 
> Reviewed-by: Geert Uytterhoeven 

Acked-by: Wolfram Sang 



signature.asc
Description: PGP signature

Re: [RFC v4 19/26] clk: renesas: r8a7794: Add rwdt clock

[Wolfram Sang]

On Wed, Jan 31, 2018 at 06:24:23PM +, Fabrizio Castro wrote:
> Add "rwdt" clock to r8a7794_mod_clks. Also, since we may need to access
> the watchdog registers at any time, declare the clock as critical.
> 
> Signed-off-by: Fabrizio Castro 
> Signed-off-by: Ramesh Shanmugasundaram 
> 

Reviewed-by: Wolfram Sang 



signature.asc
Description: PGP signature

Re: [RFC v4 18/26] clk: renesas: r8a7791/r8a7793: Add rwdt clock

[Wolfram Sang]

On Wed, Jan 31, 2018 at 06:24:22PM +, Fabrizio Castro wrote:
> Add "rwdt" clock to r8a7791_mod_clks. Also, since we may need to access
> the watchdog registers at any time, declare the clock as critical.
> 
> Signed-off-by: Fabrizio Castro 
> Signed-off-by: Ramesh Shanmugasundaram 
> 

Reviewed-by: Wolfram Sang 



signature.asc
Description: PGP signature

Re: [RFC v4 17/26] clk: renesas: r8a7790: Add rwdt clock

[Wolfram Sang]

On Wed, Jan 31, 2018 at 06:24:21PM +, Fabrizio Castro wrote:
> Add "rwdt" clock to r8a7790_mod_clks. Also, since we may need to access
> the watchdog registers at any time, declare the clock as critical.
> 
> Signed-off-by: Fabrizio Castro 
> Signed-off-by: Ramesh Shanmugasundaram 
> 

Reviewed-by: Wolfram Sang 



signature.asc
Description: PGP signature

Re: [RFC v4 16/26] clk: renesas: r8a7745: Add rwdt clock

[Wolfram Sang]

On Wed, Jan 31, 2018 at 06:24:20PM +, Fabrizio Castro wrote:
> Add "rwdt" clock to r8a7745_mod_clks. Also, since we may need to access
> the watchdog registers at any time, declare the clock as critical.
> 
> Signed-off-by: Fabrizio Castro 
> Signed-off-by: Ramesh Shanmugasundaram 
> 

Reviewed-by: Wolfram Sang 



signature.asc
Description: PGP signature

Re: [RFC v4 15/26] clk: renesas: r8a7743: Add rwdt clock

[Wolfram Sang]

On Wed, Jan 31, 2018 at 06:24:19PM +, Fabrizio Castro wrote:
> Add "rwdt" clock to r8a7743_mod_clks. Also, since we may need to access
> the watchdog registers at any time, declare the clock as critical.
> 
> Signed-off-by: Fabrizio Castro 
> Signed-off-by: Ramesh Shanmugasundaram 
> 

Reviewed-by: Wolfram Sang 



signature.asc
Description: PGP signature

Re: [RFC v4 14/26] ARM: shmobile: defconfig: Enable RENESAS_WDT_GEN

[Wolfram Sang]

On Wed, Jan 31, 2018 at 06:24:18PM +, Fabrizio Castro wrote:
> R-Car Gen2 and RZ/G1 platforms come with a watchdog IP, therefore enable
> its driver by default.
> 
> Signed-off-by: Fabrizio Castro 
> Signed-off-by: Ramesh Shanmugasundaram 
> 

Given the rest of the series works (same for the later patches):

Reviewed-by: Wolfram Sang 



signature.asc
Description: PGP signature

Re: [RFC v4 11/26] dt-bindings: watchdog: renesas-wdt: Add R-Car Gen2 support

[Wolfram Sang]

On Wed, Jan 31, 2018 at 06:24:15PM +, Fabrizio Castro wrote:
> This commit documents the compatibility with R-Car Gen2 and RZ/G
> devices by defining the generic compatible string "renesas,rcar-gen2-wdt".
> Also, this patch expands the list of SoC-specific compatible strings to
> include RZ/G and R-Car Gen2 devices.
> 
> Signed-off-by: Fabrizio Castro 
> Signed-off-by: Ramesh Shanmugasundaram 
> 
> Reviewed-by: Geert Uytterhoeven 

Reviewed-by: Wolfram Sang 



signature.asc
Description: PGP signature

Re: [RFC v4 13/26] watchdog: renesas_wdt: Add restart handler

[Wolfram Sang]

> +static int rwdt_restart(struct watchdog_device *wdev, unsigned long action,
> + void *data)
> +{
> + struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
> +
> + pm_runtime_get_sync(wdev->parent);
> +
> + rwdt_write(priv, 0x00, RWTCSRB);
> + rwdt_write(priv, 0x00, RWTCSRA);
> + rwdt_write(priv, 0x, RWTCNT);
> +
> + while (readb_relaxed(priv->base + RWTCSRA) & RWTCSRA_WRFLG)
> + cpu_relax();
> +
> + rwdt_write(priv, 0x80, RWTCSRA);
> + return 0;
> +}

I used to have this simpler version (back then for the UP case):

+   rwdt_start(wdev);
+   rwdt_write(priv, 0x, RWTCNT);
+   return 0;

It should still work probably...



signature.asc
Description: PGP signature

Re: [RFC v4 12/26] watchdog: renesas_wdt: Add R-Car Gen2 support

[Wolfram Sang]

> +#ifdef CONFIG_PM
> +static int rwdt_suspend(struct device *dev)
> +{
> + struct platform_device *pdev;
> + struct rwdt_priv *priv;
> +
> + pdev = to_platform_device(dev);
> + priv = platform_get_drvdata(pdev);

struct rwdt_priv *priv = dev_get_drvdata(dev);

?


> + if (watchdog_active(>wdev)) {
> + rwdt_write(priv, priv->cks, RWTCSRA);
> + }
> + return 0;
> +}
> +
> +static int rwdt_resume(struct device *dev)
> +{
> + struct platform_device *pdev;
> + struct rwdt_priv *priv;
> +
> + pdev = to_platform_device(dev);
> + priv = platform_get_drvdata(pdev);

ditto

> + if (watchdog_active(>wdev)) {
> + rwdt_write(priv, priv->cks | RWTCSRA_TME, RWTCSRA);
> + }
> + return 0;
> +}
> +
> +static const struct dev_pm_ops rwdt_pm = {
> + .suspend = rwdt_suspend,
> + .resume = rwdt_resume,
> +};

Use SIMPLE_DEV_PM_OPS macro (see stmp3xxx_rtc_wdt.c for an example) for
more compact code and...

> +#ifdef CONFIG_PM
> + .pm = _pm,
> +#endif

... removing this ifdeffery?



signature.asc
Description: PGP signature

Re: [PATCH v2 0/3] mmc: renesas_sdhi: add eMMC HS400 mode support

[Wolfram Sang]

> * [PATCH] clk: renesas: rcar-gen3: Fix SD divider setting

I tried to address this one before:

[PATCH 0/3] clk: renesas: rcar-gen3-cpg: updates to SD divider table

but this had problems with H3 ES1.0 :(

I haven't yet compared the above dependency patch with what I did back
then. Will need to do that hopefully tomorrow. But as I read it, it
worked fine for you on H3 ES1.0?



signature.asc
Description: PGP signature

Re: [PATCH v2 3/3] mmc: renesas_sdhi: add eMMC HS400 mode support

[Wolfram Sang]

> + /* Reset HS400 mode */
> + sd_ctrl_write16(host, CTL_SDIF_MODE, ~0x0001 &
> + sd_ctrl_read16(host, CTL_SDIF_MODE));
> + sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TMPPORT2,
> +~(SH_MOBILE_SDHI_SCC_TMPPORT2_HS400EN |
> +  SH_MOBILE_SDHI_SCC_TMPPORT2_HS400OSEL) &
> + sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_TMPPORT2));
> +

This looks like code duplication. Can't we simply call
renesas_sdhi_reset_hs400_mode()?



signature.asc
Description: PGP signature

Re: [PATCH v2 2/3] mmc: tmio: add eMMC HS400 mode support

[Wolfram Sang]

Hi Simon,

> + void (*disable_scc)(struct mmc_host *mmc);

Do we really need this callback? I'd think it can be folded into
reset_hs400_mode() because it is called only once?

> + void (*prepare_hs400_tuning)(struct mmc_host *mmc, struct mmc_ios *ios);

Can't we use the host->ops->prepare_hs400_tuning() callback invoked by
the core?

> + void (*reset_hs400_mode)(struct mmc_host *mmc);

Maybe we can get rid of this, too? See later...

> + if (host->disable_scc)
> + host->disable_scc(mmc);

(Here, this can be folded into the next callback)

> +
> + /* reset HS400 mode */
> + if (ios->timing != MMC_TIMING_MMC_HS400 && host->reset_hs400_mode)
> + host->reset_hs400_mode(mmc);

I wonder: If for any ios which is != MMC_TIMING_MMC_HS400, the
hs400_mode needs to be reset. Couldn't we as well then disable the mode
always after the MMC_TIMING_MMC_HS400 tuning was selected? Just
brainstorming here...

Regards,

   Wolfram



signature.asc
Description: PGP signature

Re: [PATCH 2/2] drm: adv7511: Add support for i2c_new_secondary_device

[Laurent Pinchart]

Hi Kieran,

On Wednesday, 7 February 2018 17:14:09 EET Kieran Bingham wrote:
> On 29/01/18 10:26, Laurent Pinchart wrote:
> > On Monday, 22 January 2018 14:50:00 EET Kieran Bingham wrote:
> >> The ADV7511 has four 256-byte maps that can be accessed via the main I²C
> >> ports. Each map has it own I²C address and acts as a standard slave
> >> device on the I²C bus.
> >> 
> >> Allow a device tree node to override the default addresses so that
> >> address conflicts with other devices on the same bus may be resolved at
> >> the board description level.
> >> 
> >> Signed-off-by: Kieran Bingham 
> >> ---
> >> 
> >>  .../bindings/display/bridge/adi,adv7511.txt| 10 +-
> > 
> > I don't mind personally, but device tree maintainers usually ask for DT
> > bindings changes to be split to a separate patch.
> > 
> >>  drivers/gpu/drm/bridge/adv7511/adv7511.h   |  4 +++
> >>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c   | 36 ++-
> >>  3 files changed, 37 insertions(+), 13 deletions(-)
> >> 
> >> diff --git
> >> a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> >> b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> >> index 0047b1394c70..f6bb9f6d3f48 100644
> >> --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> >> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> >> 
> >> @@ -70,6 +70,9 @@ Optional properties:
> >>rather than generate its own timings for HDMI output.
> >>  
> >>  - clocks: from common clock binding: reference to the CEC clock.
> >>  - clock-names: from common clock binding: must be "cec".
> >> 
> >> +- reg-names : Names of maps with programmable addresses.
> >> +  It can contain any map needing a non-default address.
> >> +  Possible maps names are : "main", "edid", "cec", "packet"
> > 
> > Is the reg-names property (and the additional maps) mandatory or optional
> > ? If mandatory you should also update the existing DT sources that use
> > those bindings.
> 
> They are currently optional. I do prefer it that way - but perhaps due to an
> issue mentioned below we might need to make this driver mandatory ?
> 
> > If optional you should define which I2C addresses will be used when
> > the maps are not specified (and in that case I think we should go for
> > the addresses listed as default in the datasheet, which correspond to the
> > current driver implementation when the main address is 0x3d/0x7a).
> 
> The current addresses do not correspond to the datasheet, even when the
> implementation main address is set to 0x3d.

Don't they ? The driver currently uses the following (8-bit) I2C addresses:

EDID:   main + 4  = 0x7e (0x3f)
Packet: main - 10 = 0x70 (0x38)
CEC:main - 2  = 0x78 (0x3c)

Those are the default addresses according to section 4.1 of the ADV7511W 
programming guide (rev. B), and they match the ones you set in this patch.

> Thus, in my opinion - they are currently 'wrong' - but perhaps changing them
> is considered breakage too.
> 
> A particular issue will arise here too - as on this device - when the device
> is in low-power mode (after probe, before use) - the maps will respond on
> their *hardware default* addresses (the ones implemented in this patch),
> and thus consume that address on the I2C bus regardless of their settings
> in the driver.

We've discussed this previously and I share you concern. Just to make sure I 
remember correctly, did all the secondary maps reset to their default 
addresses, or just the EDID map ?

> > You should also update the definition of the reg property that currently
> > just states
> > 
> > - reg: I2C slave address
> > 
> > And finally you might want to define the term "map" in this context.
> > Here's a proposal (if we make all maps mandatory).
> > 
> > The ADV7511 internal registers are split into four pages exposed through
> > different I2C addresses, creating four register maps. The I2C addresses of
> > all four maps shall be specified by the reg and reg-names property.
> > 
> > - reg: I2C slave addresses, one per reg-names entry
> > - reg-names: map names, shall be "main", "edid", "cec", "packet"
> > 
> >>  Required nodes:
> >> @@ -88,7 +91,12 @@ Example
> >> 
> >>adv7511w: hdmi@39 {
> >>compatible = "adi,adv7511w";
> >> -  reg = <39>;
> >> +  /*
> >> +   * The EDID page will be accessible on address 0x66 on the i2c
> >> +   * bus. All other maps continue to use their default addresses.
> >> +   */
> >> +  reg = <0x39 0x66>;
> >> +  reg-names = "main", "edid";
> >>interrupt-parent = <>;
> >>interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
> >>clocks = <_clock>;

[snip]

> >> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> >> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> >> index efa29db5fc2b..7ec33837752b 100644
> >> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> >> +++ 

Re: [PATCH v2 1/3] mmc: tmio: correct treatment of errors during tuning

[Wolfram Sang]

On Fri, Jan 19, 2018 at 02:39:04PM +0100, Simon Horman wrote:
> From: Masaharu Hayakawa 
> 
> If the return value of mmc_send_tuning() is error other than -EILSEQ, the
> tuning fails and process goes out of for_loop.  But the correct processing
> is to judge their TAP as bad.

Ideally, we would have more specific reasons why this is correct processing.

What other codes could happen here?

> Signed-off-by: Masaharu Hayakawa 
> Signed-off-by: Simon Horman 
> ---
> v2 [Simon Horman]
> * Added to patchset targeted at upstream
> * Minor revision of changelog
> 
> v0 [Masaharu Hayakawa]
> ---
>  drivers/mmc/host/tmio_mmc_core.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/host/tmio_mmc_core.c 
> b/drivers/mmc/host/tmio_mmc_core.c
> index 6d8719be75a8..41767d33ef97 100644
> --- a/drivers/mmc/host/tmio_mmc_core.c
> +++ b/drivers/mmc/host/tmio_mmc_core.c
> @@ -800,10 +800,7 @@ static int tmio_mmc_execute_tuning(struct mmc_host *mmc, 
> u32 opcode)
>   if (host->prepare_tuning)
>   host->prepare_tuning(host, i % host->tap_num);
>  
> - ret = mmc_send_tuning(mmc, opcode, NULL);
> - if (ret && ret != -EILSEQ)
> - goto out;
> - if (ret == 0)
> + if (!mmc_send_tuning(mmc, opcode, NULL))

I'd prefer (mmc_send_tuning() == 0) here instead of '!mmc_send_tuning()'.
This reads as 'is ok' while the other reads more 'if not ok'.

>   set_bit(i, host->taps);
>  
>   usleep_range(1000, 1200);
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

Re: [PATCH v3 14/16] mmc: tmio: move TMIO_MASK_{READOP,WRITEOP} handling to correct place

[Wolfram Sang]

On Thu, Jan 18, 2018 at 01:28:14AM +0900, Masahiro Yamada wrote:
> As far as I tested the IP on UniPhier SoCs, TMIO_STAT_{RXRDY,TXRQ}
> are asserted for DMA mode as well as for PIO.  I need to disable the
> those IRQs in dma_ops->start hook, otherwise the DMA transfer fails
> with the following error message:
>   PIO IRQ in DMA mode!
> 
> Renesas chips are the same cases since I see their dma_ops->start
> hooks explicitly clear TMIO_STAT_{RXRDY,TXRQ} (with nice comment!).
> 
> If we do this sanity check in TMIO MMC core, RXRDY/TXRQ handling
> should be entirely moved to the core.  tmio_mmc_cmd_irq() will
> be a suitable place to disable them.
> 
> The probe function sets TMIO_MASK_{READOP,WRITEOP} but this is odd.
> 
> /* Unmask the IRQs we want to know about */
> if (!_host->chan_rx)
> irq_mask |= TMIO_MASK_READOP;
> if (!_host->chan_tx)
> irq_mask |= TMIO_MASK_WRITEOP;
> 
> At this point, _host->{chan_rx,chan_tx} are _always_ NULL because
> tmio_mmc_request_dma() is called after this code.  Consequently,
> TMIO_MASK_{READOP,WRITEOP} are set here whether DMA is used or not.
> Remove this pointless code.
> 
> Signed-off-by: Masahiro Yamada 

I need to stop reviewing here because I'd need the applied version for
checking. I hope Ulf can give me a base tomorrow.

Or Yamada-san, do you meanwhile have a git repo somewhere?



signature.asc
Description: PGP signature

[PATCH] media: i2c: adv748x: Fix cleanup jump on chip identification

[Kieran Bingham]

From: Kieran Bingham 

The error handling for the adv748x_identify_chip() call erroneously
jumps to the err_cleanup_clients label before the clients have been
established.

Correct this by jumping to the next (and correct) label in the cleanup
code: err_cleanup_dt.

Fixes: 3e89586a64df ("media: i2c: adv748x: add adv748x driver")

Signed-off-by: Kieran Bingham 
---
 drivers/media/i2c/adv748x/adv748x-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c 
b/drivers/media/i2c/adv748x/adv748x-core.c
index 6d62b817ed00..6ccaad7e9eca 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -651,7 +651,7 @@ static int adv748x_probe(struct i2c_client *client,
ret = adv748x_identify_chip(state);
if (ret) {
adv_err(state, "Failed to identify chip");
-   goto err_cleanup_clients;
+   goto err_cleanup_dt;
}
 
/* Configure remaining pages as I2C clients with regmap access */
-- 
2.7.4

Re: [PATCH v3 13/16] mmc: tmio: fix never-detected card insertion bug

[Wolfram Sang]

On Thu, Jan 18, 2018 at 01:28:13AM +0900, Masahiro Yamada wrote:
> The TMIO mmc cannot detect the card insertion in native_hotplug mode
> if the driver is probed without a card inserted.
> 
> The reason is obvious; all IRQs are disabled by tmio_mmc_host_probe(),
> as follows:
> 
>   tmio_mmc_disable_mmc_irqs(_host, TMIO_MASK_ALL);
> 
> The card event IRQs are first enabled by tmio_mmc_start_command() as
> follows:
> 
>   if (!host->native_hotplug)
>   irq_mask &= ~(TMIO_STAT_CARD_REMOVE | TMIO_STAT_CARD_INSERT);
>   tmio_mmc_enable_mmc_irqs(host, irq_mask);
> 
> If the driver is probed without a card, tmio_mmc_start_command() is
> never called in the first place.  So, the card is never detected.
> 
> The card event IRQs must be enabled in probe/resume functions.
> 
> Signed-off-by: Masahiro Yamada 

Reviewed-by: Wolfram Sang 



signature.asc
Description: PGP signature

Re: [PATCH v3 12/16] mmc: tmio: support IP-builtin card detection logic

[Wolfram Sang]

On Thu, Jan 18, 2018 at 01:28:12AM +0900, Masahiro Yamada wrote:
> A card detect GPIO is set up only for platforms with "cd-gpios"
> DT property or TMIO_MMC_USE_GPIO_CD flag.  However, the driver
> core always uses mmc_gpio_get_cd, which just fails with -ENOSYS
> if ctx->cd_gpio is unset.
> 
> The bit 5 of the status register provides the current signal level
> of the CD line.  Allow to use it if the GPIO is unused.
> 
> Signed-off-by: Masahiro Yamada 

Reviewed-by: Wolfram Sang 

> @@ -1095,7 +1103,7 @@ static const struct mmc_host_ops tmio_mmc_ops = {

I just wonder why the diff-tool puts 'const' in the definition. There is
no const in my version here. And there shouldn't be because we modify
the struct in this patch.



signature.asc
Description: PGP signature

Re: [PATCH v3 11/16] mmc: tmio: deprecate "toshiba,mmc-wrprotect-disable" DT property

[Wolfram Sang]

On Thu, Jan 18, 2018 at 10:58:36AM +0900, Masahiro Yamada wrote:
> 2018-01-18 1:28 GMT+09:00 Masahiro Yamada :
> > This property is equivalent to "disable-wp" defined in
> > Documentation/devicetree/bindings/mmc/tmio_mmc.txt
> 
> This is mistake.
> 
> "disable-wp" is defined in
> 
> Documentation/devicetree/bindings/mmc/mmc.txt

With that fixed:

Reviewed-by: Wolfram Sang 



signature.asc
Description: PGP signature

Re: [PATCH v3 10/16] mmc: tmio: remove TMIO_MMC_WRPROTECT_DISABLE

[Wolfram Sang]

On Thu, Jan 18, 2018 at 01:28:10AM +0900, Masahiro Yamada wrote:
> The use of this flag has been replaced with MMC_CAP2_NO_WRITE_PROTECT.
> No platform defines this flag any more.  Remove.
> 
> Signed-off-by: Masahiro Yamada 

Reviewed-by: Wolfram Sang 



signature.asc
Description: PGP signature

Re: [PATCH v3 09/16] mmc: tmio: use MMC_CAP2_NO_WRITE_PROTECT instead of TMIO own flag

[Wolfram Sang]

On Thu, Jan 18, 2018 at 01:28:09AM +0900, Masahiro Yamada wrote:
> TMIO_MMC_WRPROTECT_DISABLE is equivalent to MMC_CAP2_NO_WRITE_PROTECT.
> 
> Only the difference is the TMIO_... makes tmio_mmc_get_ro() return 0
> (i.e. it does not affect mmc_gpio_get_ro() at all), while MMC_CAP2_...
> returns 0 before calling ->get_ro() hook (i.e. it affects both IP own
> logic and GPIO detection).
> 
> The TMIO MMC drivers do not set-up gpio_ro by themselves.  Only the
> possibility, if any, would be DT specifies "wp-gpios" property, and
> gpio_ro is set by mmc_gpiod_request_ro() called from mmc_of_parse().
> However, it does not make sense to specify "wp-gpios" property and
> "toshiba,mmc-wrprotect-disable" at the same time.
> 
> I checked under arch/arm/boot/dts/ and arch/arm64/boot/dts/renesas/,
> and I did not see any Renesas boards with "wp-gpios".  So, this
> conversion should be safe.
> 
> Signed-off-by: Masahiro Yamada 

Reviewed-by: Wolfram Sang 

>  static void tmio_mmc_of_parse(struct platform_device *pdev,
> -   struct tmio_mmc_data *pdata)
> +   struct mmc_host *mmc)

I like how this cleanups improves things in various parts of the code,
like here. Nice!



signature.asc
Description: PGP signature

Re: [PATCH v3 08/16] sh: kfr2r09: use MMC_CAP2_NO_WRITE_PROTECT instead of TMIO own flag

[Wolfram Sang]

On Thu, Jan 18, 2018 at 01:28:08AM +0900, Masahiro Yamada wrote:
> TMIO_MMC_WRPROTECT_DISABLE is equivalent to MMC_CAP2_NO_WRITE_PROTECT.
> 
> The flag is propagated as follows:
> tmio_mmc_data::capabilities2 -> mmc_host::caps2
> 
> Only the difference is the TMIO_... makes tmio_mmc_get_ro() return 0
> (i.e. it does not affect mmc_gpio_get_ro() at all), while MMC_CAP2_...
> returns 0 before calling ->get_ro() hook (i.e. it affects both IP own
> logic and GPIO detection).
> 
> The TMIO MMC drivers do not set-up gpio_ro by themselves, so gpio_ro
> is obviously unused by legacy boards like this.  So, this conversion
> should be safe.
> 
> Signed-off-by: Masahiro Yamada 

Reviewed-by: Wolfram Sang 



signature.asc
Description: PGP signature

Re: [PATCH v3 00/16] mmc: tmio: another batch of TMIO MMC fixes and cleanups

[Wolfram Sang]

> I picked patch5 and patch6, as those seemed trivial. Unless there is a
> rc9, let's aim for 4.17 for the rest.

I can't find the branch where you picked these patches? I wanted to use
it to apply the rest of the patches. They don't seem to fit on v4.15
or current linus/master. Can you share this branch?

I will do reviewing now "visually". But please wait until I tested them
before applying.

Thanks,

   Wolfram



signature.asc
Description: PGP signature

Re: [PATCH v3 06/16] mmc: tmio: refactor .get_ro hook

[Wolfram Sang]

On Thu, Jan 18, 2018 at 01:28:06AM +0900, Masahiro Yamada wrote:
> This IP provides the write protect signal level in the status
> register, but it is also possible to use GPIO for WP.  They are
> exclusive, so it is not efficient to call mmc_gpio_get_ro() every
> time from tmio_mmc_get_ro() if we know gpio_ro is not used.
> 
> Check the capability of gpio_ro just once in the probe function,
> then set mmc_gpio_get_ro to .get_ro if it is the case.
> 
> Signed-off-by: Masahiro Yamada 

Reviewed-by: Wolfram Sang 



signature.asc
Description: PGP signature

Re: [PATCH v3 05/16] mmc: slot-gpio: add a helper to check capability of GPIO WP detection

[Wolfram Sang]

On Thu, Jan 18, 2018 at 01:28:05AM +0900, Masahiro Yamada wrote:
> Like mmc_can_gpio_cd(), mmc_can_gpio_ro() will also be useful for host
> drivers to know whether GPIO write-protect detection is supported.
> 
> Signed-off-by: Masahiro Yamada 

Reviewed-by: Wolfram Sang 



signature.asc
Description: PGP signature

[PATCH 1/2] media: i2c: adv748x: Simplify regmap configuration

[Kieran Bingham]

From: Kieran Bingham 

The ADV748x has identical map configurations for each register map. The
duplication of each map can be simplified using a helper macro such that
each map is represented on a single line.

Define ADV748X_REGMAP_CONF for this purpose and un-define after it's
use.

Signed-off-by: Kieran Bingham 
---
 drivers/media/i2c/adv748x/adv748x-core.c | 111 ++-
 1 file changed, 22 insertions(+), 89 deletions(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c 
b/drivers/media/i2c/adv748x/adv748x-core.c
index fd92c9e4b519..71c69b816db2 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -35,98 +35,31 @@
  * Register manipulation
  */
 
-static const struct regmap_config adv748x_regmap_cnf[] = {
-   {
-   .name   = "io",
-   .reg_bits   = 8,
-   .val_bits   = 8,
-
-   .max_register   = 0xff,
-   .cache_type = REGCACHE_NONE,
-   },
-   {
-   .name   = "dpll",
-   .reg_bits   = 8,
-   .val_bits   = 8,
-
-   .max_register   = 0xff,
-   .cache_type = REGCACHE_NONE,
-   },
-   {
-   .name   = "cp",
-   .reg_bits   = 8,
-   .val_bits   = 8,
-
-   .max_register   = 0xff,
-   .cache_type = REGCACHE_NONE,
-   },
-   {
-   .name   = "hdmi",
-   .reg_bits   = 8,
-   .val_bits   = 8,
-
-   .max_register   = 0xff,
-   .cache_type = REGCACHE_NONE,
-   },
-   {
-   .name   = "edid",
-   .reg_bits   = 8,
-   .val_bits   = 8,
-
-   .max_register   = 0xff,
-   .cache_type = REGCACHE_NONE,
-   },
-   {
-   .name   = "repeater",
-   .reg_bits   = 8,
-   .val_bits   = 8,
-
-   .max_register   = 0xff,
-   .cache_type = REGCACHE_NONE,
-   },
-   {
-   .name   = "infoframe",
-   .reg_bits   = 8,
-   .val_bits   = 8,
-
-   .max_register   = 0xff,
-   .cache_type = REGCACHE_NONE,
-   },
-   {
-   .name   = "cec",
-   .reg_bits   = 8,
-   .val_bits   = 8,
-
-   .max_register   = 0xff,
-   .cache_type = REGCACHE_NONE,
-   },
-   {
-   .name   = "sdp",
-   .reg_bits   = 8,
-   .val_bits   = 8,
-
-   .max_register   = 0xff,
-   .cache_type = REGCACHE_NONE,
-   },
-
-   {
-   .name   = "txb",
-   .reg_bits   = 8,
-   .val_bits   = 8,
-
-   .max_register   = 0xff,
-   .cache_type = REGCACHE_NONE,
-   },
-   {
-   .name   = "txa",
-   .reg_bits   = 8,
-   .val_bits   = 8,
+#define ADV748X_REGMAP_CONF(n) \
+{ \
+   .name = n, \
+   .reg_bits = 8, \
+   .val_bits = 8, \
+   .max_register = 0xff, \
+   .cache_type = REGCACHE_NONE, \
+}
 
-   .max_register   = 0xff,
-   .cache_type = REGCACHE_NONE,
-   },
+static const struct regmap_config adv748x_regmap_cnf[] = {
+   ADV748X_REGMAP_CONF("io"),
+   ADV748X_REGMAP_CONF("dpll"),
+   ADV748X_REGMAP_CONF("cp"),
+   ADV748X_REGMAP_CONF("hdmi"),
+   ADV748X_REGMAP_CONF("edid"),
+   ADV748X_REGMAP_CONF("repeater"),
+   ADV748X_REGMAP_CONF("infoframe"),
+   ADV748X_REGMAP_CONF("cec"),
+   ADV748X_REGMAP_CONF("sdp"),
+   ADV748X_REGMAP_CONF("txa"),
+   ADV748X_REGMAP_CONF("txb"),
 };
 
+#undef ADV748X_REGMAP_CONF
+
 static int adv748x_configure_regmap(struct adv748x_state *state, int region)
 {
int err;
-- 
2.7.4

[PATCH 2/2] media: i2c: adv748x: Add missing CBUS page.

[Kieran Bingham]

From: Kieran Bingham 

The ADV748x has 12 pages mapped onto I2C addresses.

In the existing implementation only 11 are mapped correctly in the page
enumerations, which causes an off-by-one fault on pages above the
infoframe definition due to a missing 'CBUS' page.

This causes the address for the CEC, SDP, TXA, and TXB to be incorrectly
programmed during the iterations in adv748x_initialise_clients().

Until now this has gone un-noticed due to the fact that following the
creation of the clients - the device is reset and the addresses are
reprogrammed in manually by the call to "adv748x_write_regs(state,
adv748x_set_slave_address);"

As part of moving to dynamic i2c address allocations repair this by
providing the missing CBUS page definition.

Signed-off-by: Kieran Bingham 
---
 drivers/media/i2c/adv748x/adv748x-core.c | 3 +++
 drivers/media/i2c/adv748x/adv748x.h  | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c 
b/drivers/media/i2c/adv748x/adv748x-core.c
index 71c69b816db2..6d62b817ed00 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -52,6 +52,7 @@ static const struct regmap_config adv748x_regmap_cnf[] = {
ADV748X_REGMAP_CONF("edid"),
ADV748X_REGMAP_CONF("repeater"),
ADV748X_REGMAP_CONF("infoframe"),
+   ADV748X_REGMAP_CONF("cbus"),
ADV748X_REGMAP_CONF("cec"),
ADV748X_REGMAP_CONF("sdp"),
ADV748X_REGMAP_CONF("txa"),
@@ -91,6 +92,7 @@ static int adv748x_i2c_addresses[ADV748X_PAGE_MAX] = {
ADV748X_I2C_EDID,
ADV748X_I2C_REPEATER,
ADV748X_I2C_INFOFRAME,
+   ADV748X_I2C_CBUS,
ADV748X_I2C_CEC,
ADV748X_I2C_SDP,
ADV748X_I2C_TXB,
@@ -354,6 +356,7 @@ static const struct adv748x_reg_value 
adv748x_set_slave_address[] = {
{ADV748X_PAGE_IO, 0xf6, ADV748X_I2C_EDID << 1},
{ADV748X_PAGE_IO, 0xf7, ADV748X_I2C_REPEATER << 1},
{ADV748X_PAGE_IO, 0xf8, ADV748X_I2C_INFOFRAME << 1},
+   {ADV748X_PAGE_IO, 0xf9, ADV748X_I2C_CBUS << 1},
{ADV748X_PAGE_IO, 0xfa, ADV748X_I2C_CEC << 1},
{ADV748X_PAGE_IO, 0xfb, ADV748X_I2C_SDP << 1},
{ADV748X_PAGE_IO, 0xfc, ADV748X_I2C_TXB << 1},
diff --git a/drivers/media/i2c/adv748x/adv748x.h 
b/drivers/media/i2c/adv748x/adv748x.h
index 6789e2f3bc8c..725662edc4b8 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -35,6 +35,7 @@
 #define ADV748X_I2C_EDID   0x36/* EDID Map */
 #define ADV748X_I2C_REPEATER   0x32/* HDMI RX Repeater Map */
 #define ADV748X_I2C_INFOFRAME  0x31/* HDMI RX InfoFrame Map */
+#define ADV748X_I2C_CBUS   0x30/* CBUS MHL Map */
 #define ADV748X_I2C_CEC0x41/* CEC Map */
 #define ADV748X_I2C_SDP0x79/* SDP Map */
 #define ADV748X_I2C_TXB0x48/* CSI-TXB Map */
@@ -48,6 +49,7 @@ enum adv748x_page {
ADV748X_PAGE_EDID,
ADV748X_PAGE_REPEATER,
ADV748X_PAGE_INFOFRAME,
+   ADV748X_PAGE_CBUS,
ADV748X_PAGE_CEC,
ADV748X_PAGE_SDP,
ADV748X_PAGE_TXB,
-- 
2.7.4

[PATCH 0/2] media: i2c: adv748x: Fix CBUS page issue

[Kieran Bingham]

From: Kieran Bingham 

The ADV748x has 12 pages mapped on to I2C addresses.

The existing implementation only has 11 of these in the map enumeration, and
this can cause an off-by-one error when programming the map addresses.

This short series simplifies the regmap configuration tables, and adds the
missing CBUS page to better model the device, and remove the off by one error.


Kieran Bingham (2):
  media: i2c: adv748x: Simplify regmap configuration
  media: i2c: adv748x: Add missing CBUS page.

 drivers/media/i2c/adv748x/adv748x-core.c | 114 +++
 drivers/media/i2c/adv748x/adv748x.h  |   2 +
 2 files changed, 27 insertions(+), 89 deletions(-)

-- 
2.7.4

Re: [PATCH 1/2] media: adv7604: Add support for i2c_new_secondary_device

[Kieran Bingham]

Hi Rob,

On 29/01/18 20:08, Rob Herring wrote:
> On Mon, Jan 22, 2018 at 12:49:56PM +, Kieran Bingham wrote:
>> From: Jean-Michel Hautbois 
>>
>> The ADV7604 has thirteen 256-byte maps that can be accessed via the main
>> I²C ports. Each map has it own I²C address and acts as a standard slave
>> device on the I²C bus.
>>
>> Allow a device tree node to override the default addresses so that
>> address conflicts with other devices on the same bus may be resolved at
>> the board description level.
>>
>> Signed-off-by: Jean-Michel Hautbois 
>> [Kieran: Re-adapted for mainline]
>> Signed-off-by: Kieran Bingham 
>> ---
>> Based upon the original posting :
>>   https://lkml.org/lkml/2014/10/22/469
>> ---
>>  .../devicetree/bindings/media/i2c/adv7604.txt  | 18 ++-
> 
> Reviewed-by: Rob Herring 

Thank you.

> In the future, please split bindings to separate patch.

Yes, of course - sorry - I should probably have known better here.

I was clearly being lazy as the original patch had bindings in with the driver.
Although I don't think I've got an excuse for the second patch in the series :D

I've split them out for the v2.

--
Kieran

>>  drivers/media/i2c/adv7604.c| 60 
>> ++
>>  2 files changed, 55 insertions(+), 23 deletions(-)
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

RE: [RFC v4 13/26] watchdog: renesas_wdt: Add restart handler

[Fabrizio Castro]

Hello Geert,

> Subject: Re: [RFC v4 13/26] watchdog: renesas_wdt: Add restart handler
>
> Hi Fabrizio,

Thank you for your feedback!

>
> On Wed, Jan 31, 2018 at 7:24 PM, Fabrizio Castro
>  wrote:
> > On iWave's boards iwg20d and iwg22d the only way to reboot the system is
> > by means of the watchdog.
> > This patch adds a restart handler to rwdt_ops, and also makes sure we
> > keep its priority to a medium level, in order to not override other more
> > effective handlers.
> >
> > Signed-off-by: Fabrizio Castro 
> > Signed-off-by: Ramesh Shanmugasundaram 
> > 
>
> > --- a/drivers/watchdog/renesas_wdt.c
> > +++ b/drivers/watchdog/renesas_wdt.c
> > @@ -118,6 +136,7 @@ static const struct watchdog_ops rwdt_ops = {
> > .stop = rwdt_stop,
> > .ping = rwdt_init_timeout,
> > .get_timeleft = rwdt_get_timeleft,
> > +   .restart = rwdt_restart,
> >  };
> >
> >  static int rwdt_probe(struct platform_device *pdev)
> > @@ -176,6 +195,7 @@ static int rwdt_probe(struct platform_device *pdev)
> > platform_set_drvdata(pdev, priv);
> > watchdog_set_drvdata(>wdev, priv);
> > watchdog_set_nowayout(>wdev, nowayout);
> > +   watchdog_set_restart_priority(>wdev, 128);
>
> Given we want to reboot R-Car Gen2 boards equipped with a suitable PMIC
> (e.g. DA9063) using that PMIC, I think the priority should be lower (0?),
> cfr.

Yes, can do, I have no strong opinion about this.
I'll use priority 0 for the next submission.

Thanks,
Fab

>
>  *   0:   use watchdog's restart function as last resort, has limited restart
>  *capabilies
>  *   128: default restart handler, use if no other handler is expected to be
>  *available and/or if restart is sufficient to restart the entire 
> system
>  *   255: preempt all other handlers
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, 
Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered 
No. 04586709.

Re: [PATCH 2/2] drm: adv7511: Add support for i2c_new_secondary_device

[Kieran Bingham]

Hi Laurent,

On 29/01/18 10:26, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.

Thanks for your review,

> On Monday, 22 January 2018 14:50:00 EET Kieran Bingham wrote:
>> The ADV7511 has four 256-byte maps that can be accessed via the main I²C
>> ports. Each map has it own I²C address and acts as a standard slave
>> device on the I²C bus.
>>
>> Allow a device tree node to override the default addresses so that
>> address conflicts with other devices on the same bus may be resolved at
>> the board description level.
>>
>> Signed-off-by: Kieran Bingham 
>> ---
>>  .../bindings/display/bridge/adi,adv7511.txt| 10 +-
> 
> I don't mind personally, but device tree maintainers usually ask for DT 
> bindings changes to be split to a separate patch.
> 
>>  drivers/gpu/drm/bridge/adv7511/adv7511.h   |  4 +++
>>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c   | 36 ---
>>  3 files changed, 37 insertions(+), 13 deletions(-)
>>
>> diff --git
>> a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>> b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>> index 0047b1394c70..f6bb9f6d3f48 100644
>> --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>> @@ -70,6 +70,9 @@ Optional properties:
>>rather than generate its own timings for HDMI output.
>>  - clocks: from common clock binding: reference to the CEC clock.
>>  - clock-names: from common clock binding: must be "cec".
>> +- reg-names : Names of maps with programmable addresses.
>> +It can contain any map needing a non-default address.
>> +Possible maps names are : "main", "edid", "cec", "packet"
> 
> Is the reg-names property (and the additional maps) mandatory or optional ? > 
> If mandatory you should also update the existing DT sources that use those
> bindings.

They are currently optional. I do prefer it that way - but perhaps due to an
issue mentioned below we might need to make this driver mandatory ?


> If optional you should define which I2C addresses will be used when 
> the maps are not specified > (and in that case I think we should go for the
> addresses listed as default in the datasheet, which correspond to the current 
> driver implementation when the main address is 0x3d/0x7a).

The current addresses do not correspond to the datasheet, even when the
implementation main address is set to 0x3d.

Thus, in my opinion - they are currently 'wrong' - but perhaps changing them is
considered breakage too.

A particular issue will arise here too - as on this device - when the device is
in low-power mode (after probe, before use) - the maps will respond on their
*hardware default* addresses (the ones implemented in this patch), and thus
consume that address on the I2C bus regardless of their settings in the driver.


> You should also update the definition of the reg property that currently just 
> states
> 
> - reg: I2C slave address
> 
> And finally you might want to define the term "map" in this context. Here's a 
> proposal (if we make all maps mandatory).
> 
> The ADV7511 internal registers are split into four pages exposed through 
> different I2C addresses, creating four register maps. The I2C addresses of 
> all 
> four maps shall be specified by the reg and reg-names property.
> 
> - reg: I2C slave addresses, one per reg-names entry
> - reg-names: map names, shall be "main", "edid", "cec", "packet"
> 
>>  Required nodes:
>>  
>> @@ -88,7 +91,12 @@ Example
>>  
>>  adv7511w: hdmi@39 {
>>  compatible = "adi,adv7511w";
>> -reg = <39>;
>> +/*
>> + * The EDID page will be accessible on address 0x66 on the i2c
>> + * bus. All other maps continue to use their default addresses.
>> + */
>> +reg = <0x39 0x66>;
>> +reg-names = "main", "edid";
>>  interrupt-parent = <>;
>>  interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
>>  clocks = <_clock>;
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> b/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> index d034b2cb5eee..7d81ce3808e0 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> @@ -53,8 +53,10 @@
>>  #define ADV7511_REG_POWER   0x41
>>  #define ADV7511_REG_STATUS  0x42
>>  #define ADV7511_REG_EDID_I2C_ADDR   0x43
>> +#define ADV7511_REG_EDID_I2C_ADDR_DEFAULT   0x3f
>>  #define ADV7511_REG_PACKET_ENABLE1  0x44
>>  #define ADV7511_REG_PACKET_I2C_ADDR 0x45
>> +#define ADV7511_REG_PACKET_I2C_ADDR_DEFAULT 0x38
>>  #define ADV7511_REG_DSD_ENABLE  0x46
>>  #define ADV7511_REG_VIDEO_INPUT_CFG20x48
>>  #define ADV7511_REG_INFOFRAME_UPDATE0x4a
>> @@ -89,6 +91,7 @@
>>  #define ADV7511_REG_TMDS_CLOCK_INV  

Re: [PATCH 2/2] drm: adv7511: Add support for i2c_new_secondary_device

[Kieran Bingham]

Hi Archit,

Thank you for your review,

On 29/01/18 04:11, Archit Taneja wrote:
> Hi,
> 
> On 01/22/2018 06:20 PM, Kieran Bingham wrote:
>> The ADV7511 has four 256-byte maps that can be accessed via the main I²C
>> ports. Each map has it own I²C address and acts as a standard slave
>> device on the I²C bus.
>>
>> Allow a device tree node to override the default addresses so that
>> address conflicts with other devices on the same bus may be resolved at
>> the board description level.
>>
>> Signed-off-by: Kieran Bingham 
>> ---
>>   .../bindings/display/bridge/adi,adv7511.txt    | 10 +-
>>   drivers/gpu/drm/bridge/adv7511/adv7511.h   |  4 +++
>>   drivers/gpu/drm/bridge/adv7511/adv7511_drv.c   | 36 
>> ++
>>   3 files changed, 37 insertions(+), 13 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>> b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>> index 0047b1394c70..f6bb9f6d3f48 100644
>> --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>> @@ -70,6 +70,9 @@ Optional properties:
>>     rather than generate its own timings for HDMI output.
>>   - clocks: from common clock binding: reference to the CEC clock.
>>   - clock-names: from common clock binding: must be "cec".
>> +- reg-names : Names of maps with programmable addresses.
>> +    It can contain any map needing a non-default address.
>> +    Possible maps names are : "main", "edid", "cec", "packet"
>>     Required nodes:
>>   @@ -88,7 +91,12 @@ Example
>>     adv7511w: hdmi@39 {
>>   compatible = "adi,adv7511w";
>> -    reg = <39>;
>> +    /*
>> + * The EDID page will be accessible on address 0x66 on the i2c
>> + * bus. All other maps continue to use their default addresses.
>> + */
>> +    reg = <0x39 0x66>;
>> +    reg-names = "main", "edid";
>>   interrupt-parent = <>;
>>   interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
>>   clocks = <_clock>;
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> b/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> index d034b2cb5eee..7d81ce3808e0 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> @@ -53,8 +53,10 @@
>>   #define ADV7511_REG_POWER    0x41
>>   #define ADV7511_REG_STATUS    0x42
>>   #define ADV7511_REG_EDID_I2C_ADDR    0x43
>> +#define ADV7511_REG_EDID_I2C_ADDR_DEFAULT    0x3f
>>   #define ADV7511_REG_PACKET_ENABLE1    0x44   
>>   #define ADV7511_REG_PACKET_I2C_ADDR    0x45
>> +#define ADV7511_REG_PACKET_I2C_ADDR_DEFAULT    0x38
>>   #define ADV7511_REG_DSD_ENABLE    0x46
>>   #define ADV7511_REG_VIDEO_INPUT_CFG2    0x48
>>   #define ADV7511_REG_INFOFRAME_UPDATE    0x4a
>> @@ -89,6 +91,7 @@
>>   #define ADV7511_REG_TMDS_CLOCK_INV    0xde
>>   #define ADV7511_REG_ARC_CTRL    0xdf
>>   #define ADV7511_REG_CEC_I2C_ADDR    0xe1
>> +#define ADV7511_REG_CEC_I2C_ADDR_DEFAULT    0x3c
> 
> Minor comment: The defines above make look like new register
> defines. It would be nice to remove the "REG_" from them, and
> place them somewhere after the register definitions.


Sure.


>>   #define ADV7511_REG_CEC_CTRL    0xe2
>>   #define ADV7511_REG_CHIP_ID_HIGH    0xf5
>>   #define ADV7511_REG_CHIP_ID_LOW    0xf6
>> @@ -322,6 +325,7 @@ struct adv7511 {
>>   struct i2c_client *i2c_main;
>>   struct i2c_client *i2c_edid;
>>   struct i2c_client *i2c_cec;
>> +    struct i2c_client *i2c_packet;
>>     struct regmap *regmap;
>>   struct regmap *regmap_cec;
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> index efa29db5fc2b..7ec33837752b 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> @@ -969,8 +969,8 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv)
>>   {
>>   int ret;
>>   -    adv->i2c_cec = i2c_new_dummy(adv->i2c_main->adapter,
>> - adv->i2c_main->addr - 1);
> 
> This patch avoids deriving the CEC/EDID map addresses from the main map. I 
> think
> this would break what the original patch tried to do:

That was intentional.

The ADV7511 data-sheet defines default addresses for these maps.
 (or rather the hardware does)

> 
> d25a4cbba4b9da7c2d674b2f8ecf84af1b24988e
> "drm/bridge: adv7511: add support for the 2nd chip"
> 
> Maybe the default macros can be a function of the main address?

I'm loathed to do that, because then intrinsic knowledge must be known that if I
define a device at address X ... it will also use magic offset A B and C.

IMO - the driver should define the defaults to match the hardware.

Anything else is an override ...





>> +    adv->i2c_cec = 

Re: [PATCH 2/3] arm64: dts: renesas: r8a77970: Remove non-existing STBE region

[Simon Horman]

On Tue, Feb 06, 2018 at 02:05:53PM +0100, Geert Uytterhoeven wrote:
> R-Car V3M does not have the Stream Buffer for EtherAVB-IF (STBE).
> 
> Note that the RAVB driver does not use this region.
> 
> Fixes: bea2ab136eaacec2 ("arm64: dts: renesas: r8a77970: add EtherAVB 
> support")
> Signed-off-by: Geert Uytterhoeven 

Thanks, applied for v4.17.

Re: [PATCH 3/3] arm64: dts: renesas: r8a77995: Remove non-existing STBE region

[Simon Horman]

On Tue, Feb 06, 2018 at 02:05:54PM +0100, Geert Uytterhoeven wrote:
> R-Car D3 does not have the Stream Buffer for EtherAVB-IF (STBE).
> 
> Note that the RAVB driver does not use this region.
> 
> Fixes: f9ba0c4cfe6169b7 ("arm64: dts: renesas: r8a77995: Add EthernetAVB 
> device node")
> Signed-off-by: Geert Uytterhoeven 

Thanks, applied for v4.17.

Re: [PATCH] arm64: renesas_defconfig: enable R8A77980 SoC

[Simon Horman]

On Tue, Feb 06, 2018 at 03:44:13PM +0100, Geert Uytterhoeven wrote:
> On Tue, Feb 6, 2018 at 2:20 PM, Simon Horman  
> wrote:
> > Enable the Renesas R-Car V3H (R8A77980) SoC in the ARM64 renesas_defconfig.
> >
> > Signed-off-by: Simon Horman 
> 
> Reviewed-by: Geert Uytterhoeven 


I have applied this to the topic/renesas-defconfig branch which is included
in the devel branch and tags of the Renesas tree as a convenience to
developers.  The branch is not, however, included in the next branch or
tags nor is it targeted at upstream.

Re: [PATCH] arm64: defconfig: enable R8A77980 SoC

[Simon Horman]

On Tue, Feb 06, 2018 at 03:43:54PM +0100, Geert Uytterhoeven wrote:
> On Tue, Feb 6, 2018 at 2:20 PM, Simon Horman  
> wrote:
> > Enable the Renesas R-Car V3H (R8A77980) SoC in the ARM64 defconfig.
> >
> > Signed-off-by: Simon Horman 
> 
> Reviewed-by: Geert Uytterhoeven 

Thanks, applied.

Re: [PATCH 01/11] soc: renesas: identify R-Car V3H

[Simon Horman]

On Tue, Feb 06, 2018 at 07:15:34PM +0300, Sergei Shtylyov wrote:
> On 02/06/2018 03:44 PM, Simon Horman wrote:
> 
> >>> Add support for identifying the R-Car V3H (R8A77980) SoC.
> >>>
> >>> Signed-off-by: Sergei Shtylyov 
> >>
> >> Reviewed-by: Geert Uytterhoeven 
> > 
> > Thanks, applied.
> 
>Not seeing that when fetching from your repo. Haven't pushed?

Indeed, sorry about that. They should be there now in
renesas-devel-20180206-v4.15.

Re: [PATCH v3 0/9] ARM: dts: gen2: add IP core switcher for all busses

[Simon Horman]

On Tue, Feb 06, 2018 at 11:29:49PM +0100, Wolfram Sang wrote:
> Here is the updated series to add the I2C IP core switcher to all busses of
> Gen2 boards where some kind of switching is possible (mostly to/from GPIO).
> 
> These patches were tested locally on Lager and Alt, and remotely on Koelsch,
> Porter and Gose. We have no Silk board available but the patch follows the 
> same
> style as the other patches. In addition, these patches have been 
> double-checked
> by vimdiff-comparing the i2c-bus blobs before and after this patchset.
> 
> As discussed previously: the tested boards boot fine. No regression 
> encountered
> there. However, changing masters at runtime for HDMI-named busses and 
> PWR-named
> busses will likely cause kernel warnings and maybe OOPSes. This is due to the
> rebind-issues in the V4L and regulator subsystems. For the former, fixing is
> WIP. For the latter, it is unknown how to deal with rebinding in general (but
> I'd need to look at this issue again if this is desired). It was decided that
> we consider those follow-up problems which should not hold back these patches
> anymore. The EXIO named busses seemed to work fine as far as this could be
> remotely tested.
> 
> Changes since V2:
> 
> * rebased to renesas/dt-for-v4.17
> 
> Changes since V1:
> 
> * fixed I2C4 pinmux for Koelsch and Gose (thanks Geert for the report!)
> * rebased to current renesas/dt-for-v4.16
> * fixed typo in branch name (s/topc/topic/)
> 
> The branch can be found here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git 
> renesas/topic/ip-switch-rework-2018
> 
> It is based on renesas/dt-for-v4.17.
> 
> Please apply.

Thanks, applied.