lkml.org 
[lkml]   [2022]   [May]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH 3/4] serial: max310x: make accessing revision id interface-agnostic
On Tue, May 31, 2022 at 3:55 PM Cosmin Tanislav <demonsingur@gmail.com> wrote:
>
> From: Cosmin Tanislav <cosmin.tanislav@analog.com>
>
> SPI can only use 5 address bits, since one bit is reserved for
> specifying R/W and 2 bits are used to specify the UART port.
> To access registers that have addresses past 0x1F, an extended
> register space can be enabled by writing to the GlobalCommand
> register (address 0x1F).
>
> I2C uses 8 address bits. The R/W bit is placed in the slave
> address, and so is the UART port. Because of this, registers
> that have addresses higher than 0x1F can be accessed normally.
>
> To access the RevID register, on SPI, 0xCE must be written to
> the 0x1F address to enable the extended register space, after
> which the RevID register is accessible at address 0x5. 0xCD
> must be written to the 0x1F address to disable the extended
> register space.
>
> On I2C, the RevID register is accessible at address 0x25.
>
> Create an interface config struct, and add a method for
> toggling the extended register space and a member for the RevId
> register address. Implement these for SPI.

...

> struct max310x_port {
> const struct max310x_devtype *devtype;
> + const struct max310x_if_cfg *if_cfg;
> struct regmap *regmap;

I believe the most used pointer is regmap and putting it to be a first
member will make pointer arithmetic no-op at compile time. That said,
adding new member is better after this one.

> struct clk *clk;

...

> + ret = s->if_cfg->set_ext_reg_en(dev, true);

It sounds like a voodoo speech. Can we name the callback better?
->extended_reg_enable() ?

> if (ret)
> return ret;

...

> static int max310x_probe(struct device *dev, const struct max310x_devtype *devtype,
> + const struct max310x_if_cfg *if_cfg,
> struct regmap **regmaps, int irq)

It should be commented on the other patch, but since I can't see it in
my mailbox (yet) I put it here. So,
looking into usage of regmaps parameter it logically should be declared as

struct regmap *regmaps[]

(yes, I know that there is no difference for the compiler, but code
human reader).

...

> - return max310x_probe(&spi->dev, devtype, regmaps, spi->irq);
> + return max310x_probe(&spi->dev, devtype, &max310x_spi_if_cfg, regmaps,
> + spi->irq);

Can still be on one line, no?

--
With Best Regards,
Andy Shevchenko

\
 
 \ /
  Last update: 2022-05-31 16:53    [W:0.055 / U:0.452 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site