[linux-sunxi] Re: [PATCH 2/4] ARM: dts: sun7i: Add keypad node to Allwinner A20 SoC

2015-09-17 Thread Maxime Ripard 
Hi Yassin,

On Wed, Sep 16, 2015 at 12:05:55AM +1000, yassinjaf...@gmail.com wrote:
> From: Yassin Jaffer 
> 
> Add Keypad controller node definition to the A20 SoC.
> 
> Signed-off-by: Yassin Jaffer 
> ---
>  arch/arm/boot/dts/sun7i-a20.dtsi | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi 
> b/arch/arm/boot/dts/sun7i-a20.dtsi
> index 333604a..35cc8d0 100644
> --- a/arch/arm/boot/dts/sun7i-a20.dtsi
> +++ b/arch/arm/boot/dts/sun7i-a20.dtsi
> @@ -1198,6 +1198,15 @@
>   status = "disabled";
>   };
>  
> + kp: kp@01c23000 {

The node name should reflect the class of the device. keypad@01c23000
would be better for example.

It looks good otherwise.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: Digital signature

[linux-sunxi] Re: [PATCH 3/4] input: Add new sun4i-keypad driver

2015-09-17 Thread Maxime Ripard 
Hi,

On Wed, Sep 16, 2015 at 12:05:56AM +1000, yassinjaf...@gmail.com wrote:
> From: Yassin Jaffer 
> 
> Allwinnner SUN4i Keypad controller is used to interface a SoC
> with a matrix-typekeypad device.
> The keypad controller supports multiple row and column lines.
> A key can be placed at each intersection of a unique
> row and a unique column.
> The keypad controller can sense a key-press and key-release and report the
> event using a interrupt to the cpu.
> This patch adds a driver support to this.
> The keypad controller driver does not give proper information
> if more that two keys are selected.
> 
> Signed-off-by: Yassin Jaffer 
> ---
>  drivers/input/keyboard/Kconfig|  11 ++
>  drivers/input/keyboard/Makefile   |   1 +
>  drivers/input/keyboard/sun4i-keypad.c | 361 
> ++
>  3 files changed, 373 insertions(+)
>  create mode 100644 drivers/input/keyboard/sun4i-keypad.c
> 
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 2e80107..4f2f3f8 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -590,6 +590,17 @@ config KEYBOARD_SUN4I_LRADC
> To compile this driver as a module, choose M here: the
> module will be called sun4i-lradc-keys.
>  
> +config KEYBOARD_SUN4I_KEYPAD
> + tristate "Allwinner sun4i keypad support"
> + depends on ARCH_SUNXI

Is this IP found on all the know SoCs, or just a subset of them?

You probably want to add || COMPILE_TEST in that depends on too.

> + select INPUT_MATRIXKMAP
> + help
> +   This selects support for the Allwinner keypad
> +   on Allwinner sunxi SoCs.
> +
> +   To compile this driver as a module, choose M here: the
> +   module will be called sun4i-keypad.
> +
>  config KEYBOARD_DAVINCI
>   tristate "TI DaVinci Key Scan"
>   depends on ARCH_DAVINCI_DM365
> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> index 1d416dd..d9f54b4 100644
> --- a/drivers/input/keyboard/Makefile
> +++ b/drivers/input/keyboard/Makefile
> @@ -57,6 +57,7 @@ obj-$(CONFIG_KEYBOARD_STMPE)+= 
> stmpe-keypad.o
>  obj-$(CONFIG_KEYBOARD_STOWAWAY)  += stowaway.o
>  obj-$(CONFIG_KEYBOARD_ST_KEYSCAN)+= st-keyscan.o
>  obj-$(CONFIG_KEYBOARD_SUN4I_LRADC)   += sun4i-lradc-keys.o
> +obj-$(CONFIG_KEYBOARD_SUN4I_KEYPAD)  += sun4i-keypad.o
>  obj-$(CONFIG_KEYBOARD_SUNKBD)+= sunkbd.o
>  obj-$(CONFIG_KEYBOARD_TC3589X)   += tc3589x-keypad.o
>  obj-$(CONFIG_KEYBOARD_TEGRA) += tegra-kbc.o
> diff --git a/drivers/input/keyboard/sun4i-keypad.c 
> b/drivers/input/keyboard/sun4i-keypad.c
> new file mode 100644
> index 000..995f9665
> --- /dev/null
> +++ b/drivers/input/keyboard/sun4i-keypad.c
> @@ -0,0 +1,361 @@
> +/*
> + * Allwinner sun4i keypad Controller driver
> + *
> + * Copyright (C) 2015 Yassin Jaffer 
> + *
> + * Parts of this software are based on (derived from):
> + * Copyright (C) 2013-2015 lim...@allwinnertech.com,
> + *qys
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/*
> + * Keypad Controller registers
> + */
> +#define KP_CTL   0x00  /* Keypad Control register */
> +#define KP_TIMING0x04  /* Keypad Timing register */
> +#define KP_INT_CFG   0x08  /* Keypad interrupt Config register */
> +#define KP_INT_STA   0x0c  /* Keypad interrupt Status register */
> +
> +#define KP_IN_OFFSET 0x10 /* Keypad Input Data register 0 */
> +#define KP_INX_OFFSET(reg_n) (KP_IN_OFFSET + 4 * (reg_n))
> +
> +/* KP_CTL bits */
> +#define ENABLE(x)((x) << 0)
> +#define COLMASK(x)   ((~x & 0xff) << 8)
> +#define ROWMASK(x)   ((~x & 0xff) << 16)

Having the name of the register in that define would be great, to spot
more easily issues (writing a value to another register, for example).

Something like KP_CTL_ENABLE(x) in this case.

> +/* KP_TIMING bits */
> +#define SCAN_CYCLE(x)((x) << 0)
> +#define DBC_CYCLE(x) ((x) << 16)
> +
> +/* KP_INT_CFG bits */
> +#define KP_IRQ_FEDGE BIT(0)
> +#define  KP_IRQ_REDGEBIT(1)
> +
> +/* KP_INT_STA bits */
> +#define KP_STA_FEDGE BIT(0)

Re: [linux-sunxi] Re: [PATCH 2/4] ARM: dts: sun7i: Add keypad node to Allwinner A20 SoC

2015-09-17 Thread Chen-Yu Tsai 
On Thu, Sep 17, 2015 at 7:29 PM, Maxime Ripard
 wrote:
> Hi Yassin,
>
> On Wed, Sep 16, 2015 at 12:05:55AM +1000, yassinjaf...@gmail.com wrote:
>> From: Yassin Jaffer 
>>
>> Add Keypad controller node definition to the A20 SoC.
>>
>> Signed-off-by: Yassin Jaffer 
>> ---
>>  arch/arm/boot/dts/sun7i-a20.dtsi | 9 +
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi 
>> b/arch/arm/boot/dts/sun7i-a20.dtsi
>> index 333604a..35cc8d0 100644
>> --- a/arch/arm/boot/dts/sun7i-a20.dtsi
>> +++ b/arch/arm/boot/dts/sun7i-a20.dtsi
>> @@ -1198,6 +1198,15 @@
>>   status = "disabled";
>>   };
>>
>> + kp: kp@01c23000 {
>
> The node name should reflect the class of the device. keypad@01c23000
> would be better for example.

Expanding the label to "keypad" as well would be nice.
"kp" could mean other things.

Thanks.

ChenYu

> It looks good otherwise.
>
> Thanks!
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com
>
> --
> You received this message because you are subscribed to the Google Groups 
> "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an 
> email to linux-sunxi+unsubscr...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [linux-sunxi] Re: [PATCH 2/2] ASoC: sunxi: add support for the on-chip codec on early Allwinner SoCs

2015-09-17 Thread Chen-Yu Tsai 
On Thu, Sep 17, 2015 at 9:31 PM, Maxime Ripard
 wrote:
> Hi Mark,
>
> On Wed, Sep 16, 2015 at 08:16:12PM +0100, Mark Brown wrote:
>> On Sat, Sep 12, 2015 at 03:26:24PM +0200, Maxime Ripard wrote:
>>
>> This looks pretty good, there's a few minor things below but I'll apply
>> anyway - please send followup patches fixing these.
>
> Sure, I will. Thanks!
>
>>
>> > +   if (clk_set_rate(scodec->clk_module, clk_freq))
>> > +   return -EINVAL;
>>
>> Better to pass back the error code here rather than silently discard it
>> (it might have more information).
>
> Yep.
>
>>
>> > +static struct snd_soc_dai_driver sun4i_codec_dai = {
>> > +   .name   = "Codec",
>> > +   .ops= _codec_dai_ops,
>> > +   .playback = {
>> > +   .stream_name= "Codec Playback",
>> > +   .channels_min   = 1,
>> > +   .channels_max   = 2,
>> > +   .rate_min   = 8000,
>> > +   .rate_max   = 192000,
>> > +   .rates  = SNDRV_PCM_RATE_8000_48000 |
>> > + SNDRV_PCM_RATE_96000 |
>> > + SNDRV_PCM_RATE_192000 |
>> > + SNDRV_PCM_RATE_KNOT,
>>
>> No need to specify both explicit rates and _KNOT, _KNOT covers
>> everything.
>
> Ack.
>
>> > +   .formats= SNDRV_PCM_FMTBIT_S16_LE |
>> > + SNDRV_PCM_FMTBIT_S32_LE,
>> > +   .sig_bits   = 24,
>>
>> So presumably also S24_LE (ie, 24 bits packed into a 32 bit word)?
>
> Hmm, probably yes, I'll test that.

IIRC when Emilio first wrote the driver, we tried 24 bit and no sound
came out. Turns out it's an alignment issue. The codec's FIFO register
is 32 bits wide, and takes the higher 24 bits as input when set to 24
bit mode. The internal FIFO is only 24 bits wide. A20 user manual P174
describes how the bits are copied.

So for 24 bit audio, you would actually send it 32 bit audio samples,
and let it truncate or drop the least significant 8 bits. This is why
we have SNDRV_PCM_FMTBIT_S32_LE with .sig_bits = 24.

I don't know if this is just a workaround, but a few other drivers do
this as well, for example twl3040 and omap-mcpdm.


Regards
ChenYu

>> > +   /* Enable the bus clock */
>> > +   if (clk_prepare_enable(scodec->clk_apb)) {
>> > +   dev_err(>dev, "Failed to enable the APB clock\n");
>> > +   return -EINVAL;
>> > +   }
>>
>> Ideally we'd have runtime power management to disable the clocks when
>> idle.
>
> We don't really have any kind of power management support currently,
> but adding runtime pm everywhere is definitely on the todo list. It
> might not happen before a while though.
>
> Thanks!
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com
>
> --
> You received this message because you are subscribed to the Google Groups 
> "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an 
> email to linux-sunxi+unsubscr...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] Re: [PATCH 2/2] ASoC: sunxi: add support for the on-chip codec on early Allwinner SoCs

2015-09-17 Thread Maxime Ripard 
Hi Mark,

On Wed, Sep 16, 2015 at 08:16:12PM +0100, Mark Brown wrote:
> On Sat, Sep 12, 2015 at 03:26:24PM +0200, Maxime Ripard wrote:
> 
> This looks pretty good, there's a few minor things below but I'll apply
> anyway - please send followup patches fixing these.

Sure, I will. Thanks!

> 
> > +   if (clk_set_rate(scodec->clk_module, clk_freq))
> > +   return -EINVAL;
> 
> Better to pass back the error code here rather than silently discard it
> (it might have more information).

Yep.

> 
> > +static struct snd_soc_dai_driver sun4i_codec_dai = {
> > +   .name   = "Codec",
> > +   .ops= _codec_dai_ops,
> > +   .playback = {
> > +   .stream_name= "Codec Playback",
> > +   .channels_min   = 1,
> > +   .channels_max   = 2,
> > +   .rate_min   = 8000,
> > +   .rate_max   = 192000,
> > +   .rates  = SNDRV_PCM_RATE_8000_48000 |
> > + SNDRV_PCM_RATE_96000 |
> > + SNDRV_PCM_RATE_192000 |
> > + SNDRV_PCM_RATE_KNOT,
> 
> No need to specify both explicit rates and _KNOT, _KNOT covers
> everything.

Ack.

> > +   .formats= SNDRV_PCM_FMTBIT_S16_LE |
> > + SNDRV_PCM_FMTBIT_S32_LE,
> > +   .sig_bits   = 24,
> 
> So presumably also S24_LE (ie, 24 bits packed into a 32 bit word)?

Hmm, probably yes, I'll test that.

> > +   /* Enable the bus clock */
> > +   if (clk_prepare_enable(scodec->clk_apb)) {
> > +   dev_err(>dev, "Failed to enable the APB clock\n");
> > +   return -EINVAL;
> > +   }
> 
> Ideally we'd have runtime power management to disable the clocks when
> idle.

We don't really have any kind of power management support currently,
but adding runtime pm everywhere is definitely on the todo list. It
might not happen before a while though.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: Digital signature

Re: [linux-sunxi] Re: [PATCH 2/2] ASoC: sunxi: add support for the on-chip codec on early Allwinner SoCs

2015-09-17 Thread Mark Brown 
On Thu, Sep 17, 2015 at 11:10:41PM +0800, Chen-Yu Tsai wrote:
> On Thu, Sep 17, 2015 at 9:31 PM, Maxime Ripard

> >> > +   .formats= SNDRV_PCM_FMTBIT_S16_LE |
> >> > + SNDRV_PCM_FMTBIT_S32_LE,
> >> > +   .sig_bits   = 24,

> >> So presumably also S24_LE (ie, 24 bits packed into a 32 bit word)?

> > Hmm, probably yes, I'll test that.

> IIRC when Emilio first wrote the driver, we tried 24 bit and no sound
> came out. Turns out it's an alignment issue. The codec's FIFO register
> is 32 bits wide, and takes the higher 24 bits as input when set to 24
> bit mode. The internal FIFO is only 24 bits wide. A20 user manual P174
> describes how the bits are copied.

> So for 24 bit audio, you would actually send it 32 bit audio samples,
> and let it truncate or drop the least significant 8 bits. This is why
> we have SNDRV_PCM_FMTBIT_S32_LE with .sig_bits = 24.

> I don't know if this is just a workaround, but a few other drivers do
> this as well, for example twl3040 and omap-mcpdm.

That's very common - it's essentially what S24_LE mode is.

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: Digital signature

[linux-sunxi] Re: [PATCH 3/4] input: Add new sun4i-keypad driver

2015-09-17 Thread Yassin Jaffer 
Hi Maxime

I appreciate your time and efforts .

Do you need that rate to be enforced, or is it some leftover from the
> allwinner BSP?


I've found that clock rate works fine with the default denounce and scan
cycle.
By the way do you have any dev board which expose the keypad pins?

I will try to submit a follow up patch to address these issues.
Thank you again for your valuable review.


Regards.



On Thu, Sep 17, 2015 at 11:05 PM, Maxime Ripard <
maxime.rip...@free-electrons.com> wrote:

> Hi,
>
> On Wed, Sep 16, 2015 at 12:05:56AM +1000, yassinjaf...@gmail.com wrote:
> > From: Yassin Jaffer 
> >
> > Allwinnner SUN4i Keypad controller is used to interface a SoC
> > with a matrix-typekeypad device.
> > The keypad controller supports multiple row and column lines.
> > A key can be placed at each intersection of a unique
> > row and a unique column.
> > The keypad controller can sense a key-press and key-release and report
> the
> > event using a interrupt to the cpu.
> > This patch adds a driver support to this.
> > The keypad controller driver does not give proper information
> > if more that two keys are selected.
> >
> > Signed-off-by: Yassin Jaffer 
> > ---
> >  drivers/input/keyboard/Kconfig|  11 ++
> >  drivers/input/keyboard/Makefile   |   1 +
> >  drivers/input/keyboard/sun4i-keypad.c | 361
> ++
> >  3 files changed, 373 insertions(+)
> >  create mode 100644 drivers/input/keyboard/sun4i-keypad.c
> >
> > diff --git a/drivers/input/keyboard/Kconfig
> b/drivers/input/keyboard/Kconfig
> > index 2e80107..4f2f3f8 100644
> > --- a/drivers/input/keyboard/Kconfig
> > +++ b/drivers/input/keyboard/Kconfig
> > @@ -590,6 +590,17 @@ config KEYBOARD_SUN4I_LRADC
> > To compile this driver as a module, choose M here: the
> > module will be called sun4i-lradc-keys.
> >
> > +config KEYBOARD_SUN4I_KEYPAD
> > + tristate "Allwinner sun4i keypad support"
> > + depends on ARCH_SUNXI
>
> Is this IP found on all the know SoCs, or just a subset of them?
>
> You probably want to add || COMPILE_TEST in that depends on too.
>
> > + select INPUT_MATRIXKMAP
> > + help
> > +   This selects support for the Allwinner keypad
> > +   on Allwinner sunxi SoCs.
> > +
> > +   To compile this driver as a module, choose M here: the
> > +   module will be called sun4i-keypad.
> > +
> >  config KEYBOARD_DAVINCI
> >   tristate "TI DaVinci Key Scan"
> >   depends on ARCH_DAVINCI_DM365
> > diff --git a/drivers/input/keyboard/Makefile
> b/drivers/input/keyboard/Makefile
> > index 1d416dd..d9f54b4 100644
> > --- a/drivers/input/keyboard/Makefile
> > +++ b/drivers/input/keyboard/Makefile
> > @@ -57,6 +57,7 @@ obj-$(CONFIG_KEYBOARD_STMPE)+=
> stmpe-keypad.o
> >  obj-$(CONFIG_KEYBOARD_STOWAWAY)  += stowaway.o
> >  obj-$(CONFIG_KEYBOARD_ST_KEYSCAN)+= st-keyscan.o
> >  obj-$(CONFIG_KEYBOARD_SUN4I_LRADC)   += sun4i-lradc-keys.o
> > +obj-$(CONFIG_KEYBOARD_SUN4I_KEYPAD)  += sun4i-keypad.o
> >  obj-$(CONFIG_KEYBOARD_SUNKBD)+= sunkbd.o
> >  obj-$(CONFIG_KEYBOARD_TC3589X)   += tc3589x-keypad.o
> >  obj-$(CONFIG_KEYBOARD_TEGRA) += tegra-kbc.o
> > diff --git a/drivers/input/keyboard/sun4i-keypad.c
> b/drivers/input/keyboard/sun4i-keypad.c
> > new file mode 100644
> > index 000..995f9665
> > --- /dev/null
> > +++ b/drivers/input/keyboard/sun4i-keypad.c
> > @@ -0,0 +1,361 @@
> > +/*
> > + * Allwinner sun4i keypad Controller driver
> > + *
> > + * Copyright (C) 2015 Yassin Jaffer 
> > + *
> > + * Parts of this software are based on (derived from):
> > + * Copyright (C) 2013-2015 lim...@allwinnertech.com,
> > + *qys
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +/*
> > + * Keypad Controller registers
> > + */
> > +#define KP_CTL   0x00  /* Keypad Control register */
> > +#define KP_TIMING0x04  /* Keypad Timing register */
> > +#define KP_INT_CFG   0x08  /* Keypad interrupt Config register
> */
> > +#define KP_INT_STA   0x0c  /* Keypad interrupt Status register
> */
> > +
> > +#define KP_IN_OFFSET 0x10 /* Keypad Input Data register 0 */
> >