* Re: [PATCH 5/6] ARM: dts: rcar-gen2: Remove unused VIN properties
From: Niklas Söderlund @ 2018-05-16 22:13 UTC (permalink / raw)
To: Jacopo Mondi
Cc: devicetree, robh+dt, linux-renesas-soc, horms, geert,
laurent.pinchart, linux-arm-kernel, linux-media
In-Reply-To: <1526488352-898-6-git-send-email-jacopo+renesas@jmondi.org>
Hi Jacopo,
Thanks for your work.
On 2018-05-16 18:32:31 +0200, Jacopo Mondi wrote:
> The 'bus-width' and 'pclk-sample' properties are not parsed by the VIN
> driver and only confuse users. Remove them in all Gen2 SoC that used
> them.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
> arch/arm/boot/dts/r8a7790-lager.dts | 3 ---
> arch/arm/boot/dts/r8a7791-koelsch.dts | 3 ---
> arch/arm/boot/dts/r8a7791-porter.dts | 1 -
> arch/arm/boot/dts/r8a7793-gose.dts | 3 ---
> arch/arm/boot/dts/r8a7794-alt.dts | 1 -
> arch/arm/boot/dts/r8a7794-silk.dts | 1 -
> 6 files changed, 12 deletions(-)
>
> diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
> index 063fdb6..b56b309 100644
> --- a/arch/arm/boot/dts/r8a7790-lager.dts
> +++ b/arch/arm/boot/dts/r8a7790-lager.dts
> @@ -873,10 +873,8 @@
> port {
> vin0ep2: endpoint {
> remote-endpoint = <&adv7612_out>;
> - bus-width = <24>;
I can't really make up my mind if this is a good thing or not. Device
tree describes the hardware and not what the drivers make use of. And
the fact is that this bus is 24 bits wide. So I'm not sure we should
remove these properties. But I would love to hear what others think
about this.
> hsync-active = <0>;
> vsync-active = <0>;
> - pclk-sample = <1>;
> data-active = <1>;
> };
> };
> @@ -895,7 +893,6 @@
>
> vin1ep0: endpoint {
> remote-endpoint = <&adv7180>;
> - bus-width = <8>;
> };
> };
> };
> diff --git a/arch/arm/boot/dts/r8a7791-koelsch.dts b/arch/arm/boot/dts/r8a7791-koelsch.dts
> index f40321a..9967666 100644
> --- a/arch/arm/boot/dts/r8a7791-koelsch.dts
> +++ b/arch/arm/boot/dts/r8a7791-koelsch.dts
> @@ -849,10 +849,8 @@
>
> vin0ep2: endpoint {
> remote-endpoint = <&adv7612_out>;
> - bus-width = <24>;
> hsync-active = <0>;
> vsync-active = <0>;
> - pclk-sample = <1>;
> data-active = <1>;
> };
> };
> @@ -870,7 +868,6 @@
>
> vin1ep: endpoint {
> remote-endpoint = <&adv7180>;
> - bus-width = <8>;
> };
> };
> };
> diff --git a/arch/arm/boot/dts/r8a7791-porter.dts b/arch/arm/boot/dts/r8a7791-porter.dts
> index c14e6fe..055a7f1 100644
> --- a/arch/arm/boot/dts/r8a7791-porter.dts
> +++ b/arch/arm/boot/dts/r8a7791-porter.dts
> @@ -391,7 +391,6 @@
>
> vin0ep: endpoint {
> remote-endpoint = <&adv7180>;
> - bus-width = <8>;
> };
> };
> };
> diff --git a/arch/arm/boot/dts/r8a7793-gose.dts b/arch/arm/boot/dts/r8a7793-gose.dts
> index 9ed6961..9d3fba2 100644
> --- a/arch/arm/boot/dts/r8a7793-gose.dts
> +++ b/arch/arm/boot/dts/r8a7793-gose.dts
> @@ -759,10 +759,8 @@
>
> vin0ep2: endpoint {
> remote-endpoint = <&adv7612_out>;
> - bus-width = <24>;
> hsync-active = <0>;
> vsync-active = <0>;
> - pclk-sample = <1>;
> data-active = <1>;
> };
> };
> @@ -781,7 +779,6 @@
>
> vin1ep: endpoint {
> remote-endpoint = <&adv7180_out>;
> - bus-width = <8>;
> };
> };
> };
> diff --git a/arch/arm/boot/dts/r8a7794-alt.dts b/arch/arm/boot/dts/r8a7794-alt.dts
> index 26a8834..4bbb9cc 100644
> --- a/arch/arm/boot/dts/r8a7794-alt.dts
> +++ b/arch/arm/boot/dts/r8a7794-alt.dts
> @@ -380,7 +380,6 @@
>
> vin0ep: endpoint {
> remote-endpoint = <&adv7180>;
> - bus-width = <8>;
> };
> };
> };
> diff --git a/arch/arm/boot/dts/r8a7794-silk.dts b/arch/arm/boot/dts/r8a7794-silk.dts
> index 351cb3b..c0c5d31 100644
> --- a/arch/arm/boot/dts/r8a7794-silk.dts
> +++ b/arch/arm/boot/dts/r8a7794-silk.dts
> @@ -480,7 +480,6 @@
>
> vin0ep: endpoint {
> remote-endpoint = <&adv7180>;
> - bus-width = <8>;
> };
> };
> };
> --
> 2.7.4
>
--
Regards,
Niklas Söderlund
^ permalink raw reply
* Re: [PATCH v6 2/2] leds: lm3601x: Introduce the lm3601x LED driver
From: Andy Shevchenko @ 2018-05-16 22:11 UTC (permalink / raw)
To: Dan Murphy
Cc: Rob Herring, Mark Rutland, Jacek Anaszewski, Pavel Machek,
devicetree, Linux Kernel Mailing List, Linux LED Subsystem
In-Reply-To: <3a0f27b7-86a1-235c-0a96-5ec9c8feb6f0@ti.com>
On Wed, May 16, 2018 at 11:43 PM, Dan Murphy <dmurphy@ti.com> wrote:
> On 05/15/2018 05:24 PM, Andy Shevchenko wrote:
>> On Wed, May 16, 2018 at 1:08 AM, Dan Murphy <dmurphy@ti.com> wrote:
>>> On 05/15/2018 04:56 PM, Andy Shevchenko wrote:
>>>> On Tue, May 15, 2018 at 6:43 PM, Dan Murphy <dmurphy@ti.com> wrote:
>>>>> +static const struct lm3601x_max_timeouts strobe_timeouts[] = {
>>>>> + { 40000, 0x00 },
>>>>> + { 80000, 0x01 },
>>>>> + { 120000, 0x02 },
>>>>> + { 160000, 0x03 },
>>>>> + { 200000, 0x04 },
>>>>> + { 240000, 0x05 },
>>>>> + { 280000, 0x06 },
>>>>> + { 320000, 0x07 },
>>>>> + { 360000, 0x08 },
>>>>> + { 400000, 0x09 },
>>>>> + { 600000, 0x0a },
>>>>> + { 800000, 0x0b },
>>>>> + { 1000000, 0x0c },
>>>>> + { 1200000, 0x0d },
>>>>> + { 1400000, 0x0e },
>>>>> + { 1600000, 0x0f },
>>>>
>>>> Huh?!
>>>
>>> Please give comments that actually mean something other wise I will opt to ignore them.
>>
>> I did below.
>
> Sort of. "So what?" and "Huh" are not really a technical comments.
True.
> I can tell from your LVEE presentation that
> you have a passion for making the kernel better. That is awesome but if we try to make the code perfect everytime no code will
> ever get merged.
True.
> And we are human and we will miss coding issues and make mistakes.
True.
While all above is true, I still don't get why we have lookup tables
whenever we can use simple calculations.
>>>> strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC;
>>>
>>> Not sure what equation you are trying to point out here. But if you are trying to apply
>>> a timeout step you cannot do this with this part. As pointed out in the DT doc the timeout
>>> step is not linear.
>>
>> Yeah, I know people are more than often too lazy to think.
>
> Well this is why I put the table in. It is not laziness on my part as I try to add clarity for
> our newbies or productization engineers, translation/lookup tables are not as bad as you think.
Sometimes.
> I am fully aware that this bloats the data section of the image and has negative impacts
> on IoT small memory foot print devices.
>
>>
>> if (x < 9)
>> strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC;
>> else
>> strobe_timeout = (400 + (x - 9) * 200) * MSECS_IN_SEC;
>>
>
> Not sure if this will produce the register value I am looking for. I have to run through the
> algorithim. The idea is to take in the timeout and get the reg value to program.
>
> If it yields the expected output, or with a modified equation, then I can pull it in.
So, you need a reversed calculus in this case.
u8 regval;
/* To make it in range... */
timeout = clamp_val(timeout, 40000, 1600000);
/* ...other possibility to issue an error */
if (timeout >= 400000)
regval = timeout / 200000 + 0x07;
else
regval = timeout / 40000 - 0x01;
In any case, please double check it gives a correct values.
>>>>> + brightness_val = (brightness/2);
> I am still not see the extra spaces here.
Exactly, spaces are missed.
>> What do you mean? We do not appreciate +LOCs for no (or even nagative!) benefit.
> Again for non-IoT cases this would be fine but if we are striving to meet IoT size
> requirements I do understand your concern.
It has nothing with IoT in this case. The pattern
ret = foo();
if (ret)
return ret;
return 0;
is a too way verbose variant of
ret foo();
though I can imagine some rare cases where it might make sense.
OTOH the variant
ret = foo();
return ret;
pointless at all.
>>>>> + ret = of_property_read_string(led->led_node, "label", &name);
>>>> device_property_...();
>> Jacek, if you need rationale behind this comment it's here: the driver
>> has nothing DT specific and getting rid of OF centric programming
>> allows to reuse the driver on non-DT platforms w/o touching a source
>> code.
> I am deferring this to Jacek. Because this should be an overall coding change for all LED
> drivers that are new.
> Like I pointed out I don't mind adding/changing the code as long as
> all LED drivers will have that same mandate moving forward.
Whenever we might get driver working on different platforms for free,
better to do it that way. The entire exersice about unified device
properties API is to give drivers an agnostic way for getting their
resources / properties.
> If the maintainers agree we can get this driver to be the example for the use of device_property calls.
That's fair.
I dunno why Sakari made as3645a not in this way in the first place.
Will talk to him.
>>>>> + snprintf(led->led_name, sizeof(led->led_name),
>>>>> + "%s:%s", led->led_node->name, name);
>>>>> + else
>>>>> + snprintf(led->led_name, sizeof(led->led_name),
>>>>> + "%s:torch", led->led_node->name);
>>>>
>>>> const char *tmp;
>>>>
>>>> ret = device_property_read_...(&tmp);
>>>> if (ret)
>>>> tmp = ...
>>>> sprintf(...);
>>
>> No comments on this?
>
> No. This would depend on the disposition of the device_property calls.
> If we go that way then yes this will probably change. But we need to have an
> if..else as the label property is option in the DT documentation and we have to
> make a label if one does not exist.
The example is not related to device_property_ vs of_property_.
The idea is to figure out the label and use it in one place.
Though, I have no preferences of it.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH 2/6] dt-bindings: media: rcar-vin: Document data-active
From: Niklas Söderlund @ 2018-05-16 21:55 UTC (permalink / raw)
To: Jacopo Mondi
Cc: devicetree, robh+dt, linux-renesas-soc, horms, geert,
laurent.pinchart, linux-arm-kernel, linux-media
In-Reply-To: <1526488352-898-3-git-send-email-jacopo+renesas@jmondi.org>
Hi Jacopo,
Thanks for your work.
On 2018-05-16 18:32:28 +0200, Jacopo Mondi wrote:
> Document 'data-active' property in R-Car VIN device tree bindings.
> The property is optional when running with explicit synchronization
> (eg. BT.601) but mandatory when embedded synchronization is in use (eg.
> BT.656) as specified by the hardware manual.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
> Documentation/devicetree/bindings/media/rcar_vin.txt | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt b/Documentation/devicetree/bindings/media/rcar_vin.txt
> index c53ce4e..17eac8a 100644
> --- a/Documentation/devicetree/bindings/media/rcar_vin.txt
> +++ b/Documentation/devicetree/bindings/media/rcar_vin.txt
> @@ -63,6 +63,11 @@ from local SoC CSI-2 receivers (port1) depending on SoC.
> If both HSYNC and VSYNC polarities are not specified, embedded
> synchronization is selected.
>
> + - data-active: active state of data enable signal (CLOCKENB pin).
I'm not sure what you mean by active state here. video-interfaces.txt
defines data-active as 'similar to HSYNC and VSYNC, specifies data line
polarity' so I assume this is the polarity of the CLOCKENB pin?
> + 0/1 for LOW/HIGH respectively. If not specified, use HSYNC as
> + data enable signal. When using embedded synchronization this
> + property is mandatory.
I'm confused, why is this mandatory if we have no embedded sync (that is
hsync-active and vsync-active not defined)? I can't find any reference
to this in the Gen2 datasheet but I'm sure I'm just missing it :-)
> +
> - port 1 - sub-nodes describing one or more endpoints connected to
> the VIN from local SoC CSI-2 receivers. The endpoint numbers must
> use the following schema.
> --
> 2.7.4
>
--
Regards,
Niklas Söderlund
^ permalink raw reply
* Re: [PATCH v6] gpio: dwapb: Add support for 1 interrupt per port A GPIO
From: Hoan Tran @ 2018-05-16 21:40 UTC (permalink / raw)
To: Phil Edworthy, Andy Shevchenko, Hoan Tran, Linus Walleij,
Mark Rutland
Cc: Rob Herring, Lee Jones, Michel Pollet, linux-gpio@vger.kernel.org,
devicetree@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
linux-kernel@vger.kernel.org, Hoan Tran
In-Reply-To: <1526027497-32556-1-git-send-email-phil.edworthy@renesas.com>
Hi Phil,
On 5/11/18, 1:31 AM, "Phil Edworthy" <phil.edworthy@renesas.com> wrote:
The DesignWare GPIO IP can be configured for either 1 interrupt or 1
per GPIO in port A, but the driver currently only supports 1 interrupt.
See the DesignWare DW_apb_gpio Databook description of the
'GPIO_INTR_IO' parameter.
This change allows the driver to work with up to 32 interrupts, it will
get as many interrupts as specified in the DT 'interrupts' property.
It doesn't do anything clever with the different interrupts, it just calls
the same handler used for single interrupt hardware.
Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
Reviewed-by: Rob Herring <robh@kernel.org>
Acked-by: Lee Jones <lee.jones@linaro.org>
---
One point to mention is that I have made it possible for users to have
unconnected interrupts by specifying holes in the list of interrupts. This is
done by supporting the interrupts-extended DT prop.
However, I have no use for this and had to hack some test case for this.
Perhaps the driver should support 1 interrupt or all GPIOa as interrupts?
v6:
- Treat DT and ACPI the same as much as possible. Note that we can't use
platform_get_irq() to get the DT interrupts as they are in the port
sub-node and hence do not have an associated platform device.
v5:
- Rolled ACPI companion code provided by Hoan Tran into this patch.
v4:
- Use of_irq_get() instead of of_irq_parse_one()+irq_create_of_mapping()
v3:
- Rolled mfd: intel_quark_i2c_gpio fix into this patch to avoid bisect problems
v2:
- Replaced interrupt-mask DT prop with support for the interrupts-extended
prop. This means replacing the call to irq_of_parse_and_map() with calls
to of_irq_parse_one() and irq_create_of_mapping().
---
.../devicetree/bindings/gpio/snps-dwapb-gpio.txt | 9 +++-
drivers/gpio/gpio-dwapb.c | 49 +++++++++++++++-------
drivers/mfd/intel_quark_i2c_gpio.c | 3 +-
include/linux/platform_data/gpio-dwapb.h | 3 +-
4 files changed, 45 insertions(+), 19 deletions(-)
diff --git a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
index 4a75da7..3c1118b 100644
--- a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
@@ -26,8 +26,13 @@ controller.
the second encodes the triger flags encoded as described in
Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
- interrupt-parent : The parent interrupt controller.
-- interrupts : The interrupt to the parent controller raised when GPIOs
- generate the interrupts.
+- interrupts : The interrupts to the parent controller raised when GPIOs
+ generate the interrupts. If the controller provides one combined interrupt
+ for all GPIOs, specify a single interrupt. If the controller provides one
+ interrupt for each GPIO, provide a list of interrupts that correspond to each
+ of the GPIO pins. When specifying multiple interrupts, if any are unconnected,
+ use the interrupts-extended property to specify the interrupts and set the
+ interrupt controller handle for unused interrupts to 0.
- snps,nr-gpios : The number of pins in the port, a single cell.
- resets : Reset line for the controller.
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 226977f..15b4154 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -441,14 +441,19 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
irq_gc->chip_types[1].handler = handle_edge_irq;
if (!pp->irq_shared) {
- irq_set_chained_handler_and_data(pp->irq, dwapb_irq_handler,
- gpio);
+ int i;
+
+ for (i = 0; i < pp->ngpio; i++) {
+ if (pp->irq[i] >= 0)
+ irq_set_chained_handler_and_data(pp->irq[i],
+ dwapb_irq_handler, gpio);
+ }
} else {
/*
* Request a shared IRQ since where MFD would have devices
* using the same irq pin
*/
- err = devm_request_irq(gpio->dev, pp->irq,
+ err = devm_request_irq(gpio->dev, pp->irq[0],
dwapb_irq_handler_mfd,
IRQF_SHARED, "gpio-dwapb-mfd", gpio);
if (err) {
@@ -524,7 +529,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
if (pp->idx == 0)
port->gc.set_config = dwapb_gpio_set_config;
- if (pp->irq)
+ if (pp->has_irq)
dwapb_configure_irqs(gpio, port, pp);
err = gpiochip_add_data(&port->gc, port);
@@ -535,7 +540,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
port->is_registered = true;
/* Add GPIO-signaled ACPI event support */
- if (pp->irq)
+ if (pp->has_irq)
acpi_gpiochip_request_interrupts(&port->gc);
return err;
@@ -557,7 +562,7 @@ dwapb_gpio_get_pdata(struct device *dev)
struct dwapb_platform_data *pdata;
struct dwapb_port_property *pp;
int nports;
- int i;
+ int i, j;
nports = device_get_child_node_count(dev);
if (nports == 0)
@@ -575,6 +580,8 @@ dwapb_gpio_get_pdata(struct device *dev)
i = 0;
device_for_each_child_node(dev, fwnode) {
+ struct device_node *np = NULL;
+
pp = &pdata->properties[i++];
pp->fwnode = fwnode;
@@ -594,23 +601,35 @@ dwapb_gpio_get_pdata(struct device *dev)
pp->ngpio = 32;
}
+ pp->irq_shared = false;
+ pp->gpio_base = -1;
+
/*
* Only port A can provide interrupts in all configurations of
* the IP.
*/
- if (dev->of_node && pp->idx == 0 &&
- fwnode_property_read_bool(fwnode,
+ if (pp->idx != 0)
+ continue;
+
+ if (dev->of_node && fwnode_property_read_bool(fwnode,
"interrupt-controller")) {
- pp->irq = irq_of_parse_and_map(to_of_node(fwnode), 0);
- if (!pp->irq)
- dev_warn(dev, "no irq for port%d\n", pp->idx);
+ np = to_of_node(fwnode);
}
- if (has_acpi_companion(dev) && pp->idx == 0)
- pp->irq = platform_get_irq(to_platform_device(dev), 0);
+ for (j = 0; j < pp->ngpio; j++) {
+ pp->irq[j] = -ENXIO;
- pp->irq_shared = false;
- pp->gpio_base = -1;
+ if (np)
+ pp->irq[j] = of_irq_get(np, j);
+ else if (has_acpi_companion(dev))
+ pp->irq[j] = platform_get_irq(to_platform_device(dev), j);
+
+ if (pp->irq[j] >= 0)
+ pp->has_irq = true;
+ }
+
+ if (!pp->has_irq)
+ dev_warn(dev, "no irq for port%d\n", pp->idx);
}
return pdata;
diff --git a/drivers/mfd/intel_quark_i2c_gpio.c b/drivers/mfd/intel_quark_i2c_gpio.c
index 90e35de..5bddb84 100644
--- a/drivers/mfd/intel_quark_i2c_gpio.c
+++ b/drivers/mfd/intel_quark_i2c_gpio.c
@@ -233,7 +233,8 @@ static int intel_quark_gpio_setup(struct pci_dev *pdev, struct mfd_cell *cell)
pdata->properties->idx = 0;
pdata->properties->ngpio = INTEL_QUARK_MFD_NGPIO;
pdata->properties->gpio_base = INTEL_QUARK_MFD_GPIO_BASE;
- pdata->properties->irq = pdev->irq;
+ pdata->properties->irq[0] = pdev->irq;
+ pdata->properties->has_irq = true;
pdata->properties->irq_shared = true;
cell->platform_data = pdata;
diff --git a/include/linux/platform_data/gpio-dwapb.h b/include/linux/platform_data/gpio-dwapb.h
index 2dc7f4a..419cfac 100644
--- a/include/linux/platform_data/gpio-dwapb.h
+++ b/include/linux/platform_data/gpio-dwapb.h
@@ -19,7 +19,8 @@ struct dwapb_port_property {
unsigned int idx;
unsigned int ngpio;
unsigned int gpio_base;
- unsigned int irq;
+ int irq[32];
+ bool has_irq;
bool irq_shared;
};
Acked-by: Hoan Tran <hoan.tran@amperecomputing.com>
--
2.7.4
^ permalink raw reply
* Re: [RFC PATCH V2] scsi: ufs: Add specific callback for setting DMA mask
From: Subhash Jadavani @ 2018-05-16 21:31 UTC (permalink / raw)
To: Alim Akhtar
Cc: Alim Akhtar, linux-scsi, linux-kernel, jejb, martin.petersen,
vinayak holikatti, devicetree, Shaik Ameer Basha
In-Reply-To: <CAGOxZ52yZ8Xqype7AVsH43DWOA2byh2kW3SGsfHyHtzqSH6QPw@mail.gmail.com>
On 2018-05-15 21:31, Alim Akhtar wrote:
> Ping !!!
>
> On Thu, Mar 8, 2018 at 4:33 PM, Alim Akhtar <alim.akhtar@samsung.com>
> wrote:
>> Currently DMA mask for UFS HCI is set by reading CAP register's
>> [64AS] bit. Some HCI controller like Exynos support 36-bit bus
>> address.
>> This works perfectly fine with DMA mask set as 64 in case there is no
>> IOMMU attached to HCI.
>> In case if HCI is behind an IOMMU, setting DMA mask as 64 bit won't
>> work as HCI has only 36bit addressing and SMMU has created mapping of
>> 64 bit and as the device truncates the address, its mapping will not
>> be found by iommu.
>> To resolve such issues, let the variant driver sets its own DMA mask.
>>
>> Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
>> ---
>> drivers/scsi/ufs/ufshcd.c | 3 +++
>> drivers/scsi/ufs/ufshcd.h | 2 ++
>> 2 files changed, 5 insertions(+)
>>
>> I am not sure if there are other ways available to handle such cases.
>> The IOMMU I am talking about is arm-smmu and it DT binding does not
>> give much idea about handling such cases.
>> Have tested this patch with HCI controller with IOMMU attached.
>>
>> Changes Since V1:
>> - Fixed build issue as reported by Kbuild test robot.
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index a355d98..9a1374e 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -7781,6 +7781,9 @@ EXPORT_SYMBOL_GPL(ufshcd_dealloc_host);
>> */
>> static int ufshcd_set_dma_mask(struct ufs_hba *hba)
>> {
>> + if (hba->vops && hba->vops->set_dma_mask)
>> + return hba->vops->set_dma_mask(hba);
>> +
>> if (hba->capabilities & MASK_64_ADDRESSING_SUPPORT) {
>> if (!dma_set_mask_and_coherent(hba->dev,
>> DMA_BIT_MASK(64)))
>> return 0;
>> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
>> index 1332e54..89c6dae 100644
>> --- a/drivers/scsi/ufs/ufshcd.h
>> +++ b/drivers/scsi/ufs/ufshcd.h
>> @@ -297,6 +297,7 @@ struct ufs_pwr_mode_info {
>> * @resume: called during host controller PM callback
>> * @dbg_register_dump: used to dump controller debug information
>> * @phy_initialization: used to initialize phys
>> + * @set_dma_mask: used to set variant specific DMA mask
>> */
>> struct ufs_hba_variant_ops {
>> const char *name;
>> @@ -325,6 +326,7 @@ struct ufs_hba_variant_ops {
>> int (*resume)(struct ufs_hba *, enum ufs_pm_op);
>> void (*dbg_register_dump)(struct ufs_hba *hba);
>> int (*phy_initialization)(struct ufs_hba *);
>> + int (*set_dma_mask)(struct ufs_hba *hba);
>> };
>>
>> /* clock gating state */
>> --
>> 2.7.4
>>
Looks reasonable to me, you may try posting non-RFC version.
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH] arm64: dts: juno: fix graph node unit addresses for coresight components
From: Mathieu Poirier @ 2018-05-16 21:30 UTC (permalink / raw)
To: Sudeep Holla; +Cc: devicetree, Liviu Dudau, linux-arm-kernel, Suzuki K Poulose
In-Reply-To: <dd4796b7-a078-27db-0fdc-099a834ce8bc@arm.com>
On 16 May 2018 at 11:40, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>
> On 16/05/18 18:29, Mathieu Poirier wrote:
>> On 16 May 2018 at 05:49, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>>
>>>
>>> On 16/05/18 12:23, Suzuki K Poulose wrote:
>>>> On 05/16/2018 11:34 AM, Sudeep Holla wrote:
>>>
>>> [..]
>>>
>>>>> Hi Suzuki/Mathieu,
>>>>>
>>>>> I did a quick scan @ drivers/hwtracing/coresight/of_coresight.c to
>>>>> check if reg field is being used or not and whether this change
>>>>> causes any regression. I don't think so, but I may be wrong, let me
>>>>> know.
>>>>
>>>> Unfortunately, I think this would break the components like funnel,
>>>> where we need the input port number for the connected master to enable
>>>> the port. Similarly for the output port number for master components in
>>>> the paths. I have a set of patches which address this by taking care of
>>>> the port number order to find out the hardware port number.
>>>>
>>>
>>> Ah ok, I now see of_graph_parse_endpoint, sorry for missing that.
>>
>> The problem is not with of_graph_parse_endpoint(), that will work just
>> fine. In fact you can add whatever number you want there without
>> impact on how devices see each other in the framework. The problem is
>> that the port numbering doesn't reflect the HW layout anymore and as
>> such you can't rely on the port value when configuring the HW.
>>
>
> Exactly, I had a chat with Suzuki. One option is to introduce new hwid
> field to fix that. If the field is present, it will be used and reg
> field will be ignored. In that way, you won't break the compatibility.
I had the same idea a while back and something Grant Likely and I
talked about at ELC-E 2017 in Prague. This is the way I would prefer
to move forward on this.
> Just my thoughts. I am not sure if we need to support old kernel with
> updated DTs though. That may be difficult.
Not sure that can be done. The only way to do that would be to
continue using the reg property, which isn't possible anymore. As
such said DT wouldn't be upstream and I'm not sure we need to care
about those.
>
>>>
>>>> I will dust it up and send it. That would bring up another important
>>>> question.
>>>>
>>>
>>> Cool
>>>
>>>> How do we deal with the change in the port number scheme ? e.g, should
>>>> the new kernel support DTBs with old scheme ?
>>>
>>
>> DT files following the old scheme will spew out warnings like we're
>> getting on Juno and are bound to be fixed.
>>
> True, they can pick up the new changes too along with changes just
> fixing the warning.
>
>>> IIUC, that's needed for backward compatibility as it was used schema.
>>> Again I may be wrong.
>>>
>>>> If so, how do we specify that the DT uses new scheme.
>>>
>>> Perhaps, add something to indicate the change in numbering scheme ?
>>
>> The current customers should be moved to the new scheme. That way we
>> don't have to support two different DT scheme (where one will die
>> eventually).
>>
>
> But the problem is if someone has DT which can't be modified and we move
> to new scheme only in the kernel, then they will see regression on their
> platforms. If that's fine, it simplifies things a lot. But I am not so
> sure of that ...
You have a point. The code could check for both reg and hwid
properties. If both are there hwid is preferred. Otherwise the reg
property is used. That wouldn't be too difficult to implement and
avoid introducing a regression. Of course all this is fantasy until
we actually try things out.
Thanks,
Mathieu
>
> --
> Regards,
> Sudeep
^ permalink raw reply
* Re: [PATCH v6 2/2] leds: lm3601x: Introduce the lm3601x LED driver
From: Jacek Anaszewski @ 2018-05-16 21:27 UTC (permalink / raw)
To: Dan Murphy, Andy Shevchenko
Cc: Rob Herring, Mark Rutland, Pavel Machek, devicetree,
Linux Kernel Mailing List, Linux LED Subsystem
In-Reply-To: <f155e9da-e41d-1201-a9f9-c321654be42e@ti.com>
Dan,
On 05/16/2018 11:17 PM, Dan Murphy wrote:
> Jacek and Andy
>
> On 05/16/2018 04:13 PM, Dan Murphy wrote:
>> Jacek and Andy
>>
>> On 05/16/2018 04:02 PM, Jacek Anaszewski wrote:
>>> Hi Andy and Dan,
>>>
>>
>> I will make all the changes then. I don't want to go through and ack each one.
>>
>
> Let me clarify. I will make all the change Jacek has guided on. There is still a
> terminator comma vs no comma comment that needs disposition at the end of this file.
No comma option seems better in this case.
>> Thanks for the guidance and the reviews.
>>
>> It will take a couple days to find all the comments and get this all fixed up.
>>
>> Dan
>>
>>> On 05/16/2018 12:24 AM, Andy Shevchenko wrote:
>>>> On Wed, May 16, 2018 at 1:08 AM, Dan Murphy <dmurphy@ti.com> wrote:
>>>>> On 05/15/2018 04:56 PM, Andy Shevchenko wrote:
>>>>>> On Tue, May 15, 2018 at 6:43 PM, Dan Murphy <dmurphy@ti.com> wrote:
>>>>
>>>>>>> + depends on LEDS_CLASS && I2C && OF
>>>>>>
>>>>>> What is OF specific in this driver?
>>>>>
>>>>> as3645a_led_class_setup has a "of" dependency
>>>>
>>>> So what? Is it called from this driver or?
>>>>
>>>>
>>>>>>> +static const struct lm3601x_max_timeouts strobe_timeouts[] = {
>>>>>>> + { 40000, 0x00 },
>>>>>>> + { 80000, 0x01 },
>>>>>>> + { 120000, 0x02 },
>>>>>>> + { 160000, 0x03 },
>>>>>>> + { 200000, 0x04 },
>>>>>>> + { 240000, 0x05 },
>>>>>>> + { 280000, 0x06 },
>>>>>>> + { 320000, 0x07 },
>>>>>>> + { 360000, 0x08 },
>>>>>>> + { 400000, 0x09 },
>>>>>>> + { 600000, 0x0a },
>>>>>>> + { 800000, 0x0b },
>>>>>>> + { 1000000, 0x0c },
>>>>>>> + { 1200000, 0x0d },
>>>>>>> + { 1400000, 0x0e },
>>>>>>> + { 1600000, 0x0f },
>>>>>>
>>>>>> Huh?!
>>>>>
>>>>> Please give comments that actually mean something other wise I will opt to ignore them.
>>>>
>>>> I did below.
>>>>
>>>>>> strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC;
>>>>>
>>>>> Not sure what equation you are trying to point out here. But if you are trying to apply
>>>>> a timeout step you cannot do this with this part. As pointed out in the DT doc the timeout
>>>>> step is not linear.
>>>>
>>>> Yeah, I know people are more than often too lazy to think.
>>>>
>>>> if (x < 9)
>>>> strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC;
>>>> else
>>>> strobe_timeout = (400 + (x - 9) * 200) * MSECS_IN_SEC;
>>>>
>>>
>>> I like the idea.
>>>
>>>>>>> + brightness_val = (brightness/2);
>>>>>>
>>>>>> Spaces.
>>>>>
>>>>> Not sure what this means checkpatch was clean
>>>>
>>>> Even besides missed whispaces it has redundant parens.
>>>>
>>>> checkpatch is not a silver bullet to get your code clean and nice.
>>>>
>>>>>> This is return led_...();
>>>>>
>>>>> That is a preference. It does not have to be that way.
>>>
>>> I missed that. Dan, please follow Andy's advise.
>>>
>>>>
>>>> What do you mean? We do not appreciate +LOCs for no (or even nagative!) benefit.
>>>>
>>>>>>> + ret = of_property_read_string(led->led_node, "label", &name);
>>>>>>
>>>>>> device_property_...();
>>>>>
>>>>> It can be if the maintainer is requesting this.
>>>>
>>>> Jacek, if you need rationale behind this comment it's here: the driver
>>>> has nothing DT specific and getting rid of OF centric programming
>>>> allows to reuse the driver on non-DT platforms w/o touching a source
>>>> code.
>>>
>>> It has an added value, so yes, let's use it as a standard approach
>>> from now on.
>>>
>>>>> Is the trend to move to these functions?
>>>>
>>>> See above.
>>>>
>>>>> Most drivers use the "of" calls.
>>>>
>>>> So what?
>>>>
>>>>
>>>>>>> + if (!ret)
>>>>>>
>>>>>> if (ret) sounds more natural. And better just to split
>>>>>>
>>>>>>> + snprintf(led->led_name, sizeof(led->led_name),
>>>>>>> + "%s:%s", led->led_node->name, name);
>>>>>>> + else
>>>>>>> + snprintf(led->led_name, sizeof(led->led_name),
>>>>>>> + "%s:torch", led->led_node->name);
>>>>>>
>>>>>> const char *tmp;
>>>>>>
>>>>>> ret = device_property_read_...(&tmp);
>>>>>> if (ret)
>>>>>> tmp = ...
>>>>>> sprintf(...);
>>>
>>> We're no longer taking devicename section of a LED class device name
>>> from DT, so it will look differently anyway.
>>>
>>>> No comments on this?
>>>>
>>>>>>> + led = devm_kzalloc(&client->dev,
>>>>>>> + sizeof(struct lm3601x_led), GFP_KERNEL);
>>>>>>
>>>>>> sizeof(*led) and one line in the result
>>>>
>>>> And this?
>>>
>>> Ack.
>>>
>>>>
>>>>>>> + { },
>>>>>>
>>>>>> Terminators better w/o comma.
>>>>>
>>>>> Looking at other drivers adding comma's on the sentinel is accepted. See the as3645a driver
>>>>
>>>> So what?
>>>>
>>>> Terminator at compile time even better.
>>>>
>>>>>>> + {},
>>>>>>
>>>>>> Ditto.
>>>>>
>>>>> See above
>>>>
>>>> See above.
>>>>
>>>
>>
>>
>
>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply
* Re: [PATCH 1/6] dt-bindings: media: rcar-vin: Describe optional ep properties
From: Niklas Söderlund @ 2018-05-16 21:18 UTC (permalink / raw)
To: Jacopo Mondi
Cc: devicetree, robh+dt, linux-renesas-soc, horms, geert,
laurent.pinchart, linux-arm-kernel, linux-media
In-Reply-To: <1526488352-898-2-git-send-email-jacopo+renesas@jmondi.org>
Hi Jacopo,
Thanks for your patch.
On 2018-05-16 18:32:27 +0200, Jacopo Mondi wrote:
> Describe the optional endpoint properties for endpoint nodes of port@0
> and port@1 of the R-Car VIN driver device tree bindings documentation.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
> Documentation/devicetree/bindings/media/rcar_vin.txt | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt b/Documentation/devicetree/bindings/media/rcar_vin.txt
> index a19517e1..c53ce4e 100644
> --- a/Documentation/devicetree/bindings/media/rcar_vin.txt
> +++ b/Documentation/devicetree/bindings/media/rcar_vin.txt
> @@ -53,6 +53,16 @@ from local SoC CSI-2 receivers (port1) depending on SoC.
> from external SoC pins described in video-interfaces.txt[1].
> Describing more then one endpoint in port 0 is invalid. Only VIN
> instances that are connected to external pins should have port 0.
> +
> + - Optional properties for endpoint nodes of port@0:
> + - hsync-active: active state of the HSYNC signal, 0/1 for LOW/HIGH
> + respectively. Default is active high.
> + - vsync-active: active state of the VSYNC signal, 0/1 for LOW/HIGH
> + respectively. Default is active high.
> +
> + If both HSYNC and VSYNC polarities are not specified, embedded
> + synchronization is selected.
> +
> - port 1 - sub-nodes describing one or more endpoints connected to
> the VIN from local SoC CSI-2 receivers. The endpoint numbers must
> use the following schema.
> @@ -62,6 +72,8 @@ from local SoC CSI-2 receivers (port1) depending on SoC.
> - Endpoint 2 - sub-node describing the endpoint connected to CSI40
> - Endpoint 3 - sub-node describing the endpoint connected to CSI41
>
> + Endpoint nodes of port@1 do not support any optional endpoint property.
> +
> Device node example for Gen2 platforms
> --------------------------------------
>
> @@ -112,7 +124,6 @@ Board setup example for Gen2 platforms (vin1 composite video input)
>
> vin1ep0: endpoint {
> remote-endpoint = <&adv7180>;
> - bus-width = <8>;
> };
> };
> };
> --
> 2.7.4
>
--
Regards,
Niklas Söderlund
^ permalink raw reply
* Re: [PATCH v6 2/2] leds: lm3601x: Introduce the lm3601x LED driver
From: Dan Murphy @ 2018-05-16 21:17 UTC (permalink / raw)
To: Jacek Anaszewski, Andy Shevchenko
Cc: Rob Herring, Mark Rutland, Pavel Machek, devicetree,
Linux Kernel Mailing List, Linux LED Subsystem
In-Reply-To: <52380a68-9e33-f52a-134d-5d4a928b5383@ti.com>
Jacek and Andy
On 05/16/2018 04:13 PM, Dan Murphy wrote:
> Jacek and Andy
>
> On 05/16/2018 04:02 PM, Jacek Anaszewski wrote:
>> Hi Andy and Dan,
>>
>
> I will make all the changes then. I don't want to go through and ack each one.
>
Let me clarify. I will make all the change Jacek has guided on. There is still a
terminator comma vs no comma comment that needs disposition at the end of this file.
Dan
> Thanks for the guidance and the reviews.
>
> It will take a couple days to find all the comments and get this all fixed up.
>
> Dan
>
>> On 05/16/2018 12:24 AM, Andy Shevchenko wrote:
>>> On Wed, May 16, 2018 at 1:08 AM, Dan Murphy <dmurphy@ti.com> wrote:
>>>> On 05/15/2018 04:56 PM, Andy Shevchenko wrote:
>>>>> On Tue, May 15, 2018 at 6:43 PM, Dan Murphy <dmurphy@ti.com> wrote:
>>>
>>>>>> + depends on LEDS_CLASS && I2C && OF
>>>>>
>>>>> What is OF specific in this driver?
>>>>
>>>> as3645a_led_class_setup has a "of" dependency
>>>
>>> So what? Is it called from this driver or?
>>>
>>>
>>>>>> +static const struct lm3601x_max_timeouts strobe_timeouts[] = {
>>>>>> + { 40000, 0x00 },
>>>>>> + { 80000, 0x01 },
>>>>>> + { 120000, 0x02 },
>>>>>> + { 160000, 0x03 },
>>>>>> + { 200000, 0x04 },
>>>>>> + { 240000, 0x05 },
>>>>>> + { 280000, 0x06 },
>>>>>> + { 320000, 0x07 },
>>>>>> + { 360000, 0x08 },
>>>>>> + { 400000, 0x09 },
>>>>>> + { 600000, 0x0a },
>>>>>> + { 800000, 0x0b },
>>>>>> + { 1000000, 0x0c },
>>>>>> + { 1200000, 0x0d },
>>>>>> + { 1400000, 0x0e },
>>>>>> + { 1600000, 0x0f },
>>>>>
>>>>> Huh?!
>>>>
>>>> Please give comments that actually mean something other wise I will opt to ignore them.
>>>
>>> I did below.
>>>
>>>>> strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC;
>>>>
>>>> Not sure what equation you are trying to point out here. But if you are trying to apply
>>>> a timeout step you cannot do this with this part. As pointed out in the DT doc the timeout
>>>> step is not linear.
>>>
>>> Yeah, I know people are more than often too lazy to think.
>>>
>>> if (x < 9)
>>> strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC;
>>> else
>>> strobe_timeout = (400 + (x - 9) * 200) * MSECS_IN_SEC;
>>>
>>
>> I like the idea.
>>
>>>>>> + brightness_val = (brightness/2);
>>>>>
>>>>> Spaces.
>>>>
>>>> Not sure what this means checkpatch was clean
>>>
>>> Even besides missed whispaces it has redundant parens.
>>>
>>> checkpatch is not a silver bullet to get your code clean and nice.
>>>
>>>>> This is return led_...();
>>>>
>>>> That is a preference. It does not have to be that way.
>>
>> I missed that. Dan, please follow Andy's advise.
>>
>>>
>>> What do you mean? We do not appreciate +LOCs for no (or even nagative!) benefit.
>>>
>>>>>> + ret = of_property_read_string(led->led_node, "label", &name);
>>>>>
>>>>> device_property_...();
>>>>
>>>> It can be if the maintainer is requesting this.
>>>
>>> Jacek, if you need rationale behind this comment it's here: the driver
>>> has nothing DT specific and getting rid of OF centric programming
>>> allows to reuse the driver on non-DT platforms w/o touching a source
>>> code.
>>
>> It has an added value, so yes, let's use it as a standard approach
>> from now on.
>>
>>>> Is the trend to move to these functions?
>>>
>>> See above.
>>>
>>>> Most drivers use the "of" calls.
>>>
>>> So what?
>>>
>>>
>>>>>> + if (!ret)
>>>>>
>>>>> if (ret) sounds more natural. And better just to split
>>>>>
>>>>>> + snprintf(led->led_name, sizeof(led->led_name),
>>>>>> + "%s:%s", led->led_node->name, name);
>>>>>> + else
>>>>>> + snprintf(led->led_name, sizeof(led->led_name),
>>>>>> + "%s:torch", led->led_node->name);
>>>>>
>>>>> const char *tmp;
>>>>>
>>>>> ret = device_property_read_...(&tmp);
>>>>> if (ret)
>>>>> tmp = ...
>>>>> sprintf(...);
>>
>> We're no longer taking devicename section of a LED class device name
>> from DT, so it will look differently anyway.
>>
>>> No comments on this?
>>>
>>>>>> + led = devm_kzalloc(&client->dev,
>>>>>> + sizeof(struct lm3601x_led), GFP_KERNEL);
>>>>>
>>>>> sizeof(*led) and one line in the result
>>>
>>> And this?
>>
>> Ack.
>>
>>>
>>>>>> + { },
>>>>>
>>>>> Terminators better w/o comma.
>>>>
>>>> Looking at other drivers adding comma's on the sentinel is accepted. See the as3645a driver
>>>
>>> So what?
>>>
>>> Terminator at compile time even better.
>>>
>>>>>> + {},
>>>>>
>>>>> Ditto.
>>>>
>>>> See above
>>>
>>> See above.
>>>
>>
>
>
--
------------------
Dan Murphy
^ permalink raw reply
* Re: [PATCH v5 4/4] rtc: ds1307: add frequency_test_enable sysfs attribute to check tick on m41txx
From: Giulio Benetti @ 2018-05-16 21:15 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Andy Shevchenko, Alessandro Zummo, Rob Herring, Mark Rutland,
linux-rtc, devicetree, Linux Kernel Mailing List
In-Reply-To: <20180516211001.GC24496@piout.net>
Hi,
Il 16/05/2018 23:10, Alexandre Belloni ha scritto:
> On 16/05/2018 23:02:16+0200, Giulio Benetti wrote:
>>>> +static ssize_t frequency_test_enable_show(struct device *dev,
>>>> + struct device_attribute *attr,
>>>> + char *buf)
>>>> +{
>>>
>>>> + int freq_test_en = 0;
>>>
>>>> + if (ctrl_reg & M41TXX_BIT_FT)
>>>> + freq_test_en = true;
>>>> + else
>>>> + freq_test_en = false;
>>>> +
>>>> + return sprintf(buf, "%d\n", freq_test_en);
>>>
>>> So, is it boolean or integer? This code makes it confusing a lot.
>>
>> It is a boolean, so now I've updated with this:
>>
>> if (ctrl_reg & M41TXX_BIT_FT)
>> return scnprintf(buf, PAGE_SIZE, "on\n");
>> else
>> return scnprintf(buf, PAGE_SIZE, "off\n");
>>
>
> No, it has to be consistent with what you write.
>
> Here, you'd write 0 or 1 in the file and read off or on...
I'm submitting too many patches, without waiting, sorry.
I've submitted v6 patchset(too early), can you please take a look there
and tell if I'm on the right path? On that I've change also _store
function with kstrtobool().
I'm sorry I'm doing so much confusion.
I'm trying to learn to make it the best way.
>
>
^ permalink raw reply
* Re: [PATCH v6 2/2] leds: lm3601x: Introduce the lm3601x LED driver
From: Dan Murphy @ 2018-05-16 21:13 UTC (permalink / raw)
To: Jacek Anaszewski, Andy Shevchenko
Cc: Rob Herring, Mark Rutland, Pavel Machek, devicetree,
Linux Kernel Mailing List, Linux LED Subsystem
In-Reply-To: <f46e4b54-c463-6ef0-4155-a5dfb9dc4c2b@gmail.com>
Jacek and Andy
On 05/16/2018 04:02 PM, Jacek Anaszewski wrote:
> Hi Andy and Dan,
>
I will make all the changes then. I don't want to go through and ack each one.
Thanks for the guidance and the reviews.
It will take a couple days to find all the comments and get this all fixed up.
Dan
> On 05/16/2018 12:24 AM, Andy Shevchenko wrote:
>> On Wed, May 16, 2018 at 1:08 AM, Dan Murphy <dmurphy@ti.com> wrote:
>>> On 05/15/2018 04:56 PM, Andy Shevchenko wrote:
>>>> On Tue, May 15, 2018 at 6:43 PM, Dan Murphy <dmurphy@ti.com> wrote:
>>
>>>>> + depends on LEDS_CLASS && I2C && OF
>>>>
>>>> What is OF specific in this driver?
>>>
>>> as3645a_led_class_setup has a "of" dependency
>>
>> So what? Is it called from this driver or?
>>
>>
>>>>> +static const struct lm3601x_max_timeouts strobe_timeouts[] = {
>>>>> + { 40000, 0x00 },
>>>>> + { 80000, 0x01 },
>>>>> + { 120000, 0x02 },
>>>>> + { 160000, 0x03 },
>>>>> + { 200000, 0x04 },
>>>>> + { 240000, 0x05 },
>>>>> + { 280000, 0x06 },
>>>>> + { 320000, 0x07 },
>>>>> + { 360000, 0x08 },
>>>>> + { 400000, 0x09 },
>>>>> + { 600000, 0x0a },
>>>>> + { 800000, 0x0b },
>>>>> + { 1000000, 0x0c },
>>>>> + { 1200000, 0x0d },
>>>>> + { 1400000, 0x0e },
>>>>> + { 1600000, 0x0f },
>>>>
>>>> Huh?!
>>>
>>> Please give comments that actually mean something other wise I will opt to ignore them.
>>
>> I did below.
>>
>>>> strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC;
>>>
>>> Not sure what equation you are trying to point out here. But if you are trying to apply
>>> a timeout step you cannot do this with this part. As pointed out in the DT doc the timeout
>>> step is not linear.
>>
>> Yeah, I know people are more than often too lazy to think.
>>
>> if (x < 9)
>> strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC;
>> else
>> strobe_timeout = (400 + (x - 9) * 200) * MSECS_IN_SEC;
>>
>
> I like the idea.
>
>>>>> + brightness_val = (brightness/2);
>>>>
>>>> Spaces.
>>>
>>> Not sure what this means checkpatch was clean
>>
>> Even besides missed whispaces it has redundant parens.
>>
>> checkpatch is not a silver bullet to get your code clean and nice.
>>
>>>> This is return led_...();
>>>
>>> That is a preference. It does not have to be that way.
>
> I missed that. Dan, please follow Andy's advise.
>
>>
>> What do you mean? We do not appreciate +LOCs for no (or even nagative!) benefit.
>>
>>>>> + ret = of_property_read_string(led->led_node, "label", &name);
>>>>
>>>> device_property_...();
>>>
>>> It can be if the maintainer is requesting this.
>>
>> Jacek, if you need rationale behind this comment it's here: the driver
>> has nothing DT specific and getting rid of OF centric programming
>> allows to reuse the driver on non-DT platforms w/o touching a source
>> code.
>
> It has an added value, so yes, let's use it as a standard approach
> from now on.
>
>>> Is the trend to move to these functions?
>>
>> See above.
>>
>>> Most drivers use the "of" calls.
>>
>> So what?
>>
>>
>>>>> + if (!ret)
>>>>
>>>> if (ret) sounds more natural. And better just to split
>>>>
>>>>> + snprintf(led->led_name, sizeof(led->led_name),
>>>>> + "%s:%s", led->led_node->name, name);
>>>>> + else
>>>>> + snprintf(led->led_name, sizeof(led->led_name),
>>>>> + "%s:torch", led->led_node->name);
>>>>
>>>> const char *tmp;
>>>>
>>>> ret = device_property_read_...(&tmp);
>>>> if (ret)
>>>> tmp = ...
>>>> sprintf(...);
>
> We're no longer taking devicename section of a LED class device name
> from DT, so it will look differently anyway.
>
>> No comments on this?
>>
>>>>> + led = devm_kzalloc(&client->dev,
>>>>> + sizeof(struct lm3601x_led), GFP_KERNEL);
>>>>
>>>> sizeof(*led) and one line in the result
>>
>> And this?
>
> Ack.
>
>>
>>>>> + { },
>>>>
>>>> Terminators better w/o comma.
>>>
>>> Looking at other drivers adding comma's on the sentinel is accepted. See the as3645a driver
>>
>> So what?
>>
>> Terminator at compile time even better.
>>
>>>>> + {},
>>>>
>>>> Ditto.
>>>
>>> See above
>>
>> See above.
>>
>
--
------------------
Dan Murphy
^ permalink raw reply
* Re: [PATCH v5 4/4] rtc: ds1307: add frequency_test_enable sysfs attribute to check tick on m41txx
From: Alexandre Belloni @ 2018-05-16 21:10 UTC (permalink / raw)
To: Giulio Benetti
Cc: Andy Shevchenko, Alessandro Zummo, Rob Herring, Mark Rutland,
linux-rtc, devicetree, Linux Kernel Mailing List
In-Reply-To: <836407b4-8c51-5efa-8e29-f487bbcb528d@micronovasrl.com>
On 16/05/2018 23:02:16+0200, Giulio Benetti wrote:
> > > +static ssize_t frequency_test_enable_show(struct device *dev,
> > > + struct device_attribute *attr,
> > > + char *buf)
> > > +{
> >
> > > + int freq_test_en = 0;
> >
> > > + if (ctrl_reg & M41TXX_BIT_FT)
> > > + freq_test_en = true;
> > > + else
> > > + freq_test_en = false;
> > > +
> > > + return sprintf(buf, "%d\n", freq_test_en);
> >
> > So, is it boolean or integer? This code makes it confusing a lot.
>
> It is a boolean, so now I've updated with this:
>
> if (ctrl_reg & M41TXX_BIT_FT)
> return scnprintf(buf, PAGE_SIZE, "on\n");
> else
> return scnprintf(buf, PAGE_SIZE, "off\n");
>
No, it has to be consistent with what you write.
Here, you'd write 0 or 1 in the file and read off or on...
--
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH v6 4/4] rtc: ds1307: add frequency_test_enable sysfs attribute to check tick on m41txx
From: Giulio Benetti @ 2018-05-16 21:09 UTC (permalink / raw)
To: a.zummo, alexandre.belloni
Cc: robh+dt, mark.rutland, linux-rtc, devicetree, linux-kernel,
andy.shevchenko
In-Reply-To: <20180516210705.5824-4-giulio.benetti@micronovasrl.com>
Sorry, no V5 => V6 changelog, I resubmit.
Il 16/05/2018 23:07, Giulio Benetti ha scritto:
> On m41txx you can enable open-drain OUT pin to check if offset is ok.
> Enabling OUT pin with frequency_test_enable attribute, OUT pin will tick
> 512 times faster than 1s tick base.
>
> Enable or Disable FT bit on CONTROL register if freq_test is 1 or 0.
>
> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> ---
> drivers/rtc/rtc-ds1307.c | 96 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 96 insertions(+)
>
> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index 2797d01bfa1d..b2e87845d860 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c
> @@ -1053,6 +1053,96 @@ static int m41txx_rtc_set_offset(struct device *dev, long offset)
> ctrl_reg);
> }
>
> +static ssize_t frequency_test_enable_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct ds1307 *ds1307 = dev_get_drvdata(dev);
> + bool freq_test_en = 0;
> + int ret;
> +
> + ret = kstrtobool(buf, &freq_test_en);
> + if (ret == -EINVAL) {
> + dev_err(dev, "Failed to store RTC Frequency Test attribute\n");
> + return ret;
> + }
> +
> + regmap_update_bits(ds1307->regmap, M41TXX_REG_CONTROL, M41TXX_BIT_FT,
> + freq_test_en ? M41TXX_BIT_FT : 0);
> +
> + return count;
> +}
> +
> +static ssize_t frequency_test_enable_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct ds1307 *ds1307 = dev_get_drvdata(dev);
> + unsigned int ctrl_reg;
> +
> + regmap_read(ds1307->regmap, M41TXX_REG_CONTROL, &ctrl_reg);
> +
> + if (ctrl_reg & M41TXX_BIT_FT)
> + return scnprintf(buf, PAGE_SIZE, "on\n");
> + else
> + return scnprintf(buf, PAGE_SIZE, "off\n");
> +}
> +
> +static DEVICE_ATTR_RW(frequency_test_enable);
> +
> +static struct attribute *rtc_freq_test_attrs[] = {
> + &dev_attr_frequency_test_enable.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group rtc_freq_test_attr_group = {
> + .attrs = rtc_freq_test_attrs,
> +};
> +
> +static void rtc_calib_remove_sysfs_group(void *_dev)
> +{
> + struct device *dev = _dev;
> +
> + sysfs_remove_group(&dev->kobj, &rtc_freq_test_attr_group);
> +}
> +
> +static int ds1307_add_frequency_test(struct ds1307 *ds1307)
> +{
> + int err = 0;
> +
> + switch (ds1307->type) {
> + case m41t0:
> + case m41t00:
> + case m41t11:
> + /* Export sysfs entries */
> + err = sysfs_create_group(&(ds1307->dev)->kobj,
> + &rtc_freq_test_attr_group);
> + if (err) {
> + dev_err(ds1307->dev,
> + "Failed to create sysfs group: %d\n",
> + err);
> + return err;
> + }
> +
> + err = devm_add_action_or_reset(ds1307->dev,
> + rtc_calib_remove_sysfs_group,
> + ds1307->dev);
> + if (err) {
> + dev_err(ds1307->dev,
> + "Failed to add sysfs cleanup action: %d\n",
> + err);
> + sysfs_remove_group(&(ds1307->dev)->kobj,
> + &rtc_freq_test_attr_group);
> + return err;
> + }
> + break;
> + default:
> + break;
> + }
> +
> + return err;
> +}
> +
> /*----------------------------------------------------------------------*/
>
> static int ds1307_nvram_read(void *priv, unsigned int offset, void *val,
> @@ -1794,6 +1884,12 @@ static int ds1307_probe(struct i2c_client *client,
> if (err)
> return err;
>
> + err = ds1307_add_frequency_test(ds1307);
> + if (err) {
> + rtc_device_unregister(ds1307->rtc);
> + return err;
> + }
> +
> if (chip->nvram_size) {
> struct nvmem_config nvmem_cfg = {
> .name = "ds1307_nvram",
>
^ permalink raw reply
* [PATCH v6 4/4] rtc: ds1307: add frequency_test_enable sysfs attribute to check tick on m41txx
From: Giulio Benetti @ 2018-05-16 21:08 UTC (permalink / raw)
To: a.zummo, alexandre.belloni
Cc: robh+dt, mark.rutland, linux-rtc, devicetree, linux-kernel,
andy.shevchenko, Giulio Benetti
In-Reply-To: <20180516210842.5874-1-giulio.benetti@micronovasrl.com>
On m41txx you can enable open-drain OUT pin to check if offset is ok.
Enabling OUT pin with frequency_test_enable attribute, OUT pin will tick
512 times faster than 1s tick base.
Enable or Disable FT bit on CONTROL register if freq_test is 1 or 0.
Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
V5 => V6
* frequency_test_enable handled as a boolean
drivers/rtc/rtc-ds1307.c | 96 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 96 insertions(+)
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 2797d01bfa1d..b2e87845d860 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -1053,6 +1053,96 @@ static int m41txx_rtc_set_offset(struct device *dev, long offset)
ctrl_reg);
}
+static ssize_t frequency_test_enable_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct ds1307 *ds1307 = dev_get_drvdata(dev);
+ bool freq_test_en = 0;
+ int ret;
+
+ ret = kstrtobool(buf, &freq_test_en);
+ if (ret == -EINVAL) {
+ dev_err(dev, "Failed to store RTC Frequency Test attribute\n");
+ return ret;
+ }
+
+ regmap_update_bits(ds1307->regmap, M41TXX_REG_CONTROL, M41TXX_BIT_FT,
+ freq_test_en ? M41TXX_BIT_FT : 0);
+
+ return count;
+}
+
+static ssize_t frequency_test_enable_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct ds1307 *ds1307 = dev_get_drvdata(dev);
+ unsigned int ctrl_reg;
+
+ regmap_read(ds1307->regmap, M41TXX_REG_CONTROL, &ctrl_reg);
+
+ if (ctrl_reg & M41TXX_BIT_FT)
+ return scnprintf(buf, PAGE_SIZE, "on\n");
+ else
+ return scnprintf(buf, PAGE_SIZE, "off\n");
+}
+
+static DEVICE_ATTR_RW(frequency_test_enable);
+
+static struct attribute *rtc_freq_test_attrs[] = {
+ &dev_attr_frequency_test_enable.attr,
+ NULL,
+};
+
+static const struct attribute_group rtc_freq_test_attr_group = {
+ .attrs = rtc_freq_test_attrs,
+};
+
+static void rtc_calib_remove_sysfs_group(void *_dev)
+{
+ struct device *dev = _dev;
+
+ sysfs_remove_group(&dev->kobj, &rtc_freq_test_attr_group);
+}
+
+static int ds1307_add_frequency_test(struct ds1307 *ds1307)
+{
+ int err = 0;
+
+ switch (ds1307->type) {
+ case m41t0:
+ case m41t00:
+ case m41t11:
+ /* Export sysfs entries */
+ err = sysfs_create_group(&(ds1307->dev)->kobj,
+ &rtc_freq_test_attr_group);
+ if (err) {
+ dev_err(ds1307->dev,
+ "Failed to create sysfs group: %d\n",
+ err);
+ return err;
+ }
+
+ err = devm_add_action_or_reset(ds1307->dev,
+ rtc_calib_remove_sysfs_group,
+ ds1307->dev);
+ if (err) {
+ dev_err(ds1307->dev,
+ "Failed to add sysfs cleanup action: %d\n",
+ err);
+ sysfs_remove_group(&(ds1307->dev)->kobj,
+ &rtc_freq_test_attr_group);
+ return err;
+ }
+ break;
+ default:
+ break;
+ }
+
+ return err;
+}
+
/*----------------------------------------------------------------------*/
static int ds1307_nvram_read(void *priv, unsigned int offset, void *val,
@@ -1794,6 +1884,12 @@ static int ds1307_probe(struct i2c_client *client,
if (err)
return err;
+ err = ds1307_add_frequency_test(ds1307);
+ if (err) {
+ rtc_device_unregister(ds1307->rtc);
+ return err;
+ }
+
if (chip->nvram_size) {
struct nvmem_config nvmem_cfg = {
.name = "ds1307_nvram",
--
2.17.0
^ permalink raw reply related
* [PATCH v6 3/4] rtc: ds1307: add offset sysfs for mt41txx chips.
From: Giulio Benetti @ 2018-05-16 21:08 UTC (permalink / raw)
To: a.zummo, alexandre.belloni
Cc: robh+dt, mark.rutland, linux-rtc, devicetree, linux-kernel,
andy.shevchenko, Giulio Benetti
In-Reply-To: <20180516210842.5874-1-giulio.benetti@micronovasrl.com>
m41txx chips can hold a calibration value to get correct clock bias.
Add offset handling (ranging between -63ppm and 126ppm) via sysfs.
Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
drivers/rtc/rtc-ds1307.c | 81 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 81 insertions(+)
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 0ab0c166da83..2797d01bfa1d 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -114,6 +114,20 @@ enum ds_type {
# define RX8025_BIT_VDET 0x40
# define RX8025_BIT_XST 0x20
+#define M41TXX_REG_CONTROL 0x07
+# define M41TXX_BIT_OUT 0x80
+# define M41TXX_BIT_FT 0x40
+# define M41TXX_BIT_CALIB_SIGN 0x20
+# define M41TXX_M_CALIBRATION 0x1f
+
+/* negative offset step is -2.034ppm */
+#define M41TXX_NEG_OFFSET_STEP_PPM 2034
+/* positive offset step is +4.068ppm */
+#define M41TXX_POS_OFFSET_STEP_PPM 4068
+/* Min and max values supported with 'offset' interface by M41TXX */
+#define M41TXX_MIN_OFFSET (((-31) * M41TXX_NEG_OFFSET_STEP_PPM) / 1000)
+#define M41TXX_MAX_OFFSET (((31) * M41TXX_POS_OFFSET_STEP_PPM) / 1000)
+
struct ds1307 {
enum ds_type type;
unsigned long flags;
@@ -146,6 +160,9 @@ struct chip_desc {
static int ds1307_get_time(struct device *dev, struct rtc_time *t);
static int ds1307_set_time(struct device *dev, struct rtc_time *t);
+static int ds1337_read_alarm(struct device *dev, struct rtc_wkalrm *t);
+static int ds1337_set_alarm(struct device *dev, struct rtc_wkalrm *t);
+static int ds1307_alarm_irq_enable(struct device *dev, unsigned int enabled);
static u8 do_trickle_setup_ds1339(struct ds1307 *, u32 ohms, bool diode);
static irqreturn_t rx8130_irq(int irq, void *dev_id);
static int rx8130_read_alarm(struct device *dev, struct rtc_wkalrm *t);
@@ -155,6 +172,8 @@ static irqreturn_t mcp794xx_irq(int irq, void *dev_id);
static int mcp794xx_read_alarm(struct device *dev, struct rtc_wkalrm *t);
static int mcp794xx_set_alarm(struct device *dev, struct rtc_wkalrm *t);
static int mcp794xx_alarm_irq_enable(struct device *dev, unsigned int enabled);
+static int m41txx_rtc_read_offset(struct device *dev, long *offset);
+static int m41txx_rtc_set_offset(struct device *dev, long offset);
static const struct rtc_class_ops rx8130_rtc_ops = {
.read_time = ds1307_get_time,
@@ -172,6 +191,16 @@ static const struct rtc_class_ops mcp794xx_rtc_ops = {
.alarm_irq_enable = mcp794xx_alarm_irq_enable,
};
+static const struct rtc_class_ops m41txx_rtc_ops = {
+ .read_time = ds1307_get_time,
+ .set_time = ds1307_set_time,
+ .read_alarm = ds1337_read_alarm,
+ .set_alarm = ds1337_set_alarm,
+ .alarm_irq_enable = ds1307_alarm_irq_enable,
+ .read_offset = m41txx_rtc_read_offset,
+ .set_offset = m41txx_rtc_set_offset,
+};
+
static const struct chip_desc chips[last_ds_type] = {
[ds_1307] = {
.nvram_offset = 8,
@@ -227,10 +256,17 @@ static const struct chip_desc chips[last_ds_type] = {
.irq_handler = rx8130_irq,
.rtc_ops = &rx8130_rtc_ops,
},
+ [m41t0] = {
+ .rtc_ops = &m41txx_rtc_ops,
+ },
+ [m41t00] = {
+ .rtc_ops = &m41txx_rtc_ops,
+ },
[m41t11] = {
/* this is battery backed SRAM */
.nvram_offset = 8,
.nvram_size = 56,
+ .rtc_ops = &m41txx_rtc_ops,
},
[mcp794xx] = {
.alarm = 1,
@@ -972,6 +1008,51 @@ static int mcp794xx_alarm_irq_enable(struct device *dev, unsigned int enabled)
enabled ? MCP794XX_BIT_ALM0_EN : 0);
}
+static int m41txx_rtc_read_offset(struct device *dev, long *offset)
+{
+ struct ds1307 *ds1307 = dev_get_drvdata(dev);
+ unsigned int ctrl_reg;
+ u8 val;
+
+ regmap_read(ds1307->regmap, M41TXX_REG_CONTROL, &ctrl_reg);
+
+ val = ctrl_reg & M41TXX_M_CALIBRATION;
+
+ /* check if positive */
+ if (ctrl_reg & M41TXX_BIT_CALIB_SIGN)
+ *offset = (val * M41TXX_POS_OFFSET_STEP_PPM);
+ else
+ *offset = -(val * M41TXX_NEG_OFFSET_STEP_PPM);
+
+ *offset = DIV_ROUND_CLOSEST(*offset, 1000);
+
+ return 0;
+}
+
+static int m41txx_rtc_set_offset(struct device *dev, long offset)
+{
+ struct ds1307 *ds1307 = dev_get_drvdata(dev);
+ unsigned int ctrl_reg;
+
+ if ((offset < M41TXX_MIN_OFFSET) || (offset > M41TXX_MAX_OFFSET))
+ return -ERANGE;
+
+ offset *= 1000;
+
+ if (offset >= 0) {
+ ctrl_reg = DIV_ROUND_CLOSEST(offset,
+ M41TXX_POS_OFFSET_STEP_PPM);
+ ctrl_reg |= M41TXX_BIT_CALIB_SIGN;
+ } else {
+ ctrl_reg = DIV_ROUND_CLOSEST(abs(offset),
+ M41TXX_NEG_OFFSET_STEP_PPM);
+ }
+
+ return regmap_update_bits(ds1307->regmap, M41TXX_REG_CONTROL,
+ M41TXX_M_CALIBRATION | M41TXX_BIT_CALIB_SIGN,
+ ctrl_reg);
+}
+
/*----------------------------------------------------------------------*/
static int ds1307_nvram_read(void *priv, unsigned int offset, void *val,
--
2.17.0
^ permalink raw reply related
* [PATCH v6 2/4] rtc: ds1307: support m41t11 variant
From: Giulio Benetti @ 2018-05-16 21:08 UTC (permalink / raw)
To: a.zummo, alexandre.belloni
Cc: robh+dt, mark.rutland, linux-rtc, devicetree, linux-kernel,
andy.shevchenko, Giulio Benetti
In-Reply-To: <20180516210842.5874-1-giulio.benetti@micronovasrl.com>
The m41t11 variant is very similar to the already supported m41t00 and
m41t0, but it has also 56 bytes of NVRAM.
Add it to driver taking into account NVRAM section.
Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
.../devicetree/bindings/rtc/rtc-ds1307.txt | 1 +
drivers/rtc/rtc-ds1307.c | 14 ++++++++++++++
2 files changed, 15 insertions(+)
diff --git a/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt b/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt
index d28d6e7f6ae8..ce6469c1a516 100644
--- a/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt
+++ b/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt
@@ -13,6 +13,7 @@ Required properties:
"maxim,ds3231",
"st,m41t0",
"st,m41t00",
+ "st,m41t11",
"microchip,mcp7940x",
"microchip,mcp7941x",
"pericom,pt7c4338",
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 32aadcbc377f..0ab0c166da83 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -44,6 +44,7 @@ enum ds_type {
ds_3231,
m41t0,
m41t00,
+ m41t11,
mcp794xx,
rx_8025,
rx_8130,
@@ -226,6 +227,11 @@ static const struct chip_desc chips[last_ds_type] = {
.irq_handler = rx8130_irq,
.rtc_ops = &rx8130_rtc_ops,
},
+ [m41t11] = {
+ /* this is battery backed SRAM */
+ .nvram_offset = 8,
+ .nvram_size = 56,
+ },
[mcp794xx] = {
.alarm = 1,
/* this is battery backed SRAM */
@@ -248,6 +254,7 @@ static const struct i2c_device_id ds1307_id[] = {
{ "ds3231", ds_3231 },
{ "m41t0", m41t0 },
{ "m41t00", m41t00 },
+ { "m41t11", m41t11 },
{ "mcp7940x", mcp794xx },
{ "mcp7941x", mcp794xx },
{ "pt7c4338", ds_1307 },
@@ -304,6 +311,10 @@ static const struct of_device_id ds1307_of_match[] = {
.compatible = "st,m41t00",
.data = (void *)m41t00
},
+ {
+ .compatible = "st,m41t11",
+ .data = (void *)m41t11
+ },
{
.compatible = "microchip,mcp7940x",
.data = (void *)mcp794xx
@@ -346,6 +357,7 @@ static const struct acpi_device_id ds1307_acpi_ids[] = {
{ .id = "DS3231", .driver_data = ds_3231 },
{ .id = "M41T0", .driver_data = m41t0 },
{ .id = "M41T00", .driver_data = m41t00 },
+ { .id = "M41T11", .driver_data = m41t11 },
{ .id = "MCP7940X", .driver_data = mcp794xx },
{ .id = "MCP7941X", .driver_data = mcp794xx },
{ .id = "PT7C4338", .driver_data = ds_1307 },
@@ -1574,6 +1586,7 @@ static int ds1307_probe(struct i2c_client *client,
case ds_1307:
case m41t0:
case m41t00:
+ case m41t11:
/* clock halted? turn it on, so clock can tick. */
if (tmp & DS1307_BIT_CH) {
regmap_write(ds1307->regmap, DS1307_REG_SECS, 0);
@@ -1639,6 +1652,7 @@ static int ds1307_probe(struct i2c_client *client,
case ds_1340:
case m41t0:
case m41t00:
+ case m41t11:
/*
* NOTE: ignores century bits; fix before deploying
* systems that will run through year 2100.
--
2.17.0
^ permalink raw reply related
* [PATCH v6 1/4] rtc: ds1307: fix data pointer to m41t0
From: Giulio Benetti @ 2018-05-16 21:08 UTC (permalink / raw)
To: a.zummo, alexandre.belloni
Cc: robh+dt, mark.rutland, linux-rtc, devicetree, linux-kernel,
andy.shevchenko, Giulio Benetti
data field points to m41t00, instead it should point to m41t0.
Driver works correctly because on both cases(m41t0 and m41t00) chip_desc
are equal.
Point to right enum m41t0 instead of m41t00.
Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
drivers/rtc/rtc-ds1307.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index a13e59edff53..32aadcbc377f 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -298,7 +298,7 @@ static const struct of_device_id ds1307_of_match[] = {
},
{
.compatible = "st,m41t0",
- .data = (void *)m41t00
+ .data = (void *)m41t0
},
{
.compatible = "st,m41t00",
--
2.17.0
^ permalink raw reply related
* [PATCH v6 4/4] rtc: ds1307: add frequency_test_enable sysfs attribute to check tick on m41txx
From: Giulio Benetti @ 2018-05-16 21:07 UTC (permalink / raw)
To: a.zummo, alexandre.belloni
Cc: robh+dt, mark.rutland, linux-rtc, devicetree, linux-kernel,
andy.shevchenko, Giulio Benetti
In-Reply-To: <20180516210705.5824-1-giulio.benetti@micronovasrl.com>
On m41txx you can enable open-drain OUT pin to check if offset is ok.
Enabling OUT pin with frequency_test_enable attribute, OUT pin will tick
512 times faster than 1s tick base.
Enable or Disable FT bit on CONTROL register if freq_test is 1 or 0.
Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
drivers/rtc/rtc-ds1307.c | 96 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 96 insertions(+)
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 2797d01bfa1d..b2e87845d860 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -1053,6 +1053,96 @@ static int m41txx_rtc_set_offset(struct device *dev, long offset)
ctrl_reg);
}
+static ssize_t frequency_test_enable_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct ds1307 *ds1307 = dev_get_drvdata(dev);
+ bool freq_test_en = 0;
+ int ret;
+
+ ret = kstrtobool(buf, &freq_test_en);
+ if (ret == -EINVAL) {
+ dev_err(dev, "Failed to store RTC Frequency Test attribute\n");
+ return ret;
+ }
+
+ regmap_update_bits(ds1307->regmap, M41TXX_REG_CONTROL, M41TXX_BIT_FT,
+ freq_test_en ? M41TXX_BIT_FT : 0);
+
+ return count;
+}
+
+static ssize_t frequency_test_enable_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct ds1307 *ds1307 = dev_get_drvdata(dev);
+ unsigned int ctrl_reg;
+
+ regmap_read(ds1307->regmap, M41TXX_REG_CONTROL, &ctrl_reg);
+
+ if (ctrl_reg & M41TXX_BIT_FT)
+ return scnprintf(buf, PAGE_SIZE, "on\n");
+ else
+ return scnprintf(buf, PAGE_SIZE, "off\n");
+}
+
+static DEVICE_ATTR_RW(frequency_test_enable);
+
+static struct attribute *rtc_freq_test_attrs[] = {
+ &dev_attr_frequency_test_enable.attr,
+ NULL,
+};
+
+static const struct attribute_group rtc_freq_test_attr_group = {
+ .attrs = rtc_freq_test_attrs,
+};
+
+static void rtc_calib_remove_sysfs_group(void *_dev)
+{
+ struct device *dev = _dev;
+
+ sysfs_remove_group(&dev->kobj, &rtc_freq_test_attr_group);
+}
+
+static int ds1307_add_frequency_test(struct ds1307 *ds1307)
+{
+ int err = 0;
+
+ switch (ds1307->type) {
+ case m41t0:
+ case m41t00:
+ case m41t11:
+ /* Export sysfs entries */
+ err = sysfs_create_group(&(ds1307->dev)->kobj,
+ &rtc_freq_test_attr_group);
+ if (err) {
+ dev_err(ds1307->dev,
+ "Failed to create sysfs group: %d\n",
+ err);
+ return err;
+ }
+
+ err = devm_add_action_or_reset(ds1307->dev,
+ rtc_calib_remove_sysfs_group,
+ ds1307->dev);
+ if (err) {
+ dev_err(ds1307->dev,
+ "Failed to add sysfs cleanup action: %d\n",
+ err);
+ sysfs_remove_group(&(ds1307->dev)->kobj,
+ &rtc_freq_test_attr_group);
+ return err;
+ }
+ break;
+ default:
+ break;
+ }
+
+ return err;
+}
+
/*----------------------------------------------------------------------*/
static int ds1307_nvram_read(void *priv, unsigned int offset, void *val,
@@ -1794,6 +1884,12 @@ static int ds1307_probe(struct i2c_client *client,
if (err)
return err;
+ err = ds1307_add_frequency_test(ds1307);
+ if (err) {
+ rtc_device_unregister(ds1307->rtc);
+ return err;
+ }
+
if (chip->nvram_size) {
struct nvmem_config nvmem_cfg = {
.name = "ds1307_nvram",
--
2.17.0
^ permalink raw reply related
* [PATCH v6 3/4] rtc: ds1307: add offset sysfs for mt41txx chips.
From: Giulio Benetti @ 2018-05-16 21:07 UTC (permalink / raw)
To: a.zummo, alexandre.belloni
Cc: robh+dt, mark.rutland, linux-rtc, devicetree, linux-kernel,
andy.shevchenko, Giulio Benetti
In-Reply-To: <20180516210705.5824-1-giulio.benetti@micronovasrl.com>
m41txx chips can hold a calibration value to get correct clock bias.
Add offset handling (ranging between -63ppm and 126ppm) via sysfs.
Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
drivers/rtc/rtc-ds1307.c | 81 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 81 insertions(+)
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 0ab0c166da83..2797d01bfa1d 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -114,6 +114,20 @@ enum ds_type {
# define RX8025_BIT_VDET 0x40
# define RX8025_BIT_XST 0x20
+#define M41TXX_REG_CONTROL 0x07
+# define M41TXX_BIT_OUT 0x80
+# define M41TXX_BIT_FT 0x40
+# define M41TXX_BIT_CALIB_SIGN 0x20
+# define M41TXX_M_CALIBRATION 0x1f
+
+/* negative offset step is -2.034ppm */
+#define M41TXX_NEG_OFFSET_STEP_PPM 2034
+/* positive offset step is +4.068ppm */
+#define M41TXX_POS_OFFSET_STEP_PPM 4068
+/* Min and max values supported with 'offset' interface by M41TXX */
+#define M41TXX_MIN_OFFSET (((-31) * M41TXX_NEG_OFFSET_STEP_PPM) / 1000)
+#define M41TXX_MAX_OFFSET (((31) * M41TXX_POS_OFFSET_STEP_PPM) / 1000)
+
struct ds1307 {
enum ds_type type;
unsigned long flags;
@@ -146,6 +160,9 @@ struct chip_desc {
static int ds1307_get_time(struct device *dev, struct rtc_time *t);
static int ds1307_set_time(struct device *dev, struct rtc_time *t);
+static int ds1337_read_alarm(struct device *dev, struct rtc_wkalrm *t);
+static int ds1337_set_alarm(struct device *dev, struct rtc_wkalrm *t);
+static int ds1307_alarm_irq_enable(struct device *dev, unsigned int enabled);
static u8 do_trickle_setup_ds1339(struct ds1307 *, u32 ohms, bool diode);
static irqreturn_t rx8130_irq(int irq, void *dev_id);
static int rx8130_read_alarm(struct device *dev, struct rtc_wkalrm *t);
@@ -155,6 +172,8 @@ static irqreturn_t mcp794xx_irq(int irq, void *dev_id);
static int mcp794xx_read_alarm(struct device *dev, struct rtc_wkalrm *t);
static int mcp794xx_set_alarm(struct device *dev, struct rtc_wkalrm *t);
static int mcp794xx_alarm_irq_enable(struct device *dev, unsigned int enabled);
+static int m41txx_rtc_read_offset(struct device *dev, long *offset);
+static int m41txx_rtc_set_offset(struct device *dev, long offset);
static const struct rtc_class_ops rx8130_rtc_ops = {
.read_time = ds1307_get_time,
@@ -172,6 +191,16 @@ static const struct rtc_class_ops mcp794xx_rtc_ops = {
.alarm_irq_enable = mcp794xx_alarm_irq_enable,
};
+static const struct rtc_class_ops m41txx_rtc_ops = {
+ .read_time = ds1307_get_time,
+ .set_time = ds1307_set_time,
+ .read_alarm = ds1337_read_alarm,
+ .set_alarm = ds1337_set_alarm,
+ .alarm_irq_enable = ds1307_alarm_irq_enable,
+ .read_offset = m41txx_rtc_read_offset,
+ .set_offset = m41txx_rtc_set_offset,
+};
+
static const struct chip_desc chips[last_ds_type] = {
[ds_1307] = {
.nvram_offset = 8,
@@ -227,10 +256,17 @@ static const struct chip_desc chips[last_ds_type] = {
.irq_handler = rx8130_irq,
.rtc_ops = &rx8130_rtc_ops,
},
+ [m41t0] = {
+ .rtc_ops = &m41txx_rtc_ops,
+ },
+ [m41t00] = {
+ .rtc_ops = &m41txx_rtc_ops,
+ },
[m41t11] = {
/* this is battery backed SRAM */
.nvram_offset = 8,
.nvram_size = 56,
+ .rtc_ops = &m41txx_rtc_ops,
},
[mcp794xx] = {
.alarm = 1,
@@ -972,6 +1008,51 @@ static int mcp794xx_alarm_irq_enable(struct device *dev, unsigned int enabled)
enabled ? MCP794XX_BIT_ALM0_EN : 0);
}
+static int m41txx_rtc_read_offset(struct device *dev, long *offset)
+{
+ struct ds1307 *ds1307 = dev_get_drvdata(dev);
+ unsigned int ctrl_reg;
+ u8 val;
+
+ regmap_read(ds1307->regmap, M41TXX_REG_CONTROL, &ctrl_reg);
+
+ val = ctrl_reg & M41TXX_M_CALIBRATION;
+
+ /* check if positive */
+ if (ctrl_reg & M41TXX_BIT_CALIB_SIGN)
+ *offset = (val * M41TXX_POS_OFFSET_STEP_PPM);
+ else
+ *offset = -(val * M41TXX_NEG_OFFSET_STEP_PPM);
+
+ *offset = DIV_ROUND_CLOSEST(*offset, 1000);
+
+ return 0;
+}
+
+static int m41txx_rtc_set_offset(struct device *dev, long offset)
+{
+ struct ds1307 *ds1307 = dev_get_drvdata(dev);
+ unsigned int ctrl_reg;
+
+ if ((offset < M41TXX_MIN_OFFSET) || (offset > M41TXX_MAX_OFFSET))
+ return -ERANGE;
+
+ offset *= 1000;
+
+ if (offset >= 0) {
+ ctrl_reg = DIV_ROUND_CLOSEST(offset,
+ M41TXX_POS_OFFSET_STEP_PPM);
+ ctrl_reg |= M41TXX_BIT_CALIB_SIGN;
+ } else {
+ ctrl_reg = DIV_ROUND_CLOSEST(abs(offset),
+ M41TXX_NEG_OFFSET_STEP_PPM);
+ }
+
+ return regmap_update_bits(ds1307->regmap, M41TXX_REG_CONTROL,
+ M41TXX_M_CALIBRATION | M41TXX_BIT_CALIB_SIGN,
+ ctrl_reg);
+}
+
/*----------------------------------------------------------------------*/
static int ds1307_nvram_read(void *priv, unsigned int offset, void *val,
--
2.17.0
^ permalink raw reply related
* [PATCH v6 2/4] rtc: ds1307: support m41t11 variant
From: Giulio Benetti @ 2018-05-16 21:07 UTC (permalink / raw)
To: a.zummo, alexandre.belloni
Cc: robh+dt, mark.rutland, linux-rtc, devicetree, linux-kernel,
andy.shevchenko, Giulio Benetti
In-Reply-To: <20180516210705.5824-1-giulio.benetti@micronovasrl.com>
The m41t11 variant is very similar to the already supported m41t00 and
m41t0, but it has also 56 bytes of NVRAM.
Add it to driver taking into account NVRAM section.
Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
.../devicetree/bindings/rtc/rtc-ds1307.txt | 1 +
drivers/rtc/rtc-ds1307.c | 14 ++++++++++++++
2 files changed, 15 insertions(+)
diff --git a/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt b/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt
index d28d6e7f6ae8..ce6469c1a516 100644
--- a/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt
+++ b/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt
@@ -13,6 +13,7 @@ Required properties:
"maxim,ds3231",
"st,m41t0",
"st,m41t00",
+ "st,m41t11",
"microchip,mcp7940x",
"microchip,mcp7941x",
"pericom,pt7c4338",
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 32aadcbc377f..0ab0c166da83 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -44,6 +44,7 @@ enum ds_type {
ds_3231,
m41t0,
m41t00,
+ m41t11,
mcp794xx,
rx_8025,
rx_8130,
@@ -226,6 +227,11 @@ static const struct chip_desc chips[last_ds_type] = {
.irq_handler = rx8130_irq,
.rtc_ops = &rx8130_rtc_ops,
},
+ [m41t11] = {
+ /* this is battery backed SRAM */
+ .nvram_offset = 8,
+ .nvram_size = 56,
+ },
[mcp794xx] = {
.alarm = 1,
/* this is battery backed SRAM */
@@ -248,6 +254,7 @@ static const struct i2c_device_id ds1307_id[] = {
{ "ds3231", ds_3231 },
{ "m41t0", m41t0 },
{ "m41t00", m41t00 },
+ { "m41t11", m41t11 },
{ "mcp7940x", mcp794xx },
{ "mcp7941x", mcp794xx },
{ "pt7c4338", ds_1307 },
@@ -304,6 +311,10 @@ static const struct of_device_id ds1307_of_match[] = {
.compatible = "st,m41t00",
.data = (void *)m41t00
},
+ {
+ .compatible = "st,m41t11",
+ .data = (void *)m41t11
+ },
{
.compatible = "microchip,mcp7940x",
.data = (void *)mcp794xx
@@ -346,6 +357,7 @@ static const struct acpi_device_id ds1307_acpi_ids[] = {
{ .id = "DS3231", .driver_data = ds_3231 },
{ .id = "M41T0", .driver_data = m41t0 },
{ .id = "M41T00", .driver_data = m41t00 },
+ { .id = "M41T11", .driver_data = m41t11 },
{ .id = "MCP7940X", .driver_data = mcp794xx },
{ .id = "MCP7941X", .driver_data = mcp794xx },
{ .id = "PT7C4338", .driver_data = ds_1307 },
@@ -1574,6 +1586,7 @@ static int ds1307_probe(struct i2c_client *client,
case ds_1307:
case m41t0:
case m41t00:
+ case m41t11:
/* clock halted? turn it on, so clock can tick. */
if (tmp & DS1307_BIT_CH) {
regmap_write(ds1307->regmap, DS1307_REG_SECS, 0);
@@ -1639,6 +1652,7 @@ static int ds1307_probe(struct i2c_client *client,
case ds_1340:
case m41t0:
case m41t00:
+ case m41t11:
/*
* NOTE: ignores century bits; fix before deploying
* systems that will run through year 2100.
--
2.17.0
^ permalink raw reply related
* [PATCH v6 1/4] rtc: ds1307: fix data pointer to m41t0
From: Giulio Benetti @ 2018-05-16 21:07 UTC (permalink / raw)
To: a.zummo, alexandre.belloni
Cc: robh+dt, mark.rutland, linux-rtc, devicetree, linux-kernel,
andy.shevchenko, Giulio Benetti
data field points to m41t00, instead it should point to m41t0.
Driver works correctly because on both cases(m41t0 and m41t00) chip_desc
are equal.
Point to right enum m41t0 instead of m41t00.
Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
drivers/rtc/rtc-ds1307.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index a13e59edff53..32aadcbc377f 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -298,7 +298,7 @@ static const struct of_device_id ds1307_of_match[] = {
},
{
.compatible = "st,m41t0",
- .data = (void *)m41t00
+ .data = (void *)m41t0
},
{
.compatible = "st,m41t00",
--
2.17.0
^ permalink raw reply related
* Re: [PATCH v6 2/2] leds: lm3601x: Introduce the lm3601x LED driver
From: Jacek Anaszewski @ 2018-05-16 21:02 UTC (permalink / raw)
To: Andy Shevchenko, Dan Murphy
Cc: Rob Herring, Mark Rutland, Pavel Machek, devicetree,
Linux Kernel Mailing List, Linux LED Subsystem
In-Reply-To: <CAHp75Vcz08zrs3bgMFcvYwRrN_A43D=Nvt5cFDQf8m9whB=yhA@mail.gmail.com>
Hi Andy and Dan,
On 05/16/2018 12:24 AM, Andy Shevchenko wrote:
> On Wed, May 16, 2018 at 1:08 AM, Dan Murphy <dmurphy@ti.com> wrote:
>> On 05/15/2018 04:56 PM, Andy Shevchenko wrote:
>>> On Tue, May 15, 2018 at 6:43 PM, Dan Murphy <dmurphy@ti.com> wrote:
>
>>>> + depends on LEDS_CLASS && I2C && OF
>>>
>>> What is OF specific in this driver?
>>
>> as3645a_led_class_setup has a "of" dependency
>
> So what? Is it called from this driver or?
>
>
>>>> +static const struct lm3601x_max_timeouts strobe_timeouts[] = {
>>>> + { 40000, 0x00 },
>>>> + { 80000, 0x01 },
>>>> + { 120000, 0x02 },
>>>> + { 160000, 0x03 },
>>>> + { 200000, 0x04 },
>>>> + { 240000, 0x05 },
>>>> + { 280000, 0x06 },
>>>> + { 320000, 0x07 },
>>>> + { 360000, 0x08 },
>>>> + { 400000, 0x09 },
>>>> + { 600000, 0x0a },
>>>> + { 800000, 0x0b },
>>>> + { 1000000, 0x0c },
>>>> + { 1200000, 0x0d },
>>>> + { 1400000, 0x0e },
>>>> + { 1600000, 0x0f },
>>>
>>> Huh?!
>>
>> Please give comments that actually mean something other wise I will opt to ignore them.
>
> I did below.
>
>>> strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC;
>>
>> Not sure what equation you are trying to point out here. But if you are trying to apply
>> a timeout step you cannot do this with this part. As pointed out in the DT doc the timeout
>> step is not linear.
>
> Yeah, I know people are more than often too lazy to think.
>
> if (x < 9)
> strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC;
> else
> strobe_timeout = (400 + (x - 9) * 200) * MSECS_IN_SEC;
>
I like the idea.
>>>> + brightness_val = (brightness/2);
>>>
>>> Spaces.
>>
>> Not sure what this means checkpatch was clean
>
> Even besides missed whispaces it has redundant parens.
>
> checkpatch is not a silver bullet to get your code clean and nice.
>
>>> This is return led_...();
>>
>> That is a preference. It does not have to be that way.
I missed that. Dan, please follow Andy's advise.
>
> What do you mean? We do not appreciate +LOCs for no (or even nagative!) benefit.
>
>>>> + ret = of_property_read_string(led->led_node, "label", &name);
>>>
>>> device_property_...();
>>
>> It can be if the maintainer is requesting this.
>
> Jacek, if you need rationale behind this comment it's here: the driver
> has nothing DT specific and getting rid of OF centric programming
> allows to reuse the driver on non-DT platforms w/o touching a source
> code.
It has an added value, so yes, let's use it as a standard approach
from now on.
>> Is the trend to move to these functions?
>
> See above.
>
>> Most drivers use the "of" calls.
>
> So what?
>
>
>>>> + if (!ret)
>>>
>>> if (ret) sounds more natural. And better just to split
>>>
>>>> + snprintf(led->led_name, sizeof(led->led_name),
>>>> + "%s:%s", led->led_node->name, name);
>>>> + else
>>>> + snprintf(led->led_name, sizeof(led->led_name),
>>>> + "%s:torch", led->led_node->name);
>>>
>>> const char *tmp;
>>>
>>> ret = device_property_read_...(&tmp);
>>> if (ret)
>>> tmp = ...
>>> sprintf(...);
We're no longer taking devicename section of a LED class device name
from DT, so it will look differently anyway.
> No comments on this?
>
>>>> + led = devm_kzalloc(&client->dev,
>>>> + sizeof(struct lm3601x_led), GFP_KERNEL);
>>>
>>> sizeof(*led) and one line in the result
>
> And this?
Ack.
>
>>>> + { },
>>>
>>> Terminators better w/o comma.
>>
>> Looking at other drivers adding comma's on the sentinel is accepted. See the as3645a driver
>
> So what?
>
> Terminator at compile time even better.
>
>>>> + {},
>>>
>>> Ditto.
>>
>> See above
>
> See above.
>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply
* Re: [PATCH v5 4/4] rtc: ds1307: add frequency_test_enable sysfs attribute to check tick on m41txx
From: Giulio Benetti @ 2018-05-16 21:02 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Alessandro Zummo, alexandre.belloni, Rob Herring, Mark Rutland,
linux-rtc, devicetree, Linux Kernel Mailing List
In-Reply-To: <CAHp75VdC74jW+Vox2ogDVzG17Z3kD0Vg-1du4SeK30WajT7N-Q@mail.gmail.com>
Hi,
Il 16/05/2018 22:22, Andy Shevchenko ha scritto:
> On Wed, May 16, 2018 at 1:32 PM, Giulio Benetti
> <giulio.benetti@micronovasrl.com> wrote:
>> On m41txx you can enable open-drain OUT pin to check if offset is ok.
>> Enabling OUT pin with frequency_test_enable attribute, OUT pin will tick
>> 512 times faster than 1s tick base.
>>
>> Enable or Disable FT bit on CONTROL register if freq_test is 1 or 0.
>>
>> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
>
>> +static ssize_t frequency_test_enable_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct ds1307 *ds1307 = dev_get_drvdata(dev);
>> + unsigned long freq_test = 0;
>> + int retval;
>> +
>> + retval = kstrtoul(buf, 10, &freq_test);
>> + if ((retval < 0) || (retval > 1)) {
>
> kstrtobool() then?
Yes, you're right.
>
>> + dev_err(dev, "Failed to store RTC Frequency Test attribute\n");
>
>> + return -EINVAL;
>
> ...and do not shadow actual error code.
Ok
>
>> + }
>> +
>> + regmap_update_bits(ds1307->regmap, M41TXX_REG_CONTROL, M41TXX_BIT_FT,
>> + freq_test ? M41TXX_BIT_FT : 0);
>> +
>
>> + return retval ? retval : count;
>
> Does the condition make any sense?
You're right, not at all.
I changed it into:
"return count;"
>
>> +}
>
>> +static ssize_t frequency_test_enable_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>
>> + int freq_test_en = 0;
>
>> + if (ctrl_reg & M41TXX_BIT_FT)
>> + freq_test_en = true;
>> + else
>> + freq_test_en = false;
>> +
>> + return sprintf(buf, "%d\n", freq_test_en);
>
> So, is it boolean or integer? This code makes it confusing a lot.
It is a boolean, so now I've updated with this:
if (ctrl_reg & M41TXX_BIT_FT)
return scnprintf(buf, PAGE_SIZE, "on\n");
else
return scnprintf(buf, PAGE_SIZE, "off\n");
>
>> +}
>
Thank you very much for review
Giulio Benetti
^ permalink raw reply
* Re: [PATCH v5 05/14] usb: typec: add API to get typec basic port power and data config
From: Mats Karrman @ 2018-05-16 20:55 UTC (permalink / raw)
To: Heikki Krogerus, Jun Li
Cc: robh+dt@kernel.org, gregkh@linuxfoundation.org,
linux@roeck-us.net, a.hajda@samsung.com, cw00.choi@samsung.com,
shufan_lee@richtek.com, Peter Chen, gsomlo@gmail.com,
devicetree@vger.kernel.org, linux-usb@vger.kernel.org,
dl-linux-imx
In-Reply-To: <20180516122527.GC11469@kuha.fi.intel.com>
Hi,
On 05/16/2018 02:25 PM, Heikki Krogerus wrote:
> Hi guys,
>
> On Tue, May 15, 2018 at 10:52:57PM +0200, Mats Karrman wrote:
>> Hi,
>>
>> On 05/14/2018 11:36 AM, Jun Li wrote:
>>
>>> Hi
>>>> -----Original Message-----
>>>> From: Mats Karrman [mailto:mats.dev.list@gmail.com]
>>>> Sent: 2018???5???12??? 3:56
>>>> To: Jun Li <jun.li@nxp.com>; robh+dt@kernel.org; gregkh@linuxfoundation.org;
>>>> heikki.krogerus@linux.intel.com; linux@roeck-us.net
>>>> Cc: a.hajda@samsung.com; cw00.choi@samsung.com;
>>>> shufan_lee@richtek.com; Peter Chen <peter.chen@nxp.com>;
>>>> gsomlo@gmail.com; devicetree@vger.kernel.org; linux-usb@vger.kernel.org;
>>>> dl-linux-imx <linux-imx@nxp.com>
>>>> Subject: Re: [PATCH v5 05/14] usb: typec: add API to get typec basic port power
>>>> and data config
>>>>
>>>> Hi Li Jun,
>>>>
>>>> On 2018-05-03 02:24, Li Jun wrote:
>>>>
>>>>> This patch adds 3 APIs to get the typec port power and data type, and
>>>>> preferred power role by its name string.
>>>>>
>>>>> Signed-off-by: Li Jun <jun.li@nxp.com>
>>>>> ---
>>>>> drivers/usb/typec/class.c | 52
>>>> +++++++++++++++++++++++++++++++++++++++++++++++
>>>>> include/linux/usb/typec.h | 3 +++
>>>>> 2 files changed, 55 insertions(+)
>>>>>
>>>>> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
>>>>> index 53df10d..5981e18 100644
>>>>> --- a/drivers/usb/typec/class.c
>>>>> +++ b/drivers/usb/typec/class.c
>>>>> @@ -9,6 +9,7 @@
>>>>> #include <linux/device.h>
>>>>> #include <linux/module.h>
>>>>> #include <linux/mutex.h>
>>>>> +#include <linux/property.h>
> I don't think you need that anymore.
>
>>>>> #include <linux/slab.h>
>>>>> #include <linux/usb/typec.h>
>>>>> #include <linux/usb/typec_mux.h>
>>>>> @@ -802,6 +803,12 @@ static const char * const typec_port_types[] = {
>>>>> [TYPEC_PORT_DRP] = "dual",
>>>>> };
>>>>>
>>>>> +static const char * const typec_data_types[] = {
>>>>> + [TYPEC_PORT_DFP] = "host",
>>>>> + [TYPEC_PORT_UFP] = "device",
>>>>> + [TYPEC_PORT_DRD] = "dual",
>>>>> +};
>>>>> +
>>>>> static const char * const typec_port_types_drp[] = {
>>>>> [TYPEC_PORT_SRC] = "dual [source] sink",
>>>>> [TYPEC_PORT_SNK] = "dual source [sink]", @@ -1252,6 +1259,51
>>>> @@
>>>>> void typec_set_pwr_opmode(struct typec_port *port,
>>>>> }
>>>>> EXPORT_SYMBOL_GPL(typec_set_pwr_opmode);
>>>>>
>>>>> +/**
>>>>> + * typec_find_power_type - Get the typec port power type
>>>> Why is this function called typec_find_power_type() and not
>>>> typec_find_port_type()?
>>>> It's called port_type in sysfs, having different names just adds confusion.
>>>> (Otherwise I agree power_type is a better name but...)
>>> We have "port type" before the power and data role separation,
>>> this API name's intention is to reflect the power cap, anyway I
>>> leave this to be decided by Heikki then.
> I really hate the "*_type" naming. It was understandable when there
> was no separate power and data roles defined in the specification, but
> now that there are, it's just confusing. IMO we should not use it
> anywhere.
>
> So to me typec_find_type() is just as bad as typec_find_power_type()
> because it has the "type" in it. I wonder if this function is
> necessary at all? If it is, then perhaps we can think of some better
> name for it, name that gives a better hint what it is used for.
I reread this patch and tried to see it more in the context of the other
patches and the existing code. The naming of the existing string tables
doesn't help in getting this right, however I have a proposal:
typec_find_port_power_role() to get to TYPEC_PORT_SRC/SNK/DRP
typec_find_port_data_role() to get to TYPEC_PORT_DFP/UFP/DRD
typec_find_power_role() to get to TYPEC_SINK/SOURCE
and sometime, if the use should arise
typec_find_data_role() to get to TYPEC_DEVICE/HOST
I think it is fairly comprehensible, *_port_* concerns a capability and
without *_port_* it is an actual state. Plus it matches the names of the
constants.
BR // Mats
>>>>> + * @name: port type string
>>>>> + *
>>>>> + * This routine is used to find the typec_port_type by its string name.
>>>>> + *
>>>>> + * Returns typec_port_type if success, otherwise negative error code.
>>>>> + */
>>>>> +int typec_find_power_type(const char *name) {
>>>>> + return match_string(typec_port_types, ARRAY_SIZE(typec_port_types),
>>>>> + name);
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(typec_find_power_type);
>>>>> +
>>>>> +/**
>>>>> + * typec_find_preferred_role - Find the typec drp port preferred
>>>>> +power role
>>>> Why typec_find_preferred_role()? Could be used for any power_role so why not
>>>> typec_find_power_role()?
>>> I am not sure if I catch your point of this comment.
>>> For preferred role(if support try.sink or try.src) the only allowed power roles are
>>> "sink"
>>> "source"
>>> But for power role, the allowed type are
>>> "sink"
>>> "source"
>>> "dual"
>> Uhm, typing too fast again, I am. A better name would be just typec_find_role().
>> What I mean is that the function could be used for any situation when
>> someone wants to map a string to a TYPEC_{SOURCE,SINK} constant so it
>> is unnecessary to limit its usage to just preferred role.
> That sounds reasonable to me.
>
>
> Thanks,
>
^ permalink raw reply
* Re: [PATCH v6 2/2] leds: lm3601x: Introduce the lm3601x LED driver
From: Dan Murphy @ 2018-05-16 20:43 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Rob Herring, Mark Rutland, Jacek Anaszewski, Pavel Machek,
devicetree, Linux Kernel Mailing List, Linux LED Subsystem
In-Reply-To: <CAHp75Vcz08zrs3bgMFcvYwRrN_A43D=Nvt5cFDQf8m9whB=yhA@mail.gmail.com>
Andy
The first response is to Jacek the rest are addressed to you
On 05/15/2018 05:24 PM, Andy Shevchenko wrote:
> On Wed, May 16, 2018 at 1:08 AM, Dan Murphy <dmurphy@ti.com> wrote:
>> On 05/15/2018 04:56 PM, Andy Shevchenko wrote:
>>> On Tue, May 15, 2018 at 6:43 PM, Dan Murphy <dmurphy@ti.com> wrote:
>
>>>> + depends on LEDS_CLASS && I2C && OF
>>>
>>> What is OF specific in this driver?
>>
>> as3645a_led_class_setup has a "of" dependency
>
> So what? Is it called from this driver or?
>
Jacek
Do you have a preference about the use of "of" calls over the device_property calls?
I know Andy is pushing this for IoT and gave a good presentation on it last year on
common mistakes. Not sure this is a "common mistake" but more of a overall Linux compatibility issue
between x86 and ARM using two different configuration methods.
Since these lighting drivers are agnostic to the processor maybe we should adopt that philosophy.
I have no problem being the example driver on that.
>
>>>> +static const struct lm3601x_max_timeouts strobe_timeouts[] = {
>>>> + { 40000, 0x00 },
>>>> + { 80000, 0x01 },
>>>> + { 120000, 0x02 },
>>>> + { 160000, 0x03 },
>>>> + { 200000, 0x04 },
>>>> + { 240000, 0x05 },
>>>> + { 280000, 0x06 },
>>>> + { 320000, 0x07 },
>>>> + { 360000, 0x08 },
>>>> + { 400000, 0x09 },
>>>> + { 600000, 0x0a },
>>>> + { 800000, 0x0b },
>>>> + { 1000000, 0x0c },
>>>> + { 1200000, 0x0d },
>>>> + { 1400000, 0x0e },
>>>> + { 1600000, 0x0f },
>>>
>>> Huh?!
>>
>> Please give comments that actually mean something other wise I will opt to ignore them.
>
> I did below.
Sort of. "So what?" and "Huh" are not really a technical comments. I can tell from your LVEE presentation that
you have a passion for making the kernel better. That is awesome but if we try to make the code perfect everytime no code will
ever get merged. And we are human and we will miss coding issues and make mistakes.
>
>>> strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC;
>>
>> Not sure what equation you are trying to point out here. But if you are trying to apply
>> a timeout step you cannot do this with this part. As pointed out in the DT doc the timeout
>> step is not linear.
>
> Yeah, I know people are more than often too lazy to think.
Well this is why I put the table in. It is not laziness on my part as I try to add clarity for
our newbies or productization engineers, translation/lookup tables are not as bad as you think.
I am fully aware that this bloats the data section of the image and has negative impacts
on IoT small memory foot print devices.
>
> if (x < 9)
> strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC;
> else
> strobe_timeout = (400 + (x - 9) * 200) * MSECS_IN_SEC;
>
Not sure if this will produce the register value I am looking for. I have to run through the
algorithim. The idea is to take in the timeout and get the reg value to program.
If it yields the expected output, or with a modified equation, then I can pull it in.
>>>> + brightness_val = (brightness/2);
>>>
>>> Spaces.
>>
>> Not sure what this means checkpatch was clean
>
> Even besides missed whispaces it has redundant parens.
>
> checkpatch is not a silver bullet to get your code clean and nice.
>
True but it is the tool many maintainers use (sometimes) to ensure clean and nice.
I am still not see the extra spaces here but I do see the unneeded parens.
>>> This is return led_...();
>>
>> That is a preference. It does not have to be that way.
>
> What do you mean? We do not appreciate +LOCs for no (or even nagative!) benefit.
>
Again for non-IoT cases this would be fine but if we are striving to meet IoT size
requirements I do understand your concern.
>>>> + ret = of_property_read_string(led->led_node, "label", &name);
>>>
>>> device_property_...();
>>
>> It can be if the maintainer is requesting this.
>
> Jacek, if you need rationale behind this comment it's here: the driver
> has nothing DT specific and getting rid of OF centric programming
> allows to reuse the driver on non-DT platforms w/o touching a source
> code.
>
See below
>> Is the trend to move to these functions?
>
> See above.
See below
>
>> Most drivers use the "of" calls.
>
> So what?
>
I am deferring this to Jacek. Because this should be an overall coding change for all LED
drivers that are new. Like I pointed out I don't mind adding/changing the code as long as
all LED drivers will have that same mandate moving forward.
If the maintainers agree we can get this driver to be the example for the use of device_property calls.
>
>>>> + if (!ret)
>>>
>>> if (ret) sounds more natural. And better just to split
OK
>>>
>>>> + snprintf(led->led_name, sizeof(led->led_name),
>>>> + "%s:%s", led->led_node->name, name);
>>>> + else
>>>> + snprintf(led->led_name, sizeof(led->led_name),
>>>> + "%s:torch", led->led_node->name);
>>>
>>> const char *tmp;
>>>
>>> ret = device_property_read_...(&tmp);
>>> if (ret)
>>> tmp = ...
>>> sprintf(...);
>
> No comments on this?
No. This would depend on the disposition of the device_property calls.
If we go that way then yes this will probably change. But we need to have an
if..else as the label property is option in the DT documentation and we have to
make a label if one does not exist.
>
>>>> + led = devm_kzalloc(&client->dev,
>>>> + sizeof(struct lm3601x_led), GFP_KERNEL);
>>>
>>> sizeof(*led) and one line in the result
>
> And this?
Either one should yield the same allocation. So I can change it.
>
>
>>>> + { },
>>>
>>> Terminators better w/o comma.
>>
>> Looking at other drivers adding comma's on the sentinel is accepted. See the as3645a driver
>
> So what?
Not a compile issue here. I have been asked by maintainers/reviewers to add the comma on the sentinnel not just
struct definition but also for enums.
This is a maintainer decision I defer to them for guidance.
>
> Terminator at compile time even better.
>
>>>> + {},
>>>
>>> Ditto.
>>
>> See above
>
> See above.
>
See above
--
------------------
Dan Murphy
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox