Re: [PATCH] aiodev: add driver for Rockchip SARADC

[Michael Riesch <michael.riesch@wolfvision.net>]

Hello Sascha,

thanks for the review, I'll prepare a v2.

On 6/21/21 6:30 AM, Sascha Hauer wrote:
> Hi Michael,
> 
> On Thu, Jun 17, 2021 at 04:36:54PM +0200, Michael Riesch wrote:
>> +static int rockchip_saradc_read(struct aiochannel *chan, int *val)
>> +{
>> +	struct rockchip_saradc_data *data;
>> +	int timeout = SARADC_TIMEOUT;
>> +	u32 value = 0;
>> +	u32 control = 0;
>> +	u32 mask;
>> +
>> +	if (!chan || !val)
>> +		return -EINVAL;
>> +
>> +	data = container_of(chan->aiodev, struct rockchip_saradc_data, aiodev);
>> +	if (!data)
>> +		return -EINVAL;
>> +
>> +	rockchip_saradc_reg_wr(data, 8, SARADC_DLY_PU_SOC);
>> +	rockchip_saradc_reg_wr(data,
>> +			       (chan->index & SARADC_CTRL_CHN_MASK) |
>> +				       SARADC_CTRL_IRQ_ENABLE |
>> +				       SARADC_CTRL_POWER_CTRL,
>> +			       SARADC_CTRL);
>> +
>> +	do {
>> +		control = rockchip_saradc_reg_rd(data, SARADC_CTRL);
>> +
>> +		if (--timeout == 0)
>> +			return -ETIMEDOUT;
>> +		mdelay(1);
>> +	} while (!(control & SARADC_CTRL_IRQ_STATUS));
> 
> You should do a timeout loop with
> 
> 	u64 start = get_time_ns();
> 
> 	do {
> 		if (is_timeout(start, 100 * MSECOND)
> 			return -ETIMEDOUT;
> 	} while(bar);
> 
> We don't have any way nor need to put the CPU into idle, so we can poll
> as fast as we can.

OK, will replace that.

>> +
>> +	mask = (1 << data->config->num_bits) - 1;
>> +	value = rockchip_saradc_reg_rd(data, SARADC_DATA) & mask;
>> +	rockchip_saradc_reg_wr(data, 0, SARADC_CTRL);
>> +
>> +	printf("Raw value: %d\n", value);
> 
> Debugging leftover.

D'oh!

>> +fail_channels:
>> +	kfree(data->channels);
>> +	kfree(data->aiodev.channels);
>> +
>> +fail_data:
>> +	kfree(data);
>> +	return ret;
>> +}
>> +
>> +static const struct rockchip_saradc_cfg rk3568_saradc_cfg = {
>> +	.ref_voltage_mv = 1800,
> 
> From looking at the downstream dts files this doesn't seem to be SoC
> specific:
> 
> 	&saradc {
>         	status = "okay";
> 		vref-supply = <&vcca_1v8>;
> 	};
> 
> I suggest doing the same here.
> 
>> +	.num_bits = 10,
>> +	.num_channels = 8,
>> +};
>> +
>> +static const struct of_device_id of_rockchip_saradc_match[] = {
>> +	{ .compatible = "rockchip,rk3568-saradc", .data = &rk3568_saradc_cfg },
> 
> The device nodes in the downstream Kernel also contain some clocks.
> These should be handled in the driver.

I'll see what I can do :-)

Just had a look over the mainstream Kernel device tree. Resets,
interrupts and number of io-channels are also provided there. Now I
think that the interrupts are quite irrelevant for barebox, but should
the resets be addressed as well?

As to the number of iochannels, I think the maximum number should be
provided in the SoC specific struct. Not sure what the value of
specifying the actual number of channels in the device tree is, though.

Regards, Michael

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox