| Message ID | 1428361053-20411-5-git-send-email-sakari.ailus@iki.fi (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Sakari, Thank you for the patch. On Tuesday 07 April 2015 01:57:32 Sakari Ailus wrote: > Instead of parsing the link-frequencies property in the driver, let > v4l2_of_alloc_parse_endpoint() do it. > > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi> > --- > drivers/media/i2c/smiapp/smiapp-core.c | 40 +++++++++++++++-------------- > 1 file changed, 20 insertions(+), 20 deletions(-) > > diff --git a/drivers/media/i2c/smiapp/smiapp-core.c > b/drivers/media/i2c/smiapp/smiapp-core.c index 557f25d..4a2e8d3 100644 > --- a/drivers/media/i2c/smiapp/smiapp-core.c > +++ b/drivers/media/i2c/smiapp/smiapp-core.c > @@ -2975,9 +2975,9 @@ static int smiapp_resume(struct device *dev) > static struct smiapp_platform_data *smiapp_get_pdata(struct device *dev) > { > struct smiapp_platform_data *pdata; > - struct v4l2_of_endpoint bus_cfg; > + struct v4l2_of_endpoint *bus_cfg; > struct device_node *ep; > - uint32_t asize; > + int i; > int rval; > > if (!dev->of_node) > @@ -2987,13 +2987,17 @@ static struct smiapp_platform_data > *smiapp_get_pdata(struct device *dev) if (!ep) > return NULL; > > + bus_cfg = v4l2_of_alloc_parse_endpoint(ep); > + if (IS_ERR(bus_cfg)) { > + rval = PTR_ERR(bus_cfg); > + goto out_err; > + } > + > pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); > if (!pdata) > goto out_err; > > - v4l2_of_parse_endpoint(ep, &bus_cfg); > - > - switch (bus_cfg.bus_type) { > + switch (bus_cfg->bus_type) { > case V4L2_MBUS_CSI2: > pdata->csi_signalling_mode = SMIAPP_CSI_SIGNALLING_MODE_CSI2; > break; > @@ -3002,7 +3006,7 @@ static struct smiapp_platform_data > *smiapp_get_pdata(struct device *dev) goto out_err; > } > > - pdata->lanes = bus_cfg.bus.mipi_csi2.num_data_lanes; > + pdata->lanes = bus_cfg->bus.mipi_csi2.num_data_lanes; > dev_dbg(dev, "lanes %u\n", pdata->lanes); > > /* xshutdown GPIO is optional */ > @@ -3022,34 +3026,30 @@ static struct smiapp_platform_data > *smiapp_get_pdata(struct device *dev) dev_dbg(dev, "reset %d, nvm %d, clk > %d, csi %d\n", pdata->xshutdown, pdata->nvm_size, pdata->ext_clk, > pdata->csi_signalling_mode); > > - rval = of_get_property(ep, "link-frequencies", &asize) ? 0 : -ENOENT; > - if (rval) { > - dev_warn(dev, "can't get link-frequencies array size\n"); > + if (!bus_cfg->nr_of_link_frequencies) { Now that I see it being used, nr_of_link_frequencies feels a bit long. num_link_freqs could be an alternative. I'll let you decide. But for this patch, Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + dev_warn(dev, "no link frequencies defined\n"); > goto out_err; > } > > - pdata->op_sys_clock = devm_kzalloc(dev, asize, GFP_KERNEL); > + pdata->op_sys_clock = devm_kcalloc( > + dev, bus_cfg->nr_of_link_frequencies + 1 /* guardian */, > + sizeof(*pdata->op_sys_clock), GFP_KERNEL); > if (!pdata->op_sys_clock) { > rval = -ENOMEM; > goto out_err; > } > > - asize /= sizeof(*pdata->op_sys_clock); > - rval = of_property_read_u64_array( > - ep, "link-frequencies", pdata->op_sys_clock, asize); > - if (rval) { > - dev_warn(dev, "can't get link-frequencies\n"); > - goto out_err; > + for (i = 0; i < bus_cfg->nr_of_link_frequencies; i++) { > + pdata->op_sys_clock[i] = bus_cfg->link_frequencies[i]; > + dev_dbg(dev, "freq %d: %lld\n", i, pdata->op_sys_clock[i]); > } > > - for (; asize > 0; asize--) > - dev_dbg(dev, "freq %d: %lld\n", asize - 1, > - pdata->op_sys_clock[asize - 1]); > - > + v4l2_of_free_endpoint(bus_cfg); > of_node_put(ep); > return pdata; > > out_err: > + v4l2_of_free_endpoint(bus_cfg); > of_node_put(ep); > return NULL; > }
Hi Laurent, On Tue, Apr 07, 2015 at 01:10:20PM +0300, Laurent Pinchart wrote: > > @@ -3022,34 +3026,30 @@ static struct smiapp_platform_data > > *smiapp_get_pdata(struct device *dev) dev_dbg(dev, "reset %d, nvm %d, clk > > %d, csi %d\n", pdata->xshutdown, pdata->nvm_size, pdata->ext_clk, > > pdata->csi_signalling_mode); > > > > - rval = of_get_property(ep, "link-frequencies", &asize) ? 0 : -ENOENT; > > - if (rval) { > > - dev_warn(dev, "can't get link-frequencies array size\n"); > > + if (!bus_cfg->nr_of_link_frequencies) { > > Now that I see it being used, nr_of_link_frequencies feels a bit long. > num_link_freqs could be an alternative. I'll let you decide. But for this > patch, It's long, I agree, but still used in only a small number of places in drivers. I'd prefer to keep it as-is also as the name matches the name of the property. > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks.
diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c index 557f25d..4a2e8d3 100644 --- a/drivers/media/i2c/smiapp/smiapp-core.c +++ b/drivers/media/i2c/smiapp/smiapp-core.c @@ -2975,9 +2975,9 @@ static int smiapp_resume(struct device *dev) static struct smiapp_platform_data *smiapp_get_pdata(struct device *dev) { struct smiapp_platform_data *pdata; - struct v4l2_of_endpoint bus_cfg; + struct v4l2_of_endpoint *bus_cfg; struct device_node *ep; - uint32_t asize; + int i; int rval; if (!dev->of_node) @@ -2987,13 +2987,17 @@ static struct smiapp_platform_data *smiapp_get_pdata(struct device *dev) if (!ep) return NULL; + bus_cfg = v4l2_of_alloc_parse_endpoint(ep); + if (IS_ERR(bus_cfg)) { + rval = PTR_ERR(bus_cfg); + goto out_err; + } + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); if (!pdata) goto out_err; - v4l2_of_parse_endpoint(ep, &bus_cfg); - - switch (bus_cfg.bus_type) { + switch (bus_cfg->bus_type) { case V4L2_MBUS_CSI2: pdata->csi_signalling_mode = SMIAPP_CSI_SIGNALLING_MODE_CSI2; break; @@ -3002,7 +3006,7 @@ static struct smiapp_platform_data *smiapp_get_pdata(struct device *dev) goto out_err; } - pdata->lanes = bus_cfg.bus.mipi_csi2.num_data_lanes; + pdata->lanes = bus_cfg->bus.mipi_csi2.num_data_lanes; dev_dbg(dev, "lanes %u\n", pdata->lanes); /* xshutdown GPIO is optional */ @@ -3022,34 +3026,30 @@ static struct smiapp_platform_data *smiapp_get_pdata(struct device *dev) dev_dbg(dev, "reset %d, nvm %d, clk %d, csi %d\n", pdata->xshutdown, pdata->nvm_size, pdata->ext_clk, pdata->csi_signalling_mode); - rval = of_get_property(ep, "link-frequencies", &asize) ? 0 : -ENOENT; - if (rval) { - dev_warn(dev, "can't get link-frequencies array size\n"); + if (!bus_cfg->nr_of_link_frequencies) { + dev_warn(dev, "no link frequencies defined\n"); goto out_err; } - pdata->op_sys_clock = devm_kzalloc(dev, asize, GFP_KERNEL); + pdata->op_sys_clock = devm_kcalloc( + dev, bus_cfg->nr_of_link_frequencies + 1 /* guardian */, + sizeof(*pdata->op_sys_clock), GFP_KERNEL); if (!pdata->op_sys_clock) { rval = -ENOMEM; goto out_err; } - asize /= sizeof(*pdata->op_sys_clock); - rval = of_property_read_u64_array( - ep, "link-frequencies", pdata->op_sys_clock, asize); - if (rval) { - dev_warn(dev, "can't get link-frequencies\n"); - goto out_err; + for (i = 0; i < bus_cfg->nr_of_link_frequencies; i++) { + pdata->op_sys_clock[i] = bus_cfg->link_frequencies[i]; + dev_dbg(dev, "freq %d: %lld\n", i, pdata->op_sys_clock[i]); } - for (; asize > 0; asize--) - dev_dbg(dev, "freq %d: %lld\n", asize - 1, - pdata->op_sys_clock[asize - 1]); - + v4l2_of_free_endpoint(bus_cfg); of_node_put(ep); return pdata; out_err: + v4l2_of_free_endpoint(bus_cfg); of_node_put(ep); return NULL; }
Instead of parsing the link-frequencies property in the driver, let v4l2_of_alloc_parse_endpoint() do it. Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi> --- drivers/media/i2c/smiapp/smiapp-core.c | 40 ++++++++++++++++---------------- 1 file changed, 20 insertions(+), 20 deletions(-)