Re: [RFC v1 PATCH 3/3] driver: update all the code that use soc_device_match
Hi Alice,
On 4/19/21 7:27 AM, Alice Guo (OSS) wrote:
> From: Alice Guo
>
> Update all the code that use soc_device_match because add support for
> soc_device_match returning -EPROBE_DEFER.
>
> Signed-off-by: Alice Guo
> ---
> drivers/bus/ti-sysc.c | 2 +-
> drivers/clk/renesas/r8a7795-cpg-mssr.c| 4 +++-
> drivers/clk/renesas/rcar-gen2-cpg.c | 2 +-
> drivers/clk/renesas/rcar-gen3-cpg.c | 2 +-
> drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.c | 7 ++-
> drivers/dma/ti/k3-psil.c | 3 +++
> drivers/dma/ti/k3-udma.c | 2 +-
> drivers/gpu/drm/bridge/nwl-dsi.c | 2 +-
> drivers/gpu/drm/meson/meson_drv.c | 4 +++-
> drivers/gpu/drm/omapdrm/dss/dispc.c | 2 +-
> drivers/gpu/drm/omapdrm/dss/dpi.c | 4 +++-
> drivers/gpu/drm/omapdrm/dss/dsi.c | 3 +++
> drivers/gpu/drm/omapdrm/dss/dss.c | 3 +++
> drivers/gpu/drm/omapdrm/dss/hdmi4_core.c | 3 +++
> drivers/gpu/drm/omapdrm/dss/venc.c| 4 +++-
> drivers/gpu/drm/omapdrm/omap_drv.c| 3 +++
> drivers/gpu/drm/rcar-du/rcar_du_crtc.c| 4 +++-
> drivers/gpu/drm/rcar-du/rcar_lvds.c | 2 +-
> drivers/gpu/drm/tidss/tidss_dispc.c | 4 +++-
> drivers/iommu/ipmmu-vmsa.c| 7 +--
> drivers/media/platform/rcar-vin/rcar-core.c | 2 +-
> drivers/media/platform/rcar-vin/rcar-csi2.c | 2 +-
> drivers/media/platform/vsp1/vsp1_uif.c| 4 +++-
> drivers/mmc/host/renesas_sdhi_core.c | 2 +-
> drivers/mmc/host/renesas_sdhi_internal_dmac.c | 2 +-
> drivers/mmc/host/sdhci-of-esdhc.c | 21 ++-
> drivers/mmc/host/sdhci-omap.c | 2 +-
> drivers/mmc/host/sdhci_am654.c| 2 +-
> drivers/net/ethernet/renesas/ravb_main.c | 4 +++-
> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 2 +-
> drivers/net/ethernet/ti/cpsw.c| 2 +-
> drivers/net/ethernet/ti/cpsw_new.c| 2 +-
> drivers/phy/ti/phy-omap-usb2.c| 4 +++-
> drivers/pinctrl/renesas/core.c| 2 +-
> drivers/pinctrl/renesas/pfc-r8a7790.c | 5 -
> drivers/pinctrl/renesas/pfc-r8a7794.c | 5 -
> drivers/soc/fsl/dpio/dpio-driver.c| 13
> drivers/soc/renesas/r8a774c0-sysc.c | 5 -
> drivers/soc/renesas/r8a7795-sysc.c| 2 +-
> drivers/soc/renesas/r8a77990-sysc.c | 5 -
> drivers/soc/ti/k3-ringacc.c | 2 +-
> drivers/staging/mt7621-pci/pci-mt7621.c | 2 +-
> drivers/thermal/rcar_gen3_thermal.c | 4 +++-
> drivers/thermal/ti-soc-thermal/ti-bandgap.c | 10 +++--
> drivers/usb/gadget/udc/renesas_usb3.c | 2 +-
> drivers/usb/host/ehci-platform.c | 4 +++-
> drivers/usb/host/xhci-rcar.c | 2 +-
> drivers/watchdog/renesas_wdt.c| 2 +-
> 48 files changed, 131 insertions(+), 52 deletions(-)
>
...
> diff --git a/drivers/dma/ti/k3-udma.c b/drivers/dma/ti/k3-udma.c
> index 96ad21869ba7..50a4c8f0993d 100644
> --- a/drivers/dma/ti/k3-udma.c
> +++ b/drivers/dma/ti/k3-udma.c
> @@ -5188,7 +5188,7 @@ static int udma_probe(struct platform_device *pdev)
> ud->match_data = match->data;
>
> soc = soc_device_match(k3_soc_devices);
> - if (!soc) {
> + if (!IS_ERR(soc) && !soc) {
this does not sound right...
if (!soc || IS_ERR(soc))
or
if (IS_ERR_OR_NULL(soc))
is even better.
> dev_err(dev, "No compatible SoC found\n");
> return -ENODEV;
There might be a clever macro for it, but:
return soc ? PTR_ERR(soc) : -ENODEV;
> }
--
Péter
Re: [PATCH 11/16] dmaengine: ti: k3-psil-j721e: Add entry for CSI2RX
On 4/6/21 8:10 PM, Pratyush Yadav wrote: > On 06/04/21 10:25PM, Pratyush Yadav wrote: >> On 06/04/21 06:33PM, Péter Ujfalusi wrote: >>> >>> >>> On 4/6/21 6:09 PM, Pratyush Yadav wrote: >>>> On 04/04/21 04:24PM, Péter Ujfalusi wrote: >>>>> Hi Pratyush, >>>>> >>>>> On 3/30/21 8:33 PM, Pratyush Yadav wrote: >>>>>> The CSI2RX subsystem uses PSI-L DMA to transfer frames to memory. It can >>>>>> have up to 32 threads but the current driver only supports using one. So >>>>>> add an entry for that one thread. >>>>> >>>>> If you are absolutely sure that the other threads are not going to be >>>>> used, then: >>>> >>>> The opposite in fact. I do expect other threads to be used in the >>>> future. But the current driver can only use one so I figured it is >>>> better to add just the thread that is currently needed and then I can >>>> always add the rest later. >>>> >>>> Why does this have to be a one-and-done deal? Is there anything wrong >>>> with adding the other threads when the driver can actually use them? >>> >>> You can skip CCing DMAengine (and me ;) ). Less subsystems is the better >>> when sending patches... >> >> I'm a bit confused here. If you are no longer interested in maintaining >> the TI DMA drivers then that's fine, I can skip CCing you. But the patch >> is still relevant to the dmaengine list so why should I skip CCing it? >> And if I don't CC the dmaengine list then on which list would I get >> comments/reviews for the patch? > > Ignore this. Got your point. Will do it in v2. If you know that you will be adding the other threads in the near future then do it with one patch. When you add the support in the csi2rx driver then you don't need to change the DMAengine files, thus no need to CC dmaengine or me, thus you need to gather less acked-bys, thus the time to mainline might be shorter. -- Péter
Re: [PATCH 11/16] dmaengine: ti: k3-psil-j721e: Add entry for CSI2RX
On 4/6/21 6:09 PM, Pratyush Yadav wrote:
> On 04/04/21 04:24PM, Péter Ujfalusi wrote:
>> Hi Pratyush,
>>
>> On 3/30/21 8:33 PM, Pratyush Yadav wrote:
>>> The CSI2RX subsystem uses PSI-L DMA to transfer frames to memory. It can
>>> have up to 32 threads but the current driver only supports using one. So
>>> add an entry for that one thread.
>>
>> If you are absolutely sure that the other threads are not going to be
>> used, then:
>
> The opposite in fact. I do expect other threads to be used in the
> future. But the current driver can only use one so I figured it is
> better to add just the thread that is currently needed and then I can
> always add the rest later.
>
> Why does this have to be a one-and-done deal? Is there anything wrong
> with adding the other threads when the driver can actually use them?
You can skip CCing DMAengine (and me ;) ). Less subsystems is the better
when sending patches...
>
>> Acked-by: Peter Ujfalusi
>>
>> but I would consider adding the other threads if there is a chance that
>> the cs2rx will need to support it in the future.
>>
>>> Signed-off-by: Pratyush Yadav
>>> ---
>>> drivers/dma/ti/k3-psil-j721e.c | 10 ++
>>> 1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/dma/ti/k3-psil-j721e.c b/drivers/dma/ti/k3-psil-j721e.c
>>> index 7580870ed746..19ffa31e6dc6 100644
>>> --- a/drivers/dma/ti/k3-psil-j721e.c
>>> +++ b/drivers/dma/ti/k3-psil-j721e.c
>>> @@ -58,6 +58,14 @@
>>> }, \
>>> }
>>>
>>> +#define PSIL_CSI2RX(x) \
>>> + { \
>>> + .thread_id = x, \
>>> + .ep_config = { \
>>> + .ep_type = PSIL_EP_NATIVE, \
>>> + }, \
>>> + }
>>> +
>>> /* PSI-L source thread IDs, used for RX (DMA_DEV_TO_MEM) */
>>> static struct psil_ep j721e_src_ep_map[] = {
>>> /* SA2UL */
>>> @@ -138,6 +146,8 @@ static struct psil_ep j721e_src_ep_map[] = {
>>> PSIL_PDMA_XY_PKT(0x4707),
>>> PSIL_PDMA_XY_PKT(0x4708),
>>> PSIL_PDMA_XY_PKT(0x4709),
>>> + /* CSI2RX */
>>> + PSIL_CSI2RX(0x4940),
>>> /* CPSW9 */
>>> PSIL_ETHERNET(0x4a00),
>>> /* CPSW0 */
>>>
>>
>> --
>> Péter
>
--
Péter
Re: [PATCH 13/16] media: ti-vpe: csi2rx: Add CSI2RX support
Hi Pratyush,
+1 from me also to the points Tomi raised.
few minor comments on the DMAengie side.
On 3/30/21 8:33 PM, Pratyush Yadav wrote:
> TI's J721E uses the Cadence CSI2RX and DPHY peripherals to facilitate
> capture over a CSI-2 bus.
>
> The Cadence CSI2RX IP acts as a bridge between the TI specific parts and
> the CSI-2 protocol parts. TI then has a wrapper on top of this bridge
> called the SHIM layer. It takes in data from stream 0, repacks it, and
> sends it to memory over PSI-L DMA.
>
> This driver acts as the "front end" to V4L2 client applications. It
> implements the required ioctls and buffer operations, passes the
> necessary calls on to the bridge, programs the SHIM layer, and performs
> DMA via the dmaengine API to finally return the data to a buffer
> supplied by the application.
>
> Signed-off-by: Pratyush Yadav
> ---
> MAINTAINERS | 7 +
> drivers/media/platform/Kconfig| 11 +
> drivers/media/platform/ti-vpe/Makefile| 1 +
> drivers/media/platform/ti-vpe/ti-csi2rx.c | 964 ++
> 4 files changed, 983 insertions(+)
> create mode 100644 drivers/media/platform/ti-vpe/ti-csi2rx.c
...
> diff --git a/drivers/media/platform/ti-vpe/ti-csi2rx.c
> b/drivers/media/platform/ti-vpe/ti-csi2rx.c
> new file mode 100644
> index ..355204ae473b
> --- /dev/null
> +++ b/drivers/media/platform/ti-vpe/ti-csi2rx.c
...
> +static int ti_csi2rx_init_vb2q(struct ti_csi2rx_dev *csi)
> +{
> + struct vb2_queue *q = >vidq;
> + int ret;
> +
> + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> + q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF | VB2_READ;
> + q->drv_priv = csi;
> + q->buf_struct_size = sizeof(struct ti_csi2rx_buffer);
> + q->ops = _vb2_qops;
> + q->mem_ops = _dma_contig_memops;
> + q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> + q->dev = csi->dma->device->dev;
q->dev = dmaengine_get_dma_device(csi->dma);
> + q->lock = >mutex;
> +
> + ret = vb2_queue_init(q);
> + if (ret)
> + return ret;
> +
> + csi->vdev.queue = q;
> +
> + return 0;
> +}
> +
> +static int ti_csi2rx_init_dma(struct ti_csi2rx_dev *csi)
> +{
> + struct dma_slave_config cfg;
> + int ret;
> +
> + INIT_LIST_HEAD(>dmaq.list);
> +
> + csi->dma = NULL;
> +
> + csi->dma = dma_request_chan(csi->dev, "rx0");
> + if (IS_ERR(csi->dma))
> + return PTR_ERR(csi->dma);
> +
> + memset(, 0, sizeof(cfg));
> +
> + cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_16_BYTES;
> + cfg.dst_addr_width = DMA_SLAVE_BUSWIDTH_16_BYTES;
No need to set the dst_addr_width as you only support RX.
Another note: UDMA with PSI-L native peripherals ignores this
cofniguration, only used for PDMAs, but I can remain to future proof the
driver and to keep it generic.
> +
> + ret = dmaengine_slave_config(csi->dma, );
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
--
Péter
Re: [PATCH 11/16] dmaengine: ti: k3-psil-j721e: Add entry for CSI2RX
Hi Pratyush,
On 3/30/21 8:33 PM, Pratyush Yadav wrote:
> The CSI2RX subsystem uses PSI-L DMA to transfer frames to memory. It can
> have up to 32 threads but the current driver only supports using one. So
> add an entry for that one thread.
If you are absolutely sure that the other threads are not going to be
used, then:
Acked-by: Peter Ujfalusi
but I would consider adding the other threads if there is a chance that
the cs2rx will need to support it in the future.
> Signed-off-by: Pratyush Yadav
> ---
> drivers/dma/ti/k3-psil-j721e.c | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/dma/ti/k3-psil-j721e.c b/drivers/dma/ti/k3-psil-j721e.c
> index 7580870ed746..19ffa31e6dc6 100644
> --- a/drivers/dma/ti/k3-psil-j721e.c
> +++ b/drivers/dma/ti/k3-psil-j721e.c
> @@ -58,6 +58,14 @@
> }, \
> }
>
> +#define PSIL_CSI2RX(x) \
> + { \
> + .thread_id = x, \
> + .ep_config = { \
> + .ep_type = PSIL_EP_NATIVE, \
> + }, \
> + }
> +
> /* PSI-L source thread IDs, used for RX (DMA_DEV_TO_MEM) */
> static struct psil_ep j721e_src_ep_map[] = {
> /* SA2UL */
> @@ -138,6 +146,8 @@ static struct psil_ep j721e_src_ep_map[] = {
> PSIL_PDMA_XY_PKT(0x4707),
> PSIL_PDMA_XY_PKT(0x4708),
> PSIL_PDMA_XY_PKT(0x4709),
> + /* CSI2RX */
> + PSIL_CSI2RX(0x4940),
> /* CPSW9 */
> PSIL_ETHERNET(0x4a00),
> /* CPSW0 */
>
--
Péter
Re: [PATCH 15/17] ASoC: ti: omap-mcsp: remove duplicate test
Hi Pierre,
On 3/26/21 11:59 PM, Pierre-Louis Bossart wrote:
> cppcheck warning:
>
> sound/soc/ti/omap-mcbsp.c:379:11: style: The if condition is the same
> as the previous if condition [duplicateCondition]
>
> if (mcbsp->irq) {
> ^
> sound/soc/ti/omap-mcbsp.c:376:11: note: First condition
> if (mcbsp->irq)
> ^
> sound/soc/ti/omap-mcbsp.c:379:11: note: Second condition
> if (mcbsp->irq) {
> ^
>
> Keeping two separate tests was probably intentional for clarity, but
> since this generates warnings we might as well make cppcheck happy so
> that we have fewer warnings.
There might be other historical reasons why it ended up like this but
merging them does not make it less cleaner.
Acked-by: Peter Ujfalusi
> Signed-off-by: Pierre-Louis Bossart
> ---
> sound/soc/ti/omap-mcbsp.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/sound/soc/ti/omap-mcbsp.c b/sound/soc/ti/omap-mcbsp.c
> index 6025b30bbe77..db47981768c5 100644
> --- a/sound/soc/ti/omap-mcbsp.c
> +++ b/sound/soc/ti/omap-mcbsp.c
> @@ -373,10 +373,9 @@ static void omap_mcbsp_free(struct omap_mcbsp *mcbsp)
> MCBSP_WRITE(mcbsp, WAKEUPEN, 0);
>
> /* Disable interrupt requests */
> - if (mcbsp->irq)
> + if (mcbsp->irq) {
> MCBSP_WRITE(mcbsp, IRQEN, 0);
>
> - if (mcbsp->irq) {
> free_irq(mcbsp->irq, (void *)mcbsp);
> } else {
> free_irq(mcbsp->rx_irq, (void *)mcbsp);
>
--
Péter
Re: [PATCH 14/17] ASoC: ti: omap-abe-twl6040: remove useless assignment
On 3/26/21 11:59 PM, Pierre-Louis Bossart wrote: > cppcheck warning: > > sound/soc/ti/omap-abe-twl6040.c:173:10: style: Variable 'ret' is > assigned a value that is never used. [unreadVariable] > int ret = 0; > ^ Thanks, Acked-by: Peter Ujfalusi > > Signed-off-by: Pierre-Louis Bossart > --- > sound/soc/ti/omap-abe-twl6040.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/sound/soc/ti/omap-abe-twl6040.c b/sound/soc/ti/omap-abe-twl6040.c > index 16ea039ff865..91cc9a4f44d7 100644 > --- a/sound/soc/ti/omap-abe-twl6040.c > +++ b/sound/soc/ti/omap-abe-twl6040.c > @@ -170,7 +170,7 @@ static int omap_abe_twl6040_init(struct > snd_soc_pcm_runtime *rtd) > struct snd_soc_card *card = rtd->card; > struct abe_twl6040 *priv = snd_soc_card_get_drvdata(card); > int hs_trim; > - int ret = 0; > + int ret; > > /* >* Configure McPDM offset cancellation based on the HSOTRIM value from > -- Péter
Re: [PATCH] dmaengine: ti: k3-udma: Fix NULL pointer dereference error
Hi Kishon,
On 2/9/21 2:02 PM, Kishon Vijay Abraham I wrote:
> bcdma_get_*() and udma_get_*() checks if bchan/rchan/tchan/rflow is
> already allocated by checking if it has a NON NULL value. For the
> error cases, bchan/rchan/tchan/rflow will have error value
> and bcdma_get_*() and udma_get_*() considers this as already allocated
> (PASS) since the error values are NON NULL. This results in
> NULL pointer dereference error while de-referencing
> bchan/rchan/tchan/rflow.
>
> Reset the value of bchan/rchan/tchan/rflow to NULL if the allocation
> actually fails.
>
> Fixes: 017794739702 ("dmaengine: ti: k3-udma: Initial support for K3 BCDMA")
> Fixes: 25dcb5dd7b7c ("dmaengine: ti: New driver for K3 UDMA")
> Signed-off-by: Kishon Vijay Abraham I
Is this the same patch as the other with the similar subject?
--
Péter
> ---
> drivers/dma/ti/k3-udma.c | 30 +-
> 1 file changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/dma/ti/k3-udma.c b/drivers/dma/ti/k3-udma.c
> index 298460438bb4..aa4ef583ff83 100644
> --- a/drivers/dma/ti/k3-udma.c
> +++ b/drivers/dma/ti/k3-udma.c
> @@ -1330,6 +1330,7 @@ static int bcdma_get_bchan(struct udma_chan *uc)
> {
> struct udma_dev *ud = uc->ud;
> enum udma_tp_level tpl;
> + int ret;
>
> if (uc->bchan) {
> dev_dbg(ud->dev, "chan%d: already have bchan%d allocated\n",
> @@ -1347,8 +1348,11 @@ static int bcdma_get_bchan(struct udma_chan *uc)
> tpl = ud->bchan_tpl.levels - 1;
>
> uc->bchan = __udma_reserve_bchan(ud, tpl, -1);
> - if (IS_ERR(uc->bchan))
> - return PTR_ERR(uc->bchan);
> + if (IS_ERR(uc->bchan)) {
> + ret = PTR_ERR(uc->bchan);
> + uc->bchan = NULL;
> + return ret;
> + }
>
> uc->tchan = uc->bchan;
>
> @@ -1358,6 +1362,7 @@ static int bcdma_get_bchan(struct udma_chan *uc)
> static int udma_get_tchan(struct udma_chan *uc)
> {
> struct udma_dev *ud = uc->ud;
> + int ret;
>
> if (uc->tchan) {
> dev_dbg(ud->dev, "chan%d: already have tchan%d allocated\n",
> @@ -1372,8 +1377,11 @@ static int udma_get_tchan(struct udma_chan *uc)
>*/
> uc->tchan = __udma_reserve_tchan(ud, uc->config.channel_tpl,
>uc->config.mapped_channel_id);
> - if (IS_ERR(uc->tchan))
> - return PTR_ERR(uc->tchan);
> + if (IS_ERR(uc->tchan)) {
> + ret = PTR_ERR(uc->tchan);
> + uc->tchan = NULL;
> + return ret;
> + }
>
> if (ud->tflow_cnt) {
> int tflow_id;
> @@ -1403,6 +1411,7 @@ static int udma_get_tchan(struct udma_chan *uc)
> static int udma_get_rchan(struct udma_chan *uc)
> {
> struct udma_dev *ud = uc->ud;
> + int ret;
>
> if (uc->rchan) {
> dev_dbg(ud->dev, "chan%d: already have rchan%d allocated\n",
> @@ -1417,8 +1426,13 @@ static int udma_get_rchan(struct udma_chan *uc)
>*/
> uc->rchan = __udma_reserve_rchan(ud, uc->config.channel_tpl,
>uc->config.mapped_channel_id);
> + if (IS_ERR(uc->rchan)) {
> + ret = PTR_ERR(uc->rchan);
> + uc->rchan = NULL;
> + return ret;
> + }
>
> - return PTR_ERR_OR_ZERO(uc->rchan);
> + return 0;
> }
>
> static int udma_get_chan_pair(struct udma_chan *uc)
> @@ -1472,6 +1486,7 @@ static int udma_get_chan_pair(struct udma_chan *uc)
> static int udma_get_rflow(struct udma_chan *uc, int flow_id)
> {
> struct udma_dev *ud = uc->ud;
> + int ret;
>
> if (!uc->rchan) {
> dev_err(ud->dev, "chan%d: does not have rchan??\n", uc->id);
> @@ -1485,6 +1500,11 @@ static int udma_get_rflow(struct udma_chan *uc, int
> flow_id)
> }
>
> uc->rflow = __udma_get_rflow(ud, flow_id);
> + if (IS_ERR(uc->rflow)) {
> + ret = PTR_ERR(uc->rflow);
> + uc->rflow = NULL;
> + return ret;
> + }
>
> return PTR_ERR_OR_ZERO(uc->rflow);
> }
>
Re: [PATCH] dmaengine: ti: k3-udma: Fix NULL pointer dereference error
Hi Kishon,
On 2/9/21 2:45 PM, Kishon Vijay Abraham I wrote:
> Hi Peter,
>
> On 09/02/21 5:53 pm, Péter Ujfalusi wrote:
>> Hi Kishon,
>>
>> On 2/9/21 11:00 AM, Kishon Vijay Abraham I wrote:
>>> bcdma_get_*() and udma_get_*() checks if bchan/rchan/tchan/rflow is
>>> already allocated by checking if it has a NON NULL value. For the
>>> error cases, bchan/rchan/tchan/rflow will have error value
>>> and bcdma_get_*() and udma_get_*() considers this as already allocated
>>> (PASS) since the error values are NON NULL. This results in
>>> NULL pointer dereference error while de-referencing
>>> bchan/rchan/tchan/rflow.
>>
>> I think this can happen when a channel request fails and we get a second
>> request coming and faces with the not cleanup up tchan/rchan/bchan/rflow
>> from the previous failure.
>> Interesting that I have not faced with this, but it is a valid oversight
>> from me.
>
> Thank you for reviewing.
>
> Got into this issue when all the PCIe endpoint functions were requesting
> for a MEMCOPY channel (total 22 endpoint functions) specifically in
> bcdma_get_bchan() where the scenario you mentioned above happened.
I see, do we even have 22 bchan allocated for Linux out from the 40? ;)
> Vignesh asked me to fix it for all udma_get_*().
Yes, that is the right thing to do, thank you!
>>
>>> Reset the value of bchan/rchan/tchan/rflow to NULL if the allocation
>>> actually fails.
>>>
>>> Fixes: 017794739702 ("dmaengine: ti: k3-udma: Initial support for K3 BCDMA")
>>> Fixes: 25dcb5dd7b7c ("dmaengine: ti: New driver for K3 UDMA")
>>
>> Will this patch apply at any of these?
>> 25dcb5dd7b7c does not have BCDMA (bchan)
>> 017794739702 does not contain PKTDMA (tflow)
>
> I can probably split this patch
> 017794739702 for bchan and 25dcb5dd7b7c for bchan/rchan/tchan/rflow
the tflow support for PKTDMA makes the tchan fix a bit problematic for
backporting, but it might worth a try to split to bcdma and
rchan/tchan/rflow patch.
--
Péter
Re: [PATCH] dmaengine: ti: k3-udma: Fix NULL pointer dereference error
Hi Kishon,
On 2/9/21 11:00 AM, Kishon Vijay Abraham I wrote:
> bcdma_get_*() and udma_get_*() checks if bchan/rchan/tchan/rflow is
> already allocated by checking if it has a NON NULL value. For the
> error cases, bchan/rchan/tchan/rflow will have error value
> and bcdma_get_*() and udma_get_*() considers this as already allocated
> (PASS) since the error values are NON NULL. This results in
> NULL pointer dereference error while de-referencing
> bchan/rchan/tchan/rflow.
I think this can happen when a channel request fails and we get a second
request coming and faces with the not cleanup up tchan/rchan/bchan/rflow
from the previous failure.
Interesting that I have not faced with this, but it is a valid oversight
from me.
> Reset the value of bchan/rchan/tchan/rflow to NULL if the allocation
> actually fails.
>
> Fixes: 017794739702 ("dmaengine: ti: k3-udma: Initial support for K3 BCDMA")
> Fixes: 25dcb5dd7b7c ("dmaengine: ti: New driver for K3 UDMA")
Will this patch apply at any of these?
25dcb5dd7b7c does not have BCDMA (bchan)
017794739702 does not contain PKTDMA (tflow)
> Signed-off-by: Kishon Vijay Abraham I
> ---
> drivers/dma/ti/k3-udma.c | 30 +-
> 1 file changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/dma/ti/k3-udma.c b/drivers/dma/ti/k3-udma.c
> index 298460438bb4..aa4ef583ff83 100644
> --- a/drivers/dma/ti/k3-udma.c
> +++ b/drivers/dma/ti/k3-udma.c
> @@ -1330,6 +1330,7 @@ static int bcdma_get_bchan(struct udma_chan *uc)
> {
> struct udma_dev *ud = uc->ud;
> enum udma_tp_level tpl;
> + int ret;
>
> if (uc->bchan) {
> dev_dbg(ud->dev, "chan%d: already have bchan%d allocated\n",
> @@ -1347,8 +1348,11 @@ static int bcdma_get_bchan(struct udma_chan *uc)
> tpl = ud->bchan_tpl.levels - 1;
>
> uc->bchan = __udma_reserve_bchan(ud, tpl, -1);
> - if (IS_ERR(uc->bchan))
> - return PTR_ERR(uc->bchan);
> + if (IS_ERR(uc->bchan)) {
> + ret = PTR_ERR(uc->bchan);
> + uc->bchan = NULL;
> + return ret;
> + }
>
> uc->tchan = uc->bchan;
>
> @@ -1358,6 +1362,7 @@ static int bcdma_get_bchan(struct udma_chan *uc)
> static int udma_get_tchan(struct udma_chan *uc)
> {
> struct udma_dev *ud = uc->ud;
> + int ret;
>
> if (uc->tchan) {
> dev_dbg(ud->dev, "chan%d: already have tchan%d allocated\n",
> @@ -1372,8 +1377,11 @@ static int udma_get_tchan(struct udma_chan *uc)
>*/
> uc->tchan = __udma_reserve_tchan(ud, uc->config.channel_tpl,
>uc->config.mapped_channel_id);
> - if (IS_ERR(uc->tchan))
> - return PTR_ERR(uc->tchan);
> + if (IS_ERR(uc->tchan)) {
> + ret = PTR_ERR(uc->tchan);
> + uc->tchan = NULL;
> + return ret;
> + }
>
> if (ud->tflow_cnt) {
> int tflow_id;
> @@ -1403,6 +1411,7 @@ static int udma_get_tchan(struct udma_chan *uc)
> static int udma_get_rchan(struct udma_chan *uc)
> {
> struct udma_dev *ud = uc->ud;
> + int ret;
>
> if (uc->rchan) {
> dev_dbg(ud->dev, "chan%d: already have rchan%d allocated\n",
> @@ -1417,8 +1426,13 @@ static int udma_get_rchan(struct udma_chan *uc)
>*/
> uc->rchan = __udma_reserve_rchan(ud, uc->config.channel_tpl,
>uc->config.mapped_channel_id);
> + if (IS_ERR(uc->rchan)) {
> + ret = PTR_ERR(uc->rchan);
> + uc->rchan = NULL;
> + return ret;
> + }
>
> - return PTR_ERR_OR_ZERO(uc->rchan);
> + return 0;
> }
>
> static int udma_get_chan_pair(struct udma_chan *uc)
> @@ -1472,6 +1486,7 @@ static int udma_get_chan_pair(struct udma_chan *uc)
> static int udma_get_rflow(struct udma_chan *uc, int flow_id)
> {
> struct udma_dev *ud = uc->ud;
> + int ret;
>
> if (!uc->rchan) {
> dev_err(ud->dev, "chan%d: does not have rchan??\n", uc->id);
> @@ -1485,6 +1500,11 @@ static int udma_get_rflow(struct udma_chan *uc, int
> flow_id)
> }
>
> uc->rflow = __udma_get_rflow(ud, flow_id);
> + if (IS_ERR(uc->rflow)) {
> + ret = PTR_ERR(uc->rflow);
> + uc->rflow = NULL;
> + return ret;
> + }
>
> return PTR_ERR_OR_ZERO(uc->rflow);
return 0;
> }
>
--
Péter
Re: [PATCH] dmaengine: ti: k3-udma: Fix a resource leak in an error handling path
Hi,
On 1/24/21 9:09 AM, Christophe JAILLET wrote:
In 'dma_pool_create()', we return -ENOMEM, but don't release the resources
already allocated, as in all the other error handling paths.
Go to 'err_res_free' instead of returning directly.
Interesting that I only had error for the bcdma path...
Fixes: 017794739702 ("dmaengine: ti: k3-udma: Initial support for K3 BCDMA")
Signed-off-by: Christophe JAILLET
---
This patch is not even compile tested.
I don't have the needed configuration.
No issue, that patch is trivial,
Acked-by: Peter Ujfalusi
---
drivers/dma/ti/k3-udma.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/dma/ti/k3-udma.c b/drivers/dma/ti/k3-udma.c
index 8e3fd1119a77..96ad21869ba7 100644
--- a/drivers/dma/ti/k3-udma.c
+++ b/drivers/dma/ti/k3-udma.c
@@ -2447,7 +2447,8 @@ static int bcdma_alloc_chan_resources(struct dma_chan
*chan)
dev_err(ud->ddev.dev,
"Descriptor pool allocation failed\n");
uc->use_dma_pool = false;
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto err_res_free;
}
uc->use_dma_pool = true;
--
Péter
Re: [PATCH] ASoC: ti: Allocate dais dynamically for TDM and audio graph card
Hi Pavel,
On 1/24/21 11:27 AM, Pavel Machek wrote:
From: Tony Lindgren
We can have multiple connections on a single McBSP instance configured
with audio graph card when using TDM (Time Division Multiplexing). Let's
allow that by configuring dais dynamically.
Still we have _one_ DAI per McBSP. TDM slots are not DAIs.
See Documentation/devicetree/bindings/sound/audio-graph-card.txt and
Documentation/devicetree/bindings/graph.txt for more details for
multiple endpoints.
Documentation/devicetree/bindings/sound/audio-graph-card.yaml
Documentation/devicetree/bindings/sound/audio-graph.yaml
However the schemas does not provide too much insight (the txts were a
bit more verbose).
I've tested this with droid4 where cpcap pmic and modem voice are both
both wired to mcbsp3. I've also tested this on droid4 both with and
without the pending modem audio codec driver that is waiting for n_gsm
serdev dependencies to clear.
>
Cc: Aaro Koskinen
Cc: Arthur D.
Cc: Jarkko Nikula
Cc: Merlijn Wajer
Cc: Peter Ujfalusi
Cc: Sebastian Reichel
Signed-off-by: Tony Lindgren
Signed-off-by: Pavel Machek
---
sound/soc/ti/omap-mcbsp-priv.h | 2 ++
sound/soc/ti/omap-mcbsp.c | 76 +-
2 files changed, 55 insertions(+), 23 deletions(-)
diff --git a/sound/soc/ti/omap-mcbsp-priv.h b/sound/soc/ti/omap-mcbsp-priv.h
index 7865cda4bf0a..9464f5d35822 100644
--- a/sound/soc/ti/omap-mcbsp-priv.h
+++ b/sound/soc/ti/omap-mcbsp-priv.h
@@ -262,6 +262,8 @@ struct omap_mcbsp {
struct omap_mcbsp_platform_data *pdata;
struct omap_mcbsp_st_data *st_data;
struct omap_mcbsp_reg_cfg cfg_regs;
+ struct snd_soc_dai_driver *dais;
+ int dai_count;
struct snd_dmaengine_dai_dma_data dma_data[2];
unsigned int dma_req[2];
int dma_op_mode;
diff --git a/sound/soc/ti/omap-mcbsp.c b/sound/soc/ti/omap-mcbsp.c
index 6025b30bbe77..189a6461b671 100644
--- a/sound/soc/ti/omap-mcbsp.c
+++ b/sound/soc/ti/omap-mcbsp.c
@@ -14,6 +14,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -1299,23 +1300,53 @@ static int omap_mcbsp_remove(struct snd_soc_dai *dai)
return 0;
}
-static struct snd_soc_dai_driver omap_mcbsp_dai = {
- .probe = omap_mcbsp_probe,
- .remove = omap_mcbsp_remove,
- .playback = {
- .channels_min = 1,
- .channels_max = 16,
- .rates = OMAP_MCBSP_RATES,
- .formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S32_LE,
- },
- .capture = {
- .channels_min = 1,
- .channels_max = 16,
- .rates = OMAP_MCBSP_RATES,
- .formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S32_LE,
- },
- .ops = _dai_ops,
-};
+static int omap_mcbsp_init_dais(struct omap_mcbsp *mcbsp)
+{
+ struct device_node *np = mcbsp->dev->of_node;
+ int i;
+
+ if (np)
+ mcbsp->dai_count = of_graph_get_endpoint_count(np);
+
+ if (!mcbsp->dai_count)
+ mcbsp->dai_count = 1;
+
+ mcbsp->dais = devm_kcalloc(mcbsp->dev, mcbsp->dai_count,
+ sizeof(*mcbsp->dais), GFP_KERNEL);
+ if (!mcbsp->dais)
+ return -ENOMEM;
+
+ for (i = 0; i < mcbsp->dai_count; i++) {
+ struct snd_soc_dai_driver *dai = >dais[i];
+
+ dai->name = devm_kasprintf(mcbsp->dev, GFP_KERNEL, "%s-dai%i",
+ dev_name(mcbsp->dev), i);
+
+ if (i == 0) {
+ dai->probe = omap_mcbsp_probe;
+ dai->remove = omap_mcbsp_remove;
+ dai->ops = _dai_ops;
+ }
You are effectively creating extra dummy-dais (no ops), but naming them
as McBSP.
The dummy-dais will only work if the real dai is in use and the dai link
which contains the real dai must be configured (TDM slots, format,
channels) to accomodate the link which contains the dummy-dai.
The idea did not aged well for me ;)
It still looks and sounds like a hack to model the TDM slots on a single
DAI as multiple DAIs just to satisfy a generic binding which is not
aimed to satisfy the droid4 setup (which is far from anything simple).
+ dai->playback.channels_min = 1;
+ dai->playback.channels_max = 16;
+ dai->playback.rates = OMAP_MCBSP_RATES;
+ if (mcbsp->pdata->reg_size == 2)
+ dai->playback.formats = SNDRV_PCM_FMTBIT_S16_LE;
+ else
+ dai->playback.formats = SNDRV_PCM_FMTBIT_S16_LE |
+ SNDRV_PCM_FMTBIT_S32_LE;
+ dai->capture.channels_min = 1;
+ dai->capture.channels_max = 16;
+ dai->capture.rates = OMAP_MCBSP_RATES;
+ if (mcbsp->pdata->reg_size == 2)
+
Re: [PATCH 2/2] dmaengine: ti: k3-udma: Add support for burst_size configuration for mem2mem
Hi Vinod,
On 1/12/21 12:16 PM, Vinod Koul wrote:
> On 14-12-20, 10:13, Peter Ujfalusi wrote:
>> The UDMA and BCDMA can provide higher throughput if the burst_size of the
>> channel is changed from it's default (which is 64 bytes) for Ultra-high
>> and high capacity channels.
>>
>> This performance benefit is even more visible when the buffers are aligned
>> with the burst_size configuration.
>>
>> The am654 does not have a way to change the burst size, but it is using
>> 64 bytes burst, so increasing the copy_align from 8 bytes to 64 (and
>> clients taking that into account) can increase the throughput as well.
>>
>> Numbers gathered on j721e:
>> echo 800 > /sys/module/dmatest/parameters/test_buf_size
>> echo 2000 > /sys/module/dmatest/parameters/timeout
>> echo 50 > /sys/module/dmatest/parameters/iterations
>> echo 1 > /sys/module/dmatest/parameters/max_channels
>>
>> Prior this patch: ~1.3 GB/s
>> After this patch: ~1.8 GB/s
>> with 1 byte alignment: ~1.7 GB/s
>>
>> Signed-off-by: Peter Ujfalusi
>> ---
>> drivers/dma/ti/k3-udma.c | 115 +--
>> 1 file changed, 110 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/dma/ti/k3-udma.c b/drivers/dma/ti/k3-udma.c
>> index 87157cbae1b8..54e4ccb1b37e 100644
>> --- a/drivers/dma/ti/k3-udma.c
>> +++ b/drivers/dma/ti/k3-udma.c
>> @@ -121,6 +121,11 @@ struct udma_oes_offsets {
>> #define UDMA_FLAG_PDMA_ACC32BIT(0)
>> #define UDMA_FLAG_PDMA_BURSTBIT(1)
>> #define UDMA_FLAG_TDTYPEBIT(2)
>> +#define UDMA_FLAG_BURST_SIZEBIT(3)
>> +#define UDMA_FLAGS_J7_CLASS (UDMA_FLAG_PDMA_ACC32 | \
>> + UDMA_FLAG_PDMA_BURST | \
>> + UDMA_FLAG_TDTYPE | \
>> + UDMA_FLAG_BURST_SIZE)
>>
>> struct udma_match_data {
>> enum k3_dma_type type;
>> @@ -128,6 +133,7 @@ struct udma_match_data {
>> bool enable_memcpy_support;
>> u32 flags;
>> u32 statictr_z_mask;
>> +u8 burst_size[3];
>> };
>>
>> struct udma_soc_data {
>> @@ -436,6 +442,18 @@ static void k3_configure_chan_coherency(struct dma_chan
>> *chan, u32 asel)
>> }
>> }
>>
>> +static u8 udma_get_chan_tpl_index(struct udma_tpl *tpl_map, int chan_id)
>> +{
>> +int i;
>> +
>> +for (i = 0; i < tpl_map->levels; i++) {
>> +if (chan_id >= tpl_map->start_idx[i])
>> +return i;
>> +}
>
> Braces seem not required
True, they are not strictly needed but I prefer to have them when I have
any condition in the loop.
>> +
>> +return 0;
>> +}
>> +
>> static void udma_reset_uchan(struct udma_chan *uc)
>> {
>> memset(>config, 0, sizeof(uc->config));
>> @@ -1811,6 +1829,7 @@ static int udma_tisci_m2m_channel_config(struct
>> udma_chan *uc)
>> const struct ti_sci_rm_udmap_ops *tisci_ops = tisci_rm->tisci_udmap_ops;
>> struct udma_tchan *tchan = uc->tchan;
>> struct udma_rchan *rchan = uc->rchan;
>> +u8 burst_size = 0;
>> int ret = 0;
>>
>> /* Non synchronized - mem to mem type of transfer */
>> @@ -1818,6 +1837,12 @@ static int udma_tisci_m2m_channel_config(struct
>> udma_chan *uc)
>> struct ti_sci_msg_rm_udmap_tx_ch_cfg req_tx = { 0 };
>> struct ti_sci_msg_rm_udmap_rx_ch_cfg req_rx = { 0 };
>>
>> +if (ud->match_data->flags & UDMA_FLAG_BURST_SIZE) {
>> +u8 tpl = udma_get_chan_tpl_index(>tchan_tpl, tchan->id);
>
> Can we define variable at function start please
The 'tpl' is only used within this if branch, it looks a bit cleaner
imho, but if you insist, I can move the definition.
...
>> +static enum dmaengine_alignment udma_get_copy_align(struct udma_dev *ud)
>> +{
>> +const struct udma_match_data *match_data = ud->match_data;
>> +u8 tpl;
>> +
>> +if (!match_data->enable_memcpy_support)
>> +return DMAENGINE_ALIGN_8_BYTES;
>> +
>> +/* Get the highest TPL level the device supports for memcpy */
>> +if (ud->bchan_cnt) {
>> +tpl = udma_get_chan_tpl_index(>bchan_tpl, 0);
>> +} else if (ud->tchan_cnt) {
>> +tpl = udma_get_chan_tpl_index(>tchan_tpl, 0);
>> +} else {
>> +return DMAENGINE_ALIGN_8_BYTES;
>> +}
>
> Braces seem not required
Very true.
>
>> +
>> +switch (match_data->burst_size[tpl]) {
>> +case TI_SCI_RM_UDMAP_CHAN_BURST_SIZE_256_BYTES:
>> +return DMAENGINE_ALIGN_256_BYTES;
>> +case TI_SCI_RM_UDMAP_CHAN_BURST_SIZE_128_BYTES:
>> +return DMAENGINE_ALIGN_128_BYTES;
>> +case TI_SCI_RM_UDMAP_CHAN_BURST_SIZE_64_BYTES:
>> +fallthrough;
>> +default:
>> +return DMAENGINE_ALIGN_64_BYTES;
>
> ah, we are supposed to have case at same indent as switch, pls run
> checkpatch to have these flagged off
Yes, they should be.
The other me did a sloppy job for sure, this should
Re: [PATCH] dmaengine: ti: k3-udma: Set rflow count for BCDMA split channels
Hi Vignesh,
On 1/12/21 4:14 PM, Vignesh Raghavendra wrote:
BCDMA RX channels have one flow per channel, therefore set the rflow_cnt
to rchan_cnt.
Without this patch, request for BCDMA RX channel allocation fails as
rflow_cnt is 0 thus fails to reserve a rflow for the channel.
Good catch, thank you!
Acked-by: Peter Ujfalusi
Fixes: 8844898028d4 ("dmaengine: ti: k3-udma: Add support for BCDMA channel TPL
handling")
Signed-off-by: Vignesh Raghavendra
---
drivers/dma/ti/k3-udma.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/dma/ti/k3-udma.c b/drivers/dma/ti/k3-udma.c
index 298460438bb4..a1af59d901be 100644
--- a/drivers/dma/ti/k3-udma.c
+++ b/drivers/dma/ti/k3-udma.c
@@ -4305,6 +4305,7 @@ static int udma_get_mmrs(struct platform_device *pdev,
struct udma_dev *ud)
ud->bchan_cnt = BCDMA_CAP2_BCHAN_CNT(cap2);
ud->tchan_cnt = BCDMA_CAP2_TCHAN_CNT(cap2);
ud->rchan_cnt = BCDMA_CAP2_RCHAN_CNT(cap2);
+ ud->rflow_cnt = ud->rchan_cnt;
break;
case DMA_TYPE_PKTDMA:
cap4 = udma_read(ud->mmrs[MMR_GCFG], 0x30);
--
Péter
Re: [PATCH 1/3] thermal: ti-soc-thermal: Fix stuck sensor with continuous mode for 4430
Hi Tony,
On 12/30/20 10:43 AM, Tony Lindgren wrote:
> At least for 4430, trying to use the single conversion mode eventually
> hangs the thermal sensor. This can be quite easily seen with errors:
>
> thermal thermal_zone0: failed to read out thermal zone (-5)
>
> Also, trying to read the temperature shows a stuck value with:
>
> $ while true; do cat /sys/class/thermal/thermal_zone0/temp; done
>
> Where the temperature is not rising at all with the busy loop.
>
> Additionally, the EOCZ (end of conversion) bit is not rising on 4430 in
> single conversion mode while it works fine in continuous conversion mode.
> It is also possible that the hung temperature sensor can affect the
> thermal shutdown alert too.
>
> Let's fix the issue by adding TI_BANDGAP_FEATURE_CONT_MODE_ONLY flag and
> use it for 4430.
>
> Note that we also need to add udelay to for the EOCZ (end of conversion)
> bit polling as otherwise we have it time out too early on 4430. We'll be
> changing the loop to use iopoll in the following clean-up patch.
I don't yet have my setup in working condition, so I can not test these.
> Cc: Adam Ford
> Cc: Carl Philipp Klemm
> Cc: Eduardo Valentin
> Cc: Merlijn Wajer
> Cc: Pavel Machek
> Cc: Peter Ujfalusi
> Cc: Sebastian Reichel
> Signed-off-by: Tony Lindgren
> ---
> drivers/thermal/ti-soc-thermal/omap4-thermal-data.c | 3 ++-
> drivers/thermal/ti-soc-thermal/ti-bandgap.c | 9 +++--
> drivers/thermal/ti-soc-thermal/ti-bandgap.h | 2 ++
> 3 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/thermal/ti-soc-thermal/omap4-thermal-data.c
> b/drivers/thermal/ti-soc-thermal/omap4-thermal-data.c
> --- a/drivers/thermal/ti-soc-thermal/omap4-thermal-data.c
> +++ b/drivers/thermal/ti-soc-thermal/omap4-thermal-data.c
> @@ -58,7 +58,8 @@ omap4430_adc_to_temp[OMAP4430_ADC_END_VALUE -
> OMAP4430_ADC_START_VALUE + 1] = {
> const struct ti_bandgap_data omap4430_data = {
> .features = TI_BANDGAP_FEATURE_MODE_CONFIG |
> TI_BANDGAP_FEATURE_CLK_CTRL |
> - TI_BANDGAP_FEATURE_POWER_SWITCH,
> + TI_BANDGAP_FEATURE_POWER_SWITCH |
> + TI_BANDGAP_FEATURE_CONT_MODE_ONLY,
Can we add a comment with the observations?
> .fclock_name = "bandgap_fclk",
> .div_ck_name = "bandgap_fclk",
> .conv_table = omap4430_adc_to_temp,
> diff --git a/drivers/thermal/ti-soc-thermal/ti-bandgap.c
> b/drivers/thermal/ti-soc-thermal/ti-bandgap.c
> --- a/drivers/thermal/ti-soc-thermal/ti-bandgap.c
> +++ b/drivers/thermal/ti-soc-thermal/ti-bandgap.c
> @@ -15,6 +15,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -605,8 +606,10 @@ ti_bandgap_force_single_read(struct ti_bandgap *bgp, int
> id)
> u32 counter = 1000;
> struct temp_sensor_registers *tsr;
>
> - /* Select single conversion mode */
> - if (TI_BANDGAP_HAS(bgp, MODE_CONFIG))
> + /* Select continuous or single conversion mode */
> + if (TI_BANDGAP_HAS(bgp, CONT_MODE_ONLY))
> + RMW_BITS(bgp, id, bgap_mode_ctrl, mode_ctrl_mask, 1);
> + else if (TI_BANDGAP_HAS(bgp, MODE_CONFIG))
> RMW_BITS(bgp, id, bgap_mode_ctrl, mode_ctrl_mask, 0);
Would not be better to:
if (TI_BANDGAP_HAS(bgp, MODE_CONFIG)) {
if (TI_BANDGAP_HAS(bgp, CONT_MODE_ONLY))
RMW_BITS(bgp, id, bgap_mode_ctrl, mode_ctrl_mask, 1);
else
RMW_BITS(bgp, id, bgap_mode_ctrl, mode_ctrl_mask, 0);
}
One can only switch to cont/single mode if the mode config is possible.
>
> /* Start of Conversion = 1 */
> @@ -619,6 +622,7 @@ ti_bandgap_force_single_read(struct ti_bandgap *bgp, int
> id)
> if (ti_bandgap_readl(bgp, tsr->temp_sensor_ctrl) &
> tsr->bgap_eocz_mask)
> break;
> + udelay(1);
> }
>
> /* Start of Conversion = 0 */
> @@ -630,6 +634,7 @@ ti_bandgap_force_single_read(struct ti_bandgap *bgp, int
> id)
> if (!(ti_bandgap_readl(bgp, tsr->temp_sensor_ctrl) &
> tsr->bgap_eocz_mask))
> break;
> + udelay(1);
> }
>
> return 0;
> diff --git a/drivers/thermal/ti-soc-thermal/ti-bandgap.h
> b/drivers/thermal/ti-soc-thermal/ti-bandgap.h
> --- a/drivers/thermal/ti-soc-thermal/ti-bandgap.h
> +++ b/drivers/thermal/ti-soc-thermal/ti-bandgap.h
> @@ -280,6 +280,7 @@ struct ti_temp_sensor {
> * has Errata 814
> * TI_BANDGAP_FEATURE_UNRELIABLE - used when the sensor readings are too
> * inaccurate.
> + * TI_BANDGAP_FEATURE_CONT_MODE_ONLY - used when single mode hangs the sensor
> * TI_BANDGAP_HAS(b, f) - macro to check if a bandgap device is capable of a
> * specific feature (above) or not. Return non-zero, if yes.
> */
> @@ -295,6 +296,7 @@ struct ti_temp_sensor {
> #define TI_BANDGAP_FEATURE_HISTORY_BUFFERBIT(9)
> #define
Re: [PATCH] leds: leds-pwm: Convert to use devm_get_pwm
On 11/10/2012 02:48 AM, Bryan Wu wrote:
> On Wed, Nov 7, 2012 at 3:42 AM, Peter Ujfalusi wrote:
>> Update the driver to use the new API for requesting pwm so we can take
>> advantage of the pwm_lookup table to find the correct pwm to be used for the
>> LED functionality.
>> If the devm_get_pwm fails we fall back to legacy mode to try to get the pwm.
>>
>> Signed-off-by: Peter Ujfalusi
>> ---
>> drivers/leds/leds-pwm.c | 16 ++--
>> 1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
>> index f2e44c7..6bf9445 100644
>> --- a/drivers/leds/leds-pwm.c
>> +++ b/drivers/leds/leds-pwm.c
>> @@ -47,6 +47,19 @@ static void led_pwm_set(struct led_classdev *led_cdev,
>> }
>> }
>>
>> +static struct pwm_device *led_pwm_request_pwm(struct platform_device *pdev,
>> + struct led_pwm *cur_led)
>> +{
>> + struct pwm_device *pwm;
>> +
>> + pwm = devm_pwm_get(>dev, cur_led->name);
>> + if (IS_ERR(pwm)) {
>> + dev_err(>dev, "unable to request PWM, trying legacy
>> API\n");
>> + pwm = pwm_request(cur_led->pwm_id, cur_led->name);
>
> Looks good, but why we still need to keep the old API pwm_request(),
> if devm_pwm_get() is the right replacement. AFAIK, devm_***() API will
> help to clean up if device probing fails.
>
> So if it is good enough, we can just replace pwm_request() to the
> devm_pwm_get()
I have greped for "leds_pwm" in arch/ and the only place it is used is the
arch/arm/mach-omap2/board-sdp4430sdp.c and that is not working at all (using
wrong ID for the PWM).
I think I can remove the legacy way of requesting the pwm from the driver
safely (I can also mark the pwm_id in struct led_pwm as deprecated at the same
time).
Will send the v2 soon.
Thank you,
Péter
>
> Thanks,
> -Bryan
>
>> + }
>> + return pwm;
>> +}
>> +
>> static int led_pwm_probe(struct platform_device *pdev)
>> {
>> struct led_pwm_platform_data *pdata = pdev->dev.platform_data;
>> @@ -67,8 +80,7 @@ static int led_pwm_probe(struct platform_device *pdev)
>> cur_led = >leds[i];
>> led_dat = _data[i];
>>
>> - led_dat->pwm = pwm_request(cur_led->pwm_id,
>> - cur_led->name);
>> + led_dat->pwm = led_pwm_request_pwm(pdev, cur_led);
>> if (IS_ERR(led_dat->pwm)) {
>> ret = PTR_ERR(led_dat->pwm);
>> dev_err(>dev, "unable to request PWM %d\n",
>> --
>> 1.8.0
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] leds: leds-pwm: Convert to use devm_get_pwm
On 11/10/2012 02:48 AM, Bryan Wu wrote:
On Wed, Nov 7, 2012 at 3:42 AM, Peter Ujfalusi peter.ujfal...@ti.com wrote:
Update the driver to use the new API for requesting pwm so we can take
advantage of the pwm_lookup table to find the correct pwm to be used for the
LED functionality.
If the devm_get_pwm fails we fall back to legacy mode to try to get the pwm.
Signed-off-by: Peter Ujfalusi peter.ujfal...@ti.com
---
drivers/leds/leds-pwm.c | 16 ++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index f2e44c7..6bf9445 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -47,6 +47,19 @@ static void led_pwm_set(struct led_classdev *led_cdev,
}
}
+static struct pwm_device *led_pwm_request_pwm(struct platform_device *pdev,
+ struct led_pwm *cur_led)
+{
+ struct pwm_device *pwm;
+
+ pwm = devm_pwm_get(pdev-dev, cur_led-name);
+ if (IS_ERR(pwm)) {
+ dev_err(pdev-dev, unable to request PWM, trying legacy
API\n);
+ pwm = pwm_request(cur_led-pwm_id, cur_led-name);
Looks good, but why we still need to keep the old API pwm_request(),
if devm_pwm_get() is the right replacement. AFAIK, devm_***() API will
help to clean up if device probing fails.
So if it is good enough, we can just replace pwm_request() to the
devm_pwm_get()
I have greped for leds_pwm in arch/ and the only place it is used is the
arch/arm/mach-omap2/board-sdp4430sdp.c and that is not working at all (using
wrong ID for the PWM).
I think I can remove the legacy way of requesting the pwm from the driver
safely (I can also mark the pwm_id in struct led_pwm as deprecated at the same
time).
Will send the v2 soon.
Thank you,
Péter
Thanks,
-Bryan
+ }
+ return pwm;
+}
+
static int led_pwm_probe(struct platform_device *pdev)
{
struct led_pwm_platform_data *pdata = pdev-dev.platform_data;
@@ -67,8 +80,7 @@ static int led_pwm_probe(struct platform_device *pdev)
cur_led = pdata-leds[i];
led_dat = leds_data[i];
- led_dat-pwm = pwm_request(cur_led-pwm_id,
- cur_led-name);
+ led_dat-pwm = led_pwm_request_pwm(pdev, cur_led);
if (IS_ERR(led_dat-pwm)) {
ret = PTR_ERR(led_dat-pwm);
dev_err(pdev-dev, unable to request PWM %d\n,
--
1.8.0
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] pwm: New driver to support PWM driven LEDs on TWL4030/6030 series of PMICs
On 11/08/2012 01:29 PM, Grazvydas Ignotas wrote:
>> But I want to note that I'm currently trying to clean up the mess around
>> twl-core. In my view we have quite a bit of redundancy in there. The PWM A/B
>> is for driving the LED A/B outputs. We should have only these modules for
>> PWM/LED in twl-core:
>> TWL_MODULE_PWM - offset for PWM0ON register in twl4030 and PWM1ON for twl6030
>> TWL_MODULE_LED - offset for LEDEN register in twl4030 and LED_PWM_CTRL1
>> for twl6030
>>
>> From here the driver can figure out what to do IMHO.
>>
>> There's no need to have separate TWL 'modules' for:
>> TWL4030_BASEADD_PWM0
>> TWL4030_BASEADD_PWM1
>> TWL4030_BASEADD_PWMA
>> TWL4030_BASEADD_PWMB
>
> Well all these seem to come from TRM, no hard feelings here too but if
> you are going to remove them, probably worth adding a comment.
>From the 'outside' of twl4030 we have: LEDA, LEDB, PWM0 and PWM1 pins. This is
more important from system integration point of view than what name the TRM
calls the PWM (PWMA) behind of the LEDA terminal for example.
At the end in the board file you will have to use something like this:
static struct pwm_lookup zoom_pwm_lookup[] = {
PWM_LOOKUP("twl-pwm", 0, "leds_pwm", "zoom::keypad"), /* PWM0 */
PWM_LOOKUP("twl-pwm", 1, "pwm-backlight", "backlight"), /* PWM1 */
PWM_LOOKUP("twl-pwm-led", 0, "leds_pwm", "zoom::blinking"), /* LEDA */
};
I'll add comment to both the pwm-twl and pwm-twl-led driver for clarification.
--
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] pwm: New driver to support PWM driven LEDs on TWL4030/6030 series of PMICs
On 11/08/2012 01:29 PM, Grazvydas Ignotas wrote:
But I want to note that I'm currently trying to clean up the mess around
twl-core. In my view we have quite a bit of redundancy in there. The PWM A/B
is for driving the LED A/B outputs. We should have only these modules for
PWM/LED in twl-core:
TWL_MODULE_PWM - offset for PWM0ON register in twl4030 and PWM1ON for twl6030
TWL_MODULE_LED - offset for LEDEN register in twl4030 and LED_PWM_CTRL1
for twl6030
From here the driver can figure out what to do IMHO.
There's no need to have separate TWL 'modules' for:
TWL4030_BASEADD_PWM0
TWL4030_BASEADD_PWM1
TWL4030_BASEADD_PWMA
TWL4030_BASEADD_PWMB
Well all these seem to come from TRM, no hard feelings here too but if
you are going to remove them, probably worth adding a comment.
From the 'outside' of twl4030 we have: LEDA, LEDB, PWM0 and PWM1 pins. This is
more important from system integration point of view than what name the TRM
calls the PWM (PWMA) behind of the LEDA terminal for example.
At the end in the board file you will have to use something like this:
static struct pwm_lookup zoom_pwm_lookup[] = {
PWM_LOOKUP(twl-pwm, 0, leds_pwm, zoom::keypad), /* PWM0 */
PWM_LOOKUP(twl-pwm, 1, pwm-backlight, backlight), /* PWM1 */
PWM_LOOKUP(twl-pwm-led, 0, leds_pwm, zoom::blinking), /* LEDA */
};
I'll add comment to both the pwm-twl and pwm-twl-led driver for clarification.
--
Péter
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] pwm: New driver to support PWM driven LEDs on TWL4030/6030 series of PMICs
On 11/07/2012 07:12 PM, Grazvydas Ignotas wrote:
>> +static int twl4030_pwmled_config(struct pwm_chip *chip, struct pwm_device
>> *pwm,
>> + int duty_ns, int period_ns)
>> +{
>> + int duty_cycle = (duty_ns * TWL4030_LED_MAX) / period_ns;
>> + u8 on_time;
>> + u8 pwm_config[2];
>> + int base, ret;
>> +
>> + if (duty_cycle >= TWL4030_LED_MAX)
>> + on_time = TWL4030_LED_MAX;
>> + else if (!duty_cycle)
>> + on_time = TWL4030_LED_MAX - 1;
>> + else
>> + on_time = TWL4030_LED_MAX - duty_cycle;
>> +
>> + base = pwm->hwpwm * 2 + TWL4030_PWMA_REG;
>> +
>> + pwm_config[0] = on_time;
>> + pwm_config[1] = TWL4030_LED_MAX;
>> +
>> + ret = twl_i2c_write(TWL4030_MODULE_LED, pwm_config, base, 2);
>
> Shouldn't this use TWL4030_MODULE_PWMA and TWL4030_MODULE_PWMB as
> first argument? I can guess it works your way too, but
> TWL4030_MODULE_PWMx would match the TRM better.
I don't have strong opinion regarding to this. You mean changing from:
base = pwm->hwpwm * 2 + TWL4030_PWMA_REG;
ret = twl_i2c_write(TWL4030_MODULE_LED, pwm_config, base, 2);
to:
if (pwm->hwpwm)
module = TWL4030_MODULE_PWMB;
else
module = TWL4030_MODULE_PWMA;
ret = twl_i2c_write(module, pwm_config, 0, 2);
But I want to note that I'm currently trying to clean up the mess around
twl-core. In my view we have quite a bit of redundancy in there. The PWM A/B
is for driving the LED A/B outputs. We should have only these modules for
PWM/LED in twl-core:
TWL_MODULE_PWM - offset for PWM0ON register in twl4030 and PWM1ON for twl6030
TWL_MODULE_LED - offset for LEDEN register in twl4030 and LED_PWM_CTRL1
for twl6030
>From here the driver can figure out what to do IMHO.
There's no need to have separate TWL 'modules' for:
TWL4030_BASEADD_PWM0
TWL4030_BASEADD_PWM1
TWL4030_BASEADD_PWMA
TWL4030_BASEADD_PWMB
--
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] pwm: New driver to support PWMs on TWL4030/6030 series of PMICs
On 11/07/2012 06:50 PM, Grazvydas Ignotas wrote:
>> +static int twl4030_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> + int ret;
>> + u8 val;
>> +
>> + ret = twl_i2c_read_u8(TWL4030_MODULE_INTBR, , TWL4030_GPBR1_REG);
>> + if (ret < 0) {
>> + dev_err(chip->dev, "%s: Failed to read GPBR1\n", pwm->label);
>> + return ret;
>> + }
>> +
>> + val |= TWL4030_PWM_TOGGLE(pwm->hwpwm, TWL4030_PWMX_BITS);
>
> In my experience doing it like this doesn't work reliably, i.e.
> sometimes it just won't enable. I had to first set CLK_ENABLE bit, and
> then ENABLE bit with separate i2c write. Perhaps it needs a cycle or
> two of 32k clock or something (that doesn't seem to be documented
> though).
Thanks, I'll change to the reliable sequence. I do not have HW where I can
test the twl4030 PWMs.
>
>> +
>> + ret = twl_i2c_write_u8(TWL4030_MODULE_INTBR, val, TWL4030_GPBR1_REG);
>> + if (ret < 0)
>> + dev_err(chip->dev, "%s: Failed to enable PWM\n", pwm->label);
>> +
>> + return ret;
>> +}
>> +
>> +static void twl4030_pwm_disable(struct pwm_chip *chip, struct pwm_device
>> *pwm)
>> +{
>> + int ret;
>> + u8 val;
>> +
>> + ret = twl_i2c_read_u8(TWL4030_MODULE_INTBR, , TWL4030_GPBR1_REG);
>> + if (ret < 0) {
>> + dev_err(chip->dev, "%s: Failed to read GPBR1\n", pwm->label);
>> + return;
>> + }
>> +
>> + val &= ~TWL4030_PWM_TOGGLE(pwm->hwpwm, TWL4030_PWMX_BITS);
>
> Same problem here, I would sometimes get LED stuck at full brightness
> after this, first clearing ENABLE and then CLK_ENABLE fixed it (we
> have charger LED connected to PWM1 on pandora).
I would guessed that if we need special care we should have turned off CLK
followed by disabling the PWM.
I'll use the sequence you described in the next version.
>
>> +
>> + ret = twl_i2c_write_u8(TWL4030_MODULE_INTBR, val, TWL4030_GPBR1_REG);
>> + if (ret < 0)
>> + dev_err(chip->dev, "%s: Failed to disable PWM\n",
>> pwm->label);
>> +}
>> +
>> +static int twl4030_pwm_request(struct pwm_chip *chip, struct pwm_device
>> *pwm)
>> +{
>> + struct twl_pwm_chip *twl = container_of(chip, struct twl_pwm_chip,
>> + chip);
>> + int ret;
>> + u8 val, mask, bits;
>> +
>> + ret = twl_i2c_read_u8(TWL4030_MODULE_INTBR, , TWL4030_PMBR1_REG);
>> + if (ret < 0) {
>> + dev_err(chip->dev, "%s: Failed to read PMBR1\n", pwm->label);
>> + return ret;
>> + }
>> +
>> + if (pwm->hwpwm) {
>> + /* PWM 1 */
>> + mask = TWL4030_GPIO7_VIBRASYNC_PWM1_MASK;
>> + bits = TWL4030_GPIO7_VIBRASYNC_PWM1_PWM1;
>> + } else {
>> + /* PWM 0 */
>> + mask = TWL4030_GPIO6_PWM0_MUTE_MASK;
>> + bits = TWL4030_GPIO6_PWM0_MUTE_PWM0;
>> + }
>> +
>> + /* Save the current MUX configuration for the PWM */
>> + twl->twl4030_pwm_mux &= ~mask;
>> + twl->twl4030_pwm_mux |= (val & mask);
>
> Do we really need this mask clearing here? After probe twl4030_pwm_mux
> should be zero, and if twl4030_pwm_request is called twice you don't
> clear the important bits before |=, I think 'twl4030_pwm_mux = val &
> mask' would be better here.
I'm storing both PWM's state in the same variable, but in different offsets:
PWM0: bits 2-3
PWM1: bits 4-5
Probably it is over engineering to clear the relevant bits in the backup
storage, but better to be safe IMHO.
I would leave this part as it is.
--
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] pwm: New driver to support PWMs on TWL4030/6030 series of PMICs
On 11/07/2012 06:50 PM, Grazvydas Ignotas wrote:
+static int twl4030_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ int ret;
+ u8 val;
+
+ ret = twl_i2c_read_u8(TWL4030_MODULE_INTBR, val, TWL4030_GPBR1_REG);
+ if (ret 0) {
+ dev_err(chip-dev, %s: Failed to read GPBR1\n, pwm-label);
+ return ret;
+ }
+
+ val |= TWL4030_PWM_TOGGLE(pwm-hwpwm, TWL4030_PWMX_BITS);
In my experience doing it like this doesn't work reliably, i.e.
sometimes it just won't enable. I had to first set CLK_ENABLE bit, and
then ENABLE bit with separate i2c write. Perhaps it needs a cycle or
two of 32k clock or something (that doesn't seem to be documented
though).
Thanks, I'll change to the reliable sequence. I do not have HW where I can
test the twl4030 PWMs.
+
+ ret = twl_i2c_write_u8(TWL4030_MODULE_INTBR, val, TWL4030_GPBR1_REG);
+ if (ret 0)
+ dev_err(chip-dev, %s: Failed to enable PWM\n, pwm-label);
+
+ return ret;
+}
+
+static void twl4030_pwm_disable(struct pwm_chip *chip, struct pwm_device
*pwm)
+{
+ int ret;
+ u8 val;
+
+ ret = twl_i2c_read_u8(TWL4030_MODULE_INTBR, val, TWL4030_GPBR1_REG);
+ if (ret 0) {
+ dev_err(chip-dev, %s: Failed to read GPBR1\n, pwm-label);
+ return;
+ }
+
+ val = ~TWL4030_PWM_TOGGLE(pwm-hwpwm, TWL4030_PWMX_BITS);
Same problem here, I would sometimes get LED stuck at full brightness
after this, first clearing ENABLE and then CLK_ENABLE fixed it (we
have charger LED connected to PWM1 on pandora).
I would guessed that if we need special care we should have turned off CLK
followed by disabling the PWM.
I'll use the sequence you described in the next version.
+
+ ret = twl_i2c_write_u8(TWL4030_MODULE_INTBR, val, TWL4030_GPBR1_REG);
+ if (ret 0)
+ dev_err(chip-dev, %s: Failed to disable PWM\n,
pwm-label);
+}
+
+static int twl4030_pwm_request(struct pwm_chip *chip, struct pwm_device
*pwm)
+{
+ struct twl_pwm_chip *twl = container_of(chip, struct twl_pwm_chip,
+ chip);
+ int ret;
+ u8 val, mask, bits;
+
+ ret = twl_i2c_read_u8(TWL4030_MODULE_INTBR, val, TWL4030_PMBR1_REG);
+ if (ret 0) {
+ dev_err(chip-dev, %s: Failed to read PMBR1\n, pwm-label);
+ return ret;
+ }
+
+ if (pwm-hwpwm) {
+ /* PWM 1 */
+ mask = TWL4030_GPIO7_VIBRASYNC_PWM1_MASK;
+ bits = TWL4030_GPIO7_VIBRASYNC_PWM1_PWM1;
+ } else {
+ /* PWM 0 */
+ mask = TWL4030_GPIO6_PWM0_MUTE_MASK;
+ bits = TWL4030_GPIO6_PWM0_MUTE_PWM0;
+ }
+
+ /* Save the current MUX configuration for the PWM */
+ twl-twl4030_pwm_mux = ~mask;
+ twl-twl4030_pwm_mux |= (val mask);
Do we really need this mask clearing here? After probe twl4030_pwm_mux
should be zero, and if twl4030_pwm_request is called twice you don't
clear the important bits before |=, I think 'twl4030_pwm_mux = val
mask' would be better here.
I'm storing both PWM's state in the same variable, but in different offsets:
PWM0: bits 2-3
PWM1: bits 4-5
Probably it is over engineering to clear the relevant bits in the backup
storage, but better to be safe IMHO.
I would leave this part as it is.
--
Péter
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] pwm: New driver to support PWM driven LEDs on TWL4030/6030 series of PMICs
On 11/07/2012 07:12 PM, Grazvydas Ignotas wrote:
+static int twl4030_pwmled_config(struct pwm_chip *chip, struct pwm_device
*pwm,
+ int duty_ns, int period_ns)
+{
+ int duty_cycle = (duty_ns * TWL4030_LED_MAX) / period_ns;
+ u8 on_time;
+ u8 pwm_config[2];
+ int base, ret;
+
+ if (duty_cycle = TWL4030_LED_MAX)
+ on_time = TWL4030_LED_MAX;
+ else if (!duty_cycle)
+ on_time = TWL4030_LED_MAX - 1;
+ else
+ on_time = TWL4030_LED_MAX - duty_cycle;
+
+ base = pwm-hwpwm * 2 + TWL4030_PWMA_REG;
+
+ pwm_config[0] = on_time;
+ pwm_config[1] = TWL4030_LED_MAX;
+
+ ret = twl_i2c_write(TWL4030_MODULE_LED, pwm_config, base, 2);
Shouldn't this use TWL4030_MODULE_PWMA and TWL4030_MODULE_PWMB as
first argument? I can guess it works your way too, but
TWL4030_MODULE_PWMx would match the TRM better.
I don't have strong opinion regarding to this. You mean changing from:
base = pwm-hwpwm * 2 + TWL4030_PWMA_REG;
ret = twl_i2c_write(TWL4030_MODULE_LED, pwm_config, base, 2);
to:
if (pwm-hwpwm)
module = TWL4030_MODULE_PWMB;
else
module = TWL4030_MODULE_PWMA;
ret = twl_i2c_write(module, pwm_config, 0, 2);
But I want to note that I'm currently trying to clean up the mess around
twl-core. In my view we have quite a bit of redundancy in there. The PWM A/B
is for driving the LED A/B outputs. We should have only these modules for
PWM/LED in twl-core:
TWL_MODULE_PWM - offset for PWM0ON register in twl4030 and PWM1ON for twl6030
TWL_MODULE_LED - offset for LEDEN register in twl4030 and LED_PWM_CTRL1
for twl6030
From here the driver can figure out what to do IMHO.
There's no need to have separate TWL 'modules' for:
TWL4030_BASEADD_PWM0
TWL4030_BASEADD_PWM1
TWL4030_BASEADD_PWMA
TWL4030_BASEADD_PWMB
--
Péter
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] CLK: clk-twl6040: Initial clock driver for OMAP4+ McPDM fclk clock
Hi Mike, On 10/29/2012 06:30 PM, Mike Turquette wrote: > Peter, > > The patch looks good to me. To be clear, you wan this to go through > clk-next or the mfd tree for avoiding conflicts? clk-next is the best place for it I think. The MFD part to register the device can still wait till we have ccf support on OMAP4. Thank you, Péter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] CLK: clk-twl6040: Initial clock driver for OMAP4+ McPDM fclk clock
Hi Mike, On 10/29/2012 06:30 PM, Mike Turquette wrote: Peter, The patch looks good to me. To be clear, you wan this to go through clk-next or the mfd tree for avoiding conflicts? clk-next is the best place for it I think. The MFD part to register the device can still wait till we have ccf support on OMAP4. Thank you, Péter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] Input: twl*-vibra probe cleanups (devm_* conversion)
Hi Dmitry, On 06/15/2012 12:22 PM, Peter Ujfalusi wrote: > Hello, > > Convert the twl4030, twl6040 vibra drivers to use devm_ at probe time to clean > up the code a bit. > The twl6040-vibra driver needed bigger change since the ordering of the > initialization was not correct. Would it be possible to include this two patch for 3.7-rc2? I can resend them if you no longer have them. Thank you, Péter > > Regards, > Peter > --- > Peter Ujfalusi (2): > Input: twl4030-vibra: Convert to use devm_* in probe > Input: twl6040-vibra: Code cleanup in probe with devm_* conversion > > drivers/input/misc/twl4030-vibra.c |8 +-- > drivers/input/misc/twl6040-vibra.c | 99 --- > 2 files changed, 47 insertions(+), 60 deletions(-) > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] Input: twl*-vibra probe cleanups (devm_* conversion)
Hi Dmitry, On 06/15/2012 12:22 PM, Peter Ujfalusi wrote: Hello, Convert the twl4030, twl6040 vibra drivers to use devm_ at probe time to clean up the code a bit. The twl6040-vibra driver needed bigger change since the ordering of the initialization was not correct. Would it be possible to include this two patch for 3.7-rc2? I can resend them if you no longer have them. Thank you, Péter Regards, Peter --- Peter Ujfalusi (2): Input: twl4030-vibra: Convert to use devm_* in probe Input: twl6040-vibra: Code cleanup in probe with devm_* conversion drivers/input/misc/twl4030-vibra.c |8 +-- drivers/input/misc/twl6040-vibra.c | 99 --- 2 files changed, 47 insertions(+), 60 deletions(-) -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] ARM: OMAP: Trivial driver changes to remove include plat/cpu.h
On 10/08/2012 07:35 PM, Tony Lindgren wrote: > - omap-dma.c and omap-pcm.c can test the arch locally as > omap1 and omap2 cannot be compiled together because of > conflicting compiler flags > sound/soc/omap/omap-pcm.c |9 +++-- Tony: is this going to be included in 3.7? Acked-by: Peter Ujfalusi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] ARM: OMAP: Trivial driver changes to remove include plat/cpu.h
On 10/08/2012 07:35 PM, Tony Lindgren wrote: - omap-dma.c and omap-pcm.c can test the arch locally as omap1 and omap2 cannot be compiled together because of conflicting compiler flags sound/soc/omap/omap-pcm.c |9 +++-- Tony: is this going to be included in 3.7? Acked-by: Peter Ujfalusi peter.ujfal...@ti.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
