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

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: Sat, 23 Dec 2023 13:06:04 +0300
From: Nikita Shubin <nikita.shubin@...uefel.me>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
Cc: Conor Dooley <conor.dooley@...rochip.com>, Ulf Hansson
 <ulf.hansson@...aro.org>, Joel Stanley <joel@....id.au>, Walker Chen
 <walker.chen@...rfivetech.com>, Jonathan Neuschäfer
 <j.neuschaefer@....net>, Huisong Li <lihuisong@...wei.com>, Arnd Bergmann
 <arnd@...db.de>, Wei Xu <xuwei5@...ilicon.com>, Emil Renner Berthing
 <kernel@...il.dk>, Linus Walleij <linus.walleij@...aro.org>, Alexander
 Sverdlin <alexander.sverdlin@...il.com>, Hal Feng
 <hal.feng@...rfivetech.com>,  linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 08/40] soc: Add SoC driver for Cirrus ep93xx

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@...uefel.me>
> > + *
> > + * Based on a rewrite of arch/arm/mach-ep93xx/core.c
> > + * Copyright (C) 2006 Lennert Buytenhek <buytenh@...tstofly.org>
> > + * Copyright (C) 2007 Herbert Valerio Riedel <hvr@....org>
> > + *
> > + * 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.