Re: [PATCH v5 18/22] media: platform: Add Sunxi-Cedrus VPU decoder driver

[Paul Kocialkowski <paul.kocialkowski-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2573 bytes --]

Hi,

On Tue, 2018-07-10 at 10:42 +0200, Maxime Ripard wrote:
> On Tue, Jul 10, 2018 at 10:01:10AM +0200, Paul Kocialkowski wrote:
> > +static int cedrus_remove(struct platform_device *pdev)
> > +{
> > +       struct cedrus_dev *dev = platform_get_drvdata(pdev);
> > +
> > +       v4l2_info(&dev->v4l2_dev, "Removing " CEDRUS_NAME);
> 
> That log is kind of pointless.

Fair enough, I'll get rid of it.

> > +static void cedrus_hw_set_capabilities(struct cedrus_dev *dev)
> > +{
> > +	unsigned int engine_version;
> > +
> > +	engine_version = cedrus_read(dev, VE_VERSION) >> VE_VERSION_SHIFT;
> > +
> > +	if (engine_version >= 0x1667)
> > +		dev->capabilities |= CEDRUS_CAPABILITY_UNTILED;
> 
> The version used here would need a define, but I'm wondering if this
> is the right solution here. You are using at the same time the version
> ID returned by the register and the compatible in various places, and
> they are both redundant. If you want to base the capabilities on the
> compatible, then you can do it for all of those properties and
> capabilities, and if you want to use the version register, then you
> don't need all those compatibles but just one.
> 
> I think that basing all our capabilities on the compatible makes more
> sense, since you need to have access to the registers in order to read
> the version register, and this changes from one SoC generation to the
> other (for example, keeping the reset line asserted would prevent you
> from reading it, and the fact that there is a reset line depends on
> the SoC).

I concur, let's move this to a compatible-based logic instead!

> > +int cedrus_hw_probe(struct cedrus_dev *dev)
> > +{
> > +	struct resource *res;
> > +	int irq_dec;
> > +	int ret;
> > +
> > +	irq_dec = platform_get_irq(dev->pdev, 0);
> > +	if (irq_dec <= 0) {
> > +		v4l2_err(&dev->v4l2_dev, "Failed to get IRQ\n");
> > +		return -ENXIO;
> > +	}
> 
> You already have an error code returned by platform_get_irq, there's
> no point in masking it and returning -ENXIO. This can even lead to
> some bugs, for example when the error code is EPROBE_DEFER.

Right, I'll fix this and all the similar issues you mentionned.

[...]

> There's also a bunch of warnings/checks reported by checkpatch that
> should be fixed in the next iteration: the spaces after a cast, the
> NULL comparison, macros arguments precedence, parenthesis alignments
> issues, etc.)

Thanks, I'll look into that for the next revision.

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com