linux-kernel - Re: [PATCH 3/4] serial: max310x: make accessing revision id interface-agnostic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 31 May 2022 16:51:04 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Cosmin Tanislav <demonsingur@...il.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        "open list:SERIAL DRIVERS" <linux-serial@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Cosmin Tanislav <cosmin.tanislav@...log.com>
Subject: Re: [PATCH 3/4] serial: max310x: make accessing revision id interface-agnostic

On Tue, May 31, 2022 at 3:55 PM Cosmin Tanislav <demonsingur@...il.com> wrote:
>
> From: Cosmin Tanislav <cosmin.tanislav@...log.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