Re: [PATCH 1/4] ARM/omap1: switch to use dma_direct_set_offset for lbus DMA offsets
Hi Tony,
On Friday, September 18, 2020 7:49:33 A.M. CEST Tony Lindgren wrote:
> * Christoph Hellwig [200917 17:37]:
> > Switch the omap1510 platform ohci device to use dma_direct_set_offset
> > to set the DMA offset instead of using direct hooks into the DMA
> > mapping code and remove the now unused hooks.
>
> Looks nice to me :) I still can't test this probably for few more weeks
> though but hopefully Aaro or Janusz (Added to Cc) can test it.
Works for me on Amstrad Delta (tested with a USB ethernet adapter).
Tested-by: Janusz Krzysztofik
Thanks,
Janusz
>
> Regards,
>
> Tony
>
> > Signed-off-by: Christoph Hellwig
> > ---
> > arch/arm/include/asm/dma-direct.h | 18 -
> > arch/arm/mach-omap1/include/mach/memory.h | 31 ---
> > arch/arm/mach-omap1/usb.c | 22
> > 3 files changed, 22 insertions(+), 49 deletions(-)
> >
> > diff --git a/arch/arm/include/asm/dma-direct.h
> > b/arch/arm/include/asm/dma-direct.h
> > index 436544aeb83405..77fcb7ee5ec907 100644
> > --- a/arch/arm/include/asm/dma-direct.h
> > +++ b/arch/arm/include/asm/dma-direct.h
> > @@ -9,7 +9,6 @@
> > * functions used internally by the DMA-mapping API to provide DMA
> > * addresses. They must not be used by drivers.
> > */
> > -#ifndef __arch_pfn_to_dma
> > static inline dma_addr_t pfn_to_dma(struct device *dev, unsigned long pfn)
> > {
> > if (dev && dev->dma_range_map)
> > @@ -34,23 +33,6 @@ static inline dma_addr_t virt_to_dma(struct device *dev,
> > void *addr)
> > return (dma_addr_t)__virt_to_bus((unsigned long)(addr));
> > }
> >
> > -#else
> > -static inline dma_addr_t pfn_to_dma(struct device *dev, unsigned long pfn)
> > -{
> > - return __arch_pfn_to_dma(dev, pfn);
> > -}
> > -
> > -static inline unsigned long dma_to_pfn(struct device *dev, dma_addr_t addr)
> > -{
> > - return __arch_dma_to_pfn(dev, addr);
> > -}
> > -
> > -static inline dma_addr_t virt_to_dma(struct device *dev, void *addr)
> > -{
> > - return __arch_virt_to_dma(dev, addr);
> > -}
> > -#endif
> > -
> > static inline dma_addr_t phys_to_dma(struct device *dev, phys_addr_t paddr)
> > {
> > unsigned int offset = paddr & ~PAGE_MASK;
> > diff --git a/arch/arm/mach-omap1/include/mach/memory.h
> > b/arch/arm/mach-omap1/include/mach/memory.h
> > index 1142560e0078f5..36bccb6ab8 100644
> > --- a/arch/arm/mach-omap1/include/mach/memory.h
> > +++ b/arch/arm/mach-omap1/include/mach/memory.h
> > @@ -14,42 +14,11 @@
> > * OMAP-1510 bus address is translated into a Local Bus address if the
> > * OMAP bus type is lbus. We do the address translation based on the
> > * device overriding the defaults used in the dma-mapping API.
> > - * Note that the is_lbus_device() test is not very efficient on 1510
> > - * because of the strncmp().
> > */
> > -#if defined(CONFIG_ARCH_OMAP15XX) && !defined(__ASSEMBLER__)
> >
> > /*
> > * OMAP-1510 Local Bus address offset
> > */
> > #define OMAP1510_LB_OFFSET UL(0x3000)
> >
> > -#define virt_to_lbus(x)((x) - PAGE_OFFSET + OMAP1510_LB_OFFSET)
> > -#define lbus_to_virt(x)((x) - OMAP1510_LB_OFFSET + PAGE_OFFSET)
> > -#define is_lbus_device(dev)(cpu_is_omap15xx() && dev &&
> > (strncmp(dev_name(dev), "ohci", 4) == 0))
> > -
> > -#define __arch_pfn_to_dma(dev, pfn)\
> > - ({ dma_addr_t __dma = __pfn_to_phys(pfn); \
> > - if (is_lbus_device(dev)) \
> > - __dma = __dma - PHYS_OFFSET + OMAP1510_LB_OFFSET; \
> > - __dma; })
> > -
> > -#define __arch_dma_to_pfn(dev, addr) \
> > - ({ dma_addr_t __dma = addr; \
> > - if (is_lbus_device(dev)) \
> > - __dma += PHYS_OFFSET - OMAP1510_LB_OFFSET; \
> > - __phys_to_pfn(__dma);\
> > - })
> > -
> > -#define __arch_dma_to_virt(dev, addr) ({ (void *)
> > (is_lbus_device(dev) ? \
> > - lbus_to_virt(addr) : \
> > - __phys_to_virt(addr)); })
> > -
> > -#define __arch_virt_to_dma(dev, addr) ({ unsigned long __addr =
> > (unsigned long)(addr); \
> > - (dma_addr_t) (is_lbus_device(dev) ? \
> > -
[PATCH] mtd: rawnand: ams-delta: Fix non-OF build warning
Commit 7c2f66a960fc ("mtd: rawnand: ams-delta: Add module device
tables") introduced an OF module device table but wrapped a reference
to it with of_match_ptr() which resolves to NULL in non-OF configs.
That resulted in a clang compiler warning on unused variable in non-OF
builds. Fix it.
drivers/mtd/nand/raw/ams-delta.c:373:34: warning: unused variable
'gpio_nand_of_id_table' [-Wunused-const-variable]
static const struct of_device_id gpio_nand_of_id_table[] = {
^
1 warning generated.
Fixes: 7c2f66a960fc ("mtd: rawnand: ams-delta: Add module device tables")
Reported-by: kernel test robot
Signed-off-by: Janusz Krzysztofik
---
drivers/mtd/nand/raw/ams-delta.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c
index fdba155416d2..0bf4cfc25147 100644
--- a/drivers/mtd/nand/raw/ams-delta.c
+++ b/drivers/mtd/nand/raw/ams-delta.c
@@ -400,12 +400,14 @@ static int gpio_nand_remove(struct platform_device *pdev)
return 0;
}
+#ifdef CONFIG_OF
static const struct of_device_id gpio_nand_of_id_table[] = {
{
/* sentinel */
},
};
MODULE_DEVICE_TABLE(of, gpio_nand_of_id_table);
+#endif
static const struct platform_device_id gpio_nand_plat_id_table[] = {
{
--
2.26.2
Re: [PATCH 6/6] media: ov6650: Fix arbitrary selection of master clock rate
Hi Sakari,
On Friday, October 18, 2019 8:54:19 P.M. CEST Sakari Ailus wrote:
> Hi Janusz,
>
> On Sun, Oct 13, 2019 at 02:50:50PM +0200, Janusz Krzysztofik wrote:
> > A hardcoded 12 MHz master clock frequency has been assumed since
> > conversion of the driver from soc_camera sensor to a standalone V4L2
> > subdevice by commit 23a52386fabe ("media: ov6650: convert to standalone
> > v4l2 subdevice"). Fix it.
> >
> > Define a static table of supported master clock rates (fix misnamed
> > symbol while being at it), then use v4l2_clk_get/set_rate() to obtain
> > a clock rate matching one of those supported. On success, apply
> > respective master clock hardware divisor provided by the matching
> > element of the table.
> >
> > Signed-off-by: Janusz Krzysztofik
> > ---
> > drivers/media/i2c/ov6650.c | 64 ++
> > 1 file changed, 58 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
> > index f4723c0b5c70..13fd7c4699b2 100644
> > --- a/drivers/media/i2c/ov6650.c
> > +++ b/drivers/media/i2c/ov6650.c
> > @@ -124,7 +124,7 @@
> >
> > #define DEF_AECH 0x4D
> >
> > -#define CLKRC_6MHz 0x00
> > +#define CLKRC_8MHz 0x00
> > #define CLKRC_12MHz0x40
> > #define CLKRC_16MHz0x80
> > #define CLKRC_24MHz0xc0
> > @@ -201,6 +201,29 @@ struct ov6650 {
> > u32 code;
> > };
> >
> > +struct ov6650_xclk {
> > + unsigned long rate;
> > + u8 clkrc;
> > +};
> > +
> > +static const struct ov6650_xclk ov6650_xclk[] = {
> > +{
> > + .rate = 800,
> > + .clkrc = CLKRC_8MHz,
> > +},
> > +{
> > + .rate = 1200,
> > + .clkrc = CLKRC_12MHz,
> > +},
> > +{
> > + .rate = 1600,
> > + .clkrc = CLKRC_16MHz,
> > +},
> > +{
> > + .rate = 2400,
> > + .clkrc = CLKRC_24MHz,
> > +},
> > +};
> >
> > static u32 ov6650_codes[] = {
> > MEDIA_BUS_FMT_YUYV8_2X8,
> > @@ -774,7 +797,7 @@ static int ov6650_reset(struct i2c_client *client)
> > }
> >
> > /* program default register values */
> > -static int ov6650_prog_dflt(struct i2c_client *client)
> > +static int ov6650_prog_dflt(struct i2c_client *client, u8 clkrc)
> > {
> > int ret;
> >
> > @@ -782,7 +805,7 @@ static int ov6650_prog_dflt(struct i2c_client *client)
> >
> > ret = ov6650_reg_write(client, REG_COMA, 0);/* ~COMA_RESET */
> > if (!ret)
> > - ret = ov6650_reg_write(client, REG_CLKRC, CLKRC_12MHz);
> > + ret = ov6650_reg_write(client, REG_CLKRC, clkrc);
> > if (!ret)
> > ret = ov6650_reg_rmw(client, REG_COMB, 0, COMB_BAND_FILTER);
> >
> > @@ -793,8 +816,10 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
> > {
> > struct i2c_client *client = v4l2_get_subdevdata(sd);
> > struct ov6650 *priv = to_ov6650(client);
> > - u8 pidh, pidl, midh, midl;
> > - int ret;
> > + const struct ov6650_xclk *xclk;
> > + unsigned long rate;
> > + u8 pidh, pidl, midh, midl;
> > + int i, ret;
> >
> > priv->clk = v4l2_clk_get(&client->dev, NULL);
> > if (IS_ERR(priv->clk)) {
> > @@ -803,6 +828,33 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
> > return ret;
> > }
> >
> > + rate = v4l2_clk_get_rate(priv->clk);
> > + for (i = 0; rate && i < ARRAY_SIZE(ov6650_xclk); i++) {
> > + if (rate != ov6650_xclk[i].rate)
> > + continue;
> > +
> > + xclk = &ov6650_xclk[i];
> > + dev_info(&client->dev, "using host default clock rate %lukHz\n",
> > +rate / 1000);
> > + break;
> > + }
>
> xclk is undefined unless it was set in the previous loop.
Indeed, sorry for that.
> Please initialise
> to to NULL. I can fix that, too, while applying the patch.
OK, please do if you can.
Thanks,
Janusz
> > + for (i = 0; !xclk && i < ARRAY_SIZE(ov6650_xclk); i++) {
> > + ret = v4l2_clk_set_rate(priv->clk, ov6650_xclk[i].rate);
> > + if (ret || v4l2_clk_get_rate(priv->clk) != ov6650_xclk[i].rate)
> > + continue;
> > +
> > + xclk = &ov6650_xclk[i];
> &g
[PATCH 2/6] media: ov6650: Drop obsolete .pclk_limit attribute
That attribute used to be obtained from platform data by a soc_camera
host interface and passed to the sensor driver for .s_mbus_fmt() video
operation callback, later reused as .set_fmt() pad operation callback,
to be able to limit frame rate. The driver stored that value in its
private structure for further use from .g/s_parm(), later converted to
g/s_frame_interval().
On conversion of the driver from soc_camera sensor to a standalone V4L2
subdevice by commit 23a52386fabe ("media: ov6650: convert to standalone
v4l2 subdevice"), that attribute had been replaced by a constant and
hardcoded to an arbitrarily chosen pixel clock limit. Drop it. Host
interfaces can limit frame rate if needed by calling
.s_frame_interval().
Signed-off-by: Janusz Krzysztofik
---
drivers/media/i2c/ov6650.c | 14 --
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index f60aeb1f7813..a50244401491 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -197,7 +197,6 @@ struct ov6650 {
struct v4l2_clk *clk;
boolhalf_scale; /* scale down output by 2 */
struct v4l2_rectrect; /* sensor cropping window */
- unsigned long pclk_limit; /* from host */
unsigned long pclk_max; /* from resolution and format */
struct v4l2_fract tpf;/* as requested with
s_frame_interval */
u32 code;
@@ -546,8 +545,7 @@ static bool is_unscaled_ok(int width, int height, struct
v4l2_rect *rect)
return width > rect->width >> 1 || height > rect->height >> 1;
}
-static u8 to_clkrc(struct v4l2_fract *timeperframe,
- unsigned long pclk_limit, unsigned long pclk_max)
+static u8 to_clkrc(struct v4l2_fract *timeperframe, unsigned long pclk_max)
{
unsigned long pclk;
@@ -557,9 +555,6 @@ static u8 to_clkrc(struct v4l2_fract *timeperframe,
else
pclk = pclk_max;
- if (pclk_limit && pclk_limit < pclk)
- pclk = pclk_limit;
-
return (pclk_max - 1) / pclk;
}
@@ -653,10 +648,9 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct
v4l2_mbus_framefmt *mf)
clkrc = CLKRC_12MHz;
mclk = 1200;
- priv->pclk_limit = 1334000;
dev_dbg(&client->dev, "using 12MHz input clock\n");
- clkrc |= to_clkrc(&priv->tpf, priv->pclk_limit, priv->pclk_max);
+ clkrc |= to_clkrc(&priv->tpf, priv->pclk_max);
pclk = priv->pclk_max / GET_CLKRC_DIV(clkrc);
dev_dbg(&client->dev, "pixel clock divider: %ld.%ld\n",
@@ -756,7 +750,7 @@ static int ov6650_g_frame_interval(struct v4l2_subdev *sd,
struct ov6650 *priv = to_ov6650(client);
ival->interval.numerator = GET_CLKRC_DIV(to_clkrc(&priv->tpf,
- priv->pclk_limit, priv->pclk_max));
+ priv->pclk_max));
ival->interval.denominator = FRAME_RATE_MAX;
dev_dbg(&client->dev, "Frame interval: %u/%u s\n",
@@ -787,7 +781,7 @@ static int ov6650_s_frame_interval(struct v4l2_subdev *sd,
tpf->numerator = div;
tpf->denominator = FRAME_RATE_MAX;
- clkrc = to_clkrc(tpf, priv->pclk_limit, priv->pclk_max);
+ clkrc = to_clkrc(tpf, priv->pclk_max);
ret = ov6650_reg_rmw(client, REG_CLKRC, clkrc, CLKRC_DIV_MASK);
if (!ret) {
--
2.21.0
[PATCH 4/6] media: ov6650: Don't reapply pixel clock divisor on format change
As calculation of pixel clock hardware divisor no longer depends on
mbus format specific maximum pixel clock, there is no need to reapply
the divisor on format change. Drop related code from ov6650_s_fmt()
helper.
Since a master clock hardware divisor, so far applied only together
with the pixel clock divisor in a single operation, will no longer be
applied from ov6650_s_fmt(), apply it, still using a hardcoded value
for now, from ov6650_prog_dflt() helper so hardware is still
initialised correctly on device probe.
Signed-off-by: Janusz Krzysztofik
---
drivers/media/i2c/ov6650.c | 17 +++--
1 file changed, 3 insertions(+), 14 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index 61ddd4ea4c26..e95ea132ef06 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -564,8 +564,7 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct
v4l2_mbus_framefmt *mf)
.r.height = mf->height << half_scale,
};
u32 code = mf->code;
- unsigned long mclk, pclk;
- u8 coma_set = 0, coma_mask = 0, coml_set, coml_mask, clkrc;
+ u8 coma_set = 0, coma_mask = 0, coml_set, coml_mask;
int ret;
/* select color matrix configuration for given color encoding */
@@ -635,21 +634,9 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct
v4l2_mbus_framefmt *mf)
coma_mask |= COMA_QCIF;
}
- clkrc = CLKRC_12MHz;
- mclk = 1200;
- dev_dbg(&client->dev, "using 12MHz input clock\n");
-
- clkrc |= to_clkrc(priv->tpf.numerator);
-
- pclk = priv->pclk_max / GET_CLKRC_DIV(clkrc);
- dev_dbg(&client->dev, "pixel clock divider: %ld.%ld\n",
- mclk / pclk, 10 * mclk % pclk / pclk);
-
ret = ov6650_set_selection(sd, NULL, &sel);
if (!ret)
ret = ov6650_reg_rmw(client, REG_COMA, coma_set, coma_mask);
- if (!ret)
- ret = ov6650_reg_write(client, REG_CLKRC, clkrc);
if (!ret) {
priv->half_scale = half_scale;
@@ -798,6 +785,8 @@ static int ov6650_prog_dflt(struct i2c_client *client)
dev_dbg(&client->dev, "initializing\n");
ret = ov6650_reg_write(client, REG_COMA, 0);/* ~COMA_RESET */
+ if (!ret)
+ ret = ov6650_reg_write(client, REG_CLKRC, CLKRC_12MHz);
if (!ret)
ret = ov6650_reg_rmw(client, REG_COMB, 0, COMB_BAND_FILTER);
--
2.21.0
[PATCH 1/6] media: ov6650: Fix stored frame interval not in sync with hardware
The driver stores a frame interval value supposed to be in line with
hardware state in a device private structure. Since the driver initial
submission, the respective field of the structure has never been
initialised on device probe. Moreover, if updated from
.s_frame_interval(), a new value is stored before it is applied on
hardware. If an error occurs during device update, the stored value
may no longer reflect hardware state and consecutive calls to
.g_frame_interval() may return incorrect information.
Assuming a failed update of the device means its actual state hasn't
changed, update the frame interval field of the device private
structure with a new value only after it is successfully applied on
hardware so it always reflects actual hardware state to the extent
possible. Also, initialise the field with hardware default frame
interval on device probe.
Signed-off-by: Janusz Krzysztofik
---
drivers/media/i2c/ov6650.c | 21 -
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index 16887049f0cd..f60aeb1f7813 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -130,6 +130,7 @@
#define CLKRC_24MHz0xc0
#define CLKRC_DIV_MASK 0x3f
#define GET_CLKRC_DIV(x) (((x) & CLKRC_DIV_MASK) + 1)
+#define DEF_CLKRC 0x00
#define COMA_RESET BIT(7)
#define COMA_QCIF BIT(5)
@@ -783,19 +784,17 @@ static int ov6650_s_frame_interval(struct v4l2_subdev *sd,
else if (div > GET_CLKRC_DIV(CLKRC_DIV_MASK))
div = GET_CLKRC_DIV(CLKRC_DIV_MASK);
- /*
-* Keep result to be used as tpf limit
-* for subsequent clock divider calculations
-*/
- priv->tpf.numerator = div;
- priv->tpf.denominator = FRAME_RATE_MAX;
+ tpf->numerator = div;
+ tpf->denominator = FRAME_RATE_MAX;
- clkrc = to_clkrc(&priv->tpf, priv->pclk_limit, priv->pclk_max);
+ clkrc = to_clkrc(tpf, priv->pclk_limit, priv->pclk_max);
ret = ov6650_reg_rmw(client, REG_CLKRC, clkrc, CLKRC_DIV_MASK);
if (!ret) {
- tpf->numerator = GET_CLKRC_DIV(clkrc);
- tpf->denominator = FRAME_RATE_MAX;
+ priv->tpf.numerator = GET_CLKRC_DIV(clkrc);
+ priv->tpf.denominator = FRAME_RATE_MAX;
+
+ *tpf = priv->tpf;
}
return ret;
@@ -1038,6 +1037,10 @@ static int ov6650_probe(struct i2c_client *client,
priv->rect.width = W_CIF;
priv->rect.height = H_CIF;
+ /* Hardware default frame interval */
+ priv->tpf.numerator = GET_CLKRC_DIV(DEF_CLKRC);
+ priv->tpf.denominator = FRAME_RATE_MAX;
+
priv->subdev.internal_ops = &ov6650_internal_ops;
ret = v4l2_async_register_subdev(&priv->subdev);
--
2.21.0
[PATCH 5/6] media: ov6650: Drop unused .pclk_max field
This field of the driver private structure is no longer used, drop it.
Signed-off-by: Janusz Krzysztofik
---
drivers/media/i2c/ov6650.c | 4
1 file changed, 4 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index e95ea132ef06..f4723c0b5c70 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -197,7 +197,6 @@ struct ov6650 {
struct v4l2_clk *clk;
boolhalf_scale; /* scale down output by 2 */
struct v4l2_rectrect; /* sensor cropping window */
- unsigned long pclk_max; /* from resolution and format */
struct v4l2_fract tpf;/* as requested with
s_frame_interval */
u32 code;
};
@@ -618,17 +617,14 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct
v4l2_mbus_framefmt *mf)
code == MEDIA_BUS_FMT_SBGGR8_1X8) {
coml_mask = COML_ONE_CHANNEL;
coml_set = 0;
- priv->pclk_max = 400;
} else {
coml_mask = 0;
coml_set = COML_ONE_CHANNEL;
- priv->pclk_max = 800;
}
if (half_scale) {
dev_dbg(&client->dev, "max resolution: QCIF\n");
coma_set |= COMA_QCIF;
- priv->pclk_max /= 2;
} else {
dev_dbg(&client->dev, "max resolution: CIF\n");
coma_mask |= COMA_QCIF;
--
2.21.0
[PATCH 0/6] media: ov6650: Master and pixel clock handling fixes
Fix issues with handling of master and pixel clock, mostly those introduced as temporary workarounds when the driver was converted form soc_camera sensor to a standalone V4L2 subdevice driver. Janusz Krzysztofik (6): media: ov6650: Fix stored frame interval not in sync with hardware media: ov6650: Drop obsolete .pclk_limit attribute media: ov6650: Simplify clock divisor calculation media: ov6650: Don't reapply pixel clock divisor on format change media: ov6650: Drop unused .pclk_max field media: ov6650: Fix arbitrary selection of master clock rate drivers/media/i2c/ov6650.c | 129 + 1 file changed, 72 insertions(+), 57 deletions(-) -- 2.21.0
[PATCH 6/6] media: ov6650: Fix arbitrary selection of master clock rate
A hardcoded 12 MHz master clock frequency has been assumed since
conversion of the driver from soc_camera sensor to a standalone V4L2
subdevice by commit 23a52386fabe ("media: ov6650: convert to standalone
v4l2 subdevice"). Fix it.
Define a static table of supported master clock rates (fix misnamed
symbol while being at it), then use v4l2_clk_get/set_rate() to obtain
a clock rate matching one of those supported. On success, apply
respective master clock hardware divisor provided by the matching
element of the table.
Signed-off-by: Janusz Krzysztofik
---
drivers/media/i2c/ov6650.c | 64 ++
1 file changed, 58 insertions(+), 6 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index f4723c0b5c70..13fd7c4699b2 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -124,7 +124,7 @@
#define DEF_AECH 0x4D
-#define CLKRC_6MHz 0x00
+#define CLKRC_8MHz 0x00
#define CLKRC_12MHz0x40
#define CLKRC_16MHz0x80
#define CLKRC_24MHz0xc0
@@ -201,6 +201,29 @@ struct ov6650 {
u32 code;
};
+struct ov6650_xclk {
+ unsigned long rate;
+ u8 clkrc;
+};
+
+static const struct ov6650_xclk ov6650_xclk[] = {
+{
+ .rate = 800,
+ .clkrc = CLKRC_8MHz,
+},
+{
+ .rate = 1200,
+ .clkrc = CLKRC_12MHz,
+},
+{
+ .rate = 1600,
+ .clkrc = CLKRC_16MHz,
+},
+{
+ .rate = 2400,
+ .clkrc = CLKRC_24MHz,
+},
+};
static u32 ov6650_codes[] = {
MEDIA_BUS_FMT_YUYV8_2X8,
@@ -774,7 +797,7 @@ static int ov6650_reset(struct i2c_client *client)
}
/* program default register values */
-static int ov6650_prog_dflt(struct i2c_client *client)
+static int ov6650_prog_dflt(struct i2c_client *client, u8 clkrc)
{
int ret;
@@ -782,7 +805,7 @@ static int ov6650_prog_dflt(struct i2c_client *client)
ret = ov6650_reg_write(client, REG_COMA, 0);/* ~COMA_RESET */
if (!ret)
- ret = ov6650_reg_write(client, REG_CLKRC, CLKRC_12MHz);
+ ret = ov6650_reg_write(client, REG_CLKRC, clkrc);
if (!ret)
ret = ov6650_reg_rmw(client, REG_COMB, 0, COMB_BAND_FILTER);
@@ -793,8 +816,10 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
{
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct ov6650 *priv = to_ov6650(client);
- u8 pidh, pidl, midh, midl;
- int ret;
+ const struct ov6650_xclk *xclk;
+ unsigned long rate;
+ u8 pidh, pidl, midh, midl;
+ int i, ret;
priv->clk = v4l2_clk_get(&client->dev, NULL);
if (IS_ERR(priv->clk)) {
@@ -803,6 +828,33 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
return ret;
}
+ rate = v4l2_clk_get_rate(priv->clk);
+ for (i = 0; rate && i < ARRAY_SIZE(ov6650_xclk); i++) {
+ if (rate != ov6650_xclk[i].rate)
+ continue;
+
+ xclk = &ov6650_xclk[i];
+ dev_info(&client->dev, "using host default clock rate %lukHz\n",
+rate / 1000);
+ break;
+ }
+ for (i = 0; !xclk && i < ARRAY_SIZE(ov6650_xclk); i++) {
+ ret = v4l2_clk_set_rate(priv->clk, ov6650_xclk[i].rate);
+ if (ret || v4l2_clk_get_rate(priv->clk) != ov6650_xclk[i].rate)
+ continue;
+
+ xclk = &ov6650_xclk[i];
+ dev_info(&client->dev, "using negotiated clock rate %lukHz\n",
+xclk->rate / 1000);
+ break;
+ }
+ if (!xclk) {
+ dev_err(&client->dev, "unable to get supported clock rate\n");
+ if (!ret)
+ ret = -EINVAL;
+ goto eclkput;
+ }
+
ret = ov6650_s_power(sd, 1);
if (ret < 0)
goto eclkput;
@@ -836,7 +888,7 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
ret = ov6650_reset(client);
if (!ret)
- ret = ov6650_prog_dflt(client);
+ ret = ov6650_prog_dflt(client, xclk->clkrc);
if (!ret) {
struct v4l2_mbus_framefmt mf = ov6650_def_fmt;
--
2.21.0
[PATCH 3/6] media: ov6650: Simplify clock divisor calculation
As appears from an analysis of to_clkrc() helper code after its
pclk_limit argument has been dropped, its result no longer depends on
another argument - pclk_max. Moreover, assuming that a constant value
of FRAME_RATE_MAX is always used as a denominator of the only
significant argument left - a struct v4l2_fract, the result in fact
depends only on the numerator value of that argument. As a further
consequence, it no longer makes sense to recalculate frame intervals by
converting them forth and back with a GET_CLKRC_DIV(to_clkrc(tpf))
construct.
Drop use of GET_CLKRC_DIV() on results of to_clkrc() where possible -
use the frame interval value directly. Furthermore, replace the
to_clkrc() helper function with a simple macro and update its users to
always use FRAME_RATE_MAX as frame interval denominator and pass only
its numerator as an argument.
Signed-off-by: Janusz Krzysztofik
---
drivers/media/i2c/ov6650.c | 29 +
1 file changed, 5 insertions(+), 24 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index a50244401491..61ddd4ea4c26 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -545,18 +545,7 @@ static bool is_unscaled_ok(int width, int height, struct
v4l2_rect *rect)
return width > rect->width >> 1 || height > rect->height >> 1;
}
-static u8 to_clkrc(struct v4l2_fract *timeperframe, unsigned long pclk_max)
-{
- unsigned long pclk;
-
- if (timeperframe->numerator && timeperframe->denominator)
- pclk = pclk_max * timeperframe->denominator /
- (FRAME_RATE_MAX * timeperframe->numerator);
- else
- pclk = pclk_max;
-
- return (pclk_max - 1) / pclk;
-}
+#define to_clkrc(div) ((div) - 1)
/* set the format we will capture in */
static int ov6650_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf)
@@ -650,7 +639,7 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct
v4l2_mbus_framefmt *mf)
mclk = 1200;
dev_dbg(&client->dev, "using 12MHz input clock\n");
- clkrc |= to_clkrc(&priv->tpf, priv->pclk_max);
+ clkrc |= to_clkrc(priv->tpf.numerator);
pclk = priv->pclk_max / GET_CLKRC_DIV(clkrc);
dev_dbg(&client->dev, "pixel clock divider: %ld.%ld\n",
@@ -749,9 +738,7 @@ static int ov6650_g_frame_interval(struct v4l2_subdev *sd,
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct ov6650 *priv = to_ov6650(client);
- ival->interval.numerator = GET_CLKRC_DIV(to_clkrc(&priv->tpf,
- priv->pclk_max));
- ival->interval.denominator = FRAME_RATE_MAX;
+ ival->interval = priv->tpf;
dev_dbg(&client->dev, "Frame interval: %u/%u s\n",
ival->interval.numerator, ival->interval.denominator);
@@ -766,7 +753,6 @@ static int ov6650_s_frame_interval(struct v4l2_subdev *sd,
struct ov6650 *priv = to_ov6650(client);
struct v4l2_fract *tpf = &ival->interval;
int div, ret;
- u8 clkrc;
if (tpf->numerator == 0 || tpf->denominator == 0)
div = 1; /* Reset to full rate */
@@ -778,14 +764,9 @@ static int ov6650_s_frame_interval(struct v4l2_subdev *sd,
else if (div > GET_CLKRC_DIV(CLKRC_DIV_MASK))
div = GET_CLKRC_DIV(CLKRC_DIV_MASK);
- tpf->numerator = div;
- tpf->denominator = FRAME_RATE_MAX;
-
- clkrc = to_clkrc(tpf, priv->pclk_max);
-
- ret = ov6650_reg_rmw(client, REG_CLKRC, clkrc, CLKRC_DIV_MASK);
+ ret = ov6650_reg_rmw(client, REG_CLKRC, to_clkrc(div), CLKRC_DIV_MASK);
if (!ret) {
- priv->tpf.numerator = GET_CLKRC_DIV(clkrc);
+ priv->tpf.numerator = div;
priv->tpf.denominator = FRAME_RATE_MAX;
*tpf = priv->tpf;
--
2.21.0
[PATCH v3 8/9] media: ov6650: Fix stored frame format not in sync with hardware
The driver stores frame format settings supposed to be in line with
hardware state in a device private structure. Since the driver initial
submission, those settings are updated before they are actually applied
on hardware. If an error occurs on device update, the stored settings
my not reflect hardware state anymore and consecutive calls to
.get_fmt() may return incorrect information. That in turn may affect
ability of a bridge device to use correct DMA transfer settings if such
incorrect informmation on active frame format returned by .get_fmt() is
used.
Assuming a failed device update means its state hasn't changed, update
frame format related settings stored in the device private structure
only after they are successfully applied so the stored values always
reflect hardware state as closely as possible.
Fixes: 2f6e2404799a ("[media] SoC Camera: add driver for OV6650 sensor")
Signed-off-by: Janusz Krzysztofik
---
drivers/media/i2c/ov6650.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index 4fd8ac8e3994..126a662be301 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -627,7 +627,6 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct
v4l2_mbus_framefmt *mf)
dev_err(&client->dev, "Pixel format not handled: 0x%x\n", code);
return -EINVAL;
}
- priv->code = code;
if (code == MEDIA_BUS_FMT_Y8_1X8 ||
code == MEDIA_BUS_FMT_SBGGR8_1X8) {
@@ -648,7 +647,6 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct
v4l2_mbus_framefmt *mf)
dev_dbg(&client->dev, "max resolution: CIF\n");
coma_mask |= COMA_QCIF;
}
- priv->half_scale = half_scale;
clkrc = CLKRC_12MHz;
mclk = 1200;
@@ -666,8 +664,13 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct
v4l2_mbus_framefmt *mf)
ret = ov6650_reg_rmw(client, REG_COMA, coma_set, coma_mask);
if (!ret)
ret = ov6650_reg_write(client, REG_CLKRC, clkrc);
- if (!ret)
+ if (!ret) {
+ priv->half_scale = half_scale;
+
ret = ov6650_reg_rmw(client, REG_COML, coml_set, coml_mask);
+ }
+ if (!ret)
+ priv->code = code;
return ret;
}
--
2.21.0
[PATCH v3 3/9] media: ov6650: Fix crop rectangle alignment not passed back
Commit 4f996594ceaf ("[media] v4l2: make vidioc_s_crop const")
introduced a writable copy of constified user requested crop rectangle
in order to be able to perform hardware alignments on it. Later
on, commit 10d5509c8d50 ("[media] v4l2: remove g/s_crop from video
ops") replaced s_crop() video operation using that const argument with
set_selection() pad operation which had a corresponding argument not
constified, however the original behavior of the driver was not
restored. Since that time, any hardware alignment applied on a user
requested crop rectangle is not passed back to the user calling
.set_selection() as it should be.
Fix the issue by dropping the copy and replacing all references to it
with references to the crop rectangle embedded in the user argument.
Fixes: 10d5509c8d50 ("[media] v4l2: remove g/s_crop from video ops")
Signed-off-by: Janusz Krzysztofik
---
drivers/media/i2c/ov6650.c | 31 +++
1 file changed, 15 insertions(+), 16 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index cb2aa76cd6cc..983b72f33930 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -465,38 +465,37 @@ static int ov6650_set_selection(struct v4l2_subdev *sd,
{
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct ov6650 *priv = to_ov6650(client);
- struct v4l2_rect rect = sel->r;
int ret;
if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE ||
sel->target != V4L2_SEL_TGT_CROP)
return -EINVAL;
- v4l_bound_align_image(&rect.width, 2, W_CIF, 1,
- &rect.height, 2, H_CIF, 1, 0);
- v4l_bound_align_image(&rect.left, DEF_HSTRT << 1,
- (DEF_HSTRT << 1) + W_CIF - (__s32)rect.width, 1,
- &rect.top, DEF_VSTRT << 1,
- (DEF_VSTRT << 1) + H_CIF - (__s32)rect.height, 1,
- 0);
+ v4l_bound_align_image(&sel->r.width, 2, W_CIF, 1,
+ &sel->r.height, 2, H_CIF, 1, 0);
+ v4l_bound_align_image(&sel->r.left, DEF_HSTRT << 1,
+ (DEF_HSTRT << 1) + W_CIF - (__s32)sel->r.width, 1,
+ &sel->r.top, DEF_VSTRT << 1,
+ (DEF_VSTRT << 1) + H_CIF - (__s32)sel->r.height,
+ 1, 0);
- ret = ov6650_reg_write(client, REG_HSTRT, rect.left >> 1);
+ ret = ov6650_reg_write(client, REG_HSTRT, sel->r.left >> 1);
if (!ret) {
- priv->rect.left = rect.left;
+ priv->rect.left = sel->r.left;
ret = ov6650_reg_write(client, REG_HSTOP,
- (rect.left + rect.width) >> 1);
+ (sel->r.left + sel->r.width) >> 1);
}
if (!ret) {
- priv->rect.width = rect.width;
- ret = ov6650_reg_write(client, REG_VSTRT, rect.top >> 1);
+ priv->rect.width = sel->r.width;
+ ret = ov6650_reg_write(client, REG_VSTRT, sel->r.top >> 1);
}
if (!ret) {
- priv->rect.top = rect.top;
+ priv->rect.top = sel->r.top;
ret = ov6650_reg_write(client, REG_VSTOP,
- (rect.top + rect.height) >> 1);
+ (sel->r.top + sel->r.height) >> 1);
}
if (!ret)
- priv->rect.height = rect.height;
+ priv->rect.height = sel->r.height;
return ret;
}
--
2.21.0
[PATCH v3 2/9] media: ov6650: Fix control handler not freed on init error
Since commit afd9690c72c3 ("[media] ov6650: convert to the control
framework"), if an error occurs during initialization of a control
handler, resources possibly allocated to the handler are not freed
before device initialiaton is aborted. Fix it.
Fixes: afd9690c72c3 ("[media] ov6650: convert to the control framework")
Signed-off-by: Janusz Krzysztofik
---
drivers/media/i2c/ov6650.c | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index 53550cae2353..cb2aa76cd6cc 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -989,8 +989,10 @@ static int ov6650_probe(struct i2c_client *client,
V4L2_CID_GAMMA, 0, 0xff, 1, 0x12);
priv->subdev.ctrl_handler = &priv->hdl;
- if (priv->hdl.error)
- return priv->hdl.error;
+ if (priv->hdl.error) {
+ ret = priv->hdl.error;
+ goto ectlhdlfree;
+ }
v4l2_ctrl_auto_cluster(2, &priv->autogain, 0, true);
v4l2_ctrl_auto_cluster(3, &priv->autowb, 0, true);
@@ -1008,8 +1010,10 @@ static int ov6650_probe(struct i2c_client *client,
priv->subdev.internal_ops = &ov6650_internal_ops;
ret = v4l2_async_register_subdev(&priv->subdev);
- if (ret)
- v4l2_ctrl_handler_free(&priv->hdl);
+ if (!ret)
+ return 0;
+ectlhdlfree:
+ v4l2_ctrl_handler_free(&priv->hdl);
return ret;
}
--
2.21.0
[PATCH v3 6/9] media: ov6650: Fix .get_fmt() V4L2_SUBDEV_FORMAT_TRY support
Commit da298c6d98d5 ("[media] v4l2: replace video op g_mbus_fmt by pad
op get_fmt") converted a former ov6650_g_fmt() video operation callback
to an ov6650_get_fmt() pad operation callback. However, the converted
function disregards a format->which flag that pad operations should
obey and always returns active frame format settings.
That can be fixed by always responding to V4L2_SUBDEV_FORMAT_TRY with
-EINVAL, or providing the response from a pad config argument, likely
updated by a former user call to V4L2_SUBDEV_FORMAT_TRY .set_fmt().
Since implementation of the latter is trivial, go for it.
Fixes: da298c6d98d5 ("[media] v4l2: replace video op g_mbus_fmt by pad op
get_fmt")
Signed-off-by: Janusz Krzysztofik
---
drivers/media/i2c/ov6650.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index fd608aebb68d..b62a238a83d0 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -525,10 +525,16 @@ static int ov6650_get_fmt(struct v4l2_subdev *sd,
*mf = ov6650_def_fmt;
/* update media bus format code and frame size */
- mf->width = priv->rect.width >> priv->half_scale;
- mf->height = priv->rect.height >> priv->half_scale;
- mf->code= priv->code;
+ if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
+ mf->width = cfg->try_fmt.width;
+ mf->height = cfg->try_fmt.height;
+ mf->code = cfg->try_fmt.code;
+ } else {
+ mf->width = priv->rect.width >> priv->half_scale;
+ mf->height = priv->rect.height >> priv->half_scale;
+ mf->code = priv->code;
+ }
return 0;
}
--
2.21.0
[PATCH v3 4/9] media: ov6650: Fix incorrect use of JPEG colorspace
Since its initial submission, the driver selects V4L2_COLORSPACE_JPEG
for supported formats other than V4L2_MBUS_FMT_SBGGR8_1X8. According
to v4l2-compliance test program, V4L2_COLORSPACE_JPEG applies
exclusively to V4L2_PIX_FMT_JPEG. Since the sensor does not support
JPEG format, fix it to always select V4L2_COLORSPACE_SRGB.
Fixes: 2f6e2404799a ("[media] SoC Camera: add driver for OV6650 sensor")
Signed-off-by: Janusz Krzysztofik
---
drivers/media/i2c/ov6650.c | 13 ++---
1 file changed, 2 insertions(+), 11 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index 983b72f33930..a7930ace3744 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -200,7 +200,6 @@ struct ov6650 {
unsigned long pclk_max; /* from resolution and format */
struct v4l2_fract tpf;/* as requested with
s_frame_interval */
u32 code;
- enum v4l2_colorspacecolorspace;
};
@@ -514,7 +513,7 @@ static int ov6650_get_fmt(struct v4l2_subdev *sd,
mf->width = priv->rect.width >> priv->half_scale;
mf->height = priv->rect.height >> priv->half_scale;
mf->code= priv->code;
- mf->colorspace = priv->colorspace;
+ mf->colorspace = V4L2_COLORSPACE_SRGB;
mf->field = V4L2_FIELD_NONE;
return 0;
@@ -622,11 +621,6 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct
v4l2_mbus_framefmt *mf)
priv->pclk_max = 800;
}
- if (code == MEDIA_BUS_FMT_SBGGR8_1X8)
- priv->colorspace = V4L2_COLORSPACE_SRGB;
- else if (code != 0)
- priv->colorspace = V4L2_COLORSPACE_JPEG;
-
if (half_scale) {
dev_dbg(&client->dev, "max resolution: QCIF\n");
coma_set |= COMA_QCIF;
@@ -657,7 +651,6 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct
v4l2_mbus_framefmt *mf)
ret = ov6650_reg_rmw(client, REG_COML, coml_set, coml_mask);
if (!ret) {
- mf->colorspace = priv->colorspace;
mf->width = priv->rect.width >> half_scale;
mf->height = priv->rect.height >> half_scale;
}
@@ -680,6 +673,7 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
&mf->height, 2, H_CIF, 1, 0);
mf->field = V4L2_FIELD_NONE;
+ mf->colorspace = V4L2_COLORSPACE_SRGB;
switch (mf->code) {
case MEDIA_BUS_FMT_Y10_1X10:
@@ -690,13 +684,11 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
case MEDIA_BUS_FMT_YUYV8_2X8:
case MEDIA_BUS_FMT_VYUY8_2X8:
case MEDIA_BUS_FMT_UYVY8_2X8:
- mf->colorspace = V4L2_COLORSPACE_JPEG;
break;
default:
mf->code = MEDIA_BUS_FMT_SBGGR8_1X8;
/* fall through */
case MEDIA_BUS_FMT_SBGGR8_1X8:
- mf->colorspace = V4L2_COLORSPACE_SRGB;
break;
}
@@ -1004,7 +996,6 @@ static int ov6650_probe(struct i2c_client *client,
priv->rect.height = H_CIF;
priv->half_scale = false;
priv->code= MEDIA_BUS_FMT_YUYV8_2X8;
- priv->colorspace = V4L2_COLORSPACE_JPEG;
priv->subdev.internal_ops = &ov6650_internal_ops;
--
2.21.0
[PATCH v3 5/9] media: ov6650: Fix some format attributes not under control
User arguments passed to .get/set_fmt() pad operation callbacks may
contain unsupported values. The driver takes control over frame size
and pixel code as well as colorspace and field attributes but has never
cared for remainig format attributes, i.e., ycbcr_enc, quantization
and xfer_func, introduced by commit 11ff030c7365 ("[media]
v4l2-mediabus: improve colorspace support"). Fix it.
Set up a static v4l2_mbus_framefmt structure with attributes
initialized to reasonable defaults and use it for updating content of
user provided arguments. In case of V4L2_SUBDEV_FORMAT_ACTIVE,
postpone frame size update, now performed from inside ov6650_s_fmt()
helper, util the user argument is first updated in ov6650_set_fmt() with
default frame format content. For V4L2_SUBDEV_FORMAT_TRY, don't copy
all attributes to pad config, only those handled by the driver, then
fill the response with the default frame format updated with resulting
pad config format code and frame size.
Fixes: 11ff030c7365 ("[media] v4l2-mediabus: improve colorspace support")
Signed-off-by: Janusz Krzysztofik
---
drivers/media/i2c/ov6650.c | 51 +-
1 file changed, 39 insertions(+), 12 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index a7930ace3744..fd608aebb68d 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -212,6 +212,17 @@ static u32 ov6650_codes[] = {
MEDIA_BUS_FMT_Y8_1X8,
};
+static const struct v4l2_mbus_framefmt ov6650_def_fmt = {
+ .width = W_CIF,
+ .height = H_CIF,
+ .code = MEDIA_BUS_FMT_SBGGR8_1X8,
+ .colorspace = V4L2_COLORSPACE_SRGB,
+ .field = V4L2_FIELD_NONE,
+ .ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT,
+ .quantization = V4L2_QUANTIZATION_DEFAULT,
+ .xfer_func = V4L2_XFER_FUNC_DEFAULT,
+};
+
/* read a register */
static int ov6650_reg_read(struct i2c_client *client, u8 reg, u8 *val)
{
@@ -510,11 +521,13 @@ static int ov6650_get_fmt(struct v4l2_subdev *sd,
if (format->pad)
return -EINVAL;
+ /* initialize response with default media bus frame format */
+ *mf = ov6650_def_fmt;
+
+ /* update media bus format code and frame size */
mf->width = priv->rect.width >> priv->half_scale;
mf->height = priv->rect.height >> priv->half_scale;
mf->code= priv->code;
- mf->colorspace = V4L2_COLORSPACE_SRGB;
- mf->field = V4L2_FIELD_NONE;
return 0;
}
@@ -650,10 +663,6 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct
v4l2_mbus_framefmt *mf)
if (!ret)
ret = ov6650_reg_rmw(client, REG_COML, coml_set, coml_mask);
- if (!ret) {
- mf->width = priv->rect.width >> half_scale;
- mf->height = priv->rect.height >> half_scale;
- }
return ret;
}
@@ -672,9 +681,6 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
v4l_bound_align_image(&mf->width, 2, W_CIF, 1,
&mf->height, 2, H_CIF, 1, 0);
- mf->field = V4L2_FIELD_NONE;
- mf->colorspace = V4L2_COLORSPACE_SRGB;
-
switch (mf->code) {
case MEDIA_BUS_FMT_Y10_1X10:
mf->code = MEDIA_BUS_FMT_Y8_1X8;
@@ -692,10 +698,31 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
break;
}
- if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
- return ov6650_s_fmt(sd, mf);
- cfg->try_fmt = *mf;
+ if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
+ /* store media bus format code and frame size in pad config */
+ cfg->try_fmt.width = mf->width;
+ cfg->try_fmt.height = mf->height;
+ cfg->try_fmt.code = mf->code;
+ /* return default mbus frame format updated with pad config */
+ *mf = ov6650_def_fmt;
+ mf->width = cfg->try_fmt.width;
+ mf->height = cfg->try_fmt.height;
+ mf->code = cfg->try_fmt.code;
+
+ } else {
+ /* apply new media bus format code and frame size */
+ int ret = ov6650_s_fmt(sd, mf);
+
+ if (ret)
+ return ret;
+
+ /* return default format updated with active size and code */
+ *mf = ov6650_def_fmt;
+ mf->width = priv->rect.width >> priv->half_scale;
+ mf->height = priv->rect.height >> priv->half_scale;
+ mf->code = priv->code;
+ }
return 0;
}
--
2.21.0
[PATCH v3 7/9] media: ov6650: Fix default format not applied on device probe
It is not clear what pixel format is actually configured in hardware on
reset. MEDIA_BUS_FMT_YUYV8_2X8, assumed on device probe since the
driver was intiially submitted, is for sure not the one.
Fix it by explicitly applying a known, driver default frame format just
after initial device reset.
Fixes: 2f6e2404799a ("[media] SoC Camera: add driver for OV6650 sensor")
Signed-off-by: Janusz Krzysztofik
---
drivers/media/i2c/ov6650.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index b62a238a83d0..4fd8ac8e3994 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -873,6 +873,11 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
ret = ov6650_reset(client);
if (!ret)
ret = ov6650_prog_dflt(client);
+ if (!ret) {
+ struct v4l2_mbus_framefmt mf = ov6650_def_fmt;
+
+ ret = ov6650_s_fmt(sd, &mf);
+ }
if (!ret)
ret = v4l2_ctrl_handler_setup(&priv->hdl);
@@ -1027,8 +1032,6 @@ static int ov6650_probe(struct i2c_client *client,
priv->rect.top= DEF_VSTRT << 1;
priv->rect.width = W_CIF;
priv->rect.height = H_CIF;
- priv->half_scale = false;
- priv->code= MEDIA_BUS_FMT_YUYV8_2X8;
priv->subdev.internal_ops = &ov6650_internal_ops;
--
2.21.0
[PATCH v3 0/9] media: ov6650: A collection of fixes
Janusz Krzysztofik (9): media: ov6650: Fix MODULE_DESCRIPTION media: ov6650: Fix control handler not freed on init error media: ov6650: Fix crop rectangle alignment not passed back media: ov6650: Fix incorrect use of JPEG colorspace media: ov6650: Fix some format attributes not under control media: ov6650: Fix .get_fmt() V4L2_SUBDEV_FORMAT_TRY support media: ov6650: Fix default format not applied on device probe media: ov6650: Fix stored frame format not in sync with hardware media: ov6650: Fix stored crop rectangle not in sync with hardware drivers/media/i2c/ov6650.c | 137 +++-- 1 file changed, 86 insertions(+), 51 deletions(-) v2: dropped patches 3/14 through 7/14 which were adding parameter checks now called from v4l2_subdev_call() - inspired by Sakari's request, thanks! v3: * drop Cc: sta...@vger.kernel.org on Greg rquest (non-critical fixes) * fix typos and some wording in commit messages * rebase on media/v5.3-3 -- 2.21.0
[PATCH v3 1/9] media: ov6650: Fix MODULE_DESCRIPTION
Commit 23a52386fabe ("media: ov6650: convert to standalone v4l2
subdevice") converted the driver from a soc_camera sensor to a
standalone V4L subdevice driver. Unfortunately, module description was
not updated to reflect the change. Fix it.
While being at it, update email address of the module author.
Fixes: 23a52386fabe ("media: ov6650: convert to standalone v4l2 subdevice")
Signed-off-by: Janusz Krzysztofik
---
drivers/media/i2c/ov6650.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index 5b9af5e5b7f1..53550cae2353 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -1041,6 +1041,6 @@ static struct i2c_driver ov6650_i2c_driver = {
module_i2c_driver(ov6650_i2c_driver);
-MODULE_DESCRIPTION("SoC Camera driver for OmniVision OV6650");
-MODULE_AUTHOR("Janusz Krzysztofik ");
+MODULE_DESCRIPTION("V4L2 subdevice driver for OmniVision OV6650 camera
sensor");
+MODULE_AUTHOR("Janusz Krzysztofik
[PATCH v3 9/9] media: ov6650: Fix stored crop rectangle not in sync with hardware
The driver stores crop rectangle settings supposed to be in line with
hardware state in a device private structure. Since the driver initial
submission, crop rectangle width and height settings are not updated
correctly when rectangle offset settings are applied on hardware. If
an error occurs while the device is updated, the stored settings my no
longer reflect hardware state and consecutive calls to .get_selection()
as well as .get/set_fmt() may return incorrect information. That in
turn may affect ability of a bridge device to use correct DMA transfer
settings if such incorrect informamtion on active frame format returned
by .get/set_fmt() is used.
Assuming a failed update of the device means its actual settings haven't
changed, update crop rectangle width and height settings stored in the
device private structure correctly while the rectangle offset is
successfully applied on hardware so the stored values always reflect
actual hardware state to the extent possible.
Fixes: 2f6e2404799a ("[media] SoC Camera: add driver for OV6650 sensor")
Signed-off-by: Janusz Krzysztofik
---
drivers/media/i2c/ov6650.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index 126a662be301..16887049f0cd 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -491,6 +491,7 @@ static int ov6650_set_selection(struct v4l2_subdev *sd,
ret = ov6650_reg_write(client, REG_HSTRT, sel->r.left >> 1);
if (!ret) {
+ priv->rect.width += priv->rect.left - sel->r.left;
priv->rect.left = sel->r.left;
ret = ov6650_reg_write(client, REG_HSTOP,
(sel->r.left + sel->r.width) >> 1);
@@ -500,6 +501,7 @@ static int ov6650_set_selection(struct v4l2_subdev *sd,
ret = ov6650_reg_write(client, REG_VSTRT, sel->r.top >> 1);
}
if (!ret) {
+ priv->rect.height += priv->rect.top - sel->r.top;
priv->rect.top = sel->r.top;
ret = ov6650_reg_write(client, REG_VSTOP,
(sel->r.top + sel->r.height) >> 1);
--
2.21.0
Re: [RFC PATCH] iommu/vt-d: Fix IOMMU field not populated on device hot re-plug
Hi Baolu, On Tuesday, September 3, 2019 3:29:40 AM CEST Lu Baolu wrote: > Hi Janusz, > > On 9/2/19 4:37 PM, Janusz Krzysztofik wrote: > >> I am not saying that keeping data is not acceptable. I just want to > >> check whether there are any other solutions. > > Then reverting 458b7c8e0dde and applying this patch still resolves the issue > > for me. No errors appear when mappings are unmapped on device close after the > > device has been removed, and domain info preserved on device removal is > > successfully reused on device re-plug. > > This patch doesn't look good to me although I agree that keeping data is > acceptable. It updates dev->archdata.iommu, but leaves the hardware > context/pasid table unchanged. This might cause problems somewhere. > > > > > Is there anything else I can do to help? > > Can you please tell me how to reproduce the problem? The most simple way to reproduce the issue, assuming there are no non-Intel graphics adapters installed, is to run the following shell commands: #!/bin/sh # load i915 module modprobe i915 # open an i915 device and keep it open in background cat /dev/dri/card0 >/dev/null & sleep 2 # simulate device unplug echo 1 >/sys/class/drm/card0/device/remove # make the background process close the device on exit kill $! Thanks, Janusz > Keeping the per > device domain info while device is unplugged is a bit dangerous because > info->dev might be a wild pointer. We need to work out a clean fix. > > > > > Thanks, > > Janusz > > > > Best regards, > Baolu >
[RFC PATCH] iommu/vt-d: Fix IOMMU field not populated on device hot re-plug
When a perfectly working i915 device is hot unplugged (via sysfs) and
hot re-plugged again, its dev->archdata.iommu field is not populated
again with an IOMMU pointer. As a result, the device probe fails on
DMA mapping error during scratch page setup.
It looks like that happens because devices are not detached from their
MMUIO bus before they are removed on device unplug. Then, when an
already registered device/IOMMU association is identified by the
reinstantiated device's bus and function IDs on IOMMU bus re-attach
attempt, the device's archdata is not populated with IOMMU information
and the bad happens.
I'm not sure if this is a proper fix but it works for me so at least it
confirms correctness of my analysis results, I believe. So far I
haven't been able to identify a good place where the possibly missing
IOMMU bus detach on device unplug operation could be added.
Signed-off-by: Janusz Krzysztofik
---
drivers/iommu/intel-iommu.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 12d094d08c0a..7cdcd0595408 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2477,6 +2477,9 @@ static struct dmar_domain
*dmar_insert_one_dev_info(struct intel_iommu *iommu,
if (info2) {
found = info2->domain;
info2->dev = dev;
+
+ if (dev && !dev->archdata.iommu)
+ dev->archdata.iommu = info2;
}
}
--
2.21.0
[PATCH for v5.3] ARM: OMAP1: ams-delta-fiq: Fix missing irq_ack
Non-serio path of Amstrad Delta FIQ deferred handler depended on
irq_ack() method provided by OMAP GPIO driver. That method has been
removed by commit 693de831c6e5 ("gpio: omap: remove irq_ack method").
Remove useless code from the deferred handler and reimplement the
missing operation inside the base FIQ handler.
Should another dependency - irq_unmask() - be ever removed from the OMAP
GPIO driver, WARN once if missing.
Signed-off-by: Janusz Krzysztofik
---
arch/arm/mach-omap1/ams-delta-fiq-handler.S | 3 ++-
arch/arm/mach-omap1/ams-delta-fiq.c | 4 +---
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/arch/arm/mach-omap1/ams-delta-fiq-handler.S
b/arch/arm/mach-omap1/ams-delta-fiq-handler.S
index 81159af44862..14a6c3eb3298 100644
--- a/arch/arm/mach-omap1/ams-delta-fiq-handler.S
+++ b/arch/arm/mach-omap1/ams-delta-fiq-handler.S
@@ -126,6 +126,8 @@ restart:
orr r11, r11, r13 @ mask all requested interrupts
str r11, [r12, #OMAP1510_GPIO_INT_MASK]
+ str r13, [r12, #OMAP1510_GPIO_INT_STATUS] @ ack all requested interrupts
+
ands r10, r13, #KEYBRD_CLK_MASK @ extract keyboard status - set?
beq hksw@ no - try next source
@@ -133,7 +135,6 @@ restart:
@@
@ Keyboard clock FIQ mode interrupt handler
@ r10 now contains KEYBRD_CLK_MASK, use it
- str r10, [r12, #OMAP1510_GPIO_INT_STATUS] @ ack the interrupt
bic r11, r11, r10 @ unmask it
str r11, [r12, #OMAP1510_GPIO_INT_MASK]
diff --git a/arch/arm/mach-omap1/ams-delta-fiq.c
b/arch/arm/mach-omap1/ams-delta-fiq.c
index 43899fa56674..0254eb9cf8c6 100644
--- a/arch/arm/mach-omap1/ams-delta-fiq.c
+++ b/arch/arm/mach-omap1/ams-delta-fiq.c
@@ -70,9 +70,7 @@ static irqreturn_t deferred_fiq(int irq, void *dev_id)
* interrupts default to since commit 80ac93c27441
* requires interrupt already acked and unmasked.
*/
- if (irq_chip->irq_ack)
- irq_chip->irq_ack(d);
- if (irq_chip->irq_unmask)
+ if (!WARN_ON_ONCE(!irq_chip->irq_unmask))
irq_chip->irq_unmask(d);
}
for (; irq_counter[gpio] < fiq_count; irq_counter[gpio]++)
--
2.21.0
[RFC PATCH 3/6] drm/i915: Propagate "_release" function name suffix down
Replace mixed "_fini"/"_cleanup"/"_cleanup_hw" suffixes found in names
of fucntions called from i915_driver_release() with "_release" suffix
consistently. This provides better code readability, especially
helpful when trying to work out which phase the code is in.
Functions names starting with "i915_driver_", i.e., those defined in
drivers/gpu/dri/i915/i915_drv.c, just have their "cleanup" or "fini"
parts of their names replaced with the "_release" suffix, while names
of functions coming from other source files have been suffixed with
"_driver_release" to avoid ambiguity with other possible .release entry
points.
Suggested-by: Chris Wilson
Signed-off-by: Janusz Krzysztofik
---
drivers/gpu/drm/i915/i915_drv.c | 33 +
drivers/gpu/drm/i915/i915_drv.h | 2 +-
drivers/gpu/drm/i915/i915_gem.c | 2 +-
drivers/gpu/drm/i915/i915_gem_gtt.c | 4 +--
drivers/gpu/drm/i915/i915_gem_gtt.h | 2 +-
drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +-
drivers/gpu/drm/i915/intel_runtime_pm.h | 2 +-
7 files changed, 24 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index ad24957ad86d..36c872220f68 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -752,7 +752,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
cleanup_gem:
i915_gem_suspend(dev_priv);
i915_gem_fini_hw(dev_priv);
- i915_gem_fini(dev_priv);
+ i915_gem_driver_release(dev_priv);
cleanup_modeset:
intel_modeset_cleanup(dev);
cleanup_irq:
@@ -962,10 +962,11 @@ static int i915_driver_init_early(struct drm_i915_private
*dev_priv)
}
/**
- * i915_driver_cleanup_early - cleanup the setup done in
i915_driver_init_early()
+ * i915_driver_early_release - cleanup the setup done in
+ *i915_driver_init_early()
* @dev_priv: device private
*/
-static void i915_driver_cleanup_early(struct drm_i915_private *dev_priv)
+static void i915_driver_early_release(struct drm_i915_private *dev_priv)
{
intel_irq_fini(dev_priv);
intel_power_domains_cleanup(dev_priv);
@@ -1028,10 +1029,10 @@ static int i915_driver_init_mmio(struct
drm_i915_private *dev_priv)
}
/**
- * i915_driver_cleanup_mmio - cleanup the setup done in i915_driver_init_mmio()
+ * i915_driver_mmio_release - cleanup the setup done in i915_driver_init_mmio()
* @dev_priv: device private
*/
-static void i915_driver_cleanup_mmio(struct drm_i915_private *dev_priv)
+static void i915_driver_mmio_release(struct drm_i915_private *dev_priv)
{
intel_teardown_mchbar(dev_priv);
intel_uncore_fini_mmio(&dev_priv->uncore);
@@ -1684,7 +1685,7 @@ static int i915_driver_init_hw(struct drm_i915_private
*dev_priv)
pci_disable_msi(pdev);
pm_qos_remove_request(&dev_priv->pm_qos);
err_ggtt:
- i915_ggtt_cleanup_hw(dev_priv);
+ i915_ggtt_driver_release(dev_priv);
err_perf:
i915_perf_fini(dev_priv);
return ret;
@@ -1929,15 +1930,15 @@ int i915_driver_probe(struct pci_dev *pdev, const
struct pci_device_id *ent)
out_cleanup_hw:
i915_driver_cleanup_hw(dev_priv);
- i915_ggtt_cleanup_hw(dev_priv);
+ i915_ggtt_driver_release(dev_priv);
/* Paranoia: make sure we have disabled everything before we exit. */
intel_sanitize_gt_powersave(dev_priv);
out_cleanup_mmio:
- i915_driver_cleanup_mmio(dev_priv);
+ i915_driver_mmio_release(dev_priv);
out_runtime_pm_put:
enable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
- i915_driver_cleanup_early(dev_priv);
+ i915_driver_early_release(dev_priv);
out_pci_disable:
pci_disable_device(pdev);
out_fini:
@@ -2000,19 +2001,19 @@ static void i915_driver_release(struct drm_device *dev)
disable_rpm_wakeref_asserts(rpm);
- i915_gem_fini(dev_priv);
+ i915_gem_driver_release(dev_priv);
- i915_ggtt_cleanup_hw(dev_priv);
+ i915_ggtt_driver_release(dev_priv);
/* Paranoia: make sure we have disabled everything before we exit. */
intel_sanitize_gt_powersave(dev_priv);
- i915_driver_cleanup_mmio(dev_priv);
+ i915_driver_mmio_release(dev_priv);
enable_rpm_wakeref_asserts(rpm);
- intel_runtime_pm_cleanup(rpm);
+ intel_runtime_pm_driver_release(rpm);
- i915_driver_cleanup_early(dev_priv);
+ i915_driver_early_release(dev_priv);
i915_driver_destroy(dev_priv);
}
@@ -2205,7 +2206,7 @@ static int i915_drm_suspend_late(struct drm_device *dev,
bool hibernation)
out:
enable_rpm_wakeref_asserts(rpm);
if (!dev_priv->uncore.user_forcewake.count)
- intel_runtime_pm_cleanup(rpm);
+ intel_runtime_pm_driver_release(rpm);
return ret;
}
@@ -2969,7 +
[RFC PATCH 5/6] drm/i915: Propagate "_probe" function name suffix down
Similar to the "_release" and "_remove" cases, consequently replace
"_init" components of names of functions called from
i915_driver_probe() with "_probe" suffixes for better code readability.
Signed-off-by: Janusz Krzysztofik
---
drivers/gpu/drm/i915/i915_drv.c | 26 +-
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 6e83fe96d930..7241a7d14e9b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -675,7 +675,7 @@ static const struct vga_switcheroo_client_ops
i915_switcheroo_ops = {
.can_switch = i915_switcheroo_can_switch,
};
-static int i915_load_modeset_init(struct drm_device *dev)
+static int i915_driver_modeset_probe(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = to_i915(dev);
struct pci_dev *pdev = dev_priv->drm.pdev;
@@ -884,7 +884,7 @@ static void intel_detect_preproduction_hw(struct
drm_i915_private *dev_priv)
}
/**
- * i915_driver_init_early - setup state not requiring device access
+ * i915_driver_early_probe - setup state not requiring device access
* @dev_priv: device private
*
* Initialize everything that is a "SW-only" state, that is state not
@@ -893,7 +893,7 @@ static void intel_detect_preproduction_hw(struct
drm_i915_private *dev_priv)
* system memory allocation, setting up device specific attributes and
* function hooks not requiring accessing the device.
*/
-static int i915_driver_init_early(struct drm_i915_private *dev_priv)
+static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
{
int ret = 0;
@@ -963,7 +963,7 @@ static int i915_driver_init_early(struct drm_i915_private
*dev_priv)
/**
* i915_driver_early_release - cleanup the setup done in
- *i915_driver_init_early()
+ *i915_driver_early_probe()
* @dev_priv: device private
*/
static void i915_driver_early_release(struct drm_i915_private *dev_priv)
@@ -980,7 +980,7 @@ static void i915_driver_early_release(struct
drm_i915_private *dev_priv)
}
/**
- * i915_driver_init_mmio - setup device MMIO
+ * i915_driver_mmio_probe - setup device MMIO
* @dev_priv: device private
*
* Setup minimal device state necessary for MMIO accesses later in the
@@ -988,7 +988,7 @@ static void i915_driver_early_release(struct
drm_i915_private *dev_priv)
* side effects or exposing the driver via kernel internal or user space
* interfaces.
*/
-static int i915_driver_init_mmio(struct drm_i915_private *dev_priv)
+static int i915_driver_mmio_probe(struct drm_i915_private *dev_priv)
{
int ret;
@@ -1029,7 +1029,7 @@ static int i915_driver_init_mmio(struct drm_i915_private
*dev_priv)
}
/**
- * i915_driver_mmio_release - cleanup the setup done in i915_driver_init_mmio()
+ * i915_driver_mmio_release - cleanup the setup done in
i915_driver_mmio_probe()
* @dev_priv: device private
*/
static void i915_driver_mmio_release(struct drm_i915_private *dev_priv)
@@ -1525,13 +1525,13 @@ static void edram_detect(struct drm_i915_private
*dev_priv)
}
/**
- * i915_driver_init_hw - setup state requiring device access
+ * i915_driver_hw_probe - setup state requiring device access
* @dev_priv: device private
*
* Setup state that requires accessing the device, but doesn't require
* exposing the driver via kernel internal or userspace interfaces.
*/
-static int i915_driver_init_hw(struct drm_i915_private *dev_priv)
+static int i915_driver_hw_probe(struct drm_i915_private *dev_priv)
{
struct pci_dev *pdev = dev_priv->drm.pdev;
int ret;
@@ -1900,7 +1900,7 @@ int i915_driver_probe(struct pci_dev *pdev, const struct
pci_device_id *ent)
if (ret)
goto out_fini;
- ret = i915_driver_init_early(dev_priv);
+ ret = i915_driver_early_probe(dev_priv);
if (ret < 0)
goto out_pci_disable;
@@ -1908,15 +1908,15 @@ int i915_driver_probe(struct pci_dev *pdev, const
struct pci_device_id *ent)
i915_detect_vgpu(dev_priv);
- ret = i915_driver_init_mmio(dev_priv);
+ ret = i915_driver_mmio_probe(dev_priv);
if (ret < 0)
goto out_runtime_pm_put;
- ret = i915_driver_init_hw(dev_priv);
+ ret = i915_driver_hw_probe(dev_priv);
if (ret < 0)
goto out_cleanup_mmio;
- ret = i915_load_modeset_init(&dev_priv->drm);
+ ret = i915_driver_modeset_probe(&dev_priv->drm);
if (ret < 0)
goto out_cleanup_hw;
--
2.21.0
[PATCH] media: ov6650: Fix device node exposed without proper locking
Commit c62b96050bee ("media: ov6650: Register with asynchronous
subdevice framework") carelessly requested creation of a video device
node by setting a V4L2_SUBDEV_FL_HAS_DEVNODE flag. The driver is not
ready for that as it doesn't implement proper locking required for
serialization of IOCTLs.
Fix it by dropping the flag assignment.
Fixes: c62b96050bee ("media: ov6650: Register with asynchronous subdevice
framework")
Signed-off-by: Janusz Krzysztofik
---
drivers/media/i2c/ov6650.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index 1b972e591b48..ace95ba7dd19 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -1009,7 +1009,6 @@ static int ov6650_probe(struct i2c_client *client,
priv->colorspace = V4L2_COLORSPACE_JPEG;
priv->subdev.internal_ops = &ov6650_internal_ops;
- priv->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
ret = v4l2_async_register_subdev(&priv->subdev);
if (ret)
--
2.21.0
[PATCH] ASoC: ti: Fix SDMA users not providing channel names
McBSP used to work correctly as long as compat DMA probing, removed by
commit 642aafea8889 ("ASoC: ti: remove compat dma probing"), was
available. New method of DMA probing apparently requires users to
provide channel names when registering with SDMA, while McBSP passes
NULLs. Fix it.
The same probably applies to McASP (not tested), hence the patch fixes
both.
Fixes: 642aafea8889 ("ASoC: ti: remove compat dma probing")
Signed-off-by: Janusz Krzysztofik
---
sound/soc/ti/davinci-mcasp.c | 2 +-
sound/soc/ti/omap-mcbsp.c| 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/ti/davinci-mcasp.c b/sound/soc/ti/davinci-mcasp.c
index 9fbc759fdefe..f31805920e3e 100644
--- a/sound/soc/ti/davinci-mcasp.c
+++ b/sound/soc/ti/davinci-mcasp.c
@@ -2237,7 +2237,7 @@ static int davinci_mcasp_probe(struct platform_device
*pdev)
ret = edma_pcm_platform_register(&pdev->dev);
break;
case PCM_SDMA:
- ret = sdma_pcm_platform_register(&pdev->dev, NULL, NULL);
+ ret = sdma_pcm_platform_register(&pdev->dev, "tx", "rx");
break;
default:
dev_err(&pdev->dev, "No DMA controller found (%d)\n", ret);
diff --git a/sound/soc/ti/omap-mcbsp.c b/sound/soc/ti/omap-mcbsp.c
index a395598f1f20..610c5e706fd2 100644
--- a/sound/soc/ti/omap-mcbsp.c
+++ b/sound/soc/ti/omap-mcbsp.c
@@ -1438,7 +1438,7 @@ static int asoc_mcbsp_probe(struct platform_device *pdev)
if (ret)
return ret;
- return sdma_pcm_platform_register(&pdev->dev, NULL, NULL);
+ return sdma_pcm_platform_register(&pdev->dev, "tx", "rx");
}
static int asoc_mcbsp_remove(struct platform_device *pdev)
--
2.21.0
Re: [RFC PATCH 4/5] media: ov6650: Fix frame scaling not reset on crop
Hi Sakari, On Sunday, June 2, 2019 12:37:55 AM CEST Sakari Ailus wrote: > > ... I realised that the subtle effect of "media: > ov6650: Register with asynchronous subdevice framework" is that the driver > is now responsible for serialising the access to its own data structures > now. Indeed, I must have been not thinking much while preparing it, only following patterns from other implementations blindly, sorry. > And it doesn't do that. Could you submit a fix, please? It'd be good to > get that to 5.2 through the fixes branch. How about dropping that V4L2_SUBDEV_FL_HAS_DEVNODE flag for now? I think that will be the most safe approach for a quick fix. I'd then re-add it together with proper locking in a separate patch later. What do yo think? Thanks, Janusz
Re: [RFC PATCH 4/5] media: ov6650: Fix frame scaling not reset on crop
Hi Sakari,
On Friday, May 31, 2019 1:42:58 PM CEST Sakari Ailus wrote:
> Hi Janusz,
>
> On Sun, May 26, 2019 at 10:47:57PM +0200, Janusz Krzysztofik wrote:
> > According to V4L2 subdevice interface specification, frame scaling
> > factors should be reset to default values on modification of input frame
> > format. Assuming that requirement also applies in case of crop
> > rectangle modification unless V4L2_SEL_FLAG_KEEP_CONFIG is requested,
> > the driver now does not respect it.
> >
> > The KEEP_CONFIG case is already implemented, fix the other path.
> >
> > Signed-off-by: Janusz Krzysztofik
> > ---
> > drivers/media/i2c/ov6650.c | 8
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
> > index 47590cd51190..cc70d8952999 100644
> > --- a/drivers/media/i2c/ov6650.c
> > +++ b/drivers/media/i2c/ov6650.c
> > @@ -472,6 +472,8 @@ static int ov6650_get_selection(struct v4l2_subdev
*sd,
> > }
> > }
> >
> > +static int ov6650_s_fmt(struct v4l2_subdev *sd, u32 code, bool
half_scale);
> > +
>
> Would it be possible to rearrange the functions in the file so you wouldn't
> need extra prototypes? Preferrably that'd be a new patch.
Sure. I've intentionally done it like that for better readability of the
patches, but I have a task in my queue for rearrangement of functions order as
soon as other modifications are in place.
> > static int ov6650_set_selection(struct v4l2_subdev *sd,
> > struct v4l2_subdev_pad_config *cfg,
> > struct v4l2_subdev_selection *sel)
> > @@ -515,7 +517,13 @@ static int ov6650_set_selection(struct v4l2_subdev
*sd,
> > }
> > if (!ret)
> > priv->rect.height = sel->r.height;
> > + else
> > + return ret;
>
> if (ret)
> return ret;
OK
Perhaps you will have more comments on other patches so I'll wait a bit and
then resubmit the series as v2.
Thanks,
Janusz
> ...
>
> >
> > + if (priv->half_scale) {
> > + /* turn off half scaling, preserve media bus format */
> > + ret = ov6650_s_fmt(sd, priv->code, false);
> > + }
> > return ret;
> > }
> >
>
>
[RFC PATCH 4/5] media: ov6650: Fix frame scaling not reset on crop
According to V4L2 subdevice interface specification, frame scaling
factors should be reset to default values on modification of input frame
format. Assuming that requirement also applies in case of crop
rectangle modification unless V4L2_SEL_FLAG_KEEP_CONFIG is requested,
the driver now does not respect it.
The KEEP_CONFIG case is already implemented, fix the other path.
Signed-off-by: Janusz Krzysztofik
---
drivers/media/i2c/ov6650.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index 47590cd51190..cc70d8952999 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -472,6 +472,8 @@ static int ov6650_get_selection(struct v4l2_subdev *sd,
}
}
+static int ov6650_s_fmt(struct v4l2_subdev *sd, u32 code, bool half_scale);
+
static int ov6650_set_selection(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_selection *sel)
@@ -515,7 +517,13 @@ static int ov6650_set_selection(struct v4l2_subdev *sd,
}
if (!ret)
priv->rect.height = sel->r.height;
+ else
+ return ret;
+ if (priv->half_scale) {
+ /* turn off half scaling, preserve media bus format */
+ ret = ov6650_s_fmt(sd, priv->code, false);
+ }
return ret;
}
--
2.21.0
[RFC PATCH 1/5] media: ov6650: Fix V4L2_SEL_FLAG_KEEP_CONFIG handling
This flag is now ignored - output frame size is affected by new crop settings regardless of the flag value. Fix it. Since keeping output frame size untouched while applying new crop settings is not supported, simply return results of .get_selection() if V4L2_SEL_FLAG_KEEP_CONFIG is passed to .set_selection(). Signed-off-by: Janusz Krzysztofik --- drivers/media/i2c/ov6650.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c index c728f718716b..1b02479b616f 100644 --- a/drivers/media/i2c/ov6650.c +++ b/drivers/media/i2c/ov6650.c @@ -484,6 +484,10 @@ static int ov6650_set_selection(struct v4l2_subdev *sd, sel->target != V4L2_SEL_TGT_CROP) return -EINVAL; + /* No support for changing crop rectangle with frame size preserved */ + if (sel->flags & V4L2_SEL_FLAG_KEEP_CONFIG) + return ov6650_get_selection(sd, cfg, sel); + v4l_bound_align_image(&sel->r.width, 2, W_CIF, 1, &sel->r.height, 2, H_CIF, 1, 0); v4l_bound_align_image(&sel->r.left, DEF_HSTRT << 1, -- 2.21.0
[RFC PATCH 2/5] media: ov6650: Refactor ov6650_s_fmt() helper
The driver suffers from a few compliance and implementation issues:
- crop rectangle is affected by .set_fmt() pad operation callback,
- frame scaling is not reset on modification of crop rectangle,
- V4L2_SUBDEV_FORMAT_TRY support in .set_fmt() uses active crop
rectangle, not the one from a pad config, as a reference.
For easy resolution of those issues, ov6650_s_fmt() will first be
refactored.
The ov6650_s_fmt() helper function, mostly called form .set_fmt() pad
operation callback, now takes a decision if half scaling is applicable
for current crop rectangle and requested frame size, then possibly
adjusts hardware crop settings, frame scaling and media bus frame
format. It accepts a struct v4l2_mbus_framefmt argument passed by a
user to .set_fmt().
Move the decision on applicability of half scaling up to .set_fmt(),
then modify the ov6650_s_fmt() API so it accepts a half scaling flag.
In order to avoid passing full struct v4l2_mbus_framefmt argument to it
when called from functions other than .set_fmt(), also accept a target
pixel code as an argument and make the struct v4l2_mbus_framefmt used
for crop rectangle modification optional.
Signed-off-by: Janusz Krzysztofik
---
drivers/media/i2c/ov6650.c | 38 --
1 file changed, 20 insertions(+), 18 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index 1b02479b616f..8cb30f3e4851 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -570,25 +570,18 @@ static u8 to_clkrc(struct v4l2_fract *timeperframe,
}
/* set the format we will capture in */
-static int ov6650_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf)
+static int ov6650_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf,
+ u32 code, bool half_scale)
{
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct ov6650 *priv = to_ov6650(client);
- bool half_scale = !is_unscaled_ok(mf->width, mf->height, &priv->rect);
struct v4l2_subdev_selection sel = {
.which = V4L2_SUBDEV_FORMAT_ACTIVE,
.target = V4L2_SEL_TGT_CROP,
- .r.left = priv->rect.left + (priv->rect.width >> 1) -
- (mf->width >> (1 - half_scale)),
- .r.top = priv->rect.top + (priv->rect.height >> 1) -
- (mf->height >> (1 - half_scale)),
- .r.width = mf->width << half_scale,
- .r.height = mf->height << half_scale,
};
- u32 code = mf->code;
unsigned long mclk, pclk;
u8 coma_set = 0, coma_mask = 0, coml_set, coml_mask, clkrc;
- int ret;
+ int ret = 0;
/* select color matrix configuration for given color encoding */
switch (code) {
@@ -668,7 +661,16 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct
v4l2_mbus_framefmt *mf)
dev_dbg(&client->dev, "pixel clock divider: %ld.%ld\n",
mclk / pclk, 10 * mclk % pclk / pclk);
- ret = ov6650_set_selection(sd, NULL, &sel);
+ if (mf) {
+ sel.r.left = priv->rect.left + (priv->rect.width >> 1) -
+ (mf->width >> (1 - half_scale)),
+ sel.r.top = priv->rect.top + (priv->rect.height >> 1) -
+ (mf->height >> (1 - half_scale)),
+ sel.r.width = mf->width << half_scale,
+ sel.r.height = mf->height << half_scale,
+
+ ret = ov6650_set_selection(sd, NULL, &sel);
+ }
if (!ret)
ret = ov6650_reg_rmw(client, REG_COMA, coma_set, coma_mask);
if (!ret)
@@ -691,11 +693,14 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
struct v4l2_mbus_framefmt *mf = &format->format;
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct ov6650 *priv = to_ov6650(client);
+ bool half_scale;
if (format->pad)
return -EINVAL;
- if (is_unscaled_ok(mf->width, mf->height, &priv->rect))
+ half_scale = !is_unscaled_ok(mf->width, mf->height, &priv->rect);
+
+ if (!half_scale)
v4l_bound_align_image(&mf->width, 2, W_CIF, 1,
&mf->height, 2, H_CIF, 1, 0);
@@ -730,7 +735,7 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
} else {
/* apply new media bus format code and frame size */
- int ret = ov6650_s_fmt(sd, mf);
+ int ret = ov6650_s_fmt(sd, mf, mf->code, half_scale);
if (ret)
return ret;
@@ -885,11 +890,8 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
ret = ov6650_reset(client);
if (!ret)
ret = ov6650_prog_d
[RFC PATCH 5/5] media: ov6650: Add .init_cfg() pad operation callback
The driver now supports V4L2_SUBDEV_FORMAT_TRY operation mode only in
.get/set_fmt() pad operation callbacks. That means only .try_format
member of pad config is maintained. As a consequence, active crop
rectangle is used as a referece while V4L2_SUBDEV_FORMAT_TRY requests
are processed. In order to fix that, a method for initialization of
.try_crop pad config member is needed.
Implement .init_cfg() pad operation callback which initializes the pad
config from current active format and selection settings. From now on,
and before the driver V4L2_SUBDEV_FORMAT_TRY support is further
modified, host interface drivers should call .init_cfg() on a pad
config before passing it to V4L2_SUBDEV_FORMAT_TRY operations.
Signed-off-by: Janusz Krzysztofik
---
drivers/media/i2c/ov6650.c | 21 +
1 file changed, 21 insertions(+)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index cc70d8952999..c3d4c1f598b2 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -447,6 +447,26 @@ static int ov6650_s_power(struct v4l2_subdev *sd, int on)
return ret;
}
+static int ov6650_init_cfg(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(sd);
+ struct ov6650 *priv = to_ov6650(client);
+ struct v4l2_mbus_framefmt *mf;
+ struct v4l2_rect *rect;
+
+ mf = &cfg->try_fmt;
+ *mf = ov6650_def_fmt;
+ mf->width = priv->rect.width >> priv->half_scale;
+ mf->height = priv->rect.height >> priv->half_scale;
+ mf->code = priv->code;
+
+ rect = &cfg->try_crop;
+ *rect = priv->rect;
+
+ return 0;
+}
+
static int ov6650_get_selection(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_selection *sel)
@@ -959,6 +979,7 @@ static const struct v4l2_subdev_video_ops ov6650_video_ops
= {
};
static const struct v4l2_subdev_pad_ops ov6650_pad_ops = {
+ .init_cfg = ov6650_init_cfg,
.enum_mbus_code = ov6650_enum_mbus_code,
.get_selection = ov6650_get_selection,
.set_selection = ov6650_set_selection,
--
2.21.0
[RFC PATCH 3/5] media: ov6650: Fix active crop rectangle affected by .set_fmt()
According to subdevice interface specification found in V4L2 API
documentation, .set_fmt() pad operation callback should not affect
image geometry set in preceding image processing steps. Unfortunately,
ov6650_s_fmt() helper, when called from .set_fmt() with new user
requested frame size passed to it, does not respect that requirement
as that was not the case before, when the helper was still called from
.mbus_set_fmt() video operation callback.
Remmove a call to .set_selection() from ov6650_s_fmt() helper so it no
longer modifies the active crop rectangle and simplify its API by
removing no longer needed struct v4l2_mbus_framefmt argument. For
consistency of its results with active format processing, also update
try format path inside .set_fmt() (still using active crop rectangle as
a reference instead of the try one from pad config which is not yet
maintained by the driver).
Signed-off-by: Janusz Krzysztofik
---
drivers/media/i2c/ov6650.c | 32 ++--
1 file changed, 6 insertions(+), 26 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index 8cb30f3e4851..47590cd51190 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -570,15 +570,10 @@ static u8 to_clkrc(struct v4l2_fract *timeperframe,
}
/* set the format we will capture in */
-static int ov6650_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf,
- u32 code, bool half_scale)
+static int ov6650_s_fmt(struct v4l2_subdev *sd, u32 code, bool half_scale)
{
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct ov6650 *priv = to_ov6650(client);
- struct v4l2_subdev_selection sel = {
- .which = V4L2_SUBDEV_FORMAT_ACTIVE,
- .target = V4L2_SEL_TGT_CROP,
- };
unsigned long mclk, pclk;
u8 coma_set = 0, coma_mask = 0, coml_set, coml_mask, clkrc;
int ret = 0;
@@ -661,18 +656,7 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct
v4l2_mbus_framefmt *mf,
dev_dbg(&client->dev, "pixel clock divider: %ld.%ld\n",
mclk / pclk, 10 * mclk % pclk / pclk);
- if (mf) {
- sel.r.left = priv->rect.left + (priv->rect.width >> 1) -
- (mf->width >> (1 - half_scale)),
- sel.r.top = priv->rect.top + (priv->rect.height >> 1) -
- (mf->height >> (1 - half_scale)),
- sel.r.width = mf->width << half_scale,
- sel.r.height = mf->height << half_scale,
-
- ret = ov6650_set_selection(sd, NULL, &sel);
- }
- if (!ret)
- ret = ov6650_reg_rmw(client, REG_COMA, coma_set, coma_mask);
+ ret = ov6650_reg_rmw(client, REG_COMA, coma_set, coma_mask);
if (!ret)
ret = ov6650_reg_write(client, REG_CLKRC, clkrc);
if (!ret) {
@@ -700,10 +684,6 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
half_scale = !is_unscaled_ok(mf->width, mf->height, &priv->rect);
- if (!half_scale)
- v4l_bound_align_image(&mf->width, 2, W_CIF, 1,
- &mf->height, 2, H_CIF, 1, 0);
-
switch (mf->code) {
case MEDIA_BUS_FMT_Y10_1X10:
mf->code = MEDIA_BUS_FMT_Y8_1X8;
@@ -723,8 +703,8 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
/* store media bus format code and frame size in pad config */
- cfg->try_fmt.width = mf->width;
- cfg->try_fmt.height = mf->height;
+ cfg->try_fmt.width = priv->rect.width >> half_scale;
+ cfg->try_fmt.height = priv->rect.height >> half_scale;
cfg->try_fmt.code = mf->code;
/* return default mbus frame format updated with pad config */
@@ -735,7 +715,7 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
} else {
/* apply new media bus format code and frame size */
- int ret = ov6650_s_fmt(sd, mf, mf->code, half_scale);
+ int ret = ov6650_s_fmt(sd, mf->code, half_scale);
if (ret)
return ret;
@@ -891,7 +871,7 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
if (!ret)
ret = ov6650_prog_dflt(client);
if (!ret)
- ret = ov6650_s_fmt(sd, NULL, ov6650_def_fmt.code, false);
+ ret = ov6650_s_fmt(sd, ov6650_def_fmt.code, false);
if (!ret)
ret = v4l2_ctrl_handler_setup(&priv->hdl);
--
2.21.0
[RFC PATCH 0/5] media: ov6650: V4L2 subdev compliance fixes
An attepmt to make the driver more compliant with V4L2 subdevice interface specification. Non-compliant frame half scaling is kept in .set_fmt() until composing is implemented. Janusz Krzysztofik (5): media: ov6650: Fix V4L2_SEL_FLAG_KEEP_CONFIG handling media: ov6650: Refactor ov6650_s_fmt() helper media: ov6650: Fix active crop rectangle affected by .set_fmt() media: ov6650: Fix frame scaling not reset on crop media: ov6650: Add .init_cfg() pad operation callback drivers/media/i2c/ov6650.c | 71 +++--- 1 file changed, 43 insertions(+), 28 deletions(-) -- 2.21.0
Re: [PATCH v2 1/9] media: ov6650: Fix MODDULE_DESCRIPTION
On Tuesday, May 21, 2019 7:15:37 AM CEST Greg KH wrote:
> On Tue, May 21, 2019 at 12:49:59AM +0200, Janusz Krzysztofik wrote:
> > Commit 23a52386fabe ("media: ov6650: convert to standalone v4l2
> > subdevice") converted the driver from a soc_camera sensor to a
> > standalone V4L subdevice driver. Unfortunately, module description was
> > not updated to reflect the change. Fix it.
> >
> > While being at it, update email address of the module author.
> >
> > Fixes: 23a52386fabe ("media: ov6650: convert to standalone v4l2 subdevice")
> > Signed-off-by: Janusz Krzysztofik
> > cc: sta...@vger.kernel.org
> > ---
> > drivers/media/i2c/ov6650.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
> > index 1b972e591b48..a3d00afcb0c8 100644
> > --- a/drivers/media/i2c/ov6650.c
> > +++ b/drivers/media/i2c/ov6650.c
> > @@ -1045,6 +1045,6 @@ static struct i2c_driver ov6650_i2c_driver = {
> >
> > module_i2c_driver(ov6650_i2c_driver);
> >
> > -MODULE_DESCRIPTION("SoC Camera driver for OmniVision OV6650");
> > -MODULE_AUTHOR("Janusz Krzysztofik ");
> > +MODULE_DESCRIPTION("V4L2 subdevice driver for OmniVision OV6650 camera
> > sensor");
> > +MODULE_AUTHOR("Janusz Krzysztofik > MODULE_LICENSE("GPL v2");
>
> is this _really_ a patch that meets the stable kernel requirements?
> Same for this whole series...
Hi Greg,
My bad, I'm sorry. Please ignore the whole series, and I'll be more careful
in the future.
Thanks,
Janusz
>
> thanks,
>
> greg k-h
>
[PATCH v2 6/9] media: ov6650: Fix .get_fmt() V4L2_SUBDEV_FORMAT_TRY support
Commit da298c6d98d5 ("[media] v4l2: replace video op g_mbus_fmt by pad
op get_fmt") converted a former ov6650_g_fmt() video operation callback
to an ov6650_get_fmt() pad operation callback. However, the converted
function disregards a format->which flag that pad operations should
obey and always returns active frame format settings.
That can be fixed by always responding to V4L2_SUBDEV_FORMAT_TRY with
-EINVAL, or providing the response from a pad config argument, likely
updated by a former user call to V4L2_SUBDEV_FORMAT_TRY .set_fmt().
Since implementation of the latter is trivial, go for it.
Fixes: da298c6d98d5 ("[media] v4l2: replace video op g_mbus_fmt by pad op
get_fmt")
Signed-off-by: Janusz Krzysztofik
Cc: sta...@vger.kernel.org
---
drivers/media/i2c/ov6650.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index 731b03bef7a5..1915a43bff87 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -528,10 +528,16 @@ static int ov6650_get_fmt(struct v4l2_subdev *sd,
*mf = ov6650_def_fmt;
/* update media bus format code and frame size */
- mf->width = priv->rect.width >> priv->half_scale;
- mf->height = priv->rect.height >> priv->half_scale;
- mf->code= priv->code;
+ if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
+ mf->width = cfg->try_fmt.width;
+ mf->height = cfg->try_fmt.height;
+ mf->code = cfg->try_fmt.code;
+ } else {
+ mf->width = priv->rect.width >> priv->half_scale;
+ mf->height = priv->rect.height >> priv->half_scale;
+ mf->code = priv->code;
+ }
return 0;
}
--
2.21.0
[PATCH v2 2/9] media: ov6650: Fix control handler not freed on init error
Since commit afd9690c72c3 ("[media] ov6650: convert to the control
framework"), if an error occurs during initialization of a control
handler, resources possibly allocated to the handler are not freed
before device initialiaton is aborted. Fix it.
Fixes: afd9690c72c3 ("[media] ov6650: convert to the control framework")
Signed-off-by: Janusz Krzysztofik
Cc: sta...@vger.kernel.org
---
drivers/media/i2c/ov6650.c | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index a3d00afcb0c8..007f0ca24913 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -992,8 +992,10 @@ static int ov6650_probe(struct i2c_client *client,
V4L2_CID_GAMMA, 0, 0xff, 1, 0x12);
priv->subdev.ctrl_handler = &priv->hdl;
- if (priv->hdl.error)
- return priv->hdl.error;
+ if (priv->hdl.error) {
+ ret = priv->hdl.error;
+ goto ectlhdlfree;
+ }
v4l2_ctrl_auto_cluster(2, &priv->autogain, 0, true);
v4l2_ctrl_auto_cluster(3, &priv->autowb, 0, true);
@@ -1012,8 +1014,10 @@ static int ov6650_probe(struct i2c_client *client,
priv->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
ret = v4l2_async_register_subdev(&priv->subdev);
- if (ret)
- v4l2_ctrl_handler_free(&priv->hdl);
+ if (!ret)
+ return 0;
+ectlhdlfree:
+ v4l2_ctrl_handler_free(&priv->hdl);
return ret;
}
--
2.21.0
[PATCH v2 4/9] media: ov6650: Fix incorrect use of JPEG colorspace
Since its initial submission, the driver selects V4L2_COLORSPACE_JPEG
for supported formats other than V4L2_MBUS_FMT_SBGGR8_1X8. According
to v4l2-compliance test program, V4L2_COLORSPACE_JPEG applies
exclusively to V4L2_PIX_FMT_JPEG. Since the sensor does not support
JPEG format, fix it to always select V4L2_COLORSPACE_SRGB.
Fixes: 2f6e2404799a ("[media] SoC Camera: add driver for OV6650 sensor")
Signed-off-by: Janusz Krzysztofik
Cc: sta...@vger.kernel.org
---
drivers/media/i2c/ov6650.c | 13 ++---
1 file changed, 2 insertions(+), 11 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index 751b48483f27..e7790e9b8887 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -203,7 +203,6 @@ struct ov6650 {
unsigned long pclk_max; /* from resolution and format */
struct v4l2_fract tpf;/* as requested with
s_frame_interval */
u32 code;
- enum v4l2_colorspacecolorspace;
};
@@ -517,7 +516,7 @@ static int ov6650_get_fmt(struct v4l2_subdev *sd,
mf->width = priv->rect.width >> priv->half_scale;
mf->height = priv->rect.height >> priv->half_scale;
mf->code= priv->code;
- mf->colorspace = priv->colorspace;
+ mf->colorspace = V4L2_COLORSPACE_SRGB;
mf->field = V4L2_FIELD_NONE;
return 0;
@@ -625,11 +624,6 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct
v4l2_mbus_framefmt *mf)
priv->pclk_max = 800;
}
- if (code == MEDIA_BUS_FMT_SBGGR8_1X8)
- priv->colorspace = V4L2_COLORSPACE_SRGB;
- else if (code != 0)
- priv->colorspace = V4L2_COLORSPACE_JPEG;
-
if (half_scale) {
dev_dbg(&client->dev, "max resolution: QCIF\n");
coma_set |= COMA_QCIF;
@@ -660,7 +654,6 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct
v4l2_mbus_framefmt *mf)
ret = ov6650_reg_rmw(client, REG_COML, coml_set, coml_mask);
if (!ret) {
- mf->colorspace = priv->colorspace;
mf->width = priv->rect.width >> half_scale;
mf->height = priv->rect.height >> half_scale;
}
@@ -683,6 +676,7 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
&mf->height, 2, H_CIF, 1, 0);
mf->field = V4L2_FIELD_NONE;
+ mf->colorspace = V4L2_COLORSPACE_SRGB;
switch (mf->code) {
case MEDIA_BUS_FMT_Y10_1X10:
@@ -693,13 +687,11 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
case MEDIA_BUS_FMT_YUYV8_2X8:
case MEDIA_BUS_FMT_VYUY8_2X8:
case MEDIA_BUS_FMT_UYVY8_2X8:
- mf->colorspace = V4L2_COLORSPACE_JPEG;
break;
default:
mf->code = MEDIA_BUS_FMT_SBGGR8_1X8;
/* fall through */
case MEDIA_BUS_FMT_SBGGR8_1X8:
- mf->colorspace = V4L2_COLORSPACE_SRGB;
break;
}
@@ -1007,7 +999,6 @@ static int ov6650_probe(struct i2c_client *client,
priv->rect.height = H_CIF;
priv->half_scale = false;
priv->code= MEDIA_BUS_FMT_YUYV8_2X8;
- priv->colorspace = V4L2_COLORSPACE_JPEG;
priv->subdev.internal_ops = &ov6650_internal_ops;
priv->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
--
2.21.0
[PATCH v2 9/9] media: ov6650: Fix stored crop rectangle not in sync with hardware
The driver stores crop rectangle settings supposed to be in line with
hardware state in a device private structure. Since the driver initial
submission, crop rectangle width and height settings are not updated
correctly when rectangle offset settings are applied on hardware. If
an error occurs while the device is updated, the stored settings my no
longer reflect hardware state and consecutive calls to .get_selection()
as well as .get/set_fmt() may return incorrect information. That in
turn may affect ability of a bridge device to use correct DMA transfer
settings if such incorrect informamtion on active frame format returned
by .get/set_fmt() is used.
Assuming a failed update of the device means its actual settings haven't
changed, update crop rectangle width and height settings stored in the
device private structure correctly while the rectangle offset is
successfully applied on hardware so the stored values always reflect
actual hardware state to the extent possible.
Fixes: 2f6e2404799a ("[media] SoC Camera: add driver for OV6650 sensor")
Signed-off-by: Janusz Krzysztofik
Cc: sta...@vger.kernel.org
---
drivers/media/i2c/ov6650.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index 65d43390dbeb..c728f718716b 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -494,6 +494,7 @@ static int ov6650_set_selection(struct v4l2_subdev *sd,
ret = ov6650_reg_write(client, REG_HSTRT, sel->r.left >> 1);
if (!ret) {
+ priv->rect.width += priv->rect.left - sel->r.left;
priv->rect.left = sel->r.left;
ret = ov6650_reg_write(client, REG_HSTOP,
(sel->r.left + sel->r.width) >> 1);
@@ -503,6 +504,7 @@ static int ov6650_set_selection(struct v4l2_subdev *sd,
ret = ov6650_reg_write(client, REG_VSTRT, sel->r.top >> 1);
}
if (!ret) {
+ priv->rect.height += priv->rect.top - sel->r.top;
priv->rect.top = sel->r.top;
ret = ov6650_reg_write(client, REG_VSTOP,
(sel->r.top + sel->r.height) >> 1);
--
2.21.0
[PATCH v2 1/9] media: ov6650: Fix MODDULE_DESCRIPTION
Commit 23a52386fabe ("media: ov6650: convert to standalone v4l2
subdevice") converted the driver from a soc_camera sensor to a
standalone V4L subdevice driver. Unfortunately, module description was
not updated to reflect the change. Fix it.
While being at it, update email address of the module author.
Fixes: 23a52386fabe ("media: ov6650: convert to standalone v4l2 subdevice")
Signed-off-by: Janusz Krzysztofik
cc: sta...@vger.kernel.org
---
drivers/media/i2c/ov6650.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index 1b972e591b48..a3d00afcb0c8 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -1045,6 +1045,6 @@ static struct i2c_driver ov6650_i2c_driver = {
module_i2c_driver(ov6650_i2c_driver);
-MODULE_DESCRIPTION("SoC Camera driver for OmniVision OV6650");
-MODULE_AUTHOR("Janusz Krzysztofik ");
+MODULE_DESCRIPTION("V4L2 subdevice driver for OmniVision OV6650 camera
sensor");
+MODULE_AUTHOR("Janusz Krzysztofik
[PATCH v2 7/9] media: ov6650: Fix default format not applied on device probe
It is not clear what pixel format is actually configured in hardware on
reset. MEDIA_BUS_FMT_YUYV8_2X8, assumed on device probe since the
driver was intiially submitted, is for sure not the one.
Fix it by explicitly applying a known, driver default frame format just
after initial device reset.
Fixes: 2f6e2404799a ("[media] SoC Camera: add driver for OV6650 sensor")
Signed-off-by: Janusz Krzysztofik
Cc: sta...@vger.kernel.org
---
drivers/media/i2c/ov6650.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index 1915a43bff87..b199332f62d7 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -876,6 +876,11 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
ret = ov6650_reset(client);
if (!ret)
ret = ov6650_prog_dflt(client);
+ if (!ret) {
+ struct v4l2_mbus_framefmt mf = ov6650_def_fmt;
+
+ ret = ov6650_s_fmt(sd, &mf);
+ }
if (!ret)
ret = v4l2_ctrl_handler_setup(&priv->hdl);
@@ -1030,8 +1035,6 @@ static int ov6650_probe(struct i2c_client *client,
priv->rect.top= DEF_VSTRT << 1;
priv->rect.width = W_CIF;
priv->rect.height = H_CIF;
- priv->half_scale = false;
- priv->code= MEDIA_BUS_FMT_YUYV8_2X8;
priv->subdev.internal_ops = &ov6650_internal_ops;
priv->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
--
2.21.0
[PATCH v2 3/9] media: ov6650: Fix crop rectangle alignment not passed back
Commit 4f996594ceaf ("[media] v4l2: make vidioc_s_crop const")
introduced a writable copy of constified user requested crop rectangle
in order to be able to perform hardware alignments on it. Later
on, commit 10d5509c8d50 ("[media] v4l2: remove g/s_crop from video
ops") replaced s_crop() video operaion using that const argument with
set_selection() pad operation which had a corresponding argument not
constified, however the original behavior of the driver was not
restored. Since that time, any hardware alignment applied on a user
requested crop rectangle is not passed back to the user calling
.set_selection() as it should be.
Fix the issue by dropping the copy and replacing all references to it
with references to the crop rectangle embedded in the user argument.
Fixes: 10d5509c8d50 ("[media] v4l2: remove g/s_crop from video ops")
Signed-off-by: Janusz Krzysztofik
Cc: sta...@vger.kernel.org
---
drivers/media/i2c/ov6650.c | 31 +++
1 file changed, 15 insertions(+), 16 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index 007f0ca24913..751b48483f27 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -468,38 +468,37 @@ static int ov6650_set_selection(struct v4l2_subdev *sd,
{
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct ov6650 *priv = to_ov6650(client);
- struct v4l2_rect rect = sel->r;
int ret;
if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE ||
sel->target != V4L2_SEL_TGT_CROP)
return -EINVAL;
- v4l_bound_align_image(&rect.width, 2, W_CIF, 1,
- &rect.height, 2, H_CIF, 1, 0);
- v4l_bound_align_image(&rect.left, DEF_HSTRT << 1,
- (DEF_HSTRT << 1) + W_CIF - (__s32)rect.width, 1,
- &rect.top, DEF_VSTRT << 1,
- (DEF_VSTRT << 1) + H_CIF - (__s32)rect.height, 1,
- 0);
+ v4l_bound_align_image(&sel->r.width, 2, W_CIF, 1,
+ &sel->r.height, 2, H_CIF, 1, 0);
+ v4l_bound_align_image(&sel->r.left, DEF_HSTRT << 1,
+ (DEF_HSTRT << 1) + W_CIF - (__s32)sel->r.width, 1,
+ &sel->r.top, DEF_VSTRT << 1,
+ (DEF_VSTRT << 1) + H_CIF - (__s32)sel->r.height,
+ 1, 0);
- ret = ov6650_reg_write(client, REG_HSTRT, rect.left >> 1);
+ ret = ov6650_reg_write(client, REG_HSTRT, sel->r.left >> 1);
if (!ret) {
- priv->rect.left = rect.left;
+ priv->rect.left = sel->r.left;
ret = ov6650_reg_write(client, REG_HSTOP,
- (rect.left + rect.width) >> 1);
+ (sel->r.left + sel->r.width) >> 1);
}
if (!ret) {
- priv->rect.width = rect.width;
- ret = ov6650_reg_write(client, REG_VSTRT, rect.top >> 1);
+ priv->rect.width = sel->r.width;
+ ret = ov6650_reg_write(client, REG_VSTRT, sel->r.top >> 1);
}
if (!ret) {
- priv->rect.top = rect.top;
+ priv->rect.top = sel->r.top;
ret = ov6650_reg_write(client, REG_VSTOP,
- (rect.top + rect.height) >> 1);
+ (sel->r.top + sel->r.height) >> 1);
}
if (!ret)
- priv->rect.height = rect.height;
+ priv->rect.height = sel->r.height;
return ret;
}
--
2.21.0
[PATCH v2 0/9] media: ov6650: A collection of fixes
Janusz Krzysztofik (9): media: ov6650: Fix MODDULE_DESCRIPTION media: ov6650: Fix control handler not freed on init error media: ov6650: Fix crop rectangle alignment not passed back media: ov6650: Fix incorrect use of JPEG colorspace media: ov6650: Fix some format attributes not under control media: ov6650: Fix .get_fmt() V4L2_SUBDEV_FORMAT_TRY support media: ov6650: Fix default format not applied on device probe media: ov6650: Fix stored frame format not in sync with hardware media: ov6650: Fix stored crop rectangle not in sync with hardware drivers/media/i2c/ov6650.c | 137 +++-- 1 file changed, 86 insertions(+), 51 deletions(-) Changelog v1->v2: - dropped patches 3/14 through 7/14 which were adding parameter checks now called from v4l2_subdev_call() - inspired by Sakari's requiest, thanks! -- 2.21.0
[PATCH v2 8/9] media: ov6650: Fix stored frame format not in sync with hardware
The driver stores frame format settings supposed to be in line with
hardware state in a device private structure. Since the driver initial
submission, those settings are updated before they are actually applied
on hardware. If an error occurs on device update, the stored settings
my not reflect hardware state anymore and consecutive calls to
.get_fmt() may return incorrect information. That in turn may affect
ability of a bridge device to use correct DMA transfer settings if such
incorrect informmation on active frame format returned by .get_fmt() is
used.
Assuming a failed device update mean its state hasn't changed, update
frame format related settings stored in the device private structure
only after they are successfully applied so the stored values always
reflect hardware state as closely as possible.
Fixes: 2f6e2404799a ("[media] SoC Camera: add driver for OV6650 sensor")
Signed-off-by: Janusz Krzysztofik
Cc: sta...@vger.kernel.org
---
drivers/media/i2c/ov6650.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index b199332f62d7..65d43390dbeb 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -630,7 +630,6 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct
v4l2_mbus_framefmt *mf)
dev_err(&client->dev, "Pixel format not handled: 0x%x\n", code);
return -EINVAL;
}
- priv->code = code;
if (code == MEDIA_BUS_FMT_Y8_1X8 ||
code == MEDIA_BUS_FMT_SBGGR8_1X8) {
@@ -651,7 +650,6 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct
v4l2_mbus_framefmt *mf)
dev_dbg(&client->dev, "max resolution: CIF\n");
coma_mask |= COMA_QCIF;
}
- priv->half_scale = half_scale;
clkrc = CLKRC_12MHz;
mclk = 1200;
@@ -669,8 +667,13 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct
v4l2_mbus_framefmt *mf)
ret = ov6650_reg_rmw(client, REG_COMA, coma_set, coma_mask);
if (!ret)
ret = ov6650_reg_write(client, REG_CLKRC, clkrc);
- if (!ret)
+ if (!ret) {
+ priv->half_scale = half_scale;
+
ret = ov6650_reg_rmw(client, REG_COML, coml_set, coml_mask);
+ }
+ if (!ret)
+ priv->code = code;
return ret;
}
--
2.21.0
[PATCH v2 5/9] media: ov6650: Fix some format attributes not under control
User arguments passed to .get/set_fmt() pad operation callbacks may
contain unsupported values. The driver takes control over frame size
and pixel code as well as colorspace and field attributes but has never
cared for remainig format attributes, i.e., ycbcr_enc, quantization
and xfer_func, introduced by commit 11ff030c7365 ("[media]
v4l2-mediabus: improve colorspace support"). Fix it.
Set up a static v4l2_mbus_framefmt structure with attributes
initialized to reasonable defaults and use it for updating content of
user provided arguments. In case of V4L2_SUBDEV_FORMAT_ACTIVE,
postpone frame size update, now performed from inside ov6650_s_fmt()
helper, util the user argument is first updated in ov6650_set_fmt() with
default frame format content. For V4L2_SUBDEV_FORMAT_TRY, don't copy
all attributes to pad config, only those handled by the driver, then
fill the response with the default frame format updated with updated
pad config format code and frame size.
Fixes: 11ff030c7365 ("[media] v4l2-mediabus: improve colorspace support")
Signed-off-by: Janusz Krzysztofik
Cc: sta...@vger.kernel.org
---
drivers/media/i2c/ov6650.c | 51 +-
1 file changed, 39 insertions(+), 12 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index e7790e9b8887..731b03bef7a5 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -215,6 +215,17 @@ static u32 ov6650_codes[] = {
MEDIA_BUS_FMT_Y8_1X8,
};
+static const struct v4l2_mbus_framefmt ov6650_def_fmt = {
+ .width = W_CIF,
+ .height = H_CIF,
+ .code = MEDIA_BUS_FMT_SBGGR8_1X8,
+ .colorspace = V4L2_COLORSPACE_SRGB,
+ .field = V4L2_FIELD_NONE,
+ .ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT,
+ .quantization = V4L2_QUANTIZATION_DEFAULT,
+ .xfer_func = V4L2_XFER_FUNC_DEFAULT,
+};
+
/* read a register */
static int ov6650_reg_read(struct i2c_client *client, u8 reg, u8 *val)
{
@@ -513,11 +524,13 @@ static int ov6650_get_fmt(struct v4l2_subdev *sd,
if (format->pad)
return -EINVAL;
+ /* initialize response with default media bus frame format */
+ *mf = ov6650_def_fmt;
+
+ /* update media bus format code and frame size */
mf->width = priv->rect.width >> priv->half_scale;
mf->height = priv->rect.height >> priv->half_scale;
mf->code= priv->code;
- mf->colorspace = V4L2_COLORSPACE_SRGB;
- mf->field = V4L2_FIELD_NONE;
return 0;
}
@@ -653,10 +666,6 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct
v4l2_mbus_framefmt *mf)
if (!ret)
ret = ov6650_reg_rmw(client, REG_COML, coml_set, coml_mask);
- if (!ret) {
- mf->width = priv->rect.width >> half_scale;
- mf->height = priv->rect.height >> half_scale;
- }
return ret;
}
@@ -675,9 +684,6 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
v4l_bound_align_image(&mf->width, 2, W_CIF, 1,
&mf->height, 2, H_CIF, 1, 0);
- mf->field = V4L2_FIELD_NONE;
- mf->colorspace = V4L2_COLORSPACE_SRGB;
-
switch (mf->code) {
case MEDIA_BUS_FMT_Y10_1X10:
mf->code = MEDIA_BUS_FMT_Y8_1X8;
@@ -695,10 +701,31 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
break;
}
- if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
- return ov6650_s_fmt(sd, mf);
- cfg->try_fmt = *mf;
+ if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
+ /* store media bus format code and frame size in pad config */
+ cfg->try_fmt.width = mf->width;
+ cfg->try_fmt.height = mf->height;
+ cfg->try_fmt.code = mf->code;
+ /* return default mbus frame format updated with pad config */
+ *mf = ov6650_def_fmt;
+ mf->width = cfg->try_fmt.width;
+ mf->height = cfg->try_fmt.height;
+ mf->code = cfg->try_fmt.code;
+
+ } else {
+ /* apply new media bus format code and frame size */
+ int ret = ov6650_s_fmt(sd, mf);
+
+ if (ret)
+ return ret;
+
+ /* return default format updated with active size and code */
+ *mf = ov6650_def_fmt;
+ mf->width = priv->rect.width >> priv->half_scale;
+ mf->height = priv->rect.height >> priv->half_scale;
+ mf->code = priv->code;
+ }
return 0;
}
--
2.21.0
[PATCH v7 0/3] media: v4l2-subdev: Verify arguments in v4l2_subdev_call()
Correctness of format type (try or active) and pad ID parameters passed to subdevice operation callbacks is now verified only for IOCTL calls. However, those callbacks are also used by drivers, e.g., V4L2 host interfaces. Since both subdev_do_ioctl() and drivers are using v4l2_subdev_call() macro while calling subdevice operations, move those parameter checks from subdev_do_ioctl() to v4l2_subdev_call(). Also, add check for non-NULL pointers, including pad config if V4L2_SUBDEV_FORMAT_TRY is requested. Having that done, we can avoid taking care of those checks inside drivers. Janusz Krzysztofik (3): media: v4l2-subdev: Verify arguments in v4l2_subdev_call() media: v4l2-subdev: Verify v4l2_subdev_call() pointer arguments media: v4l2-subdev: Verify v4l2_subdev_call() pad config argument drivers/media/v4l2-core/v4l2-subdev.c | 268 +- include/media/v4l2-subdev.h | 6 + 2 files changed, 188 insertions(+), 86 deletions(-) Changelog: v6->v7: Changes suggested by Sakari - thanks! - never succeed pad check on media entities with pad_num == 0, - allow pad 0 on subdevies not registered as media entities. v5->v6: - rename wrappers to call_something() as suggested by Sakari - thanks! - make check_ functions inline - also on Sakari's suggestion, thanks! - drop patch 2/4 and remove WARN_ONs from remaining patches to avoid kernel WARNs on non-kernel bugs - thanks Hans for pointing this out! v4->v5: - a few coding style and code formatting changes, - require CONFIG_MEDIA_CONTROLLER, not CONFIG_VIDEO_V4L2_SUBDEV_API, for a valid pad ID check, - perform pad ID check only if at least one pad is configured so drivers which don't configure pads are not affected if built with CONFIG_MEDIA_CONTROLLER defined, - issue kernel warnings on invalid parameters (new patch - 2/4), - validate pointers before using them (new patch - 3/4). v3->v4: - fix 'struct' keyword missing from patch 2/2, - fix checkpatch reported style issue in patch 2/2 Sorry for that. v2->v3: - add patch 2/2 with pad config check, - adjust continuation line alignments in patch 1/2 to match those used in 2/2. v1->v2: - replace the horrible macro with a structure of wrapper functions; inspired by Hans' and Sakari's comments - thanks! -- 2.21.0
[PATCH v7 2/3] media: v4l2-subdev: Verify v4l2_subdev_call() pointer arguments
Parameters passed to check helpers are now obtained by dereferencing
unverified pointer arguments. Check validity of those pointers first.
Signed-off-by: Janusz Krzysztofik
---
drivers/media/v4l2-core/v4l2-subdev.c | 27 +++
1 file changed, 27 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
b/drivers/media/v4l2-core/v4l2-subdev.c
index 59fdc0f08870..957c8e5cdfe1 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -147,6 +147,9 @@ static inline int check_pad(struct v4l2_subdev *sd, __u32
pad)
static inline int check_format(struct v4l2_subdev *sd,
struct v4l2_subdev_format *format)
{
+ if (!format)
+ return -EINVAL;
+
return check_which(format->which) ? : check_pad(sd, format->pad);
}
@@ -170,6 +173,9 @@ static int call_enum_mbus_code(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_mbus_code_enum *code)
{
+ if (!code)
+ return -EINVAL;
+
return check_which(code->which) ? : check_pad(sd, code->pad) ? :
sd->ops->pad->enum_mbus_code(sd, cfg, code);
}
@@ -178,6 +184,9 @@ static int call_enum_frame_size(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_frame_size_enum *fse)
{
+ if (!fse)
+ return -EINVAL;
+
return check_which(fse->which) ? : check_pad(sd, fse->pad) ? :
sd->ops->pad->enum_frame_size(sd, cfg, fse);
}
@@ -185,6 +194,9 @@ static int call_enum_frame_size(struct v4l2_subdev *sd,
static inline int check_frame_interval(struct v4l2_subdev *sd,
struct v4l2_subdev_frame_interval *fi)
{
+ if (!fi)
+ return -EINVAL;
+
return check_pad(sd, fi->pad);
}
@@ -206,6 +218,9 @@ static int call_enum_frame_interval(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_frame_interval_enum *fie)
{
+ if (!fie)
+ return -EINVAL;
+
return check_which(fie->which) ? : check_pad(sd, fie->pad) ? :
sd->ops->pad->enum_frame_interval(sd, cfg, fie);
}
@@ -213,6 +228,9 @@ static int call_enum_frame_interval(struct v4l2_subdev *sd,
static inline int check_selection(struct v4l2_subdev *sd,
struct v4l2_subdev_selection *sel)
{
+ if (!sel)
+ return -EINVAL;
+
return check_which(sel->which) ? : check_pad(sd, sel->pad);
}
@@ -235,6 +253,9 @@ static int call_set_selection(struct v4l2_subdev *sd,
static inline int check_edid(struct v4l2_subdev *sd,
struct v4l2_subdev_edid *edid)
{
+ if (!edid)
+ return -EINVAL;
+
if (edid->blocks && edid->edid == NULL)
return -EINVAL;
@@ -254,6 +275,9 @@ static int call_set_edid(struct v4l2_subdev *sd, struct
v4l2_subdev_edid *edid)
static int call_dv_timings_cap(struct v4l2_subdev *sd,
struct v4l2_dv_timings_cap *cap)
{
+ if (!cap)
+ return -EINVAL;
+
return check_pad(sd, cap->pad) ? :
sd->ops->pad->dv_timings_cap(sd, cap);
}
@@ -261,6 +285,9 @@ static int call_dv_timings_cap(struct v4l2_subdev *sd,
static int call_enum_dv_timings(struct v4l2_subdev *sd,
struct v4l2_enum_dv_timings *dvt)
{
+ if (!dvt)
+ return -EINVAL;
+
return check_pad(sd, dvt->pad) ? :
sd->ops->pad->enum_dv_timings(sd, dvt);
}
--
2.21.0
[PATCH v7 1/3] media: v4l2-subdev: Verify arguments in v4l2_subdev_call()
Correctness of format type (try or active) and pad number parameters
passed to subdevice operation callbacks is now verified only for IOCTL
calls. However, those callbacks are also used by drivers, e.g., V4L2
host interfaces.
Since both subdev_do_ioctl() and drivers are using v4l2_subdev_call()
macro while calling subdevice operations, move those parameter checks
from subdev_do_ioctl() to v4l2_subdev_call() so we can avoid taking care
of those checks inside drivers.
Define a wrapper function for each operation callback in scope, then
gather those wrappers in a static v4l2_subdev_ops structure so the
v4l2_subdev_call() macro can find them easy if provided.
Signed-off-by: Janusz Krzysztofik
---
drivers/media/v4l2-core/v4l2-subdev.c | 238 --
include/media/v4l2-subdev.h | 6 +
2 files changed, 152 insertions(+), 92 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
b/drivers/media/v4l2-core/v4l2-subdev.c
index d75815ab0d7b..59fdc0f08870 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -120,56 +120,175 @@ static int subdev_close(struct file *file)
return 0;
}
-#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
-static int check_format(struct v4l2_subdev *sd,
- struct v4l2_subdev_format *format)
+static inline int check_which(__u32 which)
{
- if (format->which != V4L2_SUBDEV_FORMAT_TRY &&
- format->which != V4L2_SUBDEV_FORMAT_ACTIVE)
- return -EINVAL;
-
- if (format->pad >= sd->entity.num_pads)
+ if (which != V4L2_SUBDEV_FORMAT_TRY &&
+ which != V4L2_SUBDEV_FORMAT_ACTIVE)
return -EINVAL;
return 0;
}
-static int check_crop(struct v4l2_subdev *sd, struct v4l2_subdev_crop *crop)
+static inline int check_pad(struct v4l2_subdev *sd, __u32 pad)
{
- if (crop->which != V4L2_SUBDEV_FORMAT_TRY &&
- crop->which != V4L2_SUBDEV_FORMAT_ACTIVE)
+#if defined(CONFIG_MEDIA_CONTROLLER)
+ if (sd->entity.graph_obj.mdev) {
+ if (pad >= sd->entity.num_pads)
+ return -EINVAL;
+ return 0;
+ }
+#endif
+ /* allow pad 0 on subdevices not registered as media entities */
+ if (pad > 0)
return -EINVAL;
+ return 0;
+}
- if (crop->pad >= sd->entity.num_pads)
- return -EINVAL;
+static inline int check_format(struct v4l2_subdev *sd,
+ struct v4l2_subdev_format *format)
+{
+ return check_which(format->which) ? : check_pad(sd, format->pad);
+}
- return 0;
+static int call_get_fmt(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_format *format)
+{
+ return check_format(sd, format) ? :
+ sd->ops->pad->get_fmt(sd, cfg, format);
}
-static int check_selection(struct v4l2_subdev *sd,
- struct v4l2_subdev_selection *sel)
+static int call_set_fmt(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_format *format)
{
- if (sel->which != V4L2_SUBDEV_FORMAT_TRY &&
- sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
- return -EINVAL;
+ return check_format(sd, format) ? :
+ sd->ops->pad->set_fmt(sd, cfg, format);
+}
- if (sel->pad >= sd->entity.num_pads)
- return -EINVAL;
+static int call_enum_mbus_code(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_mbus_code_enum *code)
+{
+ return check_which(code->which) ? : check_pad(sd, code->pad) ? :
+ sd->ops->pad->enum_mbus_code(sd, cfg, code);
+}
- return 0;
+static int call_enum_frame_size(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_frame_size_enum *fse)
+{
+ return check_which(fse->which) ? : check_pad(sd, fse->pad) ? :
+ sd->ops->pad->enum_frame_size(sd, cfg, fse);
}
-static int check_edid(struct v4l2_subdev *sd, struct v4l2_subdev_edid *edid)
+static inline int check_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_frame_interval *fi)
{
- if (edid->pad >= sd->entity.num_pads)
- return -EINVAL;
+ return check_pad(sd, fi->pad);
+}
+
+static int call_g_frame_interval(struct v4l2_subdev *sd,
+struct v4l2_subdev_frame_interval *fi)
+{
+ return check_frame_interval(sd, fi) ? :
+ sd->ops->video->g_frame_interval(sd, fi);
+}
+
+static int
[PATCH v7 3/3] media: v4l2-subdev: Verify v4l2_subdev_call() pad config argument
Extend parameter checks performed by v4l2_subdev_call() with a check for
a non-NULL pad config pointer if V4L2_SUBDEV_FORMAT_TRY format type is
requested so drivers don't need to care.
Signed-off-by: Janusz Krzysztofik
---
drivers/media/v4l2-core/v4l2-subdev.c | 27 +--
1 file changed, 21 insertions(+), 6 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
b/drivers/media/v4l2-core/v4l2-subdev.c
index 957c8e5cdfe1..34219e489be2 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -144,20 +144,30 @@ static inline int check_pad(struct v4l2_subdev *sd, __u32
pad)
return 0;
}
+static int check_cfg(__u32 which, struct v4l2_subdev_pad_config *cfg)
+{
+ if (which == V4L2_SUBDEV_FORMAT_TRY && !cfg)
+ return -EINVAL;
+
+ return 0;
+}
+
static inline int check_format(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_format *format)
{
if (!format)
return -EINVAL;
- return check_which(format->which) ? : check_pad(sd, format->pad);
+ return check_which(format->which) ? : check_pad(sd, format->pad) ? :
+ check_cfg(format->which, cfg);
}
static int call_get_fmt(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_format *format)
{
- return check_format(sd, format) ? :
+ return check_format(sd, cfg, format) ? :
sd->ops->pad->get_fmt(sd, cfg, format);
}
@@ -165,7 +175,7 @@ static int call_set_fmt(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_format *format)
{
- return check_format(sd, format) ? :
+ return check_format(sd, cfg, format) ? :
sd->ops->pad->set_fmt(sd, cfg, format);
}
@@ -177,6 +187,7 @@ static int call_enum_mbus_code(struct v4l2_subdev *sd,
return -EINVAL;
return check_which(code->which) ? : check_pad(sd, code->pad) ? :
+ check_cfg(code->which, cfg) ? :
sd->ops->pad->enum_mbus_code(sd, cfg, code);
}
@@ -188,6 +199,7 @@ static int call_enum_frame_size(struct v4l2_subdev *sd,
return -EINVAL;
return check_which(fse->which) ? : check_pad(sd, fse->pad) ? :
+ check_cfg(fse->which, cfg) ? :
sd->ops->pad->enum_frame_size(sd, cfg, fse);
}
@@ -222,23 +234,26 @@ static int call_enum_frame_interval(struct v4l2_subdev
*sd,
return -EINVAL;
return check_which(fie->which) ? : check_pad(sd, fie->pad) ? :
+ check_cfg(fie->which, cfg) ? :
sd->ops->pad->enum_frame_interval(sd, cfg, fie);
}
static inline int check_selection(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_selection *sel)
{
if (!sel)
return -EINVAL;
- return check_which(sel->which) ? : check_pad(sd, sel->pad);
+ return check_which(sel->which) ? : check_pad(sd, sel->pad) ? :
+ check_cfg(sel->which, cfg);
}
static int call_get_selection(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_selection *sel)
{
- return check_selection(sd, sel) ? :
+ return check_selection(sd, cfg, sel) ? :
sd->ops->pad->get_selection(sd, cfg, sel);
}
@@ -246,7 +261,7 @@ static int call_set_selection(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_selection *sel)
{
- return check_selection(sd, sel) ? :
+ return check_selection(sd, cfg, sel) ? :
sd->ops->pad->set_selection(sd, cfg, sel);
}
--
2.21.0
[PATCH v7 0/3] media: v4l2-subdev: Verify arguments in v4l2_subdev_call()
Correctness of format type (try or active) and pad ID parameters passed to subdevice operation callbacks is now verified only for IOCTL calls. However, those callbacks are also used by drivers, e.g., V4L2 host interfaces. Since both subdev_do_ioctl() and drivers are using v4l2_subdev_call() macro while calling subdevice operations, move those parameter checks from subdev_do_ioctl() to v4l2_subdev_call(). Also, add check for non-NULL pointers, including pad config if V4L2_SUBDEV_FORMAT_TRY is requested. Having that done, we can avoid taking care of those checks inside drivers. Janusz Krzysztofik (3): media: v4l2-subdev: Verify arguments in v4l2_subdev_call() media: v4l2-subdev: Verify v4l2_subdev_call() pointer arguments media: v4l2-subdev: Verify v4l2_subdev_call() pad config argument drivers/media/v4l2-core/v4l2-subdev.c | 268 +- include/media/v4l2-subdev.h | 6 + 2 files changed, 188 insertions(+), 86 deletions(-) Changelog: v6->v7: Changes suggested by Sakari - thanks! - never succeed pad check on media entities with pad_num == 0, - allow pad 0 on subdevies not registered as media entities. v5->v6: - rename wrappers to call_something() as suggested by Sakari - thanks! - make check_ functions inline - also on Sakari's suggestion, thanks! - drop patch 2/4 and remove WARN_ONs from remaining patches to avoid kernel WARNs on non-kernel bugs - thanks Hans for pointing this out! v4->v5: - a few coding style and code formatting changes, - require CONFIG_MEDIA_CONTROLLER, not CONFIG_VIDEO_V4L2_SUBDEV_API, for a valid pad ID check, - perform pad ID check only if at least one pad is configured so drivers which don't configure pads are not affected if built with CONFIG_MEDIA_CONTROLLER defined, - issue kernel warnings on invalid parameters (new patch - 2/4), - validate pointers before using them (new patch - 3/4). v3->v4: - fix 'struct' keyword missing from patch 2/2, - fix checkpatch reported style issue in patch 2/2 Sorry for that. v2->v3: - add patch 2/2 with pad config check, - adjust continuation line alignments in patch 1/2 to match those used in 2/2. v1->v2: - replace the horrible macro with a structure of wrapper functions; inspired by Hans' and Sakari's comments - thanks! -- 2.21.0
Re: [PATCH v6 1/3] media: v4l2-subdev: Verify arguments in v4l2_subdev_call()
On Friday, May 17, 2019 5:58:40 PM CEST Sakari Ailus wrote:
> Hi Janusz,
>
> On Wed, May 15, 2019 at 10:56:36PM +0200, Janusz Krzysztofik wrote:
> > Hi Sakari,
> >
> > On Wednesday, May 15, 2019 9:16:02 AM CEST Sakari Ailus wrote:
> > > Hi Janusz,
> > >
> > > On Wed, May 15, 2019 at 12:48:21AM +0200, Janusz Krzysztofik wrote:
> > > > -static int check_crop(struct v4l2_subdev *sd, struct v4l2_subdev_crop
> > *crop)
> > > > +static inline int check_pad(struct v4l2_subdev *sd, __u32 pad)
> > > > {
> > > > - if (crop->which != V4L2_SUBDEV_FORMAT_TRY &&
> > > > - crop->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> > > > +#if defined(CONFIG_MEDIA_CONTROLLER)
> > > > + if (sd->entity.num_pads && pad >= sd->entity.num_pads)
> > >
> > > One more comment.
> > >
> > > The num_pads doesn't really tell whether a given op is valid for a
device.
> > > Well, in this case it would have to be a bug in the driver, but those do
> > > happen. How about checking for sd->entity.graph_obj.mdev instead? It's
> > > non-NULL if the entity is registered with a media device, i.e. when
these
> > > callback functions are supposed to be called.
> >
> > Before I do that, let me undestand your point better.
> >
> > My intentions were:
> > 1) to provide a check for validity of a pad ID passed to an operation, not
ann
> > eligibility of a driver to support the operation,
> > 2) to not break drivers which don't set pad_num, especially when building
them
> > with CONFIG_MEDIA_CONTROLLER turned on for whatever reason.
>
> Indeed.
>
> But these checks still allow calling the pad operations on sub-devices that
> have no pads. That should not be allowed. Pads are a Media controller
> concept, they do not exist outside it; therefore checking for pads only if
> the subdev is a part of the media device would be entirely correct.
OK, now I see your point. You don't want the check to succeed if a media
entity has num_pads == 0.
> It should probably accompany a check that requires the pad number is zero
> if the subdev doesn't have a graph object, even if the pad field isn't
> supposedly used for any purpose. Would that address your concern?
Yes, that's acceptable. Let's require subdevice drivers to register as media
entities if they want to use pads > 0.
I'll update the patches and submit as v7 soon.
Thanks,
Janusz
> > Since pad IDs are verified against pad_num which may be not set, we should
> > obviously check validity of pad_num before comparing against it. Since
media
> > controller compatible subdevices need at least one pad, I think the check
for
> > non-zero pad_num is quite reasonable.
> >
> > Moreover, old drivers are actually using those pad operations you describe
as
> > not supposed to be called. They are using them because they were
converted to
> > use them in place of former video ops. Already dealing with pad IDs, they
may
> > decide to turn on CONFIG_MEDIA_CONTROLLER and use selected functionality,
for
> > example register pads, without implementing fulll media controller
support.
> > Why should we refuse to perform pad ID verification for them?
>
>
Re: [PATCH v6 1/3] media: v4l2-subdev: Verify arguments in v4l2_subdev_call()
Hi Sakari,
On Wednesday, May 15, 2019 9:16:02 AM CEST Sakari Ailus wrote:
> Hi Janusz,
>
> On Wed, May 15, 2019 at 12:48:21AM +0200, Janusz Krzysztofik wrote:
> > -static int check_crop(struct v4l2_subdev *sd, struct v4l2_subdev_crop
*crop)
> > +static inline int check_pad(struct v4l2_subdev *sd, __u32 pad)
> > {
> > - if (crop->which != V4L2_SUBDEV_FORMAT_TRY &&
> > - crop->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> > +#if defined(CONFIG_MEDIA_CONTROLLER)
> > + if (sd->entity.num_pads && pad >= sd->entity.num_pads)
>
> One more comment.
>
> The num_pads doesn't really tell whether a given op is valid for a device.
> Well, in this case it would have to be a bug in the driver, but those do
> happen. How about checking for sd->entity.graph_obj.mdev instead? It's
> non-NULL if the entity is registered with a media device, i.e. when these
> callback functions are supposed to be called.
Before I do that, let me undestand your point better.
My intentions were:
1) to provide a check for validity of a pad ID passed to an operation, not ann
eligibility of a driver to support the operation,
2) to not break drivers which don't set pad_num, especially when building them
with CONFIG_MEDIA_CONTROLLER turned on for whatever reason.
Since pad IDs are verified against pad_num which may be not set, we should
obviously check validity of pad_num before comparing against it. Since media
controller compatible subdevices need at least one pad, I think the check for
non-zero pad_num is quite reasonable.
Moreover, old drivers are actually using those pad operations you describe as
not supposed to be called. They are using them because they were converted to
use them in place of former video ops. Already dealing with pad IDs, they may
decide to turn on CONFIG_MEDIA_CONTROLLER and use selected functionality, for
example register pads, without implementing fulll media controller support.
Why should we refuse to perform pad ID verification for them?
Thanks,
Janusz
[PATCH v6 2/3] media: v4l2-subdev: Verify v4l2_subdev_call() pointer arguments
Parameters passed to check helpers are now obtained by dereferencing
unverified pointer arguments. Check validity of those pointers first.
Signed-off-by: Janusz Krzysztofik
---
drivers/media/v4l2-core/v4l2-subdev.c | 27 +++
1 file changed, 27 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
b/drivers/media/v4l2-core/v4l2-subdev.c
index c61c95007d89..6933f30e5041 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -141,6 +141,9 @@ static inline int check_pad(struct v4l2_subdev *sd, __u32
pad)
static inline int check_format(struct v4l2_subdev *sd,
struct v4l2_subdev_format *format)
{
+ if (!format)
+ return -EINVAL;
+
return check_which(format->which) ? : check_pad(sd, format->pad);
}
@@ -164,6 +167,9 @@ static int call_enum_mbus_code(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_mbus_code_enum *code)
{
+ if (!code)
+ return -EINVAL;
+
return check_which(code->which) ? : check_pad(sd, code->pad) ? :
sd->ops->pad->enum_mbus_code(sd, cfg, code);
}
@@ -172,6 +178,9 @@ static int call_enum_frame_size(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_frame_size_enum *fse)
{
+ if (!fse)
+ return -EINVAL;
+
return check_which(fse->which) ? : check_pad(sd, fse->pad) ? :
sd->ops->pad->enum_frame_size(sd, cfg, fse);
}
@@ -179,6 +188,9 @@ static int call_enum_frame_size(struct v4l2_subdev *sd,
static inline int check_frame_interval(struct v4l2_subdev *sd,
struct v4l2_subdev_frame_interval *fi)
{
+ if (!fi)
+ return -EINVAL;
+
return check_pad(sd, fi->pad);
}
@@ -200,6 +212,9 @@ static int call_enum_frame_interval(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_frame_interval_enum *fie)
{
+ if (!fie)
+ return -EINVAL;
+
return check_which(fie->which) ? : check_pad(sd, fie->pad) ? :
sd->ops->pad->enum_frame_interval(sd, cfg, fie);
}
@@ -207,6 +222,9 @@ static int call_enum_frame_interval(struct v4l2_subdev *sd,
static inline int check_selection(struct v4l2_subdev *sd,
struct v4l2_subdev_selection *sel)
{
+ if (!sel)
+ return -EINVAL;
+
return check_which(sel->which) ? : check_pad(sd, sel->pad);
}
@@ -229,6 +247,9 @@ static int call_set_selection(struct v4l2_subdev *sd,
static inline int check_edid(struct v4l2_subdev *sd,
struct v4l2_subdev_edid *edid)
{
+ if (!edid)
+ return -EINVAL;
+
if (edid->blocks && edid->edid == NULL)
return -EINVAL;
@@ -248,6 +269,9 @@ static int call_set_edid(struct v4l2_subdev *sd, struct
v4l2_subdev_edid *edid)
static int call_dv_timings_cap(struct v4l2_subdev *sd,
struct v4l2_dv_timings_cap *cap)
{
+ if (!cap)
+ return -EINVAL;
+
return check_pad(sd, cap->pad) ? :
sd->ops->pad->dv_timings_cap(sd, cap);
}
@@ -255,6 +279,9 @@ static int call_dv_timings_cap(struct v4l2_subdev *sd,
static int call_enum_dv_timings(struct v4l2_subdev *sd,
struct v4l2_enum_dv_timings *dvt)
{
+ if (!dvt)
+ return -EINVAL;
+
return check_pad(sd, dvt->pad) ? :
sd->ops->pad->enum_dv_timings(sd, dvt);
}
--
2.21.0
[PATCH v6 3/3] media: v4l2-subdev: Verify v4l2_subdev_call() pad config argument
Extend parameter checks performed by v4l2_subdev_call() with a check for
a non-NULL pad config pointer if V4L2_SUBDEV_FORMAT_TRY format type is
requested so drivers don't need to care.
Signed-off-by: Janusz Krzysztofik
---
drivers/media/v4l2-core/v4l2-subdev.c | 27 +--
1 file changed, 21 insertions(+), 6 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
b/drivers/media/v4l2-core/v4l2-subdev.c
index 6933f30e5041..6a5c4f046723 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -138,20 +138,30 @@ static inline int check_pad(struct v4l2_subdev *sd, __u32
pad)
return 0;
}
+static int check_cfg(__u32 which, struct v4l2_subdev_pad_config *cfg)
+{
+ if (which == V4L2_SUBDEV_FORMAT_TRY && !cfg)
+ return -EINVAL;
+
+ return 0;
+}
+
static inline int check_format(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_format *format)
{
if (!format)
return -EINVAL;
- return check_which(format->which) ? : check_pad(sd, format->pad);
+ return check_which(format->which) ? : check_pad(sd, format->pad) ? :
+ check_cfg(format->which, cfg);
}
static int call_get_fmt(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_format *format)
{
- return check_format(sd, format) ? :
+ return check_format(sd, cfg, format) ? :
sd->ops->pad->get_fmt(sd, cfg, format);
}
@@ -159,7 +169,7 @@ static int call_set_fmt(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_format *format)
{
- return check_format(sd, format) ? :
+ return check_format(sd, cfg, format) ? :
sd->ops->pad->set_fmt(sd, cfg, format);
}
@@ -171,6 +181,7 @@ static int call_enum_mbus_code(struct v4l2_subdev *sd,
return -EINVAL;
return check_which(code->which) ? : check_pad(sd, code->pad) ? :
+ check_cfg(code->which, cfg) ? :
sd->ops->pad->enum_mbus_code(sd, cfg, code);
}
@@ -182,6 +193,7 @@ static int call_enum_frame_size(struct v4l2_subdev *sd,
return -EINVAL;
return check_which(fse->which) ? : check_pad(sd, fse->pad) ? :
+ check_cfg(fse->which, cfg) ? :
sd->ops->pad->enum_frame_size(sd, cfg, fse);
}
@@ -216,23 +228,26 @@ static int call_enum_frame_interval(struct v4l2_subdev
*sd,
return -EINVAL;
return check_which(fie->which) ? : check_pad(sd, fie->pad) ? :
+ check_cfg(fie->which, cfg) ? :
sd->ops->pad->enum_frame_interval(sd, cfg, fie);
}
static inline int check_selection(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_selection *sel)
{
if (!sel)
return -EINVAL;
- return check_which(sel->which) ? : check_pad(sd, sel->pad);
+ return check_which(sel->which) ? : check_pad(sd, sel->pad) ? :
+ check_cfg(sel->which, cfg);
}
static int call_get_selection(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_selection *sel)
{
- return check_selection(sd, sel) ? :
+ return check_selection(sd, cfg, sel) ? :
sd->ops->pad->get_selection(sd, cfg, sel);
}
@@ -240,7 +255,7 @@ static int call_set_selection(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_selection *sel)
{
- return check_selection(sd, sel) ? :
+ return check_selection(sd, cfg, sel) ? :
sd->ops->pad->set_selection(sd, cfg, sel);
}
--
2.21.0
[PATCH v6 1/3] media: v4l2-subdev: Verify arguments in v4l2_subdev_call()
Correctness of format type (try or active) and pad number parameters
passed to subdevice operation callbacks is now verified only for IOCTL
calls. However, those callbacks are also used by drivers, e.g., V4L2
host interfaces.
Since both subdev_do_ioctl() and drivers are using v4l2_subdev_call()
macro while calling subdevice operations, move those parameter checks
from subdev_do_ioctl() to v4l2_subdev_call() so we can avoid taking care
of those checks inside drivers.
Define a wrapper function for each operation callback in scope, then
gather those wrappers in a static v4l2_subdev_ops structure so the
v4l2_subdev_call() macro can find them easy if provided.
Signed-off-by: Janusz Krzysztofik
---
drivers/media/v4l2-core/v4l2-subdev.c | 232 --
include/media/v4l2-subdev.h | 6 +
2 files changed, 146 insertions(+), 92 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
b/drivers/media/v4l2-core/v4l2-subdev.c
index d75815ab0d7b..c61c95007d89 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -120,56 +120,169 @@ static int subdev_close(struct file *file)
return 0;
}
-#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
-static int check_format(struct v4l2_subdev *sd,
- struct v4l2_subdev_format *format)
+static inline int check_which(__u32 which)
{
- if (format->which != V4L2_SUBDEV_FORMAT_TRY &&
- format->which != V4L2_SUBDEV_FORMAT_ACTIVE)
- return -EINVAL;
-
- if (format->pad >= sd->entity.num_pads)
+ if (which != V4L2_SUBDEV_FORMAT_TRY &&
+ which != V4L2_SUBDEV_FORMAT_ACTIVE)
return -EINVAL;
return 0;
}
-static int check_crop(struct v4l2_subdev *sd, struct v4l2_subdev_crop *crop)
+static inline int check_pad(struct v4l2_subdev *sd, __u32 pad)
{
- if (crop->which != V4L2_SUBDEV_FORMAT_TRY &&
- crop->which != V4L2_SUBDEV_FORMAT_ACTIVE)
+#if defined(CONFIG_MEDIA_CONTROLLER)
+ if (sd->entity.num_pads && pad >= sd->entity.num_pads)
return -EINVAL;
+#endif
+ return 0;
+}
- if (crop->pad >= sd->entity.num_pads)
- return -EINVAL;
+static inline int check_format(struct v4l2_subdev *sd,
+ struct v4l2_subdev_format *format)
+{
+ return check_which(format->which) ? : check_pad(sd, format->pad);
+}
- return 0;
+static int call_get_fmt(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_format *format)
+{
+ return check_format(sd, format) ? :
+ sd->ops->pad->get_fmt(sd, cfg, format);
}
-static int check_selection(struct v4l2_subdev *sd,
- struct v4l2_subdev_selection *sel)
+static int call_set_fmt(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_format *format)
{
- if (sel->which != V4L2_SUBDEV_FORMAT_TRY &&
- sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
- return -EINVAL;
+ return check_format(sd, format) ? :
+ sd->ops->pad->set_fmt(sd, cfg, format);
+}
- if (sel->pad >= sd->entity.num_pads)
- return -EINVAL;
+static int call_enum_mbus_code(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_mbus_code_enum *code)
+{
+ return check_which(code->which) ? : check_pad(sd, code->pad) ? :
+ sd->ops->pad->enum_mbus_code(sd, cfg, code);
+}
- return 0;
+static int call_enum_frame_size(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_frame_size_enum *fse)
+{
+ return check_which(fse->which) ? : check_pad(sd, fse->pad) ? :
+ sd->ops->pad->enum_frame_size(sd, cfg, fse);
}
-static int check_edid(struct v4l2_subdev *sd, struct v4l2_subdev_edid *edid)
+static inline int check_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_frame_interval *fi)
{
- if (edid->pad >= sd->entity.num_pads)
- return -EINVAL;
+ return check_pad(sd, fi->pad);
+}
+
+static int call_g_frame_interval(struct v4l2_subdev *sd,
+struct v4l2_subdev_frame_interval *fi)
+{
+ return check_frame_interval(sd, fi) ? :
+ sd->ops->video->g_frame_interval(sd, fi);
+}
+
+static int call_s_frame_interval(struct v4l2_subdev *sd,
+struct v4l2_subdev_frame_interval *fi)
+{
+ return check_frame_interval(sd, fi) ? :
+ sd->op
[PATCH v6 0/3] media: v4l2-subdev: Verify arguments in v4l2_subdev_call()
Correctness of format type (try or active) and pad ID parameters passed to subdevice operation callbacks is now verified only for IOCTL calls. However, those callbacks are also used by drivers, e.g., V4L2 host interfaces. Since both subdev_do_ioctl() and drivers are using v4l2_subdev_call() macro while calling subdevice operations, move those parameter checks from subdev_do_ioctl() to v4l2_subdev_call(). Also, add check for non-NULL pointers, including pad config if V4L2_SUBDEV_FORMAT_tRY is requested. Having that done, we can avoid taking care of those checks inside drivers. Janusz Krzysztofik (3): media: v4l2-subdev: Verify arguments in v4l2_subdev_call() media: v4l2-subdev: Verify v4l2_subdev_call() pointer arguments media: v4l2-subdev: Verify v4l2_subdev_call() pad config argument drivers/media/v4l2-core/v4l2-subdev.c | 262 +- include/media/v4l2-subdev.h | 6 + 2 files changed, 182 insertions(+), 86 deletions(-) Changelog: v5->v6: - rename wrappers to call_something() as suggested by Sakari - thanks! - make check_ functions inline - also on Sakari's suggestion, thanks! - drop patch 2/4 and remove WARN_ONs from remaining patches to avoid kernel WARNs on non-kernel bugs - thanks Hans for pointing this out! v4->v5: - a few coding style and code formatting changes, - require CONFIG_MEDIA_CONTROLLER, not CONFIG_VIDEO_V4L2_SUBDEV_API, for a valid pad ID check, - perform pad ID check only if at least one pad is configured so drivers which don't configure pads are not affected if built with CONFIG_MEDIA_CONTROLLER defined, - issue kernel warnings on invalid parameters (new patch - 2/4), - validate pointers before using them (new patch - 3/4). v3->v4: - fix 'struct' keyword missing from patch 2/2, - fix checkpatch reported style issue in patch 2/2 Sorry for that. v2->v3: - add patch 2/2 with pad config check, - adjust continuation line alignments in patch 1/2 to match those used in 2/2. v1->v2: - replace the horrible macro with a structure of wrapper functions; inspired by Hans' and Sakari's comments - thanks! -- 2.21.0
[PATCH v5 3/4] media: v4l2-subdev: Verify v4l2_subdev_call() pointer arguments
Parameters passed to check helpers are now obtained by dereferencing
unverified pointer arguments. Check validity of those pointers first.
Signed-off-by: Janusz Krzysztofik
---
drivers/media/v4l2-core/v4l2-subdev.c | 27 +++
1 file changed, 27 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
b/drivers/media/v4l2-core/v4l2-subdev.c
index 5f2264575cd7..3fc07af26c5b 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -141,6 +141,9 @@ static int check_pad(struct v4l2_subdev *sd, __u32 pad)
static int check_format(struct v4l2_subdev *sd,
struct v4l2_subdev_format *format)
{
+ if (WARN_ON(!format))
+ return -EINVAL;
+
return check_which(format->which) ? : check_pad(sd, format->pad);
}
@@ -164,6 +167,9 @@ static int check_enum_mbus_code(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_mbus_code_enum *code)
{
+ if (WARN_ON(!code))
+ return -EINVAL;
+
return check_which(code->which) ? : check_pad(sd, code->pad) ? :
sd->ops->pad->enum_mbus_code(sd, cfg, code);
}
@@ -172,6 +178,9 @@ static int check_enum_frame_size(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_frame_size_enum *fse)
{
+ if (WARN_ON(!fse))
+ return -EINVAL;
+
return check_which(fse->which) ? : check_pad(sd, fse->pad) ? :
sd->ops->pad->enum_frame_size(sd, cfg, fse);
}
@@ -179,6 +188,9 @@ static int check_enum_frame_size(struct v4l2_subdev *sd,
static int check_frame_interval(struct v4l2_subdev *sd,
struct v4l2_subdev_frame_interval *fi)
{
+ if (WARN_ON(!fi))
+ return -EINVAL;
+
return check_pad(sd, fi->pad);
}
@@ -200,6 +212,9 @@ static int check_enum_frame_interval(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_frame_interval_enum *fie)
{
+ if (WARN_ON(!fie))
+ return -EINVAL;
+
return check_which(fie->which) ? : check_pad(sd, fie->pad) ? :
sd->ops->pad->enum_frame_interval(sd, cfg, fie);
}
@@ -207,6 +222,9 @@ static int check_enum_frame_interval(struct v4l2_subdev *sd,
static int check_selection(struct v4l2_subdev *sd,
struct v4l2_subdev_selection *sel)
{
+ if (WARN_ON(!sel))
+ return -EINVAL;
+
return check_which(sel->which) ? : check_pad(sd, sel->pad);
}
@@ -228,6 +246,9 @@ static int check_set_selection(struct v4l2_subdev *sd,
static int check_edid(struct v4l2_subdev *sd, struct v4l2_subdev_edid *edid)
{
+ if (WARN_ON(!edid))
+ return -EINVAL;
+
if (WARN_ON(edid->blocks && edid->edid == NULL))
return -EINVAL;
@@ -247,6 +268,9 @@ static int check_set_edid(struct v4l2_subdev *sd, struct
v4l2_subdev_edid *edid)
static int check_dv_timings_cap(struct v4l2_subdev *sd,
struct v4l2_dv_timings_cap *cap)
{
+ if (WARN_ON(!cap))
+ return -EINVAL;
+
return check_pad(sd, cap->pad) ? :
sd->ops->pad->dv_timings_cap(sd, cap);
}
@@ -254,6 +278,9 @@ static int check_dv_timings_cap(struct v4l2_subdev *sd,
static int check_enum_dv_timings(struct v4l2_subdev *sd,
struct v4l2_enum_dv_timings *dvt)
{
+ if (WARN_ON(!dvt))
+ return -EINVAL;
+
return check_pad(sd, dvt->pad) ? :
sd->ops->pad->enum_dv_timings(sd, dvt);
}
--
2.21.0
[PATCH v5 2/4] media: v4l2-subdev: WARN_ON invalid v4l2_subdev_call() arguments
Invalid arguments passed to v4l2_subdev_call generally mean bugs. Be
noisy if that happens.
Signed-off-by: Janusz Krzysztofik
---
drivers/media/v4l2-core/v4l2-subdev.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
b/drivers/media/v4l2-core/v4l2-subdev.c
index 890916674d42..5f2264575cd7 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -122,8 +122,8 @@ static int subdev_close(struct file *file)
static int check_which(__u32 which)
{
- if (which != V4L2_SUBDEV_FORMAT_TRY &&
- which != V4L2_SUBDEV_FORMAT_ACTIVE)
+ if (WARN_ON(which != V4L2_SUBDEV_FORMAT_TRY &&
+ which != V4L2_SUBDEV_FORMAT_ACTIVE))
return -EINVAL;
return 0;
@@ -132,7 +132,7 @@ static int check_which(__u32 which)
static int check_pad(struct v4l2_subdev *sd, __u32 pad)
{
#if defined(CONFIG_MEDIA_CONTROLLER)
- if (sd->entity.num_pads && pad >= sd->entity.num_pads)
+ if (WARN_ON(sd->entity.num_pads && pad >= sd->entity.num_pads))
return -EINVAL;
#endif
return 0;
@@ -228,7 +228,7 @@ static int check_set_selection(struct v4l2_subdev *sd,
static int check_edid(struct v4l2_subdev *sd, struct v4l2_subdev_edid *edid)
{
- if (edid->blocks && edid->edid == NULL)
+ if (WARN_ON(edid->blocks && edid->edid == NULL))
return -EINVAL;
return check_pad(sd, edid->pad);
--
2.21.0
[PATCH v5 4/4] media: v4l2-subdev: Verify v4l2_subdev_call() pad config argument
Extend parameter checks performed by v4l2_subdev_call() with a check for
a non-NULL pad config pointer if V4L2_SUBDEV_FORMAT_TRY format type is
requested so drivers don't need to care.
Signed-off-by: Janusz Krzysztofik
---
drivers/media/v4l2-core/v4l2-subdev.c | 27 +--
1 file changed, 21 insertions(+), 6 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
b/drivers/media/v4l2-core/v4l2-subdev.c
index 3fc07af26c5b..fc8c308fb060 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -138,20 +138,30 @@ static int check_pad(struct v4l2_subdev *sd, __u32 pad)
return 0;
}
+static int check_cfg(__u32 which, struct v4l2_subdev_pad_config *cfg)
+{
+ if (WARN_ON(which == V4L2_SUBDEV_FORMAT_TRY && !cfg))
+ return -EINVAL;
+
+ return 0;
+}
+
static int check_format(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_format *format)
{
if (WARN_ON(!format))
return -EINVAL;
- return check_which(format->which) ? : check_pad(sd, format->pad);
+ return check_which(format->which) ? : check_pad(sd, format->pad) ? :
+ check_cfg(format->which, cfg);
}
static int check_get_fmt(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_format *format)
{
- return check_format(sd, format) ? :
+ return check_format(sd, cfg, format) ? :
sd->ops->pad->get_fmt(sd, cfg, format);
}
@@ -159,7 +169,7 @@ static int check_set_fmt(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_format *format)
{
- return check_format(sd, format) ? :
+ return check_format(sd, cfg, format) ? :
sd->ops->pad->set_fmt(sd, cfg, format);
}
@@ -171,6 +181,7 @@ static int check_enum_mbus_code(struct v4l2_subdev *sd,
return -EINVAL;
return check_which(code->which) ? : check_pad(sd, code->pad) ? :
+ check_cfg(code->which, cfg) ? :
sd->ops->pad->enum_mbus_code(sd, cfg, code);
}
@@ -182,6 +193,7 @@ static int check_enum_frame_size(struct v4l2_subdev *sd,
return -EINVAL;
return check_which(fse->which) ? : check_pad(sd, fse->pad) ? :
+ check_cfg(fse->which, cfg) ? :
sd->ops->pad->enum_frame_size(sd, cfg, fse);
}
@@ -216,23 +228,26 @@ static int check_enum_frame_interval(struct v4l2_subdev
*sd,
return -EINVAL;
return check_which(fie->which) ? : check_pad(sd, fie->pad) ? :
+ check_cfg(fie->which, cfg) ? :
sd->ops->pad->enum_frame_interval(sd, cfg, fie);
}
static int check_selection(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_selection *sel)
{
if (WARN_ON(!sel))
return -EINVAL;
- return check_which(sel->which) ? : check_pad(sd, sel->pad);
+ return check_which(sel->which) ? : check_pad(sd, sel->pad) ? :
+ check_cfg(sel->which, cfg);
}
static int check_get_selection(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_selection *sel)
{
- return check_selection(sd, sel) ? :
+ return check_selection(sd, cfg, sel) ? :
sd->ops->pad->get_selection(sd, cfg, sel);
}
@@ -240,7 +255,7 @@ static int check_set_selection(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_selection *sel)
{
- return check_selection(sd, sel) ? :
+ return check_selection(sd, cfg, sel) ? :
sd->ops->pad->set_selection(sd, cfg, sel);
}
--
2.21.0
[PATCH v5 1/4] media: v4l2-subdev: Verify arguments in v4l2_subdev_call()
Correctness of format type (try or active) and pad ID parameters passed
to subdevice operation callbacks is now verified only for IOCTL calls.
However, those callbacks are also used by drivers, e.g., V4L2 host
interfaces.
Since both subdev_do_ioctl() and drivers are using v4l2_subdev_call()
macro while calling subdevice operations, move those parameter checks
from subdev_do_ioctl() to v4l2_subdev_call() so we can avoid taking care
of those checks inside drivers.
Define a wrapper function for each operation callback in scope, then
gather those wrappers in a static v4l2_subdev_ops structure so the
v4l2_subdev_call() macro can find them easy if provided. Move reusable
code to helpers.
Signed-off-by: Janusz Krzysztofik
---
drivers/media/v4l2-core/v4l2-subdev.c | 227 --
include/media/v4l2-subdev.h | 6 +
2 files changed, 143 insertions(+), 90 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
b/drivers/media/v4l2-core/v4l2-subdev.c
index d75815ab0d7b..890916674d42 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -120,56 +120,168 @@ static int subdev_close(struct file *file)
return 0;
}
-#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
-static int check_format(struct v4l2_subdev *sd,
- struct v4l2_subdev_format *format)
+static int check_which(__u32 which)
{
- if (format->which != V4L2_SUBDEV_FORMAT_TRY &&
- format->which != V4L2_SUBDEV_FORMAT_ACTIVE)
- return -EINVAL;
-
- if (format->pad >= sd->entity.num_pads)
+ if (which != V4L2_SUBDEV_FORMAT_TRY &&
+ which != V4L2_SUBDEV_FORMAT_ACTIVE)
return -EINVAL;
return 0;
}
-static int check_crop(struct v4l2_subdev *sd, struct v4l2_subdev_crop *crop)
+static int check_pad(struct v4l2_subdev *sd, __u32 pad)
{
- if (crop->which != V4L2_SUBDEV_FORMAT_TRY &&
- crop->which != V4L2_SUBDEV_FORMAT_ACTIVE)
+#if defined(CONFIG_MEDIA_CONTROLLER)
+ if (sd->entity.num_pads && pad >= sd->entity.num_pads)
return -EINVAL;
+#endif
+ return 0;
+}
- if (crop->pad >= sd->entity.num_pads)
- return -EINVAL;
+static int check_format(struct v4l2_subdev *sd,
+ struct v4l2_subdev_format *format)
+{
+ return check_which(format->which) ? : check_pad(sd, format->pad);
+}
- return 0;
+static int check_get_fmt(struct v4l2_subdev *sd,
+struct v4l2_subdev_pad_config *cfg,
+struct v4l2_subdev_format *format)
+{
+ return check_format(sd, format) ? :
+ sd->ops->pad->get_fmt(sd, cfg, format);
+}
+
+static int check_set_fmt(struct v4l2_subdev *sd,
+struct v4l2_subdev_pad_config *cfg,
+struct v4l2_subdev_format *format)
+{
+ return check_format(sd, format) ? :
+ sd->ops->pad->set_fmt(sd, cfg, format);
+}
+
+static int check_enum_mbus_code(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_mbus_code_enum *code)
+{
+ return check_which(code->which) ? : check_pad(sd, code->pad) ? :
+ sd->ops->pad->enum_mbus_code(sd, cfg, code);
+}
+
+static int check_enum_frame_size(struct v4l2_subdev *sd,
+struct v4l2_subdev_pad_config *cfg,
+struct v4l2_subdev_frame_size_enum *fse)
+{
+ return check_which(fse->which) ? : check_pad(sd, fse->pad) ? :
+ sd->ops->pad->enum_frame_size(sd, cfg, fse);
+}
+
+static int check_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_frame_interval *fi)
+{
+ return check_pad(sd, fi->pad);
+}
+
+static int check_g_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_frame_interval *fi)
+{
+ return check_frame_interval(sd, fi) ? :
+ sd->ops->video->g_frame_interval(sd, fi);
+}
+
+static int check_s_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_frame_interval *fi)
+{
+ return check_frame_interval(sd, fi) ? :
+ sd->ops->video->s_frame_interval(sd, fi);
+}
+
+static int check_enum_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_frame_interval_enum *fie)
+{
+ return check_which(fie->which) ? : check_pad(sd, fie->pad) ? :
+ sd->ops->pad->enum_frame_interval(sd, cfg, fie);
}
static int check_selection(struct v4l2_subdev *sd,
struct v4l2_subdev_selection *sel)
{
- if (s
[PATCH v5 0/4] media: v4l2-subdev: Verify arguments in v4l2_subdev_call()
Correctness of format type (try or active) and pad ID parameters passed to subdevice operation callbacks is now verified only for IOCTL calls. However, those callbacks are also used by drivers, e.g., V4L2 host interfaces. Since both subdev_do_ioctl() and drivers are using v4l2_subdev_call() macro while calling subdevice operations, move those parameter checks from subdev_do_ioctl() to v4l2_subdev_call(). Also, add check for non-NULL pointers, including pad config if V4L2_SUBDEV_FORMAT_tRY is requested. Since invalid arguments generally mean bugs, be noisy about them. Having that done, we can avoid taking care of those checks inside drivers. Janusz Krzysztofik (4): media: v4l2-subdev: Verify arguments in v4l2_subdev_call() media: v4l2-subdev: WARN_ON invalid v4l2_subdev_call() arguments media: v4l2-subdev: Verify v4l2_subdev_call() pointer arguments media: v4l2-subdev: Verify v4l2_subdev_call() pad config argument drivers/media/v4l2-core/v4l2-subdev.c | 257 +- include/media/v4l2-subdev.h | 6 + 2 files changed, 179 insertions(+), 84 deletions(-) Changelog: v4->v5: - a few coding style and code formatting changes, - require CONFIG_MEDIA_CONTROLLER, not CONFIG_VIDEO_V4L2_SUBDEV_API, for a valid pad ID check, - perform pad ID check only if at least one pad is configured so drivers which don't configure pads are not affected if built with CONFIG_MEDIA_CONTROLLER defined, - issue kernel warnings on invalid parameters (new patch - 2/4), - validate pointers before using them (new patch - 3/4). v3->v4: - fix 'struct' keyword missing from patch 2/2, - fix checkpatch reported style issue in patch 2/2 Sorry for that. v2->v3: - add patch 2/2 with pad config check, - adjust continuation line alignments in patch 1/2 to match those used in 2/2. v1->v2: - replace the horrible macro with a structure of wrapper functions; inspired by Hans' and Sakari's comments - thanks! -- 2.21.0
[PATCH v4 2/2] media: v4l2-subdev: Verify pad config argument
Extend parameter checks performed by v4l2_subdev_call() with a check for
a non-NULL pad config pointer if V4L2_SUBDEV_FORMAT_TRY format type is
requested so drivers don't need to care.
Signed-off-by: Janusz Krzysztofik
---
drivers/media/v4l2-core/v4l2-subdev.c | 30 +++
1 file changed, 21 insertions(+), 9 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
b/drivers/media/v4l2-core/v4l2-subdev.c
index cd50fcb86c47..4540d8abff5f 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -126,6 +126,11 @@ static inline int check_which(__u32 which)
which != V4L2_SUBDEV_FORMAT_ACTIVE ? -EINVAL : 0;
}
+static inline int check_cfg(__u32 which, struct v4l2_subdev_pad_config *cfg)
+{
+ return which == V4L2_SUBDEV_FORMAT_TRY && !cfg ? -EINVAL : 0;
+}
+
#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
static inline int check_pad(struct v4l2_subdev *sd, __u32 pad)
{
@@ -136,16 +141,18 @@ static inline int check_pad(struct v4l2_subdev *sd, __u32
pad)
#endif
static int check_format(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_format *format)
{
- return check_which(format->which) ? : check_pad(sd, format->pad);
+ return check_which(format->which) ? :
+ check_cfg(format->which, cfg) ? : check_pad(sd, format->pad);
}
static int check_get_fmt(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_format *format)
{
- return check_format(sd, format) ? :
+ return check_format(sd, cfg, format) ? :
sd->ops->pad->get_fmt(sd, cfg, format);
}
@@ -153,7 +160,7 @@ static int check_set_fmt(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_format *format)
{
- return check_format(sd, format) ? :
+ return check_format(sd, cfg, format) ? :
sd->ops->pad->set_fmt(sd, cfg, format);
}
@@ -161,7 +168,8 @@ static int check_enum_mbus_code(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_mbus_code_enum *code)
{
- return check_which(code->which) ? : check_pad(sd, code->pad) ? :
+ return check_which(code->which) ? : check_cfg(code->which, cfg) ? :
+ check_pad(sd, code->pad) ? :
sd->ops->pad->enum_mbus_code(sd, cfg, code);
}
@@ -169,7 +177,8 @@ static int check_enum_frame_size(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_frame_size_enum *fse)
{
- return check_which(fse->which) ? : check_pad(sd, fse->pad) ? :
+ return check_which(fse->which) ? : check_cfg(fse->which, cfg) ? :
+ check_pad(sd, fse->pad) ? :
sd->ops->pad->enum_frame_size(sd, cfg, fse);
}
@@ -197,21 +206,24 @@ static int check_enum_frame_interval(struct v4l2_subdev
*sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_frame_interval_enum *fie)
{
- return check_which(fie->which) ? : check_pad(sd, fie->pad) ? :
+ return check_which(fie->which) ? : check_cfg(fie->which, cfg) ? :
+ check_pad(sd, fie->pad) ? :
sd->ops->pad->enum_frame_interval(sd, cfg, fie);
}
static int check_selection(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_selection *sel)
{
- return check_which(sel->which) ? : check_pad(sd, sel->pad);
+ return check_which(sel->which) ? : check_cfg(sel->which, cfg) ? :
+ check_pad(sd, sel->pad);
}
static int check_get_selection(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_selection *sel)
{
- return check_selection(sd, sel) ? :
+ return check_selection(sd, cfg, sel) ? :
sd->ops->pad->get_selection(sd, cfg, sel);
}
@@ -219,7 +231,7 @@ static int check_set_selection(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_selection *sel)
{
- return check_selection(sd, sel) ? :
+ return check_selection(sd, cfg, sel) ? :
sd->ops->pad->set_selection(sd, cfg, sel);
}
--
2.21.0
[PATCH v4 0/2] media: v4l2-subdev: Verify arguments of v4l2_subdev_call()
Correctness of format type (try or active) and pad number parameters passed to subdevice operation callbacks is now verified only for IOCTL calls. However, those callbacks are also used by drivers, e.g., V4L2 host interfaces. Since both subdev_do_ioctl() and drivers are using v4l2_subdev_call() macro while calling subdevice operations, move those parameter checks from subdev_do_ioctl() to v4l2_subdev_call(). Also, add a check for a non-NULL pad config pointer if V4L2_SUBDEV_FORMAT_tRY is requested. This way we can avoid taking care of those checks inside drivers. Janusz Krzysztofik (2): media: v4l2-subdev: Verify arguments of v4l2_subdev_call() media: v4l2-subdev: Verify pad config argument drivers/media/v4l2-core/v4l2-subdev.c | 234 -- include/media/v4l2-subdev.h | 6 + 2 files changed, 151 insertions(+), 89 deletions(-) Changelog: v3->v4: - fix 'struct' keyword missing from patch 2/2, - fix checkpatch reported style issue in patch 2/2 Sorry for that. v2->v3: - add patch 2/2 with pad config check, - adjust continuation line alignments in patch 1/2 to match those used in 2/2. v1->v2: - replace the horrible macro with a structure of wrapper functions; inspired by Hans' and Sakari's comments - thanks! -- 2.21.0
[PATCH v4 1/2] media: v4l2-subdev: Verify arguments of v4l2_subdev_call()
Correctness of format type (try or active) and pad number parameters
passed to subdevice operation callbacks is now verified only for IOCTL
calls. However, those callbacks are also used by drivers, e.g., V4L2
host interfaces.
Since both subdev_do_ioctl() and drivers are using v4l2_subdev_call()
macro while calling subdevice operations, move those parameter checks
from subdev_do_ioctl() to v4l2_subdev_call() so we can avoid taking care
of those checks inside drivers.
Signed-off-by: Janusz Krzysztofik
---
drivers/media/v4l2-core/v4l2-subdev.c | 222 +++---
include/media/v4l2-subdev.h | 6 +
2 files changed, 139 insertions(+), 89 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
b/drivers/media/v4l2-core/v4l2-subdev.c
index d75815ab0d7b..cd50fcb86c47 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -120,56 +120,165 @@ static int subdev_close(struct file *file)
return 0;
}
+static inline int check_which(__u32 which)
+{
+ return which != V4L2_SUBDEV_FORMAT_TRY &&
+ which != V4L2_SUBDEV_FORMAT_ACTIVE ? -EINVAL : 0;
+}
+
#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
+static inline int check_pad(struct v4l2_subdev *sd, __u32 pad)
+{
+ return pad >= sd->entity.num_pads ? -EINVAL : 0;
+}
+#else
+#define check_pad(...) 0
+#endif
+
static int check_format(struct v4l2_subdev *sd,
struct v4l2_subdev_format *format)
{
- if (format->which != V4L2_SUBDEV_FORMAT_TRY &&
- format->which != V4L2_SUBDEV_FORMAT_ACTIVE)
- return -EINVAL;
+ return check_which(format->which) ? : check_pad(sd, format->pad);
+}
- if (format->pad >= sd->entity.num_pads)
- return -EINVAL;
+static int check_get_fmt(struct v4l2_subdev *sd,
+struct v4l2_subdev_pad_config *cfg,
+struct v4l2_subdev_format *format)
+{
+ return check_format(sd, format) ? :
+ sd->ops->pad->get_fmt(sd, cfg, format);
+}
- return 0;
+static int check_set_fmt(struct v4l2_subdev *sd,
+struct v4l2_subdev_pad_config *cfg,
+struct v4l2_subdev_format *format)
+{
+ return check_format(sd, format) ? :
+ sd->ops->pad->set_fmt(sd, cfg, format);
}
-static int check_crop(struct v4l2_subdev *sd, struct v4l2_subdev_crop *crop)
+static int check_enum_mbus_code(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_mbus_code_enum *code)
{
- if (crop->which != V4L2_SUBDEV_FORMAT_TRY &&
- crop->which != V4L2_SUBDEV_FORMAT_ACTIVE)
- return -EINVAL;
+ return check_which(code->which) ? : check_pad(sd, code->pad) ? :
+ sd->ops->pad->enum_mbus_code(sd, cfg, code);
+}
- if (crop->pad >= sd->entity.num_pads)
- return -EINVAL;
+static int check_enum_frame_size(struct v4l2_subdev *sd,
+struct v4l2_subdev_pad_config *cfg,
+struct v4l2_subdev_frame_size_enum *fse)
+{
+ return check_which(fse->which) ? : check_pad(sd, fse->pad) ? :
+ sd->ops->pad->enum_frame_size(sd, cfg, fse);
+}
- return 0;
+static int check_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_frame_interval *fi)
+{
+ return check_pad(sd, fi->pad);
+}
+
+static int check_g_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_frame_interval *fi)
+{
+ return check_frame_interval(sd, fi) ? :
+ sd->ops->video->g_frame_interval(sd, fi);
+}
+
+static int check_s_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_frame_interval *fi)
+{
+ return check_frame_interval(sd, fi) ? :
+ sd->ops->video->s_frame_interval(sd, fi);
+}
+
+static int check_enum_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_frame_interval_enum *fie)
+{
+ return check_which(fie->which) ? : check_pad(sd, fie->pad) ? :
+ sd->ops->pad->enum_frame_interval(sd, cfg, fie);
}
static int check_selection(struct v4l2_subdev *sd,
struct v4l2_subdev_selection *sel)
{
- if (sel->which != V4L2_SUBDEV_FORMAT_TRY &&
- sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
- return -EINVAL;
+ return check_which(sel->which) ? : check_pad(sd, sel->pad);
+}
- if (sel->pad >= sd->entity.num_pads)
- return -EINVAL;
+static int check_get_selection(struct v4l2_su
[PATCH v3 1/2] media: v4l2-subdev: Verify arguments of v4l2_subdev_call()
Correctness of format type (try or active) and pad number parameters
passed to subdevice operation callbacks is now verified only for IOCTL
calls. However, those callbacks are also used by drivers, e.g., V4L2
host interfaces.
Since both subdev_do_ioctl() and drivers are using v4l2_subdev_call()
macro while calling subdevice operations, move those parameter checks
from subdev_do_ioctl() to v4l2_subdev_call() so we can avoid taking
care of those checks inside drivers.
Signed-off-by: Janusz Krzysztofik
---
drivers/media/v4l2-core/v4l2-subdev.c | 222 +++---
include/media/v4l2-subdev.h | 6 +
2 files changed, 139 insertions(+), 89 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
b/drivers/media/v4l2-core/v4l2-subdev.c
index d75815ab0d7b..cd50fcb86c47 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -120,56 +120,165 @@ static int subdev_close(struct file *file)
return 0;
}
+static inline int check_which(__u32 which)
+{
+ return which != V4L2_SUBDEV_FORMAT_TRY &&
+ which != V4L2_SUBDEV_FORMAT_ACTIVE ? -EINVAL : 0;
+}
+
#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
+static inline int check_pad(struct v4l2_subdev *sd, __u32 pad)
+{
+ return pad >= sd->entity.num_pads ? -EINVAL : 0;
+}
+#else
+#define check_pad(...) 0
+#endif
+
static int check_format(struct v4l2_subdev *sd,
struct v4l2_subdev_format *format)
{
- if (format->which != V4L2_SUBDEV_FORMAT_TRY &&
- format->which != V4L2_SUBDEV_FORMAT_ACTIVE)
- return -EINVAL;
+ return check_which(format->which) ? : check_pad(sd, format->pad);
+}
- if (format->pad >= sd->entity.num_pads)
- return -EINVAL;
+static int check_get_fmt(struct v4l2_subdev *sd,
+struct v4l2_subdev_pad_config *cfg,
+struct v4l2_subdev_format *format)
+{
+ return check_format(sd, format) ? :
+ sd->ops->pad->get_fmt(sd, cfg, format);
+}
- return 0;
+static int check_set_fmt(struct v4l2_subdev *sd,
+struct v4l2_subdev_pad_config *cfg,
+struct v4l2_subdev_format *format)
+{
+ return check_format(sd, format) ? :
+ sd->ops->pad->set_fmt(sd, cfg, format);
}
-static int check_crop(struct v4l2_subdev *sd, struct v4l2_subdev_crop *crop)
+static int check_enum_mbus_code(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_mbus_code_enum *code)
{
- if (crop->which != V4L2_SUBDEV_FORMAT_TRY &&
- crop->which != V4L2_SUBDEV_FORMAT_ACTIVE)
- return -EINVAL;
+ return check_which(code->which) ? : check_pad(sd, code->pad) ? :
+ sd->ops->pad->enum_mbus_code(sd, cfg, code);
+}
- if (crop->pad >= sd->entity.num_pads)
- return -EINVAL;
+static int check_enum_frame_size(struct v4l2_subdev *sd,
+struct v4l2_subdev_pad_config *cfg,
+struct v4l2_subdev_frame_size_enum *fse)
+{
+ return check_which(fse->which) ? : check_pad(sd, fse->pad) ? :
+ sd->ops->pad->enum_frame_size(sd, cfg, fse);
+}
- return 0;
+static int check_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_frame_interval *fi)
+{
+ return check_pad(sd, fi->pad);
+}
+
+static int check_g_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_frame_interval *fi)
+{
+ return check_frame_interval(sd, fi) ? :
+ sd->ops->video->g_frame_interval(sd, fi);
+}
+
+static int check_s_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_frame_interval *fi)
+{
+ return check_frame_interval(sd, fi) ? :
+ sd->ops->video->s_frame_interval(sd, fi);
+}
+
+static int check_enum_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_frame_interval_enum *fie)
+{
+ return check_which(fie->which) ? : check_pad(sd, fie->pad) ? :
+ sd->ops->pad->enum_frame_interval(sd, cfg, fie);
}
static int check_selection(struct v4l2_subdev *sd,
struct v4l2_subdev_selection *sel)
{
- if (sel->which != V4L2_SUBDEV_FORMAT_TRY &&
- sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
- return -EINVAL;
+ return check_which(sel->which) ? : check_pad(sd, sel->pad);
+}
- if (sel->pad >= sd->entity.num_pads)
- return -EINVAL;
+static int check_get_selection(struct v4l2_su
[PATCH v3 2/2] media: v4l2-subdev: Verify pad config argument
Extend parameter checks performed by v4l2_subdev_call() with a check for
a non-NULL pad config pointer if V4L2_SUBDEV_FORMAT_TRY format type is
requested so drivers don't need to care.
Signed-off-by: Janusz Krzysztofik
---
drivers/media/v4l2-core/v4l2-subdev.c | 30 +++
1 file changed, 21 insertions(+), 9 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
b/drivers/media/v4l2-core/v4l2-subdev.c
index cd50fcb86c47..52eb23112804 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -126,6 +126,11 @@ static inline int check_which(__u32 which)
which != V4L2_SUBDEV_FORMAT_ACTIVE ? -EINVAL : 0;
}
+static inline int check_cfg(__u32 which, v4l2_subdev_pad_config *cfg)
+{
+ return which == V4L2_SUBDEV_FORMAT_TRY && !cfg ? -EINVAL : 0;
+}
+
#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
static inline int check_pad(struct v4l2_subdev *sd, __u32 pad)
{
@@ -136,16 +141,18 @@ static inline int check_pad(struct v4l2_subdev *sd, __u32
pad)
#endif
static int check_format(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_format *format)
{
- return check_which(format->which) ? : check_pad(sd, format->pad);
+ return check_which(format->which) ? :
+ check_cfg(format->which, cfg) ? : check_pad(sd, format->pad);
}
static int check_get_fmt(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_format *format)
{
- return check_format(sd, format) ? :
+ return check_format(sd, cfg, format) ? :
sd->ops->pad->get_fmt(sd, cfg, format);
}
@@ -153,7 +160,7 @@ static int check_set_fmt(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_format *format)
{
- return check_format(sd, format) ? :
+ return check_format(sd, cfg, format) ? :
sd->ops->pad->set_fmt(sd, cfg, format);
}
@@ -161,7 +168,8 @@ static int check_enum_mbus_code(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_mbus_code_enum *code)
{
- return check_which(code->which) ? : check_pad(sd, code->pad) ? :
+ return check_which(code->which) ? : check_cfg(code->which, cfg) ? :
+ check_pad(sd, code->pad) ? :
sd->ops->pad->enum_mbus_code(sd, cfg, code);
}
@@ -169,7 +177,8 @@ static int check_enum_frame_size(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_frame_size_enum *fse)
{
- return check_which(fse->which) ? : check_pad(sd, fse->pad) ? :
+ return check_which(fse->which) ? : check_cfg(fse->which, cfg) ? :
+ check_pad(sd, fse->pad) ? :
sd->ops->pad->enum_frame_size(sd, cfg, fse);
}
@@ -197,21 +206,24 @@ static int check_enum_frame_interval(struct v4l2_subdev
*sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_frame_interval_enum *fie)
{
- return check_which(fie->which) ? : check_pad(sd, fie->pad) ? :
+ return check_which(fie->which) ? : check_cfg(fie->which, cfg) ? :
+ check_pad(sd, fie->pad) ? :
sd->ops->pad->enum_frame_interval(sd, cfg, fie);
}
static int check_selection(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_selection *sel)
{
- return check_which(sel->which) ? : check_pad(sd, sel->pad);
+ return check_which(sel->which) ? :check_cfg(sel->which, cfg) ? :
+ check_pad(sd, sel->pad);
}
static int check_get_selection(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_selection *sel)
{
- return check_selection(sd, sel) ? :
+ return check_selection(sd, cfg, sel) ? :
sd->ops->pad->get_selection(sd, cfg, sel);
}
@@ -219,7 +231,7 @@ static int check_set_selection(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_selection *sel)
{
- return check_selection(sd, sel) ? :
+ return check_selection(sd, cfg, sel) ? :
sd->ops->pad->set_selection(sd, cfg, sel);
}
--
2.21.0
[PATCH v3 0/2] media: v4l2-subdev: Verify arguments of v4l2_subdev_call()
Correctness of format type (try or active) and pad number parameters passed to subdevice operation callbacks is now verified only for IOCTL calls. However, those callbacks are also used by drivers, e.g., V4L2 host interfaces. Since both subdev_do_ioctl() and drivers are using v4l2_subdev_call() macro while calling subdevice operations, move those parameter checks from subdev_do_ioctl() to v4l2_subdev_call(). Also, add a check for a non-NULL pad config pointer if V4L2_SUBDEV_FORMAT_tRY is requested. This way we can avoid taking care of those checks inside drivers. Janusz Krzysztofik (2): media: v4l2-subdev: Verify arguments of v4l2_subdev_call() media: v4l2-subdev: Verify pad config argument drivers/media/v4l2-core/v4l2-subdev.c | 234 -- include/media/v4l2-subdev.h | 6 + 2 files changed, 151 insertions(+), 89 deletions(-) Changelog: v2->v3: - add patch 2/2 with pad config check, - adjust continuation line alignments in patch 1/2 to match those used in 2/2. v1->v2: - replace the horrible macro with a structure of wrapper functions; inspired by Hans' and Sakari's comments - thanks! -- 2.21.0
[PATCH v2] media: v4l2-subdev: Verify arguments of v4l2_subdev_call()
Correctness of format type (try or active) and pad number parameters
passed to subdevice operation callbacks is now verified only for IOCTL
calls. However, those callbacks are also used by drivers, e.g., V4L2
host interfaces.
Since both subdev_do_ioctl() and drivers are using v4l2_subdev_call()
macro while calling subdevice operations, move those parameter checks
from subdev_do_ioctl() to v4l2_subdev_call() so we can avoid
reimplementing those checks inside drivers.
Signed-off-by: Janusz Krzysztofik
---
Changelog:
v1->v2:
- replace the horrible macro with a structure of wrapper functions;
inspired by Hans' and Sakari's comments - thanks!
drivers/media/v4l2-core/v4l2-subdev.c | 222 +++---
include/media/v4l2-subdev.h | 6 +
2 files changed, 139 insertions(+), 89 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
b/drivers/media/v4l2-core/v4l2-subdev.c
index d75815ab0d7b..213194660d52 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -120,56 +120,165 @@ static int subdev_close(struct file *file)
return 0;
}
+static inline int check_which(__u32 which)
+{
+ return which != V4L2_SUBDEV_FORMAT_TRY &&
+ which != V4L2_SUBDEV_FORMAT_ACTIVE ? -EINVAL : 0;
+}
+
#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
+static inline int check_pad(struct v4l2_subdev *sd, __u32 pad)
+{
+ return pad >= sd->entity.num_pads ? -EINVAL : 0;
+}
+#else
+#define check_pad(...) 0
+#endif
+
static int check_format(struct v4l2_subdev *sd,
struct v4l2_subdev_format *format)
{
- if (format->which != V4L2_SUBDEV_FORMAT_TRY &&
- format->which != V4L2_SUBDEV_FORMAT_ACTIVE)
- return -EINVAL;
+ return check_which(format->which) ? : check_pad(sd, format->pad);
+}
- if (format->pad >= sd->entity.num_pads)
- return -EINVAL;
+static int check_get_fmt(struct v4l2_subdev *sd,
+struct v4l2_subdev_pad_config *cfg,
+struct v4l2_subdev_format *format)
+{
+ return check_format(sd, format) ? :
+ sd->ops->pad->get_fmt(sd, cfg, format);
+}
- return 0;
+static int check_set_fmt(struct v4l2_subdev *sd,
+struct v4l2_subdev_pad_config *cfg,
+struct v4l2_subdev_format *format)
+{
+ return check_format(sd, format) ? :
+ sd->ops->pad->set_fmt(sd, cfg, format);
}
-static int check_crop(struct v4l2_subdev *sd, struct v4l2_subdev_crop *crop)
+static int check_enum_mbus_code(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_mbus_code_enum *code)
{
- if (crop->which != V4L2_SUBDEV_FORMAT_TRY &&
- crop->which != V4L2_SUBDEV_FORMAT_ACTIVE)
- return -EINVAL;
+ return check_which(code->which) ? : check_pad(sd, code->pad) ? :
+ sd->ops->pad->enum_mbus_code(sd, cfg, code);
+}
- if (crop->pad >= sd->entity.num_pads)
- return -EINVAL;
+static int check_enum_frame_size(struct v4l2_subdev *sd,
+struct v4l2_subdev_pad_config *cfg,
+struct v4l2_subdev_frame_size_enum *fse)
+{
+ return check_which(fse->which) ? : check_pad(sd, fse->pad) ? :
+ sd->ops->pad->enum_frame_size(sd, cfg, fse);
+}
- return 0;
+static int check_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_frame_interval *fi)
+{
+ return check_pad(sd, fi->pad);
+}
+
+static int check_g_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_frame_interval *fi)
+{
+ return check_frame_interval(sd, fi) ? :
+ sd->ops->video->g_frame_interval(sd, fi);
+}
+
+static int check_s_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_frame_interval *fi)
+{
+ return check_frame_interval(sd, fi) ? :
+ sd->ops->video->s_frame_interval(sd, fi);
+}
+
+static int check_enum_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_frame_interval_enum *fie)
+{
+ return check_which(fie->which) ? : check_pad(sd, fie->pad) ? :
+ sd->ops->pad->enum_frame_interval(sd, cfg, fie);
}
static int check_selection(struct v4l2_subdev *sd,
struct v4l2_subdev_selection *sel)
{
- if (sel->which != V4L2_SUBDEV_FORMAT_TRY &&
- sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
-
Re: [PATCH 03/14] media: ov6650: Fix unverified arguments used in .set_fmt()
Hi Sakari,
Sorry for late answer, I've just found your message in Gmail spam folder.
On Tuesday, April 30, 2019 3:58:09 PM CEST Sakari Ailus wrote:
> Hi Janusz,
>
> On Mon, Apr 08, 2019 at 11:42:31PM +0200, Janusz Krzysztofik wrote:
> > Commit 717fd5b4907ad ("[media] v4l2: replace try_mbus_fmt by set_fmt")
> > converted a former ov6650_try_fmt() video operation callback to an
> > ov6650_set_fmt() pad operation callback. However, the function does not
> > verify correctness of user provided format->which flag and pad config
> > pointer arguments. Fix it.
> >
> > Fixes: 717fd5b4907ad ("[media] v4l2: replace try_mbus_fmt by set_fmt")
> > Signed-off-by: Janusz Krzysztofik
> > Cc: sta...@vger.kernel.org
> > ---
> > drivers/media/i2c/ov6650.c | 11 +++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
> > index 007f0ca24913..3062c9a6c57b 100644
> > --- a/drivers/media/i2c/ov6650.c
> > +++ b/drivers/media/i2c/ov6650.c
> > @@ -679,6 +679,17 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
> > if (format->pad)
> > return -EINVAL;
> >
> > + switch (format->which) {
> > + case V4L2_SUBDEV_FORMAT_ACTIVE:
> > + break;
> > + case V4L2_SUBDEV_FORMAT_TRY:
> > + if (cfg)
> > + break;
> > + /* fall through */
> > + default:
> > + return -EINVAL;
> > + }
>
> For this to return an error, there would need to be a problem on the
> caller's side. In other words, this isn't supposed to happen.
How about raising a bug if that happens nevertheless?
@@ -677,10 +677,20 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
struct ov6650 *priv = to_ov6650(client);
if (format->pad)
return -EINVAL;
+ switch (format->which) {
+ case V4L2_SUBDEV_FORMAT_TRY:
+ BUG_ON(!cfg);
+ /* fall through */
+ case V4L2_SUBDEV_FORMAT_ACTIVE:
+ break;
+ default:
+ BUG();
+ }
+
if (is_unscaled_ok(mf->width, mf->height, &priv->rect))
v4l_bound_align_image(&mf->width, 2, W_CIF, 1,
&mf->height, 2, H_CIF, 1, 0);
mf->field = V4L2_FIELD_NONE;
Thanks,
Janusz
>
> Instead of adding such checks to all drivers, I think they instead should
> be added to the caller's side. The checks already exist for uAPI, but not
> for other drivers.
>
> The same applies to patches until 7th (including).
>
> > +
> > if (is_unscaled_ok(mf->width, mf->height, &priv->rect))
> > v4l_bound_align_image(&mf->width, 2, W_CIF, 1,
> > &mf->height, 2, H_CIF, 1, 0);
>
>
Re: [PATCH v3] mtd: rawnand: ams-delta: Drop board specific partition info
Hi Ladislav,
On Thursday, April 25, 2019 12:14:28 AM CEST Ladislav Michl wrote:
> Hi Janusz,
>
> On Wed, Apr 24, 2019 at 08:02:12PM +0200, Janusz Krzysztofik wrote:
> > After recent modifications, only a hardcoded partition info makes
> > the driver device specific. Other than that, the driver uses GPIO
> > exclusively and can be used on any hardware.
> >
> > Drop the partition info and use MTD partition parser with default list
> > of parser names instead. For the OF parser to work correctly, pass
> > device of_node to mtd.
> >
> > Amstrad Delta users should append the following partition info to their
> > kernel command line, possibly embedding it in CONFIG_CMDLINE:
> >
> > mtdparts=ams-delta-nand:3584k(Kernel),256k(u-boot),256k(u-boot_params),\
> > 256k(Amstrad_LDR),27m(File_system),768k(PBL_reserved)
>
> now, when driver is no longer Amstrad Delta specific, why would you want
> to have ams-delta-nand hardcoded on kernel cmdline? I'm assuming at some
> point this driver will become gpio-nand [*] or something like that and
> asking users to change their kernel cmdline twice is just unwise :)
Hmm, I have no idea of a good name for the driver if not "gpio-nand". Can you
suggest one?
As a workaround, I can add a platform device id table to the driver with "ams-
delta-nand" as a supported device name in hope that survives possible future
driver renaming.
> [*] btw, it is really shame gpio-nand name is already taken by driver
> living in drivers/mtd/nand/raw/gpio.c which is more likely gpio-mem-nand
> used by at least CompuLab CM-X255 and Picochip picoXcell.
I think the best approach would be to expose NAND data ports of those machines
as GPIO ports, possibly reusing the "gpio-nand" driver code while creating a
new GPIO driver for them if "basic-mmio-gpio" occurs inappropriate, and use
the pure GPIO NAND driver on top.
> Otherwise your work on this driver is so amazing that I just spent
> couple of hours finding that phone and compiling some decent userspace
> for it :) Thank you!
I'm glad you like it :-)
Janusz
>
> > For their convenience, that information is added to the board Kconfig
> > entry help text, as well as CONFIG_MTD_CMDLINE_PARTS symbol is selected
> > automatically from the board Kconfig entry if the NAND driver is also
> > selected.
> >
> > Signed-off-by: Janusz Krzysztofik
> > Cc: Tony Lindgren
> > Cc: Aaro Koskinen
> > ---
> > Changelog:
> > v2->v3:
> > - add information on the requirement for passing partition info via
> > kernel command line to the board Kconfig entry help text
> > v1->v2:
> > - fix a typo poitned out by Aaro - thanks!
> > - fix device_node not passed to OF parser via mtd_info
> > - commit message reworded and reformatted a bit for better readability
> >
> > arch/arm/mach-omap1/Kconfig | 7 ++-
> > drivers/mtd/nand/raw/ams-delta.c | 29 ++---
> > 2 files changed, 8 insertions(+), 28 deletions(-)
> >
> > diff --git a/arch/arm/mach-omap1/Kconfig b/arch/arm/mach-omap1/Kconfig
> > index c4694f26b5c4..41a47d251cac 100644
> > --- a/arch/arm/mach-omap1/Kconfig
> > +++ b/arch/arm/mach-omap1/Kconfig
> > @@ -164,17 +164,22 @@ config MACH_NOKIA770
> > have such a device.
> >
> > config MACH_AMS_DELTA
> > - bool "Amstrad E3 (Delta)"
> > + bool "Amstrad E3 (Delta) - see help for important information"
> > depends on ARCH_OMAP1 && ARCH_OMAP15XX
> > select FIQ
> > select GPIO_GENERIC_PLATFORM
> > select LEDS_GPIO_REGISTER
> > select REGULATOR
> > select REGULATOR_FIXED_VOLTAGE
> > + select MTD_CMDLINE_PARTS if MTD_NAND_AMS_DELTA
> > help
> > Support for the Amstrad E3 (codename Delta) videophone. Say Y
here
> > if you have such a device.
> >
> > + If you are using built-in NAND, append the following partition
> > + info to kernel command line:
> > + mtdparts=ams-delta-nand:3584k(Kernel),256k(u-boot),256k(u-
boot_params),256k(Amstrad_LDR),27m(File_system),768k(PBL_reserved)
> > +
> > config MACH_OMAP_GENERIC
> > bool "Generic OMAP board"
> > depends on ARCH_OMAP1 && (ARCH_OMAP15XX || ARCH_OMAP16XX)
> > diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-
delta.c
> > index 8312182088c1..e0f09179bbda 100644
> > --- a/drivers/mtd/nand/raw/ams-delta.c
> > +++ b/drivers/mtd/nand/raw/ams-delta.c
> > @@ -41,31 +41,6 @@ struct ams_delta_nand {
> > bool
[PATCH v3] mtd: rawnand: ams-delta: Drop board specific partition info
After recent modifications, only a hardcoded partition info makes
the driver device specific. Other than that, the driver uses GPIO
exclusively and can be used on any hardware.
Drop the partition info and use MTD partition parser with default list
of parser names instead. For the OF parser to work correctly, pass
device of_node to mtd.
Amstrad Delta users should append the following partition info to their
kernel command line, possibly embedding it in CONFIG_CMDLINE:
mtdparts=ams-delta-nand:3584k(Kernel),256k(u-boot),256k(u-boot_params),\
256k(Amstrad_LDR),27m(File_system),768k(PBL_reserved)
For their convenience, that information is added to the board Kconfig
entry help text, as well as CONFIG_MTD_CMDLINE_PARTS symbol is selected
automatically from the board Kconfig entry if the NAND driver is also
selected.
Signed-off-by: Janusz Krzysztofik
Cc: Tony Lindgren
Cc: Aaro Koskinen
---
Changelog:
v2->v3:
- add information on the requirement for passing partition info via
kernel command line to the board Kconfig entry help text
v1->v2:
- fix a typo poitned out by Aaro - thanks!
- fix device_node not passed to OF parser via mtd_info
- commit message reworded and reformatted a bit for better readability
arch/arm/mach-omap1/Kconfig | 7 ++-
drivers/mtd/nand/raw/ams-delta.c | 29 ++---
2 files changed, 8 insertions(+), 28 deletions(-)
diff --git a/arch/arm/mach-omap1/Kconfig b/arch/arm/mach-omap1/Kconfig
index c4694f26b5c4..41a47d251cac 100644
--- a/arch/arm/mach-omap1/Kconfig
+++ b/arch/arm/mach-omap1/Kconfig
@@ -164,17 +164,22 @@ config MACH_NOKIA770
have such a device.
config MACH_AMS_DELTA
- bool "Amstrad E3 (Delta)"
+ bool "Amstrad E3 (Delta) - see help for important information"
depends on ARCH_OMAP1 && ARCH_OMAP15XX
select FIQ
select GPIO_GENERIC_PLATFORM
select LEDS_GPIO_REGISTER
select REGULATOR
select REGULATOR_FIXED_VOLTAGE
+ select MTD_CMDLINE_PARTS if MTD_NAND_AMS_DELTA
help
Support for the Amstrad E3 (codename Delta) videophone. Say Y here
if you have such a device.
+ If you are using built-in NAND, append the following partition
+ info to kernel command line:
+
mtdparts=ams-delta-nand:3584k(Kernel),256k(u-boot),256k(u-boot_params),256k(Amstrad_LDR),27m(File_system),768k(PBL_reserved)
+
config MACH_OMAP_GENERIC
bool "Generic OMAP board"
depends on ARCH_OMAP1 && (ARCH_OMAP15XX || ARCH_OMAP16XX)
diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c
index 8312182088c1..e0f09179bbda 100644
--- a/drivers/mtd/nand/raw/ams-delta.c
+++ b/drivers/mtd/nand/raw/ams-delta.c
@@ -41,31 +41,6 @@ struct ams_delta_nand {
booldata_in;
};
-/*
- * Define partitions for flash devices
- */
-
-static const struct mtd_partition partition_info[] = {
- { .name = "Kernel",
- .offset = 0,
- .size = 3 * SZ_1M + SZ_512K },
- { .name = "u-boot",
- .offset = 3 * SZ_1M + SZ_512K,
- .size = SZ_256K },
- { .name = "u-boot params",
- .offset = 3 * SZ_1M + SZ_512K + SZ_256K,
- .size = SZ_256K },
- { .name = "Amstrad LDR",
- .offset = 4 * SZ_1M,
- .size = SZ_256K },
- { .name = "File system",
- .offset = 4 * SZ_1M + 1 * SZ_256K,
- .size = 27 * SZ_1M },
- { .name = "PBL reserved",
- .offset = 32 * SZ_1M - 3 * SZ_256K,
- .size = 3 * SZ_256K },
-};
-
static void ams_delta_write_commit(struct ams_delta_nand *priv)
{
gpiod_set_value(priv->gpiod_nwe, 0);
@@ -238,6 +213,7 @@ static int ams_delta_init(struct platform_device *pdev)
mtd->dev.parent = &pdev->dev;
nand_set_controller_data(this, priv);
+ nand_set_flash_node(this, pdev->dev.of_node);
priv->gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", GPIOD_IN);
if (IS_ERR(priv->gpiod_rdy)) {
@@ -315,8 +291,7 @@ static int ams_delta_init(struct platform_device *pdev)
return err;
/* Register the partitions */
- err = mtd_device_register(mtd, partition_info,
- ARRAY_SIZE(partition_info));
+ err = mtd_device_parse_register(mtd, NULL, NULL, NULL, 0);
if (err)
goto err_nand_cleanup;
--
2.21.0
Re: [PATCH v2] mtd: rawnand: ams-delta: Drop board specific partition info
Hi Miquèl, I'm wondering if we could register up a custom mtd partition parser from the board file. It could return a pointer to a statically defined table. Then we could add its name to the list of parsers the driver is going to try if the board is enabled in .config. Would that be acceptable from the MTD subsystem point of view? Thanks, Janusz On Thursday, April 18, 2019 8:49:29 AM CEST Miquel Raynal wrote: > Hi Janusz, > > Janusz Krzysztofik wrote on Thu, 18 Apr 2019 > 01:09:59 +0200: > > > Hi Aaro, Tony, > > > > On Wednesday, April 17, 2019 11:40:10 AM CEST Miquel Raynal wrote: > > > Hi Janusz, > > > > > > Janusz Krzysztofik wrote on Sun, 24 Mar 2019 > > > 23:33:44 +0100: > > > > > > > After recent modifications, only a hardcoded partition info makes > > > > the driver device specific. Other than that, the driver uses GPIO > > > > exclusively and can be used on any hardware. > > > > > > > > Drop the partition info and use MTD partition parser with default list > > > > of parser names instead. For the OF parser to work correctly, pass > > > > device of_node to mtd. > > > > > > > > Amstrad Delta users should append the following partition info to their > > > > kernel command line, possibly by embedding it in CONFIG_CMDLINE: > > > > > > > > mtdparts=ams-delta-nand:3584k(Kernel),256k(u-boot),256k(u- boot_params),\ > > > > 256k(Amstrad_LDR),27m(File_system),768k(PBL_reserved). > > > > > > > > For their convenience, CONFIG_MTD_CMDLINE_PARTS symbol is selected > > > > automatically from that board Kconfig if this NAND driver is also > > > > selected. > > > > > > > > Signed-off-by: Janusz Krzysztofik > > > > Cc: Tony Lindgren > > > > --- > > > > > > FYI I am okay with the change but I am waiting for acks before applying > > > it. > > > > May we have an ack from you? > > > > If still not convinced with my clarifications, I can add a comment to help text > > in Kconfig, either squashed or in a follow up patch, on the requirement of > > appending mtdparts parameter to command line. What do you think? > > In the same patch I guess that would be fine. > > Thanks, > Miquèl >
Re: [PATCH v2] mtd: rawnand: ams-delta: Drop board specific partition info
Hi Aaro, Tony, On Wednesday, April 17, 2019 11:40:10 AM CEST Miquel Raynal wrote: > Hi Janusz, > > Janusz Krzysztofik wrote on Sun, 24 Mar 2019 > 23:33:44 +0100: > > > After recent modifications, only a hardcoded partition info makes > > the driver device specific. Other than that, the driver uses GPIO > > exclusively and can be used on any hardware. > > > > Drop the partition info and use MTD partition parser with default list > > of parser names instead. For the OF parser to work correctly, pass > > device of_node to mtd. > > > > Amstrad Delta users should append the following partition info to their > > kernel command line, possibly by embedding it in CONFIG_CMDLINE: > > > > mtdparts=ams-delta-nand:3584k(Kernel),256k(u-boot),256k(u-boot_params),\ > > 256k(Amstrad_LDR),27m(File_system),768k(PBL_reserved). > > > > For their convenience, CONFIG_MTD_CMDLINE_PARTS symbol is selected > > automatically from that board Kconfig if this NAND driver is also > > selected. > > > > Signed-off-by: Janusz Krzysztofik > > Cc: Tony Lindgren > > --- > > FYI I am okay with the change but I am waiting for acks before applying > it. May we have an ack from you? If still not convinced with my clarifications, I can add a comment to help text in Kconfig, either squashed or in a follow up patch, on the requirement of appending mtdparts parameter to command line. What do you think? Thanks, Janusz > > Thanks, > Miquèl >
[PATCH 09/14] media: ov6650: Fix incorrect use of JPEG colorspace
Since its initial submission, the driver selects V4L2_COLORSPACE_JPEG
for supported formats other than V4L2_MBUS_FMT_SBGGR8_1X8. According
to v4l2-compliance test program, V4L2_COLORSPACE_JPEG applies
exclusively to V4L2_PIX_FMT_JPEG. Since the sensor does not support
JPEG format, fix it to always select V4L2_COLORSPACE_SRGB.
Fixes: 2f6e2404799a ("[media] SoC Camera: add driver for OV6650 sensor")
Signed-off-by: Janusz Krzysztofik
Cc: sta...@vger.kernel.org
---
drivers/media/i2c/ov6650.c | 13 ++---
1 file changed, 2 insertions(+), 11 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index 07d7a0708cca..dfee07ec976e 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -203,7 +203,6 @@ struct ov6650 {
unsigned long pclk_max; /* from resolution and format */
struct v4l2_fract tpf;/* as requested with
s_frame_interval */
u32 code;
- enum v4l2_colorspacecolorspace;
};
@@ -534,7 +533,7 @@ static int ov6650_get_fmt(struct v4l2_subdev *sd,
mf->width = priv->rect.width >> priv->half_scale;
mf->height = priv->rect.height >> priv->half_scale;
mf->code= priv->code;
- mf->colorspace = priv->colorspace;
+ mf->colorspace = V4L2_COLORSPACE_SRGB;
mf->field = V4L2_FIELD_NONE;
return 0;
@@ -642,11 +641,6 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct
v4l2_mbus_framefmt *mf)
priv->pclk_max = 800;
}
- if (code == MEDIA_BUS_FMT_SBGGR8_1X8)
- priv->colorspace = V4L2_COLORSPACE_SRGB;
- else if (code != 0)
- priv->colorspace = V4L2_COLORSPACE_JPEG;
-
if (half_scale) {
dev_dbg(&client->dev, "max resolution: QCIF\n");
coma_set |= COMA_QCIF;
@@ -677,7 +671,6 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct
v4l2_mbus_framefmt *mf)
ret = ov6650_reg_rmw(client, REG_COML, coml_set, coml_mask);
if (!ret) {
- mf->colorspace = priv->colorspace;
mf->width = priv->rect.width >> half_scale;
mf->height = priv->rect.height >> half_scale;
}
@@ -711,6 +704,7 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
&mf->height, 2, H_CIF, 1, 0);
mf->field = V4L2_FIELD_NONE;
+ mf->colorspace = V4L2_COLORSPACE_SRGB;
switch (mf->code) {
case MEDIA_BUS_FMT_Y10_1X10:
@@ -721,13 +715,11 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
case MEDIA_BUS_FMT_YUYV8_2X8:
case MEDIA_BUS_FMT_VYUY8_2X8:
case MEDIA_BUS_FMT_UYVY8_2X8:
- mf->colorspace = V4L2_COLORSPACE_JPEG;
break;
default:
mf->code = MEDIA_BUS_FMT_SBGGR8_1X8;
/* fall through */
case MEDIA_BUS_FMT_SBGGR8_1X8:
- mf->colorspace = V4L2_COLORSPACE_SRGB;
break;
}
@@ -1055,7 +1047,6 @@ static int ov6650_probe(struct i2c_client *client,
priv->rect.height = H_CIF;
priv->half_scale = false;
priv->code= MEDIA_BUS_FMT_YUYV8_2X8;
- priv->colorspace = V4L2_COLORSPACE_JPEG;
priv->subdev.internal_ops = &ov6650_internal_ops;
priv->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
--
2.21.0
[PATCH 12/14] media: ov6650: Fix default format not applied on device probe
It is not clear what pixel format is actually configured in hardware on
reset. MEDIA_BUS_FMT_YUYV8_2X8, assumed on device probe since the
driver was intiially submitted, is for sure not the one.
Fix it by explicitly applying a known, driver default frame format just
after initial device reset.
Fixes: 2f6e2404799a ("[media] SoC Camera: add driver for OV6650 sensor")
Signed-off-by: Janusz Krzysztofik
Cc: sta...@vger.kernel.org
---
drivers/media/i2c/ov6650.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index e72210653f55..8013afea0c02 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -920,6 +920,11 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
ret = ov6650_reset(client);
if (!ret)
ret = ov6650_prog_dflt(client);
+ if (!ret) {
+ struct v4l2_mbus_framefmt mf = ov6650_def_fmt;
+
+ ret = ov6650_s_fmt(sd, &mf);
+ }
if (!ret)
ret = v4l2_ctrl_handler_setup(&priv->hdl);
@@ -1074,8 +1079,6 @@ static int ov6650_probe(struct i2c_client *client,
priv->rect.top= DEF_VSTRT << 1;
priv->rect.width = W_CIF;
priv->rect.height = H_CIF;
- priv->half_scale = false;
- priv->code= MEDIA_BUS_FMT_YUYV8_2X8;
priv->subdev.internal_ops = &ov6650_internal_ops;
priv->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
--
2.21.0
Re: [PATCH] drm/i915: Use drm_dev_unplug()
On Fri, 2019-04-05 at 09:24 +0100, Chris Wilson wrote: > Quoting Janusz Krzysztofik (2019-04-05 09:11:54) > > On Fri, 2019-04-05 at 08:41 +0100, Chris Wilson wrote: > > > Quoting Janusz Krzysztofik (2019-04-05 08:26:57) > > > > From: Janusz Krzysztofik > > > > > > > > The driver does not currently support unbinding from a device > > > > which > > > > is > > > > in use. Since open file descriptors may still be pointing into > > > > kernel > > > > memory where the device structures used to be, entirely correct > > > > kernel > > > > panics protect the driver from being unbound as we should not > > > > be > > > > unbinding it before those dangling pointers have been made > > > > safe. > > > > > > > > According to the documentation found inside > > > > drivers/gpu/drm/drm_drv.c, > > > > drm_dev_unplug() should be used instead of drm_dev_unregister() > > > > in > > > > order to make a device inaccessible to users as soon as it is > > > > unpluged. > > > > Follow that advice to make those possibly dangling pointers > > > > safe, > > > > protected by DRM layer from a user who is otherwise left > > > > pointing > > > > into > > > > possibly reused kernel memory after the driver has been unbound > > > > from > > > > the device. > > > > > > > > Signed-off-by: Janusz Krzysztofik > > > > > > > > --- > > > > drivers/gpu/drm/i915/i915_drv.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > > > > b/drivers/gpu/drm/i915/i915_drv.c > > > > index 9df65d386d11..66163378c481 100644 > > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > > @@ -1596,7 +1596,7 @@ static void i915_driver_unregister(struct > > > > drm_i915_private *dev_priv) > > > > i915_pmu_unregister(dev_priv); > > > > > > > > i915_teardown_sysfs(dev_priv); > > > > - drm_dev_unregister(&dev_priv->drm); > > > > + drm_dev_unplug(&dev_priv->drm); > > > > > > I think we may have our onion inverted here. We want to stop the > > > users > > > as the first step, then start removing the entries. (That will > > > also > > > nicely invert the order from register, which is what we typically > > > expect). > > > > > > After calling i915_driver_unregister(); call > > > i915_gem_set_wedged() to > > > immediately (give or take external fences) cancel inflight > > > operations. > > > > OK, thanks. Do you prefer them squashed or as serparate patches? > > Quite happy to do the s/unregister/unplug/ and move in one go. Have a > pre-emptive > Reviewed-by: Chris Wilson > on that as that seems to be the right thing to do. > > And there should be no issues in placing a i915_gem_set_wedged() > immediately after the call to i915_driver_unregister, so if you > include > a line of commentary about why, for example > > /* > * After unregistering the device to prevent any new users, cancel > * all in-flight requests so that we can quickly unbind the active > * resources. > */ > i915_gem_set_wedged(dev_priv); > > Reviewed-by: Chris Wilson I've given it some testing, no side effects with test workloads I've tried, and looks like it at least helps to prevent from making the device actually wedged. With these two patches, plus the one we discussed yesterday, and yet another one I'm going to submit soon, I'm now able to unbind the driver from a device while a workload is running on it, unload the module, reload it and successfully perform basic GEM health checks, all in a quick succession :-). Unfortunately, not 100% reproducible, as well as not the case with device unplug simulated by writing 1 to device/remove sysfs file. Surely that needs the work you describe below to be done first. Thanks for your cooperation, Janusz > > I think overall though, we need to go through i915_driver_unload() > and > push the module cleanup operations to i915_driver_release -- that > will > take a bit of surgery to separate the different phases that are > currently smashed together. > -Chris > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 0/2] media: ov6650: Add V4L2 asynchronous subdevice support
Janusz Krzysztofik (2): media: ov6650: Move v4l2_clk_get() to ov6650_video_probe() helper media: ov6650: Register with asynchronous subdevice framework ov6650.c | 44 1 file changed, 28 insertions(+), 16 deletions(-) Changelog: v2->v3: - use v4l2_i2c_subdev_set_name() - thanks Sakari v1->v2: - labels should be outside conditionals - thanks Sakari - rebased on my former fix so it applied cleanly
[PATCH v3 2/2] media: ov6650: Register with asynchronous subdevice framework
Register V4L2 subdevice implemented by the driver to the V4L2
asynchronous subdevice framework.
Signed-off-by: Janusz Krzysztofik
---
drivers/media/i2c/ov6650.c | 19 ++-
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index de7d9790f054..7145bb68a9e1 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -804,8 +804,9 @@ static int ov6650_prog_dflt(struct i2c_client *client)
return ret;
}
-static int ov6650_video_probe(struct i2c_client *client)
+static int ov6650_video_probe(struct v4l2_subdev *sd)
{
+ struct i2c_client *client = v4l2_get_subdevdata(sd);
struct ov6650 *priv = to_ov6650(client);
u8 pidh, pidl, midh, midl;
int ret;
@@ -817,7 +818,7 @@ static int ov6650_video_probe(struct i2c_client *client)
return ret;
}
- ret = ov6650_s_power(&priv->subdev, 1);
+ ret = ov6650_s_power(sd, 1);
if (ret < 0)
goto eclkput;
@@ -855,7 +856,7 @@ static int ov6650_video_probe(struct i2c_client *client)
ret = v4l2_ctrl_handler_setup(&priv->hdl);
done:
- ov6650_s_power(&priv->subdev, 0);
+ ov6650_s_power(sd, 0);
if (!ret)
return 0;
eclkput:
@@ -943,6 +944,10 @@ static const struct v4l2_subdev_ops ov6650_subdev_ops = {
.pad= &ov6650_pad_ops,
};
+static const struct v4l2_subdev_internal_ops ov6650_internal_ops = {
+ .registered = ov6650_video_probe,
+};
+
/*
* i2c_driver function
*/
@@ -1003,7 +1008,11 @@ static int ov6650_probe(struct i2c_client *client,
priv->code= MEDIA_BUS_FMT_YUYV8_2X8;
priv->colorspace = V4L2_COLORSPACE_JPEG;
- ret = ov6650_video_probe(client);
+ priv->subdev.internal_ops = &ov6650_internal_ops;
+ priv->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+ v4l2_i2c_subdev_set_name(&priv->subdev, client, NULL, NULL);
+
+ ret = v4l2_async_register_subdev(&priv->subdev);
if (ret)
v4l2_ctrl_handler_free(&priv->hdl);
@@ -1015,7 +1024,7 @@ static int ov6650_remove(struct i2c_client *client)
struct ov6650 *priv = to_ov6650(client);
v4l2_clk_put(priv->clk);
- v4l2_device_unregister_subdev(&priv->subdev);
+ v4l2_async_unregister_subdev(&priv->subdev);
v4l2_ctrl_handler_free(&priv->hdl);
return 0;
}
--
2.19.2
[PATCH v3 1/2] media: ov6650: Move v4l2_clk_get() to ov6650_video_probe() helper
In preparation for adding asynchronous subdevice support to the driver,
don't acquire v4l2_clk from the driver .probe() callback as that may
fail if the clock is provided by a bridge driver which may be not yet
initialized. Move the v4l2_clk_get() to ov6650_video_probe() helper
which is going to be converted to v4l2_subdev_internal_ops.registered()
callback, executed only when the bridge driver is ready.
Signed-off-by: Janusz Krzysztofik
---
drivers/media/i2c/ov6650.c | 25 ++---
1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index f9359b11fa5c..de7d9790f054 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -810,9 +810,16 @@ static int ov6650_video_probe(struct i2c_client *client)
u8 pidh, pidl, midh, midl;
int ret;
+ priv->clk = v4l2_clk_get(&client->dev, NULL);
+ if (IS_ERR(priv->clk)) {
+ ret = PTR_ERR(priv->clk);
+ dev_err(&client->dev, "v4l2_clk request err: %d\n", ret);
+ return ret;
+ }
+
ret = ov6650_s_power(&priv->subdev, 1);
if (ret < 0)
- return ret;
+ goto eclkput;
msleep(20);
@@ -849,6 +856,11 @@ static int ov6650_video_probe(struct i2c_client *client)
done:
ov6650_s_power(&priv->subdev, 0);
+ if (!ret)
+ return 0;
+eclkput:
+ v4l2_clk_put(priv->clk);
+
return ret;
}
@@ -991,18 +1003,9 @@ static int ov6650_probe(struct i2c_client *client,
priv->code= MEDIA_BUS_FMT_YUYV8_2X8;
priv->colorspace = V4L2_COLORSPACE_JPEG;
- priv->clk = v4l2_clk_get(&client->dev, NULL);
- if (IS_ERR(priv->clk)) {
- ret = PTR_ERR(priv->clk);
- goto eclkget;
- }
-
ret = ov6650_video_probe(client);
- if (ret) {
- v4l2_clk_put(priv->clk);
-eclkget:
+ if (ret)
v4l2_ctrl_handler_free(&priv->hdl);
- }
return ret;
}
--
2.19.2
[PATCH v2] mtd: rawnand: ams-delta: Drop board specific partition info
After recent modifications, only a hardcoded partition info makes
the driver device specific. Other than that, the driver uses GPIO
exclusively and can be used on any hardware.
Drop the partition info and use MTD partition parser with default list
of parser names instead. For the OF parser to work correctly, pass
device of_node to mtd.
Amstrad Delta users should append the following partition info to their
kernel command line, possibly by embedding it in CONFIG_CMDLINE:
mtdparts=ams-delta-nand:3584k(Kernel),256k(u-boot),256k(u-boot_params),\
256k(Amstrad_LDR),27m(File_system),768k(PBL_reserved).
For their convenience, CONFIG_MTD_CMDLINE_PARTS symbol is selected
automatically from that board Kconfig if this NAND driver is also
selected.
Signed-off-by: Janusz Krzysztofik
Cc: Tony Lindgren
---
CHangelog:
v1->v2:
- fix a typo poined out by Aaro - thanks!
- fix device_node not passed to OF parser via mtd_info
- commit message reworded and reformatted a bit for better readability
arch/arm/mach-omap1/Kconfig | 1 +
drivers/mtd/nand/raw/ams-delta.c | 29 ++---
2 files changed, 3 insertions(+), 27 deletions(-)
diff --git a/arch/arm/mach-omap1/Kconfig b/arch/arm/mach-omap1/Kconfig
index c4694f26b5c4..62cf20f22828 100644
--- a/arch/arm/mach-omap1/Kconfig
+++ b/arch/arm/mach-omap1/Kconfig
@@ -171,6 +171,7 @@ config MACH_AMS_DELTA
select LEDS_GPIO_REGISTER
select REGULATOR
select REGULATOR_FIXED_VOLTAGE
+ select MTD_CMDLINE_PARTS if MTD_NAND_AMS_DELTA
help
Support for the Amstrad E3 (codename Delta) videophone. Say Y here
if you have such a device.
diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c
index 8312182088c1..e0f09179bbda 100644
--- a/drivers/mtd/nand/raw/ams-delta.c
+++ b/drivers/mtd/nand/raw/ams-delta.c
@@ -41,31 +41,6 @@ struct ams_delta_nand {
booldata_in;
};
-/*
- * Define partitions for flash devices
- */
-
-static const struct mtd_partition partition_info[] = {
- { .name = "Kernel",
- .offset = 0,
- .size = 3 * SZ_1M + SZ_512K },
- { .name = "u-boot",
- .offset = 3 * SZ_1M + SZ_512K,
- .size = SZ_256K },
- { .name = "u-boot params",
- .offset = 3 * SZ_1M + SZ_512K + SZ_256K,
- .size = SZ_256K },
- { .name = "Amstrad LDR",
- .offset = 4 * SZ_1M,
- .size = SZ_256K },
- { .name = "File system",
- .offset = 4 * SZ_1M + 1 * SZ_256K,
- .size = 27 * SZ_1M },
- { .name = "PBL reserved",
- .offset = 32 * SZ_1M - 3 * SZ_256K,
- .size = 3 * SZ_256K },
-};
-
static void ams_delta_write_commit(struct ams_delta_nand *priv)
{
gpiod_set_value(priv->gpiod_nwe, 0);
@@ -238,6 +213,7 @@ static int ams_delta_init(struct platform_device *pdev)
mtd->dev.parent = &pdev->dev;
nand_set_controller_data(this, priv);
+ nand_set_flash_node(this, pdev->dev.of_node);
priv->gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", GPIOD_IN);
if (IS_ERR(priv->gpiod_rdy)) {
@@ -315,8 +291,7 @@ static int ams_delta_init(struct platform_device *pdev)
return err;
/* Register the partitions */
- err = mtd_device_register(mtd, partition_info,
- ARRAY_SIZE(partition_info));
+ err = mtd_device_parse_register(mtd, NULL, NULL, NULL, 0);
if (err)
goto err_nand_cleanup;
--
2.19.2
Re: [PATCH] mtd: rawnand: ams-delta: Drop board specific partition info
Hi, On Sunday, March 24, 2019 8:24:48 PM CET H. Nikolaus Schaller wrote: > Hi, > > > Am 24.03.2019 um 19:59 schrieb Aaro Koskinen : > > > > Hi, > > > > On Sun, Mar 24, 2019 at 05:48:22PM +0100, Janusz Krzysztofik wrote: > >> Hi Aaro, > >> > >> Thanks for your review. > >> > >> On Wednesday, March 20, 2019 2:16:30 AM CET Aaro Koskinen wrote: > >>> On Tue, Mar 19, 2019 at 11:37:18PM +0100, Janusz Krzysztofik wrote: > >>>> After recent modifications, only a hardcoded partition info makes > >>>> the driver device specific. Other than that, the driver uses GPIO > >>>> exclusively and can be used on any hardware. > >>>> > >>>> Drop the partition info and use MTD partition parser with default > >>>> list of partition types instead. > >>>> > >>>> Amstrad Delta users should append the followig partition info to their > >>> > >>> Should be "following". > >>> > >>>> kernel command line, possibly by embedding it in CONFIG_CMDLINE: > >>>> mtdparts=ams-delta-nand:3584k(Kernel),256k(u-boot),256k(u-boot_params), \ > >>>> 256k(Amstrad_LDR),27m(File_system),768k(PBL_reserved). For their > >>>> convenience, select CONFIG_MTD_CMDLINE_PARTS symbol from that board > >>>> Kconfig automatically if this NAND driver is also selected. > >>>> > >>>> Signed-off-by: Janusz Krzysztofik > >>>> Cc: Tony Lindgren > >>> > >>> Could we move the fixed partition setup to the board file > >>> instead? Otherwise this kind of change is not really nice for the users, > >>> as it will likely break existing setups. The default partition layout > >>> should remain the same. > >> > >> I'm wondering if it would be acceptable to pass partition info from a .dts > >> file. I think that would be a better, more modern approach than adding a new > >> header under include/linux/platform_data. > > > > Hmm, I thought there was some generic way to define partitions without > > adding any new headers. But if that is not possible, then I guess your > > CMDLINE proposal is the preferred one.. > > I am not sure what you exactly need, but partitions can be defined in > the DTS as children of some NAND drivers. Example: > arch/arm/boot/dts/omap3-beagle.dts > So this design pattern could be copied instead of using CMDLINE. The problem is, OMAP1 has no device tree support. Other than that, your proposed approach already works for me locally with some basic support for device tree added to the board file. Thanks, Janusz
Re: [PATCH] mtd: rawnand: ams-delta: Drop board specific partition info
Hi, On Sunday, March 24, 2019 7:59:32 PM CET Aaro Koskinen wrote: > Hi, > > On Sun, Mar 24, 2019 at 05:48:22PM +0100, Janusz Krzysztofik wrote: > > Hi Aaro, > > > > Thanks for your review. > > > > On Wednesday, March 20, 2019 2:16:30 AM CET Aaro Koskinen wrote: > > > On Tue, Mar 19, 2019 at 11:37:18PM +0100, Janusz Krzysztofik wrote: > > > > After recent modifications, only a hardcoded partition info makes > > > > the driver device specific. Other than that, the driver uses GPIO > > > > exclusively and can be used on any hardware. > > > > > > > > Drop the partition info and use MTD partition parser with default > > > > list of partition types instead. > > > > > > > > Amstrad Delta users should append the followig partition info to their > > > > > > Should be "following". > > > > > > > kernel command line, possibly by embedding it in CONFIG_CMDLINE: > > > > mtdparts=ams-delta-nand:3584k(Kernel),256k(u-boot),256k(u- boot_params),\ > > > > 256k(Amstrad_LDR),27m(File_system),768k(PBL_reserved). For their > > > > convenience, select CONFIG_MTD_CMDLINE_PARTS symbol from that board > > > > Kconfig automatically if this NAND driver is also selected. > > > > > > > > Signed-off-by: Janusz Krzysztofik > > > > Cc: Tony Lindgren > > > > > > Could we move the fixed partition setup to the board file > > > instead? Otherwise this kind of change is not really nice for the users, > > > as it will likely break existing setups. The default partition layout > > > should remain the same. > > > > I'm wondering if it would be acceptable to pass partition info from a .dts > > file. I think that would be a better, more modern approach than adding a new > > header under include/linux/platform_data. > > Hmm, I thought there was some generic way to define partitions without > adding any new headers. But if that is not possible, then I guess your > CMDLINE proposal is the preferred one.. I could for example reuse (or abuse) struct gpio_nand_platdata defined in include/linux/mtd/nand-gpio.h for use with drivers/mtd/nand/raw/gpio.c, but I'm not sure if hi-jacking a header that belongs to another driver would be an elegant solution. Thanks, Janusz > > A. >
Re: [PATCH] mtd: rawnand: ams-delta: Drop board specific partition info
Hi Aaro, Thanks for your review. On Wednesday, March 20, 2019 2:16:30 AM CET Aaro Koskinen wrote: > On Tue, Mar 19, 2019 at 11:37:18PM +0100, Janusz Krzysztofik wrote: > > After recent modifications, only a hardcoded partition info makes > > the driver device specific. Other than that, the driver uses GPIO > > exclusively and can be used on any hardware. > > > > Drop the partition info and use MTD partition parser with default > > list of partition types instead. > > > > Amstrad Delta users should append the followig partition info to their > > Should be "following". > > > kernel command line, possibly by embedding it in CONFIG_CMDLINE: > > mtdparts=ams-delta-nand:3584k(Kernel),256k(u-boot),256k(u-boot_params),\ > > 256k(Amstrad_LDR),27m(File_system),768k(PBL_reserved). For their > > convenience, select CONFIG_MTD_CMDLINE_PARTS symbol from that board > > Kconfig automatically if this NAND driver is also selected. > > > > Signed-off-by: Janusz Krzysztofik > > Cc: Tony Lindgren > > Could we move the fixed partition setup to the board file > instead? Otherwise this kind of change is not really nice for the users, > as it will likely break existing setups. The default partition layout > should remain the same. I'm wondering if it would be acceptable to pass partition info from a .dts file. I think that would be a better, more modern approach than adding a new header under include/linux/platform_data. The problem with a device tree based implementation is, I know of no u-boot version supporting both Amstrad Delta and FDT. However, I've already tested two solutions that work for me. One uses CONFIG_ARM_APPENDED_DTB and requires a user to manually append the blob to zImage and (re)generate uImage. I'm not sure how much more user- friendly it looks for you, compared to the command line version I proposed initially. If the above is not acceptable. I can propose still another approach. The blob is automagically built and embedded into the kernel with some assembler glue, then unflattened from the board init_machine(), somehow similar to the way drivers/of/unittest.c does it. Please advise which approach sounds best to you (platform_data, CONFIG_ARM_APPENDED_DTB or unittest like). Thanks, Janusz > > A. >
[PATCH] mtd: rawnand: ams-delta: Drop board specific partition info
After recent modifications, only a hardcoded partition info makes
the driver device specific. Other than that, the driver uses GPIO
exclusively and can be used on any hardware.
Drop the partition info and use MTD partition parser with default
list of partition types instead.
Amstrad Delta users should append the followig partition info to their
kernel command line, possibly by embedding it in CONFIG_CMDLINE:
mtdparts=ams-delta-nand:3584k(Kernel),256k(u-boot),256k(u-boot_params),\
256k(Amstrad_LDR),27m(File_system),768k(PBL_reserved). For their
convenience, select CONFIG_MTD_CMDLINE_PARTS symbol from that board
Kconfig automatically if this NAND driver is also selected.
Signed-off-by: Janusz Krzysztofik
Cc: Tony Lindgren
---
arch/arm/mach-omap1/Kconfig | 1 +
drivers/mtd/nand/raw/ams-delta.c | 28 +---
2 files changed, 2 insertions(+), 27 deletions(-)
diff --git a/arch/arm/mach-omap1/Kconfig b/arch/arm/mach-omap1/Kconfig
index c4694f26b5c4..62cf20f22828 100644
--- a/arch/arm/mach-omap1/Kconfig
+++ b/arch/arm/mach-omap1/Kconfig
@@ -171,6 +171,7 @@ config MACH_AMS_DELTA
select LEDS_GPIO_REGISTER
select REGULATOR
select REGULATOR_FIXED_VOLTAGE
+ select MTD_CMDLINE_PARTS if MTD_NAND_AMS_DELTA
help
Support for the Amstrad E3 (codename Delta) videophone. Say Y here
if you have such a device.
diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c
index 8312182088c1..2e8e37ea549a 100644
--- a/drivers/mtd/nand/raw/ams-delta.c
+++ b/drivers/mtd/nand/raw/ams-delta.c
@@ -41,31 +41,6 @@ struct ams_delta_nand {
booldata_in;
};
-/*
- * Define partitions for flash devices
- */
-
-static const struct mtd_partition partition_info[] = {
- { .name = "Kernel",
- .offset = 0,
- .size = 3 * SZ_1M + SZ_512K },
- { .name = "u-boot",
- .offset = 3 * SZ_1M + SZ_512K,
- .size = SZ_256K },
- { .name = "u-boot params",
- .offset = 3 * SZ_1M + SZ_512K + SZ_256K,
- .size = SZ_256K },
- { .name = "Amstrad LDR",
- .offset = 4 * SZ_1M,
- .size = SZ_256K },
- { .name = "File system",
- .offset = 4 * SZ_1M + 1 * SZ_256K,
- .size = 27 * SZ_1M },
- { .name = "PBL reserved",
- .offset = 32 * SZ_1M - 3 * SZ_256K,
- .size = 3 * SZ_256K },
-};
-
static void ams_delta_write_commit(struct ams_delta_nand *priv)
{
gpiod_set_value(priv->gpiod_nwe, 0);
@@ -315,8 +290,7 @@ static int ams_delta_init(struct platform_device *pdev)
return err;
/* Register the partitions */
- err = mtd_device_register(mtd, partition_info,
- ARRAY_SIZE(partition_info));
+ err = mtd_device_parse_register(mtd, NULL, NULL, NULL, 0);
if (err)
goto err_nand_cleanup;
--
2.19.2
[PATCH] ARM: OMAP1: ams-delta: Fix broken GPIO ID allocation
In order to request dynamic allocationn of GPIO IDs, a negative number
should be passed as a base GPIO ID via platform data. Unfortuntely,
commit 771e53c4d1a1 ("ARM: OMAP1: ams-delta: Drop board specific global
GPIO numbers") didn't follow that rule while switching to dynamically
allocated GPIO IDs for Amstrad Delta latches, making their IDs
overlapping with those already assigned to OMAP GPIO devices. Fix it.
Fixes: 771e53c4d1a1 ("ARM: OMAP1: ams-delta: Drop board specific global GPIO
numbers")
Signed-off-by: Janusz Krzysztofik
Cc: sta...@vger.kernel.org
---
arch/arm/mach-omap1/board-ams-delta.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm/mach-omap1/board-ams-delta.c
b/arch/arm/mach-omap1/board-ams-delta.c
index be30c3c061b4..1b15d593837e 100644
--- a/arch/arm/mach-omap1/board-ams-delta.c
+++ b/arch/arm/mach-omap1/board-ams-delta.c
@@ -182,6 +182,7 @@ static struct resource latch1_resources[] = {
static struct bgpio_pdata latch1_pdata = {
.label = LATCH1_LABEL,
+ .base = -1,
.ngpio = LATCH1_NGPIO,
};
@@ -219,6 +220,7 @@ static struct resource latch2_resources[] = {
static struct bgpio_pdata latch2_pdata = {
.label = LATCH2_LABEL,
+ .base = -1,
.ngpio = LATCH2_NGPIO,
};
--
2.19.2
Re: [PATCH 2/5 v7] regulator: fixed/gpio: Pull inversion/OD into gpiolib
Hi Linus, On Monday, November 19, 2018 8:11:23 AM CET Linus Walleij wrote: > This pushes the handling of inversion semantics and open drain > settings to the GPIO descriptor and gpiolib. All affected board > files are also augmented. > > This is especially nice since we don't have to have any > confusing flags passed around to the left and right littering > the fixed and GPIO regulator drivers and the regulator core. > It is all just very straight-forward: the core asks the GPIO > line to be asserted or deasserted and gpiolib deals with the > rest depending on how the platform is configured: if the line > is active low, it deals with that, if the line is open drain, > it deals with that too. > > Cc: Janusz Krzysztofik # OMAP1 Tested-by: Janusz Krzysztofik #OMAP1 Amstrad Delta Thanks, Janusz
[PATCH] ARM: OMAP1: ams-delta: Fix audio permanently muted
Since commit 1137ceee76ba ("ARM: OMAP1: ams-delta: Don't request unused
GPIOs"), on-board audio has appeared muted. Believed to be unused GPIO
pin "hookflash1", apparently set high regardless of the corresponding
bit of "latch2" port attempted to be set low during .init_machine(),
has been identified as the reason.
According to Amstrad E3 wiki, the purpose of the pin hasn't been
clearly identified. Original Amstrad software used to produce a high
pulse on it when the phone was taken off hook or recall was pressed.
With the current finding, we can assume the pin provides a kind of
audio mute function.
Proper resolution of the issue should be done in two steps:
- resolution of an issue with the pin state not reflecting the value
the corresponding bit of the port was attempted to be initialized
with,
- extension of on-board audio driver with a new control.
For now, rename the pin to "audio_mute" to reflect its function and,
as a quick fix, hogg it as output low so on-board audio can produce
audible sound again.
Fixes: 1137ceee76ba ("ARM: OMAP1: ams-delta: Don't request unused GPIOs")
Signed-off-by: Janusz Krzysztofik
---
arch/arm/mach-omap1/board-ams-delta.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/arm/mach-omap1/board-ams-delta.c
b/arch/arm/mach-omap1/board-ams-delta.c
index 3d191fd52910..1d801f5e8308 100644
--- a/arch/arm/mach-omap1/board-ams-delta.c
+++ b/arch/arm/mach-omap1/board-ams-delta.c
@@ -247,8 +247,8 @@ static struct platform_device latch2_gpio_device = {
#define LATCH2_PIN_SCARD_CMDVCC11
#define LATCH2_PIN_MODEM_NRESET12
#define LATCH2_PIN_MODEM_CODEC 13
-#define LATCH2_PIN_HOOKFLASH1 14
-#define LATCH2_PIN_HOOKFLASH2 15
+#define LATCH2_PIN_AUDIO_MUTE 14
+#define LATCH2_PIN_HOOKFLASH 15
static struct regulator_consumer_supply modem_nreset_consumers[] = {
REGULATOR_SUPPLY("RESET#", "serial8250.1"),
@@ -588,6 +588,8 @@ static int gpiochip_match_by_label(struct gpio_chip *chip,
void *data)
static struct gpiod_hog ams_delta_gpio_hogs[] = {
GPIO_HOG(LATCH2_LABEL, LATCH2_PIN_KEYBRD_DATAOUT, "keybrd_dataout",
GPIO_ACTIVE_HIGH, GPIOD_OUT_LOW),
+ GPIO_HOG(LATCH2_LABEL, LATCH2_PIN_AUDIO_MUTE, "audio_mute",
+GPIO_ACTIVE_HIGH, GPIOD_OUT_LOW),
{},
};
--
2.18.1
[PATCH v4 1/4] ARM: OMAP1: ams-delta: Provide GPIO lookup table for NAND data port
Data port used by Amstrad Delta NAND driver is actually an OMAP MPUIO
device, already under control of gpio-omap driver. The NAND driver
gets access to the port by ioremapping it and performs read/write
operations. That is done without any proteciton from other users
legally manipulating the port pins over GPIO API.
The plan is to convert the driver to access the port over GPIO consumer
API. Before that is implemented, the driver can already obtain
exclusive access to the port by requesting an array of its GPIO
descriptors.
Add respective entries to the NAND GPIO lookup table.
Signed-off-by: Janusz Krzysztofik
Reviewed-by: Boris Brezillon
Reviewed-by: Linus Walleij
---
arch/arm/mach-omap1/board-ams-delta.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-omap1/board-ams-delta.c
b/arch/arm/mach-omap1/board-ams-delta.c
index 3d191fd52910..30c0d18f372e 100644
--- a/arch/arm/mach-omap1/board-ams-delta.c
+++ b/arch/arm/mach-omap1/board-ams-delta.c
@@ -312,7 +312,8 @@ static struct platform_device ams_delta_nand_device = {
.resource = ams_delta_nand_resources,
};
-#define OMAP_GPIO_LABEL"gpio-0-15"
+#define OMAP_GPIO_LABEL"gpio-0-15"
+#define OMAP_MPUIO_LABEL "mpuio"
static struct gpiod_lookup_table ams_delta_nand_gpio_table = {
.table = {
@@ -324,6 +325,14 @@ static struct gpiod_lookup_table ams_delta_nand_gpio_table
= {
GPIO_LOOKUP(LATCH2_LABEL, LATCH2_PIN_NAND_NWE, "nwe", 0),
GPIO_LOOKUP(LATCH2_LABEL, LATCH2_PIN_NAND_ALE, "ale", 0),
GPIO_LOOKUP(LATCH2_LABEL, LATCH2_PIN_NAND_CLE, "cle", 0),
+ GPIO_LOOKUP_IDX(OMAP_MPUIO_LABEL, 0, "data", 0, 0),
+ GPIO_LOOKUP_IDX(OMAP_MPUIO_LABEL, 1, "data", 1, 0),
+ GPIO_LOOKUP_IDX(OMAP_MPUIO_LABEL, 2, "data", 2, 0),
+ GPIO_LOOKUP_IDX(OMAP_MPUIO_LABEL, 3, "data", 3, 0),
+ GPIO_LOOKUP_IDX(OMAP_MPUIO_LABEL, 4, "data", 4, 0),
+ GPIO_LOOKUP_IDX(OMAP_MPUIO_LABEL, 5, "data", 5, 0),
+ GPIO_LOOKUP_IDX(OMAP_MPUIO_LABEL, 6, "data", 6, 0),
+ GPIO_LOOKUP_IDX(OMAP_MPUIO_LABEL, 7, "data", 7, 0),
{ },
},
};
--
2.18.1
[PATCH v4 3/4] mtd: rawnand: ams-delta: Use GPIO API for data I/O
Don't readw()/writew() data directly from/to GPIO port which is under
control of gpio-omap driver, use GPIO consumer API instead.
The driver should now work with any 8-bit bidirectional GPIO port, not
only OMAP.
Signed-off-by: Janusz Krzysztofik
Reviewed-by: Linus Walleij
---
drivers/mtd/nand/raw/ams-delta.c | 109 +--
1 file changed, 61 insertions(+), 48 deletions(-)
diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c
index bb50dda05654..8312182088c1 100644
--- a/drivers/mtd/nand/raw/ams-delta.c
+++ b/drivers/mtd/nand/raw/ams-delta.c
@@ -18,11 +18,10 @@
#include
#include
#include
-#include
#include
#include
#include
-#include
+#include
#include
/*
@@ -38,7 +37,7 @@ struct ams_delta_nand {
struct gpio_desc*gpiod_nwe;
struct gpio_desc*gpiod_ale;
struct gpio_desc*gpiod_cle;
- void __iomem*io_base;
+ struct gpio_descs *data_gpiods;
booldata_in;
};
@@ -67,42 +66,78 @@ static const struct mtd_partition partition_info[] = {
.size = 3 * SZ_256K },
};
-static void ams_delta_io_write(struct ams_delta_nand *priv, u8 byte)
+static void ams_delta_write_commit(struct ams_delta_nand *priv)
{
- writew(byte, priv->io_base + OMAP_MPUIO_OUTPUT);
gpiod_set_value(priv->gpiod_nwe, 0);
ndelay(40);
gpiod_set_value(priv->gpiod_nwe, 1);
}
+static void ams_delta_io_write(struct ams_delta_nand *priv, u8 byte)
+{
+ struct gpio_descs *data_gpiods = priv->data_gpiods;
+ DECLARE_BITMAP(values, BITS_PER_TYPE(byte)) = { byte, };
+
+ gpiod_set_raw_array_value(data_gpiods->ndescs, data_gpiods->desc,
+ data_gpiods->info, values);
+
+ ams_delta_write_commit(priv);
+}
+
+static void ams_delta_dir_output(struct ams_delta_nand *priv, u8 byte)
+{
+ struct gpio_descs *data_gpiods = priv->data_gpiods;
+ DECLARE_BITMAP(values, BITS_PER_TYPE(byte)) = { byte, };
+ int i;
+
+ for (i = 0; i < data_gpiods->ndescs; i++)
+ gpiod_direction_output_raw(data_gpiods->desc[i],
+ test_bit(i, values));
+
+ ams_delta_write_commit(priv);
+
+ priv->data_in = false;
+}
+
static u8 ams_delta_io_read(struct ams_delta_nand *priv)
{
u8 res;
+ struct gpio_descs *data_gpiods = priv->data_gpiods;
+ DECLARE_BITMAP(values, BITS_PER_TYPE(res)) = { 0, };
gpiod_set_value(priv->gpiod_nre, 0);
ndelay(40);
- res = readw(priv->io_base + OMAP_MPUIO_INPUT_LATCH);
+
+ gpiod_get_raw_array_value(data_gpiods->ndescs, data_gpiods->desc,
+ data_gpiods->info, values);
+
gpiod_set_value(priv->gpiod_nre, 1);
+ res = values[0];
return res;
}
-static void ams_delta_dir_input(struct ams_delta_nand *priv, bool in)
+static void ams_delta_dir_input(struct ams_delta_nand *priv)
{
- writew(in ? ~0 : 0, priv->io_base + OMAP_MPUIO_IO_CNTL);
- priv->data_in = in;
+ struct gpio_descs *data_gpiods = priv->data_gpiods;
+ int i;
+
+ for (i = 0; i < data_gpiods->ndescs; i++)
+ gpiod_direction_input(data_gpiods->desc[i]);
+
+ priv->data_in = true;
}
static void ams_delta_write_buf(struct ams_delta_nand *priv, const u8 *buf,
int len)
{
- int i;
+ int i = 0;
- if (priv->data_in)
- ams_delta_dir_input(priv, false);
+ if (len > 0 && priv->data_in)
+ ams_delta_dir_output(priv, buf[i++]);
- for (i = 0; i < len; i++)
- ams_delta_io_write(priv, buf[i]);
+ while (i < len)
+ ams_delta_io_write(priv, buf[i++]);
}
static void ams_delta_read_buf(struct ams_delta_nand *priv, u8 *buf, int len)
@@ -110,7 +145,7 @@ static void ams_delta_read_buf(struct ams_delta_nand *priv,
u8 *buf, int len)
int i;
if (!priv->data_in)
- ams_delta_dir_input(priv, true);
+ ams_delta_dir_input(priv);
for (i = 0; i < len; i++)
buf[i] = ams_delta_io_read(priv);
@@ -188,14 +223,9 @@ static int ams_delta_init(struct platform_device *pdev)
struct ams_delta_nand *priv;
struct nand_chip *this;
struct mtd_info *mtd;
- struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- void __iomem *io_base;
struct gpio_descs *data_gpiods;
int err = 0;
- if (!res)
- return -ENXIO;
-
/* Allocate memory for MTD device structure and private data */
priv = devm_kzalloc(&pdev->dev, sizeof(struct ams_delta_nand),
GFP_KERNEL);
@@ -207,25 +237,13 @@ static
[PATCH v4 2/4] mtd: rawnand: ams-delta: Request data port GPIO resource
Data port used by the driver is actually an OMAP MPUIO device, already
under control of gpio-omap driver. For that reason we used to not
request the memory region of the port as that would fail because the
region is already busy. Despite that, we are still accessing the port
by just ioremapping it and performing read/write operations. Moreover,
we are doing that without any proteciton from other users legally
manipulating the port pins over GPIO API.
The plan is to convert the driver to access the port over GPIO consumer
API. Before that happens, already prevent from other users accessing
the port pins by requesting an array of its GPIO descriptors.
Signed-off-by: Janusz Krzysztofik
Reviewed-by: Boris Brezillon
Reviewed-by: Linus Walleij
---
drivers/mtd/nand/raw/ams-delta.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c
index f8eb4a419e77..bb50dda05654 100644
--- a/drivers/mtd/nand/raw/ams-delta.c
+++ b/drivers/mtd/nand/raw/ams-delta.c
@@ -190,6 +190,7 @@ static int ams_delta_init(struct platform_device *pdev)
struct mtd_info *mtd;
struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
void __iomem *io_base;
+ struct gpio_descs *data_gpiods;
int err = 0;
if (!res)
@@ -275,8 +276,14 @@ static int ams_delta_init(struct platform_device *pdev)
goto err_unmap;
}
- /* Initialize data port direction to a known state */
- ams_delta_dir_input(priv, true);
+ /* Request array of data pins, initialize them as input */
+ data_gpiods = devm_gpiod_get_array(&pdev->dev, "data", GPIOD_IN);
+ if (IS_ERR(data_gpiods)) {
+ err = PTR_ERR(data_gpiods);
+ dev_err(&pdev->dev, "data GPIO request failed: %d\n", err);
+ goto err_unmap;
+ }
+ priv->data_in = true;
/* Initialize the NAND controller object embedded in ams_delta_nand. */
priv->base.ops = &ams_delta_ops;
--
2.18.1
[PATCH v4 4/4] ARM: OMAP1: ams-delta: Drop obsolete NAND resources
Amstrad Delta NAND driver now uses GPIO API for data I/O so there is no
need to assign memory I/O resource to the device any longer. Drop it.
Signed-off-by: Janusz Krzysztofik
---
arch/arm/mach-omap1/board-ams-delta.c | 11 ---
1 file changed, 11 deletions(-)
diff --git a/arch/arm/mach-omap1/board-ams-delta.c
b/arch/arm/mach-omap1/board-ams-delta.c
index 30c0d18f372e..d594f60c3224 100644
--- a/arch/arm/mach-omap1/board-ams-delta.c
+++ b/arch/arm/mach-omap1/board-ams-delta.c
@@ -296,20 +296,9 @@ struct modem_private_data {
static struct modem_private_data modem_priv;
-static struct resource ams_delta_nand_resources[] = {
- [0] = {
- .start = OMAP1_MPUIO_BASE,
- .end= OMAP1_MPUIO_BASE +
- OMAP_MPUIO_IO_CNTL + sizeof(u32) - 1,
- .flags = IORESOURCE_MEM,
- },
-};
-
static struct platform_device ams_delta_nand_device = {
.name = "ams-delta-nand",
.id = -1,
- .num_resources = ARRAY_SIZE(ams_delta_nand_resources),
- .resource = ams_delta_nand_resources,
};
#define OMAP_GPIO_LABEL"gpio-0-15"
--
2.18.1
Subject: [PATCH v4 0/4] mtd: rawnand: ams-delta: Use GPIO API for data I/O
Finalize implementation of the idea suggested by Artem Bityutskiy and
Tony Lindgren, described in commit b027274d2e3a ("mtd: ams-delta: fix
request_mem_region() failure"). Use pure GPIO consumer API, as reqested
by Boris Brezillon.
Janusz Krzysztofik (4):
ARM: OMAP1: ams-delta: Provide GPIO lookup table for NAND data port
mtd: rawnand: ams-delta: Request data port GPIO resource
mtd: rawnand: ams-delta: Use GPIO API for data I/O
ARM: OMAP1: ams-delta: Drop obsolete NAND resources
arch/arm/mach-omap1/board-ams-delta.c | 22 ++
drivers/mtd/nand/raw/ams-delta.c | 120 +++---
2 files changed, 80 insertions(+), 62 deletions(-)
Performance on Amstrad Delta is now acceptable after recent extensions
to GPIO API and rawnanad enhancements.
Series intented to be merged via mtd tree, should not conflict with
other OMAP1 patches submitted for 4.21.
Changelog:
v4:
- drop patches 1/7, 2/7, 5/7 6/7 - changes already merged,
- reintroduce postponed PATCH v2 06/12 as 4/4,
- rebase on nand-next for 4.21.
v3:
[PATCH v3 1/7] mtd: rawnand: ams-delta: show parent device in sysfs
- renamed and an explanation added based on other similar patches on
Marek Vasut request, thanks.
[PATCH v3 2/7] mtd: rawnand: ams-delta: Use private structure
- no changes.
[PATCH v3 3/7] ARM: OMAP1: ams-delta: Provide GPIO lookup table for NAND
data port
- no changes.
[PATCH v3 4/7] mtd: rawnand: ams-delta: request data port GPIO resource
- no changes.
[PATCH v3 5/7] mtd: rawnand: ams-delta: Set port direction when needed
- modified to set port direction only when needed instead of on each
transfer as suggested by Boris, thanks, though I kept separate
*_next_byte() functions to maximize performance as much as possible,
- moved back in front of "mtd: rawnand: ams-delta: use GPIO API for data
I/O" with a comment added referring to the planned switch to GPIO API.
[PATCH v3 6/7] mtd: rawnand: ams-delta: Simplify pointer resolution
- moved back in front of "mtd: rawnand: ams-delta: use GPIO API for data
I/O" on Boris request, thanks.
[RFC PATCH v3 7/7] mtd: rawnand: ams-delta: use GPIO API for data I/O
- rebased back on top of the two mentioned above,
- not intended to apply it yet due to performance issues on Amstrad Delta.
Removed from the series:
[RFC PATCH v2 09/12] gpiolib: Identify GPIO descriptor arrays with direct
mapping
[RFC PATCH v2 10/12] gpiolib: Introduce bitmap get/set array API extension
[RFC PATCH v2 11/12] mtd: rawnand: ams-delta: Use GPIO API bitmap extension
[RFC PATCH v2 12/12] gpiolib: Add fast processing path to bitmap API functions
- intended to be still iterated in a follow up series until performance
issues are resolved.
[RFC PATCH v2 06/12] ARM: OMAP1: ams-delta: drop obsolete NAND resources
- postponed until acceptable performance on Amstrad Delta is achieved.
v2:
[RFC PATCH v2 00/12] mtd: rawnand: ams-delta: Use GPIO API for data I/O
- renamed from former [RFC PATCH 0/8] mtd: rawnand: ams-delta: Use
gpio-omap accessors for data I/O
[RFC PATCH v2 01/12] mtd: rawnand: ams-delta: Assign mtd->dev.parent,
not mtd->owner
- split out from former [RFC PATCH 1/8] on Boris request, thanks.
[RFC PATCH v2 02/12] mtd: rawnand: ams-delta: Use private structure
- remaining part of the former [RFC PATCH 1/8].
[RFC PATCH v2 03/12] ARM: OMAP1: ams-delta: Provide GPIO lookup table
for NAND data port
- split out from former [RFC PATCH 5/8] on Boris requesst, thanks,
[RFC PATCH v2 04/12] mtd: rawnand: ams-delta: request data port GPIO resource
- remaining part of the former [RFC PATCH 5/8],
[RFC PATCH v2 05/12] mtd: rawnand: ams-delta: use GPIO API for data read/write
- reworked from former [RFC PATCH 8/8] on Boris requesst to use pure
GPIO API, thanks,
- moved up in front of former [RFC PATCH 3/8] on Boris request, thanks.
[RFC PATCH v2 06/12] ARM: OMAP1: ams-delta: drop obsolete NAND resources
- split out from former [RFC PATCH 8/8].
[RFC PATCH v2 07/12] mtd: rawnand: ams-delta: Set port direction once per
transfer
- reworked from former [RFC PATCH 3/8] on top of [RFC PATCH v2 05/12].
[RFC PATCH v2 08/12] mtd: rawnand: ams-delta: Simplify pointer resolution
on read/write
- reworked from former [RFC PATCH 4/8] on top of [RFC PATCH v2 08/12],
- renamed from 'Optimize' to 'Simplify' on Boris request, thanks.
[RFC PATCH v2 09/12] gpiolib: Identify GPIO descriptor arrays with direct
mapping
- new patch.
[RFC PATCH v2 10/12] gpiolib: Introduce bitmap get/set array API extension
- new patch.
[RFC PATCH v2 11/12] mtd: rawnand: ams-delta: Use GPIO API bitmap extension
- new patch.
[RFC PATCH v2 12/12] gpiolib: Add fast processing path to bitmap API functions
- new patch.
Removed from the series:
[RFC PATCH 2/8] mtd: rawnand: ams-delta: Write protect device during probe
- postponed on Bo
[PATCH] ARM: OMAP1: ams-delta: Fix possible use of uninitialized field
While playing with initialization order of modem device, it has been
discovered that under some circumstances (early console init, I
believe) its .pm() callback may be called before the
uart_port->private_data pointer is initialized from
plat_serial8250_port->private_data, resulting in NULL pointer
dereference. Fix it by checking for uninitialized pointer before using
it in modem_pm().
Fixes: aabf31737a6a ("ARM: OMAP1: ams-delta: update the modem to use regulator
API")
Signed-off-by: Janusz Krzysztofik
---
arch/arm/mach-omap1/board-ams-delta.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/arm/mach-omap1/board-ams-delta.c
b/arch/arm/mach-omap1/board-ams-delta.c
index 1947bc63074e..17e0b386e1b2 100644
--- a/arch/arm/mach-omap1/board-ams-delta.c
+++ b/arch/arm/mach-omap1/board-ams-delta.c
@@ -764,6 +764,9 @@ static void modem_pm(struct uart_port *port, unsigned int
state, unsigned old)
struct modem_private_data *priv = port->private_data;
int ret;
+ if (!priv)
+ return;
+
if (IS_ERR(priv->regulator))
return;
--
2.16.4
[PATCH 2/3] ARM: OMAP1: ams-delta: Drop unused symbols from the board header
Those bitmap symbols defining pins of latch2 register, used with read()/write() calls before the latch was converted to a GPIO device, have been obsoleted by integer symbols defined inside the board file. Signed-off-by: Janusz Krzysztofik --- arch/arm/mach-omap1/board-ams-delta.h | 4 1 file changed, 4 deletions(-) diff --git a/arch/arm/mach-omap1/board-ams-delta.h b/arch/arm/mach-omap1/board-ams-delta.h index 06e4c64a47f8..2c3aff824cf0 100644 --- a/arch/arm/mach-omap1/board-ams-delta.h +++ b/arch/arm/mach-omap1/board-ams-delta.h @@ -28,10 +28,6 @@ #if defined (CONFIG_MACH_AMS_DELTA) -#define AMD_DELTA_LATCH2_SCARD_RSTIN 0x0400 -#define AMD_DELTA_LATCH2_SCARD_CMDVCC 0x0800 -#define AMS_DELTA_LATCH2_MODEM_CODEC 0x2000 - #define AMS_DELTA_GPIO_PIN_KEYBRD_DATA 0 #define AMS_DELTA_GPIO_PIN_KEYBRD_CLK 1 #define AMS_DELTA_GPIO_PIN_MODEM_IRQ 2 -- 2.16.4
[PATCH 3/3] ARM: OMAP1: ams-delta: Move AMS_DELTA_LATCH2_NGPIO to the board file
That symbol is not used outside the board file, there is no need to
keep it in the board header.
Signed-off-by: Janusz Krzysztofik
---
arch/arm/mach-omap1/board-ams-delta.c | 6 --
arch/arm/mach-omap1/board-ams-delta.h | 2 --
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/arm/mach-omap1/board-ams-delta.c
b/arch/arm/mach-omap1/board-ams-delta.c
index a6986a83a916..1947bc63074e 100644
--- a/arch/arm/mach-omap1/board-ams-delta.c
+++ b/arch/arm/mach-omap1/board-ams-delta.c
@@ -204,11 +204,13 @@ static struct platform_device latch1_gpio_device = {
#define LATCH1_PIN_DOCKIT1 6
#define LATCH1_PIN_DOCKIT2 7
+#define LATCH2_NGPIO 16
+
static struct resource latch2_resources[] = {
[0] = {
.name = "dat",
.start = LATCH2_PHYS,
- .end= LATCH2_PHYS + (AMS_DELTA_LATCH2_NGPIO - 1) / 8,
+ .end= LATCH2_PHYS + (LATCH2_NGPIO - 1) / 8,
.flags = IORESOURCE_MEM,
},
};
@@ -217,7 +219,7 @@ static struct resource latch2_resources[] = {
static struct bgpio_pdata latch2_pdata = {
.label = LATCH2_LABEL,
- .ngpio = AMS_DELTA_LATCH2_NGPIO,
+ .ngpio = LATCH2_NGPIO,
};
static struct platform_device latch2_gpio_device = {
diff --git a/arch/arm/mach-omap1/board-ams-delta.h
b/arch/arm/mach-omap1/board-ams-delta.h
index 2c3aff824cf0..b5c4a373b905 100644
--- a/arch/arm/mach-omap1/board-ams-delta.h
+++ b/arch/arm/mach-omap1/board-ams-delta.h
@@ -37,8 +37,6 @@
#define AMS_DELTA_GPIO_PIN_CONFIG 11
#define AMS_DELTA_GPIO_PIN_NAND_RB 12
-#define AMS_DELTA_LATCH2_NGPIO 16
-
#endif /* CONFIG_MACH_AMS_DELTA */
#endif /* __ASM_ARCH_OMAP_AMS_DELTA_H */
--
2.16.4
[PATCH 1/3] ARM: OMAP1: ams-delta: Drop board specific global GPIO numbers
As all users of the board specific GPIO pins have been converted from
legacy integer-based to descriptor-based interface, there is no longer
a need to maintain statically assigned GPIO pin numbers. Drop support
for that.
Signed-off-by: Janusz Krzysztofik
---
arch/arm/mach-omap1/board-ams-delta.c | 3 ---
arch/arm/mach-omap1/board-ams-delta.h | 16
2 files changed, 19 deletions(-)
diff --git a/arch/arm/mach-omap1/board-ams-delta.c
b/arch/arm/mach-omap1/board-ams-delta.c
index 19e0c071d675..a6986a83a916 100644
--- a/arch/arm/mach-omap1/board-ams-delta.c
+++ b/arch/arm/mach-omap1/board-ams-delta.c
@@ -167,7 +167,6 @@ static struct omap_usb_config ams_delta_usb_config
__initdata = {
.pins[0]= 2,
};
-#define LATCH1_GPIO_BASE 232
#define LATCH1_NGPIO 8
static struct resource latch1_resources[] = {
@@ -183,7 +182,6 @@ static struct resource latch1_resources[] = {
static struct bgpio_pdata latch1_pdata = {
.label = LATCH1_LABEL,
- .base = LATCH1_GPIO_BASE,
.ngpio = LATCH1_NGPIO,
};
@@ -219,7 +217,6 @@ static struct resource latch2_resources[] = {
static struct bgpio_pdata latch2_pdata = {
.label = LATCH2_LABEL,
- .base = AMS_DELTA_LATCH2_GPIO_BASE,
.ngpio = AMS_DELTA_LATCH2_NGPIO,
};
diff --git a/arch/arm/mach-omap1/board-ams-delta.h
b/arch/arm/mach-omap1/board-ams-delta.h
index a74a306d7e77..06e4c64a47f8 100644
--- a/arch/arm/mach-omap1/board-ams-delta.h
+++ b/arch/arm/mach-omap1/board-ams-delta.h
@@ -41,22 +41,6 @@
#define AMS_DELTA_GPIO_PIN_CONFIG 11
#define AMS_DELTA_GPIO_PIN_NAND_RB 12
-#define AMS_DELTA_GPIO_PIN_LCD_VBLEN 240
-#define AMS_DELTA_GPIO_PIN_LCD_NDISP 241
-#define AMS_DELTA_GPIO_PIN_NAND_NCE242
-#define AMS_DELTA_GPIO_PIN_NAND_NRE243
-#define AMS_DELTA_GPIO_PIN_NAND_NWP244
-#define AMS_DELTA_GPIO_PIN_NAND_NWE245
-#define AMS_DELTA_GPIO_PIN_NAND_ALE246
-#define AMS_DELTA_GPIO_PIN_NAND_CLE247
-#define AMS_DELTA_GPIO_PIN_KEYBRD_PWR 248
-#define AMS_DELTA_GPIO_PIN_KEYBRD_DATAOUT 249
-#define AMS_DELTA_GPIO_PIN_SCARD_RSTIN 250
-#define AMS_DELTA_GPIO_PIN_SCARD_CMDVCC251
-#define AMS_DELTA_GPIO_PIN_MODEM_NRESET252
-#define AMS_DELTA_GPIO_PIN_MODEM_CODEC 253
-
-#define AMS_DELTA_LATCH2_GPIO_BASE AMS_DELTA_GPIO_PIN_LCD_VBLEN
#define AMS_DELTA_LATCH2_NGPIO 16
#endif /* CONFIG_MACH_AMS_DELTA */
--
2.16.4
[PATCH 0/3] ARN: OMAP1: ams-delta: Post-GPIO-refresh cleanups
Janusz Krzysztofik (3): ARM: OMAP1: ams-delta: Drop board specific global GPIO numbers ARM: OMAP1: ams-delta: Drop unused symbols from the board header ARM: OMAP1: ams-delta: Move AMS_DELTA_LATCH2_NGPIO to the board file board-ams-delta.c |9 - board-ams-delta.h | 22 -- 2 files changed, 4 insertions(+), 27 deletions(-)
[PATCH] ARM: OMAP1: ams-delta: Provide GPIO lookup table for LED device
Global GPIO numbers no longer have to be passed to leds-gpio driver,
replace their assignment with a lookup table.
Signed-off-by: Janusz Krzysztofik
---
arch/arm/mach-omap1/board-ams-delta.c | 95 ++-
1 file changed, 26 insertions(+), 69 deletions(-)
diff --git a/arch/arm/mach-omap1/board-ams-delta.c
b/arch/arm/mach-omap1/board-ams-delta.c
index b8acc9912a58..19e0c071d675 100644
--- a/arch/arm/mach-omap1/board-ams-delta.c
+++ b/arch/arm/mach-omap1/board-ams-delta.c
@@ -371,15 +371,9 @@ static struct gpiod_lookup_table ams_delta_lcd_gpio_table
= {
},
};
-/*
- * Dynamically allocated GPIO numbers must be obtained fromm GPIO device
- * before they can be put in the gpio_led table. Before that happens,
- * initialize the table with invalid GPIO numbers, not 0.
- */
static struct gpio_led gpio_leds[] __initdata = {
[LATCH1_PIN_LED_CAMERA] = {
.name= "camera",
- .gpio= -EINVAL,
.default_state = LEDS_GPIO_DEFSTATE_OFF,
#ifdef CONFIG_LEDS_TRIGGERS
.default_trigger = "ams_delta_camera",
@@ -387,27 +381,22 @@ static struct gpio_led gpio_leds[] __initdata = {
},
[LATCH1_PIN_LED_ADVERT] = {
.name= "advert",
- .gpio= -EINVAL,
.default_state = LEDS_GPIO_DEFSTATE_OFF,
},
[LATCH1_PIN_LED_MAIL] = {
.name= "email",
- .gpio= -EINVAL,
.default_state = LEDS_GPIO_DEFSTATE_OFF,
},
[LATCH1_PIN_LED_HANDSFREE] = {
.name= "handsfree",
- .gpio= -EINVAL,
.default_state = LEDS_GPIO_DEFSTATE_OFF,
},
[LATCH1_PIN_LED_VOICEMAIL] = {
.name= "voicemail",
- .gpio= -EINVAL,
.default_state = LEDS_GPIO_DEFSTATE_OFF,
},
[LATCH1_PIN_LED_VOICE] = {
.name= "voice",
- .gpio= -EINVAL,
.default_state = LEDS_GPIO_DEFSTATE_OFF,
},
};
@@ -417,6 +406,24 @@ static const struct gpio_led_platform_data leds_pdata
__initconst = {
.num_leds = ARRAY_SIZE(gpio_leds),
};
+static struct gpiod_lookup_table leds_gpio_table = {
+ .table = {
+ GPIO_LOOKUP_IDX(LATCH1_LABEL, LATCH1_PIN_LED_CAMERA, NULL,
+ LATCH1_PIN_LED_CAMERA, 0),
+ GPIO_LOOKUP_IDX(LATCH1_LABEL, LATCH1_PIN_LED_ADVERT, NULL,
+ LATCH1_PIN_LED_ADVERT, 0),
+ GPIO_LOOKUP_IDX(LATCH1_LABEL, LATCH1_PIN_LED_MAIL, NULL,
+ LATCH1_PIN_LED_MAIL, 0),
+ GPIO_LOOKUP_IDX(LATCH1_LABEL, LATCH1_PIN_LED_HANDSFREE, NULL,
+ LATCH1_PIN_LED_HANDSFREE, 0),
+ GPIO_LOOKUP_IDX(LATCH1_LABEL, LATCH1_PIN_LED_VOICEMAIL, NULL,
+ LATCH1_PIN_LED_VOICEMAIL, 0),
+ GPIO_LOOKUP_IDX(LATCH1_LABEL, LATCH1_PIN_LED_VOICE, NULL,
+ LATCH1_PIN_LED_VOICE, 0),
+ { },
+ },
+};
+
static struct i2c_board_info ams_delta_camera_board_info[] = {
{
I2C_BOARD_INFO("ov6650", 0x60),
@@ -677,6 +684,8 @@ static void __init ams_delta_latch2_init(void)
static void __init ams_delta_init(void)
{
+ struct platform_device *leds_pdev;
+
/* mux pins for uarts */
omap_cfg_reg(UART1_TX);
omap_cfg_reg(UART1_RTS);
@@ -740,6 +749,12 @@ static void __init ams_delta_init(void)
gpiod_add_lookup_tables(ams_delta_gpio_tables,
ARRAY_SIZE(ams_delta_gpio_tables));
+ leds_pdev = gpio_led_register_device(PLATFORM_DEVID_NONE, &leds_pdata);
+ if (!IS_ERR(leds_pdev)) {
+ leds_gpio_table.dev_id = dev_name(&leds_pdev->dev);
+ gpiod_add_lookup_table(&leds_gpio_table);
+ }
+
omap_writew(omap_readw(ARM_RSTCT1) | 0x0004, ARM_RSTCT1);
omapfb_set_lcd_config(&ams_delta_lcd_config);
@@ -793,64 +808,6 @@ static struct platform_device ams_delta_modem_device = {
},
};
-/*
- * leds-gpio driver doesn't make use of GPIO lookup tables,
- * it has to be provided with GPIO numbers over platform data
- * if GPIO descriptor info can't be obtained from device tree.
- * We could either define GPIO lookup tables and use them on behalf
- * of the leds-gpio device, or we can use GPIO driver level methods
- * for identification of GPIO numbers as long as we don't support
- * device tree. Let's do the latter.
- */
-static void __init ams_delta_led_init(struct gpio_chip *chip)
-{
- struct gpio_desc *gpiod;
- int i;
-
- f
[PATCH] ARM: OMAP1: ams-delta: make board header file local to mach-omap1
Now as the board header file is no longer included by drivers, move it
to the root directory of mach-omap1.
Signed-off-by: Janusz Krzysztofik
---
Changelog:
v4:
- rebased on top of v4.20-rc1
- resubmitted as a standalone patch, other patches of the series it
depended on have been already applied
v3:
- rebased on top of v4.19-rc1
- submitted as PATCH v3 3/3
v2:
- resent as PATCH v2 3/3
Initially submitted in series as PATCH 6/6
arch/arm/mach-omap1/ams-delta-fiq-handler.S | 2 +-
arch/arm/mach-omap1/ams-delta-fiq.c | 3 +--
arch/arm/mach-omap1/board-ams-delta.c| 2 +-
arch/arm/mach-omap1/{include/mach => }/board-ams-delta.h | 2 +-
4 files changed, 4 insertions(+), 5 deletions(-)
rename arch/arm/mach-omap1/{include/mach => }/board-ams-delta.h (97%)
diff --git a/arch/arm/mach-omap1/ams-delta-fiq-handler.S
b/arch/arm/mach-omap1/ams-delta-fiq-handler.S
index e3faa0274b56..7c9fb7fe0070 100644
--- a/arch/arm/mach-omap1/ams-delta-fiq-handler.S
+++ b/arch/arm/mach-omap1/ams-delta-fiq-handler.S
@@ -18,9 +18,9 @@
#include
#include
-#include
#include "ams-delta-fiq.h"
+#include "board-ams-delta.h"
#include "iomap.h"
#include "soc.h"
diff --git a/arch/arm/mach-omap1/ams-delta-fiq.c
b/arch/arm/mach-omap1/ams-delta-fiq.c
index b0dc7ddf5877..14c3d3f5255e 100644
--- a/arch/arm/mach-omap1/ams-delta-fiq.c
+++ b/arch/arm/mach-omap1/ams-delta-fiq.c
@@ -22,11 +22,10 @@
#include
#include
-#include
-
#include
#include "ams-delta-fiq.h"
+#include "board-ams-delta.h"
static struct fiq_handler fh = {
.name = "ams-delta-fiq"
diff --git a/arch/arm/mach-omap1/board-ams-delta.c
b/arch/arm/mach-omap1/board-ams-delta.c
index 3d191fd52910..b8acc9912a58 100644
--- a/arch/arm/mach-omap1/board-ams-delta.c
+++ b/arch/arm/mach-omap1/board-ams-delta.c
@@ -36,7 +36,6 @@
#include
#include
-#include
#include
#include
@@ -45,6 +44,7 @@
#include
#include "ams-delta-fiq.h"
+#include "board-ams-delta.h"
#include "iomap.h"
#include "common.h"
diff --git a/arch/arm/mach-omap1/include/mach/board-ams-delta.h
b/arch/arm/mach-omap1/board-ams-delta.h
similarity index 97%
rename from arch/arm/mach-omap1/include/mach/board-ams-delta.h
rename to arch/arm/mach-omap1/board-ams-delta.h
index 3b2d8019238a..a74a306d7e77 100644
--- a/arch/arm/mach-omap1/include/mach/board-ams-delta.h
+++ b/arch/arm/mach-omap1/board-ams-delta.h
@@ -1,5 +1,5 @@
/*
- * arch/arm/plat-omap/include/mach/board-ams-delta.h
+ * arch/arm/mach-omap1/board-ams-delta.h
*
* Copyright (C) 2006 Jonathan McDowell
*
--
2.16.4
