Messages in this thread | | | Date | Thu, 16 Sep 2021 13:00:14 +0200 | Subject | Re: [RFC PATCH v2 3/4] drivers/auxdisplay: senshat Raspberry Pi Sense HAT display driver | From | Matthias Brugger <> |
| |
On 30/08/2021 15:28, nsaenzju@redhat.com wrote: > 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'm working on upstreaming that again and my approach is to use framebuffer for this. What is the reason you decided against framebuffer?
Regards, Matthias
> > 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 >
| |