From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752627AbeBZLfU (ORCPT ); Mon, 26 Feb 2018 06:35:20 -0500 Received: from mail-qt0-f171.google.com ([209.85.216.171]:37049 "EHLO mail-qt0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751877AbeBZLfS (ORCPT ); Mon, 26 Feb 2018 06:35:18 -0500 X-Google-Smtp-Source: AG47ELsEwXrckX9qY33kaI34GFUjMfjHkyOS17VKCCkKmXvyBILH0DaDuDNpKTUGG750QY5L1EDa3oOW4TQfpBgo0mg= MIME-Version: 1.0 In-Reply-To: References: <20180222191647.4727-1-ard.biesheuvel@linaro.org> <20180222191647.4727-3-ard.biesheuvel@linaro.org> From: Andy Shevchenko Date: Mon, 26 Feb 2018 13:35:16 +0200 Message-ID: Subject: Re: [PATCH v2 2/2] i2c: add support for Socionext SynQuacer I2C controller To: Ard Biesheuvel Cc: Wolfram Sang , Rob Herring , Mark Rutland , linux-i2c , devicetree , Linux Kernel Mailing List , linux-arm Mailing List , Jassi Brar Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id w1QBZPEH026702 On Mon, Feb 26, 2018 at 11:59 AM, Ard Biesheuvel wrote: > On 23 February 2018 at 13:12, Andy Shevchenko wrote: >> On Fri, Feb 23, 2018 at 2:40 PM, Ard Biesheuvel >> wrote: >>> On 23 February 2018 at 12:27, Andy Shevchenko wrote: >>>> On Thu, Feb 22, 2018 at 9:16 PM, Ard Biesheuvel >>>> wrote: > ... >>>>> + ret = device_property_read_u32(&pdev->dev, "clock-frequency", >>>>> + &speed_khz); >>>>> + if (ret) { >>>>> + dev_err(&pdev->dev, >>>>> + "Missing clock-frequency property\n"); >>>>> + return -EINVAL; >>>>> + } >>>>> + speed_khz /= 1000; >> >>>>> + if (dev_of_node(&pdev->dev)) { >>>>> + i2c->clk = devm_clk_get(&pdev->dev, "pclk"); >>>>> + if (IS_ERR(i2c->clk)) { >>>>> + dev_err(&pdev->dev, "cannot get clock\n"); >>>>> + return PTR_ERR(i2c->clk); >>>>> + } >>>>> + dev_dbg(&pdev->dev, "clock source %p\n", i2c->clk); >>>>> + >>>>> + i2c->clkrate = clk_get_rate(i2c->clk); >>>>> + dev_dbg(&pdev->dev, "clock rate %d\n", i2c->clkrate); >>>>> + clk_prepare_enable(i2c->clk); >>>>> + } else { >>>>> + ret = device_property_read_u32(&pdev->dev, >>>>> + "socionext,pclk-rate", >>>>> + &i2c->clkrate); >>>>> + if (ret) >>>>> + return ret; >>>>> + } >>>> >>>> Okay, I got this case. It's more likely the one in 8250_dw.c. >>>> >>>> Can you do the similar way? >> >>> Could you elaborate? >> >> --- 8< --- 8< --- 8< --- >> device_property_read_u32(dev, "clock-frequency", &p->uartclk); >> >> /* If there is separate baudclk, get the rate from it. */ >> data->clk = devm_clk_get(dev, "baudclk"); >> ... >> if (IS_ERR(data->clk) && PTR_ERR(data->clk) == -EPROBE_DEFER) >> return -EPROBE_DEFER; >> if (!IS_ERR_OR_NULL(data->clk)) { >> err = clk_prepare_enable(data->clk); >> if (err) >> dev_warn(dev, "could not enable optional baudclk: %d\n", >> err); >> else >> p->uartclk = clk_get_rate(data->clk); >> } >> >> /* If no clock rate is defined, fail. */ >> if (!p->uartclk) { >> dev_err(dev, "clock rate not defined\n"); >> err = -EINVAL; >> goto err_clk; >> --- 8< --- 8< --- 8< --- >> >> Replace 'baudclk' with 'pclk' and p->uartclk with i2c->clkrate in >> above and you are almost done. >> > > I don't think this is better. It's a pattern over ACPI vs. clk cases at least for now. But hold on. We have already an example of dealing with ACPI / non-ACPI cases for I2C controllers — i2c-designware-platdrv.c. Check how it's done there. I actually totally forgot about ACPI slaves described in the table. We need to take into account the ones with lowest bus speed. > The generic DT I2C 'clock-frequency' > property denotes the bus clock rate not the rate of the clock that > feeds the IP block. This is rather different from the UART bindings. > Also, I don't want to support 'socionext,pclk-rate' for DT platforms, > only for ACPI platforms. Unfortunately, we are strong against deviations between DT and ACPI in case of device properties. You may provide a clock and use devm_clk_get() without any concerns from where this comes from. You won't find any deviation in DW I2C driver since there is none, while driver works for non-ACPI platforms as well. -- With Best Regards, Andy Shevchenko