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;
Message-ID: <b7e9dc19babef0e6993fa95be183c598a2f49bbd.camel@redhat.com>
Subject: Re: [RFC PATCH v2 3/4] drivers/auxdisplay: senshat Raspberry Pi
 Sense HAT display driver
From:   nsaenzju@redhat.com
To:     Charles Mirabile <cmirabil@redhat.com>,
        linux-kernel@vger.kernel.org
Cc:     Miguel Ojeda <ojeda@kernel.org>,
        Serge Schneider <serge@raspberrypi.org>,
        Stefan Wahren <stefan.wahren@i2se.com>,
        linux-rpi-kernel@lists.infradead.org, fedora-rpi@googlegroups.com,
        Mwesigwa Guma <mguma@redhat.com>,
        Joel Savitz <jsavitz@redhat.com>
Date:   Mon, 30 Aug 2021 15:28:51 +0200
In-Reply-To: <20210820180801.561119-4-cmirabil@redhat.com>
References: <20210820180801.561119-1-cmirabil@redhat.com>
         <20210820180801.561119-4-cmirabil@redhat.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.40.4 (3.40.4-1.fc34) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Charles,

On Fri, 2021-08-20 at 14:08 -0400, Charles Mirabile wrote:
> This patch implements control of the 8x8 RGB LED matrix display.

It'd be nice to get a more information on the i2c interface, and what each byte
is supposed to represent.

> Signed-off-by: Charles Mirabile <cmirabil@redhat.com>
> Signed-off-by: Mwesigwa Guma <mguma@redhat.com>
> Signed-off-by: Joel Savitz <jsavitz@redhat.com>
> ---

[...]

> +static long sensehat_display_ioctl(struct file *filp, unsigned int cmd,
> +			     unsigned long arg)
> +{
> +	struct sensehat *sensehat = container_of(filp->private_data, struct sensehat, display.mdev);
> +	struct sensehat_display *sensehat_display = &sensehat->display;
> +	void __user *user_ptr = (void __user *)arg;
> +	u8 temp[GAMMA_SIZE];
> +	int ret;
> +
> +	if (mutex_lock_interruptible(&sensehat_display->rw_mtx))
> +		return -ERESTARTSYS;
> +	switch (cmd) {
> +	case SENSEDISP_IOGET_GAMMA:
> +		if (copy_to_user(user_ptr, sensehat_display->gamma, GAMMA_SIZE)) {
> +			ret = -EFAULT;
> +			goto out_unlock;
> +		}
> +		ret = 0;
> +		goto out_unlock;
> +	case SENSEDISP_IOSET_GAMMA:
> +		if (copy_from_user(temp, user_ptr, GAMMA_SIZE)) {
> +			ret = -EFAULT;
> +			goto out_unlock;
> +		}
> +		ret = 0;
> +		goto out_update;
> +	case SENSEDISP_IORESET_GAMMA:
> +		if (arg < GAMMA_DEFAULT || arg >= GAMMA_PRESET_COUNT) {
> +			ret = -EINVAL;
> +			goto out_unlock;
> +		}
> +		memcpy(temp, gamma_presets[arg], GAMMA_SIZE);
> +		ret = 0;
> +		goto out_update;
> +	default:
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}
> +out_update:
> +	memcpy(sensehat_display->gamma, temp, GAMMA_SIZE);
> +	sensehat_update_display(sensehat);
> +out_unlock:
> +	mutex_unlock(&sensehat_display->rw_mtx);
> +	return ret;
> +}
> +
> +static const struct file_operations sensehat_display_fops = {
> +	.owner		= THIS_MODULE,
> +	.llseek		= sensehat_display_llseek,
> +	.read		= sensehat_display_read,
> +	.write		= sensehat_display_write,
> +	.unlocked_ioctl	= sensehat_display_ioctl,
> +};

I doubt this approach will make it upstream. This should use an already
existing kernel interface, or if not good enough, extend it. I'm sure this is
not the first RGB led matrix to show up anyway. Maybe drivers/leds has
infrastructure to deal with this. Or else a fb device?

I presume you want to keep the IOCTL in order to be able to run RPi specific
aplications/libraries. It'll be up to them to change once we settle on a proper
way of handling this, not the other way around. Note that the RPi engineers
have always preffered using official kernel interfaces when available, so I
don't think this'll be problematic.

Regards,
Nicolas