From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3EE9FC49361 for ; Thu, 17 Jun 2021 18:07:07 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 20E0D61042 for ; Thu, 17 Jun 2021 18:07:07 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232111AbhFQSJN (ORCPT ); Thu, 17 Jun 2021 14:09:13 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57008 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232193AbhFQSJE (ORCPT ); Thu, 17 Jun 2021 14:09:04 -0400 Received: from mail-oi1-x22d.google.com (mail-oi1-x22d.google.com [IPv6:2607:f8b0:4864:20::22d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 103B7C061574 for ; Thu, 17 Jun 2021 11:06:55 -0700 (PDT) Received: by mail-oi1-x22d.google.com with SMTP id q10so7458620oij.5 for ; Thu, 17 Jun 2021 11:06:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=j0BoPIDWyRZ7eAOWVfRNbOKSmlASWPNoA23yKtrI5ho=; b=nagItiJOYVW49+/5vU4doda2lcVxieX6XBp6OU1p7rLIlna2fWea5Hzux1s9Qv3XPp mkzQFz/7ikotqVDzlYJEj7ohH6nAydgqN72Cu9zo574X732cQLhnEgh/L13zG2sJ+qh1 q/b1FeD7vJCdGWeHuuMLDulmYr3r3z53O5102oNNr6ZdqLVzmD/xbqeG3E5vN/MmmZON bjGoe89sMUlSJHu8JKkSGkN8CZqvBk+2xvO3phc9VAsxPO7IoCf4pgZP8H/H8tidQ0l3 Hi+whcqq//54vhfzIak3thse8mRnFwNvO9xR/GOknzjYbXWSGH3Qv3DUH2ELZpbBkR2P Dcog== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=j0BoPIDWyRZ7eAOWVfRNbOKSmlASWPNoA23yKtrI5ho=; b=FICs9G3LWKmA1Wa74O3BQWDdqLMn7Lvy+01rmXPWq5o3CSjnRGALBEqICfksgl+s5Q JETtJ8xrrJfU9zFgvGn3eQ2D+BD2CuBjBnAKKJKS0MJkO+FEhIVnY20ImwDlQBwEfU4T z5RUCSF2efd+gfvDYlpHXtabqMTN8+sbkZLOE06WAoG+/fBHq9l6rPNlFYdVTl1oye55 tCI8aFKauqpyao7HVXM9iKY+3KkSRUocvVlZi7yuyP30CGoq71t3gX8rQtI3GAozuGhc gW7xV7psxi4K0xDektlpoqgKWfzRNscrPbsjMHMDXHvYS8n/73RhO80Z6CfYFFVa40nd /BBA== X-Gm-Message-State: AOAM531mU8TS+PvBxdRZkwNQ31T5nRv0Yp/R++DgpUhdiZh0mCe1yK7B ng0OglUyuleE2vwHJ7vqFQiV8g== X-Google-Smtp-Source: ABdhPJy+fr+LYbN4bzr1VMR9PctoEjjX+j+mrqwEdqZJcP5Mn9YqiVzeZEiVjQZScItvxKRqPZxw8g== X-Received: by 2002:aca:3385:: with SMTP id z127mr4149757oiz.142.1623953214100; Thu, 17 Jun 2021 11:06:54 -0700 (PDT) Received: from yoga (104-57-184-186.lightspeed.austtx.sbcglobal.net. [104.57.184.186]) by smtp.gmail.com with ESMTPSA id l24sm1219735oii.45.2021.06.17.11.06.52 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 17 Jun 2021 11:06:53 -0700 (PDT) Date: Thu, 17 Jun 2021 13:06:51 -0500 From: Bjorn Andersson To: Uwe Kleine-K?nig Cc: Andrzej Hajda , Neil Armstrong , Robert Foss , Laurent Pinchart , Jonas Karlman , Jernej Skrabec , David Airlie , Daniel Vetter , Thierry Reding , Lee Jones , Doug Anderson , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, linux-pwm@vger.kernel.org Subject: Re: [PATCH v2 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip Message-ID: References: <20210615231828.835164-1-bjorn.andersson@linaro.org> <20210615231828.835164-2-bjorn.andersson@linaro.org> <20210616075637.jtoa25uyhnqkctlu@pengutronix.de> <20210617062449.qwsjcpkyiwzdyfi3@pengutronix.de> <20210617165433.ltugrrj6ntmc6j57@pengutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20210617165433.ltugrrj6ntmc6j57@pengutronix.de> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu 17 Jun 11:54 CDT 2021, Uwe Kleine-K?nig wrote: > Hello Bjorn, > > On Thu, Jun 17, 2021 at 11:38:26AM -0500, Bjorn Andersson wrote: > > On Thu 17 Jun 01:24 CDT 2021, Uwe Kleine-K?nig wrote: > > > On Wed, Jun 16, 2021 at 10:22:17PM -0500, Bjorn Andersson wrote: > > > > > > +static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > > > > + const struct pwm_state *state) > > > > > > +{ > > > > > > + struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip); > > > > > > + unsigned int pwm_en_inv; > > > > > > + unsigned int backlight; > > > > > > + unsigned int pre_div; > > > > > > + unsigned int scale; > > > > > > + int ret; > > > > > > + > > > > > > + if (!pdata->pwm_enabled) { > > > > > > + ret = pm_runtime_get_sync(pdata->dev); > > > > > > + if (ret < 0) > > > > > > + return ret; > > > > > > + > > > > > > + ret = regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG, > > > > > > + SN_GPIO_MUX_MASK << (2 * SN_PWM_GPIO_IDX), > > > > > > + SN_GPIO_MUX_SPECIAL << (2 * SN_PWM_GPIO_IDX)); > > > > > > + if (ret) { > > > > > > + dev_err(pdata->dev, "failed to mux in PWM function\n"); > > > > > > + goto out; > > > > > > + } > > > > > > > > > > Do you need to do this even if state->enabled is false? > > > > > > > > I presume I should be able to explicitly mux in the GPIO function and > > > > configure that to output low. But I am not able to find anything in the > > > > data sheet that would indicate this to be preferred. > > > > > > My question targetted a different case. If the PWM is off > > > (!pdata->pwm_enabled) and should remain off (state->enabled is false) > > > you can shortcut here, can you not? > > > > Right, if we're going off->off then we don't need to touch the hardware. > > > > But am I expected to -EINVAL improper period and duty cycle even though > > enabled is false? > > > > And also, what is the supposed behavior of enabled = false? Is it > > supposedly equivalent of asking for a duty_cycle of 0? > > In my book enabled = false is just syntactic sugar to say: > "duty_cycle=0, period=something small". So to answer your questions: if > enabled = false, the consumer doesn't really care about period and > duty_cycle. Some care that the output becomes inactive, some others > don't, so from my POV just emit the inactive level on the output and > ignore period and duty_cycle. > Giving this some further thought. In order to have a known state of the PWM signal we need the sn65dsi86 to be powered. The documentation describes a "suspend mode", but this is currently not implemented in the driver, so there's a large power cost coming from just keeping the pin low when disabled. As such in the current implementation I use state->enabled to also control if the device should be kept powered, which means that this follows the backlight power status of the pwm-backlight. Which is sufficient as the backlight won't be powered when !state->enabled. For the typical use case the pwm-backlight has some independent control over gpio and power to toggle the actual backlight. But in the even that this wouldn't be available I think we need to extend the driver to implement the suspend mode. In which case muxing in the PWM function should probably happen at the time the PWM channel is requested. This does come at an additional power cost (not as high as keeping the chip awake though), so (in addition to the effort) I think it's reasonable to document this as a limitation for now and implement this as the need arise. Thanks, Bjorn > > > > > Does this already modify the output pin? > > > > > > > > Yes, coming out of reset this pin is configured as input, so switching > > > > the mux here will effectively start driving the pin. > > > > > > So please document this in the format the recently added drivers do, > > > too. See e.g. drivers/pwm/pwm-sifive.c. (The idea is to start that with > > > " * Limitations:" to make it easy to grep it.) > > > > > > > Okay, will do. Although I believe that for this driver it makes sense to > > place such comment close to this function, rather than at the top of the > > driver. > > Yes, agreed. > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ |