On 1/11/19 5:04 AM, tristram...@microchip.com wrote: >> On 1/10/19 3:10 AM, tristram...@microchip.com wrote: >>>>> I just looked at your regmap code and you use 3 regmap pointers for >>>> specific 8-bit, 16-bit, and 32-bit accesses. The switch access is always >>>> 8-bit. >> It >>>> has automatic register increment so that you can access arbitrary length of >>>> registers. The use of 16-bit and 32-bit accesses makes access efficient >>>> if it >>>> makes sense. >>>> >>>> Right, that's what happens here. >>>> >>>>> Most older switches define registers in 8-bit. Exceptions are the default >>>> VID and indirect access. >>>>> >>>>> A specific switch mostly defines registers in 16-bit because it shares the >>>> core design with an Ethernet controller. >>>>> >>>>> KSZ9477 is the newer designed switch and it gets some designs from >> older >>>> switches and that is why it has a mix of 8-bit, 16-bit, and 32-bit register >>>> definitions. >>>> >>>> Right, that's quite horrible. >>>> >>>>> In my code I just use regmap_bulk_read and regmap_bulk_write and >> still >>>> use the old spi access functions for specific 8-bit, 16-bit, and 32-bit >> accesses. >>>> >>>> Let's not mix regmap and non-regmap accesses, that'd be a mess. Let's >>>> stick to one, regmap. >>>> >>>>> We can combine the logic of ksz_spi_read8 and others into ksz_read8 >> and >>>> so they can be used for both SPI and I2C accesses. >>>> >>>> You can just use regmap_*() accessors and regmap will deal with i2c/spi >>>> abstraction for you, that's the idea. >>>> >>> >>> What I meant is I use bulk_read as a base and modify it to access 16-bit and >> 32-bit instead of using regmap[1] and regmap[2]. We can keep regmap[2] >> for 32-bit access just for the regmap_update_bits function. >>> >>> I intend to keep the 3 regmap pointers as a specific switch actually >>> requires >> those specific accesses as it does not have automatic register increment. >> >> I don't think the bulk read is a good idea due to register alignment. >> You see, with bulk read, the user might try to read two bytes from the >> middle of 4 byte register and I'm not sure how the HW would like that. >> If we have 32bit regmap for 32bit registers, all behaves as expected. > > I just changed bulk_read to raw_read as it is more correct. > > The switch access is 8-bit and so there is no restriction on accessing > registers. > Of course some registers like PHY registers are better accessed in 16-bit, > but you can write the high byte or low byte without problem. > > Actually the hardware has some bugs that requires writing in 32-bit for some > PHY related registers. The driver has to make sure to write in the correct > way to have the right result.
OK, so there are clearly restrictions to what can be written and how. > My point is the driver is the only one who is using these functions to write, > so the developer does not try to write the register in the wrong way. > > It turns out the switch that requires exact 8-bit, 16-bit, and 32-bit access > functions does not work using the regmap mechanism without additional > register manipulation, so we do not really need 3 regmap pointers. Can you elaborate on this ? > One benefit of having 32-bit access is the register dump from debugfs can be > much shorter than using 8-bit. You can constraint each regmap definition and have it allow only certain types of accesses to a selected set of registers, so then your debugfs regmap dump would match the register list in the datasheet. I didn't add it into this initial series for simplicity's sake though. -- Best regards, Marek Vasut
