[PATCH] gpio: Add a reference to CEC on GPIO
This adds a pointer to the CEC GPIO driver from the GPIO list of examples of drivers on top of GPIO. Cc: Hans Verkuil <hverk...@xs4all.nl> Cc: linux-media@vger.kernel.org Signed-off-by: Linus Walleij <linus.wall...@linaro.org> --- Documentation/driver-api/gpio/drivers-on-gpio.rst | 4 1 file changed, 4 insertions(+) diff --git a/Documentation/driver-api/gpio/drivers-on-gpio.rst b/Documentation/driver-api/gpio/drivers-on-gpio.rst index 7da0c1dd1f7a..f3a189320e11 100644 --- a/Documentation/driver-api/gpio/drivers-on-gpio.rst +++ b/Documentation/driver-api/gpio/drivers-on-gpio.rst @@ -85,6 +85,10 @@ hardware descriptions such as device tree or ACPI: any other serio bus to the system and makes it possible to connect drivers for e.g. keyboards and other PS/2 protocol based devices. +- cec-gpio: drivers/media/platform/cec-gpio/ is used to interact with a CEC + Consumer Electronics Control bus using only GPIO. It is used to communicate + with devices on the HDMI bus. + Apart from this there are special GPIO drivers in subsystems like MMC/SD to read card detect and write protect GPIO lines, and in the TTY serial subsystem to emulate MCTRL (modem control) signals CTS/RTS by using two GPIO lines. The -- 2.14.3
Re: [PATCH V3 1/2] [media] Add Rockchip RK1608 driver
On Tue, Mar 6, 2018 at 2:59 AM, Wen Nuan <leo@rock-chips.com> wrote:
Thank you for the patch! Nice improvements over all!
> From: Leo Wen <leo@rock-chips.com>
>
> Rk1608 is used as a PreISP to link on Soc, which mainly has two functions.
> One is to download the firmware of RK1608, and the other is to match the
> extra sensor such as camera and enable sensor by calling sensor's s_power.
>
> use below v4l2-ctl command to capture frames.
>
> v4l2-ctl --verbose -d /dev/video1 --stream-mmap=2
> --stream-to=/tmp/stream.out --stream-count=60 --stream-poll
>
> use below command to playback the video on your PC.
>
> mplayer ./stream.out -loop 0 -demuxer rawvideo -rawvideo
> w=640:h=480:size=$((640*480*3/2)):format=NV12
>
> Changes V3:
> - Instead use the new GPIO API.
This is looking really nice from a GPIO point of view!
Reviewed-by: Linus Walleij <linus.wall...@linaro.org>
The comments below are just nitpicky good-to-have.
> + pdata->gpios.reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(pdata->gpios.reset)) {
> + dev_err(dev, "could not get reset gpio\n");
> + return PTR_ERR(pdata->gpios.reset);
> + }
> + ret = gpiod_direction_output(pdata->gpios.reset, 0);
> + if (ret) {
> + dev_err(dev, "reset gpio set output error %d\n", ret);
> + return ret;
> + }
So what you do here is first assert reset and then de-assert it,
triggering a reset? Maybe you should add a comment
about that so people get it.
> + pdata->gpios.irq = devm_gpiod_get(dev, "irq", GPIOD_OUT_LOW);
> + if (IS_ERR(pdata->gpios.irq)) {
> + dev_err(dev, "could not get irq gpio\n");
> + return PTR_ERR(pdata->gpios.irq);
> + }
> + ret = gpiod_direction_input(pdata->gpios.irq);
> + if (ret) {
> + dev_err(dev, "GPIO irq set output error %d\n", ret);
> + return ret;
> + }
Why is it necessary to request the GPIO as output
and drive it low and then immediately turn it into an input?
Why not just request it with the flag GPIOD_IN and
skip the second statement?
> + pdata->gpios.pdown = devm_gpiod_get(dev, "powerdown", GPIOD_OUT_HIGH);
> + if (IS_ERR(pdata->gpios.pdown)) {
> + dev_err(dev, "could not get powerdown gpio\n");
> + return PTR_ERR(pdata->gpios.pdown);
> + }
> + ret = gpiod_direction_output(pdata->gpios.pdown, 0);
> + if (ret) {
> + dev_err(dev, "GPIO powerdown set output error %d\n", ret);
> + return ret;
> + }
This looks dangerous. Like you assert powerdown and then de-assert
it (similar to reset above). Why not just request it as
GPIOD_OUT_LOW?
> + pdata->gpios.sleepst = devm_gpiod_get(dev, "sleepst", GPIOD_OUT_HIGH);
> + if (IS_ERR(pdata->gpios.sleepst)) {
> + dev_err(dev, "Could not get powerdown gpio\n");
> + return PTR_ERR(pdata->gpios.sleepst);
> + }
> + ret = gpiod_direction_input(pdata->gpios.sleepst);
> + if (ret) {
> + dev_err(dev, "GPIO sleepst set input error %d\n", ret);
> + return ret;
> + }
Just request as GPIOD_IN and skip the second statement?
> + pdata->gpios.wakeup = devm_gpiod_get(dev, "wakeup", GPIOD_OUT_HIGH);
> + if (IS_ERR(pdata->gpios.wakeup)) {
> + dev_err(dev, "Could not get wakeup gpio\n");
> + return PTR_ERR(pdata->gpios.wakeup);
> + }
> + ret = gpiod_direction_output(pdata->gpios.wakeup, 0);
> + if (ret) {
> + dev_err(dev, "GPIO wakeup set output error %d\n", ret);
> + return ret;
> + }
Just request as GPIOD_OUT_LOW?
> +struct rk1608_gpios {
> + struct gpio_desc*reset;
> + struct gpio_desc*irq;
> + struct gpio_desc*sleepst;
> + struct gpio_desc*wakeup;
> + struct gpio_desc*pdown;
> +};
Very nice :)
Yours,
Linus Walleij
Re: [PATCH V2 2/2] dt-bindings: Document the Rockchip RK1608 bindings
On Mon, Feb 26, 2018 at 9:25 AM, Wen Nuan <leo@rock-chips.com> wrote: > From: Leo Wen <leo@rock-chips.com> > > Add DT bindings documentation for Rockchip RK1608. > > Changes V2: > - Delete spi-min-frequency property. > - Add the external sensor's control pin and clock properties. > - Delete the '' node. > > Signed-off-by: Leo Wen <leo@rock-chips.com> (...) > +- reset-gpio : GPIO connected to reset pin; > +- irq-gpio : GPIO connected to irq pin; > +- sleepst-gpio : GPIO connected to sleepst pin; > +- wakeup-gpio : GPIO connected to wakeup pin; > +- powerdown-gpio : GPIO connected to powerdown pin; All these should be named something like: reset-gpios = <>; irq-gpios = <>; etc See Documentation/devicetree/bindings/gpio/gpio.txt So all in pluralis even if it is just one line, that is the standard. > +- rockchip,powerdown0 : GPIO connected to the sensor0's powerdown pin; > +- rockchip,reset0 : GPIO connected to the sensor0's reset pin; > +- rockchip,powerdown1 : GPIO connected to the sensor1's powerdown pin; > +- rockchip,reset1 : GPIO connected to the sensor1's reset pin; Also get rid of the custom names here, either no lines should have a "rockchip", prefix or all of them. Use the name of the pin on the component, I suspect just powerdown0-gpios reset0-gpios etc By using the standard "*-gpios" suffix the kernel consumer API will be much happier as well when you use gpiod_get() & friends. Yours, Linus Walleij
Re: [PATCH V2 1/2] [media] Add Rockchip RK1608 driver
On Mon, Feb 26, 2018 at 9:16 AM, Wen Nuan <leo@rock-chips.com> wrote:
> +#include
Please do not use this old API in new drivers.
Just use GPIO descriptors and only
#include
There is documentation in
Documentation/gpio/consumer.txt
> +static int rk1608_pin_power(struct rk1608_state *pdata, int on)
> +{
> + unsigned int grf_val = 0;
> +
> + if (on) {
> + clk_prepare_enable(pdata->pd_cif);
> + clk_prepare_enable(pdata->aclk_cif);
> + clk_prepare_enable(pdata->hclk_cif);
> + clk_prepare_enable(pdata->cif_clk_in);
> + clk_prepare_enable(pdata->cif_clk_out);
> + clk_prepare_enable(pdata->clk_mipi_24m);
> + clk_prepare_enable(pdata->hclk_mipiphy);
> + clk_set_rate(pdata->cif_clk_out, RK1608_MCLK_RATE);
> +
> + clk_prepare_enable(pdata->mipi_clk);
> + clk_set_rate(pdata->mipi_clk, RK1608_MCLK_RATE);
> +
> + gpio_direction_output(pdata->rst1, 0);
Instead of this old GPIO API, put GPIO descriptors
struct gpio_desc *rst1; etc in your state container and
use gpiod_direction_output() etc.
> + pdata->grf_gpio2b_iomux = ioremap((resource_size_t)
> + (GRF_BASE_ADDR +
> + GRF_GPIO2B_IOMUX), 4);
> + grf_val = __raw_readl(pdata->grf_gpio2b_iomux);
> + __raw_writel(((grf_val) | (1 << 6) | (1 << (6 + 16))),
> +pdata->grf_gpio2b_iomux);
> +
> + pdata->grf_io_vsel = ioremap((resource_size_t)
> + (GRF_BASE_ADDR + GRF_IO_VSEL),
> 4);
> + grf_val = __raw_readl(pdata->grf_io_vsel);
> + __raw_writel(((grf_val) | (1 << 1) | (1 << (1 + 16))),
> +pdata->grf_io_vsel);
You are doing pin control on the side of the pin control subsystem
it looks like?
I think David Wu and Heiko Stubner needs to have a look at what you
are doing here to suggest other solutions.
Apart from that, why use __raw_writel instead of just writel()?
> +static int rk1608_parse_dt_property(struct rk1608_state *pdata)
> +{
(...)
> + enum of_gpio_flags flags;
Don't use this.
> + ret = of_get_named_gpio_flags(node, "rockchip,reset1", 0, );
You should not use custom properties for GPIO names,
they need names like this:
gpios-reset1 = < 1 GPIO_ACTIVE_HIGH>
See
Documentation/devicetree/bindings/gpio/gpio.txt
> + if (ret <= 0)
> + dev_err(dev, "can not find reset1 error %d\n", ret);
> + pdata->rst1 = ret;
> +
> + ret = devm_gpio_request(dev, pdata->rst1, "rockchip-reset1");
> + if (ret) {
> + dev_err(dev, "gpio pdata->rst1 %d request error %d\n",
> + pdata->rst1, ret);
> + return ret;
> + }
> + gpio_set_value(pdata->rst1, 0);
> + ret = gpio_direction_output(pdata->rst1, 0);
> + if (ret) {
> + dev_err(dev, "gpio %d direction output error %d\n",
> + pdata->rst1, ret);
> + return ret;
> + }
As you see this become tedious and repetitive.
Instead use:
pdata->rst1 = gpiod_get(dev, "reset1", GPIOD_OUT_LOW);
if (IS_ERR(pdata->rst1))
return PTR_ERR(pdata->rst1);
After this point you know that you have a valid handle on the
GPIO line and it is initialized low.
> +struct rk1608_state {
(...)
> + void __iomem*grf_gpio2b_iomux;
I suspect this should be done with pin control.
> + void __iomem*grf_io_vsel;
And this should maybe use a regulator.
> + int powerdown1;
> + int rst1;
> + int powerdown0;
> + int rst0;
> + int power_count;
> + int reset_gpio;
> + int reset_active;
> + int irq_gpio;
> + int irq;
> + int sleepst_gpio;
> + int sleepst_irq;
> + int wakeup_gpio;
> + int wakeup_active;
> + int powerdown_gpio;
> + int powerdown_active;
Anything that us a GPIO should be a struct gpio_desc *.
You do not need to keep track of if they are active I guess,
but I haven't looked close. They should probably be bool
rather than int if necessary.
But notice that we have
gpiod_get_optional() and if you use that you can
just check if the gpio_desc * is NULL like
if (!pdata->wakeup_gpio)
gpiod_set_value() ...
Yours,
Linus Walleij
Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.
On Mon, Jan 29, 2018 at 9:25 AM, Maxime Ripard
<maxime.rip...@free-electrons.com> wrote:
> On Sat, Jan 27, 2018 at 05:14:26PM +0100, Linus Walleij wrote:
>> > +void sun6i_csi_update_buf_addr(struct sun6i_csi *csi, dma_addr_t addr)
>> > +{
>> > + struct sun6i_csi_dev *sdev = sun6i_csi_to_dev(csi);
>> > + /* transform physical address to bus address */
>> > + dma_addr_t bus_addr = addr - PHYS_OFFSET;
>>
>> I am sorry if this is an unjustified drive-by comment. Maybe you
>> have already investigate other ways to do this.
>
> It's definitely not unjustified :)
>
>> Accessing PHYS_OFFSET directly seems unintuitive and not good
>> practice.
>>
>> But normally an dma_addr_t only comes from some function inside
>> such as: dma_alloc_coherent() for a contigous
>> buffer which is coherent in physical memory, or from some buffer <=
>> 64KB that is switching ownership between device and CPU explicitly
>> with dma_map* or so. Did you check with Documentation/DMA-API.txt?
>
> So, I've discussed this with Arnd a month ago or so, because I'm not
> really fond of the current approach but we haven't found better way to
> do it yet.
>
> The issue is that all the DMA accesses are done not through the main
> AXI bus, but through a separate bus dedicated for memory accesses,
> where the RAM is mapped at the address 0. So the CPU and DMA devices
> have a different mapping for the RAM.
Aha, I see the problem ... but since the dma_addr_t is supposed
to actually be the address the DMA controller sees, something is
apparently broken.
> I guess we could address this by using the field dma_pfn_offset that
> seems to be used in similar situations.
That does seem like the right thing to do (to me).
> However, in DT systems, that
> field is filled only with the parent's node dma-ranges property. In
> our case, and since the DT parenthood is based on the "control" bus,
> and not the "data" bus, our parent node would be the AXI bus, and not
> the memory bus that enforce those constraints.
>
> And other devices doing DMA through regular DMA accesses won't have
> that mapping, so we definitely shouldn't enforce it for all the
> devices there, but only the one connected to the separate memory bus.
>
> tl; dr: the DT is not really an option to store that info.
>
> I suggested setting dma_pfn_offset at probe, but Arnd didn't seem too
> fond of that approach either at the time.
>
> So, well, I guess we could do better. We just have no idea how :)
Usually of Arnd cannot suggest something smart, neither can I :(
If some aspect of the memory layout of the system *really* doesn't
fit into DT because of the assumptions that DT is doing about
how systems work, we have a problem.
Is the problem that DT's model assumes that devices have one and
only one data port to the system memory, and that is how it
populates the Linux devices?
I guess, if nothing else works, I would use auxdata in the board file.
It is our definitive last resort when DT doesn't hold.
I.e. I would go into struct of_dev_auxdata
(from ) and add a
dma_pfn_offset field that gets set into the device's dma_pfn_offset
in drivers/of/platform.c overriding anything else and use that to hammer
it down in the boardfile.
But I bet some DT people are going to have other ideas.
Yours,
Linus Walleij
Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.
On Tue, Jan 23, 2018 at 9:18 AM, Yong Deng <yong.d...@magewell.com> wrote:
> Allwinner V3s SoC features two CSI module. CSI0 is used for MIPI CSI-2
> interface and CSI1 is used for parallel interface. This is not
> documented in datasheet but by test and guess.
>
> This patch implement a v4l2 framework driver for it.
>
> Currently, the driver only support the parallel interface. MIPI-CSI2,
> ISP's support are not included in this patch.
>
> Tested-by: Maxime Ripard <maxime.rip...@free-electrons.com>
> Signed-off-by: Yong Deng <yong.d...@magewell.com>
This is cool stuff :)
> +void sun6i_csi_update_buf_addr(struct sun6i_csi *csi, dma_addr_t addr)
> +{
> + struct sun6i_csi_dev *sdev = sun6i_csi_to_dev(csi);
> + /* transform physical address to bus address */
> + dma_addr_t bus_addr = addr - PHYS_OFFSET;
I am sorry if this is an unjustified drive-by comment. Maybe you
have already investigate other ways to do this.
Accessing PHYS_OFFSET directly seems unintuitive
and not good practice.
But normally an dma_addr_t only comes from some
function inside such as:
dma_alloc_coherent() for a contigous buffer which is coherent
in physical memory, or from some buffer <= 64KB that
is switching ownership between device and CPU explicitly
with dma_map* or so. Did you check with
Documentation/DMA-API.txt?
Yours,
Linus Walleij
Re: [PATCHv5 4/5] cec-gpio: add HDMI CEC GPIO driver
On Sat, Sep 16, 2017 at 4:28 PM, Hans Verkuil <hverk...@xs4all.nl> wrote: > From: Hans Verkuil <hans.verk...@cisco.com> > > Add a simple HDMI CEC GPIO driver that sits on top of the cec-pin framework. > > While I have heard of SoCs that use the GPIO pin for CEC (apparently an > early RockChip SoC used that), the main use-case of this driver is to > function as a debugging tool. > > By connecting the CEC line to a GPIO pin on a Raspberry Pi 3 for example > it turns it into a CEC debugger and protocol analyzer. > > With 'cec-ctl --monitor-pin' the CEC traffic can be analyzed. > > But of course it can also be used with any hardware project where the > HDMI CEC line is hooked up to a pull-up gpio line. > > In addition this has (optional) support for tracing HPD changes if the > HPD is connected to a GPIO. > > Signed-off-by: Hans Verkuil <hans.verk...@cisco.com> Reviewed-by: Linus Walleij <linus.wall...@linaro.org> Yours, Linus Walleij
Re: [PATCHv5 3/5] dt-bindings: document the CEC GPIO bindings
On Sat, Sep 16, 2017 at 4:28 PM, Hans Verkuil <hverk...@xs4all.nl> wrote:
> From: Hans Verkuil <hans.verk...@cisco.com>
>
> Document the bindings for the cec-gpio module for hardware where the
> CEC line and optionally the HPD line are connected to GPIO lines.
>
> Signed-off-by: Hans Verkuil <hans.verk...@cisco.com>
Just to make things explicit:
> +Required properties:
> + - compatible: value must be "cec-gpio".
> + - cec-gpios: gpio that the CEC line is connected to.
Add "The line should be tagged as open drain."
> +Example for the Raspberry Pi 3 where the CEC line is connected to
> +pin 26 aka BCM7 aka CE1 on the GPIO pin header and the HPD line is
> +connected to pin 11 aka BCM17:
> +
#include
> +cec-gpio {
> + compatible = "cec-gpio";
> + cec-gpio = < 7 GPIO_OPEN_DRAIN>;
cec-gpios = < 7 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
> + hpd-gpio = < 17 GPIO_ACTIVE_HIGH>;
hpd-gpios = ..
With these fixups:
Reviewed-by: Linus Walleij <linus.wall...@linaro.org>
Yours,
Linus Walleij
Re: [PATCHv4 3/5] dt-bindings: document the CEC GPIO bindings
On Thu, Aug 31, 2017 at 1:01 PM, Hans Verkuil <hverk...@xs4all.nl> wrote: > + cec-gpio = < 7 GPIO_OPEN_DRAIN>; Actually if you want to be 100% specific: cec-gpio = < 7 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>; (Parens are needed.) But I'm not very picky about that, active high is implicit. Yours, Linus Walleij
Re: [PATCHv4 4/5] cec-gpio: add HDMI CEC GPIO driver
On Thu, Aug 31, 2017 at 1:01 PM, Hans Verkuil <hverk...@xs4all.nl> wrote: > From: Hans Verkuil <hans.verk...@cisco.com> > > Add a simple HDMI CEC GPIO driver that sits on top of the cec-pin framework. > > While I have heard of SoCs that use the GPIO pin for CEC (apparently an > early RockChip SoC used that), the main use-case of this driver is to > function as a debugging tool. > > By connecting the CEC line to a GPIO pin on a Raspberry Pi 3 for example > it turns it into a CEC debugger and protocol analyzer. > > With 'cec-ctl --monitor-pin' the CEC traffic can be analyzed. > > But of course it can also be used with any hardware project where the > HDMI CEC line is hooked up to a pull-up gpio line. > > In addition this has (optional) support for tracing HPD changes if the > HPD is connected to a GPIO. > > Signed-off-by: Hans Verkuil <hans.verk...@cisco.com> This looks nice! Reviewed-by: Linus Walleij <linus.wall...@linaro.org> Yours, Linus Walleij
Re: [PATCHv4 3/5] dt-bindings: document the CEC GPIO bindings
On Thu, Aug 31, 2017 at 1:01 PM, Hans Verkuil <hverk...@xs4all.nl> wrote: > From: Hans Verkuil <hans.verk...@cisco.com> > > Document the bindings for the cec-gpio module for hardware where the > CEC line and optionally the HPD line are connected to GPIO lines. > > Signed-off-by: Hans Verkuil <hans.verk...@cisco.com> Reviewed-by: Linus Walleij <linus.wall...@linaro.org> Yours, Linus Walleij
Re: [PATCHv3 3/5] dt-bindings: document the CEC GPIO bindings
On Wed, Aug 30, 2017 at 6:10 PM, Hans Verkuil <hverk...@xs4all.nl> wrote:
> From: Hans Verkuil <hans.verk...@cisco.com>
>
> Document the bindings for the cec-gpio module for hardware where the
> CEC pin and optionally the HPD pin are connected to GPIO pins.
>
> Signed-off-by: Hans Verkuil <hans.verk...@cisco.com>
I usually refer to GPIO "lines" rather than "pins" for clarity.
It's because some systems also have pin control, and then it becomes
a bit muddy what is a pin.
> +* HDMI CEC GPIO driver
> +
> +The HDMI CEC GPIO module supports CEC implementations where the CEC pin
> +is hooked up to a pull-up GPIO pin and - optionally - the HPD pin is
> +hooked up to a pull-down GPIO pin.
> +
> +Required properties:
> + - compatible: value must be "cec-gpio"
> + - cec-gpio: gpio that the CEC line is connected to
> +
> +Optional property:
> + - hpd-gpio: gpio that the HPD line is connected to
> +
> +Example for the Raspberry Pi 3 where the CEC line is connected to
> +pin 26 aka BCM7 aka CE1 on the GPIO pin header and the HPD line is
> +connected to pin 11 aka BCM17:
> +
> +cec-gpio@7 {
> + compatible = "cec-gpio";
> + cec-gpio = < 7 GPIO_ACTIVE_HIGH>;
> + hpd-gpio = < 17 GPIO_ACTIVE_HIGH>;
> +};
So what I understood from the driver is that the cec-gpio is maybe actually
an open drain output line, so in that case it should be stated in the docs and
cec-gpio = < 7 GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN>
or GPIO_LINE_OPEN_DRAIN if it is not also single-ended.
Yours,
Linus Walleij
Re: [PATCHv3 4/5] cec-gpio: add HDMI CEC GPIO driver
On Wed, Aug 30, 2017 at 6:10 PM, Hans Verkuil <hverk...@xs4all.nl> wrote:
> From: Hans Verkuil <hans.verk...@cisco.com>
>
> Add a simple HDMI CEC GPIO driver that sits on top of the cec-pin framework.
>
> While I have heard of SoCs that use the GPIO pin for CEC (apparently an
> early RockChip SoC used that), the main use-case of this driver is to
> function as a debugging tool.
>
> By connecting the CEC line to a GPIO pin on a Raspberry Pi 3 for example
> it turns it into a CEC debugger and protocol analyzer.
>
> With 'cec-ctl --monitor-pin' the CEC traffic can be analyzed.
>
> But of course it can also be used with any hardware project where the
> HDMI CEC pin is hooked up to a pull-up gpio pin.
>
> In addition this has (optional) support for tracing HPD changes if the
> HPD is connected to a GPIO.
>
> Signed-off-by: Hans Verkuil <hans.verk...@cisco.com>
Pretty cool stuff!
> +config CEC_GPIO
> + tristate "Generic GPIO-based CEC driver"
> + depends on PREEMPT
depends on GPIOLIB
or
select GPIOLIB
your pick.
> +#include
This should not be needed in new code.
> +#include
This should be enough.
> +#include
Your should not need this either.
> +struct cec_gpio {
> + struct cec_adapter *adap;
> + struct device *dev;
> + int gpio;
> + int hpd_gpio;
Think this should be:
struct gpio_desc *gpio;
struct gpio_desc *hpd_gpio;
> + int irq;
> + int hpd_irq;
> + boolhpd_is_high;
> + ktime_t hpd_ts;
> + boolis_low;
> + boolhave_irq;
> +};
> +
> +static bool cec_gpio_read(struct cec_adapter *adap)
> +{
> + struct cec_gpio *cec = cec_get_drvdata(adap);
> +
> + if (cec->is_low)
> + return false;
> + return gpio_get_value(cec->gpio);
Use descriptor accessors
gpiod_get_value()
> +static void cec_gpio_high(struct cec_adapter *adap)
> +{
> + struct cec_gpio *cec = cec_get_drvdata(adap);
> +
> + if (!cec->is_low)
> + return;
> + cec->is_low = false;
> + gpio_direction_input(cec->gpio);
Are you setting the GPIO high by setting it as input?
That sounds like you should be requesting it as
open drain in the DTS file, see
Documentation/gpio/driver.txt
for details about open drain, and use
GPIO_LINE_OPEN_DRAIN
from
> +static void cec_gpio_low(struct cec_adapter *adap)
> +{
> + struct cec_gpio *cec = cec_get_drvdata(adap);
> +
> + if (cec->is_low)
> + return;
> + if (WARN_ON_ONCE(cec->have_irq))
> + free_irq(cec->irq, cec);
> + cec->have_irq = false;
> + cec->is_low = true;
> + gpio_direction_output(cec->gpio, 0);
Yeah this looks like you're doing open drain.
gpiod_direction_output() etc.
> +static int cec_gpio_read_hpd(struct cec_adapter *adap)
> +{
> + struct cec_gpio *cec = cec_get_drvdata(adap);
> +
> + if (cec->hpd_gpio < 0)
> + return -ENOTTY;
> + return gpio_get_value(cec->hpd_gpio);
gpiod_get_value()
> +static int cec_gpio_probe(struct platform_device *pdev)
> +{
> + struct device *dev = >dev;
> + enum of_gpio_flags hpd_gpio_flags;
> + struct cec_gpio *cec;
> + int ret;
> +
> + cec = devm_kzalloc(dev, sizeof(*cec), GFP_KERNEL);
> + if (!cec)
> + return -ENOMEM;
> +
> + cec->gpio = of_get_named_gpio_flags(dev->of_node,
> + "cec-gpio", 0, _gpio_flags);
> + if (cec->gpio < 0)
> + return cec->gpio;
> +
> + cec->hpd_gpio = of_get_named_gpio_flags(dev->of_node,
> + "hpd-gpio", 0, _gpio_flags);
This is a proper device so don't use all this trouble.
cec->gpio = gpiod_get(dev, "cec-gpio", GPIOD_IN);
or similar (grep for examples!)
Same for hdp_gpio.
> + cec->irq = gpio_to_irq(cec->gpio);
gpiod_to_irq()
> + gpio_direction_input(cec->gpio);
This is not needed with the above IN flag.
But as said above, maybe you are looking for an open drain
output actually.
> + if (cec->hpd_gpio >= 0) {
> + cec->hpd_irq = gpio_to_irq(cec->hpd_gpio);
> + gpio_direction_input(cec->hpd_gpio);
Already done if you pass the right flag. This should be IN for sure.
Use gpiod_to_irq().
Please include me on subsequent posts, I want to try to use
descriptors as much as possible for new drivers.
Yours,
Linus Walleij
Re: [PATCH 8/9] iio: gyro: mpu3050: stop double error reporting
On Mon, Apr 3, 2017 at 10:38 AM, Peter Rosin <p...@axentia.se> wrote: > i2c_mux_add_adapter already logs a message on failure. > > Signed-off-by: Peter Rosin <p...@axentia.se> Reviewed-by: Linus Walleij <linus.wall...@linaro.org> Yours, Linus Walleij
Re: [PATCH 10/20] gpio: pca953x: Add optional reset gpio control
On Thu, Dec 29, 2016 at 11:27 PM, Steve Longerbeam
<slongerb...@gmail.com> wrote:
> Add optional reset-gpios pin control. If present, de-assert the
> specified reset gpio pin to bring the chip out of reset.
>
> Signed-off-by: Steve Longerbeam <steve_longerb...@mentor.com>
> Cc: Linus Walleij <linus.wall...@linaro.org>
> Cc: Alexandre Courbot <gnu...@gmail.com>
> Cc: linux-g...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
This seems like a separate patch from the other 19 patches so please send it
separately so I can handle logistics easier in the future.
> @@ -133,6 +134,7 @@ struct pca953x_chip {
> const char *const *names;
> unsigned long driver_data;
> struct regulator *regulator;
> + struct gpio_desc *reset_gpio;
Why do you even keep this around in the device state container?
As you only use it in the probe() function, use a local variable.
The descriptor will be free():ed by the devm infrastructure anyways.
> + /* see if we need to de-assert a reset pin */
> + chip->reset_gpio = devm_gpiod_get_optional(>dev,
> + "reset",
> + GPIOD_OUT_LOW);
> + if (IS_ERR(chip->reset_gpio)) {
> + dev_err(>dev, "request for reset pin
> failed\n");
> + return PTR_ERR(chip->reset_gpio);
> + }
Nice.
> + if (chip->reset_gpio) {
> + /* bring chip out of reset */
> + dev_info(>dev, "releasing reset\n");
> + gpiod_set_value(chip->reset_gpio, 0);
> + }
Is this really needed given that you set it low in the
devm_gpiod_get_optional()?
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] exynos4-is: Clear isp-i2c adapter power.ignore_children flag
On Thu, Sep 1, 2016 at 1:47 PM, Wolfram Sang <w...@the-dreams.de> wrote:
> On Thu, Sep 01, 2016 at 01:39:16PM +0200, Sylwester Nawrocki wrote:
>> Since commit 04f59143b571161d25315dd52d7a2ecc022cb71a
>> ("i2c: let I2C masters ignore their children for PM")
>> the power.ignore_children flag is set when registering an I2C
>> adapter. Since I2C transfers are not managed by the fimc-isp-i2c
>> driver its clients use pm_runtime_* calls directly to communicate
>> required power state of the bus controller.
>> However when the power.ignore_children flag is set that doesn't
>> work, so clear that flag back after registering the adapter.
>> While at it drop pm_runtime_enable() call on the i2c_adapter
>> as it is already done by the I2C subsystem when registering
>> I2C adapter.
>>
>> Cc: <sta...@vger.kernel.org> # 4.7+
>> Reported-by: Marek Szyprowski <m.szyprow...@samsung.com>
>> Signed-off-by: Sylwester Nawrocki <s.nawro...@samsung.com>
I understand what the patch is doing but not this commit message.
What does it mean when you say "Since I2C transfers are not
managed by the fimc-isp-i2c driver its clients use pm_runtime_*
calls directly to communicate required power state of the bus
controller."?
I find it very hard to understand.
The intent of the commit is to decouple I2C slave devices'
runtime PM state from their parents, so say a gyroscope on an
I2C bus does not have to bring up it's host controller to be
active, for example it usually has an IRQ line to wake up
the driver and that will talk using I2C and the I2C traffic will
wake up the I2C master.
When I look at the driver it appears it is not even used for
I2C traffic, just to take the clocks up and down and make it
possible to manage a clock using runtime PM and interface
with the DT logic... so I guess since it's likely and odd one-off
and the driver is sufficiently weird anyways, it's fine to merge
this patch making it even weirder.
Maybe a sort of mock adapter type should actually be
created in the I2C core for these things so it can be handled
there but who am I to say.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/6] ARM64: dts: amlogic: add the input pin for the IR remote
On Sat, Aug 20, 2016 at 11:54 AM, Martin Blumenstingl <martin.blumensti...@googlemail.com> wrote: > Signed-off-by: Martin Blumenstingl <martin.blumensti...@googlemail.com> Acked-by: Linus Walleij <linus.wall...@linaro.org> Merge this through the ARM SoC tree. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/6] pinctrl: amlogic: gxbb: add the IR remote input pin
On Sat, Aug 20, 2016 at 11:54 AM, Martin Blumenstingl <martin.blumensti...@googlemail.com> wrote: > This adds the IR remote receiver to the AO domain devices. > > Signed-off-by: Martin Blumenstingl <martin.blumensti...@googlemail.com> > Reviewed-by: Kevin Hilman <khil...@baylibre.com> Patch applied. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/12] gpio: Only descend into gpio directory when CONFIG_GPIOLIB is set
On Mon, Jun 13, 2016 at 10:02 PM, Andrew F. Davis <a...@ti.com> wrote: > When CONFIG_GPIOLIB is not set make will still descend into the gpio > directory but nothing will be built. This produces unneeded build > artifacts and messages in addition to slowing the build. Fix this here. > > Signed-off-by: Andrew F. Davis <a...@ti.com> Patch applied. Strange that this went unnoticed for years. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 160/182] [media]: cxd2830r: use gpiochip data pointer
This makes the driver use the data pointer added to the gpio_chip
to store a pointer to the state container instead of relying on
container_of().
Cc: Antti Palosaari <cr...@iki.fi>
Cc: Mauro Carvalho Chehab <m.che...@samsung.com>
Cc: linux-media@vger.kernel.org
Signed-off-by: Linus Walleij <linus.wall...@linaro.org>
---
Mauro: please ACK this so I can merge it in the GPIO tree.
---
drivers/media/dvb-frontends/cxd2820r_core.c | 11 ---
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/media/dvb-frontends/cxd2820r_core.c
b/drivers/media/dvb-frontends/cxd2820r_core.c
index 24a457d9d803..ba4cb7557aa5 100644
--- a/drivers/media/dvb-frontends/cxd2820r_core.c
+++ b/drivers/media/dvb-frontends/cxd2820r_core.c
@@ -606,8 +606,7 @@ static int cxd2820r_i2c_gate_ctrl(struct dvb_frontend *fe,
int enable)
static int cxd2820r_gpio_direction_output(struct gpio_chip *chip, unsigned nr,
int val)
{
- struct cxd2820r_priv *priv =
- container_of(chip, struct cxd2820r_priv, gpio_chip);
+ struct cxd2820r_priv *priv = gpiochip_get_data(chip);
u8 gpio[GPIO_COUNT];
dev_dbg(>i2c->dev, "%s: nr=%d val=%d\n", __func__, nr, val);
@@ -620,8 +619,7 @@ static int cxd2820r_gpio_direction_output(struct gpio_chip
*chip, unsigned nr,
static void cxd2820r_gpio_set(struct gpio_chip *chip, unsigned nr, int val)
{
- struct cxd2820r_priv *priv =
- container_of(chip, struct cxd2820r_priv, gpio_chip);
+ struct cxd2820r_priv *priv = gpiochip_get_data(chip);
u8 gpio[GPIO_COUNT];
dev_dbg(>i2c->dev, "%s: nr=%d val=%d\n", __func__, nr, val);
@@ -636,8 +634,7 @@ static void cxd2820r_gpio_set(struct gpio_chip *chip,
unsigned nr, int val)
static int cxd2820r_gpio_get(struct gpio_chip *chip, unsigned nr)
{
- struct cxd2820r_priv *priv =
- container_of(chip, struct cxd2820r_priv, gpio_chip);
+ struct cxd2820r_priv *priv = gpiochip_get_data(chip);
dev_dbg(>i2c->dev, "%s: nr=%d\n", __func__, nr);
@@ -731,7 +728,7 @@ struct dvb_frontend *cxd2820r_attach(const struct
cxd2820r_config *cfg,
priv->gpio_chip.base = -1; /* dynamic allocation */
priv->gpio_chip.ngpio = GPIO_COUNT;
priv->gpio_chip.can_sleep = 1;
- ret = gpiochip_add(>gpio_chip);
+ ret = gpiochip_add_data(>gpio_chip, priv);
if (ret)
goto error;
--
2.4.3
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] [media] tc358743: make reset gpio optional
On Tue, Aug 18, 2015 at 10:31 AM, Uwe Kleine-König u.kleine-koe...@pengutronix.de wrote: Commit 256148246852 ([media] tc358743: support probe from device tree) specified in the device tree binding documentation that the reset gpio is optional. Make the implementation match accordingly. Signed-off-by: Uwe Kleine-König u.kleine-koe...@pengutronix.de Reviewed-by: Linus Walleij linus.wall...@linaro.org Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] [media] tc358743: set direction of reset gpio using devm_gpiod_get
On Tue, Aug 18, 2015 at 10:31 AM, Uwe Kleine-König u.kleine-koe...@pengutronix.de wrote: Commit 256148246852 ([media] tc358743: support probe from device tree) failed to explicitly set the direction of the reset gpio. Use the optional flag of devm_gpiod_get to make up leeway. This is also necessary because the flag parameter will become mandatory soon. Signed-off-by: Uwe Kleine-König u.kleine-koe...@pengutronix.de Reviewed-by: Linus Walleij linus.wall...@linaro.org Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 02/13] pinctrl: sun6i: Add A31s pinctrl support
On Wed, Dec 17, 2014 at 6:18 PM, Hans de Goede hdego...@redhat.com wrote: The A31s is a stripped down version of the A31, as such it is missing some pins and some functions on some pins. The new pinctrl-sun6i-a31s.c this commit adds is a copy of pinctrl-sun6i-a31s.c with the missing pins and functions removed. Note there is no a31s specific version of pinctrl-sun6i-a31-r.c, as the prcm pins are identical between the A31 and the A31s. Signed-off-by: Hans de Goede hdego...@redhat.com --- Changes in v2: -Sync i2c3 muxing with v2 of pinctrl: sun6i: Add some missing functions -Add myself to the copyright header Patch applied with Maxime's ACK. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 01/13] pinctrl: sun6i: Add some missing functions
On Wed, Dec 17, 2014 at 6:18 PM, Hans de Goede hdego...@redhat.com wrote: While working on pinctrl for the A31s, I noticed that function 4 of PA15 - PA18 was missing, add these. Signed-off-by: Hans de Goede hdego...@redhat.com --- Changes in v2: -Drop the changes to the muxing of i2c3 this was based on A31s Datasheet v1.40.pdf, but all other A31 related info puts them at the pins where we already have them, so leave this as is Patch applied with Maxime's ACK. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] driver:gpio remove all usage of gpio_remove retval in driver
On Sat, Jul 12, 2014 at 10:30 PM, abdoulaye berthe berthe...@gmail.com wrote: Heads up. Requesting ACKs for this patch or I'm atleast warning that it will be applied. We're getting rid of the return value from gpiochip_remove(). this remove all reference to gpio_remove retval in all driver except pinctrl and gpio. the same thing is done for gpio and pinctrl in two different patches. Signed-off-by: abdoulaye berthe berthe...@gmail.com (...) I think this patch probably needs to be broken down per-subsystem as it hits all over the map. But let's start requesting ACKs for the individual pieces. Actually I think it will be OK to merge because there is likely not much churn around these code sites. I'm a bit torn between just wanting a big patch for this hitting drivers/gpio and smaller patches hitting one subsystem at a time. We should be able to hammer this in one switch strike. arch/arm/common/scoop.c| 10 ++ ARM SoC folks, can you ACK this? arch/mips/txx9/generic/setup.c | 4 ++-- Ralf can you ACK this? arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c | 3 ++- Benji, can you ACK this? arch/sh/boards/mach-x3proto/gpio.c | 6 ++ Aha noone can ACK this, whatever... drivers/bcma/driver_gpio.c | 3 ++- Rafał can you ACK this? drivers/hid/hid-cp2112.c | 6 ++ Jiri can you ACK this? drivers/input/keyboard/adp5588-keys.c | 4 +--- drivers/input/keyboard/adp5589-keys.c | 4 +--- drivers/input/touchscreen/ad7879.c | 10 +++--- Dmitry can you ACK this? drivers/leds/leds-pca9532.c| 10 ++ drivers/leds/leds-tca6507.c| 7 ++- Bryan can you ACK this? drivers/media/dvb-frontends/cxd2820r_core.c| 10 +++--- Mauro can you ACK this? (Hm that looks weird. Mental note to look closer at this.) drivers/mfd/asic3.c| 3 ++- drivers/mfd/htc-i2cpld.c | 8 +--- drivers/mfd/sm501.c| 17 +++-- drivers/mfd/tc6393xb.c | 13 - drivers/mfd/ucb1x00-core.c | 8 ++-- Lee/Sam can either of you ACK this? drivers/pinctrl/pinctrl-abx500.c | 15 +++ drivers/pinctrl/pinctrl-exynos5440.c | 6 +- drivers/pinctrl/pinctrl-msm.c | 10 +++--- drivers/pinctrl/pinctrl-nomadik.c | 2 +- drivers/pinctrl/pinctrl-samsung.c | 14 -- Abdoulaye: these should be in the other patch for pinctrl. drivers/platform/x86/intel_pmic_gpio.c | 3 +-- Matthew can you ACK this? drivers/ssb/driver_gpio.c | 3 ++- Michael can you (A) ACK this and (B) think of moving this driver to drivers/gpio... Patches welcome. drivers/staging/vme/devices/vme_pio2_gpio.c| 4 +--- drivers/tty/serial/max310x.c | 10 -- Greg can you ACK this? drivers/video/fbdev/via/via-gpio.c | 10 +++--- Tomi can you ACK this? sound/soc/codecs/wm5100.c | 5 + sound/soc/codecs/wm8903.c | 6 +- sound/soc/codecs/wm8962.c | 5 + sound/soc/codecs/wm8996.c | 6 +- Liam || Mark can you ACK this? Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] media: i2c: Add ADV761X support
On Wed, Nov 27, 2013 at 5:40 PM, Laurent Pinchart laurent.pinch...@ideasonboard.com wrote: (CC'ing Linus Walleij, Wolfram Sang and LAKML) On Wednesday 27 November 2013 16:32:01 Valentine wrote: On 11/27/2013 04:14 PM, Hans Verkuil wrote: Yes, of course. Although the adv7604 has two interrupt lines, so if you would want to use the second, then that would still have to be specified through the platform data. In this case the GPIO should be configured as interrupt source in the platform code. But this doesn't seem to work with R-Car GPIO since it is initialized later, and the gpio_to_irq function returns an error. The simplest way seemed to use a GPIO number in the platform data to have the adv driver configure the pin and request the IRQ. I'm not sure how to easily defer I2C board info IRQ setup (and camera/subdevice probing) until GPIO driver is ready. Good question. This looks like a core problem to me, not specific to the adv761x driver. Linus, Wolfram, do you have a comment on that ? So we recently has a large-ish discussion involving me, Stephen Warren and Jean-Christophe, leading to the conclusion that the gpio_chip and irq_chip abstractions are orthogonal, and you should be able to request a GPIO or IRQ without interacting with the other subsystem. Specifically you should be able to request an IRQ from the irq_chip portions of the driver without first requesting the GPIO line. Some drivers already support this. We added an internal API to the gpiolib so that the lib, *internally* can be made aware that a certain GPIO line is used for IRQs, see commit d468bf9ecaabd3bf3a6134e5a369ced82b1d1ca1 gpio: add API to be strict about GPIO IRQ usage So I guess the answer to the question is something like, fix the GPIO driver to stop requiring the GPIO lines to be requested and configured before being used as IRQs, delete that code, and while you're at it add a call to gpiod_lock_as_irq() to your GPIO driver in the right spot: examples are on the mailing list and my mark-irqs branch in the GPIO tree. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] media: i2c: Add ADV761X support
On Fri, Nov 29, 2013 at 11:45 AM, Lars-Peter Clausen l...@metafoo.de wrote: On 11/29/2013 11:37 AM, Linus Walleij wrote: (...) Specifically you should be able to request an IRQ from the irq_chip portions of the driver without first requesting the GPIO line. Some drivers already support this. We added an internal API to the gpiolib so that the lib, *internally* can be made aware that a certain GPIO line is used for IRQs, see commit d468bf9ecaabd3bf3a6134e5a369ced82b1d1ca1 gpio: add API to be strict about GPIO IRQ usage So I guess the answer to the question is something like, fix the GPIO driver to stop requiring the GPIO lines to be requested and configured before being used as IRQs, delete that code, and while you're at it add a call to gpiod_lock_as_irq() to your GPIO driver in the right spot: examples are on the mailing list and my mark-irqs branch in the GPIO tree. As far as I understand it this already works more or less with the driver. The problem is that the IRQ numbers are dynamically allocated, while the GPIO numbers apparently are not. So the board code knows the the GPIO number at compile time and can pass this to the diver which then does a gpio_to_irq to lookup the IRQ number. This of course isn't really a problem with devicetree, but only with platform board code. This has been solved *also* for platform board code by the new, fresh GPIO descriptor mechanism, see Documentation/gpio/* in Torvalds' git HEAD. In your board file provide something like that: http://marc.info/?l=linux-gpiom=138546046203600w=2 Then switch the driver to use the gpiod_* interface like: http://marc.info/?l=linux-gpiom=138546036028076w=2 Problem solved. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] media: i2c: Add ADV761X support
On Fri, Nov 29, 2013 at 1:14 PM, Valentine valentine.bars...@cogentembedded.com wrote: On 11/29/2013 02:45 PM, Lars-Peter Clausen wrote: On 11/29/2013 11:37 AM, Linus Walleij wrote: So I guess the answer to the question is something like, fix the GPIO driver to stop requiring the GPIO lines to be requested and configured before being used as IRQs, delete that code, and while you're at it add a call to gpiod_lock_as_irq() to your GPIO driver in the right spot: examples are on the mailing list and my mark-irqs branch in the GPIO tree. As far as I understand it this already works more or less with the driver. The problem is that the IRQ numbers are dynamically allocated, while the GPIO numbers apparently are not. So the board code knows the the GPIO number at compile time and can pass this to the diver which then does a gpio_to_irq to lookup the IRQ number. (...) This of course isn't really a problem with devicetree, but only with platform board code. I'm not sure what's the difference here and why it is not a problem with devicetree? It's no problem when using devicetree because you can obtain the GPIOs directly from the node with of_get_gpio() and of_get_named_gpio() in the special DT probe path. But don't do that! Instead switch the whole driver, and preferably the whole platform, to use descriptors. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] media: i2c: Add ADV761X support
On Fri, Nov 29, 2013 at 2:48 PM, Lars-Peter Clausen l...@metafoo.de wrote: On 11/29/2013 02:42 PM, Linus Walleij wrote: On Fri, Nov 29, 2013 at 11:45 AM, Lars-Peter Clausen l...@metafoo.de wrote: As far as I understand it this already works more or less with the driver. The problem is that the IRQ numbers are dynamically allocated, while the GPIO numbers apparently are not. So the board code knows the the GPIO number at compile time and can pass this to the diver which then does a gpio_to_irq to lookup the IRQ number. This of course isn't really a problem with devicetree, but only with platform board code. This has been solved *also* for platform board code by the new, fresh GPIO descriptor mechanism, see Documentation/gpio/* in Torvalds' git HEAD. This works when the GPIO numbers are dynamically allocated (which are static in this case), but not for IRQ numbers. Sorry I don't get what you're after here. I'm not the subsystem maintainer for IRQ chips ... In the DT boot path for platform or AMBA devices the IRQs are automatically resolved to resources and passed with the device so that is certainly not the problem, right? I guess you may be referring to the problem of instatiating a dynamic IRQ chip in *board code* and then passing the obtained dynamic IRQ numbers as resources to the devices also created in a board file? That would be like you're asking for a function that would return the base of an irq_chip, that needs to be discussed with the irq maintainers, so not much I can say, but maybe I misunderstood this? Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] media: i2c: Add ADV761X support
On Fri, Nov 29, 2013 at 9:05 PM, Lars-Peter Clausen l...@metafoo.de wrote: On 11/29/2013 08:52 PM, Linus Walleij wrote: I guess you may be referring to the problem of instatiating a dynamic IRQ chip in *board code* and then passing the obtained dynamic IRQ numbers as resources to the devices also created in a board file? Yes. That would be like you're asking for a function that would return the base of an irq_chip, that needs to be discussed with the irq maintainers, so not much I can say, but maybe I misunderstood this? I my opinion the best solution for this problem is to have the same lookup mechanism we've had for clocks, regulators, etc and now also GPIOs. Hm this needs to be discussed with some irq people... Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/5] gpio: samsung: Use CONFIG_ARCH_S3C64XX to check for S3C64xx support
On Sat, Sep 28, 2013 at 8:21 PM, Tomasz Figa tomasz.f...@gmail.com wrote: Since CONFIG_PLAT_S3C64XX is going to be removed, this patch modifies the gpio-samsung driver to use the proper way of checking for S3C64xx support - CONFIG_ARCH_S3C64XX. Signed-off-by: Tomasz Figa tomasz.f...@gmail.com Acked-by: Linus Walleij linus.wall...@linaro.org I assume that this will go through ARM SoC? Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] staging: media/lirc: switch to use gpiolib
On Fri, Sep 13, 2013 at 9:14 AM, Linus Walleij linus.wall...@linaro.org wrote: Hi Mauro, On Tue, Sep 10, 2013 at 2:31 PM, Linus Walleij linus.wall...@linaro.org wrote: The lirc serial module has special hooks to work with NSLU2, switch these over to use gpiolib, as that is available on the ixp4 platform. Not even compile tested as there is no way to select this driver from menuconfig on the ixp4 platform. Cc: Imre Kaloz ka...@openwrt.org Cc: Krzysztof Halasa k...@pm.waw.pl Cc: Alexandre Courbot acour...@nvidia.com Cc: Greg Kroah-Hartman gre...@linuxfoundation.org Signed-off-by: Linus Walleij linus.wall...@linaro.org --- Hi Greg: I'm seeking an ACK on this patch to take it through the GPIO tree as part of a clean-up attempt to remove custom GPIO APIs. Could you ACK this patch if it looks OK to you? Mauro, ping on this. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] staging: media/lirc: switch to use gpiolib
Hi Mauro, On Tue, Sep 10, 2013 at 2:31 PM, Linus Walleij linus.wall...@linaro.org wrote: The lirc serial module has special hooks to work with NSLU2, switch these over to use gpiolib, as that is available on the ixp4 platform. Not even compile tested as there is no way to select this driver from menuconfig on the ixp4 platform. Cc: Imre Kaloz ka...@openwrt.org Cc: Krzysztof Halasa k...@pm.waw.pl Cc: Alexandre Courbot acour...@nvidia.com Cc: Greg Kroah-Hartman gre...@linuxfoundation.org Signed-off-by: Linus Walleij linus.wall...@linaro.org --- Hi Greg: I'm seeking an ACK on this patch to take it through the GPIO tree as part of a clean-up attempt to remove custom GPIO APIs. Could you ACK this patch if it looks OK to you? Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 2/2] video: drm: exynos: Add pinctrl support to fimd
On Thu, Feb 28, 2013 at 5:12 AM, Vikas Sajjan vikas.saj...@linaro.org wrote: Adds support for pinctrl to drm fimd Signed-off-by: Leela Krishna Amudala l.kris...@samsung.com Signed-off-by: Vikas Sajjan vikas.saj...@linaro.org (...) + pctrl = devm_pinctrl_get_select_default(dev); NAK. The device core will do this for you as of commit ab78029ecc347debbd737f06688d788bd9d60c1d drivers/pinctrl: grab default handles from device core Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/1] video: drm: exynos: Add display-timing node parsing using video helper function
On Thu, Feb 28, 2013 at 2:51 AM, Joonyoung Shim jy0922.s...@samsung.com wrote: My mistake. If CONFIG_PINCTRL is disabled, devm_pinctrl_get_select_default can return NULL. Yes, and that is perfectly legal and *NOT* an error. devm_pinctrl_get_select() and pinctrl_get_select() also need IS_ERR_OR_NULL instead of IS_ERR? No. In fact, IS_ERR_OR_NULL() shall not be used at all. Check the LKML mailinglist for Russells recent struggle to purge it from the kernel. And many drivers using above functions don't check NULL, right? No, and they should not. Stub pinctrl handles, just like stub clocks and stub regulators, are perfectly legal, just that they have no effect. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] ux500: convert struct spinlock to spinlock_t
On Thu, Nov 29, 2012 at 9:45 PM, Luis R. Rodriguez mcg...@do-not-panic.com wrote: From: Luis R. Rodriguez mcg...@do-not-panic.com spinlock_t should always be used. I was unable to build test with allmodconfig: mcgrof@frijol ~/linux-next (git::(no branch))$ make C=1 M=drivers/crypto/ux500/ WARNING: Symbol version dump /home/mcgrof/linux-next/Module.symvers is missing; modules will have no dependencies and modversions. LD drivers/crypto/ux500/built-in.o Building modules, stage 2. MODPOST 0 modules Cc: Srinidhi Kasagar srinidhi.kasa...@stericsson.com Cc: Linus Walleij linus.wall...@linaro.org Cc: linux-arm-ker...@lists.infradead.org Reported-by: Hauke Mehrtens ha...@hauke-m.de Signed-off-by: Luis R. Rodriguez mcg...@do-not-panic.com Looks correct to me. Acked-by: Linus Walleij linus.wall...@linaro.org Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Handling Ctrl-c like events in 's_power' implementation when we have a GPIO controlling the sensor CE
On Wed, Feb 1, 2012 at 6:01 AM, Bhupesh SHARMA bhupesh.sha...@st.com wrote: Our board has a I2C controlled camera sensor whose Chip Enable (CE) pin is driven via a GPIO. This GPIO is made available by a I2C-to-GPIO expander chip (STMPE801, see user manual [1]) (...) the I2C controller driver (we use the standard SYNOPSYS designware device driver present in mainline, see [3]) returns -ERESTARTSYS in response to the write command we had requested for putting the sensor to power-off state (as it has received the ctrl-c kill signal). So what happens if you go into the I2C driver and replace all things that look like this: ret = wait_for_completion_interruptible_timeout(dev-cmd_complete, HZ); With this: ret = wait_for_completion_timeout(dev-cmd_complete, HZ); (Non-interruptible.) This is usually the problem. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [st-ericsson] v4l2 vs omx for camera
2011/2/24 Edward Hervey bilb...@gmail.com: What *needs* to be solved is an API for data allocation/passing at the kernel level which v4l2,omx,X,GL,vdpau,vaapi,... can use and that userspace (like GStreamer) can pass around, monitor and know about. I think the patches sent out from ST-Ericsson's Johan Mossberg to linux-mm for HWMEM (hardware memory) deals exactly with buffer passing, pinning of buffers and so on. The CMA (Contigous Memory Allocator) has been slightly modified to fit hand-in-glove with HWMEM, so CMA provides buffers, HWMEM pass them around. Johan, when you re-spin the HWMEM patchset, can you include linaro-dev and linux-media in the CC? I think there is *much* interest in this mechanism, people just don't know from the name what it really does. Maybe it should be called mediamem or something instead... Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [st-ericsson] v4l2 vs omx for camera
2011/2/23 Sachin Gupta sachin.gu...@linaro.org: The imaging coprocessor in today's platforms have a general purpose DSP attached to it I have seen some work being done to use this DSP for graphics/audio processing in case the camera use case is not being tried or also if the camera usecases does not consume the full bandwidth of this dsp.I am not sure how v4l2 would fit in such an architecture, Earlier in this thread I discussed TI:s DSPbridge. In drivers/staging/tidspbridge http://omappedia.org/wiki/DSPBridge_Project you find the TI hackers happy at work with providing a DSP accelerator subsystem. Isn't it possible for a V4L2 component to use this interface (or something more evolved, generic) as backend for assorted DSP offloading? So using one kernel framework does not exclude using another one at the same time. Whereas something like DSPbridge will load firmware into DSP accelerators and provide control/datapath for that, this can in turn be used by some camera or codec which in turn presents a V4L2 or ALSA interface. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/10] MCDE: Add build files and bus
2010/11/26 Arnd Bergmann a...@arndb.de: * When you say that the devices are static, I hope you do not mean static in the C language sense. We used to allow devices to be declared as static struct and registered using platform_device_register (or other bus specific functions). This is no longer valid and we are removing the existing users, do not add new ones. When creating a platform device, use platform_device_register_simple or platform_device_register_resndata. Is this part of the generic ARM runtime multi-platform kernel and device trees shebang? The Ux500 still isn't in that sector, it needs extensive rewriting of arch/arm/mach-ux500 to be done first, so as to support e.g. U8500 and U5500 with a single kernel image. Trying to skin that cat that as part of this review is a bit too much to ask IMO, I'd rather have the author of this driver adapt to whatever platform data registration mechanism is in place for the merge window. Else it needs fixing as part of a bigger endavour to root out compile-time platform configuration. Yours Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
