lkml.org 
[lkml]   [2018]   [Apr]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 5/9] media: platform: Add Sunxi Cedrus decoder driver
    From
    Date
    Hi and thanks for the review,

    On Fri, 2018-03-09 at 14:57 +0100, Maxime Ripard wrote:
    > On Fri, Mar 09, 2018 at 11:14:41AM +0100, Paul Kocialkowski wrote:
    > > +/*
    > > + * mem2mem callbacks
    > > + */
    > > +
    > > +void job_abort(void *priv)
    > > +{}
    >
    > Is that still needed?

    v2 contains a proper implementation of job abortion, so yes :)

    > > +/*
    > > + * device_run() - prepares and starts processing
    > > + */
    > > +void device_run(void *priv)
    > > +{
    >
    > This function (and the one above) should probably made static. Or at
    > least if you can't, they should have a much more specific name in
    > order not to conflict with anything from the core.

    Agreed, will fix in v2.

    > > + /*
    > > + * The VPU is only able to handle bus addresses so we have
    > > to subtract
    > > + * the RAM offset to the physcal addresses
    > > + */
    > > + in_buf -= PHYS_OFFSET;
    > > + out_luma -= PHYS_OFFSET;
    > > + out_chroma -= PHYS_OFFSET;
    >
    > You should take care of that by putting it in the dma_pfn_offset field
    > of the struct device (at least before we come up with something
    > better).
    >
    > You'll then be able to use the dma_addr_t directly without modifying
    > it.

    Ditto.

    > > + vpu->syscon = syscon_regmap_lookup_by_phandle(vpu->dev-
    > > >of_node,
    > > + "syscon");
    > > + if (IS_ERR(vpu->syscon)) {
    > > + vpu->syscon = NULL;
    > > + } else {
    > > + regmap_write_bits(vpu->syscon,
    > > SYSCON_SRAM_CTRL_REG0,
    > > + SYSCON_SRAM_C1_MAP_VE,
    > > + SYSCON_SRAM_C1_MAP_VE);
    > > + }
    >
    > This should be using our SRAM controller driver (and API), see
    > Documentation/devicetree/bindings/sram/sunxi-sram.txt
    > include/linux/soc/sunxi/sunxi_sram.h

    This will require adding support for the VE (and the A33 along the way)
    in the SRAM driver, so a dedicated patch series will be sent in this
    direction eventually.

    > > + ret = clk_prepare_enable(vpu->ahb_clk);
    > > + if (ret) {
    > > + dev_err(vpu->dev, "could not enable ahb clock\n");
    > > + return -EFAULT;
    > > + }
    > > + ret = clk_prepare_enable(vpu->mod_clk);
    > > + if (ret) {
    > > + clk_disable_unprepare(vpu->ahb_clk);
    > > + dev_err(vpu->dev, "could not enable mod clock\n");
    > > + return -EFAULT;
    > > + }
    > > + ret = clk_prepare_enable(vpu->ram_clk);
    > > + if (ret) {
    > > + clk_disable_unprepare(vpu->mod_clk);
    > > + clk_disable_unprepare(vpu->ahb_clk);
    > > + dev_err(vpu->dev, "could not enable ram clock\n");
    > > + return -EFAULT;
    > > + }
    >
    > Ideally, this should be using runtime_pm to manage the device power
    > state, and disable it when not used.
    >
    > > + reset_control_assert(vpu->rstc);
    > > + reset_control_deassert(vpu->rstc);
    >
    > You can use reset_control_reset here

    Will do in v2.

    > > + return 0;
    > > +}
    > > +
    > > +void sunxi_cedrus_hw_remove(struct sunxi_cedrus_dev *vpu)
    > > +{
    > > + clk_disable_unprepare(vpu->ram_clk);
    > > + clk_disable_unprepare(vpu->mod_clk);
    > > + clk_disable_unprepare(vpu->ahb_clk);
    >
    > The device is not put back into reset here

    Good catch!

    Cheers,

    --
    Paul Kocialkowski, Bootlin (formerly Free Electrons)
    Embedded Linux and kernel engineering
    https://bootlin.com[unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2018-04-19 17:00    [W:2.616 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site