On 12/13/22 10:42, Alexandre Belloni wrote:
> On 13/12/2022 10:22:25-0500, Sean Anderson wrote:
>> On 12/13/22 10:18, Alexandre Belloni wrote:
>> > On 05/12/2022 10:19:18-0500, Sean Anderson wrote:
>> >> This adds support for the 256-byte internal RAM. There are two windows
>> >> which can be used to access this RAM: 64 bytes at 0x40 (the "standard"
>> >> address space) and 128 bytes at 0x80 (the "alternate" address space). We
>> >> use the standard address space because it is also accessible over SPI
>> >> (if such a port is ever done). We are limited to 32-byte reads for SMBus
>> >> compatibility, so there's no advantage to using the alternate address
>> >> space.
>> >>
>> >> There are some reserved bits in the EXTRAM register, and the datasheet
>> >> doesn't say what to do with them. I've opted to skip a read/modify/write
>> >> and just write the whole thing. If this driver is ever converted to
>> >> regmap, this would be a good place to use regmap_update_bits.
>> >>
>> >> Signed-off-by: Sean Anderson <sean.anderson@xxxxxxxx>
>> >> ---
>> >>
>> >> Changes in v2:
>> >> - Fix building on non-arm platforms
>> >>
>> >> drivers/rtc/rtc-abx80x.c | 87 ++++++++++++++++++++++++++++++++++++++++
>> >> 1 file changed, 87 insertions(+)
>> >>
>> >> diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
>> >> index 9b0138d07232..e606bf126dc3 100644
>> >> --- a/drivers/rtc/rtc-abx80x.c
>> >> +++ b/drivers/rtc/rtc-abx80x.c
>> >> @@ -11,6 +11,7 @@
>> >> */
>> >>
>> >> #include <linux/bcd.h>
>> >> +#include <linux/bitfield.h>
>> >> #include <linux/i2c.h>
>> >> #include <linux/module.h>
>> >> #include <linux/of_device.h>
>> >> @@ -87,6 +88,16 @@
>> >> #define ABX8XX_TRICKLE_STANDARD_DIODE 0x8
>> >> #define ABX8XX_TRICKLE_SCHOTTKY_DIODE 0x4
>> >>
>> >> +#define ABX8XX_REG_EXTRAM 0x3f
>> >> +#define ABX8XX_EXTRAM_XADS GENMASK(1, 0)
>> >> +
>> >> +#define ABX8XX_SRAM_BASE 0x40
>> >> +#define ABX8XX_SRAM_WIN_SIZE 0x40
>> >> +#define ABX8XX_RAM_SIZE 256
>> >> +
>> >> +#define NVMEM_ADDR_LOWER GENMASK(5, 0)
>> >> +#define NVMEM_ADDR_UPPER GENMASK(7, 6)
>> >> +
>> >> static u8 trickle_resistors[] = {0, 3, 6, 11};
>> >>
>> >> enum abx80x_chip {AB0801, AB0803, AB0804, AB0805,
>> >> @@ -673,6 +684,78 @@ static int abx80x_setup_watchdog(struct abx80x_priv *priv)
>> >> }
>> >> #endif
>> >>
>> >> +#ifdef CONFIG_NVMEM
>> >> +static int abx80x_nvmem_xfer(struct abx80x_priv *priv, unsigned int offset,
>> >> + void *val, size_t bytes, bool write)
>> >> +{
>> >> + int ret;
>> >> +
>> >> + while (bytes) {
>> >> + u8 extram, reg, len, lower, upper;
>> >> +
>> >> + lower = FIELD_GET(NVMEM_ADDR_LOWER, offset);
>> >> + upper = FIELD_GET(NVMEM_ADDR_UPPER, offset);
>> >> + extram = FIELD_PREP(ABX8XX_EXTRAM_XADS, upper);
>> >> + reg = ABX8XX_SRAM_BASE + lower;
>> >> + len = min(lower + bytes, (size_t)ABX8XX_SRAM_WIN_SIZE) - lower;
>> >> + len = min_t(u8, len, I2C_SMBUS_BLOCK_MAX);
>> >> +
>> >> + ret = i2c_smbus_write_byte_data(priv->client, ABX8XX_REG_EXTRAM,
>> >> + extram);
>> >> + if (ret)
>> >> + return ret;
>> >> +
>> >> + if (write)
>> >> + ret = i2c_smbus_write_i2c_block_data(priv->client, reg,
>> >> + len, val);
>> >> + else
>> >> + ret = i2c_smbus_read_i2c_block_data(priv->client, reg,
>> >> + len, val);
>> >> + if (ret)
>> >> + return ret;
>> >> +
>> >> + offset += len;
>> >> + val += len;
>> >> + bytes -= len;
>> >> + }
>> >> +
>> >> + return 0;
>> >> +}
>> >> +
>> >> +static int abx80x_nvmem_read(void *priv, unsigned int offset, void *val,
>> >> + size_t bytes)
>> >> +{
>> >> + return abx80x_nvmem_xfer(priv, offset, val, bytes, false);
>> >> +}
>> >> +
>> >> +static int abx80x_nvmem_write(void *priv, unsigned int offset, void *val,
>> >> + size_t bytes)
>> >> +{
>> >> + return abx80x_nvmem_xfer(priv, offset, val, bytes, true);
>> >> +}
>> >> +
>> >> +static int abx80x_setup_nvmem(struct abx80x_priv *priv)
>> >> +{
>> >> + struct device *dev = &priv->client->dev;
>> >> + struct nvmem_config config = {
>> >> + .dev = dev,
>> >> + .type = NVMEM_TYPE_BATTERY_BACKED,
>> >> + .reg_read = abx80x_nvmem_read,
>> >> + .reg_write = abx80x_nvmem_write,
>> >> + .size = ABX8XX_RAM_SIZE,
>> >> + .priv = priv,
>> >> + };
>> >> +
>> >> + return PTR_ERR_OR_ZERO(devm_nvmem_register(&priv->client->dev,
>> >> + &config));
>> >
>> > Is there any reason why you are not using devm_rtc_nvmem_register ?
>> >
>> >
>>
>> I didn't know it existed. To be honest, it doesn't seem to be doing much.
>
> It properly sets the owner which is going to save you from race
> conditions and headaches.
nvmem_register has
nvmem->owner = config->owner;
if (!nvmem->owner && config->dev->driver)
nvmem->owner = config->dev->driver->owner;
which is going to get set to THIS_MODULE because we set dev. This is the
same thing that rtc->owner gets set to, so there's no difference.
> It also removes the need for the ifdefery.
This is just following the style of the watchdog code, which is
similarly ifdef'd out.
--Sean
>> With that changed, is the rest of the patch OK?
>>
>
> find the use of FIELD_* a bit too much but this is fine, yes.
>
