by Nikita Shubin

[permalink] [raw]

Subject: Re: [PATCH v6 20/40] mtd: rawnand: add support for ts72xx

Hello Greg!

On Tue, 2023-12-12 at 09:30 +0100, Greg Kroah-Hartman wrote:
> On Tue, Dec 12, 2023 at 11:20:37AM +0300, Nikita Shubin via B4 Relay
> wrote:
> > From: Nikita Shubin <[email protected]>
> >
> > Technologic Systems has it's own nand controller implementation in
> > CPLD.
> >
> > Signed-off-by: Nikita Shubin <[email protected]>
> > ---
> > drivers/mtd/nand/raw/Kconfig                       |   7 +
> > drivers/mtd/nand/raw/Makefile                      |   1 +
> > drivers/mtd/nand/raw/technologic-nand-controller.c | 220
> > +++++++++++++++++++++
> > 3 files changed, 228 insertions(+)
> >
> > diff --git a/drivers/mtd/nand/raw/Kconfig
> > b/drivers/mtd/nand/raw/Kconfig
> > index cbf8ae85e1ae..3937c10dea1c 100644
> > --- a/drivers/mtd/nand/raw/Kconfig
> > +++ b/drivers/mtd/nand/raw/Kconfig
> > @@ -449,6 +449,13 @@ config MTD_NAND_RENESAS
> >           Enables support for the NAND controller found on Renesas
> > R-Car
> >           Gen3 and RZ/N1 SoC families.
> >
> > +config MTD_NAND_TS72XX
> > +       tristate "ts72xx NAND controller"
> > +       depends on ARCH_EP93XX && HAS_IOMEM
> > +       help
> > +         Enables support for NAND controller on ts72xx SBCs.
> > +         This is a legacy driver based on gen_nand.
>
> Why is a new "legacy driver" being written these days? Why not do it
> properly?

Sorry, my bad, description is a leftover from older description (when i
was allowed to submit a legacy one).

The driver is mostly new, shiny and approved by Miquel:

https://lore.kernel.org/lkml/20231004103911.2aa65354@xps-13/

The KConfig description should be fixed.

>
> thanks,
>
> greg k-h

2023-12-12 11:55:59

On Wed, Dec 13, 2023 at 08:16:26PM +0200, Andy Shevchenko wrote:
> On Tue, Dec 12, 2023 at 11:20:53AM +0300, Nikita Shubin wrote:
> > Drop legacy acquire/release since we are using pinctrl for this now.
>
> ...
>
> > - if (IS_ERR(drv_data->dma_rx_channel)) {
> > + if (PTR_ERR_OR_ZERO(drv_data->dma_rx_channel)) {
>
> This seems incorrect.
>
> > ret = PTR_ERR(drv_data->dma_rx_channel);
> > return dev_err_probe(dev, ret, "rx DMA setup failed");
>
> return dev_err_probe(...);
>
> > }
>
> I think you wanted
>
> ret = PTR_ERR_OR_ZERO(drv_data->dma_rx_channel);
> if (ret)
> return dev_err_probe(dev, ret, "rx DMA setup failed");

How is that different from

if (IS_ERR(drv_data->dma_rx_channel))
return dev_err_probe(dev, PTR_ERR(drv_data->dma_rx_channel), "....");

(which seems to be more idiomatic to me)? While I was involved in
creating PTR_ERR_OR_ZERO, I don't particularily like it (today).

Also note that you want a \n at the end of error messages.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |

Attachments:

(No filename) (1.17 kB)
signature.asc (499.00 B)
Download all attachments

2023-12-13 18:40:18

by Andy Shevchenko

[permalink] [raw]

Subject: Re: [PATCH v6 18/40] net: cirrus: add DT support for Cirrus EP93xx

On Tue, Dec 12, 2023 at 11:20:35AM +0300, Nikita Shubin wrote:
> - add OF ID match table
> - get phy_id from the device tree, as part of mdio
> - copy_addr is now always used, as there is no SoC/board that aren't
> - dropped platform header

...

> mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> irq = platform_get_irq(pdev, 0);
> if (!mem || irq < 0)
> return -ENXIO;

Strictly speaking these should be separated and different codes being retuned as
platform_get_irq() might return something different in some cases.

irq = platform_get_irq(pdev, 0);
if (irq < 0)
return irq;

> + base_addr = ioremap(mem->start, resource_size(mem));
> + if (!base_addr)
> + return dev_err_probe(&pdev->dev, -EIO, "Failed to ioremap ethernet registers\n");

Hmm... Why not devm_platform_ioremap_resource()?

--
With Best Regards,
Andy Shevchenko

2023-12-13 18:42:17

by Andy Shevchenko

[permalink] [raw]

Subject: Re: [PATCH v6 24/40] input: keypad: ep93xx: add DT support for Cirrus EP93xx

On Tue, Dec 12, 2023 at 11:20:41AM +0300, Nikita Shubin wrote:
> - drop flags, they were not used anyway
> - add OF ID match table
> - process "autorepeat", "debounce-delay-ms", prescale from device tree
> - drop platform data usage and it's header
> - keymap goes from device tree now on

...

> static void ep93xx_keypad_config(struct ep93xx_keypad *keypad)
> {
> unsigned int val = 0;
>
> + val |= ((keypad->debounce << KEY_INIT_DBNC_SHIFT) & KEY_INIT_DBNC_MASK);

Since you are touching these lines (see below) you can drop unneeded outer
parentheses.

>
> + val |= ((keypad->prescale << KEY_INIT_PRSCL_SHIFT) & KEY_INIT_PRSCL_MASK);

See above.

> __raw_writel(val, keypad->mmio_base + KEY_INIT);
> }

--
With Best Regards,
Andy Shevchenko

2023-12-13 18:49:12

by Andy Shevchenko

[permalink] [raw]

Subject: Re: [PATCH v6 36/40] ata: pata_ep93xx: remove legacy pinctrl use

On Wed, Dec 13, 2023 at 07:33:49PM +0100, Uwe Kleine-K?nig wrote:
> On Wed, Dec 13, 2023 at 08:16:26PM +0200, Andy Shevchenko wrote:
> > On Tue, Dec 12, 2023 at 11:20:53AM +0300, Nikita Shubin wrote:
> > > Drop legacy acquire/release since we are using pinctrl for this now.

...

> > > - if (IS_ERR(drv_data->dma_rx_channel)) {
> > > + if (PTR_ERR_OR_ZERO(drv_data->dma_rx_channel)) {
> >
> > This seems incorrect.
> >
> > > ret = PTR_ERR(drv_data->dma_rx_channel);
> > > return dev_err_probe(dev, ret, "rx DMA setup failed");
> >
> > return dev_err_probe(...);
> >
> > > }
> >
> > I think you wanted
> >
> > ret = PTR_ERR_OR_ZERO(drv_data->dma_rx_channel);
> > if (ret)
> > return dev_err_probe(dev, ret, "rx DMA setup failed");
>
> How is that different from
>
> if (IS_ERR(drv_data->dma_rx_channel))
> return dev_err_probe(dev, PTR_ERR(drv_data->dma_rx_channel), "....");
>
> (which seems to be more idiomatic to me)? While I was involved in
> creating PTR_ERR_OR_ZERO, I don't particularily like it (today).

Makes lines shorter, either works for me.

> Also note that you want a \n at the end of error messages.

Indeed.

--
With Best Regards,
Andy Shevchenko

2023-12-14 10:27:36

On Sat, Dec 23, 2023 at 11:13 AM Nikita Shubin
<[email protected]> wrote:
> On Wed, 2023-12-13 at 19:59 +0200, Andy Shevchenko wrote:

...

> I haven't found any missing tags, that b4 didn't apply, the ones above
> refer to a very old iteration and were given to cover letter and i
> don't feel like they need to be included.

When somebody gives you a tag against a cover letter, it means the
entire series (if not spelled differently). `b4` even has a parameter
-t for that IIRC.

--
With Best Regards,
Andy Shevchenko