linux-kernel - Re: RFC: MEDIA: Driver for ON Semi AR0521 camera sensor

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <m35z1qs4cf.fsf@t19.piap.pl>
Date:   Wed, 17 Mar 2021 07:04:48 +0100
From:   Krzysztof Hałasa <khalasa@...p.pl>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: RFC: MEDIA: Driver for ON Semi AR0521 camera sensor

Laurent Pinchart <laurent.pinchart@...asonboard.com> writes:

>> Is there a reliable way to include national unicode characters in the
>> kernel sources?
>
> It depends where. In comments it shouldn't be a problem. In C code, I
> don't think the compiler will be too happy.

I meant in comments, sure. And in stuff like MODULE_AUTHOR.
I know gcc will handle it correctly, but the problem is getting
the unicode characters right through mail.

> Signed-off-by means that you have the right to submit the code for
> upstreaming, so it should be included in patches under review too.
> Otherwise it's a waste of time for reviewers to review something that
> may never be resubmitted with an SoB line.

I know. I obviously have rights to upstream this code. The problem is
when I publish a patch with a SOB line, anyone can take it, "make
a derivative work" (so to speak), and submit as his own. The new patch
doesn't need to be an improvement, the changes from the original are not
even looked upon. Been there BTW.

>> +#define AR0521_WIDTH_MIN	       8u
>
> We usually use an uppercase U suffix.
>> +#define AR0521_REG_RESET			0x301A
>
> But lowercase hex values. I know, lots of tribal (and sometimes
> arbitrary) knowledge :-S

Right. Is there a consensus about it?
I use lowercase U because it contrasts with "uppercase" digits, and
uppercase hex letter for consistency with (decimal) digits, but I can
change it if it's only me.

>> +	regs[0] = AR0521_REG_GREEN1_GAIN;
>> +	regs[1] = green << 7 | analog;
>> +	regs[2] = blue  << 7 | analog;
>> +	regs[3] = red   << 7 | analog;
>> +	regs[4] = green << 7 | analog;
>> +
>> +	return ar0521_write_regs(sensor, regs, ARRAY_SIZE(regs));
>
> Passing the values in an array, with the first entry being a register
> address, is a really weird API. I'd recommend either using regmap (may
> be overkill here), or use a write function that takes the register
> address and value as separate arguments. If we want to avoid sending the
> register address for each write as a performance improvement, we'll have
> to figure out what a good API would be to do so, but more importantly,
> it would be good to have numbers to justify why this would be needed.

It's a slow I^2C device. Doing a single write transfer is faster than
a series of transfers, especially on a busy bus. Do I really have to
justify why I like a faster and more efficient code?
And it's not a big API, it's just a small internal driver subroutine.
Would splitting it to several ar0521_write_reg() be better, e.g. more
readable?

>> +static int ar0521_set_mode(struct ar0521_dev *sensor)
>> +{
>> +	unsigned int speed_mod = 4 / lanes(sensor); /* 1 with 4 DDR lanes */
>> +	u64 pix_clk;
>> +	u32 pixels, pll2, num, denom, new_total_height, new_pixels;
>> +	u16 total_width, total_height, x, y, pre, mult, pre2, mult2, extra_delay;
>> +	u16 regs[9];
>> +	int ret;
>> +
>> +	/* stop streaming first */
>> +	ret = ar0521_write_reg(sensor, AR0521_REG_RESET, AR0521_REG_RESET_DEFAULTS);
>
> set_format isn't supposed to stop streaming implicitly. It should
> instead return -EBUSY if the stream if running.

It doesn't stop permanently, it's restarted after the registers are
updated. No need for -EBUSY here.

>> +static int ar0521_s_stream(struct v4l2_subdev *sd, int enable)
>> +{
>> +	struct ar0521_dev *sensor = to_ar0521_dev(sd);
>> +	int ret;
>> +
>> +	mutex_lock(&sensor->lock);
>
> Could you please use runtime PM for power management, enabling the clock
> and regulators when starting streaming ?
>
> I forgot to mention in the review of the DT bindings that regulators
> should be specified in DT.

Why? The hw using this driver doesn't have capability to disable
regulators. If someone produces hw with means to control power, the sw
support can be trivially added. When I last checked, we didn't add
driver code for functionality for which no hw support exists, did we?
-- 
Krzysztof Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa