Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 26 May 2020 12:23:01 +0200
From:   Alain Volmat <alain.volmat@st.com>
To:     Wolfram Sang <wsa@kernel.org>
CC:     Benjamin Tissoires <benjamin.tissoires@redhat.com>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "mark.rutland@arm.com" <mark.rutland@arm.com>,
        Pierre Yves MORDRET <pierre-yves.mordret@st.com>,
        "mcoquelin.stm32@gmail.com" <mcoquelin.stm32@gmail.com>,
        Alexandre TORGUE <alexandre.torgue@st.com>,
        "linux-i2c@vger.kernel.org" <linux-i2c@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-stm32@st-md-mailman.stormreply.com" 
        <linux-stm32@st-md-mailman.stormreply.com>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Fabrice GASNIER <fabrice.gasnier@st.com>
Subject: Re: [PATCH 1/4] i2c: smbus: add core function handling SMBus
 host-notify
Message-ID: <20200526102301.GA14423@gnbcxd0016.gnb.st.com>
Mail-Followup-To: Wolfram Sang <wsa@kernel.org>,
        Benjamin Tissoires <benjamin.tissoires@redhat.com>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "mark.rutland@arm.com" <mark.rutland@arm.com>,
        Pierre Yves MORDRET <pierre-yves.mordret@st.com>,
        "mcoquelin.stm32@gmail.com" <mcoquelin.stm32@gmail.com>,
        Alexandre TORGUE <alexandre.torgue@st.com>,
        "linux-i2c@vger.kernel.org" <linux-i2c@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-stm32@st-md-mailman.stormreply.com" <linux-stm32@st-md-mailman.stormreply.com>,
        "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Fabrice GASNIER <fabrice.gasnier@st.com>
References: <1588657871-14747-1-git-send-email-alain.volmat@st.com>
 <1588657871-14747-2-git-send-email-alain.volmat@st.com>
 <20200523104624.GB3459@ninjato>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20200523104624.GB3459@ninjato>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sat, May 23, 2020 at 10:46:25AM +0000, Wolfram Sang wrote:
> 
> Adding Benjamin who mainly implemented this.
> 
> On Tue, May 05, 2020 at 07:51:08AM +0200, Alain Volmat wrote:
> > SMBus Host-Notify protocol, from the adapter point of view
> > consist of receiving a message from a client, including the
> > client address and some other data.
> > 
> > It can be simply handled by creating a new slave device
> > and registering a callback performing the parsing of the
> > message received from the client.
> > 
> > This commit introduces two new core functions
> >   * i2c_new_smbus_host_notify_device
> >   * i2c_free_smbus_host_notify_device
> > that take care of registration of the new slave device and
> > callback and will call i2c_handle_smbus_host_notify once a
> > Host-Notify event is received.
> 
> Yay, cool idea to use the slave interface. I like it a lot!
> 
> > +static int i2c_smbus_host_notify_cb(struct i2c_client *client,
> > +				    enum i2c_slave_event event, u8 *val)
> > +{
> > +	struct i2c_smbus_host_notify_status *status = client->dev.platform_data;
> > +	int ret;
> > +
> > +	switch (event) {
> > +	case I2C_SLAVE_WRITE_REQUESTED:
> > +		status->notify_start = true;
> > +		break;
> > +	case I2C_SLAVE_WRITE_RECEIVED:
> > +		/* We only retrieve the first byte received (addr)
> > +		 * since there is currently no way to retrieve the data
> > +		 * parameter from the client.
> 
> Maybe s/no way/no support/ ? I still wonder if we couldn't add it
> somehow. Once we find a device which needs this, of course.

Indeed. Such support can be added later on once such device is found. For the
time being I will state "no support"

> 
> > +		 */
> > +		if (!status->notify_start)
> > +			break;
> > +		status->addr = *val;
> > +		status->notify_start = false;
> > +		break;
> > +	case I2C_SLAVE_STOP:
> 
> What about setting 'notify_start' to false here as well? In the case of
> an incomplete write?

Ok. I will check that notify_start is false before calling host_notify
(since otherwise it will call i2c_handle_smbus_host_notify with a bad addr
value) and reset notify_start to false if it is still true.

> 
> > +		ret = i2c_handle_smbus_host_notify(client->adapter,
> > +						   status->addr);
> > +		if (ret < 0) {
> > +			dev_warn(&client->adapter->dev, "failed to handle host_notify (%d)\n",
> > +				ret);
> 
> I think we should rather add such error strings to the core if we think
> they are needed. I am not convinced they are, though.

Agreed, this error can be removed.

> 
> > +			return ret;
> > +		}
> > +		break;
> > +	default:
> > +		/* Only handle necessary events */
> > +		break;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> 
> Rest of the code looks good. Maybe we should compile all this only when
> I2C_SLAVE is enabled?
> 

Yes, I will enclose that around I2C_SLAVE support check.