From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753109AbdLDCuH (ORCPT ); Sun, 3 Dec 2017 21:50:07 -0500 Received: from mga01.intel.com ([192.55.52.88]:48648 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752862AbdLDCuE (ORCPT ); Sun, 3 Dec 2017 21:50:04 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,357,1508828400"; d="scan'208";a="155722146" Subject: Re: [alsa-devel] [PATCH v4 04/15] soundwire: Add MIPI DisCo property helpers To: Vinod Koul Cc: Greg Kroah-Hartman , LKML , ALSA , Mark , Takashi , patches.audio@intel.com, alan@linux.intel.com, Charles Keepax , Sagar Dharia , srinivas.kandagatla@linaro.org, plai@codeaurora.org, Sudheer Papothi References: <1512122177-2889-1-git-send-email-vinod.koul@intel.com> <1512122177-2889-5-git-send-email-vinod.koul@intel.com> <4a79c26c-db39-700c-627b-f474200f9a7f@linux.intel.com> <20171203165203.GM32417@localhost> From: Pierre-Louis Bossart Message-ID: <557584aa-a82b-f495-4e37-cec250178d50@linux.intel.com> Date: Sun, 3 Dec 2017 20:50:07 -0600 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171203165203.GM32417@localhost> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 12/3/17 10:52 AM, Vinod Koul wrote: > On Fri, Dec 01, 2017 at 04:49:01PM -0600, Pierre-Louis Bossart wrote: > >>> +int sdw_master_read_prop(struct sdw_bus *bus) >>> +{ >>> + struct sdw_master_prop *prop = &bus->prop; >>> + struct fwnode_handle *link; >>> + unsigned int count = 0; >>> + char name[32]; >>> + int nval, i; >>> + >>> + device_property_read_u32(bus->dev, >>> + "mipi-sdw-sw-interface-revision", &prop->revision); >>> + device_property_read_u32(bus->dev, "mipi-sdw-master-count", &count); >>> + >>> + /* Find link handle */ >>> + snprintf(name, sizeof(name), >>> + "mipi-sdw-link-%d-subproperties", bus->link_id); >> >> if you follow the DisCo spec, this property is at the controller level, >> isn't there a confusion between controller/master here, and consequently are >> we reading the same things multiple times or using the wrong bus parameter? > > Not sure I follow, this one is for a specific master ie a specfic link. > we need to read respective master thru mipi-sdw-link-N-subproperties > >> If I look at intel_probe(), there is a clear reference to a link_id, and >> then you set the pointer to this read_prop which reads the number of links, >> which looks like the wrong order. You can't assign a link ID before knowing >> how many links there are - or you may be unable to detect issues. > > Sorry I dont follow this part. FWIW, when master driver is enumerated it > know the link_id value and then sets the read_prop and then these are read. > > Here we are reading "a specific link property" with the knowledge of link_id > value... the sdw_master-count is at the controller level, and the linkid has to be < master_count. The fact that you are reading this property for each master instance is the problem. > >>> + fwnode_property_read_u32(link, "mipi-sdw-default-frame-rate", >>> + &prop->default_frame_rate); >>> + fwnode_property_read_u32(link, "mipi-sdw-default-frame-row-size", >>> + &prop->default_row); >>> + fwnode_property_read_u32(link, "mipi-sdw-default-frame-col-size", >> >> This is fine, just wondering if we should warnings if the values make no >> sense, e.g. the DisCo spec states in Note1 page 15 that the values are >> interrelated. > > I think we discussed in past and that would kind of form the firmware > validation. We check all the values to see if firmware gave us sane values.. > >>> + /* >>> + * Based on each DPn port, get source and sink dpn properties. >>> + * Also, some ports can operate as both source or sink. >>> + */ >>> + >>> + /* Allocate memory for set bits in port lists */ >>> + nval = hweight32(prop->source_ports); >>> + num_of_ports += nval; >> >> this and... >> >>> + prop->src_dpn_prop = devm_kcalloc(&slave->dev, nval, >>> + sizeof(*prop->src_dpn_prop), GFP_KERNEL); >>> + if (!prop->src_dpn_prop) >>> + return -ENOMEM; >>> + >>> + /* Read dpn properties for source port(s) */ >>> + sdw_slave_read_dpn(slave, prop->src_dpn_prop, nval, >>> + prop->source_ports, "source"); >>> + >>> + nval = hweight32(prop->sink_ports); >>> + num_of_ports += nval; >> >> ... this is no longer needed since... >> >>> + prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval, >>> + sizeof(*prop->sink_dpn_prop), GFP_KERNEL); >>> + if (!prop->sink_dpn_prop) >>> + return -ENOMEM; >>> + >>> + /* Read dpn properties for sink port(s) */ >>> + sdw_slave_read_dpn(slave, prop->sink_dpn_prop, nval, >>> + prop->sink_ports, "sink"); >>> + >>> + /* some ports are bidirectional so check total ports by ORing */ >>> + nval = prop->source_ports | prop->sink_ports; >>> + num_of_ports = hweight32(nval) + 1; /* add 1 for DP0 */ >> >> ... you reassign the value here. That was one earlier feedback from me but >> you left the variable incrementation in the code. > > This seems to have artifact of merge conflicts as I clearly remember removing > this, thanks for pointing will remove these.. > >>> +/** >>> + * enum sdw_clk_stop_mode - Clock Stop modes >>> + * @SDW_CLK_STOP_MODE0: Slave can continue operation seamlessly on clock >>> + * restart >>> + * @SDW_CLK_STOP_MODE1: Slave may have entered a deeper power-saving mode, >>> + * not capable of continuing operation seamlessly when the clock restarts >>> + */ >>> +enum sdw_clk_stop_mode { >>> + SDW_CLK_STOP_MODE0 = 1, >>> + SDW_CLK_STOP_MODE1 = 2, >> >> why not 0 and 1? > > why not 1 and 2 :D > > I think it was to ensure we have a non zero value, but am not sure, will > check though.. I don't think the value matter and you should use the same conventions for such enums. >