Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a7e96623-0010-a7ae-fa3d-df1defcd51ca@linaro.org>
Date:   Thu, 13 Oct 2022 08:27:05 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.2
Subject: Re: [PATCH v1 1/5] soc: hpe: add support for HPE GXP Programmable
 Register Driver
To:     "Hawkins, Nick" <nick.hawkins@hpe.com>
Cc:     "Verdun, Jean-Marie" <verdun@hpe.com>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "krzysztof.kozlowski+dt@linaro.org" 
        <krzysztof.kozlowski+dt@linaro.org>,
        "linux@armlinux.org.uk" <linux@armlinux.org.uk>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>
References: <20221011185525.94210-1-nick.hawkins@hpe.com>
 <20221011185525.94210-2-nick.hawkins@hpe.com>
 <c6ab6116-5d92-132e-48fc-b731fd244d72@linaro.org>
 <DM4PR84MB19276978EB630CF08833E5A488229@DM4PR84MB1927.NAMPRD84.PROD.OUTLOOK.COM>
Content-Language: en-US
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <DM4PR84MB19276978EB630CF08833E5A488229@DM4PR84MB1927.NAMPRD84.PROD.OUTLOOK.COM>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 12/10/2022 16:25, Hawkins, Nick wrote:
> Greetings Krysztof,
> 
> Thanks for the feedback! I have several questions below:
> 
>>> +
>>> +static ssize_t server_id_show(struct device *dev, struct 
>>> +device_attribute *attr, char *buf) {
>>> +	struct gxp_plreg_drvdata *drvdata = dev_get_drvdata(dev);
>>> +	int value_upper;
>>> +	int value_lower;
>>> +	ssize_t ret;
>>> +	u32 trans_offset;
>>> +	u32 trans_shift;
>>> +
>>> +	/* read upper first */
>>> +	address_translation(drvdata->server_id.upper[BYTE],
>>> +			    &trans_offset,
>>> +			    &trans_shift);
>>> +	regmap_read(drvdata->plreg_map, trans_offset, &value_upper);
>>> +	value_upper = value_upper >> trans_shift;
>>> +	value_upper = value_upper & drvdata->server_id.upper[MASK];
>>> +
>>> +	/* read lower last */
>>> +	address_translation(drvdata->server_id.lower[BYTE],
>>> +			    &trans_offset,
>>> +			    &trans_shift);
>>> +	regmap_read(drvdata->plreg_map, trans_offset, &value_lower);
>>> +	value_lower = value_lower >> trans_shift;
>>> +	value_lower = value_lower & drvdata->server_id.lower[MASK];
>>> +
>>> +	ret = sprintf(buf, "0x%04x", value_upper | value_lower);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static DEVICE_ATTR_RO(server_id);
> 
>> Missing sysfs documentation.
> 
> Can you point me at the proper location / documentation for documenting sysfs? Thanks!

Documentation/ABI/README

> 
>>> +	for (i = 0; i <= MASK; i++) {
>>> +		if (of_property_read_u32_index(np, "grp5", i,
>>> +					       &drvdata->grp_intr_flags.grp5[i])) {
>>> +			dev_err(&pdev->dev,
>>> +				"grp5intsflags is missing its 'grp5' property index %d\n", i);
>>> +			return -ENODEV;
>>> +		}
>>> +	}
>>> +
>>> +	np = of_get_child_by_name(pdev->dev.of_node, "pwrbtn");
>>> +	if (!np) {
>>> +		dev_err(&pdev->dev, "%pOF is missing its 'pwrbtn' node\n", np);
>>> +		return -ENODEV;
>>> +	}
>>> +
>>> +	for (i = 0; i <= VALUE; i++) {
>>> +		if (of_property_read_u32_index(np, "latch", i, 
>>> +&drvdata->pwrbtn.latch[i])) {
> 
>> Undocumented properties. NAK.
> 
> If each child node of hpe,gxp-plreg were documented with their respective properties would this be acceptable?

I would need to see the bindings.


Best regards,
Krzysztof