* [PATCH] arm64: debug: mark a function as __init to save some memory
From: Christophe JAILLET @ 2020-05-31 11:00 UTC (permalink / raw)
To: catalin.marinas, will, tglx, paulmck, gregkh, mhiramat, dianders
Cc: Christophe JAILLET, kernel-janitors, linux-kernel,
linux-arm-kernel
'debug_monitors_init()' is only called via 'postcore_initcall'.
It can be marked as __init to save a few bytes of memory.
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
arch/arm64/kernel/debug-monitors.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index 15e80c876d46..5df49366e9ab 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -130,7 +130,7 @@ static int clear_os_lock(unsigned int cpu)
return 0;
}
-static int debug_monitors_init(void)
+static int __init debug_monitors_init(void)
{
return cpuhp_setup_state(CPUHP_AP_ARM64_DEBUG_MONITORS_STARTING,
"arm64/debug_monitors:starting",
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH] irqchip/gic-v3-its: Don't try to move a disabled irq
From: Marc Zyngier @ 2020-05-31 11:09 UTC (permalink / raw)
To: Saidi, Ali
Cc: Herrenschmidt, Benjamin, Jason Cooper, Machulsky, Zorik,
linux-kernel, Zilberman, Zeev, linux-arm-kernel, Thomas Gleixner,
Woodhouse, David
In-Reply-To: <20200530174929.7bf6d5d7@why>
On 2020-05-30 17:49, Marc Zyngier wrote:
> Hi Ali,
>
> On Fri, 29 May 2020 12:36:42 +0000
> "Saidi, Ali" <alisaidi@amazon.com> wrote:
>
>> Hi Marc,
>>
>> > On May 29, 2020, at 3:33 AM, Marc Zyngier <maz@kernel.org> wrote:
>> >
>> > Hi Ali,
>> >
>> >> On 2020-05-29 02:55, Ali Saidi wrote:
>> >> If an interrupt is disabled the ITS driver has sent a discard removing
>> >> the DeviceID and EventID from the ITT. After this occurs it can't be
>> >> moved to another collection with a MOVI and a command error occurs if
>> >> attempted. Before issuing the MOVI command make sure that the IRQ isn't
>> >> disabled and change the activate code to try and use the previous
>> >> affinity.
>> >>
>> >> Signed-off-by: Ali Saidi <alisaidi@amazon.com>
>> >> ---
>> >> drivers/irqchip/irq-gic-v3-its.c | 18 +++++++++++++++---
>> >> 1 file changed, 15 insertions(+), 3 deletions(-)
>> >>
>> >> diff --git a/drivers/irqchip/irq-gic-v3-its.c
>> >> b/drivers/irqchip/irq-gic-v3-its.c
>> >> index 124251b0ccba..1235dd9a2fb2 100644
>> >> --- a/drivers/irqchip/irq-gic-v3-its.c
>> >> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> >> @@ -1540,7 +1540,11 @@ static int its_set_affinity(struct irq_data *d,
>> >> const struct cpumask *mask_val,
>> >> /* don't set the affinity when the target cpu is same as current one
>> >> */
>> >> if (cpu != its_dev->event_map.col_map[id]) {
>> >> target_col = &its_dev->its->collections[cpu];
>> >> - its_send_movi(its_dev, target_col, id);
>> >> +
>> >> + /* If the IRQ is disabled a discard was sent so don't move */
>> >> + if (!irqd_irq_disabled(d))
>> >> + its_send_movi(its_dev, target_col, id);
>> >> +
>> >
>> > This looks wrong. What you are testing here is whether the interrupt
>> > is masked, not that there isn't a valid translation.
>> I’m not exactly sure the correct condition, but what I’m looking for
>> is interrupts which are deactivated and we have thus sent a discard.
>
> That looks like IRQD_IRQ_STARTED not being set in this case.
>
>> >
>> > In the commit message, you're saying that we've issued a discard.
>> > This hints at doing a set_affinity on an interrupt that has been
>> > deactivated (mapping removed). Is that actually the case? If so,
>> > why was it deactivated
>> > the first place?
>> This is the case. If we down a NIC, that interface’s MSIs will be
>> deactivated but remain allocated until the device is unbound from the
>> driver or the NIC is brought up.
>>
>> While stressing down/up a device I’ve found that irqbalance can move
>> interrupts and you end up with the situation described. The device is
>> downed, the interrupts are deactivated but still present and then
>> trying to move one results in sending a MOVI after the DISCARD which
>> is an error per the GIC spec.
>
> Not great indeed. But this is not, as far as I can tell, a GIC
> driver problem.
>
> The semantic of activate/deactivate (which maps to started/shutdown
> in the IRQ code) is that the HW resources for a given interrupt are
> only committed when the interrupt is activated. Trying to perform
> actions involving the HW on an interrupt that isn't active cannot be
> guaranteed to take effect.
>
> I'd rather address it in the core code, by preventing set_affinity (and
> potentially others) to take place when the interrupt is not in the
> STARTED state. Userspace would get an error, which is perfectly
> legitimate, and which it already has to deal with it for plenty of
> other
> reasons.
How about this:
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 453a8a0f4804..1a2ac1392c0f 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -147,7 +147,8 @@ cpumask_var_t irq_default_affinity;
static bool __irq_can_set_affinity(struct irq_desc *desc)
{
if (!desc || !irqd_can_balance(&desc->irq_data) ||
- !desc->irq_data.chip || !desc->irq_data.chip->irq_set_affinity)
+ !desc->irq_data.chip || !desc->irq_data.chip->irq_set_affinity ||
+ !irqd_is_started(&desc->irq_data))
return false;
return true;
}
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v8 0/3] perf arm-spe: Add support for synthetic events
From: Arnaldo Carvalho de Melo @ 2020-05-31 12:23 UTC (permalink / raw)
To: Leo Yan, Adrian Hunter
Cc: Mark Rutland, Ian Rogers, Andi Kleen, Al Grant, Mathieu Poirier,
Alexander Shishkin, Will Deacon, linux-kernel, Peter Zijlstra,
Jin Yao, Ingo Molnar, James Clark, Namhyung Kim, Thomas Gleixner,
Jiri Olsa, linux-arm-kernel, Mike Leach
In-Reply-To: <20200530122442.490-1-leo.yan@linaro.org>
Em Sat, May 30, 2020 at 08:24:39PM +0800, Leo Yan escreveu:
> This patch set is to support synthetic events with enabling Arm SPE
> decoder. This patch set is based Xiaojun Tan (Hisilicon) and
> James Clark (Arm)'s previous patches who have contributed much for
> the related task.
Applied, will push to tmp.perf/core, and then perf/core if all tests are
successful, Adrian, if you could provide an Acked-by: for the auxtrace
case, that would be good,
- Arnaldo
> This patch set has been checked with checkpatch.pl, though it leaves
> several warnings, but these warnings are deliberately kept after
> reviewing. Some warnings ask to add maintainer (so far it's not
> necessary), and some warnings complaint for patch 02 "perf auxtrace:
> Add four itrace options" for the text format, since need to keep the
> consistency with the same code format in the source code, this is why
> this patch doesn't get rid of checkpatch warnings.
>
> This patch set has been rebased on Perf tmp.perf/core branch with
> latest commit 9300acc6fed8 ("perf build: Add a LIBPFM4=1 build test
> entry"). The patches has been tested on Arm N1 machine (by James)
> and on Hisilicon D06 platform (by Leo).
>
> Changes from v7:
> * Added James's tested-by tags;
> * Rebased on Perf tmp.perf/core branch.
>
>
> Tan Xiaojun (3):
> perf tools: Move arm-spe-pkt-decoder.h/c to the new dir
> perf auxtrace: Add four itrace options
> perf arm-spe: Support synthetic events
>
> tools/perf/Documentation/itrace.txt | 6 +-
> tools/perf/util/Build | 2 +-
> tools/perf/util/arm-spe-decoder/Build | 1 +
> .../util/arm-spe-decoder/arm-spe-decoder.c | 219 +++++
> .../util/arm-spe-decoder/arm-spe-decoder.h | 82 ++
> .../arm-spe-pkt-decoder.c | 0
> .../arm-spe-pkt-decoder.h | 16 +
> tools/perf/util/arm-spe.c | 823 +++++++++++++++++-
> tools/perf/util/auxtrace.c | 17 +
> tools/perf/util/auxtrace.h | 15 +-
> 10 files changed, 1135 insertions(+), 46 deletions(-)
> create mode 100644 tools/perf/util/arm-spe-decoder/Build
> create mode 100644 tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
> create mode 100644 tools/perf/util/arm-spe-decoder/arm-spe-decoder.h
> rename tools/perf/util/{ => arm-spe-decoder}/arm-spe-pkt-decoder.c (100%)
> rename tools/perf/util/{ => arm-spe-decoder}/arm-spe-pkt-decoder.h (64%)
>
> --
> 2.17.1
>
--
- Arnaldo
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH RFCv2 9/9] arm64: Support async page fault
From: Marc Zyngier @ 2020-05-31 12:44 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Gavin Shan, catalin.marinas, linux-kernel, shan.gavin, will,
kvmarm, linux-arm-kernel
In-Reply-To: <d0bfb944-b50a-608a-7dcc-5a409cdc4524@redhat.com>
On 2020-05-29 12:11, Paolo Bonzini wrote:
> On 29/05/20 11:41, Marc Zyngier wrote:
>>>>
>>>>
>>>> For x86 the advantage is that the processor can take care of raising
>>>> the
>>>> stage2 page fault in the guest, so it's faster.
>>>>
>>> I think there might be too much overhead if the page can be populated
>>> quickly by host. For example, it's fast to populate the pages if
>>> swapin
>>> isn't involved.
>
> Those would still be handled by the host. Only those that are not
> present in the host (which you can see through the MMU notifier) would
> be routed to the guest. You can do things differently between "not
> present fault because the page table does not exist" and "not present
> fault because the page is missing in the host".
>
>>> If I'm correct enough, it seems arm64 doesn't have similar mechanism,
>>> routing stage2 page fault to guest.
>>
>> Indeed, this isn't a thing on arm64. Exception caused by a S2 fault
>> are
>> always routed to EL2.
>
> Is there an ARM-approved way to reuse the S2 fault syndromes to detect
> async page faults?
It would mean being able to set an ESR_EL2 register value into ESR_EL1,
and there is nothing in the architecture that would allow that, with
the exception of nested virt: a VHE guest hypervisor running at EL1
must be able to observe S2 faults for its own S2, as synthesized by
the host hypervisor.
The trouble is that:
- there is so far no commercially available CPU supporting NV
- even if you could get hold of such a machine, there is no
guarantee that such "EL2 syndrome at EL1" is valid outside of
the nested context
- this doesn't solve the issue for non-NV CPUs anyway
> (By the way, another "modern" use for async page faults is for postcopy
> live migration).
Right. That's definitely a more interesting version of "swap-in".
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 4/4] docs: counter: Document character device interface
From: William Breathitt Gray @ 2020-05-31 13:31 UTC (permalink / raw)
To: Pavel Machek
Cc: kamel.bouhara, gwendal, david, linux-iio, patrick.havelange,
alexandre.belloni, linux-kernel, linux-arm-kernel,
mcoquelin.stm32, fabrice.gasnier, syednwaris, linux-stm32, jic23,
alexandre.torgue
In-Reply-To: <20200529132604.GB1339@bug>
[-- Attachment #1.1: Type: text/plain, Size: 3654 bytes --]
On Fri, May 29, 2020 at 03:26:04PM +0200, Pavel Machek wrote:
> On Sat 2020-05-16 15:20:02, William Breathitt Gray wrote:
> > This patch adds high-level documentation about the Counter subsystem
> > character device interface.
> >
> > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> > ---
> > Documentation/driver-api/generic-counter.rst | 112 +++++++++++++------
> > 1 file changed, 76 insertions(+), 36 deletions(-)
> >
> > diff --git a/Documentation/driver-api/generic-counter.rst b/Documentation/driver-api/generic-counter.rst
> > index 8f85c30dea0b..58045b33b576 100644
> > --- a/Documentation/driver-api/generic-counter.rst
> > +++ b/Documentation/driver-api/generic-counter.rst
>
> > +
> > +Counter chrdev
> > +--------------
> > +Translates counter data to the standard Counter character device; data
> > +is transferred via standard character device read/write calls.
> > +
> > +Sysfs Interface
> > +===============
> > +
> > +Several sysfs attributes are generated by the Generic Counter interface,
> > +and reside under the `/sys/bus/counter/devices/counterX` directory,
> > +where `X` is to the respective counter device id. Please see
> > +Documentation/ABI/testing/sysfs-bus-counter for detailed information on
> > +each Generic Counter interface sysfs attribute.
> > +
> > +Through these sysfs attributes, programs and scripts may interact with
> > +the Generic Counter paradigm Counts, Signals, and Synapses of respective
> > +counter devices.
> > +
> > +Counter Character Device
> > +========================
> > +
> > +Counter character device nodes are created under the `/dev` directory as
> > +`counterX`, where `X` is the respective counter device id. Defines for
> > +the standard Counter data types are exposed via the userspace
> > +`include/uapi/linux/counter-types.h` file.
> > +
> > +The first 196095 bytes of the character device serve as a control
> > +selection area where control exposure of desired Counter components and
> > +extensions may be selected. Each byte serves as a boolean selection
> > +indicator for a respective Counter component or extension. The format of
> > +this area is as follows:
> > +
> > +* For each device extension, a byte is required.
> > +* For each Signal, a byte is reserved for the Signal component, and a
> > + byte is reserved for each Signal extension.
> > +* For each Count, a byte is reserved for the Count component, a byte is
> > + reserved for the count function, a byte is reserved for each Synapse
> > + action, and byte is reserved for each Count extension.
> > +
> > +The selected Counter components and extensions may then be interfaced
> > +after the first 196095 bytes via standard character device read/write
> > +operations. The number of bytes available for each component or
> > +extension is dependent on their respective data type: u8 will have 1
> > +byte available, u64 will have 8 bytes available, strings will have 64
> > +bytes available, etc.
>
> This looks like very, very strange interface, and not described in detail
> required to understand it.
>
> Could you take a look at input subsystem, /dev/input/event0? Perhaps it is
> directly usable, and if not something similar should probably be acceptable.
>
> Best regards,
> Pavel
Yes, I don't think this is a good interface afterall. I'm implementing a
different design for v3 that should be more intuitive. The input
subsystem could be useful for streams of events such as timestamps, so
I'll take a look at it as well in case something similar to it could be
used.
William Breathitt Gray
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 0/3] KVM: arm64: aarch32 ACTLR accesses
From: Marc Zyngier @ 2020-05-31 13:37 UTC (permalink / raw)
To: James Morse; +Cc: Julien Thierry, kvmarm, linux-arm-kernel, Suzuki K Poulose
In-Reply-To: <20200526161834.29165-1-james.morse@arm.com>
Hi James,
On 2020-05-26 17:18, James Morse wrote:
[...]
> 1. How does this copro[] thing work with a big-endian host?
> The cp15_regs emulation look fine as nothing uses vcpu_cp15() to read
> the
> register, but wouldn't prepare_fault32() read the wrong end of the
> register
> when using vcpu_cp15()?
This seems pretty broken indeed. How about something like this:
diff --git a/arch/arm64/include/asm/kvm_host.h
b/arch/arm64/include/asm/kvm_host.h
index 59029e90b557..e80c0e06f235 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -404,8 +404,14 @@ void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64
val, int reg);
* CP14 and CP15 live in the same array, as they are backed by the
* same system registers.
*/
-#define vcpu_cp14(v,r) ((v)->arch.ctxt.copro[(r)])
-#define vcpu_cp15(v,r) ((v)->arch.ctxt.copro[(r)])
+#ifdef CPU_BIG_ENDIAN
+#define CPx_OFFSET 1
+#else
+#define CPx_OFFSET 0
+#endif
+
+#define vcpu_cp14(v,r) ((v)->arch.ctxt.copro[(r) ^ CPx_OFFSET])
+#define vcpu_cp15(v,r) ((v)->arch.ctxt.copro[(r) ^ CPx_OFFSET])
struct kvm_vm_stat {
ulong remote_tlb_flush;
Yes, it's ugly.
> 2. How does the 32bit fault injection code work with VHE?
> vcpu_cp15() modifies the in-memory copy, surely a vcpu_put() will
> clobber
> everything it did, or fail to restore it when entering the guest.
Wow, you're really wadding into dangerous territory! ;-)
Again, you are absolutely right. I guess nobody really ever ran
32bit guests on VHE systems, as they are both rare and mostly
with 64-bit-only EL1. This code is also mostly never used
(we run well behaved guests at all times!).
Here's a hack that should do the right thing at all times:
diff --git a/arch/arm64/kvm/aarch32.c b/arch/arm64/kvm/aarch32.c
index 0a356aa91aa1..40a62a99fbf8 100644
--- a/arch/arm64/kvm/aarch32.c
+++ b/arch/arm64/kvm/aarch32.c
@@ -33,6 +33,26 @@ static const u8 return_offsets[8][2] = {
[7] = { 4, 4 }, /* FIQ, unused */
};
+static bool pre_fault_synchronize(struct kvm_vcpu *vcpu)
+{
+ preempt_disable();
+ if (vcpu->arch.sysregs_loaded_on_cpu) {
+ kvm_arch_vcpu_put(vcpu);
+ return true;
+ }
+
+ preempt_enable();
+ return false;
+}
+
+static void post_fault_synchronize(struct kvm_vcpu *vcpu, bool loaded)
+{
+ if (loaded) {
+ kvm_arch_vcpu_load(vcpu, smp_processor_id());
+ preempt_enable();
+ }
+}
+
/*
* When an exception is taken, most CPSR fields are left unchanged in
the
* handler. However, some are explicitly overridden (e.g. M[4:0]).
@@ -155,7 +175,10 @@ static void prepare_fault32(struct kvm_vcpu *vcpu,
u32 mode, u32 vect_offset)
void kvm_inject_undef32(struct kvm_vcpu *vcpu)
{
+ bool loaded = pre_fault_synchronize(vcpu);
+
prepare_fault32(vcpu, PSR_AA32_MODE_UND, 4);
+ post_fault_synchronize(vcpu, loaded);
}
/*
@@ -168,6 +191,9 @@ static void inject_abt32(struct kvm_vcpu *vcpu, bool
is_pabt,
u32 vect_offset;
u32 *far, *fsr;
bool is_lpae;
+ bool loaded;
+
+ loaded = pre_fault_synchronize(vcpu);
if (is_pabt) {
vect_offset = 12;
@@ -191,6 +217,8 @@ static void inject_abt32(struct kvm_vcpu *vcpu, bool
is_pabt,
/* no need to shuffle FS[4] into DFSR[10] as its 0 */
*fsr = DFSR_FSC_EXTABT_nLPAE;
}
+
+ post_fault_synchronize(vcpu, loaded);
}
void kvm_inject_dabt32(struct kvm_vcpu *vcpu, unsigned long addr)
Of course, none of this is tested. We really should find ways to
trigger these corner cases... :-/
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v2 2/3] media: rockchip: Introduce driver for Rockhip's camera interface
From: Ezequiel Garcia @ 2020-05-31 13:40 UTC (permalink / raw)
To: Maxime Chevallier
Cc: Mark Rutland, devicetree, Heiko Stuebner,
Linux Kernel Mailing List, Paul Kocialkowski,
open list:ARM/Rockchip SoC..., Rob Herring, Thomas Petazzoni,
Miquel Raynal, Hans Verkuil, Mauro Carvalho Chehab, Robin Murphy,
linux-arm-kernel, linux-media
In-Reply-To: <20200529130405.929429-3-maxime.chevallier@bootlin.com>
Hi Maxime,
Thanks for posting this patch. I think you can still improve it,
but it's a neat first try! :-)
On Fri, 29 May 2020 at 10:05, Maxime Chevallier
<maxime.chevallier@bootlin.com> wrote:
>
> Introduce a driver for the camera interface on some Rockchip platforms.
>
> This controller supports CSI2, Parallel and BT656 interfaces, but for
> now only the parallel interface could be tested, hence it's the only one
> that's supported in the first version of this driver.
>
I'm confused, you mention parallel as the only tested interface,
but the cover letters mentions PAL. Doesn't PAL mean BT.656
or am I completely lost?
(I am not super familiar with parallel sensors).
> This controller can be fond on PX30, RK1808, RK3128, RK3288 and RK3288,
> but for now it's only be tested on PX30.
>
My RK3288 and RK3326 (i.e. PX30) refer to this IP block as "Video
Input interface".
I am wondering if it won't be clearer for developers / users if we
rename the driver
to rockchip-vip (and of course s/cif/vip and s/CIF/VIP).
> Most of this driver was written follwing the BSP driver from rockchip,
> removing the parts that either didn't fit correctly the guidelines, or
> that couldn't be tested.
>
> This basic version doesn't support cropping nor scaling, and is only
> designed with one sensor being attached to it a any time.
>
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
>
> Changes since V1 :
>
> - Convert to the bulk APIs for clocks and resets
Note that the bulk API clock conversion was not
properly done.
> - remove useless references to priv data
> - Move around some init functions at probe time
> - Upate some helpers to more suitable ones
>
> Here is the output from v4l2-compliance. There are no fails in the final
> summary, but there is one in the output that I didn't catch previously.
>
> Still, here's the V2 in the meantime, if you have any further reviews
> ompliance SHA: not available, 64 bits
>
> Compliance test for rkcif device /dev/video0:
>
> Driver Info:
> Driver name : rkcif
> Card type : rkcif
> Bus info : platform:ff490000.cif
> Driver version : 5.7.0
> Capabilities : 0x84201000
> Video Capture Multiplanar
> Streaming
> Extended Pix Format
> Device Capabilities
> Device Caps : 0x04201000
> Video Capture Multiplanar
> Streaming
> Extended Pix Format
> Media Driver Info:
> Driver name : rkcif
> Model : rkcif
> Serial :
> Bus info :
> Media version : 5.7.0
> Hardware revision: 0x00000000 (0)
> Driver version : 5.7.0
> Interface Info:
> ID : 0x03000002
> Type : V4L Video
> Entity Info:
> ID : 0x00000001 (1)
> Name : video_rkcif
> Function : V4L2 I/O
> Pad 0x01000004 : 0: Sink
> Link 0x02000007: from remote pad 0x1000006 of entity 'tw9900 2-0044': Data, Enabled
>
> Required ioctls:
> test MC information (see 'Media Driver Info' above): OK
> test VIDIOC_QUERYCAP: OK
>
> Allow for multiple opens:
> test second /dev/video0 open: OK
> test VIDIOC_QUERYCAP: OK
> test VIDIOC_G/S_PRIORITY: OK
> test for unlimited opens: OK
>
> Debug ioctls:
> test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
> test VIDIOC_LOG_STATUS: OK (Not Supported)
>
> Input ioctls:
> test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
> test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
> test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
> test VIDIOC_ENUMAUDIO: OK (Not Supported)
> test VIDIOC_G/S/ENUMINPUT: OK
> test VIDIOC_G/S_AUDIO: OK (Not Supported)
> Inputs: 1 Audio Inputs: 0 Tuners: 0
>
> Output ioctls:
> test VIDIOC_G/S_MODULATOR: OK (Not Supported)
> test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
> test VIDIOC_ENUMAUDOUT: OK (Not Supported)
> test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
> test VIDIOC_G/S_AUDOUT: OK (Not Supported)
> Outputs: 0 Audio Outputs: 0 Modulators: 0
>
> Input/Output configuration ioctls:
> test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
> test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
> test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
> test VIDIOC_G/S_EDID: OK (Not Supported)
>
> Control ioctls (Input 0):
> test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
> test VIDIOC_QUERYCTRL: OK
> test VIDIOC_G/S_CTRL: OK
> test VIDIOC_G/S/TRY_EXT_CTRLS: OK
> test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported)
> test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
> Standard Controls: 0 Private Controls: 0
>
> Format ioctls (Input 0):
> test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
> test VIDIOC_G/S_PARM: OK (Not Supported)
> test VIDIOC_G_FBUF: OK (Not Supported)
> test VIDIOC_G_FMT: OK
> test VIDIOC_TRY_FMT: OK
> test VIDIOC_S_FMT: OK
> test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
> test Cropping: OK (Not Supported)
> test Composing: OK (Not Supported)
> fail: v4l2-test-formats.cpp(1772): node->can_scale && node->frmsizes_count[v4l_format_g_pixelformat(&cur)]
> test Scaling: OK
>
> Codec ioctls (Input 0):
> test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
> test VIDIOC_G_ENC_INDEX: OK (Not Supported)
> test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)
>
> Buffer ioctls (Input 0):
> test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
> test VIDIOC_EXPBUF: OK
> test Requests: OK (Not Supported)
>
> Total for rkcif device /dev/video0: 45, Succeeded: 45, Failed: 0, Warnings: 0
>
> drivers/media/platform/Kconfig | 13 +
> drivers/media/platform/Makefile | 1 +
> drivers/media/platform/rockchip/cif/Makefile | 3 +
> drivers/media/platform/rockchip/cif/capture.c | 1170 +++++++++++++++++
> drivers/media/platform/rockchip/cif/dev.c | 358 +++++
> drivers/media/platform/rockchip/cif/dev.h | 213 +++
> drivers/media/platform/rockchip/cif/regs.h | 256 ++++
> 7 files changed, 2014 insertions(+)
> create mode 100644 drivers/media/platform/rockchip/cif/Makefile
> create mode 100644 drivers/media/platform/rockchip/cif/capture.c
> create mode 100644 drivers/media/platform/rockchip/cif/dev.c
> create mode 100644 drivers/media/platform/rockchip/cif/dev.h
> create mode 100644 drivers/media/platform/rockchip/cif/regs.h
>
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index e01bbb9dd1c1..d4ec5e36bca7 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -460,6 +460,19 @@ config VIDEO_ROCKCHIP_RGA
>
> To compile this driver as a module choose m here.
>
> +config VIDEO_ROCKCHIP_CIF
> + tristate "Rockchip Camera Interface"
> + depends on VIDEO_DEV && VIDEO_V4L2
> + depends on ARCH_ROCKCHIP || COMPILE_TEST
> + select VIDEOBUF2_DMA_SG
> + select VIDEOBUF2_DMA_CONTIG
> + select V4L2_FWNODE
> + select V4L2_MEM2MEM_DEV
> + help
> + This is a v4l2 driver for Rockchip SOC Camera interface.
> +
> + To compile this driver as a module choose m here.
> +
> config VIDEO_TI_VPE
> tristate "TI VPE (Video Processing Engine) driver"
> depends on VIDEO_DEV && VIDEO_V4L2
> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> index d13db96e3015..67e7ac034be1 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -68,6 +68,7 @@ obj-$(CONFIG_VIDEO_RENESAS_JPU) += rcar_jpu.o
> obj-$(CONFIG_VIDEO_RENESAS_VSP1) += vsp1/
>
> obj-$(CONFIG_VIDEO_ROCKCHIP_RGA) += rockchip/rga/
> +obj-$(CONFIG_VIDEO_ROCKCHIP_CIF) += rockchip/cif/
>
> obj-y += omap/
>
> diff --git a/drivers/media/platform/rockchip/cif/Makefile b/drivers/media/platform/rockchip/cif/Makefile
> new file mode 100644
> index 000000000000..727990824316
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/cif/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +obj-$(CONFIG_VIDEO_ROCKCHIP_CIF) += video_rkcif.o
> +video_rkcif-objs += dev.o capture.o
> diff --git a/drivers/media/platform/rockchip/cif/capture.c b/drivers/media/platform/rockchip/cif/capture.c
> new file mode 100644
> index 000000000000..adab6704129f
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/cif/capture.c
> @@ -0,0 +1,1170 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Rockchip CIF Driver
> + *
> + * Copyright (C) 2018 Rockchip Electronics Co., Ltd.
> + * Copyright (C) 2020 Maxime Chevallier <maxime.chevallier@bootlin.com>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/reset.h>
> +#include <media/v4l2-common.h>
> +#include <media/v4l2-event.h>
> +#include <media/v4l2-fh.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/v4l2-ioctl.h>
> +#include <media/v4l2-mc.h>
> +#include <media/v4l2-subdev.h>
> +#include <media/videobuf2-dma-contig.h>
> +
> +#include "dev.h"
> +#include "regs.h"
> +
> +#define CIF_REQ_BUFS_MIN 1
> +#define CIF_MIN_WIDTH 64
> +#define CIF_MIN_HEIGHT 64
> +#define CIF_MAX_WIDTH 8192
> +#define CIF_MAX_HEIGHT 8192
> +
> +#define RKCIF_PLANE_Y 0
> +#define RKCIF_PLANE_CBCR 1
> +
> +#define CIF_FETCH_Y_LAST_LINE(VAL) ((VAL) & 0x1fff)
> +/* Check if swap y and c in bt1120 mode */
> +#define CIF_FETCH_IS_Y_FIRST(VAL) ((VAL) & 0xf)
> +
> +/* Get xsubs and ysubs for fourcc formats
> + *
> + * @xsubs: horizontal color samples in a 4*4 matrix, for yuv
> + * @ysubs: vertical color samples in a 4*4 matrix, for yuv
> + */
> +static int fcc_xysubs(u32 fcc, u32 *xsubs, u32 *ysubs)
You should avoid doing this math in the driver,
and instead use the nice core helpers
v4l2_fill_pixfmt_mp and v4l2_fill_pixfmt.
> +{
> + switch (fcc) {
> + case V4L2_PIX_FMT_NV16:
> + case V4L2_PIX_FMT_NV61:
> + *xsubs = 2;
> + *ysubs = 1;
> + break;
> + case V4L2_PIX_FMT_NV21:
> + case V4L2_PIX_FMT_NV12:
> + *xsubs = 2;
> + *ysubs = 2;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static const struct cif_output_fmt out_fmts[] = {
> + {
> + .fourcc = V4L2_PIX_FMT_NV16,
> + .cplanes = 2,
> + .mplanes = 1,
Only mplanes = 1 formats are supported, please
drop it here, and drop any handling due to mplanes > 1.
> + .fmt_val = YUV_OUTPUT_422 | UV_STORAGE_ORDER_UVUV,
> + .bpp = { 8, 16 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_NV61,
> + .fmt_val = YUV_OUTPUT_422 | UV_STORAGE_ORDER_VUVU,
> + .cplanes = 2,
> + .mplanes = 1,
> + .bpp = { 8, 16 },
> + },
> + {
> + .fourcc = V4L2_PIX_FMT_NV12,
> + .fmt_val = YUV_OUTPUT_420 | UV_STORAGE_ORDER_UVUV,
> + .cplanes = 2,
> + .mplanes = 1,
> + .bpp = { 8, 16 },
> + .mbus = MEDIA_BUS_FMT_UYVY8_2X8,
> + },
> + {
> + .fourcc = V4L2_PIX_FMT_NV21,
> + .fmt_val = YUV_OUTPUT_420 | UV_STORAGE_ORDER_VUVU,
> + .cplanes = 2,
> + .mplanes = 1,
> + .bpp = { 8, 16 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_RGB24,
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 24 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_RGB565,
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 16 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_BGR666,
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 18 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_SRGGB8,
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 8 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_SGRBG8,
That is odd: how does the driver distinguish
V4L2_PIX_FMT_SGRBG8 from V4L2_PIX_FMT_SRGGB8?
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 8 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_SGBRG8,
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 8 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_SBGGR8,
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 8 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_SRGGB10,
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 16 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_SGRBG10,
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 16 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_SGBRG10,
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 16 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_SBGGR10,
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 16 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_SRGGB12,
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 16 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_SGRBG12,
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 16 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_SGBRG12,
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 16 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_SBGGR12,
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 16 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_SBGGR16,
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 16 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_Y16,
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 16 },
> + }
> +};
> +
> +static const struct cif_input_fmt in_fmts[] = {
> + {
> + .mbus_code = MEDIA_BUS_FMT_YUYV8_2X8,
> + .dvp_fmt_val = YUV_INPUT_422 | YUV_INPUT_ORDER_YUYV,
> + .csi_fmt_val = CSI_WRDDR_TYPE_YUV422,
> + .fmt_type = CIF_FMT_TYPE_YUV,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_YUYV8_2X8,
> + .dvp_fmt_val = YUV_INPUT_422 | YUV_INPUT_ORDER_YUYV,
> + .csi_fmt_val = CSI_WRDDR_TYPE_YUV422,
> + .fmt_type = CIF_FMT_TYPE_YUV,
> + .field = V4L2_FIELD_INTERLACED,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_YVYU8_2X8,
> + .dvp_fmt_val = YUV_INPUT_422 | YUV_INPUT_ORDER_YVYU,
> + .csi_fmt_val = CSI_WRDDR_TYPE_YUV422,
> + .fmt_type = CIF_FMT_TYPE_YUV,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_YVYU8_2X8,
> + .dvp_fmt_val = YUV_INPUT_422 | YUV_INPUT_ORDER_YVYU,
> + .csi_fmt_val = CSI_WRDDR_TYPE_YUV422,
> + .fmt_type = CIF_FMT_TYPE_YUV,
> + .field = V4L2_FIELD_INTERLACED,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_UYVY8_2X8,
> + .dvp_fmt_val = YUV_INPUT_422 | YUV_INPUT_ORDER_UYVY,
> + .csi_fmt_val = CSI_WRDDR_TYPE_YUV422,
> + .fmt_type = CIF_FMT_TYPE_YUV,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_UYVY8_2X8,
> + .dvp_fmt_val = YUV_INPUT_422 | YUV_INPUT_ORDER_UYVY,
> + .csi_fmt_val = CSI_WRDDR_TYPE_YUV422,
> + .fmt_type = CIF_FMT_TYPE_YUV,
> + .field = V4L2_FIELD_INTERLACED,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_VYUY8_2X8,
> + .dvp_fmt_val = YUV_INPUT_422 | YUV_INPUT_ORDER_VYUY,
> + .csi_fmt_val = CSI_WRDDR_TYPE_YUV422,
> + .fmt_type = CIF_FMT_TYPE_YUV,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_VYUY8_2X8,
> + .dvp_fmt_val = YUV_INPUT_422 | YUV_INPUT_ORDER_VYUY,
> + .csi_fmt_val = CSI_WRDDR_TYPE_YUV422,
> + .fmt_type = CIF_FMT_TYPE_YUV,
> + .field = V4L2_FIELD_INTERLACED,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SBGGR8_1X8,
> + .dvp_fmt_val = INPUT_MODE_RAW | RAW_DATA_WIDTH_8,
> + .csi_fmt_val = CSI_WRDDR_TYPE_RAW8,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SGBRG8_1X8,
> + .dvp_fmt_val = INPUT_MODE_RAW | RAW_DATA_WIDTH_8,
> + .csi_fmt_val = CSI_WRDDR_TYPE_RAW8,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SGRBG8_1X8,
> + .dvp_fmt_val = INPUT_MODE_RAW | RAW_DATA_WIDTH_8,
> + .csi_fmt_val = CSI_WRDDR_TYPE_RAW8,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SRGGB8_1X8,
> + .dvp_fmt_val = INPUT_MODE_RAW | RAW_DATA_WIDTH_8,
> + .csi_fmt_val = CSI_WRDDR_TYPE_RAW8,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10,
> + .dvp_fmt_val = INPUT_MODE_RAW | RAW_DATA_WIDTH_10,
> + .csi_fmt_val = CSI_WRDDR_TYPE_RAW10,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SGBRG10_1X10,
> + .dvp_fmt_val = INPUT_MODE_RAW | RAW_DATA_WIDTH_10,
> + .csi_fmt_val = CSI_WRDDR_TYPE_RAW10,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SGRBG10_1X10,
> + .dvp_fmt_val = INPUT_MODE_RAW | RAW_DATA_WIDTH_10,
> + .csi_fmt_val = CSI_WRDDR_TYPE_RAW10,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10,
> + .dvp_fmt_val = INPUT_MODE_RAW | RAW_DATA_WIDTH_10,
> + .csi_fmt_val = CSI_WRDDR_TYPE_RAW10,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SBGGR12_1X12,
> + .dvp_fmt_val = INPUT_MODE_RAW | RAW_DATA_WIDTH_12,
> + .csi_fmt_val = CSI_WRDDR_TYPE_RAW12,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SGBRG12_1X12,
> + .dvp_fmt_val = INPUT_MODE_RAW | RAW_DATA_WIDTH_12,
> + .csi_fmt_val = CSI_WRDDR_TYPE_RAW12,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SGRBG12_1X12,
> + .dvp_fmt_val = INPUT_MODE_RAW | RAW_DATA_WIDTH_12,
> + .csi_fmt_val = CSI_WRDDR_TYPE_RAW12,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SRGGB12_1X12,
> + .dvp_fmt_val = INPUT_MODE_RAW | RAW_DATA_WIDTH_12,
> + .csi_fmt_val = CSI_WRDDR_TYPE_RAW12,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_RGB888_1X24,
> + .csi_fmt_val = CSI_WRDDR_TYPE_RGB888,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_Y8_1X8,
> + .dvp_fmt_val = INPUT_MODE_RAW | RAW_DATA_WIDTH_8,
> + .csi_fmt_val = CSI_WRDDR_TYPE_RAW8,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_Y10_1X10,
> + .dvp_fmt_val = INPUT_MODE_RAW | RAW_DATA_WIDTH_10,
> + .csi_fmt_val = CSI_WRDDR_TYPE_RAW10,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_Y12_1X12,
> + .dvp_fmt_val = INPUT_MODE_RAW | RAW_DATA_WIDTH_12,
> + .csi_fmt_val = CSI_WRDDR_TYPE_RAW12,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }
> +};
> +
> +static const struct
> +cif_input_fmt *get_input_fmt(struct v4l2_subdev *sd)
> +{
> + struct v4l2_subdev_format fmt;
> + int ret;
> + u32 i;
> +
> + fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> + fmt.pad = 0;
> + ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt);
> + if (ret < 0) {
> + v4l2_warn(sd->v4l2_dev,
> + "sensor fmt invalid, set to default size\n");
> + goto set_default;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(in_fmts); i++)
> + if (fmt.format.code == in_fmts[i].mbus_code &&
> + fmt.format.field == in_fmts[i].field)
> + return &in_fmts[i];
> +
> + v4l2_err(sd->v4l2_dev, "remote sensor mbus code not supported\n");
> +
> +set_default:
> + return NULL;
> +}
> +
> + static const struct
Tab is off. Note that Hans already mentioned this on v1.
> +cif_output_fmt *find_output_fmt(struct rkcif_stream *stream, u32 pixelfmt)
> +{
> + const struct cif_output_fmt *fmt;
> + u32 i;
> +
> + for (i = 0; i < ARRAY_SIZE(out_fmts); i++) {
> + fmt = &out_fmts[i];
> + if (fmt->fourcc == pixelfmt)
> + return fmt;
> + }
> +
> + return NULL;
> +}
> +
> +/***************************** stream operations ******************************/
> +static void rkcif_assign_new_buffer_oneframe(struct rkcif_stream *stream)
> +{
> + struct rkcif_dummy_buffer *dummy_buf = &stream->dummy_buf;
> + struct rkcif_device *dev = stream->cifdev;
> + void __iomem *base = dev->base_addr;
> +
> + /* Set up an empty buffer for the next frame */
> + spin_lock(&stream->vbq_lock);
> + if (!list_empty(&stream->buf_head)) {
> + stream->curr_buf = list_first_entry(&stream->buf_head,
> + struct rkcif_buffer, queue);
> + list_del(&stream->curr_buf->queue);
> + } else {
> + stream->curr_buf = NULL;
> + }
> + spin_unlock(&stream->vbq_lock);
> +
> + if (stream->curr_buf) {
> + write_cif_reg(base, CIF_FRM0_ADDR_Y,
> + stream->curr_buf->buff_addr[RKCIF_PLANE_Y]);
> + write_cif_reg(base, CIF_FRM0_ADDR_UV,
> + stream->curr_buf->buff_addr[RKCIF_PLANE_CBCR]);
> + write_cif_reg(base, CIF_FRM1_ADDR_Y,
> + stream->curr_buf->buff_addr[RKCIF_PLANE_Y]);
> + write_cif_reg(base, CIF_FRM1_ADDR_UV,
> + stream->curr_buf->buff_addr[RKCIF_PLANE_CBCR]);
> + } else {
> + write_cif_reg(base, CIF_FRM0_ADDR_Y, dummy_buf->dma_addr);
> + write_cif_reg(base, CIF_FRM0_ADDR_UV, dummy_buf->dma_addr);
> + write_cif_reg(base, CIF_FRM1_ADDR_Y, dummy_buf->dma_addr);
> + write_cif_reg(base, CIF_FRM1_ADDR_UV, dummy_buf->dma_addr);
Frame 0 and 1 would seem to indicate you can do better
by implementing some double buffering. Have you tried that?
> + }
> +}
> +
> +static void rkcif_stream_stop(struct rkcif_stream *stream)
> +{
> + struct rkcif_device *cif_dev = stream->cifdev;
> + void __iomem *base = cif_dev->base_addr;
> + u32 val;
> +
> + val = read_cif_reg(base, CIF_CTRL);
> + write_cif_reg(base, CIF_CTRL, val & (~ENABLE_CAPTURE));
> + write_cif_reg(base, CIF_INTEN, 0x0);
> + write_cif_reg(base, CIF_INTSTAT, 0x3ff);
> + write_cif_reg(base, CIF_FRAME_STATUS, 0x0);
> +
> + stream->state = RKCIF_STATE_READY;
> +}
> +
> +static int rkcif_queue_setup(struct vb2_queue *queue,
> + unsigned int *num_buffers,
> + unsigned int *num_planes,
> + unsigned int sizes[],
> + struct device *alloc_devs[])
> +{
> + struct rkcif_stream *stream = queue->drv_priv;
> + const struct v4l2_pix_format_mplane *pixm;
> + const struct cif_output_fmt *cif_fmt;
> + u32 i;
> +
Better to use u32 for variables that really need a fixed width,
and so use e.g. unsigned int here.
> + pixm = &stream->pixm;
> + cif_fmt = stream->cif_fmt_out;
> +
> + if (*num_planes) {
> + if (*num_planes != cif_fmt->mplanes)
> + return -EINVAL;
> +
> + for (i = 0; i < cif_fmt->mplanes; i++)
> + if (sizes[i] < pixm->plane_fmt[i].sizeimage)
> + return -EINVAL;
> + return 0;
> + }
> +
> + *num_planes = cif_fmt->mplanes;
> +
> + for (i = 0; i < cif_fmt->mplanes; i++) {
> + const struct v4l2_plane_pix_format *plane_fmt;
> +
> + plane_fmt = &pixm->plane_fmt[i];
> + sizes[i] = plane_fmt->sizeimage;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * The vb2_buffer are stored in rkcif_buffer, in order to unify
> + * mplane buffer and none-mplane buffer.
> + */
> +static void rkcif_buf_queue(struct vb2_buffer *vb)
> +{
> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> + struct rkcif_buffer *cifbuf = to_rkcif_buffer(vbuf);
> + struct vb2_queue *queue = vb->vb2_queue;
> + struct rkcif_stream *stream = queue->drv_priv;
> + struct v4l2_pix_format_mplane *pixm = &stream->pixm;
> + const struct cif_output_fmt *fmt = stream->cif_fmt_out;
> + unsigned long lock_flags = 0;
> + int i;
> +
> + memset(cifbuf->buff_addr, 0, sizeof(cifbuf->buff_addr));
> + /* If mplanes > 1, every c-plane has its own m-plane,
> + * otherwise, multiple c-planes are in the same m-plane
> + */
> + for (i = 0; i < fmt->mplanes; i++)
> + cifbuf->buff_addr[i] = vb2_dma_contig_plane_dma_addr(vb, i);
> +
> + if (fmt->mplanes == 1) {
> + for (i = 0; i < fmt->cplanes - 1; i++)
> + cifbuf->buff_addr[i + 1] = cifbuf->buff_addr[i] +
> + pixm->plane_fmt[i].bytesperline * pixm->height;
> + }
> +
> + spin_lock_irqsave(&stream->vbq_lock, lock_flags);
> + list_add_tail(&cifbuf->queue, &stream->buf_head);
> + spin_unlock_irqrestore(&stream->vbq_lock, lock_flags);
> +}
> +
> +static int rkcif_create_dummy_buf(struct rkcif_stream *stream)
> +{
As Hans pointed out, dummy buffer needs some explanation
and details.
> + struct rkcif_dummy_buffer *dummy_buf = &stream->dummy_buf;
> + struct rkcif_device *dev = stream->cifdev;
> +
> + /* get a maximum plane size */
> + dummy_buf->size = max3(stream->pixm.plane_fmt[0].bytesperline *
> + stream->pixm.height,
> + stream->pixm.plane_fmt[1].sizeimage,
> + stream->pixm.plane_fmt[2].sizeimage);
> +
> + dummy_buf->vaddr = dma_alloc_coherent(dev->dev, dummy_buf->size,
> + &dummy_buf->dma_addr,
> + GFP_KERNEL);
> + if (!dummy_buf->vaddr) {
> + v4l2_err(&dev->v4l2_dev,
> + "Failed to allocate the memory for dummy buffer\n");
> + return -ENOMEM;
> + }
> +
> + v4l2_info(&dev->v4l2_dev, "Allocate dummy buffer, size: 0x%08x\n",
> + dummy_buf->size);
> +
Drop this v4l2_info.
> + return 0;
> +}
> +
> +static void rkcif_destroy_dummy_buf(struct rkcif_stream *stream)
> +{
> + struct rkcif_dummy_buffer *dummy_buf = &stream->dummy_buf;
> + struct rkcif_device *dev = stream->cifdev;
> +
> + dma_free_coherent(dev->dev, dummy_buf->size,
> + dummy_buf->vaddr, dummy_buf->dma_addr);
> +}
> +
> +static void rkcif_stop_streaming(struct vb2_queue *queue)
> +{
> + struct rkcif_stream *stream = queue->drv_priv;
> + struct rkcif_device *dev = stream->cifdev;
> + struct rkcif_buffer *buf;
> + struct v4l2_subdev *sd;
> + int ret;
> +
> + stream->stopping = true;
> + ret = wait_event_timeout(stream->wq_stopped,
> + stream->state != RKCIF_STATE_STREAMING,
> + msecs_to_jiffies(1000));
> + if (!ret) {
> + rkcif_stream_stop(stream);
> + stream->stopping = false;
> + }
> + pm_runtime_put(dev->dev);
> +
> + /* stop the sub device*/
> + sd = dev->sensor.sd;
> + v4l2_subdev_call(sd, video, s_stream, 0);
> + v4l2_subdev_call(sd, core, s_power, 0);
> +
> + /* release buffers */
> + if (stream->curr_buf) {
> + list_add_tail(&stream->curr_buf->queue, &stream->buf_head);
> + stream->curr_buf = NULL;
> + }
> + if (stream->next_buf) {
Next buffer would seem to indicate some attempt at double-buffering,
but it's not really used.
> + list_add_tail(&stream->next_buf->queue, &stream->buf_head);
> + stream->next_buf = NULL;
> + }
> +
> + while (!list_empty(&stream->buf_head)) {
> + buf = list_first_entry(&stream->buf_head,
> + struct rkcif_buffer, queue);
> + list_del(&buf->queue);
> + vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> + }
> +
> + rkcif_destroy_dummy_buf(stream);
> +}
> +
> +static u32 rkcif_determine_input_mode(struct rkcif_device *dev)
> +{
> + struct rkcif_sensor_info *sensor_info = &dev->sensor;
> + struct rkcif_stream *stream = &dev->stream;
> + v4l2_std_id std;
> + u32 mode = INPUT_MODE_YUV;
> + int ret;
> +
> + ret = v4l2_subdev_call(sensor_info->sd, video, querystd, &std);
> + if (ret == 0) {
> + /* retrieve std from sensor if exist */
> + switch (std) {
> + case V4L2_STD_NTSC:
> + mode = INPUT_MODE_NTSC;
> + break;
> + case V4L2_STD_PAL:
> + mode = INPUT_MODE_PAL;
> + break;
> + case V4L2_STD_ATSC:
> + mode = INPUT_MODE_BT1120;
> + break;
> + default:
> + v4l2_err(&dev->v4l2_dev,
> + "std: %lld is not supported", std);
> + }
> + } else {
> + /* determine input mode by mbus_code (fmt_type) */
> + switch (stream->cif_fmt_in->fmt_type) {
> + case CIF_FMT_TYPE_YUV:
> + mode = INPUT_MODE_YUV;
> + break;
> + case CIF_FMT_TYPE_RAW:
> + mode = INPUT_MODE_RAW;
> + break;
> + }
> + }
> +
> + return mode;
> +}
> +
> +static inline u32 rkcif_scl_ctl(struct rkcif_stream *stream)
> +{
> + u32 fmt_type = stream->cif_fmt_in->fmt_type;
> +
> + return (fmt_type == CIF_FMT_TYPE_YUV) ?
> + ENABLE_YUV_16BIT_BYPASS : ENABLE_RAW_16BIT_BYPASS;
> +}
> +
> +static int rkcif_stream_start(struct rkcif_stream *stream)
> +{
> + u32 val, mbus_flags, href_pol, vsync_pol,
> + xfer_mode = 0, yc_swap = 0, skip_top = 0;
Tab is off here.
> + struct rkcif_device *dev = stream->cifdev;
> + struct rkcif_sensor_info *sensor_info;
> + void __iomem *base = dev->base_addr;
> +
> + sensor_info = &dev->sensor;
> + stream->frame_idx = 0;
> +
> + mbus_flags = sensor_info->mbus.flags;
> + href_pol = (mbus_flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH) ?
> + HSY_HIGH_ACTIVE : HSY_LOW_ACTIVE;
> + vsync_pol = (mbus_flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH) ?
> + VSY_HIGH_ACTIVE : VSY_LOW_ACTIVE;
> +
> + if (rkcif_determine_input_mode(dev) == INPUT_MODE_BT1120) {
> + if (stream->cif_fmt_in->field == V4L2_FIELD_NONE)
> + xfer_mode = BT1120_TRANSMIT_PROGRESS;
> + else
> + xfer_mode = BT1120_TRANSMIT_INTERFACE;
> + if (!CIF_FETCH_IS_Y_FIRST(stream->cif_fmt_in->dvp_fmt_val))
> + yc_swap = BT1120_YC_SWAP;
> + }
> +
> + val = vsync_pol | href_pol | rkcif_determine_input_mode(dev) |
> + stream->cif_fmt_out->fmt_val | stream->cif_fmt_in->dvp_fmt_val |
> + xfer_mode | yc_swap;
> + write_cif_reg(base, CIF_FOR, val);
> + val = stream->pixm.width;
> + if (stream->cif_fmt_in->fmt_type == CIF_FMT_TYPE_RAW)
> + val = stream->pixm.width * 2;
> + write_cif_reg(base, CIF_VIR_LINE_WIDTH, val);
> + write_cif_reg(base, CIF_SET_SIZE,
> + stream->pixm.width | (stream->pixm.height << 16));
> +
> + v4l2_subdev_call(sensor_info->sd, sensor, g_skip_top_lines, &skip_top);
> +
> + write_cif_reg(base, CIF_CROP, skip_top << CIF_CROP_Y_SHIFT);
> + write_cif_reg(base, CIF_FRAME_STATUS, FRAME_STAT_CLS);
> + write_cif_reg(base, CIF_INTSTAT, INTSTAT_CLS);
> + write_cif_reg(base, CIF_SCL_CTRL, rkcif_scl_ctl(stream));
> +
> + rkcif_assign_new_buffer_oneframe(stream);
> +
> + write_cif_reg(base, CIF_INTEN, FRAME_END_EN | LINE_ERR_EN |
> + PST_INF_FRAME_END);
> +
> + if (dev->data->chip_id == CHIP_RK1808_CIF &&
> + rkcif_determine_input_mode(dev) == INPUT_MODE_BT1120)
> + write_cif_reg(base, CIF_CTRL,
> + AXI_BURST_16 | MODE_PINGPONG | ENABLE_CAPTURE);
> + else
> + write_cif_reg(base, CIF_CTRL,
> + AXI_BURST_16 | MODE_ONEFRAME | ENABLE_CAPTURE);
> +
> + stream->state = RKCIF_STATE_STREAMING;
> +
> + return 0;
> +}
> +
> +static int rkcif_start_streaming(struct vb2_queue *queue, unsigned int count)
> +{
> + struct rkcif_stream *stream = queue->drv_priv;
> + struct rkcif_device *dev = stream->cifdev;
> + struct v4l2_device *v4l2_dev = &dev->v4l2_dev;
> + struct v4l2_subdev *sd;
> + int ret;
> +
> + if (WARN_ON(stream->state != RKCIF_STATE_READY)) {
This check should not be needed.
> + ret = -EBUSY;
> + v4l2_err(v4l2_dev, "stream in busy state\n");
> + goto destroy_buf;
> + }
> +
> + stream->cif_fmt_in = get_input_fmt(dev->sensor.sd);
> +
> + ret = rkcif_create_dummy_buf(stream);
> + if (ret < 0) {
> + v4l2_err(v4l2_dev, "Failed to create dummy_buf, %d\n", ret);
> + goto destroy_buf;
> + }
> +
> + ret = pm_runtime_get_sync(dev->dev);
> + if (ret < 0) {
> + v4l2_err(v4l2_dev, "Failed to get runtime pm, %d\n", ret);
> + goto destroy_dummy_buf;
> + }
> +
> + /* start sub-devices */
> + sd = dev->sensor.sd;
> + ret = v4l2_subdev_call(sd, core, s_power, 1);
> + if (ret < 0 && ret != -ENOIOCTLCMD)
> + goto runtime_put;
> + ret = v4l2_subdev_call(sd, video, s_stream, 1);
> + if (ret < 0)
> + goto subdev_poweroff;
> +
> + ret = rkcif_stream_start(stream);
> + if (ret < 0)
> + goto stop_stream;
> +
> + return 0;
> +
> +stop_stream:
> + rkcif_stream_stop(stream);
> +subdev_poweroff:
> + v4l2_subdev_call(sd, core, s_power, 0);
> +runtime_put:
> + pm_runtime_put(dev->dev);
> +destroy_dummy_buf:
> + rkcif_destroy_dummy_buf(stream);
> +destroy_buf:
> + while (!list_empty(&stream->buf_head)) {
Move this to a helper?
> + struct rkcif_buffer *buf;
> +
> + buf = list_first_entry(&stream->buf_head,
> + struct rkcif_buffer, queue);
> + list_del(&buf->queue);
> + vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_QUEUED);
> + }
> +
> + return ret;
> +}
> +
> +static struct vb2_ops rkcif_vb2_ops = {
> + .queue_setup = rkcif_queue_setup,
> + .buf_queue = rkcif_buf_queue,
> + .wait_prepare = vb2_ops_wait_prepare,
> + .wait_finish = vb2_ops_wait_finish,
> + .stop_streaming = rkcif_stop_streaming,
> + .start_streaming = rkcif_start_streaming,
> +};
> +
> +static int rkcif_init_vb2_queue(struct vb2_queue *q,
> + struct rkcif_stream *stream,
> + enum v4l2_buf_type buf_type)
> +{
> + q->type = buf_type;
> + q->io_modes = VB2_MMAP | VB2_DMABUF;
> + q->drv_priv = stream;
> + q->ops = &rkcif_vb2_ops;
> + q->mem_ops = &vb2_dma_contig_memops;
I can't find any CPU mapping, so maybe you'll want to
have q->dma_attrs = DMA_ATTR_NO_KERNEL_MAPPING.
> + q->buf_struct_size = sizeof(struct rkcif_buffer);
> + q->min_buffers_needed = CIF_REQ_BUFS_MIN;
> + q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> + q->lock = &stream->vlock;
> + q->dev = stream->cifdev->dev;
> +
> + return vb2_queue_init(q);
> +}
> +
> +static void rkcif_set_fmt(struct rkcif_stream *stream,
> + struct v4l2_pix_format_mplane *pixm,
> + bool try)
> +{
> + const struct cif_output_fmt *fmt;
> + struct v4l2_rect input_rect;
> + unsigned int imagesize = 0, planes;
> + u32 xsubs = 1, ysubs = 1, i;
> +
> + fmt = find_output_fmt(stream, pixm->pixelformat);
> + if (!fmt)
> + fmt = &out_fmts[0];
> +
> + input_rect.width = CIF_MAX_WIDTH;
> + input_rect.height = CIF_MAX_HEIGHT;
> +
> + pixm->width = clamp_t(u32, pixm->width,
> + CIF_MIN_WIDTH, input_rect.width);
> + pixm->height = clamp_t(u32, pixm->height,
> + CIF_MIN_HEIGHT, input_rect.height);
> +
> + pixm->num_planes = fmt->mplanes;
> + pixm->quantization = V4L2_QUANTIZATION_DEFAULT;
> + pixm->colorspace = V4L2_COLORSPACE_SRGB;
> +
See Hans' comment on v1.
> + pixm->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(pixm->colorspace);
> + pixm->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(pixm->colorspace);
> +
> + pixm->pixelformat = fmt->fourcc;
> + pixm->field = V4L2_FIELD_NONE;
> +
> + /* calculate plane size and image size */
> + fcc_xysubs(fmt->fourcc, &xsubs, &ysubs);
> +
> + planes = fmt->cplanes ? fmt->cplanes : fmt->mplanes;
> +
> + for (i = 0; i < planes; i++) {
> + struct v4l2_plane_pix_format *plane_fmt;
> + int width, height, bpl, size;
> +
> + if (i == 0) {
> + width = pixm->width;
> + height = pixm->height;
> + } else {
> + width = pixm->width / xsubs;
> + height = pixm->height / ysubs;
> + }
> +
> + bpl = width * fmt->bpp[i] / 8;
> + size = bpl * height;
> + imagesize += size;
> +
> + if (fmt->mplanes > i) {
> + /* Set bpl and size for each mplane */
> + plane_fmt = pixm->plane_fmt + i;
> + plane_fmt->bytesperline = bpl;
> + plane_fmt->sizeimage = size;
> + }
> + }
> +
> + /* convert to non-MPLANE format.
> + * It's important since we want to unify non-MPLANE
I suggest to only support MPLANE API and make your life simpler.
> + * and MPLANE.
> + */
> + if (fmt->mplanes == 1)
> + pixm->plane_fmt[0].sizeimage = imagesize;
> +
> + if (!try) {
> + stream->cif_fmt_out = fmt;
> + stream->pixm = *pixm;
> + }
> +}
> +
> +void rkcif_stream_init(struct rkcif_device *dev)
> +{
> + struct rkcif_stream *stream = &dev->stream;
> + struct v4l2_pix_format_mplane pixm;
> +
> + memset(stream, 0, sizeof(*stream));
> + memset(&pixm, 0, sizeof(pixm));
> + stream->cifdev = dev;
> +
> + INIT_LIST_HEAD(&stream->buf_head);
> + spin_lock_init(&stream->vbq_lock);
> + stream->state = RKCIF_STATE_READY;
> + init_waitqueue_head(&stream->wq_stopped);
> +
> + /* Set default format */
> + pixm.pixelformat = V4L2_PIX_FMT_NV12;
> + pixm.width = RKCIF_DEFAULT_WIDTH;
> + pixm.height = RKCIF_DEFAULT_HEIGHT;
> + rkcif_set_fmt(stream, &pixm, false);
> +
> + stream->crop.left = 0;
> + stream->crop.top = 0;
> + stream->crop.width = 10;
> + stream->crop.height = 10;
As already mentioned by Hans on the v1, please drop.
> +}
> +
> +static int rkcif_fh_open(struct file *filp)
> +{
> + struct video_device *vdev = video_devdata(filp);
> + struct rkcif_stream *stream = to_rkcif_stream(vdev);
> + struct rkcif_device *cifdev = stream->cifdev;
> +
> + rkcif_soft_reset(cifdev);
> +
As already mentioned by Hans, looks problematic.
> + return v4l2_fh_open(filp);
> +}
> +
> +static const struct v4l2_file_operations rkcif_fops = {
> + .open = rkcif_fh_open,
> + .release = vb2_fop_release,
> + .unlocked_ioctl = video_ioctl2,
> + .poll = vb2_fop_poll,
> + .mmap = vb2_fop_mmap,
> +};
> +
> +static int rkcif_enum_input(struct file *file, void *priv,
> + struct v4l2_input *input)
Why supporting the Input ioctls?
> +{
> + if (input->index > 0)
> + return -EINVAL;
> +
> + input->type = V4L2_INPUT_TYPE_CAMERA;
> + strlcpy(input->name, "Camera", sizeof(input->name));
> +
> + return 0;
> +}
> +
> +static int rkcif_try_fmt_vid_cap_mplane(struct file *file, void *fh,
> + struct v4l2_format *f)
> +{
> + struct rkcif_stream *stream = video_drvdata(file);
> +
> + rkcif_set_fmt(stream, &f->fmt.pix_mp, true);
> +
> + return 0;
> +}
> +
> +static int rkcif_enum_fmt_vid_cap(struct file *file, void *priv,
> + struct v4l2_fmtdesc *f)
> +{
> + const struct cif_output_fmt *fmt = NULL;
> +
> + if (f->index >= ARRAY_SIZE(out_fmts))
> + return -EINVAL;
> +
> + fmt = &out_fmts[f->index];
> + f->pixelformat = fmt->fourcc;
> +
> + return 0;
> +}
> +
> +static int rkcif_s_fmt_vid_cap_mplane(struct file *file,
> + void *priv, struct v4l2_format *f)
> +{
> + struct rkcif_stream *stream = video_drvdata(file);
> +
> + rkcif_set_fmt(stream, &f->fmt.pix_mp, false);
> +
> + return 0;
> +}
> +
> +static int rkcif_g_fmt_vid_cap_mplane(struct file *file, void *fh,
> + struct v4l2_format *f)
> +{
> + struct rkcif_stream *stream = video_drvdata(file);
> +
> + f->fmt.pix_mp = stream->pixm;
> +
> + return 0;
> +}
> +
> +static int rkcif_querycap(struct file *file, void *priv,
> + struct v4l2_capability *cap)
> +{
> + struct rkcif_stream *stream = video_drvdata(file);
> + struct device *dev = stream->cifdev->dev;
> +
> + strlcpy(cap->driver, dev->driver->name, sizeof(cap->driver));
> + strlcpy(cap->card, dev->driver->name, sizeof(cap->card));
> + snprintf(cap->bus_info, sizeof(cap->bus_info),
> + "platform:%s", dev_name(dev));
> +
> + return 0;
> +}
> +
> +static int rkcif_enum_framesizes(struct file *file, void *fh,
> + struct v4l2_frmsizeenum *fsize)
> +{
> + struct rkcif_stream *stream = video_drvdata(file);
> + struct rkcif_device *dev = stream->cifdev;
> + struct v4l2_subdev_frame_size_enum fse = {
> + .index = fsize->index,
> + .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> + };
> + const struct cif_output_fmt *fmt;
> + int ret;
> +
> + if (!dev->sensor.sd)
> + return -EINVAL;
> +
> + fmt = find_output_fmt(stream, fsize->pixel_format);
> + if (!fmt)
> + return -EINVAL;
> +
> + fse.code = fmt->mbus;
> +
> + ret = v4l2_subdev_call(dev->sensor.sd, pad, enum_frame_size,
> + NULL, &fse);
> + if (ret)
> + return ret;
> +
> + fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
> + fsize->discrete.width = fse.max_width;
> + fsize->discrete.height = fse.max_height;
> +
> + return 0;
> +}
> +
> +static int rkcif_g_input(struct file *file, void *fh, unsigned int *i)
> +{
> + *i = 0;
> + return 0;
> +}
> +
> +static int rkcif_s_input(struct file *file, void *fh, unsigned int i)
> +{
> + if (i)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static const struct v4l2_ioctl_ops rkcif_v4l2_ioctl_ops = {
> + .vidioc_reqbufs = vb2_ioctl_reqbufs,
> + .vidioc_querybuf = vb2_ioctl_querybuf,
> + .vidioc_create_bufs = vb2_ioctl_create_bufs,
> + .vidioc_qbuf = vb2_ioctl_qbuf,
> + .vidioc_expbuf = vb2_ioctl_expbuf,
> + .vidioc_dqbuf = vb2_ioctl_dqbuf,
> + .vidioc_prepare_buf = vb2_ioctl_prepare_buf,
> + .vidioc_streamon = vb2_ioctl_streamon,
> + .vidioc_streamoff = vb2_ioctl_streamoff,
> +
> + .vidioc_enum_fmt_vid_cap = rkcif_enum_fmt_vid_cap,
> + .vidioc_try_fmt_vid_cap_mplane = rkcif_try_fmt_vid_cap_mplane,
> + .vidioc_s_fmt_vid_cap_mplane = rkcif_s_fmt_vid_cap_mplane,
> + .vidioc_g_fmt_vid_cap_mplane = rkcif_g_fmt_vid_cap_mplane,
> + .vidioc_querycap = rkcif_querycap,
> + .vidioc_enum_framesizes = rkcif_enum_framesizes,
> +
> + .vidioc_enum_input = rkcif_enum_input,
> + .vidioc_g_input = rkcif_g_input,
> + .vidioc_s_input = rkcif_s_input,
> +};
> +
> +void rkcif_unregister_stream_vdev(struct rkcif_device *dev)
> +{
> + struct rkcif_stream *stream = &dev->stream;
> +
> + media_entity_cleanup(&stream->vdev.entity);
> + video_unregister_device(&stream->vdev);
> +}
> +
> +int rkcif_register_stream_vdev(struct rkcif_device *dev)
> +{
> + struct rkcif_stream *stream = &dev->stream;
> + struct v4l2_device *v4l2_dev = &dev->v4l2_dev;
> + struct video_device *vdev = &stream->vdev;
> + int ret;
> +
> + strlcpy(vdev->name, CIF_VIDEODEVICE_NAME, sizeof(vdev->name));
> + mutex_init(&stream->vlock);
> +
> + vdev->ioctl_ops = &rkcif_v4l2_ioctl_ops;
> + vdev->release = video_device_release_empty;
> + vdev->fops = &rkcif_fops;
> + vdev->minor = -1;
> + vdev->v4l2_dev = v4l2_dev;
> + vdev->lock = &stream->vlock;
> + vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE_MPLANE |
> + V4L2_CAP_STREAMING;
> + video_set_drvdata(vdev, stream);
> + vdev->vfl_dir = VFL_DIR_RX;
> + stream->pad.flags = MEDIA_PAD_FL_SINK;
> +
> + rkcif_init_vb2_queue(&stream->buf_queue, stream,
> + V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE |
> + V4L2_BUF_TYPE_VIDEO_CAPTURE);
As suggested, drop V4L2_BUF_TYPE_VIDEO_CAPTURE.
> + vdev->queue = &stream->buf_queue;
> + strscpy(vdev->name, KBUILD_MODNAME, sizeof(vdev->name));
> +
> + ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
> + if (ret < 0) {
> + v4l2_err(v4l2_dev,
> + "video_register_device failed with error %d\n", ret);
> + return ret;
> + }
> +
> + ret = media_entity_pads_init(&vdev->entity, 1, &stream->pad);
> + if (ret < 0)
> + goto unreg;
> +
> + return 0;
> +unreg:
> + video_unregister_device(vdev);
> + return ret;
> +}
> +
> +static void rkcif_vb_done_oneframe(struct rkcif_stream *stream,
Why this "oneframe" naming? Is there some other mode planned?
> + struct vb2_v4l2_buffer *vb_done)
> +{
> + const struct cif_output_fmt *fmt = stream->cif_fmt_out;
> + u32 i;
> +
> + /* Dequeue a filled buffer */
> + for (i = 0; i < fmt->mplanes; i++) {
> + vb2_set_plane_payload(&vb_done->vb2_buf, i,
> + stream->pixm.plane_fmt[i].sizeimage);
> + }
> + vb_done->vb2_buf.timestamp = ktime_get_ns();
vb_done->vb2_buf.sequence needs to be set.
Also, you can use it to mark drop buffers. See below.
> + vb2_buffer_done(&vb_done->vb2_buf, VB2_BUF_STATE_DONE);
> +}
> +
> +void rkcif_irq_oneframe(struct rkcif_device *cif_dev)
> +{
> + struct rkcif_stream *stream = &cif_dev->stream;
> + u32 lastline, lastpix, ctl, cif_frmst, intstat;
> + void __iomem *base = cif_dev->base_addr;
> +
> + intstat = read_cif_reg(base, CIF_INTSTAT);
> + cif_frmst = read_cif_reg(base, CIF_FRAME_STATUS);
> + lastline = CIF_FETCH_Y_LAST_LINE(read_cif_reg(base, CIF_LAST_LINE));
> + lastpix = read_cif_reg(base, CIF_LAST_PIX);
> + ctl = read_cif_reg(base, CIF_CTRL);
> +
> + /* There are two irqs enabled:
> + * - PST_INF_FRAME_END: cif FIFO is ready, this is prior to FRAME_END
> + * - FRAME_END: cif has saved frame to memory, a frame ready
> + */
> +
> + if ((intstat & PST_INF_FRAME_END)) {
> + write_cif_reg(base, CIF_INTSTAT, PST_INF_FRAME_END_CLR);
> +
> + if (stream->stopping)
> + /* To stop CIF ASAP, before FRAME_END irq */
> + write_cif_reg(base, CIF_CTRL, ctl & (~ENABLE_CAPTURE));
> + }
> +
> + if ((intstat & LINE_ERR)) {
> + write_cif_reg(base, CIF_INTSTAT, LINE_ERR_CLR);
> +
> + if (stream->stopping) {
> + rkcif_stream_stop(stream);
> + stream->stopping = false;
> + wake_up(&stream->wq_stopped);
> + return;
> + }
> +
> + v4l2_err(&cif_dev->v4l2_dev,
> + "Bad frame, irq:0x%x frmst:0x%x size:%dx%d\n",
> + intstat, cif_frmst, lastline, lastpix);
> + /* Clear status to receive into the same buffer */
> + write_cif_reg(base, CIF_FRAME_STATUS, FRM0_STAT_CLS);
> + return;
> + }
> +
> +
> + if ((intstat & FRAME_END)) {
> + struct vb2_v4l2_buffer *vb_done = NULL;
> +
> + write_cif_reg(base, CIF_INTSTAT, FRAME_END_CLR);
> +
> + if (stream->stopping) {
> + rkcif_stream_stop(stream);
> + stream->stopping = false;
> + wake_up(&stream->wq_stopped);
> + return;
> + }
> +
> + if (lastline != stream->pixm.height ||
> + !(cif_frmst & CIF_F0_READY)) {
> + v4l2_err(&cif_dev->v4l2_dev,
> + "Bad frame, irq:0x%x frmst:0x%x size:%dx%d\n",
> + intstat, cif_frmst, lastline, lastpix);
> + /* Clear status to receive into the same buffer */
> + write_cif_reg(base, CIF_FRAME_STATUS, FRM0_STAT_CLS);
> + return;
> + }
> +
> + if (stream->curr_buf)
> + vb_done = &stream->curr_buf->vb;
> + rkcif_assign_new_buffer_oneframe(stream);
> +
> + /* In one-frame mode, must clear status manually to enable
> + * the next frame end irq
> + */
> + write_cif_reg(base, CIF_FRAME_STATUS, FRM0_STAT_CLS);
> +
> + if (vb_done)
> + rkcif_vb_done_oneframe(stream, vb_done);
> +
> + stream->frame_idx++;
Although it's currently unused, frame_idx will be useful to set the sequence.
Now, it would be interesting to keep track of captures to the dummy buffer
(i.e. dropped buffers) vs. captures to real buffers (captured frames).
Skipping dropped buffers from the sequence would allow userspace
applications to detect that something is going wrong.
You could also expose this captured/dropped counters in debugfs.
> + }
> +}
> diff --git a/drivers/media/platform/rockchip/cif/dev.c b/drivers/media/platform/rockchip/cif/dev.c
> new file mode 100644
> index 000000000000..dbd5fdbd1cef
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/cif/dev.c
> @@ -0,0 +1,358 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Rockchip CIF Driver
> + *
> + * Copyright (C) 2018 Rockchip Electronics Co., Ltd.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_graph.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_reserved_mem.h>
> +#include <linux/reset.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <media/v4l2-fwnode.h>
> +
> +#include "dev.h"
> +#include "regs.h"
> +
> +#define RKCIF_VERNO_LEN 10
> +
> +static int rkcif_create_links(struct rkcif_device *dev)
> +{
> + struct v4l2_subdev *sd = dev->sensor.sd;
> + int ret;
> +
> + ret = media_entity_get_fwnode_pad(&sd->entity, sd->fwnode,
> + MEDIA_PAD_FL_SOURCE);
> + if (ret)
> + return ret;
> +
> + ret = media_create_pad_link(&sd->entity, 0,
> + &dev->stream.vdev.entity, 0,
> + MEDIA_LNK_FL_ENABLED);
> + if (ret) {
> + dev_err(dev->dev, "failed to create link");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int subdev_notifier_complete(struct v4l2_async_notifier *notifier)
> +{
> + struct rkcif_device *dev;
> + int ret;
> +
> + dev = container_of(notifier, struct rkcif_device, notifier);
> +
> + mutex_lock(&dev->media_dev.graph_mutex);
> +
> + ret = rkcif_create_links(dev);
> + if (ret < 0)
> + goto unlock;
> +
> + ret = v4l2_device_register_subdev_nodes(&dev->v4l2_dev);
> + if (ret < 0)
> + goto unlock;
> +
> +unlock:
> + mutex_unlock(&dev->media_dev.graph_mutex);
> + return ret;
> +}
> +
> +static int subdev_notifier_bound(struct v4l2_async_notifier *notifier,
> + struct v4l2_subdev *subdev,
> + struct v4l2_async_subdev *asd)
> +{
> + struct rkcif_device *cif_dev = container_of(notifier,
> + struct rkcif_device, notifier);
> +
> + int pad;
> +
> + cif_dev->sensor.sd = subdev;
> + pad = media_entity_get_fwnode_pad(&subdev->entity, subdev->fwnode,
> + MEDIA_PAD_FL_SOURCE);
> + if (pad < 0)
> + return pad;
> +
> + cif_dev->sensor.pad = pad;
> +
> + return 0;
> +}
> +
> +static const struct v4l2_async_notifier_operations subdev_notifier_ops = {
> + .bound = subdev_notifier_bound,
> + .complete = subdev_notifier_complete,
> +};
> +
> +static int cif_subdev_notifier(struct rkcif_device *cif_dev)
> +{
> + struct v4l2_async_notifier *ntf = &cif_dev->notifier;
> + struct device *dev = cif_dev->dev;
> + struct v4l2_fwnode_endpoint vep = {
> + .bus_type = V4L2_MBUS_PARALLEL,
> + };
> + struct fwnode_handle *ep;
> + int ret;
> +
> + v4l2_async_notifier_init(ntf);
> +
> + ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0,
> + FWNODE_GRAPH_ENDPOINT_NEXT);
> + if (!ep)
> + return -EINVAL;
> +
> + ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> + if (ret)
> + return ret;
> +
> + ret = v4l2_async_notifier_add_fwnode_remote_subdev(ntf, ep,
> + &cif_dev->asd);
> + if (ret)
> + return ret;
> +
> + ntf->ops = &subdev_notifier_ops;
> +
> + fwnode_handle_put(ep);
> +
> + ret = v4l2_async_notifier_register(&cif_dev->v4l2_dev, ntf);
> + return ret;
> +}
> +
> +static int rkcif_register_platform_subdevs(struct rkcif_device *cif_dev)
> +{
> + int ret;
> +
> + ret = rkcif_register_stream_vdev(cif_dev);
> + if (ret < 0)
> + return ret;
> +
> + ret = cif_subdev_notifier(cif_dev);
> + if (ret < 0) {
> + v4l2_err(&cif_dev->v4l2_dev,
> + "Failed to register subdev notifier(%d)\n", ret);
> + rkcif_unregister_stream_vdev(cif_dev);
> + }
> +
> + return 0;
> +}
> +
> +static struct clk_bulk_data px30_cif_clks[] = {
> + { .id = "aclk" },
> + { .id = "hclk" },
> + { .id = "pclkin" },
> +};
> +
> +static const struct cif_match_data px30_cif_match_data = {
> + .chip_id = CHIP_PX30_CIF,
> + .clks = px30_cif_clks,
> + .clks_num = ARRAY_SIZE(px30_cif_clks),
> +};
> +
> +static const struct of_device_id rkcif_plat_of_match[] = {
> + {
> + .compatible = "rockchip,px30-cif",
> + .data = &px30_cif_match_data,
> + },
> + {},
> +};
> +
> +static irqreturn_t rkcif_irq_handler(int irq, void *ctx)
> +{
> + struct device *dev = ctx;
> + struct rkcif_device *cif_dev = dev_get_drvdata(dev);
> +
> + rkcif_irq_oneframe(cif_dev);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void rkcif_disable_sys_clk(struct rkcif_device *cif_dev)
> +{
> + int i;
> +
> + for (i = cif_dev->data->clks_num - 1; i >= 0; i--)
> + clk_disable_unprepare(cif_dev->clks[i]);
clk_bulk_disable_unprepare
> +}
> +
> +static int rkcif_enable_sys_clk(struct rkcif_device *cif_dev)
> +{
> + int i, ret = -EINVAL;
> +
> + for (i = 0; i < cif_dev->data->clks_num; i++) {
> + ret = clk_prepare_enable(cif_dev->clks[i]);
> +
clk_bulk_enable_prepare
> + if (ret < 0)
> + goto err;
> + }
> +
> + return 0;
> +
> +err:
> + for (--i; i >= 0; --i)
> + clk_disable_unprepare(cif_dev->clks[i]);
> +
> + return ret;
> +}
> +
> +void rkcif_soft_reset(struct rkcif_device *cif_dev)
> +{
> + reset_control_assert(cif_dev->cif_rst);
> +
> + udelay(5);
> +
> + reset_control_deassert(cif_dev->cif_rst);
> +}
> +
> +static int rkcif_plat_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct v4l2_device *v4l2_dev;
> + struct rkcif_device *cif_dev;
> + const struct cif_match_data *data;
> + int ret, irq;
> +
> + cif_dev = devm_kzalloc(dev, sizeof(*cif_dev), GFP_KERNEL);
> + if (!cif_dev)
> + return -ENOMEM;
> +
> + dev_set_drvdata(dev, cif_dev);
> + cif_dev->dev = dev;
> +
> + data = of_device_get_match_data(&pdev->dev);
> + if (!data)
> + return -EINVAL;
> +
> + cif_dev->data = data;
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0)
> + return irq;
> +
> + ret = devm_request_irq(dev, irq, rkcif_irq_handler, IRQF_SHARED,
I guess it's shared with the IOMMU, or is this a mistake?
Note that you seem to always handle the interrupt.
> + dev_driver_string(dev), dev);
> + if (ret < 0) {
> + dev_err(dev, "request irq failed: %d\n", ret);
> + return ret;
> + }
> +
> + cif_dev->irq = irq;
> +
> + cif_dev->base_addr = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(cif_dev->base_addr))
> + return PTR_ERR(cif_dev->base_addr);
> +
> + ret = of_reserved_mem_device_init(dev);
Can't find any mention to this reserved memory in the bindings documentation.
Perhaps you can add an example and/or add some comments for this?
> + if (ret)
> + v4l2_info(v4l2_dev, "No reserved memory region assign to CIF\n");
> +
> + ret = devm_clk_bulk_get(dev, data->clks_num, data->clks);
> + if (ret)
> + return ret;
> +
> + cif_dev->cif_rst = devm_reset_control_array_get(dev, false, false);
> + if (IS_ERR(cif_dev->cif_rst))
> + return PTR_ERR(cif_dev->cif_rst);
> +
> + /* Initialize the stream */
> + rkcif_stream_init(cif_dev);
> +
> + strlcpy(cif_dev->media_dev.model, "rkcif",
> + sizeof(cif_dev->media_dev.model));
> + cif_dev->media_dev.dev = &pdev->dev;
> + v4l2_dev = &cif_dev->v4l2_dev;
> + v4l2_dev->mdev = &cif_dev->media_dev;
> + strlcpy(v4l2_dev->name, "rkcif", sizeof(v4l2_dev->name));
> + v4l2_ctrl_handler_init(&cif_dev->ctrl_handler, 8);
> + v4l2_dev->ctrl_handler = &cif_dev->ctrl_handler;
> +
> + ret = v4l2_device_register(cif_dev->dev, &cif_dev->v4l2_dev);
> + if (ret < 0)
> + return ret;
> +
> + media_device_init(&cif_dev->media_dev);
> +
> + ret = media_device_register(&cif_dev->media_dev);
> + if (ret < 0) {
> + v4l2_err(v4l2_dev, "Failed to register media device: %d\n",
> + ret);
> + goto err_unreg_v4l2_dev;
> + }
> +
> + /* create & register platefom subdev (from of_node) */
> + ret = rkcif_register_platform_subdevs(cif_dev);
> + if (ret < 0)
> + goto err_unreg_media_dev;
> +
> + pm_runtime_enable(&pdev->dev);
> +
Off the top of my head, I believe enabling PM runtime
after registering the device to the world could be problematic.
> + return 0;
> +
> +err_unreg_media_dev:
> + media_device_unregister(&cif_dev->media_dev);
> +err_unreg_v4l2_dev:
> + v4l2_device_unregister(&cif_dev->v4l2_dev);
> + return ret;
> +}
> +
> +static int rkcif_plat_remove(struct platform_device *pdev)
> +{
> + struct rkcif_device *cif_dev = platform_get_drvdata(pdev);
> +
> + pm_runtime_disable(&pdev->dev);
> +
> + media_device_unregister(&cif_dev->media_dev);
> + v4l2_device_unregister(&cif_dev->v4l2_dev);
> + rkcif_unregister_stream_vdev(cif_dev);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused rkcif_runtime_suspend(struct device *dev)
> +{
> + struct rkcif_device *cif_dev = dev_get_drvdata(dev);
> +
> + rkcif_disable_sys_clk(cif_dev);
> +
> + return pinctrl_pm_select_sleep_state(dev);
> +}
> +
> +static int __maybe_unused rkcif_runtime_resume(struct device *dev)
> +{
> + struct rkcif_device *cif_dev = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = pinctrl_pm_select_default_state(dev);
> + if (ret < 0)
> + return ret;
> + rkcif_enable_sys_clk(cif_dev);
> +
Just move the clock handling here and avoid defining rkcif_enable_sys_clk ?
> + return 0;
> +}
> +
> +static const struct dev_pm_ops rkcif_plat_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> + pm_runtime_force_resume)
> + SET_RUNTIME_PM_OPS(rkcif_runtime_suspend, rkcif_runtime_resume, NULL)
> +};
> +
> +static struct platform_driver rkcif_plat_drv = {
> + .driver = {
> + .name = CIF_DRIVER_NAME,
> + .of_match_table = of_match_ptr(rkcif_plat_of_match),
> + .pm = &rkcif_plat_pm_ops,
> + },
> + .probe = rkcif_plat_probe,
> + .remove = rkcif_plat_remove,
> +};
> +
> +module_platform_driver(rkcif_plat_drv);
> +MODULE_AUTHOR("Rockchip Camera/ISP team");
MODULE_AUTHOR goes to the module's metadata,
e.g. for users to query driver's authors.
I've had to upstream drivers originally written by other
people, and I've wondered what's the value of
putting a team as author.
> +MODULE_DESCRIPTION("Rockchip CIF platform driver");
> +MODULE_LICENSE("GPL");
Thanks for the hard work,
Ezequiel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* RE: [PATCH v2 1/5] scsi: ufs-mediatek: Fix imprecise waiting time for ref-clk control
From: Stanley Chu @ 2020-05-31 13:45 UTC (permalink / raw)
To: Avri Altman
Cc: pengshun.zhao@mediatek.com, linux-scsi@vger.kernel.org,
martin.petersen@oracle.com, andy.teng@mediatek.com,
jejb@linux.ibm.com, chun-hung.wu@mediatek.com,
kuohong.wang@mediatek.com, linux-kernel@vger.kernel.org,
cc.chou@mediatek.com, cang@codeaurora.org,
linux-mediatek@lists.infradead.org, peter.wang@mediatek.com,
alim.akhtar@samsung.com, matthias.bgg@gmail.com,
beanhuo@micron.com, chaotian.jing@mediatek.com,
bvanassche@acm.org, linux-arm-kernel@lists.infradead.org,
asutoshd@codeaurora.org
In-Reply-To: <SN6PR04MB464015BDF84DF7A9779BEB41FC8D0@SN6PR04MB4640.namprd04.prod.outlook.com>
Hi Avri,
On Sun, 2020-05-31 at 07:10 +0000, Avri Altman wrote:
> >
> > Currently ref-clk control timeout is implemented by Jiffies. However
> > jiffies is not accurate enough thus "false timeout" may happen.
> >
> > Use more accurate delay mechanism instead, for example, ktime.
> >
> > Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
> > Reviewed-by: Andy Teng <andy.teng@mediatek.com>
> Reviewed-by: Avri Altman <avri.altman@wdc.com>
>
Thanks for your review.
> >
> > /* Wait for ack */
> > - timeout = jiffies + msecs_to_jiffies(REFCLK_REQ_TIMEOUT_MS);
> > + timeout = ktime_add_us(ktime_get(), REFCLK_REQ_TIMEOUT_US);
> > do {
> > + time_checked = ktime_get();
> > value = ufshcd_readl(hba, REG_UFS_REFCLK_CTRL);
> >
> > /* Wait until ack bit equals to req bit */
> > @@ -144,7 +145,7 @@ static int ufs_mtk_setup_ref_clk(struct ufs_hba *hba,
> > bool on)
> > goto out;
> >
> > usleep_range(100, 200);
> > - } while (time_before(jiffies, timeout));
> > + } while (ktime_before(time_checked, timeout));
> Nit: you could get rid of time_checked if you would use ktime_compare(ktime_get(), timeout) > 0
>
> Thanks,
> Avri
If this context is preempted and scheduled out between ufshcd_readl()
and ktime_compare(ktime_get(), timeout), then the ktime_get() may get a
"timed-out" time even though the last ufshcd_readl() is actually
executed before the "timed-out" time. In this case, false alarm will
show up. Using "time_checked" here could solve above issue.
Thanks,
Stanley Chu
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] iio: at91-sama5d2_adc: remove usage of iio_priv_to_dev() helper
From: Jonathan Cameron @ 2020-05-31 14:39 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: alexandre.belloni, linux-iio, linux-kernel, ludovic.desroches,
eugen.hristev, linux-arm-kernel
In-Reply-To: <20200525105341.137945-1-alexandru.ardelean@analog.com>
On Mon, 25 May 2020 13:53:41 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> We may want to get rid of the iio_priv_to_dev() helper. The reason is that
> we will hide some of the members of the iio_dev structure (to prevent
> drivers from accessing them directly), and that will also mean hiding the
> implementation of the iio_priv_to_dev() helper inside the IIO core.
>
> Hiding the implementation of iio_priv_to_dev() implies that some fast-paths
> may not be fast anymore, so a general idea is to try to get rid of the
> iio_priv_to_dev() altogether.
> The iio_priv() helper won't be affected by the rework, as the iio_dev
> struct will keep a reference to the private information.
>
> For this driver, not using iio_priv_to_dev(), means reworking some paths to
> pass the iio device and using iio_priv() to access the private information,
> and also keeping a reference to the iio device for some quirky paths.
>
> One [quirky] path is the at91_adc_workq_handler() which requires the IIO
> device & the state struct to push to buffers.
> Since this requires the back-ref to the IIO device, the
> at91_adc_touch_pos() also uses it. This simplifies the patch a bit. The
> information required in this function is mostly for debugging purposes.
> Replacing it with a reference to the IIO device would have been a slightly
> bigger change, which may not be worth it (for just the debugging purpose
> and given that we need the back-ref to the IIO device anyway).
That workq is indeed quirky. This looks fine to me in general. I'll
want an appropriate ack from the at91 side of things if possible so
let's leave this on the list for a while longer.
Poke me if no action in a few weeks.
Thanks,
Jonathan
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
> drivers/iio/adc/at91-sama5d2_adc.c | 30 +++++++++++++++++-------------
> 1 file changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 9abbbdcc7420..7bce18444430 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -402,6 +402,7 @@ struct at91_adc_state {
> wait_queue_head_t wq_data_available;
> struct at91_adc_dma dma_st;
> struct at91_adc_touch touch_st;
> + struct iio_dev *indio_dev;
> u16 buffer[AT91_BUFFER_MAX_HWORDS];
> /*
> * lock to prevent concurrent 'single conversion' requests through
> @@ -642,13 +643,13 @@ static u16 at91_adc_touch_pos(struct at91_adc_state *st, int reg)
> /* first half of register is the x or y, second half is the scale */
> val = at91_adc_readl(st, reg);
> if (!val)
> - dev_dbg(&iio_priv_to_dev(st)->dev, "pos is 0\n");
> + dev_dbg(&st->indio_dev->dev, "pos is 0\n");
>
> pos = val & AT91_SAMA5D2_XYZ_MASK;
> result = (pos << AT91_SAMA5D2_MAX_POS_BITS) - pos;
> scale = (val >> 16) & AT91_SAMA5D2_XYZ_MASK;
> if (scale == 0) {
> - dev_err(&iio_priv_to_dev(st)->dev, "scale is 0\n");
> + dev_err(&st->indio_dev->dev, "scale is 0\n");
> return 0;
> }
> result /= scale;
> @@ -1204,9 +1205,9 @@ static unsigned at91_adc_startup_time(unsigned startup_time_min,
> return i;
> }
>
> -static void at91_adc_setup_samp_freq(struct at91_adc_state *st, unsigned freq)
> +static void at91_adc_setup_samp_freq(struct iio_dev *indio_dev, unsigned freq)
> {
> - struct iio_dev *indio_dev = iio_priv_to_dev(st);
> + struct at91_adc_state *st = iio_priv(indio_dev);
> unsigned f_per, prescal, startup, mr;
>
> f_per = clk_get_rate(st->per_clk);
> @@ -1275,9 +1276,9 @@ static void at91_adc_pen_detect_interrupt(struct at91_adc_state *st)
> st->touch_st.touching = true;
> }
>
> -static void at91_adc_no_pen_detect_interrupt(struct at91_adc_state *st)
> +static void at91_adc_no_pen_detect_interrupt(struct iio_dev *indio_dev)
> {
> - struct iio_dev *indio_dev = iio_priv_to_dev(st);
> + struct at91_adc_state *st = iio_priv(indio_dev);
>
> at91_adc_writel(st, AT91_SAMA5D2_TRGR,
> AT91_SAMA5D2_TRGR_TRGMOD_NO_TRIGGER);
> @@ -1297,7 +1298,7 @@ static void at91_adc_workq_handler(struct work_struct *workq)
> struct at91_adc_touch, workq);
> struct at91_adc_state *st = container_of(touch_st,
> struct at91_adc_state, touch_st);
> - struct iio_dev *indio_dev = iio_priv_to_dev(st);
> + struct iio_dev *indio_dev = st->indio_dev;
>
> iio_push_to_buffers(indio_dev, st->buffer);
> }
> @@ -1318,7 +1319,7 @@ static irqreturn_t at91_adc_interrupt(int irq, void *private)
> at91_adc_pen_detect_interrupt(st);
> } else if ((status & AT91_SAMA5D2_IER_NOPEN)) {
> /* nopen detected IRQ */
> - at91_adc_no_pen_detect_interrupt(st);
> + at91_adc_no_pen_detect_interrupt(indio);
> } else if ((status & AT91_SAMA5D2_ISR_PENS) &&
> ((status & rdy_mask) == rdy_mask)) {
> /* periodic trigger IRQ - during pen sense */
> @@ -1486,7 +1487,7 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
> val > st->soc_info.max_sample_rate)
> return -EINVAL;
>
> - at91_adc_setup_samp_freq(st, val);
> + at91_adc_setup_samp_freq(indio_dev, val);
> return 0;
> default:
> return -EINVAL;
> @@ -1624,8 +1625,10 @@ static int at91_adc_update_scan_mode(struct iio_dev *indio_dev,
> return 0;
> }
>
> -static void at91_adc_hw_init(struct at91_adc_state *st)
> +static void at91_adc_hw_init(struct iio_dev *indio_dev)
> {
> + struct at91_adc_state *st = iio_priv(indio_dev);
> +
> at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_SWRST);
> at91_adc_writel(st, AT91_SAMA5D2_IDR, 0xffffffff);
> /*
> @@ -1635,7 +1638,7 @@ static void at91_adc_hw_init(struct at91_adc_state *st)
> at91_adc_writel(st, AT91_SAMA5D2_MR,
> AT91_SAMA5D2_MR_TRANSFER(2) | AT91_SAMA5D2_MR_ANACH);
>
> - at91_adc_setup_samp_freq(st, st->soc_info.min_sample_rate);
> + at91_adc_setup_samp_freq(indio_dev, st->soc_info.min_sample_rate);
>
> /* configure extended mode register */
> at91_adc_config_emr(st);
> @@ -1718,6 +1721,7 @@ static int at91_adc_probe(struct platform_device *pdev)
> indio_dev->num_channels = ARRAY_SIZE(at91_adc_channels);
>
> st = iio_priv(indio_dev);
> + st->indio_dev = indio_dev;
>
> bitmap_set(&st->touch_st.channels_bitmask,
> AT91_SAMA5D2_TOUCH_X_CHAN_IDX, 1);
> @@ -1829,7 +1833,7 @@ static int at91_adc_probe(struct platform_device *pdev)
> goto vref_disable;
> }
>
> - at91_adc_hw_init(st);
> + at91_adc_hw_init(indio_dev);
>
> ret = clk_prepare_enable(st->per_clk);
> if (ret)
> @@ -1945,7 +1949,7 @@ static __maybe_unused int at91_adc_resume(struct device *dev)
> if (ret)
> goto vref_disable_resume;
>
> - at91_adc_hw_init(st);
> + at91_adc_hw_init(indio_dev);
>
> /* reconfiguring trigger hardware state */
> if (!iio_buffer_enabled(indio_dev))
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] iio: Kconfig: at91_adc: add COMPILE_TEST dependency to driver
From: Jonathan Cameron @ 2020-05-31 14:40 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: alexandre.belloni, linux-iio, linux-kernel, ludovic.desroches,
linux-arm-kernel
In-Reply-To: <20200525102744.131672-1-alexandru.ardelean@analog.com>
On Mon, 25 May 2020 13:27:44 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> Since changes can come from all sort of places, it may make sense to have
> this symbol as a dependency to make sure that the 'make allmodconfig' &&
> 'make allyesconfig' build rules cover this driver as well for a
> compile-build/test.
>
> It seemed useful [recently] when trying to apply a change for this.
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Makes sense. Given this sort of change can expose weird an wonderful
dependencies that were previously hidden, I'll be wanting an
ack from at91 people.
Jonathan
> ---
> drivers/iio/adc/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index c48c00077775..c1f4c0aec265 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -294,7 +294,7 @@ config ASPEED_ADC
>
> config AT91_ADC
> tristate "Atmel AT91 ADC"
> - depends on ARCH_AT91
> + depends on ARCH_AT91 || COMPILE_TEST
> depends on INPUT && SYSFS
> select IIO_BUFFER
> select IIO_TRIGGERED_BUFFER
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] iio: at91_adc: remove usage of iio_priv_to_dev() helper
From: Jonathan Cameron @ 2020-05-31 14:42 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: alexandre.belloni, linux-iio, linux-kernel, ludovic.desroches,
linux-arm-kernel
In-Reply-To: <20200525102513.130664-1-alexandru.ardelean@analog.com>
On Mon, 25 May 2020 13:25:13 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> We may want to get rid of the iio_priv_to_dev() helper. The reason is that
> we will hide some of the members of the iio_dev structure (to prevent
> drivers from accessing them directly), and that will also mean hiding the
> implementation of the iio_priv_to_dev() helper inside the IIO core.
>
> Hiding the implementation of iio_priv_to_dev() implies that some fast-paths
> may not be fast anymore, so a general idea is to try to get rid of the
> iio_priv_to_dev() altogether.
> The iio_priv() helper won't be affected by the rework, as the iio_dev
> struct will keep a reference to the private information.
>
> For this driver, not using iio_priv_to_dev(), means reworking some paths to
> pass the iio device and using iio_priv() to access the private information.
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Looks good to me. Will leave it a bit longer though to potentially
get some people more familiar with the driver to sanity check it.
Poke me after the usual couple of weeks if I seem to have lost it
down the back of the sofa.
Thanks,
Jonathan
> ---
> drivers/iio/adc/at91_adc.c | 30 +++++++++++++++---------------
> 1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
> index 0368b6dc6d60..896af58e88bc 100644
> --- a/drivers/iio/adc/at91_adc.c
> +++ b/drivers/iio/adc/at91_adc.c
> @@ -287,13 +287,13 @@ static void handle_adc_eoc_trigger(int irq, struct iio_dev *idev)
> }
> }
>
> -static int at91_ts_sample(struct at91_adc_state *st)
> +static int at91_ts_sample(struct iio_dev *idev)
> {
> + struct at91_adc_state *st = iio_priv(idev);
> unsigned int xscale, yscale, reg, z1, z2;
> unsigned int x, y, pres, xpos, ypos;
> unsigned int rxp = 1;
> unsigned int factor = 1000;
> - struct iio_dev *idev = iio_priv_to_dev(st);
>
> unsigned int xyz_mask_bits = st->res;
> unsigned int xyz_mask = (1 << xyz_mask_bits) - 1;
> @@ -449,7 +449,7 @@ static irqreturn_t at91_adc_9x5_interrupt(int irq, void *private)
>
> if (status & AT91_ADC_ISR_PENS) {
> /* validate data by pen contact */
> - at91_ts_sample(st);
> + at91_ts_sample(idev);
> } else {
> /* triggered by event that is no pen contact, just read
> * them to clean the interrupt and discard all.
> @@ -737,10 +737,10 @@ static int at91_adc_read_raw(struct iio_dev *idev,
> return -EINVAL;
> }
>
> -static int at91_adc_of_get_resolution(struct at91_adc_state *st,
> +static int at91_adc_of_get_resolution(struct iio_dev *idev,
> struct platform_device *pdev)
> {
> - struct iio_dev *idev = iio_priv_to_dev(st);
> + struct at91_adc_state *st = iio_priv(idev);
> struct device_node *np = pdev->dev.of_node;
> int count, i, ret = 0;
> char *res_name, *s;
> @@ -866,10 +866,10 @@ static int at91_adc_probe_dt_ts(struct device_node *node,
> }
> }
>
> -static int at91_adc_probe_dt(struct at91_adc_state *st,
> +static int at91_adc_probe_dt(struct iio_dev *idev,
> struct platform_device *pdev)
> {
> - struct iio_dev *idev = iio_priv_to_dev(st);
> + struct at91_adc_state *st = iio_priv(idev);
> struct device_node *node = pdev->dev.of_node;
> struct device_node *trig_node;
> int i = 0, ret;
> @@ -910,7 +910,7 @@ static int at91_adc_probe_dt(struct at91_adc_state *st,
> }
> st->vref_mv = prop;
>
> - ret = at91_adc_of_get_resolution(st, pdev);
> + ret = at91_adc_of_get_resolution(idev, pdev);
> if (ret)
> goto error_ret;
>
> @@ -1010,9 +1010,9 @@ static void atmel_ts_close(struct input_dev *dev)
> at91_adc_writel(st, AT91_ADC_IDR, AT91RL_ADC_IER_PEN);
> }
>
> -static int at91_ts_hw_init(struct at91_adc_state *st, u32 adc_clk_khz)
> +static int at91_ts_hw_init(struct iio_dev *idev, u32 adc_clk_khz)
> {
> - struct iio_dev *idev = iio_priv_to_dev(st);
> + struct at91_adc_state *st = iio_priv(idev);
> u32 reg = 0;
> u32 tssctim = 0;
> int i = 0;
> @@ -1085,11 +1085,11 @@ static int at91_ts_hw_init(struct at91_adc_state *st, u32 adc_clk_khz)
> return 0;
> }
>
> -static int at91_ts_register(struct at91_adc_state *st,
> +static int at91_ts_register(struct iio_dev *idev,
> struct platform_device *pdev)
> {
> + struct at91_adc_state *st = iio_priv(idev);
> struct input_dev *input;
> - struct iio_dev *idev = iio_priv_to_dev(st);
> int ret;
>
> input = input_allocate_device();
> @@ -1161,7 +1161,7 @@ static int at91_adc_probe(struct platform_device *pdev)
> st = iio_priv(idev);
>
> if (pdev->dev.of_node)
> - ret = at91_adc_probe_dt(st, pdev);
> + ret = at91_adc_probe_dt(idev, pdev);
> else
> ret = at91_adc_probe_pdata(st, pdev);
>
> @@ -1301,11 +1301,11 @@ static int at91_adc_probe(struct platform_device *pdev)
> goto error_disable_adc_clk;
> }
> } else {
> - ret = at91_ts_register(st, pdev);
> + ret = at91_ts_register(idev, pdev);
> if (ret)
> goto error_disable_adc_clk;
>
> - at91_ts_hw_init(st, adc_clk_khz);
> + at91_ts_hw_init(idev, adc_clk_khz);
> }
>
> ret = iio_device_register(idev);
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2] iio: stm32-dfsdm-adc: remove usage of iio_priv_to_dev() helper
From: Jonathan Cameron @ 2020-05-31 14:45 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: alexandre.torgue, linux-iio, linux-kernel, mcoquelin.stm32,
linux-stm32, linux-arm-kernel
In-Reply-To: <20200525082648.39656-1-alexandru.ardelean@analog.com>
On Mon, 25 May 2020 11:26:48 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> We may want to get rid of the iio_priv_to_dev() helper. The reason is that
> we will hide some of the members of the iio_dev structure (to prevent
> drivers from accessing them directly), and that will also mean hiding the
> implementation of the iio_priv_to_dev() helper inside the IIO core.
>
> Hiding the implementation of iio_priv_to_dev() implies that some fast-paths
> may not be fast anymore, so a general idea is to try to get rid of the
> iio_priv_to_dev() altogether.
> The iio_priv() helper won't be affected by the rework, as the iio_dev
> struct will keep a reference to the private information.
>
> For this driver, not using iio_priv_to_dev(), means reworking some paths to
> pass the iio device and using iio_priv() to access the private information.
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Looks great. Will let it sit a little longer on list for others to review
though.
Thanks,
Jonathan
> ---
>
> Changelog v1 -> v2:
> * changed some paths to pass a reference to ref to iio device and access
> private state-struct via iio_priv()
>
> drivers/iio/adc/stm32-dfsdm-adc.c | 65 ++++++++++++++++---------------
> 1 file changed, 33 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
> index 76a60d93fe23..03dfc0b6ba98 100644
> --- a/drivers/iio/adc/stm32-dfsdm-adc.c
> +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
> @@ -330,9 +330,9 @@ static int stm32_dfsdm_compute_all_osrs(struct iio_dev *indio_dev,
> return 0;
> }
>
> -static int stm32_dfsdm_start_channel(struct stm32_dfsdm_adc *adc)
> +static int stm32_dfsdm_start_channel(struct iio_dev *indio_dev)
> {
> - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> + struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> struct regmap *regmap = adc->dfsdm->regmap;
> const struct iio_chan_spec *chan;
> unsigned int bit;
> @@ -350,9 +350,9 @@ static int stm32_dfsdm_start_channel(struct stm32_dfsdm_adc *adc)
> return 0;
> }
>
> -static void stm32_dfsdm_stop_channel(struct stm32_dfsdm_adc *adc)
> +static void stm32_dfsdm_stop_channel(struct iio_dev *indio_dev)
> {
> - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> + struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> struct regmap *regmap = adc->dfsdm->regmap;
> const struct iio_chan_spec *chan;
> unsigned int bit;
> @@ -418,11 +418,11 @@ static void stm32_dfsdm_stop_filter(struct stm32_dfsdm *dfsdm,
> DFSDM_CR1_DFEN_MASK, DFSDM_CR1_DFEN(0));
> }
>
> -static int stm32_dfsdm_filter_set_trig(struct stm32_dfsdm_adc *adc,
> +static int stm32_dfsdm_filter_set_trig(struct iio_dev *indio_dev,
> unsigned int fl_id,
> struct iio_trigger *trig)
> {
> - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> + struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> struct regmap *regmap = adc->dfsdm->regmap;
> u32 jextsel = 0, jexten = STM32_DFSDM_JEXTEN_DISABLED;
> int ret;
> @@ -447,11 +447,11 @@ static int stm32_dfsdm_filter_set_trig(struct stm32_dfsdm_adc *adc,
> return 0;
> }
>
> -static int stm32_dfsdm_channels_configure(struct stm32_dfsdm_adc *adc,
> +static int stm32_dfsdm_channels_configure(struct iio_dev *indio_dev,
> unsigned int fl_id,
> struct iio_trigger *trig)
> {
> - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> + struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> struct regmap *regmap = adc->dfsdm->regmap;
> struct stm32_dfsdm_filter *fl = &adc->dfsdm->fl_list[fl_id];
> struct stm32_dfsdm_filter_osr *flo = &fl->flo[0];
> @@ -491,11 +491,11 @@ static int stm32_dfsdm_channels_configure(struct stm32_dfsdm_adc *adc,
> return 0;
> }
>
> -static int stm32_dfsdm_filter_configure(struct stm32_dfsdm_adc *adc,
> +static int stm32_dfsdm_filter_configure(struct iio_dev *indio_dev,
> unsigned int fl_id,
> struct iio_trigger *trig)
> {
> - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> + struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> struct regmap *regmap = adc->dfsdm->regmap;
> struct stm32_dfsdm_filter *fl = &adc->dfsdm->fl_list[fl_id];
> struct stm32_dfsdm_filter_osr *flo = &fl->flo[fl->fast];
> @@ -521,7 +521,7 @@ static int stm32_dfsdm_filter_configure(struct stm32_dfsdm_adc *adc,
> if (ret)
> return ret;
>
> - ret = stm32_dfsdm_filter_set_trig(adc, fl_id, trig);
> + ret = stm32_dfsdm_filter_set_trig(indio_dev, fl_id, trig);
> if (ret)
> return ret;
>
> @@ -729,21 +729,22 @@ static ssize_t dfsdm_adc_audio_set_spiclk(struct iio_dev *indio_dev,
> return len;
> }
>
> -static int stm32_dfsdm_start_conv(struct stm32_dfsdm_adc *adc,
> +static int stm32_dfsdm_start_conv(struct iio_dev *indio_dev,
> struct iio_trigger *trig)
> {
> + struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> struct regmap *regmap = adc->dfsdm->regmap;
> int ret;
>
> - ret = stm32_dfsdm_channels_configure(adc, adc->fl_id, trig);
> + ret = stm32_dfsdm_channels_configure(indio_dev, adc->fl_id, trig);
> if (ret < 0)
> return ret;
>
> - ret = stm32_dfsdm_start_channel(adc);
> + ret = stm32_dfsdm_start_channel(indio_dev);
> if (ret < 0)
> return ret;
>
> - ret = stm32_dfsdm_filter_configure(adc, adc->fl_id, trig);
> + ret = stm32_dfsdm_filter_configure(indio_dev, adc->fl_id, trig);
> if (ret < 0)
> goto stop_channels;
>
> @@ -757,13 +758,14 @@ static int stm32_dfsdm_start_conv(struct stm32_dfsdm_adc *adc,
> regmap_update_bits(regmap, DFSDM_CR1(adc->fl_id),
> DFSDM_CR1_CFG_MASK, 0);
> stop_channels:
> - stm32_dfsdm_stop_channel(adc);
> + stm32_dfsdm_stop_channel(indio_dev);
>
> return ret;
> }
>
> -static void stm32_dfsdm_stop_conv(struct stm32_dfsdm_adc *adc)
> +static void stm32_dfsdm_stop_conv(struct iio_dev *indio_dev)
> {
> + struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> struct regmap *regmap = adc->dfsdm->regmap;
>
> stm32_dfsdm_stop_filter(adc->dfsdm, adc->fl_id);
> @@ -771,7 +773,7 @@ static void stm32_dfsdm_stop_conv(struct stm32_dfsdm_adc *adc)
> regmap_update_bits(regmap, DFSDM_CR1(adc->fl_id),
> DFSDM_CR1_CFG_MASK, 0);
>
> - stm32_dfsdm_stop_channel(adc);
> + stm32_dfsdm_stop_channel(indio_dev);
> }
>
> static int stm32_dfsdm_set_watermark(struct iio_dev *indio_dev,
> @@ -1017,7 +1019,7 @@ static int __stm32_dfsdm_postenable(struct iio_dev *indio_dev)
> goto stop_dfsdm;
> }
>
> - ret = stm32_dfsdm_start_conv(adc, indio_dev->trig);
> + ret = stm32_dfsdm_start_conv(indio_dev, indio_dev->trig);
> if (ret) {
> dev_err(&indio_dev->dev, "Can't start conversion\n");
> goto err_stop_dma;
> @@ -1063,7 +1065,7 @@ static void __stm32_dfsdm_predisable(struct iio_dev *indio_dev)
> {
> struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
>
> - stm32_dfsdm_stop_conv(adc);
> + stm32_dfsdm_stop_conv(indio_dev);
>
> stm32_dfsdm_adc_dma_stop(indio_dev);
>
> @@ -1159,7 +1161,7 @@ static int stm32_dfsdm_single_conv(struct iio_dev *indio_dev,
>
> adc->nconv = 1;
> adc->smask = BIT(chan->scan_index);
> - ret = stm32_dfsdm_start_conv(adc, NULL);
> + ret = stm32_dfsdm_start_conv(indio_dev, NULL);
> if (ret < 0) {
> regmap_update_bits(adc->dfsdm->regmap, DFSDM_CR2(adc->fl_id),
> DFSDM_CR2_REOCIE_MASK, DFSDM_CR2_REOCIE(0));
> @@ -1180,7 +1182,7 @@ static int stm32_dfsdm_single_conv(struct iio_dev *indio_dev,
> else
> ret = IIO_VAL_INT;
>
> - stm32_dfsdm_stop_conv(adc);
> + stm32_dfsdm_stop_conv(indio_dev);
>
> stm32_dfsdm_process_data(adc, res);
>
> @@ -1313,8 +1315,8 @@ static const struct iio_info stm32_dfsdm_info_adc = {
>
> static irqreturn_t stm32_dfsdm_irq(int irq, void *arg)
> {
> - struct stm32_dfsdm_adc *adc = arg;
> - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> + struct iio_dev *indio_dev = arg;
> + struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> struct regmap *regmap = adc->dfsdm->regmap;
> unsigned int status, int_en;
>
> @@ -1574,7 +1576,7 @@ static int stm32_dfsdm_adc_probe(struct platform_device *pdev)
> iio->dev.of_node = np;
> iio->modes = INDIO_DIRECT_MODE;
>
> - platform_set_drvdata(pdev, adc);
> + platform_set_drvdata(pdev, iio);
>
> ret = of_property_read_u32(dev->of_node, "reg", &adc->fl_id);
> if (ret != 0 || adc->fl_id >= adc->dfsdm->num_fls) {
> @@ -1603,7 +1605,7 @@ static int stm32_dfsdm_adc_probe(struct platform_device *pdev)
> return irq;
>
> ret = devm_request_irq(dev, irq, stm32_dfsdm_irq,
> - 0, pdev->name, adc);
> + 0, pdev->name, iio);
> if (ret < 0) {
> dev_err(dev, "Failed to request IRQ\n");
> return ret;
> @@ -1650,8 +1652,8 @@ static int stm32_dfsdm_adc_probe(struct platform_device *pdev)
>
> static int stm32_dfsdm_adc_remove(struct platform_device *pdev)
> {
> - struct stm32_dfsdm_adc *adc = platform_get_drvdata(pdev);
> - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> + struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
>
> if (adc->dev_data->type == DFSDM_AUDIO)
> of_platform_depopulate(&pdev->dev);
> @@ -1663,8 +1665,7 @@ static int stm32_dfsdm_adc_remove(struct platform_device *pdev)
>
> static int __maybe_unused stm32_dfsdm_adc_suspend(struct device *dev)
> {
> - struct stm32_dfsdm_adc *adc = dev_get_drvdata(dev);
> - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
>
> if (iio_buffer_enabled(indio_dev))
> __stm32_dfsdm_predisable(indio_dev);
> @@ -1674,8 +1675,8 @@ static int __maybe_unused stm32_dfsdm_adc_suspend(struct device *dev)
>
> static int __maybe_unused stm32_dfsdm_adc_resume(struct device *dev)
> {
> - struct stm32_dfsdm_adc *adc = dev_get_drvdata(dev);
> - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> const struct iio_chan_spec *chan;
> struct stm32_dfsdm_channel *ch;
> int i, ret;
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v3] iio: stm32-adc: remove usage of iio_priv_to_dev() helper
From: Jonathan Cameron @ 2020-05-31 14:47 UTC (permalink / raw)
To: Fabrice Gasnier
Cc: olivier.moysan, alexandre.torgue, linux-iio, linux-kernel,
mcoquelin.stm32, Alexandru Ardelean, linux-stm32,
linux-arm-kernel
In-Reply-To: <cc5dc422-d1a6-ab95-a1c3-a8e0b38a145a@st.com>
On Tue, 26 May 2020 17:46:41 +0200
Fabrice Gasnier <fabrice.gasnier@st.com> wrote:
> On 5/26/20 3:44 PM, Alexandru Ardelean wrote:
> > We may want to get rid of the iio_priv_to_dev() helper. The reason is that
> > we will hide some of the members of the iio_dev structure (to prevent
> > drivers from accessing them directly), and that will also mean hiding the
> > implementation of the iio_priv_to_dev() helper inside the IIO core.
> >
> > Hiding the implementation of iio_priv_to_dev() implies that some fast-paths
> > may not be fast anymore, so a general idea is to try to get rid of the
> > iio_priv_to_dev() altogether.
> > The iio_priv() helper won't be affected by the rework, as the iio_dev
> > struct will keep a reference to the private information.
> >
> > For this driver, not using iio_priv_to_dev(), means reworking some paths to
> > pass the iio device and using iio_priv() to access the private information.
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> >
> > Changelog v2 -> v3:
> > - update doc-strings; warnings show-up during build with W=1 arg
> >
> > Changelog v1 -> v2:
> > - converted to pass reference to IIO device in function hooks (vs
> > reference
> > to adc private data)
> >
> > drivers/iio/adc/stm32-adc.c | 118 +++++++++++++++++++-----------------
> > 1 file changed, 63 insertions(+), 55 deletions(-)
>
> Hi Alexandru,
>
> Acked-by: Fabrice Gasnier <fabrice.gasnier@st.com>
Applied to the togreg branch of iio.git and pushed out as testing for the
autobuilders to poke at it.
Thanks,
Jonathan
>
> Thanks,
> Fabrice
>
> >
> > diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> > index ae622ee6d08c..1dd97ec5571c 100644
> > --- a/drivers/iio/adc/stm32-adc.c
> > +++ b/drivers/iio/adc/stm32-adc.c
> > @@ -162,10 +162,10 @@ struct stm32_adc_cfg {
> > struct stm32_adc_trig_info *trigs;
> > bool clk_required;
> > bool has_vregready;
> > - int (*prepare)(struct stm32_adc *);
> > - void (*start_conv)(struct stm32_adc *, bool dma);
> > - void (*stop_conv)(struct stm32_adc *);
> > - void (*unprepare)(struct stm32_adc *);
> > + int (*prepare)(struct iio_dev *);
> > + void (*start_conv)(struct iio_dev *, bool dma);
> > + void (*stop_conv)(struct iio_dev *);
> > + void (*unprepare)(struct iio_dev *);
> > const unsigned int *smp_cycles;
> > };
> >
> > @@ -538,10 +538,11 @@ static void stm32_adc_set_res(struct stm32_adc *adc)
> >
> > static int stm32_adc_hw_stop(struct device *dev)
> > {
> > - struct stm32_adc *adc = dev_get_drvdata(dev);
> > + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> >
> > if (adc->cfg->unprepare)
> > - adc->cfg->unprepare(adc);
> > + adc->cfg->unprepare(indio_dev);
> >
> > if (adc->clk)
> > clk_disable_unprepare(adc->clk);
> > @@ -551,7 +552,8 @@ static int stm32_adc_hw_stop(struct device *dev)
> >
> > static int stm32_adc_hw_start(struct device *dev)
> > {
> > - struct stm32_adc *adc = dev_get_drvdata(dev);
> > + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> > int ret;
> >
> > if (adc->clk) {
> > @@ -563,7 +565,7 @@ static int stm32_adc_hw_start(struct device *dev)
> > stm32_adc_set_res(adc);
> >
> > if (adc->cfg->prepare) {
> > - ret = adc->cfg->prepare(adc);
> > + ret = adc->cfg->prepare(indio_dev);
> > if (ret)
> > goto err_clk_dis;
> > }
> > @@ -579,7 +581,7 @@ static int stm32_adc_hw_start(struct device *dev)
> >
> > /**
> > * stm32f4_adc_start_conv() - Start conversions for regular channels.
> > - * @adc: stm32 adc instance
> > + * @indio_dev: IIO device instance
> > * @dma: use dma to transfer conversion result
> > *
> > * Start conversions for regular channels.
> > @@ -587,8 +589,10 @@ static int stm32_adc_hw_start(struct device *dev)
> > * conversions, in IIO buffer modes. Otherwise, use ADC interrupt with direct
> > * DR read instead (e.g. read_raw, or triggered buffer mode without DMA).
> > */
> > -static void stm32f4_adc_start_conv(struct stm32_adc *adc, bool dma)
> > +static void stm32f4_adc_start_conv(struct iio_dev *indio_dev, bool dma)
> > {
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> > +
> > stm32_adc_set_bits(adc, STM32F4_ADC_CR1, STM32F4_SCAN);
> >
> > if (dma)
> > @@ -605,8 +609,10 @@ static void stm32f4_adc_start_conv(struct stm32_adc *adc, bool dma)
> > stm32_adc_set_bits(adc, STM32F4_ADC_CR2, STM32F4_SWSTART);
> > }
> >
> > -static void stm32f4_adc_stop_conv(struct stm32_adc *adc)
> > +static void stm32f4_adc_stop_conv(struct iio_dev *indio_dev)
> > {
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> > +
> > stm32_adc_clr_bits(adc, STM32F4_ADC_CR2, STM32F4_EXTEN_MASK);
> > stm32_adc_clr_bits(adc, STM32F4_ADC_SR, STM32F4_STRT);
> >
> > @@ -615,8 +621,9 @@ static void stm32f4_adc_stop_conv(struct stm32_adc *adc)
> > STM32F4_ADON | STM32F4_DMA | STM32F4_DDS);
> > }
> >
> > -static void stm32h7_adc_start_conv(struct stm32_adc *adc, bool dma)
> > +static void stm32h7_adc_start_conv(struct iio_dev *indio_dev, bool dma)
> > {
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> > enum stm32h7_adc_dmngt dmngt;
> > unsigned long flags;
> > u32 val;
> > @@ -635,9 +642,9 @@ static void stm32h7_adc_start_conv(struct stm32_adc *adc, bool dma)
> > stm32_adc_set_bits(adc, STM32H7_ADC_CR, STM32H7_ADSTART);
> > }
> >
> > -static void stm32h7_adc_stop_conv(struct stm32_adc *adc)
> > +static void stm32h7_adc_stop_conv(struct iio_dev *indio_dev)
> > {
> > - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> > int ret;
> > u32 val;
> >
> > @@ -652,9 +659,9 @@ static void stm32h7_adc_stop_conv(struct stm32_adc *adc)
> > stm32_adc_clr_bits(adc, STM32H7_ADC_CFGR, STM32H7_DMNGT_MASK);
> > }
> >
> > -static int stm32h7_adc_exit_pwr_down(struct stm32_adc *adc)
> > +static int stm32h7_adc_exit_pwr_down(struct iio_dev *indio_dev)
> > {
> > - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> > int ret;
> > u32 val;
> >
> > @@ -690,9 +697,9 @@ static void stm32h7_adc_enter_pwr_down(struct stm32_adc *adc)
> > stm32_adc_set_bits(adc, STM32H7_ADC_CR, STM32H7_DEEPPWD);
> > }
> >
> > -static int stm32h7_adc_enable(struct stm32_adc *adc)
> > +static int stm32h7_adc_enable(struct iio_dev *indio_dev)
> > {
> > - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> > int ret;
> > u32 val;
> >
> > @@ -713,9 +720,9 @@ static int stm32h7_adc_enable(struct stm32_adc *adc)
> > return ret;
> > }
> >
> > -static void stm32h7_adc_disable(struct stm32_adc *adc)
> > +static void stm32h7_adc_disable(struct iio_dev *indio_dev)
> > {
> > - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> > int ret;
> > u32 val;
> >
> > @@ -730,12 +737,12 @@ static void stm32h7_adc_disable(struct stm32_adc *adc)
> >
> > /**
> > * stm32h7_adc_read_selfcalib() - read calibration shadow regs, save result
> > - * @adc: stm32 adc instance
> > + * @indio_dev: IIO device instance
> > * Note: Must be called once ADC is enabled, so LINCALRDYW[1..6] are writable
> > */
> > -static int stm32h7_adc_read_selfcalib(struct stm32_adc *adc)
> > +static int stm32h7_adc_read_selfcalib(struct iio_dev *indio_dev)
> > {
> > - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> > int i, ret;
> > u32 lincalrdyw_mask, val;
> >
> > @@ -774,12 +781,12 @@ static int stm32h7_adc_read_selfcalib(struct stm32_adc *adc)
> >
> > /**
> > * stm32h7_adc_restore_selfcalib() - Restore saved self-calibration result
> > - * @adc: stm32 adc instance
> > + * @indio_dev: IIO device instance
> > * Note: ADC must be enabled, with no on-going conversions.
> > */
> > -static int stm32h7_adc_restore_selfcalib(struct stm32_adc *adc)
> > +static int stm32h7_adc_restore_selfcalib(struct iio_dev *indio_dev)
> > {
> > - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> > int i, ret;
> > u32 lincalrdyw_mask, val;
> >
> > @@ -847,12 +854,12 @@ static int stm32h7_adc_restore_selfcalib(struct stm32_adc *adc)
> >
> > /**
> > * stm32h7_adc_selfcalib() - Procedure to calibrate ADC
> > - * @adc: stm32 adc instance
> > + * @indio_dev: IIO device instance
> > * Note: Must be called once ADC is out of power down.
> > */
> > -static int stm32h7_adc_selfcalib(struct stm32_adc *adc)
> > +static int stm32h7_adc_selfcalib(struct iio_dev *indio_dev)
> > {
> > - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> > int ret;
> > u32 val;
> >
> > @@ -903,7 +910,7 @@ static int stm32h7_adc_selfcalib(struct stm32_adc *adc)
> >
> > /**
> > * stm32h7_adc_prepare() - Leave power down mode to enable ADC.
> > - * @adc: stm32 adc instance
> > + * @indio_dev: IIO device instance
> > * Leave power down mode.
> > * Configure channels as single ended or differential before enabling ADC.
> > * Enable ADC.
> > @@ -912,30 +919,31 @@ static int stm32h7_adc_selfcalib(struct stm32_adc *adc)
> > * - Only one input is selected for single ended (e.g. 'vinp')
> > * - Two inputs are selected for differential channels (e.g. 'vinp' & 'vinn')
> > */
> > -static int stm32h7_adc_prepare(struct stm32_adc *adc)
> > +static int stm32h7_adc_prepare(struct iio_dev *indio_dev)
> > {
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> > int calib, ret;
> >
> > - ret = stm32h7_adc_exit_pwr_down(adc);
> > + ret = stm32h7_adc_exit_pwr_down(indio_dev);
> > if (ret)
> > return ret;
> >
> > - ret = stm32h7_adc_selfcalib(adc);
> > + ret = stm32h7_adc_selfcalib(indio_dev);
> > if (ret < 0)
> > goto pwr_dwn;
> > calib = ret;
> >
> > stm32_adc_writel(adc, STM32H7_ADC_DIFSEL, adc->difsel);
> >
> > - ret = stm32h7_adc_enable(adc);
> > + ret = stm32h7_adc_enable(indio_dev);
> > if (ret)
> > goto pwr_dwn;
> >
> > /* Either restore or read calibration result for future reference */
> > if (calib)
> > - ret = stm32h7_adc_restore_selfcalib(adc);
> > + ret = stm32h7_adc_restore_selfcalib(indio_dev);
> > else
> > - ret = stm32h7_adc_read_selfcalib(adc);
> > + ret = stm32h7_adc_read_selfcalib(indio_dev);
> > if (ret)
> > goto disable;
> >
> > @@ -944,16 +952,18 @@ static int stm32h7_adc_prepare(struct stm32_adc *adc)
> > return 0;
> >
> > disable:
> > - stm32h7_adc_disable(adc);
> > + stm32h7_adc_disable(indio_dev);
> > pwr_dwn:
> > stm32h7_adc_enter_pwr_down(adc);
> >
> > return ret;
> > }
> >
> > -static void stm32h7_adc_unprepare(struct stm32_adc *adc)
> > +static void stm32h7_adc_unprepare(struct iio_dev *indio_dev)
> > {
> > - stm32h7_adc_disable(adc);
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> > +
> > + stm32h7_adc_disable(indio_dev);
> > stm32h7_adc_enter_pwr_down(adc);
> > }
> >
> > @@ -1160,7 +1170,7 @@ static int stm32_adc_single_conv(struct iio_dev *indio_dev,
> >
> > stm32_adc_conv_irq_enable(adc);
> >
> > - adc->cfg->start_conv(adc, false);
> > + adc->cfg->start_conv(indio_dev, false);
> >
> > timeout = wait_for_completion_interruptible_timeout(
> > &adc->completion, STM32_ADC_TIMEOUT);
> > @@ -1173,7 +1183,7 @@ static int stm32_adc_single_conv(struct iio_dev *indio_dev,
> > ret = IIO_VAL_INT;
> > }
> >
> > - adc->cfg->stop_conv(adc);
> > + adc->cfg->stop_conv(indio_dev);
> >
> > stm32_adc_conv_irq_disable(adc);
> >
> > @@ -1227,8 +1237,8 @@ static int stm32_adc_read_raw(struct iio_dev *indio_dev,
> >
> > static irqreturn_t stm32_adc_threaded_isr(int irq, void *data)
> > {
> > - struct stm32_adc *adc = data;
> > - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> > + struct iio_dev *indio_dev = data;
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> > const struct stm32_adc_regspec *regs = adc->cfg->regs;
> > u32 status = stm32_adc_readl(adc, regs->isr_eoc.reg);
> >
> > @@ -1240,8 +1250,8 @@ static irqreturn_t stm32_adc_threaded_isr(int irq, void *data)
> >
> > static irqreturn_t stm32_adc_isr(int irq, void *data)
> > {
> > - struct stm32_adc *adc = data;
> > - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> > + struct iio_dev *indio_dev = data;
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> > const struct stm32_adc_regspec *regs = adc->cfg->regs;
> > u32 status = stm32_adc_readl(adc, regs->isr_eoc.reg);
> >
> > @@ -1514,7 +1524,7 @@ static int __stm32_adc_buffer_postenable(struct iio_dev *indio_dev)
> > if (!adc->dma_chan)
> > stm32_adc_conv_irq_enable(adc);
> >
> > - adc->cfg->start_conv(adc, !!adc->dma_chan);
> > + adc->cfg->start_conv(indio_dev, !!adc->dma_chan);
> >
> > return 0;
> >
> > @@ -1547,7 +1557,7 @@ static void __stm32_adc_buffer_predisable(struct iio_dev *indio_dev)
> > struct stm32_adc *adc = iio_priv(indio_dev);
> > struct device *dev = indio_dev->dev.parent;
> >
> > - adc->cfg->stop_conv(adc);
> > + adc->cfg->stop_conv(indio_dev);
> > if (!adc->dma_chan)
> > stm32_adc_conv_irq_disable(adc);
> >
> > @@ -1891,7 +1901,7 @@ static int stm32_adc_probe(struct platform_device *pdev)
> > indio_dev->info = &stm32_adc_iio_info;
> > indio_dev->modes = INDIO_DIRECT_MODE | INDIO_HARDWARE_TRIGGERED;
> >
> > - platform_set_drvdata(pdev, adc);
> > + platform_set_drvdata(pdev, indio_dev);
> >
> > ret = of_property_read_u32(pdev->dev.of_node, "reg", &adc->offset);
> > if (ret != 0) {
> > @@ -1905,7 +1915,7 @@ static int stm32_adc_probe(struct platform_device *pdev)
> >
> > ret = devm_request_threaded_irq(&pdev->dev, adc->irq, stm32_adc_isr,
> > stm32_adc_threaded_isr,
> > - 0, pdev->name, adc);
> > + 0, pdev->name, indio_dev);
> > if (ret) {
> > dev_err(&pdev->dev, "failed to request IRQ\n");
> > return ret;
> > @@ -1989,8 +1999,8 @@ static int stm32_adc_probe(struct platform_device *pdev)
> >
> > static int stm32_adc_remove(struct platform_device *pdev)
> > {
> > - struct stm32_adc *adc = platform_get_drvdata(pdev);
> > - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> > + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> >
> > pm_runtime_get_sync(&pdev->dev);
> > iio_device_unregister(indio_dev);
> > @@ -2012,8 +2022,7 @@ static int stm32_adc_remove(struct platform_device *pdev)
> > #if defined(CONFIG_PM_SLEEP)
> > static int stm32_adc_suspend(struct device *dev)
> > {
> > - struct stm32_adc *adc = dev_get_drvdata(dev);
> > - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> > + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> >
> > if (iio_buffer_enabled(indio_dev))
> > __stm32_adc_buffer_predisable(indio_dev);
> > @@ -2023,8 +2032,7 @@ static int stm32_adc_suspend(struct device *dev)
> >
> > static int stm32_adc_resume(struct device *dev)
> > {
> > - struct stm32_adc *adc = dev_get_drvdata(dev);
> > - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> > + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > int ret;
> >
> > ret = pm_runtime_force_resume(dev);
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 0/4] Introduce the Counter character device interface
From: Jonathan Cameron @ 2020-05-31 15:18 UTC (permalink / raw)
To: William Breathitt Gray
Cc: kamel.bouhara, gwendal, david, linux-iio, patrick.havelange,
alexandre.belloni, linux-kernel, mcoquelin.stm32, fabrice.gasnier,
syednwaris, linux-stm32, linux-arm-kernel, alexandre.torgue
In-Reply-To: <20200524175439.GA14300@shinobu>
On Sun, 24 May 2020 13:54:39 -0400
William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> On Sun, May 24, 2020 at 05:25:42PM +0100, Jonathan Cameron wrote:
> >
> > ...
> >
> > > The following are some questions I have about this patchset:
> > >
> > > 1. Should the data format of the character device be configured via a
> > > sysfs attribute?
> > >
> > > In this patchset, the first 196095 bytes of the character device are
> > > dedicated as a selection area to choose which Counter components or
> > > extensions should be exposed; the subsequent bytes are the actual
> > > data for the Counter components and extensions that were selected.
> >
> > That sounds like the worst of all possible worlds. Reality is you need
> > to do some magic library so at that point you might as well have ioctl
> > options to configure it. I wonder if you can keep the data flow
> > to be a simple 'read' from the chardev but move the control away from
> > that. Either control via some chrdevs but keep them to the 'set / get'
> > if this element is going to turn up in the read or not. You rapidly
> > run into problems though, such as now to see how large a given element
> > is going to be etc. Plus ioctls are rather messier to extend than
> > simply adding a new sysfs file. Various subsystems do complex
> > 'descriptor' type approaches to get around this, or you could do
> > self describing records rather than raw data - like an input
> > ev_dev event.
>
> Yes I agree, I don't think combining nondata with data is good design --
> it's better if users are able to simply perform read/write on the
> character device without having to keep track of valid offsets and
> controls.
>
> After giving this some more thought, I believe human-readable sysfs
> attributes are the way to go to support configuration of the character
> device. I am thinking of a system like this:
>
> * Users configure the counter character device via a sysfs attribute
> such as /sys/bus/counter/devices/counterX/chrdev_format or similar.
>
> * Users may write to this sysfs attribute to select the components they
> want to interface -- the layout can be determined as well from the
> order. For example:
>
> # echo "C0 C3 C2" > /sys/bus/counter/devices/counter0/chrdev_format
I guess that 'just' meets the sysfs requirement of one file => one thing.
>
> This would select Counts 0, 3, and 2 (in that order) to be available
> in the /dev/counter0 node as a contiguous memory region.
>
> You can select extensions in a similar fashion:
>
> # echo "C4E2 S1E0" > /sys/bus/counter/devices/counter0/chrdev_format
>
> This would select extension 2 from Count 4, and extension 0 from
> Signal 1.
I'm not totally clear why we'd want to have a chrdev access to extensions.
To be honest I'm not totally sure what an extension is today as it's been
a week ;)
Perhaps an example? I see timestamp below. What is that attached to?
If we gave multiple counters, do they each have a timestamp?
>
> * Users may read from this chrdev_format sysfs attribute in order to see
> the currently configured format of the character device.
>
> * Users may perform read/write operations on the /dev/counterX node
> directly; the layout of the data is what they user has configured via
> the chrdev_format sysfs attribute. For example:
>
> # echo "C0 C1 S0 S1" > /sys/bus/counter/devices/counter0/chrdev_format
>
> Yields the following /dev/counter0 memory layout:
>
> +-----------------+------------------+----------+----------+
> | Byte 0 - Byte 7 | Byte 8 - Byte 15 | Byte 16 | Byte 17 |
> +-----------------+------------------+----------+----------+
> | Count 0 | Count 1 | Signal 0 | Signal 2 |
> +-----------------+------------------+----------+----------+
>
> * Users may perform select/poll operations on the /dev/counterX node.
> Users can be notified if data is available or events have occurred.
One thing to think about early if watermarks. We bolted them on
late in IIO and maybe we could have done it better from the start.
I'd almost guarantee someone will want one fairly soon - particularly
as it's more than possible you'll have a counter device with a
hardware fifo. I have some vague recollection that ti-ecap
stuff could be presented as a short fifo for starters.
>
> The benefit of this design is that the format is robust so users can
> choose the components they want to interface and in the layout they
> want. For example, if I am writing a userspace application to control a
> dual-axis positioning table, I can select the two counts I care about
> for the position axes. This allows me to read just those two values
> directly from /dev/counterX with a simple read() call, without having to
> fumble around seeking to an offset and parsing the layout.
I wonder if I'm over thinking things for counters, but you may run into
the complexity of different counters having different sampling frequencies.
Here you are suggesting a scheme that I think ends up closer to IIO than
input. That makes this case a pain. Input takes the view that it's
fine to have data coming in any order and frequency because every
record is self describing. I'm not sure it matters here, but it is
a nice layer of flexibility, but you do loose the efficiency of
the description being external to the data flow.
>
> Similarly, support for future extensions is simple to implement. When
> timestamp support is implemented, users can just select the desired
> timestamp extension and read it directly from the /dev/counterX node;
> they should also be able to perform a select()/poll() call to be
> notified on new timestamps.
>
> So what do you think of this sort of design? I think there is a useful
> robustness to the simplicity of performing a single read/write call on
> /dev/counterX.
It seems like a reasonable solution to me. The only blurry
boundary to my mind is what level of buffering is behind this.
The things you can do are open, non blocking read, blocking read and select.
If we have a counter that is sampled on demand, then
1) Non blocking read - makes not sense, fair enough I guess, could make it
the same as a blocking read.
2) Blocking read - reads from the sensor.
3) Select, meaningless as all reads are done on demand - so I guess you
hardwire it to return immediately.
4) open. Nothing special
If you have a counter that is self clocking then data gets pushed into some
software structure (probably kfifo)
1) Blocking read, question of semantics to resolve
a) Return when 'some' data is available (like a socket)
b) Return when 'requested amount of data is available'?
2) Non blocking read. Return whatever happens to be available.
3) Select. Semantics to be defined.
a) Some data?
b) Watermark based (default watermark is 0 so any data triggers it)
4) Open. Starts up sampling of configured set - (typically turns on the
device, enables interrupt output etc.)
So some corners to resolve but should all work.
>
> > >
> > > Moving this selection to a sysfs attribute and dedicating the
> > > character device to just data transfer might be a better design. If
> > > such a design is chosen, should the selection attribute be
> > > human-readable or binary?
> >
> > Sysfs basically requires things are more or less human readable.
> > So if you go that way I think it needs to be.
> >
> > >
> > > 2. How much space should allotted for strings?
> > >
> > > Each Counter component and extension has a respective size allotted
> > > for its data (u8 data is allotted 1 byte, u64 data is allotted 8
> > > bytes, etc.); I have arbitrarily chosen to allot 64 bytes for
> > > strings. Is this an apt size, or should string data be allotted more
> > > or less space?
> >
> > I'd go with that being big enough, but try to keep the expose interface
> > such that the size can change it it needs to the in the future.
>
> Following along with the separation of control vs data as discussed
> above, we could support a more variable size by exposing it through a
> sysfs attribute (maybe a chrdev_string_size attribute or similar).
I'm unconvinced you'd ever want to return a string via the chardev.
People are using the chrdev to get efficiency. String based data flows
are rarely that!
>
> >
> > >
> > > 3. Should the owning component of an extension be handled by the device
> > > driver or Counter subsystem?
> > >
> > > The Counter subsystem figures out the owner (enum counter_owner_type)
> > > for each component/extension in the counter-sysfs and counter-chrdev
> > > code. When a callback must be executed, there are various switch
> > > statements throughout the code to check whether the respective
> > > Device, Signal, or Count version of the callback should be executed;
> > > similarly, the appropriate owner type must match for the struct
> > > counter_data macros such as COUNTER_DATA_DEVICE_U64,
> > > COUNTER_DATA_SIGNAL_U64, COUNTER_DATA_COUNT_U64, etc.
> > >
> > > All this complexity in the Counter subsystem code can be eliminated
> > > if a single callback type with a `void *owner` parameter is defined
> > > for use with all three owner types (Device, Signal, and Count). The
> > > device driver would then be responsible for casting the callback
> > > argument to the appropriate owner type; but in theory, this should
> > > not be much of a problem since the device driver is responsible for
> > > assigning the callbacks to the owning component anyway.
> >
> > Whilst its more complex for subsytem I think it's better to keep everything
> > typed if we possibly can. Always a trade off though, so use your discretion.
> >
> > Jonathan
>
> I'm going to keep it all typed for now since I don't want to make too
> many changes at once. Since this is somewhat unrelated to the purpose of
> introducing Counter character devices, I'll postpone the discussion to a
> later date after the Counter character device interface is merged.
Makes sense.
Jonathan
>
> William Breathitt Gray
>
> >
> >
> > >
> > > William Breathitt Gray (4):
> > > counter: Internalize sysfs interface code
> > > docs: counter: Update to reflect sysfs internalization
> > > counter: Add character device interface
> > > docs: counter: Document character device interface
> > >
> > > Documentation/driver-api/generic-counter.rst | 275 +++-
> > > MAINTAINERS | 3 +-
> > > drivers/counter/104-quad-8.c | 547 +++----
> > > drivers/counter/Makefile | 1 +
> > > drivers/counter/counter-chrdev.c | 656 ++++++++
> > > drivers/counter/counter-chrdev.h | 16 +
> > > drivers/counter/counter-core.c | 187 +++
> > > drivers/counter/counter-sysfs.c | 881 +++++++++++
> > > drivers/counter/counter-sysfs.h | 14 +
> > > drivers/counter/counter.c | 1496 ------------------
> > > drivers/counter/ftm-quaddec.c | 89 +-
> > > drivers/counter/stm32-lptimer-cnt.c | 161 +-
> > > drivers/counter/stm32-timer-cnt.c | 139 +-
> > > drivers/counter/ti-eqep.c | 211 +--
> > > include/linux/counter.h | 626 ++++----
> > > include/linux/counter_enum.h | 45 -
> > > include/uapi/linux/counter-types.h | 45 +
> > > 17 files changed, 2826 insertions(+), 2566 deletions(-)
> > > create mode 100644 drivers/counter/counter-chrdev.c
> > > create mode 100644 drivers/counter/counter-chrdev.h
> > > create mode 100644 drivers/counter/counter-core.c
> > > create mode 100644 drivers/counter/counter-sysfs.c
> > > create mode 100644 drivers/counter/counter-sysfs.h
> > > delete mode 100644 drivers/counter/counter.c
> > > delete mode 100644 include/linux/counter_enum.h
> > > create mode 100644 include/uapi/linux/counter-types.h
> > >
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 1/2] arm64: dts: Add a device tree for the Librem5 phone
From: Guido Günther @ 2020-05-31 15:36 UTC (permalink / raw)
To: Pavel Machek
Cc: Martin Kepplinger, kernel, Anson.Huang, robh, festevam, s.hauer,
angus, linux-kernel, devicetree, linux-imx, kernel, mchehab,
shawnguo, linux-arm-kernel
In-Reply-To: <20200529162850.GC3709@amd>
Hi,
On Fri, May 29, 2020 at 06:28:50PM +0200, Pavel Machek wrote:
> Hi!
>
> > From: "Angus Ainslie (Purism)" <angus@akkea.ca>
> >
> > Add a devicetree description for the Librem 5 phone. The early batches
> > that have been sold are supported as well as the mass-produced device
> > available later this year, see https://puri.sm/products/librem-5/
> >
> > This boots to a working console with working WWAN modem, wifi usdhc,
> > IMU sensor device, proximity sensor, haptic motor, gpio keys, GNSS and LEDs.
> >
> > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
> > Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
> > Signed-off-by: Guido Günther <agx@sigxcpu.org>
>
>
> > + blue {
> > + label = "phone:blue:front";
> > + label = "phone:green:front";
> > + label = "phone:red:front";
>
> Droid 4 uses "status-led:{red,green,blue}". Could this use same
> naming?
Looking at leds-class.rst we don't have a useful devicename so
should that just be omitted and s.th. like
label = "blue:status";
label = "green:status";
label = "red:status";
be used instead. If we want to map to current usage
label = "blue:status";
label = "green:boot";
label = "red:charging";
would be even closer but since that is bound to change just going with
"status" would make sense.
Cheers,
-- Guido
>
> > + label = "lm3560:flash";
> > + label = "lm3560:torch";
>
> This is one LED, right? I'm pretty sure we don't want lm3560 in the
> name... "main-camera:flash" would be better. Even better would be
> something that's already in use.
>
> > + label = "white:backlight_cluster";
>
> Make this ":backlight", please. Again, we want something that's
> already used.
>
> Best regards,
> Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 3/3] iio: remove iio_triggered_buffer_postenable()/iio_triggered_buffer_predisable()
From: Jonathan Cameron @ 2020-05-31 15:40 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: linus.walleij, Lars-Peter Clausen, alexandre.torgue, linux-iio,
s.hauer, linux-kernel, songqiang1304521, mcoquelin.stm32,
lorenzo.bianconi83, shawnguo, linux-stm32, linux-arm-kernel
In-Reply-To: <20200525113855.178821-3-alexandru.ardelean@analog.com>
On Mon, 25 May 2020 14:38:55 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> From: Lars-Peter Clausen <lars@metafoo.de>
>
> This patch should be squashed into the first one, as the first one is
> breaking the build (intentionally) to make the IIO core files easier to
> review.
>
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
Friend poke. Version log?
Other than the wistful comment below (which I'm not expecting you to
do anything about btw!) whole series looks good to me.
These are obviously no functional changes (I think) so it's only really patch 2 that
could do with more eyes and acks.
Far as I can tell that case is fine as well because of the protections
on being in the right mode, but more eyes on that would be great.
So assuming that's fine, what commit message do you want me to use for
the fused single patch?
Thanks,
Jonathan
> static const struct iio_trigger_ops atlas_interrupt_trigger_ops = {
> diff --git a/drivers/iio/dummy/iio_simple_dummy_buffer.c b/drivers/iio/dummy/iio_simple_dummy_buffer.c
> index 17606eca42b4..8e13c53d4360 100644
> --- a/drivers/iio/dummy/iio_simple_dummy_buffer.c
> +++ b/drivers/iio/dummy/iio_simple_dummy_buffer.c
> @@ -99,20 +99,6 @@ static irqreturn_t iio_simple_dummy_trigger_h(int irq, void *p)
> }
>
> static const struct iio_buffer_setup_ops iio_simple_dummy_buffer_setup_ops = {
> - /*
> - * iio_triggered_buffer_postenable:
> - * Generic function that simply attaches the pollfunc to the trigger.
> - * Replace this to mess with hardware state before we attach the
> - * trigger.
> - */
> - .postenable = &iio_triggered_buffer_postenable,
> - /*
> - * iio_triggered_buffer_predisable:
> - * Generic function that simple detaches the pollfunc from the trigger.
> - * Replace this to put hardware state back again after the trigger is
> - * detached but before userspace knows we have disabled the ring.
> - */
> - .predisable = &iio_triggered_buffer_predisable,
> };
>
Hmm. Guess we should probably 'invent' a reason to illustrate the bufer
ops in the dummy example. Anyone feeling creative?
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: arm64: Register modification during syscall entry/exit stop
From: Keno Fischer @ 2020-05-31 16:13 UTC (permalink / raw)
To: Will Deacon
Cc: Kyle Huey, Catalin Marinas, Linux Kernel Mailing List,
Oleg Nesterov, Dave Martin, linux-arm-kernel
In-Reply-To: <20200531093320.GA30204@willie-the-truck>
> Keno -- are you planning to send out a patch? You previously spoke about
> implementing this using PTRACE_SETOPTIONS.
Yes, I'll have a patch for you. Though I've come to the conclusion
that introducing a new regset is probably a better way to solve it.
We can then also expose orig_x0 at the same time and give it sane semantics
(there's some problems with the way it works currently - I'll write it up
together with the patch).
Keno
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: arm64: Register modification during syscall entry/exit stop
From: Keno Fischer @ 2020-05-31 16:20 UTC (permalink / raw)
To: Dave Martin
Cc: Kyle Huey, Catalin Marinas, Oleg Nesterov,
Linux Kernel Mailing List, Will Deacon, linux-arm-kernel
In-Reply-To: <20200527101929.GT5031@arm.com>
> Can't PTRACE_SYSEMU be emulated by using PTRACE_SYSCALL, cancelling the
> syscall at the syscall enter stop, then modifying the regs at the
> syscall exit stop?
Yes, it can. The idea behind SYSEMU is to be able to save half the
ptrace traps that would require, in theory making the ptracer
a decent amount faster. That said, the x7 issue is orthogonal to
SYSEMU, you'd have the same issues if you used PTRACE_SYSCALL.
Keno
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 0/4] Introduce the Counter character device interface
From: William Breathitt Gray @ 2020-05-31 17:14 UTC (permalink / raw)
To: Jonathan Cameron
Cc: kamel.bouhara, gwendal, david, linux-iio, patrick.havelange,
alexandre.belloni, linux-kernel, mcoquelin.stm32, fabrice.gasnier,
syednwaris, linux-stm32, linux-arm-kernel, alexandre.torgue
In-Reply-To: <20200531161813.658ffdfb@archlinux>
[-- Attachment #1.1: Type: text/plain, Size: 13367 bytes --]
On Sun, May 31, 2020 at 04:18:13PM +0100, Jonathan Cameron wrote:
> On Sun, 24 May 2020 13:54:39 -0400
> William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> > After giving this some more thought, I believe human-readable sysfs
> > attributes are the way to go to support configuration of the character
> > device. I am thinking of a system like this:
> >
> > * Users configure the counter character device via a sysfs attribute
> > such as /sys/bus/counter/devices/counterX/chrdev_format or similar.
> >
> > * Users may write to this sysfs attribute to select the components they
> > want to interface -- the layout can be determined as well from the
> > order. For example:
> >
> > # echo "C0 C3 C2" > /sys/bus/counter/devices/counter0/chrdev_format
>
> I guess that 'just' meets the sysfs requirement of one file => one thing.
We can massage this further to make it more apt, but the main idea here
is that configuration should be separate from our data; and that
configuration should be performed via sysfs.
> > This would select Counts 0, 3, and 2 (in that order) to be available
> > in the /dev/counter0 node as a contiguous memory region.
> >
> > You can select extensions in a similar fashion:
> >
> > # echo "C4E2 S1E0" > /sys/bus/counter/devices/counter0/chrdev_format
> >
> > This would select extension 2 from Count 4, and extension 0 from
> > Signal 1.
>
> I'm not totally clear why we'd want to have a chrdev access to extensions.
> To be honest I'm not totally sure what an extension is today as it's been
> a week ;)
In the context of the Counter subsystem, an extension is data/control
that is not one of the core components of the Counter paradigm (i.e. not
a Counter, Signal, nor Synapse). Extensions essentially represent
configuration options for the counter device and auxiliary
functionality.
The "Implementation" section of the Generic Counter documentation
Documentation/driver-api/generic-counter.rst file gives some good
examples of extensions, but I'll provide an example here for the sake of
this new character device interface.
Suppose we have a robot controlling a laser on a dual-axes positioning
table. A counter device is used to track the position of the laser:
Count 0 represents position on the X axis, while Count 1 represents
position on the Y axis. Because this machine is moving across two axes
at the same time, we want to grab both counts together via the
character device subsystem (grabbing them separately via sysfs would be
imprecise due to the inherent latency).
The motors are physically able the robot out of the work area, which is
not something we want to happen. A common setup in systems like this is
to set soft boundaries on the counter device to represent the edge of
the work area; when the boundary is passed, a flag is set high on the
device to indicate the position is out-of-bounds.
On the Counter subsystem side, this counter device would appear as
having four sysfs attributes: count0/count, count0/boundary,
count1/count, and count1/boundary. In terms of the character device
interface, we could perform a setup like this:
# echo "C0E0 C0 C1E1 C1" > counter0/chrdev_format
Yielding the following /dev/counter0 memory layout:
+------------+-----------------+------------+-------------------+
| Byte 0 | Byte 1 - Byte 8 | Byte 9 | Byte 10 - Byte 17 |
+------------+-----------------+------------+-------------------+
| Boundary 0 | Count 0 | Boundary 1 | Count 1 |
+------------+-----------------+------------+-------------------+
Now a single read() operation can grab the counts together as well as
their respective boundary flags to verify whether the current counts are
valid. This is a scenario where using sysfs wouldn't be viable to use:
we could check the count0/boundary sysfs attribute first, but by the
time we read the count0/count sysfs attribute second, the robot has
already moved to a new (possibly invalid) position.
> Perhaps an example? I see timestamp below. What is that attached to?
> If we gave multiple counters, do they each have a timestamp?
Some counter devices feature "timestamp" functionality. I haven't yet
implemented this in the Counter subsystem because it's new functionality
and I want to keep this patchset limited to the existing Counter
subsystem functionality support.
However, to briefly go into the topic (we'll need to discuss this more
in-depth before committing to a Counter subsystem design), some counter
devices can keep track of historic counts based on various events; we
call these "timestamps", although they may not necessary be tied to a
wall-clock time.
For example, quadrature encoders often have an "index" signal in
addition to the quadrature A and B lines. This index signal may be used
to home a positioning device, or perhaps to indicate that a full
revolution -- or some other event -- has occurred. It's common for
quadrature counter devices to provide a FIFO buffer that logs these
"index" events by saving the current count when that respective index
signal goes high. Thus we have a timestamp buffer.
In the context of the Counter subsystem, I believe we will end up
implementing these timestamps as Count extensions (or Device extensions
if it's a single buffer for the entire device). I'm not sure yet what
the sysfs attribute will display, but I'm guessing we'll have a
respective /sys/bus/counter/devices/counterX/countX/timestamps sysfs
attribute or similar.
The character device implementation should be more straight forward I
would imagine. Since it's a memory buffer, I think we can provide access
to that buffer directly in the chrdev:
# echo "C0E0 C1E1" > /sys/bus/counter/devices/counter0/chrdev_format
Yielding the following /dev/counter0 memory layout for 32-byte
timestamps:
+---------------------+---------------------+
| Byte 0 - Byte 31 | Byte 32 - Byte 63 |
+---------------------+---------------------+
| Timestamps Buffer 0 | Timestamps Buffer 1 |
+---------------------+---------------------+
> > * Users may read from this chrdev_format sysfs attribute in order to see
> > the currently configured format of the character device.
> >
> > * Users may perform read/write operations on the /dev/counterX node
> > directly; the layout of the data is what they user has configured via
> > the chrdev_format sysfs attribute. For example:
> >
> > # echo "C0 C1 S0 S1" > /sys/bus/counter/devices/counter0/chrdev_format
> >
> > Yields the following /dev/counter0 memory layout:
> >
> > +-----------------+------------------+----------+----------+
> > | Byte 0 - Byte 7 | Byte 8 - Byte 15 | Byte 16 | Byte 17 |
> > +-----------------+------------------+----------+----------+
> > | Count 0 | Count 1 | Signal 0 | Signal 2 |
> > +-----------------+------------------+----------+----------+
> >
> > * Users may perform select/poll operations on the /dev/counterX node.
> > Users can be notified if data is available or events have occurred.
>
> One thing to think about early if watermarks. We bolted them on
> late in IIO and maybe we could have done it better from the start.
> I'd almost guarantee someone will want one fairly soon - particularly
> as it's more than possible you'll have a counter device with a
> hardware fifo. I have some vague recollection that ti-ecap
> stuff could be presented as a short fifo for starters.
>
> >
> > The benefit of this design is that the format is robust so users can
> > choose the components they want to interface and in the layout they
> > want. For example, if I am writing a userspace application to control a
> > dual-axis positioning table, I can select the two counts I care about
> > for the position axes. This allows me to read just those two values
> > directly from /dev/counterX with a simple read() call, without having to
> > fumble around seeking to an offset and parsing the layout.
>
> I wonder if I'm over thinking things for counters, but you may run into
> the complexity of different counters having different sampling frequencies.
> Here you are suggesting a scheme that I think ends up closer to IIO than
> input. That makes this case a pain. Input takes the view that it's
> fine to have data coming in any order and frequency because every
> record is self describing. I'm not sure it matters here, but it is
> a nice layer of flexibility, but you do loose the efficiency of
> the description being external to the data flow.
I think one of the downsides to using a single a single character device
node to represent the entire counter device is that the frequency of two
individual counts may differ from each other. For example, using the
dual-axes positioning scenario from earlier, one axis might change more
frequently than the other (e.g. a conveyor belt situation where X is
always moving forward, while Y only changes when a part appears within
the work area).
In these cases, I think the Input subsystem approach might be better
because the user can just wait for events at large and handle each event
as it comes in, rather than try to coordinate between them all in a
shared memory space. The shortcoming with this approach is that we lose
the ability to grab Counts together at the same time, such as we require
in the constantly moving robot example earlier.
Perhaps what might work is to implement Counter events (perhaps even
timestamps) using the Input subsystem, and leave the Counter character
device interface to simple read/write operations. But we'll need to
investigate this further because we lack a concept of "events" right now
in the Counter subsystem.
> > Similarly, support for future extensions is simple to implement. When
> > timestamp support is implemented, users can just select the desired
> > timestamp extension and read it directly from the /dev/counterX node;
> > they should also be able to perform a select()/poll() call to be
> > notified on new timestamps.
> >
> > So what do you think of this sort of design? I think there is a useful
> > robustness to the simplicity of performing a single read/write call on
> > /dev/counterX.
>
> It seems like a reasonable solution to me. The only blurry
> boundary to my mind is what level of buffering is behind this.
> The things you can do are open, non blocking read, blocking read and select.
>
> If we have a counter that is sampled on demand, then
> 1) Non blocking read - makes not sense, fair enough I guess, could make it
> the same as a blocking read.
> 2) Blocking read - reads from the sensor.
> 3) Select, meaningless as all reads are done on demand - so I guess you
> hardwire it to return immediately.
> 4) open. Nothing special
>
> If you have a counter that is self clocking then data gets pushed into some
> software structure (probably kfifo)
> 1) Blocking read, question of semantics to resolve
> a) Return when 'some' data is available (like a socket)
> b) Return when 'requested amount of data is available'?
> 2) Non blocking read. Return whatever happens to be available.
> 3) Select. Semantics to be defined.
> a) Some data?
> b) Watermark based (default watermark is 0 so any data triggers it)
> 4) Open. Starts up sampling of configured set - (typically turns on the
> device, enables interrupt output etc.)
>
> So some corners to resolve but should all work.
I'm not familiar with the "watermark" terminology. Would you be able to
explain it bit more for me. Is this simply a flag that indicates if data
has changed from the last time it was checked?
> > > > Moving this selection to a sysfs attribute and dedicating the
> > > > character device to just data transfer might be a better design. If
> > > > such a design is chosen, should the selection attribute be
> > > > human-readable or binary?
> > >
> > > Sysfs basically requires things are more or less human readable.
> > > So if you go that way I think it needs to be.
> > >
> > > >
> > > > 2. How much space should allotted for strings?
> > > >
> > > > Each Counter component and extension has a respective size allotted
> > > > for its data (u8 data is allotted 1 byte, u64 data is allotted 8
> > > > bytes, etc.); I have arbitrarily chosen to allot 64 bytes for
> > > > strings. Is this an apt size, or should string data be allotted more
> > > > or less space?
> > >
> > > I'd go with that being big enough, but try to keep the expose interface
> > > such that the size can change it it needs to the in the future.
> >
> > Following along with the separation of control vs data as discussed
> > above, we could support a more variable size by exposing it through a
> > sysfs attribute (maybe a chrdev_string_size attribute or similar).
>
> I'm unconvinced you'd ever want to return a string via the chardev.
> People are using the chrdev to get efficiency. String based data flows
> are rarely that!
That's a good point. I don't think there is a situation right now where
we need to deliver strings via the character device interface, so I
think I'll remove that in v3.
William Breathitt Gray
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH 0/5] Suspend and resume fixes for omapdrm pdata removal
From: Tony Lindgren @ 2020-05-31 19:39 UTC (permalink / raw)
To: linux-omap
Cc: Nishanth Menon, Tero Kristo, Grygorii Strashko, Dave Gerlach,
Keerthy, Tomi Valkeinen, linux-kernel, dri-devel,
Andrew F . Davis, Peter Ujfalusi, Faiz Abbas, Laurent Pinchart,
Greg Kroah-Hartman, Suman Anna, linux-arm-kernel, Roger Quadros
Hi all,
Recent omapdrm related changes to drop legacy platform data caused
a suspend and resume regression. I must have only tested suspend
and resume only with the changes preparing to drop the platform data,
but looks like I forgot to test suspend after dropping the platform
data.
There seem to be other longer term DSS regressions remaining too.
Looks like at least panel-simple currently does not probe. It fails
with "panel-simple display: Reject override mode: panel has a fixed
mode". I think the solution there is to drop the board specific
panel-timing related dts lines, but that still seems to be pending.
Anyways, no luck getting the LCD enabled on am437x-sk-evm with v5.6
or v5.7-rc kernels.
Regards,
Tony
Tony Lindgren (5):
drm/omap: Fix suspend resume regression after platform data removal
bus: ti-sysc: Use optional clocks on for enable and wait for softreset
bit
bus: ti-sysc: Ignore clockactivity unless specified as a quirk
bus: ti-sysc: Fix uninitialized framedonetv_irq
ARM: OMAP2+: Fix legacy mode dss_reset
arch/arm/mach-omap2/omap_hwmod.c | 2 +-
drivers/bus/ti-sysc.c | 93 ++++++++++++++++++------
drivers/gpu/drm/omapdrm/dss/dispc.c | 6 +-
drivers/gpu/drm/omapdrm/dss/dsi.c | 6 +-
drivers/gpu/drm/omapdrm/dss/dss.c | 6 +-
drivers/gpu/drm/omapdrm/dss/venc.c | 6 +-
drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 4 +-
7 files changed, 79 insertions(+), 44 deletions(-)
--
2.26.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH 1/5] drm/omap: Fix suspend resume regression after platform data removal
From: Tony Lindgren @ 2020-05-31 19:39 UTC (permalink / raw)
To: linux-omap
Cc: Nishanth Menon, Tero Kristo, Grygorii Strashko, Dave Gerlach,
Keerthy, Tomi Valkeinen, linux-kernel, dri-devel,
Andrew F . Davis, Peter Ujfalusi, Faiz Abbas, Laurent Pinchart,
Greg Kroah-Hartman, Suman Anna, linux-arm-kernel, Roger Quadros
In-Reply-To: <20200531193941.13179-1-tony@atomide.com>
When booting without legacy platform data, we no longer have omap_device
calling PM runtime suspend for us on suspend. This causes the driver
context not be saved as we have no suspend and resume functions defined.
Let's fix the issue by switching over to use UNIVERSAL_DEV_PM_OPS as it
will call the existing PM runtime suspend functions on suspend.
Fixes: cef766300353 ("drm/omap: Prepare DSS for probing without legacy platform data")
Reported-by: Faiz Abbas <faiz_abbas@ti.com>
Cc: dri-devel@lists.freedesktop.org
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
drivers/gpu/drm/omapdrm/dss/dispc.c | 6 ++----
drivers/gpu/drm/omapdrm/dss/dsi.c | 6 ++----
drivers/gpu/drm/omapdrm/dss/dss.c | 6 ++----
drivers/gpu/drm/omapdrm/dss/venc.c | 6 ++----
drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 4 +---
5 files changed, 9 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -4933,10 +4933,8 @@ static int dispc_runtime_resume(struct device *dev)
return 0;
}
-static const struct dev_pm_ops dispc_pm_ops = {
- .runtime_suspend = dispc_runtime_suspend,
- .runtime_resume = dispc_runtime_resume,
-};
+static UNIVERSAL_DEV_PM_OPS(dispc_pm_ops, dispc_runtime_suspend,
+ dispc_runtime_resume, NULL);
struct platform_driver omap_dispchw_driver = {
.probe = dispc_probe,
diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c
--- a/drivers/gpu/drm/omapdrm/dss/dsi.c
+++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
@@ -5464,10 +5464,8 @@ static int dsi_runtime_resume(struct device *dev)
return 0;
}
-static const struct dev_pm_ops dsi_pm_ops = {
- .runtime_suspend = dsi_runtime_suspend,
- .runtime_resume = dsi_runtime_resume,
-};
+static UNIVERSAL_DEV_PM_OPS(dsi_pm_ops, dsi_runtime_suspend,
+ dsi_runtime_resume, NULL);
struct platform_driver omap_dsihw_driver = {
.probe = dsi_probe,
diff --git a/drivers/gpu/drm/omapdrm/dss/dss.c b/drivers/gpu/drm/omapdrm/dss/dss.c
--- a/drivers/gpu/drm/omapdrm/dss/dss.c
+++ b/drivers/gpu/drm/omapdrm/dss/dss.c
@@ -1611,10 +1611,8 @@ static int dss_runtime_resume(struct device *dev)
return 0;
}
-static const struct dev_pm_ops dss_pm_ops = {
- .runtime_suspend = dss_runtime_suspend,
- .runtime_resume = dss_runtime_resume,
-};
+static UNIVERSAL_DEV_PM_OPS(dss_pm_ops, dss_runtime_suspend,
+ dss_runtime_resume, NULL);
struct platform_driver omap_dsshw_driver = {
.probe = dss_probe,
diff --git a/drivers/gpu/drm/omapdrm/dss/venc.c b/drivers/gpu/drm/omapdrm/dss/venc.c
--- a/drivers/gpu/drm/omapdrm/dss/venc.c
+++ b/drivers/gpu/drm/omapdrm/dss/venc.c
@@ -942,10 +942,8 @@ static int venc_runtime_resume(struct device *dev)
return 0;
}
-static const struct dev_pm_ops venc_pm_ops = {
- .runtime_suspend = venc_runtime_suspend,
- .runtime_resume = venc_runtime_resume,
-};
+static UNIVERSAL_DEV_PM_OPS(venc_pm_ops, venc_runtime_suspend,
+ venc_runtime_resume, NULL);
static const struct of_device_id venc_of_match[] = {
{ .compatible = "ti,omap2-venc", },
diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
--- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
@@ -1169,7 +1169,6 @@ int tiler_map_show(struct seq_file *s, void *arg)
}
#endif
-#ifdef CONFIG_PM_SLEEP
static int omap_dmm_resume(struct device *dev)
{
struct tcm_area area;
@@ -1193,9 +1192,8 @@ static int omap_dmm_resume(struct device *dev)
return 0;
}
-#endif
-static SIMPLE_DEV_PM_OPS(omap_dmm_pm_ops, NULL, omap_dmm_resume);
+static UNIVERSAL_DEV_PM_OPS(omap_dmm_pm_ops, NULL, omap_dmm_resume, NULL);
#if defined(CONFIG_OF)
static const struct dmm_platform_data dmm_omap4_platform_data = {
--
2.26.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH 2/5] bus: ti-sysc: Use optional clocks on for enable and wait for softreset bit
From: Tony Lindgren @ 2020-05-31 19:39 UTC (permalink / raw)
To: linux-omap
Cc: Nishanth Menon, Tero Kristo, Grygorii Strashko, Dave Gerlach,
Keerthy, Tomi Valkeinen, linux-kernel, dri-devel,
Andrew F . Davis, Peter Ujfalusi, Faiz Abbas, Laurent Pinchart,
Greg Kroah-Hartman, Suman Anna, linux-arm-kernel, Roger Quadros
In-Reply-To: <20200531193941.13179-1-tony@atomide.com>
Some modules reset automatically when idled, and when re-enabled, we must
wait for the automatic OCP softreset to complete. And if optional clocks
are configured, we need to keep the clocks on while waiting for the reset
to complete.
Let's fix the issue by moving the OCP softreset code to a separate
function sysc_wait_softreset(), and call it also from sysc_enable_module()
with the optional clocks enabled.
This is based on what we're already doing for legacy platform data booting
in _enable_sysc().
Fixes: 7324a7a0d5e2 ("bus: ti-sysc: Implement display subsystem reset quirk")
Reported-by: Faiz Abbas <faiz_abbas@ti.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
drivers/bus/ti-sysc.c | 81 ++++++++++++++++++++++++++++++++-----------
1 file changed, 61 insertions(+), 20 deletions(-)
diff --git a/drivers/bus/ti-sysc.c b/drivers/bus/ti-sysc.c
--- a/drivers/bus/ti-sysc.c
+++ b/drivers/bus/ti-sysc.c
@@ -221,6 +221,36 @@ static u32 sysc_read_sysstatus(struct sysc *ddata)
return sysc_read(ddata, offset);
}
+/* Poll on reset status */
+static int sysc_wait_softreset(struct sysc *ddata)
+{
+ u32 sysc_mask, syss_done, rstval;
+ int sysc_offset, syss_offset, error = 0;
+
+ sysc_offset = ddata->offsets[SYSC_SYSCONFIG];
+ syss_offset = ddata->offsets[SYSC_SYSSTATUS];
+ sysc_mask = BIT(ddata->cap->regbits->srst_shift);
+
+ if (ddata->cfg.quirks & SYSS_QUIRK_RESETDONE_INVERTED)
+ syss_done = 0;
+ else
+ syss_done = ddata->cfg.syss_mask;
+
+ if (syss_offset >= 0) {
+ error = readx_poll_timeout(sysc_read_sysstatus, ddata, rstval,
+ (rstval & ddata->cfg.syss_mask) ==
+ syss_done,
+ 100, MAX_MODULE_SOFTRESET_WAIT);
+
+ } else if (ddata->cfg.quirks & SYSC_QUIRK_RESET_STATUS) {
+ error = readx_poll_timeout(sysc_read_sysconfig, ddata, rstval,
+ !(rstval & sysc_mask),
+ 100, MAX_MODULE_SOFTRESET_WAIT);
+ }
+
+ return error;
+}
+
static int sysc_add_named_clock_from_child(struct sysc *ddata,
const char *name,
const char *optfck_name)
@@ -925,8 +955,34 @@ static int sysc_enable_module(struct device *dev)
struct sysc *ddata;
const struct sysc_regbits *regbits;
u32 reg, idlemodes, best_mode;
+ int error;
ddata = dev_get_drvdata(dev);
+
+ /*
+ * Some modules like DSS reset automatically on idle. Enable optional
+ * reset clocks and wait for OCP softreset to complete.
+ */
+ if (ddata->cfg.quirks & SYSC_QUIRK_OPT_CLKS_IN_RESET) {
+ error = sysc_enable_opt_clocks(ddata);
+ if (error) {
+ dev_err(ddata->dev,
+ "Optional clocks failed for enable: %i\n",
+ error);
+ return error;
+ }
+ }
+ error = sysc_wait_softreset(ddata);
+ if (error)
+ dev_warn(ddata->dev, "OCP softreset timed out\n");
+ if (ddata->cfg.quirks & SYSC_QUIRK_OPT_CLKS_IN_RESET)
+ sysc_disable_opt_clocks(ddata);
+
+ /*
+ * Some subsystem private interconnects, like DSS top level module,
+ * need only the automatic OCP softreset handling with no sysconfig
+ * register bits to configure.
+ */
if (ddata->offsets[SYSC_SYSCONFIG] == -ENODEV)
return 0;
@@ -1828,11 +1884,10 @@ static int sysc_legacy_init(struct sysc *ddata)
*/
static int sysc_reset(struct sysc *ddata)
{
- int sysc_offset, syss_offset, sysc_val, rstval, error = 0;
- u32 sysc_mask, syss_done;
+ int sysc_offset, sysc_val, error;
+ u32 sysc_mask;
sysc_offset = ddata->offsets[SYSC_SYSCONFIG];
- syss_offset = ddata->offsets[SYSC_SYSSTATUS];
if (ddata->legacy_mode ||
ddata->cap->regbits->srst_shift < 0 ||
@@ -1841,11 +1896,6 @@ static int sysc_reset(struct sysc *ddata)
sysc_mask = BIT(ddata->cap->regbits->srst_shift);
- if (ddata->cfg.quirks & SYSS_QUIRK_RESETDONE_INVERTED)
- syss_done = 0;
- else
- syss_done = ddata->cfg.syss_mask;
-
if (ddata->pre_reset_quirk)
ddata->pre_reset_quirk(ddata);
@@ -1862,18 +1912,9 @@ static int sysc_reset(struct sysc *ddata)
if (ddata->post_reset_quirk)
ddata->post_reset_quirk(ddata);
- /* Poll on reset status */
- if (syss_offset >= 0) {
- error = readx_poll_timeout(sysc_read_sysstatus, ddata, rstval,
- (rstval & ddata->cfg.syss_mask) ==
- syss_done,
- 100, MAX_MODULE_SOFTRESET_WAIT);
-
- } else if (ddata->cfg.quirks & SYSC_QUIRK_RESET_STATUS) {
- error = readx_poll_timeout(sysc_read_sysconfig, ddata, rstval,
- !(rstval & sysc_mask),
- 100, MAX_MODULE_SOFTRESET_WAIT);
- }
+ error = sysc_wait_softreset(ddata);
+ if (error)
+ dev_warn(ddata->dev, "OCP softreset timed out\n");
if (ddata->reset_done_quirk)
ddata->reset_done_quirk(ddata);
--
2.26.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH 3/5] bus: ti-sysc: Ignore clockactivity unless specified as a quirk
From: Tony Lindgren @ 2020-05-31 19:39 UTC (permalink / raw)
To: linux-omap
Cc: Nishanth Menon, Tero Kristo, Grygorii Strashko, Dave Gerlach,
Keerthy, Tomi Valkeinen, linux-kernel, dri-devel,
Andrew F . Davis, Peter Ujfalusi, Faiz Abbas, Laurent Pinchart,
Greg Kroah-Hartman, Suman Anna, linux-arm-kernel, Roger Quadros
In-Reply-To: <20200531193941.13179-1-tony@atomide.com>
We must ignore the clockactivity bit for most modules and not set it
unless specified for the module with SYSC_QUIRK_USE_CLOCKACT. Otherwise
the interface clock can be automatically gated constantly causing
unexpected performance issues.
Fixes: ae9ae12e9daa ("bus: ti-sysc: Handle clockactivity for enable and disable")
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
drivers/bus/ti-sysc.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/bus/ti-sysc.c b/drivers/bus/ti-sysc.c
--- a/drivers/bus/ti-sysc.c
+++ b/drivers/bus/ti-sysc.c
@@ -989,10 +989,13 @@ static int sysc_enable_module(struct device *dev)
regbits = ddata->cap->regbits;
reg = sysc_read(ddata, ddata->offsets[SYSC_SYSCONFIG]);
- /* Set CLOCKACTIVITY, we only use it for ick */
+ /*
+ * Set CLOCKACTIVITY, we only use it for ick. And we only configure it
+ * based on the SYSC_QUIRK_USE_CLOCKACT flag, not based on the hardware
+ * capabilities. See the old HWMOD_SET_DEFAULT_CLOCKACT flag.
+ */
if (regbits->clkact_shift >= 0 &&
- (ddata->cfg.quirks & SYSC_QUIRK_USE_CLOCKACT ||
- ddata->cfg.sysc_val & BIT(regbits->clkact_shift)))
+ (ddata->cfg.quirks & SYSC_QUIRK_USE_CLOCKACT))
reg |= SYSC_CLOCACT_ICK << regbits->clkact_shift;
/* Set SIDLE mode */
--
2.26.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH 4/5] bus: ti-sysc: Fix uninitialized framedonetv_irq
From: Tony Lindgren @ 2020-05-31 19:39 UTC (permalink / raw)
To: linux-omap
Cc: Nishanth Menon, Tero Kristo, Grygorii Strashko, Dave Gerlach,
Keerthy, Tomi Valkeinen, linux-kernel, dri-devel,
Andrew F . Davis, Peter Ujfalusi, Faiz Abbas, Laurent Pinchart,
Greg Kroah-Hartman, Suman Anna, linux-arm-kernel, Roger Quadros
In-Reply-To: <20200531193941.13179-1-tony@atomide.com>
We are currently only setting the framedonetv_irq disabled for the SoCs
that don't have it. But we are never setting it enabled for the SoCs that
have it. Let's initialized it to true by default.
Fixes: 7324a7a0d5e2 ("bus: ti-sysc: Implement display subsystem reset quirk")
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
drivers/bus/ti-sysc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/bus/ti-sysc.c b/drivers/bus/ti-sysc.c
--- a/drivers/bus/ti-sysc.c
+++ b/drivers/bus/ti-sysc.c
@@ -1553,7 +1553,7 @@ static u32 sysc_quirk_dispc(struct sysc *ddata, int dispc_offset,
bool lcd_en, digit_en, lcd2_en = false, lcd3_en = false;
const int lcd_en_mask = BIT(0), digit_en_mask = BIT(1);
int manager_count;
- bool framedonetv_irq;
+ bool framedonetv_irq = true;
u32 val, irq_mask = 0;
switch (sysc_soc->soc) {
@@ -1570,6 +1570,7 @@ static u32 sysc_quirk_dispc(struct sysc *ddata, int dispc_offset,
break;
case SOC_AM4:
manager_count = 1;
+ framedonetv_irq = false;
break;
case SOC_UNKNOWN:
default:
--
2.26.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH 5/5] ARM: OMAP2+: Fix legacy mode dss_reset
From: Tony Lindgren @ 2020-05-31 19:39 UTC (permalink / raw)
To: linux-omap
Cc: Nishanth Menon, Tero Kristo, Grygorii Strashko, Dave Gerlach,
Keerthy, Tomi Valkeinen, linux-kernel, dri-devel,
Andrew F . Davis, Peter Ujfalusi, Faiz Abbas, Laurent Pinchart,
Greg Kroah-Hartman, Suman Anna, linux-arm-kernel, Roger Quadros
In-Reply-To: <20200531193941.13179-1-tony@atomide.com>
We must check for "dss_core" instead of "dss" to avoid also matching
also "dss_dispc". This only matters for the mixed case of data
configured in device tree but with legacy booting ti,hwmods property
still enabled.
Fixes: 8b30919a4e3c ("ARM: OMAP2+: Handle reset quirks for dynamically allocated modules")
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
arch/arm/mach-omap2/omap_hwmod.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -3489,7 +3489,7 @@ static const struct omap_hwmod_reset dra7_reset_quirks[] = {
};
static const struct omap_hwmod_reset omap_reset_quirks[] = {
- { .match = "dss", .len = 3, .reset = omap_dss_reset, },
+ { .match = "dss_core", .len = 8, .reset = omap_dss_reset, },
{ .match = "hdq1w", .len = 5, .reset = omap_hdq1w_reset, },
{ .match = "i2c", .len = 3, .reset = omap_i2c_reset, },
{ .match = "wd_timer", .len = 8, .reset = omap2_wd_timer_reset, },
--
2.26.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox