Re: [greybus-dev] [PATCH 1/1] staging: greybus: Added do - while in multi statement macro
On 11-02-21, 11:00, Greg KH wrote: > On Thu, Feb 11, 2021 at 03:24:44PM +0530, Hemansh Agnihotri wrote: > > This patch add fixes an checkpatch error for "Macros with multiple > > statements > > should be enclosed in a do - while loop" > > > > Signed-off-by: Hemansh Agnihotri > > Any reason you didn't test-build your patch before sending it out? > > That's a bit rude to reviewers :( I also wonder how two people stumbled upon the exact same thing at the same time. Copy/paste ? https://lore.kernel.org/lkml/20210210221439.3489-2-yildirim.fa...@gmail.com/ And of course NAK for the patch. The macro is used outside of any other routine, and is actually used to create routines. No do-while required here. -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 2/9] cpufreq: sfi-cpufreq: Remove driver for deprecated firmware
On 11-02-21, 15:40, Andy Shevchenko wrote: > SFI-based platforms are gone. So does this driver. > > Signed-off-by: Andy Shevchenko > Acked-by: Linus Walleij > --- > drivers/cpufreq/Kconfig.x86 | 10 --- > drivers/cpufreq/Makefile | 1 - > drivers/cpufreq/sfi-cpufreq.c | 127 -- > 3 files changed, 138 deletions(-) > delete mode 100644 drivers/cpufreq/sfi-cpufreq.c Acked-by: Viresh Kumar -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] staging: greybus: Fixed a misspelling in hid.c
On 12-02-21, 09:54, Greg KH wrote: > On Fri, Feb 12, 2021 at 09:44:04AM +0100, Johan Hovold wrote: > > On Fri, Feb 12, 2021 at 01:48:35PM +0530, Pritthijit Nath wrote: > > > Fixed the spelling of 'transfered' to 'transferred'. > > > > > > Signed-off-by: Pritthijit Nath > > > --- > > > drivers/staging/greybus/hid.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/staging/greybus/hid.c b/drivers/staging/greybus/hid.c > > > index a56c3fb5d35a..6b19ff4743a9 100644 > > > --- a/drivers/staging/greybus/hid.c > > > +++ b/drivers/staging/greybus/hid.c > > > @@ -254,7 +254,7 @@ static int __gb_hid_output_raw_report(struct > > > hid_device *hid, __u8 *buf, > > > > > > ret = gb_hid_set_report(ghid, report_type, report_id, buf, len); > > > if (report_id && ret >= 0) > > > - ret++; /* add report_id to the number of transfered bytes */ > > > + ret++; /* add report_id to the number of transferrid bytes */ > > > > You now misspelled transferred in a different way. > > Oops, will go revert this, I need more coffee... Yeah, its Friday.. You need a break too :) -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] staging: greybus: Fixed alignment issue in hid.c
On 12-02-21, 13:48, Pritthijit Nath wrote:
> This change fixes a checkpatch CHECK style issue for "Alignment should match
> open parenthesis".
>
> Signed-off-by: Pritthijit Nath
> ---
> drivers/staging/greybus/hid.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/greybus/hid.c b/drivers/staging/greybus/hid.c
> index ed706f39e87a..a56c3fb5d35a 100644
> --- a/drivers/staging/greybus/hid.c
> +++ b/drivers/staging/greybus/hid.c
> @@ -221,8 +221,8 @@ static void gb_hid_init_reports(struct gb_hid *ghid)
> }
>
> static int __gb_hid_get_raw_report(struct hid_device *hid,
> - unsigned char report_number, __u8 *buf, size_t count,
> - unsigned char report_type)
> +unsigned char report_number, __u8 *buf,
> size_t count,
> +unsigned char report_type)
> {
> struct gb_hid *ghid = hid->driver_data;
> int ret;
I can't even count the number of attempts we have seen in previous
years to make checkpatch --strict happy for greybus.
I say we make this change once and for all across greybus, so we don't
have to review or NAK someone afterwards.
Should I send a patch for this ?
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] staging: greybus: Fixed alignment issue in hid.c
On 12-02-21, 10:17, Greg KH wrote:
> On Fri, Feb 12, 2021 at 02:39:26PM +0530, Viresh Kumar wrote:
> > On 12-02-21, 13:48, Pritthijit Nath wrote:
> > > This change fixes a checkpatch CHECK style issue for "Alignment should
> > > match
> > > open parenthesis".
> > >
> > > Signed-off-by: Pritthijit Nath
> > > ---
> > > drivers/staging/greybus/hid.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/staging/greybus/hid.c b/drivers/staging/greybus/hid.c
> > > index ed706f39e87a..a56c3fb5d35a 100644
> > > --- a/drivers/staging/greybus/hid.c
> > > +++ b/drivers/staging/greybus/hid.c
> > > @@ -221,8 +221,8 @@ static void gb_hid_init_reports(struct gb_hid *ghid)
> > > }
> > >
> > > static int __gb_hid_get_raw_report(struct hid_device *hid,
> > > - unsigned char report_number, __u8 *buf, size_t count,
> > > - unsigned char report_type)
> > > +unsigned char report_number, __u8 *buf,
> > > size_t count,
> > > +unsigned char report_type)
> > > {
> > > struct gb_hid *ghid = hid->driver_data;
> > > int ret;
> >
> > I can't even count the number of attempts we have seen in previous
> > years to make checkpatch --strict happy for greybus.
> >
> > I say we make this change once and for all across greybus, so we don't
> > have to review or NAK someone afterwards.
> >
> > Should I send a patch for this ?
>
> Sure, but note that over time, checkpatch adds new things so there will
> always be something to change in here, until we move it out of the
> drivers/staging/ area :)
Right, though I wasn't worried about other checkpatch warning but
specially the "alignment - parenthesis" one. Everyone (specially
newbies) want to fix that everywhere :)
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] staging: greybus: Fixed alignment issue in hid.c
On 12-02-21, 10:52, Johan Hovold wrote: > But what will the checkpatch crew then work on? Lol.. > Better staging than the rest of the kernel. [ /me wondering on who stops them from sending patches for rest of the kernel ? ] -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 06/10] staging: greybus: spilib: use 'spi_delay_to_ns' for getting xfer delay
On 08-03-21, 16:54, Alexandru Ardelean wrote:
> The intent is the removal of the 'delay_usecs' field from the
> spi_transfer struct, as there is a 'delay' field that does the same
> thing.
>
> The spi_delay_to_ns() can be used to get the transfer delay. It works by
> using the 'delay_usecs' field first (if it is non-zero), and finally
> uses the 'delay' field.
>
> Since the 'delay_usecs' field is going away, this change makes use of the
> spi_delay_to_ns() function. This also means dividing the return value of
> the function by 1000, to convert it to microseconds.
> To prevent any potential faults when converting to microseconds and since
> the result of spi_delay_to_ns() is int, the delay is being computed in 32
> bits and then clamped between 0 & U16_MAX.
>
> Signed-off-by: Alexandru Ardelean
> ---
> drivers/staging/greybus/spilib.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/greybus/spilib.c
> b/drivers/staging/greybus/spilib.c
> index 672d540d3365..30655153df6a 100644
> --- a/drivers/staging/greybus/spilib.c
> +++ b/drivers/staging/greybus/spilib.c
> @@ -245,6 +245,7 @@ static struct gb_operation
> *gb_spi_operation_create(struct gb_spilib *spi,
> /* Fill in the transfers array */
> xfer = spi->first_xfer;
> while (msg->state != GB_SPI_STATE_OP_DONE) {
> + int xfer_delay;
> if (xfer == spi->last_xfer)
> xfer_len = spi->last_xfer_size;
> else
> @@ -259,7 +260,9 @@ static struct gb_operation
> *gb_spi_operation_create(struct gb_spilib *spi,
>
> gb_xfer->speed_hz = cpu_to_le32(xfer->speed_hz);
> gb_xfer->len = cpu_to_le32(xfer_len);
> - gb_xfer->delay_usecs = cpu_to_le16(xfer->delay_usecs);
> + xfer_delay = spi_delay_to_ns(&xfer->delay, xfer) / 1000;
> + xfer_delay = clamp_t(u16, xfer_delay, 0, U16_MAX);
> + gb_xfer->delay_usecs = cpu_to_le16(xfer_delay);
> gb_xfer->cs_change = xfer->cs_change;
> gb_xfer->bits_per_word = xfer->bits_per_word;
Acked-by: Viresh Kumar
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: greybus: fix fw is NULL but dereferenced
On 25-03-21, 18:19, Jian Dong wrote: > From: Jian Dong > > fixes coccicheck Error: > > drivers/staging/greybus/bootrom.c:301:41-45: ERROR: > fw is NULL but dereferenced. > > if procedure goto label directly, ret will be nefative, so the fw is NULL > and the if(condition) end with dereferenced fw. let's fix it. No, fw is accessed only for !ret case. > Signed-off-by: Jian Dong > --- > drivers/staging/greybus/bootrom.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/greybus/bootrom.c > b/drivers/staging/greybus/bootrom.c > index a8efb86..0439efa 100644 > --- a/drivers/staging/greybus/bootrom.c > +++ b/drivers/staging/greybus/bootrom.c > @@ -246,7 +246,7 @@ static int gb_bootrom_get_firmware(struct gb_operation > *op) > struct gb_bootrom_get_firmware_response *firmware_response; > struct device *dev = &op->connection->bundle->dev; > unsigned int offset, size; > - enum next_request_type next_request; > + enum next_request_type next_request = NEXT_REQ_GET_FIRMWARE; > int ret = 0; > > /* Disable timeouts */ > @@ -298,10 +298,10 @@ static int gb_bootrom_get_firmware(struct gb_operation > *op) > > queue_work: > /* Refresh timeout */ > - if (!ret && (offset + size == fw->size)) > - next_request = NEXT_REQ_READY_TO_BOOT; > - else > + if (!!ret) > next_request = NEXT_REQ_GET_FIRMWARE; > + else if (offset + size == fw->size) > + next_request = NEXT_REQ_READY_TO_BOOT; > > gb_bootrom_set_timeout(bootrom, next_request, NEXT_REQ_TIMEOUT_MS); The code is fine AFAICT, the coccicheck is buggy as it is detecting a bug here. -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 17/30] mmc: sdhci-tegra: Support OPP and core voltage scaling
On Thu, Nov 5, 2020 at 5:15 AM Dmitry Osipenko wrote:
> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> +static void sdhci_tegra_deinit_opp_table(void *data)
> +{
> + struct device *dev = data;
> + struct opp_table *opp_table;
> +
> + opp_table = dev_pm_opp_get_opp_table(dev);
So you need to get an OPP table to put one :)
You need to save the pointer returned by dev_pm_opp_set_regulators() instead.
> + dev_pm_opp_of_remove_table(dev);
> + dev_pm_opp_put_regulators(opp_table);
> + dev_pm_opp_put_opp_table(opp_table);
> +}
> +
> +static int devm_sdhci_tegra_init_opp_table(struct device *dev)
> +{
> + struct opp_table *opp_table;
> + const char *rname = "core";
> + int err;
> +
> + /* voltage scaling is optional */
> + if (device_property_present(dev, "core-supply"))
> + opp_table = dev_pm_opp_set_regulators(dev, &rname, 1);
> + else
> + opp_table = dev_pm_opp_get_opp_table(dev);
Nice. I didn't think that someone will end up abusing this API and so made it
available for all, but someone just did that. I will fix that in the OPP core.
Any idea why you are doing what you are doing here ?
> +
> + if (IS_ERR(opp_table))
> + return dev_err_probe(dev, PTR_ERR(opp_table),
> + "failed to prepare OPP table\n");
> +
> + /*
> +* OPP table presence is optional and we want the set_rate() of OPP
> +* API to work similarly to clk_set_rate() if table is missing in a
> +* device-tree. The add_table() errors out if OPP is missing in DT.
> +*/
> + if (device_property_present(dev, "operating-points-v2")) {
> + err = dev_pm_opp_of_add_table(dev);
> + if (err) {
> + dev_err(dev, "failed to add OPP table: %d\n", err);
> + goto put_table;
> + }
> + }
> +
> + err = devm_add_action(dev, sdhci_tegra_deinit_opp_table, dev);
> + if (err)
> + goto remove_table;
> +
> + return 0;
> +
> +remove_table:
> + dev_pm_opp_of_remove_table(dev);
> +put_table:
> + dev_pm_opp_put_regulators(opp_table);
> +
> + return err;
> +}
> +
> static int sdhci_tegra_probe(struct platform_device *pdev)
> {
> const struct of_device_id *match;
> @@ -1621,6 +1681,10 @@ static int sdhci_tegra_probe(struct platform_device
> *pdev)
> goto err_power_req;
> }
>
> + rc = devm_sdhci_tegra_init_opp_table(&pdev->dev);
> + if (rc)
> + goto err_parse_dt;
> +
> /*
> * Tegra210 has a separate SDMMC_LEGACY_TM clock used for host
> * timeout clock and SW can choose TMCLK or SDCLK for hardware
> --
> 2.27.0
>
> ___
> devel mailing list
> de...@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 00/30] Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs
On 05-11-20, 10:45, Ulf Hansson wrote: > + Viresh Thanks Ulf. I found a bug in OPP core because you cc'd me here :) > On Thu, 5 Nov 2020 at 00:44, Dmitry Osipenko wrote: > I need some more time to review this, but just a quick check found a > few potential issues... > > The "core-supply", that you specify as a regulator for each > controller's device node, is not the way we describe power domains. Maybe I misunderstood your comment here, but there are two ways of scaling the voltage of a device depending on if it is a regulator (and can be modeled as one in the kernel) or a power domain. In case of Qcom earlier (when we added the performance-state stuff), the eventual hardware was out of kernel's control and we didn't wanted (allowed) to model it as a virtual regulator just to pass the votes to the RPM. And so we did what we did. But if the hardware (where the voltage is required to be changed) is indeed a regulator and is modeled as one, then what Dmitry has done looks okay. i.e. add a supply in the device's node and microvolt property in the DT entries. -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 00/30] Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs
On 05-11-20, 11:34, Ulf Hansson wrote: > I am not objecting about scaling the voltage through a regulator, > that's fine to me. However, encoding a power domain as a regulator > (even if it may seem like a regulator) isn't. Well, unless Mark Brown > has changed his mind about this. > > In this case, it seems like the regulator supply belongs in the > description of the power domain provider. Okay, I wasn't sure if it is a power domain or a regulator here. Btw, how do we identify if it is a power domain or a regulator ? > > In case of Qcom earlier (when we added the performance-state stuff), > > the eventual hardware was out of kernel's control and we didn't wanted > > (allowed) to model it as a virtual regulator just to pass the votes to > > the RPM. And so we did what we did. > > > > But if the hardware (where the voltage is required to be changed) is > > indeed a regulator and is modeled as one, then what Dmitry has done > > looks okay. i.e. add a supply in the device's node and microvolt > > property in the DT entries. > > I guess I haven't paid enough attention how power domain regulators > are being described then. I was under the impression that the CPUfreq > case was a bit specific - and we had legacy bindings to stick with. > > Can you point me to some other existing examples of where power domain > regulators are specified as a regulator in each device's node? No, I thought it is a regulator here and not a power domain. -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 00/30] Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs
On 05-11-20, 11:56, Ulf Hansson wrote: > On Thu, 5 Nov 2020 at 11:40, Viresh Kumar wrote: > > Btw, how do we identify if it is a power domain or a regulator ? To be honest, I was a bit afraid and embarrassed to ask this question, and was hoping people to make fun of me in return :) > Good question. It's not a crystal clear line in between them, I think. And I was relieved after reading this :) > A power domain to me, means that some part of a silicon (a group of > controllers or just a single piece, for example) needs some kind of > resource (typically a power rail) to be enabled to be functional, to > start with. Isn't this what a part of regulator does as well ? i.e. enabling/disabling of the regulator or power to a group of controllers. Over that the regulator does voltage/current scaling as well, which normally the power domains don't do (though we did that in performance-state case). > If there are operating points involved, that's also a > clear indication to me, that it's not a regular regulator. Is there any example of that? I hope by OPP you meant both freq and voltage here. I am not sure if I know of a case where a power domain handles both of them. > Maybe we should try to specify this more exactly in some > documentation, somewhere. I think yes, it is very much required. And in absence of that I think, many (or most) of the platforms that also need to scale the voltage would have modeled their hardware as a regulator and not a PM domain. What I always thought was: - Module that can just enable/disable power to a block of SoC is a power domain. - Module that can enable/disable as well as scale voltage is a regulator. And so I thought that this patchset has done the right thing. This changed a bit with the qcom stuff where the IP to be configured was in control of RPM and not Linux and so we couldn't add it as a regulator. If it was controlled by Linux, it would have been a regulator in kernel for sure :) -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 17/30] mmc: sdhci-tegra: Support OPP and core voltage scaling
On 05-11-20, 17:18, Dmitry Osipenko wrote:
> 05.11.2020 12:58, Viresh Kumar пишет:
> >> +static void sdhci_tegra_deinit_opp_table(void *data)
> >> +{
> >> + struct device *dev = data;
> >> + struct opp_table *opp_table;
> >> +
> >> + opp_table = dev_pm_opp_get_opp_table(dev);
> > So you need to get an OPP table to put one :)
> > You need to save the pointer returned by dev_pm_opp_set_regulators()
> > instead.
>
> This is intentional because why do we need to save the pointer if we're
> not using it and we know that we could get this pointer using OPP API?
Because it is highly inefficient and it doesn't follow the rules set
by the OPP core. Hypothetically speaking, the OPP core is free to
allocate the OPP table structure as much as it wants, and if you don't
use the value returned back to you earlier (think of it as a cookie
assigned to your driver), then it will eventually lead to memory leak.
> This is exactly the same what I did for the CPUFreq driver [1] :)
I will strongly suggest you to save the pointer here and do the same
in the cpufreq driver as well.
> >> +static int devm_sdhci_tegra_init_opp_table(struct device *dev)
> >> +{
> >> + struct opp_table *opp_table;
> >> + const char *rname = "core";
> >> + int err;
> >> +
> >> + /* voltage scaling is optional */
> >> + if (device_property_present(dev, "core-supply"))
> >> + opp_table = dev_pm_opp_set_regulators(dev, &rname, 1);
> >> + else
> >
> >> + opp_table = dev_pm_opp_get_opp_table(dev);
To make it further clear, this will end up allocating an OPP table for
you, which it shouldn't have.
> > Nice. I didn't think that someone will end up abusing this API and so made
> > it
> > available for all, but someone just did that. I will fix that in the OPP
> > core.
To be fair, I allowed the cpufreq-dt driver to abuse it too, which I
am going to fix shortly.
> The dev_pm_opp_put_regulators() handles the case where regulator is
> missing by acting as dev_pm_opp_get_opp_table(), but the
> dev_pm_opp_set_regulators() doesn't do it. Hence I don't think this is
> an abuse, but the OPP API drawback.
I am not sure what you meant here. Normally you are required to call
dev_pm_opp_put_regulators() only if you have called
dev_pm_opp_set_regulators() earlier. And the refcount stays in
balance.
> > Any idea why you are doing what you are doing here ?
>
> Two reasons:
>
> 1. Voltage regulator is optional, but dev_pm_opp_set_regulators()
> doesn't support optional regulators.
>
> 2. We need to balance the opp_table refcount in order to use OPP API
> without polluting code with if(have_regulator), hence the
> dev_pm_opp_get_opp_table() is needed for taking the opp_table reference
> to have the same refcount as in the case of the dev_pm_opp_set_regulators().
I am going to send a patchset shortly after which this call to
dev_pm_opp_get_opp_table() will fail, if it is called before adding
the OPP table.
> I guess we could make dev_pm_opp_set_regulators(dev, count) to accept
> regulators count=0 and then act as dev_pm_opp_get_opp_table(dev), will
> it be acceptable?
Setting regulators for count as 0 doesn't sound good to me.
But, I understand that you don't want to have that if (have_regulator)
check, and it is a fair request. What I will instead do is, allow all
dev_pm_opp_put*() API to start accepting a NULL pointer for the OPP
table and fail silently. And so you won't be required to have this
unwanted check. But you will be required to save the pointer returned
back by dev_pm_opp_set_regulators(), which is the right thing to do
anyways.
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 00/30] Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs
On 08-11-20, 15:19, Dmitry Osipenko wrote: > I took a detailed look at the GENPD and tried to implement it. Here is > what was found: > > 1. GENPD framework doesn't aggregate performance requests from the > attached devices. This means that if deviceA requests performance state > 10 and then deviceB requests state 3, then framework will set domain's > state to 3 instead of 10. It does. Look at _genpd_reeval_performance_state(). -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 17/30] mmc: sdhci-tegra: Support OPP and core voltage scaling
On 06-11-20, 21:41, Frank Lee wrote: > On Fri, Nov 6, 2020 at 9:18 PM Dmitry Osipenko wrote: > > > > 06.11.2020 09:15, Viresh Kumar пишет: > > > Setting regulators for count as 0 doesn't sound good to me. > > > > > > But, I understand that you don't want to have that if (have_regulator) > > > check, and it is a fair request. What I will instead do is, allow all > > > dev_pm_opp_put*() API to start accepting a NULL pointer for the OPP > > > table and fail silently. And so you won't be required to have this > > > unwanted check. But you will be required to save the pointer returned > > > back by dev_pm_opp_set_regulators(), which is the right thing to do > > > anyways. > > > > Perhaps even a better variant could be to add a devm versions of the OPP > > API functions, then drivers won't need to care about storing the > > opp_table pointer if it's unused by drivers. > > I think so. The consumer may not be so concerned about the status of > these OPP tables. > If the driver needs to manage the release, it needs to add a pointer > to their driver global structure. > > Maybe it's worth having these devm interfaces for opp. Sure if there are enough users of this, I am all for it. I was fine with the patches you sent, just that there were not a lot of users of it and so I pushed them back. If we find that we have more users of it now, we can surely get that back. -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 17/30] mmc: sdhci-tegra: Support OPP and core voltage scaling
On 09-11-20, 08:08, Dmitry Osipenko wrote: > 09.11.2020 08:00, Viresh Kumar пишет: > > On 06-11-20, 21:41, Frank Lee wrote: > >> On Fri, Nov 6, 2020 at 9:18 PM Dmitry Osipenko wrote: > >>> > >>> 06.11.2020 09:15, Viresh Kumar пишет: > >>>> Setting regulators for count as 0 doesn't sound good to me. > >>>> > >>>> But, I understand that you don't want to have that if (have_regulator) > >>>> check, and it is a fair request. What I will instead do is, allow all > >>>> dev_pm_opp_put*() API to start accepting a NULL pointer for the OPP > >>>> table and fail silently. And so you won't be required to have this > >>>> unwanted check. But you will be required to save the pointer returned > >>>> back by dev_pm_opp_set_regulators(), which is the right thing to do > >>>> anyways. > >>> > >>> Perhaps even a better variant could be to add a devm versions of the OPP > >>> API functions, then drivers won't need to care about storing the > >>> opp_table pointer if it's unused by drivers. > >> > >> I think so. The consumer may not be so concerned about the status of > >> these OPP tables. > >> If the driver needs to manage the release, it needs to add a pointer > >> to their driver global structure. > >> > >> Maybe it's worth having these devm interfaces for opp. > > > > Sure if there are enough users of this, I am all for it. I was fine > > with the patches you sent, just that there were not a lot of users of > > it and so I pushed them back. If we find that we have more users of it > > now, we can surely get that back. > > > > There was already attempt to add the devm? Could you please give me a > link to the patches? > > I already prepared a patch which adds the devm helpers. It helps to keep > code cleaner and readable. https://lore.kernel.org/lkml/20201012135517.19468-1-fr...@allwinnertech.com/ -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 00/30] Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs
On 09-11-20, 08:10, Dmitry Osipenko wrote: > 09.11.2020 07:47, Dmitry Osipenko пишет: > > 09.11.2020 07:43, Viresh Kumar пишет: > >> On 08-11-20, 15:19, Dmitry Osipenko wrote: > >>> I took a detailed look at the GENPD and tried to implement it. Here is > >>> what was found: > >>> > >>> 1. GENPD framework doesn't aggregate performance requests from the > >>> attached devices. This means that if deviceA requests performance state > >>> 10 and then deviceB requests state 3, then framework will set domain's > >>> state to 3 instead of 10. > >> > >> It does. Look at _genpd_reeval_performance_state(). > >> > > > > Thanks, I probably had a bug in the quick prototype and then overlooked > > that function. > > > > If a non-hardware device-tree node is okay to have for the domain, then > I can try again. > > What I also haven't mentioned is that GENPD adds some extra complexity > to some drivers (3d, video decoder) because we will need to handle both > new GENPD and legacy Tegra specific pre-genpd era domains. > > I'm also not exactly sure how the topology of domains should look like > because Tegra has a power-controller (PMC) which manages power rail of a > few hardware units. Perhaps it should be > > device -> PMC domain -> CORE domain > > but not exactly sure for now. I am also confused on if it should be a domain or regulator, but that is for Ulf to tell :) -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 17/30] mmc: sdhci-tegra: Support OPP and core voltage scaling
On 09-11-20, 08:19, Dmitry Osipenko wrote:
> Thanks, I made it in a different way by simply adding helpers to the
> pm_opp.h which use devm_add_action_or_reset(). This doesn't require to
> add new kernel symbols.
I will prefer to add it in core.c itself, and yes
devm_add_action_or_reset() looks better. But I am still not sure for
which helpers do we need the devm_*() variants, as this is only useful
for non-CPU devices. But if we have users that we can add right now,
why not.
> static inline int devm_pm_opp_of_add_table(struct device *dev)
> {
> int err;
>
> err = dev_pm_opp_of_add_table(dev);
> if (err)
> return err;
>
> err = devm_add_action_or_reset(dev, (void*)dev_pm_opp_remove_table,
> dev);
> if (err)
> return err;
>
> return 0;
> }
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 17/30] mmc: sdhci-tegra: Support OPP and core voltage scaling
On 09-11-20, 08:44, Dmitry Osipenko wrote: > 09.11.2020 08:35, Viresh Kumar пишет: > > On 09-11-20, 08:19, Dmitry Osipenko wrote: > >> Thanks, I made it in a different way by simply adding helpers to the > >> pm_opp.h which use devm_add_action_or_reset(). This doesn't require to > >> add new kernel symbols. > > > > I will prefer to add it in core.c itself, and yes > > devm_add_action_or_reset() looks better. But I am still not sure for > > which helpers do we need the devm_*() variants, as this is only useful > > for non-CPU devices. But if we have users that we can add right now, > > why not. > > All current non-CPU drivers (devfreq, mmc, memory, etc) can benefit from it. > > For Tegra drivers we need these variants: > > devm_pm_opp_set_supported_hw() > devm_pm_opp_set_regulators() [if we won't use GENPD] > devm_pm_opp_set_clkname() > devm_pm_opp_of_add_table() I tried to look earlier for the stuff already merged in and didn't find a lot of stuff where the devm_* could be used, maybe I missed some of it. Frank, would you like to refresh your series based on suggestions from Dmitry and make other drivers adapt to the new APIs ? -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 00/48] Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs
On 17-12-20, 21:05, Dmitry Osipenko wrote: > Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs, which reduces > power consumption and heating of the Tegra chips. Tegra SoC has multiple > hardware units which belong to a core power domain of the SoC and share > the core voltage. The voltage must be selected in accordance to a minimum > requirement of every core hardware unit. Please submit the OPP changes separately (alone), it will never get merged this way. Send fixes at the top, any features you want later in the series. It is fine for you to base your series of patches which are under review, you just need to mention that in your cover letter for your platform's patchset. -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 43/48] ARM: tegra: Add OPP tables and power domains to Tegra20 device-tree
On 17-12-20, 21:06, Dmitry Osipenko wrote:
> diff --git a/arch/arm/boot/dts/tegra20-peripherals-opp.dtsi
> b/arch/arm/boot/dts/tegra20-peripherals-opp.dtsi
> index b84afecea154..7e015cdfbc55 100644
> --- a/arch/arm/boot/dts/tegra20-peripherals-opp.dtsi
> +++ b/arch/arm/boot/dts/tegra20-peripherals-opp.dtsi
> @@ -1,6 +1,46 @@
> // SPDX-License-Identifier: GPL-2.0
>
> / {
> + core_opp_table: core-power-domain-opp-table {
> + compatible = "operating-points-v2";
> + opp-shared;
> +
> + core_opp_950: opp@95 {
> + opp-microvolt = <95 95 130>;
> + opp-level = <95>;
> + };
I am not sure I fully understand this, why does it have both microvolt and level
properties ?
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 28/48] soc/tegra: Introduce core power domain driver
On 17-12-20, 21:06, Dmitry Osipenko wrote:
> +++ b/drivers/soc/tegra/core-power-domain.c
> @@ -0,0 +1,125 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * NVIDIA Tegra SoC Core Power Domain Driver
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +static struct lock_class_key tegra_core_domain_lock_class;
> +static bool tegra_core_domain_state_synced;
> +
> +static int tegra_genpd_set_performance_state(struct generic_pm_domain *genpd,
> + unsigned int level)
> +{
> + struct dev_pm_opp *opp;
> + int err;
> +
> + opp = dev_pm_opp_find_level_ceil(&genpd->dev, &level);
We don't need ceil or floor versions for level, but rather _exact() version. Or
maybe just call it dev_pm_opp_find_level().
> + if (IS_ERR(opp)) {
> + dev_err(&genpd->dev, "failed to find OPP for level %u: %pe\n",
> + level, opp);
> + return PTR_ERR(opp);
> + }
> +
> + err = dev_pm_opp_set_voltage(&genpd->dev, opp);
IIUC, you implemented this callback because you want to use the voltage triplet
present in the OPP table ?
And so you are setting the regulator ("power") later in this patch ?
I am not in favor of implementing this routine, as it just adds a wrapper above
the regulator API. What you should be doing rather is get the regulator by
yourself here (instead of depending on the OPP core). And then you can do
dev_pm_opp_get_voltage() here and set the voltage yourself. You may want to
implement a version supporting triplet here though for the same.
And you won't require the sync version of the API as well then.
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 09/48] opp: Add dev_pm_opp_sync_regulators()
On 17-12-20, 21:05, Dmitry Osipenko wrote: > Extend OPP API with dev_pm_opp_sync_regulators() function, which syncs > voltage state of regulators. > > Signed-off-by: Dmitry Osipenko We shouldn't be doing this, details in patch 28. -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 10/48] opp: Add dev_pm_opp_set_voltage()
On 17-12-20, 21:06, Dmitry Osipenko wrote: > Add dev_pm_opp_set_voltage() which allows OPP table users to set voltage > in accordance to a given OPP. In particular this is needed for driving > voltage of a generic power domain which uses OPPs and doesn't have a > clock. > > Signed-off-by: Dmitry Osipenko We shouldn't be doing this, details in patch 28. -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 11/48] opp: Add dev_pm_opp_find_level_ceil()
On 17-12-20, 21:06, Dmitry Osipenko wrote: > Add a ceil version of the dev_pm_opp_find_level(). It's handy to have if > levels don't start from 0 in OPP table and zero usually means a minimal > level. > > Signed-off-by: Dmitry Osipenko Why doesn't the exact version work for you here ? -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 17/30] mmc: sdhci-tegra: Support OPP and core voltage scaling
On Mon, 9 Nov 2020 at 16:51, Frank Lee wrote: > On Mon, Nov 9, 2020 at 1:53 PM Viresh Kumar wrote: > > > devm_pm_opp_set_supported_hw() > > > devm_pm_opp_set_regulators() [if we won't use GENPD] > > > devm_pm_opp_set_clkname() > > > devm_pm_opp_of_add_table() > > > > I tried to look earlier for the stuff already merged in and didn't > > find a lot of stuff where the devm_* could be used, maybe I missed > > some of it. > > > > Frank, would you like to refresh your series based on suggestions from > > Dmitry and make other drivers adapt to the new APIs ? > > I am glad to do this.：） Frank, Dmitry has submitted a series with a patch that does stuff like this since you never resent your patches. http://lore.kernel.org/lkml/20201217180638.22748-14-dig...@gmail.com Since you were the first one to get to this, I would still like to give you a chance to get these patches merged under your authorship, otherwise I would be going to pick patches from Dmitry. -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 13/48] opp: Add resource-managed versions of OPP API functions
On 17-12-20, 21:06, Dmitry Osipenko wrote: > Add resource-managed versions of OPP API functions. This removes a need > from drivers to store and manage OPP table pointers. > > Signed-off-by: Dmitry Osipenko > --- > drivers/opp/core.c | 173 + > drivers/opp/of.c | 25 ++ > include/linux/pm_opp.h | 51 > 3 files changed, 249 insertions(+) Please send a patchset of its own for this patch, along with updates to all the existing code that can make use of these. -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 14/48] opp: Filter out OPPs based on availability of a required-OPP
On 17-12-20, 21:06, Dmitry Osipenko wrote:
> A required OPP may not be available, and thus, all OPPs which are using
> this required OPP should be unavailable too.
>
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/opp/core.c | 11 ++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
Please send a separate patchset for fixes, as these can also go to 5.11 itself.
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index d9feb7639598..3d02fe33630b 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1588,7 +1588,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp
> *new_opp,
>struct opp_table *opp_table, bool rate_not_available)
> {
> struct list_head *head;
> - int ret;
> + int i, ret;
>
> mutex_lock(&opp_table->lock);
> head = &opp_table->opp_list;
> @@ -1615,6 +1615,15 @@ int _opp_add(struct device *dev, struct dev_pm_opp
> *new_opp,
>__func__, new_opp->rate);
> }
>
> + for (i = 0; i < opp_table->required_opp_count && new_opp->available;
> i++) {
> + if (new_opp->required_opps[i]->available)
> + continue;
> +
> + new_opp->available = false;
> + dev_warn(dev, "%s: OPP not supported by required OPP %pOF
> (%lu)\n",
> + __func__, new_opp->required_opps[i]->np,
> new_opp->rate);
Why not just break from here ?
> + }
> +
> return 0;
> }
>
> --
> 2.29.2
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 15/48] opp: Support set_opp() customization without requiring to use regulators
On 17-12-20, 21:06, Dmitry Osipenko wrote:
> Support set_opp() customization without requiring to use regulators. This
> is needed by drivers which want to use dev_pm_opp_set_rate() for changing
> rates of a multiple clocks and don't need to touch regulator.
>
> One example is NVIDIA Tegra30/114 SoCs which have two sibling 3D hardware
> units which should be use to the same clock rate, meanwhile voltage
> scaling is done using a power domain. In this case OPP table doesn't have
> a regulator, causing a NULL dereference in _set_opp_custom().
>
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/opp/core.c | 16
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 3d02fe33630b..625dae7a5ecb 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -828,17 +828,25 @@ static int _set_opp_custom(const struct opp_table
> *opp_table,
> struct dev_pm_opp_supply *old_supply,
> struct dev_pm_opp_supply *new_supply)
> {
> - struct dev_pm_set_opp_data *data;
> + struct dev_pm_set_opp_data *data, tmp_data;
> + unsigned int regulator_count;
> int size;
>
> - data = opp_table->set_opp_data;
> + if (opp_table->set_opp_data) {
> + data = opp_table->set_opp_data;
> + regulator_count = opp_table->regulator_count;
> + } else {
> + data = &tmp_data;
> + regulator_count = 0;
> + }
> +
> data->regulators = opp_table->regulators;
> - data->regulator_count = opp_table->regulator_count;
> + data->regulator_count = regulator_count;
> data->clk = opp_table->clk;
> data->dev = dev;
>
> data->old_opp.rate = old_freq;
> - size = sizeof(*old_supply) * opp_table->regulator_count;
> + size = sizeof(*old_supply) * regulator_count;
> if (!old_supply)
> memset(data->old_opp.supplies, 0, size);
> else
I don't see you making use of this in this patchset. How did you get this to
crash ?
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 19/48] opp: Fix adding OPP entries in a wrong order if rate is unavailable
On 17-12-20, 21:06, Dmitry Osipenko wrote:
> Fix adding OPP entries in a wrong (opposite) order if OPP rate is
> unavailable. The OPP comparison is erroneously skipped if OPP rate is
> missing, thus OPPs are left unsorted.
>
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/opp/core.c | 23 ---
> drivers/opp/opp.h | 2 +-
> 2 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 34f7e530d941..5c7f130a8de2 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1531,9 +1531,10 @@ static bool _opp_supported_by_regulators(struct
> dev_pm_opp *opp,
> return true;
> }
>
> -int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2)
> +int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2,
> + bool rate_not_available)
> {
> - if (opp1->rate != opp2->rate)
> + if (!rate_not_available && opp1->rate != opp2->rate)
rate will be 0 for both the OPPs here if rate_not_available is true and so this
change shouldn't be required.
> return opp1->rate < opp2->rate ? -1 : 1;
> if (opp1->bandwidth && opp2->bandwidth &&
> opp1->bandwidth[0].peak != opp2->bandwidth[0].peak)
> @@ -1545,7 +1546,8 @@ int _opp_compare_key(struct dev_pm_opp *opp1, struct
> dev_pm_opp *opp2)
>
> static int _opp_is_duplicate(struct device *dev, struct dev_pm_opp *new_opp,
>struct opp_table *opp_table,
> - struct list_head **head)
> + struct list_head **head,
> + bool rate_not_available)
> {
> struct dev_pm_opp *opp;
> int opp_cmp;
> @@ -1559,13 +1561,13 @@ static int _opp_is_duplicate(struct device *dev,
> struct dev_pm_opp *new_opp,
>* loop.
>*/
> list_for_each_entry(opp, &opp_table->opp_list, node) {
> - opp_cmp = _opp_compare_key(new_opp, opp);
> + opp_cmp = _opp_compare_key(new_opp, opp, rate_not_available);
> if (opp_cmp > 0) {
> *head = &opp->node;
> continue;
> }
>
> - if (opp_cmp < 0)
> + if (opp_cmp < 0 || rate_not_available)
> return 0;
This shouldn't be required as well, isn't it ?
>
> /* Duplicate OPPs */
> @@ -1601,12 +1603,11 @@ int _opp_add(struct device *dev, struct dev_pm_opp
> *new_opp,
> mutex_lock(&opp_table->lock);
> head = &opp_table->opp_list;
>
> - if (likely(!rate_not_available)) {
> - ret = _opp_is_duplicate(dev, new_opp, opp_table, &head);
> - if (ret) {
> - mutex_unlock(&opp_table->lock);
> - return ret;
> - }
> + ret = _opp_is_duplicate(dev, new_opp, opp_table, &head,
> + rate_not_available);
This is the only thing we need to do here I believe.
> + if (ret) {
> + mutex_unlock(&opp_table->lock);
> + return ret;
> }
>
> list_add(&new_opp->node, head);
> diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> index 4ced7ffa8158..6f5be6c72f13 100644
> --- a/drivers/opp/opp.h
> +++ b/drivers/opp/opp.h
> @@ -219,7 +219,7 @@ struct opp_table *_find_opp_table(struct device *dev);
> struct opp_device *_add_opp_dev(const struct device *dev, struct opp_table
> *opp_table);
> struct dev_pm_opp *_opp_allocate(struct opp_table *opp_table);
> void _opp_free(struct dev_pm_opp *opp);
> -int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2);
> +int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2, bool
> rate_not_available);
> int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct
> opp_table *opp_table, bool rate_not_available);
> int _opp_add_v1(struct opp_table *opp_table, struct device *dev, unsigned
> long freq, long u_volt, bool dynamic);
> void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, int
> last_cpu);
> --
> 2.29.2
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 44/48] ARM: tegra: Add OPP tables and power domains to Tegra30 device-tree
On 17-12-20, 21:06, Dmitry Osipenko wrote:
> diff --git a/arch/arm/boot/dts/tegra30-peripherals-opp.dtsi
> b/arch/arm/boot/dts/tegra30-peripherals-opp.dtsi
> index cbe84d25e726..983db1a06682 100644
> --- a/arch/arm/boot/dts/tegra30-peripherals-opp.dtsi
> +++ b/arch/arm/boot/dts/tegra30-peripherals-opp.dtsi
> @@ -1,6 +1,56 @@
> // SPDX-License-Identifier: GPL-2.0
>
> / {
> + core_opp_table: core-power-domain-opp-table {
> + compatible = "operating-points-v2";
> + opp-shared;
> +
> + core_opp_950: opp@95 {
> + opp-microvolt = <95 95 135>;
> + opp-level = <95>;
Perhaps you don't need to exactly copy the voltage value into the level field.
The level field can just be kept to 0, 1,2, 3, etc..
> + };
> +
> + core_opp_1000: opp@100 {
> + opp-microvolt = <100 100 135>;
> + opp-level = <100>;
> + };
> +
> + core_opp_1050: opp@105 {
> + opp-microvolt = <105 105 135>;
> + opp-level = <105>;
> + };
> +
> + core_opp_1100: opp@110 {
> + opp-microvolt = <110 110 135>;
> + opp-level = <110>;
> + };
> +
> + core_opp_1150: opp@115 {
> + opp-microvolt = <115 115 135>;
> + opp-level = <115>;
> + };
> +
> + core_opp_1200: opp@120 {
> + opp-microvolt = <120 120 135>;
> + opp-level = <120>;
> + };
> +
> + core_opp_1250: opp@125 {
> + opp-microvolt = <125 125 135>;
> + opp-level = <125>;
> + };
> +
> + core_opp_1300: opp@130 {
> + opp-microvolt = <130 130 135>;
> + opp-level = <130>;
> + };
> +
> + core_opp_1350: opp@135 {
> + opp-microvolt = <135 135 135>;
> + opp-level = <135>;
> + };
> + };
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 00/48] Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs
On 18-12-20, 16:51, Dmitry Osipenko wrote: > Alright, although I haven't pretended that v2 patches should be merged > right away since they are fundamentally different from v1, and thus, all > patches need to be reviewed first. I agree. I have done some basic review for the stuff. > If the current OPP changes look good to you, then please give yours r-b > to the patches. Thanks in advance! r-b-y isn't required as they will go through my tree itself. So if everyone is happy with the idea, please submit the patches separately (fixes, improvements, devm_*, etc). -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 11/48] opp: Add dev_pm_opp_find_level_ceil()
On 22-12-20, 22:15, Dmitry Osipenko wrote: > 22.12.2020 09:42, Viresh Kumar пишет: > > On 17-12-20, 21:06, Dmitry Osipenko wrote: > >> Add a ceil version of the dev_pm_opp_find_level(). It's handy to have if > >> levels don't start from 0 in OPP table and zero usually means a minimal > >> level. > >> > >> Signed-off-by: Dmitry Osipenko > > > > Why doesn't the exact version work for you here ? > > > > The exact version won't find OPP for level=0 if levels don't start with > 0, where 0 means that minimal level is desired. Right, but why do you need to send 0 for your platform ? -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 14/48] opp: Filter out OPPs based on availability of a required-OPP
On 22-12-20, 22:17, Dmitry Osipenko wrote:
> 22.12.2020 11:59, Viresh Kumar пишет:
> > On 17-12-20, 21:06, Dmitry Osipenko wrote:
> >> A required OPP may not be available, and thus, all OPPs which are using
> >> this required OPP should be unavailable too.
> >>
> >> Signed-off-by: Dmitry Osipenko
> >> ---
> >> drivers/opp/core.c | 11 ++-
> >> 1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > Please send a separate patchset for fixes, as these can also go to 5.11
> > itself.
>
> Alright, although I don't think that this patch fixes any problems for
> existing OPP users.
Because nobody is using this feature, but otherwise this is a fix for me.
> >> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> >> index d9feb7639598..3d02fe33630b 100644
> >> --- a/drivers/opp/core.c
> >> +++ b/drivers/opp/core.c
> >> @@ -1588,7 +1588,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp
> >> *new_opp,
> >> struct opp_table *opp_table, bool rate_not_available)
> >> {
> >>struct list_head *head;
> >> - int ret;
> >> + int i, ret;
> >>
> >>mutex_lock(&opp_table->lock);
> >>head = &opp_table->opp_list;
> >> @@ -1615,6 +1615,15 @@ int _opp_add(struct device *dev, struct dev_pm_opp
> >> *new_opp,
> >> __func__, new_opp->rate);
> >>}
> >>
> >> + for (i = 0; i < opp_table->required_opp_count && new_opp->available;
> >> i++) {
> >> + if (new_opp->required_opps[i]->available)
> >> + continue;
> >> +
> >> + new_opp->available = false;
> >> + dev_warn(dev, "%s: OPP not supported by required OPP %pOF
> >> (%lu)\n",
> >> + __func__, new_opp->required_opps[i]->np,
> >> new_opp->rate);
> >
> > Why not just break from here ?
>
> The new_opp could be already marked as unavailable by a previous voltage
> check, hence this loop should be skipped entirely in that case.
Then add a separate check for that before the loop as we don't need that check
on every iteration here.
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 19/48] opp: Fix adding OPP entries in a wrong order if rate is unavailable
On 22-12-20, 22:19, Dmitry Osipenko wrote:
> 22.12.2020 12:12, Viresh Kumar пишет:
> > On 17-12-20, 21:06, Dmitry Osipenko wrote:
> >> Fix adding OPP entries in a wrong (opposite) order if OPP rate is
> >> unavailable. The OPP comparison is erroneously skipped if OPP rate is
> >> missing, thus OPPs are left unsorted.
> >>
> >> Signed-off-by: Dmitry Osipenko
> >> ---
> >> drivers/opp/core.c | 23 ---
> >> drivers/opp/opp.h | 2 +-
> >> 2 files changed, 13 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> >> index 34f7e530d941..5c7f130a8de2 100644
> >> --- a/drivers/opp/core.c
> >> +++ b/drivers/opp/core.c
> >> @@ -1531,9 +1531,10 @@ static bool _opp_supported_by_regulators(struct
> >> dev_pm_opp *opp,
> >>return true;
> >> }
> >>
> >> -int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2)
> >> +int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2,
> >> + bool rate_not_available)
> >> {
> >> - if (opp1->rate != opp2->rate)
> >> + if (!rate_not_available && opp1->rate != opp2->rate)
> >
> > rate will be 0 for both the OPPs here if rate_not_available is true and so
> > this
> > change shouldn't be required.
>
> The rate_not_available is negated in the condition. This change is
> required because both rates are 0 and then we should proceed to the
> levels comparison.
Won't that happen without this patch ?
> I guess it's not clear by looking at this patch, please see a full
> version of the function:
>
> int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2,
> bool rate_not_available)
> {
> if (!rate_not_available && opp1->rate != opp2->rate)
> return opp1->rate < opp2->rate ? -1 : 1;
> if (opp1->bandwidth && opp2->bandwidth &&
> opp1->bandwidth[0].peak != opp2->bandwidth[0].peak)
> return opp1->bandwidth[0].peak < opp2->bandwidth[0].peak ? -1 : 1;
> if (opp1->level != opp2->level)
> return opp1->level < opp2->level ? -1 : 1;
> return 0;
> }
>
> Perhaps we could check whether opp1->rate=0, like it's done for the
> opp1->bandwidth. I'll consider this variant for v3, thanks.
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 28/48] soc/tegra: Introduce core power domain driver
On 22-12-20, 22:39, Dmitry Osipenko wrote:
> 22.12.2020 22:21, Dmitry Osipenko пишет:
> >>> + if (IS_ERR(opp)) {
> >>> + dev_err(&genpd->dev, "failed to find OPP for level %u: %pe\n",
> >>> + level, opp);
> >>> + return PTR_ERR(opp);
> >>> + }
> >>> +
> >>> + err = dev_pm_opp_set_voltage(&genpd->dev, opp);
> >> IIUC, you implemented this callback because you want to use the voltage
> >> triplet
> >> present in the OPP table ?
> >>
> >> And so you are setting the regulator ("power") later in this patch ?
> > yes
> >
> >> I am not in favor of implementing this routine, as it just adds a wrapper
> >> above
> >> the regulator API. What you should be doing rather is get the regulator by
> >> yourself here (instead of depending on the OPP core). And then you can do
> >> dev_pm_opp_get_voltage() here and set the voltage yourself. You may want to
> >> implement a version supporting triplet here though for the same.
> >>
> >> And you won't require the sync version of the API as well then.
> >>
> > That's what I initially did for this driver. I don't mind to revert back
> > to the initial variant in v3, it appeared to me that it will be nicer
> > and cleaner to have OPP API managing everything here.
>
> I forgot one important detail (why the initial variant wasn't good)..
> OPP entries that have unsupportable voltages should be filtered out and
> OPP core performs the filtering only if regulator is assigned to the OPP
> table.
>
> If regulator is assigned to the OPP table, then we need to use OPP API
> for driving the regulator, hence that's why I added
> dev_pm_opp_sync_regulators() and dev_pm_opp_set_voltage().
>
> Perhaps it should be possible to add dev_pm_opp_get_regulator() that
What's wrong with getting the regulator in the driver as well ? Apart from the
OPP core ?
> will return the OPP table regulator in order to allow driver to use the
> regulator directly. But I'm not sure whether this is a much better
> option than the opp_sync_regulators() and opp_set_voltage() APIs.
set_voltage() is still fine as there is some data that the OPP core has, but
sync_regulator() has nothing to do with OPP core.
And this may lead to more wrapper helpers in the OPP core, which I am afraid of.
And so even if it is not the best, I would like the OPP core to provide the data
and not get into this. Ofcourse there is an exception to this, opp_set_rate.
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 15/48] opp: Support set_opp() customization without requiring to use regulators
On 17-12-20, 21:06, Dmitry Osipenko wrote:
> Support set_opp() customization without requiring to use regulators. This
> is needed by drivers which want to use dev_pm_opp_set_rate() for changing
> rates of a multiple clocks and don't need to touch regulator.
>
> One example is NVIDIA Tegra30/114 SoCs which have two sibling 3D hardware
> units which should be use to the same clock rate, meanwhile voltage
> scaling is done using a power domain. In this case OPP table doesn't have
> a regulator, causing a NULL dereference in _set_opp_custom().
>
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/opp/core.c | 16
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 3d02fe33630b..625dae7a5ecb 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -828,17 +828,25 @@ static int _set_opp_custom(const struct opp_table
> *opp_table,
> struct dev_pm_opp_supply *old_supply,
> struct dev_pm_opp_supply *new_supply)
> {
> - struct dev_pm_set_opp_data *data;
> + struct dev_pm_set_opp_data *data, tmp_data;
> + unsigned int regulator_count;
> int size;
>
> - data = opp_table->set_opp_data;
> + if (opp_table->set_opp_data) {
> + data = opp_table->set_opp_data;
> + regulator_count = opp_table->regulator_count;
> + } else {
> + data = &tmp_data;
> + regulator_count = 0;
> + }
> +
We should use the same structure, you can add some checks but not replace the
structure altogether.
> data->regulators = opp_table->regulators;
> - data->regulator_count = opp_table->regulator_count;
> + data->regulator_count = regulator_count;
> data->clk = opp_table->clk;
> data->dev = dev;
>
> data->old_opp.rate = old_freq;
> - size = sizeof(*old_supply) * opp_table->regulator_count;
> + size = sizeof(*old_supply) * regulator_count;
> if (!old_supply)
> memset(data->old_opp.supplies, 0, size);
> else
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 15/48] opp: Support set_opp() customization without requiring to use regulators
On 23-12-20, 23:38, Dmitry Osipenko wrote: > Well, there is no "same structure", the opp_table->set_opp_data is NULL > there. Right, I saw that yesterday. What I meant was that we need to start allocating the structure for this case now. -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 19/48] opp: Fix adding OPP entries in a wrong order if rate is unavailable
On 23-12-20, 23:36, Dmitry Osipenko wrote:
> 23.12.2020 07:34, Viresh Kumar пишет:
> > On 22-12-20, 22:19, Dmitry Osipenko wrote:
> >> 22.12.2020 12:12, Viresh Kumar пишет:
> >>> rate will be 0 for both the OPPs here if rate_not_available is true and
> >>> so this
> >>> change shouldn't be required.
> >>
> >> The rate_not_available is negated in the condition. This change is
> >> required because both rates are 0 and then we should proceed to the
> >> levels comparison.
> >
> > Won't that happen without this patch ?
>
> No
This is how the code looks like currently:
int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2)
{
if (opp1->rate != opp2->rate)
return opp1->rate < opp2->rate ? -1 : 1;
if (opp1->bandwidth && opp2->bandwidth &&
opp1->bandwidth[0].peak != opp2->bandwidth[0].peak)
return opp1->bandwidth[0].peak < opp2->bandwidth[0].peak ? -1 :
1;
if (opp1->level != opp2->level)
return opp1->level < opp2->level ? -1 : 1;
return 0;
}
Lets consider the case you are focussing on, where rate is 0 for both the OPPs,
bandwidth isn't there and we want to run the level comparison here.
Since both the rates are 0, (opp1->rate != opp2->rate) will fail and so we will
move to bandwidth check which will fail too. And so we will get to the level
comparison.
What am I missing here ? I am sure there is something for sure as you won't have
missed this..
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 11/48] opp: Add dev_pm_opp_find_level_ceil()
On 23-12-20, 23:37, Dmitry Osipenko wrote: > 23.12.2020 07:19, Viresh Kumar пишет: > > On 22-12-20, 22:15, Dmitry Osipenko wrote: > >> 22.12.2020 09:42, Viresh Kumar пишет: > >>> On 17-12-20, 21:06, Dmitry Osipenko wrote: > >>>> Add a ceil version of the dev_pm_opp_find_level(). It's handy to have if > >>>> levels don't start from 0 in OPP table and zero usually means a minimal > >>>> level. > >>>> > >>>> Signed-off-by: Dmitry Osipenko > >>> > >>> Why doesn't the exact version work for you here ? > >>> > >> > >> The exact version won't find OPP for level=0 if levels don't start with > >> 0, where 0 means that minimal level is desired. > > > > Right, but why do you need to send 0 for your platform ? > > > > To put power domain into the lowest performance state when device is idling. I see. So you really want to set it to the lowest state or just take the vote out ? Which may end up powering off the domain in the worst case ? -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 28/48] soc/tegra: Introduce core power domain driver
On 23-12-20, 23:37, Dmitry Osipenko wrote: > 23.12.2020 08:57, Viresh Kumar пишет: > > What's wrong with getting the regulator in the driver as well ? Apart from > > the > > OPP core ? > > The voltage syncing should be done for each consumer regulator > individually [1]. > > Secondly, regulator core doesn't work well today if the same regulator > is requested more than one time for the same device. Hmm... > >> will return the OPP table regulator in order to allow driver to use the > >> regulator directly. But I'm not sure whether this is a much better > >> option than the opp_sync_regulators() and opp_set_voltage() APIs. > > > > set_voltage() is still fine as there is some data that the OPP core has, but > > sync_regulator() has nothing to do with OPP core. > > > > And this may lead to more wrapper helpers in the OPP core, which I am > > afraid of. > > And so even if it is not the best, I would like the OPP core to provide the > > data > > and not get into this. Ofcourse there is an exception to this, opp_set_rate. > > > > The regulator_sync_voltage() should be invoked only if voltage was > changed previously [1]. > > The OPP core already has the info about whether voltage was changed and > it provides the necessary locking for both set_voltage() and > sync_regulator(). Perhaps I'll need to duplicate that functionality in > the PD driver, instead of making it all generic and re-usable by other > drivers. > > [1] > https://elixir.bootlin.com/linux/v5.10.2/source/drivers/regulator/core.c#L4107 Lets do it in the OPP core and see where we go. -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 11/48] opp: Add dev_pm_opp_find_level_ceil()
On 24-12-20, 16:00, Dmitry Osipenko wrote: > In a device driver I want to set PD to the lowest performance state by > removing the performance vote when dev_pm_opp_set_rate(dev, 0) is > invoked by the driver. > > The OPP core already does this, but if OPP levels don't start from 0 in > a device-tree for PD, then it currently doesn't work since there is a > need to get a rounded-up performance state because > dev_pm_opp_set_voltage() takes OPP entry for the argument (patches 9 and > 28). > > The PD powering off and performance-changes are separate from each other > in the GENPD core. The GENPD core automatically turns off domain when > all devices within the domain are suspended by system-suspend or RPM. > > The performance state of a power domain is controlled solely by a device > driver. GENPD core only aggregates the performance requests, it doesn't > change the performance state of a domain by itself when device is > suspended or resumed, IIUC this is intentional. And I want to put domain > into lowest performance state when device is suspended. Right, so if you really want to just drop the performance vote, then with a value of 0 for the performance state the call will reach to your genpd's callback ->set_performance_state(). Just as dev_pm_opp_set_rate() accepts the frequency to be 0, I would expect dev_pm_opp_set_rate() to accept opp argument as NULL and in that case set voltage to 0 and do regulator_disable() as well. Won't that work better than going for the lowest voltage ? -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 11/48] opp: Add dev_pm_opp_find_level_ceil()
On 28-12-20, 17:03, Dmitry Osipenko wrote: > 28.12.2020 09:22, Viresh Kumar пишет: > > On 24-12-20, 16:00, Dmitry Osipenko wrote: > >> In a device driver I want to set PD to the lowest performance state by > >> removing the performance vote when dev_pm_opp_set_rate(dev, 0) is > >> invoked by the driver. > >> > >> The OPP core already does this, but if OPP levels don't start from 0 in > >> a device-tree for PD, then it currently doesn't work since there is a > >> need to get a rounded-up performance state because > >> dev_pm_opp_set_voltage() takes OPP entry for the argument (patches 9 and > >> 28). > >> > >> The PD powering off and performance-changes are separate from each other > >> in the GENPD core. The GENPD core automatically turns off domain when > >> all devices within the domain are suspended by system-suspend or RPM. > >> > >> The performance state of a power domain is controlled solely by a device > >> driver. GENPD core only aggregates the performance requests, it doesn't > >> change the performance state of a domain by itself when device is > >> suspended or resumed, IIUC this is intentional. And I want to put domain > >> into lowest performance state when device is suspended. > > > > Right, so if you really want to just drop the performance vote, then with a > > value of 0 for the performance state the call will reach to your genpd's > > callback ->set_performance_state(). Just as dev_pm_opp_set_rate() accepts > > the > > frequency to be 0, I would expect dev_pm_opp_set_rate() to accept opp > > argument > > as NULL and in that case set voltage to 0 and do regulator_disable() as > > well. > > Won't that work better than going for the lowest voltage ? > > > > We can make dev_pm_opp_set_voltage() to accept OPP=NULL in order to > disable the regulator, like it's done for dev_pm_opp_set_rate(dev, 0). > Although, I don't need this kind of behaviour for the Tegra PD driver, > and thus, would prefer to leave this for somebody else to implement in > the future, once it will be really needed. > > Still we need the dev_pm_opp_find_level_ceil() because level=0 means > that we want to set PD to the lowest (minimal) performance state, i.e. > it doesn't necessarily mean that we want to set the voltage to 0 and > disable the PD entirely. GENPD has a separate controls for on/off. Ok. -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH] greybus: audio_manager: fix a missing check of ida_simple_get
On 14-03-19, 01:45, Kangjie Lu wrote:
> ida_simple_get could fail. The fix inserts a check for its
> return value.
>
> Signed-off-by: Kangjie Lu
> ---
> drivers/staging/greybus/audio_manager.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/staging/greybus/audio_manager.c
> b/drivers/staging/greybus/audio_manager.c
> index d44b070d8862..c2a4af4c1d06 100644
> --- a/drivers/staging/greybus/audio_manager.c
> +++ b/drivers/staging/greybus/audio_manager.c
> @@ -45,6 +45,9 @@ int gb_audio_manager_add(struct
> gb_audio_manager_module_descriptor *desc)
> int err;
>
> id = ida_simple_get(&module_id, 0, 0, GFP_KERNEL);
> + if (id < 0)
> + return id;
> +
> err = gb_audio_manager_module_create(&module, manager_kset,
> id, desc);
> if (err) {
Acked-by: Viresh Kumar
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH 3/9] staging: greybus: hd: Fix up some alignment checkpatch issues
On 25-08-19, 07:54, Greg Kroah-Hartman wrote: > Some function prototypes do not match the expected alignment formatting > so fix that up so that checkpatch is happy. > > Cc: Johan Hovold > Cc: Alex Elder > Cc: greybus-...@lists.linaro.org > Cc: de...@driverdev.osuosl.org > Signed-off-by: Greg Kroah-Hartman > --- > drivers/staging/greybus/hd.c | 10 +- > 1 file changed, 5 insertions(+), 5 deletions(-) Acked-by: Viresh Kumar -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/9] staging: greybus: fix up SPDX comment in .h files
On 25-08-19, 07:54, Greg Kroah-Hartman wrote: > When these files originally got an SPDX tag, I used // instead of /* */ > for the .h files. Fix this up to use // properly. > > Cc: Viresh Kumar > Cc: Johan Hovold > Cc: Alex Elder > Cc: greybus-...@lists.linaro.org > Cc: de...@driverdev.osuosl.org > Signed-off-by: Greg Kroah-Hartman > --- > drivers/staging/greybus/firmware.h | 2 +- > drivers/staging/greybus/gb-camera.h | 2 +- > drivers/staging/greybus/gbphy.h | 2 +- > drivers/staging/greybus/greybus.h| 2 +- > drivers/staging/greybus/greybus_authentication.h | 2 +- > drivers/staging/greybus/greybus_firmware.h | 2 +- > drivers/staging/greybus/greybus_manifest.h | 2 +- > drivers/staging/greybus/greybus_protocols.h | 2 +- > drivers/staging/greybus/greybus_trace.h | 2 +- > drivers/staging/greybus/hd.h | 2 +- > drivers/staging/greybus/interface.h | 2 +- > drivers/staging/greybus/manifest.h | 2 +- > drivers/staging/greybus/module.h | 2 +- > drivers/staging/greybus/operation.h | 2 +- > drivers/staging/greybus/spilib.h | 2 +- > drivers/staging/greybus/svc.h | 2 +- > 16 files changed, 16 insertions(+), 16 deletions(-) Acked-by: Viresh Kumar -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/9] staging: greybus: remove license "boilerplate"
On 25-08-19, 07:54, Greg Kroah-Hartman wrote: > When the greybus drivers were converted to SPDX identifiers for the > license text, some license boilerplate was not removed. Clean this up > by removing this unneeded text now. > > Cc: Johan Hovold > Cc: Alex Elder > Cc: Vaibhav Agarwal > Cc: Mark Greer > Cc: Viresh Kumar > Cc: "Bryan O'Donoghue" > Cc: greybus-...@lists.linaro.org > Cc: de...@driverdev.osuosl.org > Signed-off-by: Greg Kroah-Hartman > --- > .../Documentation/firmware/authenticate.c | 46 --- > .../greybus/Documentation/firmware/firmware.c | 46 --- > drivers/staging/greybus/arpc.h| 46 --- > drivers/staging/greybus/audio_apbridgea.h | 26 +-- > .../staging/greybus/greybus_authentication.h | 46 --- > drivers/staging/greybus/greybus_firmware.h| 46 --- > drivers/staging/greybus/greybus_protocols.h | 46 --- > drivers/staging/greybus/tools/loopback_test.c | 2 - > 8 files changed, 1 insertion(+), 303 deletions(-) Acked-by: Viresh Kumar -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 7/9] staging: greybus: move core include files to include/linux/greybus/
On 25-08-19, 07:54, Greg Kroah-Hartman wrote: > With the goal of moving the core of the greybus code out of staging, the > include files need to be moved to include/linux/greybus.h and > include/linux/greybus/ > > Cc: Vaibhav Hiremath > Cc: Johan Hovold > Cc: Alex Elder > Cc: Vaibhav Agarwal > Cc: Mark Greer > Cc: Viresh Kumar > Cc: Rui Miguel Silva > Cc: David Lin > Cc: "Bryan O'Donoghue" > Cc: greybus-...@lists.linaro.org > Cc: de...@driverdev.osuosl.org > Signed-off-by: Greg Kroah-Hartman Acked-by: Viresh Kumar -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH 6/9] staging: greybus: loopback: Fix up some alignment checkpatch issues
On 25-08-19, 07:54, Greg Kroah-Hartman wrote:
> Some function prototypes do not match the expected alignment formatting
> so fix that up so that checkpatch is happy.
>
> Cc: "Bryan O'Donoghue"
> Cc: Johan Hovold
> Cc: Alex Elder
> Cc: greybus-...@lists.linaro.org
> Cc: de...@driverdev.osuosl.org
> Signed-off-by: Greg Kroah-Hartman
> ---
> drivers/staging/greybus/loopback.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/greybus/loopback.c
> b/drivers/staging/greybus/loopback.c
> index 48d85ebe404a..b0ab0eed5c18 100644
> --- a/drivers/staging/greybus/loopback.c
> +++ b/drivers/staging/greybus/loopback.c
> @@ -882,7 +882,7 @@ static int gb_loopback_fn(void *data)
> gb->type = 0;
> gb->send_count = 0;
> sysfs_notify(&gb->dev->kobj, NULL,
> - "iteration_count");
> + "iteration_count");
> dev_dbg(&bundle->dev, "load test complete\n");
> } else {
> dev_dbg(&bundle->dev,
> @@ -1054,7 +1054,7 @@ static int gb_loopback_probe(struct gb_bundle *bundle,
>
> /* Allocate kfifo */
> if (kfifo_alloc(&gb->kfifo_lat, kfifo_depth * sizeof(u32),
> - GFP_KERNEL)) {
> + GFP_KERNEL)) {
> retval = -ENOMEM;
> goto out_conn;
> }
Acked-by: Viresh Kumar
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH 5/9] staging: greybus: log: Fix up some alignment checkpatch issues
On 25-08-19, 07:54, Greg Kroah-Hartman wrote:
> Some function prototypes do not match the expected alignment formatting
> so fix that up so that checkpatch is happy.
>
> Cc: David Lin
> Cc: Johan Hovold
> Cc: Alex Elder
> Cc: greybus-...@lists.linaro.org
> Cc: de...@driverdev.osuosl.org
> Signed-off-by: Greg Kroah-Hartman
> ---
> drivers/staging/greybus/log.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/greybus/log.c b/drivers/staging/greybus/log.c
> index 15a88574dbb0..4f1f161ff11c 100644
> --- a/drivers/staging/greybus/log.c
> +++ b/drivers/staging/greybus/log.c
> @@ -31,14 +31,14 @@ static int gb_log_request_handler(struct gb_operation *op)
> /* Verify size of payload */
> if (op->request->payload_size < sizeof(*receive)) {
> dev_err(dev, "log request too small (%zu < %zu)\n",
> - op->request->payload_size, sizeof(*receive));
> + op->request->payload_size, sizeof(*receive));
> return -EINVAL;
> }
> receive = op->request->payload;
> len = le16_to_cpu(receive->len);
> if (len != (op->request->payload_size - sizeof(*receive))) {
> dev_err(dev, "log request wrong size %d vs %zu\n", len,
> - (op->request->payload_size - sizeof(*receive)));
> + (op->request->payload_size - sizeof(*receive)));
> return -EINVAL;
> }
> if (len == 0) {
> @@ -83,7 +83,7 @@ static int gb_log_probe(struct gb_bundle *bundle,
> return -ENOMEM;
>
> connection = gb_connection_create(bundle, le16_to_cpu(cport_desc->id),
> - gb_log_request_handler);
> + gb_log_request_handler);
> if (IS_ERR(connection)) {
> retval = PTR_ERR(connection);
> goto error_free;
Acked-by: Viresh Kumar
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH 4/9] staging: greybus: manifest: Fix up some alignment checkpatch issues
On 25-08-19, 07:54, Greg Kroah-Hartman wrote:
> Some function prototypes do not match the expected alignment formatting
> so fix that up so that checkpatch is happy.
>
> Cc: Johan Hovold
> Cc: Alex Elder
> Cc: greybus-...@lists.linaro.org
> Cc: de...@driverdev.osuosl.org
> Signed-off-by: Greg Kroah-Hartman
> ---
> drivers/staging/greybus/manifest.c | 39 +++---
> 1 file changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/staging/greybus/manifest.c
> b/drivers/staging/greybus/manifest.c
> index 08db49264f2b..4ebbba52b07c 100644
> --- a/drivers/staging/greybus/manifest.c
> +++ b/drivers/staging/greybus/manifest.c
> @@ -104,15 +104,15 @@ static int identify_descriptor(struct gb_interface
> *intf,
> size_t expected_size;
>
> if (size < sizeof(*desc_header)) {
> - dev_err(&intf->dev, "manifest too small (%zu < %zu)\n",
> - size, sizeof(*desc_header));
> + dev_err(&intf->dev, "manifest too small (%zu < %zu)\n", size,
> + sizeof(*desc_header));
> return -EINVAL; /* Must at least have header */
> }
Acked-by: Viresh Kumar
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH 9/9] staging: greybus: move es2 to drivers/greybus/
On 25-08-19, 07:54, Greg Kroah-Hartman wrote:
> The es2 Greybus host controller has long been stable, so move it out of
> drivers/staging/ to drivers/greybus/
>
> Cc: Johan Hovold
> Cc: Alex Elder
> Cc: greybus-...@lists.linaro.org
> Cc: linux-ker...@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman
> ---
> drivers/greybus/Kconfig | 16
> drivers/greybus/Makefile | 7 +++
> drivers/{staging => }/greybus/arpc.h | 0
> drivers/{staging => }/greybus/es2.c | 2 +-
> drivers/staging/greybus/Kconfig | 11 ---
> drivers/staging/greybus/Makefile | 5 -
> 6 files changed, 24 insertions(+), 17 deletions(-)
> rename drivers/{staging => }/greybus/arpc.h (100%)
> rename drivers/{staging => }/greybus/es2.c (99%)
Acked-by: Viresh Kumar
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH 8/9] staging: greybus: move the greybus core to drivers/greybus
On 25-08-19, 07:54, Greg Kroah-Hartman wrote: > The Greybus core code has been stable for a long time, and has been > shipping for many years in millions of phones. With the advent of a > recent Google Summer of Code project, and a number of new devices in the > works from various companies, it is time to get the core greybus code > out of staging as it really is going to be with us for a while. > > Cc: Johan Hovold > Cc: Alex Elder > Cc: linux-ker...@vger.kernel.org > Cc: greybus-...@lists.linaro.org > Signed-off-by: Greg Kroah-Hartman Acked-by: Viresh Kumar -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH v2] Staging: greybus: Cleanup in greybus driver
On 16-04-19, 17:13, Madhumitha Prabakaran wrote: > Fix a blank line after structure declarations. Also, convert > macros into inline functions in order to maintain Linux kernel > coding style based on which the inline function is > preferable over the macro. > > Blank line fixes are suggested by checkpatch.pl This was all intentional and we use such macros everywhere in kernel. No need for such a patch, sorry. -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: greybus: spilib: fix use-after-free after deregistration
On 29-10-17, 13:01, Johan Hovold wrote:
> Remove erroneous spi_master_put() after controller deregistration which
> would access the already freed spi controller.
>
> Note that spi_unregister_master() drops our only controller reference.
>
> Fixes: ba3e67001b42 ("greybus: SPI: convert to a gpbridge driver")
> Cc: stable # 4.9
> Cc: Greg Kroah-Hartman
> Signed-off-by: Johan Hovold
> ---
> drivers/staging/greybus/spilib.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/greybus/spilib.c
> b/drivers/staging/greybus/spilib.c
> index e97b19148497..1e7321a1404c 100644
> --- a/drivers/staging/greybus/spilib.c
> +++ b/drivers/staging/greybus/spilib.c
> @@ -544,11 +544,14 @@ int gb_spilib_master_init(struct gb_connection
> *connection, struct device *dev,
>
> return 0;
>
> -exit_spi_unregister:
> - spi_unregister_master(master);
> exit_spi_put:
> spi_master_put(master);
>
> + return ret;
> +
> +exit_spi_unregister:
> + spi_unregister_master(master);
> +
> return ret;
> }
> EXPORT_SYMBOL_GPL(gb_spilib_master_init);
> @@ -558,7 +561,6 @@ void gb_spilib_master_exit(struct gb_connection
> *connection)
> struct spi_master *master = gb_connection_get_data(connection);
>
> spi_unregister_master(master);
> - spi_master_put(master);
> }
> EXPORT_SYMBOL_GPL(gb_spilib_master_exit);
Acked-by: Viresh Kumar
While looking at this I think I found another problem (I will send it as a
separate patch later on) and this fixes it:
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 6e65524cbfd9..af7ca751b4f7 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2261,11 +2261,11 @@ void spi_unregister_controller(struct spi_controller
*ctlr)
mutex_unlock(&board_lock);
dummy = device_for_each_child(&ctlr->dev, NULL, __unregister);
- device_unregister(&ctlr->dev);
/* free bus id */
mutex_lock(&board_lock);
idr_remove(&spi_master_idr, ctlr->bus_num);
mutex_unlock(&board_lock);
+ device_unregister(&ctlr->dev);
}
EXPORT_SYMBOL_GPL(spi_unregister_controller);
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: greybus: spilib: fix use-after-free after deregistration
On 29-10-17, 13:51, Johan Hovold wrote: > That's right, and I already posted a fix for that use-after-free: > > https://lkml.kernel.org/r/20171029115625.32385-1-jo...@kernel.org Great :) -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH 2/2] staging: greybus/loopback: use ktime_get() for time intervals
On 02-11-17, 15:32, Arnd Bergmann wrote: > This driver is the only one using the deprecated timeval_to_ns() > helper. Changing it from do_gettimeofday() to ktime_get() makes > the code more efficient, more robust against concurrent > settimeofday(), more accurate and lets us get rid of that helper > in the future. > > Signed-off-by: Arnd Bergmann > --- > drivers/staging/greybus/loopback.c | 42 > -- > 1 file changed, 18 insertions(+), 24 deletions(-) Reviewed-by: Viresh Kumar -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] staging: greybus: remove unused kfifo_ts
On 02-11-17, 15:32, Arnd Bergmann wrote:
> As of commit 8e1d6c336d74 ("greybus: loopback: drop bus aggregate
> calculation"), nothing ever reads from kfifo_ts, so there is no
> reason to write to it or even allocate it any more.
>
> Signed-off-by: Arnd Bergmann
> ---
> drivers/staging/greybus/loopback.c | 27 +++
> 1 file changed, 3 insertions(+), 24 deletions(-)
Reviewed-by: Viresh Kumar
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 02/11] staging: greybus: Remove redundant license text
On 07-11-17, 14:58, Greg Kroah-Hartman wrote: > Now that the SPDX tag is in all greybus files, that identifies the > license in a specific and legally-defined manner. So the extra GPL text > wording can be removed as it is no longer needed at all. > > This is done on a quest to remove the 700+ different ways that files in > the kernel describe the GPL license text. And there's unneeded stuff > like the address (sometimes incorrect) for the FSF which is never > needed. > > No copyright headers or other non-license-description text was removed. > > Cc: Vaibhav Hiremath > Cc: Johan Hovold > Cc: Alex Elder > Cc: Greg Kroah-Hartman > Cc: Vaibhav Agarwal > Cc: Mark Greer > Cc: Viresh Kumar > Cc: Rui Miguel Silva > Cc: David Lin > Cc: "Bryan O'Donoghue" > Signed-off-by: Greg Kroah-Hartman Acked-by: Viresh Kumar -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 01/11] staging: greybus: add SPDX identifiers to all greybus driver files
On 07-11-17, 14:58, Greg Kroah-Hartman wrote: > It's good to have SPDX identifiers in all files to make it easier to > audit the kernel tree for correct licenses. > > Update the drivers/staging/greybus files files with the correct SPDX > license identifier based on the license text in the file itself. The > SPDX identifier is a legally binding shorthand, which can be used > instead of the full boiler plate text. > > This work is based on a script and data from Thomas Gleixner, Philippe > Ombredanne, and Kate Stewart. > > Cc: Johan Hovold > Cc: Alex Elder > Cc: Greg Kroah-Hartman > Cc: Vaibhav Hiremath > Cc: Vaibhav Agarwal > Cc: Mark Greer > Cc: Viresh Kumar > Cc: Rui Miguel Silva > Cc: David Lin > Cc: "Bryan O'Donoghue" > Cc: Thomas Gleixner > Cc: Kate Stewart > Cc: Philippe Ombredanne > Signed-off-by: Greg Kroah-Hartman Acked-by: Viresh Kumar -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] greybus: fw-management: Replace strncpy with strlcpy
Hi Tobin,
On Wed, Feb 15, 2017 at 5:39 AM, Tobin C. Harding wrote:
> Greybus currently uses strncpy() coupled with a check for '\0' on the
> last byte of various buffers. strncpy() is passed size parameter equal
> to the size of the buffer in all instances. If the source string is
> larger than the destination buffer the check catches this and, after
> logging the error, returns an error value. In one instance the
> immediate return is not required. Using strncpy() with the manual check
> adds code that could be removed by the use of strlcpy(), although truncation
> then needs to be checked.
>
> Replace calls to strncpy() with calls to strlcpy(). Replace null
> termination checks with checks for truncated string. Add log message
> if string is truncated but do not return an error code.
>
> Signed-off-by: Tobin C. Harding
> ---
> drivers/staging/greybus/fw-management.c | 59
> +++--
> 1 file changed, 19 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/staging/greybus/fw-management.c
> b/drivers/staging/greybus/fw-management.c
> index 3cd6cf0..1cd5a45 100644
> --- a/drivers/staging/greybus/fw-management.c
> +++ b/drivers/staging/greybus/fw-management.c
> @@ -108,6 +108,7 @@ static int fw_mgmt_interface_fw_version_operation(struct
> fw_mgmt *fw_mgmt,
> struct gb_connection *connection = fw_mgmt->connection;
> struct gb_fw_mgmt_interface_fw_version_response response;
> int ret;
> + size_t len;
>
> ret = gb_operation_sync(connection,
> GB_FW_MGMT_TYPE_INTERFACE_FW_VERSION, NULL, 0,
> @@ -121,18 +122,11 @@ static int
> fw_mgmt_interface_fw_version_operation(struct fw_mgmt *fw_mgmt,
> fw_info->major = le16_to_cpu(response.major);
> fw_info->minor = le16_to_cpu(response.minor);
>
> - strncpy(fw_info->firmware_tag, response.firmware_tag,
> + len = strlcpy(fw_info->firmware_tag, response.firmware_tag,
> GB_FIRMWARE_TAG_MAX_SIZE);
> -
> - /*
> -* The firmware-tag should be NULL terminated, otherwise throw error
> but
> -* don't fail.
> -*/
> - if (fw_info->firmware_tag[GB_FIRMWARE_TAG_MAX_SIZE - 1] != '\0') {
> + if (len >= GB_FIRMWARE_TAG_MAX_SIZE)
I am not sure if the new code you have written is any better than what
was already there.
We still have a strcpy variant followed by a check. What has improved ?
> @@ -142,6 +136,7 @@ static int fw_mgmt_load_and_validate_operation(struct
> fw_mgmt *fw_mgmt,
> {
> struct gb_fw_mgmt_load_and_validate_fw_request request;
> int ret;
> + size_t len;
>
> if (load_method != GB_FW_LOAD_METHOD_UNIPRO &&
> load_method != GB_FW_LOAD_METHOD_INTERNAL) {
> @@ -151,16 +146,10 @@ static int fw_mgmt_load_and_validate_operation(struct
> fw_mgmt *fw_mgmt,
> }
>
> request.load_method = load_method;
> - strncpy(request.firmware_tag, tag, GB_FIRMWARE_TAG_MAX_SIZE);
> -
> - /*
> -* The firmware-tag should be NULL terminated, otherwise throw error
> and
> -* fail.
> -*/
> - if (request.firmware_tag[GB_FIRMWARE_TAG_MAX_SIZE - 1] != '\0') {
> - dev_err(fw_mgmt->parent, "load-and-validate: firmware-tag is
> not NULL terminated\n");
> - return -EINVAL;
Sorry but the error returns here and at other places were very intentional. I
wrote them to make sure the protocol is followed properly, and the other
side doesn't break it.
So far its a NAK from me.
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH v5] staging: greybus: loop_backtest: fixed consistent spacing style issue
On 01-03-17, 02:46, Jonathan Bowie wrote: > Fixed incosistent spacing around arithmetic operator. > > Signed-off-by: Jonathan Bowie > --- > v2 > -fixed subject added changelog > v3 > -CC:ed linux-next incorrectly, added proper summary > v4 > -CC:ed correct mailing list, moved changelog to summary body > v5 > -correctly placed changelog > > drivers/staging/greybus/tools/loopback_test.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) You can still include my Acked-by: Viresh Kumar :) -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH 2/3] staging: greybus: Replace "is is" with "is"
On 04-03-17, 21:37, simran singhal wrote: > This patch replace "is is " with "is". The replacement couldn't be > automated because sometimes the first "is" was meant to be another > word. > > Signed-off-by: simran singhal > --- > drivers/staging/greybus/uart.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Acked-by: Viresh Kumar -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: greybus: Fix warning to limit chars per line
On Fri, Apr 6, 2018 at 3:39 PM, Gaurav Dhingra
wrote:
Why did you remove the commit log? You had the right one in v1.
> Signed-off-by: Gaurav Dhingra
> ---
> Changes in v2:
> - use correct format for multiline comment
> ---
> drivers/staging/greybus/audio_codec.h | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/greybus/audio_codec.h
> b/drivers/staging/greybus/audio_codec.h
> index a1d5440..4efd8b3 100644
> --- a/drivers/staging/greybus/audio_codec.h
> +++ b/drivers/staging/greybus/audio_codec.h
> @@ -23,7 +23,10 @@ enum {
> NUM_CODEC_DAIS,
> };
>
> -/* device_type should be same as defined in audio.h (Android media layer) */
> +/*
> + * device_type should be same as defined in audio.h
> + * (Android media layer)
> + */
> enum {
> GBAUDIO_DEVICE_NONE = 0x0,
> /* reserved bits */
This looks ok.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: greybus: Fix warning to limit chars per line
On 06-04-18, 16:02, Gaurav Dhingra wrote:
> I didn't realize that it would be necessary to add it to this patch set. I
> thought you guys will do squashing of the commits (v2 and v1) and using just
> one commit message (and will take it from last commit), seems like I was
> wrong. Now I've understood it.
:)
When you send V2, it doesn't have any dependency on V1 and V1 is
discarded after that point. V2 is the full patch which should be
applied alone, we mention V2 in subject so that it is easier to track
the versions.
> >>Signed-off-by: Gaurav Dhingra
> >>---
> >>Changes in v2:
> >> - use correct format for multiline comment
> >>---
> >> drivers/staging/greybus/audio_codec.h | 5 -
> >> 1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/staging/greybus/audio_codec.h
> >>b/drivers/staging/greybus/audio_codec.h
> >>index a1d5440..4efd8b3 100644
> >>--- a/drivers/staging/greybus/audio_codec.h
> >>+++ b/drivers/staging/greybus/audio_codec.h
> >>@@ -23,7 +23,10 @@ enum {
> >> NUM_CODEC_DAIS,
> >> };
> >>
> >>-/* device_type should be same as defined in audio.h (Android media layer)
> >>*/
> >>+/*
> >>+ * device_type should be same as defined in audio.h
> >>+ * (Android media layer)
> >>+ */
> >> enum {
> >> GBAUDIO_DEVICE_NONE = 0x0,
> >> /* reserved bits */
> >This looks ok.
>
> Do I now need to send v3 with these exact changes, since I missed the
> 'commit log' in this one?
Yes please.
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH 47/61] staging: greybus: simplify getting .drvdata
On 19-04-18, 16:06, Wolfram Sang wrote: > We should get drvdata from struct device directly. Going via > platform_device is an unneeded step back and forth. > > Signed-off-by: Wolfram Sang > --- > > Build tested only. buildbot is happy. Please apply individually. > > drivers/staging/greybus/arche-platform.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) Acked-by: Viresh Kumar -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH v2] staging: greybus: Use gpio_is_valid()
On 28-04-18, 10:05, Arvind Yadav wrote:
> Replace the manual validity checks for the GPIO with the
> gpio_is_valid().
>
> Signed-off-by: Arvind Yadav
> ---
> chnage in v2 :
> Returning invalid gpio as error instead of -ENODEV.
>
> drivers/staging/greybus/arche-platform.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/greybus/arche-platform.c
> b/drivers/staging/greybus/arche-platform.c
> index 83254a7..c3a7da5 100644
> --- a/drivers/staging/greybus/arche-platform.c
> +++ b/drivers/staging/greybus/arche-platform.c
> @@ -448,7 +448,7 @@ static int arche_platform_probe(struct platform_device
> *pdev)
> arche_pdata->svc_reset_gpio = of_get_named_gpio(np,
> "svc,reset-gpio",
> 0);
> - if (arche_pdata->svc_reset_gpio < 0) {
> + if (!gpio_is_valid(arche_pdata->svc_reset_gpio)) {
> dev_err(dev, "failed to get reset-gpio\n");
> return arche_pdata->svc_reset_gpio;
> }
> @@ -468,7 +468,7 @@ static int arche_platform_probe(struct platform_device
> *pdev)
> arche_pdata->svc_sysboot_gpio = of_get_named_gpio(np,
> "svc,sysboot-gpio",
> 0);
> - if (arche_pdata->svc_sysboot_gpio < 0) {
> + if (!gpio_is_valid(arche_pdata->svc_sysboot_gpio)) {
> dev_err(dev, "failed to get sysboot gpio\n");
> return arche_pdata->svc_sysboot_gpio;
> }
> @@ -487,7 +487,7 @@ static int arche_platform_probe(struct platform_device
> *pdev)
> arche_pdata->svc_refclk_req = of_get_named_gpio(np,
> "svc,refclk-req-gpio",
> 0);
> - if (arche_pdata->svc_refclk_req < 0) {
> + if (!gpio_is_valid(arche_pdata->svc_refclk_req)) {
> dev_err(dev, "failed to get svc clock-req gpio\n");
> return arche_pdata->svc_refclk_req;
> }
Acked-by: Viresh Kumar
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH] staging: greybus: Remove unused local variable
On 05-05-18, 23:50, Nathan Chancellor wrote:
> Fixes the following W=1 warning: variable ‘intf_id’ set but
> not used [-Wunused-but-set-variable]
>
> Signed-off-by: Nathan Chancellor
> ---
> drivers/staging/greybus/svc.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/staging/greybus/svc.c b/drivers/staging/greybus/svc.c
> index a874fed761a1..a2bb7e1a3db3 100644
> --- a/drivers/staging/greybus/svc.c
> +++ b/drivers/staging/greybus/svc.c
> @@ -1137,7 +1137,6 @@ static int gb_svc_intf_reset_recv(struct gb_operation
> *op)
> struct gb_svc *svc = gb_connection_get_data(op->connection);
> struct gb_message *request = op->request;
> struct gb_svc_intf_reset_request *reset;
> - u8 intf_id;
>
> if (request->payload_size < sizeof(*reset)) {
> dev_warn(&svc->dev, "short reset request received (%zu <
> %zu)\n",
> @@ -1146,8 +1145,6 @@ static int gb_svc_intf_reset_recv(struct gb_operation
> *op)
> }
> reset = request->payload;
>
> - intf_id = reset->intf_id;
> -
> /* FIXME Reset the interface here */
>
> return 0;
Don't you get a new error after removing this, i.e "reset set but unused" ? Or
the sizeof() operation on that suppresses those warnings ..
Acked-by: Viresh Kumar
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH] staging: greybus: fix spelling mistake: "Inavlid" -> "Invalid"
On 22-05-18, 09:33, Colin King wrote:
> From: Colin Ian King
>
> Trivial fix to spelling mistake in dev_err error message
>
> Signed-off-by: Colin Ian King
> ---
> drivers/staging/greybus/audio_topology.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/greybus/audio_topology.c
> b/drivers/staging/greybus/audio_topology.c
> index de4b1b2b12f3..15e57f701630 100644
> --- a/drivers/staging/greybus/audio_topology.c
> +++ b/drivers/staging/greybus/audio_topology.c
> @@ -996,7 +996,7 @@ static int gbaudio_tplg_create_widget(struct
> gbaudio_module_info *module,
>
> ret = gbaudio_validate_kcontrol_count(w);
> if (ret) {
> - dev_err(module->dev, "Inavlid kcontrol count=%d for %s\n",
> + dev_err(module->dev, "Invalid kcontrol count=%d for %s\n",
> w->ncontrols, w->name);
> return ret;
> }
Acked-by: Viresh Kumar
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH 01/13] staging: greybus: camera: no need to check debugfs return values
On 29-05-18, 16:29, Greg Kroah-Hartman wrote:
> When calling debugfs functions, there is no need to ever check the
> return value. The function can work or not, but the code logic should
> never do something different based on this.
>
> Clean up the greybus camera driver by not caring about the value of
> debugfs calls. This ends up removing a number of lines of code that
> are not needed.
>
> Cc: Johan Hovold
> Cc: Alex Elder
> Cc: Greg Kroah-Hartman
> Cc: greybus-...@lists.linaro.org
> Signed-off-by: Greg Kroah-Hartman
> ---
> drivers/staging/greybus/camera.c | 17 +++--
> 1 file changed, 3 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/staging/greybus/camera.c
> b/drivers/staging/greybus/camera.c
> index 07ebfb88db9b..341f729a9779 100644
> --- a/drivers/staging/greybus/camera.c
> +++ b/drivers/staging/greybus/camera.c
> @@ -1174,11 +1174,6 @@ static int gb_camera_debugfs_init(struct gb_camera
> *gcam)
>gcam->bundle->id);
>
> gcam->debugfs.root = debugfs_create_dir(dirname, gb_debugfs_get());
> - if (IS_ERR(gcam->debugfs.root)) {
> - gcam_err(gcam, "debugfs root create failed (%ld)\n",
> - PTR_ERR(gcam->debugfs.root));
> - return PTR_ERR(gcam->debugfs.root);
> - }
>
> gcam->debugfs.buffers = vmalloc(sizeof(*gcam->debugfs.buffers) *
> GB_CAMERA_DEBUGFS_BUFFER_MAX);
> @@ -1188,18 +1183,12 @@ static int gb_camera_debugfs_init(struct gb_camera
> *gcam)
> for (i = 0; i < ARRAY_SIZE(gb_camera_debugfs_entries); ++i) {
> const struct gb_camera_debugfs_entry *entry =
> &gb_camera_debugfs_entries[i];
> - struct dentry *dentry;
>
> gcam->debugfs.buffers[i].length = 0;
>
> - dentry = debugfs_create_file(entry->name, entry->mask,
> - gcam->debugfs.root, gcam,
> - &gb_camera_debugfs_ops);
> - if (IS_ERR(dentry)) {
> - gcam_err(gcam,
> - "debugfs operation %s create failed (%ld)\n",
> - entry->name, PTR_ERR(dentry));
> - return PTR_ERR(dentry);
> + debugfs_create_file(entry->name, entry->mask,
> + gcam->debugfs.root, gcam,
> + &gb_camera_debugfs_ops);
> }
> }
Reviewed-by: Viresh Kumar
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH] Staging:greybus Fix comparison to NULL
On 03-06-18, 08:52, Janani Sankara Babu wrote:
> This patch replaces comparison of var to NULL with !var
>
> Signed-off-by: Janani Sankara Babu
> ---
> drivers/staging/greybus/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/greybus/core.c b/drivers/staging/greybus/core.c
> index dafa430..5d14a4e 100644
> --- a/drivers/staging/greybus/core.c
> +++ b/drivers/staging/greybus/core.c
> @@ -48,7 +48,7 @@ static bool greybus_match_one_id(struct gb_bundle *bundle,
> static const struct greybus_bundle_id *
> greybus_match_id(struct gb_bundle *bundle, const struct greybus_bundle_id
> *id)
> {
> - if (id == NULL)
> + if (!id)
It is pretty much personal preference and there is no good reason to accept this
patch really.
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] MAINTAINERS: update two greybus sections
On 11-06-18, 09:42, Johan Hovold wrote: > Fix a file entry typo and drop the obsolete timesync entries, which were > all caught by: > > scripts/get_maintainer.pl --self-test=patterns > > Reported-by: Joe Perches > Signed-off-by: Johan Hovold > --- > > This has been reported and at least partially fixed in the past, but due > to various other clean-up work that was going on in MAINTAINERS at the > time, was never applied. > > Johan > > > MAINTAINERS | 6 ++ > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 9c125f705f78..09d7f9eb5c13 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -6065,7 +6065,7 @@ F: drivers/staging/greybus/bootrom.c > F: drivers/staging/greybus/firmware.h > F: drivers/staging/greybus/fw-core.c > F: drivers/staging/greybus/fw-download.c > -F: drivers/staging/greybus/fw-managament.c > +F: drivers/staging/greybus/fw-management.c > F: drivers/staging/greybus/greybus_authentication.h > F: drivers/staging/greybus/greybus_firmware.h > F: drivers/staging/greybus/hid.c > @@ -6074,12 +6074,10 @@ F:drivers/staging/greybus/spi.c > F: drivers/staging/greybus/spilib.c > F: drivers/staging/greybus/spilib.h > > -GREYBUS LOOPBACK/TIME PROTOCOLS DRIVERS > +GREYBUS LOOPBACK DRIVER > M: Bryan O'Donoghue > S: Maintained > F: drivers/staging/greybus/loopback.c > -F: drivers/staging/greybus/timesync.c > -F: drivers/staging/greybus/timesync_platform.c > > GREYBUS PLATFORM DRIVERS > M: Vaibhav Hiremath Acked-by: Viresh Kumar -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH] staging: greybus: use after free in gb_audio_manager_remove_all()
On 05-02-20, 15:32, Dan Carpenter wrote:
> When we call kobject_put() and it's the last reference to the kobject
> then it calls gb_audio_module_release() and frees module. We dereference
> "module" on the next line which is a use after free.
>
> Fixes: c77f85bbc91a ("greybus: audio: Fix incorrect counting of 'ida'")
> Signed-off-by: Dan Carpenter
> ---
> drivers/staging/greybus/audio_manager.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/greybus/audio_manager.c
> b/drivers/staging/greybus/audio_manager.c
> index 9b19ea9d3fa1..9a3f7c034ab4 100644
> --- a/drivers/staging/greybus/audio_manager.c
> +++ b/drivers/staging/greybus/audio_manager.c
> @@ -92,8 +92,8 @@ void gb_audio_manager_remove_all(void)
>
> list_for_each_entry_safe(module, next, &modules_list, list) {
> list_del(&module->list);
> - kobject_put(&module->kobj);
> ida_simple_remove(&module_id, module->id);
> + kobject_put(&module->kobj);
> }
>
> is_empty = list_empty(&modules_list);
Acked-by: Viresh Kumar
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH] staging: greybus: compress return logic
On 25-03-17, 10:50, Arushi Singhal wrote:
> Simplify function returns by merging assignment and return.
>
> Signed-off-by: Arushi Singhal
> ---
> drivers/staging/greybus/loopback.c | 5 +
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/staging/greybus/loopback.c
> b/drivers/staging/greybus/loopback.c
> index aaf29a5fac83..08e255884206 100644
> --- a/drivers/staging/greybus/loopback.c
> +++ b/drivers/staging/greybus/loopback.c
> @@ -365,11 +365,8 @@ static void gb_loopback_calculate_stats(struct
> gb_loopback *gb, bool error);
>
> static u32 gb_loopback_nsec_to_usec_latency(u64 elapsed_nsecs)
> {
> - u32 lat;
> -
> do_div(elapsed_nsecs, NSEC_PER_USEC);
> - lat = elapsed_nsecs;
> - return lat;
> + return elapsed_nsecs;
> }
>
> static u64 __gb_loopback_calc_latency(u64 t1, u64 t2)
Acked-by: Viresh Kumar
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH v2] Staging: greybus: light: Prefer kcalloc over kzalloc
On 08-05-17, 18:05, kart...@techveda.org wrote:
> From: Karthik Tummala
>
> Fixed following checkpatch.pl warning:
> * WARNING: Prefer kcalloc over kzalloc with multiply
>
> Instead of specifying no.of bytes * size as argument
> in kzalloc, prefer kcalloc.
>
> Signed-off-by: Karthik Tummala
> Reviewed-by: Rui Miguel Silva
> ---
> Changes for v2:
> - Changed subject line & fixed typo as suggested by
> Rui Miguel Silva
> ---
> drivers/staging/greybus/light.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c
> index 1681362..861a249 100644
> --- a/drivers/staging/greybus/light.c
> +++ b/drivers/staging/greybus/light.c
> @@ -1030,7 +1030,7 @@ static int gb_lights_light_config(struct gb_lights
> *glights, u8 id)
> light->channels_count = conf.channel_count;
> light->name = kstrndup(conf.name, NAMES_MAX, GFP_KERNEL);
>
> - light->channels = kzalloc(light->channels_count *
> + light->channels = kcalloc(light->channels_count,
> sizeof(struct gb_channel), GFP_KERNEL);
> if (!light->channels)
> return -ENOMEM;
> @@ -1167,7 +1167,7 @@ static int gb_lights_create_all(struct gb_lights
> *glights)
> if (ret < 0)
> goto out;
>
> - glights->lights = kzalloc(glights->lights_count *
> + glights->lights = kcalloc(glights->lights_count,
> sizeof(struct gb_light), GFP_KERNEL);
> if (!glights->lights) {
> ret = -ENOMEM;
Acked-by: Viresh Kumar
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH] staging: greybus: power_supply: replace kzalloc by kcalloc
On 11-05-17, 22:58, JB Van Puyvelde wrote: > According to checkpatch.pl, kcalloc should be preferred to kzalloc with > multiply. > > Signed-off-by: JB Van Puyvelde > --- > drivers/staging/greybus/power_supply.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) You should have kept my Ack in this patch, as I have already given it earlier. Anyway, here it is again. Acked-by: Viresh Kumar > > diff --git a/drivers/staging/greybus/power_supply.c > b/drivers/staging/greybus/power_supply.c > index e85c988b7034..20cac20518d7 100644 > --- a/drivers/staging/greybus/power_supply.c > +++ b/drivers/staging/greybus/power_supply.c > @@ -944,7 +944,7 @@ static int gb_power_supplies_setup(struct > gb_power_supplies *supplies) > if (ret < 0) > goto out; > > - supplies->supply = kzalloc(supplies->supplies_count * > + supplies->supply = kcalloc(supplies->supplies_count, >sizeof(struct gb_power_supply), >GFP_KERNEL); > > -- > 2.11.0 > > ___ > greybus-dev mailing list > greybus-...@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/greybus-dev -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH] staging: greybus: mark PM functions as __maybe_unused
On 18-05-17, 15:28, Arnd Bergmann wrote:
> Enabling the arche platform for compile testing showed a harmless
> warning with CONFIG_PM=n:
>
> drivers/staging/greybus/arche-platform.c:632:12: error:
> 'arche_platform_resume' defined but not used [-Werror=unused-function]
> drivers/staging/greybus/arche-platform.c:618:12: error:
> 'arche_platform_suspend' defined but not used [-Werror=unused-function]
>
> This marks the functions as __maybe_unused to shut up the warnings.
>
> Fixes: 2eccd4aa19fc ("staging: greybus: enable compile testing of arche
> driver")
> Signed-off-by: Arnd Bergmann
> ---
> drivers/staging/greybus/arche-platform.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/greybus/arche-platform.c
> b/drivers/staging/greybus/arche-platform.c
> index 5bce5e039596..eced2d26467b 100644
> --- a/drivers/staging/greybus/arche-platform.c
> +++ b/drivers/staging/greybus/arche-platform.c
> @@ -615,7 +615,7 @@ static int arche_platform_remove(struct platform_device
> *pdev)
> return 0;
> }
>
> -static int arche_platform_suspend(struct device *dev)
> +static __maybe_unused int arche_platform_suspend(struct device *dev)
> {
> /*
>* If timing profile premits, we may shutdown bridge
> @@ -629,7 +629,7 @@ static int arche_platform_suspend(struct device *dev)
> return 0;
> }
>
> -static int arche_platform_resume(struct device *dev)
> +static __maybe_unused int arche_platform_resume(struct device *dev)
> {
> /*
>* Atleast for ES2 we have to meet the delay requirement between
Is __maybe_unused the more preferred way than putting these routines
under CONFIG_PM_SLEEP ifdef ?
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH] staging: greybus: mark PM functions as __maybe_unused
On 18-05-17, 16:51, Arnd Bergmann wrote: > I find that a lot of users get the #ifdef wrong, either using the wrong > macro (CONFIG_PM vs CONFIG_PM_SLEEP) or not using the right > set of functions (e.g. calling a function only from the suspend handler). > > The __maybe_unused annotation avoids both problems and also gives > better build time coverage, so that's what I tend to use. Thanks for the explanation Arnd. I hope these unused routines will not be part of the binary that gets generated. Right? Reviewed-by: Viresh Kumar -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 22/23] drivers/hv: Migrate to new 'set-state' interface
Migrate hv driver to the new 'set-state' interface provided by
clockevents core, the earlier 'set-mode' interface is marked obsolete
now.
This also enables us to implement callbacks for new states of clockevent
devices, for example: ONESHOT_STOPPED.
Cc: "K. Y. Srinivasan"
Cc: Haiyang Zhang
Cc: de...@linuxdriverproject.org
Signed-off-by: Viresh Kumar
---
drivers/hv/hv.c | 45 +++--
1 file changed, 19 insertions(+), 26 deletions(-)
diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
index d3943bceecc3..76e519ec2e51 100644
--- a/drivers/hv/hv.c
+++ b/drivers/hv/hv.c
@@ -271,7 +271,7 @@ static int hv_ce_set_next_event(unsigned long delta,
{
cycle_t current_tick;
- WARN_ON(evt->mode != CLOCK_EVT_MODE_ONESHOT);
+ WARN_ON(!clockevent_state_oneshot(evt));
rdmsrl(HV_X64_MSR_TIME_REF_COUNT, current_tick);
current_tick += delta;
@@ -279,31 +279,24 @@ static int hv_ce_set_next_event(unsigned long delta,
return 0;
}
-static void hv_ce_setmode(enum clock_event_mode mode,
- struct clock_event_device *evt)
+static int hv_ce_shutdown(struct clock_event_device *evt)
+{
+ wrmsrl(HV_X64_MSR_STIMER0_COUNT, 0);
+ wrmsrl(HV_X64_MSR_STIMER0_CONFIG, 0);
+
+ return 0;
+}
+
+static int hv_ce_set_oneshot(struct clock_event_device *evt)
{
union hv_timer_config timer_cfg;
- switch (mode) {
- case CLOCK_EVT_MODE_PERIODIC:
- /* unsupported */
- break;
-
- case CLOCK_EVT_MODE_ONESHOT:
- timer_cfg.enable = 1;
- timer_cfg.auto_enable = 1;
- timer_cfg.sintx = VMBUS_MESSAGE_SINT;
- wrmsrl(HV_X64_MSR_STIMER0_CONFIG, timer_cfg.as_uint64);
- break;
-
- case CLOCK_EVT_MODE_UNUSED:
- case CLOCK_EVT_MODE_SHUTDOWN:
- wrmsrl(HV_X64_MSR_STIMER0_COUNT, 0);
- wrmsrl(HV_X64_MSR_STIMER0_CONFIG, 0);
- break;
- case CLOCK_EVT_MODE_RESUME:
- break;
- }
+ timer_cfg.enable = 1;
+ timer_cfg.auto_enable = 1;
+ timer_cfg.sintx = VMBUS_MESSAGE_SINT;
+ wrmsrl(HV_X64_MSR_STIMER0_CONFIG, timer_cfg.as_uint64);
+
+ return 0;
}
static void hv_init_clockevent_device(struct clock_event_device *dev, int cpu)
@@ -318,7 +311,8 @@ static void hv_init_clockevent_device(struct
clock_event_device *dev, int cpu)
* references to the hv_vmbus module making it impossible to unload.
*/
- dev->set_mode = hv_ce_setmode;
+ dev->set_state_shutdown = hv_ce_shutdown;
+ dev->set_state_oneshot = hv_ce_set_oneshot;
dev->set_next_event = hv_ce_set_next_event;
}
@@ -503,8 +497,7 @@ void hv_synic_cleanup(void *arg)
/* Turn off clockevent device */
if (ms_hyperv.features & HV_X64_MSR_SYNTIMER_AVAILABLE)
- hv_ce_setmode(CLOCK_EVT_MODE_SHUTDOWN,
- hv_context.clk_evt[cpu]);
+ hv_ce_shutdown(hv_context.clk_evt[cpu]);
rdmsrl(HV_X64_MSR_SINT0 + VMBUS_MESSAGE_SINT, shared_sint.as_uint64);
--
2.4.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 10/15] drivers: staging: Drop unlikely before IS_ERR(_OR_NULL)
IS_ERR(_OR_NULL) already contain an 'unlikely' compiler flag and there
is no need to do that again from its callers. Drop it.
This also replaces an IS_ERR(x) + (x == NULL) check to IS_ERR_OR_NULL
check.
Signed-off-by: Viresh Kumar
---
drivers/staging/android/ashmem.c | 2 +-
drivers/staging/lustre/include/linux/libcfs/libcfs.h | 2 +-
drivers/staging/lustre/lustre/obdclass/lu_object.c | 6 +++---
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index c5c037ccf32c..9db321c63e79 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -388,7 +388,7 @@ static int ashmem_mmap(struct file *file, struct
vm_area_struct *vma)
/* ... and allocate the backing shmem file */
vmfile = shmem_file_setup(name, asma->size, vma->vm_flags);
- if (unlikely(IS_ERR(vmfile))) {
+ if (IS_ERR(vmfile)) {
ret = PTR_ERR(vmfile);
goto out;
}
diff --git a/drivers/staging/lustre/include/linux/libcfs/libcfs.h
b/drivers/staging/lustre/include/linux/libcfs/libcfs.h
index 5dd9cdfae30c..d585041041c7 100644
--- a/drivers/staging/lustre/include/linux/libcfs/libcfs.h
+++ b/drivers/staging/lustre/include/linux/libcfs/libcfs.h
@@ -134,7 +134,7 @@ void cfs_get_random_bytes(void *buf, int size);
/* container_of depends on "likely" which is defined in libcfs_private.h */
static inline void *__container_of(void *ptr, unsigned long shift)
{
- if (unlikely(IS_ERR(ptr) || ptr == NULL))
+ if (IS_ERR_OR_NULL(ptr))
return ptr;
return (char *)ptr - shift;
}
diff --git a/drivers/staging/lustre/lustre/obdclass/lu_object.c
b/drivers/staging/lustre/lustre/obdclass/lu_object.c
index 4d9b6333eeae..6cd3af8c6237 100644
--- a/drivers/staging/lustre/lustre/obdclass/lu_object.c
+++ b/drivers/staging/lustre/lustre/obdclass/lu_object.c
@@ -602,7 +602,7 @@ static struct lu_object *lu_object_new(const struct lu_env
*env,
struct lu_site_bkt_data *bkt;
o = lu_object_alloc(env, dev, f, conf);
- if (unlikely(IS_ERR(o)))
+ if (IS_ERR(o))
return o;
hs = dev->ld_site->ls_obj_hash;
@@ -666,7 +666,7 @@ static struct lu_object *lu_object_find_try(const struct
lu_env *env,
* operations, including fld queries, inode loading, etc.
*/
o = lu_object_alloc(env, dev, f, conf);
- if (unlikely(IS_ERR(o)))
+ if (IS_ERR(o))
return o;
LASSERT(lu_fid_eq(lu_object_fid(o), f));
@@ -1558,7 +1558,7 @@ static int keys_fill(struct lu_context *ctx)
LINVRNT(key->lct_index == i);
value = key->lct_init(ctx, key);
- if (unlikely(IS_ERR(value)))
+ if (IS_ERR(value))
return PTR_ERR(value);
if (!(ctx->lc_tags & LCT_NOREF))
--
2.4.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: lustre: drop redundant check
There is no need to verify that its an error, as we are anyway going to
match the error value to -ENOENT. Drop the redundant check.
Reported-by: "Kirill A. Shutemov"
Signed-off-by: Viresh Kumar
---
drivers/staging/lustre/lustre/obdclass/lu_object.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/lustre/lustre/obdclass/lu_object.c
b/drivers/staging/lustre/lustre/obdclass/lu_object.c
index 6cd3af8c6237..8e472327c880 100644
--- a/drivers/staging/lustre/lustre/obdclass/lu_object.c
+++ b/drivers/staging/lustre/lustre/obdclass/lu_object.c
@@ -674,7 +674,7 @@ static struct lu_object *lu_object_find_try(const struct
lu_env *env,
cfs_hash_bd_lock(hs, &bd, 1);
shadow = htable_lookup(s, &bd, f, waiter, &version);
- if (likely(IS_ERR(shadow) && PTR_ERR(shadow) == -ENOENT)) {
+ if (likely(PTR_ERR(shadow) == -ENOENT)) {
struct lu_site_bkt_data *bkt;
bkt = cfs_hash_bd_extra_get(hs, &bd);
--
2.4.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: greybus: audio_module: remove redundant OOM message
On Tue, Dec 6, 2016 at 7:39 PM, Srikant Ritolia wrote: > All kmalloc-based functions print enough information on failure > > Signed-off-by: Srikant Ritolia > --- > Changes in v2: > - Added driver name in the subject for better readability. Acked-by: Viresh Kumar ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH] MAINTAINERS: add greybus subsystem mailing list
On 06-01-17, 08:20, Greg Kroah-Hartman wrote: > From: Greg Kroah-Hartman > > The Greybus driver subsystem has a mailing list, so list it in the > MAINTAINERS file so that people know to send patches there as well. > > Signed-off-by: Greg Kroah-Hartman > > diff --git a/MAINTAINERS b/MAINTAINERS > index cfff2c9e3d94..f6cb07684cea 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -5504,6 +5504,7 @@ M: Alex Elder > M: Greg Kroah-Hartman > S: Maintained > F: drivers/staging/greybus/ > +L: greybus-...@lists.linaro.org > > GREYBUS AUDIO PROTOCOLS DRIVERS > M: Vaibhav Agarwal Reviewed-by: Viresh Kumar -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: greybus: fix checkpatch braces not necessary warning
On Wed, Jan 11, 2017 at 6:25 AM, Abdul Rauf wrote:
> Fix the following warnings:
> braces {} are not necessary for any arm of this statement
>
> Signed-off-by: Abdul Rauf
> ---
> drivers/staging/greybus/loopback.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
Acked-by: Viresh Kumar
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: greybus: loopback_test: fix checkpatch bad function definition error
On Wed, Jan 11, 2017 at 6:59 AM, Abdul Rauf wrote: > Fix the following Errors: > Bad function definition - void abort() should probably be void abort(void) > > Signed-off-by: Abdul Rauf > --- > drivers/staging/greybus/tools/loopback_test.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Acked-by: Viresh Kumar ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH v3] staging: greybus: checkpatch: Change parameter type unsigned to unsigned int
On 11-01-17, 16:00, Roman Sommer wrote: > Note that this patch does not fix all checkpatch warnings for the > affected files. > > Signed-off-by: Christian Bewermeyer > Signed-off-by: Roman Sommer > > --- > drivers/staging/greybus/gpio.c | 24 > drivers/staging/greybus/loopback.c | 2 +- > 2 files changed, 13 insertions(+), 13 deletions(-) I think checkpatch should rather not warn about it. Using 'unsigned' instead of 'unsigned int' isn't that bad :) Acked-by: Viresh Kumar -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH v2] staging: greybus: timesync: validate platform state callback
On 23-01-17, 16:32, Rui Miguel Silva wrote:
> When tearingdown timesync, and not in arche platform, the state platform
> callback is not initialized. That will trigger the following NULL
> dereferencing.
> CallTrace:
>
> ? gb_timesync_platform_unlock_bus+0x11/0x20 [greybus]
> gb_timesync_teardown+0x85/0xc0 [greybus]
> gb_timesync_svc_remove+0xab/0x190 [greybus]
> gb_svc_del+0x29/0x110 [greybus]
> gb_hd_del+0x14/0x20 [greybus]
> ap_disconnect+0x24/0x60 [gb_es2]
> usb_unbind_interface+0x7a/0x2c0
> __device_release_driver+0x96/0x150
> device_release_driver+0x1e/0x30
> bus_remove_device+0xe7/0x130
> device_del+0x116/0x230
> usb_disable_device+0x97/0x1f0
> usb_disconnect+0x80/0x260
> hub_event+0x5ca/0x10e0
> process_one_work+0x126/0x3b0
> worker_thread+0x55/0x4c0
> ? process_one_work+0x3b0/0x3b0
> kthread+0xc4/0xe0
> ? kthread_park+0xb0/0xb0
> ret_from_fork+0x22/0x30
>
> So, fix that by adding checks before use the callback.
>
> Fixes: 970dc85bd95d ("greybus: timesync: Add timesync core driver")
> Cc: # 4.9.x
> Signed-off-by: Rui Miguel Silva
> ---
> Greg,
> This patch does not contain the "commit upstream" snippet since you
> already removed the timesync code from the -next tree, even though it is still
> in 4.10-rc. However this will affect the greybus users of 4.9.x. If you
> meanwhile pull that remove to rc, this patch is only necessary in stable
> 4.9.x.
>
> v1->v2:
>- add Fixes tag to changelog
>Johan Hovold:
> - for symmetry with the lock, adjust the unlock_bus function
> accordantly.
Reviewed-by: Viresh Kumar
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH] staging: greybus: uart: fix TIOCGSERIAL flags
On 25-01-17, 18:07, Johan Hovold wrote: > This driver does not have a low-latency mode and should not report > anything else. > > Also drop the skip-test flag which isn't used either. > > Signed-off-by: Johan Hovold > --- > drivers/staging/greybus/uart.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c > index 6d39f4a04754..248ad6661a02 100644 > --- a/drivers/staging/greybus/uart.c > +++ b/drivers/staging/greybus/uart.c > @@ -624,7 +624,6 @@ static int get_serial_info(struct gb_tty *gb_tty, > struct serial_struct tmp; > > memset(&tmp, 0, sizeof(tmp)); > - tmp.flags = ASYNC_LOW_LATENCY | ASYNC_SKIP_TEST; > tmp.type = PORT_16550A; > tmp.line = gb_tty->minor; > tmp.xmit_fifo_size = 16; Reviewed-by: Viresh Kumar -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH] staging: greybus: sdio: Prefer u32 over uint32_t
On 25-01-17, 18:38, Franck Demathieu wrote: > It fixes the following issue reported by checkpatch.pl: > Prefer kernel type 'u32' over 'uint32_t' > > Signed-off-by: Franck Demathieu > --- > drivers/staging/greybus/sdio.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Acked-by: Viresh Kumar -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: greybus: Move Documentation to Documentation/
Move all greybus documentation to the top level Documentation/
directory, as that's the obvious place where everyone will look for it.
Signed-off-by: Viresh Kumar
---
.../Documentation => Documentation/greybus}/firmware/authenticate.c | 0
.../Documentation => Documentation/greybus}/firmware/firmware-management | 0
.../greybus/Documentation => Documentation/greybus}/firmware/firmware.c | 0
.../greybus/Documentation => Documentation/greybus}/sysfs-bus-greybus | 0
4 files changed, 0 insertions(+), 0 deletions(-)
rename {drivers/staging/greybus/Documentation =>
Documentation/greybus}/firmware/authenticate.c (100%)
rename {drivers/staging/greybus/Documentation =>
Documentation/greybus}/firmware/firmware-management (100%)
rename {drivers/staging/greybus/Documentation =>
Documentation/greybus}/firmware/firmware.c (100%)
rename {drivers/staging/greybus/Documentation =>
Documentation/greybus}/sysfs-bus-greybus (100%)
diff --git a/drivers/staging/greybus/Documentation/firmware/authenticate.c
b/Documentation/greybus/firmware/authenticate.c
similarity index 100%
rename from drivers/staging/greybus/Documentation/firmware/authenticate.c
rename to Documentation/greybus/firmware/authenticate.c
diff --git a/drivers/staging/greybus/Documentation/firmware/firmware-management
b/Documentation/greybus/firmware/firmware-management
similarity index 100%
rename from drivers/staging/greybus/Documentation/firmware/firmware-management
rename to Documentation/greybus/firmware/firmware-management
diff --git a/drivers/staging/greybus/Documentation/firmware/firmware.c
b/Documentation/greybus/firmware/firmware.c
similarity index 100%
rename from drivers/staging/greybus/Documentation/firmware/firmware.c
rename to Documentation/greybus/firmware/firmware.c
diff --git a/drivers/staging/greybus/Documentation/sysfs-bus-greybus
b/Documentation/greybus/sysfs-bus-greybus
similarity index 100%
rename from drivers/staging/greybus/Documentation/sysfs-bus-greybus
rename to Documentation/greybus/sysfs-bus-greybus
--
2.7.1.410.g6faf27b
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: greybus: Move Documentation to Documentation/
On 07-02-17, 09:02, Greg Kroah-Hartman wrote: > On Tue, Feb 07, 2017 at 11:15:29AM +0530, Viresh Kumar wrote: > > Move all greybus documentation to the top level Documentation/ > > directory, as that's the obvious place where everyone will look for it. > > No, not until we have the core greybus code out of staging. While code > is in staging, it is self-contained, and shouldn't spread outside of > it's own directory. I see. No issues. I just had to refer to the documentation today and was wondering why shouldn't it be moved to Documentation/ :) -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers/staging/greybus: gpio.c - Fixed a checkpatch generated warning
On Mon, Sep 26, 2016 at 2:05 AM, Chase Metzger wrote:
> Removed braces for single line if statement.
>
> Signed-off-by: Chase Metzger
> ---
> drivers/staging/greybus/gpio.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/staging/greybus/gpio.c b/drivers/staging/greybus/gpio.c
> index ea8234a..5e06e42 100644
> --- a/drivers/staging/greybus/gpio.c
> +++ b/drivers/staging/greybus/gpio.c
> @@ -561,9 +561,8 @@ static void gb_gpio_irqchip_remove(struct
> gb_gpio_controller *ggc)
> irq_domain_remove(ggc->irqdomain);
> }
>
> - if (ggc->irqchip) {
> + if (ggc->irqchip)
> ggc->irqchip = NULL;
> - }
> }
>
> /**
Acked-by: Viresh Kumar
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] greybus: manifest: style fix missing space before '('
On Tue, Sep 27, 2016 at 2:50 PM, Quentin Lambert
wrote:
> Checkpatch printed a style ERROR concerning a missing space befire '('.
> This patch fix this issue.
>
> Signed-off-by: Quentin Lambert
> ---
> drivers/staging/greybus/manifest.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- a/drivers/staging/greybus/manifest.c
> +++ b/drivers/staging/greybus/manifest.c
> @@ -11,7 +11,7 @@
>
> static const char *get_descriptor_type_string(u8 type)
> {
> - switch(type) {
> + switch (type) {
> case GREYBUS_TYPE_INVALID:
> return "invalid";
> case GREYBUS_TYPE_STRING:
Acked-by: Viresh Kumar
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] greybus: audio: fix uninitialized variable errors found by cppcheck
On Sat, Sep 24, 2016 at 11:06 PM, Vaibhav Agarwal wrote:
> Currently, if info is null, the dev_err message is dereferencing an
> uninitialized module pointer. Instead, it should use codec->dev pointer
> in dev_err call and better align with other err msg in this function.
>
> Also, ret variable might be used uninitialized in a specific case.
> Avoid using it this way.
>
> Found using static analysis with cppcheck:
> Checking drivers/staging/greybus/audio_topology.c...
> [drivers/staging/greybus/audio_topology.c:175]: (error) Uninitialized
> variable: module
> [drivers/staging/greybus/audio_topology.c:495]: (error) Uninitialized
> variable: ret
>
> Reported-by: Colin Ian King
> Signed-off-by: Vaibhav Agarwal
> ---
> drivers/staging/greybus/audio_topology.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/greybus/audio_topology.c
> b/drivers/staging/greybus/audio_topology.c
> index f9f33817a092..b6251691a33d 100644
> --- a/drivers/staging/greybus/audio_topology.c
> +++ b/drivers/staging/greybus/audio_topology.c
> @@ -172,7 +172,7 @@ static int gbcodec_mixer_ctl_info(struct snd_kcontrol
> *kcontrol,
> info = (struct gb_audio_ctl_elem_info *)data->info;
>
> if (!info) {
> - dev_err(module->dev, "NULL info for %s\n", uinfo->id.name);
> + dev_err(codec->dev, "NULL info for %s\n", uinfo->id.name);
> return -EINVAL;
> }
>
> @@ -489,10 +489,11 @@ static int gbcodec_mixer_dapm_ctl_put(struct
> snd_kcontrol *kcontrol,
> dev_err_ratelimited(codec->dev,
> "%d:Error in %s for %s\n", ret,
> __func__, kcontrol->id.name);
> + return ret;
> }
> }
>
> - return ret;
> + return 0;
> }
>
> #define SOC_DAPM_MIXER_GB(xname, kcount, data) \
Acked-by: Viresh Kumar
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] greybus: manifest: style fix missing space before '('
On 27 September 2016 at 15:03, Quentin Lambert
wrote:
>
>
> On 27/09/2016 11:31, Viresh Kumar wrote:
>>
>> On Tue, Sep 27, 2016 at 2:50 PM, Quentin Lambert
>> wrote:
>>>
>>> Checkpatch printed a style ERROR concerning a missing space befire '('.
>>> This patch fix this issue.
>>>
>>> Signed-off-by: Quentin Lambert
>>> ---
>>> drivers/staging/greybus/manifest.c |2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> --- a/drivers/staging/greybus/manifest.c
>>> +++ b/drivers/staging/greybus/manifest.c
>>> @@ -11,7 +11,7 @@
>>>
>>> static const char *get_descriptor_type_string(u8 type)
>>> {
>>> - switch(type) {
>>> + switch (type) {
>>> case GREYBUS_TYPE_INVALID:
>>> return "invalid";
>>> case GREYBUS_TYPE_STRING:
>>
>> Acked-by: Viresh Kumar
>
> Wow I just noticed the typo: "befire" do you want me to resend ?
Oops, sorry for missing that.
Yes, please resend the patch as V2 and feel free to add my Ack to it.
--
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [bug report] greybus: sdio: extend sdio implementation
On Fri, Sep 30, 2016 at 3:25 PM, Dan Carpenter wrote: > Hello Rui Miguel Silva, > > The patch 3b6ecd6de6b4: "greybus: sdio: extend sdio implementation" > from Jun 22, 2015, leads to the following static checker warning: > > drivers/staging/greybus/sdio.c:481 gb_sdio_command() > warn: bitwise AND condition is false here > > drivers/staging/greybus/sdio.c >474 ret = gb_operation_sync(host->connection, > GB_SDIO_TYPE_COMMAND, >475 &request, sizeof(request), &response, >476 sizeof(response)); >477 if (ret < 0) >478 goto out; >479 >480 /* no response expected */ >481 if (cmd_flags & GB_SDIO_RSP_NONE) > > This is zero. Oh yes.. The macro is set to 0 and this is obviously wrong :( ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: greybus: light: check the correct value of delay_on
On Fri, Sep 30, 2016 at 10:56 PM, Rui Miguel Silva wrote:
> When checking the value of delay_on to set the channel as active, it was
> checked the pointer and not the value, as it should be.
>
> Fixes: cc43368a3c ("greybus: lights: Control runtime pm suspend/resume on AP
> side")
>
> Signed-off-by: Rui Miguel Silva
> ---
> drivers/staging/greybus/light.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c
> index 2de9fc3..a631338 100644
> --- a/drivers/staging/greybus/light.c
> +++ b/drivers/staging/greybus/light.c
> @@ -484,7 +484,7 @@ static int gb_blink_set(struct led_classdev *cdev,
> unsigned long *delay_on,
> if (ret < 0)
> goto out_pm_put;
>
> - if (delay_on)
> + if (*delay_on)
> channel->active = true;
> else
> channel->active = false;
Acked-by: Viresh Kumar
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/3] staging: greybus: sdio: fix cmd_flags check for none response
On Fri, Sep 30, 2016 at 11:54 PM, Rui Miguel Silva wrote: > When checking for command flags field if response is not available we > really need to compare it with the right define and not bitwise AND it. > > smatch warn: > drivers/staging/greybus/sdio.c:481 gb_sdio_command() > warn: bitwise AND condition is false here > > Reported-by: Dan Carpenter > Signed-off-by: Rui Miguel Silva > --- > drivers/staging/greybus/sdio.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/greybus/sdio.c b/drivers/staging/greybus/sdio.c > index c7133b1..5649ef1 100644 > --- a/drivers/staging/greybus/sdio.c > +++ b/drivers/staging/greybus/sdio.c > @@ -478,7 +478,7 @@ static int gb_sdio_command(struct gb_sdio_host *host, > struct mmc_command *cmd) > goto out; > > /* no response expected */ > - if (cmd_flags & GB_SDIO_RSP_NONE) > + if (cmd_flags == GB_SDIO_RSP_NONE) > goto out; > > /* long response expected */ Acked-by: Viresh Kumar ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/3] staging: greybus: light: fix attributes allocation
On Fri, Sep 30, 2016 at 11:54 PM, Rui Miguel Silva wrote: > Fix allocation of attributes with the correct size, this also fix smatch > warning: > > drivers/staging/greybus/light.c:293 channel_attr_groups_set() > warn: double check that we're allocating correct size: 8 vs 16 > > Signed-off-by: Rui Miguel Silva > --- > drivers/staging/greybus/light.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c > index b2847fe..f3cd485 100644 > --- a/drivers/staging/greybus/light.c > +++ b/drivers/staging/greybus/light.c > @@ -290,8 +290,7 @@ static int channel_attr_groups_set(struct gb_channel > *channel, > return 0; > > /* Set attributes based in the channel flags */ > - channel->attrs = kcalloc(size + 1, sizeof(**channel->attrs), > -GFP_KERNEL); > + channel->attrs = kcalloc(size + 1, sizeof(*channel->attrs), > GFP_KERNEL); > if (!channel->attrs) > return -ENOMEM; > channel->attr_group = kcalloc(1, sizeof(*channel->attr_group), Acked-by: Viresh Kumar ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 3/3] staging: greybus: light: check delay_{on|off} before use
On Fri, Sep 30, 2016 at 11:54 PM, Rui Miguel Silva wrote:
> Even though we trust leds core that the pointers should be valid, we are
> safer to check delay_{on|off} before use.
As these routines are getting called from core, I am not sure if it will
be right to have such checks.
Though, I saw the core code after writing above and the core doesn't check
these at all :)
> Also, this avoid a smatch warning:
> drivers/staging/greybus/light.c:484 gb_blink_set()
> warn: variable dereferenced before check 'delay_on' (see line 476)
>
> Signed-off-by: Rui Miguel Silva
> ---
> drivers/staging/greybus/light.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c
> index f3cd485..80dc4a9 100644
> --- a/drivers/staging/greybus/light.c
> +++ b/drivers/staging/greybus/light.c
> @@ -463,6 +463,9 @@ static int gb_blink_set(struct led_classdev *cdev,
> unsigned long *delay_on,
> if (channel->releasing)
> return -ESHUTDOWN;
>
> + if (!delay_on || !delay_off)
> + return -EINVAL;
> +
> mutex_lock(&channel->lock);
> ret = gb_pm_runtime_get_sync(bundle);
> if (ret < 0)
Acked-by: Viresh Kumar
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
