From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932317AbcARRWH (ORCPT ); Mon, 18 Jan 2016 12:22:07 -0500 Received: from mail.kmu-office.ch ([178.209.48.109]:56316 "EHLO mail.kmu-office.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932113AbcARRWF (ORCPT ); Mon, 18 Jan 2016 12:22:05 -0500 MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Date: Mon, 18 Jan 2016 09:19:52 -0800 From: Stefan Agner To: Mark Brown Cc: gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org Subject: Re: [RFC] regmap: clairify max_register meaning In-Reply-To: <20160118160557.GG6588@sirena.org.uk> References: <1453099976-9016-1-git-send-email-stefan@agner.ch> <20160118160557.GG6588@sirena.org.uk> Message-ID: User-Agent: Roundcube Webmail/1.1.3 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2016-01-18 08:05, Mark Brown wrote: > On Sun, Jan 17, 2016 at 10:52:56PM -0800, Stefan Agner wrote: > > Please submit one change per patch. You've mixed a change to the > documentation with a code here and it's really unclear what either of > them are supposed to do. Sure, one change per patch. I interpreted this issue as "one change" since the interpretation of the doc lead to this implementation issue... But I see, the issue in regcache-flat.c could be a completely independent issue. > >> Maybe I see something completely wrong here, but to me it seems >> that the regcache-flat.c is the only code interpreting max_register >> as register indexes rather then the maximum relative address... > > I don't think I understand what the above means, sorry. What is a > "relative address" in the context of a register map? My interpretation: relative address = index * stride absolute address = base address + relative address For the driver at hand I was about to set max_register to (max relative address / 4), since that matches my interpretation of index and at first sight seem to mach what is done in regcache-flat.c (it even works fine, at least for 4 byte registers).... > >> At least regmap.c compares with range_max, which seems to use >> addresses. > > That's a fairly large source file, can you be more specific please? > e.g. in __regmap_init, just after the label skip_format_initialization. (more on that see below) >> - map->cache = kcalloc(map->max_register + 1, sizeof(unsigned int), >> - GFP_KERNEL); >> + map->cache = kzalloc(map->max_register + map->reg_stride, GFP_KERNEL); > > This isn't very obvious and appears broken for a lot of devices. It > does two things, it converts from allocating an array of unsigned > integers to an array of bytes and it replaces the handling of address 0 > by adding an extra element to the array with adding stride bytes on the > end of the array. This means that for a regmap with 16 bit values and a > stride of 1 we'll end up allocating nowhere the space we need as we'll > only allocate one byte per register plus an extra byte at the end but > the implementation of the flat cache is treating the cache as an array > of unsigned ints so needs at least four bytes per register. It should > wash out as the same thing if the stride is equal to the size of an > unsigned int but most other cases will be broken. I see, agreed, my code is certainly broken if strides < sizeof(*reg_value). > > Your changelog doesn't actually say this but I think what this is trying > to do is make the cache more memory efficient by not allocating space > for the registers that don't exist due to striding (ie, if we have a 4 > byte stride then currently we'll allocate space for 4 times as many > registers as actually exist). That's a reasonable goal but I don't > immediately have a good idea for doing it bearing in mind that we are > very performance sensitive here. Yeah I saw a bug in that, but I see that this is actually just a not very space efficient implementation detail. > >> - * @max_register: Optional, specifies the maximum valid register index. >> + * @max_register: Optional, specifies the maximum valid register address. > > This is reasonable (index and address are synonyms here). Ok, in that case I clearly interpreted index wrong. Btw, range_min and range_max in struct regmap_range_cfg are defined as memory addresses: ... * @range_min: Address of the lowest register address in virtual range. * @range_max: Address of the highest register in virtual range. ... Will send out a patch for the documentation change only, if you think that this is a reasonable change too? -- Stefan