[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <13946837.RfnRDPQdge@wuerfel>
Date: Sat, 03 May 2014 17:02:37 +0200
From: Arnd Bergmann <arnd@...db.de>
To: linux-arm-kernel@...ts.infradead.org
Cc: Pankaj Dubey <pankaj.dubey@...sung.com>,
linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org,
t.figa@...sung.com, kgene.kim@...sung.com, linux@....linux.org.uk
Subject: Re: [PATCH 0/4] Introducing Exynos ChipId driver
On Saturday 03 May 2014 15:11:36 Pankaj Dubey wrote:
> This patch series attempts to get rid of soc_is_exynosXXXX macros
> and eventually with the help of this series we can probably get
> rid of CONFIG_SOC_EXYNOSXXXX in near future.
> Each Exynos SoC has ChipID block which can give information about
> SoC's product Id and revision number. Currently we have single
> DT binding information for this as "samsung,exynos4210-chipid".
> But Exynos4 and Exynos5 SoC series have one small difference in
> chip Id, with resepect to product id bit-masks. So it means we
> should have separate compatible string for these different series
> of SoCs. So I have created new binding information for handling
> this difference. Also currently I can think of putting this driver
> code under "drivers/misc/" but suggestions are welcome.
> Also current form of driver is missing platfrom driver and needs
> init function to be called from machine file (either exynos.c or
> platsmp.c). I hope lot of suggestions and comments to improve this
> further.
>
> This patch series is based on Kukjin Kim's for-next (3.14_rc1 tag)
> and prepared on top of following patch series and it's dependent
> patch series.
I think putting it into drivers/soc would be most appropriate.
We already have a few drivers lined up that we want in there,
although the directory currently doesn't exist.
However, I would ask that you use the infrastructure provided by
drivers/base/soc.c when you add this driver, to also make the
information available to user space using a standard API.
Ideally this should be done by slightly restructuring the DT
source to make all on-chip devices appear below the soc node.
We'd have to think a bit about how to best do this while
preserving compatibility with existing dts files.
Regarding patch 4, this is not what I meant when I asked for
removing the soc_is_exynos* macros. You basically do a 1:1 replacement
using a different interface, but you still have code that does
things differently based on a global identification.
The only user left in device drivers is now the cpufreq driver,
which is going to be replaced anyway, so that is ok. Having
a global variable that is accessible to random device drivers
is probably not a good idea though, it will just lead to
bad coding in drivers again.
To give an example of how I think it should really be restructured,
let's look at one function:
static const struct exynos_wkup_irq exynos4_wkup_irq[] = {
{ 76, BIT(1) }, /* RTC alarm */
{ 77, BIT(2) }, /* RTC tick */
{ /* sentinel */ },
};
static const struct exynos_wkup_irq exynos5250_wkup_irq[] = {
{ 75, BIT(1) }, /* RTC alarm */
{ 76, BIT(2) }, /* RTC tick */
{ /* sentinel */ },
};
static int exynos_irq_set_wake(struct irq_data *data, unsigned int state)
{
const struct exynos_wkup_irq *wkup_irq;
if (soc_is_exynos5250())
wkup_irq = exynos5250_wkup_irq;
else
wkup_irq = exynos4_wkup_irq;
...
}
There are multiple problems with this code:
- As mentioned, you depend on a specific SoC identification for
something that could be done completely generic.
- The knowledge about what is a wakeup source or not doesn't
really belong here. We don't have a DT binding for wakeups
as far as I'm aware of, but this should probably be handled
locally in the RTC device node, possibly in the node that
contains the S5P_WAKEUP_MASK register.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists