All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sylwester Nawrocki <spnlinux@gmail.com>
To: Marek Szyprowski <m.szyprowski@samsung.com>,
	'Kukjin Kim' <kgene.kim@samsung.com>
Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-samsung-soc@vger.kernel.org, kyungmin.park@samsung.com,
	ben-linux@fluff.org
Subject: Re: [PATCH 4/4 v2] ARM: S5PV210: Add clock definition for MIPI-CSIS DPHY
Date: Fri, 10 Dec 2010 13:01:08 +0100	[thread overview]
Message-ID: <4D021684.1000103@gmail.com> (raw)
In-Reply-To: <011101cb984a$701446d0$503cd470$%szyprowski@samsung.com>

Hi,

On 2010-12-10 10:12, Marek Szyprowski wrote:
> Hello,
> 
> On Friday, December 10, 2010 6:14 AM Kukjin Kim wrote:
> 
>> Sylwester Nawrocki wrote:
>>>
>>> MIPI DPHY control register requires special handling since it is shared
>>> between CSI (camera serial interface) and DSI (display serial interface).
>>> By creating this clock a serialized interface is provided for mipi-csis
>>> and mipi-dsim drivers, so DPHYs may be safely controlled by both drivers.
>>> Similarly dsim_dphy clock could be added for mipi-dsim.
>>>
>>> ---
>>>
>>> I am not quite sure about_"dphy_clock", perhaps power domain
>>> handling code would be better place for it.
>>>
>> Yeah, it is MIPI DPHY enable/disable control register not clock control.
>> So its proper position is not here...
>>
>> Hmm...how about driver's probe/open or some kind of setup in machine
>> directory?
> 
> I'm not sure that this is the best way of handling this phy controller. Please
> notice that mipi phy controller has different register location on S5PC110 and
> S5PC210. Please also notice that S5PC210 has 2 mipi csci controllers and phy
> interfaces, while s5pc110 has only one. Hiding all the logic and register
> specific writes behind this 'csi_dphy' clock easily resolves all these issues
> on all different samsung platforms and makes it easy to use it from the driver.
> Similar patch has been proposed some time ago for usb_phy interface and imho
> this is the right way to go.
> 
More importantly S5P_MIPI_DPHY_CONTROL* register is shared between two
devices - MIPI-CSIS and MIPI-DSIM. It means some kind of serialization
mechanism need to be employed while accessing this register, a spinlock
seems to fit best here. It also means that if we decided to create the
platform callback such a code would have to be placed in a common
compilation unit which would be built when either mipi-csis or mipi-dsim
is selected. And this callback would have to account the differences
between various SoC flavours or it would have to be repeated for each
mach-s5pvXXX.
So if the platform callback is the preferred way, where do we put an
implementation of it?

Regards,
Sylwester

WARNING: multiple messages have this Message-ID (diff)
From: spnlinux@gmail.com (Sylwester Nawrocki)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 4/4 v2] ARM: S5PV210: Add clock definition for MIPI-CSIS DPHY
Date: Fri, 10 Dec 2010 13:01:08 +0100	[thread overview]
Message-ID: <4D021684.1000103@gmail.com> (raw)
In-Reply-To: <011101cb984a$701446d0$503cd470$%szyprowski@samsung.com>

Hi,

On 2010-12-10 10:12, Marek Szyprowski wrote:
> Hello,
> 
> On Friday, December 10, 2010 6:14 AM Kukjin Kim wrote:
> 
>> Sylwester Nawrocki wrote:
>>>
>>> MIPI DPHY control register requires special handling since it is shared
>>> between CSI (camera serial interface) and DSI (display serial interface).
>>> By creating this clock a serialized interface is provided for mipi-csis
>>> and mipi-dsim drivers, so DPHYs may be safely controlled by both drivers.
>>> Similarly dsim_dphy clock could be added for mipi-dsim.
>>>
>>> ---
>>>
>>> I am not quite sure about_"dphy_clock", perhaps power domain
>>> handling code would be better place for it.
>>>
>> Yeah, it is MIPI DPHY enable/disable control register not clock control.
>> So its proper position is not here...
>>
>> Hmm...how about driver's probe/open or some kind of setup in machine
>> directory?
> 
> I'm not sure that this is the best way of handling this phy controller. Please
> notice that mipi phy controller has different register location on S5PC110 and
> S5PC210. Please also notice that S5PC210 has 2 mipi csci controllers and phy
> interfaces, while s5pc110 has only one. Hiding all the logic and register
> specific writes behind this 'csi_dphy' clock easily resolves all these issues
> on all different samsung platforms and makes it easy to use it from the driver.
> Similar patch has been proposed some time ago for usb_phy interface and imho
> this is the right way to go.
> 
More importantly S5P_MIPI_DPHY_CONTROL* register is shared between two
devices - MIPI-CSIS and MIPI-DSIM. It means some kind of serialization
mechanism need to be employed while accessing this register, a spinlock
seems to fit best here. It also means that if we decided to create the
platform callback such a code would have to be placed in a common
compilation unit which would be built when either mipi-csis or mipi-dsim
is selected. And this callback would have to account the differences
between various SoC flavours or it would have to be repeated for each
mach-s5pvXXX.
So if the platform callback is the preferred way, where do we put an
implementation of it?

Regards,
Sylwester

  reply	other threads:[~2010-12-10 12:01 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-12-03 19:52 [PATCH 0/4 v2] ARM: S5P: Add platform support for MIPI-CSI slave devices Sylwester Nawrocki
2010-12-03 19:52 ` Sylwester Nawrocki
2010-12-03 19:52 ` [PATCH 1/4 v2] ARM: S5PV210: Add resource definitions for MIPI CSIS Sylwester Nawrocki
2010-12-03 19:52   ` Sylwester Nawrocki
2010-12-03 19:52 ` [PATCH 2/4 v2] ARM: S5PV310: " Sylwester Nawrocki
2010-12-03 19:52   ` Sylwester Nawrocki
2010-12-03 19:52 ` [PATCH 3/4 v2] ARM: S5P: Add platform support for MIPI CSIS devices Sylwester Nawrocki
2010-12-03 19:52   ` Sylwester Nawrocki
2010-12-03 19:52 ` [PATCH 4/4 v2] ARM: S5PV210: Add clock definition for MIPI-CSIS DPHY Sylwester Nawrocki
2010-12-03 19:52   ` Sylwester Nawrocki
2010-12-10  5:14   ` Kukjin Kim
2010-12-10  5:14     ` Kukjin Kim
2010-12-10  9:12     ` Marek Szyprowski
2010-12-10  9:12       ` Marek Szyprowski
2010-12-10 12:01       ` Sylwester Nawrocki [this message]
2010-12-10 12:01         ` Sylwester Nawrocki
2010-12-10 13:24         ` Marek Szyprowski
2010-12-10 13:24           ` Marek Szyprowski
2010-12-10  2:27 ` [PATCH 0/4 v2] ARM: S5P: Add platform support for MIPI-CSI slave devices Kukjin Kim
2010-12-10  2:27   ` Kukjin Kim

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4D021684.1000103@gmail.com \
    --to=spnlinux@gmail.com \
    --cc=ben-linux@fluff.org \
    --cc=kgene.kim@samsung.com \
    --cc=kyungmin.park@samsung.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-samsung-soc@vger.kernel.org \
    --cc=m.szyprowski@samsung.com \
    --cc=s.nawrocki@samsung.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.