Re: [RFC 1/9] regmap: Add USB support
On Mon, Feb 17, 2020 at 11:15:37PM +0100, Noralf Trønnes wrote: > Den 17.02.2020 22.39, skrev Mark Brown: > > Out of interest why are 8 bit registers going to be a problem? > I have written 3 drivers so far and they all have some registers that > need to deal with values larger than 255. If I would need to add a lot > of code because of dropping regmap, then I would have looked at ways to > work around this in order to keep regmap, hi/lo registers perhaps with > wrapping access functions. But it looks like the LOC won't change much, > I need a few lines to ensure values are little endian, but I can also > remove some lines that's not needed anymore. Right, so you effectively have mixed register sizes which regmap isn't going to be super happy with (assuming you need all the actual display data to be 8 bit "registers"). One thing you could do there if you wanted to try the regmap route is to have multiple regmaps but since it's not clear what you're really getting from regmap other than the trace functionality you're probably right that it's not worth bothering. The only other thing I can think of is packing RGB into a single register so you're display data isn't 8 bit but that's probably not sensible from a graphics point of view (I didn't really look at that code so no idea what you're doing there). signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC 1/9] regmap: Add USB support
Den 17.02.2020 22.39, skrev Mark Brown: > On Mon, Feb 17, 2020 at 10:33:58PM +0100, Noralf Trønnes wrote: >> Den 17.02.2020 13.11, skrev Mark Brown: > >>> This looks like you just don't support a straight write operation, if >>> you need to do this emulation push it up the stack. > >> After going through the stack I realise that I have a problem. >> What I have failed to fully comprehend is this regmap requirement: > >> if (val_len % map->format.val_bytes) >> return -EINVAL; > >> There will be a spi and i2c driver down the line which will transfer >> buffers of any size, and having to use 8-bit register values will not be >> great. > > Out of interest why are 8 bit registers going to be a problem? > I have written 3 drivers so far and they all have some registers that need to deal with values larger than 255. If I would need to add a lot of code because of dropping regmap, then I would have looked at ways to work around this in order to keep regmap, hi/lo registers perhaps with wrapping access functions. But it looks like the LOC won't change much, I need a few lines to ensure values are little endian, but I can also remove some lines that's not needed anymore. Noralf. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC 1/9] regmap: Add USB support
On Mon, Feb 17, 2020 at 10:33:58PM +0100, Noralf Trønnes wrote: > Den 17.02.2020 13.11, skrev Mark Brown: > > This looks like you just don't support a straight write operation, if > > you need to do this emulation push it up the stack. > After going through the stack I realise that I have a problem. > What I have failed to fully comprehend is this regmap requirement: > if (val_len % map->format.val_bytes) > return -EINVAL; > There will be a spi and i2c driver down the line which will transfer > buffers of any size, and having to use 8-bit register values will not be > great. Out of interest why are 8 bit registers going to be a problem? signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC 1/9] regmap: Add USB support
Den 17.02.2020 13.11, skrev Mark Brown:
> On Sun, Feb 16, 2020 at 06:21:09PM +0100, Noralf Trønnes wrote:
>
>> Add support for regmap over USB for use with the Multifunction USB Device.
>> Two endpoints IN/OUT are used. Up to 255 regmaps are supported on one USB
>> interface. The register index width is always 32-bit, but the register
>> value can be 8, 16 or 32 bits wide. LZ4 compression is supported on bulk
>> transfers.
>> +static int regmap_usb_write(void *context, const void *data, size_t count)
>> +{
>> +struct regmap_usb_context *ctx = context;
>> +size_t val_len = count - sizeof(u32);
>> +void *val;
>> +int ret;
>> +
>> +/* buffer needs to be properly aligned for DMA use */
>> +val = kmemdup(data + sizeof(u32), val_len, GFP_KERNEL);
>> +if (!val)
>> +return -ENOMEM;
>> +
>> +ret = regmap_usb_gather_write(ctx, data, sizeof(u32), val, val_len);
>> +kfree(val);
>> +
>> +return ret;
>> +}
>
> This looks like you just don't support a straight write operation, if
> you need to do this emulation push it up the stack.
>
After going through the stack I realise that I have a problem.
What I have failed to fully comprehend is this regmap requirement:
if (val_len % map->format.val_bytes)
return -EINVAL;
There will be a spi and i2c driver down the line which will transfer
buffers of any size, and having to use 8-bit register values will not be
great.
When I started writing regmap-usb 6 months ago I didn't know where it
would take me. The result is now a Multifuntion USB device with an mfd
driver. So, no usage except for the children of the mfd device.
Dropping regmap won't lead to any increased code size to speak of, so
instead of trying to make regmap match my use case, I think I'll put
this code inside the mfd driver.
Oversights like this was one of the things I was hoping to unearth with
this RFC.
Thanks,
Noralf.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC 1/9] regmap: Add USB support
On Sun, Feb 16, 2020 at 06:21:09PM +0100, Noralf Trønnes wrote:
> Add support for regmap over USB for use with the Multifunction USB Device.
> Two endpoints IN/OUT are used. Up to 255 regmaps are supported on one USB
> interface. The register index width is always 32-bit, but the register
> value can be 8, 16 or 32 bits wide. LZ4 compression is supported on bulk
> transfers.
This looks like a custom thing for some particular devices rather than a
thing that will work for any USB device? If that is the case then this
should have a more specific name.
> +++ b/drivers/base/regmap/regmap-usb.c
> @@ -0,0 +1,1026 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Register map access API - USB support
Why is this GPL-2.0-or-later? The rest of the code is just plain old
GPL-2.0.
Please also make the entire comment a C++ one so things look more
intentional.
> +
> + ktime_t start; /* FIXME: Temporary debug/perf aid */
Add tracepoints, most of your debugging stuff looks like you want to use
tracepoints - you can easily work out time differences with them by
postprocessing the log, they get very fine grained timestamps.
> + mutex_lock(_usb_interfaces_lock);
> + list_for_each_entry(entry, _usb_interfaces, link)
> + if (entry->interface == interface) {
> + ruif = entry;
> + ruif->refcount++;
> + goto out_unlock;
> + }
> +
You're missing some { } here.
> +static long mud_drm_throughput(ktime_t begin, ktime_t end, size_t len)
> +{
> + long throughput;
> +
> + throughput = ktime_us_delta(end, begin);
> + throughput = throughput ? (len * 1000) / throughput : 0;
> + throughput = throughput * 1000 / 1024;
Please write normal conditional statements to improve legibility.
> +static int regmap_usb_gather_write(void *context,
> +const void *reg, size_t reg_len,
> +const void *val, size_t val_len)
> +{
> + return regmap_usb_transfer(context, false, reg, (void *)val, val_len);
> +}
Why are we casting away a const here? If we really need to modify the
raw data that's being transmitted take a copy.
> +static int regmap_usb_write(void *context, const void *data, size_t count)
> +{
> + struct regmap_usb_context *ctx = context;
> + size_t val_len = count - sizeof(u32);
> + void *val;
> + int ret;
> +
> + /* buffer needs to be properly aligned for DMA use */
> + val = kmemdup(data + sizeof(u32), val_len, GFP_KERNEL);
> + if (!val)
> + return -ENOMEM;
> +
> + ret = regmap_usb_gather_write(ctx, data, sizeof(u32), val, val_len);
> + kfree(val);
> +
> + return ret;
> +}
This looks like you just don't support a straight write operation, if
you need to do this emulation push it up the stack.
> + regmap_usb_debugfs_init(map);
> +
> + return map;
> +}
> +EXPORT_SYMBOL(__devm_regmap_init_usb);
No, this needs to be EXPORT_SYMBOL_GPL - the regmap core is and this
isn't going to be useful without those bits of the code anyway.
signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
