Re: [PATCH v6 08/40] soc: Add SoC driver for Cirrus ep93xx

[Nikita Shubin]

On Wed, 2023-12-13 at 20:37 +0200, Andy Shevchenko wrote:
> On Tue, Dec 12, 2023 at 11:20:25AM +0300, Nikita Shubin wrote:
> > Add an SoC driver for the ep93xx. Currently there is only one thing
> > not fitting into any other framework, and that is the swlock
> > setting.
> 
> ...
> 
> > +/*
> > + * SoC driver for Cirrus EP93xx chips.
> > + * Copyright (C) 2022 Nikita Shubin <nikita.shubin@xxxxxxxxxxx>
> > + *
> > + * Based on a rewrite of arch/arm/mach-ep93xx/core.c
> > + * Copyright (C) 2006 Lennert Buytenhek <buytenh@xxxxxxxxxxxxxx>
> > + * Copyright (C) 2007 Herbert Valerio Riedel <hvr@xxxxxxx>
> > + *
> > + * Thanks go to Michael Burian and Ray Lehtiniemi for their key
> > + * role in the ep93xx Linux community
> 
> Missing period.
> 
> > + */
> 
> ...
> 
> > +#include <linux/bits.h>
> > +#include <linux/init.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/of.h>
> > +#include <linux/of_fdt.h>
> > +#include <linux/platform_device.h>
> 
> Isn't this an incorrect header and should be auxiliary one?

This is still a platform driver, pinctrl, clk and reset are auxiliary
ones.

> 
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> 
> + spinlock.h ?
> 
> But since it's a new code, why not cleanup.h?
> 
> > +#include <linux/sys_soc.h>
> 
> ...
> 
> > +       enum ep93xx_soc_model model =
> > (int)(uintptr_t)of_device_get_match_data(&pdev->dev);
> 
> int?
> 
> Maybe
> 
>         strict device *dev = &pdev->dev;
>         enum ep93xx_soc_model model;
>         ...
>         model = (enum
> ep93xx_soc_model)(uintptr_t)device_get_match_data(dev);
> 
> ?
> 
> ...
> 
> > +       struct device *dev = &pdev->dev;
> 
> Ah you even have this already!
> 
> ...
> 
> > +       dev_info(dev, "EP93xx SoC revision %s\n", attrs->revision);
> 
> Hmm... Is this message anyhow useful?
> 

Can we keep it please ? It makes us happy when we see it in logs for
historical reasons - it's been there since 2.4.